[PATCH v3 0/6] dmaengine: arm-dma350: Add slave support and CIX Sky1 integration

[PATCH v3 0/6] dmaengine: arm-dma350: Add slave support and CIX Sky1 integration

[Jelly Jia]

Hello,

This series adds slave transfer support to the Arm DMA-350 driver and
the CIX Sky1 SoC integration around it.

Patch 1 fixes the CH_CTRL_USESRCTRIGIN definition: the source trigger
enable bit is bit 25, not bit 26 (which is the destination trigger
enable bit). Unchanged since v1.

Patch 2 adds slave transfer support to the Arm DMA-350 driver:
scatter-gather and cyclic preparation, command-list allocation,
per-channel resource mapping and residue reporting. Slave transfers are
needed to serve peripheral requests, for example the audio FIFOs of the
CIX Sky1 audio subsystem.

Patch 3 syncs the slave command list before starting a transfer. The
command list is mapped with dma_map_single(), so without the sync a
non-coherent DMA-350 may read stale command words and the transfer
stalls after the first command.

Patch 4 documents the CIX Sky1 DMA-350 integration in the arm,dma-350
binding: the Sky1 controller keeps the generic compatible and adds a
"cix,sky1-dma350" device compatible, with a required clock, an optional
reset and an optional reserved memory region described on the controller
node.

Patch 5 adds the matching driver support, which manages those resources;
slave resources are mapped with dma_map_resource() and translated by the
DMA API through the parent bus dma-ranges.

Patch 6 adds the two Sky1 instances (FCH and AUDSS) to the SoC
devicetree, each under a simple-bus node describing the bus integration.

Changes since v2:
- Patches 4 and 5 no longer configure the Sky1 channel interrupt gate.
  Thanks to Conor Dooley for pointing out that the gate is upstream of
  the device and must not be routed through the driver: the driver has
  no CIX match data, no syscon register access and no system resume
  hook for it, the binding does not describe a cix,irq-router property,
  and the driver no longer selects MFD_SYSCON.
- Patch 4 now requires a clock for the CIX compatible instead of
  constraining a property of the generic compatible.
- Patch 5 releases the reserved memory region on both registration
  failure paths.
- Patch 2 fixes two residue-reporting races: the scatter-gather residue
  re-reads the channel link pointer around the CH_XSIZE read so that a
  channel stepping to the next command cannot make the residue jump,
  and cyclic transfers report the progress inside the running period
  instead of only at period boundaries. The CH_TRIGINCFG block size
  description was corrected as well.
- Patch 3 syncs the command list with its DMA API address instead of a
  second, identical address kept in the descriptor.
- Patch 6 drops the interrupt gate property together with patch 4, and
  the two bus nodes carry no unit-address in their names: they have no
  reg, which made dtc warn.

The series is based on v7.3-rc1.

Testing:
- Built for arm64 with GCC 12.3, no new warnings; sparse clean;
  checkpatch --strict clean.
- dt_binding_check and dtbs_check pass for the new binding and nodes.
- Runtime-tested on a CIX Sky1 (Radxa Orion O6) board (v3 revision):
  both the FCH and the AUDSS DMA-350 instances probe and transfer data;
  dmatest passes on 12 channels x 1000 iterations, UART DMA transfers
  (1 MiB and 5000 B) are byte-exact, and audio playback through the
  AUDSS DMA-350 runs at nominal rate without underruns or xruns.

Link: https://lore.kernel.org/r/20261009051846.1115962-1-Jelly.Jia@cixtech.com

Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>

Jelly Jia (6):
  dmaengine: arm-dma350: Fix source trigger bit
  dmaengine: arm-dma350: Add slave and cyclic transfer support
  dmaengine: arm-dma350: Sync the slave command list before starting
  dt-bindings: dma: arm,dma-350: Document the CIX Sky1 integration
  dmaengine: arm-dma350: Support integration clocks, resets and reserved
    memory
  arm64: dts: cix: Describe the Sky1 DMA-350 bus integration

 .../devicetree/bindings/dma/arm,dma-350.yaml  |  26 +-
 arch/arm64/boot/dts/cix/sky1.dtsi             |  68 +-
 drivers/dma/arm-dma350.c                      | 710 +++++++++++++++++-
 3 files changed, 765 insertions(+), 39 deletions(-)

-- 
2.54.0

[PATCH v3 1/6] dmaengine: arm-dma350: Fix source trigger bit

[Jelly Jia]

CH_CTRL_USESRCTRIGIN is the source trigger enable bit in CH_CTRL and
must use bit 25, not bit 26 which is the destination trigger enable bit
(CH_CTRL_USEDESTRIGIN).

The constant has no users yet, but enabling the source trigger input
would silently enable the destination trigger input instead, breaking
any transfer that waits for a peripheral request. Fix it before the
first user arrives.

Fixes: 5d099706449d ("dmaengine: Add Arm DMA-350 driver")
Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
---
 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 09403aca8bb0..4e17130de6c8 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -63,7 +63,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.54.0

Re: [PATCH v3 1/6] dmaengine: arm-dma350: Fix source trigger bit

[Frank Li]

On Mon, Oct 12, 2026 at 04:41:50PM +0800, Jelly Jia wrote:
> CH_CTRL_USESRCTRIGIN is the source trigger enable bit in CH_CTRL and
> must use bit 25, not bit 26 which is the destination trigger enable bit
> (CH_CTRL_USEDESTRIGIN).
>
> The constant has no users yet, but enabling the source trigger input
> would silently enable the destination trigger input instead, breaking
> any transfer that waits for a peripheral request. Fix it before the
> first user arrives.
>
> Fixes: 5d099706449d ("dmaengine: Add Arm DMA-350 driver")
> Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
> Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  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 09403aca8bb0..4e17130de6c8 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c
> @@ -63,7 +63,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.54.0
>

[PATCH v3 2/6] dmaengine: arm-dma350: Add slave and cyclic transfer support

[Jelly Jia]

Add DMA slave support to the Arm DMA-350 driver, which so far only
supports memory-to-memory transfers.

Slave transfers are needed to serve peripheral requests: on the CIX
Sky1 SoC, for example, the audio subsystem uses DMA-350 channels to
move PCM data between memory and peripheral FIFOs, which requires both
scatter-gather and cyclic transfers.

Each scatterlist entry becomes one command in a linked list of DMA-350
commands, so a whole slave_sg transfer runs without software
intervention; a cyclic transfer chains the last command back to the
first one, which the controller then executes indefinitely. The trigger
input comes from the DMA specifier and is programmed into CH_TRIGINCFG:
the block size field carries the configured maxburst for memory-to-device
transfers and zero for device-to-memory transfers.

Peripheral addresses are translated through the DMA API and cached per
channel, so repeated transfers to the same FIFO do not remap. The
command list is allocated with kmalloc() and mapped with
dma_map_single() rather than dma_alloc_coherent(), because it is freed
from the virt-dma tasklet which runs with interrupts disabled, and
dma_free_coherent() may only be called with interrupts enabled.

Completion handling is reworked for the new descriptor types: the
handler claims an interrupt only when a channel interrupt is actually
pending, tolerates a spurious interrupt with no active descriptor, and
reports cyclic progress through vchan_cyclic_callback().

Residue reporting walks the command list: the channel link pointer is
read (retrying when the channel steps it between the two halves of the
address) to find the command being executed, and the length of every
command that has not started yet is added to the residue of the current
one. The link pointer is re-read around the CH_XSIZE read, and a channel
which stepped to the next command in between falls back to the residue
of the last completion, so the bytes left in the command which just
finished are not counted twice. Cyclic transfers report the progress
inside the running period the same way: the progress made in the running
period is subtracted from the residue of the last period boundary when
the channel link pointer still names the command of the running period,
and the period-boundary residue is kept otherwise.

v2 -> v3:
- The trigger block size wording was corrected: the field carries the
  maxburst for memory-to-device transfers and zero for device-to-memory
  transfers.
- Residue reporting re-reads the channel link pointer around the CH_XSIZE
  read so that a channel stepping to the next command cannot make the
  residue jump, and cyclic transfers report the progress inside the
  running period.

Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
---
 drivers/dma/arm-dma350.c | 654 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 631 insertions(+), 23 deletions(-)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 4e17130de6c8..7a198d35bef6 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -3,12 +3,17 @@
 // Arm DMA-350 driver
 
 #include <linux/bitfield.h>
+#include <linux/bitops.h>
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
 #include <linux/of.h>
+#include <linux/of_dma.h>
 #include <linux/module.h>
+#include <linux/overflow.h>
 #include <linux/platform_device.h>
+#include <linux/scatterlist.h>
+#include <linux/slab.h>
 
 #include "dmaengine.h"
 #include "virt-dma.h"
@@ -102,6 +107,10 @@
 #define CH_FILLVAL		0x38
 #define CH_SRCTRIGINCFG		0x4c
 #define CH_DESTRIGINCFG		0x50
+#define CH_TRIGINCFG_BLKSIZE	GENMASK(23, 16)
+#define CH_TRIGINCFG_MODE	GENMASK(11, 10)
+#define CH_TRIGINCFG_TYPE	GENMASK(9, 8)
+#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)
@@ -147,6 +156,7 @@
 #define LINK_LINKADDR		BIT(30)
 #define LINK_LINKADDRHI		BIT(31)
 
