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?
--
With Best Regards,
Andy Shevchenko
