by Sibi Sankar

[permalink] [raw]

Subject: Re: [PATCH 2/5] mailbox: Add support for QTI CPUCP mailbox controller

On 4/17/24 17:24, Dmitry Baryshkov wrote:
> On Wed, 17 Apr 2024 at 14:51, Sibi Sankar <[email protected]> wrote:
>>
>>
>>
>> On 4/16/24 21:51, Dmitry Baryshkov wrote:
>>> On Thu, 28 Mar 2024 at 11:52, Sibi Sankar <[email protected]> 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 <[email protected]>
>>>> ---
>>>>
>>>> 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
>>>>
>>>>
>>>
>>>
>
>
>