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

[PATCH v1 0/5] 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).

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

Patch 3 documents the CIX Sky1 DMA-350 integration binding: a wrapper
node owning the SoC resources (clocks, resets, interrupt routing via
a syscon phandle, optional reserved memory) with the generic Arm
DMA-350 controller as its child.

Patch 4 adds the matching integration driver, which manages those
resources and populates the child controller.

Patch 5 adds the two Sky1 instances (FCH and AUDSS) to the SoC
devicetree.

The series is based on v7.3-rc1.

Testing:
- Built for arm64 with GCC 12.3 as both built-in and module, with no
  new warnings; sparse clean.
- checkpatch clean; dt_binding_check and dtbs_check pass for the new
  binding and nodes.
- Runtime-tested on CIX Sky1 boards with the FCH and AUDSS DMA-350
  instances.

Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>

Jelly Jia (5):
  dmaengine: arm-dma350: Fix source trigger bit
  dmaengine: arm-dma350: Add slave transfer support
  dt-bindings: dma: Add CIX Sky1 DMA-350 integration
  dmaengine: cix-sky1-dma350: Add Sky1 integration driver
  arm64: dts: cix: Add Sky1 DMA-350 nodes

 .../bindings/dma/cix,sky1-dma350.yaml         | 102 +++
 MAINTAINERS                                   |   2 +
 arch/arm64/boot/dts/cix/sky1.dtsi             |  61 +-
 drivers/dma/Kconfig                           |  15 +
 drivers/dma/Makefile                          |   1 +
 drivers/dma/arm-dma350.c                      | 642 +++++++++++++++++-
 drivers/dma/cix-sky1-dma350.c                 | 206 ++++++
 7 files changed, 993 insertions(+), 36 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/cix,sky1-dma350.yaml
 create mode 100644 drivers/dma/cix-sky1-dma350.c


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.54.0

[PATCH v1 1/5] 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")
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 v1 1/5] dmaengine: arm-dma350: Fix source trigger bit

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver ignores the return value of `dma_set_mask_and_coherent()`, which can lead to silent memory corruption if the requested DMA mask is unsupported.
--

commit c925d144b95da8512ff2006d79c7add5a368be10
Author: Jelly Jia <Jelly.Jia@cixtech.com>
Subject: dmaengine: arm-dma350: Fix source trigger bit

This commit corrects the bit used for the source trigger enable in the
CH_CTRL register. It changes the value from bit 26 to the proper bit 25 to
avoid inadvertently enabling the destination trigger input. This prevents
silent failures in transfers that wait for a peripheral request.

> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index 09403aca8bb06..4e17130de6c87 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)

While reviewing this, I noticed a pre-existing issue in the device probe
path:

drivers/dma/arm-dma350.c:d350_probe() {
    ...
	reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG0);
	nchan = FIELD_GET(DMA_CFG_NUM_CHANNELS, reg) + 1;
	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;
    ...
}

[Severity: High]
This is a pre-existing issue, but should the return value of
dma_set_mask_and_coherent() be checked and handled here?

If the requested mask is rejected by the architecture or IOMMU during
device probe, couldn't the system fall back to a default mask (typically
32-bit)?

If the dynamically read hardware address width (aw) is less than 32, the
kernel might map memory addresses that the hardware cannot reach. Could
this lead to address truncation by the hardware and silent memory
corruption during DMA transfers?

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

[PATCH v1 2/5] dmaengine: arm-dma350: Add slave 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.

This adds scatter-gather and cyclic preparation, command-list
allocation, per-channel resource mapping, and residue reporting.

Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
---
 drivers/dma/arm-dma350.c | 640 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 617 insertions(+), 23 deletions(-)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 4e17130de6c8..85e8c5e42804 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -3,12 +3,19 @@
 // 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_address.h>
+#include <linux/of_dma.h>
 #include <linux/module.h>
+#include <linux/overflow.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/scatterlist.h>
+#include <linux/slab.h>
 
 #include "dmaengine.h"
 #include "virt-dma.h"
@@ -102,6 +109,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 +158,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 +173,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 +202,26 @@ struct d350_desc {
 	u32 command[16];
 	u16 xsize;
 	u16 xsizehi;
+	u32 *cmds;
+	dma_addr_t cmds_dma;	/* DMA API address from dma_alloc_coherent() */
+	dma_addr_t cmds_bus;	/* DMA350-visible address for CH_LINKADDR */
+	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 +232,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 +258,462 @@ 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_free_coherent(dev, desc->cmds_size, desc->cmds,
+				  desc->cmds_dma);
+	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)
+{
+	size_t cmd_size = D350_SLAVE_CMD_WORDS * sizeof(u32);
+
+	if (check_mul_overflow(ncmds, cmd_size, &desc->cmds_size))
+		return -ENOMEM;
+
+	desc->cmds = dma_alloc_coherent(dchan->device->dev, desc->cmds_size,
+					&desc->cmds_dma, GFP_NOWAIT);
+	if (!desc->cmds)
+		return -ENOMEM;
+
+	desc->cmds_bus = desc->cmds_dma;
+	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;
+}
+
+/*
+ * Translate a CPU physical/resource address to the address visible to DMA350
+ * using the dma-ranges property of its parent bus. This is needed for internal
+ * interconnect windows where the DMA master sees slave peripherals at
+ * different addresses from the CPU.
+ */
+static int d350_xlate_parent_dma_range(struct device *dev, phys_addr_t phys,
+				       size_t size, dma_addr_t *dma)
+{
+	struct device_node *parent;
+	struct of_range_parser parser;
+	struct of_range range;
+	int ret = -ENOENT;
+
+	parent = of_get_parent(dev->of_node);
+	if (!parent)
+		return -ENOENT;
+
+	if (of_pci_dma_range_parser_init(&parser, parent))
+		goto out_put;
+
+	for_each_of_range(&parser, &range) {
+		u64 offset;
+
+		if (phys < range.cpu_addr)
+			continue;
+
+		offset = phys - range.cpu_addr;
+		if (offset >= range.size)
+			continue;
+
+		if (size > range.size - offset)
+			continue;
+
+		*dma = range.bus_addr + offset;
+		ret = 0;
+		break;
+	}
+
+out_put:
+	of_node_put(parent);
+	return ret;
+}
+
+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;
+	int ret;
+
+	if (map->dir == dir && map->cpu_addr == cpu_addr &&
+	    map->size == size)
+		return map->dma_addr;
+
+	d350_unmap_resource(dch);
+
+	if (!device_iommu_mapped(dev)) {
+		ret = d350_xlate_parent_dma_range(dev, cpu_addr, size,
+						  &dma_addr);
+		if (!ret)
+			goto done;
+
+		if (ret != -ENOENT) {
+			dev_err(dev, "translate resource failed ch%u phys=%pa size=%zu\n",
+				dch->vc.chan.chan_id, &cpu_addr, size);
+			return DMA_MAPPING_ERROR;
+		}
+	}
+
+	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;
+
+done:
+	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 dma_chan *chan;
+	struct d350_chan *dch;
+	u32 req;
+
+	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;
+	}
+
+	chan = dma_get_any_slave_channel(&dmac->dma);
+	if (!chan) {
+		dev_err(dmac->dma.dev, "can't get a dma channel\n");
+		return NULL;
+	}
+
+	dch = to_d350_chan(chan);
+	if (!dch->has_trig) {
+		dev_err(dmac->dma.dev, "channel %d has no trigger support\n",
+			chan->chan_id);
+		dma_release_channel(chan);
+		return NULL;
+	}
+	dch->req = req;
+
+	return chan;
+}
+
+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, 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_addr ? 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;
+		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_addr = desc->cmds_bus + (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_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_bus +
+				  ((period + 1) % desc->periods) * cmd_size;
+
+		d350_fill_slave_cmd(dch, desc, cmd, mem, dev_dma_addr,
+				    period_len, 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 +727,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 +766,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 +840,45 @@ 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 residue;
+	u64 next_cmd;
+
+	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.
+	 */
+	next_cmd = readl_relaxed(dch->base + CH_LINKADDR) & ~CH_LINKADDR_EN;
+	next_cmd |= (u64)readl_relaxed(dch->base + CH_LINKADDRHI) << 32;
+
+	if (!next_cmd) {
+		cmd = desc->ncmds - 1;
+	} else {
+		for (i = 1; i < desc->ncmds; i++) {
+			if (next_cmd == desc->cmds_bus + i * cmd_size) {
+				cmd = i - 1;
+				break;
+			}
+		}
+		if (i == desc->ncmds)
+			return dch->residue;
+	}
+
+	residue = d350_get_residue(dch);
+	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 +909,20 @@ 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_desc *desc)
+{
+	return desc->bytes - desc->period * desc->period_len;
+}
+
+static u32 d350_get_active_residue(struct d350_chan *dch)
+{
+	if (dch->desc->cyclic)
+		return d350_get_cyclic_residue(dch->desc);
+
+	return d350_get_sg_residue(dch);
 }
 
 static enum dma_status d350_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
@@ -387,7 +940,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 +1022,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 +1055,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(desc);
+			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 +1099,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 +1143,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 +1207,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 +1224,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 +1238,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 v1 2/5] dmaengine: arm-dma350: Add slave transfer support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:

New issues:
- [High] DMA descriptors in memory are written using native endianness, causing corruption on big-endian CPUs.
- [High] Cyclic DMA residue calculation relies on fragile software tracking, lacks sub-period precision, and permanently desynchronizes if interrupts coalesce.
- [High] d350_of_xlate() incorrectly fails to allocate a channel if the first free channel lacks trigger support.
- [High] Valid DMA address of 0 is erroneously treated as a sentinel, disabling descriptor linking and breaking cyclic/SG transfers.
- [High] Calling dma_free_coherent() from the vchan_complete tasklet causes sleeping in atomic context.
- [High] Race condition between reading CH_LINKADDR and CH_XSIZE leads to invalid, non-monotonic DMA residue calculations.
- [Medium] Trigger block size ignores configured maxburst for DEV_TO_MEM transfers, degrading performance.
- [Medium] Torn read of the 64-bit hardware CH_LINKADDR register due to missing retry loop.

Pre-existing issues:
- [High] Spurious DMA_ERROR returned for descriptors that have been submitted but not yet issued.
--

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

dmaengine: arm-dma350: Add slave transfer support

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

> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index 4e17130de6c87..85e8c5e428044 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c
[ ... ]
> +static void d350_free_cmds(struct device *dev, struct d350_desc *desc)
> +{
> +	if (desc->cmds)
> +		dma_free_coherent(dev, desc->cmds_size, desc->cmds,
> +				  desc->cmds_dma);
> +	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);
> +}

