On Tue, Apr 16, 2024 at 07:28:55PM +0300, Serge Semin wrote:
> Currently the src_addr_width and dst_addr_width fields of the
> dma_slave_config structure are mapped to the CTLx.SRC_TR_WIDTH and
> CTLx.DST_TR_WIDTH fields of the peripheral bus side in order to have the
> properly aligned data passed to the target device. It's done just by
> converting the passed peripheral bus width to the encoded value using the
> __ffs() function. This implementation has several problematic sides:
>
> 1. __ffs() is undefined if no bit exist in the passed value. Thus if the
> specified addr-width is DMA_SLAVE_BUSWIDTH_UNDEFINED, __ffs() may return
> unexpected value depending on the platform-specific implementation.
>
> 2. DW AHB DMA-engine permits having the power-of-2 transfer width limited
> by the DMAH_Mk_HDATA_WIDTH IP-core synthesize parameter. Specifying
> bus-width out of that constraints scope will definitely cause unexpected
> result since the destination reg will be only partly touched than the
> client driver implied.
>
> Let's fix all of that by adding the peripheral bus width verification
> method which would make sure that the passed source or destination address
> width is valid and if undefined then the driver will just fallback to the
> 1-byte width transfer.
Please, add a word that you apply the check in the dwc_config() which is
supposed to be called before preparing any transfer?
...
> +static int dwc_verify_p_buswidth(struct dma_chan *chan)
> +{
> + struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
> + struct dw_dma *dw = to_dw_dma(chan->device);
> + u32 reg_width, max_width;
> +
> + if (dwc->dma_sconfig.direction == DMA_MEM_TO_DEV)
> + reg_width = dwc->dma_sconfig.dst_addr_width;
> + else if (dwc->dma_sconfig.direction == DMA_DEV_TO_MEM)
> + reg_width = dwc->dma_sconfig.src_addr_width;
> + else /* DMA_MEM_TO_MEM */
Actually not only this direction, but TBH I do not see value in these comments.
> + return 0;
> +
> + max_width = dw->pdata->data_width[dwc->dws.p_master];
> +
> + /* Fall-back to 1byte transfer width if undefined */
1-byte
(as you even used in the commit message correctly)
> + if (reg_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
> + reg_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> + else if (!is_power_of_2(reg_width) || reg_width > max_width)
> + return -EINVAL;
> + else /* bus width is valid */
> + return 0;
> +
> + /* Update undefined addr width value */
> + if (dwc->dma_sconfig.direction == DMA_MEM_TO_DEV)
> + dwc->dma_sconfig.dst_addr_width = reg_width;
> + else /* DMA_DEV_TO_MEM */
> + dwc->dma_sconfig.src_addr_width = reg_width;
So, can't you simply call clamp() for both fields in dwc_config()?
> + return 0;
> +}
...
> + int err;
Hmm... we have two functions one of which is using different name for this.
Can we have a patch to convert to err the other one?
--
With Best Regards,
Andy Shevchenko
