diff mbox series

[V3,2/5] mailbox: Add support for QTI CPUCP mailbox controller

Message ID 20240417132856.1106250-3-quic_sibis@quicinc.com (mailing list archive)
State Superseded
Headers show
Series qcom: x1e80100: Enable CPUFreq | expand

Commit Message

Sibi Sankar April 17, 2024, 1:28 p.m. UTC 
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>
---

v2:
* Use BIT() instead of manual shift. [Dmitry]
* Define RX_MBOX_CMD to account for chan calculation. [Dmitry]
* Clear the bit instead of the entire status within the spinlock. [Dmitry]
* Use dev_err_probe instead. [Dmitry]
* Drop superfluous error message while handling errors from get_irq. [Dmitry]
* Use devm_mbox_controller_register and drop remove path. [Dmitry]
* Define TX_MBOX_CMD to account for chan calculation.
* Use cpucp->dev in probe path for conformity.

 drivers/mailbox/Kconfig           |   8 ++
 drivers/mailbox/Makefile          |   2 +
 drivers/mailbox/qcom-cpucp-mbox.c | 188 ++++++++++++++++++++++++++++++
 3 files changed, 198 insertions(+)
 create mode 100644 drivers/mailbox/qcom-cpucp-mbox.c

Comments

Dmitry Baryshkov April 17, 2024, 6:27 p.m. UTC | #1 
On Wed, 17 Apr 2024 at 16:29, 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>
> ---
>
> v2:
> * Use BIT() instead of manual shift. [Dmitry]
> * Define RX_MBOX_CMD to account for chan calculation. [Dmitry]
> * Clear the bit instead of the entire status within the spinlock. [Dmitry]
> * Use dev_err_probe instead. [Dmitry]
> * Drop superfluous error message while handling errors from get_irq. [Dmitry]
> * Use devm_mbox_controller_register and drop remove path. [Dmitry]
> * Define TX_MBOX_CMD to account for chan calculation.
> * Use cpucp->dev in probe path for conformity.
>
>  drivers/mailbox/Kconfig           |   8 ++
>  drivers/mailbox/Makefile          |   2 +
>  drivers/mailbox/qcom-cpucp-mbox.c | 188 ++++++++++++++++++++++++++++++
>  3 files changed, 198 insertions(+)
>  create mode 100644 drivers/mailbox/qcom-cpucp-mbox.c
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 42940108a187..23741a6f054e 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -273,6 +273,14 @@ config SPRD_MBOX
>           to send message between application processors and MCU. Say Y here if
>           you want to build the Spreatrum mailbox controller driver.
>
> +config QCOM_CPUCP_MBOX
> +       tristate "Qualcomm Technologies, Inc. CPUCP mailbox driver"
> +       depends on ARCH_QCOM || COMPILE_TEST
> +       help
> +         Qualcomm Technologies, Inc. CPUSS Control Processor (CPUCP) mailbox
> +         controller driver enables communication between AP and CPUCP. Say
> +         Y here if you want to build this driver.
> +
>  config QCOM_IPCC
>         tristate "Qualcomm Technologies, Inc. IPCC driver"
>         depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index 18793e6caa2f..53b512800bde 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -59,4 +59,6 @@ obj-$(CONFIG_SUN6I_MSGBOX)    += sun6i-msgbox.o
>
>  obj-$(CONFIG_SPRD_MBOX)                += sprd-mailbox.o
>
> +obj-$(CONFIG_QCOM_CPUCP_MBOX)  += qcom-cpucp-mbox.o
> +
>  obj-$(CONFIG_QCOM_IPCC)                += qcom-ipcc.o
> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
> new file mode 100644
> index 000000000000..059eb25f217c
> --- /dev/null
> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/platform_device.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +
> +#define APSS_CPUCP_IPC_CHAN_SUPPORTED          3
> +#define APSS_CPUCP_MBOX_CMD_OFF                        0x4
> +
> +/* Tx Registers */
> +#define APSS_CPUCP_TX_MBOX_IDR                 0

I don't see _IDR defines being used.

Other than that:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


