Re: [PATCH v2 3/5] dmaengine: dw-edma: add flags at struct dw_edma_chip

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Mar 03, 2022 at 12:46:33PM -0600, Frank Li wrote:
> Allow user don't enable remote MSI irq.
> PCI ep probe dma locally, don't want to raise irq
> to remote PCI host.
> 
> Add option allow force 32bit register access even at
> 64bit system. i.MX8 hardware only allowed 32bit register
> access.
> 
> Add option allow EP side probe dma. remote side dma is
> continue physical memory, local memory is scatter list.

s/Allow user don't enable remote MSI/Allow user to prevent use of remote MSI/ ?
s/irq/IRQ/ (several, and in comment below)
s/ep/EP/ (capitalize consistently)
s/dma/DMA/ (several)
s/Add option allow force 32bit/Add option to force 32-bit/
s/even at 64bit system/even on 64-bit systems/
s/remote side/Remote side/ (start sentence with capital letter)

s/Add option allow EP side probe dma/Add option to allow EP to probe DMA/
(I'm not quite sure what this means).

s/continue physical memory/contiguous physical memory/ ?

Rewrap above to fill 75 columns.

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Change from v1 to v2
> - none
> 
>  drivers/dma/dw-edma/dw-edma-core.c    |  7 ++++++-
>  drivers/dma/dw-edma/dw-edma-v0-core.c | 14 ++++++++++----
>  include/linux/dma/edma.h              |  7 +++++++
>  3 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 0cb66434f9e14..a43bb26c8bf96 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -336,6 +336,7 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer)
>  	struct dw_edma_desc *desc;
>  	u32 cnt = 0;
>  	int i;
> +	bool b;
>  
>  	if (!chan->configured)
>  		return NULL;
> @@ -424,7 +425,11 @@ dw_edma_device_transfer(struct dw_edma_transfer *xfer)
>  		chunk->ll_region.sz += burst->sz;
>  		desc->alloc_sz += burst->sz;
>  
> -		if (chan->dir == EDMA_DIR_WRITE) {
> +		b = (chan->dir == EDMA_DIR_WRITE);
> +		if (chan->chip->flags & DW_EDMA_CHIP_LOCAL_EP)
> +			b = !b;
> +
> +		if (b) {
>  			burst->sar = src_addr;
>  			if (xfer->type == EDMA_XFER_CYCLIC) {
>  				burst->dar = xfer->xfer.cyclic.paddr;
> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> index 6e2f83e31a03a..d5c2415e2c616 100644
> --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> @@ -307,13 +307,18 @@ u32 dw_edma_v0_core_status_abort_int(struct dw_edma_chip *chip, enum dw_edma_dir
>  static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
>  {
>  	struct dw_edma_burst *child;
> +	struct dw_edma_chan *chan = chunk->chan;
>  	struct dw_edma_v0_lli __iomem *lli;
>  	struct dw_edma_v0_llp __iomem *llp;
>  	u32 control = 0, i = 0;
> +	u32 rie = 0;
>  	int j;
>  
>  	lli = chunk->ll_region.vaddr;
>  
> +	if (!(chan->chip->flags & DW_EDMA_CHIP_NO_MSI))
> +		rie = DW_EDMA_V0_RIE;
> +
>  	if (chunk->cb)
>  		control = DW_EDMA_V0_CB;
>  
> @@ -321,7 +326,7 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
>  	list_for_each_entry(child, &chunk->burst->list, list) {
>  		j--;
>  		if (!j)
> -			control |= (DW_EDMA_V0_LIE | DW_EDMA_V0_RIE);
> +			control |= (DW_EDMA_V0_LIE | rie);

Could drop "rie" and do this, which would keep DW_EDMA_CHIP_NO_MSI
closer to where it's used:

  if (!j) {
    control |= DW_EDMA_V0_LIE;
    if (!(chan->chip->flags & DW_EDMA_CHIP_NO_MSI))
      control |= DW_EDMA_V0_RIE;
  }

>  		/* Channel control */
>  		SET_LL_32(&lli[i].control, control);
> @@ -420,15 +425,16 @@ void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  		SET_CH_32(chip, chan->dir, chan->id, ch_control1,
>  			  (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE));
>  		/* Linked list */
> -		#ifdef CONFIG_64BIT
> +		if (!(chan->chip->flags & DW_EDMA_CHIP_REG32BIT) &&
> +			IS_ENABLED(CONFIG_64BIT)) {
>  			SET_CH_64(chip, chan->dir, chan->id, llp.reg,
>  				  chunk->ll_region.paddr);
> -		#else /* CONFIG_64BIT */
> +		} else {
>  			SET_CH_32(chip, chan->dir, chan->id, llp.lsb,
>  				  lower_32_bits(chunk->ll_region.paddr));
>  			SET_CH_32(chip, chan->dir, chan->id, llp.msb,
>  				  upper_32_bits(chunk->ll_region.paddr));
> -		#endif /* CONFIG_64BIT */

I think this would read better if you reversed the sense:

  if ((chan->chip->flags & DW_EDMA_CHIP_REG32BIT) ||
      !IS_ENABLED(CONFIG_64BIT)) {
    SET_CH_32(...);
    SET_CH_32(...);
  } else {
    SET_CH_64(...);
  }

> +		}
>  	}
>  	/* Doorbell */
>  	SET_RW_32(chip, chan->dir, doorbell,
> diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
> index fcfbc0f47f83d..e74ee15d9832a 100644
> --- a/include/linux/dma/edma.h
> +++ b/include/linux/dma/edma.h
> @@ -33,6 +33,10 @@ enum dw_edma_map_format {
>  	EDMA_MF_HDMA_COMPAT = 0x5
>  };
>  
> +#define DW_EDMA_CHIP_NO_MSI	BIT(0)
> +#define DW_EDMA_CHIP_REG32BIT	BIT(1)
> +#define DW_EDMA_CHIP_LOCAL_EP	BIT(2)
> +
>  /**
>   * struct dw_edma_chip - representation of DesignWare eDMA controller hardware
>   * @dev:		 struct device of the eDMA controller
> @@ -40,6 +44,8 @@ enum dw_edma_map_format {
>   * @nr_irqs:		 total dma irq number
>   * reg64bit		 if support 64bit write to register
>   * @ops			 DMA channel to IRQ number mapping
> + * @flags		 - DW_EDMA_CHIP_NO_MSI can't generate remote MSI irq
> + *			 - DW_EDMA_CHIP_REG32BIT only support 32bit register write

Looks like DW_EDMA_CHIP_LOCAL_EP should be documented here, too?

>   * @wr_ch_cnt		 DMA write channel number
>   * @rd_ch_cnt		 DMA read channel number
>   * @rg_region		 DMA register region
> @@ -53,6 +59,7 @@ struct dw_edma_chip {
>  	int			id;
>  	int			nr_irqs;
>  	const struct dw_edma_core_ops   *ops;
> +	u32			flags;
>  
>  	void __iomem		*reg_base;
>  
> -- 
> 2.24.0.rc1
>