| Message ID | 20210225141336.6149-11-marek.behun@nic.cz |
|---|---|
| State | Changes Requested |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | Support SPI NORs and OF partitions in `mtd list` | expand |
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
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
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?
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
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.
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
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 --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__ */ } }
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(+)