[PATCH v2 0/3] dmaengine: arm-dma350: add support for shared interrupt mode

[PATCH v2 0/3] dmaengine: arm-dma350: add support for shared interrupt mode

[Jun Guo]

The arm dma350 controller's hardware implementation varies: some
designs dedicate a separate interrupt line for each channel, while
others have all channels sharing a single interrupt.This patch adds
support for the hardware design where all DMA channels share a
single interrupt.

This series introduces the following enhancements for arm dma350
controller support on arm64 platforms:

Patch 1: Add a compatible string "cix,sky1-dma-350" for the cix
sky1 SoC.
Patch 2: Implement support for the shared interrupt design of the DMA
controller.
Patch 3: add DT nodes for DMA.

The patches have been tested on CIX SKY1 platform, the test steps are
as follows:
    % 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 for 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: update DT binding docs for cix sky1 SoC
  dma: arm-dma350: add support for shared interrupt mode
  arm64: dts: cix: add DT nodes for DMA

 .../devicetree/bindings/dma/arm,dma-350.yaml  |  31 +++--
 arch/arm64/boot/dts/cix/sky1.dtsi             |   7 +
 drivers/dma/arm-dma350.c                      | 124 ++++++++++++++++--
 3 files changed, 142 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] dt-bindings: dma: arm-dma350: update DT binding docs for cix sky1 SoC

[Jun Guo]

Add SoC-specific compatible strings to the DT binding documents to support
cix sky1 SoC.

When adding support for a new SoC that uses the arm dma controller,
future contributors should be encouraged to actually add SoC-specific
compatible strings. The use of the "arm,dma-350" compatible string in
isolation should be disallowed.

Signed-off-by: Jun Guo <jun.guo@cixtech.com>
---
 .../devicetree/bindings/dma/arm,dma-350.yaml  | 31 +++++++++++++------
 1 file changed, 21 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..78bcc7f9aa8b 100644
--- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
+++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
@@ -14,7 +14,11 @@ allOf:
 
 properties:
   compatible:
-    const: arm,dma-350
+    oneOf:
+      - items:
+          - enum:
+              - cix,sky1-dma-350
+          - const: arm,dma-350
 
   reg:
     items:
@@ -22,15 +26,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: |
+      The DMA controller may be configured with separate interrupts for each channel,
+      or with a single combined interrupt for all channels, depending on the SoC integration.
+    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 v2 2/3] dma: arm-dma350: add support for shared interrupt mode

[Jun Guo]

The arm dma350 controller's hardware implementation varies: some
designs dedicate a separate interrupt line for each channel, while
others have all channels sharing a single interrupt.This patch adds
support for the hardware design where all DMA channels share a
single interrupt.

Signed-off-by: Jun Guo <jun.guo@cixtech.com>
---
 drivers/dma/arm-dma350.c | 124 +++++++++++++++++++++++++++++++++++----
 1 file changed, 114 insertions(+), 10 deletions(-)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 9efe2ca7d5ec..6bea18521edd 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,11 +196,16 @@ struct d350_chan {
 
 struct d350 {
 	struct dma_device dma;
+	void __iomem *base;
 	int nchan;
 	int nreq;
 	struct d350_chan channels[] __counted_by(nchan);
 };
 
+struct d350_driver_data {
+	bool combined_irq;
+};
+
 static inline struct d350_chan *to_d350_chan(struct dma_chan *chan)
 {
 	return container_of(chan, struct d350_chan, vc.chan);
@@ -461,7 +470,61 @@ 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 irqreturn_t d350_global_irq(int irq, void *data)
+{
+	struct d350 *dmac = (struct d350 *)data;
+	struct device *dev = dmac->dma.dev;
+	irqreturn_t ret = IRQ_NONE;
+	int i;
+
+	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;
+
+		if (ch_status & CH_STAT_INTR_ERR) {
+			struct virt_dma_desc *vd = &dch->desc->vd;
+			u32 errinfo = readl_relaxed(dch->base + CH_ERRINFO);
+
+			if (errinfo &
+			    (CH_ERRINFO_AXIRDPOISERR | CH_ERRINFO_AXIRDRESPERR))
+				vd->tx_result.result = DMA_TRANS_READ_FAILED;
+			else if (errinfo & CH_ERRINFO_AXIWRRESPERR)
+				vd->tx_result.result = DMA_TRANS_WRITE_FAILED;
+			else
+				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, "Channel %d unexpected IRQ: 0x%08x\n", i,
+				 ch_status);
+		}
+
+		writel_relaxed(ch_status, dch->base + CH_STATUS);
+
+		spin_lock(&dch->vc.lock);
+		if (ch_status & CH_STAT_INTR_DONE) {
+			vchan_cookie_complete(&dch->desc->vd);
+			dch->status = DMA_COMPLETE;
+			dch->residue = 0;
+			d350_start_next(dch);
+		} else if (ch_status & CH_STAT_INTR_ERR) {
+			vchan_cookie_complete(&dch->desc->vd);
+			dch->status = DMA_ERROR;
+			dch->residue = dch->desc->vd.tx_result.residue;
+		}
+		spin_unlock(&dch->vc.lock);
+	}
+
+	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;
@@ -506,10 +569,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) {
+		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;
 }
