Re: [PATCH v3 2/2] dmaengine: ls2x-apb: new driver for the Loongson LS2X APB DMA controller

[Vinod Koul <vkoul@kernel.org>]

On 11-07-23, 20:19, Binbin Zhou wrote:
> The Loongson LS2X APB DMA controller is available on Loongson-2K chips.
> 
> It is a single-channel, configurable DMA controller IP core based on the
> AXI bus, whose main function is to integrate DMA functionality on a chip
> dedicated to carrying data between memory and peripherals in APB bus
> (e.g. nand).
> 
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> Signed-off-by: Yingkun Meng <mengyingkun@loongson.cn>
> ---
>  MAINTAINERS                |   1 +
>  drivers/dma/Kconfig        |  14 +
>  drivers/dma/Makefile       |   1 +
>  drivers/dma/ls2x-apb-dma.c | 684 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 700 insertions(+)
>  create mode 100644 drivers/dma/ls2x-apb-dma.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 60a411936ba7..709c2e9d5f5f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12248,6 +12248,7 @@ M:	Binbin Zhou <zhoubinbin@loongson.cn>
>  L:	dmaengine@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/dma/loongson,ls2x-apbdma.yaml
> +F:	drivers/dma/ls2x-apb-dma.c
>  
>  LOONGSON LS2X I2C DRIVER
>  M:	Binbin Zhou <zhoubinbin@loongson.cn>
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 644c188d6a11..9b41b59ba2b4 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -376,6 +376,20 @@ config LPC18XX_DMAMUX
>  	  Enable support for DMA on NXP LPC18xx/43xx platforms
>  	  with PL080 and multiplexed DMA request lines.
>  
> +config LS2X_APB_DMA
> +	tristate "Loongson LS2X APB DMA support"
> +	depends on LOONGARCH || COMPILE_TEST
> +	select DMA_ENGINE
> +	select DMA_VIRTUAL_CHANNELS
> +	help
> +	  Support for the Loongson LS2X APB DMA controller driver. The
> +	  DMA controller is having single DMA channel which can be
> +	  configured for different peripherals like audio, nand, sdio
> +	  etc which is in APB bus.
> +
> +	  This DMA controller transfers data from memory to peripheral fifo.
> +	  It does not support memory to memory data transfer.
> +
>  config MCF_EDMA
>  	tristate "Freescale eDMA engine support, ColdFire mcf5441x SoCs"
>  	depends on M5441x || COMPILE_TEST
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index a4fd1ce29510..9b28ddb1ea3b 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_INTEL_IOATDMA) += ioat/
>  obj-y += idxd/
>  obj-$(CONFIG_K3_DMA) += k3dma.o
>  obj-$(CONFIG_LPC18XX_DMAMUX) += lpc18xx-dmamux.o
> +obj-$(CONFIG_LS2X_APB_DMA) += ls2x-apb-dma.o
>  obj-$(CONFIG_MILBEAUT_HDMAC) += milbeaut-hdmac.o
>  obj-$(CONFIG_MILBEAUT_XDMAC) += milbeaut-xdmac.o
>  obj-$(CONFIG_MMP_PDMA) += mmp_pdma.o
> diff --git a/drivers/dma/ls2x-apb-dma.c b/drivers/dma/ls2x-apb-dma.c
> new file mode 100644
> index 000000000000..b3efe86e4330
> --- /dev/null
> +++ b/drivers/dma/ls2x-apb-dma.c
> @@ -0,0 +1,684 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Driver for the Loongson LS2X APB DMA Controller
> + *
> + * Copyright (C) 2017-2023 Loongson Corporation
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmapool.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>

drop this header, of.h should suffice

