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 <[email protected]>
>>>>> Cc: Heiko Schocher <[email protected]>
>>>>> Cc: Jagan Teki <[email protected]>
>>>>> Cc: Marek Behún <[email protected]>
>>>>> Cc: Miquel Raynal <[email protected]>
>>>>> Cc: Pali Rohár <[email protected]>
>>>>> Cc: Patrice Chotard <[email protected]>
>>>>> Cc: Patrick Delaunay <[email protected]>
>>>>> Cc: Priyanka Jain <[email protected]>
>>>>> Cc: Simon Glass <[email protected]>
>>>>> ---
>>>>> 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 <[email protected]>
>>>>
>>>> @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@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@0
>> - parent: spi@58003000
>> - driver: jedec_spi_nor
>> - path: /soc/spi@58003000/mx66l51235l@0
>> - type: NOR flash
>> - block size: 0x10000 bytes
>> - min I/O: 0x1 bytes
>> - 0x000000000000-0x000004000000 : "nor2"
>> * nor2
>> - device: mx66l51235l@1
>> - parent: spi@58003000
>> - driver: jedec_spi_nor
>> - path: /soc/spi@58003000/mx66l51235l@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".
