All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media
@ 2019-06-08  1:26 Andre Przywara
  2019-06-08  1:26 ` [U-Boot] [PATCH 1/5] env: allow undefined CONFIG_SYS_MMC_ENV_DEV Andre Przywara
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Andre Przywara @ 2019-06-08  1:26 UTC (permalink / raw)
  To: u-boot

At the moment we need to configure the place where U-Boot tries to load
its environment from at compile time. This is not only inflexible, but
also unnecessary, as we have easy access to the boot source.

This series prepares U-Boot on Allwinner boards to load the environment
from the same media where the SPL and U-Boot proper were loaded from.
This allows to keep one firmware binary, and copy it to an SD card,
eMMC or even SPI flash, without needing to configure it differently.

Patches 1 and 2 introduce that runtime decision for a raw MMC environment,
patches 3 and 4 for FAT.
Patch 5 considers the boot source when working out the location priority
for the environment: it will always consider the boot source first, then
fall back to the existing algorithm.

This enables to build flasher images, which boot from SD card, but for
instance provide a bootmenu with the option to copy this very image to
eMMC or SPI flash.

Cheers,
Andre.

Andre Przywara (5):
  env: allow undefined CONFIG_SYS_MMC_ENV_DEV
  sunxi: autodetect SD/eMMC device for environment
  env: allow runtime determination of FAT environment partition
  sunxi: use FAT environment from boot source
  sunxi: use boot source for determining environment location

 board/sunxi/board.c            | 67 +++++++++++++++++++++++++++++++++++++-----
 env/fat.c                      | 13 +++++---
 env/mmc.c                      |  4 +++
 include/configs/sunxi-common.h |  7 -----
 include/environment.h          |  4 +++
 5 files changed, 76 insertions(+), 19 deletions(-)

-- 
2.14.5

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 1/5] env: allow undefined CONFIG_SYS_MMC_ENV_DEV
  2019-06-08  1:26 [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media Andre Przywara
@ 2019-06-08  1:26 ` Andre Przywara
  2019-06-08 13:13   ` Tom Rini
  2019-06-08  1:26 ` [U-Boot] [PATCH 2/5] sunxi: autodetect SD/eMMC device for environment Andre Przywara
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Andre Przywara @ 2019-06-08  1:26 UTC (permalink / raw)
  To: u-boot

So far we are required to always define the CONFIG_SYS_MMC_ENV_DEV
variable, even if a platform specific function overrides the weak
function that is using it.

Check for the existence of this Kconfig variable, eliminating the need
to define a dummy value.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 env/mmc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/env/mmc.c b/env/mmc.c
index c3cf35d01b..122fec3af8 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -124,7 +124,11 @@ __weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr)
 
 __weak int mmc_get_env_dev(void)
 {
+#ifdef CONFIG_SYS_MMC_ENV_DEV
 	return CONFIG_SYS_MMC_ENV_DEV;
+#else
+	return 0;
+#endif
 }
 
 #ifdef CONFIG_SYS_MMC_ENV_PART
-- 
2.14.5

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 2/5] sunxi: autodetect SD/eMMC device for environment
  2019-06-08  1:26 [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media Andre Przywara
  2019-06-08  1:26 ` [U-Boot] [PATCH 1/5] env: allow undefined CONFIG_SYS_MMC_ENV_DEV Andre Przywara
@ 2019-06-08  1:26 ` Andre Przywara
  2019-06-08  1:26 ` [U-Boot] [PATCH 3/5] env: allow runtime determination of FAT environment partition Andre Przywara
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Andre Przywara @ 2019-06-08  1:26 UTC (permalink / raw)
  To: u-boot

When using the environment from a raw MMC device, we are required to
specify the desired device number at configuration time. This creates
problems when the same image is booted from SD cards and eMMC devices.

Override the generic weak definition of mmc_get_env_dev() to query for
the boot source, returning the corresponding device number. This will
automatically pick the environment from the right device.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 board/sunxi/board.c            | 19 +++++++++++++++++++
 include/configs/sunxi-common.h |  7 -------
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 98bc3cd0c1..973dba4578 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -208,6 +208,25 @@ enum env_location env_get_location(enum env_operation op, int prio)
 }
 #endif
 
+/* Determine whether to use the environment from SD card or eMMC. */
+int mmc_get_env_dev(void)
+{
+	/* Use the environment from the boot device, if booted from (e)MMC. */
+	switch (sunxi_get_boot_device()) {
+	case BOOT_DEVICE_MMC1:
+		return 0;
+	case BOOT_DEVICE_MMC2:
+		return 1;
+	}
+
+	/* If there is no eMMC configured, use the SD card. */
+	if (CONFIG_MMC_SUNXI_SLOT_EXTRA == -1)
+		return 0;
+
+	/* Otherwise use the environment on the eMMC. */
+	return 1;
+}
+
 #ifdef CONFIG_DM_MMC
 static void mmc_pinmux_setup(int sdc);
 #endif
diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index fceb812448..aca0eca806 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -138,13 +138,6 @@
 #define CONFIG_BOARD_SIZE_LIMIT		0x7e000
 #endif
 
-#if CONFIG_MMC_SUNXI_SLOT_EXTRA != -1
-/* If we have two devices (most likely eMMC + MMC), favour the eMMC */
-#define CONFIG_SYS_MMC_ENV_DEV		1
-#else
-/* Otherwise, use the only device we have */
-#define CONFIG_SYS_MMC_ENV_DEV		0
-#endif
 #define CONFIG_SYS_MMC_MAX_DEVICE	4
 #endif
 
-- 
2.14.5

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 3/5] env: allow runtime determination of FAT environment partition
  2019-06-08  1:26 [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media Andre Przywara
  2019-06-08  1:26 ` [U-Boot] [PATCH 1/5] env: allow undefined CONFIG_SYS_MMC_ENV_DEV Andre Przywara
  2019-06-08  1:26 ` [U-Boot] [PATCH 2/5] sunxi: autodetect SD/eMMC device for environment Andre Przywara
@ 2019-06-08  1:26 ` Andre Przywara
  2019-06-08 13:13   ` Tom Rini
  2019-06-08  1:26 ` [U-Boot] [PATCH 4/5] sunxi: use FAT environment from boot source Andre Przywara
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Andre Przywara @ 2019-06-08  1:26 UTC (permalink / raw)
  To: u-boot

Currently the FAT device and partition which contains the environment
must be configured at compile time. This creates problems for boards
which have multiple bootable devices (like SD card and eMMC).

Similar to what we do for raw MMC environments, introduce a weak function
to allow boards to override this static configuration. Right now we just
return the Kconfig variable.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 env/fat.c             | 13 +++++++++----
 include/environment.h |  4 ++++
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/env/fat.c b/env/fat.c
index 7f74c64dfe..320f73b6d3 100644
--- a/env/fat.c
+++ b/env/fat.c
@@ -30,6 +30,11 @@
 # endif
 #endif
 
+__weak char *fat_get_env_dev_part(void)
+{
+	return CONFIG_ENV_FAT_DEVICE_AND_PART;
+}
+
 #ifdef CMD_SAVEENV
 static int env_fat_save(void)
 {
@@ -45,8 +50,8 @@ static int env_fat_save(void)
 		return err;
 
 	part = blk_get_device_part_str(CONFIG_ENV_FAT_INTERFACE,
-					CONFIG_ENV_FAT_DEVICE_AND_PART,
-					&dev_desc, &info, 1);
+				       fat_get_env_dev_part(),
+				       &dev_desc, &info, 1);
 	if (part < 0)
 		return 1;
 
@@ -92,8 +97,8 @@ static int env_fat_load(void)
 #endif
 
 	part = blk_get_device_part_str(CONFIG_ENV_FAT_INTERFACE,
-					CONFIG_ENV_FAT_DEVICE_AND_PART,
-					&dev_desc, &info, 1);
+				       fat_get_env_dev_part(),
+				       &dev_desc, &info, 1);
 	if (part < 0)
 		goto err_env_relocate;
 