+#define D350_SLAVE_CMD_WORDS	14
 
 enum ch_ctrl_donetype {
 	CH_CTRL_DONETYPE_NONE = 0,
@@ -161,6 +171,18 @@ enum ch_ctrl_xtype {
 	CH_CTRL_XTYPE_FILL = 3
 };
 
+enum ch_trigincfg_mode {
+	CH_TRIGINCFG_MODE_COMMAND = 0,
+	CH_TRIGINCFG_MODE_DMA_FC = 2,
+	CH_TRIGINCFG_MODE_PERIPH_FC = 3
+};
+
+enum ch_trigincfg_type {
+	CH_TRIGINCFG_TYPE_SW = 0,
+	CH_TRIGINCFG_TYPE_HW = 2,
+	CH_TRIGINCFG_TYPE_INTERNAL = 3
+};
+
 enum ch_cfg_shareattr {
 	SHAREATTR_NSH = 0,
 	SHAREATTR_OSH = 2,
@@ -178,7 +200,25 @@ struct d350_desc {
 	u32 command[16];
 	u16 xsize;
 	u16 xsizehi;
+	u32 *cmds;
+	dma_addr_t cmds_dma;	/* DMA API address of the command list */
+	size_t cmds_size;
+	u32 *cmd_len;
+	size_t ncmds;
+	size_t bytes;
+	size_t period_len;
+	size_t periods;
+	size_t period;
 	u8 tsz;
+	bool cyclic;
+};
+
+struct d350_chan_map {
+	phys_addr_t cpu_addr;
+	dma_addr_t dma_addr;
+	size_t size;
+	enum dma_data_direction dir;
+	bool needs_unmap;
 };
 
 struct d350_chan {
@@ -189,10 +229,13 @@ struct d350_chan {
 	enum dma_status status;
 	dma_cookie_t cookie;
 	u32 residue;
+	u32 req;
 	u8 tsz;
 	bool has_trig;
 	bool has_wrap;
 	bool coherent;
+	struct d350_chan_map map;
+	struct dma_slave_config sconfig;
 };
 
 struct d350 {
@@ -212,9 +255,418 @@ static inline struct d350_desc *to_d350_desc(struct virt_dma_desc *vd)
 	return container_of(vd, struct d350_desc, vd);
 }
 
+static void d350_free_cmds(struct device *dev, struct d350_desc *desc)
+{
+	if (desc->cmds) {
+		dma_unmap_single(dev, desc->cmds_dma, desc->cmds_size,
+				 DMA_TO_DEVICE);
+		kfree(desc->cmds);
+	}
+	kfree(desc->cmd_len);
+}
+
 static void d350_desc_free(struct virt_dma_desc *vd)
 {
-	kfree(to_d350_desc(vd));
+	struct d350_desc *desc = to_d350_desc(vd);
+
+	d350_free_cmds(vd->tx.chan->device->dev, desc);
+	kfree(desc);
+}
+
+static int d350_alloc_cmds(struct dma_chan *dchan, struct d350_desc *desc,
+			   size_t ncmds)
+{
+	struct device *dev = dchan->device->dev;
+	size_t cmd_size = D350_SLAVE_CMD_WORDS * sizeof(u32);
+
+	if (check_mul_overflow(ncmds, cmd_size, &desc->cmds_size))
+		return -ENOMEM;
+
+	/*
+	 * The command list is freed from the virt-dma tasklet, which runs with
+	 * interrupts disabled, so it cannot come from dma_alloc_coherent():
+	 * dma_free_coherent() may only be called with interrupts enabled.
+	 */
+	desc->cmds = kmalloc(desc->cmds_size, GFP_NOWAIT);
+	if (!desc->cmds)
+		return -ENOMEM;
+
+	desc->cmds_dma = dma_map_single(dev, desc->cmds, desc->cmds_size,
+					DMA_TO_DEVICE);
+	if (dma_mapping_error(dev, desc->cmds_dma)) {
+		kfree(desc->cmds);
+		desc->cmds = NULL;
+		return -ENOMEM;
+	}
+
+	desc->ncmds = ncmds;
+
+	return 0;
+}
+
+static void d350_unmap_resource(struct d350_chan *dch)
+{
+	struct device *dev = dch->vc.chan.device->dev;
+	struct d350_chan_map *map = &dch->map;
+
+	if (map->dir == DMA_NONE)
+		return;
+
+	if (map->needs_unmap)
+		dma_unmap_resource(dev, map->dma_addr, map->size, map->dir, 0);
+
+	map->dir = DMA_NONE;
+	map->needs_unmap = false;
+}
+
+static dma_addr_t d350_map_resource(struct d350_chan *dch,
+				    phys_addr_t cpu_addr, size_t size,
+				    enum dma_data_direction dir)
+{
+	struct device *dev = dch->vc.chan.device->dev;
+	struct d350_chan_map *map = &dch->map;
+	dma_addr_t dma_addr;
+
+	if (map->dir == dir && map->cpu_addr == cpu_addr &&
+	    map->size == size)
+		return map->dma_addr;
+
+	d350_unmap_resource(dch);
+
+	dma_addr = dma_map_resource(dev, cpu_addr, size, dir, 0);
+	if (dma_mapping_error(dev, dma_addr)) {
+		dev_err(dev, "map slave failed ch%u phys=%pa size=%zu dir=%d\n",
+			dch->vc.chan.chan_id, &cpu_addr, size, dir);
+		return DMA_MAPPING_ERROR;
+	}
+	map->needs_unmap = true;
+
+	map->cpu_addr = cpu_addr;
+	map->dma_addr = dma_addr;
+	map->size = size;
+	map->dir = dir;
+
+	return map->dma_addr;
+}
+
+static bool d350_buswidth_supported(u32 widths, enum dma_slave_buswidth width)
+{
+	return width < BITS_PER_TYPE(widths) && (widths & BIT(width));
+}
+
+static int d350_check_slave_config(struct dma_device *dma,
+				   struct dma_slave_config *config)
+{
+	u32 maxburst = FIELD_MAX(CH_CFG_MAXBURSTLEN) + 1;
+	u32 widths = dma->src_addr_widths | dma->dst_addr_widths;
+
+	if (!d350_buswidth_supported(widths, config->src_addr_width) ||
+	    !d350_buswidth_supported(widths, config->dst_addr_width))
+		return -EINVAL;
+
+	if (config->src_maxburst > maxburst ||
+	    config->dst_maxburst > maxburst)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int d350_config(struct dma_chan *chan, struct dma_slave_config *config)
+{
+	struct d350_chan *dch = to_d350_chan(chan);
+	struct dma_device *dma = chan->device;
+	unsigned long flags;
+	int ret;
+
+	ret = d350_check_slave_config(dma, config);
+	if (ret) {
+		dev_err(dma->dev, "invalid slave configuration\n");
+		return ret;
+	}
+
+	spin_lock_irqsave(&dch->vc.lock, flags);
+	if (dch->desc || !list_empty(&dch->vc.desc_allocated) ||
+	    !list_empty(&dch->vc.desc_submitted) ||
+	    !list_empty(&dch->vc.desc_issued)) {
+		spin_unlock_irqrestore(&dch->vc.lock, flags);
+		return -EBUSY;
+	}
+	spin_unlock_irqrestore(&dch->vc.lock, flags);
+
+	d350_unmap_resource(dch);
+	memcpy(&dch->sconfig, config, sizeof(dch->sconfig));
+
+	return 0;
+}
+
+static struct dma_chan *d350_of_xlate(struct of_phandle_args *dma_spec,
+				      struct of_dma *ofdma)
+{
+	struct d350 *dmac = ofdma->of_dma_data;
+	struct d350_chan *dch;
+	struct dma_chan *chan;
+	u32 req;
+	int i;
+
+	if (dma_spec->args_count != 1) {
+		dev_err(dmac->dma.dev, "dma phandle must have one argument\n");
+		return NULL;
+	}
+
+	req = dma_spec->args[0];
+	if (req >= dmac->nreq) {
+		dev_err(dmac->dma.dev, "invalid DMA request %u, have %d\n",
+			req, dmac->nreq);
+		return NULL;
+	}
+
+	for (i = 0; i < dmac->nchan; i++) {
+		dch = &dmac->channels[i];
+		if (!dch->has_trig)
+			continue;
+
+		chan = dma_get_slave_channel(&dch->vc.chan);
+		if (chan) {
+			dch->req = req;
+			return chan;
+		}
+	}
+
+	dev_err(dmac->dma.dev, "no free channel with trigger support\n");
+
+	return NULL;
+}
+
+static u32 d350_device_transcfg(u32 maxburst)
+{
+	return FIELD_PREP(CH_CFG_MAXBURSTLEN, maxburst - 1) |
+	       FIELD_PREP(CH_CFG_SHAREATTR, SHAREATTR_OSH) |
+	       FIELD_PREP(CH_CFG_MEMATTR, MEMATTR_DEVICE);
+}
+
+static int d350_slave_params(struct d350_chan *dch,
+			     enum dma_transfer_direction direction,
+			     phys_addr_t *dev_cpu_addr,
+			     enum dma_data_direction *dev_dir,
+			     enum dma_slave_buswidth *width, u32 *maxburst)
+{
+	struct dma_slave_config *config = &dch->sconfig;
+
+	if (direction == DMA_MEM_TO_DEV) {
+		*dev_cpu_addr = config->dst_addr;
+		*dev_dir = DMA_FROM_DEVICE;
+		*width = config->dst_addr_width;
+		*maxburst = config->dst_maxburst;
+	} else if (direction == DMA_DEV_TO_MEM) {
+		*dev_cpu_addr = config->src_addr;
+		*dev_dir = DMA_TO_DEVICE;
+		*width = config->src_addr_width;
+		*maxburst = config->src_maxburst;
+	} else {
+		return -EINVAL;
+	}
+
+	return *width && *maxburst ? 0 : -EINVAL;
+}
+
+static void d350_fill_slave_cmd(struct d350_chan *dch, struct d350_desc *desc,
+				u32 *cmd, dma_addr_t mem, dma_addr_t dev_dma_addr,
+				size_t len, bool link, dma_addr_t link_addr,
+				enum dma_transfer_direction direction,
+				enum dma_slave_buswidth width, u32 maxburst,
+				enum ch_ctrl_donetype donetype)
+{
+	bool mem_to_dev = direction == DMA_MEM_TO_DEV;
+	u16 xsize, xsizehi;
+	u32 devcfg;
+	u32 memcfg;
+	u32 trigcfg;
+
+	desc->tsz = __ffs(width);
+	xsize = lower_16_bits(len >> desc->tsz);
+	xsizehi = upper_16_bits(len >> desc->tsz);
+	devcfg = d350_device_transcfg(maxburst);
+	memcfg = dch->coherent ? TRANSCFG_WB : TRANSCFG_NC;
+
+	trigcfg = FIELD_PREP(CH_TRIGINCFG_BLKSIZE,
+			     mem_to_dev ? maxburst - 1 : 0) |
+		  FIELD_PREP(CH_TRIGINCFG_MODE, CH_TRIGINCFG_MODE_PERIPH_FC) |
+		  FIELD_PREP(CH_TRIGINCFG_TYPE, CH_TRIGINCFG_TYPE_HW) |
+		  FIELD_PREP(CH_TRIGINCFG_SEL, dch->req);
+
+	cmd[0] = LINK_CTRL | LINK_SRCADDR | LINK_SRCADDRHI | LINK_DESADDR |
+		 LINK_DESADDRHI | LINK_XSIZE | LINK_XSIZEHI | LINK_SRCTRANSCFG |
+		 LINK_DESTRANSCFG | LINK_XADDRINC | LINK_LINKADDR |
+		 LINK_LINKADDRHI |
+		 (mem_to_dev ? LINK_DESTRIGINCFG : LINK_SRCTRIGINCFG);
+	cmd[1] = (mem_to_dev ? CH_CTRL_USEDESTRIGIN : CH_CTRL_USESRCTRIGIN) |
+		 FIELD_PREP(CH_CTRL_TRANSIZE, desc->tsz) |
+		 FIELD_PREP(CH_CTRL_XTYPE, CH_CTRL_XTYPE_CONTINUE) |
+		 FIELD_PREP(CH_CTRL_DONETYPE, donetype);
+	cmd[2] = lower_32_bits(mem_to_dev ? mem : dev_dma_addr);
+	cmd[3] = upper_32_bits(mem_to_dev ? mem : dev_dma_addr);
+	cmd[4] = lower_32_bits(mem_to_dev ? dev_dma_addr : mem);
+	cmd[5] = upper_32_bits(mem_to_dev ? dev_dma_addr : mem);
+	cmd[6] = FIELD_PREP(CH_XY_SRC, xsize) |
+		 FIELD_PREP(CH_XY_DES, xsize);
+	cmd[7] = FIELD_PREP(CH_XY_SRC, xsizehi) |
+		 FIELD_PREP(CH_XY_DES, xsizehi);
+	cmd[8] = mem_to_dev ? memcfg : devcfg;
+	cmd[9] = mem_to_dev ? devcfg : memcfg;
+	cmd[10] = mem_to_dev ? FIELD_PREP(CH_XY_SRC, 1) :
+				FIELD_PREP(CH_XY_DES, 1);
+	cmd[11] = trigcfg;
+	cmd[12] = lower_32_bits(link_addr) | (link ? CH_LINKADDR_EN : 0);
+	cmd[13] = upper_32_bits(link_addr);
+}
+
+static struct dma_async_tx_descriptor *
+d350_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
+		   unsigned int sg_len,
+		   enum dma_transfer_direction direction,
+		   unsigned long flags, void *context)
+{
+	struct d350_chan *dch = to_d350_chan(dchan);
+	size_t cmd_size = D350_SLAVE_CMD_WORDS * sizeof(u32);
+	enum dma_data_direction dev_dir;
+	enum dma_slave_buswidth width;
+	struct d350_desc *desc;
+	phys_addr_t dev_cpu_addr;
+	dma_addr_t dev_dma_addr, mem;
+	struct scatterlist *sg;
+	u32 maxburst;
+	size_t len;
+	int i;
+
+	if (unlikely(!is_slave_direction(direction) || !sg_len))
+		return NULL;
+
+	if (d350_slave_params(dch, direction, &dev_cpu_addr, &dev_dir, &width,
+			      &maxburst))
+		return NULL;
+
+	dev_dma_addr = d350_map_resource(dch, dev_cpu_addr, width, dev_dir);
+	if (dma_mapping_error(dchan->device->dev, dev_dma_addr))
+		return NULL;
+
+	desc = kzalloc_obj(*desc, GFP_NOWAIT);
+	if (!desc)
+		return NULL;
+
+	if (sg_len > 1) {
+		if (d350_alloc_cmds(dchan, desc, sg_len))
+			goto err_free_desc;
+
+		desc->cmd_len = kcalloc(sg_len, sizeof(*desc->cmd_len),
+					GFP_NOWAIT);
+		if (!desc->cmd_len)
+			goto err_free_cmds;
+	}
+
+	for_each_sg(sgl, sg, sg_len, i) {
+		enum ch_ctrl_donetype donetype = CH_CTRL_DONETYPE_CMD;
+		dma_addr_t link_addr = 0;
+		bool link = false;
+		u32 *cmd = desc->command;
+
+		mem = sg_dma_address(sg);
+		len = sg_dma_len(sg);
+		if (!len || (len >> __ffs(width)) > U32_MAX ||
+		    !IS_ALIGNED(len | mem | dev_dma_addr, width))
+			goto err_free_cmds;
+
+		if (sg_len > 1) {
+			cmd = desc->cmds + i * D350_SLAVE_CMD_WORDS;
+			if (i < sg_len - 1) {
+				link = true;
+				link_addr = desc->cmds_dma + (i + 1) * cmd_size;
+				donetype = CH_CTRL_DONETYPE_NONE;
+			}
+			desc->cmd_len[i] = len;
+		}
+
+		if (check_add_overflow(desc->bytes, len, &desc->bytes) ||
+		    desc->bytes > U32_MAX)
+			goto err_free_cmds;
+
+		d350_fill_slave_cmd(dch, desc, cmd, mem, dev_dma_addr, len,
+				    link, link_addr, direction, width, maxburst,
+				    donetype);
+	}
+
+	if (sg_len > 1)
+		memcpy(desc->command, desc->cmds, cmd_size);
+
+	return vchan_tx_prep(&dch->vc, &desc->vd, flags);
+
+err_free_cmds:
+	d350_free_cmds(dchan->device->dev, desc);
+err_free_desc:
+	kfree(desc);
+
+	return NULL;
+}
+
+static struct dma_async_tx_descriptor *
+d350_prep_dma_cyclic(struct dma_chan *dchan, dma_addr_t buf_addr,
+		     size_t buf_len, size_t period_len,
+		     enum dma_transfer_direction direction, unsigned long flags)
+{
+	struct d350_chan *dch = to_d350_chan(dchan);
+	struct d350_desc *desc;
+	phys_addr_t dev_cpu_addr;
+	dma_addr_t dev_dma_addr;
+	enum dma_data_direction dev_dir;
+	enum dma_slave_buswidth width;
+	size_t period, cmd_size;
+	u32 maxburst;
+	int ret;
+
+	if (!buf_len || !period_len || buf_len % period_len ||
+	    buf_len > U32_MAX || !is_slave_direction(direction))
+		return NULL;
+
+	ret = d350_slave_params(dch, direction, &dev_cpu_addr, &dev_dir, &width,
+				&maxburst);
+	if (ret)
+		return NULL;
+
+	dev_dma_addr = d350_map_resource(dch, dev_cpu_addr, width, dev_dir);
+	if (dma_mapping_error(dchan->device->dev, dev_dma_addr))
+		return NULL;
+
+	if (!IS_ALIGNED(buf_addr | dev_dma_addr | period_len, width))
+		return NULL;
+
+	desc = kzalloc_obj(*desc, GFP_NOWAIT);
+	if (!desc)
+		return NULL;
+
+	desc->bytes = buf_len;
+	desc->period_len = period_len;
+	desc->periods = buf_len / period_len;
+	desc->cyclic = true;
+
+	if (d350_alloc_cmds(dchan, desc, desc->periods)) {
+		kfree(desc);
+		return NULL;
+	}
+
+	cmd_size = D350_SLAVE_CMD_WORDS * sizeof(u32);
+
+	for (period = 0; period < desc->periods; period++) {
+		u32 *cmd = desc->cmds + period * D350_SLAVE_CMD_WORDS;
+		dma_addr_t mem = buf_addr + period * period_len;
+		dma_addr_t next = desc->cmds_dma +
+				  ((period + 1) % desc->periods) * cmd_size;
+
+		d350_fill_slave_cmd(dch, desc, cmd, mem, dev_dma_addr,
+				    period_len, true, next, direction, width,
+				    maxburst, CH_CTRL_DONETYPE_CMD);
+	}
+	memcpy(desc->command, desc->cmds, cmd_size);
+
+	return vchan_tx_prep(&dch->vc, &desc->vd, flags);
 }
 
 static struct dma_async_tx_descriptor *d350_prep_memcpy(struct dma_chan *chan,
@@ -228,6 +680,7 @@ static struct dma_async_tx_descriptor *d350_prep_memcpy(struct dma_chan *chan,
 	if (!desc)
 		return NULL;
 
+	desc->bytes = len;
 	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);
@@ -266,6 +719,7 @@ static struct dma_async_tx_descriptor *d350_prep_memset(struct dma_chan *chan,
 	if (!desc)
 		return NULL;
 
+	desc->bytes = len;
 	desc->tsz = __ffs(len | dest | (1 << dch->tsz));
 	desc->xsize = lower_16_bits(len >> desc->tsz);
 	desc->xsizehi = upper_16_bits(len >> desc->tsz);
@@ -339,6 +793,66 @@ static u32 d350_get_residue(struct d350_chan *dch)
 	return res << dch->desc->tsz;
 }
 
+static u32 d350_get_sg_residue(struct d350_chan *dch)
+{
+	struct d350_desc *desc = dch->desc;
+	size_t cmd_size = D350_SLAVE_CMD_WORDS * sizeof(u32);
+	size_t cmd = 0, i;
+	u32 linkaddr, linkaddrhi, hi_new, lo_new;
+	u32 residue;
+	u64 next_cmd;
+	int retries = 3; /* 1st time unlucky, 2nd improbable, 3rd just broken */
+
+	if (!desc->cmd_len)
+		return d350_get_residue(dch);
+
+	/*
+	 * CH_LINKADDR points at the next command. Match it against the command
+	 * array to find the command currently executing, then add every later
+	 * command which has not started yet.
+	 */
+	/* The channel may step the link pointer between the two reads */
+	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);
+
+	next_cmd = (linkaddr & ~CH_LINKADDR_EN) | ((u64)linkaddrhi << 32);
+
+	if (!next_cmd) {
+		cmd = desc->ncmds - 1;
+	} else {
+		for (i = 1; i < desc->ncmds; i++) {
+			if (next_cmd == desc->cmds_dma + i * cmd_size) {
+				cmd = i - 1;
+				break;
+			}
+		}
+		if (i == desc->ncmds)
+			return dch->residue;
+	}
+
+	residue = d350_get_residue(dch);
+
+	/*
+	 * If the channel stepped to the next command while CH_XSIZE was read,
+	 * the size belongs to the command which just finished and the sum below
+	 * would count the bytes left in it twice. Keep the residue from the last
+	 * completion in that case.
+	 */
+	hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
+	lo_new = readl_relaxed(dch->base + CH_LINKADDR);
+	if (hi_new != linkaddrhi || lo_new != linkaddr)
+		return dch->residue;
+
+	for (i = cmd + 1; i < desc->ncmds; i++)
+		residue += desc->cmd_len[i];
+
+	return residue;
+}
+
 static int d350_terminate_all(struct dma_chan *chan)
 {
 	struct d350_chan *dch = to_d350_chan(chan);
@@ -369,7 +883,60 @@ 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;
+	return desc->bytes;
+}
+
+static u32 d350_get_cyclic_residue(struct d350_chan *dch)
+{
+	struct d350_desc *desc = dch->desc;
+	size_t cmd_size = D350_SLAVE_CMD_WORDS * sizeof(u32);
+	u32 period_residue = desc->bytes - desc->period * desc->period_len;
+	u32 linkaddr, linkaddrhi, hi_new, lo_new;
+	u64 next_cmd;
+	int retries = 3; /* 1st time unlucky, 2nd improbable, 3rd just broken */
+
+	/*
+	 * desc->period counts the completed periods, so period_residue is the
+	 * residue at the last period boundary. Subtract the progress made in
+	 * the running period, read from CH_XSIZE: CH_LINKADDR points at the
+	 * command after the one executing, so while the channel is still on the
+	 * command of the running period it names the first command of the next
+	 * period.
+	 */
+	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);
+
+	next_cmd = (linkaddr & ~CH_LINKADDR_EN) | ((u64)linkaddrhi << 32);
+	if (next_cmd != desc->cmds_dma +
+			((desc->period + 1) % desc->periods) * cmd_size)
+		return period_residue;
+
+	period_residue -= desc->period_len - d350_get_residue(dch);
+
+	/*
+	 * If the channel stepped to the next period while CH_XSIZE was read,
+	 * the size belongs to a command of the next period and mixing it with
+	 * the current one would overstate the residue: keep the value from the
+	 * period boundary instead.
+	 */
+	hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
+	lo_new = readl_relaxed(dch->base + CH_LINKADDR);
+	if (hi_new != linkaddrhi || lo_new != linkaddr)
+		return d350_desc_bytes(desc) - desc->period * desc->period_len;
+
+	return period_residue;
+}
+
+static u32 d350_get_active_residue(struct d350_chan *dch)
+{
+	if (dch->desc->cyclic)
+		return d350_get_cyclic_residue(dch);
+
+	return d350_get_sg_residue(dch);
 }
 
 static enum dma_status d350_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
@@ -387,7 +954,7 @@ static enum dma_status d350_tx_status(struct dma_chan *chan, dma_cookie_t cookie
 	if (cookie == dch->cookie) {
 		status = dch->status;
 		if (status == DMA_IN_PROGRESS || status == DMA_PAUSED)
-			dch->residue = d350_get_residue(dch);
+			dch->residue = d350_get_active_residue(dch);
 		residue = dch->residue;
 	} else if ((vd = vchan_find_desc(&dch->vc, cookie))) {
 		residue = d350_desc_bytes(to_d350_desc(vd));
@@ -469,17 +1036,32 @@ static void d350_issue_pending(struct dma_chan *chan)
 static irqreturn_t d350_irq(int irq, void *data)
 {
 	struct d350_chan *dch = data;
-	struct device *dev = dch->vc.chan.device->dev;
-	struct virt_dma_desc *vd = &dch->desc->vd;
+	struct virt_dma_desc *vd;
+	struct d350_desc *desc;
+	u32 residue = 0;
 	u32 ch_status;
+	u32 irq_status;
+	u32 errinfo = 0;
 
 	ch_status = readl(dch->base + CH_STATUS);
-	if (!ch_status)
+	irq_status = ch_status & (CH_STAT_INTR_DONE | CH_STAT_INTR_ERR);
+	if (!irq_status)
 		return IRQ_NONE;
 
-	if (ch_status & CH_STAT_INTR_ERR) {
-		u32 errinfo = readl_relaxed(dch->base + CH_ERRINFO);
+	if (irq_status & CH_STAT_INTR_ERR)
+		errinfo = readl_relaxed(dch->base + CH_ERRINFO);
 
+	writel_relaxed(ch_status, dch->base + CH_STATUS);
+
+	spin_lock(&dch->vc.lock);
+	desc = dch->desc;
+	if (!desc) {
+		spin_unlock(&dch->vc.lock);
+		return IRQ_HANDLED;
+	}
+
+	vd = &desc->vd;
+	if (irq_status & CH_STAT_INTR_ERR) {
 		if (errinfo & (CH_ERRINFO_AXIRDPOISERR | CH_ERRINFO_AXIRDRESPERR))
 			vd->tx_result.result = DMA_TRANS_READ_FAILED;
 		else if (errinfo & CH_ERRINFO_AXIWRRESPERR)
@@ -487,21 +1069,27 @@ static irqreturn_t d350_irq(int irq, void *data)
 		else
 			vd->tx_result.result = DMA_TRANS_ABORTED;
 
-		vd->tx_result.residue = d350_get_residue(dch);
-	} else if (!(ch_status & CH_STAT_INTR_DONE)) {
-		dev_warn(dev, "Unexpected IRQ source? 0x%08x\n", ch_status);
-	}
-	writel_relaxed(ch_status, dch->base + CH_STATUS);
-
-	spin_lock(&dch->vc.lock);
-	vchan_cookie_complete(vd);
-	if (ch_status & CH_STAT_INTR_DONE) {
-		dch->status = DMA_COMPLETE;
-		dch->residue = 0;
-		d350_start_next(dch);
-	} else {
+		residue = d350_get_active_residue(dch);
+		vd->tx_result.residue = residue;
 		dch->status = DMA_ERROR;
-		dch->residue = vd->tx_result.residue;
+		dch->residue = residue;
+		dch->desc = NULL;
+		if (desc->cyclic)
+			vchan_terminate_vdesc(vd);
+		else
+			vchan_cookie_complete(vd);
+	} else {
+		if (desc->cyclic) {
+			desc->period = (desc->period + 1) % desc->periods;
+			dch->residue = d350_get_cyclic_residue(dch);
+			vchan_cyclic_callback(vd);
+		} else {
+			dch->status = DMA_COMPLETE;
+			dch->residue = 0;
+			dch->desc = NULL;
+			vchan_cookie_complete(vd);
+			d350_start_next(dch);
+		}
 	}
 	spin_unlock(&dch->vc.lock);
 
@@ -525,6 +1113,7 @@ static void d350_free_chan_resources(struct dma_chan *chan)
 
 	writel_relaxed(0, dch->base + CH_INTREN);
 	free_irq(dch->irq, dch);
+	d350_unmap_resource(dch);
 	vchan_free_chan_resources(&dch->vc);
 }
 
@@ -568,23 +1157,32 @@ static int d350_probe(struct platform_device *pdev)
 	dev_dbg(dev, "DMA-350 r%dp%d with %d channels, %d requests\n", r, p, dmac->nchan, dmac->nreq);
 
 	dmac->dma.dev = dev;
+	dmac->dma.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED);
+	dmac->dma.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_UNDEFINED);
 	for (int i = min(dw, 16); i > 0; i /= 2) {
 		dmac->dma.src_addr_widths |= BIT(i);
 		dmac->dma.dst_addr_widths |= BIT(i);
 	}
-	dmac->dma.directions = BIT(DMA_MEM_TO_MEM);
+	dmac->dma.directions = BIT(DMA_MEM_TO_MEM) |
+			BIT(DMA_MEM_TO_DEV) |
+			BIT(DMA_DEV_TO_MEM);
 	dmac->dma.descriptor_reuse = true;
 	dmac->dma.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
 	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);
+	dma_cap_set(DMA_SLAVE, dmac->dma.cap_mask);
+	dma_cap_set(DMA_CYCLIC, dmac->dma.cap_mask);
 	dmac->dma.device_prep_dma_memcpy = d350_prep_memcpy;
+	dmac->dma.device_prep_slave_sg = d350_prep_slave_sg;
+	dmac->dma.device_prep_dma_cyclic = d350_prep_dma_cyclic;
 	dmac->dma.device_pause = d350_pause;
 	dmac->dma.device_resume = d350_resume;
 	dmac->dma.device_terminate_all = d350_terminate_all;
 	dmac->dma.device_synchronize = d350_synchronize;
 	dmac->dma.device_tx_status = d350_tx_status;
 	dmac->dma.device_issue_pending = d350_issue_pending;
+	dmac->dma.device_config = d350_config;
 	INIT_LIST_HEAD(&dmac->dma.channels);
 
 	reg = readl_relaxed(base + DMANSECCTRL + NSEC_CTRL);
@@ -623,6 +1221,8 @@ static int d350_probe(struct platform_device *pdev)
 		reg |= FIELD_PREP(CH_LINK_MEMATTR, coherent ? MEMATTR_WB : MEMATTR_NC);
 		writel_relaxed(reg, dch->base + CH_LINKATTR);
 
+		dch->coherent = coherent;
+		dch->map.dir = DMA_NONE;
 		dch->vc.desc_free = d350_desc_free;
 		vchan_init(&dch->vc, &dmac->dma);
 	}
@@ -638,6 +1238,13 @@ static int d350_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to register DMA device\n");
 
+	ret = of_dma_controller_register(dev->of_node, d350_of_xlate, dmac);
+	if (ret) {
+		dma_async_device_unregister(&dmac->dma);
+		return dev_err_probe(dev, ret,
+				     "Failed to register OF DMA controller\n");
+	}
+
 	return 0;
 }
 
