Re: [PATCH 3/5] drivers: dma: sh: Add DMAC driver for RZ/G2L SoC

[Vinod Koul <vkoul@kernel.org>]

On 11-06-21, 12:36, Biju Das wrote:
> Add DMA Controller driver for RZ/G2L SoC.
> 
> Based on the work done by Chris Brandt for RZ/A DMA driver.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/dma/sh/Kconfig   |    8 +
>  drivers/dma/sh/Makefile  |    1 +
>  drivers/dma/sh/rz-dmac.c | 1050 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1059 insertions(+)
>  create mode 100644 drivers/dma/sh/rz-dmac.c
> 
> diff --git a/drivers/dma/sh/Kconfig b/drivers/dma/sh/Kconfig
> index 13437323a85b..280a6d359e36 100644
> --- a/drivers/dma/sh/Kconfig
> +++ b/drivers/dma/sh/Kconfig
> @@ -47,3 +47,11 @@ config RENESAS_USB_DMAC
>  	help
>  	  This driver supports the USB-DMA controller found in the Renesas
>  	  SoCs.
> +
> +config RZ_DMAC
> +	tristate "Renesas RZ/G2L Controller"
> +	depends on ARCH_R9A07G044 || COMPILE_TEST
> +	select RENESAS_DMA
> +	help
> +	  This driver supports the general purpose DMA controller found in the
> +	  Renesas RZ/G2L SoC variants.
> diff --git a/drivers/dma/sh/Makefile b/drivers/dma/sh/Makefile
> index 112fbd22bb3f..9b2927f543bf 100644
> --- a/drivers/dma/sh/Makefile
> +++ b/drivers/dma/sh/Makefile
> @@ -15,3 +15,4 @@ obj-$(CONFIG_SH_DMAE) += shdma.o
>  
>  obj-$(CONFIG_RCAR_DMAC) += rcar-dmac.o
>  obj-$(CONFIG_RENESAS_USB_DMAC) += usb-dmac.o
> +obj-$(CONFIG_RZ_DMAC) += rz-dmac.o
> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> new file mode 100644
> index 000000000000..87a902ba3cfa
> --- /dev/null
> +++ b/drivers/dma/sh/rz-dmac.c
> @@ -0,0 +1,1050 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/G2L Controller Driver
> + *
> + * Based on imx-dma.c
> + *
> + * Copyright (C) 2021 Renesas Electronics Corp.
> + * Copyright 2010 Sascha Hauer, Pengutronix <s.hauer@pengutronix.de>
> + * Copyright 2012 Javier Martin, Vista Silicon <javier.martin@vista-silicon.com>
> + */
> +
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +#include <linux/interrupt.h>
> +#include <linux/list.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/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include "../dmaengine.h"
> +
> +struct rz_dmac_slave_config {
> +	u32 mid_rid;
> +	dma_addr_t addr;
> +	u32 chcfg;
> +};

why not use dma_slave_config()

> +
> +enum  rz_dmac_prep_type {
> +	RZ_DMAC_DESC_MEMCPY,
> +	RZ_DMAC_DESC_SLAVE_SG,
> +};
> +
> +struct rz_lmdesc {
> +	u32 header;
> +	u32 sa;
> +	u32 da;
> +	u32 tb;
> +	u32 chcfg;
> +	u32 chitvl;
> +	u32 chext;
> +	u32 nxla;
> +};
> +
> +struct rz_dmac_desc {
> +	struct list_head node;

what is this list node for?

> +	struct dma_async_tx_descriptor desc;
> +	enum dma_status status;
> +	dma_addr_t src;
> +	dma_addr_t dest;
> +	size_t len;
> +	enum dma_transfer_direction direction;
> +	enum rz_dmac_prep_type type;
> +	/* For memcpy */
> +	unsigned int config_port;
> +	unsigned int config_mem;
> +	/* For slave sg */
> +	struct scatterlist *sg;
> +	unsigned int sgcount;
> +};

why not use virt_dma_desc ?

> +
> +struct rz_dmac_channel {
> +	struct rz_dmac_engine *rzdma;
> +	unsigned int index;
> +	int irq;
> +
> +	spinlock_t lock;
> +	struct list_head ld_free;
> +	struct list_head ld_queue;
> +	struct list_head ld_active;

why not use virt_dma_chan() ?

> +
> +	int descs_allocated;
> +	enum dma_slave_buswidth word_size;
> +	dma_addr_t per_address;
> +	struct dma_chan chan;
> +	struct dma_async_tx_descriptor desc;
> +	enum dma_status status;

Both desc and chan need status?

> +	const struct rz_dmac_slave_config *slave;
> +	void __iomem *ch_base;
> +	void __iomem *ch_cmn_base;
> +
> +	struct {
> +		struct rz_lmdesc *base;
> +		struct rz_lmdesc *head;
> +		struct rz_lmdesc *tail;
> +		int valid;
> +		dma_addr_t base_dma;
> +	} lmdesc;
> +
> +	u32 chcfg;
> +	u32 chctrl;
> +
> +	struct {
> +		int issue;
> +		int prep_slave_sg;
> +	} stat;
> +};

I have glanced at the rest of the driver, looks mostly okay but please
move this to use virt_dma_chan and virt_dma_desc that would ease a lot
of code from driver

Thanks
-- 
~Vinod