Received-SPF: pass (google.com: domain of linux-kernel+bounces-148435-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <f32b7e43-5e39-0c82-1e03-18a2219adfdb@quicinc.com>
Date: Wed, 17 Apr 2024 17:21:21 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH 2/5] mailbox: Add support for QTI CPUCP mailbox controller
Content-Language: en-US
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
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>
References: <20240328095044.2926125-1-quic_sibis@quicinc.com>
 <20240328095044.2926125-3-quic_sibis@quicinc.com>
 <CAA8EJpoQyzF1E2xFPHvzz5Nk=w3J2abd3Y13nc+4FK-jRQbnFw@mail.gmail.com>
From: Sibi Sankar <quic_sibis@quicinc.com>
In-Reply-To: <CAA8EJpoQyzF1E2xFPHvzz5Nk=w3J2abd3Y13nc+4FK-jRQbnFw@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


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