@@ -645,6 +1252,7 @@ static void d350_remove(struct platform_device *pdev)
 {
 	struct d350 *dmac = platform_get_drvdata(pdev);
 
+	of_dma_controller_free(pdev->dev.of_node);
 	dma_async_device_unregister(&dmac->dma);
 }
 
-- 
2.54.0

Re: [PATCH v3 2/6] dmaengine: arm-dma350: Add slave and cyclic transfer support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dma: Premature peripheral unmap in d350_map_resource destroys queued mappings
- [Medium] dma: Cyclic residue calculation rewinds position on race fallback
--

commit 8c6ce8c99f8eafa2efa38cc3958ee695ea49436e
Author: Jelly Jia <Jelly.Jia@cixtech.com>
Subject: dmaengine: arm-dma350: Add slave and cyclic transfer support

This commit adds DMA slave and cyclic transfer support to the Arm DMA-350
driver. It translates and caches peripheral addresses and provides completion
handling and residue reporting for the new descriptor types.

> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c

[ ... ]

> +static dma_addr_t d350_map_resource(struct d350_chan *dch,
> +				    phys_addr_t cpu_addr, size_t size,
> +				    enum dma_data_direction dir)
> +{
> +	struct device *dev = dch->vc.chan.device->dev;
> +	struct d350_chan_map *map = &dch->map;
> +	dma_addr_t dma_addr;
> +
> +	if (map->dir == dir && map->cpu_addr == cpu_addr &&
> +	    map->size == size)
> +		return map->dma_addr;
> +
> +	d350_unmap_resource(dch);
> +
> +	dma_addr = dma_map_resource(dev, cpu_addr, size, dir, 0);

[Severity: High]
If a client queues multiple descriptors sequentially for different
directions, does unmapping the resource immediately here break any pending
descriptors that still rely on the old mapping? Since this function modifies
the dch->map state without holding the channel lock, could concurrent
preparation calls also race and cause double-unmaps or unmapped accesses?

[ ... ]

> +static u32 d350_get_cyclic_residue(struct d350_chan *dch)
> +{
> +	struct d350_desc *desc = dch->desc;
> +	size_t cmd_size = D350_SLAVE_CMD_WORDS * sizeof(u32);
> +	u32 period_residue = desc->bytes - desc->period * desc->period_len;
> +	u32 linkaddr, linkaddrhi, hi_new, lo_new;
> +	u64 next_cmd;
> +	int retries = 3; /* 1st time unlucky, 2nd improbable, 3rd just broken */
> +
> +	/*
> +	 * desc->period counts the completed periods, so period_residue is the
> +	 * residue at the last period boundary. Subtract the progress made in
> +	 * the running period, read from CH_XSIZE: CH_LINKADDR points at the
> +	 * command after the one executing, so while the channel is still on the
> +	 * command of the running period it names the first command of the next
> +	 * period.
> +	 */
> +	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);
> +
> +	next_cmd = (linkaddr & ~CH_LINKADDR_EN) | ((u64)linkaddrhi << 32);
> +	if (next_cmd != desc->cmds_dma +
> +			((desc->period + 1) % desc->periods) * cmd_size)
> +		return period_residue;

[Severity: Medium]
When the hardware races ahead to the next period, does returning the
start-of-period residue cause the reported position to jump backwards?
Because progress was likely made and reported during previous polling,
reverting to period_residue could violate the monotonically advancing
position expected by the ALSA PCM core. Would returning the cached
dch->residue freeze the pointer safely instead?

> +
> +	period_residue -= desc->period_len - d350_get_residue(dch);
> +
> +	/*
> +	 * If the channel stepped to the next period while CH_XSIZE was read,
> +	 * the size belongs to a command of the next period and mixing it with
> +	 * the current one would overstate the residue: keep the value from the
> +	 * period boundary instead.
> +	 */
> +	hi_new = readl_relaxed(dch->base + CH_LINKADDRHI);
> +	lo_new = readl_relaxed(dch->base + CH_LINKADDR);
> +	if (hi_new != linkaddrhi || lo_new != linkaddr)
> +		return d350_desc_bytes(desc) - desc->period * desc->period_len;

[Severity: Medium]
Similar to the fallback above, if the hardware steps while we read CH_XSIZE,
does returning the start-of-period residue here also cause a backwards jump
in the reported position? Could this also be resolved by returning
dch->residue to freeze the progress safely, matching the fallback behavior
in d350_get_sg_residue()?

> +
> +	return period_residue;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012084155.2547480-1-Jelly.Jia@cixtech.com?part=2

Re: [PATCH v3 2/6] dmaengine: arm-dma350: Add slave and cyclic transfer support

[Frank Li]

On Mon, Oct 12, 2026 at 08:52:14AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] dma: Premature peripheral unmap in d350_map_resource destroys queued mappings
> - [Medium] dma: Cyclic residue calculation rewinds position on race fallback
> --

