Messages in this thread | | | Date | Tue, 16 Apr 2024 21:00:50 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH 1/4] dmaengine: dw: Add peripheral bus width verification |
| |
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
| |