diff --git a/include/environment.h b/include/environment.h
index cd96676141..364adc76ba 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -65,6 +65,10 @@
 # endif
 #endif
 
+#if defined(CONFIG_ENV_IS_IN_FAT)
+char *fat_get_env_dev_part(void);
+#endif
+
 #if defined(CONFIG_ENV_IS_IN_NAND)
 # if defined(CONFIG_ENV_OFFSET_OOB)
 #  ifdef CONFIG_ENV_OFFSET_REDUND
-- 
2.14.5

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 4/5] sunxi: use FAT environment from boot source
  2019-06-08  1:26 [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media Andre Przywara
                   ` (2 preceding siblings ...)
  2019-06-08  1:26 ` [U-Boot] [PATCH 3/5] env: allow runtime determination of FAT environment partition Andre Przywara
@ 2019-06-08  1:26 ` Andre Przywara
  2019-06-08  1:26 ` [U-Boot] [PATCH 5/5] sunxi: use boot source for determining environment location Andre Przywara
  2019-06-10  8:30 ` [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media Maxime Ripard
  5 siblings, 0 replies; 19+ messages in thread
From: Andre Przywara @ 2019-06-08  1:26 UTC (permalink / raw)
  To: u-boot

Until now we were required to configure the environment FAT partition
and device at compile time, which led to problems if boards have both SD
cards and eMMC and we wanted to use the same image for booting from
both.

Check the boot source and use this device for finding the environment.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 board/sunxi/board.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 973dba4578..5802b079b6 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -227,6 +227,15 @@ int mmc_get_env_dev(void)
 	return 1;
 }
 
+static char fat_device[7];
+
+char *fat_get_env_dev_part(void)
+{
+	sprintf(fat_device, "%d:auto", mmc_get_env_dev());
+
+	return fat_device;
+}
+
 #ifdef CONFIG_DM_MMC
 static void mmc_pinmux_setup(int sdc);
 #endif
-- 
2.14.5

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 5/5] sunxi: use boot source for determining environment location
  2019-06-08  1:26 [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media Andre Przywara
                   ` (3 preceding siblings ...)
  2019-06-08  1:26 ` [U-Boot] [PATCH 4/5] sunxi: use FAT environment from boot source Andre Przywara
@ 2019-06-08  1:26 ` Andre Przywara
  2019-06-10  8:30 ` [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media Maxime Ripard
  5 siblings, 0 replies; 19+ messages in thread
From: Andre Przywara @ 2019-06-08  1:26 UTC (permalink / raw)
  To: u-boot

Currently we only support to load the environment from raw MMC or FAT
locations on Allwinner boards. With the advent of SPI flash we probably
also want to support using the environment there, so we need to become
a bit more flexible.

Change the environment priority function to take the boot source into
account. When booted from eMMC or SD card, we use FAT or MMC, if
configured, as before.
If we are booted from SPI flash, we try to use the environment from
there, if possible. The same is true for NAND flash booting, although
this is somewhat theoretical right now (as untested).

This way we can use the same image for SD and SPI flash booting, which
allows us to simply copy a booted image from SD card to the SPI flash,
for instance.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 board/sunxi/board.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 5802b079b6..b95a1a0feb 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -192,21 +192,44 @@ void i2c_init_board(void)
 #endif
 }
 
-#if defined(CONFIG_ENV_IS_IN_MMC) && defined(CONFIG_ENV_IS_IN_FAT)
 enum env_location env_get_location(enum env_operation op, int prio)
 {
-	switch (prio) {
-	case 0:
-		return ENVL_FAT;
+	enum env_location boot_loc = ENVL_UNKNOWN;
 
-	case 1:
-		return ENVL_MMC;
+	gd->env_load_prio = prio;
 
+	switch (sunxi_get_boot_device()) {
+	case BOOT_DEVICE_MMC1:
+	case BOOT_DEVICE_MMC2:
+		boot_loc = ENVL_FAT;
+		break;
+	case BOOT_DEVICE_NAND:
+#ifdef CONFIG_ENV_IS_IN_NAND
+		boot_loc = ENVL_NAND;
+#endif
+		break;
+	case BOOT_DEVICE_SPI:
+#ifdef CONFIG_ENV_IS_IN_SPI_FLASH
+		boot_loc = ENVL_SPI_FLASH;
+#endif
+		break;
+	case BOOT_DEVICE_BOARD:
+		boot_loc = ENVL_FAT;	/* dummy to prevent crash */
+		break;
 	default:
-		return ENVL_UNKNOWN;
+		break;
 	}
-}
+
+	/* Always try to access the environment on the boot device first. */
+	if (prio == 0)
+		return boot_loc;
+
+#ifdef CONFIG_ENV_IS_IN_MMC
+	if (prio == 1 && boot_loc == ENVL_FAT)
+		return ENVL_MMC;
 #endif
+	return ENVL_UNKNOWN;
+}
 
 /* Determine whether to use the environment from SD card or eMMC. */
 int mmc_get_env_dev(void)
-- 
2.14.5

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 3/5] env: allow runtime determination of FAT environment partition
  2019-06-08  1:26 ` [U-Boot] [PATCH 3/5] env: allow runtime determination of FAT environment partition Andre Przywara
@ 2019-06-08 13:13   ` Tom Rini
  0 siblings, 0 replies; 19+ messages in thread
From: Tom Rini @ 2019-06-08 13:13 UTC (permalink / raw)
  To: u-boot

On Sat, Jun 08, 2019 at 02:26:56AM +0100, Andre Przywara wrote:

> Currently the FAT device and partition which contains the environment
> must be configured at compile time. This creates problems for boards
> which have multiple bootable devices (like SD card and eMMC).
> 
> Similar to what we do for raw MMC environments, introduce a weak function
> to allow boards to override this static configuration. Right now we just
> return the Kconfig variable.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
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/20190608/d285a819/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 1/5] env: allow undefined CONFIG_SYS_MMC_ENV_DEV
  2019-06-08  1:26 ` [U-Boot] [PATCH 1/5] env: allow undefined CONFIG_SYS_MMC_ENV_DEV Andre Przywara
@ 2019-06-08 13:13   ` Tom Rini
  2019-06-10  9:35     ` Andre Przywara
  0 siblings, 1 reply; 19+ messages in thread
From: Tom Rini @ 2019-06-08 13:13 UTC (permalink / raw)
  To: u-boot

On Sat, Jun 08, 2019 at 02:26:54AM +0100, Andre Przywara wrote:
> So far we are required to always define the CONFIG_SYS_MMC_ENV_DEV
> variable, even if a platform specific function overrides the weak
> function that is using it.
> 
> Check for the existence of this Kconfig variable, eliminating the need
> to define a dummy value.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  env/mmc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/env/mmc.c b/env/mmc.c
> index c3cf35d01b..122fec3af8 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -124,7 +124,11 @@ __weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr)
>  
>  __weak int mmc_get_env_dev(void)
>  {
> +#ifdef CONFIG_SYS_MMC_ENV_DEV
>  	return CONFIG_SYS_MMC_ENV_DEV;
> +#else
> +	return 0;
> +#endif
>  }
>  
>  #ifdef CONFIG_SYS_MMC_ENV_PART

Since 0 is a valid device, I'm concerned this might lead to unintended
behavior.  Can we return some error code here and catch it later?