Please fix all sashiko report problem or provide explain.

Frank

[PATCH v3 3/6] dmaengine: arm-dma350: Sync the slave command list before starting

[Jelly Jia]

The slave command list is mapped with dma_map_single(DMA_TO_DEVICE)
before the command words are written into it. For a non-coherent device
this leaves the channel reading stale memory: the direct mapping path
only performs cache maintenance at map time, and a mapping bounced
through the SWIOTLB keeps the copy taken at map time, so writes after
the map never reach the device.

The channel then executes only the first (register-programmed) command
and stops when it follows CH_LINKADDR into the stale list. On Sky1 the
audio playback died after exactly one period (0.34 s at 48 kHz S16
stereo) with "tx data underrun"; the same path is used for multi-entry
slave sg lists.

Sync the command list for the device after filling it in, which also
refreshes the bounce buffer. This is a no-op for a DMA coherent device.

v2 -> v3:
- The descriptor keeps a single DMA API address for the command list, so
  the sync uses it directly.

Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
---
 drivers/dma/arm-dma350.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 7a198d35bef6..79bd6a04ea0c 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -304,6 +304,21 @@ static int d350_alloc_cmds(struct dma_chan *dchan, struct d350_desc *desc,
 	return 0;
 }
 
+/*
+ * The command words are written after the command list has been mapped, so
+ * make them visible to the device before the descriptor is submitted. This
+ * also refreshes the bounce buffer when the mapping is bounced through the
+ * SWIOTLB. It is a no-op for a DMA coherent device.
+ */
+static void d350_sync_cmds(struct device *dev, struct d350_desc *desc)
+{
+	if (!desc->cmds)
+		return;
+
+	dma_sync_single_for_device(dev, desc->cmds_dma, desc->cmds_size,
+				   DMA_TO_DEVICE);
+}
+
 static void d350_unmap_resource(struct d350_chan *dch)
 {
 	struct device *dev = dch->vc.chan.device->dev;
@@ -597,6 +612,8 @@ d350_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
 	if (sg_len > 1)
 		memcpy(desc->command, desc->cmds, cmd_size);
 
+	d350_sync_cmds(dchan->device->dev, desc);
+
 	return vchan_tx_prep(&dch->vc, &desc->vd, flags);
 
 err_free_cmds:
@@ -666,6 +683,8 @@ d350_prep_dma_cyclic(struct dma_chan *dchan, dma_addr_t buf_addr,
 	}
 	memcpy(desc->command, desc->cmds, cmd_size);
 
+	d350_sync_cmds(dchan->device->dev, desc);
+
 	return vchan_tx_prep(&dch->vc, &desc->vd, flags);
 }
 