@@ -526,7 +597,8 @@ static void d350_free_chan_resources(struct dma_chan *chan)
 static int d350_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct d350 *dmac;
+	struct d350 *dmac = NULL;
+	const struct d350_driver_data *data;
 	void __iomem *base;
 	u32 reg;
 	int ret, nchan, dw, aw, r, p;
@@ -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,6 +655,27 @@ static int d350_probe(struct platform_device *pdev)
 	dmac->dma.device_issue_pending = d350_issue_pending;
 	INIT_LIST_HEAD(&dmac->dma.channels);
 
+	data = device_get_match_data(dev);
+	/* Cix Sky1 has a common host IRQ for all its channels. */
+	if (data && data->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++) {
@@ -595,10 +689,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 (!data) {
+			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) &
@@ -639,7 +738,12 @@ static void d350_remove(struct platform_device *pdev)
 	dma_async_device_unregister(&dmac->dma);
 }
 
+static const struct d350_driver_data sky1_dma350_data = {
+	.combined_irq = true,
+};
+
 static const struct of_device_id d350_of_match[] __maybe_unused = {
+	{ .compatible = "cix,sky1-dma-350", .data = &sky1_dma350_data },
 	{ .compatible = "arm,dma-350" },
 	{}
 };
-- 
2.34.1

[PATCH v2 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 189b9a3be55c..8bd7136e822d 100644
--- a/arch/arm64/boot/dts/cix/sky1.dtsi
+++ b/arch/arm64/boot/dts/cix/sky1.dtsi
@@ -353,6 +353,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 v2 1/3] dt-bindings: dma: arm-dma350: update DT binding docs for cix sky1 SoC

[Robin Murphy]

On 2025-12-16 12:30 pm, Jun Guo wrote:
> Add SoC-specific compatible strings to the DT binding documents to support
> cix sky1 SoC.
> 
> When adding support for a new SoC that uses the arm dma controller,
> future contributors should be encouraged to actually add SoC-specific
> compatible strings. The use of the "arm,dma-350" compatible string in
> isolation should be disallowed.

No, you've missed the point - however many channels the hardware 
implements, the DT should list that many interrupt specifiers; it 
doesn't matter whether any of them happen to be the same.

Thanks,
Robin.

> Signed-off-by: Jun Guo <jun.guo@cixtech.com>
> ---
>   .../devicetree/bindings/dma/arm,dma-350.yaml  | 31 +++++++++++++------
>   1 file changed, 21 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..78bcc7f9aa8b 100644
> --- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> +++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> @@ -14,7 +14,11 @@ allOf:
>   
>   properties:
>     compatible:
> -    const: arm,dma-350
> +    oneOf:
> +      - items:
> +          - enum:
> +              - cix,sky1-dma-350
> +          - const: arm,dma-350
>   
>     reg:
>       items:
> @@ -22,15 +26,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: |
> +      The DMA controller may be configured with separate interrupts for each channel,
> +      or with a single combined interrupt for all channels, depending on the SoC integration.
> +    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 v2 2/3] dma: arm-dma350: add support for shared interrupt mode

[Robin Murphy]

