diff mbox series

[u-boot-dm,+,u-boot-spi,v3,10/11] mtd: compare also with OF path and device name in get_mtd_device_nm()

Message ID 20210225141336.6149-11-marek.behun@nic.cz
State New
Delegated to: Jagannadha Sutradharudu Teki
Headers show
Series Support SPI NORs and OF partitions in `mtd list` | expand

Commit Message

Marek Behun Feb. 25, 2021, 2:13 p.m. UTC
The get_mtd_device_nm() function (code imported from Linux) simply
iterates all registered MTD devices and compares the given name with
all MTDs' names.

With SPI_FLASH_MTD enabled U-Boot registers a SPI-NOR as a MTD device
with name identical to the SPI flash chip name (from SPI ID table). Thus
for a board with multiple same SPI-NORs it registers multiple MTDs, but
all with the same name (such as "s25fl164k"). We do not want to change
this behaviour, since such a change could break existing boot scripts,
which can rely on a hardcoded name.

In order to allow somehow to uniqely select a MTD device, change
get_mtd_device_nm() function as such:
- if first character of name is '/', interpret it as OF path
- otherwise compare the name with MTDs name and MTDs device name.

In the following example a board has two "s25fl164k" SPI-NORs. They both
have name "s25fl164k", thus cannot be uniquely selected via this name.
With this change, the user can select the second SPI-NOR either with
"spi-nor@1" or "/soc/spi@10600/spi-nor@1".

Example:
  => mtd list
  List of MTD devices:
  * s25fl164k
    - device: spi-nor@0
    - parent: spi@10600
    - driver: jedec_spi_nor
    - path: /soc/spi@10600/spi-nor@0
    - type: NOR flash
    - block size: 0x1000 bytes
    - min I/O: 0x1 bytes
    - 0x000000000000-0x000000800000 : "s25fl164k"
  * s25fl164k
    - device: spi-nor@1
    - parent: spi@10600
    - driver: jedec_spi_nor
    - path: /soc/spi@10600/spi-nor@1
    - type: NOR flash
    - block size: 0x1000 bytes
    - min I/O: 0x1 bytes
    - 0x000000000000-0x000000800000 : "s25fl164k"

This change adds code that depends on CONFIG_DM. Although CONFIG_DM
is compulsory since v2020.01, there are still some boards (for example
tqma6s_wru4_mmc_defconfig) that don't enable it. Therefore the code
guards this parts by #ifdefs.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
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>
Cc: Patrice CHOTARD <patrice.chotard@foss.st.com>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/mtdcore.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Simon Glass Feb. 25, 2021, 7:31 p.m. UTC | #1
Hi Marek,

