From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Date: Tue, 13 Nov 2018 15:46:40 +0100 Subject: [U-Boot] [PATCH v3 1/5] mtd: Use default mtdparts/mtids when not defined in the environment In-Reply-To: <20181113144225.GA11247@bill-the-cat> References: <20181113114313.30387-1-boris.brezillon@bootlin.com> <20181113133951.12ffd7b6@bbrezillon> <1272596c-a789-7a93-0198-3aacdd169194@gmail.com> <20181113153726.5a586d05@bbrezillon> <20181113144225.GA11247@bill-the-cat> Message-ID: <20181113154640.5d1a5ec9@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, 13 Nov 2018 09:42:25 -0500 Tom Rini 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 wrote: > > > > > On 11/13/2018 01:39 PM, Boris Brezillon wrote: > > > > Hi Marek, > > > > > > > > On Tue, 13 Nov 2018 13:19:52 +0100 > > > > Marek Vasut 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 > > > >>> Signed-off-by: Boris Brezillon > > > >>> Tested-by: Stefan Roese > > > >>> Reviewed-by: Lukasz Majewski > > > >>> --- > > > >>> 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.