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

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

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()

Commit message could be reworded a bit:

"struct dw_edma_chip" contains an internal structure "struct dw_edma" that is
used by the eDMA core internally. This structure should not be touched
by the eDMA controller drivers themselves. But currently, the eDMA
controller drivers like "dw-edma-pci" allocates and populates this
internal structure then passes it on to eDMA core. The eDMA core further
populates the structure and uses it. This is wrong!

Hence, move all the "struct dw_edma" specifics from controller drivers
to the eDMA core.

> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  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;
> +

The eDMA core should be able to reuse the existing fields from "dw_edma_chip".
I don't think the duplication is needed here.

> +	dw->wr_ch_cnt = min_t(u16, chip->wr_ch_cnt,
>  			      dw_edma_v0_core_ch_count(dw, EDMA_DIR_WRITE));

Eventhough this is an old code, I don't get the logic. Can the controller driver
pass channel count that is beyond the range hardware actually supports?

I think we should let the eDMA core to read the registers and populate
these fields instead of relying on the controller drivers.

> -	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;
>  

As I said above, can we just reuse the existing fields from "dw_edma_chip"?

Thanks,
Mani