[PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Patrick Delaunay]

This serie is a V4 for [1].

Now the SPI nor are named "norN" with N after the CFI nor device:
"nor0" to "norM" => N= M+1.

See also an other proposal from Marek (not working after test)
"mtd: spi-nor: Fix SF MTDIDS when registering multiple MTDs with
DM enabled"

http://patchwork.ozlabs.org/project/uboot/list/?series=262362

The first patch of the serie fixed the compilation issues around
'cfi_flash_num_flash_banks' found in CI:

https://source.denx.de/u-boot/custodians/u-boot-stm/-/pipelines/9138

[1] Series: mtd: spi: nor: force mtd name to "nor%d"
http://patchwork.ozlabs.org/project/uboot/list/?series=262632&state=*
http://patchwork.ozlabs.org/project/uboot/list/?series=262017&state=*
http://patchwork.ozlabs.org/project/uboot/list/?series=262013&state=*

Patrick


Changes in v4:
- introduce macro MTD_NAME_SIZE for mtd_name size and use MTD_DEV_TYPE
  to retrieved the "nor" string.

Changes in v3:
- NEW: solve compilation issue when CONFIG_SYS_MAX_FLASH_BANKS is used
- start index after the last CFI device, use CONFIG_SYS_MAX_FLASH_BANKS

Changes in v2:
- correct commit message

Patrick Delaunay (2):
  mtd: cfi_flash: use cfi_flash_num_flash_banks only when supported
  mtd: spi: nor: force mtd name to "nor%d"

 drivers/mtd/spi/spi-nor-core.c | 17 ++++++++++++++---
 include/dm/device.h            |  3 ++-
 include/linux/mtd/spi-nor.h    |  2 ++
 include/mtd.h                  |  4 ++++
 include/mtd/cfi_flash.h        |  8 +++++++-
 5 files changed, 29 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH v4 1/2] mtd: cfi_flash: use cfi_flash_num_flash_banks only when supported

[Patrick Delaunay]

When CONFIG_SYS_MAX_FLASH_BANKS_DETECT is activated,
CONFIG_SYS_MAX_FLASH_BANKS is replaced by cfi_flash_num_flash_banks,
but this variable is defined in drivers/mtd/cfi_flash.c, which is
compiled only when CONFIG_FLASH_CFI_DRIVER is activated, in U-Boot
or in SPL when CONFIG_SPL_MTD_SUPPORT is activated.

This patch deactivates this feature CONFIG_SYS_MAX_FLASH_BANKS_DETECT
when flash cfi driver is not activated to avoid compilation issue in
the next patch, when CONFIG_SYS_MAX_FLASH_BANKS is used in spi_nor_scan().

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---
see error in
https://source.denx.de/u-boot/custodians/u-boot-stm/-/pipelines/9138

drivers/mtd/spi/spi-nor-core.o: in function `spi_nor_scan':
drivers/mtd/spi/spi-nor-core.c:3672:
    undefined reference to `cfi_flash_num_flash_banks'

compilation issue for the boards:
- j721e_hs_evm_r5
- j721e_evm_r5j
- j721e_hs_evm_a72
- j721e_evm_a72
- sagem_f@st1704_ram

(no changes since v3)

Changes in v3:
- NEW: solve compilation issue when CONFIG_SYS_MAX_FLASH_BANKS is used

 include/mtd/cfi_flash.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/mtd/cfi_flash.h b/include/mtd/cfi_flash.h
