lkml.org 
[lkml]   [2024]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/5] mailbox: Add support for QTI CPUCP mailbox controller
From


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.

>
>> + }
>> + }
>> +
>> + 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.

>
>> + }
>> +
>> + 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
>>
>>
>
>

\
 
 \ /
  Last update: 2024-04-17 13:53    [W:0.105 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site