On 2025-12-16 12:30 pm, Jun Guo wrote:
> The arm dma350 controller's hardware implementation varies: some
> designs dedicate a separate interrupt line for each channel, while
> others have all channels sharing a single interrupt.This patch adds
> support for the hardware design where all DMA channels share a
> single interrupt.
> 
> Signed-off-by: Jun Guo <jun.guo@cixtech.com>
> ---
>   drivers/dma/arm-dma350.c | 124 +++++++++++++++++++++++++++++++++++----
>   1 file changed, 114 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index 9efe2ca7d5ec..6bea18521edd 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,11 +196,16 @@ struct d350_chan {
>   
>   struct d350 {
>   	struct dma_device dma;
> +	void __iomem *base;
>   	int nchan;
>   	int nreq;
>   	struct d350_chan channels[] __counted_by(nchan);
>   };
>   
> +struct d350_driver_data {
> +	bool combined_irq;
> +};
> +
>   static inline struct d350_chan *to_d350_chan(struct dma_chan *chan)
>   {
>   	return container_of(chan, struct d350_chan, vc.chan);
> @@ -461,7 +470,61 @@ 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 irqreturn_t d350_global_irq(int irq, void *data)
> +{
> +	struct d350 *dmac = (struct d350 *)data;
> +	struct device *dev = dmac->dma.dev;
> +	irqreturn_t ret = IRQ_NONE;
> +	int i;
> +
> +	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;
> +
> +		if (ch_status & CH_STAT_INTR_ERR) {
> +			struct virt_dma_desc *vd = &dch->desc->vd;
> +			u32 errinfo = readl_relaxed(dch->base + CH_ERRINFO);
> +
> +			if (errinfo &
> +			    (CH_ERRINFO_AXIRDPOISERR | CH_ERRINFO_AXIRDRESPERR))
> +				vd->tx_result.result = DMA_TRANS_READ_FAILED;
> +			else if (errinfo & CH_ERRINFO_AXIWRRESPERR)
> +				vd->tx_result.result = DMA_TRANS_WRITE_FAILED;
> +			else
> +				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, "Channel %d unexpected IRQ: 0x%08x\n", i,
> +				 ch_status);
> +		}
> +
> +		writel_relaxed(ch_status, dch->base + CH_STATUS);
> +
> +		spin_lock(&dch->vc.lock);
> +		if (ch_status & CH_STAT_INTR_DONE) {
> +			vchan_cookie_complete(&dch->desc->vd);
> +			dch->status = DMA_COMPLETE;
> +			dch->residue = 0;
> +			d350_start_next(dch);
> +		} else if (ch_status & CH_STAT_INTR_ERR) {
> +			vchan_cookie_complete(&dch->desc->vd);
> +			dch->status = DMA_ERROR;
> +			dch->residue = dch->desc->vd.tx_result.residue;
> +		}
> +		spin_unlock(&dch->vc.lock);
> +	}
> +
> +	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;
> @@ -506,10 +569,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) {
> +		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;
>   }
> @@ -526,7 +597,8 @@ static void d350_free_chan_resources(struct dma_chan *chan)
>   static int d350_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> -	struct d350 *dmac;
> +	struct d350 *dmac = NULL;
> +	const struct d350_driver_data *data;
>   	void __iomem *base;
>   	u32 reg;
>   	int ret, nchan, dw, aw, r, p;
> @@ -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,6 +655,27 @@ static int d350_probe(struct platform_device *pdev)
>   	dmac->dma.device_issue_pending = d350_issue_pending;
>   	INIT_LIST_HEAD(&dmac->dma.channels);
>   
> +	data = device_get_match_data(dev);
> +	/* Cix Sky1 has a common host IRQ for all its channels. */
> +	if (data && data->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);

This one line is all that should be needed - all the rest is pointless 
overcomplication and churn. And either way, copy-pasting the entire IRQ 
handler is not OK.

Thanks,
Robin.

