[PATCH 0/3] of: root #{size,address}-cells clean-ups

[PATCH 0/3] of: root #{size,address}-cells clean-ups

[Rob Herring]

This is a small restructuring and clean-up of handling root node
#size-cells and #address-cells (or lack of). As only Sparc needs a 
different default value and only for #address-cells, we can handle that 
locally and remove one more dependency on asm/prom.h.

Rob

Rob Herring (3):
  of/fdt: Scan the root node properties earlier
  of/fdt: avoid re-parsing '#{address,size}-cells' in
    of_fdt_limit_memory
  of: make default address and size cells sizes private

 arch/sparc/include/asm/prom.h |  3 ---
 drivers/of/fdt.c              | 30 +++++-------------------------
 drivers/of/of_private.h       |  8 ++++++++
 include/linux/of.h            |  6 ------
 4 files changed, 13 insertions(+), 34 deletions(-)

-- 
2.17.1

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

[Rob Herring]

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

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);
 }
-- 
2.17.1

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

[Frank Rowand]

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

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.

-Frank

> 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);
>  }
> 

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

[Rob Herring]

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.

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

> 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 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
all, if they claim their bindings are unstable, they can't really
claim their bootloader is stable/fixed.

Rob

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

[Frank Rowand]

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
> 

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

[Rob Herring]

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

Rob

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

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
> 

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

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

[Frank Rowand]

Hi Rob,

On 09/05/18 14:31, Rob Herring wrote:
> 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).

I would agree with just dropping patch 1 and 2.  Patch 3 is still fine.

> 
> Rob
> 

[PATCH 2/3] of/fdt: avoid re-parsing '#{address,size}-cells' in of_fdt_limit_memory

[Rob Herring]

Now that we initialize dt_root_addr_cells and dt_root_size_cells earlier,
use them and simplify of_fdt_limit_memory.

Cc: Frank Rowand <frowand.list@gmail.com>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/of/fdt.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 49abe18f1bde..fef4b2c8a171 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -44,28 +44,7 @@ void of_fdt_limit_memory(int limit)
 	int memory;
 	int len;
 	const void *val;
-	int nr_address_cells = OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
-	int nr_size_cells = OF_ROOT_NODE_SIZE_CELLS_DEFAULT;
-	const __be32 *addr_prop;
-	const __be32 *size_prop;
-	int root_offset;
-	int cell_size;
-
-	root_offset = fdt_path_offset(initial_boot_params, "/");
-	if (root_offset < 0)
-		return;
-
-	addr_prop = fdt_getprop(initial_boot_params, root_offset,
-				"#address-cells", NULL);
-	if (addr_prop)
-		nr_address_cells = fdt32_to_cpu(*addr_prop);
-
-	size_prop = fdt_getprop(initial_boot_params, root_offset,
-				"#size-cells", NULL);
-	if (size_prop)
-		nr_size_cells = fdt32_to_cpu(*size_prop);
-
-	cell_size = sizeof(uint32_t)*(nr_address_cells + nr_size_cells);
+	int cell_size = sizeof(uint32_t)*(dt_root_addr_cells + dt_root_size_cells);
 
 	memory = fdt_path_offset(initial_boot_params, "/memory");
 	if (memory > 0) {
-- 
2.17.1

Re: [PATCH 2/3] of/fdt: avoid re-parsing '#{address,size}-cells' in of_fdt_limit_memory

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1592 bytes --]

Hi Rob,

I love your patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.19-rc1 next-20180831]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Rob-Herring/of-root-size-address-cells-clean-ups/20180831-160759
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

>> WARNING: vmlinux.o(.text+0x718907): Section mismatch in reference from the function of_fdt_limit_memory() to the variable .init.data:dt_root_addr_cells
   The function of_fdt_limit_memory() references
   the variable __initdata dt_root_addr_cells.
   This is often because of_fdt_limit_memory lacks a __initdata
   annotation or the annotation of dt_root_addr_cells is wrong.
--
>> WARNING: vmlinux.o(.text+0x718917): Section mismatch in reference from the function of_fdt_limit_memory() to the variable .init.data:dt_root_size_cells
   The function of_fdt_limit_memory() references
   the variable __initdata dt_root_size_cells.
   This is often because of_fdt_limit_memory lacks a __initdata
   annotation or the annotation of dt_root_size_cells is wrong.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65138 bytes --]

Re: [PATCH 2/3] of/fdt: avoid re-parsing '#{address,size}-cells' in of_fdt_limit_memory

