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

[Frank Rowand]

On 09/05/18 13:06, Rob Herring wrote:
> On Wed, Sep 5, 2018 at 1:19 PM Frank Rowand <frowand.list@xxxxxxxxx> wrote:
>>
>> On 09/05/18 04:51, Rob Herring wrote:
>>> On Tue, Sep 4, 2018 at 8:49 PM Frank Rowand <frowand.list@xxxxxxxxx> 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@xxxxxxxxx>
>>>>> Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
>>>>> ---
>>>>>  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.
> 
> So 2 functions? One to set the blob and one to verify it. Then we can

No, I would not add yet another function.  All of these side effects are
an argument in favor of a single setup_machine_fdt(), as I suggested below.
Then all of these side effects could be in setup_machine_fdt() instead
of hiding them in sub-functions that are called by all of the different
architectures.


> just let arches decide if they want to do any verification or not.
> 
> Perhaps it should be called fdt_init(blob) and then it is vague enough
> I can do whatever I want.
> 
>>>> 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().
> 
> Those functions are all quite a bit different. ARM matches the machine
> desc while arm64 doesn't have any such thing. How the DTB gets mapped
> into virtual space also varies.

I argue that they _should be_ made to be more alike than different.  You
have only pointed out two differences.  Of those, the mapping could be
cleanly handled by an mdesc-> callback.  (I would have to look at the
match to see if that could be handled easily, but I would expect so.)

On the other hand, in a previous reply you considered removing
of_fdt_limit_memory(), which is only used for an exynos fixup.  If
you do that, then patch 1 disappears, and we can continue to
sweep under the rug the side effects that you reminded me of
with patch 1.

> 
> Rob
>