-- 
2.54.0

[PATCH v3 4/6] dt-bindings: dma: arm,dma-350: Document the CIX Sky1 integration

[Jelly Jia]

The CIX Sky1 DMA-350 integration is the same device with SoC glue around
it, so it is described by the arm,dma-350 node itself with an additional
"cix,sky1-dma350" compatible rather than by a separate wrapper node.

Extend the binding with the CIX compatible and the resources the
integration needs: a clock, which the CIX variant requires, an optional
reset, and an optional reserved memory region used when the controller
cannot reach all of system memory.

v2 -> v3:
- Drop the cix,irq-router property and its conditional: the Sky1
  interrupt gate it described is set up by the boot firmware and is not
  configured by Linux, so no property describes it.
- The CIX compatible now requires a clock.

Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
---
 .../devicetree/bindings/dma/arm,dma-350.yaml  | 26 ++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
index 429f682f15d8..4e0f9ffb0039 100644
--- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
+++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
@@ -11,10 +11,22 @@ maintainers:
 
 allOf:
   - $ref: dma-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: cix,sky1-dma350
+    then:
+      required:
+        - clocks
 
 properties:
   compatible:
-    const: arm,dma-350
+    oneOf:
+      - const: arm,dma-350
+      - items:
+          - const: cix,sky1-dma350
+          - const: arm,dma-350
 
   reg:
     items:
