Re: [PATCH 1/5] dmaengine: dw-edma: fix dw_edma_probe() can't be call globally

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

Hi Frank,

+ Dmitry

On Tue, Mar 01, 2022 at 09:26:42PM -0600, Frank Li wrote:
> API dw_edma_probe(struct dw_edma_chip *chip) defined at include/linux/dma,
> but struct dw_edma_chip have a hidden internal struct dw_edma *dw,
> supposed dw should be allocated in API dw_edma_probe().
> 
> 	@dw: struct dw_edma that is filed by dw_edma_probe()
> 
> but dw need allocate and fill chip related information  before call dw_edma_probe()
> in current code. See ref
> 	drivers/dma/dw-edma/dw-edma-pci.c
> 
> Move chip related information from dw-edma-core.h to edma.h
> allocate memory inside dw_edma_probe()
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

I started reviewing your eDMA patches because me and my colleague Dmitry are
also working on the eDMA support for Qualcomm platforms. By looking at the eDMA
driver in mainline, it looks like there is a bit of cleanup required. So for
avoiding the duplication of efforts, let's work together for fixing the eDMA
driver and add support for respective platforms.

Please CC us for all of your future patches as well. We will also share the
patches so that all can be combined into a single series.

Thanks,
Mani

> ---
>  drivers/dma/dw-edma/dw-edma-core.c       | 31 ++++++++++++---
>  drivers/dma/dw-edma/dw-edma-core.h       | 28 +++-----------
>  drivers/dma/dw-edma/dw-edma-v0-core.c    |  2 +-
>  drivers/dma/dw-edma/dw-edma-v0-debugfs.c |  2 +-
>  include/linux/dma/edma.h                 | 48 +++++++++++++++++++++++-
>  5 files changed, 78 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 53289927dd0d6..029085c035067 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -905,19 +905,32 @@ int dw_edma_probe(struct dw_edma_chip *chip)
>  	if (!dev)
>  		return -EINVAL;
>  
> -	dw = chip->dw;
> -	if (!dw || !dw->irq || !dw->ops || !dw->ops->irq_vector)
> +	dw = devm_kzalloc(dev, sizeof(*dw), GFP_KERNEL);
> +	if (!dw)
> +		return -ENOMEM;
> +
> +	chip->dw = dw;
> +
> +	if (!chip->nr_irqs || !chip->ops)
>  		return -EINVAL;
>  
>  	raw_spin_lock_init(&dw->lock);
>  
> -	dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt,
> +	dw->rg_region = &chip->rg_region;
> +	dw->ll_region_wr = chip->ll_region_wr;
> +	dw->ll_region_rd = chip->ll_region_rd;
> +	dw->dt_region_wr = chip->dt_region_wr;
> +	dw->dt_region_rd = chip->dt_region_rd;
> +
> +	dw->mf = chip->mf;
> +
> +	dw->wr_ch_cnt = min_t(u16, chip->wr_ch_cnt,
>  			      dw_edma_v0_core_ch_count(dw, EDMA_DIR_WRITE));
> -	dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt, EDMA_MAX_WR_CH);
> +	dw->wr_ch_cnt = min_t(u16, chip->wr_ch_cnt, EDMA_MAX_WR_CH);
>  
> -	dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt,
> +	dw->rd_ch_cnt = min_t(u16, chip->rd_ch_cnt,
>  			      dw_edma_v0_core_ch_count(dw, EDMA_DIR_READ));
> -	dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt, EDMA_MAX_RD_CH);
> +	dw->rd_ch_cnt = min_t(u16, chip->rd_ch_cnt, EDMA_MAX_RD_CH);
>  
>  	if (!dw->wr_ch_cnt && !dw->rd_ch_cnt)
>  		return -EINVAL;
> @@ -936,6 +949,12 @@ int dw_edma_probe(struct dw_edma_chip *chip)
>  	/* Disable eDMA, only to establish the ideal initial conditions */
>  	dw_edma_v0_core_off(dw);
>  
> +	dw->nr_irqs = chip->nr_irqs;
> +	dw->ops = chip->ops;
> +	dw->irq = devm_kcalloc(dev, dw->nr_irqs, sizeof(*dw->irq), GFP_KERNEL);
> +	if (!dw->irq)
> +		return -ENOMEM;
> +
>  	/* Request IRQs */
>  	err = dw_edma_irq_request(chip, &wr_alloc, &rd_alloc);
>  	if (err)
> diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h
> index 60316d408c3e0..8ca195814a878 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.h
> +++ b/drivers/dma/dw-edma/dw-edma-core.h
> @@ -15,20 +15,12 @@
>  #include "../virt-dma.h"
>  
>  #define EDMA_LL_SZ					24
> -#define EDMA_MAX_WR_CH					8
> -#define EDMA_MAX_RD_CH					8
>  
>  enum dw_edma_dir {
>  	EDMA_DIR_WRITE = 0,
>  	EDMA_DIR_READ
>  };
>  
> -enum dw_edma_map_format {
> -	EDMA_MF_EDMA_LEGACY = 0x0,
> -	EDMA_MF_EDMA_UNROLL = 0x1,
> -	EDMA_MF_HDMA_COMPAT = 0x5
> -};
> -
>  enum dw_edma_request {
>  	EDMA_REQ_NONE = 0,
>  	EDMA_REQ_STOP,
> @@ -57,12 +49,6 @@ struct dw_edma_burst {
>  	u32				sz;
>  };
>  
> -struct dw_edma_region {
> -	phys_addr_t			paddr;
> -	void				__iomem *vaddr;
> -	size_t				sz;
> -};
> -
>  struct dw_edma_chunk {
>  	struct list_head		list;
>  	struct dw_edma_chan		*chan;
> @@ -109,10 +95,6 @@ struct dw_edma_irq {
>  	struct dw_edma			*dw;
>  };
>  
> -struct dw_edma_core_ops {
> -	int	(*irq_vector)(struct device *dev, unsigned int nr);
> -};
> -
>  struct dw_edma {
>  	char				name[20];
>  
> @@ -122,11 +104,11 @@ struct dw_edma {
>  	struct dma_device		rd_edma;
>  	u16				rd_ch_cnt;
>  
> -	struct dw_edma_region		rg_region;	/* Registers */
> -	struct dw_edma_region		ll_region_wr[EDMA_MAX_WR_CH];
> -	struct dw_edma_region		ll_region_rd[EDMA_MAX_RD_CH];
> -	struct dw_edma_region		dt_region_wr[EDMA_MAX_WR_CH];
> -	struct dw_edma_region		dt_region_rd[EDMA_MAX_RD_CH];
> +	struct dw_edma_region		*rg_region;	/* Registers */
> +	struct dw_edma_region		*ll_region_wr;
> +	struct dw_edma_region		*ll_region_rd;
> +	struct dw_edma_region		*dt_region_wr;
> +	struct dw_edma_region		*dt_region_rd;
>  
>  	struct dw_edma_irq		*irq;
>  	int				nr_irqs;
> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> index 329fc2e57b703..884ba55fbd530 100644
> --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> @@ -25,7 +25,7 @@ enum dw_edma_control {
>  
>  static inline struct dw_edma_v0_regs __iomem *__dw_regs(struct dw_edma *dw)
>  {
> -	return dw->rg_region.vaddr;
> +	return dw->rg_region->vaddr;
>  }
>  
>  #define SET_32(dw, name, value)				\
> diff --git a/drivers/dma/dw-edma/dw-edma-v0-debugfs.c b/drivers/dma/dw-edma/dw-edma-v0-debugfs.c
> index 4b3bcffd15ef1..a42047791e727 100644
> --- a/drivers/dma/dw-edma/dw-edma-v0-debugfs.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-debugfs.c
> @@ -288,7 +288,7 @@ void dw_edma_v0_debugfs_on(struct dw_edma_chip *chip)
>  	if (!dw)
>  		return;
>  
> -	regs = dw->rg_region.vaddr;
> +	regs = dw->rg_region->vaddr;
>  	if (!regs)
>  		return;
>  
> diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
> index cab6e18773dad..fdb19c717aa09 100644
> --- a/include/linux/dma/edma.h
> +++ b/include/linux/dma/edma.h
> @@ -12,19 +12,63 @@
>  #include <linux/device.h>
>  #include <linux/dmaengine.h>
>  
> +#define EDMA_MAX_WR_CH                                  8
> +#define EDMA_MAX_RD_CH                                  8
> +
>  struct dw_edma;
>  
> +struct dw_edma_region {
> +	phys_addr_t	paddr;
> +	void __iomem	*vaddr;
> +	size_t		sz;
> +};
> +
> +struct dw_edma_core_ops {
> +	int (*irq_vector)(struct device *dev, unsigned int nr);
> +};
> +
> +enum dw_edma_map_format {
> +	EDMA_MF_EDMA_LEGACY = 0x0,
> +	EDMA_MF_EDMA_UNROLL = 0x1,
> +	EDMA_MF_HDMA_COMPAT = 0x5
> +};
> +
>  /**
>   * struct dw_edma_chip - representation of DesignWare eDMA controller hardware
>   * @dev:		 struct device of the eDMA controller
>   * @id:			 instance ID
> - * @irq:		 irq line
> + * @nr_irqs:		 total dma irq number
> + * reg64bit		 if support 64bit write to register
> + * @ops			 DMA channel to IRQ number mapping
> + * @wr_ch_cnt		 DMA write channel number
> + * @rd_ch_cnt		 DMA read channel number
> + * @rg_region		 DMA register region
> + * @ll_region_wr	 DMA descriptor link list memory for write channel
> + * @ll_region_rd	 DMA descriptor link list memory for read channel
> + * @mf			 DMA register map format
>   * @dw:			 struct dw_edma that is filed by dw_edma_probe()
>   */
>  struct dw_edma_chip {
>  	struct device		*dev;
>  	int			id;
> -	int			irq;
> +	int			nr_irqs;
> +	const struct dw_edma_core_ops   *ops;
> +
> +	u16			wr_ch_cnt;
> +	u16			rd_ch_cnt;
> +
> +	struct dw_edma_region	rg_region;      /* Registers */
> +
> +	/* link list address */
> +	struct dw_edma_region	ll_region_wr[EDMA_MAX_WR_CH];
> +	struct dw_edma_region	ll_region_rd[EDMA_MAX_RD_CH];
> +
> +	/* data region */
> +	struct dw_edma_region	dt_region_wr[EDMA_MAX_WR_CH];
> +	struct dw_edma_region	dt_region_rd[EDMA_MAX_RD_CH];
> +
> +	enum dw_edma_map_format	mf;
> +
>  	struct dw_edma		*dw;
>  };
>  
> -- 
> 2.24.0.rc1
>