-- 
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/20190608/34fb9c30/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media
  2019-06-08  1:26 [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media Andre Przywara
                   ` (4 preceding siblings ...)
  2019-06-08  1:26 ` [U-Boot] [PATCH 5/5] sunxi: use boot source for determining environment location Andre Przywara
@ 2019-06-10  8:30 ` Maxime Ripard
  2019-06-10  9:11   ` Andre Przywara
  5 siblings, 1 reply; 19+ messages in thread
From: Maxime Ripard @ 2019-06-10  8:30 UTC (permalink / raw)
  To: u-boot

Hi,

On Sat, Jun 08, 2019 at 02:26:53AM +0100, Andre Przywara wrote:
> At the moment we need to configure the place where U-Boot tries to load
> its environment from at compile time. This is not only inflexible, but
> also unnecessary, as we have easy access to the boot source.
>
> This series prepares U-Boot on Allwinner boards to load the environment
> from the same media where the SPL and U-Boot proper were loaded from.
> This allows to keep one firmware binary, and copy it to an SD card,
> eMMC or even SPI flash, without needing to configure it differently.

This does change a couple of things though. The environment used to be
loaded always from the same source, no matter the boot device. This
means that if you would set an SD card, you would get the environment
from the eMMC. Same thing for FEL. This is no longer the case.

I don't know whether it's a good or a bad thing, but it should be
mentionned.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190610/89c72410/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media
  2019-06-10  8:30 ` [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media Maxime Ripard
@ 2019-06-10  9:11   ` Andre Przywara
  2019-06-11  9:37     ` Maxime Ripard
  0 siblings, 1 reply; 19+ messages in thread
From: Andre Przywara @ 2019-06-10  9:11 UTC (permalink / raw)
  To: u-boot

On Mon, 10 Jun 2019 10:30:37 +0200
Maxime Ripard <maxime.ripard@bootlin.com> wrote:

Hi Maxime,

thanks for having a look!

> On Sat, Jun 08, 2019 at 02:26:53AM +0100, Andre Przywara wrote:
> > At the moment we need to configure the place where U-Boot tries to load
> > its environment from at compile time. This is not only inflexible, but
> > also unnecessary, as we have easy access to the boot source.
> >
> > This series prepares U-Boot on Allwinner boards to load the environment
> > from the same media where the SPL and U-Boot proper were loaded from.
> > This allows to keep one firmware binary, and copy it to an SD card,
> > eMMC or even SPI flash, without needing to configure it differently.  
> 
> This does change a couple of things though. The environment used to be
> loaded always from the same source, no matter the boot device. This
> means that if you would set an SD card, you would get the environment
> from the eMMC. Same thing for FEL. This is no longer the case.
> 
> I don't know whether it's a good or a bad thing, but it should be
> mentionned.

This is true, I failed to mention that.

To start a discussion on this:
I consider the current (fixed location) behaviour somewhat surprising and
limiting, and couldn't find a real use case where this would be required.
Happy to hear of one!
Instead I thought about those cases:
- There is some botched U-Boot plus environment on the eMMC. You want to
boot from SD card to have a clean start, possibly to fix it. But it will
load the possibly outdated, broken or even unrelated environment from eMMC.
- You want to boot from SD card without touching the eMMC at all. Saving
the environment will spoil that.
- You want to have one image for all possible boot media. Putting the
environment on eMMC seems like a commonly accessible place, but isn't
really reliable for those eMMC socket boards. I guess not many actually
have a chip connected in the Pine64-LTS, SoPine or Pine-H64, for instance.
Forcing the environment to SD card is equally troublesome.

I think the situation gets even worse with SPI flash booting (which was
the trigger for these patches). And no, we definitely don't want multiple
defconfig files per board just to cover those cases. Also asking the user
to manually change the config sounds more like a workaround.

Happy to hear of cases where this behaviour would cause trouble.

For the context:
These patches are part of a branch of mine, which allows to create a nice
flasher image for the Pine64-LTS (and other boards with SPI flash): It
boots from SD card and presents a U-Boot bootmenu, which among normal boot
options offers to install this image to SPI flash, or eMMC, if connected.
The installer bootmenu items live in "raw MMC" envirnment on the SD card
and are not copied to SPI flash. So with these changes everything falls
into place naturally.

Cheers,
Andre.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 1/5] env: allow undefined CONFIG_SYS_MMC_ENV_DEV
  2019-06-08 13:13   ` Tom Rini
@ 2019-06-10  9:35     ` Andre Przywara
  2019-06-10 21:53       ` Tom Rini
  0 siblings, 1 reply; 19+ messages in thread
From: Andre Przywara @ 2019-06-10  9:35 UTC (permalink / raw)
  To: u-boot

On Sat, 8 Jun 2019 09:13:52 -0400
Tom Rini <trini@konsulko.com> wrote:

Hi Tom,

thanks for having a look!

> On Sat, Jun 08, 2019 at 02:26:54AM +0100, Andre Przywara wrote:
> > So far we are required to always define the CONFIG_SYS_MMC_ENV_DEV
> > variable, even if a platform specific function overrides the weak
> > function that is using it.
> > 
> > Check for the existence of this Kconfig variable, eliminating the need
> > to define a dummy value.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  env/mmc.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/env/mmc.c b/env/mmc.c
> > index c3cf35d01b..122fec3af8 100644
> > --- a/env/mmc.c
> > +++ b/env/mmc.c
> > @@ -124,7 +124,11 @@ __weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr)
> >  
> >  __weak int mmc_get_env_dev(void)
> >  {
> > +#ifdef CONFIG_SYS_MMC_ENV_DEV
> >  	return CONFIG_SYS_MMC_ENV_DEV;
> > +#else
> > +	return 0;
> > +#endif
> >  }
> >  
> >  #ifdef CONFIG_SYS_MMC_ENV_PART  
> 
> Since 0 is a valid device, I'm concerned this might lead to unintended
> behavior.  Can we return some error code here and catch it later?

I see your point. I think originally my idea was that 0 would hopefully be a valid device in any case, so it would just work in those cases. But I see the trouble that this papers over a bug (namely CONFIG_SYS_MMC_ENV_DEV not being defined).
Looking at all those users I find it rather dangerous (and tedious) to check for this in all callers.

What about printing an error message, here in that function? Ideally this would be spotted immediately upon initial testing, so would never be exposed to a real user.
Something like "Please either define CONFIG_SYS_MMC_ENV_DEV or provide a mmc_get_en_dev() implementation."?

Cheers,
Andre.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 1/5] env: allow undefined CONFIG_SYS_MMC_ENV_DEV
  2019-06-10  9:35     ` Andre Przywara
@ 2019-06-10 21:53       ` Tom Rini
  0 siblings, 0 replies; 19+ messages in thread
From: Tom Rini @ 2019-06-10 21:53 UTC (permalink / raw)
  To: u-boot

On Mon, Jun 10, 2019 at 10:35:36AM +0100, Andre Przywara wrote:
> On Sat, 8 Jun 2019 09:13:52 -0400
> Tom Rini <trini@konsulko.com> wrote:
> 
> Hi Tom,
> 
> thanks for having a look!
> 
> > On Sat, Jun 08, 2019 at 02:26:54AM +0100, Andre Przywara wrote:
> > > So far we are required to always define the CONFIG_SYS_MMC_ENV_DEV
> > > variable, even if a platform specific function overrides the weak
> > > function that is using it.
> > > 
> > > Check for the existence of this Kconfig variable, eliminating the need
> > > to define a dummy value.
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > >  env/mmc.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/env/mmc.c b/env/mmc.c
> > > index c3cf35d01b..122fec3af8 100644
> > > --- a/env/mmc.c
> > > +++ b/env/mmc.c
> > > @@ -124,7 +124,11 @@ __weak int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr)
> > >  
> > >  __weak int mmc_get_env_dev(void)
> > >  {
> > > +#ifdef CONFIG_SYS_MMC_ENV_DEV
> > >  	return CONFIG_SYS_MMC_ENV_DEV;
> > > +#else
> > > +	return 0;
> > > +#endif
> > >  }
> > >  
> > >  #ifdef CONFIG_SYS_MMC_ENV_PART  
> > 
> > Since 0 is a valid device, I'm concerned this might lead to unintended
> > behavior.  Can we return some error code here and catch it later?
> 
> I see your point. I think originally my idea was that 0 would hopefully be a valid device in any case, so it would just work in those cases. But I see the trouble that this papers over a bug (namely CONFIG_SYS_MMC_ENV_DEV not being defined).
> Looking at all those users I find it rather dangerous (and tedious) to check for this in all callers.
> 
> What about printing an error message, here in that function? Ideally this would be spotted immediately upon initial testing, so would never be exposed to a real user.
> Something like "Please either define CONFIG_SYS_MMC_ENV_DEV or provide a mmc_get_en_dev() implementation."?

