[PATCH 00/14] dmaengine: dma350: Support slave_sg, cyclic and DMA-250

[PATCH 00/14] dmaengine: dma350: Support slave_sg, cyclic and DMA-250

[Jisheng Zhang]

This series firstly does some cleanups, then add support of
device_prep_slave_sg and device_prep_dma_cyclic to DMA-350. 

Lastly, add support for ARM DMA-250 IP. Compared with DMA-350, DMA-250
is a simplified version. They share many common parts, but they do
have difference. Add DMA-250 support by handling their difference by
using different device_prep_slave_sg, device_prep_dma_cyclic and
device_prep_dma_memcpy. DMA-250 doesn't support device_prep_dma_memset.


Jisheng Zhang (14):
  dmaengine: dma350: Fix CH_CTRL_USESRCTRIGIN definition
  dmaengine: dma350: Add missing dch->coherent setting
  dmaengine: dma350: Check vchan_next_desc() return value
  dmaengine: dma350: Check dma_cookie_status() ret code and txstate
  dmaengine: dma350: Register the DMA controller to DT DMA helpers
  dmaengine: dma350: Use dmaenginem_async_device_register
  dmaengine: dma350: Remove redundant err msg if platform_get_irq()
    fails
  dt-bindings: dma: dma350: Document interrupt-names
  dmaengine: dma350: Support dma-channel-mask
  dmaengine: dma350: Alloc command[] from dma pool
  dmaengine: dma350: Support device_prep_slave_sg
  dmaengine: dma350: Support device_prep_dma_cyclic
  dt-bindings: dma: dma350: Support ARM DMA-250
  dmaengine: dma350: Support ARM DMA-250

 .../devicetree/bindings/dma/arm,dma-350.yaml  |   6 +
 drivers/dma/arm-dma350.c                      | 909 ++++++++++++++++--
 2 files changed, 852 insertions(+), 63 deletions(-)

-- 
2.50.0

[PATCH 01/14] dmaengine: dma350: Fix CH_CTRL_USESRCTRIGIN definition

[Jisheng Zhang]

Per the arm-dma350 TRM, The CH_CTRL_USESRCTRIGIN is BIT(25).

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/dma/arm-dma350.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 9efe2ca7d5ec..bf3962f00650 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -58,7 +58,7 @@
 
 #define CH_CTRL			0x0c
 #define CH_CTRL_USEDESTRIGIN	BIT(26)
-#define CH_CTRL_USESRCTRIGIN	BIT(26)
+#define CH_CTRL_USESRCTRIGIN	BIT(25)
 #define CH_CTRL_DONETYPE	GENMASK(23, 21)
 #define CH_CTRL_REGRELOADTYPE	GENMASK(20, 18)
 #define CH_CTRL_XTYPE		GENMASK(11, 9)
-- 
2.50.0

Re: [PATCH 01/14] dmaengine: dma350: Fix CH_CTRL_USESRCTRIGIN definition

[Robin Murphy]

On 2025-08-23 4:39 pm, Jisheng Zhang wrote:
> Per the arm-dma350 TRM, The CH_CTRL_USESRCTRIGIN is BIT(25).

Oops!

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>   drivers/dma/arm-dma350.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index 9efe2ca7d5ec..bf3962f00650 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c
> @@ -58,7 +58,7 @@
>   
>   #define CH_CTRL			0x0c
>   #define CH_CTRL_USEDESTRIGIN	BIT(26)
> -#define CH_CTRL_USESRCTRIGIN	BIT(26)
> +#define CH_CTRL_USESRCTRIGIN	BIT(25)
>   #define CH_CTRL_DONETYPE	GENMASK(23, 21)
>   #define CH_CTRL_REGRELOADTYPE	GENMASK(20, 18)
>   #define CH_CTRL_XTYPE		GENMASK(11, 9)

[PATCH 02/14] dmaengine: dma350: Add missing dch->coherent setting

[Jisheng Zhang]

The dch->coherent setting is missing.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/dma/arm-dma350.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index bf3962f00650..24cbadc5f076 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -587,6 +587,7 @@ static int d350_probe(struct platform_device *pdev)
 	for (int i = 0; i < nchan; i++) {
 		struct d350_chan *dch = &dmac->channels[i];
 
+		dch->coherent = coherent;
 		dch->base = base + DMACH(i);
 		writel_relaxed(CH_CMD_CLEAR, dch->base + CH_CMD);
 
-- 
2.50.0

Re: [PATCH 02/14] dmaengine: dma350: Add missing dch->coherent setting

[Robin Murphy]

On 2025-08-23 4:39 pm, Jisheng Zhang wrote:
> The dch->coherent setting is missing.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>   drivers/dma/arm-dma350.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index bf3962f00650..24cbadc5f076 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c
> @@ -587,6 +587,7 @@ static int d350_probe(struct platform_device *pdev)
>   	for (int i = 0; i < nchan; i++) {
>   		struct d350_chan *dch = &dmac->channels[i];
>   
> +		dch->coherent = coherent;

Nit: I'd put this a bit further down with the CH_LINKATTR setup, but 
otherwise,

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

>   		dch->base = base + DMACH(i);
>   		writel_relaxed(CH_CMD_CLEAR, dch->base + CH_CMD);
>   

[PATCH 03/14] dmaengine: dma350: Check vchan_next_desc() return value

[Jisheng Zhang]

vchan_next_desc() may return NULL, check its return value.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/dma/arm-dma350.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 24cbadc5f076..96350d15ed85 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -399,11 +399,14 @@ static enum dma_status d350_tx_status(struct dma_chan *chan, dma_cookie_t cookie
 static void d350_start_next(struct d350_chan *dch)
 {
 	u32 hdr, *reg;
+	struct virt_dma_desc *vd;
 
-	dch->desc = to_d350_desc(vchan_next_desc(&dch->vc));
-	if (!dch->desc)
+	vd = vchan_next_desc(&dch->vc);
+	if (!vd)
 		return;
 
+	dch->desc = to_d350_desc(vd);
+
 	list_del(&dch->desc->vd.node);
 	dch->status = DMA_IN_PROGRESS;
 	dch->cookie = dch->desc->vd.tx.cookie;
-- 
2.50.0

Re: [PATCH 03/14] dmaengine: dma350: Check vchan_next_desc() return value

[Robin Murphy]

On 2025-08-23 4:39 pm, Jisheng Zhang wrote:
> vchan_next_desc() may return NULL, check its return value.

IIRC it's important that dch->desc gets set to NULL in that case, 
otherwise things can go wonky after a completion interrupt - i.e. the 
current code *is* using the return value both ways, just the sneaky 
thing is that it does actually depend on "vd" being the first member of 
d350_desc to do it concisely, sorry I didn't document that.

Thanks,
Robin.
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>   drivers/dma/arm-dma350.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index 24cbadc5f076..96350d15ed85 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c
> @@ -399,11 +399,14 @@ static enum dma_status d350_tx_status(struct dma_chan *chan, dma_cookie_t cookie
>   static void d350_start_next(struct d350_chan *dch)
>   {
>   	u32 hdr, *reg;
> +	struct virt_dma_desc *vd;
>   
> -	dch->desc = to_d350_desc(vchan_next_desc(&dch->vc));
> -	if (!dch->desc)
> +	vd = vchan_next_desc(&dch->vc);
> +	if (!vd)
>   		return;
>   
> +	dch->desc = to_d350_desc(vd);
> +
>   	list_del(&dch->desc->vd.node);
>   	dch->status = DMA_IN_PROGRESS;
>   	dch->cookie = dch->desc->vd.tx.cookie;

[PATCH 04/14] dmaengine: dma350: Check dma_cookie_status() ret code and txstate

[Jisheng Zhang]

If dma_cookie_status() returns DMA_COMPLETE, we can return immediately.

From another side, the txstate is an optional parameter used to get a
struct with auxilary transfer status information. When not provided
the call to device_tx_status() should return the status of the dma
cookie. Return the status of dma cookie when the txstate optional
parameter is not provided.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/dma/arm-dma350.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 96350d15ed85..17af9bb2a18f 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -377,6 +377,8 @@ static enum dma_status d350_tx_status(struct dma_chan *chan, dma_cookie_t cookie
 	u32 residue = 0;
 
 	status = dma_cookie_status(chan, cookie, state);
+	if (status == DMA_COMPLETE || !state)
+		return status;
 
 	spin_lock_irqsave(&dch->vc.lock, flags);
 	if (cookie == dch->cookie) {
-- 
2.50.0

Re: [PATCH 04/14] dmaengine: dma350: Check dma_cookie_status() ret code and txstate

[Robin Murphy]

On 2025-08-23 4:39 pm, Jisheng Zhang wrote:
> If dma_cookie_status() returns DMA_COMPLETE, we can return immediately.
> 
>  From another side, the txstate is an optional parameter used to get a
> struct with auxilary transfer status information. When not provided
> the call to device_tx_status() should return the status of the dma
> cookie. Return the status of dma cookie when the txstate optional
> parameter is not provided.

Again, the current code was definitely intentional - I think this was 
down to the hardware error case, where for reasons I now can't remember 
I still had to nominally complete the aborted descriptor from the IRQ 
handler to avoid causing some worse problem, and hence we don't return 
early without cross-checking dch->status here, because returning 
DMA_COMPLETE when the descriptor hasn't done its job makes dmatest unhappy.

I did spend a *lot* of time exercising all the error cases, and trying 
to get a sensible result across all of the different reporting APIs was 
fiddly to say the least - there's a huge lack of consistency between 
drivers in this regard, and this was just my attempt to be the 
least-worst one :)

Thanks,
Robin.

> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>   drivers/dma/arm-dma350.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index 96350d15ed85..17af9bb2a18f 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c
> @@ -377,6 +377,8 @@ static enum dma_status d350_tx_status(struct dma_chan *chan, dma_cookie_t cookie
>   	u32 residue = 0;
>   
>   	status = dma_cookie_status(chan, cookie, state);
> +	if (status == DMA_COMPLETE || !state)
> +		return status;
>   
>   	spin_lock_irqsave(&dch->vc.lock, flags);
>   	if (cookie == dch->cookie) {

[PATCH 05/14] dmaengine: dma350: Register the DMA controller to DT DMA helpers

[Jisheng Zhang]

Register the DMA controller to DT DMA helpers so that we convert a DT
phandle to a dma_chan structure.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/dma/arm-dma350.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 17af9bb2a18f..6a9f81f941b0 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -7,6 +7,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
 #include <linux/of.h>
+#include <linux/of_dma.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 
@@ -635,7 +636,7 @@ static int d350_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to register DMA device\n");
 
-	return 0;
+	return of_dma_controller_register(dev->of_node, of_dma_xlate_by_chan_id, &dmac->dma);
 }
 
 static void d350_remove(struct platform_device *pdev)
@@ -643,6 +644,7 @@ static void d350_remove(struct platform_device *pdev)
 	struct d350 *dmac = platform_get_drvdata(pdev);
 
 	dma_async_device_unregister(&dmac->dma);
+	of_dma_controller_free(pdev->dev.of_node);
 }
 
 static const struct of_device_id d350_of_match[] __maybe_unused = {
-- 
2.50.0

Re: [PATCH 05/14] dmaengine: dma350: Register the DMA controller to DT DMA helpers

[Robin Murphy]

On 2025-08-23 4:40 pm, Jisheng Zhang wrote:
> Register the DMA controller to DT DMA helpers so that we convert a DT
> phandle to a dma_chan structure.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>   drivers/dma/arm-dma350.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index 17af9bb2a18f..6a9f81f941b0 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c
> @@ -7,6 +7,7 @@
>   #include <linux/dma-mapping.h>
>   #include <linux/io.h>
>   #include <linux/of.h>
> +#include <linux/of_dma.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
>   
> @@ -635,7 +636,7 @@ static int d350_probe(struct platform_device *pdev)
>   	if (ret)
>   		return dev_err_probe(dev, ret, "Failed to register DMA device\n");
>   
> -	return 0;
> +	return of_dma_controller_register(dev->of_node, of_dma_xlate_by_chan_id, &dmac->dma);

This only works for channels with HAS_TRIGSEL=0 (where I guess HAS_TRIG 
can be assumed from DT describing one) - with selectable triggers the 
trigger number (which the dma-cell specifier actually is) doesn't bear 
any relation to the channel number, so channel selection is both simpler 
and more complicated at the same time, since we could pick any free 
channel with HAS_TRIGSEL, but that's not necessarily just *any* free 
channel...

Given that at this point the driver only considers nominal trigger 
capability for channels with HAS_TRIGSEL=1, this patch seems effectively 
broken.

Thanks,
Robin.

>   }
>   
>   static void d350_remove(struct platform_device *pdev)
> @@ -643,6 +644,7 @@ static void d350_remove(struct platform_device *pdev)
>   	struct d350 *dmac = platform_get_drvdata(pdev);
>   
>   	dma_async_device_unregister(&dmac->dma);
> +	of_dma_controller_free(pdev->dev.of_node);
>   }
>   
>   static const struct of_device_id d350_of_match[] __maybe_unused = {

[PATCH 06/14] dmaengine: dma350: Use dmaenginem_async_device_register

[Jisheng Zhang]

Use managed API dmaenginem_async_device_register() to simplify code
path.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/dma/arm-dma350.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 6a9f81f941b0..36c8bfa67d70 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -632,7 +632,7 @@ static int d350_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dmac);
 
-	ret = dma_async_device_register(&dmac->dma);
+	ret = dmaenginem_async_device_register(&dmac->dma);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to register DMA device\n");
 
@@ -641,9 +641,6 @@ static int d350_probe(struct platform_device *pdev)
 
 static void d350_remove(struct platform_device *pdev)
 {
-	struct d350 *dmac = platform_get_drvdata(pdev);
-
-	dma_async_device_unregister(&dmac->dma);
 	of_dma_controller_free(pdev->dev.of_node);
 }
 
-- 
2.50.0

[PATCH 07/14] dmaengine: dma350: Remove redundant err msg if platform_get_irq() fails

[Jisheng Zhang]

The platform_get_irq() prints an error message if finding the IRQ
fails.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/dma/arm-dma350.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 36c8bfa67d70..6a6d1c2a3ee6 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -604,8 +604,7 @@ static int d350_probe(struct platform_device *pdev)
 		}
 		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);
+			return dch->irq;
 
 		dch->has_wrap = FIELD_GET(CH_CFG_HAS_WRAP, reg);
 		dch->has_trig = FIELD_GET(CH_CFG_HAS_TRIGIN, reg) &
-- 
2.50.0

[PATCH 08/14] dt-bindings: dma: dma350: Document interrupt-names

[Jisheng Zhang]

Currently, the dma350 driver assumes all channels are available to
linux, this may not be true on some platforms, so it's possible no
irq(s) for the unavailable channel(s). What's more, the available
channels may not be continuous. To handle this case, we'd better
get the irq of each channel by name.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 Documentation/devicetree/bindings/dma/arm,dma-350.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
index 429f682f15d8..94752516e51a 100644
--- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
+++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
@@ -32,6 +32,10 @@ properties:
       - description: Channel 6 interrupt
       - description: Channel 7 interrupt
 
+  interrupt-names:
+    minItems: 1
+    maxItems: 8
+
   "#dma-cells":
     const: 1
     description: The cell is the trigger input number
@@ -40,5 +44,6 @@ required:
   - compatible
   - reg
   - interrupts
+  - interrupt-names
 
 unevaluatedProperties: false
-- 
2.50.0

Re: [PATCH 08/14] dt-bindings: dma: dma350: Document interrupt-names

[Krzysztof Kozlowski]

On 23/08/2025 17:40, Jisheng Zhang wrote:
> Currently, the dma350 driver assumes all channels are available to
> linux, this may not be true on some platforms, so it's possible no
> irq(s) for the unavailable channel(s). What's more, the available
> channels may not be continuous. To handle this case, we'd better
> get the irq of each channel by name.

You did not solve the actual problem - binding still lists the
interrupts in specific order.

> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>  Documentation/devicetree/bindings/dma/arm,dma-350.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> index 429f682f15d8..94752516e51a 100644
> --- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> +++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> @@ -32,6 +32,10 @@ properties:
>        - description: Channel 6 interrupt
>        - description: Channel 7 interrupt
>  
> +  interrupt-names:
> +    minItems: 1
> +    maxItems: 8

You need to list the items.


> +
>    "#dma-cells":
>      const: 1
>      description: The cell is the trigger input number
> @@ -40,5 +44,6 @@ required:
>    - compatible
>    - reg
>    - interrupts
> +  - interrupt-names

That's ABI break, so no.



Best regards,
Krzysztof

Re: [PATCH 08/14] dt-bindings: dma: dma350: Document interrupt-names

[Jisheng Zhang]

On Sat, Aug 23, 2025 at 06:09:22PM +0200, Krzysztof Kozlowski wrote:
> On 23/08/2025 17:40, Jisheng Zhang wrote:
> > Currently, the dma350 driver assumes all channels are available to
> > linux, this may not be true on some platforms, so it's possible no
> > irq(s) for the unavailable channel(s). What's more, the available
> > channels may not be continuous. To handle this case, we'd better
> > get the irq of each channel by name.
> 
> You did not solve the actual problem - binding still lists the
> interrupts in specific order.
> 
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > ---
> >  Documentation/devicetree/bindings/dma/arm,dma-350.yaml | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> > index 429f682f15d8..94752516e51a 100644
> > --- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> > +++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> > @@ -32,6 +32,10 @@ properties:
> >        - description: Channel 6 interrupt
> >        - description: Channel 7 interrupt
> >  
> > +  interrupt-names:
> > +    minItems: 1
> > +    maxItems: 8
> 
> You need to list the items.

I found in current dt-bindings, not all doc list the items. So is it
changed now?

> 
> 
> > +
> >    "#dma-cells":
> >      const: 1
> >      description: The cell is the trigger input number
> > @@ -40,5 +44,6 @@ required:
> >    - compatible
> >    - reg
> >    - interrupts
> > +  - interrupt-names
> 
> That's ABI break, so no.

If there's no users of arm-dma350 in upstream so far, is ABI break
allowed? The reason is simple: to simplify the driver to parse
the irq.

Thanks

Re: [PATCH 08/14] dt-bindings: dma: dma350: Document interrupt-names

[Krzysztof Kozlowski]

On 24/08/2025 11:49, Jisheng Zhang wrote:
> On Sat, Aug 23, 2025 at 06:09:22PM +0200, Krzysztof Kozlowski wrote:
>> On 23/08/2025 17:40, Jisheng Zhang wrote:
>>> Currently, the dma350 driver assumes all channels are available to
>>> linux, this may not be true on some platforms, so it's possible no
>>> irq(s) for the unavailable channel(s). What's more, the available
>>> channels may not be continuous. To handle this case, we'd better
>>> get the irq of each channel by name.
>>
>> You did not solve the actual problem - binding still lists the
>> interrupts in specific order.
>>
>>>
>>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
>>> ---
>>>  Documentation/devicetree/bindings/dma/arm,dma-350.yaml | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
>>> index 429f682f15d8..94752516e51a 100644
>>> --- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
>>> +++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
>>> @@ -32,6 +32,10 @@ properties:
>>>        - description: Channel 6 interrupt
>>>        - description: Channel 7 interrupt
>>>  
>>> +  interrupt-names:
>>> +    minItems: 1
>>> +    maxItems: 8
>>
>> You need to list the items.
> 
> I found in current dt-bindings, not all doc list the items. So is it
> changed now?

Close to impossible... :) But even if you found 1% of bindings with
mistake, please kindly take 99% of bindings as the example. Not 1%.

Which bindings were these with undefined names?

> 
>>
>>
>>> +
>>>    "#dma-cells":
>>>      const: 1
>>>      description: The cell is the trigger input number
>>> @@ -40,5 +44,6 @@ required:
>>>    - compatible
>>>    - reg
>>>    - interrupts
>>> +  - interrupt-names
>>
>> That's ABI break, so no.
> 
> If there's no users of arm-dma350 in upstream so far, is ABI break
> allowed? The reason is simple: to simplify the driver to parse
> the irq.

You can try to make your case - see writing bindings. But what about all
out of tree users? All other open source projects? All other kernels? I
really do not ask about anything new here - that's a policy since long time.


Best regards,
Krzysztof

Re: [PATCH 08/14] dt-bindings: dma: dma350: Document interrupt-names

[Jisheng Zhang]

On Sun, Aug 24, 2025 at 12:30:40PM +0200, Krzysztof Kozlowski wrote:
> On 24/08/2025 11:49, Jisheng Zhang wrote:
> > On Sat, Aug 23, 2025 at 06:09:22PM +0200, Krzysztof Kozlowski wrote:
> >> On 23/08/2025 17:40, Jisheng Zhang wrote:
> >>> Currently, the dma350 driver assumes all channels are available to
> >>> linux, this may not be true on some platforms, so it's possible no
> >>> irq(s) for the unavailable channel(s). What's more, the available
> >>> channels may not be continuous. To handle this case, we'd better
> >>> get the irq of each channel by name.
> >>
> >> You did not solve the actual problem - binding still lists the
> >> interrupts in specific order.
> >>
> >>>
> >>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> >>> ---
> >>>  Documentation/devicetree/bindings/dma/arm,dma-350.yaml | 5 +++++
> >>>  1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> >>> index 429f682f15d8..94752516e51a 100644
> >>> --- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> >>> +++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> >>> @@ -32,6 +32,10 @@ properties:
> >>>        - description: Channel 6 interrupt
> >>>        - description: Channel 7 interrupt
> >>>  
> >>> +  interrupt-names:
> >>> +    minItems: 1
> >>> +    maxItems: 8
> >>
> >> You need to list the items.
> > 
> > I found in current dt-bindings, not all doc list the items. So is it
> > changed now?
> 
> Close to impossible... :) But even if you found 1% of bindings with
> mistake, please kindly take 99% of bindings as the example. Not 1%.
> 
> Which bindings were these with undefined names?
> 
> > 
> >>
> >>
> >>> +
> >>>    "#dma-cells":
> >>>      const: 1
> >>>      description: The cell is the trigger input number
> >>> @@ -40,5 +44,6 @@ required:
> >>>    - compatible
> >>>    - reg
> >>>    - interrupts
> >>> +  - interrupt-names
> >>
> >> That's ABI break, so no.
> > 
> > If there's no users of arm-dma350 in upstream so far, is ABI break
> > allowed? The reason is simple: to simplify the driver to parse
> > the irq.
> 
> You can try to make your case - see writing bindings. But what about all
> out of tree users? All other open source projects? All other kernels? I

make sense! I will address your comments in v2. Before that, let me collect
some review comments, especially from dmaengine maintainer and Robin.

Thanks

> really do not ask about anything new here - that's a policy since long time.

Re: [PATCH 08/14] dt-bindings: dma: dma350: Document interrupt-names

[Robin Murphy]

On 2025-08-23 4:40 pm, Jisheng Zhang wrote:
> Currently, the dma350 driver assumes all channels are available to
> linux,

No, it doesn't - the CH_CFG_HAS_CMDLINK check is designed to safely skip 
channels reserved for Secure world (or that don't exist due to an FVP 
bug...), since their CH_BUILDCFG1 will as zero.

> this may not be true on some platforms, so it's possible no
> irq(s) for the unavailable channel(s). What's more, the available
> channels may not be continuous. To handle this case, we'd better
> get the irq of each channel by name.

Unless you're suggesting these channels physically do not have their 
IRQs wired up to anything at all, what's wrong with describing the 
hardware in DT? Linux won't request IRQs for channels it isn't actually 
using, and nor should any other reasonable DT consumer - precisely 
because channels *can* be arbitrarily taken by the Secure world, and 
that can be entirely dynamic based on firmware configuration even for a 
single hardware platform.

And even in the weird case that some channel literally has no IRQ, my 
thought was we'd just have a placeholder entry in the array, such that 
attempting to request it would fail.

Thanks,
Robin.

> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>   Documentation/devicetree/bindings/dma/arm,dma-350.yaml | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> index 429f682f15d8..94752516e51a 100644
> --- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> +++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> @@ -32,6 +32,10 @@ properties:
>         - description: Channel 6 interrupt
>         - description: Channel 7 interrupt
>   
> +  interrupt-names:
> +    minItems: 1
> +    maxItems: 8
> +
>     "#dma-cells":
>       const: 1
>       description: The cell is the trigger input number
> @@ -40,5 +44,6 @@ required:
>     - compatible
>     - reg
>     - interrupts
> +  - interrupt-names
>   
>   unevaluatedProperties: false

[PATCH 09/14] dmaengine: dma350: Support dma-channel-mask

[Jisheng Zhang]

Not all channels are available to kernel, we need to support
dma-channel-mask.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/dma/arm-dma350.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 6a6d1c2a3ee6..72067518799e 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -534,7 +534,7 @@ static int d350_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct d350 *dmac;
 	void __iomem *base;
-	u32 reg;
+	u32 reg, dma_chan_mask;
 	int ret, nchan, dw, aw, r, p;
 	bool coherent, memset;
 
@@ -563,6 +563,15 @@ static int d350_probe(struct platform_device *pdev)
 
 	dmac->nchan = nchan;
 
+	/* Enable all channels by default */
+	dma_chan_mask = nchan - 1;
+
+	ret = of_property_read_u32(dev->of_node, "dma-channel-mask", &dma_chan_mask);
+	if (ret < 0 && (ret != -EINVAL)) {
+		dev_err(&pdev->dev, "dma-channel-mask is not complete.\n");
+		return ret;
+	}
+
 	reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG1);
 	dmac->nreq = FIELD_GET(DMA_CFG_NUM_TRIGGER_IN, reg);
 
@@ -592,6 +601,11 @@ static int d350_probe(struct platform_device *pdev)
 	memset = true;
 	for (int i = 0; i < nchan; i++) {
 		struct d350_chan *dch = &dmac->channels[i];
+		char ch_irqname[8];
+
+		/* skip for reserved channels */
+		if (!test_bit(i, (unsigned long *)&dma_chan_mask))
+			continue;
 
 		dch->coherent = coherent;
 		dch->base = base + DMACH(i);
@@ -602,7 +616,9 @@ 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);
+
+		snprintf(ch_irqname, sizeof(ch_irqname), "ch%d", i);
+		dch->irq = platform_get_irq_byname(pdev, ch_irqname);
 		if (dch->irq < 0)
 			return dch->irq;
 
-- 
2.50.0

Re: [PATCH 09/14] dmaengine: dma350: Support dma-channel-mask

[Krzysztof Kozlowski]

On 23/08/2025 17:40, Jisheng Zhang wrote:
> Not all channels are available to kernel, we need to support
> dma-channel-mask.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>  drivers/dma/arm-dma350.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index 6a6d1c2a3ee6..72067518799e 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c
> @@ -534,7 +534,7 @@ static int d350_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct d350 *dmac;
>  	void __iomem *base;
> -	u32 reg;
> +	u32 reg, dma_chan_mask;
>  	int ret, nchan, dw, aw, r, p;
>  	bool coherent, memset;
>  
> @@ -563,6 +563,15 @@ static int d350_probe(struct platform_device *pdev)
>  
>  	dmac->nchan = nchan;
>  
> +	/* Enable all channels by default */
> +	dma_chan_mask = nchan - 1;
> +
> +	ret = of_property_read_u32(dev->of_node, "dma-channel-mask", &dma_chan_mask);
> +	if (ret < 0 && (ret != -EINVAL)) {
> +		dev_err(&pdev->dev, "dma-channel-mask is not complete.\n");
> +		return ret;
> +	}
> +
>  	reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG1);
>  	dmac->nreq = FIELD_GET(DMA_CFG_NUM_TRIGGER_IN, reg);
>  
> @@ -592,6 +601,11 @@ static int d350_probe(struct platform_device *pdev)
>  	memset = true;
>  	for (int i = 0; i < nchan; i++) {
>  		struct d350_chan *dch = &dmac->channels[i];
> +		char ch_irqname[8];
> +
> +		/* skip for reserved channels */
> +		if (!test_bit(i, (unsigned long *)&dma_chan_mask))
> +			continue;
>  
>  		dch->coherent = coherent;
>  		dch->base = base + DMACH(i);
> @@ -602,7 +616,9 @@ 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);
> +
> +		snprintf(ch_irqname, sizeof(ch_irqname), "ch%d", i);
> +		dch->irq = platform_get_irq_byname(pdev, ch_irqname);

