[PATCH u-boot 0/4] mmc: Explain and cleanup partition selection

[PATCH u-boot 0/4] mmc: Explain and cleanup partition selection

[Pali Rohár]

Some people do not want to read review comments in emails. So put
comments and explanation into the source code itself; make emmc
partition selection code more explicit and validate configuration in
bubt command.

Pali Rohár (4):
  mmc: spl: Make partition choice in
    default_spl_mmc_emmc_boot_partition() more explicit
  cmd: mvebu/bubt: Validate EXT_CSD[179] eMMC register in
    mmc_burn_image()
  sunxi: eMMC: Add comments explaining mapping between bootpart and
    mmc_switch_part()
  board: purism: Use U-Boot mmc function for converting boot part to
    part access

 arch/arm/mach-sunxi/board.c    | 12 ++++++++++-
 board/purism/librem5/librem5.c |  6 +-----
 cmd/mvebu/bubt.c               | 24 +++++++++++++++++++--
 common/spl/spl_mmc.c           | 38 +++++++++++++++++++++++++++-------
 4 files changed, 64 insertions(+), 16 deletions(-)

-- 
2.20.1

[PATCH u-boot 1/4] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit

[Pali Rohár]

To make eMMC partition choosing in default_spl_mmc_emmc_boot_partition()
function better understandable, rewrite it via explicit switch-case code
pattern.

Also add a warning when eMMC EXT_CSD[179] register is configured by user to
value which is not suitable for eMMC booting and SPL do not know how to
interpret it.

Note that when booting from eMMC device via EXT_CSD[179] register is
explicitly disabled then SPL still loads and boots from this eMMC device
from User Area partition. This behavior was not changed in this commit and
should be revisited in the future.

Signed-off-by: Pali Rohár <pali@kernel.org>

---
This patch depends on another patch:
mmc: spl: Add comments for default_spl_mmc_emmc_boot_partition()
https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/
---
 common/spl/spl_mmc.c | 38 ++++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index 2426500dbcb9..df20a75efc29 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -408,15 +408,37 @@ int default_spl_mmc_emmc_boot_partition(struct mmc *mmc)
 	 *
 	 * Note: See difference between EXT_CSD_EXTRACT_PARTITION_ACCESS
 	 * and EXT_CSD_EXTRACT_BOOT_PART, specially about User area value.
-	 *
-	 * FIXME: When booting from this eMMC device is explicitly
-	 * disabled then we use User area for booting. This is incorrect.
-	 * Probably we should skip this eMMC device and select the next
-	 * one for booting. Or at least throw warning about this fallback.
 	 */
-	part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
-	if (part == 7)
-		part = 0;
+	if (mmc->part_config == MMCPART_NOAVAILABLE)
+		part = 0; /* If partitions are not supported then we have only User Area partition */
+	else {
+		switch(EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) {
+		case 0: /* Booting from this eMMC device is disabled */
+#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
+			puts("spl: WARNING: Booting from this eMMC device is disabled in EXT_CSD[179] register\n");
+			puts("spl: WARNING: Continuing anyway and selecting User Area partition for booting\n");
+#endif
+			/* FIXME: This is incorrect and probably we should select next eMMC device for booting */
+			part = 0;
+			break;
+		case 1: /* Boot partition 1 is used for booting */
+			part = 1;
+			break;
+		case 2: /* Boot partition 2 is used for booting */
+			part = 2;
+			break;
+		case 7: /* User area is used for booting */
+			part = 0;
+			break;
+		default: /* Other values are reserved */
+#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
+			puts("spl: WARNING: EXT_CSD[179] register is configured to boot from Reserved value\n");
+			puts("spl: WARNING: Selecting User Area partition for booting\n");
+#endif
+			part = 0;
+			break;
+		}
+	}
 #endif
 	return part;
 }
-- 
2.20.1

[PATCH u-boot 2/4] cmd: mvebu/bubt: Validate EXT_CSD[179] eMMC register in mmc_burn_image()

[Pali Rohár]

When determining eMMC boot partition for a bootloader, validate that
EXT_CSD[179] eMMC register is set to recognized value.

This prevent situation that EXT_CSD[179] Boot Enable value is improperly
parsed and passed into EXT_CSD[179] Partition Access.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 cmd/mvebu/bubt.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
index ca24a5c1c4ba..b8fe7c7a1461 100644
--- a/cmd/mvebu/bubt.c
+++ b/cmd/mvebu/bubt.c
@@ -223,9 +223,29 @@ static int mmc_burn_image(size_t image_size)
 	orig_part = mmc->block_dev.hwpart;
 #endif
 
-	part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
-	if (part == 7)
+	if (mmc->part_config == MMCPART_NOAVAILABLE) {
 		part = 0;
+	} else {
+		switch (EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) {
+		case 0: /* Booting from this eMMC device is disabled */
+			printf("Error - Booting from this eMMC device is disabled\n");
+			printf("Hint: Use 'mmc partconf' command to choose boot partition\n");
+			return -ENODEV;
+		case 1: /* Boot partition 1 is used for booting */
+			part = 1;
+			break;
+		case 2: /* Boot partition 2 is used for booting */
+			part = 2;
+			break;
+		case 7: /* User area is used for booting */
+			part = 0;
+			break;
+		default: /* Other values are reserved / unsupported */
+			printf("Error - This eMMC device has configured Reserved boot option\n");
+			printf("Hint: Use 'mmc partconf' command to choose boot partition\n");
+			return -ENODEV;
+		}
+	}
 
 #ifdef CONFIG_BLK
 	err = blk_dselect_hwpart(blk_desc, part);
-- 
2.20.1

[PATCH u-boot 3/4] sunxi: eMMC: Add comments explaining mapping between bootpart and mmc_switch_part()

[Pali Rohár]

Mapping between bootpart taken from EXT_CSD_EXTRACT_BOOT_PART() and
Partition Access bits used by the mmc_switch_part() function may be quite
misleading. So add extended comment describing why in sunxi case is this
mapping just a simple identity. Because in generic case this mapping
requires non-trivial mapping table.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 arch/arm/mach-sunxi/board.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
index 391a65a5495f..73519f6262ec 100644
--- a/arch/arm/mach-sunxi/board.c
+++ b/arch/arm/mach-sunxi/board.c
@@ -381,7 +381,17 @@ static bool sunxi_valid_emmc_boot(struct mmc *mmc)
 	    (mmc->ext_csd[EXT_CSD_BOOT_BUS_WIDTH] & 0x1b) != 0x09)
 		return false;
 
-	/* Partition 0 is the user data partition, bootpart must be 1 or 2. */
+	/*
+	 * bootpart == 0 means that eMMC booting is disabled.
+	 * bootpart == 1 or 2 means to boot from Boot Partition 1 or 2.
+	 * bootpart == 7 means to boot from User Area.
+	 * Other bootpart values are reserved.
+	 * mmc_switch_part() takes partition access value which is:
+	 * 0 for User Area; 1-2 for Boot Partition 1-2; 3 for RPMB; 4-7 for GP 1-4.
+	 * We allow booting only from Boot Partition 1 or 2 so
+	 * bootpart mapping between EXT_CSD_EXTRACT_BOOT_PART()
+	 * and mmc_switch_part() is straightforward identity.
+	 */
 	if (bootpart != 1 && bootpart != 2)
 		return false;
 
-- 
2.20.1

[PATCH u-boot 4/4] board: purism: Use U-Boot mmc function for converting boot part to part access

[Pali Rohár]

eMMC Boot Partition Enable bits in mmc->part_config (EXT_CSD[179]) has
different coding than eMMC Partition Access bits.

Use spl_mmc_emmc_boot_partition() function which does this conversion
properly (hopefully).

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 board/purism/librem5/librem5.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/board/purism/librem5/librem5.c b/board/purism/librem5/librem5.c
index 386ed1b4fb22..36ecac9d9ed0 100644
--- a/board/purism/librem5/librem5.c
+++ b/board/purism/librem5/librem5.c
@@ -41,11 +41,7 @@ int board_early_init_f(void)
 #if IS_ENABLED(CONFIG_LOAD_ENV_FROM_MMC_BOOT_PARTITION)
 uint board_mmc_get_env_part(struct mmc *mmc)
 {
-	uint part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
-
-	if (part == 7)
-		part = 0;
-	return part;
+	return spl_mmc_emmc_boot_partition(mmc);
 }
 #endif
 
-- 
2.20.1

Re: [PATCH u-boot 3/4] sunxi: eMMC: Add comments explaining mapping between bootpart and mmc_switch_part()

[Andre Przywara]

On Thu, 13 Apr 2023 23:10:56 +0200
Pali Rohár <pali@kernel.org> wrote:

> Mapping between bootpart taken from EXT_CSD_EXTRACT_BOOT_PART() and
> Partition Access bits used by the mmc_switch_part() function may be quite
> misleading. So add extended comment describing why in sunxi case is this
> mapping just a simple identity. Because in generic case this mapping
> requires non-trivial mapping table.

Ah, indeed, many thanks for clearing this up! I missed the subtleties
of bootpart meaning slightly different things between
EXT_CSD_EXTRACT_BOOT_PART and mmc_switch_part(). It looks like we were
quite lucky as we only care about 1 and 2 here!

> Signed-off-by: Pali Rohár <pali@kernel.org>

Acked-by: Andre Przywara <andre.przywara@arm.com>

Queued for sunxi/master.

Cheers,
Andre

> ---
>  arch/arm/mach-sunxi/board.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-sunxi/board.c b/arch/arm/mach-sunxi/board.c
> index 391a65a5495f..73519f6262ec 100644
> --- a/arch/arm/mach-sunxi/board.c
> +++ b/arch/arm/mach-sunxi/board.c
> @@ -381,7 +381,17 @@ static bool sunxi_valid_emmc_boot(struct mmc *mmc)
>  	    (mmc->ext_csd[EXT_CSD_BOOT_BUS_WIDTH] & 0x1b) != 0x09)
>  		return false;
>  
> -	/* Partition 0 is the user data partition, bootpart must be 1 or 2. */
> +	/*
> +	 * bootpart == 0 means that eMMC booting is disabled.
> +	 * bootpart == 1 or 2 means to boot from Boot Partition 1 or 2.
> +	 * bootpart == 7 means to boot from User Area.
> +	 * Other bootpart values are reserved.
> +	 * mmc_switch_part() takes partition access value which is:
> +	 * 0 for User Area; 1-2 for Boot Partition 1-2; 3 for RPMB; 4-7 for GP 1-4.
> +	 * We allow booting only from Boot Partition 1 or 2 so
> +	 * bootpart mapping between EXT_CSD_EXTRACT_BOOT_PART()
> +	 * and mmc_switch_part() is straightforward identity.
> +	 */
>  	if (bootpart != 1 && bootpart != 2)
>  		return false;
>  

Re: [PATCH u-boot 4/4] board: purism: Use U-Boot mmc function for converting boot part to part access

[Angus Ainslie]

On 2023-04-13 14:10, Pali Rohár wrote:
> eMMC Boot Partition Enable bits in mmc->part_config (EXT_CSD[179]) has
> different coding than eMMC Partition Access bits.
> 
> Use spl_mmc_emmc_boot_partition() function which does this conversion
> properly (hopefully).
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Angus Ainslie <angus@akkea.ca>

Re: [PATCH u-boot 2/4] cmd: mvebu/bubt: Validate EXT_CSD[179] eMMC register in mmc_burn_image()

[Stefan Roese]

On 4/13/23 23:10, Pali Rohár wrote:
> When determining eMMC boot partition for a bootloader, validate that
> EXT_CSD[179] eMMC register is set to recognized value.
> 
> This prevent situation that EXT_CSD[179] Boot Enable value is improperly
> parsed and passed into EXT_CSD[179] Partition Access.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   cmd/mvebu/bubt.c | 24 ++++++++++++++++++++++--
>   1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
> index ca24a5c1c4ba..b8fe7c7a1461 100644
> --- a/cmd/mvebu/bubt.c
> +++ b/cmd/mvebu/bubt.c
> @@ -223,9 +223,29 @@ static int mmc_burn_image(size_t image_size)
>   	orig_part = mmc->block_dev.hwpart;
>   #endif
>   
> -	part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
> -	if (part == 7)
> +	if (mmc->part_config == MMCPART_NOAVAILABLE) {
>   		part = 0;
> +	} else {
> +		switch (EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) {
> +		case 0: /* Booting from this eMMC device is disabled */
> +			printf("Error - Booting from this eMMC device is disabled\n");
> +			printf("Hint: Use 'mmc partconf' command to choose boot partition\n");
> +			return -ENODEV;
> +		case 1: /* Boot partition 1 is used for booting */
> +			part = 1;
> +			break;
> +		case 2: /* Boot partition 2 is used for booting */
> +			part = 2;
> +			break;
> +		case 7: /* User area is used for booting */
> +			part = 0;
> +			break;
> +		default: /* Other values are reserved / unsupported */
> +			printf("Error - This eMMC device has configured Reserved boot option\n");
> +			printf("Hint: Use 'mmc partconf' command to choose boot partition\n");
> +			return -ENODEV;
> +		}
> +	}
>   
>   #ifdef CONFIG_BLK
>   	err = blk_dselect_hwpart(blk_desc, part);

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot 2/4] cmd: mvebu/bubt: Validate EXT_CSD[179] eMMC register in mmc_burn_image()

