On Wed, 17 Apr 2024 at 14:51, Sibi Sankar <quic_sibis@xxxxxxxxxxx> wrote:
>
>
>
> On 4/16/24 21:51, Dmitry Baryshkov wrote:
> > On Thu, 28 Mar 2024 at 11:52, Sibi Sankar <quic_sibis@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >> ---
> >>
> >> 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.
>
> >
> >> + }
> >> + }
> >> +
> >> + 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.
>
> >
> >> + }
> >> +
> >> + 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
> >>
> >>
> >
> >
--
With best wishes
Dmitry
