Received-SPF: pass (google.com: domain of linux-kernel+bounces-148575-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <c736bd7c-0ad3-f35f-d6f0-332bbcd15899@quicinc.com>
Date: Wed, 17 Apr 2024 18:58:19 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH 2/5] mailbox: Add support for QTI CPUCP mailbox controller
Content-Language: en-US
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC: <sudeep.holla@arm.com>, <cristian.marussi@arm.com>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <jassisinghbrar@gmail.com>,
        <robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <quic_rgottimu@quicinc.com>,
        <quic_kshivnan@quicinc.com>, <conor+dt@kernel.org>,
        <quic_gkohli@quicinc.com>, <quic_nkela@quicinc.com>,
        <quic_psodagud@quicinc.com>
References: <20240328095044.2926125-1-quic_sibis@quicinc.com>
 <20240328095044.2926125-3-quic_sibis@quicinc.com>
 <CAA8EJpoQyzF1E2xFPHvzz5Nk=w3J2abd3Y13nc+4FK-jRQbnFw@mail.gmail.com>
 <f32b7e43-5e39-0c82-1e03-18a2219adfdb@quicinc.com>
 <CAA8EJppQ9Saoytar7-xXORR=BppddWQ5fnrqg+x1rzRFctOt8Q@mail.gmail.com>
