[PATCH] mtd: spi-nor: Fix SF MTDIDS when registering multiple MTDs with DM enabled
Patrick DELAUNAY
patrick.delaunay at foss.st.com
Thu Sep 16 16:06:23 CEST 2021
Hi,
On 9/16/21 9:50 AM, Patrick DELAUNAY wrote:
> 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.
>
>
V3 sent => mtd: spi: nor: force mtd name to "nor%d"
http://patchwork.ozlabs.org/project/uboot/list/?series=262632&state=*
I need to solve some compilation issue when I use
CONFIG_SYS_MAX_FLASH_BANKS
Patrick
More information about the U-Boot
mailing list