[PATCH] Convert CONFIG_SYS_MMC_ENV_DEV et al to Kconfig

[David Woodhouse]

On Wed, 2020-08-05 at 10:52 -0400, Tom Rini wrote:
> 
> > >> FAT and ext4 don't need to grow their own config symbols because they
> > >already *have* them. The only reason they're involved here is because
> > >of the case where they explicitly want to *abdicate* responsibility and
> > >let platform code determine the device to use, not hard-coded config.
> > >Like the MT7623 platforms where the preloader tells U-Boot if it was
> > >actually loaded from the internal eMMC or external SD card.
> > >> 
> > >> If someone is setting up the ENV_FAT_DEVICE_AND_PART to start with a
> > >colon and thus abdicate the device part, but then that decision is only
> > >coming from a hard-coded SYS_MMC_ENV_DEV option, then they're probably
> > >somewhat confused.
> > >
> > >Right, but the default mmc_get_env_dev() you added uses
> > >CONFIG_SYS_MMC_ENV_DEV.
> > 
> > Perhaps I could revamp the FAT/ext4 patches so that they don't use
> the default weak function, and if you use the "get it at runtime"
> behaviour on any build which doesn't actually have a real platform
> mmc_get_env_dev() function, the build fails.  That would be sane, but
> might involve a separate explicit config option instead of just "the
> DEVICE_AND_PART option starts with a colon".
> 
> That sounds like a good idea, thanks!


Hm. I took a quick look at this. Forgetting my ext4 patch for the
moment and looking just at what's in HEAD...

The initial naïve approach is to rip out the weak implementation of
mmc_get_env_dev() from env/fat.c and trust the compiler spot that this
is tautologically false because the strings are constants:

		part_str = CONFIG_ENV_FAT_DEVICE_AND_PART;
		if (!strcmp(CONFIG_ENV_FAT_INTERFACE, "mmc") &&
                    part_str[0] == ':') {

Sadly, GCC doesn't seem to be clever enough for that.

So the actual code for env_fat_device_and_part() would need some extra
compile-time conditional.

It would be possible to make platforms with their own 'real'
mmc_get_env_dev() function select a config symbol called something like
(imaginatively) PLATFORM_HAS_MMC_GET_ENV_DEV, and then the code in
env_fat_device_and_part() could be conditional on that as well as
CONFIG_MMC as it is at the moment.

On the whole though, it seems like overkill just to "forbid" a mildly
pointless combination of config options. And in fact, perhaps it isn't
even that pointless.

I wrote the "find environment FAT partition using mmc_get_env_dev()"
support for OpenWrt, specifically on the mt7623 platforms where it is
indeed a runtime choice. But it's actually quite sensible for OpenWrt
just to have a consistent partition layout across multiple devices, set
CONFIG_ENV_FAT_DEVICE_AND_PART to ":2" on *all* of those platforms, and
then *only* CONFIG_SYS_MMC_ENV_DEV needs to vary in a platform-specific 
manner.

I think the best option is probably to consolidate and move the weak
mmc_get_env_dev() into drivers/mmc/mmc.c and let env/{fat,ext4,mmc}.c
use it from there.

Yes, some people might be able to set CONFIG_ENV_FAT_DEVICE_AND_PART to
something starting with a colon, deferring the device number to a hard-
coded CONFIG_SYS_MMC_ENV_DEV — which at *runtime* is kind of redundant.
But that isn't actually all that insane anyway. We don't *need* to
forbid it.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5174 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200814/64b18324/attachment.bin>