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 <[email protected]> > > > > > > --- > > > > > > 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/[email protected]/ > > > > > > --- > > > > > > 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....
