Hi Vinod,
Thanks for the feedback.
> Subject: Re: [PATCH 3/5] drivers: dma: sh: Add DMAC driver for RZ/G2L SoC
>
> 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@xxxxxxxxxxxxxx>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxx>
> > + * Copyright 2012 Javier Martin, Vista Silicon
> > +<javier.martin@xxxxxxxxxxxxxxxxx> */
> > +
> > +#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()
OK, will use dma_slav_config and remove this structure.
>
> > +
> > +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?
This node is for managing list descriptors.
>
> > + 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 ?
OK. Will check with 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() ?
OK. Will check with 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?
This status is unused. Will take it out.
>
> > + 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
OK, will check with virt_dma_chan and virt_dma_desc.
Regards,
Biju
