Re: [PATCH v2 3/6] dmaengine: dw: Set DMA device max segment size parameter

[Andy Shevchenko]

+Cc (Vineet, for information you probably know)

On Fri, May 08, 2020 at 01:53:01PM +0300, Serge Semin wrote:
> Maximum block size DW DMAC configuration corresponds to the max segment
> size DMA parameter in the DMA core subsystem notation. Lets set it with a
> value specific to the probed DW DMA controller. It shall help the DMA
> clients to create size-optimized SG-list items for the controller. This in
> turn will cause less dw_desc allocations, less LLP reinitializations,
> better DMA device performance.

Thank you for the patch.
My comments below.

...

> +		/*
> +		 * Find maximum block size to be set as the DMA device maximum
> +		 * segment size. By doing so we'll have size optimized SG-list
> +		 * items for the channels with biggest block size. This won't
> +		 * be a problem for the rest of the channels, since they will
> +		 * still be able to split the requests up by allocating
> +		 * multiple DW DMA LLP descriptors, which they would have done
> +		 * anyway.
> +		 */
> +		if (dwc->block_size > block_size)
> +			block_size = dwc->block_size;
>  	}
>  
>  	/* Clear all interrupts on all channels. */
> @@ -1220,6 +1233,10 @@ int do_dma_probe(struct dw_dma_chip *chip)
>  			     BIT(DMA_MEM_TO_MEM);
>  	dw->dma.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
>  
> +	/* Block size corresponds to the maximum sg size */
> +	dw->dma.dev->dma_parms = &dw->dma_parms;
> +	dma_set_max_seg_size(dw->dma.dev, block_size);
> +
>  	err = dma_async_device_register(&dw->dma);
>  	if (err)
>  		goto err_dma_register;

Yeah, I have locally something like this and I didn't dare to upstream because
there is an issue. We have this information per DMA controller, while we
actually need this on per DMA channel basis.

Above will work only for synthesized DMA with all channels having same block
size. That's why above conditional is not needed anyway.

OTOH, I never saw the DesignWare DMA to be synthesized differently (I remember
that Intel Medfield has interesting settings, but I don't remember if DMA
channels are different inside the same controller).

Vineet, do you have any information that Synopsys customers synthesized DMA
controllers with different channel characteristics inside one DMA IP?

...

>  #include <linux/bitops.h>

> +#include <linux/device.h>

Isn't enough to supply

struct device;

?

>  #include <linux/interrupt.h>
>  #include <linux/dmaengine.h>

Also this change needs a separate patch I suppose.

...

> -	struct dma_device	dma;
> -	char			name[20];
> -	void __iomem		*regs;
> -	struct dma_pool		*desc_pool;
> -	struct tasklet_struct	tasklet;
> +	struct dma_device		dma;
> +	struct device_dma_parameters	dma_parms;
> +	char				name[20];
> +	void __iomem			*regs;
> +	struct dma_pool			*desc_pool;
> +	struct tasklet_struct		tasklet;
>  
>  	/* channels */
> -	struct dw_dma_chan	*chan;
> -	u8			all_chan_mask;
> -	u8			in_use;
> +	struct dw_dma_chan		*chan;
> +	u8				all_chan_mask;
> +	u8				in_use;

Please split formatting fixes into a separate patch.


-- 
With Best Regards,
Andy Shevchenko