From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Fri, 14 Aug 2020 15:52:30 -0400 Subject: [PATCH] Convert CONFIG_SYS_MMC_ENV_DEV et al to Kconfig In-Reply-To: <3af35e56a03f0960cc5b3d4a393e1dea5ab50220.camel@infradead.org> References: <20200724211447.3137-1-trini@konsulko.com> <174b6654-bbe1-9fee-90a6-83e1bd3207a0@prevas.dk> <863b8bdb-1518-ee95-9eaa-03c0b0ab6d7c@denx.de> <20200805125143.GK6965@bill-the-cat> <4D0928EC-7CF2-4017-A870-E3950C42753F@infradead.org> <20200805131458.GM6965@bill-the-cat> <7AF803CA-9A38-4A15-9BBB-5A6762A40A18@infradead.org> <20200805145237.GO6965@bill-the-cat> <3af35e56a03f0960cc5b3d4a393e1dea5ab50220.camel@infradead.org> Message-ID: <20200814195230.GX20605@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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: