From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Date: Tue, 30 Oct 2018 23:43:01 +0100 Subject: [U-Boot] [PATCH] cmd: mtdparts: Probe MTD devices in mtdparts_init() In-Reply-To: <20181030230250.736979d0@bbrezillon> References: <20181030095151.12442-1-sr@denx.de> <20181030110354.48e1736d@bbrezillon> <20181030114138.12ce4a8e@bbrezillon> <20181030230250.736979d0@bbrezillon> Message-ID: <20181030234301.3f926b1c@bbrezillon> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tue, 30 Oct 2018 23:02:50 +0100 Boris Brezillon wrote: > On Tue, 30 Oct 2018 11:59:13 +0100 > Stefan Roese wrote: > > > On 30.10.18 11:41, Boris Brezillon wrote: > > > On Tue, 30 Oct 2018 11:13:37 +0100 > > > Stefan Roese wrote: > > > > > >> Hi Boris, > > >> > > >> On 30.10.18 11:03, Boris Brezillon wrote: > > >>> On Tue, 30 Oct 2018 10:51:51 +0100 > > >>> Stefan Roese wrote: > > >>> > > >>>> Calling "mtdparts" currently fails when its called before any other mtd > > >>>> command (or ubi command) has been called. The MTD devices are not > > >>>> probed at this point and therefore it fails e.g. with this message: > > >>>> > > >>>> => mtdparts > > >>>> Device spi-nand0 not found! > > >>> > > >>> IIRC, we decided that mtdparts should not call mtd_probe_devices() to > > >>> encourage people to stop using it. > > >> > > >> I see. But I don't quite get how this missing call (and reslting > > >> error message) would encourage people to stop using it. > > > > > > You're right, this message does not encourage people to stop using > > > mtdparts on existing setups (mtdparts should work just fine on any MTD > > > devices except SPI NANDs) but it does discourage them from using it on > > > spi-nand devices since it returns an error. > > > > IMHO, that's more confusing than discouraging. > > > > >>>> > > >>>> This patch adds a call to mtd_probe_devices() to mtdparts_init() to > > >>>> solve this issue. This also fixes a problem when calling "ubi part" > > >>>> as first flash storage related command. > > >>> > > >>> Hm, this one is unexpected. Miquel, any idea why this happens. Do we > > >>> need to enable a specific option if we want mtd_probe_devices() to be > > >>> called in the ubi part path? > > >> > > >> Please note that "ubi part part-foo" does still work. It only > > >> prints this error message before attaching the MTD partition. > > >> The error is printed because of this call-chain: > > >> > > >> ubi_part() > > >> -> ubi_detach() > > >> -> mtdparts_init() > > >> > > >> So again, mtdparts_init() is called without the MTD devices > > >> being probed. > > > > > > I guess we forgot to remove this mtdparts_init() call from the detach > > > path. I think it's no longer needed since we now call > > > mtd_probe_devices() in ubi_part(), and mtd_probe_devices() will take > > > care of creating MTD partitions based on the mtdparts= and mtdids= > > > variables. > > > > A quick test reveals that this removal does not remove the > > error message. Instead the command does not work anymore at > > all: > > > > => ubi part nand > > Partition nand not found! > > > > Before (and without my patch) its this: > > > > => ubi part nand > > Device spi-nand0 not found! > > Error initializing mtdparts! > > ubi0: attaching mtd2 > > ubi0: scanning is finished > > ubi0: attached mtd2 (name "nand", size 128 MiB) > > I think I found what's missing in mtd_probe_devices(): we don't use the > default mtdparts and mtdids when those env vars are NULL (see what's > done in mtdparts_init() to handle this case [1]). Can you try with the following diff applied? --->8--- 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 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;