[U-Boot] [PATCH v1] env_mmc: configure environment offsets via device tree

[Simon Glass]

Hi Igor,

On 22 February 2017 at 00:35, Igor Grinberg <grinberg at compulab.co.il> wrote:
> Hi Philipp, Simon,
>
> On 02/22/17 05:59, Simon Glass wrote:
>> Hi,
>>
>> On 20 February 2017 at 02:18, Dr. Philipp Tomsich
>> <philipp.tomsich at theobroma-systems.com> wrote:
>>>
>>> On 20 Feb 2017, at 08:22, Igor Grinberg <grinberg at compulab.co.il> wrote:
>>>
>>> That sounds too odd...
>>> DT's purpose is to describe the h/w... and that does not look so...
>>> We also, have a dt file name in the environment, so this creates will create
>>> a chicken and an egg problem…
>>>
>>>
>>> I don’t really follow… as far as I knew the DT name would have to come
>>> from some other source anyway, as the device containing the env might
>>> only be described through the device tree (e.g. mmc0).
>>>
>>>
>>> Why? U-Boot can live pretty well w/o DT.
>>>
>>>
>>> If U-Boot runs without DT, then nothing will/should change about how the
>>> setting
>>> is retrieved from CONFIG_ENV_OFFSET.
>
> ok.
>
>>>
>>> The platform that motivated this change is ARCH_SUNXI, which does not use
>>> per-board defines but aims to have one generic bootloader per-SoC.
>
> Well, after a ten year experience with boars and different SoC vendors,
> I don't think it is possible...
> Unless all boards are copies of their respective reference design...
>
>>>
>>> I really don't think we should go that direction. DT is not meant to provide
>>> a solution to all your problems...
>>>
>>>
>>> I don’t see how this is different from other entries in chosen and config as
>>> of today:
>>> common/autoboot.c allows an override through /config/bootdelay
>>> common/board_r.c uses /config/load-environment
>>> common/cli.c can pull in /config/bootcmd
>>> drivers/serial/serial-uclass.c uses /chosen/stdout-path
>>>
>>> In fact, it is the absence of this mechanism that is causing problems today:
>>> CONFIG_ENV_OFFSET is not configurable through Kconfig, so we would
>>> need board-specific defines (e.g. CONFIG_SUNXI_BOARD_LYNX) and
>>> matching #ifdef primitives in a shared header (sunxi-common.h in our case).
>>>
>>>
>>> Right. Exactly, I think we should move the CONFIG_ENV_OFFSET to Kconfig.
>>> And that will solve the problem.
>>>
>>>
>>> Doing this would still get into the way of architectures that want to build
>>> a single
>>> ‘universal’ bootloader for their SoC: the ENV_OFFSET may not be the same
>>> across all board and vendor configurations. This can easily be handled with
>>> the
>>> (optional) prop in the DT, but not with the compile-time ENV_OFFSET.
>
> I think Kconfig would be enough for this, but please try your approach.
> The 'universal' thing will probably work if you don't have too many boards and
> they don't differ too much from each other...
>
>>>
>>> If we decide to this, I’d at least like to introduce the function call to
>>> (the weak
>>> function) mmc_get_env_addr(…), so we can override this in the board code.
>
> That is how it works today:
> include/environment.h:185:extern int mmc_get_env_addr(struct mmc *mmc, int copy, u32 *env_addr);
>
>>>
>>> So putting this in the DT is the best (and least intrusive) option
>>> available.
>>>
>>>
>>> Ok. I see your point now. Yet I think we should keep the DT to its purpose -
>>> which
>>> is to describe the h/w (and not the s/w placement layout).
>>
>> Well actually we put things like flash map in there and the
>> environment position seems like a very good thing to have there.
>
> Well, I thought so too... But I had a discussion with kernel people
> some time ago and they discourage this approach and would not like to
> have the flash mapping in the DT.

I'm sorry to hear that, but it doesn't change the fact that it is very
useful for dealing with hardware-specific differences between models.

Building the same software multiple times with different #ifdefs is
often painful. Using a DT to handle these minor differences reduces
the number of build combinations and simplifies testing.

>
>> So I
>> like this patch. There is too compile-time configuration in U-Boot
>> still...
>
> Yeah, I know you like it ;-) but DT is not the place for it,
> unless DT specs. guys decide to change its purpose and place
> s/w configuration straps inside...
>
> Although, U-Boot build process is not a pain at all, you might want
> to design another abstraction layer for s/w configuration straps.
> That way you will be able to keep the U-Boot core binary generic...
> Is it worse the effort? I don't know. Currently, having the board
> infrastructure, provides that layer and works fine.

At present in U-Boot DT is that layer. We don't need to ban useful
additions like this and I really am not keen on adding another config
mechanism.

>
>>
>> In fact I wish we could support this for all environment types.
>> Overall the environment drivers need some work to make them more
>> similar...
>
> Indeed, but not just that... It would be great to add a DM to env. drivers.
> This will make it possible to use more than one environment driver
> in runtime and make many people who struggle with it happy...

Yes.

Regards,
Simon