[U-Boot] [PATCH 0/6] cmd: mtdparts: Add support for runtime generated defaults

[U-Boot] [PATCH 0/6] cmd: mtdparts: Add support for runtime generated defaults

[Ladislav Michl]

Greetings,

this patch serie is a by-product of UBI SPL patchset, see
http://lists.denx.de/pipermail/u-boot/2016-January/242962.html (and earlier)
First three patches are just bugfixes. With fourth one, controversy
starts, but here can be easily overcome by a config option for a boards
wanting to use defaults by default :-) Core change is fifth patch, it is
more likely a draft, documentation is missing, etc...
As always, comments and suggestions even for completely different
solution welcome.

	ladis

[U-Boot] [PATCH 1/6] cmd: mtdparts: fix mtdparts variable presence confusion in mtdparts_init

[Ladislav Michl]

A private buffer is used to read mtdparts variable from non-relocated
environment. A pointer to that buffer is returned unconditionally,
confusing later test for variable presence in the environment.
Fix it by returning NULL when getenv_f fails.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 cmd/mtdparts.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
index 44b2c3a..3a88a10 100644
--- a/cmd/mtdparts.c
+++ b/cmd/mtdparts.c
@@ -1720,11 +1720,13 @@ int mtdparts_init(void)
 	 * before the env is relocated, then we need to use our own stack
 	 * buffer.  gd->env_buf will be too small.
 	 */
