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

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

[Patrick Delaunay]

This serie is a V3 for
http://patchwork.ozlabs.org/project/uboot/list/?series=262017&state=*
http://patchwork.ozlabs.org/project/uboot/list/?series=262013&state=*

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

Patrick


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 | 15 ++++++++++++---
 include/linux/mtd/spi-nor.h    |  1 +
 include/mtd/cfi_flash.h        |  8 +++++++-
 3 files changed, 20 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH v3 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

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 v3 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 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 | 15 ++++++++++++---
 include/linux/mtd/spi-nor.h    |  1 +
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index d5d905fa5a..7da5ca6285 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,10 @@ 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, "nor%d",  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 +3830,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/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 7ddc4ba2bf..8c3d5032e3 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -561,6 +561,7 @@ struct spi_nor {
 	int (*ready)(struct spi_nor *nor);
 
 	void *priv;
+	char mtd_name[10];
 /* Compatibility for spi_flash, remove once sf layer is merged with mtd */
 	const char *name;
 	u32 size;
-- 
2.25.1

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

[Marek Behún]

On Thu, 16 Sep 2021 16:01:18 +0200
Patrick Delaunay <patrick.delaunay@foss.st.com> 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:

This is an unfortunate hack :( This is another reason why the whole mtd
subsystem should be refactored.

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

[Marek Behún]

Hi Marek, Patrick,

the patches that made this change also include patch
https://source.denx.de/u-boot/u-boot/-/commit/dcb9a80359d699cf659c95b9b6e6604e2d68adb9

This patch was added so that when there are multiple identical SPI-NOR
chips on the board, you can still select between them in the `mtd`
command via the OF path. That's why the `mtd list` command lists OF
path.

In the discussion with Tom, we also were talking about backwards
compatibility with mtdparts, and if I remember correctly, the
conclusion was that mtdparts is deprecated:
- many configs with MTDPARTS can now be converted to define the
  partitions via OF
- the remaining which can't need their mtd driver updated

Afterwards there will be no need for mtdpart.

I think this is the correct solution here. Not the one where we are
returning back to the "norN" names.

Marek

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

[Marek Vasut]

On 9/16/21 4:01 PM, 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().

Maybe just migrate this config option to Kconfig and let Kconfig handle 
the macro magic ?

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

[Marek Vasut]

On 9/16/21 4:01 PM, Patrick Delaunay wrote:

[...]

> @@ -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

Are we covering all the NORs (HF and co.) with this ?

>   	/* Reset SPI protocol for all commands. */
>   	nor->reg_proto = SNOR_PROTO_1_1_1;
> @@ -3715,8 +3722,10 @@ 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, "nor%d",  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 +3830,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/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 7ddc4ba2bf..8c3d5032e3 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -561,6 +561,7 @@ struct spi_nor {
>   	int (*ready)(struct spi_nor *nor);
>   
>   	void *priv;
> +	char mtd_name[10];

should be 14, because nor%d\0 can be up to 14 bytes long.

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

[Marek Vasut]

On 9/16/21 4:01 PM, Patrick Delaunay wrote:
> 
> This serie is a V3 for
> http://patchwork.ozlabs.org/project/uboot/list/?series=262017&state=*
> http://patchwork.ozlabs.org/project/uboot/list/?series=262013&state=*
> 
> 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
> 
> Patrick

This looks good to me, except for a few nits.

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

[Patrick DELAUNAY]

Hi Marek,

 > Marek VasutSept. 16, 2021, 5:24 p.m. UTC | #1

 > On 9/16/21 4:01 PM, 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().

 > Maybe just migrate this config option to Kconfig and let Kconfig handle
 > the macro magic ?


Sorry for the format  of my answer (it is just copy paste from archive)

because I don't received the U-Boot mails on my @foss.st.com mailbo

since yesterday.


I think about migration but is difficult to don't break the existing 
behaviour in kconfig

CONFIG_SYS_MAX_FLASH_BANKS and CONFIG_SYS_MAX_FLASH_BANKS_DETECT are 
define as 'int'

but can be absent => 2 new config CONFIG_USE need to be added

CONFIG_USE_SYS_MAX_FLASH_BANKS

CONFIG_USE_SYS_MAX_FLASH_BANKS_DETECT


and I don't fully understood the mix between the 2 options and 
CFI_MAX_FLASH_BANKS

in some part of code I think CONFIG_SYS_MAX_FLASH_BANKS should be 
replaced by CFI_MAX_FLASH_BANKS

to avoid to define CONFIG_SYS_MAX_FLASH_BANKS = 
cfi_flash_num_flash_banks (as it is not possible in Kconfig)


=> too huge task just to solve compilation issues.


and I also think to use CONFIG_IS_ENABLED(MTD_SUPPORT)

but it not possible because today

- CONFIG_SPL_MTD_SUPPORT exist

- CONFIG_MTD_SUPPORT don't exit ( test on $(mtd-y) in Makefile)


=> the creation of this config is a huge task just to solve compilation 
issue.


Patrick

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

[Patrick DELAUNAY]

Hi Marek,

 > Marek VasutSept. 16, 2021, 5:27 p.m. UTC | #3
 > On 9/16/21 4:01 PM, Patrick Delaunay wrote:

 > [...]

 > > @@ -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

 > Are we covering all the NORs (HF and co.) with this ?


Yes, except if I miss something


any NOR (including hyperflash) wich use the CFI interface / 
CONFIG_FLASH_CFI_MTD

define the the 'nor%d' name with the calling stack:


initr_flash()

=> flash_init()

==> cfi_flash_init_dm()

==> cfi_mtd_init()   "nor%d" wich use loop on CONFIG_SYS_MAX_FLASH_BANKS


I have only one concern today...


if one cfi bank is missing (not activated in DT by example)

and CONFIG_SYS_MAX_FLASH_BANKS_DETECT is not activated

some holes can be done in index


example:

with CONFIG_SYS_MAX_FLASH_BANKS = 2 but with only one NOR on the board

=> "nor1" is absent and we have only 2 MTD device named "nor"

- "nor0" => NOR or HF, first CFI bank

- "nor2" => SPI-NOR (first)


but I don't think that it is blocking


 > >       /* Reset SPI protocol for all commands. */
 > >       nor->reg_proto = SNOR_PROTO_1_1_1;
 > > @@ -3715,8 +3722,10 @@ 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, "nor%d",  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 +3830,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/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
 > > index 7ddc4ba2bf..8c3d5032e3 100644
 > > --- a/include/linux/mtd/spi-nor.h
 > > +++ b/include/linux/mtd/spi-nor.h
 > > @@ -561,6 +561,7 @@ struct spi_nor {
 > >       int (*ready)(struct spi_nor *nor);
 > >
 > >       void *priv;
 > > +    char mtd_name[10];

 > should be 14, because nor%d\0 can be up to 14 bytes long.

normally DM_MAX_SEQ = 999 (but never used)

but Ok with you for 14 with "nor" = 3 + "%d" = 10 for max u32 value + 
"/0" = 1


for cfi_mtd_names => 16 byte used with "nor%d"

     static char cfi_mtd_names[CFI_MAX_FLASH_BANKS][16];

for nand => 8 bytes (./drivers/mtd/nand/raw/nand.c:59)

     static char dev_name[CONFIG_SYS_MAX_NAND_DEVICE][8];

for spi-nand => 20 bytes (drivers/mtd/nand/spi/core.c:1169)

     mtd->name = malloc(20);


Patrick

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

[Marek Vasut]

On 9/17/21 12:55 PM, Patrick DELAUNAY wrote:
> Hi Marek,
> 
>  > Marek VasutSept. 16, 2021, 5:24 p.m. UTC | #1
> 
>  > On 9/16/21 4:01 PM, 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().
> 
>  > Maybe just migrate this config option to Kconfig and let Kconfig handle
>  > the macro magic ?
> 
> 
> Sorry for the format  of my answer (it is just copy paste from archive)
> 
> because I don't received the U-Boot mails on my @foss.st.com mailbo
> 
> since yesterday.
> 
> 
> I think about migration but is difficult to don't break the existing 
> behaviour in kconfig
> 
> CONFIG_SYS_MAX_FLASH_BANKS and CONFIG_SYS_MAX_FLASH_BANKS_DETECT are 
> define as 'int'
> 
> but can be absent => 2 new config CONFIG_USE need to be added
> 
> CONFIG_USE_SYS_MAX_FLASH_BANKS
> 
> CONFIG_USE_SYS_MAX_FLASH_BANKS_DETECT
> 
> 
> and I don't fully understood the mix between the 2 options and 
> CFI_MAX_FLASH_BANKS
> 
> in some part of code I think CONFIG_SYS_MAX_FLASH_BANKS should be 
> replaced by CFI_MAX_FLASH_BANKS
> 
> to avoid to define CONFIG_SYS_MAX_FLASH_BANKS = 
> cfi_flash_num_flash_banks (as it is not possible in Kconfig)
> 
> 
> => too huge task just to solve compilation issues.
> 
> 
> and I also think to use CONFIG_IS_ENABLED(MTD_SUPPORT)
> 
> but it not possible because today
> 
> - CONFIG_SPL_MTD_SUPPORT exist
> 
> - CONFIG_MTD_SUPPORT don't exit ( test on $(mtd-y) in Makefile)
> 
> 
> => the creation of this config is a huge task just to solve compilation 
> issue.

All right, well, ew.

Can you send a subsequent patchset _after_ this one to fix this flash 
banks mess ?

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

[Marek Vasut]

On 9/17/21 3:06 PM, Patrick DELAUNAY wrote:
> Hi Marek,
> 
>  > Marek VasutSept. 16, 2021, 5:27 p.m. UTC | #3
>  > On 9/16/21 4:01 PM, Patrick Delaunay wrote:
> 
>  > [...]
> 
>  > > @@ -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
> 
>  > Are we covering all the NORs (HF and co.) with this ?
> 
> 
> Yes, except if I miss something
> 
> 
> any NOR (including hyperflash) wich use the CFI interface / 
> CONFIG_FLASH_CFI_MTD
> 
> define the the 'nor%d' name with the calling stack:

Yes

> initr_flash()
> 
> => flash_init()
> 
> ==> cfi_flash_init_dm()
> 
> ==> cfi_mtd_init()   "nor%d" wich use loop on CONFIG_SYS_MAX_FLASH_BANKS
> 
> 
> I have only one concern today...
> 
> 
> if one cfi bank is missing (not activated in DT by example)
> 
> and CONFIG_SYS_MAX_FLASH_BANKS_DETECT is not activated
> 
> some holes can be done in index
> 
> 
> example:
> 
> with CONFIG_SYS_MAX_FLASH_BANKS = 2 but with only one NOR on the board
> 
> => "nor1" is absent and we have only 2 MTD device named "nor"
> 
> - "nor0" => NOR or HF, first CFI bank
> 
> - "nor2" => SPI-NOR (first)
> 
> 
> but I don't think that it is blocking

Wasn't the old behavior exactly the same anyway ?

>  > >       /* Reset SPI protocol for all commands. */
>  > >       nor->reg_proto = SNOR_PROTO_1_1_1;
>  > > @@ -3715,8 +3722,10 @@ 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, "nor%d",  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 +3830,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/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>  > > index 7ddc4ba2bf..8c3d5032e3 100644
>  > > --- a/include/linux/mtd/spi-nor.h
>  > > +++ b/include/linux/mtd/spi-nor.h
>  > > @@ -561,6 +561,7 @@ struct spi_nor {
>  > >       int (*ready)(struct spi_nor *nor);
>  > >
>  > >       void *priv;
>  > > +    char mtd_name[10];
> 
>  > should be 14, because nor%d\0 can be up to 14 bytes long.
> 
> normally DM_MAX_SEQ = 999 (but never used)
> 
> but Ok with you for 14 with "nor" = 3 + "%d" = 10 for max u32 value + 
> "/0" = 1
> 
> 
> for cfi_mtd_names => 16 byte used with "nor%d"
> 
>      static char cfi_mtd_names[CFI_MAX_FLASH_BANKS][16];
> 
> for nand => 8 bytes (./drivers/mtd/nand/raw/nand.c:59)
> 
>      static char dev_name[CONFIG_SYS_MAX_NAND_DEVICE][8];
> 
> for spi-nand => 20 bytes (drivers/mtd/nand/spi/core.c:1169)
> 
>      mtd->name = malloc(20);

Maybe we need a macro for this length of DM seq string which we could 
use in these embedded array cases.

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

[Patrick DELAUNAY]

Hi,

On 9/17/21 3:36 PM, Marek Vasut wrote:
> On 9/17/21 12:55 PM, Patrick DELAUNAY wrote:
>> Hi Marek,
>>
>>  > Marek VasutSept. 16, 2021, 5:24 p.m. UTC | #1
>>
>>  > On 9/16/21 4:01 PM, 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().
>>
>>  > Maybe just migrate this config option to Kconfig and let Kconfig 
>> handle
>>  > the macro magic ?
>>
>>
>> Sorry for the format  of my answer (it is just copy paste from archive)
>>
>> because I don't received the U-Boot mails on my @foss.st.com mailbo
>>
>> since yesterday.
>>
>>
>> I think about migration but is difficult to don't break the existing 
>> behaviour in kconfig
>>
>> CONFIG_SYS_MAX_FLASH_BANKS and CONFIG_SYS_MAX_FLASH_BANKS_DETECT are 
>> define as 'int'
>>
>> but can be absent => 2 new config CONFIG_USE need to be added
>>
>> CONFIG_USE_SYS_MAX_FLASH_BANKS
>>
>> CONFIG_USE_SYS_MAX_FLASH_BANKS_DETECT
>>
>>
>> and I don't fully understood the mix between the 2 options and 
>> CFI_MAX_FLASH_BANKS
>>
>> in some part of code I think CONFIG_SYS_MAX_FLASH_BANKS should be 
>> replaced by CFI_MAX_FLASH_BANKS
>>
>> to avoid to define CONFIG_SYS_MAX_FLASH_BANKS = 
>> cfi_flash_num_flash_banks (as it is not possible in Kconfig)
>>
>>
>> => too huge task just to solve compilation issues.
>>
>>
>> and I also think to use CONFIG_IS_ENABLED(MTD_SUPPORT)
>>
>> but it not possible because today
>>
>> - CONFIG_SPL_MTD_SUPPORT exist
>>
>> - CONFIG_MTD_SUPPORT don't exit ( test on $(mtd-y) in Makefile)
>>
>>
>> => the creation of this config is a huge task just to solve 
>> compilation issue.
>
> All right, well, ew.
>
> Can you send a subsequent patchset _after_ this one to fix this flash 
> banks mess ?


I can try,

I will add it in my TODO list.


Patrick