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

[Igor Grinberg]

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.

> 
> 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.

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

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...

> 
>>
>>> 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?


-- 
Regards,
Igor.