index 4963c89642..a1af6fc200 100644
--- a/include/mtd/cfi_flash.h
+++ b/include/mtd/cfi_flash.h
@@ -157,11 +157,17 @@ struct cfi_pri_hdr {
  * Use CONFIG_SYS_MAX_FLASH_BANKS_DETECT if defined
  */
 #if defined(CONFIG_SYS_MAX_FLASH_BANKS_DETECT)
-#define CONFIG_SYS_MAX_FLASH_BANKS	(cfi_flash_num_flash_banks)
 #define CFI_MAX_FLASH_BANKS	CONFIG_SYS_MAX_FLASH_BANKS_DETECT
+/* map to cfi_flash_num_flash_banks only when supported */
+#if IS_ENABLED(CONFIG_FLASH_CFI_DRIVER) && \
+    (!IS_ENABLED(CONFIG_SPL_BUILD) || IS_ENABLED(CONFIG_SPL_MTD_SUPPORT))
+#define CONFIG_SYS_MAX_FLASH_BANKS	(cfi_flash_num_flash_banks)
 /* board code can update this variable before CFI detection */
 extern int cfi_flash_num_flash_banks;
 #else
+#define CONFIG_SYS_MAX_FLASH_BANKS	CONFIG_SYS_MAX_FLASH_BANKS_DETECT
+#endif
+#else
 #define CFI_MAX_FLASH_BANKS	CONFIG_SYS_MAX_FLASH_BANKS
 #endif
 
-- 
2.25.1

[PATCH v4 2/2] mtd: spi: nor: force mtd name to "nor%d"

[Patrick Delaunay]

Force the mtd name of spi-nor to "nor" + the driver sequence number:
"nor0", "nor1"... beginning after the existing nor devices.

This patch is coherent with existing "nand" and "spi-nand"
mtd device names.

When CFI MTD NOR device are supported, the spi-nor index is chosen after
the last CFI device defined by CONFIG_SYS_MAX_FLASH_BANKS.

When CONFIG_SYS_MAX_FLASH_BANKS_DETECT is activated, this config
is replaced by to cfi_flash_num_flash_banks in the include file
mtd/cfi_flash.h.

This generic name "nor%d" can be use to identify the mtd spi-nor device
without knowing the real device name or the DT path of the device,
used with API get_mtd_device_nm() and is used in mtdparts command.

This patch also avoids issue when the same NOR device is present 2 times,
for example on STM32MP15F-EV1:

STM32MP> mtd list
SF: Detected mx66l51235l 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"
* mx66l51235l
  - 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 : "mx66l51235l"
* mx66l51235l
  - 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 : "mx66l51235l"

The same mtd name "mx66l51235l" identify the 2 instances
mx66l51235l@0 and mx66l51235l@1.

This patch fixes a ST32CubeProgrammer / stm32prog command issue
with nor0 target on STM32MP157C-EV1 board introduced by
commit b7f060565e31 ("mtd: spi-nor: allow registering multiple MTDs when
DM is enabled").

Fixes: b7f060565e31 ("mtd: spi-nor: allow registering multiple MTDs when DM is enabled")
Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---
For other device, the mtd name are hardcoded in each MTD driver:

drivers/mtd/nand/spi/core.c:1169:
        sprintf(mtd->name, "spi-nand%d", spi_nand_idx++);
drivers/mtd/nand/raw/nand.c:59:
        sprintf(dev_name[devnum], "nand%d", devnum);

And the nor device name "nor%d" is also used for CFI in
./drivers/mtd/cfi_mtd.c with i=0 to CONFIG_SYS_MAX_FLASH_BANKS - 1

        sprintf(cfi_mtd_names[i], "nor%d", i);
        mtd->name               = cfi_mtd_names[i];

Today the number of CFI device is hardcoded by this config.


Changes in v4:
- introduce macro MTD_NAME_SIZE for mtd_name size and use MTD_DEV_TYPE
  to retrieved the "nor" string.

Changes in v3:
- start index after the last CFI device, use CONFIG_SYS_MAX_FLASH_BANKS

Changes in v2:
- correct commit message

 drivers/mtd/spi/spi-nor-core.c | 17 ++++++++++++++---
 include/dm/device.h            |  3 ++-
 include/linux/mtd/spi-nor.h    |  2 ++
 include/mtd.h                  |  4 ++++
 4 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index d5d905fa5a..f1b4e5ea8e 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -10,6 +10,7 @@
  */
 
 #include <common.h>
+#include <flash.h>
 #include <log.h>
 #include <watchdog.h>
 #include <dm.h>
@@ -26,6 +27,7 @@
 
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/spi-nor.h>
+#include <mtd/cfi_flash.h>
 #include <spi-mem.h>
 #include <spi.h>
 
@@ -3664,6 +3666,11 @@ int spi_nor_scan(struct spi_nor *nor)
 	struct mtd_info *mtd = &nor->mtd;
 	struct spi_slave *spi = nor->spi;
 	int ret;
+	int cfi_mtd_nb = 0;
+
+#ifdef CONFIG_SYS_MAX_FLASH_BANKS
+	cfi_mtd_nb = CONFIG_SYS_MAX_FLASH_BANKS;
+#endif
 
 	/* Reset SPI protocol for all commands. */
 	nor->reg_proto = SNOR_PROTO_1_1_1;
@@ -3715,8 +3722,12 @@ int spi_nor_scan(struct spi_nor *nor)
 	if (ret)
 		return ret;
 
-	if (!mtd->name)
-		mtd->name = info->name;
+	if (!mtd->name) {
+		sprintf(nor->mtd_name, "%s%d",
+			MTD_DEV_TYPE(MTD_DEV_TYPE_NOR),
+			cfi_mtd_nb + dev_seq(nor->dev));
+		mtd->name = nor->mtd_name;
+	}
 	mtd->dev = nor->dev;
 	mtd->priv = nor;
 	mtd->type = MTD_NORFLASH;
@@ -3821,7 +3832,7 @@ int spi_nor_scan(struct spi_nor *nor)
 
 	nor->rdsr_dummy = params.rdsr_dummy;
 	nor->rdsr_addr_nbytes = params.rdsr_addr_nbytes;
-	nor->name = mtd->name;
+	nor->name = info->name;
 	nor->size = mtd->size;
 	nor->erase_size = mtd->erasesize;
 	nor->sector_size = mtd->erasesize;
diff --git a/include/dm/device.h b/include/dm/device.h
index 0a9718a5b8..b1d8da747a 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -207,8 +207,9 @@ struct udevice_rt {
 	u32 flags_;
 };
 
-/* Maximum sequence number supported */
+/* Maximum sequence number supported and associated string lenght */
 #define DM_MAX_SEQ	999
+#define DM_MAX_SEQ_STR	3
 
 /* Returns the operations for a device */
 #define device_get_ops(dev)	(dev->driver->ops)
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 7ddc4ba2bf..4ceeae623d 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -7,6 +7,7 @@
 #ifndef __LINUX_MTD_SPI_NOR_H
 #define __LINUX_MTD_SPI_NOR_H
 
+#include <mtd.h>
 #include <linux/bitops.h>
 #include <linux/mtd/cfi.h>
 #include <linux/mtd/mtd.h>
@@ -561,6 +562,7 @@ struct spi_nor {
 	int (*ready)(struct spi_nor *nor);
 
 	void *priv;
+	char mtd_name[MTD_NAME_SIZE(MTD_DEV_TYPE_NOR)];
 /* Compatibility for spi_flash, remove once sf layer is merged with mtd */
 	const char *name;
 	u32 size;
diff --git a/include/mtd.h b/include/mtd.h
index b569331edb..5006137836 100644
--- a/include/mtd.h
+++ b/include/mtd.h
@@ -6,10 +6,14 @@
 #ifndef _MTD_H_
 #define _MTD_H_
 
+#include <jffs2/load_kernel.h>
 #include <linux/mtd/mtd.h>
 
 int mtd_probe_devices(void);
 
 void board_mtdparts_default(const char **mtdids, const char **mtdparts);
 
+/* compute the max size for the string associated to a dev type */
+#define MTD_NAME_SIZE(type) (sizeof(MTD_DEV_TYPE(type)) +  DM_MAX_SEQ_STR)
+
 #endif	/* _MTD_H_ */
-- 
2.25.1

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Marek Behún]

(Adding also Tom.)

Hi Patrick, Marek,

I find this either not complete or not needed:

- either you need mtd names to be of this format so that old MTDPARTS
  config definitions do not need to be changed, i.e. something like
    CONFIG_MTDPARTS_DEFAULT="nor0:1M(u-boot),0x1000@0xfff000(env)"
  does not work currently, and you want to make it work.

  I find your solution here incomplete because MTDPARTS can also be
  used to be passed to Linux as mtdparts parameter, but there is no
  guarantee that the "norN" numbering you are creating in U-Boot will
  be the same as the one in kernel.

- or it is not needed, because you can remove MTDPARTS definition from
  the board config entirely and move the information into device tree. 
  In fact this was the main idea behind making the series
    Support SPI NORs and OF partitions in `mtd list`
  The SPI-NOR MTDs after this series can have conflicting names,
  because you can still choose between them via OF path with the `mtd`
  command.

  Tom and I were of the opinion that MTDPARTS should be deprecated and
  removed in favor of OF. Marek Vasut says that this is not possible
  for every board, and so needs to stay.

BTW, I find it a little weird for Marek to defend old API which should
be converted to DT, when in discussion about DM USB / Nokia N900
USB TTY console [1] he was defending the opinion that we should be
heading to DT in U-Boot.

[1]
https://patchwork.ozlabs.org/project/uboot/patch/20210618145724.2558-1-pali@kernel.org/

On Wed, 22 Sep 2021 18:29:06 +0200
Patrick Delaunay <patrick.delaunay@foss.st.com> wrote:

> This serie is a V4 for [1].
> 
> Now the SPI nor are named "norN" with N after the CFI nor device:
> "nor0" to "norM" => N= M+1.
> 
> See also an other proposal from Marek (not working after test)
> "mtd: spi-nor: Fix SF MTDIDS when registering multiple MTDs with
> DM enabled"
> 
> http://patchwork.ozlabs.org/project/uboot/list/?series=262362
> 
> The first patch of the serie fixed the compilation issues around
> 'cfi_flash_num_flash_banks' found in CI:
> 
> https://source.denx.de/u-boot/custodians/u-boot-stm/-/pipelines/9138
> 
> [1] Series: mtd: spi: nor: force mtd name to "nor%d"
> http://patchwork.ozlabs.org/project/uboot/list/?series=262632&state=*
> http://patchwork.ozlabs.org/project/uboot/list/?series=262017&state=*
> http://patchwork.ozlabs.org/project/uboot/list/?series=262013&state=*
> 
> Patrick
> 
> 
> Changes in v4:
> - introduce macro MTD_NAME_SIZE for mtd_name size and use MTD_DEV_TYPE
>   to retrieved the "nor" string.
> 
> Changes in v3:
> - NEW: solve compilation issue when CONFIG_SYS_MAX_FLASH_BANKS is used
> - start index after the last CFI device, use CONFIG_SYS_MAX_FLASH_BANKS
> 
> Changes in v2:
> - correct commit message
> 
> Patrick Delaunay (2):
>   mtd: cfi_flash: use cfi_flash_num_flash_banks only when supported
>   mtd: spi: nor: force mtd name to "nor%d"
> 
>  drivers/mtd/spi/spi-nor-core.c | 17 ++++++++++++++---
>  include/dm/device.h            |  3 ++-
>  include/linux/mtd/spi-nor.h    |  2 ++
>  include/mtd.h                  |  4 ++++
>  include/mtd/cfi_flash.h        |  8 +++++++-
>  5 files changed, 29 insertions(+), 5 deletions(-)
> 

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Marek Vasut]

On 9/22/21 7:29 PM, Marek Behún wrote:
> (Adding also Tom.)
> 
> Hi Patrick, Marek,
> 
> I find this either not complete or not needed:
> 
> - either you need mtd names to be of this format so that old MTDPARTS
>    config definitions do not need to be changed, i.e. something like
>      CONFIG_MTDPARTS_DEFAULT="nor0:1M(u-boot),0x1000@0xfff000(env)"
>    does not work currently, and you want to make it work.
> 
>    I find your solution here incomplete because MTDPARTS can also be
>    used to be passed to Linux as mtdparts parameter, but there is no
>    guarantee that the "norN" numbering you are creating in U-Boot will
>    be the same as the one in kernel.
> 
> - or it is not needed, because you can remove MTDPARTS definition from
>    the board config entirely and move the information into device tree.
>    In fact this was the main idea behind making the series
>      Support SPI NORs and OF partitions in `mtd list`
>    The SPI-NOR MTDs after this series can have conflicting names,
>    because you can still choose between them via OF path with the `mtd`
>    command.
> 
>    Tom and I were of the opinion that MTDPARTS should be deprecated and
>    removed in favor of OF. Marek Vasut says that this is not possible
>    for every board, and so needs to stay.
> 
> BTW, I find it a little weird for Marek to defend old API which should
> be converted to DT, when in discussion about DM USB / Nokia N900
> USB TTY console [1] he was defending the opinion that we should be
> heading to DT in U-Boot.
> 
> [1]
> https://patchwork.ozlabs.org/project/uboot/patch/20210618145724.2558-1-pali@kernel.org/

That USB discussion is completely unrelated to the problem here, the USB 
discussion is about internal (i.e. not user facing) conversion to DM/DT. 
The user-facing ABI does not change there. Also, that discussion was 
about patching USB stack to permit new non-DM/DT operation, not fixing 
existing one.

This problem here is user facing ABI, the mtdparts/mtdids. That user 
facing ABI got broken. Boards which do depend on it, even those 
currently in tree, are broken. Not all boards can update their 
environment, so some backward compatibility of the user facing ABI 
should be in place, even though it might not be to the degree Linux 
kernel does so. So far, it seems most of the U-Boot command line 
interface has managed to retain backward compatibility, I don't see why 
this here should be handled any differently.

Note that there are not just a few boards that are broken, but hundreds. 
I believe that itself justifies a fix, instead of just throwing all 
those hundreds of boards overboard.

u-boot$ git grep -l CONFIG_MTDIDS configs | wc -l
203

Hopefully that clarifies the difference.

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4280 bytes --]

On Wed, Sep 22, 2021 at 08:24:18PM +0200, Marek Vasut wrote:
> On 9/22/21 7:29 PM, Marek Behún wrote:
> > (Adding also Tom.)
> > 
> > Hi Patrick, Marek,
> > 
> > I find this either not complete or not needed:
> > 
> > - either you need mtd names to be of this format so that old MTDPARTS
> >    config definitions do not need to be changed, i.e. something like
> >      CONFIG_MTDPARTS_DEFAULT="nor0:1M(u-boot),0x1000@0xfff000(env)"
> >    does not work currently, and you want to make it work.
> > 
> >    I find your solution here incomplete because MTDPARTS can also be
> >    used to be passed to Linux as mtdparts parameter, but there is no
> >    guarantee that the "norN" numbering you are creating in U-Boot will
> >    be the same as the one in kernel.
> > 
> > - or it is not needed, because you can remove MTDPARTS definition from
> >    the board config entirely and move the information into device tree.
> >    In fact this was the main idea behind making the series
> >      Support SPI NORs and OF partitions in `mtd list`
> >    The SPI-NOR MTDs after this series can have conflicting names,
> >    because you can still choose between them via OF path with the `mtd`
> >    command.
> > 
> >    Tom and I were of the opinion that MTDPARTS should be deprecated and
> >    removed in favor of OF. Marek Vasut says that this is not possible
> >    for every board, and so needs to stay.
> > 
> > BTW, I find it a little weird for Marek to defend old API which should
> > be converted to DT, when in discussion about DM USB / Nokia N900
> > USB TTY console [1] he was defending the opinion that we should be
> > heading to DT in U-Boot.
> > 
> > [1]
> > https://patchwork.ozlabs.org/project/uboot/patch/20210618145724.2558-1-pali@kernel.org/
> 
> That USB discussion is completely unrelated to the problem here, the USB
> discussion is about internal (i.e. not user facing) conversion to DM/DT. The
> user-facing ABI does not change there. Also, that discussion was about
> patching USB stack to permit new non-DM/DT operation, not fixing existing
> one.
> 
> This problem here is user facing ABI, the mtdparts/mtdids. That user facing
> ABI got broken. Boards which do depend on it, even those currently in tree,
> are broken. Not all boards can update their environment, so some backward
> compatibility of the user facing ABI should be in place, even though it
> might not be to the degree Linux kernel does so. So far, it seems most of
> the U-Boot command line interface has managed to retain backward
> compatibility, I don't see why this here should be handled any differently.
> 
> Note that there are not just a few boards that are broken, but hundreds. I
> believe that itself justifies a fix, instead of just throwing all those
> hundreds of boards overboard.
> 
> u-boot$ git grep -l CONFIG_MTDIDS configs | wc -l
> 203
> 
> Hopefully that clarifies the difference.

