Re: [PATCH] dmaengine: sun4i: Set the maximum segment size

[Vinod Koul <vkoul@kernel.org>]

On 20-06-22, 22:13, Samuel Holland wrote:
> The sun4i DMA engine supports transfer sizes up to 128k for normal DMA
> and 16M for dedicated DMA, as documented in the A10 and A20 manuals.
> 
> Since this is larger than the default segment size limit (64k), exposing
> the real limit reduces the number of transfers needed for a transaction.
> However, because the device can only report one segment size limit, we
> have to expose the smaller limit from normal DMA.
> 
> One complication is that the driver combines pairs of periodic transfers
> to reduce programming overhead. This only works when the period size is
> at most half of the maximum transfer size. With the default 64k segment
> size limit, this was always the case, but for normal DMA it is no longer
> guaranteed. Skip the optimization if the period is too long; even
> without it, the overhead is less than before.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> I don't have any A10 or A20 boards I can test this on.
> 
>  drivers/dma/sun4i-dma.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/sun4i-dma.c b/drivers/dma/sun4i-dma.c
> index 93f1645ae928..f291b1b4db32 100644
> --- a/drivers/dma/sun4i-dma.c
> +++ b/drivers/dma/sun4i-dma.c
> @@ -7,6 +7,7 @@
>  #include <linux/bitmap.h>
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
> +#include <linux/dma-mapping.h>
>  #include <linux/dmaengine.h>
>  #include <linux/dmapool.h>
>  #include <linux/interrupt.h>
> @@ -122,6 +123,15 @@
>  	 SUN4I_DDMA_PARA_DST_WAIT_CYCLES(2) |				\
>  	 SUN4I_DDMA_PARA_SRC_WAIT_CYCLES(2))
>  
> +/*
> + * Normal DMA supports individual transfers (segments) up to 128k.
> + * Dedicated DMA supports transfers up to 16M. We can only report
> + * one size limit, so we have to use the smaller value.
> + */
> +#define SUN4I_NDMA_MAX_SEG_SIZE		SZ_128K
> +#define SUN4I_DDMA_MAX_SEG_SIZE		SZ_16M

This new define is not used, so why add?

> +#define SUN4I_DMA_MAX_SEG_SIZE		SUN4I_NDMA_MAX_SEG_SIZE
> +
>  struct sun4i_dma_pchan {
>  	/* Register base of channel */
>  	void __iomem			*base;
> @@ -155,7 +165,8 @@ struct sun4i_dma_contract {
>  	struct virt_dma_desc		vd;
>  	struct list_head		demands;
>  	struct list_head		completed_demands;
> -	int				is_cyclic;
> +	bool				is_cyclic : 1;
> +	bool				use_half_int : 1;
>  };
>  
>  struct sun4i_dma_dev {
> @@ -372,7 +383,7 @@ static int __execute_vchan_pending(struct sun4i_dma_dev *priv,
>  	if (promise) {
>  		vchan->contract = contract;
>  		vchan->pchan = pchan;
> -		set_pchan_interrupt(priv, pchan, contract->is_cyclic, 1);
> +		set_pchan_interrupt(priv, pchan, contract->use_half_int, 1);

not interrupt when cyclic?

>  		configure_pchan(pchan, promise);
>  	}
>  
> @@ -735,12 +746,21 @@ sun4i_dma_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf, size_t len,
>  	 *
>  	 * Which requires half the engine programming for the same
>  	 * functionality.
> +	 *
> +	 * This only works if two periods fit in a single promise. That will
> +	 * always be the case for dedicated DMA, where the hardware has a much
> +	 * larger maximum transfer size than advertised to clients.
>  	 */
> -	nr_periods = DIV_ROUND_UP(len / period_len, 2);
> +	if (vchan->is_dedicated || period_len <= SUN4I_NDMA_MAX_SEG_SIZE / 2) {
> +		period_len *= 2;
> +		contract->use_half_int = 1;
> +	}
> +
> +	nr_periods = DIV_ROUND_UP(len, period_len);
>  	for (i = 0; i < nr_periods; i++) {
>  		/* Calculate the offset in the buffer and the length needed */
> -		offset = i * period_len * 2;
> -		plength = min((len - offset), (period_len * 2));
> +		offset = i * period_len;
> +		plength = min((len - offset), period_len);
>  		if (dir == DMA_MEM_TO_DEV)
>  			src = buf + offset;
>  		else
> @@ -1149,6 +1169,8 @@ static int sun4i_dma_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, priv);
>  	spin_lock_init(&priv->lock);
>  
> +	dma_set_max_seg_size(&pdev->dev, SUN4I_DMA_MAX_SEG_SIZE);
> +
>  	dma_cap_zero(priv->slave.cap_mask);
>  	dma_cap_set(DMA_PRIVATE, priv->slave.cap_mask);
>  	dma_cap_set(DMA_MEMCPY, priv->slave.cap_mask);
> -- 
> 2.35.1

-- 
~Vinod