[PATCH] mtd: spi-nor: Fix SF MTDIDS when registering multiple MTDs with DM enabled
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@denx.de Cc: Heiko Schocher hs@denx.de Cc: Jagan Teki jagan@amarulasolutions.com Cc: Marek Behún marek.behun@nic.cz Cc: Miquel Raynal miquel.raynal@bootlin.com Cc: Pali Rohár pali@kernel.org Cc: Patrice Chotard patrice.chotard@foss.st.com Cc: Patrick Delaunay patrick.delaunay@st.com Cc: Priyanka Jain priyanka.jain@nxp.com Cc: Simon Glass sjg@chromium.org --- drivers/mtd/spi/sf_mtd.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/sf_mtd.c b/drivers/mtd/spi/sf_mtd.c index 04de8680809..1a81689fe4a 100644 --- a/drivers/mtd/spi/sf_mtd.c +++ b/drivers/mtd/spi/sf_mtd.c @@ -14,7 +14,10 @@
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; }
void spi_flash_mtd_unregister(struct spi_flash *flash)
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@denx.de Cc: Heiko Schocher hs@denx.de Cc: Jagan Teki jagan@amarulasolutions.com Cc: Marek Behún marek.behun@nic.cz Cc: Miquel Raynal miquel.raynal@bootlin.com Cc: Pali Rohár pali@kernel.org Cc: Patrice Chotard patrice.chotard@foss.st.com Cc: Patrick Delaunay patrick.delaunay@st.com Cc: Priyanka Jain priyanka.jain@nxp.com Cc: Simon Glass sjg@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.I73dae4...
I find your approach cleaner, so:
Acked-by: Heiko Schocher hs@denx.de
@Patrick: Could you test this patch please?
Thanks!
bye, Heiko
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@denx.de Cc: Heiko Schocher hs@denx.de Cc: Jagan Teki jagan@amarulasolutions.com Cc: Marek Behún marek.behun@nic.cz Cc: Miquel Raynal miquel.raynal@bootlin.com Cc: Pali Rohár pali@kernel.org Cc: Patrice Chotard patrice.chotard@foss.st.com Cc: Patrick Delaunay patrick.delaunay@st.com Cc: Priyanka Jain priyanka.jain@nxp.com Cc: Simon Glass sjg@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.I73dae4...
I find your approach cleaner, so:
Acked-by: Heiko Schocher hs@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.
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@denx.de Cc: Heiko Schocher hs@denx.de Cc: Jagan Teki jagan@amarulasolutions.com Cc: Marek Behún marek.behun@nic.cz Cc: Miquel Raynal miquel.raynal@bootlin.com Cc: Pali Rohár pali@kernel.org Cc: Patrice Chotard patrice.chotard@foss.st.com Cc: Patrick Delaunay patrick.delaunay@st.com Cc: Priyanka Jain priyanka.jain@nxp.com Cc: Simon Glass sjg@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.I73dae4...
I find your approach cleaner, so:
Acked-by: Heiko Schocher hs@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@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"
Patrice
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@denx.de Cc: Heiko Schocher hs@denx.de Cc: Jagan Teki jagan@amarulasolutions.com Cc: Marek Behún marek.behun@nic.cz Cc: Miquel Raynal miquel.raynal@bootlin.com Cc: Pali Rohár pali@kernel.org Cc: Patrice Chotard patrice.chotard@foss.st.com Cc: Patrick Delaunay patrick.delaunay@st.com Cc: Priyanka Jain priyanka.jain@nxp.com Cc: Simon Glass sjg@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.I73dae4...
I find your approach cleaner, so:
Acked-by: Heiko Schocher hs@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@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.
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@denx.de Cc: Heiko Schocher hs@denx.de Cc: Jagan Teki jagan@amarulasolutions.com Cc: Marek Behún marek.behun@nic.cz Cc: Miquel Raynal miquel.raynal@bootlin.com Cc: Pali Rohár pali@kernel.org Cc: Patrice Chotard patrice.chotard@foss.st.com Cc: Patrick Delaunay patrick.delaunay@st.com Cc: Priyanka Jain priyanka.jain@nxp.com Cc: Simon Glass sjg@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.I73dae4...
I find your approach cleaner, so:
Acked-by: Heiko Schocher hs@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@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".
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@denx.de Cc: Heiko Schocher hs@denx.de Cc: Jagan Teki jagan@amarulasolutions.com Cc: Marek Behún marek.behun@nic.cz Cc: Miquel Raynal miquel.raynal@bootlin.com Cc: Pali Rohár pali@kernel.org Cc: Patrice Chotard patrice.chotard@foss.st.com Cc: Patrick Delaunay patrick.delaunay@st.com Cc: Priyanka Jain priyanka.jain@nxp.com Cc: Simon Glass sjg@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.I73dae4...
I find your approach cleaner, so:
Acked-by: Heiko Schocher hs@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@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".
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,
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@denx.de > Cc: Heiko Schocher hs@denx.de > Cc: Jagan Teki jagan@amarulasolutions.com > Cc: Marek Behún marek.behun@nic.cz > Cc: Miquel Raynal miquel.raynal@bootlin.com > Cc: Pali Rohár pali@kernel.org > Cc: Patrice Chotard patrice.chotard@foss.st.com > Cc: Patrick Delaunay patrick.delaunay@st.com > Cc: Priyanka Jain priyanka.jain@nxp.com > Cc: Simon Glass sjg@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.I73dae4...
I find your approach cleaner, so:
Acked-by: Heiko Schocher hs@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@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".
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
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@denx.de >> Cc: Heiko Schocher hs@denx.de >> Cc: Jagan Teki jagan@amarulasolutions.com >> Cc: Marek Behún marek.behun@nic.cz >> Cc: Miquel Raynal miquel.raynal@bootlin.com >> Cc: Pali Rohár pali@kernel.org >> Cc: Patrice Chotard patrice.chotard@foss.st.com >> Cc: Patrick Delaunay patrick.delaunay@st.com >> Cc: Priyanka Jain priyanka.jain@nxp.com >> Cc: Simon Glass sjg@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.I73dae4... > > > I find your approach cleaner, so: > > Acked-by: Heiko Schocher hs@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@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".
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.
[...]
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@denx.de >>> Cc: Heiko Schocher hs@denx.de >>> Cc: Jagan Teki jagan@amarulasolutions.com >>> Cc: Marek Behún marek.behun@nic.cz >>> Cc: Miquel Raynal miquel.raynal@bootlin.com >>> Cc: Pali Rohár pali@kernel.org >>> Cc: Patrice Chotard patrice.chotard@foss.st.com >>> Cc: Patrick Delaunay patrick.delaunay@st.com >>> Cc: Priyanka Jain priyanka.jain@nxp.com >>> Cc: Simon Glass sjg@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.I73dae4... >> >> >> I find your approach cleaner, so: >> >> Acked-by: Heiko Schocher hs@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@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".
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@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 : "spi-nor0" * spi-nor1 - 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 : "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
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@denx.de >>>> Cc: Heiko Schocher hs@denx.de >>>> Cc: Jagan Teki jagan@amarulasolutions.com >>>> Cc: Marek Behún marek.behun@nic.cz >>>> Cc: Miquel Raynal miquel.raynal@bootlin.com >>>> Cc: Pali Rohár pali@kernel.org >>>> Cc: Patrice Chotard patrice.chotard@foss.st.com >>>> Cc: Patrick Delaunay patrick.delaunay@st.com >>>> Cc: Priyanka Jain priyanka.jain@nxp.com >>>> Cc: Simon Glass sjg@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.I73dae4... >>> >>> >>> I find your approach cleaner, so: >>> >>> Acked-by: Heiko Schocher hs@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@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".
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@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 : "spi-nor0"
- spi-nor1
- 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 : "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
On Wed, 15 Sep 2021 01:06:22 +0200 Marek Vasut marex@denx.de 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.
CONFIG_MTDIDS_DEFAULT="nor0=47040000.spi.0" is defined for configs/am65x_evm_a53_defconfig and configs/am65x_hs_evm_a53_defconfig
both using device tree arch/arm/dts/k3-am654-base-board.dts
where you have defined DT node flash@0.
Just add partition subnodes, and then you won't need mtdparts.
This can be done for most users of mtdparts, and we won't to convert mtdpart to OF and get rid of it.
Marek
On 9/16/21 5:16 PM, Marek Behún wrote:
On Wed, 15 Sep 2021 01:06:22 +0200 Marek Vasut marex@denx.de 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.
CONFIG_MTDIDS_DEFAULT="nor0=47040000.spi.0" is defined for configs/am65x_evm_a53_defconfig and configs/am65x_hs_evm_a53_defconfig
both using device tree arch/arm/dts/k3-am654-base-board.dts
where you have defined DT node flash@0.
Just add partition subnodes, and then you won't need mtdparts.
This can be done for most users of mtdparts, and we won't to convert mtdpart to OF and get rid of it.
You are assuming that every system is capable of updating either DT or env, that is not always the case.
participants (5)
-
Heiko Schocher -
Marek Behún -
Marek Vasut -
Patrice CHOTARD -
Patrick DELAUNAY