[PATCH 1/9] mtd: spi-nor: Remove recently added nor->addr_width == 3 test

Thu Oct 31 17:53:53 CET 2024

On Thu, Oct 31, 2024 at 02:39:14PM +0100, Marek Vasut wrote:
> On 10/31/24 8:10 AM, Tudor Ambarus wrote:
> > 
> > 
> > On 10/30/24 4:56 PM, Tom Rini wrote:
> > > On Wed, Oct 30, 2024 at 04:20:32PM +0100, Michal Simek wrote:
> > > > 
> > > > 
> > > > On 10/30/24 15:49, Tudor Ambarus wrote:
> > > > > 
> > > > > 
> > > > > On 10/30/24 2:17 PM, Jagan Teki wrote:
> > > > > > On Wed, Oct 30, 2024 at 4:15 PM Tudor Ambarus <tudor.ambarus at linaro.org> wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On 10/30/24 10:33 AM, Jagan Teki wrote:
> > > > > > > > Hi Marek,
> > > > > > > > 
> > > > > > > > On Sun, Oct 27, 2024 at 1:48 AM Marek Vasut
> > > > > > > > <marek.vasut+renesas at mailbox.org> wrote:
> > > > > > > > > 
> > > > > > > > > Remove undocumented nor->addr_width == 3 test. This was added in commit
> > > > > > > > > 5d40b3d384dc ("mtd: spi-nor: Add parallel and stacked memories support")
> > > > > > > > > without any explanation in the commit message. Remove it.
> > > > > > > > > 
> > > > > > > > > This also has a bad side-effect which breaks READ operation of every SPI NOR
> > > > > > > > > which does not use addr_width == 3, e.g. s25fs512s does not work at all. This
> > > > > > > > > is because if addr_width != 3, rem_bank_len is always 0, and if rem_bank_len
> > > > > > > > > is 0, then read_len is 0 and if read_len is 0, then the spi_nor_read() returns
> > > > > > > > > -EIO.
> > > > > > > > > 
> > > > > > > > > Basic reproducer is as follows:
> > > > > > > > > "
> > > > > > > > > => sf probe ; sf read 0x50000000 0 0x10000
> > > > > > > > > SF: Detected s25fs512s with page size 256 Bytes, erase size 256 KiB, total 64 MiB
> > > > > > > > > device 0 offset 0x0, size 0x10000
> > > > > > > > > SF: 65536 bytes @ 0x0 Read: ERROR -5
> > > > > > > > > "
> > > > > > > > > 
> > > > > > > > > Fixes: 5d40b3d384dc ("mtd: spi-nor: Add parallel and stacked memories support")
> > > > > > > > > Signed-off-by: Marek Vasut <marek.vasut+renesas at mailbox.org>
> > > > > > > > > ---
> > > > > > > > > Cc: Andre Przywara <andre.przywara at arm.com>
> > > > > > > > > Cc: Ashok Reddy Soma <ashok.reddy.soma at amd.com>
> > > > > > > > > Cc: Jagan Teki <jagan at amarulasolutions.com>
> > > > > > > > > Cc: Michael Walle <mwalle at kernel.org>
> > > > > > > > > Cc: Michal Simek <michal.simek at amd.com>
> > > > > > > > > Cc: Patrice Chotard <patrice.chotard at foss.st.com>
> > > > > > > > > Cc: Patrick Delaunay <patrick.delaunay at foss.st.com>
> > > > > > > > > Cc: Pratyush Yadav <p.yadav at ti.com>
> > > > > > > > > Cc: Quentin Schulz <quentin.schulz at cherry.de>
> > > > > > > > > Cc: Sean Anderson <seanga2 at gmail.com>
> > > > > > > > > Cc: Simon Glass <sjg at chromium.org>
> > > > > > > > > Cc: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
> > > > > > > > > Cc: Tom Rini <trini at konsulko.com>
> > > > > > > > > Cc: Tudor Ambarus <tudor.ambarus at linaro.org>
> > > > > > > > > Cc: Venkatesh Yadav Abbarapu <venkatesh.abbarapu at amd.com>
> > > > > > > > > Cc: u-boot at lists.denx.de
> > > > > > > > > Cc: uboot-stm32 at st-md-mailman.stormreply.com
> > > > > > > > > ---
> > > > > > > > 
> > > > > > > > Is this patch-set next version for 'previous' reverted series?
> > > > > > > > 
> > > > > > > 
> > > > > > > my 2c: I think I lean towards reverting the stacked/parallel support.
> > > > > > > The only one that benefits of is AMD, while affecting the core code
> > > > > > > quality. It also slows down further contributions and I assume it
> > > > > > > hardens maintainer's job.
> > > > > > 
> > > > > > I did try this before and it is hard to separate from the core. and at
> > > > > > the same time it is hard to maintain the core too.
> > > > > > 
> > > > > > > 
> > > > > > > Even if I (we?) haven't made my mind on how to best implement this, it's
> > > > > > > clear that it shall be above SPI NOR without affecting current devices.
> > > > > > > 
> > > > > > > Not sure if it's fine to revert everything, haven't followed u-boot
> > > > > > > lately, your choice to make.
> > > > > > 
> > > > > > If we find a way (not sure if it's possible) separate like a wrapper
> > > > > > or fix the things without revert - these are two points I can see as
> > > > > > of now.
> > > > > > 
> > > > > 
> > > > > Then this set shall help move in this direction. Some involvement from
> > > > > the stacked/parallel authors would be nice here, and some commitment
> > > > > that the current status in just a temporary situation.
> > > > 
> > > > I hope that by authors you mean SW owners not really HW authors of this wiring.
> > > > Jagan is aware that we are using this configuration for quite a long time
> > > > and we are still here and not leaving.
> > > > As you know Amit is trying to find a solution in Linux but nothing has been
> > > > agreed yet. If there is agreement to separate it to own driver or so we will
> > > > be definitely working with u-boot to get it to the same state.
> > > > 
> > > > This patchset is using the latest approved DT binding created by Miquel and
> > > > if new binding is accepted we will be working to align to it.
> > > > Also intention is to bring this functionality to customers and not break
> > > > others. That's why these patches are pulled into rc1 to get better test
> > > > coverage.
> > > > 
> > > > And to be fair to say version which has been merged was v14 and anybody
> > > > could comment it before and we are definitely here to help to resolve issues
> > > > on other boards caused by this patchset. But we need to have help from
> > > > others because obviously we don't have other boards and they are likely also
> > > > not wired in CI.
> > > 
> > > To be clear, we need to resolve the problems that have shown up now on
> > > all of the hardware that was working in v2024.10. My current inclination
> > > is to merge
> > > https://patchwork.ozlabs.org/project/uboot/list/?series=429932&state=*
> > > (aka this series in question) ASAP.
> > > 
> > 
> > We can surely live with this temporary situation. Now that we're assured
> > to have AMD's help in moving forward, it feels less like a transfer of
> > responsibility on the poorly designed stacked/parallel software support.
> > Fine by me to choose the smaller fixes/improvements over the full revert.
> Considering how many unexplained subtle broken changes even the first commit
> of the stacked/parallel series included, I would be banking toward a full
> revert. The hints on which changes were unexplained are in this series.

I'm going to start with this series. If there's still unaddressable
problems, now that we have more eyes on everything, it's not overly hard
to revert this series merge and then the initial series merge.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20241031/5dd714aa/attachment.sig>