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

[Simon Glass]

Hi Igor,

On 5 March 2017 at 01:39, Igor Grinberg <grinberg at compulab.co.il> wrote:
> Hi Simon,
>
> On 03/03/17 06:53, Simon Glass wrote:
>> 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.
>
> According to kernel guys, these are not h/w specific, but rather device
> specific... and it actually makes sense - on the same h/w design different
> applications can use different flash mapping - just like the block devices.

Really? That sounds like a pretty esoteric case to me.

In my experience the flash mapping is normally fixed at production
time and there may be parts that are write-protected, e.g. with a
hardware switch. The flash map generally needs to be understood by
both firmware and kernel. It is as much a feature of the hardware as
which serial console to use.

>
>>
>> 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.
>
> Well, partially...
> I agree that building different binaries with #ifdefs is very painful
> and I highly discourage this approach.
> Using a DT to handle these differences, IMO, is better but also not that
> good, as it requires building multiple DT binaries (which I consider the
> same as multiple U-Boot binaries) and therefore does not reduce the number
> of build combinations... you just abstract the problem to a separate binary.

It has a big positive impact on complexity though. You build U-Boot
once and attach the appropriate device tree at the end, perhaps during
production. This is similar to how Linux boots on different hardware
times, with just a different DT.

>
> I'm more in favor of run time detection and adjustment of static data
> along with dynamic code run.

Well that is certainly possible. I think the trade-off here depends on
your production syste,.

>
> Regarding the environment, I think it would be great to have U-Boot
> detect where environment location is and proceed with it.
> Just like the boot sequence stuff...

Well that is a separate feature from what is being discussed here. But
IMO a solid flash map available to everything is better. You can
define where the environment is, and anything else you find.

>
>>
>>>
>>>> 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.
>
> So, how would you sync between the DTs in U-Boot and kernel?

Do you mean how to sync the flashmap? It can be passed down to Linux
in the DT, if that is what you are asking?

Regards,
Simon