That might be OK.  Just check the resulting binaries that the string
does get discarded from the build when we have a non-weak function in
that case.  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/20190610/045325fc/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media
  2019-06-10  9:11   ` Andre Przywara
@ 2019-06-11  9:37     ` Maxime Ripard
  2019-06-11 14:28       ` Tom Rini
  0 siblings, 1 reply; 19+ messages in thread
From: Maxime Ripard @ 2019-06-11  9:37 UTC (permalink / raw)
  To: u-boot

On Mon, Jun 10, 2019 at 10:11:39AM +0100, Andre Przywara wrote:
> On Mon, 10 Jun 2019 10:30:37 +0200
> Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> Hi Maxime,
>
> thanks for having a look!
>
> > On Sat, Jun 08, 2019 at 02:26:53AM +0100, Andre Przywara wrote:
> > > At the moment we need to configure the place where U-Boot tries to load
> > > its environment from at compile time. This is not only inflexible, but
> > > also unnecessary, as we have easy access to the boot source.
> > >
> > > This series prepares U-Boot on Allwinner boards to load the environment
> > > from the same media where the SPL and U-Boot proper were loaded from.
> > > This allows to keep one firmware binary, and copy it to an SD card,
> > > eMMC or even SPI flash, without needing to configure it differently.
> >
> > This does change a couple of things though. The environment used to be
> > loaded always from the same source, no matter the boot device. This
> > means that if you would set an SD card, you would get the environment
> > from the eMMC. Same thing for FEL. This is no longer the case.
> >
> > I don't know whether it's a good or a bad thing, but it should be
> > mentionned.
>
> This is true, I failed to mention that.
>
> To start a discussion on this:
> I consider the current (fixed location) behaviour somewhat surprising and
> limiting, and couldn't find a real use case where this would be required.
> Happy to hear of one!
> Instead I thought about those cases:
> - There is some botched U-Boot plus environment on the eMMC. You want to
> boot from SD card to have a clean start, possibly to fix it. But it will
> load the possibly outdated, broken or even unrelated environment from eMMC.

This one might be a feature though. Being able to restore / fix an
environment in the eMMC running from an SD card has save me a couple
of times. Or booting from the SD card because the U-Boot on the eMMC
is broken, while the environment is working.

> - You want to boot from SD card without touching the eMMC at all. Saving
> the environment will spoil that.

But it goes against that one, which might be more important / sensible.

> - You want to have one image for all possible boot media.

That won't happen, only because NAND is a thing.

And even then, I'm not really sure that it's a good thing. A U-Boot
build these days is roughly in the same sizes than a stripped down
Linux image. For an inferior solution in pretty much every aspect.

Loading the environment from the boot device instead of hardcoding
is pretty orthogonal to that though.

> Putting the environment on eMMC seems like a commonly accessible
> place, but isn't really reliable for those eMMC socket boards. I
> guess not many actually have a chip connected in the Pine64-LTS,
> SoPine or Pine-H64, for instance.  Forcing the environment to SD
> card is equally troublesome.

Yeah, that's true. I guess we could also rely on whether the eMMC / SD
is reseponding to commands, but I don't see any particular benefit to
do that.

> I think the situation gets even worse with SPI flash booting (which was
> the trigger for these patches). And no, we definitely don't want multiple
> defconfig files per board just to cover those cases. Also asking the user
> to manually change the config sounds more like a workaround.

Yeah, well, let's agree to disagree on this one.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190611/b4f22ac5/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media
  2019-06-11  9:37     ` Maxime Ripard
@ 2019-06-11 14:28       ` Tom Rini
  2019-06-11 14:53         ` Maxime Ripard
  0 siblings, 1 reply; 19+ messages in thread
From: Tom Rini @ 2019-06-11 14:28 UTC (permalink / raw)
  To: u-boot

On Tue, Jun 11, 2019 at 11:37:28AM +0200, Maxime Ripard wrote:
> On Mon, Jun 10, 2019 at 10:11:39AM +0100, Andre Przywara wrote:
> > On Mon, 10 Jun 2019 10:30:37 +0200
> > Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > Hi Maxime,
> >
> > thanks for having a look!
> >
> > > On Sat, Jun 08, 2019 at 02:26:53AM +0100, Andre Przywara wrote:
> > > > At the moment we need to configure the place where U-Boot tries to load
> > > > its environment from at compile time. This is not only inflexible, but
> > > > also unnecessary, as we have easy access to the boot source.
> > > >
> > > > This series prepares U-Boot on Allwinner boards to load the environment
> > > > from the same media where the SPL and U-Boot proper were loaded from.
> > > > This allows to keep one firmware binary, and copy it to an SD card,
> > > > eMMC or even SPI flash, without needing to configure it differently.
> > >
> > > This does change a couple of things though. The environment used to be
> > > loaded always from the same source, no matter the boot device. This
> > > means that if you would set an SD card, you would get the environment
> > > from the eMMC. Same thing for FEL. This is no longer the case.
> > >
> > > I don't know whether it's a good or a bad thing, but it should be
> > > mentionned.
> >
> > This is true, I failed to mention that.
> >
> > To start a discussion on this:
> > I consider the current (fixed location) behaviour somewhat surprising and
> > limiting, and couldn't find a real use case where this would be required.
> > Happy to hear of one!
> > Instead I thought about those cases:
> > - There is some botched U-Boot plus environment on the eMMC. You want to
> > boot from SD card to have a clean start, possibly to fix it. But it will
> > load the possibly outdated, broken or even unrelated environment from eMMC.
> 
> This one might be a feature though. Being able to restore / fix an
> environment in the eMMC running from an SD card has save me a couple
> of times. Or booting from the SD card because the U-Boot on the eMMC
> is broken, while the environment is working.
> 
> > - You want to boot from SD card without touching the eMMC at all. Saving
> > the environment will spoil that.
> 
> But it goes against that one, which might be more important / sensible.
> 
> > - You want to have one image for all possible boot media.
> 
> That won't happen, only because NAND is a thing.

Some of this is perhaps an argument for adding a sub-command to specify
where the environment is to be read from.  Heuristics are still only a
best guess and won't get it right every time.

> And even then, I'm not really sure that it's a good thing. A U-Boot
> build these days is roughly in the same sizes than a stripped down
> Linux image. For an inferior solution in pretty much every aspect.

Hey now.  We aren't _quite_ that large.  And we are (really!) trying to
find a happy medium between "distros want X/Y/Z for everyone" and "can
we commonly get back to UNDER 512kB maybe?  Please?".

-- 
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/20190611/37732981/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media
  2019-06-11 14:28       ` Tom Rini
@ 2019-06-11 14:53         ` Maxime Ripard
  2019-06-11 15:20           ` Tom Rini
  2019-06-11 15:34           ` Andre Przywara
  0 siblings, 2 replies; 19+ messages in thread
From: Maxime Ripard @ 2019-06-11 14:53 UTC (permalink / raw)
  To: u-boot

Hi!