Actual ABI break.

That's a no-go, sorry. You cannot decide to break all users just because
"Not all channels are available to the kernel". That's really, really
incomplete ABI breakage reasoning.

See also writing bindings doc.

Best regards,
Krzysztof

Re: [PATCH 09/14] dmaengine: dma350: Support dma-channel-mask

[kernel test robot]

Hi Jisheng,

kernel test robot noticed the following build warnings:

[auto build test WARNING on vkoul-dmaengine/next]
[also build test WARNING on robh/for-next linus/master v6.17-rc2 next-20250822]
[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/Jisheng-Zhang/dmaengine-dma350-Fix-CH_CTRL_USESRCTRIGIN-definition/20250824-000425
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git next
patch link:    https://lore.kernel.org/r/20250823154009.25992-10-jszhang%40kernel.org
patch subject: [PATCH 09/14] dmaengine: dma350: Support dma-channel-mask
config: arm64-randconfig-002-20250824 (https://download.01.org/0day-ci/archive/20250824/202508241415.b7kiTLel-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 11.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250824/202508241415.b7kiTLel-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/202508241415.b7kiTLel-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/dma/arm-dma350.c: In function 'd350_probe':
>> drivers/dma/arm-dma350.c:620:61: warning: '%d' directive output may be truncated writing between 1 and 10 bytes into a region of size 6 [-Wformat-truncation=]
     620 |                 snprintf(ch_irqname, sizeof(ch_irqname), "ch%d", i);
         |                                                             ^~
   drivers/dma/arm-dma350.c:620:58: note: directive argument in the range [0, 2147483646]
     620 |                 snprintf(ch_irqname, sizeof(ch_irqname), "ch%d", i);
         |                                                          ^~~~~~
   drivers/dma/arm-dma350.c:620:17: note: 'snprintf' output between 4 and 13 bytes into a destination of size 8
     620 |                 snprintf(ch_irqname, sizeof(ch_irqname), "ch%d", i);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +620 drivers/dma/arm-dma350.c

   531	
   532	static int d350_probe(struct platform_device *pdev)
   533	{
   534		struct device *dev = &pdev->dev;
   535		struct d350 *dmac;
   536		void __iomem *base;
   537		u32 reg, dma_chan_mask;
   538		int ret, nchan, dw, aw, r, p;
   539		bool coherent, memset;
   540	
   541		base = devm_platform_ioremap_resource(pdev, 0);
   542		if (IS_ERR(base))
   543			return PTR_ERR(base);
   544	
   545		reg = readl_relaxed(base + DMAINFO + IIDR);
   546		r = FIELD_GET(IIDR_VARIANT, reg);
   547		p = FIELD_GET(IIDR_REVISION, reg);
   548		if (FIELD_GET(IIDR_IMPLEMENTER, reg) != IMPLEMENTER_ARM ||
   549		    FIELD_GET(IIDR_PRODUCTID, reg) != PRODUCTID_DMA350)
   550			return dev_err_probe(dev, -ENODEV, "Not a DMA-350!");
   551	
   552		reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG0);
   553		nchan = FIELD_GET(DMA_CFG_NUM_CHANNELS, reg) + 1;
   554		dw = 1 << FIELD_GET(DMA_CFG_DATA_WIDTH, reg);
   555		aw = FIELD_GET(DMA_CFG_ADDR_WIDTH, reg) + 1;
   556	
   557		dma_set_mask_and_coherent(dev, DMA_BIT_MASK(aw));
   558		coherent = device_get_dma_attr(dev) == DEV_DMA_COHERENT;
   559	
   560		dmac = devm_kzalloc(dev, struct_size(dmac, channels, nchan), GFP_KERNEL);
   561		if (!dmac)
   562			return -ENOMEM;
   563	
   564		dmac->nchan = nchan;
   565	
   566		/* Enable all channels by default */
   567		dma_chan_mask = nchan - 1;
   568	
   569		ret = of_property_read_u32(dev->of_node, "dma-channel-mask", &dma_chan_mask);
   570		if (ret < 0 && (ret != -EINVAL)) {
   571			dev_err(&pdev->dev, "dma-channel-mask is not complete.\n");
   572			return ret;
   573		}
   574	
   575		reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG1);
   576		dmac->nreq = FIELD_GET(DMA_CFG_NUM_TRIGGER_IN, reg);
   577	
   578		dev_dbg(dev, "DMA-350 r%dp%d with %d channels, %d requests\n", r, p, dmac->nchan, dmac->nreq);
   579	
   580		dmac->dma.dev = dev;
   581		for (int i = min(dw, 16); i > 0; i /= 2) {
   582			dmac->dma.src_addr_widths |= BIT(i);
   583			dmac->dma.dst_addr_widths |= BIT(i);
   584		}
   585		dmac->dma.directions = BIT(DMA_MEM_TO_MEM);
   586		dmac->dma.descriptor_reuse = true;
   587		dmac->dma.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
   588		dmac->dma.device_alloc_chan_resources = d350_alloc_chan_resources;
   589		dmac->dma.device_free_chan_resources = d350_free_chan_resources;
   590		dma_cap_set(DMA_MEMCPY, dmac->dma.cap_mask);
   591		dmac->dma.device_prep_dma_memcpy = d350_prep_memcpy;
   592		dmac->dma.device_pause = d350_pause;
   593		dmac->dma.device_resume = d350_resume;
   594		dmac->dma.device_terminate_all = d350_terminate_all;
   595		dmac->dma.device_synchronize = d350_synchronize;
   596		dmac->dma.device_tx_status = d350_tx_status;
   597		dmac->dma.device_issue_pending = d350_issue_pending;
   598		INIT_LIST_HEAD(&dmac->dma.channels);
   599	
   600		/* Would be nice to have per-channel caps for this... */
   601		memset = true;
   602		for (int i = 0; i < nchan; i++) {
   603			struct d350_chan *dch = &dmac->channels[i];
   604			char ch_irqname[8];
   605	
   606			/* skip for reserved channels */
   607			if (!test_bit(i, (unsigned long *)&dma_chan_mask))
   608				continue;
   609	
   610			dch->coherent = coherent;
   611			dch->base = base + DMACH(i);
   612			writel_relaxed(CH_CMD_CLEAR, dch->base + CH_CMD);
   613	
   614			reg = readl_relaxed(dch->base + CH_BUILDCFG1);
   615			if (!(FIELD_GET(CH_CFG_HAS_CMDLINK, reg))) {
   616				dev_warn(dev, "No command link support on channel %d\n", i);
   617				continue;
   618			}
   619	
 > 620			snprintf(ch_irqname, sizeof(ch_irqname), "ch%d", i);
   621			dch->irq = platform_get_irq_byname(pdev, ch_irqname);
   622			if (dch->irq < 0)
   623				return dch->irq;
   624	
   625			dch->has_wrap = FIELD_GET(CH_CFG_HAS_WRAP, reg);
   626			dch->has_trig = FIELD_GET(CH_CFG_HAS_TRIGIN, reg) &
   627					FIELD_GET(CH_CFG_HAS_TRIGSEL, reg);
   628	
   629			/* Fill is a special case of Wrap */
   630			memset &= dch->has_wrap;
   631	
   632			reg = readl_relaxed(dch->base + CH_BUILDCFG0);
   633			dch->tsz = FIELD_GET(CH_CFG_DATA_WIDTH, reg);
   634	
   635			reg = FIELD_PREP(CH_LINK_SHAREATTR, coherent ? SHAREATTR_ISH : SHAREATTR_OSH);
   636			reg |= FIELD_PREP(CH_LINK_MEMATTR, coherent ? MEMATTR_WB : MEMATTR_NC);
   637			writel_relaxed(reg, dch->base + CH_LINKATTR);
   638	
   639			dch->vc.desc_free = d350_desc_free;
   640			vchan_init(&dch->vc, &dmac->dma);
   641		}
   642	
   643		if (memset) {
   644			dma_cap_set(DMA_MEMSET, dmac->dma.cap_mask);
   645			dmac->dma.device_prep_dma_memset = d350_prep_memset;
   646		}
   647	
   648		platform_set_drvdata(pdev, dmac);
   649	
   650		ret = dmaenginem_async_device_register(&dmac->dma);
   651		if (ret)
   652			return dev_err_probe(dev, ret, "Failed to register DMA device\n");
   653	
   654		return of_dma_controller_register(dev->of_node, of_dma_xlate_by_chan_id, &dmac->dma);
   655	}
   656	

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

[PATCH 10/14] dmaengine: dma350: Alloc command[] from dma pool

[Jisheng Zhang]

Currently, the command[] is allocated with kzalloc(), but dma350 may be
used on dma-non-coherent platforms, to prepare the support of peripheral
and scatter-gather chaining on both dma-coherent and dma-non-coherent
platforms, let's alloc them from dma pool.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/dma/arm-dma350.c | 143 +++++++++++++++++++++++++++++++--------
 1 file changed, 113 insertions(+), 30 deletions(-)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 72067518799e..3d26a1f020df 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -4,6 +4,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/dmaengine.h>
+#include <linux/dmapool.h>
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
 #include <linux/of.h>
@@ -143,6 +144,7 @@
 #define LINK_LINKADDR		BIT(30)
 #define LINK_LINKADDRHI		BIT(31)
 