-	if (gd->flags & GD_FLG_ENV_READY) {
+	if (gd->flags & GD_FLG_ENV_READY)
 		parts = getenv("mtdparts");
-	} else {
-		parts = tmp_parts;
-		getenv_f("mtdparts", tmp_parts, MTDPARTS_MAXLEN);
+	else {
+		if (getenv_f("mtdparts", tmp_parts, MTDPARTS_MAXLEN) != -1)
+			parts = tmp_parts;
+		else
+			parts = NULL;
 	}
 	current_partition = getenv("partition");
 
-- 
2.1.4

[U-Boot] [PATCH 2/6] cmd: mtdparts: fix null pointer dereference in parse_mtdparts

[Ladislav Michl]

In case there is no mtdparts variable in relocated environment,
NULL is assigned to p, which is later fed to strncpy.
Also function parameter mtdparts is completely ignored, so use it
in case mtdparts variable is not found in environment. This
parameter is checked not to be NULL in caller.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 cmd/mtdparts.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
index 3a88a10..995cb87 100644
--- a/cmd/mtdparts.c
+++ b/cmd/mtdparts.c
@@ -1524,7 +1524,7 @@ static int spread_partitions(void)
  */
 static int parse_mtdparts(const char *const mtdparts)
 {
-	const char *p = mtdparts;
+	const char *p;
 	struct mtd_device *dev;
 	int err = 1;
 	char tmp_parts[MTDPARTS_MAXLEN];
@@ -1538,20 +1538,25 @@ static int parse_mtdparts(const char *const mtdparts)
 	}
 
 	/* re-read 'mtdparts' variable, mtd_devices_init may be updating env */
-	if (gd->flags & GD_FLG_ENV_READY) {
+	if (gd->flags & GD_FLG_ENV_READY)
 		p = getenv("mtdparts");
-	} else {
-		p = tmp_parts;
-		getenv_f("mtdparts", tmp_parts, MTDPARTS_MAXLEN);
+	else {
+		if (getenv_f("mtdparts", tmp_parts, MTDPARTS_MAXLEN) != -1)
+			p = tmp_parts;
+		else
+			p = NULL;
 	}
 
+	if (!p)
+		p = mtdparts;
+
 	if (strncmp(p, "mtdparts=", 9) != 0) {
 		printf("mtdparts variable doesn't start with 'mtdparts='\n");
 		return err;
 	}
 	p += 9;
 
-	while (p && (*p != '\0')) {
+	while (*p != '\0') {
 		err = 1;
 		if ((device_parse(p, &p, &dev) != 0) || (!dev))
 			break;
@@ -1569,12 +1574,10 @@ static int parse_mtdparts(const char *const mtdparts)
 		list_add_tail(&dev->link, &devices);
 		err = 0;
 	}
-	if (err == 1) {
+	if (err == 1)
 		device_delall(&devices);
-		return 1;
-	}
 
-	return 0;
+	return err;
 }
 
 /**
-- 
2.1.4

[U-Boot] [PATCH 3/6] cmd: mtdparts: consolidate mtdparts reading from env

[Ladislav Michl]

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 cmd/mtdparts.c | 42 ++++++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
index 995cb87..7860ed9 100644
--- a/cmd/mtdparts.c
+++ b/cmd/mtdparts.c
@@ -1516,6 +1516,23 @@ static int spread_partitions(void)
 #endif /* CONFIG_CMD_MTDPARTS_SPREAD */
 
 /**
+ * The mtdparts variable tends to be long. If we need to access it
+ * before the env is relocated, then we need to use our own stack
+ * buffer.  gd->env_buf will be too small.
+ *
+ * @param buf temporary buffer pointer MTDPARTS_MAXLEN long
+ * @return mtdparts variable string, NULL if not found
+ */
+static const char *getenv_mtdparts(char *buf)
+{
+	if (gd->flags & GD_FLG_ENV_READY)
+		return getenv("mtdparts");
+	if (getenv_f("mtdparts", buf, MTDPARTS_MAXLEN) != -1)
+		return buf;
+	return NULL;
+}
+
+/**
  * Accept character string describing mtd partitions and call device_parse()
  * for each entry. Add created devices to the global devices list.
  *
@@ -1538,15 +1555,7 @@ static int parse_mtdparts(const char *const mtdparts)
 	}
 
 	/* re-read 'mtdparts' variable, mtd_devices_init may be updating env */
-	if (gd->flags & GD_FLG_ENV_READY)
-		p = getenv("mtdparts");
-	else {
-		if (getenv_f("mtdparts", tmp_parts, MTDPARTS_MAXLEN) != -1)
-			p = tmp_parts;
-		else
-			p = NULL;
-	}
-
+	p = getenv_mtdparts(tmp_parts);
 	if (!p)
 		p = mtdparts;
 
@@ -1691,6 +1700,7 @@ static int parse_mtdids(const char *const ids)
 	return 0;
 }
 
+
 /**
  * Parse and initialize global mtdids mapping and create global
  * device/partition list.
@@ -1718,19 +1728,7 @@ int mtdparts_init(void)
 
 	/* get variables */
 	ids = getenv("mtdids");
-	/*
-	 * The mtdparts variable tends to be long. If we need to access it
-	 * before the env is relocated, then we need to use our own stack
-	 * buffer.  gd->env_buf will be too small.
-	 */
-	if (gd->flags & GD_FLG_ENV_READY)
-		parts = getenv("mtdparts");
-	else {
-		if (getenv_f("mtdparts", tmp_parts, MTDPARTS_MAXLEN) != -1)
-			parts = tmp_parts;
-		else
-			parts = NULL;
-	}
+	parts = getenv_mtdparts(tmp_parts);
 	current_partition = getenv("partition");
 
 	/* save it for later parsing, cannot rely on current partition pointer
-- 
2.1.4

[U-Boot] [PATCH 4/6] cmd: mtdparts: use defaults by default

[Ladislav Michl]

Boards which are defining default mtdparts often need them early
in boot process (to load environment from UBI volume, for example).
This is currently solved by adding mtdparts and mtdids variable
definitions also to default environment. With this change, default
partitions are used by default unless explicitely deleted or
redefined.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 cmd/mtdparts.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
index 7860ed9..53074a1 100644
--- a/cmd/mtdparts.c
+++ b/cmd/mtdparts.c
@@ -142,6 +142,8 @@ static struct list_head devices;
 struct mtd_device *current_mtd_dev = NULL;
 u8 current_mtd_partnum = 0;
 
+u8 use_defaults;
+
 static struct part_info* mtd_part_info(struct mtd_device *dev, unsigned int part_num);
 
 /* command line only routines */
@@ -1723,6 +1725,7 @@ int mtdparts_init(void)
 		memset(last_ids, 0, MTDIDS_MAXLEN);
 		memset(last_parts, 0, MTDPARTS_MAXLEN);
 		memset(last_partition, 0, PARTITION_MAXLEN);
+		use_defaults = 1;
 		initialized = 1;
 	}
 
@@ -1761,10 +1764,16 @@ int mtdparts_init(void)
 		return 1;
 	}
 
