[U-Boot] [PATCH v2 1/5] mtd: Use default mtdparts/mtids when not defined in the environment
U-boot provides a mean to define default values for mtdids and mtdparts when they're not defined in the environment. Patch mtd_probe_devices() to use those default values when env_get("mtdparts") or env_get("mtdids") return NULL.
This implementation is based on the logic found in cmd/mtdparts.c.
Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Reported-by: Stefan Roese sr@denx.de Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Tested-by: Stefan Roese sr@denx.de --- Changes in v2: - none --- drivers/mtd/mtd_uboot.c | 62 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c index 7d7a11c990d6..1d0a505754f2 100644 --- a/drivers/mtd/mtd_uboot.c +++ b/drivers/mtd/mtd_uboot.c @@ -92,12 +92,70 @@ static void mtd_probe_uclass_mtd_devs(void) { } #endif
#if defined(CONFIG_MTD_PARTITIONS) +extern void board_mtdparts_default(const char **mtdids, + const char **mtdparts); + +static const char *get_mtdids(void) +{ + __maybe_unused const char *mtdparts = NULL; + const char *mtdids = env_get("mtdids"); + + if (mtdids) + return mtdids; + +#if defined(CONFIG_SYS_MTDPARTS_RUNTIME) + board_mtdparts_default(&mtdids, &mtdparts); +#elif defined(MTDIDS_DEFAULT) + mtdids = MTDIDS_DEFAULT; +#elif defined(CONFIG_MTDIDS_DEFAULT) + mtdids = CONFIG_MTDIDS_DEFAULT; +#endif + + if (mtdids) + env_set("mtdids", mtdids); + + return mtdids; +} + +#define MTDPARTS_MAXLEN 512 + +static const char *get_mtdparts(void) +{ + __maybe_unused const char *mtdids = NULL; + static char tmp_parts[MTDPARTS_MAXLEN]; + static bool use_defaults = true; + const char *mtdparts = NULL; + + if (gd->flags & GD_FLG_ENV_READY) + mtdparts = env_get("mtdparts"); + else if (env_get_f("mtdparts", tmp_parts, sizeof(tmp_parts) != -1)) + mtdparts = tmp_parts; + + if (mtdparts || !use_defaults) + return mtdparts; + +#if defined(CONFIG_SYS_MTDPARTS_RUNTIME) + board_mtdparts_default(&mtdids, &mtdparts); +#elif defined(MTDPARTS_DEFAULT) + mtdparts = MTDPARTS_DEFAULT; +#elif defined(CONFIG_MTDPARTS_DEFAULT) + mtdparts = CONFIG_MTDPARTS_DEFAULT; +#endif + + if (mtdparts) + env_set("mtdparts", mtdparts); + + use_defaults = false; + + return mtdparts; +} + int mtd_probe_devices(void) { static char *old_mtdparts; static char *old_mtdids; - const char *mtdparts = env_get("mtdparts"); - const char *mtdids = env_get("mtdids"); + const char *mtdparts = get_mtdparts(); + const char *mtdids = get_mtdids(); bool remaining_partitions = true; struct mtd_info *mtd;
dfu_fill_entity_nand() uses find_dev_and_part() and mtdparts_init() which are provided by cmd/mtdparts.c.
Add the dependency to avoid build failures when CMD_MTDPARTS is not selected.
Reported-by: Jagan Teki jagan@amarulasolutions.com Fixes: 6828e602b722d ("dfu: Migrate to Kconfig") Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com --- Changes in v2: - Moved ealier in the series to avoid introducing a build failure --- drivers/dfu/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig index 51ab484c2a49..4692736c9d24 100644 --- a/drivers/dfu/Kconfig +++ b/drivers/dfu/Kconfig @@ -30,6 +30,7 @@ config DFU_MMC
config DFU_NAND bool "NAND back end for DFU" + depends on CMD_MTDPARTS help This option enables using DFU to read and write to NAND based storage.
Hi Boris,
dfu_fill_entity_nand() uses find_dev_and_part() and mtdparts_init() which are provided by cmd/mtdparts.c.
Add the dependency to avoid build failures when CMD_MTDPARTS is not selected.
Reported-by: Jagan Teki jagan@amarulasolutions.com Fixes: 6828e602b722d ("dfu: Migrate to Kconfig") Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
Changes in v2:
- Moved ealier in the series to avoid introducing a build failure
drivers/dfu/Kconfig | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig index 51ab484c2a49..4692736c9d24 100644 --- a/drivers/dfu/Kconfig +++ b/drivers/dfu/Kconfig @@ -30,6 +30,7 @@ config DFU_MMC
config DFU_NAND bool "NAND back end for DFU"
- depends on CMD_MTDPARTS help This option enables using DFU to read and write to NAND
based storage.
Acked-by: Lukasz Majewski lukma@denx.de
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
gwventana configs are relying on CMD_UBI to select CMD_MTDPARTS, which is then making {MTDIDS,MTDPARTS}_DEFAULT options available.
We are about to remove the 'select CMD_MTDPARTS' statement in the CMD_UBI entry, but if we do that without first making sure {MTDIDS,MTDPARTS}_DEFAULT are visible, we end up with a build failure when building gwventana configs.
Address that by adding a depends on MTD_PARTITIONS to {MTDIDS,MTDPARTS}_DEFAULT which does the trick since CMD_UBI selects MTD_UBI which in turn selects MTD_PARTITIONS.
We also get rid of the depends on CMD_MTD, since CMD_MTD also selects MTD_PARTITIONS.
Reported-by: Jagan Teki jagan@amarulasolutions.com Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com --- Changes in v2: - Moved ealier in the series to avoid introducing a build failure --- cmd/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index d66f710ad0f8..9310a8bdb2b7 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1728,14 +1728,14 @@ config CMD_MTDPARTS
config MTDIDS_DEFAULT string "Default MTD IDs" - depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH + depends on MTD_PARTITIONS || CMD_MTDPARTS || CMD_NAND || CMD_FLASH help Defines a default MTD IDs list for use with MTD partitions in the Linux MTD command line partitions format.
config MTDPARTS_DEFAULT string "Default MTD partition scheme" - depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH + depends on MTD_PARTITIONS || CMD_MTDPARTS || CMD_NAND || CMD_FLASH help Defines a default MTD partitioning scheme in the Linux MTD command line partitions format
On Mon, 12 Nov 2018 09:28:07 +0100 Boris Brezillon boris.brezillon@bootlin.com wrote:
gwventana configs are relying on CMD_UBI to select CMD_MTDPARTS, which is then making {MTDIDS,MTDPARTS}_DEFAULT options available.
We are about to remove the 'select CMD_MTDPARTS' statement in the CMD_UBI entry, but if we do that without first making sure {MTDIDS,MTDPARTS}_DEFAULT are visible, we end up with a build failure when building gwventana configs.
Address that by adding a depends on MTD_PARTITIONS to {MTDIDS,MTDPARTS}_DEFAULT which does the trick since CMD_UBI selects MTD_UBI which in turn selects MTD_PARTITIONS.
We also get rid of the depends on CMD_MTD, since CMD_MTD also selects MTD_PARTITIONS.
Reported-by: Jagan Teki jagan@amarulasolutions.com Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
Changes in v2:
- Moved ealier in the series to avoid introducing a build failure
cmd/Kconfig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index d66f710ad0f8..9310a8bdb2b7 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1728,14 +1728,14 @@ config CMD_MTDPARTS
config MTDIDS_DEFAULT string "Default MTD IDs"
- depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
- depends on MTD_PARTITIONS || CMD_MTDPARTS || CMD_NAND ||
CMD_FLASH help Defines a default MTD IDs list for use with MTD partitions in the Linux MTD command line partitions format.
config MTDPARTS_DEFAULT string "Default MTD partition scheme"
- depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
- depends on MTD_PARTITIONS || CMD_MTDPARTS || CMD_NAND ||
CMD_FLASH help Defines a default MTD partitioning scheme in the Linux MTD command line partitions format
Reviewed-by: Lukasz Majewski lukma@denx.de
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Commit c58fb2cdb3e4 ("cmd: ubi: clean the partition handling") introduced a call to mtd_probe_devices() in the ubi_attach() path and this function takes care of parsing mtdparts/mtdids and creating/registering the associated mtd partitions.
The mtdparts_init() call in the ubi_detach() path is not only unnecessary but can sometimes print error messages even when things work properly (that's the case with SPI NAND devices that have not been probed with 'mtd list'), which is misleading.
Remove this call to mtdparts_init() and drop the dependency on CMD_MTDPARTS.
Fixes: c58fb2cdb3e4 ("cmd: ubi: clean the partition handling") Reported-by: Stefan Roese sr@denx.de Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Tested-by: Stefan Roese sr@denx.de --- Changes in v2: - Moved after the patches reworking the Kconfig deps --- cmd/Kconfig | 1 - cmd/ubi.c | 5 ----- 2 files changed, 6 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 9310a8bdb2b7..ad14c9ce7124 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1856,7 +1856,6 @@ endmenu
config CMD_UBI tristate "Enable UBI - Unsorted block images commands" - select CMD_MTDPARTS select CRC32 select MTD_UBI help diff --git a/cmd/ubi.c b/cmd/ubi.c index 767a4a453640..2b74a9814463 100644 --- a/cmd/ubi.c +++ b/cmd/ubi.c @@ -417,11 +417,6 @@ static int ubi_dev_scan(struct mtd_info *info, const char *vid_header_offset)
int ubi_detach(void) { - if (mtdparts_init() != 0) { - printf("Error initializing mtdparts!\n"); - return 1; - } - #ifdef CONFIG_CMD_UBIFS /* * Automatically unmount UBIFS partition when user
On Mon, 12 Nov 2018 09:28:08 +0100 Boris Brezillon boris.brezillon@bootlin.com wrote:
Commit c58fb2cdb3e4 ("cmd: ubi: clean the partition handling") introduced a call to mtd_probe_devices() in the ubi_attach() path and this function takes care of parsing mtdparts/mtdids and creating/registering the associated mtd partitions.
The mtdparts_init() call in the ubi_detach() path is not only unnecessary but can sometimes print error messages even when things work properly (that's the case with SPI NAND devices that have not been probed with 'mtd list'), which is misleading.
Remove this call to mtdparts_init() and drop the dependency on CMD_MTDPARTS.
Fixes: c58fb2cdb3e4 ("cmd: ubi: clean the partition handling") Reported-by: Stefan Roese sr@denx.de Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Tested-by: Stefan Roese sr@denx.de
Changes in v2:
- Moved after the patches reworking the Kconfig deps
cmd/Kconfig | 1 - cmd/ubi.c | 5 ----- 2 files changed, 6 deletions(-)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 9310a8bdb2b7..ad14c9ce7124 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1856,7 +1856,6 @@ endmenu
config CMD_UBI tristate "Enable UBI - Unsorted block images commands"
- select CMD_MTDPARTS select CRC32 select MTD_UBI help
diff --git a/cmd/ubi.c b/cmd/ubi.c index 767a4a453640..2b74a9814463 100644 --- a/cmd/ubi.c +++ b/cmd/ubi.c @@ -417,11 +417,6 @@ static int ubi_dev_scan(struct mtd_info *info, const char *vid_header_offset) int ubi_detach(void) {
- if (mtdparts_init() != 0) {
printf("Error initializing mtdparts!\n");return 1;- }
#ifdef CONFIG_CMD_UBIFS /* * Automatically unmount UBIFS partition when user
Reviewed-by: Lukasz Majewski lukma@denx.de
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Commit 9c5b00973bce ("Convert CONFIG_MTD_PARTITIONS et al to Kconfig") introduced a publicly visible Kconfig entry for the CONFIG_MTD_PARTITIONS option, while the rework on MTD partitioning was in progress, and we somehow did not notice that the same Kconfig entry was added by commit 4048a5c519a8 ("mtd: declare MTD_PARTITIONS symbol in Kconfig"), but this time as an invisible entry (this can only be selected by other options).
Keep the non-visible version of this symbol, since MTD_PARTITIONS is not something the user should be able to enable/disable directly.
Fixes: 4048a5c519a8 ("mtd: declare MTD_PARTITIONS symbol in Kconfig") Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com --- Changes in v2: - Moved after the patches reworking the Kconfig deps --- drivers/mtd/Kconfig | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig index 11cf12bb5599..0050fb2b9bf1 100644 --- a/drivers/mtd/Kconfig +++ b/drivers/mtd/Kconfig @@ -22,12 +22,6 @@ config MTD_DEVICE Adds the MTD device infrastructure from the Linux kernel. Needed for mtdparts command support.
-config MTD_PARTITIONS - bool "Add MTD Partioning infrastructure" - help - Adds the MTD partitioning infrastructure from the Linux - kernel. Needed for UBI support. - config FLASH_CFI_DRIVER bool "Enable CFI Flash driver" help
On Mon, 12 Nov 2018 09:28:09 +0100 Boris Brezillon boris.brezillon@bootlin.com wrote:
Commit 9c5b00973bce ("Convert CONFIG_MTD_PARTITIONS et al to Kconfig") introduced a publicly visible Kconfig entry for the CONFIG_MTD_PARTITIONS option, while the rework on MTD partitioning was in progress, and we somehow did not notice that the same Kconfig entry was added by commit 4048a5c519a8 ("mtd: declare MTD_PARTITIONS symbol in Kconfig"), but this time as an invisible entry (this can only be selected by other options).
Keep the non-visible version of this symbol, since MTD_PARTITIONS is not something the user should be able to enable/disable directly.
Fixes: 4048a5c519a8 ("mtd: declare MTD_PARTITIONS symbol in Kconfig") Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
Changes in v2:
- Moved after the patches reworking the Kconfig deps
drivers/mtd/Kconfig | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig index 11cf12bb5599..0050fb2b9bf1 100644 --- a/drivers/mtd/Kconfig +++ b/drivers/mtd/Kconfig @@ -22,12 +22,6 @@ config MTD_DEVICE Adds the MTD device infrastructure from the Linux kernel. Needed for mtdparts command support.
-config MTD_PARTITIONS
- bool "Add MTD Partioning infrastructure"
- help
Adds the MTD partitioning infrastructure from the Linuxkernel. Needed for UBI support.config FLASH_CFI_DRIVER bool "Enable CFI Flash driver" help
Reviewed-by: Lukasz Majewski lukma@denx.de
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Hi Boris, Tom,
Boris Brezillon boris.brezillon@bootlin.com wrote on Mon, 12 Nov 2018 09:28:05 +0100:
U-boot provides a mean to define default values for mtdids and mtdparts when they're not defined in the environment. Patch mtd_probe_devices() to use those default values when env_get("mtdparts") or env_get("mtdids") return NULL.
This implementation is based on the logic found in cmd/mtdparts.c.
Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Reported-by: Stefan Roese sr@denx.de Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Tested-by: Stefan Roese sr@denx.de
For the whole series:
Reviewed-by: Miquel Raynal miquel.raynal@bootlin.com
This should be (if possible) in 2018.11, otherwise the release will be buggy with certain configurations. Maybe we should sometimes send PR directly to Tom as MTD is orphaned to avoid latencies between developers/maintainers and reach mainline quickly (at least for the fixes)?
Thanks, Miquèl
On Mon, Nov 12, 2018 at 2:54 PM Miquel Raynal miquel.raynal@bootlin.com wrote:
Hi Boris, Tom,
Boris Brezillon boris.brezillon@bootlin.com wrote on Mon, 12 Nov 2018 09:28:05 +0100:
U-boot provides a mean to define default values for mtdids and mtdparts when they're not defined in the environment. Patch mtd_probe_devices() to use those default values when env_get("mtdparts") or env_get("mtdids") return NULL.
This implementation is based on the logic found in cmd/mtdparts.c.
Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Reported-by: Stefan Roese sr@denx.de Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Tested-by: Stefan Roese sr@denx.de
For the whole series:
Reviewed-by: Miquel Raynal miquel.raynal@bootlin.com
This should be (if possible) in 2018.11, otherwise the release will be buggy with certain configurations. Maybe we should sometimes send PR directly to Tom as MTD is orphaned to avoid latencies between developers/maintainers and reach mainline quickly (at least for the fixes)?
ie one of the reason I requesting travis-ci build before sending the generic changes.
Hi Jagan,
Jagan Teki jagan@amarulasolutions.com wrote on Mon, 12 Nov 2018 15:39:20 +0530:
On Mon, Nov 12, 2018 at 2:54 PM Miquel Raynal miquel.raynal@bootlin.com wrote:
Hi Boris, Tom,
Boris Brezillon boris.brezillon@bootlin.com wrote on Mon, 12 Nov 2018 09:28:05 +0100:
U-boot provides a mean to define default values for mtdids and mtdparts when they're not defined in the environment. Patch mtd_probe_devices() to use those default values when env_get("mtdparts") or env_get("mtdids") return NULL.
This implementation is based on the logic found in cmd/mtdparts.c.
Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Reported-by: Stefan Roese sr@denx.de Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Tested-by: Stefan Roese sr@denx.de
For the whole series:
Reviewed-by: Miquel Raynal miquel.raynal@bootlin.com
This should be (if possible) in 2018.11, otherwise the release will be buggy with certain configurations. Maybe we should sometimes send PR directly to Tom as MTD is orphaned to avoid latencies between developers/maintainers and reach mainline quickly (at least for the fixes)?
ie one of the reason I requesting travis-ci build before sending the generic changes.
Yes, we should have run a CI test first. I am not complaining at all about the time between having the series posted and you testing it. I understand this delay and really, I can't blame you for that. I'm just saying that this is the second time we (almost?) miss a release because of the additional delays between us, which are, IMHO, not really needed while there is no actual code review as long as we do run Travis.
Thanks, Miquèl
On Mon, Nov 12, 2018 at 1:58 PM Boris Brezillon boris.brezillon@bootlin.com wrote:
U-boot provides a mean to define default values for mtdids and mtdparts when they're not defined in the environment. Patch mtd_probe_devices() to use those default values when env_get("mtdparts") or env_get("mtdids") return NULL.
This implementation is based on the logic found in cmd/mtdparts.c.
Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Reported-by: Stefan Roese sr@denx.de Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Tested-by: Stefan Roese sr@denx.de
Changes in v2:
- none
Trigger travis-ci [1], will send v2 PR once all built fine.
On Mon, 12 Nov 2018 15:55:18 +0530 Jagan Teki jagan@amarulasolutions.com wrote:
On Mon, Nov 12, 2018 at 1:58 PM Boris Brezillon boris.brezillon@bootlin.com wrote:
U-boot provides a mean to define default values for mtdids and mtdparts when they're not defined in the environment. Patch mtd_probe_devices() to use those default values when env_get("mtdparts") or env_get("mtdids") return NULL.
This implementation is based on the logic found in cmd/mtdparts.c.
Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Reported-by: Stefan Roese sr@denx.de Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Tested-by: Stefan Roese sr@denx.de
Changes in v2:
- none
Trigger travis-ci [1], will send v2 PR once all built fine.
Looks like mine is all green [1] ;-). And yes, now I have travis-ci set up to track my u-boot repo, so hopefully I won't end up submitting patches that regress the tests described in .travis.yaml in the future. Still, I'd like to insist on that this test result shouldn't replace human review, which I think is still valuable to spot subtle issues.
Also, I'd like to point that some of the failures caused by the spi-nand patchset are actually coming from the weird way MTD related config options are selected/defined in u-boot. This should probably be simplified somehow, but that's a different story.
Hi Boris,
U-boot provides a mean to define default values for mtdids and mtdparts when they're not defined in the environment. Patch mtd_probe_devices() to use those default values when env_get("mtdparts") or env_get("mtdids") return NULL.
This implementation is based on the logic found in cmd/mtdparts.c.
Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Reported-by: Stefan Roese sr@denx.de Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Tested-by: Stefan Roese sr@denx.de
Changes in v2:
- none
drivers/mtd/mtd_uboot.c | 62 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c index 7d7a11c990d6..1d0a505754f2 100644 --- a/drivers/mtd/mtd_uboot.c +++ b/drivers/mtd/mtd_uboot.c @@ -92,12 +92,70 @@ static void mtd_probe_uclass_mtd_devs(void) { } #endif
#if defined(CONFIG_MTD_PARTITIONS) +extern void board_mtdparts_default(const char **mtdids,
const char **mtdparts);+static const char *get_mtdids(void) +{
- __maybe_unused const char *mtdparts = NULL;
- const char *mtdids = env_get("mtdids");
- if (mtdids)
return mtdids;+#if defined(CONFIG_SYS_MTDPARTS_RUNTIME)
- board_mtdparts_default(&mtdids, &mtdparts);
+#elif defined(MTDIDS_DEFAULT)
- mtdids = MTDIDS_DEFAULT;
+#elif defined(CONFIG_MTDIDS_DEFAULT)
- mtdids = CONFIG_MTDIDS_DEFAULT;
+#endif
- if (mtdids)
env_set("mtdids", mtdids);- return mtdids;
+}
+#define MTDPARTS_MAXLEN 512
+static const char *get_mtdparts(void) +{
- __maybe_unused const char *mtdids = NULL;
- static char tmp_parts[MTDPARTS_MAXLEN];
- static bool use_defaults = true;
- const char *mtdparts = NULL;
- if (gd->flags & GD_FLG_ENV_READY)
mtdparts = env_get("mtdparts");- else if (env_get_f("mtdparts", tmp_parts,
sizeof(tmp_parts) != -1))
mtdparts = tmp_parts;- if (mtdparts || !use_defaults)
return mtdparts;+#if defined(CONFIG_SYS_MTDPARTS_RUNTIME)
- board_mtdparts_default(&mtdids, &mtdparts);
+#elif defined(MTDPARTS_DEFAULT)
- mtdparts = MTDPARTS_DEFAULT;
+#elif defined(CONFIG_MTDPARTS_DEFAULT)
- mtdparts = CONFIG_MTDPARTS_DEFAULT;
+#endif
- if (mtdparts)
env_set("mtdparts", mtdparts);- use_defaults = false;
- return mtdparts;
+}
int mtd_probe_devices(void) { static char *old_mtdparts; static char *old_mtdids;
- const char *mtdparts = env_get("mtdparts");
- const char *mtdids = env_get("mtdids");
- const char *mtdparts = get_mtdparts();
- const char *mtdids = get_mtdids(); bool remaining_partitions = true; struct mtd_info *mtd;
Reviewed-by: Lukasz Majewski lukma@denx.de
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
On 11/12/2018 09:28 AM, Boris Brezillon wrote:
U-boot provides a mean to define default values for mtdids and mtdparts when they're not defined in the environment. Patch mtd_probe_devices() to use those default values when env_get("mtdparts") or env_get("mtdids") return NULL.
This implementation is based on the logic found in cmd/mtdparts.c.
Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Reported-by: Stefan Roese sr@denx.de Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Tested-by: Stefan Roese sr@denx.de
Changes in v2:
- none
drivers/mtd/mtd_uboot.c | 62 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c index 7d7a11c990d6..1d0a505754f2 100644 --- a/drivers/mtd/mtd_uboot.c +++ b/drivers/mtd/mtd_uboot.c @@ -92,12 +92,70 @@ static void mtd_probe_uclass_mtd_devs(void) { } #endif
#if defined(CONFIG_MTD_PARTITIONS) +extern void board_mtdparts_default(const char **mtdids,
const char **mtdparts);+static const char *get_mtdids(void) +{
- __maybe_unused const char *mtdparts = NULL;
- const char *mtdids = env_get("mtdids");
- if (mtdids)
return mtdids;+#if defined(CONFIG_SYS_MTDPARTS_RUNTIME)
- board_mtdparts_default(&mtdids, &mtdparts);
+#elif defined(MTDIDS_DEFAULT)
- mtdids = MTDIDS_DEFAULT;
+#elif defined(CONFIG_MTDIDS_DEFAULT)
- mtdids = CONFIG_MTDIDS_DEFAULT;
+#endif
- if (mtdids)
env_set("mtdids", mtdids);- return mtdids;
+}
+#define MTDPARTS_MAXLEN 512
+static const char *get_mtdparts(void) +{
- __maybe_unused const char *mtdids = NULL;
- static char tmp_parts[MTDPARTS_MAXLEN];
- static bool use_defaults = true;
- const char *mtdparts = NULL;
- if (gd->flags & GD_FLG_ENV_READY)
mtdparts = env_get("mtdparts");- else if (env_get_f("mtdparts", tmp_parts, sizeof(tmp_parts) != -1))
sizeof(tmp_parts) != -1 is always true, the parenthesis are misplaced.
Also, include/common.h says that * env_get_f() - Look up the value of an environment variable (early) ... * @return value of variable, or NULL if not found
So the -1 is likely wrong too ?
On Tue, 13 Nov 2018 12:01:06 +0100 Marek Vasut marek.vasut@gmail.com wrote:
On 11/12/2018 09:28 AM, Boris Brezillon wrote:
U-boot provides a mean to define default values for mtdids and mtdparts when they're not defined in the environment. Patch mtd_probe_devices() to use those default values when env_get("mtdparts") or env_get("mtdids") return NULL.
This implementation is based on the logic found in cmd/mtdparts.c.
Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Reported-by: Stefan Roese sr@denx.de Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Tested-by: Stefan Roese sr@denx.de
Changes in v2:
- none
drivers/mtd/mtd_uboot.c | 62 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c index 7d7a11c990d6..1d0a505754f2 100644 --- a/drivers/mtd/mtd_uboot.c +++ b/drivers/mtd/mtd_uboot.c @@ -92,12 +92,70 @@ static void mtd_probe_uclass_mtd_devs(void) { } #endif
#if defined(CONFIG_MTD_PARTITIONS) +extern void board_mtdparts_default(const char **mtdids,
const char **mtdparts);+static const char *get_mtdids(void) +{
- __maybe_unused const char *mtdparts = NULL;
- const char *mtdids = env_get("mtdids");
- if (mtdids)
return mtdids;+#if defined(CONFIG_SYS_MTDPARTS_RUNTIME)
- board_mtdparts_default(&mtdids, &mtdparts);
+#elif defined(MTDIDS_DEFAULT)
- mtdids = MTDIDS_DEFAULT;
+#elif defined(CONFIG_MTDIDS_DEFAULT)
- mtdids = CONFIG_MTDIDS_DEFAULT;
+#endif
- if (mtdids)
env_set("mtdids", mtdids);- return mtdids;
+}
+#define MTDPARTS_MAXLEN 512
+static const char *get_mtdparts(void) +{
- __maybe_unused const char *mtdids = NULL;
- static char tmp_parts[MTDPARTS_MAXLEN];
- static bool use_defaults = true;
- const char *mtdparts = NULL;
- if (gd->flags & GD_FLG_ENV_READY)
mtdparts = env_get("mtdparts");- else if (env_get_f("mtdparts", tmp_parts, sizeof(tmp_parts) != -1))
sizeof(tmp_parts) != -1 is always true, the parenthesis are misplaced.
Also, include/common.h says that
- env_get_f() - Look up the value of an environment variable (early)
...
- @return value of variable, or NULL if not found
That's clearly not matching the implementation [1].
[1]https://elixir.bootlin.com/u-boot/v2018.11-rc3/source/cmd/nvedit.c#L680
On 11/13/2018 12:34 PM, Boris Brezillon wrote:
On Tue, 13 Nov 2018 12:01:06 +0100 Marek Vasut marek.vasut@gmail.com wrote:
On 11/12/2018 09:28 AM, Boris Brezillon wrote:
U-boot provides a mean to define default values for mtdids and mtdparts when they're not defined in the environment. Patch mtd_probe_devices() to use those default values when env_get("mtdparts") or env_get("mtdids") return NULL.
This implementation is based on the logic found in cmd/mtdparts.c.
Fixes: 5db66b3aee6f ("cmd: mtd: add 'mtd' command") Reported-by: Stefan Roese sr@denx.de Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com Tested-by: Stefan Roese sr@denx.de
Changes in v2:
- none
drivers/mtd/mtd_uboot.c | 62 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c index 7d7a11c990d6..1d0a505754f2 100644 --- a/drivers/mtd/mtd_uboot.c +++ b/drivers/mtd/mtd_uboot.c @@ -92,12 +92,70 @@ static void mtd_probe_uclass_mtd_devs(void) { } #endif
#if defined(CONFIG_MTD_PARTITIONS) +extern void board_mtdparts_default(const char **mtdids,
const char **mtdparts);+static const char *get_mtdids(void) +{
- __maybe_unused const char *mtdparts = NULL;
- const char *mtdids = env_get("mtdids");
- if (mtdids)
return mtdids;+#if defined(CONFIG_SYS_MTDPARTS_RUNTIME)
- board_mtdparts_default(&mtdids, &mtdparts);
+#elif defined(MTDIDS_DEFAULT)
- mtdids = MTDIDS_DEFAULT;
+#elif defined(CONFIG_MTDIDS_DEFAULT)
- mtdids = CONFIG_MTDIDS_DEFAULT;
+#endif
- if (mtdids)
env_set("mtdids", mtdids);- return mtdids;
+}
+#define MTDPARTS_MAXLEN 512
+static const char *get_mtdparts(void) +{
- __maybe_unused const char *mtdids = NULL;
- static char tmp_parts[MTDPARTS_MAXLEN];
- static bool use_defaults = true;
- const char *mtdparts = NULL;
- if (gd->flags & GD_FLG_ENV_READY)
mtdparts = env_get("mtdparts");- else if (env_get_f("mtdparts", tmp_parts, sizeof(tmp_parts) != -1))
sizeof(tmp_parts) != -1 is always true, the parenthesis are misplaced.
Also, include/common.h says that
- env_get_f() - Look up the value of an environment variable (early)
...
- @return value of variable, or NULL if not found
That's clearly not matching the implementation [1].
[1]https://elixir.bootlin.com/u-boot/v2018.11-rc3/source/cmd/nvedit.c#L680
Another thing to fix ...
participants (5)
-
Boris Brezillon -
Jagan Teki -
Lukasz Majewski -
Marek Vasut -
Miquel Raynal