+#define D350_MAX_CMDS		16
 
 enum ch_ctrl_donetype {
 	CH_CTRL_DONETYPE_NONE = 0,
@@ -169,18 +171,25 @@ enum ch_cfg_memattr {
 	MEMATTR_WB = 0xff
 };
 
-struct d350_desc {
-	struct virt_dma_desc vd;
-	u32 command[16];
+struct d350_sg {
+	u32 *command;
+	dma_addr_t phys;
 	u16 xsize;
 	u16 xsizehi;
 	u8 tsz;
 };
 
+struct d350_desc {
+	struct virt_dma_desc vd;
+	u32 sglen;
+	struct d350_sg sg[] __counted_by(sglen);
+};
+
 struct d350_chan {
 	struct virt_dma_chan vc;
 	struct d350_desc *desc;
 	void __iomem *base;
+	struct dma_pool *cmd_pool;
 	int irq;
 	enum dma_status status;
 	dma_cookie_t cookie;
@@ -210,7 +219,14 @@ static inline struct d350_desc *to_d350_desc(struct virt_dma_desc *vd)
 
 static void d350_desc_free(struct virt_dma_desc *vd)
 {
-	kfree(to_d350_desc(vd));
+	struct d350_chan *dch = to_d350_chan(vd->tx.chan);
+	struct d350_desc *desc = to_d350_desc(vd);
+	int i;
+
+	for (i = 0; i < desc->sglen; i++)
+		dma_pool_free(dch->cmd_pool, desc->sg[i].command, desc->sg[i].phys);
+
+	kfree(desc);
 }
 
 static struct dma_async_tx_descriptor *d350_prep_memcpy(struct dma_chan *chan,
@@ -218,22 +234,32 @@ static struct dma_async_tx_descriptor *d350_prep_memcpy(struct dma_chan *chan,
 {
 	struct d350_chan *dch = to_d350_chan(chan);
 	struct d350_desc *desc;
+	struct d350_sg *sg;
+	dma_addr_t phys;
 	u32 *cmd;
 
-	desc = kzalloc(sizeof(*desc), GFP_NOWAIT);
+	desc = kzalloc(struct_size(desc, sg, 1), GFP_NOWAIT);
 	if (!desc)
 		return NULL;
 
-	desc->tsz = __ffs(len | dest | src | (1 << dch->tsz));
-	desc->xsize = lower_16_bits(len >> desc->tsz);
-	desc->xsizehi = upper_16_bits(len >> desc->tsz);
+	sg = &desc->sg[0];
+	sg->command = dma_pool_zalloc(dch->cmd_pool, GFP_NOWAIT, &phys);
+	if (unlikely(!sg->command)) {
+		kfree(desc);
+		return NULL;
+	}
+	sg->phys = phys;
+
+	sg->tsz = __ffs(len | dest | src | (1 << dch->tsz));
+	sg->xsize = lower_16_bits(len >> sg->tsz);
+	sg->xsizehi = upper_16_bits(len >> sg->tsz);
 
-	cmd = desc->command;
+	cmd = sg->command;
 	cmd[0] = LINK_CTRL | LINK_SRCADDR | LINK_SRCADDRHI | LINK_DESADDR |
 		 LINK_DESADDRHI | LINK_XSIZE | LINK_XSIZEHI | LINK_SRCTRANSCFG |
 		 LINK_DESTRANSCFG | LINK_XADDRINC | LINK_LINKADDR;
 
-	cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, desc->tsz) |
+	cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, sg->tsz) |
 		 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_CONTINUE) |
 		 FIELD_PREP(CH_CTRL_DONETYPE, CH_CTRL_DONETYPE_CMD);
 
@@ -241,13 +267,15 @@ static struct dma_async_tx_descriptor *d350_prep_memcpy(struct dma_chan *chan,
 	cmd[3] = upper_32_bits(src);
 	cmd[4] = lower_32_bits(dest);
 	cmd[5] = upper_32_bits(dest);
-	cmd[6] = FIELD_PREP(CH_XY_SRC, desc->xsize) | FIELD_PREP(CH_XY_DES, desc->xsize);
-	cmd[7] = FIELD_PREP(CH_XY_SRC, desc->xsizehi) | FIELD_PREP(CH_XY_DES, desc->xsizehi);
+	cmd[6] = FIELD_PREP(CH_XY_SRC, sg->xsize) | FIELD_PREP(CH_XY_DES, sg->xsize);
+	cmd[7] = FIELD_PREP(CH_XY_SRC, sg->xsizehi) | FIELD_PREP(CH_XY_DES, sg->xsizehi);
 	cmd[8] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
 	cmd[9] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
 	cmd[10] = FIELD_PREP(CH_XY_SRC, 1) | FIELD_PREP(CH_XY_DES, 1);
 	cmd[11] = 0;
 
+	mb();
+
 	return vchan_tx_prep(&dch->vc, &desc->vd, flags);
 }
 
@@ -256,34 +284,46 @@ static struct dma_async_tx_descriptor *d350_prep_memset(struct dma_chan *chan,
 {
 	struct d350_chan *dch = to_d350_chan(chan);
 	struct d350_desc *desc;
+	struct d350_sg *sg;
+	dma_addr_t phys;
 	u32 *cmd;
 
-	desc = kzalloc(sizeof(*desc), GFP_NOWAIT);
+	desc = kzalloc(struct_size(desc, sg, 1), GFP_NOWAIT);
 	if (!desc)
 		return NULL;
 
-	desc->tsz = __ffs(len | dest | (1 << dch->tsz));
-	desc->xsize = lower_16_bits(len >> desc->tsz);
-	desc->xsizehi = upper_16_bits(len >> desc->tsz);
+	sg = &desc->sg[0];
+	sg->command = dma_pool_zalloc(dch->cmd_pool, GFP_NOWAIT, &phys);
+	if (unlikely(!sg->command)) {
+		kfree(desc);
+		return NULL;
+	}
+	sg->phys = phys;
+
+	sg->tsz = __ffs(len | dest | (1 << dch->tsz));
+	sg->xsize = lower_16_bits(len >> sg->tsz);
+	sg->xsizehi = upper_16_bits(len >> sg->tsz);
 
-	cmd = desc->command;
+	cmd = sg->command;
 	cmd[0] = LINK_CTRL | LINK_DESADDR | LINK_DESADDRHI |
 		 LINK_XSIZE | LINK_XSIZEHI | LINK_DESTRANSCFG |
 		 LINK_XADDRINC | LINK_FILLVAL | LINK_LINKADDR;
 
-	cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, desc->tsz) |
+	cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, sg->tsz) |
 		 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_FILL) |
 		 FIELD_PREP(CH_CTRL_DONETYPE, CH_CTRL_DONETYPE_CMD);
 
 	cmd[2] = lower_32_bits(dest);
 	cmd[3] = upper_32_bits(dest);
-	cmd[4] = FIELD_PREP(CH_XY_DES, desc->xsize);
-	cmd[5] = FIELD_PREP(CH_XY_DES, desc->xsizehi);
+	cmd[4] = FIELD_PREP(CH_XY_DES, sg->xsize);
+	cmd[5] = FIELD_PREP(CH_XY_DES, sg->xsizehi);
 	cmd[6] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
 	cmd[7] = FIELD_PREP(CH_XY_DES, 1);
 	cmd[8] = (u8)value * 0x01010101;
 	cmd[9] = 0;
 
+	mb();
+
 	return vchan_tx_prep(&dch->vc, &desc->vd, flags);
 }
 
@@ -319,8 +359,9 @@ static int d350_resume(struct dma_chan *chan)
 
 static u32 d350_get_residue(struct d350_chan *dch)
 {
-	u32 res, xsize, xsizehi, hi_new;
-	int retries = 3; /* 1st time unlucky, 2nd improbable, 3rd just broken */
+	u32 res, xsize, xsizehi, linkaddr, linkaddrhi, hi_new;
+	int i, sgcur, retries = 3; /* 1st time unlucky, 2nd improbable, 3rd just broken */
+	struct d350_desc *desc = dch->desc;
 
 	hi_new = readl_relaxed(dch->base + CH_XSIZEHI);
 	do {
@@ -329,10 +370,26 @@ static u32 d350_get_residue(struct d350_chan *dch)
 		hi_new = readl_relaxed(dch->base + CH_XSIZEHI);
 	} while (xsizehi != hi_new && --retries);
 
+	hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
+	do {
+		linkaddrhi = hi_new;
+		linkaddr = readl_relaxed(dch->base + CH_LINKADDR);
+		hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
+	} while (linkaddrhi != hi_new && --retries);
+
+	for (i = 0; i < desc->sglen; i++) {
+		if (desc->sg[i].phys == (((u64)linkaddrhi << 32) | (linkaddr & ~CH_LINKADDR_EN)))
+			sgcur = i;
+	}
+
 	res = FIELD_GET(CH_XY_DES, xsize);
 	res |= FIELD_GET(CH_XY_DES, xsizehi) << 16;
+	res <<= desc->sg[sgcur].tsz;
+
+	for (i = sgcur + 1; i < desc->sglen; i++)
+		res += (((u32)desc->sg[i].xsizehi << 16 | desc->sg[i].xsize) << desc->sg[i].tsz);
 
-	return res << dch->desc->tsz;
+	return res;
 }
 
 static int d350_terminate_all(struct dma_chan *chan)
@@ -365,7 +422,13 @@ static void d350_synchronize(struct dma_chan *chan)
 
 static u32 d350_desc_bytes(struct d350_desc *desc)
 {
-	return ((u32)desc->xsizehi << 16 | desc->xsize) << desc->tsz;
+	int i;
+	u32 bytes = 0;
+
+	for (i = 0; i < desc->sglen; i++)
+		bytes += (((u32)desc->sg[i].xsizehi << 16 | desc->sg[i].xsize) << desc->sg[i].tsz);
+
+	return bytes;
 }
 
 static enum dma_status d350_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
@@ -415,8 +478,8 @@ static void d350_start_next(struct d350_chan *dch)
 	dch->cookie = dch->desc->vd.tx.cookie;
 	dch->residue = d350_desc_bytes(dch->desc);
 
-	hdr = dch->desc->command[0];
-	reg = &dch->desc->command[1];
+	hdr = dch->desc->sg[0].command[0];
+	reg = &dch->desc->sg[0].command[1];
 
 	if (hdr & LINK_INTREN)
 		writel_relaxed(*reg++, dch->base + CH_INTREN);
@@ -512,11 +575,29 @@ 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;
+
+	dch->cmd_pool = dma_pool_create(dma_chan_name(chan),
+					  chan->device->dev,
+					  D350_MAX_CMDS * sizeof(u32),
+					  sizeof(u32), 0);
+	if (!dch->cmd_pool) {
+		dev_err(chan->device->dev, "No memory for cmd pool\n");
+		return -ENOMEM;
+	}
+
+	ret = request_irq(dch->irq, d350_irq, 0,
+			  dev_name(&dch->vc.chan.dev->device), dch);
+	if (ret < 0)
+		goto err_irq;
+
+	writel_relaxed(CH_INTREN_DONE | CH_INTREN_ERR, dch->base + CH_INTREN);
+
+	return 0;
 
+err_irq:
+	dma_pool_destroy(dch->cmd_pool);
+	dch->cmd_pool = NULL;
 	return ret;
 }
 
@@ -527,6 +608,8 @@ static void d350_free_chan_resources(struct dma_chan *chan)
 	writel_relaxed(0, dch->base + CH_INTREN);
 	free_irq(dch->irq, dch);
 	vchan_free_chan_resources(&dch->vc);
+	dma_pool_destroy(dch->cmd_pool);
+	dch->cmd_pool = NULL;
 }
 
 static int d350_probe(struct platform_device *pdev)
-- 
2.50.0

Re: [PATCH 10/14] dmaengine: dma350: Alloc command[] from dma pool

[Robin Murphy]

On 2025-08-23 4:40 pm, Jisheng Zhang wrote:
> Currently, the command[] is allocated with kzalloc(), but dma350 may be
> used on dma-non-coherent platforms, to prepare the support of peripheral
> and scatter-gather chaining on both dma-coherent and dma-non-coherent
> platforms, let's alloc them from dma pool.

FWIW my plan was to use dma_map_single() for command linking, since the 
descriptors themselves are short-lived one-way data transfers which 
really don't need any of the (potentially costly) properties of a 
dma-coherent allocation.

> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>   drivers/dma/arm-dma350.c | 143 +++++++++++++++++++++++++++++++--------
>   1 file changed, 113 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index 72067518799e..3d26a1f020df 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c
> @@ -4,6 +4,7 @@
>   
>   #include <linux/bitfield.h>
>   #include <linux/dmaengine.h>
> +#include <linux/dmapool.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/io.h>
>   #include <linux/of.h>
> @@ -143,6 +144,7 @@
>   #define LINK_LINKADDR		BIT(30)
>   #define LINK_LINKADDRHI		BIT(31)
>   
> +#define D350_MAX_CMDS		16

What's that based on? We should be able to link arbitrarily-long chains 
of commands, no?