It doesn't quite, sorry.  If you have "mtdparts=... mtdids=..." in your
cmdline that you pass to Linux, U-Boot doesn't care.  That's one of the
main users of CONFIG_MTDIDS/MTDPARTS today, which could in some good
number of cases be removed (take am335x_evm_defconfig for example, the
table has been defined in the upstream DT for forever).  Taking a look
at:
commit 938db6fe5da3581ed2c17d64c7e016a7a8df5237
Author: Miquel Raynal <miquel.raynal@bootlin.com>
Date:   Sat Sep 29 12:58:30 2018 +0200

    cmd: mtdparts: describe as legacy
    
    The 'mtdparts' command is not needed anymore. While the environment
    variable is still valid (and useful, along with the 'mtdids' one), the
    command has been replaced by 'mtd' which is much more close to the MTD
    stack and do not add its own specific glue.
    
    Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
    Reviewed-by: Stefan Roese <sr@denx.de>
    Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>

Is when "mtdparts" in U-Boot was noted as legacy.  So what exactly are
we fixing with this series?  Nothing changed about hard-coded values
being passed along.  What may have broken was some progmatic way to set
those, but I think that's both fragile and deprecated in favor of the
table being in the DT.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Marek Behún]

On Wed, 22 Sep 2021 20:24:18 +0200
Marek Vasut <marex@denx.de> wrote:

> On 9/22/21 7:29 PM, Marek Behún wrote:
> > (Adding also Tom.)
> > 
> > Hi Patrick, Marek,
> > 
> > I find this either not complete or not needed:
> > 
> > - either you need mtd names to be of this format so that old MTDPARTS
> >    config definitions do not need to be changed, i.e. something like
> >      CONFIG_MTDPARTS_DEFAULT="nor0:1M(u-boot),0x1000@0xfff000(env)"
> >    does not work currently, and you want to make it work.
> > 
> >    I find your solution here incomplete because MTDPARTS can also be
> >    used to be passed to Linux as mtdparts parameter, but there is no
> >    guarantee that the "norN" numbering you are creating in U-Boot will
> >    be the same as the one in kernel.
> > 
> > - or it is not needed, because you can remove MTDPARTS definition from
> >    the board config entirely and move the information into device tree.
> >    In fact this was the main idea behind making the series
> >      Support SPI NORs and OF partitions in `mtd list`
> >    The SPI-NOR MTDs after this series can have conflicting names,
> >    because you can still choose between them via OF path with the `mtd`
> >    command.
> > 
> >    Tom and I were of the opinion that MTDPARTS should be deprecated and
> >    removed in favor of OF. Marek Vasut says that this is not possible
> >    for every board, and so needs to stay.
> > 
> > BTW, I find it a little weird for Marek to defend old API which should
> > be converted to DT, when in discussion about DM USB / Nokia N900
> > USB TTY console [1] he was defending the opinion that we should be
> > heading to DT in U-Boot.
> > 
> > [1]
> > https://patchwork.ozlabs.org/project/uboot/patch/20210618145724.2558-1-pali@kernel.org/  
> 
> That USB discussion is completely unrelated to the problem here, the USB 
> discussion is about internal (i.e. not user facing) conversion to DM/DT. 
> The user-facing ABI does not change there. Also, that discussion was 
> about patching USB stack to permit new non-DM/DT operation, not fixing 
> existing one.

This is not only about the user ABI (altough now I agree that you are
correct there, see below). What I meant is this:
  Should we push for converting to device-tree even if for some boards
  it is not possible, and would mean removing them?

  Because you are saying that MTDPARTS cannot be converted to DT for
  some boards.

  But N900 also cannot be reasonably converted because of space
  issues, as far as I understood. Yes, it has gigabytes of eMMC storage,
  and it was proposed to put SPL in MTD and U-Boot proper into eMMC on
  VFAT/ext4, but this simply cannot be done reasonably, because:
  - it would break Linux userspace (existing OS upgrade system would
    have to be rewritten and backwords compatibility would be broken)
  - it would make bootstrapping (booting newer version of U-Boot) while
    developing U-Boot a pain in the ass or maybe even impossible
  - I beleive there was some other reason Pali mentioned, but I cannot
    remember anymore

> This problem here is user facing ABI, the mtdparts/mtdids. That user 
> facing ABI got broken. Boards which do depend on it, even those 
> currently in tree, are broken. Not all boards can update their 
> environment, so some backward compatibility of the user facing ABI 
> should be in place, even though it might not be to the degree Linux 
> kernel does so. So far, it seems most of the U-Boot command line 
> interface has managed to retain backward compatibility, I don't see why 
> this here should be handled any differently.

OK, I get that the if `mtd nor0` was working before, it should work also
now. But the conversion from MTDPARTS to device tree could be probably
done for lots of these, see below.

> Note that there are not just a few boards that are broken, but hundreds. 
> I believe that itself justifies a fix, instead of just throwing all 
> those hundreds of boards overboard.
> 
> u-boot$ git grep -l CONFIG_MTDIDS configs | wc -l
> 203

Only 96 of those also grep the substring "nor". But okay, that is still
a lot. The question is how many of them could be rewritten to DT:

  for cfg in $(git grep -l 'CONFIG_MTDIDS.*nor[0-9]' configs); do
    fgrep CONFIG_DEFAULT_DEVICE_TREE "$cfg"
  done | wc -l

92 of those 96 have CONFIG_DEFAULT_DEVICE_TREE defined.

Of these, 65 contain CONFIG_DM_SPI_FLASH=y, so at least these 65 could
be converted. Of the rest 27, how many could also be converted to DM?
How may use non-DM drivers?

Marek

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Marek Behún]

On Wed, 22 Sep 2021 14:42:30 -0400
Tom Rini <trini@konsulko.com> wrote:

> On Wed, Sep 22, 2021 at 08:24:18PM +0200, Marek Vasut wrote:
> > On 9/22/21 7:29 PM, Marek Behún wrote:  
> > > (Adding also Tom.)
> > > 
> > > Hi Patrick, Marek,
> > > 
> > > I find this either not complete or not needed:
> > > 
> > > - either you need mtd names to be of this format so that old MTDPARTS
> > >    config definitions do not need to be changed, i.e. something like
> > >      CONFIG_MTDPARTS_DEFAULT="nor0:1M(u-boot),0x1000@0xfff000(env)"
> > >    does not work currently, and you want to make it work.
> > > 
> > >    I find your solution here incomplete because MTDPARTS can also be
> > >    used to be passed to Linux as mtdparts parameter, but there is no
> > >    guarantee that the "norN" numbering you are creating in U-Boot will
> > >    be the same as the one in kernel.
> > > 
> > > - or it is not needed, because you can remove MTDPARTS definition from
> > >    the board config entirely and move the information into device tree.
> > >    In fact this was the main idea behind making the series
> > >      Support SPI NORs and OF partitions in `mtd list`
> > >    The SPI-NOR MTDs after this series can have conflicting names,
> > >    because you can still choose between them via OF path with the `mtd`
> > >    command.
> > > 
> > >    Tom and I were of the opinion that MTDPARTS should be deprecated and
> > >    removed in favor of OF. Marek Vasut says that this is not possible
> > >    for every board, and so needs to stay.
> > > 
> > > BTW, I find it a little weird for Marek to defend old API which should
> > > be converted to DT, when in discussion about DM USB / Nokia N900
> > > USB TTY console [1] he was defending the opinion that we should be
> > > heading to DT in U-Boot.
> > > 
> > > [1]
> > > https://patchwork.ozlabs.org/project/uboot/patch/20210618145724.2558-1-pali@kernel.org/  
> > 
> > That USB discussion is completely unrelated to the problem here, the USB
> > discussion is about internal (i.e. not user facing) conversion to DM/DT. The
> > user-facing ABI does not change there. Also, that discussion was about
> > patching USB stack to permit new non-DM/DT operation, not fixing existing
> > one.
> > 
> > This problem here is user facing ABI, the mtdparts/mtdids. That user facing
> > ABI got broken. Boards which do depend on it, even those currently in tree,
> > are broken. Not all boards can update their environment, so some backward
> > compatibility of the user facing ABI should be in place, even though it
> > might not be to the degree Linux kernel does so. So far, it seems most of
> > the U-Boot command line interface has managed to retain backward
> > compatibility, I don't see why this here should be handled any differently.
> > 
> > Note that there are not just a few boards that are broken, but hundreds. I
> > believe that itself justifies a fix, instead of just throwing all those
> > hundreds of boards overboard.
> > 
> > u-boot$ git grep -l CONFIG_MTDIDS configs | wc -l
> > 203
> > 
> > Hopefully that clarifies the difference.  
> 
> It doesn't quite, sorry.  If you have "mtdparts=... mtdids=..." in your
> cmdline that you pass to Linux, U-Boot doesn't care.  That's one of the
> main users of CONFIG_MTDIDS/MTDPARTS today, which could in some good
> number of cases be removed (take am335x_evm_defconfig for example, the
> table has been defined in the upstream DT for forever).  Taking a look
> at:
> commit 938db6fe5da3581ed2c17d64c7e016a7a8df5237
> Author: Miquel Raynal <miquel.raynal@bootlin.com>
> Date:   Sat Sep 29 12:58:30 2018 +0200
> 
>     cmd: mtdparts: describe as legacy
>     
>     The 'mtdparts' command is not needed anymore. While the environment
>     variable is still valid (and useful, along with the 'mtdids' one), the
>     command has been replaced by 'mtd' which is much more close to the MTD
>     stack and do not add its own specific glue.
>     
>     Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>     Reviewed-by: Stefan Roese <sr@denx.de>
>     Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> 
> Is when "mtdparts" in U-Boot was noted as legacy.  So what exactly are
> we fixing with this series?  Nothing changed about hard-coded values
> being passed along.  What may have broken was some progmatic way to set
> those, but I think that's both fragile and deprecated in favor of the
> table being in the DT.
> 

We may be fixing user scripts hardcoded with something like
  mtd read nor0

The question is how many users use something like this, and how many
just need the norN names for mtdparts.

Marek

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Marek Vasut]

