[U-Boot] [PATCH v2 22/34] env: Add a new implementation of environment access

[Simon Glass]

Hi Tom,

On 26 July 2017 at 14:13, Tom Rini <trini at konsulko.com> wrote:
> On Wed, Jul 26, 2017 at 12:02:55PM -0600, Simon Glass wrote:
>> Hi Tom,
>>
>> On 26 July 2017 at 11:23, Tom Rini <trini at konsulko.com> wrote:
>> > On Wed, Jul 26, 2017 at 10:32:51AM -0600, Simon Glass wrote:
>> >> Hi Tom,
>> >>
>> >> On 26 July 2017 at 10:04, Tom Rini <trini at konsulko.com> wrote:
>> >> > On Sun, Jul 23, 2017 at 09:19:57PM -0600, Simon Glass wrote:
>> >> >
>> >> >> We plan to move to a environment access via drivers for each location
>> >> >> where the environment can be stored. Add an implementation for this. So
>> >> >> far it is not used, but will be pressed into service in a future patch.
>> >> >>
>> >> >> Signed-off-by: Simon Glass <sjg at chromium.org>
>> >> > [snip]
>> >> >> +static enum env_location env_get_default_location(void)
>> >> >> +{
>> >> >> +     if IS_ENABLED(CONFIG_ENV_IS_IN_DATAFLASH)
>> >> >> +             return ENVL_DATAFLASH;
>> >> >> +     else if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM)
>> >> >> +             return ENVL_EEPROM;
>> >> >> +     else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT)
>> >> >> +             return ENVL_FAT;
>> >> >> +     else if IS_ENABLED(CONFIG_ENV_IS_IN_FLASH)
>> >> >> +             return ENVL_FLASH;
>> >> >> +     else if IS_ENABLED(CONFIG_ENV_IS_IN_MMC)
>> >> >> +             return ENVL_MMC;
>> >> >> +     else if IS_ENABLED(CONFIG_ENV_IS_IN_NAND)
>> >> >> +             return ENVL_NAND;
>> >> >> +     else if IS_ENABLED(CONFIG_ENV_IS_IN_NVRAM)
>> >> >> +             return ENVL_NVRAM;
>> >> >> +     else if IS_ENABLED(CONFIG_ENV_IS_IN_REMOTE)
>> >> >> +             return ENVL_REMOTE;
>> >> >> +     else if IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)
>> >> >> +             return ENVL_SPI_FLASH;
>> >> >> +     else if IS_ENABLED(CONFIG_ENV_IS_IN_UBI)
>> >> >> +             return ENVL_UBI;
>> >> >> +     else if IS_ENABLED(CONFIG_ENV_IS_NOWHERE)
>> >> >> +             return ENVL_NOWHERE;
>> >> >> +     else
>> >> >> +             return ENVL_UNKNOWN;
>> >> >> +}
>> >> >
>> >> > I think the kernel has a few examples of how the Kconfig side of things
>> >> > looks so that you can offer N options and then pick a default one (io
>> >> > scheduler, etc).  We should do that rather than alphabetical IS_ENABLED
>> >> > tests.
>> >>
>> >> Yes it is possible to have a choice in Kconfig, but for this first
>> >> series I have left it as a option. My plan is to allow the environment
>> >> to be in multiple places in the future, e.g. try using mmc and fall
>> >> back to SPI flash. I think that the environment location should be
>> >> able to be a run-time decision.
>> >>
>> >> What do you think?
>> >
>> > I think run-time decision will be helpful, yeah, but we'll still need a
>> > way to say "this is the default place to start from" at build time and
>> > be explicit about it, rather than rely on link order or "first probed"
>> > or whatever.
>>
>> At present you can really only enable one environment location and the
>> above function enforces that. This function is intended to provide the
>> 'default place to start from'. It's just that it's not enforced by
>> Kconfig. What do you think we should do in the interim? I suspect I
>> could add a patch to change it to a choice (in the interim) now that
>> the Kconfig version is done?
>
> Hmm, I thought at the end of your series it was at least linkable to
> have more than one at a time.  But even so, I think we'll still need a
> build-time "user selected X as default" when more than one are allowed.

Yes it is possible to link more than one, but we still (as of this
series) don't support it. Needs more work.

If more than one if enabled at present, it will just use the one
selected by this function. There is no way to specify a default other
than via this function. And since it can never select a different one
(other than the default), I think it is currently meaningless to talk
about having  more than one.

I'll take a look at this for follow-up work but it will be next release now.

Regards,
Simon