[u-boot-dm,+,u-boot-spi,v4,04/10] mtd: spi-nor: allow registering multiple MTDs when DM is enabled

Currently when the SPI_FLASH_MTD config option is enabled, only one SPI
can be registered as MTD at any time - it is the last one probed (since
with old non-DM model only one SPI NOR could be probed at any time).

When DM is enabled, allow for registering multiple SPI NORs as MTDs by
utilizing the nor->mtd structure, which is filled in by spi_nor_scan
anyway, instead of filling a separate struct mtd_info.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
Reviewed-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
Tested-by: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Priyanka Jain <priyanka.jain@nxp.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Heiko Schocher <hs@denx.de>
Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
---
 drivers/mtd/spi/sf_internal.h |  4 ++--
 drivers/mtd/spi/sf_mtd.c      | 18 +++++++++++++++++-
 drivers/mtd/spi/sf_probe.c    |  6 ++++--
 3 files changed, 23 insertions(+), 5 deletions(-)

Hi Marek,

I found that this changes the mtd device name and makes 'mtdparts'
doesn't work on my developerbox platform.

Before this change,
-------
=> sf probe
SF: Detected mx66u51235f with page size 256 Bytes, erase size 4 KiB,
total 64 MiB
=> mtd list
List of MTD devices:
* nor1
  - type: NOR flash
  - block size: 0x1000 bytes
  - min I/O: 0x1 bytes
  - 0x000000000000-0x000004000000 : "nor1"
          - 0x000000000000-0x000000070000 : "BootStrap-BL1"
          - 0x000000070000-0x000000100000 : "Flash-Writer"
          - 0x000000100000-0x000000180000 : "SCP-BL2"
          - 0x000000180000-0x0000001f8000 : "FIP-TFA"
          - 0x0000001f8000-0x000000200000 : "Stg2-Tables"
          - 0x000000200000-0x000000400000 : "EDK2"
          - 0x000000400000-0x000000500000 : "UEFI-Vars"
          - 0x000000500000-0x000000700000 : "OPTEE"
          - 0x000000700000-0x000000800000 : "UBoot-Env"
          - 0x000000800000-0x000000900000 : "U-Boot"
          - 0x000000900000-0x000004000000 : "Free"
=>
-------
after this change,
-------
=> sf probe
SF: Detected mx66u51235f with page size 256 Bytes, erase size 4 KiB,
total 64 MiB
=> mtd list
Could not find a valid device for nor1
List of MTD devices:
* mx66u51235f
  - device: spi-flash@0
  - parent: spi@54800000
  - driver: jedec_spi_nor
  - path: /spi@54800000/spi-flash@0
  - type: NOR flash
  - block size: 0x1000 bytes
  - min I/O: 0x1 bytes
  - 0x000000000000-0x000004000000 : "mx66u51235f"
-------

I think I should update CONFIG_MTDIDS_DEFAULT and CONFIG_MTDPARTS_DEFAULT.
But before that, I would like to confirm that this is an intended
change, and what should I do. (replace nor1 with mx66u51235f ?)

Thank you,

2021年5月26日(水) 21:10 Marek Behún <marek.behun@nic.cz>:

