[PATCH v4 0/3] dmaengine: arm-dma350: support combined IRQ topology

[PATCH v4 0/3] dmaengine: arm-dma350: support combined IRQ topology

[Jun Guo]

DMA-350 can be integrated with either one interrupt per channel or a
single combined interrupt for all channels. This series adds support
for the combined IRQ topology while keeping compatibility with the
per-channel topology.

Patch 1 updates the DT binding to describe both interrupt topologies
(1 combined IRQ or 8 per-channel IRQs) and keeps "arm,dma-350" as the
generic compatible, with optional SoC-specific fallback compatible.

Patch 2 updates the driver to detect IRQ topology at runtime using
platform_irq_count(), handles both modes in one code path, and enables
DMANSECCTRL.INTREN_ANYCHINTR only when combined IRQ mode is used.

Patch 3 adds the Sky1 DMA DT node using the combined IRQ topology.

Tested on CIX SKY1 with dmatest:
  % echo 2000 > /sys/module/dmatest/parameters/timeout
  % echo 1 > /sys/module/dmatest/parameters/iterations
  % echo "" > /sys/module/dmatest/parameters/channel
  % echo 1 > /sys/module/dmatest/parameters/run

Changes in v4:
- Reword binding text to align with kernel style.
- Revise the AI attribution to the standard format.
- Remove redundant links from the commit log.

Changes in v3:
- Rework binding compatible description to match generic-first model.
- Keep interrupts schema support for both 1-IRQ and 8-IRQ topologies.
- Drop SoC match-data dependency for IRQ mode selection.
- Detect IRQ topology via platform_irq_count() in probe path.
- Refactor IRQ handling into a shared channel handler.
- Enable DMANSECCTRL.INTREN_ANYCHINTR only in combined IRQ mode.

Changes in v2:
- Update to kernel standards, enhance patch description, and refactor
 driver to use match data for hardware differentiation instead of
 compatible strings.

Jun Guo (3):
  dt-bindings: dma: arm-dma350: document generic and combined IRQ
    topologies
  dma: arm-dma350: support combined IRQ mode with runtime IRQ topology
    detection
  arm64: dts: cix: add DT nodes for DMA

 .../devicetree/bindings/dma/arm,dma-350.yaml  |  34 ++--
 arch/arm64/boot/dts/cix/sky1.dtsi             |   7 +
 drivers/dma/arm-dma350.c                      | 165 +++++++++++++++---
 3 files changed, 170 insertions(+), 36 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] dt-bindings: dma: arm-dma350: document generic and combined IRQ topologies

[Jun Guo]

Update the DMA-350 DT binding to match the current driver behavior.

Allow both:
- "arm,dma-350" as the generic compatible, and
- "cix,sky1-dma-350", "arm,dma-350" for SoC-specific fallback usage.

Also document interrupt topology variants supported by hardware
integration:
- one combined interrupt for all channels, or
- one interrupt per channel (up to 8 channels).

Assisted-by: Cursor: GPT-5.3-Codex
Signed-off-by: Jun Guo <jun.guo@cixtech.com>
---
 .../devicetree/bindings/dma/arm,dma-350.yaml  | 34 +++++++++++++------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
index 429f682f15d8..47091614d1b4 100644
--- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
+++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
@@ -14,7 +14,14 @@ allOf:
 
 properties:
   compatible:
-    const: arm,dma-350
+    description:
+      Use "arm,dma-350" for generic integration. A SoC-specific
+      compatible may be listed first, followed by "arm,dma-350".
+    oneOf:
+      - const: arm,dma-350
+      - items:
+          - const: cix,sky1-dma-350
+          - const: arm,dma-350
 
   reg:
     items:
@@ -22,15 +29,22 @@ properties:
 
   interrupts:
     minItems: 1
-    items:
-      - description: Channel 0 interrupt
-      - description: Channel 1 interrupt
-      - description: Channel 2 interrupt
-      - description: Channel 3 interrupt
-      - description: Channel 4 interrupt
-      - description: Channel 5 interrupt
-      - description: Channel 6 interrupt
-      - description: Channel 7 interrupt
+    maxItems: 8
+    description:
+      Either one interrupt per channel (8 interrupts), or one
+      combined interrupt for all channels.
+    oneOf:
+      - items:
+          - description: Channel 0 interrupt
+          - description: Channel 1 interrupt
+          - description: Channel 2 interrupt
+          - description: Channel 3 interrupt
+          - description: Channel 4 interrupt
+          - description: Channel 5 interrupt
+          - description: Channel 6 interrupt
+          - description: Channel 7 interrupt
+      - items:
+          - description: Combined interrupt shared by all channels
 
   "#dma-cells":
     const: 1
-- 
2.34.1

[PATCH v4 2/3] dma: arm-dma350: support combined IRQ mode with runtime IRQ topology detection

[Jun Guo]

DMA-350 can be integrated with either per-channel IRQ lines or a single
combined IRQ line. Add support for both layouts in a unified way.

Detect IRQ topology at probe time via platform_irq_count(), then:
- request one global IRQ and enable DMANSECCTRL.INTREN_ANYCHINTR for
  combined mode, or
- request per-channel IRQs for channel mode.

Refactor IRQ completion/error handling into a shared channel handler
used by both global and per-channel IRQ paths, and guard against IRQs
arriving without an active descriptor.