On 9/22/21 8:42 PM, Tom Rini wrote:
> On Wed, Sep 22, 2021 at 08:24:18PM +0200, Marek Vasut wrote:
>> On 9/22/21 7:29 PM, Marek Behún wrote:
>>> (Adding also Tom.)
>>>
>>> Hi Patrick, Marek,
>>>
>>> I find this either not complete or not needed:
>>>
>>> - either you need mtd names to be of this format so that old MTDPARTS
>>>     config definitions do not need to be changed, i.e. something like
>>>       CONFIG_MTDPARTS_DEFAULT="nor0:1M(u-boot),0x1000@0xfff000(env)"
>>>     does not work currently, and you want to make it work.
>>>
>>>     I find your solution here incomplete because MTDPARTS can also be
>>>     used to be passed to Linux as mtdparts parameter, but there is no
>>>     guarantee that the "norN" numbering you are creating in U-Boot will
>>>     be the same as the one in kernel.
>>>
>>> - or it is not needed, because you can remove MTDPARTS definition from
>>>     the board config entirely and move the information into device tree.
>>>     In fact this was the main idea behind making the series
>>>       Support SPI NORs and OF partitions in `mtd list`
>>>     The SPI-NOR MTDs after this series can have conflicting names,
>>>     because you can still choose between them via OF path with the `mtd`
>>>     command.
>>>
>>>     Tom and I were of the opinion that MTDPARTS should be deprecated and
>>>     removed in favor of OF. Marek Vasut says that this is not possible
>>>     for every board, and so needs to stay.
>>>
>>> BTW, I find it a little weird for Marek to defend old API which should
>>> be converted to DT, when in discussion about DM USB / Nokia N900
>>> USB TTY console [1] he was defending the opinion that we should be
>>> heading to DT in U-Boot.
>>>
>>> [1]
>>> https://patchwork.ozlabs.org/project/uboot/patch/20210618145724.2558-1-pali@kernel.org/
>>
>> That USB discussion is completely unrelated to the problem here, the USB
>> discussion is about internal (i.e. not user facing) conversion to DM/DT. The
>> user-facing ABI does not change there. Also, that discussion was about
>> patching USB stack to permit new non-DM/DT operation, not fixing existing
>> one.
>>
>> This problem here is user facing ABI, the mtdparts/mtdids. That user facing
>> ABI got broken. Boards which do depend on it, even those currently in tree,
>> are broken. Not all boards can update their environment, so some backward
>> compatibility of the user facing ABI should be in place, even though it
>> might not be to the degree Linux kernel does so. So far, it seems most of
>> the U-Boot command line interface has managed to retain backward
>> compatibility, I don't see why this here should be handled any differently.
>>
>> Note that there are not just a few boards that are broken, but hundreds. I
>> believe that itself justifies a fix, instead of just throwing all those
>> hundreds of boards overboard.
>>
>> u-boot$ git grep -l CONFIG_MTDIDS configs | wc -l
>> 203
>>
>> Hopefully that clarifies the difference.
> 
> It doesn't quite, sorry.  If you have "mtdparts=... mtdids=..." in your
> cmdline that you pass to Linux, U-Boot doesn't care.

The MTDIDS is used by UBI code in U-Boot to locate from which MTD device 
to attach UBI. That is currently broken at least for UBI on SPI NOR or 
parallel NOR. This has nothing to do with passing mtdparts/mtdids to Linux.

> That's one of the
> main users of CONFIG_MTDIDS/MTDPARTS today, which could in some good
> number of cases be removed (take am335x_evm_defconfig for example, the
> table has been defined in the upstream DT for forever).  Taking a look
> at:
> commit 938db6fe5da3581ed2c17d64c7e016a7a8df5237
> Author: Miquel Raynal <miquel.raynal@bootlin.com>
> Date:   Sat Sep 29 12:58:30 2018 +0200
> 
>      cmd: mtdparts: describe as legacy
>      
>      The 'mtdparts' command is not needed anymore. While the environment
>      variable is still valid (and useful, along with the 'mtdids' one), the
>      command has been replaced by 'mtd' which is much more close to the MTD
>      stack and do not add its own specific glue.
>      
>      Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>      Reviewed-by: Stefan Roese <sr@denx.de>
>      Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> 
> Is when "mtdparts" in U-Boot was noted as legacy.  So what exactly are
> we fixing with this series?  Nothing changed about hard-coded values
> being passed along.  What may have broken was some progmatic way to set
> those, but I think that's both fragile and deprecated in favor of the
> table being in the DT.

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5207 bytes --]

On Wed, Sep 22, 2021 at 09:05:36PM +0200, Marek Behún wrote:
> On Wed, 22 Sep 2021 20:24:18 +0200
> Marek Vasut <marex@denx.de> wrote:
> 
> > On 9/22/21 7:29 PM, Marek Behún wrote:
> > > (Adding also Tom.)
> > > 
> > > Hi Patrick, Marek,
> > > 
> > > I find this either not complete or not needed:
> > > 
> > > - either you need mtd names to be of this format so that old MTDPARTS
> > >    config definitions do not need to be changed, i.e. something like
> > >      CONFIG_MTDPARTS_DEFAULT="nor0:1M(u-boot),0x1000@0xfff000(env)"
> > >    does not work currently, and you want to make it work.
> > > 
> > >    I find your solution here incomplete because MTDPARTS can also be
> > >    used to be passed to Linux as mtdparts parameter, but there is no
> > >    guarantee that the "norN" numbering you are creating in U-Boot will
> > >    be the same as the one in kernel.
> > > 
> > > - or it is not needed, because you can remove MTDPARTS definition from
> > >    the board config entirely and move the information into device tree.
> > >    In fact this was the main idea behind making the series
> > >      Support SPI NORs and OF partitions in `mtd list`
> > >    The SPI-NOR MTDs after this series can have conflicting names,
> > >    because you can still choose between them via OF path with the `mtd`
> > >    command.
> > > 
> > >    Tom and I were of the opinion that MTDPARTS should be deprecated and
> > >    removed in favor of OF. Marek Vasut says that this is not possible
> > >    for every board, and so needs to stay.
> > > 
> > > BTW, I find it a little weird for Marek to defend old API which should
> > > be converted to DT, when in discussion about DM USB / Nokia N900
> > > USB TTY console [1] he was defending the opinion that we should be
> > > heading to DT in U-Boot.
> > > 
> > > [1]
> > > https://patchwork.ozlabs.org/project/uboot/patch/20210618145724.2558-1-pali@kernel.org/  
> > 
> > That USB discussion is completely unrelated to the problem here, the USB 
> > discussion is about internal (i.e. not user facing) conversion to DM/DT. 
> > The user-facing ABI does not change there. Also, that discussion was 
> > about patching USB stack to permit new non-DM/DT operation, not fixing 
> > existing one.
> 
> This is not only about the user ABI (altough now I agree that you are
> correct there, see below). What I meant is this:
>   Should we push for converting to device-tree even if for some boards
>   it is not possible, and would mean removing them?
> 
>   Because you are saying that MTDPARTS cannot be converted to DT for
>   some boards.
> 
>   But N900 also cannot be reasonably converted because of space
>   issues, as far as I understood. Yes, it has gigabytes of eMMC storage,
>   and it was proposed to put SPL in MTD and U-Boot proper into eMMC on
>   VFAT/ext4, but this simply cannot be done reasonably, because:
>   - it would break Linux userspace (existing OS upgrade system would
>     have to be rewritten and backwords compatibility would be broken)
>   - it would make bootstrapping (booting newer version of U-Boot) while
>     developing U-Boot a pain in the ass or maybe even impossible
>   - I beleive there was some other reason Pali mentioned, but I cannot
>     remember anymore
> 
> > This problem here is user facing ABI, the mtdparts/mtdids. That user 
> > facing ABI got broken. Boards which do depend on it, even those 
> > currently in tree, are broken. Not all boards can update their 
> > environment, so some backward compatibility of the user facing ABI 
> > should be in place, even though it might not be to the degree Linux 
> > kernel does so. So far, it seems most of the U-Boot command line 
> > interface has managed to retain backward compatibility, I don't see why 
> > this here should be handled any differently.
> 
> OK, I get that the if `mtd nor0` was working before, it should work also
> now. But the conversion from MTDPARTS to device tree could be probably
> done for lots of these, see below.
> 
> > Note that there are not just a few boards that are broken, but hundreds. 
> > I believe that itself justifies a fix, instead of just throwing all 
> > those hundreds of boards overboard.
> > 
> > u-boot$ git grep -l CONFIG_MTDIDS configs | wc -l
> > 203
> 
> Only 96 of those also grep the substring "nor". But okay, that is still
> a lot. The question is how many of them could be rewritten to DT:
> 
>   for cfg in $(git grep -l 'CONFIG_MTDIDS.*nor[0-9]' configs); do
>     fgrep CONFIG_DEFAULT_DEVICE_TREE "$cfg"
>   done | wc -l
> 
> 92 of those 96 have CONFIG_DEFAULT_DEVICE_TREE defined.
> 
> Of these, 65 contain CONFIG_DM_SPI_FLASH=y, so at least these 65 could
> be converted. Of the rest 27, how many could also be converted to DM?
> How may use non-DM drivers?

I was thinking maybe we have problems with the platforms that "mtdparts
default", of which we have a handful and most of that handful also do it
to then make use of the partition table within U-Boot (dfu, or update
the on-flash U-Boot).  Of those, it might make most sense to poke the
maintainer directly on how to proceed.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Marek Vasut]

On 9/22/21 9:05 PM, Marek Behún wrote:

Hi,

[...]

>>> I find this either not complete or not needed:
>>>
>>> - either you need mtd names to be of this format so that old MTDPARTS
>>>     config definitions do not need to be changed, i.e. something like
>>>       CONFIG_MTDPARTS_DEFAULT="nor0:1M(u-boot),0x1000@0xfff000(env)"
>>>     does not work currently, and you want to make it work.
>>>
>>>     I find your solution here incomplete because MTDPARTS can also be
>>>     used to be passed to Linux as mtdparts parameter, but there is no
>>>     guarantee that the "norN" numbering you are creating in U-Boot will
>>>     be the same as the one in kernel.
>>>
>>> - or it is not needed, because you can remove MTDPARTS definition from
>>>     the board config entirely and move the information into device tree.
>>>     In fact this was the main idea behind making the series
>>>       Support SPI NORs and OF partitions in `mtd list`
>>>     The SPI-NOR MTDs after this series can have conflicting names,
>>>     because you can still choose between them via OF path with the `mtd`
>>>     command.
>>>
>>>     Tom and I were of the opinion that MTDPARTS should be deprecated and
>>>     removed in favor of OF. Marek Vasut says that this is not possible
>>>     for every board, and so needs to stay.
>>>
>>> BTW, I find it a little weird for Marek to defend old API which should
>>> be converted to DT, when in discussion about DM USB / Nokia N900
>>> USB TTY console [1] he was defending the opinion that we should be
>>> heading to DT in U-Boot.
>>>
>>> [1]
>>> https://patchwork.ozlabs.org/project/uboot/patch/20210618145724.2558-1-pali@kernel.org/
>>
>> That USB discussion is completely unrelated to the problem here, the USB
>> discussion is about internal (i.e. not user facing) conversion to DM/DT.
>> The user-facing ABI does not change there. Also, that discussion was
>> about patching USB stack to permit new non-DM/DT operation, not fixing
>> existing one.
> 
> This is not only about the user ABI (altough now I agree that you are
> correct there, see below). What I meant is this:
>    Should we push for converting to device-tree even if for some boards
>    it is not possible, and would mean removing them?