>   enum ch_ctrl_donetype {
>   	CH_CTRL_DONETYPE_NONE = 0,
> @@ -169,18 +171,25 @@ enum ch_cfg_memattr {
>   	MEMATTR_WB = 0xff
>   };
>   
> -struct d350_desc {
> -	struct virt_dma_desc vd;
> -	u32 command[16];
> +struct d350_sg {
> +	u32 *command;
> +	dma_addr_t phys;
>   	u16 xsize;
>   	u16 xsizehi;
>   	u8 tsz;
>   };
>   
> +struct d350_desc {
> +	struct virt_dma_desc vd;
> +	u32 sglen;
> +	struct d350_sg sg[] __counted_by(sglen);
> +};

Perhaps it's mostly the naming, but this seems rather hard to make sense 
of. To clarify, the current driver design was in anticipation of a split 
more like so:

struct d350_cmd {
	u32 command[16];
	u16 xsize;
	u16 xsizehi;
	u8 tsz;
	struct d350_cmd *next;
};

struct d350_desc {
	struct virt_dma_desc vd;
	// any totals etc. to help with residue calculation
	struct d350_cmd cmd;
};

Or perhaps what I'd more likely have ended up with (which is maybe sort 
of what you've tried to do?):

struct d350_cmd {
	u32 command[16];
	u16 xsize;
	u16 xsizehi;
	u8 tsz;
};

struct d350_desc {
	struct virt_dma_desc vd;
	// anything else as above
	int num_cmds;
	struct d350_cmd cmd[1]; //extensible
};

Conveniently taking advantage of the fact that either way the DMA 
address will inherently be stored in the LINKADDR fields of the first 
command (which doesn't need DMA mapping itself), so at worst we still 
only need one or two allocations plus a single dma_map per prep 
operation (since we can keep all the linked commands as a single block). 
I don't see a DMA pool approach being beneficial here, since it seems 
like it's always going to be considerably less efficient.

(Not to mention that separate pools per channel is complete overkill 
anyway.)

Thanks,
Robin.

> +
>   struct d350_chan {
>   	struct virt_dma_chan vc;
>   	struct d350_desc *desc;
>   	void __iomem *base;
> +	struct dma_pool *cmd_pool;
>   	int irq;
>   	enum dma_status status;
>   	dma_cookie_t cookie;
> @@ -210,7 +219,14 @@ static inline struct d350_desc *to_d350_desc(struct virt_dma_desc *vd)
>   
>   static void d350_desc_free(struct virt_dma_desc *vd)
>   {
> -	kfree(to_d350_desc(vd));
> +	struct d350_chan *dch = to_d350_chan(vd->tx.chan);
> +	struct d350_desc *desc = to_d350_desc(vd);
> +	int i;
> +
> +	for (i = 0; i < desc->sglen; i++)
> +		dma_pool_free(dch->cmd_pool, desc->sg[i].command, desc->sg[i].phys);
> +
> +	kfree(desc);
>   }
>   
>   static struct dma_async_tx_descriptor *d350_prep_memcpy(struct dma_chan *chan,
> @@ -218,22 +234,32 @@ static struct dma_async_tx_descriptor *d350_prep_memcpy(struct dma_chan *chan,
>   {
>   	struct d350_chan *dch = to_d350_chan(chan);
>   	struct d350_desc *desc;
> +	struct d350_sg *sg;
> +	dma_addr_t phys;
>   	u32 *cmd;
>   
> -	desc = kzalloc(sizeof(*desc), GFP_NOWAIT);
> +	desc = kzalloc(struct_size(desc, sg, 1), GFP_NOWAIT);
>   	if (!desc)
>   		return NULL;
>   
> -	desc->tsz = __ffs(len | dest | src | (1 << dch->tsz));
> -	desc->xsize = lower_16_bits(len >> desc->tsz);
> -	desc->xsizehi = upper_16_bits(len >> desc->tsz);
> +	sg = &desc->sg[0];
> +	sg->command = dma_pool_zalloc(dch->cmd_pool, GFP_NOWAIT, &phys);
> +	if (unlikely(!sg->command)) {
> +		kfree(desc);
> +		return NULL;
> +	}
> +	sg->phys = phys;
> +
> +	sg->tsz = __ffs(len | dest | src | (1 << dch->tsz));
> +	sg->xsize = lower_16_bits(len >> sg->tsz);
> +	sg->xsizehi = upper_16_bits(len >> sg->tsz);
>   
> -	cmd = desc->command;
> +	cmd = sg->command;
>   	cmd[0] = LINK_CTRL | LINK_SRCADDR | LINK_SRCADDRHI | LINK_DESADDR |
>   		 LINK_DESADDRHI | LINK_XSIZE | LINK_XSIZEHI | LINK_SRCTRANSCFG |
>   		 LINK_DESTRANSCFG | LINK_XADDRINC | LINK_LINKADDR;
>   
> -	cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, desc->tsz) |
> +	cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, sg->tsz) |
>   		 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_CONTINUE) |
>   		 FIELD_PREP(CH_CTRL_DONETYPE, CH_CTRL_DONETYPE_CMD);
>   
> @@ -241,13 +267,15 @@ static struct dma_async_tx_descriptor *d350_prep_memcpy(struct dma_chan *chan,
>   	cmd[3] = upper_32_bits(src);
>   	cmd[4] = lower_32_bits(dest);
>   	cmd[5] = upper_32_bits(dest);
> -	cmd[6] = FIELD_PREP(CH_XY_SRC, desc->xsize) | FIELD_PREP(CH_XY_DES, desc->xsize);
> -	cmd[7] = FIELD_PREP(CH_XY_SRC, desc->xsizehi) | FIELD_PREP(CH_XY_DES, desc->xsizehi);
> +	cmd[6] = FIELD_PREP(CH_XY_SRC, sg->xsize) | FIELD_PREP(CH_XY_DES, sg->xsize);
> +	cmd[7] = FIELD_PREP(CH_XY_SRC, sg->xsizehi) | FIELD_PREP(CH_XY_DES, sg->xsizehi);
>   	cmd[8] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
>   	cmd[9] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
>   	cmd[10] = FIELD_PREP(CH_XY_SRC, 1) | FIELD_PREP(CH_XY_DES, 1);
>   	cmd[11] = 0;
>   
> +	mb();
> +
>   	return vchan_tx_prep(&dch->vc, &desc->vd, flags);
>   }
>   
> @@ -256,34 +284,46 @@ static struct dma_async_tx_descriptor *d350_prep_memset(struct dma_chan *chan,
>   {
>   	struct d350_chan *dch = to_d350_chan(chan);
>   	struct d350_desc *desc;
> +	struct d350_sg *sg;
> +	dma_addr_t phys;
>   	u32 *cmd;
>   
> -	desc = kzalloc(sizeof(*desc), GFP_NOWAIT);
> +	desc = kzalloc(struct_size(desc, sg, 1), GFP_NOWAIT);
>   	if (!desc)
>   		return NULL;
>   
> -	desc->tsz = __ffs(len | dest | (1 << dch->tsz));
> -	desc->xsize = lower_16_bits(len >> desc->tsz);
> -	desc->xsizehi = upper_16_bits(len >> desc->tsz);
> +	sg = &desc->sg[0];
> +	sg->command = dma_pool_zalloc(dch->cmd_pool, GFP_NOWAIT, &phys);
> +	if (unlikely(!sg->command)) {
> +		kfree(desc);
> +		return NULL;
> +	}
> +	sg->phys = phys;
> +
> +	sg->tsz = __ffs(len | dest | (1 << dch->tsz));
> +	sg->xsize = lower_16_bits(len >> sg->tsz);
> +	sg->xsizehi = upper_16_bits(len >> sg->tsz);
>   
> -	cmd = desc->command;
> +	cmd = sg->command;
>   	cmd[0] = LINK_CTRL | LINK_DESADDR | LINK_DESADDRHI |
>   		 LINK_XSIZE | LINK_XSIZEHI | LINK_DESTRANSCFG |
>   		 LINK_XADDRINC | LINK_FILLVAL | LINK_LINKADDR;
>   
> -	cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, desc->tsz) |
> +	cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, sg->tsz) |
>   		 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_FILL) |
>   		 FIELD_PREP(CH_CTRL_DONETYPE, CH_CTRL_DONETYPE_CMD);
>   
>   	cmd[2] = lower_32_bits(dest);
>   	cmd[3] = upper_32_bits(dest);
> -	cmd[4] = FIELD_PREP(CH_XY_DES, desc->xsize);
> -	cmd[5] = FIELD_PREP(CH_XY_DES, desc->xsizehi);
> +	cmd[4] = FIELD_PREP(CH_XY_DES, sg->xsize);
> +	cmd[5] = FIELD_PREP(CH_XY_DES, sg->xsizehi);
>   	cmd[6] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
>   	cmd[7] = FIELD_PREP(CH_XY_DES, 1);
>   	cmd[8] = (u8)value * 0x01010101;
>   	cmd[9] = 0;
>   
> +	mb();
> +
>   	return vchan_tx_prep(&dch->vc, &desc->vd, flags);
>   }
>   
> @@ -319,8 +359,9 @@ static int d350_resume(struct dma_chan *chan)
>   
>   static u32 d350_get_residue(struct d350_chan *dch)
>   {
> -	u32 res, xsize, xsizehi, hi_new;
> -	int retries = 3; /* 1st time unlucky, 2nd improbable, 3rd just broken */
> +	u32 res, xsize, xsizehi, linkaddr, linkaddrhi, hi_new;
> +	int i, sgcur, retries = 3; /* 1st time unlucky, 2nd improbable, 3rd just broken */
> +	struct d350_desc *desc = dch->desc;
>   
>   	hi_new = readl_relaxed(dch->base + CH_XSIZEHI);
>   	do {
> @@ -329,10 +370,26 @@ static u32 d350_get_residue(struct d350_chan *dch)
>   		hi_new = readl_relaxed(dch->base + CH_XSIZEHI);
>   	} while (xsizehi != hi_new && --retries);
>   
> +	hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
> +	do {
> +		linkaddrhi = hi_new;
> +		linkaddr = readl_relaxed(dch->base + CH_LINKADDR);
> +		hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
> +	} while (linkaddrhi != hi_new && --retries);
> +
> +	for (i = 0; i < desc->sglen; i++) {
> +		if (desc->sg[i].phys == (((u64)linkaddrhi << 32) | (linkaddr & ~CH_LINKADDR_EN)))
> +			sgcur = i;
> +	}
> +
>   	res = FIELD_GET(CH_XY_DES, xsize);
>   	res |= FIELD_GET(CH_XY_DES, xsizehi) << 16;
> +	res <<= desc->sg[sgcur].tsz;
> +
> +	for (i = sgcur + 1; i < desc->sglen; i++)
> +		res += (((u32)desc->sg[i].xsizehi << 16 | desc->sg[i].xsize) << desc->sg[i].tsz);
>   
> -	return res << dch->desc->tsz;
> +	return res;
>   }
>   
>   static int d350_terminate_all(struct dma_chan *chan)
> @@ -365,7 +422,13 @@ static void d350_synchronize(struct dma_chan *chan)
>   
>   static u32 d350_desc_bytes(struct d350_desc *desc)
>   {
> -	return ((u32)desc->xsizehi << 16 | desc->xsize) << desc->tsz;
> +	int i;
> +	u32 bytes = 0;
> +
> +	for (i = 0; i < desc->sglen; i++)
> +		bytes += (((u32)desc->sg[i].xsizehi << 16 | desc->sg[i].xsize) << desc->sg[i].tsz);
> +
> +	return bytes;
>   }
>   
>   static enum dma_status d350_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
> @@ -415,8 +478,8 @@ static void d350_start_next(struct d350_chan *dch)
>   	dch->cookie = dch->desc->vd.tx.cookie;
>   	dch->residue = d350_desc_bytes(dch->desc);
>   
> -	hdr = dch->desc->command[0];
> -	reg = &dch->desc->command[1];
> +	hdr = dch->desc->sg[0].command[0];
> +	reg = &dch->desc->sg[0].command[1];
>   
>   	if (hdr & LINK_INTREN)
>   		writel_relaxed(*reg++, dch->base + CH_INTREN);
> @@ -512,11 +575,29 @@ 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;
> +
> +	dch->cmd_pool = dma_pool_create(dma_chan_name(chan),
> +					  chan->device->dev,
> +					  D350_MAX_CMDS * sizeof(u32),
> +					  sizeof(u32), 0);
> +	if (!dch->cmd_pool) {
> +		dev_err(chan->device->dev, "No memory for cmd pool\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = request_irq(dch->irq, d350_irq, 0,
> +			  dev_name(&dch->vc.chan.dev->device), dch);
> +	if (ret < 0)
> +		goto err_irq;
> +
> +	writel_relaxed(CH_INTREN_DONE | CH_INTREN_ERR, dch->base + CH_INTREN);
> +
> +	return 0;
>   
> +err_irq:
> +	dma_pool_destroy(dch->cmd_pool);
> +	dch->cmd_pool = NULL;
>   	return ret;
>   }
>   
> @@ -527,6 +608,8 @@ static void d350_free_chan_resources(struct dma_chan *chan)
>   	writel_relaxed(0, dch->base + CH_INTREN);
>   	free_irq(dch->irq, dch);
>   	vchan_free_chan_resources(&dch->vc);
> +	dma_pool_destroy(dch->cmd_pool);
> +	dch->cmd_pool = NULL;
>   }
>   
>   static int d350_probe(struct platform_device *pdev)

Re: [PATCH 10/14] dmaengine: dma350: Alloc command[] from dma pool

[Dan Carpenter]

Hi Jisheng,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jisheng-Zhang/dmaengine-dma350-Fix-CH_CTRL_USESRCTRIGIN-definition/20250824-000425
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git next
patch link:    https://lore.kernel.org/r/20250823154009.25992-11-jszhang%40kernel.org
patch subject: [PATCH 10/14] dmaengine: dma350: Alloc command[] from dma pool
config: arm-randconfig-r073-20250829 (https://download.01.org/0day-ci/archive/20250829/202508291556.kjNumYgR-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.4.0

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>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202508291556.kjNumYgR-lkp@intel.com/

smatch warnings:
drivers/dma/arm-dma350.c:387 d350_get_residue() error: uninitialized symbol 'sgcur'.

vim +/sgcur +387 drivers/dma/arm-dma350.c

5d099706449d54 Robin Murphy  2025-03-12  360  static u32 d350_get_residue(struct d350_chan *dch)
5d099706449d54 Robin Murphy  2025-03-12  361  {
eae79fde2ff50c Jisheng Zhang 2025-08-23  362  	u32 res, xsize, xsizehi, linkaddr, linkaddrhi, hi_new;
eae79fde2ff50c Jisheng Zhang 2025-08-23  363  	int i, sgcur, retries = 3; /* 1st time unlucky, 2nd improbable, 3rd just broken */
eae79fde2ff50c Jisheng Zhang 2025-08-23  364  	struct d350_desc *desc = dch->desc;
5d099706449d54 Robin Murphy  2025-03-12  365  
5d099706449d54 Robin Murphy  2025-03-12  366  	hi_new = readl_relaxed(dch->base + CH_XSIZEHI);
5d099706449d54 Robin Murphy  2025-03-12  367  	do {
5d099706449d54 Robin Murphy  2025-03-12  368  		xsizehi = hi_new;
5d099706449d54 Robin Murphy  2025-03-12  369  		xsize = readl_relaxed(dch->base + CH_XSIZE);
5d099706449d54 Robin Murphy  2025-03-12  370  		hi_new = readl_relaxed(dch->base + CH_XSIZEHI);
5d099706449d54 Robin Murphy  2025-03-12  371  	} while (xsizehi != hi_new && --retries);
5d099706449d54 Robin Murphy  2025-03-12  372  
eae79fde2ff50c Jisheng Zhang 2025-08-23  373  	hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
eae79fde2ff50c Jisheng Zhang 2025-08-23  374  	do {
eae79fde2ff50c Jisheng Zhang 2025-08-23  375  		linkaddrhi = hi_new;
eae79fde2ff50c Jisheng Zhang 2025-08-23  376  		linkaddr = readl_relaxed(dch->base + CH_LINKADDR);
eae79fde2ff50c Jisheng Zhang 2025-08-23  377  		hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
eae79fde2ff50c Jisheng Zhang 2025-08-23  378  	} while (linkaddrhi != hi_new && --retries);
eae79fde2ff50c Jisheng Zhang 2025-08-23  379  
eae79fde2ff50c Jisheng Zhang 2025-08-23  380  	for (i = 0; i < desc->sglen; i++) {
eae79fde2ff50c Jisheng Zhang 2025-08-23  381  		if (desc->sg[i].phys == (((u64)linkaddrhi << 32) | (linkaddr & ~CH_LINKADDR_EN)))
eae79fde2ff50c Jisheng Zhang 2025-08-23  382  			sgcur = i;

I'm suprised there isn't a break statement after this assignment.
What if we exit the loop with i == desc->sglen?

eae79fde2ff50c Jisheng Zhang 2025-08-23  383  	}
eae79fde2ff50c Jisheng Zhang 2025-08-23  384  
5d099706449d54 Robin Murphy  2025-03-12  385  	res = FIELD_GET(CH_XY_DES, xsize);
5d099706449d54 Robin Murphy  2025-03-12  386  	res |= FIELD_GET(CH_XY_DES, xsizehi) << 16;
eae79fde2ff50c Jisheng Zhang 2025-08-23 @387  	res <<= desc->sg[sgcur].tsz;
                                                                 ^^^^^
Uninitialized.

eae79fde2ff50c Jisheng Zhang 2025-08-23  388  
eae79fde2ff50c Jisheng Zhang 2025-08-23  389  	for (i = sgcur + 1; i < desc->sglen; i++)
eae79fde2ff50c Jisheng Zhang 2025-08-23  390  		res += (((u32)desc->sg[i].xsizehi << 16 | desc->sg[i].xsize) << desc->sg[i].tsz);
5d099706449d54 Robin Murphy  2025-03-12  391  
eae79fde2ff50c Jisheng Zhang 2025-08-23  392  	return res;
5d099706449d54 Robin Murphy  2025-03-12  393  }

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

[PATCH 11/14] dmaengine: dma350: Support device_prep_slave_sg

[Jisheng Zhang]

Add device_prep_slave_sg() callback function so that DMA_MEM_TO_DEV
and DMA_DEV_TO_MEM operations in single mode can be supported.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/dma/arm-dma350.c | 180 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 174 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 3d26a1f020df..a285778264b9 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (C) 2024-2025 Arm Limited
+// Copyright (C) 2025 Synaptics Incorporated
 // Arm DMA-350 driver
 
 #include <linux/bitfield.h>
@@ -98,7 +99,23 @@
 
 #define CH_FILLVAL		0x38
 #define CH_SRCTRIGINCFG		0x4c
+#define CH_SRCTRIGINMODE	GENMASK(11, 10)
+#define CH_SRCTRIG_CMD		0
+#define CH_SRCTRIG_DMA_FC	2
+#define CH_SRCTRIG_PERIF_FC	3
+#define CH_SRCTRIGINTYPE	GENMASK(9, 8)
+#define CH_SRCTRIG_SW_REQ	0
+#define CH_SRCTRIG_HW_REQ	2
+#define CH_SRCTRIG_INTERN_REQ	3
 #define CH_DESTRIGINCFG		0x50
+#define CH_DESTRIGINMODE	GENMASK(11, 10)
+#define CH_DESTRIG_CMD		0
+#define CH_DESTRIG_DMA_FC	2
+#define CH_DESTRIG_PERIF_FC	3
+#define CH_DESTRIGINTYPE	GENMASK(9, 8)
+#define CH_DESTRIG_SW_REQ	0
+#define CH_DESTRIG_HW_REQ	2
+#define CH_DESTRIG_INTERN_REQ	3
 #define CH_LINKATTR		0x70
 #define CH_LINK_SHAREATTR	GENMASK(9, 8)
 #define CH_LINK_MEMATTR		GENMASK(7, 0)
@@ -190,11 +207,13 @@ struct d350_chan {
 	struct d350_desc *desc;
 	void __iomem *base;
 	struct dma_pool *cmd_pool;
+	struct dma_slave_config config;
 	int irq;
 	enum dma_status status;
 	dma_cookie_t cookie;
 	u32 residue;
 	u8 tsz;
+	u8 ch;
 	bool has_trig;
 	bool has_wrap;
 	bool coherent;
@@ -327,6 +346,144 @@ static struct dma_async_tx_descriptor *d350_prep_memset(struct dma_chan *chan,
 	return vchan_tx_prep(&dch->vc, &desc->vd, flags);
 }
 
+static struct dma_async_tx_descriptor *
+d350_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
+		   unsigned int sg_len, enum dma_transfer_direction dir,
+		   unsigned long flags, void *context)
+{
+	struct d350_chan *dch = to_d350_chan(chan);
+	dma_addr_t src, dst, phys;
+	struct d350_desc *desc;
+	struct scatterlist *sg;
+	u32 len, trig, *cmd, *la_cmd, tsz;
+	struct d350_sg *dsg;
+	int i, j;
+
+	if (unlikely(!is_slave_direction(dir) || !sg_len))
+		return NULL;
+
+	desc = kzalloc(struct_size(desc, sg, sg_len), GFP_NOWAIT);
+	if (!desc)
+		return NULL;
+
+	desc->sglen = sg_len;
+
+	if (dir == DMA_MEM_TO_DEV)
+		tsz = __ffs(dch->config.dst_addr_width | (1 << dch->tsz));
+	else
+		tsz = __ffs(dch->config.src_addr_width | (1 << dch->tsz));
+
+	for_each_sg(sgl, sg, sg_len, i) {
+		desc->sg[i].command = dma_pool_zalloc(dch->cmd_pool, GFP_NOWAIT, &phys);
+		if (unlikely(!desc->sg[i].command))
+			goto err_cmd_alloc;
+
+		desc->sg[i].phys = phys;
+		dsg = &desc->sg[i];
+		len = sg_dma_len(sg);
+
+		if (dir == DMA_MEM_TO_DEV) {
+			src = sg_dma_address(sg);
+			dst = dch->config.dst_addr;
+			trig = CH_CTRL_USEDESTRIGIN;
+		} else {
+			src = dch->config.src_addr;
+			dst = sg_dma_address(sg);
+			trig = CH_CTRL_USESRCTRIGIN;
+		}
+		dsg->tsz = tsz;
+		dsg->xsize = lower_16_bits(len >> dsg->tsz);
+		dsg->xsizehi = upper_16_bits(len >> dsg->tsz);
+
+		cmd = dsg->command;
+		if (!i) {
+			cmd[0] = LINK_CTRL | LINK_SRCADDR | LINK_SRCADDRHI | LINK_DESADDR |
+				 LINK_DESADDRHI | LINK_XSIZE | LINK_XSIZEHI | LINK_SRCTRANSCFG |
+				 LINK_DESTRANSCFG | LINK_XADDRINC | LINK_LINKADDR;
+
+			cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, dsg->tsz) | trig |
+				 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_CONTINUE);
+
+			cmd[2] = lower_32_bits(src);
+			cmd[3] = upper_32_bits(src);
+			cmd[4] = lower_32_bits(dst);
+			cmd[5] = upper_32_bits(dst);
+			cmd[6] = FIELD_PREP(CH_XY_SRC, dsg->xsize) |
+				 FIELD_PREP(CH_XY_DES, dsg->xsize);
+			cmd[7] = FIELD_PREP(CH_XY_SRC, dsg->xsizehi) |
+				 FIELD_PREP(CH_XY_DES, dsg->xsizehi);
+			if (dir == DMA_MEM_TO_DEV) {
+				cmd[0] |= LINK_DESTRIGINCFG;
+				cmd[8] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
+				cmd[9] = TRANSCFG_DEVICE;
+				cmd[10] = FIELD_PREP(CH_XY_SRC, 1);
+				cmd[11] = FIELD_PREP(CH_DESTRIGINMODE, CH_DESTRIG_DMA_FC) |
+					  FIELD_PREP(CH_DESTRIGINTYPE, CH_DESTRIG_HW_REQ);
+			} else {
+				cmd[0] |= LINK_SRCTRIGINCFG;
+				cmd[8] = TRANSCFG_DEVICE;
+				cmd[9] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
+				cmd[10] = FIELD_PREP(CH_XY_DES, 1);
+				cmd[11] = FIELD_PREP(CH_SRCTRIGINMODE, CH_SRCTRIG_DMA_FC) |
+					  FIELD_PREP(CH_SRCTRIGINTYPE, CH_SRCTRIG_HW_REQ);
+			}
+			la_cmd = &cmd[12];
+		} else {
+			*la_cmd = phys | CH_LINKADDR_EN;
+			if (i == sg_len - 1) {
+				cmd[0] = LINK_CTRL | LINK_SRCADDR | LINK_SRCADDRHI | LINK_DESADDR |
+					 LINK_DESADDRHI | LINK_XSIZE | LINK_XSIZEHI | LINK_LINKADDR;
+				cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, dsg->tsz) | trig |
+					 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_CONTINUE);
+				cmd[2] = lower_32_bits(src);
+				cmd[3] = upper_32_bits(src);
+				cmd[4] = lower_32_bits(dst);
+				cmd[5] = upper_32_bits(dst);
+				cmd[6] = FIELD_PREP(CH_XY_SRC, dsg->xsize) |
+					 FIELD_PREP(CH_XY_DES, dsg->xsize);
+				cmd[7] = FIELD_PREP(CH_XY_SRC, dsg->xsizehi) |
+					 FIELD_PREP(CH_XY_DES, dsg->xsizehi);
+				la_cmd = &cmd[8];
+			} else {
+				cmd[0] = LINK_SRCADDR | LINK_SRCADDRHI | LINK_DESADDR |
+					 LINK_DESADDRHI | LINK_XSIZE | LINK_XSIZEHI | LINK_LINKADDR;
+				cmd[1] = lower_32_bits(src);
+				cmd[2] = upper_32_bits(src);
+				cmd[3] = lower_32_bits(dst);
+				cmd[4] = upper_32_bits(dst);
+				cmd[5] = FIELD_PREP(CH_XY_SRC, dsg->xsize) |
+					 FIELD_PREP(CH_XY_DES, dsg->xsize);
+				cmd[6] = FIELD_PREP(CH_XY_SRC, dsg->xsizehi) |
+					 FIELD_PREP(CH_XY_DES, dsg->xsizehi);
+				la_cmd = &cmd[7];
+			}
+		}
+	}
+
+	/* the last command */
+	*la_cmd = 0;
+	desc->sg[sg_len - 1].command[1] |= FIELD_PREP(CH_CTRL_DONETYPE, CH_CTRL_DONETYPE_CMD);
+
+	mb();
+
+	return vchan_tx_prep(&dch->vc, &desc->vd, flags);
+
+err_cmd_alloc:
+	for (j = 0; j < i; j++)
+		dma_pool_free(dch->cmd_pool, desc->sg[j].command, desc->sg[j].phys);
+	kfree(desc);
+	return NULL;
+}
+
+static int d350_slave_config(struct dma_chan *chan, struct dma_slave_config *config)
+{
+	struct d350_chan *dch = to_d350_chan(chan);
+
+	memcpy(&dch->config, config, sizeof(*config));
+
+	return 0;
+}
+
 static int d350_pause(struct dma_chan *chan)
 {
 	struct d350_chan *dch = to_d350_chan(chan);
@@ -558,8 +715,9 @@ static irqreturn_t d350_irq(int irq, void *data)
 	writel_relaxed(ch_status, dch->base + CH_STATUS);
 
 	spin_lock(&dch->vc.lock);
-	vchan_cookie_complete(vd);
 	if (ch_status & CH_STAT_INTR_DONE) {
+		vchan_cookie_complete(vd);
+		dch->desc = NULL;
 		dch->status = DMA_COMPLETE;
 		dch->residue = 0;
 		d350_start_next(dch);
@@ -617,7 +775,7 @@ static int d350_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct d350 *dmac;
 	void __iomem *base;
-	u32 reg, dma_chan_mask;
+	u32 reg, dma_chan_mask, trig_bits = 0;
 	int ret, nchan, dw, aw, r, p;
 	bool coherent, memset;
 
@@ -637,13 +795,11 @@ static int d350_probe(struct platform_device *pdev)
 	dw = 1 << FIELD_GET(DMA_CFG_DATA_WIDTH, reg);
 	aw = FIELD_GET(DMA_CFG_ADDR_WIDTH, reg) + 1;
 
-	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(aw));
-	coherent = device_get_dma_attr(dev) == DEV_DMA_COHERENT;
-
 	dmac = devm_kzalloc(dev, struct_size(dmac, channels, nchan), GFP_KERNEL);
 	if (!dmac)
 		return -ENOMEM;
 
+	dmac->dma.dev = dev;
 	dmac->nchan = nchan;
 
 	/* Enable all channels by default */
@@ -655,12 +811,14 @@ static int d350_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(aw));
+	coherent = device_get_dma_attr(dev) == DEV_DMA_COHERENT;
+
 	reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG1);
 	dmac->nreq = FIELD_GET(DMA_CFG_NUM_TRIGGER_IN, reg);
 
 	dev_dbg(dev, "DMA-350 r%dp%d with %d channels, %d requests\n", r, p, dmac->nchan, dmac->nreq);
 
-	dmac->dma.dev = dev;
 	for (int i = min(dw, 16); i > 0; i /= 2) {
 		dmac->dma.src_addr_widths |= BIT(i);
 		dmac->dma.dst_addr_widths |= BIT(i);
@@ -692,6 +850,7 @@ static int d350_probe(struct platform_device *pdev)
 
 		dch->coherent = coherent;
 		dch->base = base + DMACH(i);
+		dch->ch = i;
 		writel_relaxed(CH_CMD_CLEAR, dch->base + CH_CMD);
 
 		reg = readl_relaxed(dch->base + CH_BUILDCFG1);
@@ -711,6 +870,7 @@ static int d350_probe(struct platform_device *pdev)
 
 		/* Fill is a special case of Wrap */
 		memset &= dch->has_wrap;
+		trig_bits |= dch->has_trig << dch->ch;
 
 		reg = readl_relaxed(dch->base + CH_BUILDCFG0);
 		dch->tsz = FIELD_GET(CH_CFG_DATA_WIDTH, reg);
@@ -723,6 +883,13 @@ static int d350_probe(struct platform_device *pdev)
 		vchan_init(&dch->vc, &dmac->dma);
 	}
 
