Re: [PATCH 2/5] mailbox: Add support for QTI CPUCP mailbox controller
From: Sibi Sankar
Date: Wed Apr 17 2024 - 09:30:16 EST
On 4/17/24 17:24, Dmitry Baryshkov wrote:
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.
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