The N900 could however be converted to DT as far as I can tell, there 
was a solution which didn't end up patching the USB core with legacy stuff.

>    Because you are saying that MTDPARTS cannot be converted to DT for
>    some boards.
> 
>    But N900 also cannot be reasonably converted because of space
>    issues, as far as I understood. Yes, it has gigabytes of eMMC storage,
>    and it was proposed to put SPL in MTD and U-Boot proper into eMMC on
>    VFAT/ext4, but this simply cannot be done reasonably, because:
>    - it would break Linux userspace (existing OS upgrade system would
>      have to be rewritten and backwords compatibility would be broken)

Not really, there was also the suggestion to use falcon boot and have 
SPL boot Linux directly (i.e. use SPL instead of U-Boot), while only put 
U-Boot into eMMC and boot it if needed and/or as a fallback.

That way you wouldn't break the existing updaters, because you install 
that SPL instead of current U-Boot, and new one could be added to 
install the extra u-boot binary. And that solves your space issue, 
forever no less.

>    - it would make bootstrapping (booting newer version of U-Boot) while
>      developing U-Boot a pain in the ass or maybe even impossible

Booting U-Boot from U-Boot is unsupported as far as I can tell. It may 
or may not work.

>    - I beleive there was some other reason Pali mentioned, but I cannot
>      remember anymore
> 
>> This problem here is user facing ABI, the mtdparts/mtdids. That user
>> facing ABI got broken. Boards which do depend on it, even those
>> currently in tree, are broken. Not all boards can update their
>> environment, so some backward compatibility of the user facing ABI
>> should be in place, even though it might not be to the degree Linux
>> kernel does so. So far, it seems most of the U-Boot command line
>> interface has managed to retain backward compatibility, I don't see why
>> this here should be handled any differently.
> 
> OK, I get that the if `mtd nor0` was working before, it should work also
> now. But the conversion from MTDPARTS to device tree could be probably
> done for lots of these, see below.

Based on the comment from Tom, I think we are talking about two 
different things here. I am NOT talking about passing mtdparts to Linux 
at all, there using DT is clear.

I am talking about using nor%d in MTDIDS in U-Boot UBI code to look up 
from which device to attach UBI in U-Boot.

>> Note that there are not just a few boards that are broken, but hundreds.
>> I believe that itself justifies a fix, instead of just throwing all
>> those hundreds of boards overboard.
>>
>> u-boot$ git grep -l CONFIG_MTDIDS configs | wc -l
>> 203
> 
> Only 96 of those also grep the substring "nor". But okay, that is still
> a lot. The question is how many of them could be rewritten to DT:
> 
>    for cfg in $(git grep -l 'CONFIG_MTDIDS.*nor[0-9]' configs); do
>      fgrep CONFIG_DEFAULT_DEVICE_TREE "$cfg"
>    done | wc -l
> 
> 92 of those 96 have CONFIG_DEFAULT_DEVICE_TREE defined.
> 
> Of these, 65 contain CONFIG_DM_SPI_FLASH=y, so at least these 65 could
> be converted. Of the rest 27, how many could also be converted to DM?
> How may use non-DM drivers?

See my remark above, are we both talking about the same thing -- mtd%d 
usage in U-Boot MTD code to look up MTD device for UBI in U-Boot ?

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Marek Vasut]

On 9/22/21 9:23 PM, Tom Rini wrote:
> On Wed, Sep 22, 2021 at 09:05:36PM +0200, Marek Behún wrote:
>> On Wed, 22 Sep 2021 20:24:18 +0200
>> Marek Vasut <marex@denx.de> wrote:
>>
>>> On 9/22/21 7:29 PM, Marek Behún wrote:
>>>> (Adding also Tom.)
>>>>
>>>> Hi Patrick, Marek,
>>>>
>>>> I find this either not complete or not needed:
>>>>
>>>> - either you need mtd names to be of this format so that old MTDPARTS
>>>>     config definitions do not need to be changed, i.e. something like
>>>>       CONFIG_MTDPARTS_DEFAULT="nor0:1M(u-boot),0x1000@0xfff000(env)"
>>>>     does not work currently, and you want to make it work.
>>>>
>>>>     I find your solution here incomplete because MTDPARTS can also be
>>>>     used to be passed to Linux as mtdparts parameter, but there is no
>>>>     guarantee that the "norN" numbering you are creating in U-Boot will
>>>>     be the same as the one in kernel.
>>>>
>>>> - or it is not needed, because you can remove MTDPARTS definition from
>>>>     the board config entirely and move the information into device tree.
>>>>     In fact this was the main idea behind making the series
>>>>       Support SPI NORs and OF partitions in `mtd list`
>>>>     The SPI-NOR MTDs after this series can have conflicting names,
>>>>     because you can still choose between them via OF path with the `mtd`
>>>>     command.
>>>>
>>>>     Tom and I were of the opinion that MTDPARTS should be deprecated and
>>>>     removed in favor of OF. Marek Vasut says that this is not possible
>>>>     for every board, and so needs to stay.
>>>>
>>>> BTW, I find it a little weird for Marek to defend old API which should
>>>> be converted to DT, when in discussion about DM USB / Nokia N900
>>>> USB TTY console [1] he was defending the opinion that we should be
>>>> heading to DT in U-Boot.
>>>>
>>>> [1]
>>>> https://patchwork.ozlabs.org/project/uboot/patch/20210618145724.2558-1-pali@kernel.org/
>>>
>>> That USB discussion is completely unrelated to the problem here, the USB
>>> discussion is about internal (i.e. not user facing) conversion to DM/DT.
>>> The user-facing ABI does not change there. Also, that discussion was
>>> about patching USB stack to permit new non-DM/DT operation, not fixing
>>> existing one.
>>
>> This is not only about the user ABI (altough now I agree that you are
>> correct there, see below). What I meant is this:
>>    Should we push for converting to device-tree even if for some boards
>>    it is not possible, and would mean removing them?
>>
>>    Because you are saying that MTDPARTS cannot be converted to DT for
>>    some boards.
>>
>>    But N900 also cannot be reasonably converted because of space
>>    issues, as far as I understood. Yes, it has gigabytes of eMMC storage,
>>    and it was proposed to put SPL in MTD and U-Boot proper into eMMC on
>>    VFAT/ext4, but this simply cannot be done reasonably, because:
>>    - it would break Linux userspace (existing OS upgrade system would
>>      have to be rewritten and backwords compatibility would be broken)
>>    - it would make bootstrapping (booting newer version of U-Boot) while
>>      developing U-Boot a pain in the ass or maybe even impossible
>>    - I beleive there was some other reason Pali mentioned, but I cannot
>>      remember anymore
>>
>>> This problem here is user facing ABI, the mtdparts/mtdids. That user
>>> facing ABI got broken. Boards which do depend on it, even those
>>> currently in tree, are broken. Not all boards can update their
>>> environment, so some backward compatibility of the user facing ABI
>>> should be in place, even though it might not be to the degree Linux
>>> kernel does so. So far, it seems most of the U-Boot command line
>>> interface has managed to retain backward compatibility, I don't see why
>>> this here should be handled any differently.
>>
>> OK, I get that the if `mtd nor0` was working before, it should work also
>> now. But the conversion from MTDPARTS to device tree could be probably
>> done for lots of these, see below.
>>
>>> Note that there are not just a few boards that are broken, but hundreds.
>>> I believe that itself justifies a fix, instead of just throwing all
>>> those hundreds of boards overboard.
>>>
>>> u-boot$ git grep -l CONFIG_MTDIDS configs | wc -l
>>> 203
>>
>> Only 96 of those also grep the substring "nor". But okay, that is still
>> a lot. The question is how many of them could be rewritten to DT:
>>
>>    for cfg in $(git grep -l 'CONFIG_MTDIDS.*nor[0-9]' configs); do
>>      fgrep CONFIG_DEFAULT_DEVICE_TREE "$cfg"
>>    done | wc -l
>>
>> 92 of those 96 have CONFIG_DEFAULT_DEVICE_TREE defined.
>>
>> Of these, 65 contain CONFIG_DM_SPI_FLASH=y, so at least these 65 could
>> be converted. Of the rest 27, how many could also be converted to DM?
>> How may use non-DM drivers?
> 
> I was thinking maybe we have problems with the platforms that "mtdparts
> default", of which we have a handful and most of that handful also do it
> to then make use of the partition table within U-Boot (dfu, or update
> the on-flash U-Boot).  Of those, it might make most sense to poke the
> maintainer directly on how to proceed.

I have a feeling you are talking about a different problem here.

What is broken is U-Boot only look up of MTD device from which to attach 
e.g. UBI or jffs2. That's MTDIDS. There you have that nor0 stuff, see:
cmd/jffs2.c: * mtdids=nor0=edb7312-nor,nand0=edb7312-nand
That is what currently does not work in U-Boot, it has nothing to do 
with Linux.

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2925 bytes --]

On Wed, Sep 22, 2021 at 09:24:24PM +0200, Marek Vasut wrote:
> On 9/22/21 9:05 PM, Marek Behún wrote:
> 
> Hi,
> 
> [...]
> 
> > > > I find this either not complete or not needed:
> > > > 
> > > > - either you need mtd names to be of this format so that old MTDPARTS
> > > >     config definitions do not need to be changed, i.e. something like
> > > >       CONFIG_MTDPARTS_DEFAULT="nor0:1M(u-boot),0x1000@0xfff000(env)"
> > > >     does not work currently, and you want to make it work.
> > > > 
> > > >     I find your solution here incomplete because MTDPARTS can also be
> > > >     used to be passed to Linux as mtdparts parameter, but there is no
> > > >     guarantee that the "norN" numbering you are creating in U-Boot will
> > > >     be the same as the one in kernel.
> > > > 
> > > > - or it is not needed, because you can remove MTDPARTS definition from
> > > >     the board config entirely and move the information into device tree.
> > > >     In fact this was the main idea behind making the series
> > > >       Support SPI NORs and OF partitions in `mtd list`
> > > >     The SPI-NOR MTDs after this series can have conflicting names,
> > > >     because you can still choose between them via OF path with the `mtd`
> > > >     command.
> > > > 
> > > >     Tom and I were of the opinion that MTDPARTS should be deprecated and
> > > >     removed in favor of OF. Marek Vasut says that this is not possible
> > > >     for every board, and so needs to stay.
> > > > 
> > > > BTW, I find it a little weird for Marek to defend old API which should
> > > > be converted to DT, when in discussion about DM USB / Nokia N900
> > > > USB TTY console [1] he was defending the opinion that we should be
> > > > heading to DT in U-Boot.
> > > > 
> > > > [1]
> > > > https://patchwork.ozlabs.org/project/uboot/patch/20210618145724.2558-1-pali@kernel.org/
> > > 
> > > That USB discussion is completely unrelated to the problem here, the USB
> > > discussion is about internal (i.e. not user facing) conversion to DM/DT.
> > > The user-facing ABI does not change there. Also, that discussion was
> > > about patching USB stack to permit new non-DM/DT operation, not fixing
> > > existing one.
> > 
> > This is not only about the user ABI (altough now I agree that you are
> > correct there, see below). What I meant is this:
> >    Should we push for converting to device-tree even if for some boards
> >    it is not possible, and would mean removing them?
> 
> The N900 could however be converted to DT as far as I can tell, there was a
> solution which didn't end up patching the USB core with legacy stuff.

