[PATCH] mtd: spi-nor: Fix SF MTDIDS when registering multiple MTDs with DM enabled

Thu Sep 16 09:50:25 CEST 2021

Hi,

On 9/15/21 2:10 PM, Marek Vasut wrote:
> On 9/15/21 2:00 PM, Patrick DELAUNAY wrote:
>
> Hi,
>
>>>> On 9/15/21 10:57 AM, Marek Vasut wrote:
>>>>> On 9/15/21 10:53 AM, Patrice CHOTARD wrote:
>>>>>> Hi All
>>>>>>
>>>>>> On 9/15/21 7:59 AM, Marek Vasut wrote:
>>>>>>> On 9/15/21 6:23 AM, Heiko Schocher wrote:
>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> On 15.09.21 01:06, Marek Vasut wrote:
>>>>>>>>> The flash->mtd.name used to be nor%d before, now it is the 
>>>>>>>>> type of the
>>>>>>>>> SPI NOR like e.g. mt25ql02g. It is possible to find plenty of 
>>>>>>>>> examples
>>>>>>>>> of the former in U-Boot configs by searching for 
>>>>>>>>> MTDIDS.*nor.*spi, while
>>>>>>>>> the later is ambiguous if there are multiple flashes of the 
>>>>>>>>> same type in
>>>>>>>>> the system and breaks existing environments.
>>>>>>>>>
>>>>>>>>> This does no longer get recognized when running 'mtdparts' for 
>>>>>>>>> example:
>>>>>>>>> CONFIG_MTDIDS_DEFAULT="nor0=47040000.spi.0"
>>>>>>>>>
>>>>>>>>> Fix this by setting the correct mtd.name to nor%d.
>>>>>>>>>
>>>>>>>>> Fixes: b7f060565e3 ("mtd: spi-nor: allow registering multiple 
>>>>>>>>> MTDs when DM is enabled")
>>>>>>>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>>>>>>>> Cc: Heiko Schocher <hs at denx.de>
>>>>>>>>> Cc: Jagan Teki <jagan at amarulasolutions.com>
>>>>>>>>> Cc: Marek Behún <marek.behun at nic.cz>
>>>>>>>>> Cc: Miquel Raynal <miquel.raynal at bootlin.com>
>>>>>>>>> Cc: Pali Rohár <pali at kernel.org>
>>>>>>>>> Cc: Patrice Chotard <patrice.chotard at foss.st.com>
>>>>>>>>> Cc: Patrick Delaunay <patrick.delaunay at st.com>
>>>>>>>>> Cc: Priyanka Jain <priyanka.jain at nxp.com>
>>>>>>>>> Cc: Simon Glass <sjg at chromium.org>
>>>>>>>>> ---
>>>>>>>>>     drivers/mtd/spi/sf_mtd.c | 5 ++++-
>>>>>>>>>     1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> Seem fixes the same problem as Patrick already posted here:
>>>>>>>>
>>>>>>>> https://patchwork.ozlabs.org/project/uboot/patch/20210913095742.v2.1.I73dae4b93f0587dc130e512e95a1f4794e0b0233@changeid/ 
>>>>>>>>
>>>>>>>>
>>>>>>>> I find your approach cleaner, so:
>>>>>>>>
>>>>>>>> Acked-by: Heiko Schocher <hs at denx.de>
>>>>>>>>
>>>>>>>> @Patrick: Could you test this patch please?
>>>>>>>
>>>>>>> The option from Patrick does look more predictable, but looking 
>>>>>>> at the old implementation of spi_flash_mtd_number(), that one 
>>>>>>> handled even cases of parallel-nor and spi-nor (both using the 
>>>>>>> nor%d) present on the same system. This:
>>>>>>>
>>>>>>>     static int spi_flash_mtd_number(void)
>>>>>>>     {
>>>>>>>     #ifdef CONFIG_SYS_MAX_FLASH_BANKS
>>>>>>>            return CONFIG_SYS_MAX_FLASH_BANKS;
>>>>>>>     #else
>>>>>>>            return 0;
>>>>>>>     #endif
>>>>>>>     }
>>>>>>>     ...
>>>>>>>     sprintf(sf_mtd_name, "nor%d", spi_flash_mtd_number());
>>>>>>>
>>>>>>> The patch from Patrick does not, it does per-spi-nor-only 
>>>>>>> counting using the dev_seq() there, which is more predictable, 
>>>>>>> but local to spi-nor.
>>>>>>>
>>>>>>> The MTD core has its own IDR counting, which is used by this 
>>>>>>> patch and is global to the MTD core. That has two issues, one is 
>>>>>>> that it is possibly too global and counts both nor%d and nand%d, 
>>>>>>> which means it would fail on systems with both SPI NOR and 
>>>>>>> regular NAND. The other is it is likely less predictable (what 
>>>>>>> happens if the SPI NOR and parallel NOR gets probed in the 
>>>>>>> reverse order ? I know it is unlikely, but it can happen in the 
>>>>>>> future).
>>>>>>>
>>>>>>> So I think we need a third option which I would suggest be based 
>>>>>>> on the patch from Patrick, but which would take into account the 
>>>>>>> other nor%d devices like parallel NOR flashes.
>>>>>>>
>>>>>>> That would likely have to happen in the mtd core using some 
>>>>>>> modified IDR counting. I think you might need to modify it such 
>>>>>>> that you would pass in the prefix of the device (like 'nor') and 
>>>>>>> then do per-prefix counting, with the special case that parallel 
>>>>>>> NORs are counted first. Also, that would also have to consider 
>>>>>>> probing of multiple SPI NORs in either order (say you have two, 
>>>>>>> if you do 'sf probe 0:0 ; sf probe 0:1' vs. 'sf probe 0:1 ; sf 
>>>>>>> probe 0:0'), and make sure they get enumerated with the same 
>>>>>>> nor%d value either way, that might be where the dev_seq() would 
>>>>>>> come in.
>>>>>>
>>>>>> I just got a try with this patch and unfortunately, it doesn't 
>>>>>> solve the issue.
>>>>>> I tested it on STM32MP1-ev1 board which embedds 1 NAND and 2 
>>>>>> SPI-NORs.
>>>>>> Here is the output of mtd list command:
>>>>>>
>>>>>> U-Boot 2021.10-rc4-00001-g1923b5a21d (Sep 15 2021 - 10:48:48 +0200)
>>>>>>
>>>>>> CPU: STM32MP157AAA Rev.B
>>>>>> Model: STMicroelectronics STM32MP157C eval daughter on eval mother
>>>>>> Board: stm32mp1 in basic mode (st,stm32mp157c-ev1)
>>>>>> Board: MB1263 Var1.0 Rev.C-01
>>>>>> DRAM:  1 GiB
>>>>>> Clocks:
>>>>>> - MPU : 650 MHz
>>>>>> - MCU : 208.878 MHz
>>>>>> - AXI : 266.500 MHz
>>>>>> - PER : 24 MHz
>>>>>> - DDR : 533 MHz
>>>>>> WDT:   Started with servicing (32s timeout)
>>>>>> NAND:  1024 MiB
>>>>>> MMC:   STM32 SD/MMC: 0, STM32 SD/MMC: 1
>>>>>> Loading Environment from MMC... *** Warning - bad CRC, using 
>>>>>> default environment
>>>>>>
>>>>>> In:    serial
>>>>>> Out:   serial
>>>>>> Err:   serial
>>>>>> Net:   eth0: ethernet at 5800a000
>>>>>> Hit any key to stop autoboot:  0
>>>>>> STM32MP> mtd list
>>>>>> SF: Detected mx66l51235l with page size 256 Bytes, erase size 64 
>>>>>> KiB, total 64 MiB
>>>>>> SF: Detected nor1 with page size 256 Bytes, erase size 64 KiB, 
>>>>>> total 64 MiB
>>>>>> List of MTD devices:
>>>>>> * nand0
>>>>>>     - type: NAND flash
>>>>>>     - block size: 0x40000 bytes
>>>>>>     - min I/O: 0x1000 bytes
>>>>>>     - OOB size: 224 bytes
>>>>>>     - OOB available: 118 bytes
>>>>>>     - ECC strength: 8 bits
>>>>>>     - ECC step size: 512 bytes
>>>>>>     - bitflip threshold: 6 bits
>>>>>>     - 0x000000000000-0x000040000000 : "nand0"
>>>>>> * nor2
>>>>>>     - device: mx66l51235l at 0
>>>>>>     - parent: spi at 58003000
>>>>>>     - driver: jedec_spi_nor
>>>>>>     - path: /soc/spi at 58003000/mx66l51235l at 0
>>>>>>     - type: NOR flash
>>>>>>     - block size: 0x10000 bytes
>>>>>>     - min I/O: 0x1 bytes
>>>>>>     - 0x000000000000-0x000004000000 : "nor2"
>>>>>> * nor2
>>>>>>     - device: mx66l51235l at 1
>>>>>>     - parent: spi at 58003000
>>>>>>     - driver: jedec_spi_nor
>>>>>>     - path: /soc/spi at 58003000/mx66l51235l at 1
>>>>>>     - type: NOR flash
>>>>>>     - block size: 0x10000 bytes
>>>>>>     - min I/O: 0x1 bytes
>>>>>>     - 0x000000000000-0x000004000000 : "nor2"
>>>>>
>>>>> Right , that's what I was afraid of. The patch from Patrick would 
>>>>> fail if there are multiple disparate NOR technologies (parallel 
>>>>> NOR and SPI NOR). So we need a third option.
>>>>
>>>> And also HyperFlash, which today, are seen as "nor".
>>>
>>> Dang ... that too. I have a system with HF and SPI NOR which I could 
>>> use for testing I _think_. Do you expect Patrick to send out a V2 of 
>>> his patch with this stuff above addressed ?
>>
>>
>> Hi,
>>
>>
>> I will change the name to "spi-nor%d" in V2 (alligned with "spi-nand")
>>
>> to avoid conflict with other "nor" device.
>
> No, that is not an option, because that will break existing devices, 
> grep for MTDIDS.*nor in configs/ .
>
>> I need to check if that solve my issue.
>
> [...]

Yes, after check / test it is not a option and that is not working

https://github.com/patrickdelaunay/u-boot/pull/new/mtd_nor

STM32MP> env print mtdids
mtdids=nand0=nand0,nor0=spi-nor0

STM32MP> env print mtdparts
mtdparts=mtdparts=nand0:2m(fsbl),4m(fip1),4m(fip2),-(UBI);nor0:256k(fsbl1),256k(fsbl2),4m(fip),512k(u-boot-env),-(nor_user)

STM32MP> mtd list
List of MTD devices:
* nand0
   - type: NAND flash
   - block size: 0x40000 bytes
   - min I/O: 0x1000 bytes
   - OOB size: 224 bytes
   - OOB available: 118 bytes
   - ECC strength: 8 bits
   - ECC step size: 512 bytes
   - bitflip threshold: 6 bits
   - 0x000000000000-0x000040000000 : "nand0"
       - 0x000000000000-0x000000200000 : "fsbl"
       - 0x000000200000-0x000000600000 : "fip1"
       - 0x000000600000-0x000000a00000 : "fip2"
       - 0x000000a00000-0x000040000000 : "UBI"
* spi-nor0
   - device: mx66l51235l at 0
   - parent: spi at 58003000
   - driver: jedec_spi_nor
   - path: /soc/spi at 58003000/mx66l51235l at 0
   - type: NOR flash
   - block size: 0x10000 bytes
   - min I/O: 0x1 bytes
   - 0x000000000000-0x000004000000 : "spi-nor0"
* spi-nor1
   - device: mx66l51235l at 1
   - parent: spi at 58003000
   - driver: jedec_spi_nor
   - path: /soc/spi at 58003000/mx66l51235l at 1
   - type: NOR flash
   - block size: 0x10000 bytes
   - min I/O: 0x1 bytes
   - 0x000000000000-0x000004000000 : "spi-nor1"

But I get the error:

STM32MP> mtdparts
Device nor0 not found!

it is linked to get_mtd_info() in cmd/mtdparts.c

=> search for  "nor%d" device associated to MTD_DEV_TYPE_NOR

MTD_DEV_TYPE(type), num

New solution:

to align the device name in cfi and sf part

in cfi_mtd_init(void)

for (i = 0; i < CONFIG_SYS_MAX_FLASH_BANKS; i++) {

...

        sprintf(cfi_mtd_names[i], "nor%d", i);
         mtd->name        = cfi_mtd_names[i];

...

}

so for CFI NOR => "nor0" ..... "norN"

and I update my patch to handle the spi nor AFTER the CFI nor....

based on CONFIG_SYS_MAX_FLASH_BANKS

CFI NOR => "norM" ..... "norZ"

with M = N+1

I will propose it soon.

Patrick