On Tue, Jun 11, 2019 at 10:28:19AM -0400, Tom Rini wrote:
> On Tue, Jun 11, 2019 at 11:37:28AM +0200, Maxime Ripard wrote:
> > On Mon, Jun 10, 2019 at 10:11:39AM +0100, Andre Przywara wrote:
> > > On Mon, 10 Jun 2019 10:30:37 +0200
> > > Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > Hi Maxime,
> > >
> > > thanks for having a look!
> > >
> > > > On Sat, Jun 08, 2019 at 02:26:53AM +0100, Andre Przywara wrote:
> > > > > At the moment we need to configure the place where U-Boot tries to load
> > > > > its environment from at compile time. This is not only inflexible, but
> > > > > also unnecessary, as we have easy access to the boot source.
> > > > >
> > > > > This series prepares U-Boot on Allwinner boards to load the environment
> > > > > from the same media where the SPL and U-Boot proper were loaded from.
> > > > > This allows to keep one firmware binary, and copy it to an SD card,
> > > > > eMMC or even SPI flash, without needing to configure it differently.
> > > >
> > > > This does change a couple of things though. The environment used to be
> > > > loaded always from the same source, no matter the boot device. This
> > > > means that if you would set an SD card, you would get the environment
> > > > from the eMMC. Same thing for FEL. This is no longer the case.
> > > >
> > > > I don't know whether it's a good or a bad thing, but it should be
> > > > mentionned.
> > >
> > > This is true, I failed to mention that.
> > >
> > > To start a discussion on this:
> > > I consider the current (fixed location) behaviour somewhat surprising and
> > > limiting, and couldn't find a real use case where this would be required.
> > > Happy to hear of one!
> > > Instead I thought about those cases:
> > > - There is some botched U-Boot plus environment on the eMMC. You want to
> > > boot from SD card to have a clean start, possibly to fix it. But it will
> > > load the possibly outdated, broken or even unrelated environment from eMMC.
> >
> > This one might be a feature though. Being able to restore / fix an
> > environment in the eMMC running from an SD card has save me a couple
> > of times. Or booting from the SD card because the U-Boot on the eMMC
> > is broken, while the environment is working.
> >
> > > - You want to boot from SD card without touching the eMMC at all. Saving
> > > the environment will spoil that.
> >
> > But it goes against that one, which might be more important / sensible.
> >
> > > - You want to have one image for all possible boot media.
> >
> > That won't happen, only because NAND is a thing.
>
> Some of this is perhaps an argument for adding a sub-command to specify
> where the environment is to be read from.  Heuristics are still only a
> best guess and won't get it right every time.

I was mostly talking about the distribution of the image itself. While
eMMC, SD and SPI flash can be made to take the same image, NAND will
require a particular ECC and randomizer setup that requires that it's
bundled separately.

> > And even then, I'm not really sure that it's a good thing. A U-Boot
> > build these days is roughly in the same sizes than a stripped down
> > Linux image. For an inferior solution in pretty much every aspect.
>
> Hey now.  We aren't _quite_ that large.  And we are (really!) trying to
> find a happy medium between "distros want X/Y/Z for everyone" and "can
> we commonly get back to UNDER 512kB maybe?  Please?".

I'm exagerating a little, but barely. A current build for the SoC
Andre was mentionning takes 600kB. And since the trend has been for
the binaries to grow for quite some time now, I'm pretty sure everyone
will reserve 1MB just to be sure they have some room to spare.

Now, getting a kernel image to fit in a MB takes a bit of time, but
it's not really impossible to achieve. And if you can reclaim that MB
dedicated to U-Boot, it's actually fairly easy to do.

I've tried to have the size reduced at some time because we were
corrupting our U-Boot binary as soon as we where using saveenv (which
is probably one of the worst issue we could have), and it ended up
with everyone agreeing that we needed to reduce the size, just not the
one they were using.

It's kind of weird that people haven't yet embraced defconfig as they
are in Linux, where every distro has its own configuration, and it's
perfectly fine.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190611/f2ac5714/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media
  2019-06-11 14:53         ` Maxime Ripard
@ 2019-06-11 15:20           ` Tom Rini
  2019-06-11 15:34           ` Andre Przywara
  1 sibling, 0 replies; 19+ messages in thread
From: Tom Rini @ 2019-06-11 15:20 UTC (permalink / raw)
  To: u-boot

On Tue, Jun 11, 2019 at 04:53:58PM +0200, Maxime Ripard wrote:
> Hi!
> 
> On Tue, Jun 11, 2019 at 10:28:19AM -0400, Tom Rini wrote:
> > On Tue, Jun 11, 2019 at 11:37:28AM +0200, Maxime Ripard wrote:
> > > On Mon, Jun 10, 2019 at 10:11:39AM +0100, Andre Przywara wrote:
> > > > On Mon, 10 Jun 2019 10:30:37 +0200
> > > > Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > Hi Maxime,
> > > >
> > > > thanks for having a look!
> > > >
> > > > > On Sat, Jun 08, 2019 at 02:26:53AM +0100, Andre Przywara wrote:
> > > > > > At the moment we need to configure the place where U-Boot tries to load
> > > > > > its environment from at compile time. This is not only inflexible, but
> > > > > > also unnecessary, as we have easy access to the boot source.
> > > > > >
> > > > > > This series prepares U-Boot on Allwinner boards to load the environment
> > > > > > from the same media where the SPL and U-Boot proper were loaded from.
> > > > > > This allows to keep one firmware binary, and copy it to an SD card,
> > > > > > eMMC or even SPI flash, without needing to configure it differently.
> > > > >
> > > > > This does change a couple of things though. The environment used to be
> > > > > loaded always from the same source, no matter the boot device. This
> > > > > means that if you would set an SD card, you would get the environment
> > > > > from the eMMC. Same thing for FEL. This is no longer the case.
> > > > >
> > > > > I don't know whether it's a good or a bad thing, but it should be
> > > > > mentionned.
> > > >
> > > > This is true, I failed to mention that.
> > > >
> > > > To start a discussion on this:
> > > > I consider the current (fixed location) behaviour somewhat surprising and
> > > > limiting, and couldn't find a real use case where this would be required.
> > > > Happy to hear of one!
> > > > Instead I thought about those cases:
> > > > - There is some botched U-Boot plus environment on the eMMC. You want to
> > > > boot from SD card to have a clean start, possibly to fix it. But it will
> > > > load the possibly outdated, broken or even unrelated environment from eMMC.
> > >
> > > This one might be a feature though. Being able to restore / fix an
> > > environment in the eMMC running from an SD card has save me a couple
> > > of times. Or booting from the SD card because the U-Boot on the eMMC
> > > is broken, while the environment is working.
> > >
> > > > - You want to boot from SD card without touching the eMMC at all. Saving
> > > > the environment will spoil that.
> > >
> > > But it goes against that one, which might be more important / sensible.
> > >
> > > > - You want to have one image for all possible boot media.
> > >
> > > That won't happen, only because NAND is a thing.
> >
> > Some of this is perhaps an argument for adding a sub-command to specify
> > where the environment is to be read from.  Heuristics are still only a
> > best guess and won't get it right every time.
> 
> I was mostly talking about the distribution of the image itself. While
> eMMC, SD and SPI flash can be made to take the same image, NAND will
> require a particular ECC and randomizer setup that requires that it's
> bundled separately.

Right.  But as you noted, there's use cases for boot from one to fix
another.

> > > And even then, I'm not really sure that it's a good thing. A U-Boot
> > > build these days is roughly in the same sizes than a stripped down
> > > Linux image. For an inferior solution in pretty much every aspect.
> >
> > Hey now.  We aren't _quite_ that large.  And we are (really!) trying to
> > find a happy medium between "distros want X/Y/Z for everyone" and "can
> > we commonly get back to UNDER 512kB maybe?  Please?".
> 
> I'm exagerating a little, but barely. A current build for the SoC
> Andre was mentionning takes 600kB. And since the trend has been for
> the binaries to grow for quite some time now, I'm pretty sure everyone
> will reserve 1MB just to be sure they have some room to spare.

Yes, it's larger than I would like, and features keep being added.  But
also, yes, DM stuff needs a harder look.

> Now, getting a kernel image to fit in a MB takes a bit of time, but
> it's not really impossible to achieve. And if you can reclaim that MB
> dedicated to U-Boot, it's actually fairly easy to do.
> 
> I've tried to have the size reduced at some time because we were
> corrupting our U-Boot binary as soon as we where using saveenv (which
> is probably one of the worst issue we could have), and it ended up
> with everyone agreeing that we needed to reduce the size, just not the
> one they were using.
> 
> It's kind of weird that people haven't yet embraced defconfig as they
> are in Linux, where every distro has its own configuration, and it's
> perfectly fine.

To the last point, distributions seemingly very much want to avoid
building U-Boot and there's a growing contingent that wishes the
hardware shipped with the DTB for the hardware as well.

-- 
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/20190611/2ed5591c/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media
  2019-06-11 14:53         ` Maxime Ripard
  2019-06-11 15:20           ` Tom Rini
@ 2019-06-11 15:34           ` Andre Przywara
  2019-06-11 16:10             ` Tom Rini
  2019-06-12 13:08             ` Maxime Ripard
  1 sibling, 2 replies; 19+ messages in thread