[Severity: High]
Can this sleep in atomic context?

d350_desc_free() is registered as the virt-dma desc_free callback, which
executes in a tasklet (softirq) context. However, dma_free_coherent()
might sleep (e.g., when flushing IOMMU page tables), which could trigger
scheduling-while-atomic BUGs.

[ ... ]
> +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 dma_chan *chan;
> +	struct d350_chan *dch;
> +	u32 req;
[ ... ]
> +	chan = dma_get_any_slave_channel(&dmac->dma);
> +	if (!chan) {
> +		dev_err(dmac->dma.dev, "can't get a dma channel\n");
> +		return NULL;
> +	}
> +
> +	dch = to_d350_chan(chan);
> +	if (!dch->has_trig) {
> +		dev_err(dmac->dma.dev, "channel %d has no trigger support\n",
> +			chan->chan_id);
> +		dma_release_channel(chan);
> +		return NULL;
> +	}

[Severity: High]
Does this incorrectly reject valid requests on heterogeneous DMA configurations?

If dma_get_any_slave_channel() happens to return a channel lacking trigger
support, the request is dropped entirely instead of continuing to search
the remaining free channels for a capable one.

[ ... ]
> +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, 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) |

[Severity: Medium]
Does this degrade performance for device-to-memory transfers?

The trigger block size ignores the configured maxburst and is hardcoded to 0
for DEV_TO_MEM transfers. This means only a single item is processed per
hardware trigger, which might cause peripheral FIFOs to overflow if they
expect burst servicing.

> +		  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);

[Severity: High]
Will this cause corruption on big-endian CPUs?

Native CPU-endian words are being written directly to the DMA command list
buffer (desc->cmds), which will be fetched by the little-endian hardware.
Should this use cpu_to_le32 wrappers to avoid swapped addresses and sizes?

[ ... ]
> +	cmd[11] = trigcfg;
> +	cmd[12] = lower_32_bits(link_addr) |
> +		  (link_addr ? CH_LINKADDR_EN : 0);

[Severity: High]
Is it safe to use link_addr as a boolean condition here?

dma_alloc_coherent() can validly return 0 as a bus address. If it does,
CH_LINKADDR_EN will not be set, which would prematurely terminate and break
cyclic or scatter-gather transfers.

> +	cmd[13] = upper_32_bits(link_addr);
> +}
[ ... ]
> +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 residue;
> +	u64 next_cmd;
> +
> +	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.
> +	 */
> +	next_cmd = readl_relaxed(dch->base + CH_LINKADDR) & ~CH_LINKADDR_EN;
> +	next_cmd |= (u64)readl_relaxed(dch->base + CH_LINKADDRHI) << 32;

[Severity: Medium]
Could this result in a torn read of the 64-bit hardware CH_LINKADDR register?

The two 32-bit reads are executed sequentially without protection against
concurrent hardware updates, unlike the safe retry loop you used for CH_XSIZE.
If the hardware advances the command chain between the reads, the CPU constructs
a torn address.

> +
> +	if (!next_cmd) {
> +		cmd = desc->ncmds - 1;
> +	} else {
> +		for (i = 1; i < desc->ncmds; i++) {
> +			if (next_cmd == desc->cmds_bus + i * cmd_size) {
> +				cmd = i - 1;
> +				break;
> +			}
> +		}
> +		if (i == desc->ncmds)
> +			return dch->residue;
> +	}
> +
> +	residue = d350_get_residue(dch);

[Severity: High]
Is there a race condition between reading CH_LINKADDR and CH_XSIZE?

d350_get_residue() reads CH_XSIZE. If the hardware completes command N and
starts N+1 between the earlier read of CH_LINKADDR and the read of CH_XSIZE,
the driver calculates residue using command N's position in the loop but
command N+1's remaining bytes. This would double-count the size of N+1 and
cause the reported residue to jump upwards, breaking monotonicity.

> +	for (i = cmd + 1; i < desc->ncmds; i++)
> +		residue += desc->cmd_len[i];
> +
> +	return residue;
> +}
[ ... ]
> +static u32 d350_get_cyclic_residue(struct d350_desc *desc)
> +{
> +	return desc->bytes - desc->period * desc->period_len;
> +}

[Severity: High]
Does this provide sufficient precision for cyclic consumers like ALSA?

This calculates an inaccurate residue that ignores intra-period hardware
progress, returning only period boundaries. ALSA PCM pointers typically
require high-resolution, accurate sub-period updates.

[ ... ]
>  static enum dma_status d350_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
>  				      struct dma_tx_state *state)
>  {
>  	struct d350_chan *dch = to_d350_chan(chan);
>  	struct virt_dma_desc *vd;
>  	enum dma_status status;
>  	unsigned long flags;
>  	u32 residue = 0;
>  
>  	status = dma_cookie_status(chan, cookie, state);
>  
>  	spin_lock_irqsave(&dch->vc.lock, flags);
>  	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));
>  	} else if (status == DMA_IN_PROGRESS) {
>  		/* Somebody else terminated it? */
>  		status = DMA_ERROR;
>  	}

[Severity: High]
This is a pre-existing issue, but does this incorrectly return DMA_ERROR for
submitted but unissued descriptors?

vchan_find_desc() only searches the desc_issued list. If a client queries
status after tx_submit but before issue_pending, this explicit overwrite
converts a valid DMA_IN_PROGRESS status into DMA_ERROR.

[ ... ]
>  static irqreturn_t d350_irq(int irq, void *data)
>  {
[ ... ]
> +	if (irq_status & CH_STAT_INTR_ERR) {
[ ... ]
> +	} else {
> +		if (desc->cyclic) {
> +			desc->period = (desc->period + 1) % desc->periods;
> +			dch->residue = d350_get_cyclic_residue(desc);
> +			vchan_cyclic_callback(vd);

[Severity: High]
Can this permanently desynchronize the cyclic stream?

Unconditionally advancing the period by 1 software counter will lag behind the
actual hardware position if interrupt coalescing occurs or interrupts are
delayed, causing multiple hardware periods to complete between IRQ handling.

> +		} else {
> +			dch->status = DMA_COMPLETE;
> +			dch->residue = 0;
> +			dch->desc = NULL;
> +			vchan_cookie_complete(vd);
> +			d350_start_next(dch);
> +		}
>  	}
>  	spin_unlock(&dch->vc.lock);
>  
>  	return IRQ_HANDLED;
>  }

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

[PATCH v1 3/5] dt-bindings: dma: Add CIX Sky1 DMA-350 integration

[Jelly Jia]

Document the CIX Sky1 integration wrapper around an Arm DMA-350 controller.

The wrapper owns SoC integration resources. The child node describes the
generic Arm DMA-350 IP.

Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
---
 .../bindings/dma/cix,sky1-dma350.yaml         | 102 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 103 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/cix,sky1-dma350.yaml

