[U-Boot] [PATCH v3 1/5] mtd: Use default mtdparts/mtids when not defined in the environment

[Boris Brezillon]

On Tue, 13 Nov 2018 09:42:25 -0500
Tom Rini <trini at konsulko.com> wrote:

> On Tue, Nov 13, 2018 at 03:37:26PM +0100, Boris Brezillon wrote:
> > On Tue, 13 Nov 2018 15:10:14 +0100
> > Marek Vasut <marek.vasut at gmail.com> wrote:
> >   
> > > On 11/13/2018 01:39 PM, Boris Brezillon wrote:  
> > > > Hi Marek,
> > > > 
> > > > On Tue, 13 Nov 2018 13:19:52 +0100
> > > > Marek Vasut <marek.vasut at gmail.com> wrote:
> > > >     
> > > >> On 11/13/2018 12:43 PM, 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 at denx.de>
> > > >>> Signed-off-by: Boris Brezillon <boris.brezillon at bootlin.com>
> > > >>> Tested-by: Stefan Roese <sr at denx.de>
> > > >>> Reviewed-by: Lukasz Majewski <lukma at denx.de>
> > > >>> ---
> > > >>> Changes in v3:
> > > >>> - Fix env_get_f() call
> > > >>> - Add Lukasz R-b
> > > >>>
> > > >>> 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..5ca560c96879 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);      
> > > >>
> > > >> Why is there this extern ?    
> > > > 
> > > > extern is not needed indeed.
> > > >     
> > > >> This should use a prototype of function
> > > >> defined in a header file. Once someone changes the prototype and forgets
> > > >> to update this location, it'll be a disaster.    
> > > > 
> > > > When I provide a fix I try to avoid dependencies on changes that are
> > > > not absolutely required hence the decision to keep this function
> > > > prototype locally defined.    
> > > 
> > > I am also expecting a patch which cleans this up then.
> > > I think __weak function would be appropriate.  
> > 
> > Well, I think the best would be to get rid of this function. It's only
> > implemented by board/isee/igep00x0/igep00x0.c, and we might be able to
> > use CONFIG_{MTDPARTS,MTDIDS}_DEFAULT to replace it:
> > 
> > CONFIG_MTDPARTS_DEFAULT "omap2-nand:Xk(SPL),-(UBI);omap2-onenand:Yk(SPL),-(UBI)"
> > CONFIG_MTDIDS_DEFAULT "nand0=omap2-nand,onenand0=omap2-onenand"
> > 
> > This assumes we know the eraseblock size of the NAND and OneNAND devices of
> > course (X and Y in the above def).  
> 
> I'm not sure, I think it's doing run-time checks to see if we're on the
> NAND or OneNAND variant.

Yes, but by having both of them defined in the mtdids/mtdparts defaults
we solve this problem. The only problem we might have is if they those
boards use NAND/OneNAND parts exposing different eraseblock size.