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

Wed Sep 15 14:10:35 CEST 2021

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.

[...]