[Pali Rohár]

+ Martin

On Thursday 13 April 2023 23:10:55 Pali Rohár wrote:
> When determining eMMC boot partition for a bootloader, validate that
> EXT_CSD[179] eMMC register is set to recognized value.
> 
> This prevent situation that EXT_CSD[179] Boot Enable value is improperly
> parsed and passed into EXT_CSD[179] Partition Access.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  cmd/mvebu/bubt.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c
> index ca24a5c1c4ba..b8fe7c7a1461 100644
> --- a/cmd/mvebu/bubt.c
> +++ b/cmd/mvebu/bubt.c
> @@ -223,9 +223,29 @@ static int mmc_burn_image(size_t image_size)
>  	orig_part = mmc->block_dev.hwpart;
>  #endif
>  
> -	part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
> -	if (part == 7)
> +	if (mmc->part_config == MMCPART_NOAVAILABLE) {
>  		part = 0;
> +	} else {
> +		switch (EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) {
> +		case 0: /* Booting from this eMMC device is disabled */
> +			printf("Error - Booting from this eMMC device is disabled\n");
> +			printf("Hint: Use 'mmc partconf' command to choose boot partition\n");
> +			return -ENODEV;
> +		case 1: /* Boot partition 1 is used for booting */
> +			part = 1;
> +			break;
> +		case 2: /* Boot partition 2 is used for booting */
> +			part = 2;
> +			break;
> +		case 7: /* User area is used for booting */
> +			part = 0;
> +			break;
> +		default: /* Other values are reserved / unsupported */
> +			printf("Error - This eMMC device has configured Reserved boot option\n");
> +			printf("Hint: Use 'mmc partconf' command to choose boot partition\n");
> +			return -ENODEV;
> +		}
> +	}
>  
>  #ifdef CONFIG_BLK
>  	err = blk_dselect_hwpart(blk_desc, part);
> -- 
> 2.20.1
> 

Re: [PATCH u-boot 0/4] mmc: Explain and cleanup partition selection

[Jaehoon Chung]

Hi,

On 4/14/23 06:10, Pali Rohár wrote:
> Some people do not want to read review comments in emails. So put
> comments and explanation into the source code itself; make emmc
> partition selection code more explicit and validate configuration in
> bubt command.

Sorry for too late. 
After applied this patch-set, some board are failed with spl's size overflowed 

The below is one of failed log.

+arm-linux-gnueabi-ld.bfd: region `.sram' overflowed by 352 bytes
+make[2]: *** [scripts/Makefile.spl:527: spl/u-boot-spl] Error 1
+make[1]: *** [Makefile:2049: spl/u-boot-spl] Error 2


Best Regards,
Jaehoon Chung

> 
> Pali Rohár (4):
>   mmc: spl: Make partition choice in
>     default_spl_mmc_emmc_boot_partition() more explicit
>   cmd: mvebu/bubt: Validate EXT_CSD[179] eMMC register in
>     mmc_burn_image()
>   sunxi: eMMC: Add comments explaining mapping between bootpart and
>     mmc_switch_part()
>   board: purism: Use U-Boot mmc function for converting boot part to
>     part access
> 
>  arch/arm/mach-sunxi/board.c    | 12 ++++++++++-
>  board/purism/librem5/librem5.c |  6 +-----
>  cmd/mvebu/bubt.c               | 24 +++++++++++++++++++--
>  common/spl/spl_mmc.c           | 38 +++++++++++++++++++++++++++-------
>  4 files changed, 64 insertions(+), 16 deletions(-)

Re: [PATCH u-boot 0/4] mmc: Explain and cleanup partition selection

[Pali Rohár]

On Monday 03 July 2023 21:16:37 Jaehoon Chung wrote:
> Hi,
> 
> On 4/14/23 06:10, Pali Rohár wrote:
> > Some people do not want to read review comments in emails. So put
> > comments and explanation into the source code itself; make emmc
> > partition selection code more explicit and validate configuration in
> > bubt command.
> 
> Sorry for too late. 
> After applied this patch-set, some board are failed with spl's size overflowed 
> 
> The below is one of failed log.
> 
> +arm-linux-gnueabi-ld.bfd: region `.sram' overflowed by 352 bytes
> +make[2]: *** [scripts/Makefile.spl:527: spl/u-boot-spl] Error 1
> +make[1]: *** [Makefile:2049: spl/u-boot-spl] Error 2

Crap, and again we have there same issue. Patch after months does not
apply anymore. And resending the patch would again result in the same
problem that it would not apply anymore after half of the year...


Anyway, only first patch is SPL specific, other should apply without any
problem.

> 
> Best Regards,
> Jaehoon Chung
> 
> > 
> > Pali Rohár (4):
> >   mmc: spl: Make partition choice in
> >     default_spl_mmc_emmc_boot_partition() more explicit
> >   cmd: mvebu/bubt: Validate EXT_CSD[179] eMMC register in
> >     mmc_burn_image()
> >   sunxi: eMMC: Add comments explaining mapping between bootpart and
> >     mmc_switch_part()
> >   board: purism: Use U-Boot mmc function for converting boot part to
> >     part access
> > 
> >  arch/arm/mach-sunxi/board.c    | 12 ++++++++++-
> >  board/purism/librem5/librem5.c |  6 +-----
> >  cmd/mvebu/bubt.c               | 24 +++++++++++++++++++--
> >  common/spl/spl_mmc.c           | 38 +++++++++++++++++++++++++++-------
> >  4 files changed, 64 insertions(+), 16 deletions(-)
> 

[PATCH v2 u-boot] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit

[Pali Rohár]

To make eMMC partition choosing in default_spl_mmc_emmc_boot_partition()
function better understandable, rewrite it via explicit switch-case code
pattern.

Also add a warning when eMMC EXT_CSD[179] register is configured by user to
value which is not suitable for eMMC booting and SPL do not know how to
interpret it.

Note that when booting from eMMC device via EXT_CSD[179] register is
explicitly disabled then SPL still loads and boots from this eMMC device
from User Area partition. This behavior was not changed in this commit and
should be revisited in the future.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
Changes in v2:
* Disable showing warning on sama5d2_xplained due to size restrictions
---
This patch depends on another patch:
mmc: spl: Add comments for default_spl_mmc_emmc_boot_partition()
https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/
---
 common/spl/Kconfig   |  7 +++++++
 common/spl/spl_mmc.c | 46 ++++++++++++++++++++++++++++++++++++--------
 2 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index 865571d4579c..0574d22b3b25 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -855,6 +855,13 @@ config SPL_MMC_WRITE
 	help
 	  Enable write access to MMC and SD Cards in SPL
 
+config SPL_MMC_WARNINGS
+	bool "Print MMC warnings"
+	depends on SPL_MMC
+	default y if !TARGET_SAMA5D2_XPLAINED
+	help
+	  Print SPL MMC warnings. You can disable this option to reduce SPL size.
+
 
 config SPL_MPC8XXX_INIT_DDR
 	bool "Support MPC8XXX DDR init"
diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index f7a42a11477d..ec424ceded0e 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -408,15 +408,45 @@ int default_spl_mmc_emmc_boot_partition(struct mmc *mmc)
 	 *
 	 * Note: See difference between EXT_CSD_EXTRACT_PARTITION_ACCESS
 	 * and EXT_CSD_EXTRACT_BOOT_PART, specially about User area value.
-	 *
-	 * FIXME: When booting from this eMMC device is explicitly
-	 * disabled then we use User area for booting. This is incorrect.
-	 * Probably we should skip this eMMC device and select the next
-	 * one for booting. Or at least throw warning about this fallback.
 	 */
-	part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
-	if (part == 7)
-		part = 0;
+	if (mmc->part_config == MMCPART_NOAVAILABLE)
+		part = 0; /* If partitions are not supported then we have only User Area partition */
+	else {
+		switch(EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) {
+		case 0: /* Booting from this eMMC device is disabled */
+#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
+#ifdef CONFIG_SPL_MMC_WARNINGS
+			puts("spl: WARNING: Booting from this eMMC device is disabled in EXT_CSD[179] register\n");
+			puts("spl: WARNING: Continuing anyway and selecting User Area partition for booting\n");
+#else
+			puts("spl: mmc: fallback to user area\n");
+#endif
+#endif
+			/* FIXME: This is incorrect and probably we should select next eMMC device for booting */
+			part = 0;
+			break;
+		case 1: /* Boot partition 1 is used for booting */
+			part = 1;
+			break;
+		case 2: /* Boot partition 2 is used for booting */
+			part = 2;
+			break;
+		case 7: /* User area is used for booting */
+			part = 0;
+			break;
+		default: /* Other values are reserved */
+#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
+#ifdef CONFIG_SPL_MMC_WARNINGS
+			puts("spl: WARNING: EXT_CSD[179] register is configured to boot from Reserved value\n");
+			puts("spl: WARNING: Selecting User Area partition for booting\n");
+#else
+			puts("spl: mmc: fallback to user area\n");
+#endif
+#endif
+			part = 0;
+			break;
+		}
+	}
 #endif
 	return part;
 }
-- 
2.20.1

Re: [PATCH u-boot 0/4] mmc: Explain and cleanup partition selection

[Pali Rohár]

On Thursday 06 July 2023 12:50:34 Pali Rohár wrote:
> On Monday 03 July 2023 21:16:37 Jaehoon Chung wrote:
> > Hi,
> > 
> > On 4/14/23 06:10, Pali Rohár wrote:
> > > Some people do not want to read review comments in emails. So put
> > > comments and explanation into the source code itself; make emmc
> > > partition selection code more explicit and validate configuration in
> > > bubt command.
> > 
> > Sorry for too late. 
> > After applied this patch-set, some board are failed with spl's size overflowed 
> > 
> > The below is one of failed log.
> > 
> > +arm-linux-gnueabi-ld.bfd: region `.sram' overflowed by 352 bytes
> > +make[2]: *** [scripts/Makefile.spl:527: spl/u-boot-spl] Error 1
> > +make[1]: *** [Makefile:2049: spl/u-boot-spl] Error 2
> 
> Crap, and again we have there same issue. Patch after months does not
> apply anymore. And resending the patch would again result in the same
> problem that it would not apply anymore after half of the year...

Here is v2 patch with simple fix for that platform:
https://patchwork.ozlabs.org/project/uboot/patch/20230706173502.2796-1-pali@kernel.org/

And it passed _now_ CI pipeline:
https://github.com/u-boot/u-boot/pull/340

> 
> Anyway, only first patch is SPL specific, other should apply without any
> problem.
> 
> > 
> > Best Regards,
> > Jaehoon Chung
> > 
> > > 
> > > Pali Rohár (4):
> > >   mmc: spl: Make partition choice in
> > >     default_spl_mmc_emmc_boot_partition() more explicit
> > >   cmd: mvebu/bubt: Validate EXT_CSD[179] eMMC register in
> > >     mmc_burn_image()
> > >   sunxi: eMMC: Add comments explaining mapping between bootpart and
> > >     mmc_switch_part()
> > >   board: purism: Use U-Boot mmc function for converting boot part to
> > >     part access
> > > 
> > >  arch/arm/mach-sunxi/board.c    | 12 ++++++++++-
> > >  board/purism/librem5/librem5.c |  6 +-----
> > >  cmd/mvebu/bubt.c               | 24 +++++++++++++++++++--
> > >  common/spl/spl_mmc.c           | 38 +++++++++++++++++++++++++++-------
> > >  4 files changed, 64 insertions(+), 16 deletions(-)
> > 

Re: [PATCH v2 u-boot] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4049 bytes --]

On Thu, Jul 06, 2023 at 07:35:02PM +0200, Pali Rohár wrote:
> To make eMMC partition choosing in default_spl_mmc_emmc_boot_partition()
> function better understandable, rewrite it via explicit switch-case code
> pattern.
> 
> Also add a warning when eMMC EXT_CSD[179] register is configured by user to
> value which is not suitable for eMMC booting and SPL do not know how to
> interpret it.
> 
> Note that when booting from eMMC device via EXT_CSD[179] register is
> explicitly disabled then SPL still loads and boots from this eMMC device
> from User Area partition. This behavior was not changed in this commit and
> should be revisited in the future.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
> Changes in v2:
> * Disable showing warning on sama5d2_xplained due to size restrictions
> ---
> This patch depends on another patch:
> mmc: spl: Add comments for default_spl_mmc_emmc_boot_partition()
> https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/
> ---
>  common/spl/Kconfig   |  7 +++++++
>  common/spl/spl_mmc.c | 46 ++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 45 insertions(+), 8 deletions(-)
> 
> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> index 865571d4579c..0574d22b3b25 100644
> --- a/common/spl/Kconfig
> +++ b/common/spl/Kconfig
> @@ -855,6 +855,13 @@ config SPL_MMC_WRITE
>  	help
>  	  Enable write access to MMC and SD Cards in SPL
>  
> +config SPL_MMC_WARNINGS
> +	bool "Print MMC warnings"
> +	depends on SPL_MMC
> +	default y if !TARGET_SAMA5D2_XPLAINED
> +	help
> +	  Print SPL MMC warnings. You can disable this option to reduce SPL size.
> +
>  
>  config SPL_MPC8XXX_INIT_DDR
>  	bool "Support MPC8XXX DDR init"
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index f7a42a11477d..ec424ceded0e 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -408,15 +408,45 @@ int default_spl_mmc_emmc_boot_partition(struct mmc *mmc)
>  	 *
>  	 * Note: See difference between EXT_CSD_EXTRACT_PARTITION_ACCESS
>  	 * and EXT_CSD_EXTRACT_BOOT_PART, specially about User area value.
> -	 *
> -	 * FIXME: When booting from this eMMC device is explicitly
> -	 * disabled then we use User area for booting. This is incorrect.
> -	 * Probably we should skip this eMMC device and select the next
> -	 * one for booting. Or at least throw warning about this fallback.
>  	 */
> -	part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
> -	if (part == 7)
> -		part = 0;
> +	if (mmc->part_config == MMCPART_NOAVAILABLE)
> +		part = 0; /* If partitions are not supported then we have only User Area partition */
> +	else {
> +		switch(EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) {
> +		case 0: /* Booting from this eMMC device is disabled */
> +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> +#ifdef CONFIG_SPL_MMC_WARNINGS
> +			puts("spl: WARNING: Booting from this eMMC device is disabled in EXT_CSD[179] register\n");
> +			puts("spl: WARNING: Continuing anyway and selecting User Area partition for booting\n");
> +#else
> +			puts("spl: mmc: fallback to user area\n");
> +#endif
> +#endif
> +			/* FIXME: This is incorrect and probably we should select next eMMC device for booting */
> +			part = 0;
> +			break;
> +		case 1: /* Boot partition 1 is used for booting */
> +			part = 1;
> +			break;
> +		case 2: /* Boot partition 2 is used for booting */
> +			part = 2;
> +			break;
> +		case 7: /* User area is used for booting */
> +			part = 0;
> +			break;
> +		default: /* Other values are reserved */
> +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> +#ifdef CONFIG_SPL_MMC_WARNINGS
> +			puts("spl: WARNING: EXT_CSD[179] register is configured to boot from Reserved value\n");
> +			puts("spl: WARNING: Selecting User Area partition for booting\n");
> +#else
> +			puts("spl: mmc: fallback to user area\n");
> +#endif
> +#endif
> +			part = 0;
> +			break;
> +		}
> +	}
>  #endif

Please just use debug() for these messages.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 u-boot] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit

[Pali Rohár]

On Thursday 06 July 2023 13:42:18 Tom Rini wrote:
> On Thu, Jul 06, 2023 at 07:35:02PM +0200, Pali Rohár wrote:
> > To make eMMC partition choosing in default_spl_mmc_emmc_boot_partition()
> > function better understandable, rewrite it via explicit switch-case code
> > pattern.
> > 
> > Also add a warning when eMMC EXT_CSD[179] register is configured by user to
> > value which is not suitable for eMMC booting and SPL do not know how to
> > interpret it.
> > 
> > Note that when booting from eMMC device via EXT_CSD[179] register is
> > explicitly disabled then SPL still loads and boots from this eMMC device
> > from User Area partition. This behavior was not changed in this commit and
> > should be revisited in the future.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> > Changes in v2:
> > * Disable showing warning on sama5d2_xplained due to size restrictions
> > ---
> > This patch depends on another patch:
> > mmc: spl: Add comments for default_spl_mmc_emmc_boot_partition()
> > https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/
> > ---
> >  common/spl/Kconfig   |  7 +++++++
> >  common/spl/spl_mmc.c | 46 ++++++++++++++++++++++++++++++++++++--------
> >  2 files changed, 45 insertions(+), 8 deletions(-)
> > 
> > diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> > index 865571d4579c..0574d22b3b25 100644
> > --- a/common/spl/Kconfig
> > +++ b/common/spl/Kconfig
> > @@ -855,6 +855,13 @@ config SPL_MMC_WRITE
> >  	help
> >  	  Enable write access to MMC and SD Cards in SPL
> >  
> > +config SPL_MMC_WARNINGS
> > +	bool "Print MMC warnings"
> > +	depends on SPL_MMC
> > +	default y if !TARGET_SAMA5D2_XPLAINED
> > +	help
> > +	  Print SPL MMC warnings. You can disable this option to reduce SPL size.
> > +
> >  
> >  config SPL_MPC8XXX_INIT_DDR
> >  	bool "Support MPC8XXX DDR init"
> > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> > index f7a42a11477d..ec424ceded0e 100644
> > --- a/common/spl/spl_mmc.c
> > +++ b/common/spl/spl_mmc.c
> > @@ -408,15 +408,45 @@ int default_spl_mmc_emmc_boot_partition(struct mmc *mmc)
> >  	 *
> >  	 * Note: See difference between EXT_CSD_EXTRACT_PARTITION_ACCESS
> >  	 * and EXT_CSD_EXTRACT_BOOT_PART, specially about User area value.
> > -	 *
> > -	 * FIXME: When booting from this eMMC device is explicitly
> > -	 * disabled then we use User area for booting. This is incorrect.
> > -	 * Probably we should skip this eMMC device and select the next
> > -	 * one for booting. Or at least throw warning about this fallback.
> >  	 */
> > -	part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
> > -	if (part == 7)
> > -		part = 0;
> > +	if (mmc->part_config == MMCPART_NOAVAILABLE)
> > +		part = 0; /* If partitions are not supported then we have only User Area partition */
> > +	else {
> > +		switch(EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) {
> > +		case 0: /* Booting from this eMMC device is disabled */
> > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > +#ifdef CONFIG_SPL_MMC_WARNINGS
> > +			puts("spl: WARNING: Booting from this eMMC device is disabled in EXT_CSD[179] register\n");
> > +			puts("spl: WARNING: Continuing anyway and selecting User Area partition for booting\n");
> > +#else
> > +			puts("spl: mmc: fallback to user area\n");
> > +#endif
> > +#endif
> > +			/* FIXME: This is incorrect and probably we should select next eMMC device for booting */
> > +			part = 0;
> > +			break;
> > +		case 1: /* Boot partition 1 is used for booting */
> > +			part = 1;
> > +			break;
> > +		case 2: /* Boot partition 2 is used for booting */
> > +			part = 2;
> > +			break;
> > +		case 7: /* User area is used for booting */
> > +			part = 0;
> > +			break;
> > +		default: /* Other values are reserved */
> > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > +#ifdef CONFIG_SPL_MMC_WARNINGS
> > +			puts("spl: WARNING: EXT_CSD[179] register is configured to boot from Reserved value\n");
> > +			puts("spl: WARNING: Selecting User Area partition for booting\n");
> > +#else
> > +			puts("spl: mmc: fallback to user area\n");
> > +#endif
> > +#endif
> > +			part = 0;
> > +			break;
> > +		}
> > +	}
> >  #endif
> 
> Please just use debug() for these messages.
> 
> -- 
> Tom

All other error/warning messages in this file are printed via puts().
So I'm just following the current style (and I'm really not going to
change all occurrences in this patch).

Re: [PATCH v2 u-boot] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5246 bytes --]

On Thu, Jul 06, 2023 at 07:49:18PM +0200, Pali Rohár wrote:
> On Thursday 06 July 2023 13:42:18 Tom Rini wrote:
> > On Thu, Jul 06, 2023 at 07:35:02PM +0200, Pali Rohár wrote:
> > > To make eMMC partition choosing in default_spl_mmc_emmc_boot_partition()
> > > function better understandable, rewrite it via explicit switch-case code
> > > pattern.
> > > 
> > > Also add a warning when eMMC EXT_CSD[179] register is configured by user to
> > > value which is not suitable for eMMC booting and SPL do not know how to
> > > interpret it.
> > > 
> > > Note that when booting from eMMC device via EXT_CSD[179] register is
> > > explicitly disabled then SPL still loads and boots from this eMMC device
> > > from User Area partition. This behavior was not changed in this commit and
> > > should be revisited in the future.
> > > 
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > ---
> > > Changes in v2:
> > > * Disable showing warning on sama5d2_xplained due to size restrictions
> > > ---
> > > This patch depends on another patch:
> > > mmc: spl: Add comments for default_spl_mmc_emmc_boot_partition()
> > > https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/
> > > ---
> > >  common/spl/Kconfig   |  7 +++++++
> > >  common/spl/spl_mmc.c | 46 ++++++++++++++++++++++++++++++++++++--------
> > >  2 files changed, 45 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> > > index 865571d4579c..0574d22b3b25 100644
> > > --- a/common/spl/Kconfig
> > > +++ b/common/spl/Kconfig
> > > @@ -855,6 +855,13 @@ config SPL_MMC_WRITE
> > >  	help
> > >  	  Enable write access to MMC and SD Cards in SPL
> > >  
> > > +config SPL_MMC_WARNINGS
> > > +	bool "Print MMC warnings"
> > > +	depends on SPL_MMC
> > > +	default y if !TARGET_SAMA5D2_XPLAINED
> > > +	help
> > > +	  Print SPL MMC warnings. You can disable this option to reduce SPL size.
> > > +
> > >  
> > >  config SPL_MPC8XXX_INIT_DDR
> > >  	bool "Support MPC8XXX DDR init"
> > > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> > > index f7a42a11477d..ec424ceded0e 100644
> > > --- a/common/spl/spl_mmc.c
> > > +++ b/common/spl/spl_mmc.c
> > > @@ -408,15 +408,45 @@ int default_spl_mmc_emmc_boot_partition(struct mmc *mmc)
> > >  	 *
> > >  	 * Note: See difference between EXT_CSD_EXTRACT_PARTITION_ACCESS
> > >  	 * and EXT_CSD_EXTRACT_BOOT_PART, specially about User area value.
> > > -	 *
> > > -	 * FIXME: When booting from this eMMC device is explicitly
> > > -	 * disabled then we use User area for booting. This is incorrect.
> > > -	 * Probably we should skip this eMMC device and select the next
> > > -	 * one for booting. Or at least throw warning about this fallback.
> > >  	 */
> > > -	part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
> > > -	if (part == 7)
> > > -		part = 0;
> > > +	if (mmc->part_config == MMCPART_NOAVAILABLE)
> > > +		part = 0; /* If partitions are not supported then we have only User Area partition */
> > > +	else {
> > > +		switch(EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) {
> > > +		case 0: /* Booting from this eMMC device is disabled */
> > > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > > +#ifdef CONFIG_SPL_MMC_WARNINGS
> > > +			puts("spl: WARNING: Booting from this eMMC device is disabled in EXT_CSD[179] register\n");
> > > +			puts("spl: WARNING: Continuing anyway and selecting User Area partition for booting\n");
> > > +#else
> > > +			puts("spl: mmc: fallback to user area\n");
> > > +#endif
> > > +#endif
> > > +			/* FIXME: This is incorrect and probably we should select next eMMC device for booting */
> > > +			part = 0;
> > > +			break;
> > > +		case 1: /* Boot partition 1 is used for booting */
> > > +			part = 1;
> > > +			break;
> > > +		case 2: /* Boot partition 2 is used for booting */
> > > +			part = 2;
> > > +			break;
> > > +		case 7: /* User area is used for booting */
> > > +			part = 0;
> > > +			break;
> > > +		default: /* Other values are reserved */
> > > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > > +#ifdef CONFIG_SPL_MMC_WARNINGS
> > > +			puts("spl: WARNING: EXT_CSD[179] register is configured to boot from Reserved value\n");
> > > +			puts("spl: WARNING: Selecting User Area partition for booting\n");
> > > +#else
> > > +			puts("spl: mmc: fallback to user area\n");
> > > +#endif
> > > +#endif
> > > +			part = 0;
> > > +			break;
> > > +		}
> > > +	}
> > >  #endif
> > 
> > Please just use debug() for these messages.
> 
> All other error/warning messages in this file are printed via puts().
> So I'm just following the current style (and I'm really not going to
> change all occurrences in this patch).

Except for the messages in that file which use debug() they use puts(),
yes. Since none of these are fatal messages (you're falling through)
please switch them to debug() rather than introduce a new CONFIG symbol,
so that if someone is bringing up a platform where this is a problem
they'll be able to debug it, but the general case does not increase
the binary size of most platforms. I'm not asking you to change anything
existing in the file, only what you're adding.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

RE: [PATCH u-boot 0/4] mmc: Explain and cleanup partition selection

[Jaehoon Chung]

Hi

> -----Original Message-----
> From: Pali Rohár <pali@kernel.org>
> Sent: Friday, July 7, 2023 2:39 AM
> To: Jaehoon Chung <jh80.chung@samsung.com>; Tom Rini <trini@konsulko.com>
> Cc: Peng Fan <peng.fan@nxp.com>; Stefan Roese <sr@denx.de>; u-boot@lists.denx.de
> Subject: Re: [PATCH u-boot 0/4] mmc: Explain and cleanup partition selection
> 
> On Thursday 06 July 2023 12:50:34 Pali Rohár wrote:
> > On Monday 03 July 2023 21:16:37 Jaehoon Chung wrote:
> > > Hi,
> > >
> > > On 4/14/23 06:10, Pali Rohár wrote:
> > > > Some people do not want to read review comments in emails. So put
> > > > comments and explanation into the source code itself; make emmc
> > > > partition selection code more explicit and validate configuration in
> > > > bubt command.
> > >
> > > Sorry for too late.
> > > After applied this patch-set, some board are failed with spl's size overflowed
> > >
> > > The below is one of failed log.
> > >
> > > +arm-linux-gnueabi-ld.bfd: region `.sram' overflowed by 352 bytes
> > > +make[2]: *** [scripts/Makefile.spl:527: spl/u-boot-spl] Error 1
> > > +make[1]: *** [Makefile:2049: spl/u-boot-spl] Error 2
> >
> > Crap, and again we have there same issue. Patch after months does not
> > apply anymore. And resending the patch would again result in the same
> > problem that it would not apply anymore after half of the year...
> 
> Here is v2 patch with simple fix for that platform:
> https://protect2.fireeye.com/v1/url?k=bec9bc29-df42a906-bec83766-74fe485cbfe7-
> 2e40f47b53afb09d&q=1&e=e8f6a6f2-8f6f-4bf1-9b91-
> 3f71d3577fc7&u=https%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Fuboot%2Fpatch%2F20230706173502.2796-1-
> pali%40kernel.org%2F
> 
> And it passed _now_ CI pipeline:
> https://protect2.fireeye.com/v1/url?k=543f3751-35b4227e-543ebc1e-74fe485cbfe7-
> ff5f636a3c274f12&q=1&e=e8f6a6f2-8f6f-4bf1-9b91-3f71d3577fc7&u=https%3A%2F%2Fgithub.com%2Fu-boot%2Fu-
> boot%2Fpull%2F340
> 

