| Message ID | 20180830190523.31474-2-robh@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | of: root #{size,address}-cells clean-ups | expand |
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
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); }
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