> +
>   	/* Would be nice to have per-channel caps for this... */
>   	memset = true;
>   	for (int i = 0; i < nchan; i++) {
> @@ -595,10 +689,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 (!data) {
> +			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) &
> @@ -639,7 +738,12 @@ static void d350_remove(struct platform_device *pdev)
>   	dma_async_device_unregister(&dmac->dma);
>   }
>   
> +static const struct d350_driver_data sky1_dma350_data = {
> +	.combined_irq = true,
> +};
> +
>   static const struct of_device_id d350_of_match[] __maybe_unused = {
> +	{ .compatible = "cix,sky1-dma-350", .data = &sky1_dma350_data },
>   	{ .compatible = "arm,dma-350" },
>   	{}
>   };

Re: [PATCH v2 2/3] dma: arm-dma350: add support for shared interrupt mode

[Jun Guo]

On 12/16/2025 8:51 PM, Robin Murphy wrote:
> On 2025-12-16 12:30 pm, Jun Guo wrote:
>> The arm dma350 controller's hardware implementation varies: some
>> designs dedicate a separate interrupt line for each channel, while
>> others have all channels sharing a single interrupt.This patch adds
>> support for the hardware design where all DMA channels share a
>> single interrupt.
>>
>> Signed-off-by: Jun Guo <jun.guo@cixtech.com>
>> ---
>>   drivers/dma/arm-dma350.c | 124 +++++++++++++++++++++++++++++++++++----
>>   1 file changed, 114 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
>> index 9efe2ca7d5ec..6bea18521edd 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,11 +196,16 @@ struct d350_chan {
>>
>>   struct d350 {
>>       struct dma_device dma;
>> +     void __iomem *base;
>>       int nchan;
>>       int nreq;
>>       struct d350_chan channels[] __counted_by(nchan);
>>   };
>>
>> +struct d350_driver_data {
>> +     bool combined_irq;
>> +};
>> +
>>   static inline struct d350_chan *to_d350_chan(struct dma_chan *chan)
>>   {
>>       return container_of(chan, struct d350_chan, vc.chan);
>> @@ -461,7 +470,61 @@ 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 irqreturn_t d350_global_irq(int irq, void *data)
>> +{
>> +     struct d350 *dmac = (struct d350 *)data;
>> +     struct device *dev = dmac->dma.dev;
>> +     irqreturn_t ret = IRQ_NONE;
>> +     int i;
>> +
>> +     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;
>> +
>> +             if (ch_status & CH_STAT_INTR_ERR) {
>> +                     struct virt_dma_desc *vd = &dch->desc->vd;
>> +                     u32 errinfo = readl_relaxed(dch->base + 
>> CH_ERRINFO);
>> +
>> +                     if (errinfo &
>> +                         (CH_ERRINFO_AXIRDPOISERR | 
>> CH_ERRINFO_AXIRDRESPERR))
>> +                             vd->tx_result.result = 
>> DMA_TRANS_READ_FAILED;
>> +                     else if (errinfo & CH_ERRINFO_AXIWRRESPERR)
>> +                             vd->tx_result.result = 
>> DMA_TRANS_WRITE_FAILED;
>> +                     else
>> +                             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, "Channel %d unexpected IRQ: 
>> 0x%08x\n", i,
>> +                              ch_status);
>> +             }
>> +
>> +             writel_relaxed(ch_status, dch->base + CH_STATUS);
>> +
>> +             spin_lock(&dch->vc.lock);
>> +             if (ch_status & CH_STAT_INTR_DONE) {
>> +                     vchan_cookie_complete(&dch->desc->vd);
>> +                     dch->status = DMA_COMPLETE;
>> +                     dch->residue = 0;
>> +                     d350_start_next(dch);
>> +             } else if (ch_status & CH_STAT_INTR_ERR) {
>> +                     vchan_cookie_complete(&dch->desc->vd);
>> +                     dch->status = DMA_ERROR;
>> +                     dch->residue = dch->desc->vd.tx_result.residue;
>> +             }
>> +             spin_unlock(&dch->vc.lock);
>> +     }
>> +
>> +     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;
>> @@ -506,10 +569,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) {
>> +             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;
>>   }
>> @@ -526,7 +597,8 @@ static void d350_free_chan_resources(struct 
>> dma_chan *chan)
>>   static int d350_probe(struct platform_device *pdev)
>>   {
>>       struct device *dev = &pdev->dev;
>> -     struct d350 *dmac;
>> +     struct d350 *dmac = NULL;
>> +     const struct d350_driver_data *data;
>>       void __iomem *base;
>>       u32 reg;
>>       int ret, nchan, dw, aw, r, p;
>> @@ -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,6 +655,27 @@ static int d350_probe(struct platform_device *pdev)
>>       dmac->dma.device_issue_pending = d350_issue_pending;
>>       INIT_LIST_HEAD(&dmac->dma.channels);
>>
>> +     data = device_get_match_data(dev);
>> +     /* Cix Sky1 has a common host IRQ for all its channels. */
>> +     if (data && data->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);
> 
> This one line is all that should be needed - all the rest is pointless
> overcomplication and churn. And either way, copy-pasting the entire IRQ
> handler is not OK.
> 
> Thanks,
> Robin.

If the design uses a single interrupt line for all channels, then I only 
need to request one interrupt. When the interrupt occurs, I have to poll 
within the interrupt handler to determine which channel triggered it. 
Are you saying that just this one line is enough to achieve that? I 
don't quite understand.

Best wishes,
Jun Guo

Re: [PATCH v2 2/3] dma: arm-dma350: add support for shared interrupt mode

[Jun Guo]

Hi Robin,

   Regarding your previous comments on this patch, I wrote some 
questions in my last email and would like to continue the discussion.

On 12/17/2025 10:12 AM, Jun Guo wrote:
> 
> On 12/16/2025 8:51 PM, Robin Murphy wrote:
>> On 2025-12-16 12:30 pm, Jun Guo wrote:
>>> The arm dma350 controller's hardware implementation varies: some
>>> designs dedicate a separate interrupt line for each channel, while
>>> others have all channels sharing a single interrupt.This patch adds
>>> support for the hardware design where all DMA channels share a
>>> single interrupt.
>>>
>>> Signed-off-by: Jun Guo <jun.guo@cixtech.com>
>>> ---
>>>   drivers/dma/arm-dma350.c | 124 +++++++++++++++++++++++++++++++++++----
>>>   1 file changed, 114 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
>>> index 9efe2ca7d5ec..6bea18521edd 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,11 +196,16 @@ struct d350_chan {
>>>
>>>   struct d350 {
>>>       struct dma_device dma;
>>> +     void __iomem *base;
>>>       int nchan;
>>>       int nreq;
>>>       struct d350_chan channels[] __counted_by(nchan);
>>>   };
>>>
>>> +struct d350_driver_data {
>>> +     bool combined_irq;
>>> +};
>>> +
>>>   static inline struct d350_chan *to_d350_chan(struct dma_chan *chan)
>>>   {
>>>       return container_of(chan, struct d350_chan, vc.chan);
>>> @@ -461,7 +470,61 @@ 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 irqreturn_t d350_global_irq(int irq, void *data)
>>> +{
>>> +     struct d350 *dmac = (struct d350 *)data;
>>> +     struct device *dev = dmac->dma.dev;
>>> +     irqreturn_t ret = IRQ_NONE;
>>> +     int i;
>>> +
>>> +     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;
>>> +
>>> +             if (ch_status & CH_STAT_INTR_ERR) {
>>> +                     struct virt_dma_desc *vd = &dch->desc->vd;
>>> +                     u32 errinfo = readl_relaxed(dch->base + 
>>> CH_ERRINFO);
>>> +
>>> +                     if (errinfo &
>>> +                         (CH_ERRINFO_AXIRDPOISERR | 
>>> CH_ERRINFO_AXIRDRESPERR))
>>> +                             vd->tx_result.result = 
>>> DMA_TRANS_READ_FAILED;
>>> +                     else if (errinfo & CH_ERRINFO_AXIWRRESPERR)
>>> +                             vd->tx_result.result = 
>>> DMA_TRANS_WRITE_FAILED;
>>> +                     else
>>> +                             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, "Channel %d unexpected IRQ: 
>>> 0x%08x\n", i,
>>> +                              ch_status);
>>> +             }
>>> +
>>> +             writel_relaxed(ch_status, dch->base + CH_STATUS);
>>> +
>>> +             spin_lock(&dch->vc.lock);
>>> +             if (ch_status & CH_STAT_INTR_DONE) {
>>> +                     vchan_cookie_complete(&dch->desc->vd);
>>> +                     dch->status = DMA_COMPLETE;
>>> +                     dch->residue = 0;
>>> +                     d350_start_next(dch);
>>> +             } else if (ch_status & CH_STAT_INTR_ERR) {
>>> +                     vchan_cookie_complete(&dch->desc->vd);
>>> +                     dch->status = DMA_ERROR;
>>> +                     dch->residue = dch->desc->vd.tx_result.residue;
>>> +             }
>>> +             spin_unlock(&dch->vc.lock);
>>> +     }
>>> +
>>> +     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;
>>> @@ -506,10 +569,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) {
>>> +             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;
>>>   }
>>> @@ -526,7 +597,8 @@ static void d350_free_chan_resources(struct 
>>> dma_chan *chan)
>>>   static int d350_probe(struct platform_device *pdev)
>>>   {
>>>       struct device *dev = &pdev->dev;
>>> -     struct d350 *dmac;
>>> +     struct d350 *dmac = NULL;
>>> +     const struct d350_driver_data *data;
>>>       void __iomem *base;
>>>       u32 reg;
>>>       int ret, nchan, dw, aw, r, p;
>>> @@ -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,6 +655,27 @@ static int d350_probe(struct platform_device *pdev)
>>>       dmac->dma.device_issue_pending = d350_issue_pending;
>>>       INIT_LIST_HEAD(&dmac->dma.channels);
>>>
>>> +     data = device_get_match_data(dev);
>>> +     /* Cix Sky1 has a common host IRQ for all its channels. */
>>> +     if (data && data->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);
>>
>> This one line is all that should be needed - all the rest is pointless
>> overcomplication and churn. And either way, copy-pasting the entire IRQ
>> handler is not OK.
>>
>> Thanks,
>> Robin.
> 
> If the design uses a single interrupt line for all channels, then I only 
> need to request one interrupt. When the interrupt occurs, I have to poll 
> within the interrupt handler to determine which channel triggered it. 
> Are you saying that just this one line is enough to achieve that? I 
> don't quite understand.
> 


Best wishes,
Jun Guo

Re: [PATCH v2 1/3] dt-bindings: dma: arm-dma350: update DT binding docs for cix sky1 SoC

[Jun Guo]

Hi Robin,

  Looking forward to your reply.

On 12/16/2025 8:43 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 2025-12-16 12:30 pm, Jun Guo wrote:
>> Add SoC-specific compatible strings to the DT binding documents to 
>> support
>> cix sky1 SoC.
>>
>> When adding support for a new SoC that uses the arm dma controller,
>> future contributors should be encouraged to actually add SoC-specific
>> compatible strings. The use of the "arm,dma-350" compatible string in
>> isolation should be disallowed.
> 
> No, you've missed the point - however many channels the hardware
> implements, the DT should list that many interrupt specifiers; it
> doesn't matter whether any of them happen to be the same.
> 
The interrupts defined in the device tree are based on the hardware 
interrupt numbers of the GIC, not the DMA interrupt specifiers. 
Therefore, I believe it is not possible to configure them as you 
suggested. If you have a demo, you could share it for reference. 
Typically, interrupts in the device tree are defined as follows:

interrupts = <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH>;

> Thanks,
> Robin.
> 
>> Signed-off-by: Jun Guo <jun.guo@cixtech.com>
>> ---
>>   .../devicetree/bindings/dma/arm,dma-350.yaml  | 31 +++++++++++++------
>>   1 file changed, 21 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..78bcc7f9aa8b 100644
>> --- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
>> +++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
>> @@ -14,7 +14,11 @@ allOf:
>>
>>   properties:
>>     compatible:
>> -    const: arm,dma-350
>> +    oneOf:
>> +      - items:
>> +          - enum:
>> +              - cix,sky1-dma-350
>> +          - const: arm,dma-350
>>
>>     reg:
>>       items:
>> @@ -22,15 +26,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: |
>> +      The DMA controller may be configured with separate interrupts 
>> for each channel,
>> +      or with a single combined interrupt for all channels, depending 
>> on the SoC integration.
>> +    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
> 

Best wishes,
Jun Guo