Thanks for sharing it. 

Best Regards,
Jaehoon Chung

> >
> > Anyway, only first patch is SPL specific, other should apply without any
> > problem.
> >
> > >
> > > Best Regards,
> > > Jaehoon Chung
> > >
> > > >
> > > > Pali Rohár (4):
> > > >   mmc: spl: Make partition choice in
> > > >     default_spl_mmc_emmc_boot_partition() more explicit
> > > >   cmd: mvebu/bubt: Validate EXT_CSD[179] eMMC register in
> > > >     mmc_burn_image()
> > > >   sunxi: eMMC: Add comments explaining mapping between bootpart and
> > > >     mmc_switch_part()
> > > >   board: purism: Use U-Boot mmc function for converting boot part to
> > > >     part access
> > > >
> > > >  arch/arm/mach-sunxi/board.c    | 12 ++++++++++-
> > > >  board/purism/librem5/librem5.c |  6 +-----
> > > >  cmd/mvebu/bubt.c               | 24 +++++++++++++++++++--
> > > >  common/spl/spl_mmc.c           | 38 +++++++++++++++++++++++++++-------
> > > >  4 files changed, 64 insertions(+), 16 deletions(-)
> > >

RE: [PATCH u-boot 0/4] mmc: Explain and cleanup partition selection

[Jaehoon Chung]

