| Message ID | 20210916155040.v3.2.Ia461e670c7438478aa8f8939209d45c818ccd284@changeid |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | mtd: spi: nor: force mtd name to "nor%d" | expand |
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.
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
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.
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
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.
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;
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(-)