Re: [PATCH v8 2/2] dmaengine: atcdmac300: Add driver for Andes ATCDMAC300 DMA controller

[Frank Li <Frank.li@oss.nxp.com>]

On Thu, Oct 01, 2026 at 09:45:03AM +0800, CL Wang wrote:
> This patch adds support for the Andes ATCDMAC300 DMA controller.
>
> The ATCDMAC300 is a memory-to-memory and peripheral DMA controller
> that provides scatter-gather, cyclic, and slave transfer capabilities.
>
> Signed-off-by: CL Wang <cl634@andestech.com>
>
> ---
>   Changes for v8:
>     - Fold in fourteen further correctness fixes; no interface or binding
>       change.  Five of them answer findings reported on v7; the other
>       three findings from that posting are explained in the reply to it
>       rather than changed.  The remaining nine come from a review of the
>       whole driver.
>
>       Descriptor lifetime:
>         - Track cyclic mode on the descriptor rather than on the channel.
>           A cyclic and a non-cyclic transfer on one channel could leave
>           the flag set for the wrong descriptor and dereference a NULL
>           callback.
>         - Drop the num_sg field, which was written but never read.
>       Suspend and resume:
>         - Restore stop_mask when suspend times out, instead of leaving
>           every channel stopped for good.
>         - Start transfers that were queued while suspended.
>       Driver model:
>         - Do not offer the controller for unbinding.  A client holding a
>           channel across an unbind would run against devres memory that
>           remove() has already released.
>       Register encoding:
>         - Reject a DMA request line the request-select field cannot hold,
>           rather than truncating it silently.
>         - Keep the burst field out of its reserved range, which the
>           controller reports as an error exception.
>         - Refuse a transfer width the width field cannot encode.
>       Slave configuration:
>         - Clear the stored configuration when a channel is allocated, so a
>           client cannot inherit the previous one's device address.
>         - Accept a configuration that describes only the direction it
>           uses.  Requiring all four of the width and burst fields to be
>           powers of two rejected any client that fills in one direction,
>           and the transfer then divided by a zero device width.
>         - Reject a length that is not a whole number of device-width
>           units, which either transfers fewer bytes than asked for or
>           programs a zero transfer size.
>       Probe:
>         - Use struct_size() for the channel array, which is already
>           declared with __counted_by().
>         - Check the configuration register read, and refuse a channel
>           count of zero rather than registering a controller with no
>           channels.
>       Cleanups:
>         - Return a bool from the channel-enabled helper rather than a
>           value that is negative on a failed register read; pass a
>           register value rather than a bool to regmap_update_bits(); use
>           dma_cookie_status() directly.
>
>   Changes for v7:
>     - Fold in twelve correctness fixes found by review and confirmed on an
>       FPGA; no interface or binding change.  Interrupt path:
>         - Acknowledge only the status bits that were actually read, and key
>           the acknowledge on channel ownership rather than on in-flight
>           state the handler itself clears.  The old mask could drop a
>           completion that arrived while it was being cleared, leaving the
>           source asserted until the core disabled the line ("nobody cared").
>         - Use raw spinlocks for the state shared with the hard IRQ handler.
>       Termination and descriptor lifetime:
>         - Do not run the callback twice, or recycle a descriptor, when a
>           transfer is terminated while its completion is in flight.
>         - Initialise desc_node when a descriptor is allocated.
>         - Wait for the channel to stop without holding the channel lock;
>           the poll could spin 100 ms with interrupts disabled.
>       Suspend and resume:
>         - Move the PM callbacks to the late phase so clients terminate
>           their transfers first, and name the channels that fail to idle.
>         - Drain in-flight completions across suspend and resume.
>         - Do not restart an idle cyclic channel on resume.
>       Probe:
>         - Register the DMA device only after the interrupt and cache
>           coherency setup, closing a window in which a client could obtain
>           a channel through dma_request_channel() on an unready controller.
>         - Do not read-modify-write write-only registers.
>         - Name the device in the probe-time reset error.
>
>   Changes for v6:
>     - Rebase onto dmaengine/next; no functional changes from v5
>     - Fold the MAINTAINERS entry into this patch instead of carrying it
>       as a separate patch
>
>   Changes for v5:
>     - Update copyright year to 2026
>     - Remove redundant headers (init.h, iopoll.h, mod_devicetable.h)
>     - Move atcdmac_init_iocp() before of_dma_controller_register() in probe
>     - Change builtin_platform_driver() to module_platform_driver()
>     - Implement .remove callback to support safe module unloading
>     - Update Kconfig entry from bool to tristate
>     - Add MODULE_AUTHOR, MODULE_DESCRIPTION, MODULE_LICENSE macros
>
>   Changes for v4:
>     - No changes from v3
>
>   Changes for v3:
>     - Remove "andestech,atcdmac300" from of_device_id
>     - Replace deprecated tasklet with threaded IRQ using
>       devm_request_threaded_irq() and IRQF_ONESHOT to handle bottom-half
>       processing.
>     - Update locking mechanism from spin_lock_bh() to spin_lock_irqsave()
>     - Minor cleanups and correctness fixes
>         - Initialize descriptor pointers (first = NULL) explicitly
>         - Add missing headers (err.h, iopoll.h, log2.h, sprintf.h)
>         - Remove unused code paths related to tasklets
>         - Use builtin_platform_driver() instead of module_platform_driver()
>         - Remove "select DMATEST" from Kconfig
> ---
>  MAINTAINERS              |    6 +
>  drivers/dma/Kconfig      |   11 +
>  drivers/dma/Makefile     |    1 +
>  drivers/dma/atcdmac300.c | 1759 ++++++++++++++++++++++++++++++++++++++
>  drivers/dma/atcdmac300.h |  298 +++++++
>  5 files changed, 2075 insertions(+)
>  create mode 100644 drivers/dma/atcdmac300.c
>  create mode 100644 drivers/dma/atcdmac300.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c31b81eb42d7..2c48bb3053b3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1974,6 +1974,12 @@ S:	Supported
>  F:	drivers/clk/analogbits/*
>  F:	include/linux/clk/analogbits*
>
> +ANDES ATCDMAC300 DMA DRIVER
> +M:	CL Wang <cl634@andestech.com>
> +S:	Supported
> +F:	Documentation/devicetree/bindings/dma/andestech,ae350-dma.yaml
> +F:	drivers/dma/atcdmac300*
> +
>  ANDES ATCSPI200 SPI DRIVER
>  M:	CL Wang <cl634@andestech.com>
>  S:	Supported
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index ae6a682c9f76..dabd2c8cc94f 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -100,6 +100,17 @@ config ARM_DMA350
>  	help
>  	  Enable support for the Arm DMA-350 controller.
>
> +config ATCDMAC300
> +	tristate "Andes ATCDMAC300 DMA support"
> +	depends on ARCH_ANDES

|| COMPILE_TEST

make sure it can build

> +	depends on OF
> +	select DMA_ENGINE
> +	help
> +	  Enable support for the Andes ATCDMAC300 DMA controller.
> +	  Select Y or M if your platform includes an ATCDMAC300 device that
> +	  requires DMA engine support. This driver supports DMA_SLAVE,
> +	  DMA_MEMCPY, and DMA_CYCLIC transfer modes.
> +
>  config AT_HDMAC
>  	tristate "Atmel AHB DMA support"
>  	depends on ARCH_AT91 || COMPILE_TEST
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index 14aa086629d5..c8fffb31efc4 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_AMBA_PL08X) += amba-pl08x.o
>  obj-$(CONFIG_AMCC_PPC440SPE_ADMA) += ppc4xx/
>  obj-$(CONFIG_APPLE_ADMAC) += apple-admac.o
>  obj-$(CONFIG_ARM_DMA350) += arm-dma350.o
> +obj-$(CONFIG_ATCDMAC300) += atcdmac300.o
>  obj-$(CONFIG_AT_HDMAC) += at_hdmac.o
>  obj-$(CONFIG_AT_XDMAC) += at_xdmac.o
>  obj-$(CONFIG_AXI_DMAC) += dma-axi-dmac.o
> diff --git a/drivers/dma/atcdmac300.c b/drivers/dma/atcdmac300.c
> new file mode 100644
> index 000000000000..b67675647b36
> --- /dev/null
> +++ b/drivers/dma/atcdmac300.c
> @@ -0,0 +1,1759 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Andes ATCDMAC300 controller driver
> + *
> + * Copyright (C) 2026 Andes Technology Corporation
> + */
> +#include <linux/align.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dmapool.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/log2.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_dma.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/slab.h>
> +#include <linux/sprintf.h>
> +#include <linux/regmap.h>

