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
