[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 at 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....