Messages in this thread | | | Date | Wed, 17 Apr 2024 18:58:19 +0530 | Subject | Re: [PATCH 2/5] mailbox: Add support for QTI CPUCP mailbox controller | From | Sibi Sankar <> |
| |
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 >>>> >>>> >>> >>> > > >
| |