@@ -36,6 +48,18 @@ properties:
     const: 1
     description: The cell is the trigger input number
 
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  memory-region:
+    maxItems: 1
+    description:
+      Reserved memory region used for DMA allocations when the controller
+      cannot reach all of system memory.
+
 required:
   - compatible
   - reg
-- 
2.54.0

Re: [PATCH v3 4/6] dt-bindings: dma: arm,dma-350: Document the CIX Sky1 integration

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2481 bytes --]

On Mon, Oct 12, 2026 at 04:41:53PM +0800, Jelly Jia wrote:
> The CIX Sky1 DMA-350 integration is the same device with SoC glue around
> it, so it is described by the arm,dma-350 node itself with an additional
> "cix,sky1-dma350" compatible rather than by a separate wrapper node.
> 
> Extend the binding with the CIX compatible and the resources the
> integration needs: a clock, which the CIX variant requires, an optional
> reset, and an optional reserved memory region used when the controller
> cannot reach all of system memory.
> 
> v2 -> v3:
> - Drop the cix,irq-router property and its conditional: the Sky1
>   interrupt gate it described is set up by the boot firmware and is not
>   configured by Linux, so no property describes it.
> - The CIX compatible now requires a clock.
> 
> Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
> Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
> ---
>  .../devicetree/bindings/dma/arm,dma-350.yaml  | 26 ++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> index 429f682f15d8..4e0f9ffb0039 100644
> --- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> +++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> @@ -11,10 +11,22 @@ maintainers:
>  
>  allOf:
>    - $ref: dma-controller.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: cix,sky1-dma350
> +    then:
> +      required:
> +        - clocks
>  
>  properties:
>    compatible:
> -    const: arm,dma-350
> +    oneOf:
> +      - const: arm,dma-350
> +      - items:
> +          - const: cix,sky1-dma350
> +          - const: arm,dma-350

Provided the device is actually functional if the fallback compatible is
used, this patch is fine. Otherwise, please drop the fallback from here.

Thanks,
Conor.

>  
>    reg:
>      items:
> @@ -36,6 +48,18 @@ properties:
>      const: 1
>      description: The cell is the trigger input number
>  
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  memory-region:
> +    maxItems: 1
> +    description:
> +      Reserved memory region used for DMA allocations when the controller
> +      cannot reach all of system memory.
> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.54.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v3 5/6] dmaengine: arm-dma350: Support integration clocks, resets and reserved memory

[Jelly Jia]

SoC integrations of the DMA-350 gate the controller with a clock and may
hold it in reset until the driver brings it up, and a DMA master which
cannot reach all of system memory may have a reserved memory region
attached to it. Bring the controller up with these resources: enable
the clock, release the reset, and attach the reserved memory region when
the device tree provides one.

The integration is described entirely by the device tree; the driver
contains no SoC-specific code.

v2 -> v3:
- The patch no longer carries CIX Sky1 match data, the interrupt router
  handling or the system resume hook: the Sky1 interrupt gate is set up
  by the boot firmware and is not configured by Linux.
- of_reserved_mem_device_release() is now also called when the DMA
  device or the OF DMA controller registration fails.

Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
---
 drivers/dma/arm-dma350.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 79bd6a04ea0c..d811a0dc9fe9 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -4,14 +4,17 @@
 
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
+#include <linux/clk.h>
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_dma.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/module.h>
 #include <linux/overflow.h>
 #include <linux/platform_device.h>
+#include <linux/reset.h>
 #include <linux/scatterlist.h>
 #include <linux/slab.h>
 
@@ -1139,12 +1142,28 @@ static void d350_free_chan_resources(struct dma_chan *chan)
 static int d350_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct reset_control *reset;
+	struct clk *clk;
 	struct d350 *dmac;
 	void __iomem *base;
 	u32 reg;
 	int ret, nchan, dw, aw, r, p;
 	bool coherent, memset;
 
+	clk = devm_clk_get_optional_enabled(dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "failed to enable clock\n");
+
+	reset = devm_reset_control_get_optional_exclusive(dev, NULL);
+	if (IS_ERR(reset))
+		return dev_err_probe(dev, PTR_ERR(reset),
+				     "failed to get reset\n");
+
+	ret = reset_control_reset(reset);
+	if (ret)
+		return ret;
+
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
@@ -1251,15 +1270,26 @@ static int d350_probe(struct platform_device *pdev)
 		dmac->dma.device_prep_dma_memset = d350_prep_memset;
 	}
 
+	if (of_property_present(dev->of_node, "memory-region")) {
+		ret = of_reserved_mem_device_init(dev);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "failed to attach reserved memory\n");
+	}
+
 	platform_set_drvdata(pdev, dmac);
 
 	ret = dma_async_device_register(&dmac->dma);
-	if (ret)
-		return dev_err_probe(dev, ret, "Failed to register DMA device\n");
+	if (ret) {
+		of_reserved_mem_device_release(dev);
+		return dev_err_probe(dev, ret,
+				     "Failed to register DMA device\n");
+	}
 
 	ret = of_dma_controller_register(dev->of_node, d350_of_xlate, dmac);
 	if (ret) {
 		dma_async_device_unregister(&dmac->dma);
+		of_reserved_mem_device_release(dev);
 		return dev_err_probe(dev, ret,
 				     "Failed to register OF DMA controller\n");
 	}
@@ -1273,6 +1303,7 @@ static void d350_remove(struct platform_device *pdev)
 
 	of_dma_controller_free(pdev->dev.of_node);
 	dma_async_device_unregister(&dmac->dma);