-	/* do no try to use defaults when mtdparts variable is not defined,
-	 * just check the length */
-	if (!parts)
-		printf("mtdparts variable not set, see 'help mtdparts'\n");
+	/* use defaults when mtdparts variable is not defined
+	 * once mtdparts is saved environment, drop use_defaults flag */
+	if (!parts) {
+		if (mtdparts_default && use_defaults) {
+			parts = mtdparts_default;
+			if (setenv("mtdparts", (char *)parts) == 0)
+				use_defaults = 0;
+		} else
+			printf("mtdparts variable not set, see 'help mtdparts'\n");
+	}
 
 	if (parts && (strlen(parts) > MTDPARTS_MAXLEN - 1)) {
 		printf("mtdparts too long (> %d)\n", MTDPARTS_MAXLEN);
@@ -1936,9 +1945,10 @@ static int do_mtdparts(cmd_tbl_t *cmdtp, int flag, int argc,
 {
 	if (argc == 2) {
 		if (strcmp(argv[1], "default") == 0) {
-			setenv("mtdids", (char *)mtdids_default);
-			setenv("mtdparts", (char *)mtdparts_default);
+			setenv("mtdids", NULL);
+			setenv("mtdparts", NULL);
 			setenv("partition", NULL);
+			use_defaults = 1;
 
 			mtdparts_init();
 			return 0;
-- 
2.1.4

[U-Boot] [PATCH 5/6] cmd: mtdparts: support runtime generated mtdparts

[Ladislav Michl]

Some CPUs contains boot ROM code capable reading first few blocks
(where SPL resides) of NAND flash and executing it. It is wise to
create separate partition here for SPL. As block size depends on
NAND chip used, we could either use worst case (biggest) partition
size or base its size on actual block size. This patch adds support
for the latter option.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 cmd/mtdparts.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
index 53074a1..71c7acb 100644
--- a/cmd/mtdparts.c
+++ b/cmd/mtdparts.c
@@ -109,17 +109,20 @@ DECLARE_GLOBAL_DATA_PTR;
 #define MTD_WRITEABLE_CMD		1
 
 /* default values for mtdids and mtdparts variables */
-#if defined(MTDIDS_DEFAULT)
-static const char *const mtdids_default = MTDIDS_DEFAULT;
-#else
-static const char *const mtdids_default = NULL;
+#if !defined(MTDIDS_DEFAULT)
+#define MTDIDS_DEFAULT NULL
 #endif
-
-#if defined(MTDPARTS_DEFAULT)
-static const char *const mtdparts_default = MTDPARTS_DEFAULT;
+#if !defined(MTDPARTS_DEFAULT)
+#define MTDPARTS_DEFAULT NULL
+#endif
+#if defined(CONFIG_SYS_MTDPARTS_DEFAULT_RUNTIME)
+#define MTD_DEFAULTS_READONLY
+extern void board_mtdparts_default(const char **mtdids, const char **mtdparts);
 #else
-static const char *const mtdparts_default = NULL;
+#define MTD_DEFAULTS_READONLY const
 #endif
+static const char *MTD_DEFAULTS_READONLY mtdids_default = MTDIDS_DEFAULT;
+static const char *MTD_DEFAULTS_READONLY mtdparts_default = MTDPARTS_DEFAULT;
 
 /* copies of last seen 'mtdids', 'mtdparts' and 'partition' env variables */
 #define MTDIDS_MAXLEN		128
@@ -1725,6 +1728,9 @@ int mtdparts_init(void)
 		memset(last_ids, 0, MTDIDS_MAXLEN);
 		memset(last_parts, 0, MTDPARTS_MAXLEN);
 		memset(last_partition, 0, PARTITION_MAXLEN);
+#if defined(CONFIG_SYS_MTDPARTS_DEFAULT_RUNTIME)
+		board_mtdparts_default(&mtdids_default, &mtdparts_default);
+#endif
 		use_defaults = 1;
 		initialized = 1;
 	}
-- 
2.1.4

[U-Boot] [PATCH 6/6] igep00x0: generate default mtdparts according NAND chip used

[Ladislav Michl]

Just a sample implementation. Eventually become part of of UBI SPL
patch series.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
 board/isee/igep00x0/igep00x0.c   | 16 ++++++++++++++++
 include/configs/omap3_igep00x0.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/board/isee/igep00x0/igep00x0.c b/board/isee/igep00x0/igep00x0.c
index 759daef..beaab79 100644
--- a/board/isee/igep00x0/igep00x0.c
+++ b/board/isee/igep00x0/igep00x0.c
@@ -18,6 +18,7 @@
 #include <asm/arch/mux.h>
 #include <asm/arch/sys_proto.h>
 #include <asm/mach-types.h>
+#include <linux/mtd/mtd.h>
 #include "igep00x0.h"
 
 DECLARE_GLOBAL_DATA_PTR;
@@ -184,6 +185,21 @@ int misc_init_r(void)
 	return 0;
 }
 
+void board_mtdparts_default(const char **mtdids, const char **mtdparts)
+{
+	struct mtd_info *mtd = get_mtd_device(NULL, 0);
+	if (mtd) {
+		static char ids[24];
+		static char parts[48];
+		const char *linux_name = "omap2-nand";
+		snprintf(ids, sizeof(ids), "%s=%s", mtd->name, linux_name);
+		snprintf(parts, sizeof(parts), "mtdparts=%s:%dk(SPL),-(UBI)",
+		         linux_name, 4 * mtd->erasesize >> 10);
+		*mtdids = ids;
+		*mtdparts = parts;
+	}
+}
+
 /*
  * Routine: set_muxconf_regs
  * Description: Setting up the configuration Mux registers specific to the
diff --git a/include/configs/omap3_igep00x0.h b/include/configs/omap3_igep00x0.h
index 364d759..41782e3 100644
--- a/include/configs/omap3_igep00x0.h
+++ b/include/configs/omap3_igep00x0.h
@@ -146,6 +146,7 @@
 
 #define CONFIG_RBTREE
 #define CONFIG_MTD_PARTITIONS
+#define CONFIG_SYS_MTDPARTS_DEFAULT_RUNTIME
 
 /* OneNAND boot config */
 #ifdef CONFIG_BOOT_ONENAND
-- 
2.1.4

[U-Boot] [PATCH 5/6] cmd: mtdparts: support runtime generated mtdparts

[Michal Suchanek]

Hello,

On 5 June 2016 at 19:43, Ladislav Michl <ladis@linux-mips.org> wrote:
> Some CPUs contains boot ROM code capable reading first few blocks
> (where SPL resides) of NAND flash and executing it. It is wise to
> create separate partition here for SPL. As block size depends on
> NAND chip used, we could either use worst case (biggest) partition
> size or base its size on actual block size. This patch adds support
> for the latter option.
>

There is similar problem on sunxi.

Given this flash is non-removable and has many pins you are unlikely
going to encounter its content on any system that was not loaded with
u-boot.

Still Linux can only produce fixed size mtdparts. You can prepend
perfectly sized mtdparts for u-boot but until Linux is taught to
follow that with its own partitions without gap you still need to use
the worst case scenario for the start of the Linux partitions.

On sunxi the range of supported block sizes and the size of the
SPL+U-BOOT are not large so this is generally not worth the effort.

If support for this is attempted Linux should be probably taught to
get mtdparts in pages and eraseblocks and then the default mtdparts
can be in those.

If on the other hand you generate the fixed layout for Linux
partitions on the fly and this patch changes how the Linux partitions
are generated it can happen that the Linux partitions are at different
place with different versions of u-boot giving potentially disastrous
results.

Thanks

Miichal

[U-Boot] [PATCH 5/6] cmd: mtdparts: support runtime generated mtdparts

[Ladislav Michl]

On Sun, Jun 05, 2016 at 07:58:46PM +0200, Michal Suchanek wrote:
> Hello,
> 
> On 5 June 2016 at 19:43, Ladislav Michl <ladis@linux-mips.org> wrote:
> > Some CPUs contains boot ROM code capable reading first few blocks
> > (where SPL resides) of NAND flash and executing it. It is wise to
> > create separate partition here for SPL. As block size depends on
> > NAND chip used, we could either use worst case (biggest) partition
> > size or base its size on actual block size. This patch adds support
> > for the latter option.
> >
> 
> There is similar problem on sunxi.
> 
> Given this flash is non-removable and has many pins you are unlikely
> going to encounter its content on any system that was not loaded with
> u-boot.
> 
> Still Linux can only produce fixed size mtdparts. You can prepend
> perfectly sized mtdparts for u-boot but until Linux is taught to
> follow that with its own partitions without gap you still need to use
> the worst case scenario for the start of the Linux partitions.

I didn't get 'fixed size mtdparts' part. Linux supports cmdline
partition layout passing since the dawn of mtd subsystem (I used it more
than a decade ago for netstar board). Finally that's a reason mtdparts
implementation in U-Boot is done this way. Both U-Boot and Linux (can)
see the same partition layout as it is passed either via kernel cmdline
or device tree blob.

> On sunxi the range of supported block sizes and the size of the
> SPL+U-BOOT are not large so this is generally not worth the effort.
> 
> If support for this is attempted Linux should be probably taught to
> get mtdparts in pages and eraseblocks and then the default mtdparts
> can be in those.
> 
> If on the other hand you generate the fixed layout for Linux
> partitions on the fly and this patch changes how the Linux partitions
> are generated it can happen that the Linux partitions are at different
> place with different versions of u-boot giving potentially disastrous
> results.

Unless I'm missing something, partition layout is passed to the kernel
from the bootloader. So if kernel ends up with a different layout than
a bootloader, there's a bug somewhere and that should be fixed.

Regards,
	ladis

[U-Boot] [PATCH 5/6] cmd: mtdparts: support runtime generated mtdparts

[Michal Suchanek]

On 5 June 2016 at 20:23, Ladislav Michl <ladis@linux-mips.org> wrote:
> On Sun, Jun 05, 2016 at 07:58:46PM +0200, Michal Suchanek wrote:
>> Hello,
>>
>> On 5 June 2016 at 19:43, Ladislav Michl <ladis@linux-mips.org> wrote:
>> > Some CPUs contains boot ROM code capable reading first few blocks
>> > (where SPL resides) of NAND flash and executing it. It is wise to
>> > create separate partition here for SPL. As block size depends on
>> > NAND chip used, we could either use worst case (biggest) partition
>> > size or base its size on actual block size. This patch adds support
>> > for the latter option.
>> >
>>
>> There is similar problem on sunxi.
>>
>> Given this flash is non-removable and has many pins you are unlikely
>> going to encounter its content on any system that was not loaded with
>> u-boot.
>>
>> Still Linux can only produce fixed size mtdparts. You can prepend
>> perfectly sized mtdparts for u-boot but until Linux is taught to
>> follow that with its own partitions without gap you still need to use
>> the worst case scenario for the start of the Linux partitions.
>
> I didn't get 'fixed size mtdparts' part. Linux supports cmdline
> partition layout passing since the dawn of mtd subsystem (I used it more

Sure.

It does not, however, support size in anything but some kind of fixed
size blocks.

It cannot adjust the layout for different media page size or erase
block size. It cannot specify that a partition follows after another
or extends to the end of medium.

What you specify on the commandline is in no way related to the actual
medium you are partitioning.

> than a decade ago for netstar board). Finally that's a reason mtdparts
> implementation in U-Boot is done this way. Both U-Boot and Linux (can)
> see the same partition layout as it is passed either via kernel cmdline
> or device tree blob.
>
>> On sunxi the range of supported block sizes and the size of the
>> SPL+U-BOOT are not large so this is generally not worth the effort.
>>
>> If support for this is attempted Linux should be probably taught to
>> get mtdparts in pages and eraseblocks and then the default mtdparts
>> can be in those.
>>
>> If on the other hand you generate the fixed layout for Linux
>> partitions on the fly and this patch changes how the Linux partitions
>> are generated it can happen that the Linux partitions are at different
>> place with different versions of u-boot giving potentially disastrous
>> results.
>
> Unless I'm missing something, partition layout is passed to the kernel
> from the bootloader. So if kernel ends up with a different layout than
> a bootloader, there's a bug somewhere and that should be fixed.

That's not it. The problem is that if this patch changes the layout
then building u-boot before this patch gives one layout and after this
patch it gives another layout effectively changing the partitioning.
All to save a few megabytes of a several gigiabyte medium. And if you
say that people can always set the partitioning by hand then it
completely defeats the purpose of fine-tuning the default in the first
place.

If on the other hand Linux got support for sizing partitions in nand
pages or eraseblocks and a patch changed the mtdparts layout to use
the new units then either both u-boot and Linux support the units or
parsing the partition fails. So it's safe and flexible and more
general and probably even less work on u-boot's part.

Thanks

Michal

[U-Boot] [PATCH 5/6] cmd: mtdparts: support runtime generated mtdparts

[Ladislav Michl]

On Mon, Jun 06, 2016 at 09:08:47AM +0200, Michal Suchanek wrote:
> On 5 June 2016 at 20:23, Ladislav Michl <ladis@linux-mips.org> wrote:
> > On Sun, Jun 05, 2016 at 07:58:46PM +0200, Michal Suchanek wrote:
> >> There is similar problem on sunxi.
> >>
> >> Given this flash is non-removable and has many pins you are unlikely
> >> going to encounter its content on any system that was not loaded with
> >> u-boot.
> >>
> >> Still Linux can only produce fixed size mtdparts. You can prepend
> >> perfectly sized mtdparts for u-boot but until Linux is taught to
> >> follow that with its own partitions without gap you still need to use
> >> the worst case scenario for the start of the Linux partitions.
> >
> > I didn't get 'fixed size mtdparts' part. Linux supports cmdline
> > partition layout passing since the dawn of mtd subsystem (I used it more
> 
> Sure.
> 
> It does not, however, support size in anything but some kind of fixed
> size blocks.
> 
> It cannot adjust the layout for different media page size or erase
> block size. It cannot specify that a partition follows after another
> or extends to the end of medium.

That's not actually needed, as we know both page size and erase block size.
Therefore it is easy to make layout respect those values, which was actual
motivation for this patch. The later is simply not true, as you can
certainly create a partition that extends to the end of medium:
mtdparts=omap2-nand.0:512k(xloader),1920k(uboot),128k(uboot-env),10m(boot),-(rootfs)
Above is something that I want to avoid and replace with:
mtdparts=omap2-nand.0:<4*size_of_block>(SPL),-(UBI)
(actually this is going to be translated to DTB before booting kernel)
More free eraseblock for UBI, the better... Boot ROM reads from first
(4) bare sector(s) and everything else is stored in UBI volumes. This
is currently the only reliable way to make a use of MLC NAND.

> What you specify on the commandline is in no way related to the actual
> medium you are partitioning.

What I'm specifying on cmdline (DTB) is what I'm going to get. I has always
worked this way. Well, some drivers of even board support files had
partitioning hardcoded, but we are trying to get rid of that.

[snip]
> > Unless I'm missing something, partition layout is passed to the kernel
> > from the bootloader. So if kernel ends up with a different layout than
> > a bootloader, there's a bug somewhere and that should be fixed.
> 
> That's not it. The problem is that if this patch changes the layout
> then building u-boot before this patch gives one layout and after this
> patch it gives another layout effectively changing the partitioning.
> All to save a few megabytes of a several gigiabyte medium. And if you
> say that people can always set the partitioning by hand then it
> completely defeats the purpose of fine-tuning the default in the first
> place.

U-Boot never prevented people from shooting their own leg, therefore
there's no 'feature' preventing them to change default layout. They
are still free to do whatever they want it they find it useful.
Also I'm not proposing to change layout for exiting boards, just
proving way to do it. igep00x0 boards had buggy NAND support since
begining, so while fixing that, let's do it right way. Note that
this feature is completely optional.

> If on the other hand Linux got support for sizing partitions in nand
> pages or eraseblocks and a patch changed the mtdparts layout to use
> the new units then either both u-boot and Linux support the units or
> parsing the partition fails. So it's safe and flexible and more
> general and probably even less work on u-boot's part.

Okay, specifing size in therms of eraseblock would solve my problem
and I have to admit I'm not aware such a thing exists. Any pointer to
the patch introducing that change?

Thank you,
	ladis

[U-Boot] [PATCH 5/6] cmd: mtdparts: support runtime generated mtdparts

[Michal Suchanek]

On 6 June 2016 at 09:48, Ladislav Michl <ladis@linux-mips.org> wrote:
> On Mon, Jun 06, 2016 at 09:08:47AM +0200, Michal Suchanek wrote:
>> On 5 June 2016 at 20:23, Ladislav Michl <ladis@linux-mips.org> wrote:

>> > Unless I'm missing something, partition layout is passed to the kernel
>> > from the bootloader. So if kernel ends up with a different layout than
>> > a bootloader, there's a bug somewhere and that should be fixed.
>>
>> That's not it. The problem is that if this patch changes the layout
>> then building u-boot before this patch gives one layout and after this
>> patch it gives another layout effectively changing the partitioning.
>> All to save a few megabytes of a several gigiabyte medium. And if you
>> say that people can always set the partitioning by hand then it
>> completely defeats the purpose of fine-tuning the default in the first
>> place.
>
> U-Boot never prevented people from shooting their own leg, therefore
> there's no 'feature' preventing them to change default layout. They
> are still free to do whatever they want it they find it useful.
> Also I'm not proposing to change layout for exiting boards, just
> proving way to do it. igep00x0 boards had buggy NAND support since
> begining, so while fixing that, let's do it right way. Note that
> this feature is completely optional.
>
>> If on the other hand Linux got support for sizing partitions in nand
>> pages or eraseblocks and a patch changed the mtdparts layout to use
>> the new units then either both u-boot and Linux support the units or
>> parsing the partition fails. So it's safe and flexible and more
>> general and probably even less work on u-boot's part.
>
> Okay, specifing size in therms of eraseblock would solve my problem
> and I have to admit I'm not aware such a thing exists. Any pointer to
> the patch introducing that change?
>

I am not aware of any. it's on the list of nice to have things I will
probably not get to,

Anyway, It's imho the place to put this code so that Linux does not
depend on u-boot for pre-chewing it's partition table to get the
partitions right.

Thanks

Michal

[U-Boot] [PATCH 5/6] cmd: mtdparts: support runtime generated mtdparts

[Ladislav Michl]

On Mon, Jun 06, 2016 at 08:50:55PM +0200, Michal Suchanek wrote:
> On 6 June 2016 at 09:48, Ladislav Michl <ladis@linux-mips.org> wrote:
[snip]
> > Okay, specifing size in therms of eraseblock would solve my problem
> > and I have to admit I'm not aware such a thing exists. Any pointer to
> > the patch introducing that change?
> >
> 
> I am not aware of any. it's on the list of nice to have things I will
> probably not get to,
> 
> Anyway, It's imho the place to put this code so that Linux does not
> depend on u-boot for pre-chewing it's partition table to get the
> partitions right.

Linux already depends on U-Boot as U-Boot feeds Linux with MTD partitions.
So this patch does not introduce any change in this regard. And whenever
are paritions altered by user, saved to U-Boot environment or default
layout provided according actual eraseblock size is just not important
here. In the end both U-Boot and Linux are working with the same partition
layout. This is the only thing that matters and this patch does not change
that. It changes only this: instead of hardcoding default layout into string
provided at compile time, there is now posibility to create that string
runtime. That's all.

And now... Your recomended solution is to change both U-Boot and Linux
to understand partition layout based on indexes and sizes expressed
in eraseblocks. While size of SPL could be nicely evaluated in terms
of eraseblocks, it is a bit more complicated with kernel partition,
because 10 eraseblocks says nothing about partition size, therefore
you cannot say whenever kernel will fit that space without knowledge
of eraseblock size. So you just reverted the problem and both U-Boot
and Linux would end with more code to be maintained for compatibility.

Best regards,
	ladis

[U-Boot] [PATCH 5/6] cmd: mtdparts: support runtime generated mtdparts

[Michal Suchanek]

On 6 June 2016 at 23:21, Ladislav Michl <ladis@linux-mips.org> wrote:
> On Mon, Jun 06, 2016 at 08:50:55PM +0200, Michal Suchanek wrote:
>> On 6 June 2016 at 09:48, Ladislav Michl <ladis@linux-mips.org> wrote:
> [snip]
>> > Okay, specifing size in therms of eraseblock would solve my problem
>> > and I have to admit I'm not aware such a thing exists. Any pointer to
>> > the patch introducing that change?
>> >
>>
>> I am not aware of any. it's on the list of nice to have things I will
>> probably not get to,
>>
>> Anyway, It's imho the place to put this code so that Linux does not
>> depend on u-boot for pre-chewing it's partition table to get the
>> partitions right.
>
> Linux already depends on U-Boot as U-Boot feeds Linux with MTD partitions.

No. It depends on getting the partition layout on commandline or in
DT. This can be something U-Boot generates, or something it passes on
from its environment or something it does not touch at all.

> So this patch does not introduce any change in this regard. And whenever
> are paritions altered by user, saved to U-Boot environment or default
> layout provided according actual eraseblock size is just not important
> here. In the end both U-Boot and Linux are working with the same partition
> layout. This is the only thing that matters and this patch does not change

But for the layout to be correct you must build current u-boot with
this patch and not use compiled-in command line, devicetree, older
u-boot or different bootloader.

> that. It changes only this: instead of hardcoding default layout into string
> provided at compile time, there is now posibility to create that string
> runtime. That's all.

The only problem with that is that the code for generating the string
is in u-boot and the string is needed in Linux so the logical thing is
to move the code to Linux where it is needed.

>
> And now... Your recomended solution is to change both U-Boot and Linux
> to understand partition layout based on indexes and sizes expressed
> in eraseblocks. While size of SPL could be nicely evaluated in terms
> of eraseblocks, it is a bit more complicated with kernel partition,
> because 10 eraseblocks says nothing about partition size, therefore
> you cannot say whenever kernel will fit that space without knowledge
> of eraseblock size.

That's why you need the possibility to start a partition at the end of
preceding partition. That way you can have sizes of different
partitions in different units.

It's interesting that your firmware uses erase block size for the boot
partition. The sunxi firmware loads the bootloader from fixed number
of pages.

> So you just reverted the problem and both U-Boot
> and Linux would end with more code to be maintained for compatibility.

Yes, if you want U-Boot and Linux to talk the same units you need both
implementing the code to handle them. Still U-Boot will only need to
pass around a string with partition layout and parse it in the cases
it is accessing the partitions.

Thanks

Michal

[U-Boot] [PATCH 5/6] cmd: mtdparts: support runtime generated mtdparts

[Ladislav Michl]

On Tue, Jun 07, 2016 at 09:43:20AM +0200, Michal Suchanek wrote:
> On 6 June 2016 at 23:21, Ladislav Michl <ladis@linux-mips.org> wrote:
> > On Mon, Jun 06, 2016 at 08:50:55PM +0200, Michal Suchanek wrote:
> > Linux already depends on U-Boot as U-Boot feeds Linux with MTD partitions.
> 
> No. It depends on getting the partition layout on commandline or in
> DT. This can be something U-Boot generates, or something it passes on
> from its environment or something it does not touch at all.

I really fail to understand how your wording conflicts with what I wrote
above. Both cmdline and DT are provided by bootloader, so Linux depends on
that. Unlike HDD for example where its layout is stored on media in the
partition table.

> > So this patch does not introduce any change in this regard. And whenever
> > are paritions altered by user, saved to U-Boot environment or default
> > layout provided according actual eraseblock size is just not important
> > here. In the end both U-Boot and Linux are working with the same partition
> > layout. This is the only thing that matters and this patch does not change
> 
> But for the layout to be correct you must build current u-boot with
> this patch and not use compiled-in command line, devicetree, older
> u-boot or different bootloader.

No, layout is correct as it is for all boards I have there. But I was told
boards with different NAND chips were produced. Also I have to change
paritioning anyway, as original solution does not work in long term and
devices are failing in field. I'm not proposing to break any existing setup.
Your solution does the contrary as it requires both recent U-Boot and recent
kernel. Also, that function is only helper one. If you decide to use static
defaults, I'm fine with that. But with above patch, every user of that board
will start with sane and optimal defaults. This is the sole point.
Also I'm pretty sure previous patch which enables mtdparts to use defaults
implicitely will not break anything, otherwise someone would notice bugs
present in mtdparts code. (Perhaps it's time to read UBI SPL patch serie
and related discusion for a reason I started to work on that)

> > that. It changes only this: instead of hardcoding default layout into string
> > provided at compile time, there is now posibility to create that string
> > runtime. That's all.
> 
> The only problem with that is that the code for generating the string
> is in u-boot and the string is needed in Linux so the logical thing is
> to move the code to Linux where it is needed.

No. The string is needed in both U-Boot _and_ Linux. I see no reason to
change kernel as it is not byuig anything.

> > And now... Your recomended solution is to change both U-Boot and Linux
> > to understand partition layout based on indexes and sizes expressed
> > in eraseblocks. While size of SPL could be nicely evaluated in terms
> > of eraseblocks, it is a bit more complicated with kernel partition,
> > because 10 eraseblocks says nothing about partition size, therefore
> > you cannot say whenever kernel will fit that space without knowledge
> > of eraseblock size.
> 
> That's why you need the possibility to start a partition at the end of
> preceding partition. That way you can have sizes of different
> partitions in different units.

That posibility is already there for ages.

> It's interesting that your firmware uses erase block size for the boot
> partition. The sunxi firmware loads the bootloader from fixed number
> of pages.

DM3730 tries to load from first eraseblock, if that fails, then second
and finaly gives up at fourth (including).

> > So you just reverted the problem and both U-Boot
> > and Linux would end with more code to be maintained for compatibility.
> 
> Yes, if you want U-Boot and Linux to talk the same units you need both
> implementing the code to handle them. Still U-Boot will only need to
> pass around a string with partition layout and parse it in the cases
> it is accessing the partitions.

And since U-Boot environment is usually stored in partition(s)...
Also note that this change will break non-Linux systems and older Linux
systems. And as you said, I'm doing that to save few megabytes on media,
so now it seems to be too high price even to me.

Best regards,
	ladis