Re: [PATCH 1/3] of/fdt: Scan the root node properties earlier

[Frank Rowand <frowand.list@gmail.com>]

On 09/05/18 04:51, Rob Herring wrote:
> On Tue, Sep 4, 2018 at 8:49 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 08/30/18 12:05, Rob Herring wrote:
>>> Scan the root node properties (#{size,address}-cells) earlier,
>>
>>                                                         ^^^^^^^
>>                              before mdesc->dt_fixup() is called
>>
>>> so that
>>> the dt_root_addr_cells and dt_root_size_cells variables are initialized
>>> and can be used.
>>                  by mdesc->dt_fixup()
> 
> That's an ARM specific detail. Granted, ARM is the only caller.

The dt_root_addr_cells and dt_root_size_cells variables are being
initialized earlier in this patch series so that of_fdt_limit_memory()
can use them.  The only caller of of_fdt_limit_memory() is
exynos_dt_fixup(), which is an mdesc->dt_fixup() function.


> 
>>>
>>> Cc: Frank Rowand <frowand.list@gmail.com>
>>> Signed-off-by: Rob Herring <robh@kernel.org>
>>> ---
>>>  drivers/of/fdt.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>> Moving early_init_dt_scan_root() to inside early_init_dt_verify()
>> puts something that has nothing to do with verifying the fdt
>> into a function whose purpose is the verify.  It hides the side
>> effect of initializing the dt_root_addr_cells and dt_root_size_cells
>> variables.
> 
> It already has the side effect of setting initial_boot_params which
> every subsequent function needs.

And that side effect should probably also be moved.


>> I suggest creating a new function early_init_dt_scan_init_pre_dt_fixup(),
>> move the chunk of code there instead of to early_init_dt_scan_nodes(),
>> and call the new function from setup_machine_fdt(), just before
>> calling  mdesc->dt_fixup().  This would be a little bit more code,
>> but more clearly showing the intent.
> 
> I'm trying to reduce the number of functions arches call

I like that goal.


> and renaming
> would need a bunch of arch changes. This change will also let me make
> early_init_dt_scan_root private as powerpc is the only user. I need to
> dust off a patch for that.
> 
> I'd be more inclined to push exynos to remove this altogether. After

Not a bad idea.

> all, if they claim their bindings are unstable, they can't really
> claim their bootloader is stable/fixed.

It seems that this series is showing us that maybe the three architecture
specific (arc, arm, arm64) versions of setup_machine_fdt() should be
consolidated so that we have consistent behavior for FDT.

If we had a single setup_machine_fdt() then some of he hidden side
effects of functions called by setup_machine_fdt() could instead
be hoisted into setup_machine_fdt().

> 
> Rob
>