Just for the record and to hopefully end this specific tangent, the
"fix" at the time was to correct the "CONFIG_USB means host or gadget"
so that N900 has more time (and an active developer on) migrating gadget
as it does not use host.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

On Wed, Sep 22, 2021 at 09:24:24PM +0200, Marek Vasut wrote:
[snip]
> Based on the comment from Tom, I think we are talking about two different
> things here. I am NOT talking about passing mtdparts to Linux at all, there
> using DT is clear.
> 
> I am talking about using nor%d in MTDIDS in U-Boot UBI code to look up from
> which device to attach UBI in U-Boot.

Thanks for clarifying what the use case here is, that does mean we can
stop making incorrect guesses about the problem.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 311 bytes --]

On Wed, Sep 22, 2021 at 09:24:24PM +0200, Marek Vasut wrote:

> I am talking about using nor%d in MTDIDS in U-Boot UBI code to look up from
> which device to attach UBI in U-Boot.

OK, so are we not able to pass in the correct name now?  Or just worried
about old environment and new U-Boot?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Marek Vasut]

On 9/22/21 9:46 PM, Tom Rini wrote:
> On Wed, Sep 22, 2021 at 09:24:24PM +0200, Marek Vasut wrote:
> 
>> I am talking about using nor%d in MTDIDS in U-Boot UBI code to look up from
>> which device to attach UBI in U-Boot.
> 
> OK, so are we not able to pass in the correct name now?  Or just worried
> about old environment and new U-Boot?

Say you have the following in board config:

CONFIG_MTDIDS_DEFAULT="nor0=ff705000.spi.0"
CONFIG_MTDPARTS_DEFAULT="mtdparts=ff705000.spi.0:-(fs);"

Then run "=> ubi part fs", which will fail to find nor0, because now 
that nor0 is called something else. That is what this series tries to fix.

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 818 bytes --]

On Wed, Sep 22, 2021 at 09:56:26PM +0200, Marek Vasut wrote:
> On 9/22/21 9:46 PM, Tom Rini wrote:
> > On Wed, Sep 22, 2021 at 09:24:24PM +0200, Marek Vasut wrote:
> > 
> > > I am talking about using nor%d in MTDIDS in U-Boot UBI code to look up from
> > > which device to attach UBI in U-Boot.
> > 
> > OK, so are we not able to pass in the correct name now?  Or just worried
> > about old environment and new U-Boot?
> 
> Say you have the following in board config:
> 
> CONFIG_MTDIDS_DEFAULT="nor0=ff705000.spi.0"
> CONFIG_MTDPARTS_DEFAULT="mtdparts=ff705000.spi.0:-(fs);"
> 
> Then run "=> ubi part fs", which will fail to find nor0, because now that
> nor0 is called something else. That is what this series tries to fix.

Yes, and what is nor0 now, and what happens if you use it?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Marek Vasut]

On 9/22/21 10:00 PM, Tom Rini wrote:
> On Wed, Sep 22, 2021 at 09:56:26PM +0200, Marek Vasut wrote:
>> On 9/22/21 9:46 PM, Tom Rini wrote:
>>> On Wed, Sep 22, 2021 at 09:24:24PM +0200, Marek Vasut wrote:
>>>
>>>> I am talking about using nor%d in MTDIDS in U-Boot UBI code to look up from
>>>> which device to attach UBI in U-Boot.
>>>
>>> OK, so are we not able to pass in the correct name now?  Or just worried
>>> about old environment and new U-Boot?
>>
>> Say you have the following in board config:
>>
>> CONFIG_MTDIDS_DEFAULT="nor0=ff705000.spi.0"
>> CONFIG_MTDPARTS_DEFAULT="mtdparts=ff705000.spi.0:-(fs);"
>>
>> Then run "=> ubi part fs", which will fail to find nor0, because now that
>> nor0 is called something else. That is what this series tries to fix.
> 
> Yes, and what is nor0 now, and what happens if you use it?

Now it is "mt25ql02g", for all mt25ql02g on the board, so ... I cannot 
even select the one SPI NOR I want to use, since they are not even 
enumerated in any way, they are all the same. You might want to look at 
get_mtd_device_nm() in drivers/mtd/mtdcore.c .

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Patrick DELAUNAY]

Hi,

On 9/23/21 3:32 AM, Marek Vasut wrote:
> On 9/22/21 10:00 PM, Tom Rini wrote:
>> On Wed, Sep 22, 2021 at 09:56:26PM +0200, Marek Vasut wrote:
>>> On 9/22/21 9:46 PM, Tom Rini wrote:
>>>> On Wed, Sep 22, 2021 at 09:24:24PM +0200, Marek Vasut wrote:
>>>>
>>>>> I am talking about using nor%d in MTDIDS in U-Boot UBI code to 
>>>>> look up from
>>>>> which device to attach UBI in U-Boot.
>>>>
>>>> OK, so are we not able to pass in the correct name now?  Or just 
>>>> worried
>>>> about old environment and new U-Boot?
>>>
>>> Say you have the following in board config:
>>>
>>> CONFIG_MTDIDS_DEFAULT="nor0=ff705000.spi.0"
>>> CONFIG_MTDPARTS_DEFAULT="mtdparts=ff705000.spi.0:-(fs);"
>>>
>>> Then run "=> ubi part fs", which will fail to find nor0, because now 
>>> that
>>> nor0 is called something else. That is what this series tries to fix.
>>
>> Yes, and what is nor0 now, and what happens if you use it?
>
> Now it is "mt25ql02g", for all mt25ql02g on the board, so ... I cannot 
> even select the one SPI NOR I want to use, since they are not even 
> enumerated in any way, they are all the same. You might want to look 
> at get_mtd_device_nm() in drivers/mtd/mtdcore.c .


To comple me use case, on EV1 board can boot from NOR / NAND / SPI-NAND

so mtdparts and mtdids are buidl dynamically with 
CONFIG_SYS_MTDPARTS_RUNTIME in

afraided board/st/common/stm32mp_mtdparts.c::board_mtdparts_default()


I don't use MTDIDS_DEFAULT / MTDPARTS_DEFAULT.


For example, when I force NOR / NAND presence, I create the MTD variables:

mtdids=nand0=nand0,nor0=nor0

mtdparts=mtdparts=nand0:2m(fsbl),2m(ssbl1),2m(ssbl2),-(UBI);nor0:256k(fsbl1),256k(fsbl2),2m(ssbl),512k(u-boot-env),-(nor_user)


The command "mtdparts" is working in previous U-Boot releaseafraided

and it is not more working as the name of MTD device change


Today, without my patch I have

STM32MP> mtd list
SF: Detected mx66l51235l with page size 256 Bytes, erase size 64 KiB, 
total 64 MiB
Could not find a valid device for nor0
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-0x000000400000 : "ssbl1"
       - 0x000000400000-0x000000600000 : "ssbl2"
       - 0x000000600000-0x000040000000 : "UBI"
* mx66l51235l
   - 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 : "mx66l51235l"
* mx66l51235l
   - 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 : "mx66l51235l"



before my patch, Ihave always the error "Device nor0 not found!" on 
mtdparts command

=> get_mtd_info

==> get_mtd_device_nm("nor0")   build with MTD_DEV_TYPE(type)

===> mtd_device_matches_name()

             and here "nor0" must be  mtd->name acoring the code


or I miss something...


I don't found any way to solve my issue only with "mtdids" variable.

so I restore the previous behavior as I expect the mtd name

modification can impact many other boards.


A other solution can be change get_mtd_info(),

but I was also afraid of side effect.


Patrick

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4191 bytes --]