> -----Original Message-----
> From: Pali Rohár <pali@kernel.org>
> Sent: Thursday, July 6, 2023 7:51 PM
> To: Jaehoon Chung <jh80.chung@samsung.com>; Tom Rini <trini@konsulko.com>
> Cc: Peng Fan <peng.fan@nxp.com>; Stefan Roese <sr@denx.de>; u-boot@lists.denx.de
> Subject: Re: [PATCH u-boot 0/4] mmc: Explain and cleanup partition selection
> 
> On Monday 03 July 2023 21:16:37 Jaehoon Chung wrote:
> > Hi,
> >
> > On 4/14/23 06:10, Pali Rohár wrote:
> > > Some people do not want to read review comments in emails. So put
> > > comments and explanation into the source code itself; make emmc
> > > partition selection code more explicit and validate configuration in
> > > bubt command.
> >
> > Sorry for too late.
> > After applied this patch-set, some board are failed with spl's size overflowed
> >
> > The below is one of failed log.
> >
> > +arm-linux-gnueabi-ld.bfd: region `.sram' overflowed by 352 bytes
> > +make[2]: *** [scripts/Makefile.spl:527: spl/u-boot-spl] Error 1
> > +make[1]: *** [Makefile:2049: spl/u-boot-spl] Error 2
> 
> Crap, and again we have there same issue. Patch after months does not
> apply anymore. And resending the patch would again result in the same
> problem that it would not apply anymore after half of the year...
> 
> 
> Anyway, only first patch is SPL specific, other should apply without any
> problem.

Except spl specific patch, I will check again about your other patches. Thanks!

Best Regards,
Jaehoon Chung

> 
> >
> > Best Regards,
> > Jaehoon Chung
> >
> > >
> > > Pali Rohár (4):
> > >   mmc: spl: Make partition choice in
> > >     default_spl_mmc_emmc_boot_partition() more explicit
> > >   cmd: mvebu/bubt: Validate EXT_CSD[179] eMMC register in
> > >     mmc_burn_image()
> > >   sunxi: eMMC: Add comments explaining mapping between bootpart and
> > >     mmc_switch_part()
> > >   board: purism: Use U-Boot mmc function for converting boot part to
> > >     part access
> > >
> > >  arch/arm/mach-sunxi/board.c    | 12 ++++++++++-
> > >  board/purism/librem5/librem5.c |  6 +-----
> > >  cmd/mvebu/bubt.c               | 24 +++++++++++++++++++--
> > >  common/spl/spl_mmc.c           | 38 +++++++++++++++++++++++++++-------
> > >  4 files changed, 64 insertions(+), 16 deletions(-)
> >

Re: [PATCH v2 u-boot] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit

[Pali Rohár]

On Thursday 06 July 2023 13:52:14 Tom Rini wrote:
> On Thu, Jul 06, 2023 at 07:49:18PM +0200, Pali Rohár wrote:
> > On Thursday 06 July 2023 13:42:18 Tom Rini wrote:
> > > On Thu, Jul 06, 2023 at 07:35:02PM +0200, Pali Rohár wrote:
> > > > To make eMMC partition choosing in default_spl_mmc_emmc_boot_partition()
> > > > function better understandable, rewrite it via explicit switch-case code
> > > > pattern.
> > > > 
> > > > Also add a warning when eMMC EXT_CSD[179] register is configured by user to
> > > > value which is not suitable for eMMC booting and SPL do not know how to
> > > > interpret it.
> > > > 
> > > > Note that when booting from eMMC device via EXT_CSD[179] register is
> > > > explicitly disabled then SPL still loads and boots from this eMMC device
> > > > from User Area partition. This behavior was not changed in this commit and
> > > > should be revisited in the future.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > ---
> > > > Changes in v2:
> > > > * Disable showing warning on sama5d2_xplained due to size restrictions
> > > > ---
> > > > This patch depends on another patch:
> > > > mmc: spl: Add comments for default_spl_mmc_emmc_boot_partition()
> > > > https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/
> > > > ---
> > > >  common/spl/Kconfig   |  7 +++++++
> > > >  common/spl/spl_mmc.c | 46 ++++++++++++++++++++++++++++++++++++--------
> > > >  2 files changed, 45 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> > > > index 865571d4579c..0574d22b3b25 100644
> > > > --- a/common/spl/Kconfig
> > > > +++ b/common/spl/Kconfig
> > > > @@ -855,6 +855,13 @@ config SPL_MMC_WRITE
> > > >  	help
> > > >  	  Enable write access to MMC and SD Cards in SPL
> > > >  
> > > > +config SPL_MMC_WARNINGS
> > > > +	bool "Print MMC warnings"
> > > > +	depends on SPL_MMC
> > > > +	default y if !TARGET_SAMA5D2_XPLAINED
> > > > +	help
> > > > +	  Print SPL MMC warnings. You can disable this option to reduce SPL size.
> > > > +
> > > >  
> > > >  config SPL_MPC8XXX_INIT_DDR
> > > >  	bool "Support MPC8XXX DDR init"
> > > > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> > > > index f7a42a11477d..ec424ceded0e 100644
> > > > --- a/common/spl/spl_mmc.c
> > > > +++ b/common/spl/spl_mmc.c
> > > > @@ -408,15 +408,45 @@ int default_spl_mmc_emmc_boot_partition(struct mmc *mmc)
> > > >  	 *
> > > >  	 * Note: See difference between EXT_CSD_EXTRACT_PARTITION_ACCESS
> > > >  	 * and EXT_CSD_EXTRACT_BOOT_PART, specially about User area value.
> > > > -	 *
> > > > -	 * FIXME: When booting from this eMMC device is explicitly
> > > > -	 * disabled then we use User area for booting. This is incorrect.
> > > > -	 * Probably we should skip this eMMC device and select the next
> > > > -	 * one for booting. Or at least throw warning about this fallback.
> > > >  	 */
> > > > -	part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
> > > > -	if (part == 7)
> > > > -		part = 0;
> > > > +	if (mmc->part_config == MMCPART_NOAVAILABLE)
> > > > +		part = 0; /* If partitions are not supported then we have only User Area partition */
> > > > +	else {
> > > > +		switch(EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) {
> > > > +		case 0: /* Booting from this eMMC device is disabled */
> > > > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > > > +#ifdef CONFIG_SPL_MMC_WARNINGS
> > > > +			puts("spl: WARNING: Booting from this eMMC device is disabled in EXT_CSD[179] register\n");
> > > > +			puts("spl: WARNING: Continuing anyway and selecting User Area partition for booting\n");
> > > > +#else
> > > > +			puts("spl: mmc: fallback to user area\n");
> > > > +#endif
> > > > +#endif
> > > > +			/* FIXME: This is incorrect and probably we should select next eMMC device for booting */
> > > > +			part = 0;
> > > > +			break;
> > > > +		case 1: /* Boot partition 1 is used for booting */
> > > > +			part = 1;
> > > > +			break;
> > > > +		case 2: /* Boot partition 2 is used for booting */
> > > > +			part = 2;
> > > > +			break;
> > > > +		case 7: /* User area is used for booting */
> > > > +			part = 0;
> > > > +			break;
> > > > +		default: /* Other values are reserved */
> > > > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > > > +#ifdef CONFIG_SPL_MMC_WARNINGS
> > > > +			puts("spl: WARNING: EXT_CSD[179] register is configured to boot from Reserved value\n");
> > > > +			puts("spl: WARNING: Selecting User Area partition for booting\n");
> > > > +#else
> > > > +			puts("spl: mmc: fallback to user area\n");
> > > > +#endif
> > > > +#endif
> > > > +			part = 0;
> > > > +			break;
> > > > +		}
> > > > +	}
> > > >  #endif
> > > 
> > > Please just use debug() for these messages.
> > 
> > All other error/warning messages in this file are printed via puts().
> > So I'm just following the current style (and I'm really not going to
> > change all occurrences in this patch).
> 
> Except for the messages in that file which use debug() they use puts(),
> yes. Since none of these are fatal messages (you're falling through)
> please switch them to debug() rather than introduce a new CONFIG symbol,
> so that if someone is bringing up a platform where this is a problem
> they'll be able to debug it, but the general case does not increase
> the binary size of most platforms. I'm not asking you to change anything
> existing in the file, only what you're adding.
> 
> -- 
> Tom

We should at those two places fail. But I do not want to break existing
improperly configured setups, so warning a good way to show people that
they have something misconfigured. Later in future we switch warnings to
fatal errors. But if we do not show anything at these points, nobody
would figure out that has improper setup configuration. debug() is IIRC
not shown by default.

Re: [PATCH v2 u-boot] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 6765 bytes --]

On Fri, Jul 07, 2023 at 06:46:39PM +0200, Pali Rohár wrote:
> On Thursday 06 July 2023 13:52:14 Tom Rini wrote:
> > On Thu, Jul 06, 2023 at 07:49:18PM +0200, Pali Rohár wrote:
> > > On Thursday 06 July 2023 13:42:18 Tom Rini wrote:
> > > > On Thu, Jul 06, 2023 at 07:35:02PM +0200, Pali Rohár wrote:
> > > > > To make eMMC partition choosing in default_spl_mmc_emmc_boot_partition()
> > > > > function better understandable, rewrite it via explicit switch-case code
> > > > > pattern.
> > > > > 
> > > > > Also add a warning when eMMC EXT_CSD[179] register is configured by user to
> > > > > value which is not suitable for eMMC booting and SPL do not know how to
> > > > > interpret it.
> > > > > 
> > > > > Note that when booting from eMMC device via EXT_CSD[179] register is
> > > > > explicitly disabled then SPL still loads and boots from this eMMC device
> > > > > from User Area partition. This behavior was not changed in this commit and
> > > > > should be revisited in the future.
> > > > > 
> > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > ---
> > > > > Changes in v2:
> > > > > * Disable showing warning on sama5d2_xplained due to size restrictions
> > > > > ---
> > > > > This patch depends on another patch:
> > > > > mmc: spl: Add comments for default_spl_mmc_emmc_boot_partition()
> > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/
> > > > > ---
> > > > >  common/spl/Kconfig   |  7 +++++++
> > > > >  common/spl/spl_mmc.c | 46 ++++++++++++++++++++++++++++++++++++--------
> > > > >  2 files changed, 45 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> > > > > index 865571d4579c..0574d22b3b25 100644
> > > > > --- a/common/spl/Kconfig
> > > > > +++ b/common/spl/Kconfig
> > > > > @@ -855,6 +855,13 @@ config SPL_MMC_WRITE
> > > > >  	help
> > > > >  	  Enable write access to MMC and SD Cards in SPL
> > > > >  
> > > > > +config SPL_MMC_WARNINGS
> > > > > +	bool "Print MMC warnings"
> > > > > +	depends on SPL_MMC
> > > > > +	default y if !TARGET_SAMA5D2_XPLAINED
> > > > > +	help
> > > > > +	  Print SPL MMC warnings. You can disable this option to reduce SPL size.
> > > > > +
> > > > >  
> > > > >  config SPL_MPC8XXX_INIT_DDR
> > > > >  	bool "Support MPC8XXX DDR init"
> > > > > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> > > > > index f7a42a11477d..ec424ceded0e 100644
> > > > > --- a/common/spl/spl_mmc.c
> > > > > +++ b/common/spl/spl_mmc.c
> > > > > @@ -408,15 +408,45 @@ int default_spl_mmc_emmc_boot_partition(struct mmc *mmc)
> > > > >  	 *
> > > > >  	 * Note: See difference between EXT_CSD_EXTRACT_PARTITION_ACCESS
> > > > >  	 * and EXT_CSD_EXTRACT_BOOT_PART, specially about User area value.
> > > > > -	 *
> > > > > -	 * FIXME: When booting from this eMMC device is explicitly
> > > > > -	 * disabled then we use User area for booting. This is incorrect.
> > > > > -	 * Probably we should skip this eMMC device and select the next
> > > > > -	 * one for booting. Or at least throw warning about this fallback.
> > > > >  	 */
> > > > > -	part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
> > > > > -	if (part == 7)
> > > > > -		part = 0;
> > > > > +	if (mmc->part_config == MMCPART_NOAVAILABLE)
> > > > > +		part = 0; /* If partitions are not supported then we have only User Area partition */
> > > > > +	else {
> > > > > +		switch(EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) {
> > > > > +		case 0: /* Booting from this eMMC device is disabled */
> > > > > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > > > > +#ifdef CONFIG_SPL_MMC_WARNINGS
> > > > > +			puts("spl: WARNING: Booting from this eMMC device is disabled in EXT_CSD[179] register\n");
> > > > > +			puts("spl: WARNING: Continuing anyway and selecting User Area partition for booting\n");
> > > > > +#else
> > > > > +			puts("spl: mmc: fallback to user area\n");
> > > > > +#endif
> > > > > +#endif
> > > > > +			/* FIXME: This is incorrect and probably we should select next eMMC device for booting */
> > > > > +			part = 0;
> > > > > +			break;
> > > > > +		case 1: /* Boot partition 1 is used for booting */
> > > > > +			part = 1;
> > > > > +			break;
> > > > > +		case 2: /* Boot partition 2 is used for booting */
> > > > > +			part = 2;
> > > > > +			break;
> > > > > +		case 7: /* User area is used for booting */
> > > > > +			part = 0;
> > > > > +			break;
> > > > > +		default: /* Other values are reserved */
> > > > > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > > > > +#ifdef CONFIG_SPL_MMC_WARNINGS
> > > > > +			puts("spl: WARNING: EXT_CSD[179] register is configured to boot from Reserved value\n");
> > > > > +			puts("spl: WARNING: Selecting User Area partition for booting\n");
> > > > > +#else
> > > > > +			puts("spl: mmc: fallback to user area\n");
> > > > > +#endif
> > > > > +#endif
> > > > > +			part = 0;
> > > > > +			break;
> > > > > +		}
> > > > > +	}
> > > > >  #endif
> > > > 
> > > > Please just use debug() for these messages.
> > > 
> > > All other error/warning messages in this file are printed via puts().
> > > So I'm just following the current style (and I'm really not going to
> > > change all occurrences in this patch).
> > 
> > Except for the messages in that file which use debug() they use puts(),
> > yes. Since none of these are fatal messages (you're falling through)
> > please switch them to debug() rather than introduce a new CONFIG symbol,
> > so that if someone is bringing up a platform where this is a problem
> > they'll be able to debug it, but the general case does not increase
> > the binary size of most platforms. I'm not asking you to change anything
> > existing in the file, only what you're adding.
> 
> We should at those two places fail. But I do not want to break existing
> improperly configured setups, so warning a good way to show people that
> they have something misconfigured. Later in future we switch warnings to
> fatal errors. But if we do not show anything at these points, nobody
> would figure out that has improper setup configuration. debug() is IIRC
> not shown by default.

Ah, so the plan is they should be fatal, and there's a way to fix the
configuration? Lets just panic() them now, instead. I really really do
not want to grow every SPL+MMC using board (which is a lot of them) by
several hundred bytes of strings.  Lets catch the mis-configuration,
merge it in early in the cycle (right now for v2023.10 for example,
especially since the MMC custodian is promising to review things and get
a PR out ASAP), and see what falls out.  A panic() and a big comment
explaining things should suffice.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 u-boot] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit

[Pali Rohár]

On Friday 07 July 2023 12:54:58 Tom Rini wrote:
> On Fri, Jul 07, 2023 at 06:46:39PM +0200, Pali Rohár wrote:
> > On Thursday 06 July 2023 13:52:14 Tom Rini wrote:
> > > On Thu, Jul 06, 2023 at 07:49:18PM +0200, Pali Rohár wrote:
> > > > On Thursday 06 July 2023 13:42:18 Tom Rini wrote:
> > > > > On Thu, Jul 06, 2023 at 07:35:02PM +0200, Pali Rohár wrote:
> > > > > > To make eMMC partition choosing in default_spl_mmc_emmc_boot_partition()
> > > > > > function better understandable, rewrite it via explicit switch-case code
> > > > > > pattern.
> > > > > > 
> > > > > > Also add a warning when eMMC EXT_CSD[179] register is configured by user to
> > > > > > value which is not suitable for eMMC booting and SPL do not know how to
> > > > > > interpret it.
> > > > > > 
> > > > > > Note that when booting from eMMC device via EXT_CSD[179] register is
> > > > > > explicitly disabled then SPL still loads and boots from this eMMC device
> > > > > > from User Area partition. This behavior was not changed in this commit and
> > > > > > should be revisited in the future.
> > > > > > 
> > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > > ---
> > > > > > Changes in v2:
> > > > > > * Disable showing warning on sama5d2_xplained due to size restrictions
> > > > > > ---
> > > > > > This patch depends on another patch:
> > > > > > mmc: spl: Add comments for default_spl_mmc_emmc_boot_partition()
> > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/
> > > > > > ---
> > > > > >  common/spl/Kconfig   |  7 +++++++
> > > > > >  common/spl/spl_mmc.c | 46 ++++++++++++++++++++++++++++++++++++--------
> > > > > >  2 files changed, 45 insertions(+), 8 deletions(-)
> > > > > > 
> > > > > > diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> > > > > > index 865571d4579c..0574d22b3b25 100644
> > > > > > --- a/common/spl/Kconfig
> > > > > > +++ b/common/spl/Kconfig
> > > > > > @@ -855,6 +855,13 @@ config SPL_MMC_WRITE
> > > > > >  	help
> > > > > >  	  Enable write access to MMC and SD Cards in SPL
> > > > > >  
> > > > > > +config SPL_MMC_WARNINGS
> > > > > > +	bool "Print MMC warnings"
> > > > > > +	depends on SPL_MMC
> > > > > > +	default y if !TARGET_SAMA5D2_XPLAINED
> > > > > > +	help
> > > > > > +	  Print SPL MMC warnings. You can disable this option to reduce SPL size.
> > > > > > +
> > > > > >  
> > > > > >  config SPL_MPC8XXX_INIT_DDR
> > > > > >  	bool "Support MPC8XXX DDR init"
> > > > > > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> > > > > > index f7a42a11477d..ec424ceded0e 100644
> > > > > > --- a/common/spl/spl_mmc.c
> > > > > > +++ b/common/spl/spl_mmc.c
> > > > > > @@ -408,15 +408,45 @@ int default_spl_mmc_emmc_boot_partition(struct mmc *mmc)
> > > > > >  	 *
> > > > > >  	 * Note: See difference between EXT_CSD_EXTRACT_PARTITION_ACCESS
> > > > > >  	 * and EXT_CSD_EXTRACT_BOOT_PART, specially about User area value.
> > > > > > -	 *
> > > > > > -	 * FIXME: When booting from this eMMC device is explicitly
> > > > > > -	 * disabled then we use User area for booting. This is incorrect.
> > > > > > -	 * Probably we should skip this eMMC device and select the next
> > > > > > -	 * one for booting. Or at least throw warning about this fallback.
> > > > > >  	 */
> > > > > > -	part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
> > > > > > -	if (part == 7)
> > > > > > -		part = 0;
> > > > > > +	if (mmc->part_config == MMCPART_NOAVAILABLE)
> > > > > > +		part = 0; /* If partitions are not supported then we have only User Area partition */
> > > > > > +	else {
> > > > > > +		switch(EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) {
> > > > > > +		case 0: /* Booting from this eMMC device is disabled */
> > > > > > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > > > > > +#ifdef CONFIG_SPL_MMC_WARNINGS
> > > > > > +			puts("spl: WARNING: Booting from this eMMC device is disabled in EXT_CSD[179] register\n");
> > > > > > +			puts("spl: WARNING: Continuing anyway and selecting User Area partition for booting\n");
> > > > > > +#else
> > > > > > +			puts("spl: mmc: fallback to user area\n");
> > > > > > +#endif
> > > > > > +#endif
> > > > > > +			/* FIXME: This is incorrect and probably we should select next eMMC device for booting */
> > > > > > +			part = 0;
> > > > > > +			break;
> > > > > > +		case 1: /* Boot partition 1 is used for booting */
> > > > > > +			part = 1;
> > > > > > +			break;
> > > > > > +		case 2: /* Boot partition 2 is used for booting */
> > > > > > +			part = 2;
> > > > > > +			break;
> > > > > > +		case 7: /* User area is used for booting */
> > > > > > +			part = 0;
> > > > > > +			break;
> > > > > > +		default: /* Other values are reserved */
> > > > > > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > > > > > +#ifdef CONFIG_SPL_MMC_WARNINGS
> > > > > > +			puts("spl: WARNING: EXT_CSD[179] register is configured to boot from Reserved value\n");
> > > > > > +			puts("spl: WARNING: Selecting User Area partition for booting\n");
> > > > > > +#else
> > > > > > +			puts("spl: mmc: fallback to user area\n");
> > > > > > +#endif
> > > > > > +#endif
> > > > > > +			part = 0;
> > > > > > +			break;
> > > > > > +		}
> > > > > > +	}
> > > > > >  #endif
> > > > > 
> > > > > Please just use debug() for these messages.
> > > > 
> > > > All other error/warning messages in this file are printed via puts().
> > > > So I'm just following the current style (and I'm really not going to
> > > > change all occurrences in this patch).
> > > 
> > > Except for the messages in that file which use debug() they use puts(),
> > > yes. Since none of these are fatal messages (you're falling through)
> > > please switch them to debug() rather than introduce a new CONFIG symbol,
> > > so that if someone is bringing up a platform where this is a problem
> > > they'll be able to debug it, but the general case does not increase
> > > the binary size of most platforms. I'm not asking you to change anything
> > > existing in the file, only what you're adding.
> > 
> > We should at those two places fail. But I do not want to break existing
> > improperly configured setups, so warning a good way to show people that
> > they have something misconfigured. Later in future we switch warnings to
> > fatal errors. But if we do not show anything at these points, nobody
> > would figure out that has improper setup configuration. debug() is IIRC
> > not shown by default.
> 
> Ah, so the plan is they should be fatal, and there's a way to fix the
> configuration?

Yes, EXT_CSD[179] is configurable register, you can change boot
partition bits (those are non-volatile) via your favorite emmc config
tool. U-Boot has also tool "mmc partconf" which can do that. But you
first need to be able enter into u-boot console and also you need to
enable this tool your board config file.

> Lets just panic() them now, instead. I really really do
> not want to grow every SPL+MMC using board (which is a lot of them) by
> several hundred bytes of strings.  Lets catch the mis-configuration,
> merge it in early in the cycle (right now for v2023.10 for example,
> especially since the MMC custodian is promising to review things and get
> a PR out ASAP), and see what falls out.  A panic() and a big comment
> explaining things should suffice.
> 
> -- 
> Tom

Ok, I can change "reserved values" to panic().

About "booting is disabled" - I do not think that panic is correct here.
See inline comments in code. Rather do the correct thing - skip emmc and
let SPL to boot from next source. But some message needs to be printed
here why emmc was skipped....

Re: [PATCH v2 u-boot] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 8400 bytes --]

On Fri, Jul 07, 2023 at 07:05:45PM +0200, Pali Rohár wrote:
> On Friday 07 July 2023 12:54:58 Tom Rini wrote:
> > On Fri, Jul 07, 2023 at 06:46:39PM +0200, Pali Rohár wrote:
> > > On Thursday 06 July 2023 13:52:14 Tom Rini wrote:
> > > > On Thu, Jul 06, 2023 at 07:49:18PM +0200, Pali Rohár wrote:
> > > > > On Thursday 06 July 2023 13:42:18 Tom Rini wrote:
> > > > > > On Thu, Jul 06, 2023 at 07:35:02PM +0200, Pali Rohár wrote:
> > > > > > > To make eMMC partition choosing in default_spl_mmc_emmc_boot_partition()
> > > > > > > function better understandable, rewrite it via explicit switch-case code
> > > > > > > pattern.
> > > > > > > 
> > > > > > > Also add a warning when eMMC EXT_CSD[179] register is configured by user to
> > > > > > > value which is not suitable for eMMC booting and SPL do not know how to
> > > > > > > interpret it.
> > > > > > > 
> > > > > > > Note that when booting from eMMC device via EXT_CSD[179] register is
> > > > > > > explicitly disabled then SPL still loads and boots from this eMMC device
> > > > > > > from User Area partition. This behavior was not changed in this commit and
> > > > > > > should be revisited in the future.
> > > > > > > 
> > > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > > > ---
> > > > > > > Changes in v2:
> > > > > > > * Disable showing warning on sama5d2_xplained due to size restrictions
> > > > > > > ---
> > > > > > > This patch depends on another patch:
> > > > > > > mmc: spl: Add comments for default_spl_mmc_emmc_boot_partition()
> > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/
> > > > > > > ---
> > > > > > >  common/spl/Kconfig   |  7 +++++++
> > > > > > >  common/spl/spl_mmc.c | 46 ++++++++++++++++++++++++++++++++++++--------
> > > > > > >  2 files changed, 45 insertions(+), 8 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> > > > > > > index 865571d4579c..0574d22b3b25 100644
> > > > > > > --- a/common/spl/Kconfig
> > > > > > > +++ b/common/spl/Kconfig
> > > > > > > @@ -855,6 +855,13 @@ config SPL_MMC_WRITE
> > > > > > >  	help
> > > > > > >  	  Enable write access to MMC and SD Cards in SPL
> > > > > > >  
> > > > > > > +config SPL_MMC_WARNINGS
> > > > > > > +	bool "Print MMC warnings"
> > > > > > > +	depends on SPL_MMC
> > > > > > > +	default y if !TARGET_SAMA5D2_XPLAINED
> > > > > > > +	help
> > > > > > > +	  Print SPL MMC warnings. You can disable this option to reduce SPL size.
> > > > > > > +
> > > > > > >  
> > > > > > >  config SPL_MPC8XXX_INIT_DDR
> > > > > > >  	bool "Support MPC8XXX DDR init"
> > > > > > > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> > > > > > > index f7a42a11477d..ec424ceded0e 100644
> > > > > > > --- a/common/spl/spl_mmc.c
> > > > > > > +++ b/common/spl/spl_mmc.c
> > > > > > > @@ -408,15 +408,45 @@ int default_spl_mmc_emmc_boot_partition(struct mmc *mmc)
> > > > > > >  	 *
> > > > > > >  	 * Note: See difference between EXT_CSD_EXTRACT_PARTITION_ACCESS
> > > > > > >  	 * and EXT_CSD_EXTRACT_BOOT_PART, specially about User area value.
> > > > > > > -	 *
> > > > > > > -	 * FIXME: When booting from this eMMC device is explicitly
> > > > > > > -	 * disabled then we use User area for booting. This is incorrect.
> > > > > > > -	 * Probably we should skip this eMMC device and select the next
> > > > > > > -	 * one for booting. Or at least throw warning about this fallback.
> > > > > > >  	 */
> > > > > > > -	part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
> > > > > > > -	if (part == 7)
> > > > > > > -		part = 0;
> > > > > > > +	if (mmc->part_config == MMCPART_NOAVAILABLE)
> > > > > > > +		part = 0; /* If partitions are not supported then we have only User Area partition */
> > > > > > > +	else {
> > > > > > > +		switch(EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) {
> > > > > > > +		case 0: /* Booting from this eMMC device is disabled */
> > > > > > > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > > > > > > +#ifdef CONFIG_SPL_MMC_WARNINGS
> > > > > > > +			puts("spl: WARNING: Booting from this eMMC device is disabled in EXT_CSD[179] register\n");
> > > > > > > +			puts("spl: WARNING: Continuing anyway and selecting User Area partition for booting\n");
> > > > > > > +#else
> > > > > > > +			puts("spl: mmc: fallback to user area\n");
> > > > > > > +#endif
> > > > > > > +#endif
> > > > > > > +			/* FIXME: This is incorrect and probably we should select next eMMC device for booting */
> > > > > > > +			part = 0;
> > > > > > > +			break;
> > > > > > > +		case 1: /* Boot partition 1 is used for booting */
> > > > > > > +			part = 1;
> > > > > > > +			break;
> > > > > > > +		case 2: /* Boot partition 2 is used for booting */
> > > > > > > +			part = 2;
> > > > > > > +			break;
> > > > > > > +		case 7: /* User area is used for booting */
> > > > > > > +			part = 0;
> > > > > > > +			break;
> > > > > > > +		default: /* Other values are reserved */
> > > > > > > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > > > > > > +#ifdef CONFIG_SPL_MMC_WARNINGS
> > > > > > > +			puts("spl: WARNING: EXT_CSD[179] register is configured to boot from Reserved value\n");
> > > > > > > +			puts("spl: WARNING: Selecting User Area partition for booting\n");
> > > > > > > +#else
> > > > > > > +			puts("spl: mmc: fallback to user area\n");
> > > > > > > +#endif
> > > > > > > +#endif
> > > > > > > +			part = 0;
> > > > > > > +			break;
> > > > > > > +		}
> > > > > > > +	}
> > > > > > >  #endif
> > > > > > 
> > > > > > Please just use debug() for these messages.
> > > > > 
> > > > > All other error/warning messages in this file are printed via puts().
> > > > > So I'm just following the current style (and I'm really not going to
> > > > > change all occurrences in this patch).
> > > > 
> > > > Except for the messages in that file which use debug() they use puts(),
> > > > yes. Since none of these are fatal messages (you're falling through)
> > > > please switch them to debug() rather than introduce a new CONFIG symbol,
> > > > so that if someone is bringing up a platform where this is a problem
> > > > they'll be able to debug it, but the general case does not increase
> > > > the binary size of most platforms. I'm not asking you to change anything
> > > > existing in the file, only what you're adding.
> > > 
> > > We should at those two places fail. But I do not want to break existing
> > > improperly configured setups, so warning a good way to show people that
> > > they have something misconfigured. Later in future we switch warnings to
> > > fatal errors. But if we do not show anything at these points, nobody
> > > would figure out that has improper setup configuration. debug() is IIRC
> > > not shown by default.
> > 
> > Ah, so the plan is they should be fatal, and there's a way to fix the
> > configuration?
> 
> Yes, EXT_CSD[179] is configurable register, you can change boot
> partition bits (those are non-volatile) via your favorite emmc config
> tool. U-Boot has also tool "mmc partconf" which can do that. But you
> first need to be able enter into u-boot console and also you need to
> enable this tool your board config file.
> 
> > Lets just panic() them now, instead. I really really do
> > not want to grow every SPL+MMC using board (which is a lot of them) by
> > several hundred bytes of strings.  Lets catch the mis-configuration,
> > merge it in early in the cycle (right now for v2023.10 for example,
> > especially since the MMC custodian is promising to review things and get
> > a PR out ASAP), and see what falls out.  A panic() and a big comment
> > explaining things should suffice.
> 
> Ok, I can change "reserved values" to panic().
> 
> About "booting is disabled" - I do not think that panic is correct here.
> See inline comments in code. Rather do the correct thing - skip emmc and
> let SPL to boot from next source. But some message needs to be printed
> here why emmc was skipped....

How verbose all of this needs to be depends on who is likely to
encounter this type of problem. Are we talking about systems in the wild
today that are misconfigured, or are we talking about problems that will
arise as part of bringing up a new design and if you don't do X
correctly then Y will happen oddly/wrongly?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 u-boot] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit

[Pali Rohár]

On Friday 07 July 2023 13:10:19 Tom Rini wrote:
> On Fri, Jul 07, 2023 at 07:05:45PM +0200, Pali Rohár wrote:
> > On Friday 07 July 2023 12:54:58 Tom Rini wrote:
> > > On Fri, Jul 07, 2023 at 06:46:39PM +0200, Pali Rohár wrote:
> > > > On Thursday 06 July 2023 13:52:14 Tom Rini wrote:
> > > > > On Thu, Jul 06, 2023 at 07:49:18PM +0200, Pali Rohár wrote:
> > > > > > On Thursday 06 July 2023 13:42:18 Tom Rini wrote:
> > > > > > > On Thu, Jul 06, 2023 at 07:35:02PM +0200, Pali Rohár wrote:
> > > > > > > > To make eMMC partition choosing in default_spl_mmc_emmc_boot_partition()
> > > > > > > > function better understandable, rewrite it via explicit switch-case code
> > > > > > > > pattern.
> > > > > > > > 
> > > > > > > > Also add a warning when eMMC EXT_CSD[179] register is configured by user to
> > > > > > > > value which is not suitable for eMMC booting and SPL do not know how to
> > > > > > > > interpret it.
> > > > > > > > 
> > > > > > > > Note that when booting from eMMC device via EXT_CSD[179] register is
> > > > > > > > explicitly disabled then SPL still loads and boots from this eMMC device
> > > > > > > > from User Area partition. This behavior was not changed in this commit and
> > > > > > > > should be revisited in the future.
> > > > > > > > 
> > > > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > > > > ---
> > > > > > > > Changes in v2:
> > > > > > > > * Disable showing warning on sama5d2_xplained due to size restrictions
> > > > > > > > ---
> > > > > > > > This patch depends on another patch:
> > > > > > > > mmc: spl: Add comments for default_spl_mmc_emmc_boot_partition()
> > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/
> > > > > > > > ---
> > > > > > > >  common/spl/Kconfig   |  7 +++++++
> > > > > > > >  common/spl/spl_mmc.c | 46 ++++++++++++++++++++++++++++++++++++--------
> > > > > > > >  2 files changed, 45 insertions(+), 8 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> > > > > > > > index 865571d4579c..0574d22b3b25 100644
> > > > > > > > --- a/common/spl/Kconfig
> > > > > > > > +++ b/common/spl/Kconfig
> > > > > > > > @@ -855,6 +855,13 @@ config SPL_MMC_WRITE
> > > > > > > >  	help
> > > > > > > >  	  Enable write access to MMC and SD Cards in SPL
> > > > > > > >  
> > > > > > > > +config SPL_MMC_WARNINGS
> > > > > > > > +	bool "Print MMC warnings"
> > > > > > > > +	depends on SPL_MMC
> > > > > > > > +	default y if !TARGET_SAMA5D2_XPLAINED
> > > > > > > > +	help
> > > > > > > > +	  Print SPL MMC warnings. You can disable this option to reduce SPL size.
> > > > > > > > +
> > > > > > > >  
> > > > > > > >  config SPL_MPC8XXX_INIT_DDR
> > > > > > > >  	bool "Support MPC8XXX DDR init"
> > > > > > > > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> > > > > > > > index f7a42a11477d..ec424ceded0e 100644
> > > > > > > > --- a/common/spl/spl_mmc.c
> > > > > > > > +++ b/common/spl/spl_mmc.c
> > > > > > > > @@ -408,15 +408,45 @@ int default_spl_mmc_emmc_boot_partition(struct mmc *mmc)
> > > > > > > >  	 *
> > > > > > > >  	 * Note: See difference between EXT_CSD_EXTRACT_PARTITION_ACCESS
> > > > > > > >  	 * and EXT_CSD_EXTRACT_BOOT_PART, specially about User area value.
> > > > > > > > -	 *
> > > > > > > > -	 * FIXME: When booting from this eMMC device is explicitly
> > > > > > > > -	 * disabled then we use User area for booting. This is incorrect.
> > > > > > > > -	 * Probably we should skip this eMMC device and select the next
> > > > > > > > -	 * one for booting. Or at least throw warning about this fallback.
> > > > > > > >  	 */
> > > > > > > > -	part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
> > > > > > > > -	if (part == 7)
> > > > > > > > -		part = 0;
> > > > > > > > +	if (mmc->part_config == MMCPART_NOAVAILABLE)
> > > > > > > > +		part = 0; /* If partitions are not supported then we have only User Area partition */
> > > > > > > > +	else {
> > > > > > > > +		switch(EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) {
> > > > > > > > +		case 0: /* Booting from this eMMC device is disabled */
> > > > > > > > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > > > > > > > +#ifdef CONFIG_SPL_MMC_WARNINGS
> > > > > > > > +			puts("spl: WARNING: Booting from this eMMC device is disabled in EXT_CSD[179] register\n");
> > > > > > > > +			puts("spl: WARNING: Continuing anyway and selecting User Area partition for booting\n");
> > > > > > > > +#else
> > > > > > > > +			puts("spl: mmc: fallback to user area\n");
> > > > > > > > +#endif
> > > > > > > > +#endif
> > > > > > > > +			/* FIXME: This is incorrect and probably we should select next eMMC device for booting */
> > > > > > > > +			part = 0;
> > > > > > > > +			break;
> > > > > > > > +		case 1: /* Boot partition 1 is used for booting */
> > > > > > > > +			part = 1;
> > > > > > > > +			break;
> > > > > > > > +		case 2: /* Boot partition 2 is used for booting */
> > > > > > > > +			part = 2;
> > > > > > > > +			break;
> > > > > > > > +		case 7: /* User area is used for booting */
> > > > > > > > +			part = 0;
> > > > > > > > +			break;
> > > > > > > > +		default: /* Other values are reserved */
> > > > > > > > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > > > > > > > +#ifdef CONFIG_SPL_MMC_WARNINGS
> > > > > > > > +			puts("spl: WARNING: EXT_CSD[179] register is configured to boot from Reserved value\n");
> > > > > > > > +			puts("spl: WARNING: Selecting User Area partition for booting\n");
> > > > > > > > +#else
> > > > > > > > +			puts("spl: mmc: fallback to user area\n");
> > > > > > > > +#endif
> > > > > > > > +#endif
> > > > > > > > +			part = 0;
> > > > > > > > +			break;
> > > > > > > > +		}
> > > > > > > > +	}
> > > > > > > >  #endif
> > > > > > > 
> > > > > > > Please just use debug() for these messages.
> > > > > > 
> > > > > > All other error/warning messages in this file are printed via puts().
> > > > > > So I'm just following the current style (and I'm really not going to
> > > > > > change all occurrences in this patch).
> > > > > 
> > > > > Except for the messages in that file which use debug() they use puts(),
> > > > > yes. Since none of these are fatal messages (you're falling through)
> > > > > please switch them to debug() rather than introduce a new CONFIG symbol,
> > > > > so that if someone is bringing up a platform where this is a problem
> > > > > they'll be able to debug it, but the general case does not increase
> > > > > the binary size of most platforms. I'm not asking you to change anything
> > > > > existing in the file, only what you're adding.
> > > > 
> > > > We should at those two places fail. But I do not want to break existing
> > > > improperly configured setups, so warning a good way to show people that
> > > > they have something misconfigured. Later in future we switch warnings to
> > > > fatal errors. But if we do not show anything at these points, nobody
> > > > would figure out that has improper setup configuration. debug() is IIRC
> > > > not shown by default.
> > > 
> > > Ah, so the plan is they should be fatal, and there's a way to fix the
> > > configuration?
> > 
> > Yes, EXT_CSD[179] is configurable register, you can change boot
> > partition bits (those are non-volatile) via your favorite emmc config
> > tool. U-Boot has also tool "mmc partconf" which can do that. But you
> > first need to be able enter into u-boot console and also you need to
> > enable this tool your board config file.
> > 
> > > Lets just panic() them now, instead. I really really do
> > > not want to grow every SPL+MMC using board (which is a lot of them) by
> > > several hundred bytes of strings.  Lets catch the mis-configuration,
> > > merge it in early in the cycle (right now for v2023.10 for example,
> > > especially since the MMC custodian is promising to review things and get
> > > a PR out ASAP), and see what falls out.  A panic() and a big comment
> > > explaining things should suffice.
> > 
> > Ok, I can change "reserved values" to panic().
> > 
> > About "booting is disabled" - I do not think that panic is correct here.
> > See inline comments in code. Rather do the correct thing - skip emmc and
> > let SPL to boot from next source. But some message needs to be printed
> > here why emmc was skipped....
> 
> How verbose all of this needs to be depends on who is likely to
> encounter this type of problem. Are we talking about systems in the wild
> today that are misconfigured, or are we talking about problems that will
> arise as part of bringing up a new design and if you don't do X
> correctly then Y will happen oddly/wrongly?
> 
> -- 
> Tom

I have no idea. We already seen that not all people understood how it
works. And also people are always creative in inventing hacks. So
this is mainly for existing systems.

For future in mmc area I have just cleanup patches which should not
result in code change or in behavior change.

Re: [PATCH v2 u-boot] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 10283 bytes --]

On Fri, Jul 07, 2023 at 07:56:56PM +0200, Pali Rohár wrote:
> On Friday 07 July 2023 13:10:19 Tom Rini wrote:
> > On Fri, Jul 07, 2023 at 07:05:45PM +0200, Pali Rohár wrote:
> > > On Friday 07 July 2023 12:54:58 Tom Rini wrote:
> > > > On Fri, Jul 07, 2023 at 06:46:39PM +0200, Pali Rohár wrote:
> > > > > On Thursday 06 July 2023 13:52:14 Tom Rini wrote:
> > > > > > On Thu, Jul 06, 2023 at 07:49:18PM +0200, Pali Rohár wrote:
> > > > > > > On Thursday 06 July 2023 13:42:18 Tom Rini wrote:
> > > > > > > > On Thu, Jul 06, 2023 at 07:35:02PM +0200, Pali Rohár wrote:
> > > > > > > > > To make eMMC partition choosing in default_spl_mmc_emmc_boot_partition()
> > > > > > > > > function better understandable, rewrite it via explicit switch-case code
> > > > > > > > > pattern.
> > > > > > > > > 
> > > > > > > > > Also add a warning when eMMC EXT_CSD[179] register is configured by user to
> > > > > > > > > value which is not suitable for eMMC booting and SPL do not know how to
> > > > > > > > > interpret it.
> > > > > > > > > 
> > > > > > > > > Note that when booting from eMMC device via EXT_CSD[179] register is
> > > > > > > > > explicitly disabled then SPL still loads and boots from this eMMC device
> > > > > > > > > from User Area partition. This behavior was not changed in this commit and
> > > > > > > > > should be revisited in the future.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > > > > > ---
> > > > > > > > > Changes in v2:
> > > > > > > > > * Disable showing warning on sama5d2_xplained due to size restrictions
> > > > > > > > > ---
> > > > > > > > > This patch depends on another patch:
> > > > > > > > > mmc: spl: Add comments for default_spl_mmc_emmc_boot_partition()
> > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/
> > > > > > > > > ---
> > > > > > > > >  common/spl/Kconfig   |  7 +++++++
> > > > > > > > >  common/spl/spl_mmc.c | 46 ++++++++++++++++++++++++++++++++++++--------
> > > > > > > > >  2 files changed, 45 insertions(+), 8 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> > > > > > > > > index 865571d4579c..0574d22b3b25 100644
> > > > > > > > > --- a/common/spl/Kconfig
> > > > > > > > > +++ b/common/spl/Kconfig
> > > > > > > > > @@ -855,6 +855,13 @@ config SPL_MMC_WRITE
> > > > > > > > >  	help
> > > > > > > > >  	  Enable write access to MMC and SD Cards in SPL
> > > > > > > > >  
> > > > > > > > > +config SPL_MMC_WARNINGS
> > > > > > > > > +	bool "Print MMC warnings"
> > > > > > > > > +	depends on SPL_MMC
> > > > > > > > > +	default y if !TARGET_SAMA5D2_XPLAINED
> > > > > > > > > +	help
> > > > > > > > > +	  Print SPL MMC warnings. You can disable this option to reduce SPL size.
> > > > > > > > > +
> > > > > > > > >  
> > > > > > > > >  config SPL_MPC8XXX_INIT_DDR
> > > > > > > > >  	bool "Support MPC8XXX DDR init"
> > > > > > > > > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> > > > > > > > > index f7a42a11477d..ec424ceded0e 100644
> > > > > > > > > --- a/common/spl/spl_mmc.c
> > > > > > > > > +++ b/common/spl/spl_mmc.c
> > > > > > > > > @@ -408,15 +408,45 @@ int default_spl_mmc_emmc_boot_partition(struct mmc *mmc)
> > > > > > > > >  	 *
> > > > > > > > >  	 * Note: See difference between EXT_CSD_EXTRACT_PARTITION_ACCESS
> > > > > > > > >  	 * and EXT_CSD_EXTRACT_BOOT_PART, specially about User area value.
> > > > > > > > > -	 *
> > > > > > > > > -	 * FIXME: When booting from this eMMC device is explicitly
> > > > > > > > > -	 * disabled then we use User area for booting. This is incorrect.
> > > > > > > > > -	 * Probably we should skip this eMMC device and select the next
> > > > > > > > > -	 * one for booting. Or at least throw warning about this fallback.
> > > > > > > > >  	 */
> > > > > > > > > -	part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
> > > > > > > > > -	if (part == 7)
> > > > > > > > > -		part = 0;
> > > > > > > > > +	if (mmc->part_config == MMCPART_NOAVAILABLE)
> > > > > > > > > +		part = 0; /* If partitions are not supported then we have only User Area partition */
> > > > > > > > > +	else {
> > > > > > > > > +		switch(EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) {
> > > > > > > > > +		case 0: /* Booting from this eMMC device is disabled */
> > > > > > > > > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > > > > > > > > +#ifdef CONFIG_SPL_MMC_WARNINGS
> > > > > > > > > +			puts("spl: WARNING: Booting from this eMMC device is disabled in EXT_CSD[179] register\n");
> > > > > > > > > +			puts("spl: WARNING: Continuing anyway and selecting User Area partition for booting\n");
> > > > > > > > > +#else
> > > > > > > > > +			puts("spl: mmc: fallback to user area\n");
> > > > > > > > > +#endif
> > > > > > > > > +#endif
> > > > > > > > > +			/* FIXME: This is incorrect and probably we should select next eMMC device for booting */
> > > > > > > > > +			part = 0;
> > > > > > > > > +			break;
> > > > > > > > > +		case 1: /* Boot partition 1 is used for booting */
> > > > > > > > > +			part = 1;
> > > > > > > > > +			break;
> > > > > > > > > +		case 2: /* Boot partition 2 is used for booting */
> > > > > > > > > +			part = 2;
> > > > > > > > > +			break;
> > > > > > > > > +		case 7: /* User area is used for booting */
> > > > > > > > > +			part = 0;
> > > > > > > > > +			break;
> > > > > > > > > +		default: /* Other values are reserved */
> > > > > > > > > +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > > > > > > > > +#ifdef CONFIG_SPL_MMC_WARNINGS
> > > > > > > > > +			puts("spl: WARNING: EXT_CSD[179] register is configured to boot from Reserved value\n");
> > > > > > > > > +			puts("spl: WARNING: Selecting User Area partition for booting\n");
> > > > > > > > > +#else
> > > > > > > > > +			puts("spl: mmc: fallback to user area\n");
> > > > > > > > > +#endif
> > > > > > > > > +#endif
> > > > > > > > > +			part = 0;
> > > > > > > > > +			break;
> > > > > > > > > +		}
> > > > > > > > > +	}
> > > > > > > > >  #endif
> > > > > > > > 
> > > > > > > > Please just use debug() for these messages.
> > > > > > > 
> > > > > > > All other error/warning messages in this file are printed via puts().
> > > > > > > So I'm just following the current style (and I'm really not going to
> > > > > > > change all occurrences in this patch).
> > > > > > 
> > > > > > Except for the messages in that file which use debug() they use puts(),
> > > > > > yes. Since none of these are fatal messages (you're falling through)
> > > > > > please switch them to debug() rather than introduce a new CONFIG symbol,
> > > > > > so that if someone is bringing up a platform where this is a problem
> > > > > > they'll be able to debug it, but the general case does not increase
> > > > > > the binary size of most platforms. I'm not asking you to change anything
> > > > > > existing in the file, only what you're adding.
> > > > > 
> > > > > We should at those two places fail. But I do not want to break existing
> > > > > improperly configured setups, so warning a good way to show people that
> > > > > they have something misconfigured. Later in future we switch warnings to
> > > > > fatal errors. But if we do not show anything at these points, nobody
> > > > > would figure out that has improper setup configuration. debug() is IIRC
> > > > > not shown by default.
> > > > 
> > > > Ah, so the plan is they should be fatal, and there's a way to fix the
> > > > configuration?
> > > 
> > > Yes, EXT_CSD[179] is configurable register, you can change boot
> > > partition bits (those are non-volatile) via your favorite emmc config
> > > tool. U-Boot has also tool "mmc partconf" which can do that. But you
> > > first need to be able enter into u-boot console and also you need to
> > > enable this tool your board config file.
> > > 
> > > > Lets just panic() them now, instead. I really really do
> > > > not want to grow every SPL+MMC using board (which is a lot of them) by
> > > > several hundred bytes of strings.  Lets catch the mis-configuration,
> > > > merge it in early in the cycle (right now for v2023.10 for example,
> > > > especially since the MMC custodian is promising to review things and get
> > > > a PR out ASAP), and see what falls out.  A panic() and a big comment
> > > > explaining things should suffice.
> > > 
> > > Ok, I can change "reserved values" to panic().
> > > 
> > > About "booting is disabled" - I do not think that panic is correct here.
> > > See inline comments in code. Rather do the correct thing - skip emmc and
> > > let SPL to boot from next source. But some message needs to be printed
> > > here why emmc was skipped....
> > 
> > How verbose all of this needs to be depends on who is likely to
> > encounter this type of problem. Are we talking about systems in the wild
> > today that are misconfigured, or are we talking about problems that will
> > arise as part of bringing up a new design and if you don't do X
> > correctly then Y will happen oddly/wrongly?
> > 
> > -- 
> > Tom
> 
> I have no idea. We already seen that not all people understood how it
> works. And also people are always creative in inventing hacks. So
> this is mainly for existing systems.
> 
> For future in mmc area I have just cleanup patches which should not
> result in code change or in behavior change.

OK, so how about something like:
case 0: /* Booting from this eMMC device is disabled */
  /* Long comment to explain likely mis-configuration and how to fix it */
  puts("spl: mmc: eMMC misconfigured, falling back to eMMC user area\n");
  fallthrough;
case 7:
  part = 0;
  break;
case 1: /* Boot partition 1 is used for booting */
  part = 1;
  break;
case 2: /* Boot partition 2 is used for booting */
  part = 2;
  break;

As that shouldn't be too much of a size increase from strings, but still
be "scary" enough that users will report the message to find out what's
going on, and we can investigate from there (my first open question is
what case the TI ARCH_OMAP2PLUS trigger as ROM does not support the boot
partitions there for booting, that was only fixed in the K3 parts).

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH v3 u-boot] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit

[Pali Rohár]

To make eMMC partition choosing in default_spl_mmc_emmc_boot_partition()
function better understandable, rewrite it via explicit switch-case code
pattern.

Also indicate an error when eMMC EXT_CSD[179] register is configured by
user to value which is not suitable for eMMC booting and SPL do not know
how to interpret it.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
Changes in v3:
* Instead of showing (verbose) warning, make them as errors and
  propagate them back to caller.
* Add comment with explanation what happened and how to fix possible
  misconfigured configuration.

Changes in v2:
* Disable showing warning on sama5d2_xplained due to size restrictions
---
This patch depends on another patch:
mmc: spl: Add comments for default_spl_mmc_emmc_boot_partition()
https://patchwork.ozlabs.org/project/uboot/patch/20230404202805.8523-1-pali@kernel.org/
---
 common/spl/spl_mmc.c | 56 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 48 insertions(+), 8 deletions(-)

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index f7a42a11477d..656363a3b51a 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -408,15 +408,46 @@ int default_spl_mmc_emmc_boot_partition(struct mmc *mmc)
 	 *
 	 * Note: See difference between EXT_CSD_EXTRACT_PARTITION_ACCESS
 	 * and EXT_CSD_EXTRACT_BOOT_PART, specially about User area value.
-	 *
-	 * FIXME: When booting from this eMMC device is explicitly
-	 * disabled then we use User area for booting. This is incorrect.
-	 * Probably we should skip this eMMC device and select the next
-	 * one for booting. Or at least throw warning about this fallback.
 	 */
-	part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
-	if (part == 7)
-		part = 0;
+	if (mmc->part_config == MMCPART_NOAVAILABLE)
+		part = 0; /* If partitions are not supported then we have only User Area partition */
+	else {
+		switch(EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config)) {
+		case 0: /* Booting from this eMMC device is disabled */
+			/*
+			 * This eMMC device has disabled booting.
+			 * This may happen because of misconfiguration of eMMC device or
+			 * because user explicitly wanted to not boot from this eMMC device.
+			 * eMMC boot configuration can be changed by "mmc partconf" command.
+			 */
+			part = -ENXIO; /* negative value indicates error */
+			/* Note that error message is printed by caller of this function. */
+			break;
+		case 1: /* Boot partition 1 is used for booting */
+			part = 1;
+			break;
+		case 2: /* Boot partition 2 is used for booting */
+			part = 2;
+			break;
+		case 7: /* User area is used for booting */
+			part = 0;
+			break;
+		default: /* Other values are reserved */
+			/*
+			 * This eMMC device has configured booting from reserved value.
+			 * This may happen because of misconfiguration of eMMC device or
+			 * because this is newer eMMC device than what U-Boot understand.
+			 * If newer eMMC specification defines meaning for some reserved
+			 * values then switch above should be updated for new cases.
+			 * At this stage we do not know how to interpret this reserved
+			 * value so return error.
+			 * eMMC boot configuration can be changed by "mmc partconf" command.
+			 */
+			part = -EINVAL; /* negative value indicates error */
+			/* Note that error message is printed by caller of this function. */
+			break;
+		}
+	}
 #endif
 	return part;
 }
@@ -471,6 +502,15 @@ int spl_mmc_load(struct spl_image_info *spl_image,
 	switch (boot_mode) {
 	case MMCSD_MODE_EMMCBOOT:
 		part = spl_mmc_emmc_boot_partition(mmc);
+		if (part < 0) {
+#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
+			if (part == -ENXIO)
+				puts("spl: mmc booting disabled\n");
+			else
+				puts("spl: mmc misconfigured\n");
+#endif
+			return part;
+		}
 
 		if (CONFIG_IS_ENABLED(MMC_TINY))
 			err = mmc_switch_part(mmc, part);
-- 
2.20.1

Re: [PATCH v3 u-boot] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 644 bytes --]

On Sat, Jul 08, 2023 at 12:54:38AM +0200, Pali Rohár wrote:

> To make eMMC partition choosing in default_spl_mmc_emmc_boot_partition()
> function better understandable, rewrite it via explicit switch-case code
> pattern.
> 
> Also indicate an error when eMMC EXT_CSD[179] register is configured by
> user to value which is not suitable for eMMC booting and SPL do not know
> how to interpret it.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

I did some quick local testing to check on the size impact and this is
indeed quite manageable, thanks for reworking things!

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

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 u-boot] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit

[Pali Rohár]

On Friday 07 July 2023 19:43:34 Tom Rini wrote:
> On Sat, Jul 08, 2023 at 12:54:38AM +0200, Pali Rohár wrote:
> 
> > To make eMMC partition choosing in default_spl_mmc_emmc_boot_partition()
> > function better understandable, rewrite it via explicit switch-case code
> > pattern.
> > 
> > Also indicate an error when eMMC EXT_CSD[179] register is configured by
> > user to value which is not suitable for eMMC booting and SPL do not know
> > how to interpret it.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> I did some quick local testing to check on the size impact and this is
> indeed quite manageable, thanks for reworking things!
> 
> Reviewed-by: Tom Rini <trini@konsulko.com>
> 
> -- 
> Tom

Unfortunately no, sama5d2_xplained_emmc, sama5d2_xplained_mmc and
sama5d2_xplained_qspiflash are failing with v3; but not with v2.

Seems that these boards have their SPL at limit and adding any new
puts() increase size over the limit.

So I think that there is no other way than just adding a new config
option to hide this new puts().

Re: [PATCH v3 u-boot] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]

On Sat, Jul 08, 2023 at 10:30:47AM +0200, Pali Rohár wrote:
> On Friday 07 July 2023 19:43:34 Tom Rini wrote:
> > On Sat, Jul 08, 2023 at 12:54:38AM +0200, Pali Rohár wrote:
> > 
> > > To make eMMC partition choosing in default_spl_mmc_emmc_boot_partition()
> > > function better understandable, rewrite it via explicit switch-case code
> > > pattern.
> > > 
> > > Also indicate an error when eMMC EXT_CSD[179] register is configured by
> > > user to value which is not suitable for eMMC booting and SPL do not know
> > > how to interpret it.
> > > 
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > 
> > I did some quick local testing to check on the size impact and this is
> > indeed quite manageable, thanks for reworking things!
> > 
> > Reviewed-by: Tom Rini <trini@konsulko.com>
> 
> Unfortunately no, sama5d2_xplained_emmc, sama5d2_xplained_mmc and
> sama5d2_xplained_qspiflash are failing with v3; but not with v2.
> 
> Seems that these boards have their SPL at limit and adding any new
> puts() increase size over the limit.
> 
> So I think that there is no other way than just adding a new config
> option to hide this new puts().

We should enable LTO on those platforms then and buy us some space.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 u-boot] mmc: spl: Make partition choice in default_spl_mmc_emmc_boot_partition() more explicit

[Pali Rohár]

On Saturday 08 July 2023 11:28:34 Tom Rini wrote:
> On Sat, Jul 08, 2023 at 10:30:47AM +0200, Pali Rohár wrote:
> > On Friday 07 July 2023 19:43:34 Tom Rini wrote:
> > > On Sat, Jul 08, 2023 at 12:54:38AM +0200, Pali Rohár wrote:
> > > 
> > > > To make eMMC partition choosing in default_spl_mmc_emmc_boot_partition()
> > > > function better understandable, rewrite it via explicit switch-case code
> > > > pattern.
> > > > 
> > > > Also indicate an error when eMMC EXT_CSD[179] register is configured by
> > > > user to value which is not suitable for eMMC booting and SPL do not know
> > > > how to interpret it.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > 
> > > I did some quick local testing to check on the size impact and this is
> > > indeed quite manageable, thanks for reworking things!
> > > 
> > > Reviewed-by: Tom Rini <trini@konsulko.com>
> > 
> > Unfortunately no, sama5d2_xplained_emmc, sama5d2_xplained_mmc and
> > sama5d2_xplained_qspiflash are failing with v3; but not with v2.
> > 
> > Seems that these boards have their SPL at limit and adding any new
> > puts() increase size over the limit.
> > 
> > So I think that there is no other way than just adding a new config
> > option to hide this new puts().
> 
> We should enable LTO on those platforms then and buy us some space.
> 
> -- 
> Tom


Ok, but then this is really out of what I have a free time to do...