> +#define APSS_CPUCP_TX_MBOX_CMD(i)              (0x100 + ((i) * 8))
> +
> +/* Rx Registers */
> +#define APSS_CPUCP_RX_MBOX_IDR                 0
> +#define APSS_CPUCP_RX_MBOX_CMD(i)              (0x100 + ((i) * 8))
> +#define APSS_CPUCP_RX_MBOX_MAP                 0x4000
> +#define APSS_CPUCP_RX_MBOX_STAT                        0x4400
> +#define APSS_CPUCP_RX_MBOX_CLEAR               0x4800
> +#define APSS_CPUCP_RX_MBOX_EN                  0x4C00
> +#define APSS_CPUCP_RX_MBOX_CMD_MASK            0xFFFFFFFFFFFFFFFF
Bjorn Andersson April 17, 2024, 9:26 p.m. UTC | #2 
On Wed, Apr 17, 2024 at 06:58:53PM +0530, Sibi Sankar wrote:
> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
> new file mode 100644
> index 000000000000..059eb25f217c
> --- /dev/null
> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.

Nope.

> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/platform_device.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +
> +#define APSS_CPUCP_IPC_CHAN_SUPPORTED		3
> +#define APSS_CPUCP_MBOX_CMD_OFF			0x4
> +
> +/* Tx Registers */
> +#define APSS_CPUCP_TX_MBOX_IDR			0
> +#define APSS_CPUCP_TX_MBOX_CMD(i)		(0x100 + ((i) * 8))
> +
> +/* Rx Registers */
> +#define APSS_CPUCP_RX_MBOX_IDR			0
> +#define APSS_CPUCP_RX_MBOX_CMD(i)		(0x100 + ((i) * 8))
> +#define APSS_CPUCP_RX_MBOX_MAP			0x4000
> +#define APSS_CPUCP_RX_MBOX_STAT			0x4400
> +#define APSS_CPUCP_RX_MBOX_CLEAR		0x4800
> +#define APSS_CPUCP_RX_MBOX_EN			0x4C00

Can we have lower case hex digits, plz?