On Thu, Sep 23, 2021 at 11:04:28AM +0200, Patrick DELAUNAY wrote:
> Hi,
> 
> On 9/23/21 3:32 AM, Marek Vasut wrote:
> > On 9/22/21 10:00 PM, Tom Rini wrote:
> > > On Wed, Sep 22, 2021 at 09:56:26PM +0200, Marek Vasut wrote:
> > > > On 9/22/21 9:46 PM, Tom Rini wrote:
> > > > > On Wed, Sep 22, 2021 at 09:24:24PM +0200, Marek Vasut wrote:
> > > > > 
> > > > > > I am talking about using nor%d in MTDIDS in U-Boot UBI
> > > > > > code to look up from
> > > > > > which device to attach UBI in U-Boot.
> > > > > 
> > > > > OK, so are we not able to pass in the correct name now?  Or
> > > > > just worried
> > > > > about old environment and new U-Boot?
> > > > 
> > > > Say you have the following in board config:
> > > > 
> > > > CONFIG_MTDIDS_DEFAULT="nor0=ff705000.spi.0"
> > > > CONFIG_MTDPARTS_DEFAULT="mtdparts=ff705000.spi.0:-(fs);"
> > > > 
> > > > Then run "=> ubi part fs", which will fail to find nor0, because
> > > > now that
> > > > nor0 is called something else. That is what this series tries to fix.
> > > 
> > > Yes, and what is nor0 now, and what happens if you use it?
> > 
> > Now it is "mt25ql02g", for all mt25ql02g on the board, so ... I cannot
> > even select the one SPI NOR I want to use, since they are not even
> > enumerated in any way, they are all the same. You might want to look at
> > get_mtd_device_nm() in drivers/mtd/mtdcore.c .
> 
> 
> To comple me use case, on EV1 board can boot from NOR / NAND / SPI-NAND
> 
> so mtdparts and mtdids are buidl dynamically with
> CONFIG_SYS_MTDPARTS_RUNTIME in
> 
> afraided board/st/common/stm32mp_mtdparts.c::board_mtdparts_default()
> 
> 
> I don't use MTDIDS_DEFAULT / MTDPARTS_DEFAULT.
> 
> 
> For example, when I force NOR / NAND presence, I create the MTD variables:
> 
> mtdids=nand0=nand0,nor0=nor0
> 
> mtdparts=mtdparts=nand0:2m(fsbl),2m(ssbl1),2m(ssbl2),-(UBI);nor0:256k(fsbl1),256k(fsbl2),2m(ssbl),512k(u-boot-env),-(nor_user)
> 
> 
> The command "mtdparts" is working in previous U-Boot releaseafraided
> 
> and it is not more working as the name of MTD device change
> 
> 
> Today, without my patch I have
> 
> STM32MP> mtd list
> SF: Detected mx66l51235l with page size 256 Bytes, erase size 64 KiB, total
> 64 MiB
> Could not find a valid device for nor0
> 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-0x000000400000 : "ssbl1"
>       - 0x000000400000-0x000000600000 : "ssbl2"
>       - 0x000000600000-0x000040000000 : "UBI"
> * mx66l51235l
>   - 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 : "mx66l51235l"
> * mx66l51235l
>   - 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 : "mx66l51235l"
> 
> 
> 
> before my patch, Ihave always the error "Device nor0 not found!" on mtdparts
> command
> 
> => get_mtd_info
> 
> ==> get_mtd_device_nm("nor0")   build with MTD_DEV_TYPE(type)
> 
> ===> mtd_device_matches_name()
> 
>             and here "nor0" must be  mtd->name acoring the code
> 
> 
> or I miss something...
> 
> 
> I don't found any way to solve my issue only with "mtdids" variable.
> 
> so I restore the previous behavior as I expect the mtd name
> 
> modification can impact many other boards.
> 
> 
> A other solution can be change get_mtd_info(),
> 
> but I was also afraid of side effect.

Thanks for explaining more.  Marek, any ideas on how to resolve this
best, other than logic to restore some form of nor%d being created here?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Marek Behún]

On Fri, 24 Sep 2021 14:22:57 -0400
Tom Rini <trini@konsulko.com> wrote:

> On Thu, Sep 23, 2021 at 11:04:28AM +0200, Patrick DELAUNAY wrote:
> > Hi,
> > 
> > On 9/23/21 3:32 AM, Marek Vasut wrote:  
> > > On 9/22/21 10:00 PM, Tom Rini wrote:  
> > > > On Wed, Sep 22, 2021 at 09:56:26PM +0200, Marek Vasut wrote:  
> > > > > On 9/22/21 9:46 PM, Tom Rini wrote:  
> > > > > > On Wed, Sep 22, 2021 at 09:24:24PM +0200, Marek Vasut wrote:
> > > > > >   
> > > > > > > I am talking about using nor%d in MTDIDS in U-Boot UBI
> > > > > > > code to look up from
> > > > > > > which device to attach UBI in U-Boot.  
> > > > > > 
> > > > > > OK, so are we not able to pass in the correct name now?  Or
> > > > > > just worried
> > > > > > about old environment and new U-Boot?  
> > > > > 
> > > > > Say you have the following in board config:
> > > > > 
> > > > > CONFIG_MTDIDS_DEFAULT="nor0=ff705000.spi.0"
> > > > > CONFIG_MTDPARTS_DEFAULT="mtdparts=ff705000.spi.0:-(fs);"
> > > > > 
> > > > > Then run "=> ubi part fs", which will fail to find nor0, because
> > > > > now that
> > > > > nor0 is called something else. That is what this series tries to fix.  
> > > > 
> > > > Yes, and what is nor0 now, and what happens if you use it?  
> > > 
> > > Now it is "mt25ql02g", for all mt25ql02g on the board, so ... I cannot
> > > even select the one SPI NOR I want to use, since they are not even
> > > enumerated in any way, they are all the same. You might want to look at
> > > get_mtd_device_nm() in drivers/mtd/mtdcore.c .  
> > 
> > 
> > To comple me use case, on EV1 board can boot from NOR / NAND / SPI-NAND
> > 
> > so mtdparts and mtdids are buidl dynamically with
> > CONFIG_SYS_MTDPARTS_RUNTIME in
> > 
> > afraided board/st/common/stm32mp_mtdparts.c::board_mtdparts_default()
> > 
> > 
> > I don't use MTDIDS_DEFAULT / MTDPARTS_DEFAULT.
> > 
> > 
> > For example, when I force NOR / NAND presence, I create the MTD variables:
> > 
> > mtdids=nand0=nand0,nor0=nor0
> > 
> > mtdparts=mtdparts=nand0:2m(fsbl),2m(ssbl1),2m(ssbl2),-(UBI);nor0:256k(fsbl1),256k(fsbl2),2m(ssbl),512k(u-boot-env),-(nor_user)
> > 
> > 
> > The command "mtdparts" is working in previous U-Boot releaseafraided
> > 
> > and it is not more working as the name of MTD device change
> > 
> > 
> > Today, without my patch I have
> >   
> > STM32MP> mtd list  
> > SF: Detected mx66l51235l with page size 256 Bytes, erase size 64 KiB, total
> > 64 MiB
> > Could not find a valid device for nor0
> > 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-0x000000400000 : "ssbl1"
> >       - 0x000000400000-0x000000600000 : "ssbl2"
> >       - 0x000000600000-0x000040000000 : "UBI"
> > * mx66l51235l
> >   - 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 : "mx66l51235l"
> > * mx66l51235l
> >   - 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 : "mx66l51235l"
> > 
> > 
> > 
> > before my patch, Ihave always the error "Device nor0 not found!" on mtdparts
> > command
> >   
> > => get_mtd_info  
> >   
> > ==> get_mtd_device_nm("nor0")   build with MTD_DEV_TYPE(type)  
> >   
> > ===> mtd_device_matches_name()  
> > 
> >             and here "nor0" must be  mtd->name acoring the code
> > 
> > 
> > or I miss something...
> > 
> > 
> > I don't found any way to solve my issue only with "mtdids" variable.
> > 
> > so I restore the previous behavior as I expect the mtd name
> > 
> > modification can impact many other boards.
> > 
> > 
> > A other solution can be change get_mtd_info(),
> > 
> > but I was also afraid of side effect.  
> 
> Thanks for explaining more.  Marek, any ideas on how to resolve this
> best, other than logic to restore some form of nor%d being created here?

So in board/st/common/stm32mp_mtdparts.c the mtd partitions are defined
in a options configurable in the Kconfig file in that directory:
  config MTDPARTS_NAND0_BOOT
    default "2m(fsbl),2m(ssbl1),2m(ssbl2)" if STM32MP15x_STM32IMAGE ||
                                              !TFABOOT
    default "2m(fsbl),4m(fip1),4m(fip2)"

  config MTDPARTS_NAND0_TEE
    default "512k(teeh),512k(teed),512k(teex)"
and so on.

This should be moved to device tree, and if needed, fixed up via
board_fix_fdt() if some needed fixes can only be discovered in runtime.

When mtd partitions are defined this way, the mtd command will be able
to work with them all.

The other thing is that the boot device is chosen according to the
result of get_bootmode() function, and can be UART, USB, NAND, SPINAND,
NOR.

This could be implemented via distro boot by setting some env variables
to needed values. include/configs/stm32mp1.h already does define
bootcmd_stm32mp, for example.

Anyway, this whole thing can be converted to not use mtdparts. I can
elaborate more to Patrick if he has some questions. He can also look
at how board/CZ.NIC/turris_omnia.c changes distro boot in
handle_reset_button() function.



I think all the other code using mtdparts can be converted in a similar
way. The question is how hard would it be to convert things such as UBI.

Marek

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Marek Vasut]