+	if (trig_bits) {
+		dmac->dma.directions |= (BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV));
+		dma_cap_set(DMA_SLAVE, dmac->dma.cap_mask);
+		dmac->dma.device_config = d350_slave_config;
+		dmac->dma.device_prep_slave_sg = d350_prep_slave_sg;
+	}
+
 	if (memset) {
 		dma_cap_set(DMA_MEMSET, dmac->dma.cap_mask);
 		dmac->dma.device_prep_dma_memset = d350_prep_memset;
@@ -759,5 +926,6 @@ static struct platform_driver d350_driver = {
 module_platform_driver(d350_driver);
 
 MODULE_AUTHOR("Robin Murphy <robin.murphy@arm.com>");
+MODULE_AUTHOR("Jisheng Zhang <jszhang@kernel.org>");
 MODULE_DESCRIPTION("Arm DMA-350 driver");
 MODULE_LICENSE("GPL v2");
-- 
2.50.0

Re: [PATCH 11/14] dmaengine: dma350: Support device_prep_slave_sg

[Robin Murphy]

On 2025-08-23 4:40 pm, Jisheng Zhang wrote:
> Add device_prep_slave_sg() callback function so that DMA_MEM_TO_DEV
> and DMA_DEV_TO_MEM operations in single mode can be supported.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>   drivers/dma/arm-dma350.c | 180 +++++++++++++++++++++++++++++++++++++--
>   1 file changed, 174 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index 3d26a1f020df..a285778264b9 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c
> @@ -1,5 +1,6 @@
>   // SPDX-License-Identifier: GPL-2.0
>   // Copyright (C) 2024-2025 Arm Limited
> +// Copyright (C) 2025 Synaptics Incorporated
>   // Arm DMA-350 driver
>   
>   #include <linux/bitfield.h>
> @@ -98,7 +99,23 @@
>   
>   #define CH_FILLVAL		0x38
>   #define CH_SRCTRIGINCFG		0x4c
> +#define CH_SRCTRIGINMODE	GENMASK(11, 10)
> +#define CH_SRCTRIG_CMD		0
> +#define CH_SRCTRIG_DMA_FC	2
> +#define CH_SRCTRIG_PERIF_FC	3
> +#define CH_SRCTRIGINTYPE	GENMASK(9, 8)
> +#define CH_SRCTRIG_SW_REQ	0
> +#define CH_SRCTRIG_HW_REQ	2
> +#define CH_SRCTRIG_INTERN_REQ	3
>   #define CH_DESTRIGINCFG		0x50
> +#define CH_DESTRIGINMODE	GENMASK(11, 10)
> +#define CH_DESTRIG_CMD		0
> +#define CH_DESTRIG_DMA_FC	2
> +#define CH_DESTRIG_PERIF_FC	3
> +#define CH_DESTRIGINTYPE	GENMASK(9, 8)
> +#define CH_DESTRIG_SW_REQ	0
> +#define CH_DESTRIG_HW_REQ	2
> +#define CH_DESTRIG_INTERN_REQ	3

Like the CH_CFG_* definitions, let's just have common CH_TRIG* fields 
that work for both SRC and DES to simplify matters. FWIW, my attempt to 
balance clarity with conciseness would be:

#define CH_TRIGINCFG_BLKSIZE	GENMASK(23, 16)
#define CH_TRIGINCFG_MODE	GENMASK(11, 10)
#define CH_TRIG_MODE_CMD	0
#define CH_TRIG_MODE_DMA	2
#define CH_TRIG_MODE_PERIPH	3
#define CH_TRIGINCFG_TYPE	GENMASK(9, 8)
#define CH_TRIG_TYPE_SW		0
#define CH_TRIG_TYPE_HW		2
#define CH_TRIG_TYPE_INTERN	3
#define CH_TRIGINCFG_SEL	GENMASK(7, 0)

>   #define CH_LINKATTR		0x70
>   #define CH_LINK_SHAREATTR	GENMASK(9, 8)
>   #define CH_LINK_MEMATTR		GENMASK(7, 0)
> @@ -190,11 +207,13 @@ struct d350_chan {
>   	struct d350_desc *desc;
>   	void __iomem *base;
>   	struct dma_pool *cmd_pool;
> +	struct dma_slave_config config;
>   	int irq;
>   	enum dma_status status;
>   	dma_cookie_t cookie;
>   	u32 residue;
>   	u8 tsz;
> +	u8 ch;

What's this for? It doesn't seem to be anything meaningful.

>   	bool has_trig;
>   	bool has_wrap;
>   	bool coherent;
> @@ -327,6 +346,144 @@ static struct dma_async_tx_descriptor *d350_prep_memset(struct dma_chan *chan,
>   	return vchan_tx_prep(&dch->vc, &desc->vd, flags);
>   }
>   
> +static struct dma_async_tx_descriptor *
> +d350_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
> +		   unsigned int sg_len, enum dma_transfer_direction dir,
> +		   unsigned long flags, void *context)
> +{
> +	struct d350_chan *dch = to_d350_chan(chan);
> +	dma_addr_t src, dst, phys;
> +	struct d350_desc *desc;
> +	struct scatterlist *sg;
> +	u32 len, trig, *cmd, *la_cmd, tsz;
> +	struct d350_sg *dsg;
> +	int i, j;
> +
> +	if (unlikely(!is_slave_direction(dir) || !sg_len))
> +		return NULL;
> +
> +	desc = kzalloc(struct_size(desc, sg, sg_len), GFP_NOWAIT);
> +	if (!desc)
> +		return NULL;
> +
> +	desc->sglen = sg_len;
> +
> +	if (dir == DMA_MEM_TO_DEV)
> +		tsz = __ffs(dch->config.dst_addr_width | (1 << dch->tsz));
> +	else
> +		tsz = __ffs(dch->config.src_addr_width | (1 << dch->tsz));

Surely we should just use the exact addr_width requested?

> +	for_each_sg(sgl, sg, sg_len, i) {
> +		desc->sg[i].command = dma_pool_zalloc(dch->cmd_pool, GFP_NOWAIT, &phys);
> +		if (unlikely(!desc->sg[i].command))
> +			goto err_cmd_alloc;
> +
> +		desc->sg[i].phys = phys;
> +		dsg = &desc->sg[i];
> +		len = sg_dma_len(sg);
> +
> +		if (dir == DMA_MEM_TO_DEV) {
> +			src = sg_dma_address(sg);
> +			dst = dch->config.dst_addr;
> +			trig = CH_CTRL_USEDESTRIGIN;
> +		} else {
> +			src = dch->config.src_addr;
> +			dst = sg_dma_address(sg);
> +			trig = CH_CTRL_USESRCTRIGIN;
> +		}
> +		dsg->tsz = tsz;
> +		dsg->xsize = lower_16_bits(len >> dsg->tsz);
> +		dsg->xsizehi = upper_16_bits(len >> dsg->tsz);
> +
> +		cmd = dsg->command;
> +		if (!i) {
> +			cmd[0] = LINK_CTRL | LINK_SRCADDR | LINK_SRCADDRHI | LINK_DESADDR |
> +				 LINK_DESADDRHI | LINK_XSIZE | LINK_XSIZEHI | LINK_SRCTRANSCFG |
> +				 LINK_DESTRANSCFG | LINK_XADDRINC | LINK_LINKADDR;
> +
> +			cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, dsg->tsz) | trig |
> +				 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_CONTINUE);
> +
> +			cmd[2] = lower_32_bits(src);
> +			cmd[3] = upper_32_bits(src);
> +			cmd[4] = lower_32_bits(dst);
> +			cmd[5] = upper_32_bits(dst);
> +			cmd[6] = FIELD_PREP(CH_XY_SRC, dsg->xsize) |
> +				 FIELD_PREP(CH_XY_DES, dsg->xsize);
> +			cmd[7] = FIELD_PREP(CH_XY_SRC, dsg->xsizehi) |
> +				 FIELD_PREP(CH_XY_DES, dsg->xsizehi);
> +			if (dir == DMA_MEM_TO_DEV) {
> +				cmd[0] |= LINK_DESTRIGINCFG;
> +				cmd[8] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
> +				cmd[9] = TRANSCFG_DEVICE;
> +				cmd[10] = FIELD_PREP(CH_XY_SRC, 1);
> +				cmd[11] = FIELD_PREP(CH_DESTRIGINMODE, CH_DESTRIG_DMA_FC) |
> +					  FIELD_PREP(CH_DESTRIGINTYPE, CH_DESTRIG_HW_REQ);
> +			} else {
> +				cmd[0] |= LINK_SRCTRIGINCFG;
> +				cmd[8] = TRANSCFG_DEVICE;
> +				cmd[9] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
> +				cmd[10] = FIELD_PREP(CH_XY_DES, 1);
> +				cmd[11] = FIELD_PREP(CH_SRCTRIGINMODE, CH_SRCTRIG_DMA_FC) |
> +					  FIELD_PREP(CH_SRCTRIGINTYPE, CH_SRCTRIG_HW_REQ);
> +			}
> +			la_cmd = &cmd[12];
> +		} else {
> +			*la_cmd = phys | CH_LINKADDR_EN;
> +			if (i == sg_len - 1) {
> +				cmd[0] = LINK_CTRL | LINK_SRCADDR | LINK_SRCADDRHI | LINK_DESADDR |
> +					 LINK_DESADDRHI | LINK_XSIZE | LINK_XSIZEHI | LINK_LINKADDR;
> +				cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, dsg->tsz) | trig |
> +					 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_CONTINUE);
> +				cmd[2] = lower_32_bits(src);
> +				cmd[3] = upper_32_bits(src);
> +				cmd[4] = lower_32_bits(dst);
> +				cmd[5] = upper_32_bits(dst);
> +				cmd[6] = FIELD_PREP(CH_XY_SRC, dsg->xsize) |
> +					 FIELD_PREP(CH_XY_DES, dsg->xsize);
> +				cmd[7] = FIELD_PREP(CH_XY_SRC, dsg->xsizehi) |
> +					 FIELD_PREP(CH_XY_DES, dsg->xsizehi);
> +				la_cmd = &cmd[8];
> +			} else {
> +				cmd[0] = LINK_SRCADDR | LINK_SRCADDRHI | LINK_DESADDR |
> +					 LINK_DESADDRHI | LINK_XSIZE | LINK_XSIZEHI | LINK_LINKADDR;
> +				cmd[1] = lower_32_bits(src);
> +				cmd[2] = upper_32_bits(src);
> +				cmd[3] = lower_32_bits(dst);
> +				cmd[4] = upper_32_bits(dst);
> +				cmd[5] = FIELD_PREP(CH_XY_SRC, dsg->xsize) |
> +					 FIELD_PREP(CH_XY_DES, dsg->xsize);
> +				cmd[6] = FIELD_PREP(CH_XY_SRC, dsg->xsizehi) |
> +					 FIELD_PREP(CH_XY_DES, dsg->xsizehi);
> +				la_cmd = &cmd[7];
> +			}
> +		}
> +	}
Again, the structure and duplication here is unbearable. I fail to 
comprehend why the "if (i == sg_len - 1)" case even exists...

More crucially, it clearly can't even work for DMA-350 since it would 
always be selecting trigger 0. How much of this has been tested?

> +	/* the last command */
> +	*la_cmd = 0;
> +	desc->sg[sg_len - 1].command[1] |= FIELD_PREP(CH_CTRL_DONETYPE, CH_CTRL_DONETYPE_CMD);
> +
> +	mb();
> +
> +	return vchan_tx_prep(&dch->vc, &desc->vd, flags);
> +
> +err_cmd_alloc:
> +	for (j = 0; j < i; j++)
> +		dma_pool_free(dch->cmd_pool, desc->sg[j].command, desc->sg[j].phys);
> +	kfree(desc);
> +	return NULL;
> +}
> +
> +static int d350_slave_config(struct dma_chan *chan, struct dma_slave_config *config)
> +{
> +	struct d350_chan *dch = to_d350_chan(chan);
Shouldn't we validate that the given channel has any chance of 
supporting the given config?

> +	memcpy(&dch->config, config, sizeof(*config));
> +
> +	return 0;
> +}
> +
>   static int d350_pause(struct dma_chan *chan)
>   {
>   	struct d350_chan *dch = to_d350_chan(chan);
> @@ -558,8 +715,9 @@ static irqreturn_t d350_irq(int irq, void *data)
>   	writel_relaxed(ch_status, dch->base + CH_STATUS);
>   
>   	spin_lock(&dch->vc.lock);
> -	vchan_cookie_complete(vd);
>   	if (ch_status & CH_STAT_INTR_DONE) {
> +		vchan_cookie_complete(vd);
> +		dch->desc = NULL;

What's this about? As I mentioned on the earlier patches it was very 
fiddly to get consistently appropriate handling of errors for 
MEM_TO_MEM, so please explain if this change is somehow necessary for 
MEM_TO_DEV/DEV_TO_MEM. Otherwise, if anything it just looks like an 
undocumented bodge around what those previous patches subtly broke.

>   		dch->status = DMA_COMPLETE;
>   		dch->residue = 0;
>   		d350_start_next(dch);
> @@ -617,7 +775,7 @@ static int d350_probe(struct platform_device *pdev)
>   	struct device *dev = &pdev->dev;
>   	struct d350 *dmac;
>   	void __iomem *base;
> -	u32 reg, dma_chan_mask;
> +	u32 reg, dma_chan_mask, trig_bits = 0;
>   	int ret, nchan, dw, aw, r, p;
>   	bool coherent, memset;
>   
> @@ -637,13 +795,11 @@ static int d350_probe(struct platform_device *pdev)
>   	dw = 1 << FIELD_GET(DMA_CFG_DATA_WIDTH, reg);
>   	aw = FIELD_GET(DMA_CFG_ADDR_WIDTH, reg) + 1;
>   
> -	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(aw));
> -	coherent = device_get_dma_attr(dev) == DEV_DMA_COHERENT;
> -
>   	dmac = devm_kzalloc(dev, struct_size(dmac, channels, nchan), GFP_KERNEL);
>   	if (!dmac)
>   		return -ENOMEM;
>   
> +	dmac->dma.dev = dev;

Similarly, why are these two hunks just moving lines around apparently 
at random?

>   	dmac->nchan = nchan;
>   
>   	/* Enable all channels by default */
> @@ -655,12 +811,14 @@ static int d350_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>   
> +	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(aw));
> +	coherent = device_get_dma_attr(dev) == DEV_DMA_COHERENT;
> +
>   	reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG1);
>   	dmac->nreq = FIELD_GET(DMA_CFG_NUM_TRIGGER_IN, reg);
>   
>   	dev_dbg(dev, "DMA-350 r%dp%d with %d channels, %d requests\n", r, p, dmac->nchan, dmac->nreq);
>   
> -	dmac->dma.dev = dev;
>   	for (int i = min(dw, 16); i > 0; i /= 2) {
>   		dmac->dma.src_addr_widths |= BIT(i);
>   		dmac->dma.dst_addr_widths |= BIT(i);
> @@ -692,6 +850,7 @@ static int d350_probe(struct platform_device *pdev)
>   
>   		dch->coherent = coherent;
>   		dch->base = base + DMACH(i);
> +		dch->ch = i;
>   		writel_relaxed(CH_CMD_CLEAR, dch->base + CH_CMD);
>   
>   		reg = readl_relaxed(dch->base + CH_BUILDCFG1);
> @@ -711,6 +870,7 @@ static int d350_probe(struct platform_device *pdev)
>   
>   		/* Fill is a special case of Wrap */
>   		memset &= dch->has_wrap;
> +		trig_bits |= dch->has_trig << dch->ch;

This is a bool merely dressed up as a bitmap, the shift is doing nothing.

Thanks,
Robin.

>   		reg = readl_relaxed(dch->base + CH_BUILDCFG0);
>   		dch->tsz = FIELD_GET(CH_CFG_DATA_WIDTH, reg);
> @@ -723,6 +883,13 @@ static int d350_probe(struct platform_device *pdev)
>   		vchan_init(&dch->vc, &dmac->dma);
>   	}
>   
> +	if (trig_bits) {
> +		dmac->dma.directions |= (BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV));
> +		dma_cap_set(DMA_SLAVE, dmac->dma.cap_mask);
> +		dmac->dma.device_config = d350_slave_config;
> +		dmac->dma.device_prep_slave_sg = d350_prep_slave_sg;
> +	}
> +
>   	if (memset) {
>   		dma_cap_set(DMA_MEMSET, dmac->dma.cap_mask);
>   		dmac->dma.device_prep_dma_memset = d350_prep_memset;
> @@ -759,5 +926,6 @@ static struct platform_driver d350_driver = {
>   module_platform_driver(d350_driver);
>   
>   MODULE_AUTHOR("Robin Murphy <robin.murphy@arm.com>");
> +MODULE_AUTHOR("Jisheng Zhang <jszhang@kernel.org>");
>   MODULE_DESCRIPTION("Arm DMA-350 driver");
>   MODULE_LICENSE("GPL v2");

[PATCH 12/14] dmaengine: dma350: Support device_prep_dma_cyclic

[Jisheng Zhang]

Add support for device_prep_dma_cyclic() callback function to benefit
DMA cyclic client, for example ALSA.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/dma/arm-dma350.c | 118 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 113 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index a285778264b9..5abb965c6687 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -212,8 +212,10 @@ struct d350_chan {
 	enum dma_status status;
 	dma_cookie_t cookie;
 	u32 residue;
+	u32 periods;
 	u8 tsz;
 	u8 ch;
+	bool cyclic;
 	bool has_trig;
 	bool has_wrap;
 	bool coherent;
@@ -475,6 +477,105 @@ d350_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 	return NULL;
 }
 
+static struct dma_async_tx_descriptor *
+d350_prep_cyclic(struct dma_chan *chan, dma_addr_t buf_addr,
+		 size_t buf_len, size_t period_len, enum dma_transfer_direction dir,
+		 unsigned long flags)
+{
+	struct d350_chan *dch = to_d350_chan(chan);
+	u32 periods, trig, *cmd, tsz;
+	dma_addr_t src, dst, phys;
+	struct d350_desc *desc;
+	struct d350_sg *dsg;
+	int i, j;
+
+	if (unlikely(!is_slave_direction(dir) || !buf_len || !period_len))
+		return NULL;
+
+	periods = buf_len / period_len;
+
+	desc = kzalloc(struct_size(desc, sg, periods), GFP_NOWAIT);
+	if (!desc)
+		return NULL;
+
+	dch->cyclic = true;
+	desc->sglen = periods;
+
+	if (dir == DMA_MEM_TO_DEV)
+		tsz = __ffs(dch->config.dst_addr_width | (1 << dch->tsz));
+	else
+		tsz = __ffs(dch->config.src_addr_width | (1 << dch->tsz));
+
+	for (i = 0; i < periods; i++) {
+		desc->sg[i].command = dma_pool_zalloc(dch->cmd_pool, GFP_NOWAIT, &phys);
+		if (unlikely(!desc->sg[i].command))
+			goto err_cmd_alloc;
+
+		desc->sg[i].phys = phys;
+		dsg = &desc->sg[i];
+
+		if (dir == DMA_MEM_TO_DEV) {
+			src = buf_addr + i * period_len;
+			dst = dch->config.dst_addr;
+			trig = CH_CTRL_USEDESTRIGIN;
+		} else {
+			src = dch->config.src_addr;
+			dst = buf_addr + i * period_len;
+			trig = CH_CTRL_USESRCTRIGIN;
+		}
+		dsg->tsz = tsz;
+		dsg->xsize = lower_16_bits(period_len >> dsg->tsz);
+		dsg->xsizehi = upper_16_bits(period_len >> dsg->tsz);
+
+		cmd = dsg->command;
+		cmd[0] = LINK_CTRL | LINK_SRCADDR | LINK_SRCADDRHI | LINK_DESADDR |
+			 LINK_DESADDRHI | LINK_XSIZE | LINK_XSIZEHI | LINK_SRCTRANSCFG |
+			 LINK_DESTRANSCFG | LINK_XADDRINC | LINK_LINKADDR;
+
+		cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, dsg->tsz) |
+			 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_CONTINUE) |
+			 FIELD_PREP(CH_CTRL_DONETYPE, CH_CTRL_DONETYPE_CMD) | trig;
+
+		cmd[2] = lower_32_bits(src);
+		cmd[3] = upper_32_bits(src);
+		cmd[4] = lower_32_bits(dst);
+		cmd[5] = upper_32_bits(dst);
+		cmd[6] = FIELD_PREP(CH_XY_SRC, dsg->xsize) | FIELD_PREP(CH_XY_DES, dsg->xsize);
+		cmd[7] = FIELD_PREP(CH_XY_SRC, dsg->xsizehi) | FIELD_PREP(CH_XY_DES, dsg->xsizehi);
+		if (dir == DMA_MEM_TO_DEV) {
+			cmd[0] |= LINK_DESTRIGINCFG;
+			cmd[8] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
+			cmd[9] = TRANSCFG_DEVICE;
+			cmd[10] = FIELD_PREP(CH_XY_SRC, 1);
+			cmd[11] = FIELD_PREP(CH_DESTRIGINMODE, CH_DESTRIG_DMA_FC) |
+				  FIELD_PREP(CH_DESTRIGINTYPE, CH_DESTRIG_HW_REQ);
+		} else {
+			cmd[0] |= LINK_SRCTRIGINCFG;
+			cmd[8] = TRANSCFG_DEVICE;
+			cmd[9] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
+			cmd[10] = FIELD_PREP(CH_XY_DES, 1);
+			cmd[11] = FIELD_PREP(CH_SRCTRIGINMODE, CH_SRCTRIG_DMA_FC) |
+				  FIELD_PREP(CH_SRCTRIGINTYPE, CH_SRCTRIG_HW_REQ);
+		}
+
+		if (i)
+			desc->sg[i - 1].command[12] = phys | CH_LINKADDR_EN;
+	}
+
+	/* cyclic list */
+	desc->sg[periods - 1].command[12] = desc->sg[0].phys | CH_LINKADDR_EN;
+
+	mb();
+
+	return vchan_tx_prep(&dch->vc, &desc->vd, flags);
+
+err_cmd_alloc:
+	for (j = 0; j < i; j++)
+		dma_pool_free(dch->cmd_pool, desc->sg[j].command, desc->sg[j].phys);
+	kfree(desc);
+	return NULL;
+}
+
 static int d350_slave_config(struct dma_chan *chan, struct dma_slave_config *config)
 {
 	struct d350_chan *dch = to_d350_chan(chan);
@@ -565,6 +666,7 @@ static int d350_terminate_all(struct dma_chan *chan)
 	}
 	vchan_get_all_descriptors(&dch->vc, &list);
 	list_splice_tail(&list, &dch->vc.desc_terminated);
+	dch->cyclic = false;
 	spin_unlock_irqrestore(&dch->vc.lock, flags);
 
 	return 0;