+	of_reserved_mem_device_release(&pdev->dev);
 }
 
 static const struct of_device_id d350_of_match[] __maybe_unused = {
-- 
2.54.0

Re: [PATCH v3 5/6] dmaengine: arm-dma350: Support integration clocks, resets and reserved memory

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dmaengine: probe fails on level-based reset controllers due to -ENOTSUPP
--

commit 9e76b19e390586bfc4ec916b67c4dd6c3a4800c5
Author: Jelly Jia <Jelly.Jia@cixtech.com>

dmaengine: arm-dma350: Support integration clocks, resets and reserved memory

This commit adds support for integrating the arm-dma350 controller with
clocks, resets, and reserved memory regions using the device tree. It enables
the clock, triggers the reset control, and attaches the reserved memory
before registering the DMA device.

> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index 79bd6a04ea0c5..d811a0dc9fe9e 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c

[ ... ]

> @@ -1139,12 +1142,28 @@ static void d350_free_chan_resources(struct dma_chan *chan)
>  static int d350_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	struct reset_control *reset;
> +	struct clk *clk;
>  	struct d350 *dmac;
>  	void __iomem *base;
>  	u32 reg;
>  	int ret, nchan, dw, aw, r, p;
>  	bool coherent, memset;
>  
> +	clk = devm_clk_get_optional_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk),
> +				     "failed to enable clock\n");
> +
> +	reset = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	if (IS_ERR(reset))
> +		return dev_err_probe(dev, PTR_ERR(reset),
> +				     "failed to get reset\n");
> +
> +	ret = reset_control_reset(reset);
> +	if (ret)
> +		return ret;

[Severity: High]
Is it intentional to fail the probe if the reset controller does not support
pulse-based resets?

If the underlying reset controller (such as reset-simple without a pulse
configuration) only supports level-based assert and deassert semantics,
reset_control_reset() will return -ENOTSUPP. Because the probe function
aborts on any error here, it will fatally fail to bind on those platforms.

The commit message states the intent is to release the reset, which usually
corresponds to reset_control_deassert(). Could this use
reset_control_deassert() instead, or is there a way to handle the -ENOTSUPP
error gracefully so the probe can succeed?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012084155.2547480-1-Jelly.Jia@cixtech.com?part=5

[PATCH v3 6/6] arm64: dts: cix: Describe the Sky1 DMA-350 bus integration

[Jelly Jia]

The FCH DMA-350 controller sits behind an interconnect window, so the
addresses its DMA master uses differ from the CPU physical addresses.
Describe the integration as a bus node carrying the dma-ranges of that
window, with the controller itself as a child, and give the controller
the SoC compatible and the clock feeding it.

The AUDSS DMA-350 controller is described the same way, with its clock
and reset. It has no consumer in the SoC dtsi yet: the audio controllers
that use it are not described upstream.

The bus nodes are address domains, not devices: they have no reg, so
they carry no unit-address in their node names.

v1 -> v2:
- The DMA-350 nodes are no longer wrappers with the generic controller
  as a child: each Sky1 DMA-350 is a single device node with the
  "cix,sky1-dma350", "arm,dma-350" compatible, its real MMIO region and
  the integration resources (clock, reset) that v1 kept on the wrapper.
  The integration and the block are one device, and the MMIO is the same
  as the generic node it replaces.
- The FCH and AUDSS DMA-350 masters sit behind SoC interconnect windows
  in a static DMA address domain: the AUDSS masters see the audio
  register and FIFO block at 0x20000000 and the DRAM window at
  0x30000000, which the CPU sees at 0x07010000 and 0xc0000000, and the
  FCH masters see the 0x040b0000 window at 0x040a0000. Their DMA view of
  the address space is not the CPU physical view.
- The windows are static hardware configuration, set up before Linux
  starts by the boot firmware / secure world: they are not programmable
  from Linux and must not be reconfigured by it. There is nothing for a
  Linux interconnect provider to manage here, so the translation cannot
  be modeled as one; it is a fixed property of the bus.
- It is described the standard devicetree way instead: a generic
  "simple-bus" parent with an identity "ranges;" and the "dma-ranges" of
  the window. This is not the rejected v1 wrapper and not a separate DMA
  wrapper/integration device: the parent has no device-specific or
  vendor wrapper compatible, only the generic "simple-bus" compatible,
  no reg, no clocks, no resets, no driver and no private address
  translation. Its only job is to carry the static translation, so it is
  only a generic bus/address-domain node.
- The "dma-ranges" have to sit on that parent for the OF core to build
  dev->dma_range_map: of_dma_configure_id() starts the lookup at the
  device's parent (bus_np = __of_get_dma_parent(np) in
  drivers/of/device.c), so a "dma-ranges" on the controller node itself
  would be ignored. The translation is applied by the DMA API, not by
  driver code: the driver maps slave resources with dma_map_resource()
  and contains no address parser.
- Mainline has similar bus nodes: the Agilex5 DMA bus (dma-bus@10db0000,
  a simple-bus with dma-ranges whose controller carries
  "altr,agilex5-axi-dma", "snps,axi-dma-1.01a") and the Marvell AC5
  buses (bus@80500000 with "ranges;" and "dma-ranges", and
  behind-32bit-controller@7f000000, whose only purpose is the DMA
  mapping of the devices below it).

v2 -> v3:
- The AUDSS interrupt gate is set up by the boot firmware and is not
  configured by Linux, so the cix,irq-router property is gone.
- The bus nodes carry no unit-address in their names: they have no reg,
  which made dtc warn (unit_address_vs_reg).

Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
---
 arch/arm64/boot/dts/cix/sky1.dtsi | 68 +++++++++++++++++++++++++------
 1 file changed, 56 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
index 0a820f45feb9..1262f34794d2 100644
--- a/arch/arm64/boot/dts/cix/sky1.dtsi
+++ b/arch/arm64/boot/dts/cix/sky1.dtsi
@@ -518,18 +518,33 @@ iomuxc: pinctrl@4170000 {
 			reg = <0x0 0x04170000 0x0 0x1000>;
 		};
 
