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

[Marek Vasut]

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.