On 9/24/21 9:25 PM, Marek Behún wrote:
> On Fri, 24 Sep 2021 14:22:57 -0400
> Tom Rini <trini@konsulko.com> wrote:
> 
>> On Thu, Sep 23, 2021 at 11:04:28AM +0200, Patrick DELAUNAY wrote:
>>> Hi,
>>>
>>> On 9/23/21 3:32 AM, Marek Vasut wrote:
>>>> On 9/22/21 10:00 PM, Tom Rini wrote:
>>>>> On Wed, Sep 22, 2021 at 09:56:26PM +0200, Marek Vasut wrote:
>>>>>> On 9/22/21 9:46 PM, Tom Rini wrote:
>>>>>>> On Wed, Sep 22, 2021 at 09:24:24PM +0200, Marek Vasut wrote:
>>>>>>>    
>>>>>>>> I am talking about using nor%d in MTDIDS in U-Boot UBI
>>>>>>>> code to look up from
>>>>>>>> which device to attach UBI in U-Boot.
>>>>>>>
>>>>>>> OK, so are we not able to pass in the correct name now?  Or
>>>>>>> just worried
>>>>>>> about old environment and new U-Boot?
>>>>>>
>>>>>> Say you have the following in board config:
>>>>>>
>>>>>> CONFIG_MTDIDS_DEFAULT="nor0=ff705000.spi.0"
>>>>>> CONFIG_MTDPARTS_DEFAULT="mtdparts=ff705000.spi.0:-(fs);"
>>>>>>
>>>>>> Then run "=> ubi part fs", which will fail to find nor0, because
>>>>>> now that
>>>>>> nor0 is called something else. That is what this series tries to fix.
>>>>>
>>>>> Yes, and what is nor0 now, and what happens if you use it?
>>>>
>>>> Now it is "mt25ql02g", for all mt25ql02g on the board, so ... I cannot
>>>> even select the one SPI NOR I want to use, since they are not even
>>>> enumerated in any way, they are all the same. You might want to look at
>>>> get_mtd_device_nm() in drivers/mtd/mtdcore.c .
>>>
>>>
>>> To comple me use case, on EV1 board can boot from NOR / NAND / SPI-NAND
>>>
>>> so mtdparts and mtdids are buidl dynamically with
>>> CONFIG_SYS_MTDPARTS_RUNTIME in
>>>
>>> afraided board/st/common/stm32mp_mtdparts.c::board_mtdparts_default()
>>>
>>>
>>> I don't use MTDIDS_DEFAULT / MTDPARTS_DEFAULT.
>>>
>>>
>>> For example, when I force NOR / NAND presence, I create the MTD variables:
>>>
>>> mtdids=nand0=nand0,nor0=nor0
>>>
>>> mtdparts=mtdparts=nand0:2m(fsbl),2m(ssbl1),2m(ssbl2),-(UBI);nor0:256k(fsbl1),256k(fsbl2),2m(ssbl),512k(u-boot-env),-(nor_user)
>>>
>>>
>>> The command "mtdparts" is working in previous U-Boot releaseafraided
>>>
>>> and it is not more working as the name of MTD device change
>>>
>>>
>>> Today, without my patch I have
>>>    
>>> STM32MP> mtd list
>>> SF: Detected mx66l51235l with page size 256 Bytes, erase size 64 KiB, total
>>> 64 MiB
>>> Could not find a valid device for nor0
>>> 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-0x000000400000 : "ssbl1"
>>>        - 0x000000400000-0x000000600000 : "ssbl2"
>>>        - 0x000000600000-0x000040000000 : "UBI"
>>> * mx66l51235l
>>>    - 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 : "mx66l51235l"
>>> * mx66l51235l
>>>    - 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 : "mx66l51235l"
>>>
>>>
>>>
>>> before my patch, Ihave always the error "Device nor0 not found!" on mtdparts
>>> command
>>>    
>>> => get_mtd_info
>>>    
>>> ==> get_mtd_device_nm("nor0")   build with MTD_DEV_TYPE(type)
>>>    
>>> ===> mtd_device_matches_name()
>>>
>>>              and here "nor0" must be  mtd->name acoring the code
>>>
>>>
>>> or I miss something...
>>>
>>>
>>> I don't found any way to solve my issue only with "mtdids" variable.
>>>
>>> so I restore the previous behavior as I expect the mtd name
>>>
>>> modification can impact many other boards.
>>>
>>>
>>> A other solution can be change get_mtd_info(),
>>>
>>> but I was also afraid of side effect.
>>
>> Thanks for explaining more.  Marek, any ideas on how to resolve this
>> best, other than logic to restore some form of nor%d being created here?
> 
> So in board/st/common/stm32mp_mtdparts.c the mtd partitions are defined
> in a options configurable in the Kconfig file in that directory:
>    config MTDPARTS_NAND0_BOOT
>      default "2m(fsbl),2m(ssbl1),2m(ssbl2)" if STM32MP15x_STM32IMAGE ||
>                                                !TFABOOT
>      default "2m(fsbl),4m(fip1),4m(fip2)"
> 
>    config MTDPARTS_NAND0_TEE
>      default "512k(teeh),512k(teed),512k(teex)"
> and so on.
> 
> This should be moved to device tree, and if needed, fixed up via
> board_fix_fdt() if some needed fixes can only be discovered in runtime.
> 
> When mtd partitions are defined this way, the mtd command will be able
> to work with them all.
> 
> The other thing is that the boot device is chosen according to the
> result of get_bootmode() function, and can be UART, USB, NAND, SPINAND,
> NOR.
> 
> This could be implemented via distro boot by setting some env variables
> to needed values. include/configs/stm32mp1.h already does define
> bootcmd_stm32mp, for example.
> 
> Anyway, this whole thing can be converted to not use mtdparts. I can
> elaborate more to Patrick if he has some questions. He can also look
> at how board/CZ.NIC/turris_omnia.c changes distro boot in
> handle_reset_button() function.
> 
> 
> 
> I think all the other code using mtdparts can be converted in a similar
> way. The question is how hard would it be to convert things such as UBI.

I have a feeling the discussion is again banking toward "mtdparts" and 
"DT", which really is a solved problem, or at least we agreed upon the 
solution.

The patch is trying to fix MTDIDS and their look up in 
get_mtd_device_nm(). This is all U-Boot stuff, it has nothing to do with 
passing mtd partitions to Linux at all.

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Marek Behún]

On Fri, 24 Sep 2021 22:09:15 +0200
Marek Vasut <marex@denx.de> wrote:

> I have a feeling the discussion is again banking toward "mtdparts" and 
> "DT", which really is a solved problem, or at least we agreed upon the 
> solution.

I was trying to explain to Patrick how he can convert the ST board so
that the mtd partitions will be defined in DT and board code won't need
mtdids if he also converts to distro boot correctly.

> The patch is trying to fix MTDIDS and their look up in 
> get_mtd_device_nm(). This is all U-Boot stuff, it has nothing to do with 
> passing mtd partitions to Linux at all.

The solution here is to get rid of MTDIDS also. But I guess this will
take some time, so it does make sense for get_mtd_device_nm() to
return the same device as previously when given argument "norN".

The reason why I put SPI NOR name into mtd->name was because otherwise
the `mtd list` command did not print that name anywhere (since it
doesn't know how, because that SPI-NOR data are private for the
jedec_spi_nor driver).

Since
- every mtd device has mtd->type, for NOR flash this is MTD_NORFLASH
- we can iterate mtd devices in order they were registered
  (mtd_for_each_device does this)
we can easily implement a function
  get_mtd_device_by_type_and_id()
that, when given string "norN" will find the N-th mtd device of type
MTD_NORFLASH.

This function could than be called from get_mtd_device_nm() if
everything failed, or in some other way.

Tom, Marek, Patrick, what do you think?

Marek

Re: [PATCH v4 0/2] mtd: spi: nor: force mtd name to "nor%d"

[Marek Vasut]

On 9/25/21 2:12 AM, Marek Behún wrote:
> On Fri, 24 Sep 2021 22:09:15 +0200
> Marek Vasut <marex@denx.de> wrote:
> 
>> I have a feeling the discussion is again banking toward "mtdparts" and
>> "DT", which really is a solved problem, or at least we agreed upon the
>> solution.
> 
> I was trying to explain to Patrick how he can convert the ST board so
> that the mtd partitions will be defined in DT and board code won't need
> mtdids if he also converts to distro boot correctly.
> 
>> The patch is trying to fix MTDIDS and their look up in
>> get_mtd_device_nm(). This is all U-Boot stuff, it has nothing to do with
>> passing mtd partitions to Linux at all.
> 
> The solution here is to get rid of MTDIDS also. But I guess this will
> take some time, so it does make sense for get_mtd_device_nm() to
> return the same device as previously when given argument "norN".

I'd say MTDIDS is user ABI, so we shouldn't break it until there is 
clear way forward. (yes, I agree with pretty much all you are saying, I 
just want to make this clear)

> The reason why I put SPI NOR name into mtd->name was because otherwise
> the `mtd list` command did not print that name anywhere (since it
> doesn't know how, because that SPI-NOR data are private for the
> jedec_spi_nor driver).
> 
> Since
> - every mtd device has mtd->type, for NOR flash this is MTD_NORFLASH
> - we can iterate mtd devices in order they were registered
>    (mtd_for_each_device does this)
> we can easily implement a function
>    get_mtd_device_by_type_and_id()

No, we cannot. We have to make sure the Parallel NORs go first, SPI NORs 
second, otherwise we break the old ABI. That's what Patrick is valiantly 
battling here.

> that, when given string "norN" will find the N-th mtd device of type
> MTD_NORFLASH.
> 
> This function could than be called from get_mtd_device_nm() if
> everything failed, or in some other way.
> 
> Tom, Marek, Patrick, what do you think?

Maybe we should go with a simpler counting approach first (at least for 
this release) and then improve on that ? That would give us some time to 
think the solution through ... at which point I wouldn't even be opposed 
to passing full DT path to ubi part, something like:

ubi part /soc/controller@0x1234/flash@0/partition@1

or something like that. The above is always stable and unique because it 
is hardware path, it does get flushed right down to get_mtd_device_nm(), 
and it basically skips MTDIDS. With DT aliases you can probably write 
some shorthand for a long HW path.

Re: [PATCH v4 1/2] mtd: cfi_flash: use cfi_flash_num_flash_banks only when supported

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

On Wed, Sep 22, 2021 at 06:29:07PM +0200, Patrick Delaunay wrote:

> When CONFIG_SYS_MAX_FLASH_BANKS_DETECT is activated,
> CONFIG_SYS_MAX_FLASH_BANKS is replaced by cfi_flash_num_flash_banks,
> but this variable is defined in drivers/mtd/cfi_flash.c, which is
> compiled only when CONFIG_FLASH_CFI_DRIVER is activated, in U-Boot
> or in SPL when CONFIG_SPL_MTD_SUPPORT is activated.
> 
> This patch deactivates this feature CONFIG_SYS_MAX_FLASH_BANKS_DETECT
> when flash cfi driver is not activated to avoid compilation issue in
> the next patch, when CONFIG_SYS_MAX_FLASH_BANKS is used in spi_nor_scan().
> 
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 2/2] mtd: spi: nor: force mtd name to "nor%d"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]

On Wed, Sep 22, 2021 at 06:29:08PM +0200, Patrick Delaunay wrote:

> Force the mtd name of spi-nor to "nor" + the driver sequence number:
> "nor0", "nor1"... beginning after the existing nor devices.
> 
> This patch is coherent with existing "nand" and "spi-nand"
> mtd device names.
> 
> When CFI MTD NOR device are supported, the spi-nor index is chosen after
> the last CFI device defined by CONFIG_SYS_MAX_FLASH_BANKS.
> 
> When CONFIG_SYS_MAX_FLASH_BANKS_DETECT is activated, this config
> is replaced by to cfi_flash_num_flash_banks in the include file
> mtd/cfi_flash.h.
> 
> This generic name "nor%d" can be use to identify the mtd spi-nor device
> without knowing the real device name or the DT path of the device,
> used with API get_mtd_device_nm() and is used in mtdparts command.
> 
> This patch also avoids issue when the same NOR device is present 2 times,
> for example on STM32MP15F-EV1:
> 
> STM32MP> mtd list
> SF: Detected mx66l51235l 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"
> * mx66l51235l
>   - 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 : "mx66l51235l"
> * mx66l51235l
>   - 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 : "mx66l51235l"
> 
> The same mtd name "mx66l51235l" identify the 2 instances
> mx66l51235l@0 and mx66l51235l@1.
> 
> This patch fixes a ST32CubeProgrammer / stm32prog command issue
> with nor0 target on STM32MP157C-EV1 board introduced by
> commit b7f060565e31 ("mtd: spi-nor: allow registering multiple MTDs when
> DM is enabled").
> 
> Fixes: b7f060565e31 ("mtd: spi-nor: allow registering multiple MTDs when DM is enabled")
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]