[U-Boot] [PATCH 06/17] dm: Add callback to modify the device tree

[Mario Six]

Hi Simon,

On Mon, Dec 5, 2016 at 7:24 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi,
>
> On 1 December 2016 at 01:39, Stefan Roese <sr at denx.de> wrote:
>> (Adding Simon and Maxim to Cc)
>>
>> On 23.11.2016 16:12, Mario Six wrote:
>>>
>>> Certain boards come in different variations by way of utilizing daughter
>>> boards, for example. These boards might contain additional chips, which
>>> are added to the main board's busses, e.g. I2C.
>>>
>>> The device tree support for such boards would either, quite naturally,
>>> employ the overlay mechanism to add such chips to the tree, or would use
>>> one large default device tree, and delete the devices that are actually
>>> not present.
>>>
>>> Regardless of approach, even on the U-Boot level, a modification of the
>>> device tree is a prerequisite to have such modular families of boards
>>> supported properly.
>>>
>>> Therefore, we add an option to make the U-Boot device tree (the actual
>>> copy later used by the driver model) writeable, and add a callback
>>> method that allows boards to modify the device tree at an early stage,
>>> at which, hopefully, also the application of device tree overlays will
>>> be possible.
>>>
>>> Signed-off-by: Mario Six <mario.six at gdsys.cc>
>>
>>
>> I didn't follow DT overlay lately closely especially not in U-Boot.
>> Simon, Maxim could you please take a look at this patch and comment
>> on its necessity?
>>
>>
>>> ---
>>>  common/board_f.c                  |  3 +++
>>>  dts/Kconfig                       | 10 ++++++++++
>>>  include/asm-generic/global_data.h |  4 ++++
>>>  include/common.h                  |  1 +
>>>  4 files changed, 18 insertions(+)
>>>
>>> diff --git a/common/board_f.c b/common/board_f.c
>>> index 4b74835..cda5aae 100644
>>> --- a/common/board_f.c
>>> +++ b/common/board_f.c
>>> @@ -1034,6 +1034,9 @@ static init_fnc_t init_sequence_f[] = {
>>>  #ifdef CONFIG_SYS_EXTBDINFO
>>>         setup_board_extra,
>>>  #endif
>>> +#ifdef CONFIG_OF_BOARD_FIXUP
>>> +       board_fix_fdt,
>>> +#endif
>
> Can you add documentation for this somewhere? E.g. in the driver model readme?
>

OK, I'll document the feature in v2.

>>>         INIT_FUNC_WATCHDOG_RESET
>>>         reloc_fdt,
>>>         setup_reloc,
>>> diff --git a/dts/Kconfig b/dts/Kconfig
>>> index 4b7d8b1..3f64eda 100644
>>> --- a/dts/Kconfig
>>> +++ b/dts/Kconfig
>>> @@ -14,6 +14,16 @@ config OF_CONTROL
>>>           This feature provides for run-time configuration of U-Boot
>>>           via a flattened device tree.
>>>
>>> +config OF_BOARD_FIXUP
>>> +       bool "Board-specific manipulation of Device Tree"
>>> +       help
>>> +         In certain circumstances it is necessary to be able to modify
>>> +         U-Boot's device tree (e.g. to delete device from it). This
>>> option
>>> +         make the Device Tree writeable and provides a board-specific
>>> +         "board_fix_fdt" callback (called during pre-relocation time),
>>> which
>>> +         enables the board initialization to modifiy the Device Tree. The
>>> +         modified copy is subsequently used by U-Boot after relocation.
>>> +
>>>  config SPL_OF_CONTROL
>>>         bool "Enable run-time configuration via Device Tree in SPL"
>>>         depends on SPL && OF_CONTROL
>>> diff --git a/include/asm-generic/global_data.h
>>> b/include/asm-generic/global_data.h
>>> index e02863d..f566186 100644
>>> --- a/include/asm-generic/global_data.h
>>> +++ b/include/asm-generic/global_data.h
>>> @@ -69,7 +69,11 @@ typedef struct global_data {
>>>         struct udevice  *timer;         /* Timer instance for Driver Model
>>> */
>>>  #endif
>>>
>>> +#ifdef CONFIG_OF_BOARD_FIXUP
>>> +       void *fdt_blob;                 /* Our device tree, NULL if none
>>> */
>>> +#else
>>>         const void *fdt_blob;           /* Our device tree, NULL if none
>>> */
>>> +#endif
>
> Can we keep it as const and just use a cast when it needs to change?
> You could add a function which returns a writable pointer.
>

Having the pointer globally writable has the advantage that you can get a
writable pointer wherever you need it and don't have to pass it around, but
separating the parts where the pointer is writable from the ones where it is
not is probably a good idea. Will fix in v2.

>
>>>         void *new_fdt;                  /* Relocated FDT */
>>>         unsigned long fdt_size;         /* Space reserved for relocated
>>> FDT */
>>>         struct jt_funcs *jt;            /* jump table */
>>> diff --git a/include/common.h b/include/common.h
>>> index a8d833b..293ce23 100644
>>> --- a/include/common.h
>>> +++ b/include/common.h
>>> @@ -502,6 +502,7 @@ extern ssize_t spi_write (uchar *, int, uchar *, int);
>>>
>>>  /* $(BOARD)/$(BOARD).c */
>>>  int board_early_init_f (void);
>>> +int board_fix_fdt (void);
>
> Comment please. Perhaps it should pass a writable fdt pointer?
>

I'll add a comment in v2.

>>>  int board_late_init (void);
>>>  int board_postclk_init (void); /* after clocks/timebase, before
>>> env/serial */
>>>  int board_early_init_r (void);
>
> There have been discussions about moving to a live tree (hierarchical
> format) in U-Boot post-relocation. What do you think? It might make
> these changes easier or more efficient.
>

Yes, that would definitely make things easier. The approach of modifying the
tree before the main U-Boot "sees it" that I'm taking here seems a bit hacky.
The other approach I've been experimenting with consisted of starting the board
with a base device tree, which only has the devices in it that are guaranteed
to exists, gather all needed data to decide which overlays to apply, then tear
down the DM (using dm_uninit), make the modifications to the tree, and restart
the DM (with dm_init_and_scan). While this worked more or less fine, it seems
very wasteful, and a genuinely modifiable tree would be a much nicer solution.

Also, we need a solution to this problem one way or another; the way our
hardware is constructed, we need to check which devices actually exist and
which do not by querying the hardware itself. And the most natural thing to do
here would be to have a modifiable device tree.

> Regards,
> Simon
>