From: Sibi Sankar <quic_sibis@quicinc.com>
In-Reply-To: <CAA8EJppQ9Saoytar7-xXORR=BppddWQ5fnrqg+x1rzRFctOt8Q@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 4/17/24 17:24, Dmitry Baryshkov wrote:
> On Wed, 17 Apr 2024 at 14:51, Sibi Sankar <quic_sibis@quicinc.com> wrote:
>>
>>
>>
>> On 4/16/24 21:51, Dmitry Baryshkov wrote:
>>> On Thu, 28 Mar 2024 at 11:52, Sibi Sankar <quic_sibis@quicinc.com> wrote:
>>>>
>>>> Add support for CPUSS Control Processor (CPUCP) mailbox controller,
>>>> this driver enables communication between AP and CPUCP by acting as
>>>> a doorbell between them.
>>>>
>>>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>>>> ---
>>>>
>>>> rfc:
>>>> * Use chan->lock and chan->cl to detect if the channel is no longer
>>>>     Available. [Dmitry]
>>>> * Use BIT() instead of using manual shifts. [Dmitry]
>>>> * Don't use integer as a pointer value. [Dmitry]
>>>> * Allow it to default to of_mbox_index_xlate. [Dmitry]
>>>> * Use devm_of_iomap. [Dmitry]
>>>> * Use module_platform_driver instead of module init/exit. [Dmitry]
>>>> * Get channel number using mailbox core (like other drivers) and
>>>>     further simplify the driver by dropping setup_mbox func.
>>
>> Hey Dmitry,
>>
>> Thanks for taking time to review the series.
>>
>>>>
>>>>    drivers/mailbox/Kconfig           |   8 ++
>>>>    drivers/mailbox/Makefile          |   2 +
>>>>    drivers/mailbox/qcom-cpucp-mbox.c | 205 ++++++++++++++++++++++++++++++
>>>>    3 files changed, 215 insertions(+)
>>>>    create mode 100644 drivers/mailbox/qcom-cpucp-mbox.c
>>>>
>> [snip]
>> ...
>>>> +
>>>> +       status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
>>>> +
>>>> +       for (i = 0; i < APSS_CPUCP_IPC_CHAN_SUPPORTED; i++) {
>>>> +               val = 0;
>>>> +               if (status & ((u64)1 << i)) {
>>>
>>> BIT() or test_bit()
>>
>> I'll use BIT()
>>
>>>
>>>> +                       val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD + (i * 8) + APSS_CPUCP_MBOX_CMD_OFF);
>>>
>>> #define APSS_CPUCP_MBOX_CMD_OFF(i)
>>
>> ack
>>
>>>
>>>> +                       chan = &cpucp->chans[i];
>>>> +                       spin_lock_irqsave(&chan->lock, flags);
>>>> +                       if (chan->cl)
>>>> +                               mbox_chan_received_data(chan, &val);
>>>> +                       spin_unlock_irqrestore(&chan->lock, flags);
>>>> +                       writeq(status, cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
>>>
>>> Why is status written from inside the loop? If the bits are cleared by
>>> writing 1, then you should be writing BIT(i) to that register. Also
>>> make sure that it is written at the correct time, so that if there is
>>> an event before notifying the driver, it doesn't get lost.
>>
>> Thanks for catching this. I probably didn't run into this scenario
>> because of using just one channel at point any time. I'll move it
>> outside the loop.
> 
> It might be better to write single bits from within the loop under the spinlock.

Sure, will do that instead.

> 
>>
>>>
>>>> +               }
>>>> +       }
>>>> +
>>>> +       return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
>>>> +{
>>>> +       struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
>>>> +       unsigned long chan_id = channel_number(chan);
>>>> +       u64 val;
>>>> +
>>>> +       val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
>>>> +       val |= BIT(chan_id);
>>>> +       writeq(val, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static void qcom_cpucp_mbox_shutdown(struct mbox_chan *chan)
>>>> +{
>>>> +       struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
>>>> +       unsigned long chan_id = channel_number(chan);
>>>> +       u64 val;
>>>> +
>>>> +       val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
>>>> +       val &= ~BIT(chan_id);
>>>> +       writeq(val, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
>>>> +}
>>>> +
>>>> +static int qcom_cpucp_mbox_send_data(struct mbox_chan *chan, void *data)
>>>> +{
>>>> +       struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
>>>> +       unsigned long chan_id = channel_number(chan);
>>>> +       u32 *val = data;
>>>> +
>>>> +       writel(*val, cpucp->tx_base + APSS_CPUCP_TX_MBOX_CMD + (chan_id * 8) + APSS_CPUCP_MBOX_CMD_OFF);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static const struct mbox_chan_ops qcom_cpucp_mbox_chan_ops = {
>>>> +       .startup = qcom_cpucp_mbox_startup,
>>>> +       .send_data = qcom_cpucp_mbox_send_data,
>>>> +       .shutdown = qcom_cpucp_mbox_shutdown
>>>> +};
>>>> +
>>>> +static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
>>>> +{
>>>> +       struct qcom_cpucp_mbox *cpucp;
>>>> +       struct mbox_controller *mbox;
>>>> +       int ret;
>>>> +
>>>> +       cpucp = devm_kzalloc(&pdev->dev, sizeof(*cpucp), GFP_KERNEL);
>>>> +       if (!cpucp)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       cpucp->dev = &pdev->dev;
>>>> +
>>>> +       cpucp->rx_base = devm_of_iomap(cpucp->dev, cpucp->dev->of_node, 0, NULL);
>>>> +       if (IS_ERR(cpucp->rx_base))
>>>> +               return PTR_ERR(cpucp->rx_base);
>>>> +
>>>> +       cpucp->tx_base = devm_of_iomap(cpucp->dev, cpucp->dev->of_node, 1, NULL);
>>>> +       if (IS_ERR(cpucp->tx_base))
>>>> +               return PTR_ERR(cpucp->tx_base);
>>>> +
>>>> +       writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
>>>> +       writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
>>>> +       writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
>>>> +
>>>> +       cpucp->irq = platform_get_irq(pdev, 0);
>>>> +       if (cpucp->irq < 0) {
>>>> +               dev_err(&pdev->dev, "Failed to get the IRQ\n");
>>>> +               return cpucp->irq;
>>>
>>> It already prints the error message.
>>
>> ack
>>
>>>
>>>> +       }
>>>> +
>>>> +       ret = devm_request_irq(&pdev->dev, cpucp->irq, qcom_cpucp_mbox_irq_fn,
>>>> +                              IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
>>>> +       if (ret < 0) {
>>>> +               dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret);
>>>> +               return ret;
>>>
>>> return dev_err_probe();
>>
>> ack
>>
>>>
>>>> +       }
>>>> +
>>>> +       writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
>>>> +
>>>> +       mbox = &cpucp->mbox;
>>>> +       mbox->dev = cpucp->dev;
>>>> +       mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
>>>> +       mbox->chans = cpucp->chans;
>>>> +       mbox->ops = &qcom_cpucp_mbox_chan_ops;
>>>> +       mbox->txdone_irq = false;
>>>> +       mbox->txdone_poll = false;
>>>> +
>>>> +       ret = mbox_controller_register(mbox);
>>>
>>> Use devm_mbox_controller_register()
>>
>> ack
>>
>>>   >> +       if (ret) {
>>>> +               dev_err(&pdev->dev, "Failed to create mailbox\n");
>>>> +               return ret;
>>>
>>> return dev_err_probe();
>>
>> I guess ^^ is a typo? Since devm_mbox_controller_register wouldn't
>> return -EPROBE_DEFER.
> 
> Anyway, using dev_err_probe is a simpler and better style. It's not a
> question of returning -EPROBE_DEFER.

ack

-Sibi

> 
>>
>>>
>>>> +       }
>>>> +
>>>> +       platform_set_drvdata(pdev, cpucp);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int qcom_cpucp_mbox_remove(struct platform_device *pdev)
>>>> +{
>>>> +       struct qcom_cpucp_mbox *cpucp = platform_get_drvdata(pdev);
>>>> +
>>>> +       mbox_controller_unregister(&cpucp->mbox);
>>>   > This will be replaced by devm_mbox_controller_register().
>>
>> ack
>>
>>>
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
>>>> +       { .compatible = "qcom,x1e80100-cpucp-mbox"},
>>>> +       {}
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
>>>> +
>>>> +static struct platform_driver qcom_cpucp_mbox_driver = {
>>>> +       .probe = qcom_cpucp_mbox_probe,
>>>> +       .remove = qcom_cpucp_mbox_remove,
>>>> +       .driver = {
>>>> +               .name = "qcom_cpucp_mbox",
>>>> +               .of_match_table = qcom_cpucp_mbox_of_match,
>>>> +               .suppress_bind_attrs = true,
>>>
>>> No need to. Please drop.
>>
>> ack
>>
>> -Sibi
>>
>>>
>>>> +       },
>>>> +};
>>>> +module_platform_driver(qcom_cpucp_mbox_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("QTI CPUCP MBOX Driver");
>>>> +MODULE_LICENSE("GPL");
>>>> --
>>>> 2.34.1
>>>>
>>>>
>>>
>>>
> 
> 
>