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

Message ID 20180830190523.31474-2-robh@kernel.org
State New
Headers show
Series
  • of: root #{size,address}-cells clean-ups
Related show

Commit Message

Rob Herring Aug. 30, 2018, 7:05 p.m.
Scan the root node properties (#{size,address}-cells) earlier, so that
the dt_root_addr_cells and dt_root_size_cells variables are initialized
and can be used.

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(-)

-- 
2.17.1

Comments

Rob Herring Sept. 5, 2018, 9:31 p.m. | #1
On Wed, Sep 5, 2018 at 4:10 PM Frank Rowand <frowand.list@gmail.com> wrote:
>

> On 09/05/18 13:06, Rob Herring wrote:

> > On Wed, Sep 5, 2018 at 1:19 PM Frank Rowand <frowand.list@gmail.com> wrote:

> >>

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

> >

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

Patch

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 800ad252cf9c..49abe18f1bde 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1215,6 +1215,10 @@  bool __init early_init_dt_verify(void *params)
 	initial_boot_params = params;
 	of_fdt_crc32 = crc32_be(~0, initial_boot_params,
 				fdt_totalsize(initial_boot_params));
+
+	/* Initialize {size,address}-cells info */
+	of_scan_flat_dt(early_init_dt_scan_root, NULL);
+
 	return true;
 }
 
@@ -1224,9 +1228,6 @@  void __init early_init_dt_scan_nodes(void)
 	/* Retrieve various information from the /chosen node */
 	of_scan_flat_dt(early_init_dt_scan_chosen, boot_command_line);
 
-	/* Initialize {size,address}-cells info */
-	of_scan_flat_dt(early_init_dt_scan_root, NULL);
-
 	/* Setup memory, calling early_init_dt_add_memory_arch */
 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
 }