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

[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: 0x4 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
  - 0x-0x4000 : "nand0"
* mx66l51235l
  - device: mx66l51235l@0
  - parent: spi@58003000
  - driver: jedec_spi_nor
  - path: /soc/spi@58003000/mx66l51235l@0
  - type: NOR flash
  - block size: 0x1 bytes
  - min I/O: 0x1 bytes
  - 0x-0x0400 : "mx66l51235l"
* mx66l51235l
  - device: mx66l51235l@1
  - parent: spi@58003000
  - driver: jedec_spi_nor
  - path: /soc/spi@58003000/mx66l51235l@1
  - type: NOR flash
  - block size: 0x1 bytes
  - min I/O: 0x1 bytes
  - 0x-0x0400 : "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 
---
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 
+#include 
 #include 
 #include 
 #include 
@@ -26,6 +27,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -3664,6 +3666,11 @@ int spi_nor_scan(struct spi_nor *nor)
struct mtd_info *mtd = >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