From: Andre Przywara @ 2019-06-11 15:34 UTC (permalink / raw)
  To: u-boot

On Tue, 11 Jun 2019 16:53:58 +0200
Maxime Ripard <maxime.ripard@bootlin.com> wrote:

Hi,

> On Tue, Jun 11, 2019 at 10:28:19AM -0400, Tom Rini wrote:
> > On Tue, Jun 11, 2019 at 11:37:28AM +0200, Maxime Ripard wrote:  
> > > On Mon, Jun 10, 2019 at 10:11:39AM +0100, Andre Przywara wrote:  
> > > > On Mon, 10 Jun 2019 10:30:37 +0200
> > > > Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > Hi Maxime,
> > > >
> > > > thanks for having a look!
> > > >  
> > > > > On Sat, Jun 08, 2019 at 02:26:53AM +0100, Andre Przywara wrote:  
> > > > > > At the moment we need to configure the place where U-Boot tries to load
> > > > > > its environment from at compile time. This is not only inflexible, but
> > > > > > also unnecessary, as we have easy access to the boot source.
> > > > > >
> > > > > > This series prepares U-Boot on Allwinner boards to load the environment
> > > > > > from the same media where the SPL and U-Boot proper were loaded from.
> > > > > > This allows to keep one firmware binary, and copy it to an SD card,
> > > > > > eMMC or even SPI flash, without needing to configure it differently.  
> > > > >
> > > > > This does change a couple of things though. The environment used to be
> > > > > loaded always from the same source, no matter the boot device. This
> > > > > means that if you would set an SD card, you would get the environment
> > > > > from the eMMC. Same thing for FEL. This is no longer the case.
> > > > >
> > > > > I don't know whether it's a good or a bad thing, but it should be
> > > > > mentionned.  
> > > >
> > > > This is true, I failed to mention that.
> > > >
> > > > To start a discussion on this:
> > > > I consider the current (fixed location) behaviour somewhat surprising and
> > > > limiting, and couldn't find a real use case where this would be required.
> > > > Happy to hear of one!
> > > > Instead I thought about those cases:
> > > > - There is some botched U-Boot plus environment on the eMMC. You want to
> > > > boot from SD card to have a clean start, possibly to fix it. But it will
> > > > load the possibly outdated, broken or even unrelated environment from eMMC.  
> > >
> > > This one might be a feature though. Being able to restore / fix an
> > > environment in the eMMC running from an SD card has save me a couple
> > > of times. Or booting from the SD card because the U-Boot on the eMMC
> > > is broken, while the environment is working.
> > >  
> > > > - You want to boot from SD card without touching the eMMC at all. Saving
> > > > the environment will spoil that.  
> > >
> > > But it goes against that one, which might be more important / sensible.
> > >  
> > > > - You want to have one image for all possible boot media.  
> > >
> > > That won't happen, only because NAND is a thing.

Looks like there are exactly 2 out of 146 Allwinner boards with mainline
(_defconfig) NAND support out there, and fortunately I haven't seen NAND
flash on modern (sunxi) boards in a while.
And if I see this correctly, there is neither eMMC nor SD card on the
C.H.I.P. devices? So their defconfig will also "boot on all possible boot
media".
What I was aiming at with "have one image for all possible boot media" is
to not throw away what works already: the _defconfig build for most (all?)
boards works naturally on eMMC and SD card, also on SPI flash, for the SPL
and for U-Boot proper. It's just the environment that requires fixing.
Which is what this series is about.

> > Some of this is perhaps an argument for adding a sub-command to specify
> > where the environment is to be read from.  Heuristics are still only a
> > best guess and won't get it right every time.  

Yeah, I was wondering about this as well. Maxime's case of: "I want to
save the environment to this device" seems like a valid use case.

Wouldn't a parameter to "saveenv" solve this rather elegantly? Happy to
look into a patch for this.

> I was mostly talking about the distribution of the image itself. While
> eMMC, SD and SPI flash can be made to take the same image, NAND will
> require a particular ECC and randomizer setup that requires that it's
> bundled separately.

Well, we can't save everyone ;-)
But given the number of boards with NAND flash out there and their
mainline U-Boot support ...

> > > And even then, I'm not really sure that it's a good thing. A U-Boot
> > > build these days is roughly in the same sizes than a stripped down
> > > Linux image. For an inferior solution in pretty much every aspect.  
> >
> > Hey now.  We aren't _quite_ that large.  And we are (really!) trying to
> > find a happy medium between "distros want X/Y/Z for everyone" and "can
> > we commonly get back to UNDER 512kB maybe?  Please?".  
> 
> I'm exagerating a little, but barely. A current build for the SoC
> Andre was mentionning takes 600kB. And since the trend has been for
> the binaries to grow for quite some time now, I'm pretty sure everyone
> will reserve 1MB just to be sure they have some room to spare.
> 
> Now, getting a kernel image to fit in a MB takes a bit of time, but
> it's not really impossible to achieve. And if you can reclaim that MB
> dedicated to U-Boot, it's actually fairly easy to do.
> 
> I've tried to have the size reduced at some time because we were
> corrupting our U-Boot binary as soon as we where using saveenv (which
> is probably one of the worst issue we could have), and it ended up
> with everyone agreeing that we needed to reduce the size, just not the
> one they were using.

I think that illustrates that we are coming from *very* different areas:
My kernels are 16-20 MB (distro kernels or my own monolithic test
kernels), plus tons of modules. I don't care about U-Boot being less than
600KB, since it goes into the first MB of an SD card or eMMC device, or
into the SPI flash, which is at least 2MB, but mostly 16MB. And honestly I
don't use the U-Boot environment too much, I am just relying on the distro
defaults environment to find the EFI boot app.
Your use cases seem to be quite different, mostly embedded in the original
sense ("doesn't look like a computer"), I guess. Also I guess you will
have your tailored config, probably plus tailored environment anyway.

So do you want a CONFIG_ option to turn this new behaviour off (or on)?
As mentioned I couldn't think of too many use cases, so considered this as
not needed.

> It's kind of weird that people haven't yet embraced defconfig as they
> are in Linux, where every distro has its own configuration, and it's
> perfectly fine.

That's indeed one thing I realised as well. In the past U-Boot wasn't very
good as using menuconfig to tailor some defconfig, though I think this
mostly works now. But I am not sure that's a real problem: if people care
about bootloader tuning, they are hopefully professionals and can be
bothered with providing their own .config.

