of: root #{size,address}-cells clean-ups
Subsystems (2)
Rob Herring
maintainercover of: root #{size,address}-cells clean-ups
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(-)
Signature · 1 line
2.17.1
Rob Herring
maintainerPATCH 1/3 of/fdt: Scan the root node properties earlier
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.
--- drivers/of/fdt.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/of/fdt.c +4 -3
@@ -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 ); }
Signature · 1 line
2.17.1
Re: [PATCH 1/3] of/fdt: Scan the root node properties earlier
Quoted 1 line · Rob Herring
Scan the root node properties (#{size,address}-cells) earlier,
^^^^^^^
before mdesc->dt_fixup() is calledQuoted 3 lines
so that
thedt_root_addr_cellsanddt_root_size_cellsvariables are initialized and can be used.
by mdesc->dt_fixup()
Quoted 7 lines
Cc Frank Rowand <[email protected]>Signed-off-by Rob Herring <[email protected]>--- 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
Quoted 26 lines
drivers/of/fdt.c +4
-3@@ -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 ); }
Rob Herring
maintainerRe: [PATCH 1/3] of/fdt: Scan the root node properties earlier
Quoted 11 lines · Frank Rowand
Quoted 1 line · Rob Herring
Scan the root node properties (#{size,address}-cells) earlier,
^^^^^^^ before mdesc->dt_fixup() is calledQuoted 3 lines
so that
thedt_root_addr_cellsanddt_root_size_cellsvariables are initialized and can be used.by mdesc->dt_fixup()
That's an ARM specific detail. Granted, ARM is the only caller.
Quoted 7 lines
Quoted 7 lines
Cc Frank Rowand <[email protected]>Signed-off-by Rob Herring <[email protected]>--- drivers/of/fdt.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
Quoted 5 lines
Moving
early_init_dt_scan_root()to insideearly_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 thedt_root_addr_cellsanddt_root_size_cellsvariables.
It already has the side effect of setting initial_boot_params which every subsequent function needs.
Quoted 5 lines
I suggest creating a new function
early_init_dt_scan_init_pre_dt_fixup(), move the chunk of code there instead of toearly_init_dt_scan_nodes(), and call the new function fromsetup_machine_fdt(), just before callingmdesc->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
Quoted 8 lines · Rob Herring
Quoted 7 lines · Frank Rowand
Quoted 1 line · Rob Herring
Scan the root node properties (#{size,address}-cells) earlier,
^^^^^^^ before mdesc->dt_fixup() is called
Quoted 6 lines
Quoted 4 lines
Quoted 3 lines
so that
thedt_root_addr_cellsanddt_root_size_cellsvariables 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.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Cc Frank Rowand <[email protected]>Signed-off-by Rob Herring <[email protected]>--- drivers/of/fdt.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
Quoted 8 lines
Quoted 5 lines
Moving
early_init_dt_scan_root()to insideearly_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 thedt_root_addr_cellsanddt_root_size_cellsvariables.It already has the side effect of setting
initial_boot_paramswhich every subsequent function needs.
And that side effect should probably also be moved.
Quoted 7 lines
Quoted 5 lines
I suggest creating a new function
early_init_dt_scan_init_pre_dt_fixup(), move the chunk of code there instead of toearly_init_dt_scan_nodes(), and call the new function fromsetup_machine_fdt(), just before callingmdesc->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.
Quoted 6 lines
and renaming
would need a bunch of arch changes. This change will also let me makeearly_init_dt_scan_rootprivate 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.
Quoted 2 lines
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().
Quoted 3 lines
Rob
Rob Herring
maintainerRe: [PATCH 1/3] of/fdt: Scan the root node properties earlier
Quoted 37 lines · Frank Rowand
Quoted 8 lines · Rob Herring
Quoted 7 lines · Frank Rowand
Quoted 1 line · Rob Herring
Scan the root node properties (#{size,address}-cells) earlier,
^^^^^^^ before mdesc->dt_fixup() is calledQuoted 6 lines
Quoted 4 lines
Quoted 3 lines
so that
thedt_root_addr_cellsanddt_root_size_cellsvariables 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_cellsanddt_root_size_cellsvariables are being initialized earlier in this patch series so thatof_fdt_limit_memory()can use them. The only caller ofof_fdt_limit_memory()isexynos_dt_fixup(), which is anmdesc->dt_fixup()function.Quoted 14 lines
Quoted 13 lines
Quoted 8 lines
Cc Frank Rowand <[email protected]>Signed-off-by Rob Herring <[email protected]>--- drivers/of/fdt.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)Moving
early_init_dt_scan_root()to insideearly_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 thedt_root_addr_cellsanddt_root_size_cellsvariables.
Quoted 4 lines
Quoted 2 lines
It already has the side effect of setting
initial_boot_paramswhich 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.
Quoted 23 lines
Quoted 7 lines
Quoted 5 lines
I suggest creating a new function
early_init_dt_scan_init_pre_dt_fixup(), move the chunk of code there instead of toearly_init_dt_scan_nodes(), and call the new function fromsetup_machine_fdt(), just before callingmdesc->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.
Quoted 6 lines
and renaming
would need a bunch of arch changes. This change will also let me makeearly_init_dt_scan_rootprivate 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.
Quoted 2 lines
all, if they claim their bindings are unstable, they can't really claim their bootloader is stable/fixed.
Quoted 7 lines
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 bysetup_machine_fdt()could instead be hoisted intosetup_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
Quoted 38 lines · Rob Herring
Quoted 37 lines · Frank Rowand
Quoted 8 lines · Rob Herring
Quoted 7 lines · Frank Rowand
Quoted 1 line · Rob Herring
Scan the root node properties (#{size,address}-cells) earlier,
^^^^^^^ before mdesc->dt_fixup() is calledQuoted 6 lines
Quoted 4 lines
Quoted 3 lines
so that
thedt_root_addr_cellsanddt_root_size_cellsvariables 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_cellsanddt_root_size_cellsvariables are being initialized earlier in this patch series so thatof_fdt_limit_memory()can use them. The only caller ofof_fdt_limit_memory()isexynos_dt_fixup(), which is anmdesc->dt_fixup()function.Quoted 14 lines
Quoted 13 lines
Quoted 8 lines
Cc Frank Rowand <[email protected]>Signed-off-by Rob Herring <[email protected]>--- drivers/of/fdt.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)Moving
early_init_dt_scan_root()to insideearly_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 thedt_root_addr_cellsanddt_root_size_cellsvariables.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
It already has the side effect of setting
initial_boot_paramswhich 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.
Quoted 32 lines
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.Quoted 27 lines
Quoted 7 lines
Quoted 5 lines
I suggest creating a new function
early_init_dt_scan_init_pre_dt_fixup(), move the chunk of code there instead of toearly_init_dt_scan_nodes(), and call the new function fromsetup_machine_fdt(), just before callingmdesc->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.
Quoted 6 lines
and renaming
would need a bunch of arch changes. This change will also let me makeearly_init_dt_scan_rootprivate 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.
Quoted 2 lines
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.
Quoted 7 lines
Quoted 3 lines
If we had a single
setup_machine_fdt()then some of he hidden side effects of functions called bysetup_machine_fdt()could instead be hoisted intosetup_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.
Quoted 3 lines
Rob
Rob Herring
maintainerRe: [PATCH 1/3] of/fdt: Scan the root node properties earlier
Quoted 90 lines · Frank Rowand
Quoted 38 lines · Rob Herring
Quoted 37 lines · Frank Rowand
Quoted 8 lines · Rob Herring
Quoted 7 lines · Frank Rowand
Quoted 1 line · Rob Herring
Scan the root node properties (#{size,address}-cells) earlier,
^^^^^^^ before mdesc->dt_fixup() is calledQuoted 6 lines
Quoted 4 lines
Quoted 3 lines
so that
thedt_root_addr_cellsanddt_root_size_cellsvariables 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_cellsanddt_root_size_cellsvariables are being initialized earlier in this patch series so thatof_fdt_limit_memory()can use them. The only caller ofof_fdt_limit_memory()isexynos_dt_fixup(), which is anmdesc->dt_fixup()function.Quoted 14 lines
Quoted 13 lines
Quoted 8 lines
Cc Frank Rowand <[email protected]>Signed-off-by Rob Herring <[email protected]>--- drivers/of/fdt.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)Moving
early_init_dt_scan_root()to insideearly_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 thedt_root_addr_cellsanddt_root_size_cellsvariables.Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
It already has the side effect of setting
initial_boot_paramswhich 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 insetup_machine_fdt()instead of hiding them in sub-functions that are called by all of the different architectures.Quoted 36 lines
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.Quoted 30 lines
Quoted 7 lines
Quoted 5 lines
I suggest creating a new function
early_init_dt_scan_init_pre_dt_fixup(), move the chunk of code there instead of toearly_init_dt_scan_nodes(), and call the new function fromsetup_machine_fdt(), just before callingmdesc->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.
Quoted 6 lines
and renaming
would need a bunch of arch changes. This change will also let me makeearly_init_dt_scan_rootprivate 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.
Quoted 2 lines
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 bysetup_machine_fdt()could instead be hoisted intosetup_machine_fdt().
Quoted 8 lines
Quoted 3 lines
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
_shouldbe_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.
Quoted 5 lines
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
Hi Rob,
Quoted 112 lines · Rob Herring
Quoted 90 lines · Frank Rowand
Quoted 38 lines · Rob Herring
Quoted 37 lines · Frank Rowand
Quoted 8 lines · Rob Herring
Quoted 7 lines · Frank Rowand
Quoted 1 line · Rob Herring
Scan the root node properties (#{size,address}-cells) earlier,
^^^^^^^ before mdesc->dt_fixup() is calledQuoted 6 lines
Quoted 4 lines
Quoted 3 lines
so that
thedt_root_addr_cellsanddt_root_size_cellsvariables 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_cellsanddt_root_size_cellsvariables are being initialized earlier in this patch series so thatof_fdt_limit_memory()can use them. The only caller ofof_fdt_limit_memory()isexynos_dt_fixup(), which is anmdesc->dt_fixup()function.Quoted 14 lines
Quoted 13 lines
Quoted 8 lines
Cc Frank Rowand <[email protected]>Signed-off-by Rob Herring <[email protected]>--- drivers/of/fdt.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)Moving
early_init_dt_scan_root()to insideearly_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 thedt_root_addr_cellsanddt_root_size_cellsvariables.Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
It already has the side effect of setting
initial_boot_paramswhich 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 insetup_machine_fdt()instead of hiding them in sub-functions that are called by all of the different architectures.Quoted 36 lines
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.Quoted 30 lines
Quoted 7 lines
Quoted 5 lines
I suggest creating a new function
early_init_dt_scan_init_pre_dt_fixup(), move the chunk of code there instead of toearly_init_dt_scan_nodes(), and call the new function fromsetup_machine_fdt(), just before callingmdesc->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.
Quoted 6 lines
and renaming
would need a bunch of arch changes. This change will also let me makeearly_init_dt_scan_rootprivate 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.
Quoted 2 lines
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 bysetup_machine_fdt()could instead be hoisted intosetup_machine_fdt().Quoted 8 lines
Quoted 3 lines
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
_shouldbe_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.
Quoted 5 lines
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.
Quoted 4 lines
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.
Quoted 3 lines
Rob
Rob Herring
maintainerPATCH 2/3 of/fdt: avoid re-parsing '#{address,size}-cells' in of_fdt_limit_memory
In reply to Rob HerringNow that we initialize dt_root_addr_cells and dt_root_size_cells earlier, use them and simplify of_fdt_limit_memory.
--- drivers/of/fdt.c | 23 +---------------------- 1 file changed, 1 insertion(+), 22 deletions(-)
drivers/of/fdt.c +1 -22
@@ -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 ) {
Signature · 1 line
2.17.1
kbuild test robot
build botRe: [PATCH 2/3] of/fdt: avoid re-parsing '#{address,size}-cells' in of_fdt_limit_memory
I love your patch! Perhaps something to improve:
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
Re: [PATCH 2/3] of/fdt: avoid re-parsing '#{address,size}-cells' in of_fdt_limit_memory
In reply to Rob Herringdrivers/of/fdt.c
Quoted 13 lines · Rob Herring
Now that we initialize
dt_root_addr_cellsanddt_root_size_cellsearlier, use them and simplifyof_fdt_limit_memory.Cc Frank Rowand <[email protected]>Signed-off-by Rob Herring <[email protected]>--- drivers/of/fdt.c | 23 +---------------------- 1 file changed, 1 insertion(+), 22 deletions(-)drivers/of/fdt.c +0
-0
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)
+ {
Quoted 31 lines
drivers/of/fdt.c +1
-22@@ -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 ) {
Rob Herring
maintainerPATCH 3/3 of: make default address and size cells sizes private
In reply to Rob HerringOnly 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.
--- 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(-)
arch/sparc/include/asm/prom.h +0 -3
@@ -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))
drivers/of/of_private.h +8 -0
@@ -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;
include/linux/of.h +0 -6
@@ -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)
Signature · 1 line
2.17.1
Re: [PATCH 3/3] of: make default address and size cells sizes private
Quoted 12 lines · 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.hdependency on Sparc.Cc "David S. Miller" <[email protected]>Cc Frank Rowand <[email protected]>Signed-off-by Rob Herring <[email protected]>
Quoted 5 lines
--- 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(-)
Quoted 51 lines
arch/sparc/include/asm/prom.h +0
-3@@ -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))drivers/of/of_private.h +8
-0@@ -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;include/linux/of.h +0
-6@@ -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
Quoted 8 lines · Frank Rowand
Quoted 7 lines · 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.hdependency 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.
Rob Herring
maintainerRe: [PATCH 3/3] of: make default address and size cells sizes private
Quoted 9 lines · Mark Cave-Ayland
Quoted 6 lines · Frank Rowand
Quoted 5 lines · 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.
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
This also gets us one step closer to removing the
asm/prom.hdependency 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.
Quoted 6 lines
"#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.
Quoted 7 lines
"#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
Quoted 37 lines · Rob Herring
Quoted 9 lines · Mark Cave-Ayland
Quoted 6 lines · Frank Rowand
Quoted 5 lines · 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.
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
This also gets us one step closer to removing the
asm/prom.hdependency 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.
Quoted 6 lines
"#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.
Quoted 5 lines
"#size-cells"
Standard property name to define the package’s address size format.
...
A missing "#size-cells" property signifies the default value of one.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 13 lines · 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
Quoted 6 lines
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