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

[Simon Glass]

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?

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


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

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

Regards,
Simon