[Frank Rowand]

On 08/30/18 12:05, Rob Herring wrote:
> Now that we initialize dt_root_addr_cells and dt_root_size_cells earlier,
> use them and simplify of_fdt_limit_memory.
> 
> Cc: Frank Rowand <frowand.list@gmail.com>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  drivers/of/fdt.c | 23 +----------------------
>  1 file changed, 1 insertion(+), 22 deletions(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 49abe18f1bde..fef4b2c8a171 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c

As kbuild test robot noted, of_fdt_limit_memory() will access
__initdata dt_root_addr_cells and __initdata dt_root_size_cells.
A possible (untested) fix would be:

- void of_fdt_limit_memory(int limit)
- {

+ void __init of_fdt_limit_memory(int limit)
+ {


> @@ -44,28 +44,7 @@ void of_fdt_limit_memory(int limit)
>  	int memory;
>  	int len;
>  	const void *val;
> -	int nr_address_cells = OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
> -	int nr_size_cells = OF_ROOT_NODE_SIZE_CELLS_DEFAULT;
> -	const __be32 *addr_prop;
> -	const __be32 *size_prop;
> -	int root_offset;
> -	int cell_size;
> -
> -	root_offset = fdt_path_offset(initial_boot_params, "/");
> -	if (root_offset < 0)
> -		return;
> -
> -	addr_prop = fdt_getprop(initial_boot_params, root_offset,
> -				"#address-cells", NULL);
> -	if (addr_prop)
> -		nr_address_cells = fdt32_to_cpu(*addr_prop);
> -
> -	size_prop = fdt_getprop(initial_boot_params, root_offset,
> -				"#size-cells", NULL);
> -	if (size_prop)
> -		nr_size_cells = fdt32_to_cpu(*size_prop);
> -
> -	cell_size = sizeof(uint32_t)*(nr_address_cells + nr_size_cells);
> +	int cell_size = sizeof(uint32_t)*(dt_root_addr_cells + dt_root_size_cells);
>  
>  	memory = fdt_path_offset(initial_boot_params, "/memory");
>  	if (memory > 0) {
> 

[PATCH 3/3] of: make default address and size cells sizes private

[Rob Herring]

Only some old OpenFirmware implementations rely on default sizes. Any
FDT and modern implementation should have explicit properties. Make the
OF_ROOT_NODE_*_CELLS_DEFAULT defines private so we don't get any outside
users.

This also gets us one step closer to removing the asm/prom.h dependency on
Sparc.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: sparclinux@vger.kernel.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
 arch/sparc/include/asm/prom.h | 3 ---
 drivers/of/of_private.h       | 8 ++++++++
 include/linux/of.h            | 6 ------
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/sparc/include/asm/prom.h b/arch/sparc/include/asm/prom.h
index d955c8df62d6..1902db27ff4b 100644
--- a/arch/sparc/include/asm/prom.h
+++ b/arch/sparc/include/asm/prom.h
@@ -24,9 +24,6 @@
 #include <linux/atomic.h>
 #include <linux/irqdomain.h>
 
-#define OF_ROOT_NODE_ADDR_CELLS_DEFAULT	2
-#define OF_ROOT_NODE_SIZE_CELLS_DEFAULT	1
-
 #define of_compat_cmp(s1, s2, l)	strncmp((s1), (s2), (l))
 #define of_prop_cmp(s1, s2)		strcasecmp((s1), (s2))
 #define of_node_cmp(s1, s2)		strcmp((s1), (s2))
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 216175d11d3d..5d1567025358 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -27,6 +27,14 @@ struct alias_prop {
 	char stem[0];
 };
 
+#if defined(CONFIG_SPARC)
+#define OF_ROOT_NODE_ADDR_CELLS_DEFAULT 2
+#else
+#define OF_ROOT_NODE_ADDR_CELLS_DEFAULT 1
+#endif
+
+#define OF_ROOT_NODE_SIZE_CELLS_DEFAULT 1
+
 extern struct mutex of_mutex;
 extern struct list_head aliases_lookup;
 extern struct kset *of_kset;
diff --git a/include/linux/of.h b/include/linux/of.h
index 506beca9588d..49d85f670c75 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -247,12 +247,6 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
 #include <asm/prom.h>
 #endif
 
-/* Default #address and #size cells.  Allow arch asm/prom.h to override */
-#if !defined(OF_ROOT_NODE_ADDR_CELLS_DEFAULT)
-#define OF_ROOT_NODE_ADDR_CELLS_DEFAULT 1
-#define OF_ROOT_NODE_SIZE_CELLS_DEFAULT 1
-#endif
-
 #define OF_IS_DYNAMIC(x) test_bit(OF_DYNAMIC, &x->_flags)
 #define OF_MARK_DYNAMIC(x) set_bit(OF_DYNAMIC, &x->_flags)
 
-- 
2.17.1

Re: [PATCH 3/3] of: make default address and size cells sizes private

[Frank Rowand]

On 08/30/18 12:05, Rob Herring wrote:
> Only some old OpenFirmware implementations rely on default sizes. Any
> FDT and modern implementation should have explicit properties. Make the
> OF_ROOT_NODE_*_CELLS_DEFAULT defines private so we don't get any outside
> users.
> 
> This also gets us one step closer to removing the asm/prom.h dependency on
> Sparc.
> 
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: sparclinux@vger.kernel.org
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  arch/sparc/include/asm/prom.h | 3 ---
>  drivers/of/of_private.h       | 8 ++++++++
>  include/linux/of.h            | 6 ------
>  3 files changed, 8 insertions(+), 9 deletions(-)

Reviewed-by: Frank Rowand <frank.rowand@sony.com>


> diff --git a/arch/sparc/include/asm/prom.h b/arch/sparc/include/asm/prom.h
> index d955c8df62d6..1902db27ff4b 100644
> --- a/arch/sparc/include/asm/prom.h
> +++ b/arch/sparc/include/asm/prom.h
> @@ -24,9 +24,6 @@
>  #include <linux/atomic.h>
>  #include <linux/irqdomain.h>
>  
> -#define OF_ROOT_NODE_ADDR_CELLS_DEFAULT	2
> -#define OF_ROOT_NODE_SIZE_CELLS_DEFAULT	1
> -
>  #define of_compat_cmp(s1, s2, l)	strncmp((s1), (s2), (l))
>  #define of_prop_cmp(s1, s2)		strcasecmp((s1), (s2))
>  #define of_node_cmp(s1, s2)		strcmp((s1), (s2))
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 216175d11d3d..5d1567025358 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -27,6 +27,14 @@ struct alias_prop {
>  	char stem[0];
>  };
>  
> +#if defined(CONFIG_SPARC)
> +#define OF_ROOT_NODE_ADDR_CELLS_DEFAULT 2
> +#else
> +#define OF_ROOT_NODE_ADDR_CELLS_DEFAULT 1
> +#endif
> +
> +#define OF_ROOT_NODE_SIZE_CELLS_DEFAULT 1
> +
>  extern struct mutex of_mutex;
>  extern struct list_head aliases_lookup;
>  extern struct kset *of_kset;
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 506beca9588d..49d85f670c75 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -247,12 +247,6 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
>  #include <asm/prom.h>
>  #endif
>  
> -/* Default #address and #size cells.  Allow arch asm/prom.h to override */
> -#if !defined(OF_ROOT_NODE_ADDR_CELLS_DEFAULT)
> -#define OF_ROOT_NODE_ADDR_CELLS_DEFAULT 1
> -#define OF_ROOT_NODE_SIZE_CELLS_DEFAULT 1
> -#endif
> -
>  #define OF_IS_DYNAMIC(x) test_bit(OF_DYNAMIC, &x->_flags)
>  #define OF_MARK_DYNAMIC(x) set_bit(OF_DYNAMIC, &x->_flags)
>  
> 

Re: [PATCH 3/3] of: make default address and size cells sizes private

[Mark Cave-Ayland]

On 05/09/18 02:55, Frank Rowand wrote:

> On 08/30/18 12:05, Rob Herring wrote:
>> Only some old OpenFirmware implementations rely on default sizes. Any
>> FDT and modern implementation should have explicit properties. Make the
>> OF_ROOT_NODE_*_CELLS_DEFAULT defines private so we don't get any outside
>> users.
>>
>> This also gets us one step closer to removing the asm/prom.h dependency on
>> Sparc.

Just for the record: you say "any FDT and modern implementation should
have explicit properties", however the default values of these
properties when missing are clearly defined in the IEEE-1275
specification (Annex A):

"#address-cells"
Standard property name to define the package’s address format.
...
In a package with a "decode-unit" method, a missing "#address-cells"
property signifies that the number of
address cells is two.

"#size-cells"
Standard property name to define the package’s address size format.
...
A missing "#size-cells" property signifies the default value of one.

I can't speak for FDT but it isn't completely unreasonable for a guest
parsing a DT without these properties to assume these default values.


ATB,

Mark.

Re: [PATCH 3/3] of: make default address and size cells sizes private

[Rob Herring]

On Tue, Sep 4, 2018 at 11:59 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 05/09/18 02:55, Frank Rowand wrote:
>
> > On 08/30/18 12:05, Rob Herring wrote:
> >> Only some old OpenFirmware implementations rely on default sizes. Any
> >> FDT and modern implementation should have explicit properties. Make the
> >> OF_ROOT_NODE_*_CELLS_DEFAULT defines private so we don't get any outside
> >> users.
> >>
> >> This also gets us one step closer to removing the asm/prom.h dependency on
> >> Sparc.
>
> Just for the record: you say "any FDT and modern implementation should
> have explicit properties", however the default values of these
> properties when missing are clearly defined in the IEEE-1275
> specification (Annex A):

The spec may define defaults, but best practice is to be explicit. For
FDT, dtc doesn't allow defaults (and never has). No arch added in the
last 10 years has relied on the defaults.

> "#address-cells"
> Standard property name to define the package’s address format.
> ...
> In a package with a "decode-unit" method, a missing "#address-cells"
> property signifies that the number of
> address cells is two.

So only Sparc is compliant as the default for everyone else is 1.

> "#size-cells"
> Standard property name to define the package’s address size format.
> ...
> A missing "#size-cells" property signifies the default value of one.
>
> I can't speak for FDT but it isn't completely unreasonable for a guest
> parsing a DT without these properties to assume these default values.

I'm not removing the defaults. Just not allowing for code outside of
drivers/of/ to use the defaults.

Rob

Re: [PATCH 3/3] of: make default address and size cells sizes private

[Mark Cave-Ayland]

On 05/09/18 13:12, Rob Herring wrote:

> On Tue, Sep 4, 2018 at 11:59 PM Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> On 05/09/18 02:55, Frank Rowand wrote:
>>
>>> On 08/30/18 12:05, Rob Herring wrote:
>>>> Only some old OpenFirmware implementations rely on default sizes. Any
>>>> FDT and modern implementation should have explicit properties. Make the
>>>> OF_ROOT_NODE_*_CELLS_DEFAULT defines private so we don't get any outside
>>>> users.
>>>>
>>>> This also gets us one step closer to removing the asm/prom.h dependency on
>>>> Sparc.
>>
>> Just for the record: you say "any FDT and modern implementation should
>> have explicit properties", however the default values of these
>> properties when missing are clearly defined in the IEEE-1275
>> specification (Annex A):
> 
> The spec may define defaults, but best practice is to be explicit. For
> FDT, dtc doesn't allow defaults (and never has). No arch added in the
> last 10 years has relied on the defaults.
> 
>> "#address-cells"
>> Standard property name to define the package’s address format.
>> ...
>> In a package with a "decode-unit" method, a missing "#address-cells"
>> property signifies that the number of
>> address cells is two.
> 
> So only Sparc is compliant as the default for everyone else is 1.
> 
>> "#size-cells"
>> Standard property name to define the package’s address size format.
>> ...
>> A missing "#size-cells" property signifies the default value of one.
>>
>> I can't speak for FDT but it isn't completely unreasonable for a guest
>> parsing a DT without these properties to assume these default values.
> 
> I'm not removing the defaults. Just not allowing for code outside of
> drivers/of/ to use the defaults.

Got it - if dtc requires explicit properties, then I agree there should
be no issues with this patch. Sorry for the noise.


ATB,

Mark.

Re: [PATCH 0/3] of: root #{size,address}-cells clean-ups

[Frank Rowand]

On 08/30/18 12:05, Rob Herring wrote:
> This is a small restructuring and clean-up of handling root node
> #size-cells and #address-cells (or lack of). As only Sparc needs a 
> different default value and only for #address-cells, we can handle that 
> locally and remove one more dependency on asm/prom.h.
> 
> Rob
> 
> Rob Herring (3):
>   of/fdt: Scan the root node properties earlier
>   of/fdt: avoid re-parsing '#{address,size}-cells' in
>     of_fdt_limit_memory
>   of: make default address and size cells sizes private
> 
>  arch/sparc/include/asm/prom.h |  3 ---
>  drivers/of/fdt.c              | 30 +++++-------------------------
>  drivers/of/of_private.h       |  8 ++++++++
>  include/linux/of.h            |  6 ------
>  4 files changed, 13 insertions(+), 34 deletions(-)
> 

Nice cleanup!

-Frank