Cheers,
Andre.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media
  2019-06-11 15:34           ` Andre Przywara
@ 2019-06-11 16:10             ` Tom Rini
  2019-06-12 13:08             ` Maxime Ripard
  1 sibling, 0 replies; 19+ messages in thread
From: Tom Rini @ 2019-06-11 16:10 UTC (permalink / raw)
  To: u-boot

On Tue, Jun 11, 2019 at 04:34:25PM +0100, Andre Przywara wrote:
> On Tue, 11 Jun 2019 16:53:58 +0200
> Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> 
> Hi,
> 
> > On Tue, Jun 11, 2019 at 10:28:19AM -0400, Tom Rini wrote:
> > > On Tue, Jun 11, 2019 at 11:37:28AM +0200, Maxime Ripard wrote:  
> > > > On Mon, Jun 10, 2019 at 10:11:39AM +0100, Andre Przywara wrote:  
> > > > > On Mon, 10 Jun 2019 10:30:37 +0200
> > > > > Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > >
> > > > > Hi Maxime,
> > > > >
> > > > > thanks for having a look!
> > > > >  
> > > > > > On Sat, Jun 08, 2019 at 02:26:53AM +0100, Andre Przywara wrote:  
> > > > > > > At the moment we need to configure the place where U-Boot tries to load
> > > > > > > its environment from at compile time. This is not only inflexible, but
> > > > > > > also unnecessary, as we have easy access to the boot source.
> > > > > > >
> > > > > > > This series prepares U-Boot on Allwinner boards to load the environment
> > > > > > > from the same media where the SPL and U-Boot proper were loaded from.
> > > > > > > This allows to keep one firmware binary, and copy it to an SD card,
> > > > > > > eMMC or even SPI flash, without needing to configure it differently.  
> > > > > >
> > > > > > This does change a couple of things though. The environment used to be
> > > > > > loaded always from the same source, no matter the boot device. This
> > > > > > means that if you would set an SD card, you would get the environment
> > > > > > from the eMMC. Same thing for FEL. This is no longer the case.
> > > > > >
> > > > > > I don't know whether it's a good or a bad thing, but it should be
> > > > > > mentionned.  
> > > > >
> > > > > This is true, I failed to mention that.
> > > > >
> > > > > To start a discussion on this:
> > > > > I consider the current (fixed location) behaviour somewhat surprising and
> > > > > limiting, and couldn't find a real use case where this would be required.
> > > > > Happy to hear of one!
> > > > > Instead I thought about those cases:
> > > > > - There is some botched U-Boot plus environment on the eMMC. You want to
> > > > > boot from SD card to have a clean start, possibly to fix it. But it will
> > > > > load the possibly outdated, broken or even unrelated environment from eMMC.  
> > > >
> > > > This one might be a feature though. Being able to restore / fix an
> > > > environment in the eMMC running from an SD card has save me a couple
> > > > of times. Or booting from the SD card because the U-Boot on the eMMC
> > > > is broken, while the environment is working.
> > > >  
> > > > > - You want to boot from SD card without touching the eMMC at all. Saving
> > > > > the environment will spoil that.  
> > > >
> > > > But it goes against that one, which might be more important / sensible.
> > > >  
> > > > > - You want to have one image for all possible boot media.  
> > > >
> > > > That won't happen, only because NAND is a thing.
> 
> Looks like there are exactly 2 out of 146 Allwinner boards with mainline
> (_defconfig) NAND support out there, and fortunately I haven't seen NAND
> flash on modern (sunxi) boards in a while.
> And if I see this correctly, there is neither eMMC nor SD card on the
> C.H.I.P. devices? So their defconfig will also "boot on all possible boot
> media".
> What I was aiming at with "have one image for all possible boot media" is
> to not throw away what works already: the _defconfig build for most (all?)
> boards works naturally on eMMC and SD card, also on SPI flash, for the SPL
> and for U-Boot proper. It's just the environment that requires fixing.
> Which is what this series is about.
> 
> > > Some of this is perhaps an argument for adding a sub-command to specify
> > > where the environment is to be read from.  Heuristics are still only a
> > > best guess and won't get it right every time.  
> 
> Yeah, I was wondering about this as well. Maxime's case of: "I want to
> save the environment to this device" seems like a valid use case.
> 
> Wouldn't a parameter to "saveenv" solve this rather elegantly? Happy to
> look into a patch for this.

I was thinking for the whole of "env".  Pass in a string maybe, and then
re-init the env portion based on that.  I think that might cover the
most cases of "we guessed wrong".

> > I was mostly talking about the distribution of the image itself. While
> > eMMC, SD and SPI flash can be made to take the same image, NAND will
> > require a particular ECC and randomizer setup that requires that it's
> > bundled separately.
> 
> Well, we can't save everyone ;-)
> But given the number of boards with NAND flash out there and their
> mainline U-Boot support ...

Is it even anything other than CHIP?

> > > > And even then, I'm not really sure that it's a good thing. A U-Boot
> > > > build these days is roughly in the same sizes than a stripped down
> > > > Linux image. For an inferior solution in pretty much every aspect.  
> > >
> > > Hey now.  We aren't _quite_ that large.  And we are (really!) trying to
> > > find a happy medium between "distros want X/Y/Z for everyone" and "can
> > > we commonly get back to UNDER 512kB maybe?  Please?".  
> > 
> > I'm exagerating a little, but barely. A current build for the SoC
> > Andre was mentionning takes 600kB. And since the trend has been for
> > the binaries to grow for quite some time now, I'm pretty sure everyone
> > will reserve 1MB just to be sure they have some room to spare.
> > 
> > Now, getting a kernel image to fit in a MB takes a bit of time, but
> > it's not really impossible to achieve. And if you can reclaim that MB
> > dedicated to U-Boot, it's actually fairly easy to do.
> > 
> > I've tried to have the size reduced at some time because we were
> > corrupting our U-Boot binary as soon as we where using saveenv (which
> > is probably one of the worst issue we could have), and it ended up
> > with everyone agreeing that we needed to reduce the size, just not the
> > one they were using.
> 
> I think that illustrates that we are coming from *very* different areas:
> My kernels are 16-20 MB (distro kernels or my own monolithic test
> kernels), plus tons of modules. I don't care about U-Boot being less than
> 600KB, since it goes into the first MB of an SD card or eMMC device, or
> into the SPI flash, which is at least 2MB, but mostly 16MB. And honestly I
> don't use the U-Boot environment too much, I am just relying on the distro
> defaults environment to find the EFI boot app.
> Your use cases seem to be quite different, mostly embedded in the original
> sense ("doesn't look like a computer"), I guess. Also I guess you will
> have your tailored config, probably plus tailored environment anyway.
> 
> So do you want a CONFIG_ option to turn this new behaviour off (or on)?
> As mentioned I couldn't think of too many use cases, so considered this as
> not needed.

This I think is helpful in illustrating the problem we have, which is
a very wide set of use cases.  We could save 70kB by dropping EFI
loader, and probably another "big" chunk of kB if we drop the oddball
and largely only used by "distro boot" commands.  But that would very
much break the distro boot use case, which is important :)

And not to single out EFI, since Heinrich is working on splitting out
"needed for UEFI self test" vs "needed for common use case/EBBR"
functionality, but the defaults for that start to conflict with "why is
my bootloader more than 256kB?".  And I think we need to take a harder
look at the DM framework and see if/how we can trim space.  Maybe figure
out a way to link-time/compile-time optimize the one-board/one-dts case.

> > It's kind of weird that people haven't yet embraced defconfig as they
> > are in Linux, where every distro has its own configuration, and it's
> > perfectly fine.
> 
> That's indeed one thing I realised as well. In the past U-Boot wasn't very
> good as using menuconfig to tailor some defconfig, though I think this
> mostly works now. But I am not sure that's a real problem: if people care
> about bootloader tuning, they are hopefully professionals and can be
> bothered with providing their own .config.

We're getting farther with the Kconfig migration but it's still a good
bit from complete.  And even then, people expect the defconfig to be
"it".

-- 
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/20190611/890dce96/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media
  2019-06-11 15:34           ` Andre Przywara
  2019-06-11 16:10             ` Tom Rini
@ 2019-06-12 13:08             ` Maxime Ripard
  1 sibling, 0 replies; 19+ messages in thread
From: Maxime Ripard @ 2019-06-12 13:08 UTC (permalink / raw)
  To: u-boot

