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

[Rob Herring]

On Wed, Sep 5, 2018 at 4:10 PM Frank Rowand <frowand.list@xxxxxxxxx> wrote:
>
> 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.)

The machine desc is in no way common and only used on a few arches
(and not even common across those arches). So there's no way the core
DT code can just call a mdesc callback without addressing making that
common first. And callbacks are just another way to call arch specific
functions which are another thing I'm trying to remove.

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

I'm inclined to just drop the patch. Seemed like a simple clean-up and
I'm not interested in doing more right now (did you look at the stack
of stuff in dt/testing branch). Maybe someone else will care (spoiler:
they won't).

Rob