[PATCH] mtd: spi-nor: Fix SF MTDIDS when registering multiple MTDs with DM enabled
Patrick DELAUNAY
patrick.delaunay at foss.st.com
Wed Sep 15 14:00:01 CEST 2021
Hi,
On 9/15/21 11:21 AM, Marek Vasut wrote:
> On 9/15/21 11:17 AM, Patrice CHOTARD wrote:
>>
>>
>> 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.
I need to check if that solve my issue.
In my first proposal, I was afraid of side effect for other user....
In particular for macro MTD_DEV_TYPE()
#define MTD_DEV_TYPE(type) (type == MTD_DEV_TYPE_NAND ? "nand" : \
(type == MTD_DEV_TYPE_NOR ? "nor" : \
(type == MTD_DEV_TYPE_ONENAND ? "onenand" : \
"spi-nand"))) \
But this name is build with in cmd/mtdparts.c:
dev = struct mtd_device
with MTD_DEV_TYPE(dev->id->type), dev->id->num, .....
but the mapping mtd_device for mtd->name is managed by mtdids,
for example, it can be managed by "nor0=spi-nor0"
PS: I think the marek patch failed because the memory "flash->mtd.name"
is not allocated
it is only a pointer...
int spi_flash_mtd_register(struct spi_flash *flash)
{
- return add_mtd_device(&flash->mtd);
+ int ret = add_mtd_device(&flash->mtd);
+ if (!ret)
+ sprintf(flash->mtd.name, "nor%d", flash->mtd.index);
+ return ret;
}
and index is provided by idr_alloc
=> drivers/mtd/mtdcore.c:440
it the index of mtd device for "mtd%d" selection,
it is not the correct index to used I think
(it is not working for mtd0=nand + mtd1= nor => "nand0" + "nor1" )
Patrick
More information about the U-Boot
mailing list