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