[PATCH] Convert CONFIG_SYS_MMC_ENV_DEV et al to Kconfig

[Tom Rini]

On Fri, Aug 14, 2020 at 03:02:25PM +0100, David Woodhouse wrote:
> 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.

Sounds like a good plan to me!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200814/dc5558fd/attachment.sig>