diff --git a/Documentation/devicetree/bindings/dma/cix,sky1-dma350.yaml b/Documentation/devicetree/bindings/dma/cix,sky1-dma350.yaml
new file mode 100644
index 000000000000..e533f355d135
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/cix,sky1-dma350.yaml
@@ -0,0 +1,102 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/cix,sky1-dma350.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: CIX Sky1 DMA-350 Integration
+
+maintainers:
+  - CIX Kernel Team <cix-kernel-upstream@cixtech.com>
+
+description:
+  The CIX Sky1 DMA-350 integration driver owns SoC resources around an
+  Arm DMA-350 controller, such as clocks, resets, interrupt routing, and
+  optional reserved memory. Reserved memory is described on this parent node
+  and assigned by the integration driver to the child DMA-350 device. The child
+  node describes the generic DMA-350 IP.
+
+properties:
+  $nodename:
+    pattern: "^dma@[0-9a-f]+$"
+
+  compatible:
+    const: cix,sky1-dma350
+
+  "#address-cells":
+    const: 2
+
+  "#size-cells":
+    const: 2
+
+  ranges: true
+
+  dma-ranges: true
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  power-domains:
+    maxItems: 1
+
+  cix,irq-router:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Syscon phandle for the Sky1 subsystem register block that routes
+      DMA-350 channel interrupts to the AP interrupt controller.
+
+  memory-region:
+    maxItems: 1
+    description:
+      Reserved memory pool assigned by the CIX Sky1 DMA-350 integration driver
+      to the child Arm DMA-350 device.
+
+patternProperties:
+  '^dma-controller@[0-9a-f]+$':
+    $ref: arm,dma-350.yaml#
+
+required:
+  - compatible
+  - "#address-cells"
+  - "#size-cells"
+  - ranges
+  - dma-ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    / {
+        #address-cells = <2>;
+        #size-cells = <2>;
+        interrupt-parent = <&gic>;
+
+        gic: interrupt-controller {
+            #address-cells = <0>;
+            interrupt-controller;
+            #interrupt-cells = <4>;
+        };
+
+        dma@7010000 {
+            compatible = "cix,sky1-dma350";
+            #address-cells = <2>;
+            #size-cells = <2>;
+            ranges = <0x0 0x0 0x0 0x07010000 0x0 0x10000>;
+            dma-ranges = <0x0 0x20000000 0x0 0x07010000 0x0 0x00100000>;
+            clocks = <&clk 9>;
+            resets = <&rst 15>;
+            cix,irq-router = <&audss_cru>;
+
+            dma-controller@0 {
+                compatible = "arm,dma-350";
+                reg = <0x0 0x0 0x0 0x10000>;
+                interrupts = <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>;
+                #dma-cells = <1>;
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 3a19da74d00c..feff8f9ecb2b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2825,6 +2825,7 @@ L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
 T:	git https://github.com/cixtech/linux-mainline.git
 F:	Documentation/devicetree/bindings/arm/cix.yaml
+F:	Documentation/devicetree/bindings/dma/cix,sky1-dma350.yaml
 F:	Documentation/devicetree/bindings/mailbox/cix,sky1-mbox.yaml
 F:	arch/arm64/boot/dts/cix/
 F:	drivers/mailbox/cix-mailbox.c
-- 
2.54.0

Re: [PATCH v1 3/5] dt-bindings: dma: Add CIX Sky1 DMA-350 integration

[Conor Dooley]

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

On Mon, Sep 07, 2026 at 11:34:02AM +0800, Jelly Jia wrote:
> Document the CIX Sky1 integration wrapper around an Arm DMA-350 controller.
> 
> The wrapper owns SoC integration resources. The child node describes the
> generic Arm DMA-350 IP.
> 
> Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
> ---
>  .../bindings/dma/cix,sky1-dma350.yaml         | 102 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 103 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/dma/cix,sky1-dma350.yaml
> 
> diff --git a/Documentation/devicetree/bindings/dma/cix,sky1-dma350.yaml b/Documentation/devicetree/bindings/dma/cix,sky1-dma350.yaml
> new file mode 100644
> index 000000000000..e533f355d135
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/cix,sky1-dma350.yaml
> @@ -0,0 +1,102 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/dma/cix,sky1-dma350.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: CIX Sky1 DMA-350 Integration
> +
> +maintainers:
> +  - CIX Kernel Team <cix-kernel-upstream@cixtech.com>
> +
> +description:
> +  The CIX Sky1 DMA-350 integration driver owns SoC resources around an
> +  Arm DMA-350 controller, such as clocks, resets, interrupt routing, and
> +  optional reserved memory. Reserved memory is described on this parent node
> +  and assigned by the integration driver to the child DMA-350 device. The child
> +  node describes the generic DMA-350 IP.

I cannot really speak to whether this is a correct thing to do with a
dma-350, but it seems to me like something that should be resolved with
a device specific comaptible in the dma-350 node. Someone more familar
with the IP will have to comment on that.

However I would like to know how this impacts the existing dma-350 in he
sky1 devicetree.

> +
> +properties:
> +  $nodename:
> +    pattern: "^dma@[0-9a-f]+$"
> +
> +  compatible:
> +    const: cix,sky1-dma350
> +
> +  "#address-cells":
> +    const: 2
> +
> +  "#size-cells":
> +    const: 2
> +
> +  ranges: true
> +
> +  dma-ranges: true
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  cix,irq-router:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Syscon phandle for the Sky1 subsystem register block that routes
> +      DMA-350 channel interrupts to the AP interrupt controller.
> +
> +  memory-region:
> +    maxItems: 1
> +    description:
> +      Reserved memory pool assigned by the CIX Sky1 DMA-350 integration driver
> +      to the child Arm DMA-350 device.
> +
> +patternProperties:
> +  '^dma-controller@[0-9a-f]+$':
> +    $ref: arm,dma-350.yaml#
> +
> +required:
> +  - compatible
> +  - "#address-cells"
> +  - "#size-cells"
> +  - ranges
> +  - dma-ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    / {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +        interrupt-parent = <&gic>;
> +
> +        gic: interrupt-controller {
> +            #address-cells = <0>;
> +            interrupt-controller;
> +            #interrupt-cells = <4>;
> +        };
> +
> +        dma@7010000 {
> +            compatible = "cix,sky1-dma350";
> +            #address-cells = <2>;
> +            #size-cells = <2>;
> +            ranges = <0x0 0x0 0x0 0x07010000 0x0 0x10000>;
> +            dma-ranges = <0x0 0x20000000 0x0 0x07010000 0x0 0x00100000>;
> +            clocks = <&clk 9>;
> +            resets = <&rst 15>;
> +            cix,irq-router = <&audss_cru>;
> +
> +            dma-controller@0 {
> +                compatible = "arm,dma-350";
> +                reg = <0x0 0x0 0x0 0x10000>;
> +                interrupts = <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>;
> +                #dma-cells = <1>;
> +            };
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3a19da74d00c..feff8f9ecb2b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2825,6 +2825,7 @@ L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
>  S:	Maintained
>  T:	git https://github.com/cixtech/linux-mainline.git
>  F:	Documentation/devicetree/bindings/arm/cix.yaml
> +F:	Documentation/devicetree/bindings/dma/cix,sky1-dma350.yaml
>  F:	Documentation/devicetree/bindings/mailbox/cix,sky1-mbox.yaml
>  F:	arch/arm64/boot/dts/cix/
>  F:	drivers/mailbox/cix-mailbox.c
> -- 
> 2.54.0
> 

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

Re: [PATCH v1 3/5] dt-bindings: dma: Add CIX Sky1 DMA-350 integration

[Jelly Jia]

Hi Conor,

Thanks for the review.

> I cannot really speak to whether this is a correct thing to do with a
> dma-350, but it seems to me like something that should be resolved with
> a device specific comaptible in the dma-350 node. Someone more familar
> with the IP will have to comment on that.

The wrapper is there to keep the platform integration bits (clocks,
resets, interrupt routing) out of the generic driver: the arm-dma-350
child stays plain so the existing driver binds to it unchanged. I don't
know whether other dma350 integrations need the same resources, so I
did not want to push them into the generic node.

> However I would like to know how this impacts the existing dma-350 in he
> sky1 devicetree.

Patch 5 converts the existing FCH node to this form: the register window
and the eight GIC interrupts are unchanged, the arm-dma-350 controller
becomes the dma-controller@0 child, and the SCMI clock feeding the
instance is added. The AUDSS instance is new. I'll describe this
conversion in the patch 5 commit message in v2.

Best regards,
Jelly

Re: [PATCH v1 3/5] dt-bindings: dma: Add CIX Sky1 DMA-350 integration

[Conor Dooley]

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

On Wed, Sep 09, 2026 at 02:05:12PM +0800, Jelly Jia wrote:
> Hi Conor,
> 
> Thanks for the review.
> 
> > I cannot really speak to whether this is a correct thing to do with a
> > dma-350, but it seems to me like something that should be resolved with
> > a device specific comaptible in the dma-350 node. Someone more familar
> > with the IP will have to comment on that.
> 
> The wrapper is there to keep the platform integration bits (clocks,
> resets, interrupt routing) out of the generic driver: the arm-dma-350
> child stays plain so the existing driver binds to it unchanged. I don't
> know whether other dma350 integrations need the same resources, so I
> did not want to push them into the generic node.

Don't worry about this, given the limited extent of the wrapper driver,
at worst you will end up with a different probe function. The specific
compatible that you'll use will prevent the code relating to these
resources running on other platforms.

Thanks,
Conor.

> 
> > However I would like to know how this impacts the existing dma-350 in he
> > sky1 devicetree.
> 
> Patch 5 converts the existing FCH node to this form: the register window
> and the eight GIC interrupts are unchanged, the arm-dma-350 controller
> becomes the dma-controller@0 child, and the SCMI clock feeding the
> instance is added. The AUDSS instance is new. I'll describe this
> conversion in the patch 5 commit message in v2.
> 
> Best regards,
> Jelly

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

Re: [PATCH v1 3/5] dt-bindings: dma: Add CIX Sky1 DMA-350 integration

[Jelly Jia]

Hi Conor,

Thanks, understood: with the SoC-specific compatible scoping the
integration code, other platforms are unaffected either way.

For v2 my preference is to keep the wrapper, since it leaves the
generic arm-dma350 driver and its binding untouched. Does that work
for you, or would you rather see the dma-350 node itself carry a
device-specific compatible, with the clocks, resets and
memory-region described on it? I'd also like Robin's take on the
arm-dma350 side before restructuring that far.

Best regards,
Jelly

Re: [PATCH v1 3/5] dt-bindings: dma: Add CIX Sky1 DMA-350 integration

[Conor Dooley]

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

On Sun, Sep 20, 2026 at 01:15:01PM +0800, Jelly Jia wrote:
> Hi Conor,
> 
> Thanks, understood: with the SoC-specific compatible scoping the
> integration code, other platforms are unaffected either way.
> 
> For v2 my preference is to keep the wrapper, since it leaves the
> generic arm-dma350 driver and its binding untouched. Does that work
> for you, or would you rather see the dma-350 node itself carry a
> device-specific compatible, with the clocks, resets and
> memory-region described on it? I'd also like Robin's take on the
> arm-dma350 side before restructuring that far.

The latter, since that's what we do for almost every other device under
the sun. Robin's take would probably be worth hearing, but you removed
all the context while replying which makes that more difficult...

> 
> Best regards,
> Jelly

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

Re: [PATCH v1 3/5] dt-bindings: dma: Add CIX Sky1 DMA-350 integration

[Krzysztof Kozlowski]

On 22/09/2026 19:04, Conor Dooley wrote:
> On Sun, Sep 20, 2026 at 01:15:01PM +0800, Jelly Jia wrote:
>> Hi Conor,
>>
>> Thanks, understood: with the SoC-specific compatible scoping the
>> integration code, other platforms are unaffected either way.
>>
>> For v2 my preference is to keep the wrapper, since it leaves the
>> generic arm-dma350 driver and its binding untouched. Does that work
>> for you, or would you rather see the dma-350 node itself carry a
>> device-specific compatible, with the clocks, resets and
>> memory-region described on it? I'd also like Robin's take on the
>> arm-dma350 side before restructuring that far.
> 
> The latter, since that's what we do for almost every other device under
> the sun. Robin's take would probably be worth hearing, but you removed
> all the context while replying which makes that more difficult...

Following DWC USB example, I think wrappers are discouraged. The
integration of a block is together with that block an entire device.
Integration is not something separate, at least usually. If it is,
author should provide some arguments in terms of hardware. The arguments
of driver are unconvincing.

And to prove this is same device (integration = block), the MMIO is the
same.

Best regards,
Krzysztof

[PATCH v1 4/5] dmaengine: cix-sky1-dma350: Add Sky1 integration driver

[Jelly Jia]

Add a CIX Sky1 integration driver for Arm DMA-350 instances.

The driver manages clocks, reset, optional interrupt routing, optional
reserved memory attachment, and populates the child Arm DMA-350
controller.

Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
---
 MAINTAINERS                   |   1 +
 drivers/dma/Kconfig           |  15 +++
 drivers/dma/Makefile          |   1 +
 drivers/dma/cix-sky1-dma350.c | 206 ++++++++++++++++++++++++++++++++++
 4 files changed, 223 insertions(+)
 create mode 100644 drivers/dma/cix-sky1-dma350.c

diff --git a/MAINTAINERS b/MAINTAINERS
index feff8f9ecb2b..8d972fe2c994 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2828,6 +2828,7 @@ F:	Documentation/devicetree/bindings/arm/cix.yaml
 F:	Documentation/devicetree/bindings/dma/cix,sky1-dma350.yaml
 F:	Documentation/devicetree/bindings/mailbox/cix,sky1-mbox.yaml
 F:	arch/arm64/boot/dts/cix/
+F:	drivers/dma/cix-sky1-dma350.c
 F:	drivers/mailbox/cix-mailbox.c
 K:	\bcix\b
 
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index ae6a682c9f76..51c881dfa6d3 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -147,6 +147,21 @@ config DMA_BCM2835
 	select DMA_ENGINE
 	select DMA_VIRTUAL_CHANNELS
 
+config CIX_SKY1_DMA350
+	tristate "CIX Sky1 DMA-350 integration support"
+	depends on OF && ARM_DMA350
+	depends on OF_RESERVED_MEM
+	depends on ARCH_CIX || COMPILE_TEST
+	default ARCH_CIX
+	select MFD_SYSCON
+	help
+	  Enable support for the CIX Sky1 integration around Arm
+	  DMA-350 controllers.
+
+	  This driver owns SoC integration resources such as clocks, resets,
+	  reserved memory attachment, and interrupt routing registers, while the
+	  child controller is handled by the generic Arm DMA-350 driver.
+
 config DMA_JZ4780
 	tristate "JZ4780 DMA support"
 	depends on MIPS || COMPILE_TEST
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index 14aa086629d5..a69964251f68 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_AT_XDMAC) += at_xdmac.o
 obj-$(CONFIG_AXI_DMAC) += dma-axi-dmac.o
 obj-$(CONFIG_BCM_SBA_RAID) += bcm-sba-raid.o
 obj-$(CONFIG_DMA_BCM2835) += bcm2835-dma.o
+obj-$(CONFIG_CIX_SKY1_DMA350) += cix-sky1-dma350.o
 obj-$(CONFIG_DMA_JZ4780) += dma-jz4780.o
 obj-$(CONFIG_DMA_SA11X0) += sa11x0-dma.o
 obj-$(CONFIG_DMA_SUN4I) += sun4i-dma.o
diff --git a/drivers/dma/cix-sky1-dma350.c b/drivers/dma/cix-sky1-dma350.c
new file mode 100644
index 000000000000..5904453f3481
--- /dev/null
+++ b/drivers/dma/cix-sky1-dma350.c
@@ -0,0 +1,206 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CIX Sky1 DMA-350 integration driver
+ */
+
+#include <linux/clk.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#define SKY1_DMA350_CRU_DMAC_AP_IRQ	0x54
+#define SKY1_DMA350_IRQ_ROUTE_MASK	0xff
+
+struct cix_sky1_dma350 {
+	struct clk_bulk_data *clks;
+	struct reset_control *reset;
+	struct regmap *irq_router;
+	struct device *rmem_dev;
+	int num_clks;
+};
+
+static int cix_sky1_dma350_route_irqs(struct device *dev)
+{
+	struct cix_sky1_dma350 *data = dev_get_drvdata(dev);
+
+	if (!data->irq_router)
+		return 0;
+
+	return regmap_update_bits(data->irq_router,
+				  SKY1_DMA350_CRU_DMAC_AP_IRQ,
+				  SKY1_DMA350_IRQ_ROUTE_MASK,
+				  SKY1_DMA350_IRQ_ROUTE_MASK);
+}
+
+static int cix_sky1_dma350_enable_resources(struct device *dev)
+{
+	struct cix_sky1_dma350 *data = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_bulk_prepare_enable(data->num_clks, data->clks);
+	if (ret)
+		return ret;
+
+	ret = reset_control_reset(data->reset);
+	if (ret)
+		goto err_disable_clks;
+
+	ret = cix_sky1_dma350_route_irqs(dev);
+	if (ret)
+		goto err_disable_clks;
+
+	return 0;
+
+err_disable_clks:
+	clk_bulk_disable_unprepare(data->num_clks, data->clks);
+	return ret;
+}
+
+static void cix_sky1_dma350_disable_resources(struct device *dev)
+{
+	struct cix_sky1_dma350 *data = dev_get_drvdata(dev);
+
+	reset_control_assert(data->reset);
+	clk_bulk_disable_unprepare(data->num_clks, data->clks);
+}
+
+static int cix_sky1_dma350_attach_reserved_mem(struct device *dev)
+{
+	struct cix_sky1_dma350 *data = dev_get_drvdata(dev);
+	struct platform_device *child_pdev;
+	struct device_node *child_np;
+	int ret;
+
+	if (!of_property_present(dev->of_node, "memory-region"))
+		return 0;
+
+	child_np = of_get_compatible_child(dev->of_node, "arm,dma-350");
+	if (!child_np)
+		return -ENODEV;
+
+	child_pdev = of_find_device_by_node(child_np);
+	of_node_put(child_np);
+	if (!child_pdev)
+		return -EPROBE_DEFER;
+
+	/*
+	 * Reserved memory is attached after the child has probed. This relies
+	 * on arm-dma350 not allocating coherent command buffers in probe;
+	 * those allocations happen per descriptor at prep time.
+	 */
+	ret = of_reserved_mem_device_init_by_idx(&child_pdev->dev,
+						 dev->of_node, 0);
+	if (ret) {
+		put_device(&child_pdev->dev);
+		return ret;
+	}
+
+	data->rmem_dev = &child_pdev->dev;
+
+	return 0;
+}
+
+static void cix_sky1_dma350_release_reserved_mem(struct device *dev)
+{
+	struct cix_sky1_dma350 *data = dev_get_drvdata(dev);
+
+	if (data->rmem_dev) {
+		of_reserved_mem_device_release(data->rmem_dev);
+		put_device(data->rmem_dev);
+		data->rmem_dev = NULL;
+	}
+}
+
+static int cix_sky1_dma350_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cix_sky1_dma350 *data;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, data);
+
+	data->num_clks = devm_clk_bulk_get_all(dev, &data->clks);
+	if (data->num_clks < 0)
+		return dev_err_probe(dev, data->num_clks,
+				     "failed to get clocks\n");
+
+	data->reset = devm_reset_control_get_optional_exclusive(dev, NULL);
+	if (IS_ERR(data->reset))
+		return dev_err_probe(dev, PTR_ERR(data->reset),
+				     "failed to get reset\n");
+
+	data->irq_router = syscon_regmap_lookup_by_phandle_optional(dev->of_node,
+								    "cix,irq-router");
+	if (IS_ERR(data->irq_router))
+		return dev_err_probe(dev, PTR_ERR(data->irq_router),
+				     "failed to get IRQ router\n");
+
+	ret = cix_sky1_dma350_enable_resources(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to enable resources\n");
+
+	ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
+	if (ret)
+		goto err_disable_resources;
+
+	ret = cix_sky1_dma350_attach_reserved_mem(dev);
+	if (ret)
+		goto err_depopulate;
+
+	return 0;
+
+err_depopulate:
+	of_platform_depopulate(dev);
+err_disable_resources:
+	cix_sky1_dma350_disable_resources(dev);
+	return dev_err_probe(dev, ret, "failed to initialize child devices\n");
+}
+
+static void cix_sky1_dma350_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	of_platform_depopulate(dev);
+	cix_sky1_dma350_release_reserved_mem(dev);
+	cix_sky1_dma350_disable_resources(dev);
+}
+
+static int __maybe_unused cix_sky1_dma350_resume_noirq(struct device *dev)
+{
+	return cix_sky1_dma350_route_irqs(dev);
+}
+
+static const struct dev_pm_ops cix_sky1_dma350_pm = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, cix_sky1_dma350_resume_noirq)
+};
+
+static const struct of_device_id cix_sky1_dma350_of_match[] = {
+	{ .compatible = "cix,sky1-dma350" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, cix_sky1_dma350_of_match);
+
+static struct platform_driver cix_sky1_dma350_driver = {
+	.probe = cix_sky1_dma350_probe,
+	.remove = cix_sky1_dma350_remove,
+	.driver = {
+		.name = "cix-sky1-dma350",
+		.of_match_table = cix_sky1_dma350_of_match,
+		.pm = pm_sleep_ptr(&cix_sky1_dma350_pm),
+	},
+};
+module_platform_driver(cix_sky1_dma350_driver);
+
+MODULE_AUTHOR("Jelly Jia <Jelly.Jia@cixtech.com>");
+MODULE_DESCRIPTION("CIX Sky1 DMA-350 integration driver");
+MODULE_LICENSE("GPL");
-- 
2.54.0

Re: [PATCH v1 4/5] dmaengine: cix-sky1-dma350: Add Sky1 integration driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Race condition allocating DMA buffers due to delayed reserved memory attachment.
- [High] Infinite probe deferral if the child 'arm,dma-350' device node is disabled.
- [Medium] Improper mixing of pulse and level reset semantics leads to ignored errors and potential power-off failures.
- [Medium] Leaking partially populated child platform devices on `of_platform_populate` error path.
--

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

dmaengine: cix-sky1-dma350: Add Sky1 integration driver

Add a CIX Sky1 integration driver for Arm DMA-350 instances. The driver
manages clocks, reset, optional interrupt routing, optional reserved
memory attachment, and populates the child Arm DMA-350 controller.

> diff --git a/drivers/dma/cix-sky1-dma350.c b/drivers/dma/cix-sky1-dma350.c
> new file mode 100644
> index 0000000000000..5904453f34818
> --- /dev/null
> +++ b/drivers/dma/cix-sky1-dma350.c

[ ... ]

> +static int cix_sky1_dma350_enable_resources(struct device *dev)
> +{
> +	struct cix_sky1_dma350 *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_bulk_prepare_enable(data->num_clks, data->clks);
> +	if (ret)
> +		return ret;
> +
> +	ret = reset_control_reset(data->reset);
> +	if (ret)
> +		goto err_disable_clks;

[ ... ]

> +static void cix_sky1_dma350_disable_resources(struct device *dev)
> +{
> +	struct cix_sky1_dma350 *data = dev_get_drvdata(dev);
> +
> +	reset_control_assert(data->reset);

[Severity: Medium]
Is it safe to mix pulse and level reset semantics here?

The enable path uses reset_control_reset() for a pulse, but the disable path
uses reset_control_assert() for a level assertion.

If the underlying reset controller only implements the .reset callback
(pulse), reset_control_assert() will return -ENOTSUPP. Because the return
value is ignored, won't this silently fail to reset the hardware during
unbind or system suspend, leaving it active?

> +	clk_bulk_disable_unprepare(data->num_clks, data->clks);
> +}
> +
> +static int cix_sky1_dma350_attach_reserved_mem(struct device *dev)
> +{
> +	struct cix_sky1_dma350 *data = dev_get_drvdata(dev);
> +	struct platform_device *child_pdev;
> +	struct device_node *child_np;
> +	int ret;
> +
> +	if (!of_property_present(dev->of_node, "memory-region"))
> +		return 0;
> +
> +	child_np = of_get_compatible_child(dev->of_node, "arm,dma-350");
> +	if (!child_np)
> +		return -ENODEV;
> +
> +	child_pdev = of_find_device_by_node(child_np);
> +	of_node_put(child_np);
> +	if (!child_pdev)
> +		return -EPROBE_DEFER;

[Severity: High]
Can this lead to an infinite probe deferral loop if the child device node is
disabled in the device tree?

of_get_compatible_child() does not check the node's availability, so it will
return the child node even if it has status = "disabled". Because
of_platform_populate() correctly skips disabled nodes, no device will be
instantiated.

As a result, of_find_device_by_node() will return NULL, causing the driver
to continually return -EPROBE_DEFER instead of ignoring the disabled device.

> +
> +	/*
> +	 * Reserved memory is attached after the child has probed. This relies
> +	 * on arm-dma350 not allocating coherent command buffers in probe;
> +	 * those allocations happen per descriptor at prep time.
> +	 */
> +	ret = of_reserved_mem_device_init_by_idx(&child_pdev->dev,
> +						 dev->of_node, 0);

[Severity: High]
Is there a race condition with allocating DMA buffers here?

of_platform_populate() in the probe function synchronously instantiates and
probes the child device, exposing it to the rest of the system.

Could a concurrent DMA client request a channel and prepare a transfer before
cix_sky1_dma350_attach_reserved_mem() executes? If that happens, allocations
might draw from the default system pool instead of the designated reserved
region, potentially causing hardware faults.

> +	if (ret) {
> +		put_device(&child_pdev->dev);
> +		return ret;
> +	}

[ ... ]

> +static int cix_sky1_dma350_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	ret = cix_sky1_dma350_enable_resources(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to enable resources\n");
> +
> +	ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
> +	if (ret)
> +		goto err_disable_resources;

[Severity: Medium]
Does this leak partially populated child platform devices?

If of_platform_populate() encounters an error while instantiating multiple
children, it breaks and returns an error without cleaning up the ones it
already created.

Bypassing err_depopulate here prevents cleanup of platform devices created
prior to the failure, permanently leaking them.

> +
> +	ret = cix_sky1_dma350_attach_reserved_mem(dev);
> +	if (ret)
> +		goto err_depopulate;
> +
> +	return 0;

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

[PATCH v1 5/5] arm64: dts: cix: Add Sky1 DMA-350 nodes

[Jelly Jia]

Add CIX Sky1 DMA-350 integration nodes with child Arm DMA-350 controllers.

The nodes describe the FCH and AUDSS DMA instances.

Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
---
 arch/arm64/boot/dts/cix/sky1.dtsi | 61 +++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
index 0a820f45feb9..7a5e463dc7c5 100644
--- a/arch/arm64/boot/dts/cix/sky1.dtsi
+++ b/arch/arm64/boot/dts/cix/sky1.dtsi
@@ -518,18 +518,29 @@ 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>;
+		cix_fch_dmac: dma@4190000 {
+			compatible = "cix,sky1-dma350";
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges = <0x0 0x0 0x0 0x4190000 0x0 0x10000>;
+			dma-ranges = <0x0 0x040a0000 0x0 0x040b0000 0x0 0x00040000>,
+				     <0x0 0x80000000 0x0 0x80000000 0x8 0x00000000>,
+				     <0x80 0x00000000 0x80 0x00000000 0x80 0x00000000>;
+			clocks = <&scmi_clk CLK_TREE_FCH_DMA_ACLK>;
+
+			fch_dmac: dma-controller@0 {
+				compatible = "arm,dma-350";
+				reg = <0x0 0x0 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>;
+			};
 		};
 
 		mbox_ap2se: mailbox@5060000 {
@@ -576,6 +587,32 @@ mbox_pm2ap: mailbox@65a0080 {
 			cix,mbox-dir = "rx";
 		};
 
+		cix_audss_dmac: dma@7010000 {
+			compatible = "cix,sky1-dma350";
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges = <0x0 0x0 0x0 0x07010000 0x0 0x10000>;
+			dma-ranges = <0x0 0x20000000 0x0 0x07010000 0x0 0x00100000>,
+				     <0x0 0x30000000 0x0 0xc0000000 0x0 0x20000000>;
+			clocks = <&audss_cru CLK_DMAC_AXI>;
+			resets = <&audss_cru AUDSS_DMAC_SW_RST>;
+			cix,irq-router = <&audss_cru>;
+
+			audss_dmac: dma-controller@0 {
+				compatible = "arm,dma-350";
+				reg = <0x0 0x0 0x0 0x10000>;
+				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

[PATCH v2 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 is new in v2: it 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 clocks, resets, interrupt
routing through a syscon phandle and an optional reserved memory pool
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 v1:
- The wrapper node is gone. The Sky1 DMA-350 is described by a single
  device node with "cix,sky1-dma350", "arm,dma-350" and its real MMIO
  region, carrying the clock, reset and interrupt routing that v1 kept
  on the wrapper; the separate cix,sky1-dma350 binding and the wrapper
  driver are dropped. The integration and the block are one device, and
  the MMIO is unchanged. Thanks to Krzysztof Kozlowski and Conor Dooley
  for the review.
- The FCH and AUDSS DMA-350 masters sit behind SoC interconnect windows
  in a static DMA address domain, so their DMA view differs from the CPU
  physical view; the windows are fixed before Linux boots, so Linux
  cannot manage them as an interconnect. They are described with a
  generic "simple-bus" address-domain parent carrying the window
  "dma-ranges", not with a CIX DMA wrapper device: the parent has no
  device-specific or vendor wrapper compatible, only the generic
  "simple-bus" compatible, and no reg, clocks, resets or driver of its
  own. The OF DMA core builds dev->dma_range_map from that parent
  property and the DMA API applies it; the driver has no private
  translation. Same shape as the mainline Agilex5 and Marvell AC5 buses;
  patch 6 explains the hardware in detail.
- Patch 2 now allocates the command list with kmalloc()/dma_map_single()
  instead of dma_alloc_coherent(), because it is freed from the virt-dma
  tasklet where dma_free_coherent() may not be called; it picks a free
  trigger-capable channel in .of_xlate(); and it reads the link address
  registers in a retry loop, since the channel can step the link pointer
  between the two reads.
- Patch 3 is new, see above.
- Patch 5 merged the Sky1 integration into arm-dma350.c; slave resources
  are mapped with dma_map_resource() and translated by the DMA API
  through the parent bus dma-ranges.

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: both the FCH and
  the AUDSS DMA-350 instances probe and transfer data, and audio
  playback through the AUDSS DMA-350 runs without underruns.

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: Add CIX Sky1 integration support
  arm64: dts: cix: Describe the Sky1 DMA-350 bus integration

 .../devicetree/bindings/dma/arm,dma-350.yaml  |  33 +-
 arch/arm64/boot/dts/cix/sky1.dtsi             |  66 +-
 drivers/dma/Kconfig                           |   4 +
 drivers/dma/arm-dma350.c                      | 717 +++++++++++++++++-
 4 files changed, 783 insertions(+), 37 deletions(-)


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.54.0

[PATCH v2 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

[PATCH v2 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
with a per-direction block size matching the configured maxburst.

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.

Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
---
v1 -> v2:
- Allocate the command list with kmalloc() and map it with
  dma_map_single(); it is freed from the virt-dma tasklet, where
  dma_free_coherent() may not be called.
- Pick a free trigger-capable channel in .of_xlate() instead of taking
  an arbitrary channel and failing when it has no trigger support.
- Pass an explicit "link" flag to the command builders instead of
  inferring it from the link address.
- Read the link address registers in a retry loop, since the channel
  can step the link pointer between the two reads.
- The parent bus dma-ranges translation moved to the CIX integration
  patch, which is its only user.
 drivers/dma/arm-dma350.c | 604 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 581 insertions(+), 23 deletions(-)

diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index 4e17130de6c8..bbcf4071c3e7 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,26 @@ struct d350_desc {
 	u32 command[16];
 	u16 xsize;
 	u16 xsizehi;
+	u32 *cmds;
+	dma_addr_t cmds_dma;	/* DMA API address of the command list */
+	dma_addr_t cmds_bus;	/* DMA350-visible address for CH_LINKADDR */
+	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 +230,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 +256,419 @@ 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->cmds_bus = desc->cmds_dma;
+	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_bus + (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_bus +
+				  ((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 +682,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 +721,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 +795,54 @@ 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;
+	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_bus + i * cmd_size) {
+				cmd = i - 1;
+				break;
+			}
+		}
+		if (i == desc->ncmds)
+			return dch->residue;
+	}
+
+	residue = d350_get_residue(dch);
+	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 +873,20 @@ 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_desc *desc)
+{
+	return desc->bytes - desc->period * desc->period_len;
+}
+
+static u32 d350_get_active_residue(struct d350_chan *dch)
+{
+	if (dch->desc->cyclic)
+		return d350_get_cyclic_residue(dch->desc);
+
+	return d350_get_sg_residue(dch);
 }
 
 static enum dma_status d350_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
@@ -387,7 +904,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 +986,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 +1019,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(desc);
+			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 +1063,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 +1107,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 +1171,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 +1188,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 +1202,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 v2 2/6] dmaengine: arm-dma350: Add slave and cyclic transfer support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] dma: inaccurate cyclic residue reporting omits hardware progress
- [High] dmaengine: arm-dma350: Missing DMA sync for command list after CPU writes
- [High] dmaengine: arm-dma350: TOCTOU race in SG residue calculation
- [Medium] dma: DEV_TO_MEM trigger block size ignores maxburst configuration
- [Medium] dmaengine: arm-dma350: false DMA_RESIDUE_GRANULARITY_BURST for cyclic transfers
--

commit 2323319884614c7159efde4dd0a7bd92ada52a37
Author: Jelly Jia <Jelly.Jia@cixtech.com>

dmaengine: arm-dma350: Add slave and cyclic transfer support

This commit adds DMA slave support to the Arm DMA-350 driver. It enables
scatter-gather and cyclic transfers, handles address translations and caching,
and reworks the completion logic for the new descriptor types.

> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index 4e17130de6c87..bbcf4071c3e75 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c

[ ... ]

> @@ -212,9 +256,419 @@ 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)
> +{
[ ... ]
> +static int d350_alloc_cmds(struct dma_chan *dchan, struct d350_desc *desc,
> +			   size_t ncmds)
> +{
[ ... ]
> +	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);

[Severity: High]
This code maps the command list memory for DMA (which performs a cache
clean) before any commands are actually written to it.

Later, in d350_prep_slave_sg() and d350_prep_dma_cyclic(), the CPU populates
desc->cmds:

> +static struct dma_async_tx_descriptor *
> +d350_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
[ ... ]
> +		if (sg_len > 1) {
> +			cmd = desc->cmds + i * D350_SLAVE_CMD_WORDS;
[ ... ]
> +		d350_fill_slave_cmd(dch, desc, cmd, mem, dev_dma_addr, len,
> +				    link, link_addr, direction, width, maxburst,
> +				    donetype);

Is a dma_sync_single_for_device() call missing after these writes?

Because the mapping and its associated cache flush occur before the CPU writes
the command data, the new commands may remain in the CPU cache. When the DMA
controller fetches subsequent commands from RAM, it could read stale memory.
I noticed this is intentionally fixed later in the patch series, but could
it be squashed here to prevent a broken intermediate state?

[ ... ]

> +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;
[ ... ]
> +	trigcfg = FIELD_PREP(CH_TRIGINCFG_BLKSIZE,
> +			     mem_to_dev ? maxburst - 1 : 0) |

[Severity: Medium]
For DEV_TO_MEM transfers (mem_to_dev == false), the block size is
unconditionally hardcoded to 0, completely ignoring the requested maxburst.

The commit message states that the trigger input is programmed with a
"per-direction block size matching the configured maxburst." Does this
mismatch between the peripheral's FIFO watermark and the DMA controller's
fetch size degrade performance or stall the transfer?

[ ... ]

> @@ -339,6 +795,54 @@ static u32 d350_get_residue(struct d350_chan *dch)
>  	return res << dch->desc->tsz;
>  }
>  
> +static u32 d350_get_sg_residue(struct d350_chan *dch)
> +{
[ ... ]
> +	/* 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_bus + i * cmd_size) {
> +				cmd = i - 1;
> +				break;
> +			}
> +		}
> +		if (i == desc->ncmds)
> +			return dch->residue;
> +	}
> +
> +	residue = d350_get_residue(dch);
> +	for (i = cmd + 1; i < desc->ncmds; i++)
> +		residue += desc->cmd_len[i];
> +
> +	return residue;
> +}

[Severity: High]
Could the DMA controller advance to the next command between the CH_LINKADDR
reads and the CH_XSIZE read in d350_get_residue()?

If the hardware advances, CH_XSIZE will reflect the new command's remaining
bytes, but the calculation here adds them to the previous command's length.
This would double-count the new command's bytes, causing the reported residue
to increase rather than remaining strictly monotonic decreasing.

Should the driver read CH_LINKADDR again after CH_XSIZE to verify it hasn't
changed?

[ ... ]

> @@ -369,7 +873,20 @@ 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_desc *desc)
> +{
> +	return desc->bytes - desc->period * desc->period_len;
> +}

[Severity: High]
Does this calculation omit the intra-period transferred bytes by entirely
ignoring the hardware transfer progress (CH_XSIZE)?

[Severity: Medium]
Because this relies solely on software period counters, it only provides
period-level granularity. The driver explicitly advertises
DMA_RESIDUE_GRANULARITY_BURST in d350_probe():

> @@ -568,23 +1107,32 @@ static int d350_probe(struct platform_device *pdev)
[ ... ]
>  	dmac->dma.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;

Does the omission of hardware progress cause audio pointer updates in ALSA
to jump in large discrete steps, leading to stuttering or underruns?

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

[PATCH v2 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.

Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
---
v1 -> v2:
- New patch: the command list is streaming-mapped, so it must be synced
  before the channel is started. Without it a non-coherent DMA-350
  reads stale command words and the transfer stalls after the first
  command.
 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 bbcf4071c3e7..e3fbfcce66e8 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -306,6 +306,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_bus, desc->cmds_size,
+				   DMA_TO_DEVICE);
+}
+
 static void d350_unmap_resource(struct d350_chan *dch)
 {
 	struct device *dev = dch->vc.chan.device->dev;
@@ -599,6 +614,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:
@@ -668,6 +685,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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dma: arm-dma350: API misuse passing cmds_bus instead of cmds_dma to DMA API
--

commit 2fca36adfda1fb6f57b81f4afd90229dd12fdb46
Author: Jelly Jia <Jelly.Jia@cixtech.com>

dmaengine: arm-dma350: Sync the slave command list before starting

This commit fixes a stalled transfer issue on non-coherent devices by
synchronizing the slave command list before starting the channel. It adds a
sync step to ensure written command words are visible to the hardware and
SWIOTLB bounce buffers are refreshed.

> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index bbcf4071c3e75..e3fbfcce66e8e 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c
> @@ -306,6 +306,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_bus, desc->cmds_size,
> +				   DMA_TO_DEVICE);

[Severity: Low]
Is desc->cmds_bus the correct address to pass to the DMA API here?

The DMA API requires the dma_addr_t handle returned by dma_map_single()
for all synchronization operations. It looks like the d350_desc struct
explicitly defines desc->cmds_dma for this purpose, while desc->cmds_bus is
designated as the hardware-visible address.

While both fields might currently hold identical values, passing the hardware
address to the DMA API could violate the API semantics. If bus address
translation or IOMMU offsets are introduced in the future where the hardware
view diverges from the DMA API token, could this result in SWIOTLB sync
failures or memory corruption?

Should this use desc->cmds_dma instead?

> +}
> +

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

[PATCH v2 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, a reset, an optional reserved memory region
used when the controller cannot reach all of system memory, and the
syscon that routes the channel interrupts to the AP interrupt
controller on Sky1. The cix,irq-router property is only valid together
with the CIX compatible.

Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
---
v1 -> v2:
- Folded the integration into arm,dma-350.yaml: the Sky1 DMA-350 is the
  same device, so it keeps the generic compatible plus a device
  specific "cix,sky1-dma350" compatible, and the integration resources
  are described on the controller node.
- Dropped the separate cix,sky1-dma350.yaml wrapper binding.
 .../devicetree/bindings/dma/arm,dma-350.yaml  | 33 ++++++++++++++++++-
 1 file changed, 32 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..3b991985c416 100644
--- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
+++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
@@ -11,10 +11,23 @@ maintainers:
 
 allOf:
   - $ref: dma-controller.yaml#
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              const: cix,sky1-dma350
+    then:
+      properties:
+        cix,irq-router: false
 
 properties:
   compatible:
-    const: arm,dma-350
+    oneOf:
+      - const: arm,dma-350
+      - items:
+          - const: cix,sky1-dma350
+          - const: arm,dma-350
 
   reg:
     items:
@@ -36,6 +49,24 @@ 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.
+
+  cix,irq-router:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Syscon phandle for the CIX Sky1 subsystem register block that routes
+      DMA-350 channel interrupts to the AP interrupt controller.
+
 required:
   - compatible
   - reg
-- 
2.54.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dt-bindings: missing conditionally required properties for CIX variant
--

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

dt-bindings: dma: arm,dma-350: Document the CIX Sky1 integration

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.

> diff --git a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> index 429f682f15d81..3b991985c416a 100644
> --- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> +++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> @@ -11,10 +11,23 @@ maintainers:
>  
>  allOf:
>    - $ref: dma-controller.yaml#
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            contains:
> +              const: cix,sky1-dma350
> +    then:
> +      properties:
> +        cix,irq-router: false

[Severity: Low]
Does the allOf block in Documentation/devicetree/bindings/dma/arm,dma-350.yaml
need an additional conditional block to enforce the required properties for the
cix,sky1-dma350 compatible?

The commit message explicitly states that the CIX integration needs a clock,
a reset, and the syscon (cix,irq-router), contrasting them with the
memory-region which is described as optional.

However, while the block above successfully forbids cix,irq-router for the
generic compatible, the schema fails to include an additional block that makes
the newly added integration resources (cix,irq-router, clocks, resets)
mandatory for the cix,sky1-dma350 variant.

Incomplete device tree nodes using the CIX compatible could silently pass
schema validation despite missing these hardware resources.

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

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

[Conor Dooley]

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

On Fri, Oct 09, 2026 at 01:18:44PM +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, a reset, an optional reserved memory region
> used when the controller cannot reach all of system memory, and the
> syscon that routes the channel interrupts to the AP interrupt
> controller on Sky1. The cix,irq-router property is only valid together
> with the CIX compatible.
> 
> Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
> Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
> ---
> v1 -> v2:
> - Folded the integration into arm,dma-350.yaml: the Sky1 DMA-350 is the
>   same device, so it keeps the generic compatible plus a device
>   specific "cix,sky1-dma350" compatible, and the integration resources
>   are described on the controller node.
> - Dropped the separate cix,sky1-dma350.yaml wrapper binding.
>  .../devicetree/bindings/dma/arm,dma-350.yaml  | 33 ++++++++++++++++++-
>  1 file changed, 32 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..3b991985c416 100644
> --- a/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> +++ b/Documentation/devicetree/bindings/dma/arm,dma-350.yaml
> @@ -11,10 +11,23 @@ maintainers:
>  
>  allOf:
>    - $ref: dma-controller.yaml#
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            contains:
> +              const: cix,sky1-dma350
> +    then:
> +      properties:
> +        cix,irq-router: false
>  
>  properties:
>    compatible:
> -    const: arm,dma-350
> +    oneOf:
> +      - const: arm,dma-350
> +      - items:
> +          - const: cix,sky1-dma350
> +          - const: arm,dma-350
>  
>    reg:
>      items:
> @@ -36,6 +49,24 @@ 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.
> +
> +  cix,irq-router:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Syscon phandle for the CIX Sky1 subsystem register block that routes
> +      DMA-350 channel interrupts to the AP interrupt controller.

See comments on the driver for more on this, but I'm not convinced that
this should be in this node - especially if this thing controls other
device's routing too.

Thanks,
Conor.

> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.54.0
> 

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

[PATCH v2 5/6] dmaengine: arm-dma350: Add CIX Sky1 integration support

[Jelly Jia]

The CIX Sky1 SoC integrates DMA-350 controllers whose DMA masters see
peripheral resources at different addresses than the CPU, and whose
channel interrupts are gated by a subsystem register block before they
reach the AP interrupt controller.

Add match data for the "cix,sky1-dma350" compatible, describing the
interrupt routing register, and use it to bring the controller up:
enable the clock, reset the controller, and route the channel
interrupts through the register block. Routing is redone after system
resume because that block may lose its state.

The address translation is described in the device tree, not in the
driver: a bus node wrapping the controller carries the dma-ranges of
the integration, the OF core builds dev->dma_range_map from them, and
the DMA API applies the translation to both buffers and resources, so
slave resources are mapped with dma_map_resource() like on any other
device. Integrations without such a bus node keep the identity mapping.

The controller may also be attached to a reserved memory region, for
integrations whose DMA master cannot reach all of system memory.

Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
---
v1 -> v2:
- Merged the Sky1 integration into arm-dma350.c: the SoC resources are
  managed by the DMA-350 driver itself, matched through the
  "cix,sky1-dma350" compatible.
- Slave resources are mapped with dma_map_resource() and translated by
  the standard DMA API through the dma-ranges of the parent bus node.
- Dropped drivers/dma/cix-sky1-dma350.c and its MAINTAINERS entry.
- ARM_DMA350 now selects MFD_SYSCON for the interrupt router syscon.
 drivers/dma/Kconfig      |  4 ++
 drivers/dma/arm-dma350.c | 92 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+)

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index ae6a682c9f76..efa6d280bb36 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -97,9 +97,13 @@ config ARM_DMA350
 	depends on ARM || ARM64 || COMPILE_TEST
 	select DMA_ENGINE
 	select DMA_VIRTUAL_CHANNELS
+	select MFD_SYSCON
 	help
 	  Enable support for the Arm DMA-350 controller.
 
+	  Some integrations need to poke a syscon to route the channel
+	  interrupts, so MFD_SYSCON is selected here.
+
 config AT_HDMAC
 	tristate "Atmel AHB DMA support"
 	depends on ARCH_AT91 || COMPILE_TEST
diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
index e3fbfcce66e8..e443a9c3189b 100644
--- a/drivers/dma/arm-dma350.c
+++ b/drivers/dma/arm-dma350.c
@@ -4,14 +4,21 @@
 
 #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/mfd/syscon.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/pm.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
 #include <linux/scatterlist.h>
 #include <linux/slab.h>
 
@@ -158,6 +165,9 @@
 
 #define D350_SLAVE_CMD_WORDS	14
 
+#define SKY1_DMA350_CRU_DMAC_AP_IRQ	0x54
+#define SKY1_DMA350_IRQ_ROUTE_MASK	0xff
+
 enum ch_ctrl_donetype {
 	CH_CTRL_DONETYPE_NONE = 0,
 	CH_CTRL_DONETYPE_CMD = 1,
@@ -222,6 +232,13 @@ struct d350_chan_map {
 	bool needs_unmap;
 };
 
+struct d350;
+
+struct d350_data {
+	u32 irq_route_reg;
+	u32 irq_route_mask;
+};
+
 struct d350_chan {
 	struct virt_dma_chan vc;
 	struct d350_desc *desc;
@@ -241,6 +258,8 @@ struct d350_chan {
 
 struct d350 {
 	struct dma_device dma;
+	const struct d350_data *data;
+	struct regmap *irq_router;
 	int nchan;
 	int nreq;
 	struct d350_chan channels[] __counted_by(nchan);
@@ -256,6 +275,16 @@ static inline struct d350_desc *to_d350_desc(struct virt_dma_desc *vd)
 	return container_of(vd, struct d350_desc, vd);
 }
 
+static int d350_route_irqs(struct d350 *dmac)
+{
+	if (!dmac->irq_router)
+		return 0;
+
+	return regmap_update_bits(dmac->irq_router, dmac->data->irq_route_reg,
+				  dmac->data->irq_route_mask,
+				  dmac->data->irq_route_mask);
+}
+
 static void d350_free_cmds(struct device *dev, struct d350_desc *desc)
 {
 	if (desc->cmds) {
@@ -1089,12 +1118,40 @@ static void d350_free_chan_resources(struct dma_chan *chan)
 static int d350_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	const struct d350_data *data = device_get_match_data(dev);
+	struct reset_control *reset;
+	struct regmap *irq_router = NULL;
+	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");
+
+	if (data && data->irq_route_mask &&
+	    of_property_present(dev->of_node, "cix,irq-router")) {
+		irq_router =
+			syscon_regmap_lookup_by_phandle(dev->of_node,
+							"cix,irq-router");
+		if (IS_ERR(irq_router))
+			return dev_err_probe(dev, PTR_ERR(irq_router),
+					     "failed to get IRQ router\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);
@@ -1118,8 +1175,14 @@ static int d350_probe(struct platform_device *pdev)
 	if (!dmac)
 		return -ENOMEM;
 
+	dmac->data = data;
+	dmac->irq_router = irq_router;
 	dmac->nchan = nchan;
 
+	ret = d350_route_irqs(dmac);
+	if (ret)
+		return ret;
+
 	reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG1);
 	dmac->nreq = FIELD_GET(DMA_CFG_NUM_TRIGGER_IN, reg);
 
@@ -1201,6 +1264,13 @@ 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);
@@ -1223,9 +1293,30 @@ 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 int __maybe_unused d350_resume_noirq(struct device *dev)
+{
+	struct d350 *dmac = dev_get_drvdata(dev);
+
+	if (!dmac)
+		return 0;
+
+	return d350_route_irqs(dmac);
 }
 
+static const struct dev_pm_ops d350_pm = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, d350_resume_noirq)
+};
+
+static const struct d350_data d350_sky1_data = {
+	.irq_route_reg = SKY1_DMA350_CRU_DMAC_AP_IRQ,
+	.irq_route_mask = SKY1_DMA350_IRQ_ROUTE_MASK,
+};
+
 static const struct of_device_id d350_of_match[] __maybe_unused = {
+	{ .compatible = "cix,sky1-dma350", .data = &d350_sky1_data },
 	{ .compatible = "arm,dma-350" },
 	{}
 };
@@ -1235,6 +1326,7 @@ static struct platform_driver d350_driver = {
 	.driver = {
 		.name = "arm-dma350",
 		.of_match_table = of_match_ptr(d350_of_match),
+		.pm = pm_sleep_ptr(&d350_pm),
 	},
 	.probe = d350_probe,
 	.remove = d350_remove,
-- 
2.54.0

Re: [PATCH v2 5/6] dmaengine: arm-dma350: Add CIX Sky1 integration support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] dmaengine: arm-dma350: Tasklet Use-After-Free on teardown
- [Medium] dmaengine: arm-dma350: leak of reserved memory on probe error paths
- [Medium] dmaengine: arm-dma350: leak of reserved memory assignment on probe error
- [Medium] dmaengine: arm-dma350: reserved memory leak in probe error paths
- [Low] kconfig: ARM_DMA350 selects MFD_SYSCON without HAS_IOMEM dependency
--

commit a78806dbf20ce04511d1cbf853c6105e7756c04b
Author: Jelly Jia <Jelly.Jia@cixtech.com>
dmaengine: arm-dma350: Add CIX Sky1 integration support

The CIX Sky1 SoC integrates DMA-350 controllers with custom peripheral
address mapping and interrupt routing. This commit adds device tree matching,
clock, reset, and syscon-based interrupt routing during probe and resume.
It also introduces support for attaching to a reserved memory region.

> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index ae6a682c9f769..efa6d280bb36e 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -97,9 +97,13 @@ config ARM_DMA350
>  	depends on ARM || ARM64 || COMPILE_TEST
>  	select DMA_ENGINE
>  	select DMA_VIRTUAL_CHANNELS
> +	select MFD_SYSCON

[Severity: Low]
Is this missing a dependency on HAS_IOMEM? MFD_SYSCON inherently depends on
HAS_IOMEM in drivers/mfd/Kconfig, so this unconditional select can cause
Kconfig unmet direct dependency warnings on architectures without it (e.g.
UML) during COMPILE_TEST. Since the driver uses MMIO functions, should it
depend on HAS_IOMEM?

[ ... ]

> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index e3fbfcce66e8e..e443a9c3189b2 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c

[ ... ]

> @@ -1201,6 +1264,13 @@ 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);

[Severity: Medium]
If dma_async_device_register() or the subsequent of_dma_controller_register()
fails here, the function returns directly. Does this code leak the reserved
memory assignment from of_reserved_mem_device_init() above? It seems
of_reserved_mem_device_release() is needed on these error paths.

[ ... ]

> @@ -1223,9 +1293,30 @@ 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);
>  }

[Severity: High]
Can a pending tasklet trigger a use-after-free here?

When d350_remove() is called, dma_async_device_unregister() unregisters the
device, but clients might have freed their channels via
d350_free_chan_resources() without killing the tasklet (since
vchan_free_chan_resources() does not call tasklet_kill()).

If a tasklet is pending, and d350_remove() returns, the dmac structure
will be freed via devres, but the tasklet might still execute and dereference
the freed virtual channel. Should tasklet_kill() be called during channel
resource cleanup to prevent this?

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

Re: [PATCH v2 5/6] dmaengine: arm-dma350: Add CIX Sky1 integration support

[Conor Dooley]

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

On Fri, Oct 09, 2026 at 01:18:45PM +0800, Jelly Jia wrote:
> The CIX Sky1 SoC integrates DMA-350 controllers whose DMA masters see
> peripheral resources at different addresses than the CPU, and whose
> channel interrupts are gated by a subsystem register block before they
> reach the AP interrupt controller.
> 
> Add match data for the "cix,sky1-dma350" compatible, describing the
> interrupt routing register, and use it to bring the controller up:
> enable the clock, reset the controller, and route the channel
> interrupts through the register block. Routing is redone after system
> resume because that block may lose its state.
> 
> The address translation is described in the device tree, not in the
> driver: a bus node wrapping the controller carries the dma-ranges of
> the integration, the OF core builds dev->dma_range_map from them, and
> the DMA API applies the translation to both buffers and resources, so
> slave resources are mapped with dma_map_resource() like on any other
> device. Integrations without such a bus node keep the identity mapping.
> 
> The controller may also be attached to a reserved memory region, for
> integrations whose DMA master cannot reach all of system memory.
> 
> Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
> Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
> ---
> v1 -> v2:
> - Merged the Sky1 integration into arm-dma350.c: the SoC resources are
>   managed by the DMA-350 driver itself, matched through the
>   "cix,sky1-dma350" compatible.
> - Slave resources are mapped with dma_map_resource() and translated by
>   the standard DMA API through the dma-ranges of the parent bus node.
> - Dropped drivers/dma/cix-sky1-dma350.c and its MAINTAINERS entry.
> - ARM_DMA350 now selects MFD_SYSCON for the interrupt router syscon.
>  drivers/dma/Kconfig      |  4 ++
>  drivers/dma/arm-dma350.c | 92 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 96 insertions(+)
> 
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index ae6a682c9f76..efa6d280bb36 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -97,9 +97,13 @@ config ARM_DMA350
>  	depends on ARM || ARM64 || COMPILE_TEST
>  	select DMA_ENGINE
>  	select DMA_VIRTUAL_CHANNELS
> +	select MFD_SYSCON
>  	help
>  	  Enable support for the Arm DMA-350 controller.
>  
> +	  Some integrations need to poke a syscon to route the channel
> +	  interrupts, so MFD_SYSCON is selected here.
> +
>  config AT_HDMAC
>  	tristate "Atmel AHB DMA support"
>  	depends on ARCH_AT91 || COMPILE_TEST
> diff --git a/drivers/dma/arm-dma350.c b/drivers/dma/arm-dma350.c
> index e3fbfcce66e8..e443a9c3189b 100644
> --- a/drivers/dma/arm-dma350.c
> +++ b/drivers/dma/arm-dma350.c
> @@ -4,14 +4,21 @@
>  
>  #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/mfd/syscon.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/pm.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
>  #include <linux/scatterlist.h>
>  #include <linux/slab.h>
>  
> @@ -158,6 +165,9 @@
>  
>  #define D350_SLAVE_CMD_WORDS	14
>  
> +#define SKY1_DMA350_CRU_DMAC_AP_IRQ	0x54
> +#define SKY1_DMA350_IRQ_ROUTE_MASK	0xff
> +
>  enum ch_ctrl_donetype {
>  	CH_CTRL_DONETYPE_NONE = 0,
>  	CH_CTRL_DONETYPE_CMD = 1,
> @@ -222,6 +232,13 @@ struct d350_chan_map {
>  	bool needs_unmap;
>  };
>  
> +struct d350;
> +
> +struct d350_data {
> +	u32 irq_route_reg;
> +	u32 irq_route_mask;

Since this is a fixed value, there's little point having it as part of
your match data. Ditto the reg value, for the same reason. The user is
called directly from your probe code, so you can use the defines
directly.

That said, should you actually be doing this from here? It feels like
a bit of a problem with abstractions where you're fiddling with another
device. Syscon stuff is okay when you're changing something internal to
your device's functionality or something "downstream" of your device -
between you and the output pins. But this is something "upstream" of our
device and I dunno if this is the right way to do it. If you need to set
some sort of interrupt routing, should you actually do something like
microchip,mpfs-irqmux or renesas,rzn1-gpioirqmux and preserve the
abstraction/separation between devices?

Cheers,
Conor.

> +};
> +
>  struct d350_chan {
>  	struct virt_dma_chan vc;
>  	struct d350_desc *desc;
> @@ -241,6 +258,8 @@ struct d350_chan {
>  
>  struct d350 {
>  	struct dma_device dma;
> +	const struct d350_data *data;
> +	struct regmap *irq_router;
>  	int nchan;
>  	int nreq;
>  	struct d350_chan channels[] __counted_by(nchan);
> @@ -256,6 +275,16 @@ static inline struct d350_desc *to_d350_desc(struct virt_dma_desc *vd)
>  	return container_of(vd, struct d350_desc, vd);
>  }
>  
> +static int d350_route_irqs(struct d350 *dmac)
> +{
> +	if (!dmac->irq_router)
> +		return 0;
> +
> +	return regmap_update_bits(dmac->irq_router, dmac->data->irq_route_reg,
> +				  dmac->data->irq_route_mask,
> +				  dmac->data->irq_route_mask);
> +}
> +
>  static void d350_free_cmds(struct device *dev, struct d350_desc *desc)
>  {
>  	if (desc->cmds) {
> @@ -1089,12 +1118,40 @@ static void d350_free_chan_resources(struct dma_chan *chan)
>  static int d350_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	const struct d350_data *data = device_get_match_data(dev);
> +	struct reset_control *reset;
> +	struct regmap *irq_router = NULL;
> +	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");
> +
> +	if (data && data->irq_route_mask &&
> +	    of_property_present(dev->of_node, "cix,irq-router")) {
> +		irq_router =
> +			syscon_regmap_lookup_by_phandle(dev->of_node,
> +							"cix,irq-router");
> +		if (IS_ERR(irq_router))
> +			return dev_err_probe(dev, PTR_ERR(irq_router),
> +					     "failed to get IRQ router\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);
> @@ -1118,8 +1175,14 @@ static int d350_probe(struct platform_device *pdev)
>  	if (!dmac)
>  		return -ENOMEM;
>  
> +	dmac->data = data;
> +	dmac->irq_router = irq_router;
>  	dmac->nchan = nchan;
>  
> +	ret = d350_route_irqs(dmac);
> +	if (ret)
> +		return ret;
> +
>  	reg = readl_relaxed(base + DMAINFO + DMA_BUILDCFG1);
>  	dmac->nreq = FIELD_GET(DMA_CFG_NUM_TRIGGER_IN, reg);
>  
> @@ -1201,6 +1264,13 @@ 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);
> @@ -1223,9 +1293,30 @@ 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 int __maybe_unused d350_resume_noirq(struct device *dev)
> +{
> +	struct d350 *dmac = dev_get_drvdata(dev);
> +
> +	if (!dmac)
> +		return 0;
> +
> +	return d350_route_irqs(dmac);
>  }
>  
> +static const struct dev_pm_ops d350_pm = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, d350_resume_noirq)
> +};
> +
> +static const struct d350_data d350_sky1_data = {
> +	.irq_route_reg = SKY1_DMA350_CRU_DMAC_AP_IRQ,
> +	.irq_route_mask = SKY1_DMA350_IRQ_ROUTE_MASK,
> +};
> +
>  static const struct of_device_id d350_of_match[] __maybe_unused = {
> +	{ .compatible = "cix,sky1-dma350", .data = &d350_sky1_data },
>  	{ .compatible = "arm,dma-350" },
>  	{}
>  };
> @@ -1235,6 +1326,7 @@ static struct platform_driver d350_driver = {
>  	.driver = {
>  		.name = "arm-dma350",
>  		.of_match_table = of_match_ptr(d350_of_match),
> +		.pm = pm_sleep_ptr(&d350_pm),
>  	},
>  	.probe = d350_probe,
>  	.remove = d350_remove,
> -- 
> 2.54.0
> 

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

[PATCH v2 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,
reset and the AUDSS CRU syscon that gates the channel interrupts before
they reach the AP interrupt controller. It has no consumer in the SoC
dtsi yet: the audio controllers that use it are not described upstream.

Assisted-by: LLM checkpatch sparse dt_binding_check dtbs_check
Signed-off-by: Jelly Jia <Jelly.Jia@cixtech.com>
---
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, interrupt router) 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.
- The same shape is used in mainline: 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).
 arch/arm64/boot/dts/cix/sky1.dtsi | 66 +++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
index 0a820f45feb9..e3bd6c0e383f 100644
--- a/arch/arm64/boot/dts/cix/sky1.dtsi
+++ b/arch/arm64/boot/dts/cix/sky1.dtsi
@@ -518,18 +518,29 @@ 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>;
+		bus@4190000 {
+			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 +587,37 @@ mbox_pm2ap: mailbox@65a0080 {
 			cix,mbox-dir = "rx";
 		};
 
+		bus@7010000 {
+			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>;
+				/*
+				 * The AUDSS CRU gates each DMAC channel interrupt
+				 * before it reaches the GIC. AUDSS_DMAC_INFO_AP_IRQ
+				 * is at CRU offset 0x54.
+				 */
+				cix,irq-router = <&audss_cru>;
+				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 v2 6/6] arm64: dts: cix: Describe the Sky1 DMA-350 bus integration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt_bindings: audss_cru syscon lacks 'syscon' compatible string, causing regmap lookup failure
- [Medium] dts: cix: invalid generic fallback compatible due to custom irq routing
- [Low] dt_bindings: bus node has unit address without reg or non-empty ranges, triggering dtc warning
--

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

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

This commit describes the integration of the FCH and AUDSS DMA-350 controllers
behind interconnect windows in the Sky1 devicetree. It introduces wrapper bus
nodes with identity ranges; and specific dma-ranges, and configures clocks,
interrupts, and reset lines for the DMA blocks.

> - The same shape is used in mainline: 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).

[Severity: Low]
Is this description accurate regarding mainline usage without a reg property?

The Marvell AC5 bus (bus@80500000) explicitly defines a reg property, and the
Agilex5 bus (dma-bus@10db0000) has a non-empty ranges property.

> diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> index 0a820f45feb96..e3bd6c0e383fb 100644
> --- a/arch/arm64/boot/dts/cix/sky1.dtsi
> +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
> @@ -518,18 +518,29 @@ iomuxc: pinctrl@4170000 {
[ ... ]
> +		bus@4190000 {
> +			compatible = "simple-bus";
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			ranges;

[Severity: Low]
Does this node trigger a dtc warning for having a unit address without a reg or
non-empty ranges property?

Compiling the devicetree will generate the warning "node has a unit name, but
no reg or ranges property" for the wrapper buses. The dtc compiler specifically
ignores empty ranges; properties (length 0) when evaluating the
unit_address_vs_reg check.

To fix this, the nodes must either omit the @4190000 suffix or add a dummy reg
property.

> +			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 {
[ ... ]
> @@ -576,6 +587,37 @@ mbox_pm2ap: mailbox@65a0080 {
[ ... ]
> +		bus@7010000 {
> +			compatible = "simple-bus";
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			ranges;

[Severity: Low]
Does this node also trigger a dtc warning for the same reason?  It uses a unit
address with an empty ranges property and no reg property.

> +			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: Medium]
Is the "arm,dma-350" generic fallback compatible valid here, given the custom
IRQ routing below?

Devicetree rules dictate that a fallback compatible must only be used if the
generic driver can fully drive the hardware without vendor extensions. A generic
driver binding to "arm,dma-350" will ignore cix,irq-router, fail to ungate the
interrupts, and thus the hardware will be non-functional (e.g., interrupt
timeouts).

The custom routing should either be modeled using a standard interrupt-parent /
interrupt-map so the IRQ subsystem handles it transparently, or the generic
fallback compatible must be removed.

> +				reg = <0x0 0x07010000 0x0 0x10000>;
> +				clocks = <&audss_cru CLK_DMAC_AXI>;
> +				resets = <&audss_cru AUDSS_DMAC_SW_RST>;
> +				/*
> +				 * The AUDSS CRU gates each DMAC channel interrupt
> +				 * before it reaches the GIC. AUDSS_DMAC_INFO_AP_IRQ
> +				 * is at CRU offset 0x54.
> +				 */
> +				cix,irq-router = <&audss_cru>;

[Severity: Medium]
Will this syscon lookup fail because the audss_cru node below lacks the
"syscon" compatible string?

The DMA controller references the CRU via cix,irq-router = <&audss_cru>;. The
DMA driver resolves this using syscon_regmap_lookup_by_phandle(), which relies
on syscon_node_to_regmap().

This core function only instantiates a regmap if
of_device_is_compatible(np, "syscon") is true. Occurs unconditionally on device
boot: the audss_dmac DMA controller will fail to probe, receiving -EPROBE_DEFER
indefinitely.

> +				interrupts = <GIC_SPI 230 IRQ_TYPE_LEVEL_HIGH 0>,
[ ... ]
> +		audss_cru: clock-controller@7110000 {
> +			compatible = "cix,sky1-audss-cru";

[Severity: Medium]
Can the "syscon" compatible string be added here?

Because audss_cru only has compatible = "cix,sky1-audss-cru"; and omits the
standard "syscon" fallback compatible, the regmap lookup will fail.

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