[PATCH 0/3] qcom: sc7280: Enable cpucp mbox

[PATCH 0/3] qcom: sc7280: Enable cpucp mbox

[Shivnandan Kumar]

This series enables CPUCP mailbox support on the SC7280 SoC,
facilitating communication between Linux and the CPUCP firmware.

Shivnandan Kumar (3):
  dt-bindings: mailbox: qcom,cpucp-mbox: Add sc7280 cpucp mailbox
    instance
  mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox
    controller
  arm64: dts: qcom: sc7280: Add cpucp mbox node

 .../bindings/mailbox/qcom,cpucp-mbox.yaml     |   5 +-
 arch/arm64/boot/dts/qcom/sc7280.dtsi          |   8 +
 drivers/mailbox/qcom-cpucp-mbox.c             | 156 ++++++++++++++----
 3 files changed, 133 insertions(+), 36 deletions(-)

--
2.25.1

[PATCH 1/3] dt-bindings: mailbox: qcom,cpucp-mbox: Add sc7280 cpucp mailbox instance

[Shivnandan Kumar]

sc7280 has a cpucp mailbox. Document them.

Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
---
 .../devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml         | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
index f7342d04beec..4a7ea072a3c1 100644
--- a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
+++ b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
@@ -15,8 +15,9 @@ description:

 properties:
   compatible:
-    items:
-      - const: qcom,x1e80100-cpucp-mbox
+    enum:
+      - qcom,x1e80100-cpucp-mbox
+      - qcom,sc7280-cpucp-mbox

   reg:
     items:
--
2.25.1

[PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller

[Shivnandan Kumar]

The SC7280 CPUCP mailbox controller is compatible with legacy mailbox
hardware.
Implement support for this functionality which enable HLOS to
CPUCP communication.

Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
---
 drivers/mailbox/qcom-cpucp-mbox.c | 156 +++++++++++++++++++++++-------
 1 file changed, 122 insertions(+), 34 deletions(-)

diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
index e5437c294803..faae6e069ea1 100644
--- a/drivers/mailbox/qcom-cpucp-mbox.c
+++ b/drivers/mailbox/qcom-cpucp-mbox.c
@@ -13,18 +13,24 @@
 #include <linux/platform_device.h>

 #define APSS_CPUCP_IPC_CHAN_SUPPORTED		3
-#define APSS_CPUCP_MBOX_CMD_OFF			0x4
-
-/* Tx Registers */
-#define APSS_CPUCP_TX_MBOX_CMD(i)		(0x100 + ((i) * 8))

 /* Rx Registers */
-#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		GENMASK_ULL(63, 0)
+#define APSS_CPUCP_V2_RX_MBOX_CMD_MASK		GENMASK_ULL(63, 0)
+#define APSS_CPUCP_V1_SEND_IRQ_VAL		BIT(28)
+#define APSS_CPUCP_V1_CLEAR_IRQ_VAL		BIT(3)
+#define APSS_CPUCP_V1_STATUS_IRQ_VAL		BIT(3)
+
+struct qcom_cpucp_mbox_desc {
+	u32 enable_reg;
+	u32 map_reg;
+	u32 rx_reg;
+	u32 tx_reg;
+	u32 status_reg;
+	u32 clear_reg;
+	u32 chan_stride;
+	bool v2_mbox;
+	u32 num_chans;
+};

 /**
  * struct qcom_cpucp_mbox - Holder for the mailbox driver
@@ -35,6 +41,7 @@
  */
 struct qcom_cpucp_mbox {
 	struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
+	const struct qcom_cpucp_mbox_desc *desc;
 	struct mbox_controller mbox;
 	void __iomem *tx_base;
 	void __iomem *rx_base;
@@ -48,13 +55,40 @@ static inline int channel_number(struct mbox_chan *chan)
 static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
 {
 	struct qcom_cpucp_mbox *cpucp = data;
+	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
+	int i;
+
+	for (i = 0; i < desc->num_chans; i++) {
+		u32 val = readl(cpucp->rx_base + desc->status_reg + (i * desc->chan_stride));
+		struct mbox_chan *chan = &cpucp->chans[i];
+		unsigned long flags;
+
+		if (val & APSS_CPUCP_V1_STATUS_IRQ_VAL) {
+			writel(APSS_CPUCP_V1_CLEAR_IRQ_VAL,
+			       cpucp->rx_base + desc->clear_reg + (i * desc->chan_stride));
+			/* Make sure reg write is complete before proceeding */
+			mb();
+			spin_lock_irqsave(&chan->lock, flags);
+			if (chan->cl)
+				mbox_chan_received_data(chan, NULL);
+			spin_unlock_irqrestore(&chan->lock, flags);
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t qcom_cpucp_v2_mbox_irq_fn(int irq, void *data)
+{
+	struct qcom_cpucp_mbox *cpucp = data;
+	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
 	u64 status;
 	int i;

-	status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
+	status = readq(cpucp->rx_base + desc->status_reg);

-	for_each_set_bit(i, (unsigned long *)&status, APSS_CPUCP_IPC_CHAN_SUPPORTED) {
-		u32 val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
+	for_each_set_bit(i, (unsigned long *)&status, desc->num_chans) {
+		u32 val = readl(cpucp->rx_base + desc->rx_reg + (i * desc->chan_stride));
 		struct mbox_chan *chan = &cpucp->chans[i];
 		unsigned long flags;

@@ -62,7 +96,7 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
 		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);
+		writeq(BIT(i), cpucp->rx_base + desc->clear_reg);
 		spin_unlock_irqrestore(&chan->lock, flags);
 	}

@@ -72,12 +106,15 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
 static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
 {
 	struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
+	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
 	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);
+	if (desc->v2_mbox) {
+		val = readq(cpucp->rx_base + desc->enable_reg);
+		val |= BIT(chan_id);
+		writeq(val, cpucp->rx_base + desc->enable_reg);
+	}

 	return 0;
 }
@@ -85,22 +122,26 @@ static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
 static void qcom_cpucp_mbox_shutdown(struct mbox_chan *chan)
 {
 	struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
+	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
 	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);
+	if (desc->v2_mbox) {
+		val = readq(cpucp->rx_base + desc->enable_reg);
+		val &= ~BIT(chan_id);
+		writeq(val, cpucp->rx_base + desc->enable_reg);
+	}
 }

 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);
+	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
+	u32 val = desc->v2_mbox ? *(u32 *)data : APSS_CPUCP_V1_SEND_IRQ_VAL;
 	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);
+	u32 offset = desc->v2_mbox ? (chan_id * desc->chan_stride) : 0;

+	writel(val, cpucp->tx_base + desc->tx_reg + offset);
 	return 0;
 }

@@ -112,41 +153,66 @@ static const struct mbox_chan_ops qcom_cpucp_mbox_chan_ops = {

 static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
 {
+	const struct qcom_cpucp_mbox_desc *desc;
 	struct device *dev = &pdev->dev;
 	struct qcom_cpucp_mbox *cpucp;
 	struct mbox_controller *mbox;
+	struct resource *res;
 	int irq, ret;

+	desc = device_get_match_data(&pdev->dev);
+	if (!desc)
+		return -EINVAL;
+
 	cpucp = devm_kzalloc(dev, sizeof(*cpucp), GFP_KERNEL);
 	if (!cpucp)
 		return -ENOMEM;

-	cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
-	if (IS_ERR(cpucp->rx_base))
-		return PTR_ERR(cpucp->rx_base);
+	cpucp->desc = desc;
+
+	if (desc->v2_mbox) {
+		cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
+		if (IS_ERR(cpucp->rx_base))
+			return PTR_ERR(cpucp->rx_base);
+	/* Legacy mailbox quirks due to shared region with EPSS register space */
+	} else {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (!res) {
+			dev_err(&pdev->dev, "Failed to get the device base address\n");
+			return -ENODEV;
+		}
+		cpucp->rx_base = devm_ioremap(dev, res->start, resource_size(res));
+		if (!cpucp->rx_base) {
+			dev_err(dev, "Failed to ioremap the cpucp rx irq addr\n");
+			return -ENOMEM;
+		}
+	}

 	cpucp->tx_base = devm_of_iomap(dev, 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);
+	if (desc->v2_mbox) {
+		writeq(0, cpucp->rx_base + desc->enable_reg);
+		writeq(0, cpucp->rx_base + desc->clear_reg);
+		writeq(0, cpucp->rx_base + desc->map_reg);
+	}

 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
 		return irq;

-	ret = devm_request_irq(dev, irq, qcom_cpucp_mbox_irq_fn,
-			       IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
+	ret = devm_request_irq(dev, irq, desc->v2_mbox ? qcom_cpucp_v2_mbox_irq_fn :
+		qcom_cpucp_mbox_irq_fn, IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to register irq: %d\n", irq);

-	writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
+	if (desc->v2_mbox)
+		writeq(APSS_CPUCP_V2_RX_MBOX_CMD_MASK, cpucp->rx_base + desc->map_reg);

 	mbox = &cpucp->mbox;
 	mbox->dev = dev;
-	mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
+	mbox->num_chans = desc->num_chans;
 	mbox->chans = cpucp->chans;
 	mbox->ops = &qcom_cpucp_mbox_chan_ops;

@@ -157,8 +223,30 @@ static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
 	return 0;
 }

+static const struct qcom_cpucp_mbox_desc sc7280_cpucp_mbox = {
+	.tx_reg = 0xC,
+	.chan_stride = 0x1000,
+	.status_reg = 0x30C,
+	.clear_reg = 0x308,
+	.v2_mbox = false,
+	.num_chans = 2,
+};
+
+static const struct qcom_cpucp_mbox_desc x1e80100_cpucp_mbox = {
+	.rx_reg = 0x104,
+	.tx_reg = 0x104,
+	.chan_stride = 0x8,
+	.map_reg = 0x4000,
+	.status_reg = 0x4400,
+	.clear_reg = 0x4800,
+	.enable_reg = 0x4C00,
+	.v2_mbox = true,
+	.num_chans = 3,
+};
+
 static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
-	{ .compatible = "qcom,x1e80100-cpucp-mbox" },
+	{ .compatible = "qcom,x1e80100-cpucp-mbox", .data = &x1e80100_cpucp_mbox},
+	{ .compatible = "qcom,sc7280-cpucp-mbox", .data = &sc7280_cpucp_mbox},
 	{}
 };
 MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
--
2.25.1

[PATCH 3/3] arm64: dts: qcom: sc7280: Add cpucp mbox node

[Shivnandan Kumar]

Add the CPUCP mailbox node required for communication with CPUCP.

Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 3d8410683402..4b9b26a75c62 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -4009,6 +4009,14 @@ gem_noc: interconnect@9100000 {
 			qcom,bcm-voters = <&apps_bcm_voter>;
 		};

+		cpucp_mbox: mailbox@17430000 {
+			compatible = "qcom,sc7280-cpucp-mbox";
+			reg = <0 0x18590000 0 0x2000>,
+			      <0 0x17C00000 0 0x10>;
+			interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
+			#mbox-cells = <1>;
+		};
+
 		system-cache-controller@9200000 {
 			compatible = "qcom,sc7280-llcc";
 			reg = <0 0x09200000 0 0x58000>, <0 0x09280000 0 0x58000>,
--
2.25.1

Re: [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller

[Dmitry Baryshkov]

On Tue, Sep 24, 2024 at 10:39:40AM GMT, Shivnandan Kumar wrote:
> The SC7280 CPUCP mailbox controller is compatible with legacy mailbox
> hardware.
> Implement support for this functionality which enable HLOS to

s/HLOS/Linux/

> CPUCP communication.

This enables Linux to do this and that.

> 
> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
> ---
>  drivers/mailbox/qcom-cpucp-mbox.c | 156 +++++++++++++++++++++++-------
>  1 file changed, 122 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
> index e5437c294803..faae6e069ea1 100644
> --- a/drivers/mailbox/qcom-cpucp-mbox.c
> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
> @@ -13,18 +13,24 @@
>  #include <linux/platform_device.h>
> 
>  #define APSS_CPUCP_IPC_CHAN_SUPPORTED		3
> -#define APSS_CPUCP_MBOX_CMD_OFF			0x4
> -
> -/* Tx Registers */
> -#define APSS_CPUCP_TX_MBOX_CMD(i)		(0x100 + ((i) * 8))
> 
>  /* Rx Registers */
> -#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		GENMASK_ULL(63, 0)
> +#define APSS_CPUCP_V2_RX_MBOX_CMD_MASK		GENMASK_ULL(63, 0)
> +#define APSS_CPUCP_V1_SEND_IRQ_VAL		BIT(28)
> +#define APSS_CPUCP_V1_CLEAR_IRQ_VAL		BIT(3)
> +#define APSS_CPUCP_V1_STATUS_IRQ_VAL		BIT(3)
> +
> +struct qcom_cpucp_mbox_desc {
> +	u32 enable_reg;
> +	u32 map_reg;
> +	u32 rx_reg;
> +	u32 tx_reg;
> +	u32 status_reg;
> +	u32 clear_reg;
> +	u32 chan_stride;
> +	bool v2_mbox;
> +	u32 num_chans;
> +};
> 
>  /**
>   * struct qcom_cpucp_mbox - Holder for the mailbox driver
> @@ -35,6 +41,7 @@
>   */
>  struct qcom_cpucp_mbox {
>  	struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
> +	const struct qcom_cpucp_mbox_desc *desc;
>  	struct mbox_controller mbox;
>  	void __iomem *tx_base;
>  	void __iomem *rx_base;
> @@ -48,13 +55,40 @@ static inline int channel_number(struct mbox_chan *chan)
>  static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)

qcom_cpucp_v1_mbox_irq_fn()

>  {
>  	struct qcom_cpucp_mbox *cpucp = data;
> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
> +	int i;
> +
> +	for (i = 0; i < desc->num_chans; i++) {
> +		u32 val = readl(cpucp->rx_base + desc->status_reg + (i * desc->chan_stride));
> +		struct mbox_chan *chan = &cpucp->chans[i];
> +		unsigned long flags;
> +
> +		if (val & APSS_CPUCP_V1_STATUS_IRQ_VAL) {
> +			writel(APSS_CPUCP_V1_CLEAR_IRQ_VAL,
> +			       cpucp->rx_base + desc->clear_reg + (i * desc->chan_stride));
> +			/* Make sure reg write is complete before proceeding */
> +			mb();
> +			spin_lock_irqsave(&chan->lock, flags);
> +			if (chan->cl)
> +				mbox_chan_received_data(chan, NULL);

v1 clears IRQ before processing, v2 does it after pinging mbox subsys.
Is that expected? What warrants such a difference?

> +			spin_unlock_irqrestore(&chan->lock, flags);
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t qcom_cpucp_v2_mbox_irq_fn(int irq, void *data)
> +{
> +	struct qcom_cpucp_mbox *cpucp = data;
> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>  	u64 status;
>  	int i;
> 
> -	status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
> +	status = readq(cpucp->rx_base + desc->status_reg);
> 
> -	for_each_set_bit(i, (unsigned long *)&status, APSS_CPUCP_IPC_CHAN_SUPPORTED) {
> -		u32 val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
> +	for_each_set_bit(i, (unsigned long *)&status, desc->num_chans) {
> +		u32 val = readl(cpucp->rx_base + desc->rx_reg + (i * desc->chan_stride));
>  		struct mbox_chan *chan = &cpucp->chans[i];
>  		unsigned long flags;
> 
> @@ -62,7 +96,7 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
>  		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);
> +		writeq(BIT(i), cpucp->rx_base + desc->clear_reg);
>  		spin_unlock_irqrestore(&chan->lock, flags);
>  	}
> 
> @@ -72,12 +106,15 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
>  static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
>  {
>  	struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>  	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);
> +	if (desc->v2_mbox) {
> +		val = readq(cpucp->rx_base + desc->enable_reg);
> +		val |= BIT(chan_id);
> +		writeq(val, cpucp->rx_base + desc->enable_reg);
> +	}

So, startup and shutdown are effectively empty for v1 CPUCP chips? Could
you please add two separate families of functions and two separate
mbox_chan_ops instances? Which probably will mean one register less in
the qcom_cpucp_mbox_desc structure.

> 
>  	return 0;
>  }
> @@ -85,22 +122,26 @@ static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
>  static void qcom_cpucp_mbox_shutdown(struct mbox_chan *chan)
>  {
>  	struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>  	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);
> +	if (desc->v2_mbox) {
> +		val = readq(cpucp->rx_base + desc->enable_reg);
> +		val &= ~BIT(chan_id);
> +		writeq(val, cpucp->rx_base + desc->enable_reg);
> +	}
>  }
> 
>  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);
> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
> +	u32 val = desc->v2_mbox ? *(u32 *)data : APSS_CPUCP_V1_SEND_IRQ_VAL;
>  	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);
> +	u32 offset = desc->v2_mbox ? (chan_id * desc->chan_stride) : 0;
> 
> +	writel(val, cpucp->tx_base + desc->tx_reg + offset);
>  	return 0;
>  }
> 
> @@ -112,41 +153,66 @@ static const struct mbox_chan_ops qcom_cpucp_mbox_chan_ops = {
> 
>  static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
>  {
> +	const struct qcom_cpucp_mbox_desc *desc;
>  	struct device *dev = &pdev->dev;
>  	struct qcom_cpucp_mbox *cpucp;
>  	struct mbox_controller *mbox;
> +	struct resource *res;
>  	int irq, ret;
> 
> +	desc = device_get_match_data(&pdev->dev);
> +	if (!desc)
> +		return -EINVAL;
> +
>  	cpucp = devm_kzalloc(dev, sizeof(*cpucp), GFP_KERNEL);
>  	if (!cpucp)
>  		return -ENOMEM;
> 
> -	cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
> -	if (IS_ERR(cpucp->rx_base))
> -		return PTR_ERR(cpucp->rx_base);
> +	cpucp->desc = desc;
> +
> +	if (desc->v2_mbox) {
> +		cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
> +		if (IS_ERR(cpucp->rx_base))
> +			return PTR_ERR(cpucp->rx_base);
> +	/* Legacy mailbox quirks due to shared region with EPSS register space */

Does that mean that on these platforms cpucp should be a child node of the EPSS? Also it might be a high time when the drivers should be switched to use regmaps.

> +	} else {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (!res) {
> +			dev_err(&pdev->dev, "Failed to get the device base address\n");
> +			return -ENODEV;
> +		}
> +		cpucp->rx_base = devm_ioremap(dev, res->start, resource_size(res));
> +		if (!cpucp->rx_base) {
> +			dev_err(dev, "Failed to ioremap the cpucp rx irq addr\n");
> +			return -ENOMEM;
> +		}
> +	}
> 
>  	cpucp->tx_base = devm_of_iomap(dev, 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);
> +	if (desc->v2_mbox) {
> +		writeq(0, cpucp->rx_base + desc->enable_reg);
> +		writeq(0, cpucp->rx_base + desc->clear_reg);
> +		writeq(0, cpucp->rx_base + desc->map_reg);
> +	}

If these registers are only present on V2, there is no need to have them
in the qcom_cpucp_mbox_desc.

> 
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
>  		return irq;
> 
> -	ret = devm_request_irq(dev, irq, qcom_cpucp_mbox_irq_fn,
> -			       IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
> +	ret = devm_request_irq(dev, irq, desc->v2_mbox ? qcom_cpucp_v2_mbox_irq_fn :
> +		qcom_cpucp_mbox_irq_fn, IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to register irq: %d\n", irq);
> 
> -	writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
> +	if (desc->v2_mbox)
> +		writeq(APSS_CPUCP_V2_RX_MBOX_CMD_MASK, cpucp->rx_base + desc->map_reg);
> 
>  	mbox = &cpucp->mbox;
>  	mbox->dev = dev;
> -	mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
> +	mbox->num_chans = desc->num_chans;
>  	mbox->chans = cpucp->chans;
>  	mbox->ops = &qcom_cpucp_mbox_chan_ops;
> 
> @@ -157,8 +223,30 @@ static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
>  	return 0;
>  }
> 
> +static const struct qcom_cpucp_mbox_desc sc7280_cpucp_mbox = {
> +	.tx_reg = 0xC,
> +	.chan_stride = 0x1000,
> +	.status_reg = 0x30C,
> +	.clear_reg = 0x308,
> +	.v2_mbox = false,
> +	.num_chans = 2,
> +};
> +
> +static const struct qcom_cpucp_mbox_desc x1e80100_cpucp_mbox = {
> +	.rx_reg = 0x104,
> +	.tx_reg = 0x104,
> +	.chan_stride = 0x8,
> +	.map_reg = 0x4000,
> +	.status_reg = 0x4400,
> +	.clear_reg = 0x4800,
> +	.enable_reg = 0x4C00,
> +	.v2_mbox = true,
> +	.num_chans = 3,
> +};
> +
>  static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
> -	{ .compatible = "qcom,x1e80100-cpucp-mbox" },
> +	{ .compatible = "qcom,x1e80100-cpucp-mbox", .data = &x1e80100_cpucp_mbox},
> +	{ .compatible = "qcom,sc7280-cpucp-mbox", .data = &sc7280_cpucp_mbox},

Please keep the table sorted.

>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
> --
> 2.25.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller

[kernel test robot]

Hi Shivnandan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.11 next-20240924]
[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/Shivnandan-Kumar/dt-bindings-mailbox-qcom-cpucp-mbox-Add-sc7280-cpucp-mailbox-instance/20240924-133657
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240924050941.1251485-3-quic_kshivnan%40quicinc.com
patch subject: [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240924/202409242349.yXq4CDaH-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240924/202409242349.yXq4CDaH-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/202409242349.yXq4CDaH-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/mailbox/qcom-cpucp-mbox.c:48: warning: Function parameter or struct member 'desc' not described in 'qcom_cpucp_mbox'


vim +48 drivers/mailbox/qcom-cpucp-mbox.c

0e2a9a03106cd5 Sibi Sankar      2024-06-12  34  
0e2a9a03106cd5 Sibi Sankar      2024-06-12  35  /**
0e2a9a03106cd5 Sibi Sankar      2024-06-12  36   * struct qcom_cpucp_mbox - Holder for the mailbox driver
0e2a9a03106cd5 Sibi Sankar      2024-06-12  37   * @chans:			The mailbox channel
0e2a9a03106cd5 Sibi Sankar      2024-06-12  38   * @mbox:			The mailbox controller
0e2a9a03106cd5 Sibi Sankar      2024-06-12  39   * @tx_base:			Base address of the CPUCP tx registers
0e2a9a03106cd5 Sibi Sankar      2024-06-12  40   * @rx_base:			Base address of the CPUCP rx registers
0e2a9a03106cd5 Sibi Sankar      2024-06-12  41   */
0e2a9a03106cd5 Sibi Sankar      2024-06-12  42  struct qcom_cpucp_mbox {
0e2a9a03106cd5 Sibi Sankar      2024-06-12  43  	struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
654ed3904f995a Shivnandan Kumar 2024-09-24  44  	const struct qcom_cpucp_mbox_desc *desc;
0e2a9a03106cd5 Sibi Sankar      2024-06-12  45  	struct mbox_controller mbox;
0e2a9a03106cd5 Sibi Sankar      2024-06-12  46  	void __iomem *tx_base;
0e2a9a03106cd5 Sibi Sankar      2024-06-12  47  	void __iomem *rx_base;
0e2a9a03106cd5 Sibi Sankar      2024-06-12 @48  };
0e2a9a03106cd5 Sibi Sankar      2024-06-12  49  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/3] dt-bindings: mailbox: qcom,cpucp-mbox: Add sc7280 cpucp mailbox instance

[Rob Herring]

On Tue, Sep 24, 2024 at 10:39:39AM +0530, Shivnandan Kumar wrote:
> sc7280 has a cpucp mailbox. Document them.

And is different from the existing device how?

> 
> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
> ---
>  .../devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml         | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
> index f7342d04beec..4a7ea072a3c1 100644
> --- a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
> +++ b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
> @@ -15,8 +15,9 @@ description:
> 
>  properties:
>    compatible:
> -    items:
> -      - const: qcom,x1e80100-cpucp-mbox
> +    enum:
> +      - qcom,x1e80100-cpucp-mbox
> +      - qcom,sc7280-cpucp-mbox
> 
>    reg:
>      items:
> --
> 2.25.1
> 

Re: [PATCH 3/3] arm64: dts: qcom: sc7280: Add cpucp mbox node

[Krzysztof Kozlowski]

On 24/09/2024 07:09, Shivnandan Kumar wrote:
> Add the CPUCP mailbox node required for communication with CPUCP.
> 
> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 3d8410683402..4b9b26a75c62 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -4009,6 +4009,14 @@ gem_noc: interconnect@9100000 {
>  			qcom,bcm-voters = <&apps_bcm_voter>;
>  		};
> 
> +		cpucp_mbox: mailbox@17430000 {

Are you sure you placed it in correct location (the order is by unit
address, see DTS coding style).

> +			compatible = "qcom,sc7280-cpucp-mbox";
> +			reg = <0 0x18590000 0 0x2000>,
> +			      <0 0x17C00000 0 0x10>;

Lowercase hex... we just fixed it everywhere and you introduce again
same issues.



Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: mailbox: qcom,cpucp-mbox: Add sc7280 cpucp mailbox instance

[Krzysztof Kozlowski]

On 24/09/2024 07:09, Shivnandan Kumar wrote:
> sc7280 has a cpucp mailbox. Document them.
> 
> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
> ---
>  .../devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml         | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
> index f7342d04beec..4a7ea072a3c1 100644
> --- a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
> +++ b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
> @@ -15,8 +15,9 @@ description:
> 
>  properties:
>    compatible:
> -    items:
> -      - const: qcom,x1e80100-cpucp-mbox
> +    enum:
> +      - qcom,x1e80100-cpucp-mbox
> +      - qcom,sc7280-cpucp-mbox

Keep alphabetical order.

Best regards,
Krzysztof

Re: [PATCH 3/3] arm64: dts: qcom: sc7280: Add cpucp mbox node

[Shivnandan Kumar]

On 9/25/2024 7:52 PM, Krzysztof Kozlowski wrote:
> On 24/09/2024 07:09, Shivnandan Kumar wrote:
>> Add the CPUCP mailbox node required for communication with CPUCP.
>>
>> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sc7280.dtsi | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index 3d8410683402..4b9b26a75c62 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -4009,6 +4009,14 @@ gem_noc: interconnect@9100000 {
>>   			qcom,bcm-voters = <&apps_bcm_voter>;
>>   		};
>>
>> +		cpucp_mbox: mailbox@17430000 {
> 
> Are you sure you placed it in correct location (the order is by unit
> address, see DTS coding style).
> 

I will correct it in next series.


>> +			compatible = "qcom,sc7280-cpucp-mbox";
>> +			reg = <0 0x18590000 0 0x2000>,
>> +			      <0 0x17C00000 0 0x10>;
> 
> Lowercase hex... we just fixed it everywhere and you introduce again
> same issues.
>


ACK

> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller

[Shivnandan Kumar]

Thanks Dmitry for reviewing this patch.


On 9/24/2024 12:14 PM, Dmitry Baryshkov wrote:
> On Tue, Sep 24, 2024 at 10:39:40AM GMT, Shivnandan Kumar wrote:
>> The SC7280 CPUCP mailbox controller is compatible with legacy mailbox
>> hardware.
>> Implement support for this functionality which enable HLOS to
> 
> s/HLOS/Linux/
> 

ACK

>> CPUCP communication.
> 
> This enables Linux to do this and that.
> 
>>
>> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
>> ---
>>   drivers/mailbox/qcom-cpucp-mbox.c | 156 +++++++++++++++++++++++-------
>>   1 file changed, 122 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
>> index e5437c294803..faae6e069ea1 100644
>> --- a/drivers/mailbox/qcom-cpucp-mbox.c
>> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
>> @@ -13,18 +13,24 @@
>>   #include <linux/platform_device.h>
>>
>>   #define APSS_CPUCP_IPC_CHAN_SUPPORTED		3
>> -#define APSS_CPUCP_MBOX_CMD_OFF			0x4
>> -
>> -/* Tx Registers */
>> -#define APSS_CPUCP_TX_MBOX_CMD(i)		(0x100 + ((i) * 8))
>>
>>   /* Rx Registers */
>> -#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		GENMASK_ULL(63, 0)
>> +#define APSS_CPUCP_V2_RX_MBOX_CMD_MASK		GENMASK_ULL(63, 0)
>> +#define APSS_CPUCP_V1_SEND_IRQ_VAL		BIT(28)
>> +#define APSS_CPUCP_V1_CLEAR_IRQ_VAL		BIT(3)
>> +#define APSS_CPUCP_V1_STATUS_IRQ_VAL		BIT(3)
>> +
>> +struct qcom_cpucp_mbox_desc {
>> +	u32 enable_reg;
>> +	u32 map_reg;
>> +	u32 rx_reg;
>> +	u32 tx_reg;
>> +	u32 status_reg;
>> +	u32 clear_reg;
>> +	u32 chan_stride;
>> +	bool v2_mbox;
>> +	u32 num_chans;
>> +};
>>
>>   /**
>>    * struct qcom_cpucp_mbox - Holder for the mailbox driver
>> @@ -35,6 +41,7 @@
>>    */
>>   struct qcom_cpucp_mbox {
>>   	struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
>> +	const struct qcom_cpucp_mbox_desc *desc;
>>   	struct mbox_controller mbox;
>>   	void __iomem *tx_base;
>>   	void __iomem *rx_base;
>> @@ -48,13 +55,40 @@ static inline int channel_number(struct mbox_chan *chan)
>>   static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
> 
> qcom_cpucp_v1_mbox_irq_fn()
> 
>>   {
>>   	struct qcom_cpucp_mbox *cpucp = data;
>> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>> +	int i;
>> +
>> +	for (i = 0; i < desc->num_chans; i++) {
>> +		u32 val = readl(cpucp->rx_base + desc->status_reg + (i * desc->chan_stride));
>> +		struct mbox_chan *chan = &cpucp->chans[i];
>> +		unsigned long flags;
>> +
>> +		if (val & APSS_CPUCP_V1_STATUS_IRQ_VAL) {
>> +			writel(APSS_CPUCP_V1_CLEAR_IRQ_VAL,
>> +			       cpucp->rx_base + desc->clear_reg + (i * desc->chan_stride));
>> +			/* Make sure reg write is complete before proceeding */
>> +			mb();
>> +			spin_lock_irqsave(&chan->lock, flags);
>> +			if (chan->cl)
>> +				mbox_chan_received_data(chan, NULL);
> 
> v1 clears IRQ before processing, v2 does it after pinging mbox subsys.
> Is that expected? What warrants such a difference?
> 

ACK, I will change it to make it same as v2.

>> +			spin_unlock_irqrestore(&chan->lock, flags);
>> +		}
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t qcom_cpucp_v2_mbox_irq_fn(int irq, void *data)
>> +{
>> +	struct qcom_cpucp_mbox *cpucp = data;
>> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>>   	u64 status;
>>   	int i;
>>
>> -	status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
>> +	status = readq(cpucp->rx_base + desc->status_reg);
>>
>> -	for_each_set_bit(i, (unsigned long *)&status, APSS_CPUCP_IPC_CHAN_SUPPORTED) {
>> -		u32 val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
>> +	for_each_set_bit(i, (unsigned long *)&status, desc->num_chans) {
>> +		u32 val = readl(cpucp->rx_base + desc->rx_reg + (i * desc->chan_stride));
>>   		struct mbox_chan *chan = &cpucp->chans[i];
>>   		unsigned long flags;
>>
>> @@ -62,7 +96,7 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
>>   		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);
>> +		writeq(BIT(i), cpucp->rx_base + desc->clear_reg);
>>   		spin_unlock_irqrestore(&chan->lock, flags);
>>   	}
>>
>> @@ -72,12 +106,15 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
>>   static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
>>   {
>>   	struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
>> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>>   	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);
>> +	if (desc->v2_mbox) {
>> +		val = readq(cpucp->rx_base + desc->enable_reg);
>> +		val |= BIT(chan_id);
>> +		writeq(val, cpucp->rx_base + desc->enable_reg);
>> +	}
> 
> So, startup and shutdown are effectively empty for v1 CPUCP chips? Could
> you please add two separate families of functions and two separate
> mbox_chan_ops instances? Which probably will mean one register less in
> the qcom_cpucp_mbox_desc structure.
> 

ACK,

>>
>>   	return 0;
>>   }
>> @@ -85,22 +122,26 @@ static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
>>   static void qcom_cpucp_mbox_shutdown(struct mbox_chan *chan)
>>   {
>>   	struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
>> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>>   	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);
>> +	if (desc->v2_mbox) {
>> +		val = readq(cpucp->rx_base + desc->enable_reg);
>> +		val &= ~BIT(chan_id);
>> +		writeq(val, cpucp->rx_base + desc->enable_reg);
>> +	}
>>   }
>>
>>   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);
>> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>> +	u32 val = desc->v2_mbox ? *(u32 *)data : APSS_CPUCP_V1_SEND_IRQ_VAL;
>>   	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);
>> +	u32 offset = desc->v2_mbox ? (chan_id * desc->chan_stride) : 0;
>>
>> +	writel(val, cpucp->tx_base + desc->tx_reg + offset);
>>   	return 0;
>>   }
>>
>> @@ -112,41 +153,66 @@ static const struct mbox_chan_ops qcom_cpucp_mbox_chan_ops = {
>>
>>   static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
>>   {
>> +	const struct qcom_cpucp_mbox_desc *desc;
>>   	struct device *dev = &pdev->dev;
>>   	struct qcom_cpucp_mbox *cpucp;
>>   	struct mbox_controller *mbox;
>> +	struct resource *res;
>>   	int irq, ret;
>>
>> +	desc = device_get_match_data(&pdev->dev);
>> +	if (!desc)
>> +		return -EINVAL;
>> +
>>   	cpucp = devm_kzalloc(dev, sizeof(*cpucp), GFP_KERNEL);
>>   	if (!cpucp)
>>   		return -ENOMEM;
>>
>> -	cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
>> -	if (IS_ERR(cpucp->rx_base))
>> -		return PTR_ERR(cpucp->rx_base);
>> +	cpucp->desc = desc;
>> +
>> +	if (desc->v2_mbox) {
>> +		cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
>> +		if (IS_ERR(cpucp->rx_base))
>> +			return PTR_ERR(cpucp->rx_base);
>> +	/* Legacy mailbox quirks due to shared region with EPSS register space */
> 
> Does that mean that on these platforms cpucp should be a child node of the EPSS? Also it might be a high time when the drivers should be switched to use regmaps.
> 

This will not work as mailbox tx path is not shared with EPSS.

>> +	} else {
>> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +		if (!res) {
>> +			dev_err(&pdev->dev, "Failed to get the device base address\n");
>> +			return -ENODEV;
>> +		}
>> +		cpucp->rx_base = devm_ioremap(dev, res->start, resource_size(res));
>> +		if (!cpucp->rx_base) {
>> +			dev_err(dev, "Failed to ioremap the cpucp rx irq addr\n");
>> +			return -ENOMEM;
>> +		}
>> +	}
>>
>>   	cpucp->tx_base = devm_of_iomap(dev, 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);
>> +	if (desc->v2_mbox) {
>> +		writeq(0, cpucp->rx_base + desc->enable_reg);
>> +		writeq(0, cpucp->rx_base + desc->clear_reg);
>> +		writeq(0, cpucp->rx_base + desc->map_reg);
>> +	}
> 
> If these registers are only present on V2, there is no need to have them
> in the qcom_cpucp_mbox_desc.
> 

ACK

>>
>>   	irq = platform_get_irq(pdev, 0);
>>   	if (irq < 0)
>>   		return irq;
>>
>> -	ret = devm_request_irq(dev, irq, qcom_cpucp_mbox_irq_fn,
>> -			       IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
>> +	ret = devm_request_irq(dev, irq, desc->v2_mbox ? qcom_cpucp_v2_mbox_irq_fn :
>> +		qcom_cpucp_mbox_irq_fn, IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
>>   	if (ret < 0)
>>   		return dev_err_probe(dev, ret, "Failed to register irq: %d\n", irq);
>>
>> -	writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
>> +	if (desc->v2_mbox)
>> +		writeq(APSS_CPUCP_V2_RX_MBOX_CMD_MASK, cpucp->rx_base + desc->map_reg);
>>
>>   	mbox = &cpucp->mbox;
>>   	mbox->dev = dev;
>> -	mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
>> +	mbox->num_chans = desc->num_chans;
>>   	mbox->chans = cpucp->chans;
>>   	mbox->ops = &qcom_cpucp_mbox_chan_ops;
>>
>> @@ -157,8 +223,30 @@ static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
>>   	return 0;
>>   }
>>
>> +static const struct qcom_cpucp_mbox_desc sc7280_cpucp_mbox = {
>> +	.tx_reg = 0xC,
>> +	.chan_stride = 0x1000,
>> +	.status_reg = 0x30C,
>> +	.clear_reg = 0x308,
>> +	.v2_mbox = false,
>> +	.num_chans = 2,
>> +};
>> +
>> +static const struct qcom_cpucp_mbox_desc x1e80100_cpucp_mbox = {
>> +	.rx_reg = 0x104,
>> +	.tx_reg = 0x104,
>> +	.chan_stride = 0x8,
>> +	.map_reg = 0x4000,
>> +	.status_reg = 0x4400,
>> +	.clear_reg = 0x4800,
>> +	.enable_reg = 0x4C00,
>> +	.v2_mbox = true,
>> +	.num_chans = 3,
>> +};
>> +
>>   static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
>> -	{ .compatible = "qcom,x1e80100-cpucp-mbox" },
>> +	{ .compatible = "qcom,x1e80100-cpucp-mbox", .data = &x1e80100_cpucp_mbox},
>> +	{ .compatible = "qcom,sc7280-cpucp-mbox", .data = &sc7280_cpucp_mbox},
> 
> Please keep the table sorted.
> 

ACK

Thanks,
Shivnandan

>>   	{}
>>   };
>>   MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
>> --
>> 2.25.1
>>
> 

Re: [PATCH 1/3] dt-bindings: mailbox: qcom,cpucp-mbox: Add sc7280 cpucp mailbox instance

[Shivnandan Kumar]

Thanks Krzysztof for reviewing the patch.


On 9/25/2024 7:53 PM, Krzysztof Kozlowski wrote:
> On 24/09/2024 07:09, Shivnandan Kumar wrote:
>> sc7280 has a cpucp mailbox. Document them.
>>
>> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
>> ---
>>   .../devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml         | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
>> index f7342d04beec..4a7ea072a3c1 100644
>> --- a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
>> +++ b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
>> @@ -15,8 +15,9 @@ description:
>>
>>   properties:
>>     compatible:
>> -    items:
>> -      - const: qcom,x1e80100-cpucp-mbox
>> +    enum:
>> +      - qcom,x1e80100-cpucp-mbox
>> +      - qcom,sc7280-cpucp-mbox
> 
> Keep alphabetical order.

ACK

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/3] dt-bindings: mailbox: qcom,cpucp-mbox: Add sc7280 cpucp mailbox instance

[Shivnandan Kumar]

thanks Rob for reviewing this patch.


On 9/25/2024 4:55 AM, Rob Herring wrote:
> On Tue, Sep 24, 2024 at 10:39:39AM +0530, Shivnandan Kumar wrote:
>> sc7280 has a cpucp mailbox. Document them.
> 
> And is different from the existing device how?

It is different with respect to the register placement.

Thanks,
Shivnandan

> 
>>
>> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
>> ---
>>   .../devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml         | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
>> index f7342d04beec..4a7ea072a3c1 100644
>> --- a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
>> +++ b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
>> @@ -15,8 +15,9 @@ description:
>>
>>   properties:
>>     compatible:
>> -    items:
>> -      - const: qcom,x1e80100-cpucp-mbox
>> +    enum:
>> +      - qcom,x1e80100-cpucp-mbox
>> +      - qcom,sc7280-cpucp-mbox
>>
>>     reg:
>>       items:
>> --
>> 2.25.1
>>

Re: [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller

[Bjorn Andersson]

On Tue, Sep 24, 2024 at 10:39:40AM GMT, Shivnandan Kumar wrote:
> The SC7280 CPUCP mailbox controller is compatible with legacy mailbox
> hardware.

"mailbox hardware" is a very vague description of something.

> Implement support for this functionality which enable HLOS to
> CPUCP communication.
> 

Please describe the problem that this solves. What "legacy mailbox
hardware"? Why do you want to talk to the CPUCP?  What is a HLOS? What
is the CPUCP?

It seems from the patch that the current implementation supports
something we call "version 2" of the cpucp mailbox interface and you're
adding support for v1. Please make sure that the commit message describe
such things.

> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
> ---
>  drivers/mailbox/qcom-cpucp-mbox.c | 156 +++++++++++++++++++++++-------
>  1 file changed, 122 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
> index e5437c294803..faae6e069ea1 100644
> --- a/drivers/mailbox/qcom-cpucp-mbox.c
> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
> @@ -13,18 +13,24 @@
>  #include <linux/platform_device.h>
> 
>  #define APSS_CPUCP_IPC_CHAN_SUPPORTED		3
> -#define APSS_CPUCP_MBOX_CMD_OFF			0x4
> -
> -/* Tx Registers */
> -#define APSS_CPUCP_TX_MBOX_CMD(i)		(0x100 + ((i) * 8))
> 
>  /* Rx Registers */
> -#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		GENMASK_ULL(63, 0)
> +#define APSS_CPUCP_V2_RX_MBOX_CMD_MASK		GENMASK_ULL(63, 0)
> +#define APSS_CPUCP_V1_SEND_IRQ_VAL		BIT(28)
> +#define APSS_CPUCP_V1_CLEAR_IRQ_VAL		BIT(3)
> +#define APSS_CPUCP_V1_STATUS_IRQ_VAL		BIT(3)
> +
> +struct qcom_cpucp_mbox_desc {
> +	u32 enable_reg;

Do you really need these parameters to be dynamic? E.g. you only touch
enable_reg from the v2 code paths...

> +	u32 map_reg;
> +	u32 rx_reg;
> +	u32 tx_reg;
> +	u32 status_reg;
> +	u32 clear_reg;
> +	u32 chan_stride;

"u32" tells me that this has to be 32 bits, e.g. because the value is
going into a register... But these are just offsets...

Please use "unsigned int" to denote "a natural number".

> +	bool v2_mbox;

How about "version" and give it a value 1 or 2?

> +	u32 num_chans;
> +};
> 
>  /**
>   * struct qcom_cpucp_mbox - Holder for the mailbox driver
> @@ -35,6 +41,7 @@
>   */
>  struct qcom_cpucp_mbox {
>  	struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
> +	const struct qcom_cpucp_mbox_desc *desc;
>  	struct mbox_controller mbox;
>  	void __iomem *tx_base;
>  	void __iomem *rx_base;
> @@ -48,13 +55,40 @@ static inline int channel_number(struct mbox_chan *chan)
>  static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)

Why is the existing function renamed "v2" and this newly introduced
function not given a version?

>  {
>  	struct qcom_cpucp_mbox *cpucp = data;
> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
> +	int i;
> +
> +	for (i = 0; i < desc->num_chans; i++) {
> +		u32 val = readl(cpucp->rx_base + desc->status_reg + (i * desc->chan_stride));
> +		struct mbox_chan *chan = &cpucp->chans[i];
> +		unsigned long flags;
> +
> +		if (val & APSS_CPUCP_V1_STATUS_IRQ_VAL) {
> +			writel(APSS_CPUCP_V1_CLEAR_IRQ_VAL,
> +			       cpucp->rx_base + desc->clear_reg + (i * desc->chan_stride));
> +			/* Make sure reg write is complete before proceeding */
> +			mb();
> +			spin_lock_irqsave(&chan->lock, flags);
> +			if (chan->cl)
> +				mbox_chan_received_data(chan, NULL);
> +			spin_unlock_irqrestore(&chan->lock, flags);
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t qcom_cpucp_v2_mbox_irq_fn(int irq, void *data)
> +{
> +	struct qcom_cpucp_mbox *cpucp = data;
> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>  	u64 status;
>  	int i;
> 
> -	status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
> +	status = readq(cpucp->rx_base + desc->status_reg);
> 
> -	for_each_set_bit(i, (unsigned long *)&status, APSS_CPUCP_IPC_CHAN_SUPPORTED) {
> -		u32 val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
> +	for_each_set_bit(i, (unsigned long *)&status, desc->num_chans) {
> +		u32 val = readl(cpucp->rx_base + desc->rx_reg + (i * desc->chan_stride));
>  		struct mbox_chan *chan = &cpucp->chans[i];
>  		unsigned long flags;
> 
> @@ -62,7 +96,7 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
>  		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);
> +		writeq(BIT(i), cpucp->rx_base + desc->clear_reg);
>  		spin_unlock_irqrestore(&chan->lock, flags);
>  	}
> 
> @@ -72,12 +106,15 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
>  static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
>  {
>  	struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>  	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);
> +	if (desc->v2_mbox) {
> +		val = readq(cpucp->rx_base + desc->enable_reg);
> +		val |= BIT(chan_id);
> +		writeq(val, cpucp->rx_base + desc->enable_reg);
> +	}

No equivalent in "legacy"?
> 
>  	return 0;
>  }
> @@ -85,22 +122,26 @@ static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
>  static void qcom_cpucp_mbox_shutdown(struct mbox_chan *chan)
>  {
>  	struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>  	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);
> +	if (desc->v2_mbox) {
> +		val = readq(cpucp->rx_base + desc->enable_reg);
> +		val &= ~BIT(chan_id);
> +		writeq(val, cpucp->rx_base + desc->enable_reg);
> +	}

Ditto

>  }
> 
>  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);
> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
> +	u32 val = desc->v2_mbox ? *(u32 *)data : APSS_CPUCP_V1_SEND_IRQ_VAL;

Please rewrite this without ternary operators.

>  	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);
> +	u32 offset = desc->v2_mbox ? (chan_id * desc->chan_stride) : 0;
> 
> +	writel(val, cpucp->tx_base + desc->tx_reg + offset);
>  	return 0;
>  }
> 
> @@ -112,41 +153,66 @@ static const struct mbox_chan_ops qcom_cpucp_mbox_chan_ops = {
> 
>  static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
>  {
> +	const struct qcom_cpucp_mbox_desc *desc;
>  	struct device *dev = &pdev->dev;
>  	struct qcom_cpucp_mbox *cpucp;
>  	struct mbox_controller *mbox;
> +	struct resource *res;
>  	int irq, ret;
> 
> +	desc = device_get_match_data(&pdev->dev);
> +	if (!desc)
> +		return -EINVAL;
> +
>  	cpucp = devm_kzalloc(dev, sizeof(*cpucp), GFP_KERNEL);
>  	if (!cpucp)
>  		return -ENOMEM;
> 
> -	cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
> -	if (IS_ERR(cpucp->rx_base))
> -		return PTR_ERR(cpucp->rx_base);
> +	cpucp->desc = desc;
> +
> +	if (desc->v2_mbox) {
> +		cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
> +		if (IS_ERR(cpucp->rx_base))
> +			return PTR_ERR(cpucp->rx_base);
> +	/* Legacy mailbox quirks due to shared region with EPSS register space */

Why can't we have the same code in both cases?

> +	} else {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (!res) {
> +			dev_err(&pdev->dev, "Failed to get the device base address\n");

It's not only base address.

> +			return -ENODEV;
> +		}
> +		cpucp->rx_base = devm_ioremap(dev, res->start, resource_size(res));
> +		if (!cpucp->rx_base) {
> +			dev_err(dev, "Failed to ioremap the cpucp rx irq addr\n");
> +			return -ENOMEM;
> +		}
> +	}
> 
>  	cpucp->tx_base = devm_of_iomap(dev, 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);
> +	if (desc->v2_mbox) {
> +		writeq(0, cpucp->rx_base + desc->enable_reg);
> +		writeq(0, cpucp->rx_base + desc->clear_reg);
> +		writeq(0, cpucp->rx_base + desc->map_reg);


Is there a reason why the legacy system does not need or want to clear
these?

> +	}
> 
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
>  		return irq;
> 
> -	ret = devm_request_irq(dev, irq, qcom_cpucp_mbox_irq_fn,
> -			       IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
> +	ret = devm_request_irq(dev, irq, desc->v2_mbox ? qcom_cpucp_v2_mbox_irq_fn :
> +		qcom_cpucp_mbox_irq_fn, IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);

The use of a ternary operator, in combination with odd line wrapping
makes this completely unreadable. Please fix.

>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to register irq: %d\n", irq);
> 
> -	writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
> +	if (desc->v2_mbox)
> +		writeq(APSS_CPUCP_V2_RX_MBOX_CMD_MASK, cpucp->rx_base + desc->map_reg);
> 
>  	mbox = &cpucp->mbox;
>  	mbox->dev = dev;
> -	mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
> +	mbox->num_chans = desc->num_chans;
>  	mbox->chans = cpucp->chans;
>  	mbox->ops = &qcom_cpucp_mbox_chan_ops;
> 
> @@ -157,8 +223,30 @@ static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
>  	return 0;
>  }
> 
> +static const struct qcom_cpucp_mbox_desc sc7280_cpucp_mbox = {
> +	.tx_reg = 0xC,
> +	.chan_stride = 0x1000,
> +	.status_reg = 0x30C,

Lowercase hex digits please (although the question above whether these
needs to be defined remains).

> +	.clear_reg = 0x308,
> +	.v2_mbox = false,
> +	.num_chans = 2,
> +};
> +
> +static const struct qcom_cpucp_mbox_desc x1e80100_cpucp_mbox = {
> +	.rx_reg = 0x104,
> +	.tx_reg = 0x104,
> +	.chan_stride = 0x8,
> +	.map_reg = 0x4000,
> +	.status_reg = 0x4400,
> +	.clear_reg = 0x4800,
> +	.enable_reg = 0x4C00,
> +	.v2_mbox = true,
> +	.num_chans = 3,
> +};
> +
>  static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
> -	{ .compatible = "qcom,x1e80100-cpucp-mbox" },
> +	{ .compatible = "qcom,x1e80100-cpucp-mbox", .data = &x1e80100_cpucp_mbox},
> +	{ .compatible = "qcom,sc7280-cpucp-mbox", .data = &sc7280_cpucp_mbox},

Perhaps I'm missing something, but seems like the only information you
actually need to pass here is 1 or 2, to denote which version/code paths
you should take through the driver.

Regards,
Bjorn

>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
> --
> 2.25.1
> 

Re: [PATCH 3/3] arm64: dts: qcom: sc7280: Add cpucp mbox node

[Bjorn Andersson]

On Tue, Sep 24, 2024 at 10:39:41AM GMT, Shivnandan Kumar wrote:
> Add the CPUCP mailbox node required for communication with CPUCP.

I'd like to see a description of why that's useful...

But perhaps more importantly, why are there no user(s) of this?

Regards,
Bjorn

> 
> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 3d8410683402..4b9b26a75c62 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -4009,6 +4009,14 @@ gem_noc: interconnect@9100000 {
>  			qcom,bcm-voters = <&apps_bcm_voter>;
>  		};
> 
> +		cpucp_mbox: mailbox@17430000 {
> +			compatible = "qcom,sc7280-cpucp-mbox";
> +			reg = <0 0x18590000 0 0x2000>,
> +			      <0 0x17C00000 0 0x10>;
> +			interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
> +			#mbox-cells = <1>;
> +		};
> +
>  		system-cache-controller@9200000 {
>  			compatible = "qcom,sc7280-llcc";
>  			reg = <0 0x09200000 0 0x58000>, <0 0x09280000 0 0x58000>,
> --
> 2.25.1
> 

Re: [PATCH 1/3] dt-bindings: mailbox: qcom,cpucp-mbox: Add sc7280 cpucp mailbox instance

[Dmitry Baryshkov]

On Thu, Oct 03, 2024 at 11:13:02AM GMT, Shivnandan Kumar wrote:
> thanks Rob for reviewing this patch.
> 
> 
> On 9/25/2024 4:55 AM, Rob Herring wrote:
> > On Tue, Sep 24, 2024 at 10:39:39AM +0530, Shivnandan Kumar wrote:
> > > sc7280 has a cpucp mailbox. Document them.
> > 
> > And is different from the existing device how?
> 
> It is different with respect to the register placement.

Register placement in the global map or the internal register structure?

> 
> Thanks,
> Shivnandan
> 
> > 
> > > 
> > > Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
> > > ---
> > >   .../devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml         | 5 +++--
> > >   1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
> > > index f7342d04beec..4a7ea072a3c1 100644
> > > --- a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
> > > +++ b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
> > > @@ -15,8 +15,9 @@ description:
> > > 
> > >   properties:
> > >     compatible:
> > > -    items:
> > > -      - const: qcom,x1e80100-cpucp-mbox
> > > +    enum:
> > > +      - qcom,x1e80100-cpucp-mbox
> > > +      - qcom,sc7280-cpucp-mbox
> > > 
> > >     reg:
> > >       items:
> > > --
> > > 2.25.1
> > > 

-- 
With best wishes
Dmitry

Re: [PATCH 1/3] dt-bindings: mailbox: qcom,cpucp-mbox: Add sc7280 cpucp mailbox instance

[Shivnandan Kumar]

Thanks Dmitry for reviewing the patch

On 10/6/2024 10:41 PM, Dmitry Baryshkov wrote:
> On Thu, Oct 03, 2024 at 11:13:02AM GMT, Shivnandan Kumar wrote:
>> thanks Rob for reviewing this patch.
>>
>>
>> On 9/25/2024 4:55 AM, Rob Herring wrote:
>>> On Tue, Sep 24, 2024 at 10:39:39AM +0530, Shivnandan Kumar wrote:
>>>> sc7280 has a cpucp mailbox. Document them.
>>>
>>> And is different from the existing device how?
>>
>> It is different with respect to the register placement.
> 
> Register placement in the global map or the internal register structure?

the register placement varies both internally and globally as well.

> 
>>
>> Thanks,
>> Shivnandan
>>
>>>
>>>>
>>>> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
>>>> ---
>>>>    .../devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml         | 5 +++--
>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
>>>> index f7342d04beec..4a7ea072a3c1 100644
>>>> --- a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
>>>> +++ b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
>>>> @@ -15,8 +15,9 @@ description:
>>>>
>>>>    properties:
>>>>      compatible:
>>>> -    items:
>>>> -      - const: qcom,x1e80100-cpucp-mbox
>>>> +    enum:
>>>> +      - qcom,x1e80100-cpucp-mbox
>>>> +      - qcom,sc7280-cpucp-mbox
>>>>
>>>>      reg:
>>>>        items:
>>>> --
>>>> 2.25.1
>>>>
> 

Re: [PATCH 3/3] arm64: dts: qcom: sc7280: Add cpucp mbox node

[Shivnandan Kumar]

On 10/6/2024 8:05 AM, Bjorn Andersson wrote:
> On Tue, Sep 24, 2024 at 10:39:41AM GMT, Shivnandan Kumar wrote:
>> Add the CPUCP mailbox node required for communication with CPUCP.
> 
> I'd like to see a description of why that's useful...
> 

I will add in next patch set.

> But perhaps more importantly, why are there no user(s) of this?
> 

We will later add features such as BUS DCVS (memlat algorithm in CPUCP) 
and CPUCP logging based on this series.

> Regards,
> Bjorn
> 


>>
>> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sc7280.dtsi | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index 3d8410683402..4b9b26a75c62 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -4009,6 +4009,14 @@ gem_noc: interconnect@9100000 {
>>   			qcom,bcm-voters = <&apps_bcm_voter>;
>>   		};
>>
>> +		cpucp_mbox: mailbox@17430000 {
>> +			compatible = "qcom,sc7280-cpucp-mbox";
>> +			reg = <0 0x18590000 0 0x2000>,
>> +			      <0 0x17C00000 0 0x10>;
>> +			interrupts = <GIC_SPI 62 IRQ_TYPE_LEVEL_HIGH>;
>> +			#mbox-cells = <1>;
>> +		};
>> +
>>   		system-cache-controller@9200000 {
>>   			compatible = "qcom,sc7280-llcc";
>>   			reg = <0 0x09200000 0 0x58000>, <0 0x09280000 0 0x58000>,
>> --
>> 2.25.1
>>

Re: [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller

[Shivnandan Kumar]

Thank you, Bjorn, for taking the time to review this patch series.


On 10/6/2024 8:03 AM, Bjorn Andersson wrote:
> On Tue, Sep 24, 2024 at 10:39:40AM GMT, Shivnandan Kumar wrote:
>> The SC7280 CPUCP mailbox controller is compatible with legacy mailbox
>> hardware.
> 
> "mailbox hardware" is a very vague description of something.
> 
>> Implement support for this functionality which enable HLOS to
>> CPUCP communication.
>>
> 
> Please describe the problem that this solves. What "legacy mailbox
> hardware"? Why do you want to talk to the CPUCP?  What is a HLOS? What
> is the CPUCP?
> 

ACK, I will add description in next patch series.

> It seems from the patch that the current implementation supports
> something we call "version 2" of the cpucp mailbox interface and you're
> adding support for v1. Please make sure that the commit message describe
> such things.
> 

ACK

>> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
>> ---
>>   drivers/mailbox/qcom-cpucp-mbox.c | 156 +++++++++++++++++++++++-------
>>   1 file changed, 122 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/mailbox/qcom-cpucp-mbox.c b/drivers/mailbox/qcom-cpucp-mbox.c
>> index e5437c294803..faae6e069ea1 100644
>> --- a/drivers/mailbox/qcom-cpucp-mbox.c
>> +++ b/drivers/mailbox/qcom-cpucp-mbox.c
>> @@ -13,18 +13,24 @@
>>   #include <linux/platform_device.h>
>>
>>   #define APSS_CPUCP_IPC_CHAN_SUPPORTED		3
>> -#define APSS_CPUCP_MBOX_CMD_OFF			0x4
>> -
>> -/* Tx Registers */
>> -#define APSS_CPUCP_TX_MBOX_CMD(i)		(0x100 + ((i) * 8))
>>
>>   /* Rx Registers */
>> -#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		GENMASK_ULL(63, 0)
>> +#define APSS_CPUCP_V2_RX_MBOX_CMD_MASK		GENMASK_ULL(63, 0)
>> +#define APSS_CPUCP_V1_SEND_IRQ_VAL		BIT(28)
>> +#define APSS_CPUCP_V1_CLEAR_IRQ_VAL		BIT(3)
>> +#define APSS_CPUCP_V1_STATUS_IRQ_VAL		BIT(3)
>> +
>> +struct qcom_cpucp_mbox_desc {
>> +	u32 enable_reg;
> 
> Do you really need these parameters to be dynamic? E.g. you only touch
> enable_reg from the v2 code paths...
> 

Will remove this in next patch series.

>> +	u32 map_reg;
>> +	u32 rx_reg;
>> +	u32 tx_reg;
>> +	u32 status_reg;
>> +	u32 clear_reg;
>> +	u32 chan_stride;
> 
> "u32" tells me that this has to be 32 bits, e.g. because the value is
> going into a register... But these are just offsets...
> 
> Please use "unsigned int" to denote "a natural number".
> 

ACK

>> +	bool v2_mbox;
> 
> How about "version" and give it a value 1 or 2?
> 

Ok, will do like that.

>> +	u32 num_chans;
>> +};
>>
>>   /**
>>    * struct qcom_cpucp_mbox - Holder for the mailbox driver
>> @@ -35,6 +41,7 @@
>>    */
>>   struct qcom_cpucp_mbox {
>>   	struct mbox_chan chans[APSS_CPUCP_IPC_CHAN_SUPPORTED];
>> +	const struct qcom_cpucp_mbox_desc *desc;
>>   	struct mbox_controller mbox;
>>   	void __iomem *tx_base;
>>   	void __iomem *rx_base;
>> @@ -48,13 +55,40 @@ static inline int channel_number(struct mbox_chan *chan)
>>   static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
> 
> Why is the existing function renamed "v2" and this newly introduced
> function not given a version?
> 

ACK

>>   {
>>   	struct qcom_cpucp_mbox *cpucp = data;
>> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>> +	int i;
>> +
>> +	for (i = 0; i < desc->num_chans; i++) {
>> +		u32 val = readl(cpucp->rx_base + desc->status_reg + (i * desc->chan_stride));
>> +		struct mbox_chan *chan = &cpucp->chans[i];
>> +		unsigned long flags;
>> +
>> +		if (val & APSS_CPUCP_V1_STATUS_IRQ_VAL) {
>> +			writel(APSS_CPUCP_V1_CLEAR_IRQ_VAL,
>> +			       cpucp->rx_base + desc->clear_reg + (i * desc->chan_stride));
>> +			/* Make sure reg write is complete before proceeding */
>> +			mb();
>> +			spin_lock_irqsave(&chan->lock, flags);
>> +			if (chan->cl)
>> +				mbox_chan_received_data(chan, NULL);
>> +			spin_unlock_irqrestore(&chan->lock, flags);
>> +		}
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t qcom_cpucp_v2_mbox_irq_fn(int irq, void *data)
>> +{
>> +	struct qcom_cpucp_mbox *cpucp = data;
>> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>>   	u64 status;
>>   	int i;
>>
>> -	status = readq(cpucp->rx_base + APSS_CPUCP_RX_MBOX_STAT);
>> +	status = readq(cpucp->rx_base + desc->status_reg);
>>
>> -	for_each_set_bit(i, (unsigned long *)&status, APSS_CPUCP_IPC_CHAN_SUPPORTED) {
>> -		u32 val = readl(cpucp->rx_base + APSS_CPUCP_RX_MBOX_CMD(i) + APSS_CPUCP_MBOX_CMD_OFF);
>> +	for_each_set_bit(i, (unsigned long *)&status, desc->num_chans) {
>> +		u32 val = readl(cpucp->rx_base + desc->rx_reg + (i * desc->chan_stride));
>>   		struct mbox_chan *chan = &cpucp->chans[i];
>>   		unsigned long flags;
>>
>> @@ -62,7 +96,7 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
>>   		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);
>> +		writeq(BIT(i), cpucp->rx_base + desc->clear_reg);
>>   		spin_unlock_irqrestore(&chan->lock, flags);
>>   	}
>>
>> @@ -72,12 +106,15 @@ static irqreturn_t qcom_cpucp_mbox_irq_fn(int irq, void *data)
>>   static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
>>   {
>>   	struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
>> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>>   	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);
>> +	if (desc->v2_mbox) {
>> +		val = readq(cpucp->rx_base + desc->enable_reg);
>> +		val |= BIT(chan_id);
>> +		writeq(val, cpucp->rx_base + desc->enable_reg);
>> +	}
> 
> No equivalent in "legacy"?

Yes, right

>>
>>   	return 0;
>>   }
>> @@ -85,22 +122,26 @@ static int qcom_cpucp_mbox_startup(struct mbox_chan *chan)
>>   static void qcom_cpucp_mbox_shutdown(struct mbox_chan *chan)
>>   {
>>   	struct qcom_cpucp_mbox *cpucp = container_of(chan->mbox, struct qcom_cpucp_mbox, mbox);
>> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>>   	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);
>> +	if (desc->v2_mbox) {
>> +		val = readq(cpucp->rx_base + desc->enable_reg);
>> +		val &= ~BIT(chan_id);
>> +		writeq(val, cpucp->rx_base + desc->enable_reg);
>> +	}
> 
> Ditto
> 

We do not have equivalent in "legacy".

>>   }
>>
>>   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);
>> +	const struct qcom_cpucp_mbox_desc *desc = cpucp->desc;
>> +	u32 val = desc->v2_mbox ? *(u32 *)data : APSS_CPUCP_V1_SEND_IRQ_VAL;
> 
> Please rewrite this without ternary operators.
> 

ACK

>>   	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);
>> +	u32 offset = desc->v2_mbox ? (chan_id * desc->chan_stride) : 0;
>>
>> +	writel(val, cpucp->tx_base + desc->tx_reg + offset);
>>   	return 0;
>>   }
>>
>> @@ -112,41 +153,66 @@ static const struct mbox_chan_ops qcom_cpucp_mbox_chan_ops = {
>>
>>   static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
>>   {
>> +	const struct qcom_cpucp_mbox_desc *desc;
>>   	struct device *dev = &pdev->dev;
>>   	struct qcom_cpucp_mbox *cpucp;
>>   	struct mbox_controller *mbox;
>> +	struct resource *res;
>>   	int irq, ret;
>>
>> +	desc = device_get_match_data(&pdev->dev);
>> +	if (!desc)
>> +		return -EINVAL;
>> +
>>   	cpucp = devm_kzalloc(dev, sizeof(*cpucp), GFP_KERNEL);
>>   	if (!cpucp)
>>   		return -ENOMEM;
>>
>> -	cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
>> -	if (IS_ERR(cpucp->rx_base))
>> -		return PTR_ERR(cpucp->rx_base);
>> +	cpucp->desc = desc;
>> +
>> +	if (desc->v2_mbox) {
>> +		cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
>> +		if (IS_ERR(cpucp->rx_base))
>> +			return PTR_ERR(cpucp->rx_base);
>> +	/* Legacy mailbox quirks due to shared region with EPSS register space */
> 
> Why can't we have the same code in both cases?
> 


RX address space share region with EPSS. Due to which devm_of_iomap 
returns -EBUSY.

>> +	} else {
>> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +		if (!res) {
>> +			dev_err(&pdev->dev, "Failed to get the device base address\n");
> 
> It's not only base address.
> 

Will add appropriate print statement.

>> +			return -ENODEV;
>> +		}
>> +		cpucp->rx_base = devm_ioremap(dev, res->start, resource_size(res));
>> +		if (!cpucp->rx_base) {
>> +			dev_err(dev, "Failed to ioremap the cpucp rx irq addr\n");
>> +			return -ENOMEM;
>> +		}
>> +	}
>>
>>   	cpucp->tx_base = devm_of_iomap(dev, 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);
>> +	if (desc->v2_mbox) {
>> +		writeq(0, cpucp->rx_base + desc->enable_reg);
>> +		writeq(0, cpucp->rx_base + desc->clear_reg);
>> +		writeq(0, cpucp->rx_base + desc->map_reg);
> 
> 
> Is there a reason why the legacy system does not need or want to clear
> these?
> 

Legacy system does not have equivalent registers.

>> +	}
>>
>>   	irq = platform_get_irq(pdev, 0);
>>   	if (irq < 0)
>>   		return irq;
>>
>> -	ret = devm_request_irq(dev, irq, qcom_cpucp_mbox_irq_fn,
>> -			       IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
>> +	ret = devm_request_irq(dev, irq, desc->v2_mbox ? qcom_cpucp_v2_mbox_irq_fn :
>> +		qcom_cpucp_mbox_irq_fn, IRQF_TRIGGER_HIGH, "apss_cpucp_mbox", cpucp);
> 
> The use of a ternary operator, in combination with odd line wrapping
> makes this completely unreadable. Please fix.
> 

ACK

>>   	if (ret < 0)
>>   		return dev_err_probe(dev, ret, "Failed to register irq: %d\n", irq);
>>
>> -	writeq(APSS_CPUCP_RX_MBOX_CMD_MASK, cpucp->rx_base + APSS_CPUCP_RX_MBOX_MAP);
>> +	if (desc->v2_mbox)
>> +		writeq(APSS_CPUCP_V2_RX_MBOX_CMD_MASK, cpucp->rx_base + desc->map_reg);
>>
>>   	mbox = &cpucp->mbox;
>>   	mbox->dev = dev;
>> -	mbox->num_chans = APSS_CPUCP_IPC_CHAN_SUPPORTED;
>> +	mbox->num_chans = desc->num_chans;
>>   	mbox->chans = cpucp->chans;
>>   	mbox->ops = &qcom_cpucp_mbox_chan_ops;
>>
>> @@ -157,8 +223,30 @@ static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
>>   	return 0;
>>   }
>>
>> +static const struct qcom_cpucp_mbox_desc sc7280_cpucp_mbox = {
>> +	.tx_reg = 0xC,
>> +	.chan_stride = 0x1000,
>> +	.status_reg = 0x30C,
> 
> Lowercase hex digits please (although the question above whether these
> needs to be defined remains).

ACK

> 
>> +	.clear_reg = 0x308,
>> +	.v2_mbox = false,
>> +	.num_chans = 2,
>> +};
>> +
>> +static const struct qcom_cpucp_mbox_desc x1e80100_cpucp_mbox = {
>> +	.rx_reg = 0x104,
>> +	.tx_reg = 0x104,
>> +	.chan_stride = 0x8,
>> +	.map_reg = 0x4000,
>> +	.status_reg = 0x4400,
>> +	.clear_reg = 0x4800,
>> +	.enable_reg = 0x4C00,
>> +	.v2_mbox = true,
>> +	.num_chans = 3,
>> +};
>> +
>>   static const struct of_device_id qcom_cpucp_mbox_of_match[] = {
>> -	{ .compatible = "qcom,x1e80100-cpucp-mbox" },
>> +	{ .compatible = "qcom,x1e80100-cpucp-mbox", .data = &x1e80100_cpucp_mbox},
>> +	{ .compatible = "qcom,sc7280-cpucp-mbox", .data = &sc7280_cpucp_mbox},
> 
> Perhaps I'm missing something, but seems like the only information you
> actually need to pass here is 1 or 2, to denote which version/code paths
> you should take through the driver.
> 

okay, I will rename sc7280_cpucp_mbox and x1e80100_cpucp_mbox structures 
as v1 and v2 respectively.

> Regards,
> Bjorn
> 
>>   	{}
>>   };
>>   MODULE_DEVICE_TABLE(of, qcom_cpucp_mbox_of_match);
>> --
>> 2.25.1
>>

Re: [PATCH 3/3] arm64: dts: qcom: sc7280: Add cpucp mbox node

[Konrad Dybcio]

On 17.10.2024 1:51 PM, Shivnandan Kumar wrote:
> 
> 
> On 10/6/2024 8:05 AM, Bjorn Andersson wrote:
>> On Tue, Sep 24, 2024 at 10:39:41AM GMT, Shivnandan Kumar wrote:
>>> Add the CPUCP mailbox node required for communication with CPUCP.
>>
>> I'd like to see a description of why that's useful...
>>
> 
> I will add in next patch set.
> 
>> But perhaps more importantly, why are there no user(s) of this?
>>
> 
> We will later add features such as BUS DCVS (memlat algorithm in CPUCP) and CPUCP logging based on this series.

I think Bjorn's question here is also "what kind of boards is this going
to be useful on", especially given 7280 was more or less released in
basically all firmware flavors that we make..

Konrad

Re: [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller

[Konrad Dybcio]

On 24.09.2024 7:09 AM, Shivnandan Kumar wrote:
> The SC7280 CPUCP mailbox controller is compatible with legacy mailbox
> hardware.
> Implement support for this functionality which enable HLOS to
> CPUCP communication.
> 
> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
> ---

If you need to if-out everything that isn't Linux boilerplate, I don't
think this driver is the correct plate to describe the 7280 mailbox

Konrad

Re: [PATCH 1/3] dt-bindings: mailbox: qcom,cpucp-mbox: Add sc7280 cpucp mailbox instance

[Dmitry Baryshkov]

On Thu, Oct 17, 2024 at 10:48:32AM +0530, Shivnandan Kumar wrote:
> Thanks Dmitry for reviewing the patch
> 
> On 10/6/2024 10:41 PM, Dmitry Baryshkov wrote:
> > On Thu, Oct 03, 2024 at 11:13:02AM GMT, Shivnandan Kumar wrote:
> > > thanks Rob for reviewing this patch.
> > > 
> > > 
> > > On 9/25/2024 4:55 AM, Rob Herring wrote:
> > > > On Tue, Sep 24, 2024 at 10:39:39AM +0530, Shivnandan Kumar wrote:
> > > > > sc7280 has a cpucp mailbox. Document them.
> > > > 
> > > > And is different from the existing device how?
> > > 
> > > It is different with respect to the register placement.
> > 
> > Register placement in the global map or the internal register structure?
> 
> the register placement varies both internally and globally as well.

Please mention in the commit message that internal regiter map is
different.

> 
> > 
> > > 
> > > Thanks,
> > > Shivnandan
> > > 
> > > > 
> > > > > 
> > > > > Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com>
> > > > > ---
> > > > >    .../devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml         | 5 +++--
> > > > >    1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
> > > > > index f7342d04beec..4a7ea072a3c1 100644
> > > > > --- a/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
> > > > > +++ b/Documentation/devicetree/bindings/mailbox/qcom,cpucp-mbox.yaml
> > > > > @@ -15,8 +15,9 @@ description:
> > > > > 
> > > > >    properties:
> > > > >      compatible:
> > > > > -    items:
> > > > > -      - const: qcom,x1e80100-cpucp-mbox
> > > > > +    enum:
> > > > > +      - qcom,x1e80100-cpucp-mbox
> > > > > +      - qcom,sc7280-cpucp-mbox
> > > > > 
> > > > >      reg:
> > > > >        items:
> > > > > --
> > > > > 2.25.1
> > > > > 
> > 

-- 
With best wishes
Dmitry

Re: [PATCH 2/3] mailbox: qcom-cpucp-mbox: Add support for SC7280 CPUCP mailbox controller

[Bjorn Andersson]

On Thu, Oct 17, 2024 at 05:22:36PM +0530, Shivnandan Kumar wrote:
> On 10/6/2024 8:03 AM, Bjorn Andersson wrote:
> > On Tue, Sep 24, 2024 at 10:39:40AM GMT, Shivnandan Kumar wrote:
[..]
> > >   static int qcom_cpucp_mbox_probe(struct platform_device *pdev)
> > >   {
> > > +	const struct qcom_cpucp_mbox_desc *desc;
> > >   	struct device *dev = &pdev->dev;
> > >   	struct qcom_cpucp_mbox *cpucp;
> > >   	struct mbox_controller *mbox;
> > > +	struct resource *res;
> > >   	int irq, ret;
> > > 
> > > +	desc = device_get_match_data(&pdev->dev);
> > > +	if (!desc)
> > > +		return -EINVAL;
> > > +
> > >   	cpucp = devm_kzalloc(dev, sizeof(*cpucp), GFP_KERNEL);
> > >   	if (!cpucp)
> > >   		return -ENOMEM;
> > > 
> > > -	cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
> > > -	if (IS_ERR(cpucp->rx_base))
> > > -		return PTR_ERR(cpucp->rx_base);
> > > +	cpucp->desc = desc;
> > > +
> > > +	if (desc->v2_mbox) {
> > > +		cpucp->rx_base = devm_of_iomap(dev, dev->of_node, 0, NULL);
> > > +		if (IS_ERR(cpucp->rx_base))
> > > +			return PTR_ERR(cpucp->rx_base);
> > > +	/* Legacy mailbox quirks due to shared region with EPSS register space */
> > 
> > Why can't we have the same code in both cases?
> > 
> 
> 
> RX address space share region with EPSS. Due to which devm_of_iomap returns
> -EBUSY.
> 

I assumed that was the case, and that explains why the legacy system
needs a different code path.

But, couldn't you use the same for the v2 solution, so we avoid having
two different code paths?

Regards,
Bjorn