Received-SPF: pass (google.com: domain of linux-kernel+bounces-148438-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240328095044.2926125-1-quic_sibis@quicinc.com>
 <20240328095044.2926125-3-quic_sibis@quicinc.com> <CAA8EJpoQyzF1E2xFPHvzz5Nk=w3J2abd3Y13nc+4FK-jRQbnFw@mail.gmail.com>
 <f32b7e43-5e39-0c82-1e03-18a2219adfdb@quicinc.com>
In-Reply-To: <f32b7e43-5e39-0c82-1e03-18a2219adfdb@quicinc.com>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date: Wed, 17 Apr 2024 14:54:30 +0300
Message-ID: <CAA8EJppQ9Saoytar7-xXORR=BppddWQ5fnrqg+x1rzRFctOt8Q@mail.gmail.com>
Subject: Re: [PATCH 2/5] mailbox: Add support for QTI CPUCP mailbox controller
To: Sibi Sankar <quic_sibis@quicinc.com>
Cc: sudeep.holla@arm.com, cristian.marussi@arm.com, andersson@kernel.org, 
	konrad.dybcio@linaro.org, jassisinghbrar@gmail.com, robh+dt@kernel.org, 
	krzysztof.kozlowski+dt@linaro.org, linux-kernel@vger.kernel.org, 
	linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, 
	quic_rgottimu@quicinc.com, quic_kshivnan@quicinc.com, conor+dt@kernel.org, 
	quic_gkohli@quicinc.com, quic_nkela@quicinc.com, quic_psodagud@quicinc.com
Content-Type: text/plain; charset="UTF-8"

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.

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