[U-Boot] [PATCH] cmd: mtdparts: Probe MTD devices in mtdparts_init()

[Boris Brezillon]

On Tue, 30 Oct 2018 23:02:50 +0100
Boris Brezillon <boris.brezillon at bootlin.com> wrote:

> On Tue, 30 Oct 2018 11:59:13 +0100
> Stefan Roese <sr at denx.de> wrote:
> 
> > On 30.10.18 11:41, Boris Brezillon wrote:  
> > > On Tue, 30 Oct 2018 11:13:37 +0100
> > > Stefan Roese <sr at denx.de> wrote:
> > >     
> > >> Hi Boris,
> > >>
> > >> On 30.10.18 11:03, Boris Brezillon wrote:    
> > >>> On Tue, 30 Oct 2018 10:51:51 +0100
> > >>> Stefan Roese <sr at denx.de> 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;