On Mon, Sep 16, 2024 at 02:43:48PM +0300, Andy Shevchenko wrote: > On Sat, Sep 14, 2024 at 10:22:22PM +0300, Serge Semin wrote: > > On Sat, Sep 14, 2024 at 10:12:35PM +0300, Andy Shevchenko wrote: > > > On Fri, Aug 02, 2024 at 10:50:46AM +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 and calling it in dwc_config() which is supposed to be executed > > > > before preparing any transfer. The new method will 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. > > > > > > This patch broke Intel Merrifield iDMA32 + SPI PXA2xx configuration to > > > me. Since it's first in the series and most likely the rest is > > > dependent and we are almost at the release date I propose to roll back > > > and start again after v6.12-rc1 will be out. Vinod, can we revert the > > > entire series, please? > > > > I guess it's not the best option, since the patch has already been > > backported to the stable kernels anyway. Rolling back it from all of > > them seems tiresome. Let's at least try to fix the just discovered > > problem? > > Please, provide one we can test! > > > Could you please provide more details about what exactly happening? > > Sure. AFAICT the only problematic line is this: > > else if (!is_power_of_2(reg_width) || reg_width > max_width) > > in your patch, and it may trigger, for example, when max_width == 0. > This, in accordance with my brief investigation, happens due to the following. > > The DMA slave configuration is being copied twice in DW DMA code: > 1) when respective filter function triggers (see acpi/of glue code); > 2) when the channel is about to be allocated. > > The iDMA32 has only a single master, and hence m_master == p_master, > BUT the filter function in the acpi code is universal and it copies > the wrong (from the iDMA32 perspective) value to p_master. > As the result, when you retrieve the max_width, it takes the value from > p_master, which is defined to 1 (sic!), and hence assigns it to 0. > > I don't know how to quickfix this as the proper fix seems to provide > the correct data in the first place. > > Any ideas, patches we may test? P.S. for your advocacy it seems that your change actually revealed an inconsistency in the existing code. But still, it made a regression. -- With Best Regards, Andy Shevchenko
