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