@@ -716,11 +818,15 @@ static irqreturn_t d350_irq(int irq, void *data)
 
 	spin_lock(&dch->vc.lock);
 	if (ch_status & CH_STAT_INTR_DONE) {
-		vchan_cookie_complete(vd);
-		dch->desc = NULL;
-		dch->status = DMA_COMPLETE;
-		dch->residue = 0;
-		d350_start_next(dch);
+		if (dch->cyclic) {
+			vchan_cyclic_callback(vd);
+		} else {
+			vchan_cookie_complete(vd);
+			dch->desc = NULL;
+			dch->status = DMA_COMPLETE;
+			dch->residue = 0;
+			d350_start_next(dch);
+		}
 	} else {
 		dch->status = DMA_ERROR;
 		dch->residue = vd->tx_result.residue;
@@ -886,8 +992,10 @@ static int d350_probe(struct platform_device *pdev)
 	if (trig_bits) {
 		dmac->dma.directions |= (BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV));
 		dma_cap_set(DMA_SLAVE, dmac->dma.cap_mask);
+		dma_cap_set(DMA_CYCLIC, dmac->dma.cap_mask);
 		dmac->dma.device_config = d350_slave_config;
 		dmac->dma.device_prep_slave_sg = d350_prep_slave_sg;
+		dmac->dma.device_prep_dma_cyclic = d350_prep_cyclic;
 	}
 
 	if (memset) {
-- 
2.50.0

[PATCH 13/14] dt-bindings: dma: dma350: Support ARM DMA-250

[Jisheng Zhang]

Compared with ARM DMA-350, DMA-250 is a simplified version, they share
many common parts, but they do have difference. The difference will be
handled in next driver patch, while let's add DMA-250 compatible string
to dt-binding now.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 Documentation/devicetree/bindings/dma/arm,dma-350.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
index 94752516e51a..d49736b7de5e 100644
--- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
+++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
@@ -15,6 +15,7 @@ allOf:
 properties:
   compatible:
     const: arm,dma-350
+    const: arm,dma-250
 
   reg:
     items:
-- 
2.50.0

Re: [PATCH 13/14] dt-bindings: dma: dma350: Support ARM DMA-250

[Krzysztof Kozlowski]

On 23/08/2025 17:40, Jisheng Zhang wrote:
> Compared with ARM DMA-350, DMA-250 is a simplified version, they share
> many common parts, but they do have difference. The difference will be
> handled in next driver patch, while let's add DMA-250 compatible string
> to dt-binding now.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>  Documentation/devicetree/bindings/dma/arm,dma-350.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> index 94752516e51a..d49736b7de5e 100644
> --- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> +++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> @@ -15,6 +15,7 @@ allOf:
>  properties:
>    compatible:
>      const: arm,dma-350
> +    const: arm,dma-250

This obviously cannot work and was NEVER tested. Please test your code
before you send it to mailing lists.

Best regards,
Krzysztof

Re: [PATCH 13/14] dt-bindings: dma: dma350: Support ARM DMA-250

[Robin Murphy]

On 2025-08-23 5:11 pm, Krzysztof Kozlowski wrote:
> On 23/08/2025 17:40, Jisheng Zhang wrote:
>> Compared with ARM DMA-350, DMA-250 is a simplified version, they share
>> many common parts, but they do have difference. The difference will be
>> handled in next driver patch, while let's add DMA-250 compatible string
>> to dt-binding now.
>>
>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
>> ---
>>   Documentation/devicetree/bindings/dma/arm,dma-350.yaml | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
>> index 94752516e51a..d49736b7de5e 100644
>> --- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
>> +++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
>> @@ -15,6 +15,7 @@ allOf:
>>   properties:
>>     compatible:
>>       const: arm,dma-350
>> +    const: arm,dma-250
> 
> This obviously cannot work and was NEVER tested. Please test your code
> before you send it to mailing lists.

Also, DMA-250 should be 100% "compatible" with DMA-350 in the DT sense, 
since it shares the same register layout and general functionality, and 
the detailed features and even exact model are discoverable from ID 
registers (hence why the current driver explicitly checks for 
PRODUCTID_DMA350 as that's the only one it knows it definitely understands).

Thanks,
Robin.

[PATCH 14/14] dmaengine: dma350: Support ARM DMA-250

[Jisheng Zhang]

Compared with ARM DMA-350, DMA-250 is a simplified version. They share
many common parts, but they do have difference. Add DMA-250 support
by handling their difference by using different device_prep_slave_sg,
device_prep_dma_cyclic and device_prep_dma_memcpy. DMA-250 doesn't
support device_prep_dma_memset.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/dma/arm-dma350.c | 444 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 424 insertions(+), 20 deletions(-)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 5abb965c6687..0ee807424b7e 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (C) 2024-2025 Arm Limited
 // Copyright (C) 2025 Synaptics Incorporated
-// Arm DMA-350 driver
+// Arm DMA-350/DMA-250 driver
 
 #include <linux/bitfield.h>
 #include <linux/dmaengine.h>
@@ -16,6 +16,10 @@
 #include "dmaengine.h"
 #include "virt-dma.h"
 
+#define DMANSECCTRL		0x0200
+
+#define NSEC_CNTXBASE		0x10
+
 #define DMAINFO			0x0f00
 
 #define DMA_BUILDCFG0		0xb0
@@ -26,12 +30,16 @@
 #define DMA_BUILDCFG1		0xb4
 #define DMA_CFG_NUM_TRIGGER_IN	GENMASK(8, 0)
 
+#define DMA_BUILDCFG2		0xb8
+#define DMA_CFG_HAS_TZ		BIT(8)
+
 #define IIDR			0xc8
 #define IIDR_PRODUCTID		GENMASK(31, 20)
 #define IIDR_VARIANT		GENMASK(19, 16)
 #define IIDR_REVISION		GENMASK(15, 12)
 #define IIDR_IMPLEMENTER	GENMASK(11, 0)
 
+#define PRODUCTID_DMA250	0x250
 #define PRODUCTID_DMA350	0x3a0
 #define IMPLEMENTER_ARM		0x43b
 
@@ -140,6 +148,7 @@
 #define CH_CFG_HAS_TRIGSEL	BIT(7)
 #define CH_CFG_HAS_TRIGIN	BIT(5)
 #define CH_CFG_HAS_WRAP		BIT(1)
+#define CH_CFG_HAS_XSIZEHI	BIT(0)
 
 
 #define LINK_REGCLEAR		BIT(0)
@@ -218,6 +227,7 @@ struct d350_chan {
 	bool cyclic;
 	bool has_trig;
 	bool has_wrap;
+	bool has_xsizehi;
 	bool coherent;
 };
 
@@ -225,6 +235,10 @@ struct d350 {
 	struct dma_device dma;
 	int nchan;
 	int nreq;
+	bool is_d250;
+	dma_addr_t cntx_mem_paddr;
+	void *cntx_mem;
+	u32 cntx_mem_size;
 	struct d350_chan channels[] __counted_by(nchan);
 };
 
@@ -238,6 +252,11 @@ static inline struct d350_desc *to_d350_desc(struct virt_dma_desc *vd)
 	return container_of(vd, struct d350_desc, vd);
 }
 
+static inline struct d350 *to_d350(struct dma_device *dd)
+{
+	return container_of(dd, struct d350, dma);
+}
+
 static void d350_desc_free(struct virt_dma_desc *vd)
 {
 	struct d350_chan *dch = to_d350_chan(vd->tx.chan);
@@ -585,6 +604,337 @@ static int d350_slave_config(struct dma_chan *chan, struct dma_slave_config *con
 	return 0;
 }
 
+static struct dma_async_tx_descriptor *d250_prep_memcpy(struct dma_chan *chan,
+		dma_addr_t dest, dma_addr_t src, size_t len, unsigned long flags)
+{
+	struct d350_chan *dch = to_d350_chan(chan);
+	struct d350_desc *desc;
+	u32 *cmd, *la_cmd, tsz;
+	int sglen, i;
+	struct d350_sg *sg;
+	size_t xfer_len, step_max;
+	dma_addr_t phys;
+
+	tsz = __ffs(len | dest | src | (1 << dch->tsz));
+	step_max = ((1UL << 16) - 1) << tsz;
+	sglen = DIV_ROUND_UP(len, step_max);
+
+	desc = kzalloc(struct_size(desc, sg, sglen), GFP_NOWAIT);
+	if (!desc)
+		return NULL;
+
+	desc->sglen = sglen;
+	sglen = 0;
+	while (len) {
+		sg = &desc->sg[sglen];
+		xfer_len = (len > step_max) ? step_max : len;
+		sg->tsz = __ffs(xfer_len | dest | src | (1 << dch->tsz));
+		sg->xsize = lower_16_bits(xfer_len >> sg->tsz);
+
+		sg->command = dma_pool_zalloc(dch->cmd_pool, GFP_NOWAIT, &phys);
+		if (unlikely(!sg->command))
+			goto err_cmd_alloc;
+		sg->phys = phys;
+
+		cmd = sg->command;
+		if (!sglen) {
+			cmd[0] = LINK_CTRL | LINK_SRCADDR | LINK_DESADDR |
+				 LINK_XSIZE | LINK_SRCTRANSCFG |
+				 LINK_DESTRANSCFG | LINK_XADDRINC | LINK_LINKADDR;
+
+			cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, sg->tsz) |
+				 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_CONTINUE);
+
+			cmd[2] = lower_32_bits(src);
+			cmd[3] = lower_32_bits(dest);
+			cmd[4] = FIELD_PREP(CH_XY_SRC, sg->xsize) |
+				 FIELD_PREP(CH_XY_DES, sg->xsize);
+			cmd[5] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
+			cmd[6] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
+			cmd[7] = FIELD_PREP(CH_XY_SRC, 1) | FIELD_PREP(CH_XY_DES, 1);
+			la_cmd = &cmd[8];
+		} else {
+			*la_cmd = phys | CH_LINKADDR_EN;
+			if (len <= step_max) {
+				cmd[0] = LINK_CTRL | LINK_XSIZE | LINK_LINKADDR;
+				cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, sg->tsz) |
+					 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_CONTINUE);
+				cmd[2] = FIELD_PREP(CH_XY_SRC, sg->xsize) |
+					 FIELD_PREP(CH_XY_DES, sg->xsize);
+				la_cmd = &cmd[3];
+			} else {
+				cmd[0] = LINK_XSIZE | LINK_LINKADDR;
+				cmd[1] = FIELD_PREP(CH_XY_SRC, sg->xsize) |
+					 FIELD_PREP(CH_XY_DES, sg->xsize);
+				la_cmd = &cmd[2];
+			}
+		}
+
+		len -= xfer_len;
+		src += xfer_len;
+		dest += xfer_len;
+		sglen++;
+	}
+
+	/* the last cmdlink */
+	*la_cmd = 0;
+	desc->sg[sglen - 1].command[1] |= FIELD_PREP(CH_CTRL_DONETYPE, CH_CTRL_DONETYPE_CMD);
+
+	mb();
+
+	return vchan_tx_prep(&dch->vc, &desc->vd, flags);
+
+err_cmd_alloc:
+	for (i = 0; i < sglen; i++)
+		dma_pool_free(dch->cmd_pool, desc->sg[i].command, desc->sg[i].phys);
+	kfree(desc);
+	return NULL;
+}
+
+static struct dma_async_tx_descriptor *
+d250_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
+		   unsigned int sg_len, enum dma_transfer_direction dir,
+		   unsigned long flags, void *context)
+{
+	struct d350_chan *dch = to_d350_chan(chan);
+	dma_addr_t src, dst, phys, mem_addr;
+	size_t xfer_len, step_max;
+	u32 len, trig, *cmd, *la_cmd, tsz;
+	struct d350_desc *desc;
+	struct scatterlist *sg;
+	struct d350_sg *dsg;
+	int i, sglen = 0;
+
+	if (unlikely(!is_slave_direction(dir) || !sg_len))
+		return NULL;
+
+	if (dir == DMA_MEM_TO_DEV)
+		tsz = __ffs(dch->config.dst_addr_width | (1 << dch->tsz));
+	else
+		tsz = __ffs(dch->config.src_addr_width | (1 << dch->tsz));
+	step_max = ((1UL << 16) - 1) << tsz;
+
+	for_each_sg(sgl, sg, sg_len, i)
+		sglen += DIV_ROUND_UP(sg_dma_len(sg), step_max);
+
+	desc = kzalloc(struct_size(desc, sg, sglen), GFP_NOWAIT);
+	if (!desc)
+		return NULL;
+
+	desc->sglen = sglen;
+
+	sglen = 0;
+	for_each_sg(sgl, sg, sg_len, i) {
+		len = sg_dma_len(sg);
+		mem_addr = sg_dma_address(sg);
+
+		do {
+			desc->sg[sglen].command = dma_pool_zalloc(dch->cmd_pool, GFP_NOWAIT, &phys);
+			if (unlikely(!desc->sg[sglen].command))
+				goto err_cmd_alloc;
+
+			xfer_len = (len > step_max) ? step_max : len;
+			desc->sg[sglen].phys = phys;
+			dsg = &desc->sg[sglen];
+
+			if (dir == DMA_MEM_TO_DEV) {
+				src = mem_addr;
+				dst = dch->config.dst_addr;
+				trig = CH_CTRL_USEDESTRIGIN;
+			} else {
+				src = dch->config.src_addr;
+				dst = mem_addr;
+				trig = CH_CTRL_USESRCTRIGIN;
+			}
+			dsg->tsz = tsz;
+			dsg->xsize = lower_16_bits(xfer_len >> dsg->tsz);
+
+			cmd = dsg->command;
+			if (!sglen) {
+				cmd[0] = LINK_CTRL | LINK_SRCADDR | LINK_DESADDR |
+					 LINK_XSIZE | LINK_SRCTRANSCFG |
+					 LINK_DESTRANSCFG | LINK_XADDRINC | LINK_LINKADDR;
+
+				cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, dsg->tsz) | trig |
+					 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_CONTINUE);
+
+				cmd[2] = lower_32_bits(src);
+				cmd[3] = lower_32_bits(dst);
+				cmd[4] = FIELD_PREP(CH_XY_SRC, dsg->xsize) |
+					 FIELD_PREP(CH_XY_DES, dsg->xsize);
+				if (dir == DMA_MEM_TO_DEV) {
+					cmd[0] |= LINK_DESTRIGINCFG;
+					cmd[5] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
+					cmd[6] = TRANSCFG_DEVICE;
+					cmd[7] = FIELD_PREP(CH_XY_SRC, 1);
+					cmd[8] = FIELD_PREP(CH_DESTRIGINMODE, CH_DESTRIG_DMA_FC) |
+						  FIELD_PREP(CH_DESTRIGINTYPE, CH_DESTRIG_HW_REQ);
+				} else {
+					cmd[0] |= LINK_SRCTRIGINCFG;
+					cmd[5] = TRANSCFG_DEVICE;
+					cmd[6] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
+					cmd[7] = FIELD_PREP(CH_XY_DES, 1);
+					cmd[8] = FIELD_PREP(CH_SRCTRIGINMODE, CH_SRCTRIG_DMA_FC) |
+						  FIELD_PREP(CH_SRCTRIGINTYPE, CH_SRCTRIG_HW_REQ);
+				}
+				la_cmd = &cmd[9];
+			} else {
+				*la_cmd = phys | CH_LINKADDR_EN;
+				if (sglen == desc->sglen - 1) {
+					cmd[0] = LINK_CTRL | LINK_SRCADDR | LINK_DESADDR |
+						 LINK_XSIZE | LINK_LINKADDR;
+					cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, dsg->tsz) | trig |
+						 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_CONTINUE);
+					cmd[2] = lower_32_bits(src);
+					cmd[3] = lower_32_bits(dst);
+					cmd[4] = FIELD_PREP(CH_XY_SRC, dsg->xsize) |
+						 FIELD_PREP(CH_XY_DES, dsg->xsize);
+					la_cmd = &cmd[5];
+				} else {
+					cmd[0] = LINK_SRCADDR | LINK_DESADDR |
+						 LINK_XSIZE | LINK_LINKADDR;
+					cmd[1] = lower_32_bits(src);
+					cmd[2] = lower_32_bits(dst);
+					cmd[3] = FIELD_PREP(CH_XY_SRC, dsg->xsize) |
+						 FIELD_PREP(CH_XY_DES, dsg->xsize);
+					la_cmd = &cmd[4];
+				}
+			}
+
+			len -= xfer_len;
+			mem_addr += xfer_len;
+			sglen++;
+		} while (len);
+	}
+
+	/* the last command */
+	*la_cmd = 0;
+	desc->sg[sglen - 1].command[1] |= FIELD_PREP(CH_CTRL_DONETYPE, CH_CTRL_DONETYPE_CMD);
+
+	mb();
+
+	return vchan_tx_prep(&dch->vc, &desc->vd, flags);
+
+err_cmd_alloc:
+	for (i = 0; i < sglen; i++)
+		dma_pool_free(dch->cmd_pool, desc->sg[i].command, desc->sg[i].phys);
+	kfree(desc);
+	return NULL;
+}
+
+static struct dma_async_tx_descriptor *
+d250_prep_cyclic(struct dma_chan *chan, dma_addr_t buf_addr,
+		 size_t buf_len, size_t period_len, enum dma_transfer_direction dir,
+		 unsigned long flags)
+{
+	struct d350_chan *dch = to_d350_chan(chan);
+	u32 len, periods, trig, *cmd, tsz;
+	dma_addr_t src, dst, phys, mem_addr;
+	size_t xfer_len, step_max;
+	struct d350_desc *desc;
+	struct scatterlist *sg;
+	struct d350_sg *dsg;
+	int sglen, i;
+
+	if (unlikely(!is_slave_direction(dir) || !buf_len || !period_len))
+		return NULL;
+
+	if (dir == DMA_MEM_TO_DEV)
+		tsz = __ffs(dch->config.dst_addr_width | (1 << dch->tsz));
+	else
+		tsz = __ffs(dch->config.src_addr_width | (1 << dch->tsz));
+	step_max = ((1UL << 16) - 1) << tsz;
+
+	periods = buf_len / period_len;
+	sglen = DIV_ROUND_UP(sg_dma_len(sg), step_max) * periods;
+
+	desc = kzalloc(struct_size(desc, sg, sglen), GFP_NOWAIT);
+	if (!desc)
+		return NULL;
+
+	dch->cyclic = true;
+	dch->periods = periods;
+	desc->sglen = sglen;
+
+	sglen = 0;
+	for (i = 0; i < periods; i++) {
+		len = period_len;
+		mem_addr = buf_addr + i * period_len;
+		do {
+			desc->sg[sglen].command = dma_pool_zalloc(dch->cmd_pool, GFP_NOWAIT, &phys);
+			if (unlikely(!desc->sg[sglen].command))
+				goto err_cmd_alloc;
+
+			xfer_len = (len > step_max) ? step_max : len;
+			desc->sg[sglen].phys = phys;
+			dsg = &desc->sg[sglen];
+
+			if (dir == DMA_MEM_TO_DEV) {
+				src = mem_addr;
+				dst = dch->config.dst_addr;
+				trig = CH_CTRL_USEDESTRIGIN;
+			} else {
+				src = dch->config.src_addr;
+				dst = mem_addr;
+				trig = CH_CTRL_USESRCTRIGIN;
+			}
+			dsg->tsz = tsz;
+			dsg->xsize = lower_16_bits(xfer_len >> dsg->tsz);
+
+			cmd = dsg->command;
+			cmd[0] = LINK_CTRL | LINK_SRCADDR | LINK_DESADDR |
+				 LINK_XSIZE | LINK_SRCTRANSCFG |
+				 LINK_DESTRANSCFG | LINK_XADDRINC | LINK_LINKADDR;
+
+			cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, dsg->tsz) |
+				 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_CONTINUE) |
+				 FIELD_PREP(CH_CTRL_DONETYPE, CH_CTRL_DONETYPE_CMD) | trig;
+
+			cmd[2] = lower_32_bits(src);
+			cmd[3] = lower_32_bits(dst);
+			cmd[4] = FIELD_PREP(CH_XY_SRC, dsg->xsize) |
+				 FIELD_PREP(CH_XY_DES, dsg->xsize);
+			if (dir == DMA_MEM_TO_DEV) {
+				cmd[0] |= LINK_DESTRIGINCFG;
+				cmd[5] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
+				cmd[6] = TRANSCFG_DEVICE;
+				cmd[7] = FIELD_PREP(CH_XY_SRC, 1);
+				cmd[8] = FIELD_PREP(CH_DESTRIGINMODE, CH_DESTRIG_DMA_FC) |
+					  FIELD_PREP(CH_DESTRIGINTYPE, CH_DESTRIG_HW_REQ);
+			} else {
+				cmd[0] |= LINK_SRCTRIGINCFG;
+				cmd[5] = TRANSCFG_DEVICE;
+				cmd[6] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
+				cmd[7] = FIELD_PREP(CH_XY_DES, 1);
+				cmd[8] = FIELD_PREP(CH_SRCTRIGINMODE, CH_SRCTRIG_DMA_FC) |
+					  FIELD_PREP(CH_SRCTRIGINTYPE, CH_SRCTRIG_HW_REQ);
+			}
+
+			if (sglen)
+				desc->sg[sglen - 1].command[9] = phys | CH_LINKADDR_EN;
+
+			len -= xfer_len;
+			mem_addr += xfer_len;
+			sglen++;
+		} while (len);
+		desc->sg[sglen - 1].command[1] |= FIELD_PREP(CH_CTRL_DONETYPE,
+							     CH_CTRL_DONETYPE_CMD);
+	}
+
+	/* cyclic list */
+	desc->sg[sglen - 1].command[9] = desc->sg[0].phys | CH_LINKADDR_EN;
+
+	mb();
+
+	return vchan_tx_prep(&dch->vc, &desc->vd, flags);
+
+err_cmd_alloc:
+	for (i = 0; i < sglen; i++)
+		dma_pool_free(dch->cmd_pool, desc->sg[i].command, desc->sg[i].phys);
+	kfree(desc);
+	return NULL;
+}
+
 static int d350_pause(struct dma_chan *chan)
 {
 	struct d350_chan *dch = to_d350_chan(chan);
@@ -620,20 +970,31 @@ static u32 d350_get_residue(struct d350_chan *dch)
 	u32 res, xsize, xsizehi, linkaddr, linkaddrhi, hi_new;
 	int i, sgcur, retries = 3; /* 1st time unlucky, 2nd improbable, 3rd just broken */
 	struct d350_desc *desc = dch->desc;
+	struct d350 *dmac = to_d350(dch->vc.chan.device);
 
-	hi_new = readl_relaxed(dch->base + CH_XSIZEHI);
-	do {
-		xsizehi = hi_new;
-		xsize = readl_relaxed(dch->base + CH_XSIZE);
+	if (dch->has_xsizehi) {
 		hi_new = readl_relaxed(dch->base + CH_XSIZEHI);
-	} while (xsizehi != hi_new && --retries);
+		do {
+			xsizehi = hi_new;
+			xsize = readl_relaxed(dch->base + CH_XSIZE);
+			hi_new = readl_relaxed(dch->base + CH_XSIZEHI);
+		} while (xsizehi != hi_new && --retries);
+	} else {
+		xsize = readl_relaxed(dch->base + CH_XSIZE);
+		xsizehi = 0;
+	}
 
-	hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
-	do {
-		linkaddrhi = hi_new;
-		linkaddr = readl_relaxed(dch->base + CH_LINKADDR);
+	if (!dmac->is_d250) {
 		hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
-	} while (linkaddrhi != hi_new && --retries);
+		do {
+			linkaddrhi = hi_new;
+			linkaddr = readl_relaxed(dch->base + CH_LINKADDR);
+			hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
+		} while (linkaddrhi != hi_new && --retries);
+	} else {
+		linkaddr = readl_relaxed(dch->base + CH_LINKADDR);
+		linkaddrhi = 0;
+	}
 
 	for (i = 0; i < desc->sglen; i++) {
 		if (desc->sg[i].phys == (((u64)linkaddrhi << 32) | (linkaddr & ~CH_LINKADDR_EN)))
@@ -876,6 +1237,14 @@ static void d350_free_chan_resources(struct dma_chan *chan)
 	dch->cmd_pool = NULL;
 }
 
+static void d250_cntx_mem_release(void *ptr)
+{
+	struct d350 *dmac = ptr;
+	struct device *dev = dmac->dma.dev;
+
+	dma_free_coherent(dev, dmac->cntx_mem_size, dmac->cntx_mem, dmac->cntx_mem_paddr);
+}
+
 static int d350_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -893,8 +1262,9 @@ static int d350_probe(struct platform_device *pdev)
 	r = FIELD_GET(IIDR_VARIANT, reg);
 	p = FIELD_GET(IIDR_REVISION, reg);
 	if (FIELD_GET(IIDR_IMPLEMENTER, reg) != IMPLEMENTER_ARM ||
-	    FIELD_GET(IIDR_PRODUCTID, reg) != PRODUCTID_DMA350)
-		return dev_err_probe(dev, -ENODEV, "Not a DMA-350!");
+	    ((FIELD_GET(IIDR_PRODUCTID, reg) != PRODUCTID_DMA350) &&
+	    FIELD_GET(IIDR_PRODUCTID, reg) != PRODUCTID_DMA250))
+		return dev_err_probe(dev, -ENODEV, "Not a DMA-350/DMA-250!");
 
 	reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG0);
 	nchan = FIELD_GET(DMA_CFG_NUM_CHANNELS, reg) + 1;