-		fch_dmac: dma-controller@4190000 {
-			compatible = "arm,dma-350";
-			reg = <0x0 0x4190000 0x0 0x10000>;
-			interrupts = <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
-				     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
-				     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
-				     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
-				     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
-				     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
-				     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
-				     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>;
-			#dma-cells = <1>;
+		/*
+		 * The FCH DMA-350 master sits behind an interconnect window;
+		 * the dma-ranges below are its view of the address space.
+		 */
+		fch-dma-bus {
+			compatible = "simple-bus";
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges;
+			dma-ranges = <0x0 0x040a0000 0x0 0x040b0000 0x0 0x00040000>,
+				     <0x0 0x80000000 0x0 0x80000000 0x8 0x00000000>,
+				     <0x80 0x00000000 0x80 0x00000000 0x80 0x00000000>;
+
+			fch_dmac: dma-controller@4190000 {
+				compatible = "cix,sky1-dma350", "arm,dma-350";
+				reg = <0x0 0x04190000 0x0 0x10000>;
+				clocks = <&scmi_clk CLK_TREE_FCH_DMA_ACLK>;
+				interrupts = <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
+					     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
+					     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
+					     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
+					     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
+					     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
+					     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
+					     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>;
+				#dma-cells = <1>;
+			};
 		};
 
 		mbox_ap2se: mailbox@5060000 {
@@ -576,6 +591,35 @@ mbox_pm2ap: mailbox@65a0080 {
 			cix,mbox-dir = "rx";
 		};
 
+		/*
+		 * The AUDSS DMA-350 master sits behind an interconnect window;
+		 * the dma-ranges below are its view of the address space.
+		 */
+		audss-dma-bus {
+			compatible = "simple-bus";
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges;
+			dma-ranges = <0x0 0x20000000 0x0 0x07010000 0x0 0x00100000>,
+				     <0x0 0x30000000 0x0 0xc0000000 0x0 0x20000000>;
+
+			audss_dmac: dma-controller@7010000 {
+				compatible = "cix,sky1-dma350", "arm,dma-350";
+				reg = <0x0 0x07010000 0x0 0x10000>;
+				clocks = <&audss_cru CLK_DMAC_AXI>;
+				resets = <&audss_cru AUDSS_DMAC_SW_RST>;
+				interrupts = <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>,
+					     <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>,
+					     <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>,
+					     <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>,
+					     <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>,
+					     <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>,
+					     <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>,
+					     <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>;
+				#dma-cells = <1>;
+			};
+		};
+
 		audss_cru: clock-controller@7110000 {
 			compatible = "cix,sky1-audss-cru";
 			reg = <0x0 0x07110000 0x0 0x10000>;
-- 
2.54.0

Re: [PATCH v3 6/6] arm64: dts: cix: Describe the Sky1 DMA-350 bus integration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dts: cix: missing memory-region for restricted AUDSS DMA
--

commit bb152839bba0e4fcf17541cd1a7e5a05b3b33ae1
Author: Jelly Jia <Jelly.Jia@cixtech.com>

arm64: dts: cix: Describe the Sky1 DMA-350 bus integration

The FCH DMA-350 controller sits behind an interconnect window, so the
addresses its DMA master uses differ from the CPU physical addresses.
Describe the integration as a bus node carrying the dma-ranges of that
window, with the controller itself as a child, and give the controller
the SoC compatible and the clock feeding it.

> diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> index 0a820f45feb96..1262f34794d21 100644
> --- a/arch/arm64/boot/dts/cix/sky1.dtsi
> +++ b/arch/arm64/boot/dts/cix/sky1.dtsi

[ ... ]

> @@ -576,6 +591,35 @@ mbox_pm2ap: mailbox@65a0080 {
>  			cix,mbox-dir = "rx";
>  		};
>  
> +		/*
> +		 * The AUDSS DMA-350 master sits behind an interconnect window;
> +		 * the dma-ranges below are its view of the address space.
> +		 */
> +		audss-dma-bus {
> +			compatible = "simple-bus";
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			ranges;
> +			dma-ranges = <0x0 0x20000000 0x0 0x07010000 0x0 0x00100000>,
> +				     <0x0 0x30000000 0x0 0xc0000000 0x0 0x20000000>;
> +
> +			audss_dmac: dma-controller@7010000 {
> +				compatible = "cix,sky1-dma350", "arm,dma-350";

[Severity: High]
Does this node need a memory-region property to provide a restricted DMA pool?

The AUDSS DMA-350 controller restricts its DRAM access to a 512MB window at
0xc0000000 via its parent's dma-ranges. Since system RAM normally starts at
0x80000000, default allocations for descriptors will fall outside this reachable
window.

When a driver calls generic routines to allocate and map these descriptors,
such as in drivers/dma/arm-dma350.c:d350_alloc_cmds():

    desc->cmds_dma = dma_map_single(dev, desc->cmds, desc->cmds_size,
                                    DMA_TO_DEVICE);
    if (dma_mapping_error(dev, desc->cmds_dma)) {

the mapping will fail because the generic allocation is out of bounds for this
device's DMA ranges. The global SWIOTLB bounce buffer would also be
inaccessible for the same reason.

> +				reg = <0x0 0x07010000 0x0 0x10000>;
> +				clocks = <&audss_cru CLK_DMAC_AXI>;
> +				resets = <&audss_cru AUDSS_DMAC_SW_RST>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012084155.2547480-1-Jelly.Jia@cixtech.com?part=6

Re: [PATCH v3 6/6] arm64: dts: cix: Describe the Sky1 DMA-350 bus integration

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 7554 bytes --]

On Mon, Oct 12, 2026 at 04:41:55PM +0800, Jelly Jia wrote:
> The FCH DMA-350 controller sits behind an interconnect window, so the
> addresses its DMA master uses differ from the CPU physical addresses.
> Describe the integration as a bus node carrying the dma-ranges of that
> window, with the controller itself as a child, and give the controller
> the SoC compatible and the clock feeding it.
> 
> The AUDSS DMA-350 controller is described the same way, with its clock
> and reset. It has no consumer in the SoC dtsi yet: the audio controllers
> that use it are not described upstream.
> 
> The bus nodes are address domains, not devices: they have no reg, so
> they carry no unit-address in their node names.
> 

> v1 -> v2:
> - The DMA-350 nodes are no longer wrappers with the generic controller
>   as a child: each Sky1 DMA-350 is a single device node with the
>   "cix,sky1-dma350", "arm,dma-350" compatible, its real MMIO region and
>   the integration resources (clock, reset) that v1 kept on the wrapper.
>   The integration and the block are one device, and the MMIO is the same
>   as the generic node it replaces.
> - The FCH and AUDSS DMA-350 masters sit behind SoC interconnect windows
>   in a static DMA address domain: the AUDSS masters see the audio
>   register and FIFO block at 0x20000000 and the DRAM window at
>   0x30000000, which the CPU sees at 0x07010000 and 0xc0000000, and the
>   FCH masters see the 0x040b0000 window at 0x040a0000. Their DMA view of
>   the address space is not the CPU physical view.
> - The windows are static hardware configuration, set up before Linux
>   starts by the boot firmware / secure world: they are not programmable
>   from Linux and must not be reconfigured by it. There is nothing for a
>   Linux interconnect provider to manage here, so the translation cannot
>   be modeled as one; it is a fixed property of the bus.
> - It is described the standard devicetree way instead: a generic
>   "simple-bus" parent with an identity "ranges;" and the "dma-ranges" of
>   the window. This is not the rejected v1 wrapper and not a separate DMA
>   wrapper/integration device: the parent has no device-specific or
>   vendor wrapper compatible, only the generic "simple-bus" compatible,
>   no reg, no clocks, no resets, no driver and no private address
>   translation. Its only job is to carry the static translation, so it is
>   only a generic bus/address-domain node.
> - The "dma-ranges" have to sit on that parent for the OF core to build
>   dev->dma_range_map: of_dma_configure_id() starts the lookup at the
>   device's parent (bus_np = __of_get_dma_parent(np) in
>   drivers/of/device.c), so a "dma-ranges" on the controller node itself
>   would be ignored. The translation is applied by the DMA API, not by
>   driver code: the driver maps slave resources with dma_map_resource()
>   and contains no address parser.
> - Mainline has similar bus nodes: the Agilex5 DMA bus (dma-bus@10db0000,
>   a simple-bus with dma-ranges whose controller carries
>   "altr,agilex5-axi-dma", "snps,axi-dma-1.01a") and the Marvell AC5
>   buses (bus@80500000 with "ranges;" and "dma-ranges", and
>   behind-32bit-controller@7f000000, whose only purpose is the DMA
>   mapping of the devices below it).
> 
> v2 -> v3:
> - The AUDSS interrupt gate is set up by the boot firmware and is not
>   configured by Linux, so the cix,irq-router property is gone.
> - The bus nodes carry no unit-address in their names: they have no reg,
>   which made dtc warn (unit_address_vs_reg).

None of this should be in the commit message.

> 
> Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
> Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
> ---
>  arch/arm64/boot/dts/cix/sky1.dtsi | 68 +++++++++++++++++++++++++------
>  1 file changed, 56 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> index 0a820f45feb9..1262f34794d2 100644
> --- a/arch/arm64/boot/dts/cix/sky1.dtsi
> +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
> @@ -518,18 +518,33 @@ iomuxc: pinctrl@4170000 {
>  			reg = <0x0 0x04170000 0x0 0x1000>;
>  		};
>  
> -		fch_dmac: dma-controller@4190000 {
> -			compatible = "arm,dma-350";
> -			reg = <0x0 0x4190000 0x0 0x10000>;
> -			interrupts = <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
> -				     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
> -				     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
> -				     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
> -				     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
> -				     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
> -				     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
> -				     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>;
> -			#dma-cells = <1>;
> +		/*
> +		 * The FCH DMA-350 master sits behind an interconnect window;
> +		 * the dma-ranges below are its view of the address space.
> +		 */
> +		fch-dma-bus {
> +			compatible = "simple-bus";
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			ranges;
> +			dma-ranges = <0x0 0x040a0000 0x0 0x040b0000 0x0 0x00040000>,
> +				     <0x0 0x80000000 0x0 0x80000000 0x8 0x00000000>,
> +				     <0x80 0x00000000 0x80 0x00000000 0x80 0x00000000>;
> +
> +			fch_dmac: dma-controller@4190000 {
> +				compatible = "cix,sky1-dma350", "arm,dma-350";
> +				reg = <0x0 0x04190000 0x0 0x10000>;
> +				clocks = <&scmi_clk CLK_TREE_FCH_DMA_ACLK>;
> +				interrupts = <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
> +					     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
> +					     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
> +					     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
> +					     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
> +					     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
> +					     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>,
> +					     <GIC_SPI 303 IRQ_TYPE_LEVEL_HIGH 0>;

As I said on v2 an hour or so ago, I don't think this is correct, and
you should put an interrupt-map node into the syscon that contains the
irq routing registers, and have the syscon be the interrupt parent for
this node.

Thanks,
Conor.

> +				#dma-cells = <1>;
> +			};
>  		};
>  
>  		mbox_ap2se: mailbox@5060000 {
> @@ -576,6 +591,35 @@ mbox_pm2ap: mailbox@65a0080 {
>  			cix,mbox-dir = "rx";
>  		};
>  
> +		/*
> +		 * The AUDSS DMA-350 master sits behind an interconnect window;
> +		 * the dma-ranges below are its view of the address space.
> +		 */
> +		audss-dma-bus {
> +			compatible = "simple-bus";
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			ranges;
> +			dma-ranges = <0x0 0x20000000 0x0 0x07010000 0x0 0x00100000>,
> +				     <0x0 0x30000000 0x0 0xc0000000 0x0 0x20000000>;
> +
> +			audss_dmac: dma-controller@7010000 {
> +				compatible = "cix,sky1-dma350", "arm,dma-350";
> +				reg = <0x0 0x07010000 0x0 0x10000>;
> +				clocks = <&audss_cru CLK_DMAC_AXI>;
> +				resets = <&audss_cru AUDSS_DMAC_SW_RST>;
> +				interrupts = <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>,
> +					     <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>,
> +					     <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>,
> +					     <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>,
> +					     <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>,
> +					     <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>,
> +					     <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>,
> +					     <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>;
> +				#dma-cells = <1>;
> +			};
> +		};
> +
>  		audss_cru: clock-controller@7110000 {
>  			compatible = "cix,sky1-audss-cru";
>  			reg = <0x0 0x07110000 0x0 0x10000>;
> -- 
> 2.54.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]