Add empty line here.

> +#include "dmaengine.h"
> +#include "atcdmac300.h"
> +
> +static bool atcdmac_is_chan_enabled(struct atcdmac_chan *dmac_chan)
> +{
> +	struct atcdmac_dmac *dmac =
> +		atcdmac_dev_to_dmac(dmac_chan->dma_chan.device);
> +
> +	/*
> +	 * regmap_test_bits() reports a failed read as a negative errno,
> +	 * which every caller here would otherwise read as "enabled".
> +	 */
> +	return regmap_test_bits(dmac->regmap,
> +				REG_CH_EN,
> +				BIT(dmac_chan->chan_id)) > 0;
> +}
> +
> +static void atcdmac_enable_chan(struct atcdmac_chan *dmac_chan, bool enable)
> +{
> +	regmap_update_bits(dmac_chan->regmap, REG_CH_CTL_OFF, CHEN,
> +			   enable ? CHEN : 0);

Use regmap_assign_bits() will be more simple

> +}
> +
> +static void atcdmac_abort_chan(struct atcdmac_chan *dmac_chan)
> +{
> +	/*
> +	 * ChAbort is write-only: write 1 to the channel's bit to abort it.
> +	 * Do not read-modify-write, the read value is undefined and writing
> +	 * it back could abort a channel owned by someone else.
> +	 */
> +	regmap_write(dmac_chan->dma_dev->regmap,
> +		     REG_CH_ABT,
> +		     BIT(dmac_chan->chan_id));
> +}
> +
> +static dma_cookie_t atcdmac_tx_submit(struct dma_async_tx_descriptor *tx)
> +{
> +	struct atcdmac_chan *dmac_chan = atcdmac_chan_to_dmac_chan(tx->chan);
> +	struct atcdmac_desc *desc = atcdmac_txd_to_dma_desc(tx);
> +	dma_cookie_t cookie;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dmac_chan->lock, flags);
> +	cookie = dma_cookie_assign(tx);
> +	list_add_tail(&desc->desc_node, &dmac_chan->queue_list);
> +	spin_unlock_irqrestore(&dmac_chan->lock, flags);
> +
> +	return cookie;
> +}
> +
> +static struct atcdmac_desc *
> +atcdmac_get_active_head(struct atcdmac_chan *dmac_chan)
> +{
> +	return list_first_entry(&dmac_chan->active_list,
> +				struct atcdmac_desc,
> +				desc_node);
> +}
> +
> +static struct atcdmac_desc *atcdmac_alloc_desc(struct dma_chan *chan,
> +					       gfp_t gfp_flags)
> +{
> +	struct atcdmac_dmac *dmac = atcdmac_dev_to_dmac(chan->device);
> +	struct atcdmac_desc *desc;
> +	dma_addr_t phys;
> +
> +	desc = dma_pool_zalloc(dmac->dma_desc_pool, gfp_flags, &phys);
> +	if (desc) {
> +		INIT_LIST_HEAD(&desc->desc_node);
> +		INIT_LIST_HEAD(&desc->tx_list);
> +		dma_async_tx_descriptor_init(&desc->txd, chan);
> +		desc->txd.flags = DMA_CTRL_ACK;
> +		desc->txd.tx_submit = atcdmac_tx_submit;
> +		desc->txd.phys = phys;
> +	}
> +
> +	return desc;
> +}
> +
> +static struct atcdmac_desc *atcdmac_get_desc(struct atcdmac_chan *dmac_chan)
> +{
> +	struct atcdmac_desc *ret = NULL;
> +	struct atcdmac_desc *desc_next;
> +	struct atcdmac_desc *desc;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dmac_chan->lock, flags);
> +	list_for_each_entry_safe(desc, desc_next,
> +				 &dmac_chan->free_list,
> +				 desc_node) {
> +		if (async_tx_test_ack(&desc->txd)) {
> +			list_del_init(&desc->desc_node);
> +			ret = desc;
> +			break;
> +		}
> +	}
> +	spin_unlock_irqrestore(&dmac_chan->lock, flags);

Not sure why need manually manage free_list. Most likely dma_pool_zalloc()
is quick enough.

And why not use virt-dma?

Frank