>
> Currently when the SPI_FLASH_MTD config option is enabled, only one SPI
> can be registered as MTD at any time - it is the last one probed (since
> with old non-DM model only one SPI NOR could be probed at any time).
>
> When DM is enabled, allow for registering multiple SPI NORs as MTDs by
> utilizing the nor->mtd structure, which is filled in by spi_nor_scan
> anyway, instead of filling a separate struct mtd_info.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> Reviewed-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Tested-by: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Priyanka Jain <priyanka.jain@nxp.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Heiko Schocher <hs@denx.de>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>  drivers/mtd/spi/sf_internal.h |  4 ++--
>  drivers/mtd/spi/sf_mtd.c      | 18 +++++++++++++++++-
>  drivers/mtd/spi/sf_probe.c    |  6 ++++--
>  3 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h
> index 786301ba4a..0b63e1bfc2 100644
> --- a/drivers/mtd/spi/sf_internal.h
> +++ b/drivers/mtd/spi/sf_internal.h
> @@ -81,14 +81,14 @@ int spi_flash_cmd_get_sw_write_prot(struct spi_flash *flash);
>
>  #if CONFIG_IS_ENABLED(SPI_FLASH_MTD)
>  int spi_flash_mtd_register(struct spi_flash *flash);
> -void spi_flash_mtd_unregister(void);
> +void spi_flash_mtd_unregister(struct spi_flash *flash);
>  #else
>  static inline int spi_flash_mtd_register(struct spi_flash *flash)
>  {
>         return 0;
>  }
>
> -static inline void spi_flash_mtd_unregister(void)
> +static inline void spi_flash_mtd_unregister(struct spi_flash *flash)
>  {
>  }
>  #endif
> diff --git a/drivers/mtd/spi/sf_mtd.c b/drivers/mtd/spi/sf_mtd.c
> index 987fac2501..94854fbfc4 100644
> --- a/drivers/mtd/spi/sf_mtd.c
> +++ b/drivers/mtd/spi/sf_mtd.c
> @@ -10,6 +10,20 @@
>  #include <linux/mtd/mtd.h>
>  #include <spi_flash.h>
>
> +#if CONFIG_IS_ENABLED(DM_SPI_FLASH)
> +
> +int spi_flash_mtd_register(struct spi_flash *flash)
> +{
> +       return add_mtd_device(&flash->mtd);
> +}
> +
> +void spi_flash_mtd_unregister(struct spi_flash *flash)
> +{
> +       del_mtd_device(&flash->mtd);
> +}
> +
> +#else /* !CONFIG_IS_ENABLED(DM_SPI_FLASH) */
> +
>  static struct mtd_info sf_mtd_info;
>  static bool sf_mtd_registered;
>  static char sf_mtd_name[8];
> @@ -123,7 +137,7 @@ int spi_flash_mtd_register(struct spi_flash *flash)
>         return ret;
>  }
>
> -void spi_flash_mtd_unregister(void)
> +void spi_flash_mtd_unregister(struct spi_flash *flash)
>  {
>         int ret;
>
> @@ -146,3 +160,5 @@ void spi_flash_mtd_unregister(void)
>         printf("Failed to unregister MTD %s and the spi_flash object is going away: you're in deep trouble!",
>                sf_mtd_info.name);
>  }
> +
> +#endif /* !CONFIG_IS_ENABLED(DM_SPI_FLASH) */
> diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
> index 3befbe91ca..7edb8759fd 100644
> --- a/drivers/mtd/spi/sf_probe.c
> +++ b/drivers/mtd/spi/sf_probe.c
> @@ -84,7 +84,7 @@ struct spi_flash *spi_flash_probe(unsigned int busnum, unsigned int cs,
>  void spi_flash_free(struct spi_flash *flash)
>  {
>         if (CONFIG_IS_ENABLED(SPI_FLASH_MTD))
> -               spi_flash_mtd_unregister();
> +               spi_flash_mtd_unregister(flash);
>
>         spi_free_slave(flash->spi);
>         free(flash);
> @@ -150,8 +150,10 @@ int spi_flash_std_probe(struct udevice *dev)
>
>  static int spi_flash_std_remove(struct udevice *dev)
>  {
> +       struct spi_flash *flash = dev_get_uclass_priv(dev);
> +
>         if (CONFIG_IS_ENABLED(SPI_FLASH_MTD))
> -               spi_flash_mtd_unregister();
> +               spi_flash_mtd_unregister(flash);
>
>         return 0;
>  }
> --
> 2.26.3
>


--
Masami Hiramatsu

On Thu, 8 Jul 2021 08:54:51 +0900
Masami Hiramatsu <masami.hiramatsu@linaro.org> wrote:

> Hi Marek,
> 
> I found that this changes the mtd device name and makes 'mtdparts'
> doesn't work on my developerbox platform.
> 
> Before this change,
> -------
> => sf probe  
> SF: Detected mx66u51235f with page size 256 Bytes, erase size 4 KiB,
> total 64 MiB
> => mtd list  
> List of MTD devices:
> * nor1
>   - type: NOR flash
>   - block size: 0x1000 bytes
>   - min I/O: 0x1 bytes
>   - 0x000000000000-0x000004000000 : "nor1"
>           - 0x000000000000-0x000000070000 : "BootStrap-BL1"
>           - 0x000000070000-0x000000100000 : "Flash-Writer"
>           - 0x000000100000-0x000000180000 : "SCP-BL2"
>           - 0x000000180000-0x0000001f8000 : "FIP-TFA"
>           - 0x0000001f8000-0x000000200000 : "Stg2-Tables"
>           - 0x000000200000-0x000000400000 : "EDK2"
>           - 0x000000400000-0x000000500000 : "UEFI-Vars"
>           - 0x000000500000-0x000000700000 : "OPTEE"
>           - 0x000000700000-0x000000800000 : "UBoot-Env"
>           - 0x000000800000-0x000000900000 : "U-Boot"
>           - 0x000000900000-0x000004000000 : "Free"
> =>  
> -------
> after this change,
> -------
> => sf probe  
> SF: Detected mx66u51235f with page size 256 Bytes, erase size 4 KiB,
> total 64 MiB
> => mtd list  
> Could not find a valid device for nor1
> List of MTD devices:
> * mx66u51235f
>   - device: spi-flash@0
>   - parent: spi@54800000
>   - driver: jedec_spi_nor
>   - path: /spi@54800000/spi-flash@0
>   - type: NOR flash
>   - block size: 0x1000 bytes
>   - min I/O: 0x1 bytes
>   - 0x000000000000-0x000004000000 : "mx66u51235f"
> -------
> 
> I think I should update CONFIG_MTDIDS_DEFAULT and
> CONFIG_MTDPARTS_DEFAULT. But before that, I would like to confirm
> that this is an intended change, and what should I do. (replace nor1
> with mx66u51235f ?)

Hi Masami,

no. The intended solution here for you is to remove MTDIDS / MTDPARTS
completely and instead define the partitions in your device tree:
  https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi

You should add something like this into the spi-flash@0 node:

  partitions {
    compatible = "fixed-partitions";
    #address-cells = <1>;
    #size-cells = <1>;

    partition@0 {
      label = "BootStrap-BL1";
      reg = <0x0 0x70000>;
    };

    partition@70000 {
      label = "Flash-Writer";
      reg = <0x70000 0x90000>;
    };

    partition@100000 {
      label = "SCP-BL2";
      reg = <0x100000 0x80000>;
    };

    ...
  };

I wonder though now whether we should force other boards to do this or
whether we should fix the code to be backwards compatible with the old
names.

Tom, Miquel, Jagan, what do you think?

Marek

On Thu, Jul 08, 2021 at 04:15:17PM +0200, Marek Behún wrote:
> On Thu, 8 Jul 2021 08:54:51 +0900
> Masami Hiramatsu <masami.hiramatsu@linaro.org> wrote:
> 
> > Hi Marek,
> > 
> > I found that this changes the mtd device name and makes 'mtdparts'
> > doesn't work on my developerbox platform.
> > 
> > Before this change,
> > -------
> > => sf probe  
> > SF: Detected mx66u51235f with page size 256 Bytes, erase size 4 KiB,
> > total 64 MiB
> > => mtd list  
> > List of MTD devices:
> > * nor1
> >   - type: NOR flash
> >   - block size: 0x1000 bytes
> >   - min I/O: 0x1 bytes
> >   - 0x000000000000-0x000004000000 : "nor1"
> >           - 0x000000000000-0x000000070000 : "BootStrap-BL1"
> >           - 0x000000070000-0x000000100000 : "Flash-Writer"
> >           - 0x000000100000-0x000000180000 : "SCP-BL2"
> >           - 0x000000180000-0x0000001f8000 : "FIP-TFA"
> >           - 0x0000001f8000-0x000000200000 : "Stg2-Tables"
> >           - 0x000000200000-0x000000400000 : "EDK2"
> >           - 0x000000400000-0x000000500000 : "UEFI-Vars"
> >           - 0x000000500000-0x000000700000 : "OPTEE"
> >           - 0x000000700000-0x000000800000 : "UBoot-Env"
> >           - 0x000000800000-0x000000900000 : "U-Boot"
> >           - 0x000000900000-0x000004000000 : "Free"
> > =>  
> > -------
> > after this change,
> > -------
> > => sf probe  
> > SF: Detected mx66u51235f with page size 256 Bytes, erase size 4 KiB,
> > total 64 MiB
> > => mtd list  
> > Could not find a valid device for nor1
> > List of MTD devices:
> > * mx66u51235f
> >   - device: spi-flash@0
> >   - parent: spi@54800000
> >   - driver: jedec_spi_nor
> >   - path: /spi@54800000/spi-flash@0
> >   - type: NOR flash
> >   - block size: 0x1000 bytes
> >   - min I/O: 0x1 bytes
> >   - 0x000000000000-0x000004000000 : "mx66u51235f"
> > -------
> > 
> > I think I should update CONFIG_MTDIDS_DEFAULT and
> > CONFIG_MTDPARTS_DEFAULT. But before that, I would like to confirm
> > that this is an intended change, and what should I do. (replace nor1
> > with mx66u51235f ?)
> 
> Hi Masami,
> 
> no. The intended solution here for you is to remove MTDIDS / MTDPARTS
> completely and instead define the partitions in your device tree:
>   https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
> 
> You should add something like this into the spi-flash@0 node:
> 
>   partitions {
>     compatible = "fixed-partitions";
>     #address-cells = <1>;
>     #size-cells = <1>;
> 
>     partition@0 {
>       label = "BootStrap-BL1";
>       reg = <0x0 0x70000>;
>     };
> 
>     partition@70000 {
>       label = "Flash-Writer";
>       reg = <0x70000 0x90000>;
>     };
> 
>     partition@100000 {
>       label = "SCP-BL2";
>       reg = <0x100000 0x80000>;
>     };
> 
>     ...
>   };
> 
> I wonder though now whether we should force other boards to do this or
> whether we should fix the code to be backwards compatible with the old
> names.
> 
> Tom, Miquel, Jagan, what do you think?

I think we need for passing mtdparts/mtdids via the kernel command line
to continue to work like they used it.  And some checkpatch magic to
catch people introducing new ones?  It certainly would be best for
people to define them in dts as allowed, but there's a good size
existing base that doesn't.  Thanks!

Hi Marek,

2021年7月8日(木) 23:15 Marek Behún <marek.behun@nic.cz>:
>
> On Thu, 8 Jul 2021 08:54:51 +0900
> Masami Hiramatsu <masami.hiramatsu@linaro.org> wrote:
>
> > Hi Marek,
> >
> > I found that this changes the mtd device name and makes 'mtdparts'
> > doesn't work on my developerbox platform.
> >
> > Before this change,
> > -------
> > => sf probe
> > SF: Detected mx66u51235f with page size 256 Bytes, erase size 4 KiB,
> > total 64 MiB
> > => mtd list
> > List of MTD devices:
> > * nor1
> >   - type: NOR flash
> >   - block size: 0x1000 bytes
> >   - min I/O: 0x1 bytes
> >   - 0x000000000000-0x000004000000 : "nor1"
> >           - 0x000000000000-0x000000070000 : "BootStrap-BL1"
> >           - 0x000000070000-0x000000100000 : "Flash-Writer"
> >           - 0x000000100000-0x000000180000 : "SCP-BL2"
> >           - 0x000000180000-0x0000001f8000 : "FIP-TFA"
> >           - 0x0000001f8000-0x000000200000 : "Stg2-Tables"
> >           - 0x000000200000-0x000000400000 : "EDK2"
> >           - 0x000000400000-0x000000500000 : "UEFI-Vars"
> >           - 0x000000500000-0x000000700000 : "OPTEE"
> >           - 0x000000700000-0x000000800000 : "UBoot-Env"
> >           - 0x000000800000-0x000000900000 : "U-Boot"
> >           - 0x000000900000-0x000004000000 : "Free"
> > =>
> > -------
> > after this change,
> > -------
> > => sf probe
> > SF: Detected mx66u51235f with page size 256 Bytes, erase size 4 KiB,
> > total 64 MiB
> > => mtd list
> > Could not find a valid device for nor1
> > List of MTD devices:
> > * mx66u51235f
> >   - device: spi-flash@0
> >   - parent: spi@54800000
> >   - driver: jedec_spi_nor
> >   - path: /spi@54800000/spi-flash@0
> >   - type: NOR flash
> >   - block size: 0x1000 bytes
> >   - min I/O: 0x1 bytes
> >   - 0x000000000000-0x000004000000 : "mx66u51235f"
> > -------
> >
> > I think I should update CONFIG_MTDIDS_DEFAULT and
> > CONFIG_MTDPARTS_DEFAULT. But before that, I would like to confirm
> > that this is an intended change, and what should I do. (replace nor1
> > with mx66u51235f ?)
>
> Hi Masami,
>
> no. The intended solution here for you is to remove MTDIDS / MTDPARTS
> completely and instead define the partitions in your device tree:
>   https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/dts/synquacer-sc2a11-developerbox-u-boot.dtsi
>
> You should add something like this into the spi-flash@0 node:
>
>   partitions {
>     compatible = "fixed-partitions";
>     #address-cells = <1>;
>     #size-cells = <1>;
>
>     partition@0 {
>       label = "BootStrap-BL1";
>       reg = <0x0 0x70000>;
>     };
>
>     partition@70000 {
>       label = "Flash-Writer";
>       reg = <0x70000 0x90000>;
>     };
>
>     partition@100000 {
>       label = "SCP-BL2";
>       reg = <0x100000 0x80000>;
>     };
>
>     ...
>   };

OK, I'll then update the dtb according to your suggestion.

Thank you!

>
> I wonder though now whether we should force other boards to do this or
> whether we should fix the code to be backwards compatible with the old
> names.
>
> Tom, Miquel, Jagan, what do you think?
>
> Marek