On Tue, Jun 11, 2019 at 04:34:25PM +0100, Andre Przywara wrote:
> On Tue, 11 Jun 2019 16:53:58 +0200
> Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > On Tue, Jun 11, 2019 at 10:28:19AM -0400, Tom Rini wrote:
> > > On Tue, Jun 11, 2019 at 11:37:28AM +0200, Maxime Ripard wrote:
> > > > On Mon, Jun 10, 2019 at 10:11:39AM +0100, Andre Przywara wrote:
> > > > > On Mon, 10 Jun 2019 10:30:37 +0200
> > > > > Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > >
> > > > > Hi Maxime,
> > > > >
> > > > > thanks for having a look!
> > > > >
> > > > > > On Sat, Jun 08, 2019 at 02:26:53AM +0100, Andre Przywara wrote:
> > > > > > > At the moment we need to configure the place where U-Boot tries to load
> > > > > > > its environment from at compile time. This is not only inflexible, but
> > > > > > > also unnecessary, as we have easy access to the boot source.
> > > > > > >
> > > > > > > This series prepares U-Boot on Allwinner boards to load the environment
> > > > > > > from the same media where the SPL and U-Boot proper were loaded from.
> > > > > > > This allows to keep one firmware binary, and copy it to an SD card,
> > > > > > > eMMC or even SPI flash, without needing to configure it differently.
> > > > > >
> > > > > > This does change a couple of things though. The environment used to be
> > > > > > loaded always from the same source, no matter the boot device. This
> > > > > > means that if you would set an SD card, you would get the environment
> > > > > > from the eMMC. Same thing for FEL. This is no longer the case.
> > > > > >
> > > > > > I don't know whether it's a good or a bad thing, but it should be
> > > > > > mentionned.
> > > > >
> > > > > This is true, I failed to mention that.
> > > > >
> > > > > To start a discussion on this:
> > > > > I consider the current (fixed location) behaviour somewhat surprising and
> > > > > limiting, and couldn't find a real use case where this would be required.
> > > > > Happy to hear of one!
> > > > > Instead I thought about those cases:
> > > > > - There is some botched U-Boot plus environment on the eMMC. You want to
> > > > > boot from SD card to have a clean start, possibly to fix it. But it will
> > > > > load the possibly outdated, broken or even unrelated environment from eMMC.
> > > >
> > > > This one might be a feature though. Being able to restore / fix an
> > > > environment in the eMMC running from an SD card has save me a couple
> > > > of times. Or booting from the SD card because the U-Boot on the eMMC
> > > > is broken, while the environment is working.
> > > >
> > > > > - You want to boot from SD card without touching the eMMC at all. Saving
> > > > > the environment will spoil that.
> > > >
> > > > But it goes against that one, which might be more important / sensible.
> > > >
> > > > > - You want to have one image for all possible boot media.
> > > >
> > > > That won't happen, only because NAND is a thing.
>
> Looks like there are exactly 2 out of 146 Allwinner boards with mainline
> (_defconfig) NAND support out there, and fortunately I haven't seen NAND
> flash on modern (sunxi) boards in a while.

That's a rather big underestimation though. Most of the boards before
the H3 era have a NAND. UBI cannot deal with the NAND chips technology
yet, but they do have a NAND chip that should be supported in the
future.

> And if I see this correctly, there is neither eMMC nor SD card on the
> C.H.I.P. devices?

Yes.

> So their defconfig will also "boot on all possible boot media".

But not the Olinuxinos, Cubieboard, pcduino's, etc. Basically all the
A10/A13/A20/A33 boards have a NAND chip, and most of them will also
have an SD card slot.

> What I was aiming at with "have one image for all possible boot media" is
> to not throw away what works already: the _defconfig build for most (all?)
> boards works naturally on eMMC and SD card, also on SPI flash, for the SPL
> and for U-Boot proper. It's just the environment that requires fixing.
> Which is what this series is about.

It works because the support is not complete at this point, so it's
pretty fragile to market it as such.

> > > Some of this is perhaps an argument for adding a sub-command to specify
> > > where the environment is to be read from.  Heuristics are still only a
> > > best guess and won't get it right every time.
>
> Yeah, I was wondering about this as well. Maxime's case of: "I want to
> save the environment to this device" seems like a valid use case.
>
> Wouldn't a parameter to "saveenv" solve this rather elegantly? Happy to
> look into a patch for this.

Yeah, passing the argument is pretty elegant.

> > I was mostly talking about the distribution of the image itself. While
> > eMMC, SD and SPI flash can be made to take the same image, NAND will
> > require a particular ECC and randomizer setup that requires that it's
> > bundled separately.
>
> Well, we can't save everyone ;-)

Yeah, but my main objection is that you're saying you want to :)

> But given the number of boards with NAND flash out there and their
> mainline U-Boot support ...

Like I said, it's pretty much 50/50.

> > > > And even then, I'm not really sure that it's a good thing. A U-Boot
> > > > build these days is roughly in the same sizes than a stripped down
> > > > Linux image. For an inferior solution in pretty much every aspect.
> > >
> > > Hey now.  We aren't _quite_ that large.  And we are (really!) trying to
> > > find a happy medium between "distros want X/Y/Z for everyone" and "can
> > > we commonly get back to UNDER 512kB maybe?  Please?".
> >
> > I'm exagerating a little, but barely. A current build for the SoC
> > Andre was mentionning takes 600kB. And since the trend has been for
> > the binaries to grow for quite some time now, I'm pretty sure everyone
> > will reserve 1MB just to be sure they have some room to spare.
> >
> > Now, getting a kernel image to fit in a MB takes a bit of time, but
> > it's not really impossible to achieve. And if you can reclaim that MB
> > dedicated to U-Boot, it's actually fairly easy to do.
> >
> > I've tried to have the size reduced at some time because we were
> > corrupting our U-Boot binary as soon as we where using saveenv (which
> > is probably one of the worst issue we could have), and it ended up
> > with everyone agreeing that we needed to reduce the size, just not the
> > one they were using.
>
> I think that illustrates that we are coming from *very* different areas:
> My kernels are 16-20 MB (distro kernels or my own monolithic test
> kernels), plus tons of modules. I don't care about U-Boot being less than
> 600KB, since it goes into the first MB of an SD card or eMMC device, or
> into the SPI flash, which is at least 2MB, but mostly 16MB. And honestly I
> don't use the U-Boot environment too much, I am just relying on the distro
> defaults environment to find the EFI boot app.
> Your use cases seem to be quite different, mostly embedded in the original
> sense ("doesn't look like a computer"), I guess. Also I guess you will
> have your tailored config, probably plus tailored environment anyway.
>
> So do you want a CONFIG_ option to turn this new behaviour off (or on)?
> As mentioned I couldn't think of too many use cases, so considered this as
> not needed.

It's not really my point though. My point is that as U-Boot grows
bigger, we are wandering in the same order of magnitude that Linux,
and Linux can be turned into a "bootloader" as well. Loading Linux,
then loading your actual Linux image and kexec'ing is a thing.

The closer we come to Linux, the more valid that solution is,
especially now that everything that could be provided by U-Boot (like
PSCI) has been pushed down to ATF.

(And yeah, on embedded systems, you can even skip the kexec and just
run the minimal kernel)

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190612/e7294971/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2019-06-12 13:08 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-08  1:26 [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media Andre Przywara
2019-06-08  1:26 ` [U-Boot] [PATCH 1/5] env: allow undefined CONFIG_SYS_MMC_ENV_DEV Andre Przywara
2019-06-08 13:13   ` Tom Rini
2019-06-10  9:35     ` Andre Przywara
2019-06-10 21:53       ` Tom Rini
2019-06-08  1:26 ` [U-Boot] [PATCH 2/5] sunxi: autodetect SD/eMMC device for environment Andre Przywara
2019-06-08  1:26 ` [U-Boot] [PATCH 3/5] env: allow runtime determination of FAT environment partition Andre Przywara
2019-06-08 13:13   ` Tom Rini
2019-06-08  1:26 ` [U-Boot] [PATCH 4/5] sunxi: use FAT environment from boot source Andre Przywara
2019-06-08  1:26 ` [U-Boot] [PATCH 5/5] sunxi: use boot source for determining environment location Andre Przywara
2019-06-10  8:30 ` [U-Boot] [PATCH 0/5] sunxi: env: Load environment from boot media Maxime Ripard
2019-06-10  9:11   ` Andre Przywara
2019-06-11  9:37     ` Maxime Ripard
2019-06-11 14:28       ` Tom Rini
2019-06-11 14:53         ` Maxime Ripard
2019-06-11 15:20           ` Tom Rini
2019-06-11 15:34           ` Andre Przywara
2019-06-11 16:10             ` Tom Rini
2019-06-12 13:08             ` Maxime Ripard

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.