Assisted-by: Cursor: GPT-5.3-Codex
Signed-off-by: Jun Guo <jun.guo@cixtech.com>
---
 drivers/dma/arm-dma350.c | 165 +++++++++++++++++++++++++++++++++------
 1 file changed, 139 insertions(+), 26 deletions(-)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 84220fa83029..2cf6f783b44f 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -14,6 +14,7 @@
 #include "virt-dma.h"
 
 #define DMAINFO			0x0f00
+#define DRIVER_NAME		"arm-dma350"
 
 #define DMA_BUILDCFG0		0xb0
 #define DMA_CFG_DATA_WIDTH	GENMASK(18, 16)
@@ -142,6 +143,9 @@
 #define LINK_LINKADDR		BIT(30)
 #define LINK_LINKADDRHI		BIT(31)
 
+/* DMA NONSECURE CONTROL REGISTER */
+#define DMANSECCTRL		0x20c
+#define INTREN_ANYCHINTR_EN	BIT(0)
 
 enum ch_ctrl_donetype {
 	CH_CTRL_DONETYPE_NONE = 0,
@@ -192,6 +196,7 @@ struct d350_chan {
 
 struct d350 {
 	struct dma_device dma;
+	void __iomem *base;
 	int nchan;
 	int nreq;
 	struct d350_chan channels[] __counted_by(nchan);
@@ -461,18 +466,40 @@ static void d350_issue_pending(struct dma_chan *chan)
 	spin_unlock_irqrestore(&dch->vc.lock, flags);
 }
 
-static irqreturn_t d350_irq(int irq, void *data)
+static void d350_handle_chan_irq(struct d350_chan *dch, struct device *dev,
+				 int chan_id, u32 ch_status)
 {
-	struct d350_chan *dch = data;
-	struct device *dev = dch->vc.chan.device->dev;
-	struct virt_dma_desc *vd = &dch->desc->vd;
-	u32 ch_status;
+	struct virt_dma_desc *vd;
+	bool intr_done = ch_status & CH_STAT_INTR_DONE;
+	bool intr_err = ch_status & CH_STAT_INTR_ERR;
 
-	ch_status = readl(dch->base + CH_STATUS);
-	if (!ch_status)
-		return IRQ_NONE;
+	if (!intr_done && !intr_err) {
+		if (chan_id >= 0)
+			dev_warn(dev, "Channel %d unexpected IRQ: 0x%08x\n",
+				 chan_id, ch_status);
+		else
+			dev_warn(dev, "Unexpected IRQ source? 0x%08x\n", ch_status);
+		writel_relaxed(ch_status, dch->base + CH_STATUS);
+		return;
+	}
+
+	writel_relaxed(ch_status, dch->base + CH_STATUS);
+
+	spin_lock(&dch->vc.lock);
+	if (!dch->desc) {
+		if (chan_id >= 0)
+			dev_warn(dev,
+				 "Channel %d IRQ without active descriptor: 0x%08x\n",
+				 chan_id, ch_status);
+		else
+			dev_warn(dev, "IRQ without active descriptor: 0x%08x\n",
+				 ch_status);
+		spin_unlock(&dch->vc.lock);
+		return;
+	}
 
-	if (ch_status & CH_STAT_INTR_ERR) {
+	vd = &dch->desc->vd;
+	if (intr_err) {
 		u32 errinfo = readl_relaxed(dch->base + CH_ERRINFO);
 
 		if (errinfo & (CH_ERRINFO_AXIRDPOISERR | CH_ERRINFO_AXIRDRESPERR))
@@ -483,14 +510,10 @@ static irqreturn_t d350_irq(int irq, void *data)
 			vd->tx_result.result = DMA_TRANS_ABORTED;
 
 		vd->tx_result.residue = d350_get_residue(dch);
-	} else if (!(ch_status & CH_STAT_INTR_DONE)) {
-		dev_warn(dev, "Unexpected IRQ source? 0x%08x\n", ch_status);
 	}
-	writel_relaxed(ch_status, dch->base + CH_STATUS);
 
-	spin_lock(&dch->vc.lock);
 	vchan_cookie_complete(vd);
-	if (ch_status & CH_STAT_INTR_DONE) {
+	if (intr_done) {
 		dch->status = DMA_COMPLETE;
 		dch->residue = 0;
 		d350_start_next(dch);
@@ -499,6 +522,44 @@ static irqreturn_t d350_irq(int irq, void *data)
 		dch->residue = vd->tx_result.residue;
 	}
 	spin_unlock(&dch->vc.lock);
+}
+
+static irqreturn_t d350_global_irq(int irq, void *data)
+{
+	struct d350 *dmac = (struct d350 *)data;
+	irqreturn_t ret = IRQ_NONE;
+	int i;
+
+	(void)irq;
+
+	for (i = 0; i < dmac->nchan; i++) {
+		struct d350_chan *dch = &dmac->channels[i];
+		u32 ch_status;
+
+		ch_status = readl(dch->base + CH_STATUS);
+		if (!ch_status)
+			continue;
+
+		ret = IRQ_HANDLED;
+		d350_handle_chan_irq(dch, dmac->dma.dev, i, ch_status);
+	}
+
+	return ret;
+}
+
+static irqreturn_t d350_channel_irq(int irq, void *data)
+{
+	struct d350_chan *dch = data;
+	struct device *dev = dch->vc.chan.device->dev;
+	u32 ch_status;
+
+	(void)irq;
+
+	ch_status = readl(dch->base + CH_STATUS);
+	if (!ch_status)
+		return IRQ_NONE;
+
+	d350_handle_chan_irq(dch, dev, -1, ch_status);
 
 	return IRQ_HANDLED;
 }
@@ -506,10 +567,18 @@ static irqreturn_t d350_irq(int irq, void *data)
 static int d350_alloc_chan_resources(struct dma_chan *chan)
 {
 	struct d350_chan *dch = to_d350_chan(chan);
-	int ret = request_irq(dch->irq, d350_irq, IRQF_SHARED,
-			      dev_name(&dch->vc.chan.dev->device), dch);
-	if (!ret)
-		writel_relaxed(CH_INTREN_DONE | CH_INTREN_ERR, dch->base + CH_INTREN);
+	int ret = 0;
+
+	if (dch->irq >= 0) {
+		ret = request_irq(dch->irq, d350_channel_irq, IRQF_SHARED,
+				  dev_name(&dch->vc.chan.dev->device), dch);
+		if (ret) {
+			dev_err(chan->device->dev, "Failed to request IRQ %d\n", dch->irq);
+			return ret;
+		}
+	}
+
+	writel_relaxed(CH_INTREN_DONE | CH_INTREN_ERR, dch->base + CH_INTREN);
 
 	return ret;
 }
@@ -519,18 +588,21 @@ static void d350_free_chan_resources(struct dma_chan *chan)
 	struct d350_chan *dch = to_d350_chan(chan);
 
 	writel_relaxed(0, dch->base + CH_INTREN);
-	free_irq(dch->irq, dch);
+	if (dch->irq >= 0) {
+		free_irq(dch->irq, dch);
+		dch->irq = -EINVAL;
+	}
 	vchan_free_chan_resources(&dch->vc);
 }
 
 static int d350_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct d350 *dmac;
+	struct d350 *dmac = NULL;
 	void __iomem *base;
 	u32 reg;
-	int ret, nchan, dw, aw, r, p;
-	bool coherent, memset;
+	int ret, nchan, dw, aw, r, p, irq_count;
+	bool coherent, memset, combined_irq;
 
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
@@ -556,6 +628,7 @@ static int d350_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	dmac->nchan = nchan;
+	dmac->base = base;
 
 	reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG1);
 	dmac->nreq = FIELD_GET(DMA_CFG_NUM_TRIGGER_IN, reg);
@@ -582,12 +655,46 @@ static int d350_probe(struct platform_device *pdev)
 	dmac->dma.device_issue_pending = d350_issue_pending;
 	INIT_LIST_HEAD(&dmac->dma.channels);
 
+	irq_count = platform_irq_count(pdev);
+	if (irq_count < 0)
+		return dev_err_probe(dev, irq_count,
+				     "Failed to count interrupts\n");
+
+	if (irq_count == 1) {
+		combined_irq = true;
+	} else if (irq_count >= nchan) {
+		combined_irq = false;
+	} else {
+		return dev_err_probe(dev, -EINVAL,
+				     "Invalid IRQ count %d for %d channels\n",
+				     irq_count, nchan);
+	}
+
+	if (combined_irq) {
+		int host_irq = platform_get_irq(pdev, 0);
+
+		if (host_irq < 0)
+			return dev_err_probe(dev, host_irq,
+					     "Failed to get IRQ\n");
+
+		ret = devm_request_irq(&pdev->dev, host_irq, d350_global_irq,
+				       IRQF_SHARED, DRIVER_NAME, dmac);
+		if (ret)
+			return dev_err_probe(
+				dev, ret,
+				"Failed to request the combined IRQ %d\n",
+				host_irq);
+		/* Combined Non-Secure Channel Interrupt Enable */
+		writel_relaxed(INTREN_ANYCHINTR_EN, dmac->base + DMANSECCTRL);
+	}
+
 	/* Would be nice to have per-channel caps for this... */
 	memset = true;
 	for (int i = 0; i < nchan; i++) {
 		struct d350_chan *dch = &dmac->channels[i];
 
 		dch->base = base + DMACH(i);
+		dch->irq = -EINVAL;
 		writel_relaxed(CH_CMD_CLEAR, dch->base + CH_CMD);
 
 		reg = readl_relaxed(dch->base + CH_BUILDCFG1);
@@ -595,10 +702,15 @@ static int d350_probe(struct platform_device *pdev)
 			dev_warn(dev, "No command link support on channel %d\n", i);
 			continue;
 		}
-		dch->irq = platform_get_irq(pdev, i);
-		if (dch->irq < 0)
-			return dev_err_probe(dev, dch->irq,
-					     "Failed to get IRQ for channel %d\n", i);
+
+		if (!combined_irq) {
+			dch->irq = platform_get_irq(pdev, i);
+			if (dch->irq < 0)
+				return dev_err_probe(
+					dev, dch->irq,
+					"Failed to get IRQ for channel %d\n",
+					i);
+		}
 
 		dch->has_wrap = FIELD_GET(CH_CFG_HAS_WRAP, reg);
 		dch->has_trig = FIELD_GET(CH_CFG_HAS_TRIGIN, reg) &
@@ -640,6 +752,7 @@ static void d350_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id d350_of_match[] __maybe_unused = {
+	{ .compatible = "cix,sky1-dma-350" },
 	{ .compatible = "arm,dma-350" },
 	{}
 };
-- 
2.34.1

[PATCH v4 3/3] arm64: dts: cix: add DT nodes for DMA

[Jun Guo]

Add the device tree node for the dma controller of the CIX SKY1 SoC.

Signed-off-by: Jun Guo <jun.guo@cixtech.com>
---
 arch/arm64/boot/dts/cix/sky1.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
index 210739beac6d..1185c99d8d9d 100644
--- a/arch/arm64/boot/dts/cix/sky1.dtsi
+++ b/arch/arm64/boot/dts/cix/sky1.dtsi
@@ -480,6 +480,13 @@ iomuxc: pinctrl@4170000 {
 			reg = <0x0 0x04170000 0x0 0x1000>;
 		};
 
+		fch_dmac: dma-controller@4190000 {
+			compatible = "cix,sky1-dma-350", "arm,dma-350";
+			reg = <0x0 0x4190000 0x0 0x10000>;
+			interrupts = <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>;
+			#dma-cells = <1>;
+		};
+
 		mbox_ap2se: mailbox@5060000 {
 			compatible = "cix,sky1-mbox";
 			reg = <0x0 0x05060000 0x0 0x10000>;
-- 
2.34.1

Re: [PATCH v4 1/3] dt-bindings: dma: arm-dma350: document generic and combined IRQ topologies

[Krzysztof Kozlowski]

On 23/03/2026 12:48, Jun Guo wrote:
> Update the DMA-350 DT binding to match the current driver behavior.
> 
> Allow both:
> - "arm,dma-350" as the generic compatible, and
> - "cix,sky1-dma-350", "arm,dma-350" for SoC-specific fallback usage.
> 
> Also document interrupt topology variants supported by hardware
> integration:
> - one combined interrupt for all channels, or
> - one interrupt per channel (up to 8 channels).
> 
> Assisted-by: Cursor: GPT-5.3-Codex

There is no space here. Read the docs, I quite insisted on this last
time. If you make mistakes in this, I doubt you read the docs thus I
doubt you followed the requirements - have actual rights to send it for
example.

> Signed-off-by: Jun Guo <jun.guo@cixtech.com>
> ---
>  .../devicetree/bindings/dma/arm,dma-350.yaml  | 34 +++++++++++++------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> index 429f682f15d8..47091614d1b4 100644
> --- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> +++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> @@ -14,7 +14,14 @@ allOf:
>  
>  properties:
>    compatible:
> -    const: arm,dma-350
> +    description:
> +      Use "arm,dma-350" for generic integration. A SoC-specific
> +      compatible may be listed first, followed by "arm,dma-350".

What is the point of explaining it? What is the difference between
generic integration and non-generic?

Best regards,
Krzysztof

Re: [PATCH v4 1/3] dt-bindings: dma: arm-dma350: document generic and combined IRQ topologies

[Krzysztof Kozlowski]

On 23/03/2026 13:00, Krzysztof Kozlowski wrote:
> On 23/03/2026 12:48, Jun Guo wrote:
>> Update the DMA-350 DT binding to match the current driver behavior.
>>
>> Allow both:
>> - "arm,dma-350" as the generic compatible, and
>> - "cix,sky1-dma-350", "arm,dma-350" for SoC-specific fallback usage.
>>
>> Also document interrupt topology variants supported by hardware
>> integration:
>> - one combined interrupt for all channels, or
>> - one interrupt per channel (up to 8 channels).
>>
>> Assisted-by: Cursor: GPT-5.3-Codex
> 
> There is no space here. Read the docs, I quite insisted on this last
> time. If you make mistakes in this, I doubt you read the docs thus I
> doubt you followed the requirements - have actual rights to send it for
> example.

And you already received that comment:

"Wrong tag, please read carefully the guideline before using LLM tools."

Best regards,
Krzysztof

Re: [PATCH v4 2/3] dma: arm-dma350: support combined IRQ mode with runtime IRQ topology detection

[Krzysztof Kozlowski]

On 23/03/2026 12:48, Jun Guo wrote:
>  
>  		dch->has_wrap = FIELD_GET(CH_CFG_HAS_WRAP, reg);
>  		dch->has_trig = FIELD_GET(CH_CFG_HAS_TRIGIN, reg) &
> @@ -640,6 +752,7 @@ static void d350_remove(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id d350_of_match[] __maybe_unused = {
> +	{ .compatible = "cix,sky1-dma-350" },

Redundant. Drop. Review your LLM generated code...


>  	{ .compatible = "arm,dma-350" },
>  	{}
>  };


Best regards,
Krzysztof

Re: [PATCH v4 1/3] dt-bindings: dma: arm-dma350: document generic and combined IRQ topologies

[Jun Guo]

On 3/23/2026 8:00 PM, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL
> 
> On 23/03/2026 12:48, Jun Guo wrote:
>> Update the DMA-350 DT binding to match the current driver behavior.
>>
>> Allow both:
>> - "arm,dma-350" as the generic compatible, and
>> - "cix,sky1-dma-350", "arm,dma-350" for SoC-specific fallback usage.
>>
>> Also document interrupt topology variants supported by hardware
>> integration:
>> - one combined interrupt for all channels, or
>> - one interrupt per channel (up to 8 channels).
>>
>> Assisted-by: Cursor: GPT-5.3-Codex
> 
> There is no space here. Read the docs, I quite insisted on this last
> time. If you make mistakes in this, I doubt you read the docs thus I
> doubt you followed the requirements - have actual rights to send it for
> example.
Sorry, I did overlook the format between AGENT_NAME and MODEL_VERSION. I 
will fix it.
> 
>> Signed-off-by: Jun Guo <jun.guo@cixtech.com>
>> ---
>>   .../devicetree/bindings/dma/arm,dma-350.yaml  | 34 +++++++++++++------
>>   1 file changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
>> index 429f682f15d8..47091614d1b4 100644
>> --- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
>> +++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
>> @@ -14,7 +14,14 @@ allOf:
>>
>>   properties:
>>     compatible:
>> -    const: arm,dma-350
>> +    description:
>> +      Use "arm,dma-350" for generic integration. A SoC-specific
>> +      compatible may be listed first, followed by "arm,dma-350".
> 
> What is the point of explaining it? What is the difference between
> generic integration and non-generic?
I might not need to add the "cix,sky1-dma-350" and can directly use 
"arm,dma-350" instead. I will rework the code and description accordingly.

Best regards,
Jun

Re: [PATCH v4 1/3] dt-bindings: dma: arm-dma350: document generic and combined IRQ topologies

[Jun Guo]

On 3/23/2026 8:02 PM, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL
> 
> On 23/03/2026 13:00, Krzysztof Kozlowski wrote:
>> On 23/03/2026 12:48, Jun Guo wrote:
>>> Update the DMA-350 DT binding to match the current driver behavior.
>>>
>>> Allow both:
>>> - "arm,dma-350" as the generic compatible, and
>>> - "cix,sky1-dma-350", "arm,dma-350" for SoC-specific fallback usage.
>>>
>>> Also document interrupt topology variants supported by hardware
>>> integration:
>>> - one combined interrupt for all channels, or
>>> - one interrupt per channel (up to 8 channels).
>>>
>>> Assisted-by: Cursor: GPT-5.3-Codex
>>
>> There is no space here. Read the docs, I quite insisted on this last
>> time. If you make mistakes in this, I doubt you read the docs thus I
>> doubt you followed the requirements - have actual rights to send it for
>> example.
> 
> And you already received that comment:
> 
> "Wrong tag, please read carefully the guideline before using LLM tools."
I did receive that feedback, and I will learn from this experience to 
submit more reliable patches in the future.

Best regards,
Jun

Re: [PATCH v4 1/3] dt-bindings: dma: arm-dma350: document generic and combined IRQ topologies

[Robin Murphy]

On 2026-03-23 11:48 am, Jun Guo wrote:
> Update the DMA-350 DT binding to match the current driver behavior.
> 
> Allow both:
> - "arm,dma-350" as the generic compatible, and
> - "cix,sky1-dma-350", "arm,dma-350" for SoC-specific fallback usage.
> 
> Also document interrupt topology variants supported by hardware
> integration:
> - one combined interrupt for all channels, or
> - one interrupt per channel (up to 8 channels).

To repeat myself for the 3rd time, this is at best unnecessary, and at 
worst arguably wrong. Here's an example of a system which happens to use 
the combined interrupt from another IP block which also offers both options:

https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8qm.dtsi#n279

Same thing here; each channel is a distinct interrupt source, so it is 
perfectly honest to describe that consistently in DT, regardless of 
whether or not the interrupt signals are still distinct by the time they 
reach the interrupt controller.

Furthermore, in this case the IRQ_COMB_NONSEC interrupt actually has 
additional functionality beyond just being a mux of the individual 
IRQ_CHANNEL interrupts. So although Linux probably won't ever care, if 
it's going to be in the DT binding then it should really be distinct 
from the channel interrupts anyway, since systems could well wire them 
*all* up, and an OS could choose to use the IRQ_CHANNEL outputs directly 
for individual channel completion/error status, while also using the 
IRQ_COMB_NONSEC just for its overall INTR_ALLCH{STOPPED,PAUSED,IDLE} status.

If you only want to make your thing work in Linux, all that is needed is 
a 1-line change in the driver to enable the INTR_ANYCHINTR bit (which as 
I've also said before, we can do unconditionally because we're *not* 
using the other INTR_ALLCH stuff), and to write your DT using the 
existing binding. "One interrupt per channel" already carries no 
expectation that they all have to be *different* interrupts.

Thanks,
Robin.

> Assisted-by: Cursor: GPT-5.3-Codex
> Signed-off-by: Jun Guo <jun.guo@cixtech.com>
> ---
>   .../devicetree/bindings/dma/arm,dma-350.yaml  | 34 +++++++++++++------
>   1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> index 429f682f15d8..47091614d1b4 100644
> --- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> +++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> @@ -14,7 +14,14 @@ allOf:
>   
>   properties:
>     compatible:
> -    const: arm,dma-350
> +    description:
> +      Use "arm,dma-350" for generic integration. A SoC-specific
> +      compatible may be listed first, followed by "arm,dma-350".
> +    oneOf:
> +      - const: arm,dma-350
> +      - items:
> +          - const: cix,sky1-dma-350
> +          - const: arm,dma-350
>   
>     reg:
>       items:
> @@ -22,15 +29,22 @@ properties:
>   
>     interrupts:
>       minItems: 1
> -    items:
> -      - description: Channel 0 interrupt
> -      - description: Channel 1 interrupt
> -      - description: Channel 2 interrupt
> -      - description: Channel 3 interrupt
> -      - description: Channel 4 interrupt
> -      - description: Channel 5 interrupt
> -      - description: Channel 6 interrupt
> -      - description: Channel 7 interrupt
> +    maxItems: 8
> +    description:
> +      Either one interrupt per channel (8 interrupts), or one
> +      combined interrupt for all channels.
> +    oneOf:
> +      - items:
> +          - description: Channel 0 interrupt
> +          - description: Channel 1 interrupt
> +          - description: Channel 2 interrupt
> +          - description: Channel 3 interrupt
> +          - description: Channel 4 interrupt
> +          - description: Channel 5 interrupt
> +          - description: Channel 6 interrupt
> +          - description: Channel 7 interrupt
> +      - items:
> +          - description: Combined interrupt shared by all channels
>   
>     "#dma-cells":
>       const: 1

Re: [PATCH v4 2/3] dma: arm-dma350: support combined IRQ mode with runtime IRQ topology detection

[Robin Murphy]

On 2026-03-23 11:48 am, Jun Guo wrote:
> DMA-350 can be integrated with either per-channel IRQ lines or a single
> combined IRQ line. Add support for both layouts in a unified way.
> 
> Detect IRQ topology at probe time via platform_irq_count(), then:
> - request one global IRQ and enable DMANSECCTRL.INTREN_ANYCHINTR for
>    combined mode, or
> - request per-channel IRQs for channel mode.
> 
> Refactor IRQ completion/error handling into a shared channel handler
> used by both global and per-channel IRQ paths, and guard against IRQs
> arriving without an active descriptor.
> 
> Assisted-by: Cursor: GPT-5.3-Codex
> Signed-off-by: Jun Guo <jun.guo@cixtech.com>
> ---
>   drivers/dma/arm-dma350.c | 165 +++++++++++++++++++++++++++++++++------
>   1 file changed, 139 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index 84220fa83029..2cf6f783b44f 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c
> @@ -14,6 +14,7 @@
>   #include "virt-dma.h"
>   
>   #define DMAINFO			0x0f00
> +#define DRIVER_NAME		"arm-dma350"

Why is this here in the middle of register definitions? But frankly why 
is it being added at all?

>   #define DMA_BUILDCFG0		0xb0
>   #define DMA_CFG_DATA_WIDTH	GENMASK(18, 16)
> @@ -142,6 +143,9 @@
>   #define LINK_LINKADDR		BIT(30)
>   #define LINK_LINKADDRHI		BIT(31)
>   
> +/* DMA NONSECURE CONTROL REGISTER */
> +#define DMANSECCTRL		0x20c

Please follow the existing code rather than making a confusing ugly 
mess. DMANSECCTRL is a register frame at offset 0x0200; it contains 
(among others) a register named NSEC_CTRL at offset 0x0c.

> +#define INTREN_ANYCHINTR_EN	BIT(0)
>   
>   enum ch_ctrl_donetype {
>   	CH_CTRL_DONETYPE_NONE = 0,
> @@ -192,6 +196,7 @@ struct d350_chan {
>   
>   struct d350 {
>   	struct dma_device dma;
> +	void __iomem *base;

Why?

>   	int nchan;
>   	int nreq;
>   	struct d350_chan channels[] __counted_by(nchan);
> @@ -461,18 +466,40 @@ static void d350_issue_pending(struct dma_chan *chan)
>   	spin_unlock_irqrestore(&dch->vc.lock, flags);
>   }
>   
> -static irqreturn_t d350_irq(int irq, void *data)
> +static void d350_handle_chan_irq(struct d350_chan *dch, struct device *dev,
> +				 int chan_id, u32 ch_status)
>   {
> -	struct d350_chan *dch = data;
> -	struct device *dev = dch->vc.chan.device->dev;
> -	struct virt_dma_desc *vd = &dch->desc->vd;
> -	u32 ch_status;
> +	struct virt_dma_desc *vd;
> +	bool intr_done = ch_status & CH_STAT_INTR_DONE;
> +	bool intr_err = ch_status & CH_STAT_INTR_ERR;
>   
> -	ch_status = readl(dch->base + CH_STATUS);
> -	if (!ch_status)
> -		return IRQ_NONE;
> +	if (!intr_done && !intr_err) {

Why is the logic being changed here?

> +		if (chan_id >= 0)
> +			dev_warn(dev, "Channel %d unexpected IRQ: 0x%08x\n",
> +				 chan_id, ch_status);

Why? I see no reason why "dev" shouldn't still be the channel device.

> +		else
> +			dev_warn(dev, "Unexpected IRQ source? 0x%08x\n", ch_status);
> +		writel_relaxed(ch_status, dch->base + CH_STATUS);
> +		return;
> +	}
> +
> +	writel_relaxed(ch_status, dch->base + CH_STATUS);
> +
> +	spin_lock(&dch->vc.lock);
> +	if (!dch->desc) {
> +		if (chan_id >= 0)
> +			dev_warn(dev,
> +				 "Channel %d IRQ without active descriptor: 0x%08x\n",
> +				 chan_id, ch_status);
> +		else
> +			dev_warn(dev, "IRQ without active descriptor: 0x%08x\n",
> +				 ch_status);
> +		spin_unlock(&dch->vc.lock);
> +		return;
> +	}

This seems entirely new and nothing to do with refactoring. Why do you 
think it's necessary?

> -	if (ch_status & CH_STAT_INTR_ERR) {
> +	vd = &dch->desc->vd;
> +	if (intr_err) {
>   		u32 errinfo = readl_relaxed(dch->base + CH_ERRINFO);
>   
>   		if (errinfo & (CH_ERRINFO_AXIRDPOISERR | CH_ERRINFO_AXIRDRESPERR))
> @@ -483,14 +510,10 @@ static irqreturn_t d350_irq(int irq, void *data)
>   			vd->tx_result.result = DMA_TRANS_ABORTED;
>   
>   		vd->tx_result.residue = d350_get_residue(dch);
> -	} else if (!(ch_status & CH_STAT_INTR_DONE)) {
> -		dev_warn(dev, "Unexpected IRQ source? 0x%08x\n", ch_status);
>   	}
> -	writel_relaxed(ch_status, dch->base + CH_STATUS);
>   
> -	spin_lock(&dch->vc.lock);

I can't recall the details off-hand right now, but I have a strong 
feeling the order of operations here was reasonably significant - what's 
the justifcation for changing it?

>   	vchan_cookie_complete(vd);
> -	if (ch_status & CH_STAT_INTR_DONE) {
> +	if (intr_done) {
>   		dch->status = DMA_COMPLETE;
>   		dch->residue = 0;
>   		d350_start_next(dch);
> @@ -499,6 +522,44 @@ static irqreturn_t d350_irq(int irq, void *data)
>   		dch->residue = vd->tx_result.residue;
>   	}
>   	spin_unlock(&dch->vc.lock);
> +}
> +
> +static irqreturn_t d350_global_irq(int irq, void *data)
> +{
> +	struct d350 *dmac = (struct d350 *)data;

This isn't C++

> +	irqreturn_t ret = IRQ_NONE;
> +	int i;
> +
> +	(void)irq;

Why?

> +	for (i = 0; i < dmac->nchan; i++) {
> +		struct d350_chan *dch = &dmac->channels[i];
> +		u32 ch_status;
> +
> +		ch_status = readl(dch->base + CH_STATUS);
> +		if (!ch_status)

Why is this factored out, only to be duplicated in both callers?

> +			continue;
> +
> +		ret = IRQ_HANDLED;
> +		d350_handle_chan_irq(dch, dmac->dma.dev, i, ch_status);
> +	}
> +
> +	return ret;
> +}
> +
> +static irqreturn_t d350_channel_irq(int irq, void *data)
> +{
> +	struct d350_chan *dch = data;
> +	struct device *dev = dch->vc.chan.device->dev;
> +	u32 ch_status;
> +
> +	(void)irq;
> +
> +	ch_status = readl(dch->base + CH_STATUS);
> +	if (!ch_status)
> +		return IRQ_NONE;
> +
> +	d350_handle_chan_irq(dch, dev, -1, ch_status);
>   
>   	return IRQ_HANDLED;
>   }
> @@ -506,10 +567,18 @@ static irqreturn_t d350_irq(int irq, void *data)
>   static int d350_alloc_chan_resources(struct dma_chan *chan)
>   {
>   	struct d350_chan *dch = to_d350_chan(chan);
> -	int ret = request_irq(dch->irq, d350_irq, IRQF_SHARED,
> -			      dev_name(&dch->vc.chan.dev->device), dch);
> -	if (!ret)
> -		writel_relaxed(CH_INTREN_DONE | CH_INTREN_ERR, dch->base + CH_INTREN);
> +	int ret = 0;
> +
> +	if (dch->irq >= 0) {
> +		ret = request_irq(dch->irq, d350_channel_irq, IRQF_SHARED,
> +				  dev_name(&dch->vc.chan.dev->device), dch);
> +		if (ret) {
> +			dev_err(chan->device->dev, "Failed to request IRQ %d\n", dch->irq);
> +			return ret;
> +		}
> +	}
> +
> +	writel_relaxed(CH_INTREN_DONE | CH_INTREN_ERR, dch->base + CH_INTREN);
>   
>   	return ret;
>   }
> @@ -519,18 +588,21 @@ static void d350_free_chan_resources(struct dma_chan *chan)
>   	struct d350_chan *dch = to_d350_chan(chan);
>   
>   	writel_relaxed(0, dch->base + CH_INTREN);
> -	free_irq(dch->irq, dch);
> +	if (dch->irq >= 0) {
> +		free_irq(dch->irq, dch);
> +		dch->irq = -EINVAL;
> +	}
>   	vchan_free_chan_resources(&dch->vc);
>   }
>   
>   static int d350_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> -	struct d350 *dmac;
> +	struct d350 *dmac = NULL;

Why?

>   	void __iomem *base;
>   	u32 reg;
> -	int ret, nchan, dw, aw, r, p;
> -	bool coherent, memset;
> +	int ret, nchan, dw, aw, r, p, irq_count;
> +	bool coherent, memset, combined_irq;
>   
>   	base = devm_platform_ioremap_resource(pdev, 0);
>   	if (IS_ERR(base))
> @@ -556,6 +628,7 @@ static int d350_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	dmac->nchan = nchan;
> +	dmac->base = base;
>   
>   	reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG1);
>   	dmac->nreq = FIELD_GET(DMA_CFG_NUM_TRIGGER_IN, reg);
> @@ -582,12 +655,46 @@ static int d350_probe(struct platform_device *pdev)
>   	dmac->dma.device_issue_pending = d350_issue_pending;
>   	INIT_LIST_HEAD(&dmac->dma.channels);
>   
> +	irq_count = platform_irq_count(pdev);
> +	if (irq_count < 0)
> +		return dev_err_probe(dev, irq_count,
> +				     "Failed to count interrupts\n");
> +
> +	if (irq_count == 1) {
> +		combined_irq = true;

What if there's only 1 channel and it is actually using its dedicated IRQ?

> +	} else if (irq_count >= nchan) {
> +		combined_irq = false;
> +	} else {
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid IRQ count %d for %d channels\n",
> +				     irq_count, nchan);

Thsi is a change in behaviour, since we wouldm't currently consider it 
an error if the last 1 or more channels don't have an interrupt, but 
aren't accessible anyway - if a system *always* reserved the last couple 
of channels for the Secure world, arguably there would be no real 
requiremnt to describe them in the Non-Secure DT at all.

> +	}
> +
> +	if (combined_irq) {
> +		int host_irq = platform_get_irq(pdev, 0);
> +
> +		if (host_irq < 0)
> +			return dev_err_probe(dev, host_irq,
> +					     "Failed to get IRQ\n");
> +
> +		ret = devm_request_irq(&pdev->dev, host_irq, d350_global_irq,
> +				       IRQF_SHARED, DRIVER_NAME, dmac);
> +		if (ret)
> +			return dev_err_probe(
> +				dev, ret,
> +				"Failed to request the combined IRQ %d\n",
> +				host_irq);
> +		/* Combined Non-Secure Channel Interrupt Enable */
> +		writel_relaxed(INTREN_ANYCHINTR_EN, dmac->base + DMANSECCTRL);
> +	}
> +
>   	/* Would be nice to have per-channel caps for this... */
>   	memset = true;
>   	for (int i = 0; i < nchan; i++) {
>   		struct d350_chan *dch = &dmac->channels[i];
>   
>   		dch->base = base + DMACH(i);
> +		dch->irq = -EINVAL;
>   		writel_relaxed(CH_CMD_CLEAR, dch->base + CH_CMD);
>   
>   		reg = readl_relaxed(dch->base + CH_BUILDCFG1);
> @@ -595,10 +702,15 @@ static int d350_probe(struct platform_device *pdev)
>   			dev_warn(dev, "No command link support on channel %d\n", i);
>   			continue;
>   		}
> -		dch->irq = platform_get_irq(pdev, i);
> -		if (dch->irq < 0)
> -			return dev_err_probe(dev, dch->irq,
> -					     "Failed to get IRQ for channel %d\n", i);
> +
> +		if (!combined_irq) {
> +			dch->irq = platform_get_irq(pdev, i);

But here's the thing - even if we *were* to do something special in the 
DT binding, why not simply have:

	if (combined_irq)
		dch->irq = combined_irq_num;
	else
		dch->irq = platform_get_irq(pdev, i);

at this point, and then let everything else keep working the way it 
already does?

Thanks,
Robin.

> +			if (dch->irq < 0)
> +				return dev_err_probe(
> +					dev, dch->irq,
> +					"Failed to get IRQ for channel %d\n",
> +					i);
> +		}
>   
>   		dch->has_wrap = FIELD_GET(CH_CFG_HAS_WRAP, reg);
>   		dch->has_trig = FIELD_GET(CH_CFG_HAS_TRIGIN, reg) &
> @@ -640,6 +752,7 @@ static void d350_remove(struct platform_device *pdev)
>   }
>   
>   static const struct of_device_id d350_of_match[] __maybe_unused = {
> +	{ .compatible = "cix,sky1-dma-350" },
>   	{ .compatible = "arm,dma-350" },
>   	{}
>   };

Re: [PATCH v4 1/3] dt-bindings: dma: arm-dma350: document generic and combined IRQ topologies

[Jun Guo]

On 3/24/2026 8:04 PM, Robin Murphy wrote:
> [Some people who received this message don't often get email from 
> robin.murphy@arm.com. Learn why this is important at https://aka.ms/ 
> LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL
> 
> On 2026-03-23 11:48 am, Jun Guo wrote:
>> Update the DMA-350 DT binding to match the current driver behavior.
>>
>> Allow both:
>> - "arm,dma-350" as the generic compatible, and
>> - "cix,sky1-dma-350", "arm,dma-350" for SoC-specific fallback usage.
>>
>> Also document interrupt topology variants supported by hardware
>> integration:
>> - one combined interrupt for all channels, or
>> - one interrupt per channel (up to 8 channels).
> 
> To repeat myself for the 3rd time, this is at best unnecessary, and at
> worst arguably wrong. Here's an example of a system which happens to use
> the combined interrupt from another IP block which also offers both 
> options:
> 
> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ 
> tree/arch/arm64/boot/dts/freescale/imx8qm.dtsi#n279
> 
> Same thing here; each channel is a distinct interrupt source, so it is
> perfectly honest to describe that consistently in DT, regardless of
> whether or not the interrupt signals are still distinct by the time they
> reach the interrupt controller.
> 
> Furthermore, in this case the IRQ_COMB_NONSEC interrupt actually has
> additional functionality beyond just being a mux of the individual
> IRQ_CHANNEL interrupts. So although Linux probably won't ever care, if
> it's going to be in the DT binding then it should really be distinct
> from the channel interrupts anyway, since systems could well wire them
> *all* up, and an OS could choose to use the IRQ_CHANNEL outputs directly
> for individual channel completion/error status, while also using the
> IRQ_COMB_NONSEC just for its overall INTR_ALLCH{STOPPED,PAUSED,IDLE} 
> status.
> 
> If you only want to make your thing work in Linux, all that is needed is
> a 1-line change in the driver to enable the INTR_ANYCHINTR bit (which as
> I've also said before, we can do unconditionally because we're *not*
> using the other INTR_ALLCH stuff), and to write your DT using the
> existing binding. "One interrupt per channel" already carries no
> expectation that they all have to be *different* interrupts.
> 
You've indeed said this for the third time, but I did not ignore your
comments earlier. I carefully reviewed your feedback on both the V1
and V2 patches. However, since your initial comments were not as detailed,
I promptly replied to your emails hoping to discuss them further.
Unfortunately, you did not respond to either of my follow-up emails,
so I proceeded with submitting the current version of the patch.

You can refer to the records here:
https://lore.kernel.org/all/20251216123026.3519923-2-jun.guo@cixtech.com/
or
https://lore.kernel.org/all/20251117015943.2858-3-jun.guo@cixtech.com/

Now, with this latest email, I clearly understand the point you are making.
I will revise and resubmit the patch accordingly, which should result in
a much more concise version. Thank you for your reply.

Best regards,
Jun