> +/*
> + * struct ls2x_dma_hw_desc - DMA HW descriptor
> + * @ndesc_addr: the next descriptor low address.
> + * @mem_addr: memory low address.
> + * @apb_addr: device buffer address.
> + * @len: length of a piece of carried content, in words.
> + * @step_len: length between two moved memory data blocks.
> + * @step_times: number of blocks to be carried in a single DMA operation.
> + * @cmd: descriptor command or state.
> + * @stats: DMA status.
> + * @high_ndesc_addr: the next descriptor high address.
> + * @high_mem_addr: memory high address.
> + * @reserved: reserved
> + */
> +struct ls2x_dma_hw_desc {
> +	u32 ndesc_addr;
> +	u32 mem_addr;
> +	u32 apb_addr;

why not use dma_addr_t for this?

> +static void ls2x_dma_start_transfer(struct ls2x_dma_chan *lchan)
> +{
> +	struct ls2x_dma_priv *priv = to_ldma_priv(lchan->vchan.chan.device);
> +	struct ls2x_dma_sg *ldma_sg;
> +	struct virt_dma_desc *vdesc;
> +	u64 val;
> +
> +	/* Get the next descriptor */
> +	vdesc = vchan_next_desc(&lchan->vchan);
> +	if (!vdesc) {
> +		lchan->desc = NULL;
> +		return;
> +	}
> +
> +	list_del(&vdesc->node);
> +	lchan->desc = to_ldma_desc(vdesc);
> +	ldma_sg = &lchan->desc->sg[0];
> +
> +	/* Start DMA */
> +	lo_hi_writeq(0, priv->regs + LDMA_ORDER_ERG);
> +	val = (ldma_sg->llp & ~LDMA_CONFIG_MASK) | LDMA_64BIT_EN | LDMA_START;
> +	lo_hi_writeq(val, priv->regs + LDMA_ORDER_ERG);
> +}
> +
> +static void ls2x_dma_fill_desc(struct ls2x_dma_chan *lchan, u32 i,
> +			       struct ls2x_dma_desc *desc)

pls align this one to precceding open brace (hint: checkpatch.pl
--strict would warn you about this)

> +{
> +	struct ls2x_dma_sg *ldma_sg = &desc->sg[i];
> +
> +	ldma_sg->hw->mem_addr = lower_32_bits(ldma_sg->phys);
> +	ldma_sg->hw->high_mem_addr = upper_32_bits(ldma_sg->phys);
> +	/* Word count register takes input in words */
> +	ldma_sg->hw->len = ldma_sg->len >> 2;
> +	ldma_sg->hw->step_len = 0;
> +	ldma_sg->hw->step_times = 1;
> +
> +	if (desc->direction == DMA_MEM_TO_DEV) {
> +		ldma_sg->hw->cmd = LDMA_INT | LDMA_DATA_DIRECTION;
> +		ldma_sg->hw->apb_addr = lchan->sconfig.dst_addr;
> +	} else {
> +		ldma_sg->hw->cmd = LDMA_INT;
> +		ldma_sg->hw->apb_addr = lchan->sconfig.src_addr;

only addr are used here, what about data width, why is that ignored?

> +	}
> +
> +	/* lets make a link list */
> +	if (i) {

what does i refer to here..?

> +/*
> + * of_ls2x_dma_xlate - Translation function
> + * @dma_spec: Pointer to DMA specifier as found in the device tree
> + * @ofdma: Pointer to DMA controller data
> + *
> + * Return: DMA channel pointer on success and NULL on error
> + */
> +static struct dma_chan *of_ls2x_dma_xlate(struct of_phandle_args *dma_spec,
> +					  struct of_dma *ofdma)
> +{
> +	struct ls2x_dma_priv *priv = ofdma->of_dma_data;
> +	struct ls2x_dma_chan *lchan;
> +
> +	/* We are single channel DMA, just get the channel from priv. */
> +	lchan = &priv->lchan;
> +	if (!lchan)
> +		return NULL;
> +
> +	return dma_get_slave_channel(&lchan->vchan.chan);
> +}

why not use generic xlate?
-- 
~Vinod