> +#define APSS_CPUCP_RX_MBOX_CMD_MASK		0xFFFFFFFFFFFFFFFF
> +
> +/**
> + * struct qcom_cpucp_mbox - Holder for the mailbox driver
> + * @chans:			The mailbox channel
> + * @mbox:			The mailbox controller
> + * @tx_base:			Base address of the CPUCP tx registers
> + * @rx_base:			Base address of the CPUCP rx registers
> + * @dev:			Device associated with this instance
> + * @irq:			CPUCP to AP irq

@dev and @irq can be a local variables in qcom_cpucp_mbox_probe().

> + */
> +struct qcom_cpucp_mbox {
> +	struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
> +	struct mbox_controller mbox;
> +	void __iomem *tx_base;
> +	void __iomem *rx_base;
> +	struct device *dev;
> +	int irq;
> +};
> +
> +static inline int channel_number(struct mbox_chan *chan)
> +{
> +	return chan - chan->mbox->chans;
> +}
> +
> +static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
> +{
> +	struct qcom_cpucp_mbox *cpucp = data;
> +	struct mbox_chan *chan;
> +	unsigned long flags;
> +	u64 status;
> +	u32 val;
> +	int i;
> +
> +	status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
> +
> +	for (i = 0; i < APSS_CPUCP_IPC_CHAN_SUPPORTED; i++) {
> +		val = 0;

This value is immediately overwritten (or unused).

> +		if (status & BIT(i)) {

Can't you combine the for loop and this conditional into a
for_each_bit_set()?

> +			val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
> +			chan = &cpucp->chans[i];
> +			spin_lock_irqsave(&chan->lock, flags);

Can you please add a comment here to document that the lock is taken
here to deal with races against client registration? (It wasn't obvious
to me...)

> +			if (chan->cl)
> +				mbox_chan_received_data(chan, &val);
> +			writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
> +			spin_unlock_irqrestore(&chan->lock, flags);
> +		}
> +	}
> +
> +	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) + 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)
> +		return cpucp->irq;
> +
> +	ret = devm_request_irq(cpucp->dev, cpucp->irq, qcom_cpucp_mbox_irq_fn,
> +			       IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
> +	if (ret < 0)
> +		return dev_err_probe(cpucp->dev, ret, "Failed to register irq: %d\n", cpucp->irq);
> +
> +	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 = devm_mbox_controller_register(cpucp->dev, mbox);
> +	if (ret)
> +		return dev_err_probe(cpucp->dev, ret, "Failed to create mailbox\n");
> +
> +	platform_set_drvdata(pdev, cpucp);

I don't see you using the drvdata anywhere, can we drop this?

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
> +	{ .compatible = "qcom,x1e80100-cpucp-mbox"},

A space after the final '"' would be good for aesthetics.

Regards,
Bjorn

> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
> +
> +static struct platform_driver qcom_cpucp_mbox_driver = {
> +	.probe = qcom_cpucp_mbox_probe,
> +	.driver = {
> +		.name = "qcom_cpucp_mbox",
> +		.of_match_table = qcom_cpucp_mbox_of_match,
> +	},
> +};
> +module_platform_driver(qcom_cpucp_mbox_driver);
> +
> +MODULE_DESCRIPTION("QTI CPUCP MBOX Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.34.1
>
Sibi Sankar April 18, 2024, 7:31 a.m. UTC | #3 
On 4/18/24 02:56, Bjorn Andersson wrote:
> On Wed, Apr 17, 2024 at 06:58:53PM +0530, Sibi Sankar wrote:
>> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
>> new file mode 100644
>> index 000000000000..059eb25f217c
>> --- /dev/null
>> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
>> @@ -0,0 +1,188 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*

Hey Bjorn,

Thanks for taking time to review the series :)

>> + * Copyright (c) 2024, The Linux Foundation. All rights reserved.
> 
> Nope.

ack, artefact from the v1 of legacy driver :(

> 
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/mailbox_controller.h>
>> +#include <linux/module.h>
>> +
>> +#define APSS_CPUCP_IPC_CHAN_SUPPORTED		3
>> +#define APSS_CPUCP_MBOX_CMD_OFF			0x4
>> +
>> +/* Tx Registers */
>> +#define APSS_CPUCP_TX_MBOX_IDR			0
>> +#define APSS_CPUCP_TX_MBOX_CMD(i)		(0x100 + ((i) * 8))
>> +
>> +/* Rx Registers */
>> +#define APSS_CPUCP_RX_MBOX_IDR			0
>> +#define APSS_CPUCP_RX_MBOX_CMD(i)		(0x100 + ((i) * 8))
>> +#define APSS_CPUCP_RX_MBOX_MAP			0x4000
>> +#define APSS_CPUCP_RX_MBOX_STAT			0x4400
>> +#define APSS_CPUCP_RX_MBOX_CLEAR		0x4800
>> +#define APSS_CPUCP_RX_MBOX_EN			0x4C00
> 
> Can we have lower case hex digits, plz?

Sure

> 
>> +#define APSS_CPUCP_RX_MBOX_CMD_MASK		0xFFFFFFFFFFFFFFFF
>> +
>> +/**
>> + * struct qcom_cpucp_mbox - Holder for the mailbox driver
>> + * @chans:			The mailbox channel
>> + * @mbox:			The mailbox controller
>> + * @tx_base:			Base address of the CPUCP tx registers
>> + * @rx_base:			Base address of the CPUCP rx registers
>> + * @dev:			Device associated with this instance
>> + * @irq:			CPUCP to AP irq
> 
> @dev and @irq can be a local variables in qcom_cpucp_mbox_probe().

Ack

> 
>> + */
>> +struct qcom_cpucp_mbox {
>> +	struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
>> +	struct mbox_controller mbox;
>> +	void __iomem *tx_base;
>> +	void __iomem *rx_base;
>> +	struct device *dev;
>> +	int irq;
>> +};
>> +
>> +static inline int channel_number(struct mbox_chan *chan)
>> +{
>> +	return chan - chan->mbox->chans;
>> +}
>> +
>> +static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
>> +{
>> +	struct qcom_cpucp_mbox *cpucp = data;
>> +	struct mbox_chan *chan;
>> +	unsigned long flags;
>> +	u64 status;
>> +	u32 val;
>> +	int i;
>> +
>> +	status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
>> +
>> +	for (i = 0; i < APSS_CPUCP_IPC_CHAN_SUPPORTED; i++) {
>> +		val = 0;
> 
> This value is immediately overwritten (or unused).

Ack

> 
>> +		if (status & BIT(i)) {
> 
> Can't you combine the for loop and this conditional into a
> for_each_bit_set()?

The only drawback I see here is if the number of channels increase to
it's full capacity of 64 since for_each_set_bit expects unsigned long.

> 
>> +			val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
>> +			chan = &cpucp->chans[i];
>> +			spin_lock_irqsave(&chan->lock, flags);
> 
> Can you please add a comment here to document that the lock is taken
> here to deal with races against client registration? (It wasn't obvious
> to me...)

This is was put in to handle irqs after channel closure. Meaning we
don't want to send data on a closed/empty channel.

> 
>> +			if (chan->cl)
>> +				mbox_chan_received_data(chan, &val);
>> +			writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
>> +			spin_unlock_irqrestore(&chan->lock, flags);
>> +		}
>> +	}
>> +
>> +	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) + 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)
>> +		return cpucp->irq;
>> +
>> +	ret = devm_request_irq(cpucp->dev, cpucp->irq, qcom_cpucp_mbox_irq_fn,
>> +			       IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
>> +	if (ret < 0)
>> +		return dev_err_probe(cpucp->dev, ret, "Failed to register irq: %d\n", cpucp->irq);
>> +
>> +	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 = devm_mbox_controller_register(cpucp->dev, mbox);
>> +	if (ret)
>> +		return dev_err_probe(cpucp->dev, ret, "Failed to create mailbox\n");
>> +
>> +	platform_set_drvdata(pdev, cpucp);
> 
> I don't see you using the drvdata anywhere, can we drop this?

Yeash I'll drop this in the next re-spin.

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
>> +	{ .compatible = "qcom,x1e80100-cpucp-mbox"},
> 
> A space after the final '"' would be good for aesthetics.

Not sure how I missed it :(

-Sibi

> 
> Regards,
> Bjorn
> 
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
>> +
>> +static struct platform_driver qcom_cpucp_mbox_driver = {
>> +	.probe = qcom_cpucp_mbox_probe,
>> +	.driver = {
>> +		.name = "qcom_cpucp_mbox",
>> +		.of_match_table = qcom_cpucp_mbox_of_match,
>> +	},
>> +};
>> +module_platform_driver(qcom_cpucp_mbox_driver);
>> +
>> +MODULE_DESCRIPTION("QTI CPUCP MBOX Driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.34.1
>>
Bjorn Andersson April 18, 2024, 3:49 p.m. UTC | #4 
On Thu, Apr 18, 2024 at 01:01:50PM +0530, Sibi Sankar wrote:
> On 4/18/24 02:56, Bjorn Andersson wrote:
> > On Wed, Apr 17, 2024 at 06:58:53PM +0530, Sibi Sankar wrote:
> > > diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
[..]
> > 
> > > +		if (status & BIT(i)) {
> > 
> > Can't you combine the for loop and this conditional into a
> > for_each_bit_set()?
> 
> The only drawback I see here is if the number of channels increase to
> it's full capacity of 64 since for_each_set_bit expects unsigned long.
> 

It takes a unsigned long * and it can take a size > BITS_PER_LONG. But
I've not convinced myself that the bit order across two of those matches
the u64 bits.

> > 
> > > +			val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
> > > +			chan = &cpucp->chans[i];
> > > +			spin_lock_irqsave(&chan->lock, flags);
> > 
> > Can you please add a comment here to document that the lock is taken
> > here to deal with races against client registration? (It wasn't obvious
> > to me...)
> 
> This is was put in to handle irqs after channel closure. Meaning we
> don't want to send data on a closed/empty channel.
> 

You're dealing with that through the chan->cl check below, not the lock
itself. So the lock here would be for synchronizing this code with
potentially concurrent execution of __mbox_bind_client() or e.g.
mbox_free_channel().

But if this is indeed the problem, then we're locking here to ensure
that mbox_chan_received_data() will not dereference chan->cl while it's
being modified elsewhere int he mailbox core.

If that's the case, I think this needs to be strongly documented in the
API, or perhaps better yet the lock being moved into
mbox_chan_received_data().

Regards,
Bjorn
kernel test robot April 21, 2024, 8:49 a.m. UTC | #5 
Hi Sibi,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.9-rc4 next-20240419]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sibi-Sankar/dt-bindings-mailbox-qcom-Add-CPUCP-mailbox-controller-bindings/20240417-213339
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240417132856.1106250-3-quic_sibis%40quicinc.com
patch subject: [PATCH V3 2/5] mailbox: Add support for QTI CPUCP mailbox controller
config: hexagon-randconfig-r121-20240421 (https://download.01.org/0day-ci/archive/20240421/202404211602.d8vcGEH0-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce: (https://download.01.org/0day-ci/archive/20240421/202404211602.d8vcGEH0-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404211602.d8vcGEH0-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/mailbox/qcom-cpucp-mbox.c:6:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/mailbox/qcom-cpucp-mbox.c:6:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/mailbox/qcom-cpucp-mbox.c:6:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
>> drivers/mailbox/qcom-cpucp-mbox.c:61:11: error: call to undeclared function 'readq'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
           status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
                    ^
>> drivers/mailbox/qcom-cpucp-mbox.c:71:4: error: call to undeclared function 'writeq'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
                           writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
                           ^
   drivers/mailbox/qcom-cpucp-mbox.c:85:8: error: call to undeclared function 'readq'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
           val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
                 ^
   drivers/mailbox/qcom-cpucp-mbox.c:87:2: error: call to undeclared function 'writeq'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
           writeq(val, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
           ^
   drivers/mailbox/qcom-cpucp-mbox.c:98:8: error: call to undeclared function 'readq'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
           val = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
                 ^
   drivers/mailbox/qcom-cpucp-mbox.c:100:2: error: call to undeclared function 'writeq'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
           writeq(val, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
           ^
   drivers/mailbox/qcom-cpucp-mbox.c:140:2: error: call to undeclared function 'writeq'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
           writeq(0, cpucp->rx_base + APSS_CPUCP_RX_MBOX_EN);
           ^
   6 warnings and 7 errors generated.


vim +/readq +61 drivers/mailbox/qcom-cpucp-mbox.c

    51	
    52	static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
    53	{
    54		struct qcom_cpucp_mbox *cpucp = data;
    55		struct mbox_chan *chan;
    56		unsigned long flags;
    57		u64 status;
    58		u32 val;
    59		int i;
    60	
  > 61		status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
    62	
    63		for (i = 0; i < APSS_CPUCP_IPC_CHAN_SUPPORTED; i++) {
    64			val = 0;
    65			if (status & BIT(i)) {
    66				val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
    67				chan = &cpucp->chans[i];
    68				spin_lock_irqsave(&chan->lock, flags);
    69				if (chan->cl)
    70					mbox_chan_received_data(chan, &val);
  > 71				writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
    72				spin_unlock_irqrestore(&chan->lock, flags);
    73			}
    74		}
    75	
    76		return IRQ_HANDLED;
    77	}
    78
diff mbox series

Patch

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 42940108a187..23741a6f054e 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -273,6 +273,14 @@  config SPRD_MBOX
 	  to send message between application processors and MCU. Say Y here if
 	  you want to build the Spreatrum mailbox controller driver.
 
+config QCOM_CPUCP_MBOX
+	tristate "Qualcomm Technologies, Inc. CPUCP mailbox driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	help
+	  Qualcomm Technologies, Inc. CPUSS Control Processor (CPUCP) mailbox
+	  controller driver enables communication between AP and CPUCP. Say
+	  Y here if you want to build this driver.
+
 config QCOM_IPCC
 	tristate "Qualcomm Technologies, Inc. IPCC driver"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 18793e6caa2f..53b512800bde 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -59,4 +59,6 @@  obj-$(CONFIG_SUN6I_MSGBOX)	+= sun6i-msgbox.o
 
 obj-$(CONFIG_SPRD_MBOX)		+= sprd-mailbox.o
 
+obj-$(CONFIG_QCOM_CPUCP_MBOX)	+= qcom-cpucp-mbox.o
+
 obj-$(CONFIG_QCOM_IPCC)		+= qcom-ipcc.o
diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
new file mode 100644
index 000000000000..059eb25f217c
--- /dev/null
+++ b/drivers/mailbox/qcom-cpucp-mbox.c
@@ -0,0 +1,188 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2024, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/platform_device.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+
+#define APSS_CPUCP_IPC_CHAN_SUPPORTED		3
+#define APSS_CPUCP_MBOX_CMD_OFF			0x4
+
+/* Tx Registers */
+#define APSS_CPUCP_TX_MBOX_IDR			0
+#define APSS_CPUCP_TX_MBOX_CMD(i)		(0x100 + ((i) * 8))
+
+/* Rx Registers */
+#define APSS_CPUCP_RX_MBOX_IDR			0
+#define APSS_CPUCP_RX_MBOX_CMD(i)		(0x100 + ((i) * 8))
+#define APSS_CPUCP_RX_MBOX_MAP			0x4000
+#define APSS_CPUCP_RX_MBOX_STAT			0x4400
+#define APSS_CPUCP_RX_MBOX_CLEAR		0x4800
+#define APSS_CPUCP_RX_MBOX_EN			0x4C00
+#define APSS_CPUCP_RX_MBOX_CMD_MASK		0xFFFFFFFFFFFFFFFF
+
+/**
+ * struct qcom_cpucp_mbox - Holder for the mailbox driver
+ * @chans:			The mailbox channel
+ * @mbox:			The mailbox controller
+ * @tx_base:			Base address of the CPUCP tx registers
+ * @rx_base:			Base address of the CPUCP rx registers
+ * @dev:			Device associated with this instance
+ * @irq:			CPUCP to AP irq
+ */
+struct qcom_cpucp_mbox {
+	struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
+	struct mbox_controller mbox;
+	void __iomem *tx_base;
+	void __iomem *rx_base;
+	struct device *dev;
+	int irq;
+};
+
+static inline int channel_number(struct mbox_chan *chan)
+{
+	return chan - chan->mbox->chans;
+}
+
+static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
+{
+	struct qcom_cpucp_mbox *cpucp = data;
+	struct mbox_chan *chan;
+	unsigned long flags;
+	u64 status;
+	u32 val;
+	int i;
+
+	status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
+
+	for (i = 0; i < APSS_CPUCP_IPC_CHAN_SUPPORTED; i++) {
+		val = 0;
+		if (status & BIT(i)) {
+			val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
+			chan = &cpucp->chans[i];
+			spin_lock_irqsave(&chan->lock, flags);
+			if (chan->cl)
+				mbox_chan_received_data(chan, &val);
+			writeq(BIT(i), cpucp->rx_base + APSS_CPUCP_RX_MBOX_CLEAR);
+			spin_unlock_irqrestore(&chan->lock, flags);
+		}
+	}
+
+	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) + 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)
+		return cpucp->irq;
+
+	ret = devm_request_irq(cpucp->dev, cpucp->irq, qcom_cpucp_mbox_irq_fn,
+			       IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
+	if (ret < 0)
+		return dev_err_probe(cpucp->dev, ret, "Failed to register irq: %d\n", cpucp->irq);
+
+	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 = devm_mbox_controller_register(cpucp->dev, mbox);
+	if (ret)
+		return dev_err_probe(cpucp->dev, ret, "Failed to create mailbox\n");
+
+	platform_set_drvdata(pdev, cpucp);
+
+	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,
+	.driver = {
+		.name = "qcom_cpucp_mbox",
+		.of_match_table = qcom_cpucp_mbox_of_match,
+	},
+};
+module_platform_driver(qcom_cpucp_mbox_driver);
+
+MODULE_DESCRIPTION("QTI CPUCP MBOX Driver");
+MODULE_LICENSE("GPL");