[PATCH 1/6] Revert "spi: zynq_qspi: Add parallel memories support in QSPI driver"

Fri Dec 6 10:46:08 CET 2024

> -----Original Message-----
> From: Jon Humphreys <j-humphreys at ti.com>
> Sent: Friday, December 6, 2024 10:04 AM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu at amd.com>; Marek Vasut
> <marek.vasut at mailbox.org>; Simek, Michal <michal.simek at amd.com>; u-
> boot at lists.denx.de; Tom Rini <trini at konsulko.com>
> Cc: Andre Przywara <andre.przywara at arm.com>; Ashok Reddy Soma
> <ashok.reddy.soma at amd.com>; Jagan Teki <jagan at amarulasolutions.com>;
> Michael Walle <mwalle at kernel.org>; Patrice Chotard
> <patrice.chotard at foss.st.com>; Patrick Delaunay <patrick.delaunay at foss.st.com>;
> Pratyush Yadav <p.yadav at ti.com>; Quentin Schulz <quentin.schulz at cherry.de>;
> Sean Anderson <seanga2 at gmail.com>; Simon Glass <sjg at chromium.org>;
> Takahiro Kuwano <Takahiro.Kuwano at infineon.com>; Tudor Ambarus
> <tudor.ambarus at linaro.org>; uboot-stm32 at st-md-mailman.stormreply.com
> Subject: RE: [PATCH 1/6] Revert "spi: zynq_qspi: Add parallel memories support in
> QSPI driver"
> 
> "Abbarapu, Venkatesh" <venkatesh.abbarapu at amd.com> writes:
> 
> > Hi,
> >
> >> -----Original Message-----
> >> From: Jon Humphreys <j-humphreys at ti.com>
> >> Sent: Thursday, December 5, 2024 9:47 AM
> >> To: Marek Vasut <marek.vasut at mailbox.org>; Simek, Michal
> >> <michal.simek at amd.com>; u-boot at lists.denx.de; Tom Rini
> >> <trini at konsulko.com>
> >> Cc: Andre Przywara <andre.przywara at arm.com>; Ashok Reddy Soma
> >> <ashok.reddy.soma at amd.com>; Jagan Teki <jagan at amarulasolutions.com>;
> >> Michael Walle <mwalle at kernel.org>; Patrice Chotard
> >> <patrice.chotard at foss.st.com>; Patrick Delaunay
> >> <patrick.delaunay at foss.st.com>; Pratyush Yadav <p.yadav at ti.com>;
> >> Quentin Schulz <quentin.schulz at cherry.de>; Sean Anderson
> >> <seanga2 at gmail.com>; Simon Glass <sjg at chromium.org>; Takahiro Kuwano
> >> <Takahiro.Kuwano at infineon.com>; Tudor Ambarus
> >> <tudor.ambarus at linaro.org>; Abbarapu, Venkatesh
> >> <venkatesh.abbarapu at amd.com>;
> >> uboot-stm32 at st-md-mailman.stormreply.com
> >> Subject: Re: [PATCH 1/6] Revert "spi: zynq_qspi: Add parallel
> >> memories support in QSPI driver"
> >>
> >> Jon Humphreys <j-humphreys at ti.com> writes:
> >>
> >> > Marek Vasut <marek.vasut at mailbox.org> writes:
> >> >
> >> >> On 11/7/24 4:49 PM, Jon Humphreys wrote:
> >> >>> Marek Vasut <marek.vasut at mailbox.org> writes:
> >> >>>
> >> >>>> On 11/6/24 10:58 PM, Jon Humphreys wrote:
> >> >>>>> Marek Vasut <marek.vasut at mailbox.org> writes:
> >> >>>>>
> >> >>>>>> On 11/6/24 8:18 PM, Jon Humphreys wrote:
> >> >>>>>>> Marek Vasut <marek.vasut at mailbox.org> writes:
> >> >>>>>>>
> >> >>>>>>>> On 10/23/24 10:17 AM, Michal Simek wrote:
> >> >>>>>>>>>
> >> >>>>>>>>>
> >> >>>>>>>>> On 10/22/24 23:06, Marek Vasut wrote:
> >> >>>>>>>>>> This reverts commit
> 1e36d34b52e7a1ebe5a2a5339d6905540f4253aa.
> >> >>>>>>>>>>
> >> >>>>>>>>>> This parallel/stacked support breaks basic SPI NOR
> >> >>>>>>>>>> support, e.g. this no longer works:
> >> >>>>>>>>>>
> >> >>>>>>>>>> => sf probe && sf update 0x50000000 0 0x160000
> >> >>>>>>>>>> SF: Detected s25fs512s with page size 256 Bytes, erase
> >> >>>>>>>>>> size
> >> >>>>>>>>>> 256 KiB, total 64 MiB device 0 offset 0x0, size 0x160000
> >> >>>>>>>>>> SPI flash failed in read step
> >> >>>>>>>>>
> >> >>>>>>>>> Reverting everything seems to me too much. Tom has tested
> >> >>>>>>>>> it on his HW and didn't see any issue. That's why better to
> >> >>>>>>>>> look at code which is causing this.
> >> >>>>>>>>> You are reverting everything but likely there is specific
> >> >>>>>>>>> patch which is causing this. Which one is it?
> >> >>>>>>>>> Which board was used for your testing? Likely we don't have
> >> >>>>>>>>> access to
> >> it.
> >> >>>>>>>>> Is there any QEMU available which can be used for debugging?
> >> >>>>>>>>
> >> >>>>>>>> The testcase including the exact SPI NOR model is above.
> >> >>>>>>>>
> >> >>>>>>>> iMX6 with w25q16dw seems to be broken too.
> >> >>>>>>>>
> >> >>>>>>>> Basically every board I have access no longer has a working
> >> >>>>>>>> "sf probe ; sf update" combination ... so yeah, this means
> >> >>>>>>>> this patchset is fundamentally broken.
> >> >>>>>>>>
> >> >>>>>>>
> >> >>>>>>> I can also confirm that the patch series:
> >> >>>>>>>
> >> >>>>>>> f8efc68b30e Merge patch series "spi-nor: Add parallel and
> >> >>>>>>> stacked memories support"
> >> >>>>>>>
> >> >>>>>>> breaks SPI NOR on TI platforms, particularly SK-AM62 and SK-
> AM62P:
> >> >>>>>>>
> >> >>>>>>> U-Boot 2024.10-00752-gf8efc68b30e2 (Nov 06 2024 - 12:25:13
> >> >>>>>>> -0600)
> >> >>>>>>>
> >> >>>>>>> SoC:   AM62X SR1.0 HS-FS
> >> >>>>>>> Model: Texas Instruments AM625 SK ...
> >> >>>>>>> Hit any key to stop autoboot:  0 => sf probe && sf update
> >> >>>>>>> ${loadaddr} 0x400000 0x10
> >> >>>>>>> SF: Detected s28hs512t with page size 256 Bytes, erase size
> >> >>>>>>> 256 KiB, total 64 MiB device 0 offset 0x400000, size 0x10 SPI
> >> >>>>>>> flash failed in read step =>
> >> >>>>>> Sigh ... can you please test current u-boot/master and see if
> >> >>>>>> the error is fixed there ?
> >> >>>>>>
> >> >>>>>
> >> >>>>> Yes I had verified it also fails against master, although the
> >> >>>>> behavior was a bit different.  The .'s below are our DMA engine
> >> >>>>> waiting
> >> indefinitely.
> >> >>>>>
> >> >>>>> => sf probe && sf update ${loadaddr} 0x400000 0x10
> >> >>>>> SF: Detected s28hs512t with page size 256 Bytes, erase size 256
> >> >>>>> KiB, total 64 MiB device 0 offset 0x400000, size 0x10
> >> >>>>> .....................................................
> >> >>>>>
> >> >>>>> I have not investigated further.
> >> >>>>
> >> >>>> Can you try and run some 'git bisect' to find out exactly which
> >> >>>> commit broke your use case ? There is a bunch of fixes for the
> >> >>>> worst breakage that landed recently, but clearly there is more.
> >> >>>>
> >> >>>> Full revert seems increasingly appealing ...
> >> >>>
> >> >>> commit 5d40b3d384d
> >> >> So there is still something broken in that specific commit that I
> >> >> missed when removing the defects ? Sigh ... can you try to narrow it down ?
> >> >
> >> > Hi Marek, I tried to narrow the changes in commit
> >> > 5d40b3d384dc536ec26ce9b76b20b0b84749d2d1 a bit by first applying
> >> > all of the changes in the .h files, and then for spi-nor-core.c,
> >> > only applying changes per function. The only function change that
> >> > causes errors was spi_nor_read(). With the changes in
> >> > spi_nor_read() in commit
> >> > 5d40b3d384dc536ec26ce9b76b20b0b84749d2d1 applied to the prior
> >> > commit fbe16bc2801, did I get:
> >> >
> >> > => sf probe && sf update ${loadaddr} 0x400000 0x10
> >> > SF: Detected s28hs512t with page size 256 Bytes, erase size 256
> >> > KiB, total 64 MiB device 0 offset 0x400000, size 0x10 SPI flash
> >> > failed in read step
> >> >
> >>
> >> Hi all.  What is the status of addressing the failures introduced by
> >> this patchset?  I tried building from u-boot next and still see the failure.
> >>
> >> Marek, was the isolation of changes I performed above helpful in
> >> understanding the failure?
> >>
> >
> > Can you try by applying this change
> > https://lore.kernel.org/u-boot/20241118090544.21956-1-venkatesh.abbara
> > pu at amd.com/
> >
> 
> Hi Venkatesh, the patch seems to remove the issue I was seeing.
> 
> Thanks
> Jon

Hi Humphreys,
Thank you for confirming that the patch worked and resolved the issue.

Thanks
Venkatesh
> 
> > Thanks
> > Venkatesh
> >> Thanks
> >> Jon
> >>
> >> > Let me know if this helps.
> >> >
> >> > Jon