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

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

[Boris Brezillon]

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>
Reviewed-by: Lukasz Majewski <lukma@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);
+
+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;
 
-- 
2.17.1

[U-Boot] [PATCH v3 2/5] dfu: nand: Add missing dependency on CMD_MTDPARTS

[Boris Brezillon]

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>
Reviewed-by: Lukasz Majewski <lukma@denx.de>
---
Changes in v3:
- Add Lujasz R-b

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.
-- 
2.17.1

[U-Boot] [PATCH v3 3/5] mtd: Make {MTDIDS, MTDPARTS}_DEFAULT visible when MTD_PARTITIONS is selected

[Boris Brezillon]

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>
Reviewed-by: Lukasz Majewski <lukma@denx.de>
---
Changes in v3:
- Add Lujasz R-b

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
-- 
2.17.1

[U-Boot] [PATCH v3 4/5] cmd: ubi: Remove useless call to mtdparts_init()

[Boris Brezillon]

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>
Reviewed-by: Lukasz Majewski <lukma@denx.de>
---
Changes in v3:
- Add Lujasz R-b

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
-- 
2.17.1

[U-Boot] [PATCH v3 5/5] mtd: Drop duplicate MTD_PARTITIONS Kconfig option

[Boris Brezillon]

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>
Reviewed-by: Lukasz Majewski <lukma@denx.de>
---
Changes in v3:
- Add Lujasz R-b

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
-- 
2.17.1

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

[Marek Vasut]

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@denx.de>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Tested-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Lukasz Majewski <lukma@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 ? 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.

> +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;
>  
> 


-- 
Best regards,
Marek Vasut

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

[Boris Brezillon]

Hi Marek,

On Tue, 13 Nov 2018 13:19:52 +0100
Marek Vasut <marek.vasut@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@denx.de>
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > Tested-by: Stefan Roese <sr@denx.de>
> > Reviewed-by: Lukasz Majewski <lukma@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.

It seems you don't want this series to be queued for the v2018.11.
That's your choice, and that's also your responsibility since you're
blocking something that is fixing another known bug. I clearly won't
fight for that, just know that the fix was posted 2 weeks ago, and
until Saturday I had no news. Once Jagan reported the build failure, I
came up with a fix in less than a day. You found a bug today, and I
posted a new version right away. So I think I did everything I could to
get this addressed in time. 

Regards,

Boris

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

[Tom Rini]

On Tue, Nov 13, 2018 at 01:39:51PM +0100, Boris Brezillon wrote:
> Hi Marek,
> 
> On Tue, 13 Nov 2018 13:19:52 +0100
> Marek Vasut <marek.vasut@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@denx.de>
> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > Tested-by: Stefan Roese <sr@denx.de>
> > > Reviewed-by: Lukasz Majewski <lukma@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.

Towards the end of a release cycle (regardless of how long it is), we
should aim for being more conservative in our code changes towards the
end.  At this particular point in time, doing it this way is fine.
Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181113/2fcb3baa/attachment.sig>

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

[Marek Vasut]

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@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@denx.de>
>>> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
>>> Tested-by: Stefan Roese <sr@denx.de>
>>> Reviewed-by: Lukasz Majewski <lukma@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.

> It seems you don't want this series to be queued for the v2018.11.

Please stop guessing what I do or do not want. What I do not want is
having patches with obvious and known bugs being queued, I think that is
a reasonable requirement ?

-- 
Best regards,
Marek Vasut

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

[Tom Rini]

On Tue, Nov 13, 2018 at 03:10:14PM +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 <marek.vasut@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@denx.de>
> >>> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> >>> Tested-by: Stefan Roese <sr@denx.de>
> >>> Reviewed-by: Lukasz Majewski <lukma@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.

I don't think that's worth it.  We have exactly one user of this fixup
and weak functions do grow the binaries.

> > It seems you don't want this series to be queued for the v2018.11.
> 
> Please stop guessing what I do or do not want. What I do not want is
> having patches with obvious and known bugs being queued, I think that is
> a reasonable requirement ?

It's true that none of us can read minds.  But at this point in this
patch series cycle, no, you're not talking about "known bugs".  Yes,
externs should always be in header files.  But a quick and simple git
grep shows 171 .c files with extern in them.  So I hardly think this is
a critical must change thing right now.  Deserving of a spot on the TODO
list?  Yes.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181113/1f118789/attachment.sig>

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

[Boris Brezillon]

On Tue, 13 Nov 2018 15:10:14 +0100
Marek Vasut <marek.vasut@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@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@denx.de>
> >>> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> >>> Tested-by: Stefan Roese <sr@denx.de>
> >>> Reviewed-by: Lukasz Majewski <lukma@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).

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

[Tom Rini]

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@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@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@denx.de>
> > >>> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > >>> Tested-by: Stefan Roese <sr@denx.de>
> > >>> Reviewed-by: Lukasz Majewski <lukma@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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20181113/f27a4f5c/attachment.sig>

[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@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@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@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@denx.de>
> > > >>> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > >>> Tested-by: Stefan Roese <sr@denx.de>
> > > >>> Reviewed-by: Lukasz Majewski <lukma@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.

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

[Marek Vasut]

On 11/13/2018 03:37 PM, Boris Brezillon wrote:
> On Tue, 13 Nov 2018 15:10:14 +0100
> Marek Vasut <marek.vasut@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@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@denx.de>
>>>>> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
>>>>> Tested-by: Stefan Roese <sr@denx.de>
>>>>> Reviewed-by: Lukasz Majewski <lukma@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).

Fine by me , and yes, that can be put on ToDo.

-- 
Best regards,
Marek Vasut