On Thu, 25 Feb 2021 at 09:14, Marek Behún <marek.behun@nic.cz> wrote:
>
> The get_mtd_device_nm() function (code imported from Linux) simply
> iterates all registered MTD devices and compares the given name with
> all MTDs' names.
>
> With SPI_FLASH_MTD enabled U-Boot registers a SPI-NOR as a MTD device
> with name identical to the SPI flash chip name (from SPI ID table). Thus
> for a board with multiple same SPI-NORs it registers multiple MTDs, but
> all with the same name (such as "s25fl164k"). We do not want to change
> this behaviour, since such a change could break existing boot scripts,
> which can rely on a hardcoded name.
>
> In order to allow somehow to uniqely select a MTD device, change
> get_mtd_device_nm() function as such:
> - if first character of name is '/', interpret it as OF path
> - otherwise compare the name with MTDs name and MTDs device name.
>
> In the following example a board has two "s25fl164k" SPI-NORs. They both
> have name "s25fl164k", thus cannot be uniquely selected via this name.
> With this change, the user can select the second SPI-NOR either with
> "spi-nor@1" or "/soc/spi@10600/spi-nor@1".
>
> Example:
>   => mtd list
>   List of MTD devices:
>   * s25fl164k
>     - device: spi-nor@0
>     - parent: spi@10600
>     - driver: jedec_spi_nor
>     - path: /soc/spi@10600/spi-nor@0
>     - type: NOR flash
>     - block size: 0x1000 bytes
>     - min I/O: 0x1 bytes
>     - 0x000000000000-0x000000800000 : "s25fl164k"
>   * s25fl164k
>     - device: spi-nor@1
>     - parent: spi@10600
>     - driver: jedec_spi_nor
>     - path: /soc/spi@10600/spi-nor@1
>     - type: NOR flash
>     - block size: 0x1000 bytes
>     - min I/O: 0x1 bytes
>     - 0x000000000000-0x000000800000 : "s25fl164k"
>
> This change adds code that depends on CONFIG_DM. Although CONFIG_DM
> is compulsory since v2020.01, there are still some boards (for example
> tqma6s_wru4_mmc_defconfig) that don't enable it. Therefore the code
> guards this parts by #ifdefs.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> 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>
> Cc: Patrice CHOTARD <patrice.chotard@foss.st.com>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/mtdcore.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 0d1f94c6cb..7c894b2e41 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -780,13 +780,42 @@ struct mtd_info *get_mtd_device_nm(const char *name)
>  {
>         int err = -ENODEV;
>         struct mtd_info *mtd = NULL, *other;
> +#ifdef CONFIG_DM
> +       struct udevice *dev = NULL;
> +#endif
> +
> +#ifdef CONFIG_DM

We should not need CONFIG_DM here...it should be enabled for all
boards. You can always disable MTD for a board if not, or send a
removable patch.

If for some reason you do, please use if (IS_ENABLED() so that 'dev'
can always be declared.

> +       /*
> +        * If the first character of mtd name is '/', interpret it as OF path.
> +        * Otherwise try comparing by mtd->name and mtd->dev->name.
> +        */
> +       if (*name == '/') {
> +               err = device_get_global_by_ofnode(ofnode_path(name), &dev);
> +               if (err)
> +                       return ERR_PTR(err);
> +       }
> +#endif
>
>         mutex_lock(&mtd_table_mutex);
>
>         mtd_for_each_device(other) {
> +#ifdef CONFIG_DM
> +               if ((dev && !mtd_is_partition(other) && other->dev == dev) ||
> +                   !strcmp(name, other->name) ||
> +                   (!mtd_is_partition(other) && other->dev &&
> +                    !strcmp(name, other->dev->name))) {
> +#else
>                 if (!strcmp(name, other->name)) {
> +#endif
> +#ifdef __UBOOT__
> +                       if (mtd)
> +                               printf("\nWarning: MTD name \"%s\" is not unique!\n\n",
> +                                      name);
> +                       mtd = other;
> +#else /* !__UBOOT__ */
>                         mtd = other;
>                         break;
> +#endif /* !__UBOOT__ */
>                 }
>         }
>
> --
> 2.26.2
>

Regards,
Simon
Marek Behun Feb. 25, 2021, 8:07 p.m. UTC | #2
On Thu, 25 Feb 2021 14:31:42 -0500
Simon Glass <sjg@chromium.org> wrote:

> We should not need CONFIG_DM here...it should be enabled for all
> boards. You can always disable MTD for a board if not, or send a
> removable patch.
> 
> If for some reason you do, please use if (IS_ENABLED() so that 'dev'
> can always be declared.

Simon, it still isn't enabled for all boards. For example
tqma6s_wru4_mmc_defconfig does not compile with this. I actually wrote
this into commit message:

  Although CONFIG_DM is compulsory since v2020.01, there are still some
  boards (for example tqma6s_wru4_mmc_defconfig) that don't enable it. 

But if breaking such boards is not a problem anymore, I will gladly
just remove the ifdefs :) Should I?

Marek
Tom Rini Feb. 25, 2021, 8:28 p.m. UTC | #3
On Thu, Feb 25, 2021 at 09:07:40PM +0100, Marek Behun wrote:
> On Thu, 25 Feb 2021 14:31:42 -0500
> Simon Glass <sjg@chromium.org> wrote:
> 
> > We should not need CONFIG_DM here...it should be enabled for all
> > boards. You can always disable MTD for a board if not, or send a
> > removable patch.
> > 
> > If for some reason you do, please use if (IS_ENABLED() so that 'dev'
> > can always be declared.
> 
> Simon, it still isn't enabled for all boards. For example
> tqma6s_wru4_mmc_defconfig does not compile with this. I actually wrote
> this into commit message:
> 
>   Although CONFIG_DM is compulsory since v2020.01, there are still some
>   boards (for example tqma6s_wru4_mmc_defconfig) that don't enable it. 
> 
> But if breaking such boards is not a problem anymore, I will gladly
> just remove the ifdefs :) Should I?

So, I'm working hard at dropping boards that are well past migration
deadlines.  That specific one fails DM_MMC more importantly, and I will
be dropping it if it's not migrated, after v2021.04 is out.  What else
fails?  If you rebase your series on my
WIP/remove-non-AHCI_LIBATA-drivers (as it has the most board removals in
it), what fails to build if your series is just DM only?
Marek Behun Feb. 25, 2021, 8:35 p.m. UTC | #4
On Thu, 25 Feb 2021 15:28:56 -0500
Tom Rini <trini@konsulko.com> wrote:

> On Thu, Feb 25, 2021 at 09:07:40PM +0100, Marek Behun wrote:
> > On Thu, 25 Feb 2021 14:31:42 -0500
> > Simon Glass <sjg@chromium.org> wrote:
> >   
> > > We should not need CONFIG_DM here...it should be enabled for all
> > > boards. You can always disable MTD for a board if not, or send a
> > > removable patch.
> > > 
> > > If for some reason you do, please use if (IS_ENABLED() so that 'dev'
> > > can always be declared.  
> > 
> > Simon, it still isn't enabled for all boards. For example
> > tqma6s_wru4_mmc_defconfig does not compile with this. I actually wrote
> > this into commit message:
> > 
> >   Although CONFIG_DM is compulsory since v2020.01, there are still some
> >   boards (for example tqma6s_wru4_mmc_defconfig) that don't enable it. 
> > 
> > But if breaking such boards is not a problem anymore, I will gladly
> > just remove the ifdefs :) Should I?  
> 
> So, I'm working hard at dropping boards that are well past migration
> deadlines.  That specific one fails DM_MMC more importantly, and I will
> be dropping it if it's not migrated, after v2021.04 is out.  What else
> fails?  If you rebase your series on my
> WIP/remove-non-AHCI_LIBATA-drivers (as it has the most board removals in
> it), what fails to build if your series is just DM only?
> 

I haven't tried building all boards, just quickly found a defconfig
with disabled CONFIG_DM :)
With removing the CONFIG_DM ifdefs this series shouldn't break anything
that is already past migration deadline.

Marek
Tom Rini Feb. 25, 2021, 8:39 p.m. UTC | #5
On Thu, Feb 25, 2021 at 09:35:13PM +0100, Marek Behun wrote:
> On Thu, 25 Feb 2021 15:28:56 -0500
> Tom Rini <trini@konsulko.com> wrote:
> 
> > On Thu, Feb 25, 2021 at 09:07:40PM +0100, Marek Behun wrote:
> > > On Thu, 25 Feb 2021 14:31:42 -0500
> > > Simon Glass <sjg@chromium.org> wrote:
> > >   
> > > > We should not need CONFIG_DM here...it should be enabled for all
> > > > boards. You can always disable MTD for a board if not, or send a
> > > > removable patch.
> > > > 
> > > > If for some reason you do, please use if (IS_ENABLED() so that 'dev'
> > > > can always be declared.  
> > > 
> > > Simon, it still isn't enabled for all boards. For example
> > > tqma6s_wru4_mmc_defconfig does not compile with this. I actually wrote
> > > this into commit message:
> > > 
> > >   Although CONFIG_DM is compulsory since v2020.01, there are still some
> > >   boards (for example tqma6s_wru4_mmc_defconfig) that don't enable it. 
> > > 
> > > But if breaking such boards is not a problem anymore, I will gladly
> > > just remove the ifdefs :) Should I?  
> > 
> > So, I'm working hard at dropping boards that are well past migration
> > deadlines.  That specific one fails DM_MMC more importantly, and I will
> > be dropping it if it's not migrated, after v2021.04 is out.  What else
> > fails?  If you rebase your series on my
> > WIP/remove-non-AHCI_LIBATA-drivers (as it has the most board removals in
> > it), what fails to build if your series is just DM only?
> > 
> 
> I haven't tried building all boards, just quickly found a defconfig
> with disabled CONFIG_DM :)
> With removing the CONFIG_DM ifdefs this series shouldn't break anything
> that is already past migration deadline.

Well, past migration deadline doesn't mean it can cause CI to fail.  I'm
just now throwing things out that are 2 years past the deadline, and
we'll probably be doing that for a few releases.
Marek Behun March 1, 2021, 2:03 p.m. UTC | #6
On Thu, 25 Feb 2021 15:39:09 -0500
Tom Rini <trini@konsulko.com> wrote:

> Well, past migration deadline doesn't mean it can cause CI to fail.  I'm
> just now throwing things out that are 2 years past the deadline, and
> we'll probably be doing that for a few releases.

Tom, I would like to send v4 without the CONFIG_DM #ifdefs.

tqma6s_wru4_mmc_defconfig is the only defconfig that defines CONFIG_DM=n

Since I do not have this board, I cannot work on converting it to DM.

I can either
- add patch removing this board from U-Boot to this patch series
- send patch removing this board as separate from this series and send
  this series only after the board is removed
- stick if #ifdefs
- try to use IS_ENABLED(CONFIG_DM), but this would need some extra
  patches into header files because of undefined references (we really
  don't want this)
- something different

How should I proceed?

Marek
Tom Rini March 1, 2021, 2:33 p.m. UTC | #7
On Mon, Mar 01, 2021 at 03:03:11PM +0100, Marek Behun wrote:

> On Thu, 25 Feb 2021 15:39:09 -0500
> Tom Rini <trini@konsulko.com> wrote:
> 
> > Well, past migration deadline doesn't mean it can cause CI to fail.  I'm
> > just now throwing things out that are 2 years past the deadline, and
> > we'll probably be doing that for a few releases.
> 
> Tom, I would like to send v4 without the CONFIG_DM #ifdefs.
> 
> tqma6s_wru4_mmc_defconfig is the only defconfig that defines CONFIG_DM=n
> 
> Since I do not have this board, I cannot work on converting it to DM.
> 
> I can either
> - add patch removing this board from U-Boot to this patch series
> - send patch removing this board as separate from this series and send
>   this series only after the board is removed
> - stick if #ifdefs
> - try to use IS_ENABLED(CONFIG_DM), but this would need some extra
>   patches into header files because of undefined references (we really
>   don't want this)
> - something different
> 
> How should I proceed?

You can say your series depends on:
https://patchwork.ozlabs.org/project/uboot/patch/20210221010634.21310-42-trini@konsulko.com/
which drops tqma6s_wru4_mmc_defconfig and you should submit a PR against
https://github.com/u-boot/u-boot/ which will trigger a CI run in Azure
so you can see what else fails to build, or if that really is the only
board that would fail to build, in this case.  Thanks!
diff mbox series

Patch

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 0d1f94c6cb..7c894b2e41 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -780,13 +780,42 @@  struct mtd_info *get_mtd_device_nm(const char *name)
 {
 	int err = -ENODEV;
 	struct mtd_info *mtd = NULL, *other;
+#ifdef CONFIG_DM
+	struct udevice *dev = NULL;
+#endif
+
+#ifdef CONFIG_DM
+	/*
+	 * If the first character of mtd name is '/', interpret it as OF path.
+	 * Otherwise try comparing by mtd->name and mtd->dev->name.
+	 */
+	if (*name == '/') {
+		err = device_get_global_by_ofnode(ofnode_path(name), &dev);
+		if (err)
+			return ERR_PTR(err);
+	}
+#endif
 
 	mutex_lock(&mtd_table_mutex);
 
 	mtd_for_each_device(other) {
+#ifdef CONFIG_DM
+		if ((dev && !mtd_is_partition(other) && other->dev == dev) ||
+		    !strcmp(name, other->name) ||
+		    (!mtd_is_partition(other) && other->dev &&
+		     !strcmp(name, other->dev->name))) {
+#else
 		if (!strcmp(name, other->name)) {
+#endif
+#ifdef __UBOOT__
+			if (mtd)
+				printf("\nWarning: MTD name \"%s\" is not unique!\n\n",
+				       name);
+			mtd = other;
+#else /* !__UBOOT__ */
 			mtd = other;
 			break;
+#endif /* !__UBOOT__ */
 		}
 	}