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

[Simon Glass]

Hi Mario,

On 5 December 2016 at 10:32, Mario Six <mariosix1986 at gmail.com> wrote:
> 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.

Creating a live tree is pretty easy - perhaps 500 lines of code.

Trickier is to adjust the existing fdtdec API so that you can pass a
node pointer instead of a blob and offset. I suppose we need to
support the existing API for a while.

I think ideally we should have a flat tree before relocation (to save
time and space) and a live tree afterwards. So your approach of making
the flip just before relocation seems right to me.

Regards,
Simon