@@ -917,13 +1287,38 @@ static int d350_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	if (device_is_compatible(dev, "arm,dma-250")) {
+		u32 cfg2;
+		int secext_present;
+
+		dmac->is_d250 = true;
+
+		cfg2 = readl_relaxed(base + DMAINFO + DMA_BUILDCFG2);
+		secext_present = (cfg2 & DMA_CFG_HAS_TZ) ? 1 : 0;
+		dmac->cntx_mem_size = nchan * 64 * (1 + secext_present);
+		dmac->cntx_mem = dma_alloc_coherent(dev, dmac->cntx_mem_size,
+						    &dmac->cntx_mem_paddr,
+						    GFP_KERNEL);
+		if (!dmac->cntx_mem)
+			return dev_err_probe(dev, -ENOMEM, "Failed to alloc context memory\n");
+
+		ret = devm_add_action_or_reset(dev, d250_cntx_mem_release, dmac);
+		if (ret) {
+			dma_free_coherent(dev, dmac->cntx_mem_size,
+					  dmac->cntx_mem, dmac->cntx_mem_paddr);
+			return ret;
+		}
+		writel_relaxed(dmac->cntx_mem_paddr, base + DMANSECCTRL + NSEC_CNTXBASE);
+	}
+
 	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(aw));
 	coherent = device_get_dma_attr(dev) == DEV_DMA_COHERENT;
 
 	reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG1);
 	dmac->nreq = FIELD_GET(DMA_CFG_NUM_TRIGGER_IN, reg);
 
-	dev_dbg(dev, "DMA-350 r%dp%d with %d channels, %d requests\n", r, p, dmac->nchan, dmac->nreq);
+	dev_info(dev, "%s r%dp%d with %d channels, %d requests\n",
+		 dmac->is_d250 ? "DMA-250" : "DMA-350", r, p, dmac->nchan, dmac->nreq);
 
 	for (int i = min(dw, 16); i > 0; i /= 2) {
 		dmac->dma.src_addr_widths |= BIT(i);
@@ -935,7 +1330,10 @@ static int d350_probe(struct platform_device *pdev)
 	dmac->dma.device_alloc_chan_resources = d350_alloc_chan_resources;
 	dmac->dma.device_free_chan_resources = d350_free_chan_resources;
 	dma_cap_set(DMA_MEMCPY, dmac->dma.cap_mask);
-	dmac->dma.device_prep_dma_memcpy = d350_prep_memcpy;
+	if (dmac->is_d250)
+		dmac->dma.device_prep_dma_memcpy = d250_prep_memcpy;
+	else
+		dmac->dma.device_prep_dma_memcpy = d350_prep_memcpy;
 	dmac->dma.device_pause = d350_pause;
 	dmac->dma.device_resume = d350_resume;
 	dmac->dma.device_terminate_all = d350_terminate_all;
@@ -971,8 +1369,8 @@ static int d350_probe(struct platform_device *pdev)
 			return dch->irq;
 
 		dch->has_wrap = FIELD_GET(CH_CFG_HAS_WRAP, reg);
-		dch->has_trig = FIELD_GET(CH_CFG_HAS_TRIGIN, reg) &
-				FIELD_GET(CH_CFG_HAS_TRIGSEL, reg);
+		dch->has_xsizehi = FIELD_GET(CH_CFG_HAS_XSIZEHI, reg);
+		dch->has_trig = FIELD_GET(CH_CFG_HAS_TRIGIN, reg);
 
 		/* Fill is a special case of Wrap */
 		memset &= dch->has_wrap;
@@ -994,8 +1392,13 @@ static int d350_probe(struct platform_device *pdev)
 		dma_cap_set(DMA_SLAVE, dmac->dma.cap_mask);
 		dma_cap_set(DMA_CYCLIC, dmac->dma.cap_mask);
 		dmac->dma.device_config = d350_slave_config;
-		dmac->dma.device_prep_slave_sg = d350_prep_slave_sg;
-		dmac->dma.device_prep_dma_cyclic = d350_prep_cyclic;
+		if (dmac->is_d250) {
+			dmac->dma.device_prep_slave_sg = d250_prep_slave_sg;
+			dmac->dma.device_prep_dma_cyclic = d250_prep_cyclic;
+		} else {
+			dmac->dma.device_prep_slave_sg = d350_prep_slave_sg;
+			dmac->dma.device_prep_dma_cyclic = d350_prep_cyclic;
+		}
 	}
 
 	if (memset) {
@@ -1019,6 +1422,7 @@ static void d350_remove(struct platform_device *pdev)
 
 static const struct of_device_id d350_of_match[] __maybe_unused = {
 	{ .compatible = "arm,dma-350" },
+	{ .compatible = "arm,dma-250" },
 	{}
 };
 MODULE_DEVICE_TABLE(of, d350_of_match);
@@ -1035,5 +1439,5 @@ module_platform_driver(d350_driver);
 
 MODULE_AUTHOR("Robin Murphy <robin.murphy@arm.com>");
 MODULE_AUTHOR("Jisheng Zhang <jszhang@kernel.org>");
-MODULE_DESCRIPTION("Arm DMA-350 driver");
+MODULE_DESCRIPTION("Arm DMA-350/DMA-250 driver");
 MODULE_LICENSE("GPL v2");
-- 
2.50.0

Re: [PATCH 14/14] dmaengine: dma350: Support ARM DMA-250

[Krzysztof Kozlowski]

On 23/08/2025 17:40, Jisheng Zhang wrote:
>  	struct device *dev = &pdev->dev;
> @@ -893,8 +1262,9 @@ static int d350_probe(struct platform_device *pdev)
>  	r = FIELD_GET(IIDR_VARIANT, reg);
>  	p = FIELD_GET(IIDR_REVISION, reg);
>  	if (FIELD_GET(IIDR_IMPLEMENTER, reg) != IMPLEMENTER_ARM ||
> -	    FIELD_GET(IIDR_PRODUCTID, reg) != PRODUCTID_DMA350)
> -		return dev_err_probe(dev, -ENODEV, "Not a DMA-350!");
> +	    ((FIELD_GET(IIDR_PRODUCTID, reg) != PRODUCTID_DMA350) &&
> +	    FIELD_GET(IIDR_PRODUCTID, reg) != PRODUCTID_DMA250))
> +		return dev_err_probe(dev, -ENODEV, "Not a DMA-350/DMA-250!");
>  
>  	reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG0);
>  	nchan = FIELD_GET(DMA_CFG_NUM_CHANNELS, reg) + 1;
> @@ -917,13 +1287,38 @@ static int d350_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	if (device_is_compatible(dev, "arm,dma-250")) {

No, don't sprinkle compatibles through driver code. driver match data is
for that.

> +		u32 cfg2;
> +		int secext_present;
> +
> +		dmac->is_d250 = true;
> +
> +		cfg2 = readl_relaxed(base + DMAINFO + DMA_BUILDCFG2);
> +		secext_present = (cfg2 & DMA_CFG_HAS_TZ) ? 1 : 0;
> +		dmac->cntx_mem_size = nchan * 64 * (1 + secext_present);
> +		dmac->cntx_mem = dma_alloc_coherent(dev, dmac->cntx_mem_size,
> +						    &dmac->cntx_mem_paddr,
> +						    GFP_KERNEL);
> +		if (!dmac->cntx_mem)
> +			return dev_err_probe(dev, -ENOMEM, "Failed to alloc context memory\n");
> +
> +		ret = devm_add_action_or_reset(dev, d250_cntx_mem_release, dmac);
> +		if (ret) {
> +			dma_free_coherent(dev, dmac->cntx_mem_size,
> +					  dmac->cntx_mem, dmac->cntx_mem_paddr);
> +			return ret;
> +		}
> +		writel_relaxed(dmac->cntx_mem_paddr, base + DMANSECCTRL + NSEC_CNTXBASE);
> +	}
> +
>  	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(aw));
>  	coherent = device_get_dma_attr(dev) == DEV_DMA_COHERENT;
>  
>  	reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG1);
>  	dmac->nreq = FIELD_GET(DMA_CFG_NUM_TRIGGER_IN, reg);
>  
> -	dev_dbg(dev, "DMA-350 r%dp%d with %d channels, %d requests\n", r, p, dmac->nchan, dmac->nreq);
> +	dev_info(dev, "%s r%dp%d with %d channels, %d requests\n",
> +		 dmac->is_d250 ? "DMA-250" : "DMA-350", r, p, dmac->nchan, dmac->nreq);

No, don't makek drivers more verbose. Please follow Linux driver
design/coding style - this should be silent on success.

>  
>  	for (int i = min(dw, 16); i > 0; i /= 2) {
>  		dmac->dma.src_addr_widths |= BIT(i);
> @@ -935,7 +1330,10 @@ static int d350_probe(struct platform_device *pdev)
>  	dmac->dma.device_alloc_chan_resources = d350_alloc_chan_resources;
>  	dmac->dma.device_free_chan_resources = d350_free_chan_resources;
>  	dma_cap_set(DMA_MEMCPY, dmac->dma.cap_mask);
> -	dmac->dma.device_prep_dma_memcpy = d350_prep_memcpy;
> +	if (dmac->is_d250)
> +		dmac->dma.device_prep_dma_memcpy = d250_prep_memcpy;
> +	else
> +		dmac->dma.device_prep_dma_memcpy = d350_prep_memcpy;
>  	dmac->dma.device_pause = d350_pause;
>  	dmac->dma.device_resume = d350_resume;
>  	dmac->dma.device_terminate_all = d350_terminate_all;
> @@ -971,8 +1369,8 @@ static int d350_probe(struct platform_device *pdev)
>  			return dch->irq;
>  
>  		dch->has_wrap = FIELD_GET(CH_CFG_HAS_WRAP, reg);
> -		dch->has_trig = FIELD_GET(CH_CFG_HAS_TRIGIN, reg) &
> -				FIELD_GET(CH_CFG_HAS_TRIGSEL, reg);
> +		dch->has_xsizehi = FIELD_GET(CH_CFG_HAS_XSIZEHI, reg);
> +		dch->has_trig = FIELD_GET(CH_CFG_HAS_TRIGIN, reg);
>  
>  		/* Fill is a special case of Wrap */
>  		memset &= dch->has_wrap;
> @@ -994,8 +1392,13 @@ static int d350_probe(struct platform_device *pdev)
>  		dma_cap_set(DMA_SLAVE, dmac->dma.cap_mask);
>  		dma_cap_set(DMA_CYCLIC, dmac->dma.cap_mask);
>  		dmac->dma.device_config = d350_slave_config;
> -		dmac->dma.device_prep_slave_sg = d350_prep_slave_sg;
> -		dmac->dma.device_prep_dma_cyclic = d350_prep_cyclic;
> +		if (dmac->is_d250) {
> +			dmac->dma.device_prep_slave_sg = d250_prep_slave_sg;
> +			dmac->dma.device_prep_dma_cyclic = d250_prep_cyclic;
> +		} else {
> +			dmac->dma.device_prep_slave_sg = d350_prep_slave_sg;
> +			dmac->dma.device_prep_dma_cyclic = d350_prep_cyclic;
> +		}
>  	}
>  
>  	if (memset) {
> @@ -1019,6 +1422,7 @@ static void d350_remove(struct platform_device *pdev)
>  
>  static const struct of_device_id d350_of_match[] __maybe_unused = {
>  	{ .compatible = "arm,dma-350" },
> +	{ .compatible = "arm,dma-250" },

And based on that devices would be compatible...

BTW, incorrect order - 2 < 3.



Best regards,
Krzysztof

Re: [PATCH 14/14] dmaengine: dma350: Support ARM DMA-250

[kernel test robot]

Hi Jisheng,

kernel test robot noticed the following build warnings:

[auto build test WARNING on vkoul-dmaengine/next]
[also build test WARNING on robh/for-next krzk-dt/for-next linus/master v6.17-rc2 next-20250822]
[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/Jisheng-Zhang/dmaengine-dma350-Fix-CH_CTRL_USESRCTRIGIN-definition/20250824-000425
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git next
patch link:    https://lore.kernel.org/r/20250823154009.25992-15-jszhang%40kernel.org
patch subject: [PATCH 14/14] dmaengine: dma350: Support ARM DMA-250
config: x86_64-buildonly-randconfig-002-20250824 (https://download.01.org/0day-ci/archive/20250825/202508250351.vxyvTsJa-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250825/202508250351.vxyvTsJa-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/202508250351.vxyvTsJa-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/dma/arm-dma350.c:849:34: warning: variable 'sg' is uninitialized when used here [-Wuninitialized]
     849 |         sglen = DIV_ROUND_UP(sg_dma_len(sg), step_max) * periods;
         |                                         ^~
   include/linux/scatterlist.h:34:27: note: expanded from macro 'sg_dma_len'
      34 | #define sg_dma_len(sg)          ((sg)->dma_length)
         |                                   ^~
   include/uapi/linux/const.h:51:40: note: expanded from macro '__KERNEL_DIV_ROUND_UP'
      51 | #define __KERNEL_DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
         |                                        ^
   drivers/dma/arm-dma350.c:835:24: note: initialize the variable 'sg' to silence this warning
     835 |         struct scatterlist *sg;
         |                               ^
         |                                = NULL
   1 warning generated.


vim +/sg +849 drivers/dma/arm-dma350.c

   824	
   825	static struct dma_async_tx_descriptor *
   826	d250_prep_cyclic(struct dma_chan *chan, dma_addr_t buf_addr,
   827			 size_t buf_len, size_t period_len, enum dma_transfer_direction dir,
   828			 unsigned long flags)
   829	{
   830		struct d350_chan *dch = to_d350_chan(chan);
   831		u32 len, periods, trig, *cmd, tsz;
   832		dma_addr_t src, dst, phys, mem_addr;
   833		size_t xfer_len, step_max;
   834		struct d350_desc *desc;
   835		struct scatterlist *sg;
   836		struct d350_sg *dsg;
   837		int sglen, i;
   838	
   839		if (unlikely(!is_slave_direction(dir) || !buf_len || !period_len))
   840			return NULL;
   841	
   842		if (dir == DMA_MEM_TO_DEV)
   843			tsz = __ffs(dch->config.dst_addr_width | (1 << dch->tsz));
   844		else
   845			tsz = __ffs(dch->config.src_addr_width | (1 << dch->tsz));
   846		step_max = ((1UL << 16) - 1) << tsz;
   847	
   848		periods = buf_len / period_len;
 > 849		sglen = DIV_ROUND_UP(sg_dma_len(sg), step_max) * periods;
   850	
   851		desc = kzalloc(struct_size(desc, sg, sglen), GFP_NOWAIT);
   852		if (!desc)
   853			return NULL;
   854	
   855		dch->cyclic = true;
   856		dch->periods = periods;
   857		desc->sglen = sglen;
   858	
   859		sglen = 0;
   860		for (i = 0; i < periods; i++) {
   861			len = period_len;
   862			mem_addr = buf_addr + i * period_len;
   863			do {
   864				desc->sg[sglen].command = dma_pool_zalloc(dch->cmd_pool, GFP_NOWAIT, &phys);
   865				if (unlikely(!desc->sg[sglen].command))
   866					goto err_cmd_alloc;
   867	
   868				xfer_len = (len > step_max) ? step_max : len;
   869				desc->sg[sglen].phys = phys;
   870				dsg = &desc->sg[sglen];
   871	
   872				if (dir == DMA_MEM_TO_DEV) {
   873					src = mem_addr;
   874					dst = dch->config.dst_addr;
   875					trig = CH_CTRL_USEDESTRIGIN;
   876				} else {
   877					src = dch->config.src_addr;
   878					dst = mem_addr;
   879					trig = CH_CTRL_USESRCTRIGIN;
   880				}
   881				dsg->tsz = tsz;
   882				dsg->xsize = lower_16_bits(xfer_len >> dsg->tsz);
   883	
   884				cmd = dsg->command;
   885				cmd[0] = LINK_CTRL | LINK_SRCADDR | LINK_DESADDR |
   886					 LINK_XSIZE | LINK_SRCTRANSCFG |
   887					 LINK_DESTRANSCFG | LINK_XADDRINC | LINK_LINKADDR;
   888	
   889				cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, dsg->tsz) |
   890					 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_CONTINUE) |
   891					 FIELD_PREP(CH_CTRL_DONETYPE, CH_CTRL_DONETYPE_CMD) | trig;
   892	
   893				cmd[2] = lower_32_bits(src);
   894				cmd[3] = lower_32_bits(dst);
   895				cmd[4] = FIELD_PREP(CH_XY_SRC, dsg->xsize) |
   896					 FIELD_PREP(CH_XY_DES, dsg->xsize);
   897				if (dir == DMA_MEM_TO_DEV) {
   898					cmd[0] |= LINK_DESTRIGINCFG;
   899					cmd[5] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
   900					cmd[6] = TRANSCFG_DEVICE;
   901					cmd[7] = FIELD_PREP(CH_XY_SRC, 1);
   902					cmd[8] = FIELD_PREP(CH_DESTRIGINMODE, CH_DESTRIG_DMA_FC) |
   903						  FIELD_PREP(CH_DESTRIGINTYPE, CH_DESTRIG_HW_REQ);
   904				} else {
   905					cmd[0] |= LINK_SRCTRIGINCFG;
   906					cmd[5] = TRANSCFG_DEVICE;
   907					cmd[6] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
   908					cmd[7] = FIELD_PREP(CH_XY_DES, 1);
   909					cmd[8] = FIELD_PREP(CH_SRCTRIGINMODE, CH_SRCTRIG_DMA_FC) |
   910						  FIELD_PREP(CH_SRCTRIGINTYPE, CH_SRCTRIG_HW_REQ);
   911				}
   912	
   913				if (sglen)
   914					desc->sg[sglen - 1].command[9] = phys | CH_LINKADDR_EN;
   915	
   916				len -= xfer_len;
   917				mem_addr += xfer_len;
   918				sglen++;
   919			} while (len);
   920			desc->sg[sglen - 1].command[1] |= FIELD_PREP(CH_CTRL_DONETYPE,
   921								     CH_CTRL_DONETYPE_CMD);
   922		}
   923	
   924		/* cyclic list */
   925		desc->sg[sglen - 1].command[9] = desc->sg[0].phys | CH_LINKADDR_EN;
   926	
   927		mb();
   928	
   929		return vchan_tx_prep(&dch->vc, &desc->vd, flags);
   930	
   931	err_cmd_alloc:
   932		for (i = 0; i < sglen; i++)
   933			dma_pool_free(dch->cmd_pool, desc->sg[i].command, desc->sg[i].phys);
   934		kfree(desc);
   935		return NULL;
   936	}
   937	

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

Re: [PATCH 14/14] dmaengine: dma350: Support ARM DMA-250

[Robin Murphy]

On 2025-08-23 4:40 pm, Jisheng Zhang wrote:
> Compared with ARM DMA-350, DMA-250 is a simplified version. They share
> many common parts, but they do have difference. Add DMA-250 support
> by handling their difference by using different device_prep_slave_sg,
> device_prep_dma_cyclic and device_prep_dma_memcpy. DMA-250 doesn't
> support device_prep_dma_memset.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>   drivers/dma/arm-dma350.c | 444 +++++++++++++++++++++++++++++++++++++--
>   1 file changed, 424 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index 5abb965c6687..0ee807424b7e 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c
> @@ -1,7 +1,7 @@
>   // SPDX-License-Identifier: GPL-2.0
>   // Copyright (C) 2024-2025 Arm Limited
>   // Copyright (C) 2025 Synaptics Incorporated
> -// Arm DMA-350 driver
> +// Arm DMA-350/DMA-250 driver

Yeah, that's going to get old fast... By all means update the Kconfig 
help text if you think it's helpful to end users, but I don't think 
anyone expects comments like this to be exhaustive, so honestly I'd save 
the churn.
>   #include <linux/bitfield.h>
>   #include <linux/dmaengine.h>
> @@ -16,6 +16,10 @@
>   #include "dmaengine.h"
>   #include "virt-dma.h"
>   
> +#define DMANSECCTRL		0x0200
> +
> +#define NSEC_CNTXBASE		0x10
> +
>   #define DMAINFO			0x0f00
>   
>   #define DMA_BUILDCFG0		0xb0
> @@ -26,12 +30,16 @@
>   #define DMA_BUILDCFG1		0xb4
>   #define DMA_CFG_NUM_TRIGGER_IN	GENMASK(8, 0)
>   
> +#define DMA_BUILDCFG2		0xb8
> +#define DMA_CFG_HAS_TZ		BIT(8)I don't think we need to care about that. Yes, the TRM describes the 
total context memory size required from the PoV of the hardware itself, 
but even if SEC_CNTXBASE does exist, Non-Secure Linux can't set it, so 
clearly Linux can't need to provide memory for it.

> +
>   #define IIDR			0xc8
>   #define IIDR_PRODUCTID		GENMASK(31, 20)
>   #define IIDR_VARIANT		GENMASK(19, 16)
>   #define IIDR_REVISION		GENMASK(15, 12)
>   #define IIDR_IMPLEMENTER	GENMASK(11, 0)
>   
> +#define PRODUCTID_DMA250	0x250
>   #define PRODUCTID_DMA350	0x3a0
>   #define IMPLEMENTER_ARM		0x43b
>   
> @@ -140,6 +148,7 @@
>   #define CH_CFG_HAS_TRIGSEL	BIT(7)
>   #define CH_CFG_HAS_TRIGIN	BIT(5)
>   #define CH_CFG_HAS_WRAP		BIT(1)
> +#define CH_CFG_HAS_XSIZEHI	BIT(0)
>   
>   
>   #define LINK_REGCLEAR		BIT(0)
> @@ -218,6 +227,7 @@ struct d350_chan {
>   	bool cyclic;
>   	bool has_trig;
>   	bool has_wrap;
> +	bool has_xsizehi;
>   	bool coherent;
>   };
>   
> @@ -225,6 +235,10 @@ struct d350 {
>   	struct dma_device dma;
>   	int nchan;
>   	int nreq;
> +	bool is_d250;

That won't scale, but it also shouldn't be needed anyway - other than 
the context memory which is easily handled within the scope of the probe 
routine that already has the IIDR to hand, everything else ought to be 
based on the relevant feature flags.

> +	dma_addr_t cntx_mem_paddr;
> +	void *cntx_mem;
> +	u32 cntx_mem_size;
>   	struct d350_chan channels[] __counted_by(nchan);
>   };
>   
> @@ -238,6 +252,11 @@ static inline struct d350_desc *to_d350_desc(struct virt_dma_desc *vd)
>   	return container_of(vd, struct d350_desc, vd);
>   }
>   
> +static inline struct d350 *to_d350(struct dma_device *dd)
> +{
> +	return container_of(dd, struct d350, dma);
> +}
> +
>   static void d350_desc_free(struct virt_dma_desc *vd)
>   {
>   	struct d350_chan *dch = to_d350_chan(vd->tx.chan);
> @@ -585,6 +604,337 @@ static int d350_slave_config(struct dma_chan *chan, struct dma_slave_config *con
>   	return 0;
>   }
>   
> +static struct dma_async_tx_descriptor *d250_prep_memcpy(struct dma_chan *chan,
> +		dma_addr_t dest, dma_addr_t src, size_t len, unsigned long flags)

Case in point: We don't need a mess of separate copy-pasted functions, 
we just need to evolve the existing ones to split the respective 
operations into either 32-bit or 16-bit chunks depending on has_xsizehi 
- even on DMA-350, >32-bit sizes aren't properly supported since I never 
got as far as command linking, but there's no reason they shouldn't be.

> +{
> +	struct d350_chan *dch = to_d350_chan(chan);
> +	struct d350_desc *desc;
> +	u32 *cmd, *la_cmd, tsz;
> +	int sglen, i;
> +	struct d350_sg *sg;
> +	size_t xfer_len, step_max;
> +	dma_addr_t phys;
> +
> +	tsz = __ffs(len | dest | src | (1 << dch->tsz));
> +	step_max = ((1UL << 16) - 1) << tsz;
> +	sglen = DIV_ROUND_UP(len, step_max);
> +
> +	desc = kzalloc(struct_size(desc, sg, sglen), GFP_NOWAIT);
> +	if (!desc)
> +		return NULL;
> +
> +	desc->sglen = sglen;
> +	sglen = 0;
> +	while (len) {
> +		sg = &desc->sg[sglen];
> +		xfer_len = (len > step_max) ? step_max : len;

If only we had a min() function...

> +		sg->tsz = __ffs(xfer_len | dest | src | (1 << dch->tsz));

Um, what? By this point we've already decided to split based on the 
initial tsz, what purpose does recalculating it serve?

> +		sg->xsize = lower_16_bits(xfer_len >> sg->tsz);
> +
> +		sg->command = dma_pool_zalloc(dch->cmd_pool, GFP_NOWAIT, &phys);
> +		if (unlikely(!sg->command))
> +			goto err_cmd_alloc;
> +		sg->phys = phys;
> +
> +		cmd = sg->command;
> +		if (!sglen) {
> +			cmd[0] = LINK_CTRL | LINK_SRCADDR | LINK_DESADDR |
> +				 LINK_XSIZE | LINK_SRCTRANSCFG |
> +				 LINK_DESTRANSCFG | LINK_XADDRINC | LINK_LINKADDR;
> +
> +			cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, sg->tsz) |
> +				 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_CONTINUE);
> +
> +			cmd[2] = lower_32_bits(src);
> +			cmd[3] = lower_32_bits(dest);
> +			cmd[4] = FIELD_PREP(CH_XY_SRC, sg->xsize) |
> +				 FIELD_PREP(CH_XY_DES, sg->xsize);
> +			cmd[5] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
> +			cmd[6] = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
> +			cmd[7] = FIELD_PREP(CH_XY_SRC, 1) | FIELD_PREP(CH_XY_DES, 1);
> +			la_cmd = &cmd[8];
> +		} else {
> +			*la_cmd = phys | CH_LINKADDR_EN;
> +			if (len <= step_max) {
> +				cmd[0] = LINK_CTRL | LINK_XSIZE | LINK_LINKADDR;
> +				cmd[1] = FIELD_PREP(CH_CTRL_TRANSIZE, sg->tsz) |
> +					 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_CONTINUE);
> +				cmd[2] = FIELD_PREP(CH_XY_SRC, sg->xsize) |
> +					 FIELD_PREP(CH_XY_DES, sg->xsize);
> +				la_cmd = &cmd[3];
> +			} else {
> +				cmd[0] = LINK_XSIZE | LINK_LINKADDR;
> +				cmd[1] = FIELD_PREP(CH_XY_SRC, sg->xsize) |
> +					 FIELD_PREP(CH_XY_DES, sg->xsize);
> +				la_cmd = &cmd[2];
> +			}

Ok, we really need to figure out a better abstraction for command 
construction, the hard-coded array indices were a bad enough idea to 
start with, but this is almost impossible to make sense of.

> +		}
> +
> +		len -= xfer_len;
> +		src += xfer_len;
> +		dest += xfer_len;
> +		sglen++;
> +	}
> +
> +	/* the last cmdlink */
> +	*la_cmd = 0;
> +	desc->sg[sglen - 1].command[1] |= FIELD_PREP(CH_CTRL_DONETYPE, CH_CTRL_DONETYPE_CMD);

