Re: [PATCH 4/4] dmaengine: dw: Simplify max-burst calculation procedure

[Andy Shevchenko]

On Tue, Apr 16, 2024 at 07:28:58PM +0300, Serge Semin wrote:
> In order to have a more coherent DW AHB DMA slave configuration method
> let's simplify the source and destination channel max-burst calculation
> procedure:
> 
> 1. Create the max-burst verification method as it has been just done for
> the memory and peripheral address widths. Thus the DWC DMA slave config

dwc_config() method

?

> method will turn to a set of the verification methods execution.
> 
> 2. Since both the generic DW AHB DMA and Intel DMA32 engines support the

"i" in iDMA 32-bit stands for "integrated", so 'Intel iDMA 32-bit'

> power-of-2 bursts only, then the specified by the client driver max-burst
> values can be converted to being power-of-2 right in the max-burst
> verification method.
> 
> 3. Since max-burst encoded value is required on the CTL_LO fields
> calculation stage, the encode_maxburst() callback can be easily dropped
> from the dw_dma structure meanwhile the encoding procedure will be
> executed right in the CTL_LO register value calculation.
> 
> Thus the update will provide the next positive effects: the internal
> DMA-slave config structure will contain only the real DMA-transfer config
> value, which will be encoded to the DMA-controller register fields only
> when it's required on the buffer mapping; the redundant encode_maxburst()
> callback will be dropped simplifying the internal HW-abstraction API;
> DWC-config method will look more readable executing the verification

dwc_config() method

?

> functions one-by-one.

..

> +static void dwc_verify_maxburst(struct dma_chan *chan)

It's inconsistent to the rest of _verify methods. It doesn't verify as it
doesn't return anything. Make it int or rename the function.

> +{
> +	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
> +
> +	dwc->dma_sconfig.src_maxburst =
> +		clamp(dwc->dma_sconfig.src_maxburst, 1U, dwc->max_burst);
> +	dwc->dma_sconfig.dst_maxburst =
> +		clamp(dwc->dma_sconfig.dst_maxburst, 1U, dwc->max_burst);
> +
> +	dwc->dma_sconfig.src_maxburst =
> +		rounddown_pow_of_two(dwc->dma_sconfig.src_maxburst);
> +	dwc->dma_sconfig.dst_maxburst =
> +		rounddown_pow_of_two(dwc->dma_sconfig.dst_maxburst);
> +}

..

>  static int dwc_verify_p_buswidth(struct dma_chan *chan)
> -		reg_burst = rounddown_pow_of_two(dwc->dma_sconfig.src_maxburst);
> +		reg_burst = dwc->dma_sconfig.src_maxburst;

Seems you have a dependency, need a comment below that maxburst has to be
"verified" [whatever] first.

..

> +static inline u8 dw_dma_encode_maxburst(u32 maxburst)
> +{
> +	/*
> +	 * Fix burst size according to dw_dmac. We need to convert them as:
> +	 * 1 -> 0, 4 -> 1, 8 -> 2, 16 -> 3.
> +	 */
> +	return maxburst > 1 ? fls(maxburst) - 2 : 0;
> +}

Split these moves to another preparatory patch.

-- 
With Best Regards,
Andy Shevchenko