As for that, I don't even...

Furthermore, all these loops and conditionals are crazy anyway, and 
thoroughly failing to do justice to the hardware actually being pretty 
cool, namely that *commands can loop themselves*! Any single buffer/sg 
segment should take at most two commands - one dividing as much of the 
length as possible between XSIZE{HI} and CMDRESTARTCOUNT using 
REGRELOADTYPE=1, and/or one to transfer whatever non-multiple tail 
portion remains.

Honestly I'm sad the project for which I originally started this driver 
got canned, as this is the part I was really looking forward to having 
some fun with...

[...]
>   static int d350_pause(struct dma_chan *chan)
>   {
>   	struct d350_chan *dch = to_d350_chan(chan);
> @@ -620,20 +970,31 @@ static u32 d350_get_residue(struct d350_chan *dch)
>   	u32 res, xsize, xsizehi, linkaddr, linkaddrhi, hi_new;
>   	int i, sgcur, retries = 3; /* 1st time unlucky, 2nd improbable, 3rd just broken */
>   	struct d350_desc *desc = dch->desc;
> +	struct d350 *dmac = to_d350(dch->vc.chan.device);
>   
> -	hi_new = readl_relaxed(dch->base + CH_XSIZEHI);
> -	do {
> -		xsizehi = hi_new;
> -		xsize = readl_relaxed(dch->base + CH_XSIZE);
> +	if (dch->has_xsizehi) {
>   		hi_new = readl_relaxed(dch->base + CH_XSIZEHI);
> -	} while (xsizehi != hi_new && --retries);
> +		do {
> +			xsizehi = hi_new;
> +			xsize = readl_relaxed(dch->base + CH_XSIZE);
> +			hi_new = readl_relaxed(dch->base + CH_XSIZEHI);
> +		} while (xsizehi != hi_new && --retries);
> +	} else {
> +		xsize = readl_relaxed(dch->base + CH_XSIZE);
> +		xsizehi = 0;
> +	}
This is unnecessary - if the CH_XSIZEHI location isn't the actual 
register then it's RAZ/WI, which means the existing logic can take full 
advantage of it reading as zero and still work just the same.

> -	hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
> -	do {
> -		linkaddrhi = hi_new;
> -		linkaddr = readl_relaxed(dch->base + CH_LINKADDR);
> +	if (!dmac->is_d250) {

And similarly here. The only thing we should perhaps do specially for 
LINKADDRHI is omit it from command generation when ADDR_WIDTH <= 32 in 
general. I admit I was lazy there, since it's currently harmless for 
d350_start_next() to write the register location unconditionally, but 
I'm not sure how a 32-bit DMA-350 would handle it in an actual command 
link header.

>   		hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
> -	} while (linkaddrhi != hi_new && --retries);
> +		do {
> +			linkaddrhi = hi_new;
> +			linkaddr = readl_relaxed(dch->base + CH_LINKADDR);
> +			hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
> +		} while (linkaddrhi != hi_new && --retries);
> +	} else {
> +		linkaddr = readl_relaxed(dch->base + CH_LINKADDR);
> +		linkaddrhi = 0;
> +	}
>   
>   	for (i = 0; i < desc->sglen; i++) {
>   		if (desc->sg[i].phys == (((u64)linkaddrhi << 32) | (linkaddr & ~CH_LINKADDR_EN)))
> @@ -876,6 +1237,14 @@ static void d350_free_chan_resources(struct dma_chan *chan)
>   	dch->cmd_pool = NULL;
>   }
>   
> +static void d250_cntx_mem_release(void *ptr)
> +{
> +	struct d350 *dmac = ptr;
> +	struct device *dev = dmac->dma.dev;
> +
> +	dma_free_coherent(dev, dmac->cntx_mem_size, dmac->cntx_mem, dmac->cntx_mem_paddr);
> +}
> +
>   static int d350_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -893,8 +1262,9 @@ static int d350_probe(struct platform_device *pdev)
>   	r = FIELD_GET(IIDR_VARIANT, reg);
>   	p = FIELD_GET(IIDR_REVISION, reg);
>   	if (FIELD_GET(IIDR_IMPLEMENTER, reg) != IMPLEMENTER_ARM ||
> -	    FIELD_GET(IIDR_PRODUCTID, reg) != PRODUCTID_DMA350)
> -		return dev_err_probe(dev, -ENODEV, "Not a DMA-350!");
> +	    ((FIELD_GET(IIDR_PRODUCTID, reg) != PRODUCTID_DMA350) &&
> +	    FIELD_GET(IIDR_PRODUCTID, reg) != PRODUCTID_DMA250))
> +		return dev_err_probe(dev, -ENODEV, "Not a DMA-350/DMA-250!");
>   
>   	reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG0);
>   	nchan = FIELD_GET(DMA_CFG_NUM_CHANNELS, reg) + 1;
> @@ -917,13 +1287,38 @@ static int d350_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>   
> +	if (device_is_compatible(dev, "arm,dma-250")) {
If only we had a completely reliable product ID from the hardware itself...

> +		u32 cfg2;
> +		int secext_present;
> +
> +		dmac->is_d250 = true;
> +
> +		cfg2 = readl_relaxed(base + DMAINFO + DMA_BUILDCFG2);
> +		secext_present = (cfg2 & DMA_CFG_HAS_TZ) ? 1 : 0;
> +		dmac->cntx_mem_size = nchan * 64 * (1 + secext_present);

As before I think that's wrong.

> +		dmac->cntx_mem = dma_alloc_coherent(dev, dmac->cntx_mem_size,
> +						    &dmac->cntx_mem_paddr,
> +						    GFP_KERNEL);

This is too early, it needs to wait until after we've set the DMA mask. 
Also since this is purely private memory for the device, it may as well 
use DMA_ATTR_NO_KERNEL_MAPPING.

> +		if (!dmac->cntx_mem)
> +			return dev_err_probe(dev, -ENOMEM, "Failed to alloc context memory\n");
Just return -ENOMEM - dev_err_probe() adds nothing.

> +		ret = devm_add_action_or_reset(dev, d250_cntx_mem_release, dmac);
> +		if (ret) {
> +			dma_free_coherent(dev, dmac->cntx_mem_size,
> +					  dmac->cntx_mem, dmac->cntx_mem_paddr);

a) Understand that the mildly non-obvious "or reset" means it already 
calls the cleanup action on error, so this would be a double-free.

b) Don't reinvent dmam_alloc_*() in the first place though.

> +			return ret;
> +		}
> +		writel_relaxed(dmac->cntx_mem_paddr, base + DMANSECCTRL + NSEC_CNTXBASE);

Perhaps we should check that this hasn't already been set up first? I 
mean, we can't necessarily even be sure teh context memory interface can 
access the same address space as the DMA transfer interface at all; the 
design intent is at least partly to allow connecting a dedicated SRAM 
directly, see figure 1 here: 
https://developer.arm.com/documentation/108001/0000/DMAC-interfaces/AHB5-manager-interfaces/Separate-AHB5-ports-for-data-and-virtual-channel-context?lang=en

However I'm not sure how feasible that is to detect from software - the 
base register alone clearly isn't foolproof since 0 could be a valid 
address (especially in a private SRAM). At worst I suppose we might end 
up needing a DMA-250-specific DT property to say whether it does or 
doesn't need context memory from the OS...

> +	}
> +
>   	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(aw));
>   	coherent = device_get_dma_attr(dev) == DEV_DMA_COHERENT;
>   
>   	reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG1);
>   	dmac->nreq = FIELD_GET(DMA_CFG_NUM_TRIGGER_IN, reg);
>   
> -	dev_dbg(dev, "DMA-350 r%dp%d with %d channels, %d requests\n", r, p, dmac->nchan, dmac->nreq);
> +	dev_info(dev, "%s r%dp%d with %d channels, %d requests\n",
> +		 dmac->is_d250 ? "DMA-250" : "DMA-350", r, p, dmac->nchan, dmac->nreq);

As Krzysztof said, this is a debug message and it's staying a debug 
message. And just replace "DMA-350" with "ProductID 0x%x" - it's only 
meant as a sanity-check that we're looking at the hardware we expect to 
be looking at.
>   	for (int i = min(dw, 16); i > 0; i /= 2) {
>   		dmac->dma.src_addr_widths |= BIT(i);
> @@ -935,7 +1330,10 @@ static int d350_probe(struct platform_device *pdev)
>   	dmac->dma.device_alloc_chan_resources = d350_alloc_chan_resources;
>   	dmac->dma.device_free_chan_resources = d350_free_chan_resources;
>   	dma_cap_set(DMA_MEMCPY, dmac->dma.cap_mask);
> -	dmac->dma.device_prep_dma_memcpy = d350_prep_memcpy;
> +	if (dmac->is_d250)
> +		dmac->dma.device_prep_dma_memcpy = d250_prep_memcpy;
> +	else
> +		dmac->dma.device_prep_dma_memcpy = d350_prep_memcpy;
>   	dmac->dma.device_pause = d350_pause;
>   	dmac->dma.device_resume = d350_resume;
>   	dmac->dma.device_terminate_all = d350_terminate_all;
> @@ -971,8 +1369,8 @@ static int d350_probe(struct platform_device *pdev)
>   			return dch->irq;
>   
>   		dch->has_wrap = FIELD_GET(CH_CFG_HAS_WRAP, reg);
> -		dch->has_trig = FIELD_GET(CH_CFG_HAS_TRIGIN, reg) &
> -				FIELD_GET(CH_CFG_HAS_TRIGSEL, reg);

Not only is this in the wrong patch, it's the wrong change to make 
anyway. If you're only adding support for fixed triggers, you need to 
explicitly *exclude* selectable triggers from that, because they work 
differently.

Thanks,
Robin.

> +		dch->has_xsizehi = FIELD_GET(CH_CFG_HAS_XSIZEHI, reg);
> +		dch->has_trig = FIELD_GET(CH_CFG_HAS_TRIGIN, reg);
>   
>   		/* Fill is a special case of Wrap */
>   		memset &= dch->has_wrap;
> @@ -994,8 +1392,13 @@ static int d350_probe(struct platform_device *pdev)
>   		dma_cap_set(DMA_SLAVE, dmac->dma.cap_mask);
>   		dma_cap_set(DMA_CYCLIC, dmac->dma.cap_mask);
>   		dmac->dma.device_config = d350_slave_config;
> -		dmac->dma.device_prep_slave_sg = d350_prep_slave_sg;
> -		dmac->dma.device_prep_dma_cyclic = d350_prep_cyclic;
> +		if (dmac->is_d250) {
> +			dmac->dma.device_prep_slave_sg = d250_prep_slave_sg;
> +			dmac->dma.device_prep_dma_cyclic = d250_prep_cyclic;
> +		} else {
> +			dmac->dma.device_prep_slave_sg = d350_prep_slave_sg;
> +			dmac->dma.device_prep_dma_cyclic = d350_prep_cyclic;
> +		}
>   	}
>   
>   	if (memset) {
> @@ -1019,6 +1422,7 @@ static void d350_remove(struct platform_device *pdev)
>   
>   static const struct of_device_id d350_of_match[] __maybe_unused = {
>   	{ .compatible = "arm,dma-350" },
> +	{ .compatible = "arm,dma-250" },
>   	{}
>   };
>   MODULE_DEVICE_TABLE(of, d350_of_match);
> @@ -1035,5 +1439,5 @@ module_platform_driver(d350_driver);
>   
>   MODULE_AUTHOR("Robin Murphy <robin.murphy@arm.com>");
>   MODULE_AUTHOR("Jisheng Zhang <jszhang@kernel.org>");
> -MODULE_DESCRIPTION("Arm DMA-350 driver");
> +MODULE_DESCRIPTION("Arm DMA-350/DMA-250 driver");
>   MODULE_LICENSE("GPL v2");