[PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Rob Herring (Arm)]

While OpenFirmware originally allowed walking parent nodes and default
root values for #address-cells and #size-cells, FDT has long required
explicit values. It's been a warning in dtc for the root node since the
beginning (2005) and for any parent node since 2007. Of course, not all
FDT uses dtc, but that should be the majority by far. The various
extracted OF devicetrees I have dating back to the 1990s (various
PowerMac, OLPC, PASemi Nemo) all have explicit root node properties. The
warning is disabled for Sparc as there are known systems relying on
default root node values.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v2:
 - Add a define for excluded platforms to help clarify the intent
   is to have an exclude list and make adding platforms easier.
 - Also warn when walking parent nodes.
---
 drivers/of/base.c | 28 ++++++++++++++++++++++------
 drivers/of/fdt.c  |  4 ++--
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 20603d3c9931..39fb59b666f3 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -87,15 +87,25 @@ static bool __of_node_is_type(const struct device_node *np, const char *type)
 	return np && match && type && !strcmp(match, type);
 }
 
+#define EXCLUDED_DEFAULT_CELLS_PLATFORMS ( \
+	IS_ENABLED(CONFIG_SPARC) \
+)
+
 int of_bus_n_addr_cells(struct device_node *np)
 {
 	u32 cells;
 
-	for (; np; np = np->parent)
+	for (; np; np = np->parent) {
 		if (!of_property_read_u32(np, "#address-cells", &cells))
 			return cells;
-
-	/* No #address-cells property for the root node */
+		/*
+		 * Default root value and walking parent nodes for "#address-cells"
+		 * is deprecated. Any platforms which hit this warning should
+		 * be added to the excluded list.
+		 */
+		WARN_ONCE(!EXCLUDED_DEFAULT_CELLS_PLATFORMS,
+			  "Missing '#address-cells' in %pOF\n", np);
+	}
 	return OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
 }
 
@@ -112,11 +122,17 @@ int of_bus_n_size_cells(struct device_node *np)
 {
 	u32 cells;
 
-	for (; np; np = np->parent)
+	for (; np; np = np->parent) {
 		if (!of_property_read_u32(np, "#size-cells", &cells))
 			return cells;
-
-	/* No #size-cells property for the root node */
+		/*
+		 * Default root value and walking parent nodes for "#size-cells"
+		 * is deprecated. Any platforms which hit this warning should
+		 * be added to the excluded list.
+		 */
+		WARN_ONCE(!EXCLUDED_DEFAULT_CELLS_PLATFORMS,
+			  "Missing '#size-cells' in %pOF\n", np);
+	}
 	return OF_ROOT_NODE_SIZE_CELLS_DEFAULT;
 }
 
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 4d528c10df3a..d79707fb2eb1 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -938,12 +938,12 @@ int __init early_init_dt_scan_root(void)
 	dt_root_addr_cells = OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
 
 	prop = of_get_flat_dt_prop(node, "#size-cells", NULL);
-	if (prop)
+	if (!WARN(!prop, "No '#size-cells' in root node\n"))
 		dt_root_size_cells = be32_to_cpup(prop);
 	pr_debug("dt_root_size_cells = %x\n", dt_root_size_cells);
 
 	prop = of_get_flat_dt_prop(node, "#address-cells", NULL);
-	if (prop)
+	if (!WARN(!prop, "No '#address-cells' in root node\n"))
 		dt_root_addr_cells = be32_to_cpup(prop);
 	pr_debug("dt_root_addr_cells = %x\n", dt_root_addr_cells);
 
-- 
2.45.2

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Michael Ellerman]

"Rob Herring (Arm)" <robh@kernel.org> writes:
> While OpenFirmware originally allowed walking parent nodes and default
> root values for #address-cells and #size-cells, FDT has long required
> explicit values. It's been a warning in dtc for the root node since the
> beginning (2005) and for any parent node since 2007. Of course, not all
> FDT uses dtc, but that should be the majority by far. The various
> extracted OF devicetrees I have dating back to the 1990s (various
> PowerMac, OLPC, PASemi Nemo) all have explicit root node properties.

I have various old device trees that have been given to me over the
years, and as far as I can tell they all have these properties (some of
them are partial trees so it's hard to be 100% sure).

So LGTM.

cheers

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Geert Uytterhoeven]

On Thu, Nov 7, 2024 at 12:37 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> "Rob Herring (Arm)" <robh@kernel.org> writes:
> > While OpenFirmware originally allowed walking parent nodes and default
> > root values for #address-cells and #size-cells, FDT has long required
> > explicit values. It's been a warning in dtc for the root node since the
> > beginning (2005) and for any parent node since 2007. Of course, not all
> > FDT uses dtc, but that should be the majority by far. The various
> > extracted OF devicetrees I have dating back to the 1990s (various
> > PowerMac, OLPC, PASemi Nemo) all have explicit root node properties.
>
> I have various old device trees that have been given to me over the
> years, and as far as I can tell they all have these properties (some of
> them are partial trees so it's hard to be 100% sure).

Apparently CHRP LongTrail only had #address-cells in the root node.
Interestingly, /cpus does have a (zero) @size-cells property.
http://g33rt.be/migrated/Linux/PPC/root.html
http://g33rt.be/migrated/Linux/PPC/DeviceTree.html

No idea if any of them are still alive.

> So LGTM.

Indeed.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Marek Szyprowski]

Hi Rob,

On 06.11.2024 18:10, Rob Herring (Arm) wrote:
> While OpenFirmware originally allowed walking parent nodes and default
> root values for #address-cells and #size-cells, FDT has long required
> explicit values. It's been a warning in dtc for the root node since the
> beginning (2005) and for any parent node since 2007. Of course, not all
> FDT uses dtc, but that should be the majority by far. The various
> extracted OF devicetrees I have dating back to the 1990s (various
> PowerMac, OLPC, PASemi Nemo) all have explicit root node properties. The
> warning is disabled for Sparc as there are known systems relying on
> default root node values.
>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> v2:
>   - Add a define for excluded platforms to help clarify the intent
>     is to have an exclude list and make adding platforms easier.
>   - Also warn when walking parent nodes.
> ---
>   drivers/of/base.c | 28 ++++++++++++++++++++++------
>   drivers/of/fdt.c  |  4 ++--
>   2 files changed, 24 insertions(+), 8 deletions(-)

This patch landed in today's linux-next as commit 4b28a0dec185 ("of: 
WARN on deprecated #address-cells/#size-cells handling"). In my tests I 
found that it introduces warnings on almost all of my test systems. I 
took a look at the first one I got in my logs (Samsung Exynos Rinato 
board: arch/arm/boot/dts/samsung/exynos3250-rinato.dts):

------------[ cut here ]------------
WARNING: CPU: 1 PID: 1 at drivers/of/base.c:107 
of_bus_n_addr_cells+0x98/0xcc
Missing '#address-cells' in /soc/system-controller@10020000
Modules linked in:
CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted 
6.12.0-rc6-next-20241108 #9360
Hardware name: Samsung Exynos (Flattened Device Tree)
Call trace:
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x68/0x88
  dump_stack_lvl from __warn+0x150/0x1dc
  __warn from warn_slowpath_fmt+0x128/0x1b0
  warn_slowpath_fmt from of_bus_n_addr_cells+0x98/0xcc
  of_bus_n_addr_cells from of_bus_default_flags_match+0x8/0x18
  of_bus_default_flags_match from of_match_bus+0x28/0x58
  of_match_bus from __of_get_address+0x3c/0x1c8
  __of_get_address from __of_address_to_resource.constprop.2+0x3c/0x1e8
  __of_address_to_resource.constprop.2 from of_device_alloc+0x54/0x164
  of_device_alloc from of_platform_device_create_pdata+0x60/0xfc
  of_platform_device_create_pdata from of_platform_bus_create+0x1b0/0x4dc
  of_platform_bus_create from of_platform_populate+0x80/0x114
  of_platform_populate from devm_of_platform_populate+0x50/0x98
  devm_of_platform_populate from exynos_pmu_probe+0x12c/0x284
  exynos_pmu_probe from platform_probe+0x80/0xc0
  platform_probe from really_probe+0x154/0x3ac
  really_probe from __driver_probe_device+0xa0/0x1d4
  __driver_probe_device from driver_probe_device+0x30/0xd0
  driver_probe_device from __device_attach_driver+0xbc/0x11c
  __device_attach_driver from bus_for_each_drv+0x74/0xc0
  bus_for_each_drv from __device_attach+0xec/0x1b4
  __device_attach from bus_probe_device+0x8c/0x90
  bus_probe_device from device_add+0x56c/0x77c
  device_add from of_platform_device_create_pdata+0xac/0xfc
  of_platform_device_create_pdata from of_platform_bus_create+0x1b0/0x4dc
  of_platform_bus_create from of_platform_bus_create+0x218/0x4dc
  of_platform_bus_create from of_platform_populate+0x80/0x114
  of_platform_populate from of_platform_default_populate_init+0xc0/0xd0
  of_platform_default_populate_init from do_one_initcall+0x6c/0x328
  do_one_initcall from kernel_init_freeable+0x1c8/0x218
  kernel_init_freeable from kernel_init+0x1c/0x12c
  kernel_init from ret_from_fork+0x14/0x28
Exception stack(0xe0045fb0 to 0xe0045ff8)
...
---[ end trace 0000000000000000 ]---

To silence the above warning and the rest of them on this board I had to 
do the following change:

diff --git a/arch/arm/boot/dts/samsung/exynos3250.dtsi 
b/arch/arm/boot/dts/samsung/exynos3250.dtsi
index b6c3826a9424..c09afbcd1cab 100644
--- a/arch/arm/boot/dts/samsung/exynos3250.dtsi
+++ b/arch/arm/boot/dts/samsung/exynos3250.dtsi
@@ -347,6 +347,8 @@ pmu_system_controller: system-controller@10020000 {
                         reg = <0x10020000 0x4000>;
                         interrupt-controller;
                         #interrupt-cells = <3>;
+                       #size-cells = <0>;
+                       #address-cells = <0>;
                         interrupt-parent = <&gic>;
                         clock-names = "clkout8";
                         clocks = <&cmu CLK_FIN_PLL>;
@@ -615,6 +617,8 @@ pdma1: dma-controller@12690000 {
                 adc: adc@126c0000 {
                         compatible = "samsung,exynos3250-adc";
                         reg = <0x126c0000 0x100>;
+                       #size-cells = <0>;
+                       #address-cells = <0>;
                         interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>;
                         clock-names = "adc", "sclk";
                         clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;

I'm not a device tree expert, but for me this size/address cells 
requirement for nodes, which don't have addressable children looks a bit 
excessive. I bet, that if it was really needed from specification point 
of view, Krzysztof would already add it to Samsung Exynos dtsi/dts, as 
he spent quite a lot of time making them conformant with the spec. 
Krzysztof, could you comment on this?


> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 20603d3c9931..39fb59b666f3 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -87,15 +87,25 @@ static bool __of_node_is_type(const struct device_node *np, const char *type)
>   	return np && match && type && !strcmp(match, type);
>   }
>   
> +#define EXCLUDED_DEFAULT_CELLS_PLATFORMS ( \
> +	IS_ENABLED(CONFIG_SPARC) \
> +)
> +
>   int of_bus_n_addr_cells(struct device_node *np)
>   {
>   	u32 cells;
>   
> -	for (; np; np = np->parent)
> +	for (; np; np = np->parent) {
>   		if (!of_property_read_u32(np, "#address-cells", &cells))
>   			return cells;
> -
> -	/* No #address-cells property for the root node */
> +		/*
> +		 * Default root value and walking parent nodes for "#address-cells"
> +		 * is deprecated. Any platforms which hit this warning should
> +		 * be added to the excluded list.
> +		 */
> +		WARN_ONCE(!EXCLUDED_DEFAULT_CELLS_PLATFORMS,
> +			  "Missing '#address-cells' in %pOF\n", np);
> +	}
>   	return OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
>   }
>   
> @@ -112,11 +122,17 @@ int of_bus_n_size_cells(struct device_node *np)
>   {
>   	u32 cells;
>   
> -	for (; np; np = np->parent)
> +	for (; np; np = np->parent) {
>   		if (!of_property_read_u32(np, "#size-cells", &cells))
>   			return cells;
> -
> -	/* No #size-cells property for the root node */
> +		/*
> +		 * Default root value and walking parent nodes for "#size-cells"
> +		 * is deprecated. Any platforms which hit this warning should
> +		 * be added to the excluded list.
> +		 */
> +		WARN_ONCE(!EXCLUDED_DEFAULT_CELLS_PLATFORMS,
> +			  "Missing '#size-cells' in %pOF\n", np);
> +	}
>   	return OF_ROOT_NODE_SIZE_CELLS_DEFAULT;
>   }
>   
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 4d528c10df3a..d79707fb2eb1 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -938,12 +938,12 @@ int __init early_init_dt_scan_root(void)
>   	dt_root_addr_cells = OF_ROOT_NODE_ADDR_CELLS_DEFAULT;
>   
>   	prop = of_get_flat_dt_prop(node, "#size-cells", NULL);
> -	if (prop)
> +	if (!WARN(!prop, "No '#size-cells' in root node\n"))
>   		dt_root_size_cells = be32_to_cpup(prop);
>   	pr_debug("dt_root_size_cells = %x\n", dt_root_size_cells);
>   
>   	prop = of_get_flat_dt_prop(node, "#address-cells", NULL);
> -	if (prop)
> +	if (!WARN(!prop, "No '#address-cells' in root node\n"))
>   		dt_root_addr_cells = be32_to_cpup(prop);
>   	pr_debug("dt_root_addr_cells = %x\n", dt_root_addr_cells);
>   

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Rob Herring]

On Fri, Nov 8, 2024 at 5:04 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi Rob,
>
> On 06.11.2024 18:10, Rob Herring (Arm) wrote:
> > While OpenFirmware originally allowed walking parent nodes and default
> > root values for #address-cells and #size-cells, FDT has long required
> > explicit values. It's been a warning in dtc for the root node since the
> > beginning (2005) and for any parent node since 2007. Of course, not all
> > FDT uses dtc, but that should be the majority by far. The various
> > extracted OF devicetrees I have dating back to the 1990s (various
> > PowerMac, OLPC, PASemi Nemo) all have explicit root node properties. The
> > warning is disabled for Sparc as there are known systems relying on
> > default root node values.
> >
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> > v2:
> >   - Add a define for excluded platforms to help clarify the intent
> >     is to have an exclude list and make adding platforms easier.
> >   - Also warn when walking parent nodes.
> > ---
> >   drivers/of/base.c | 28 ++++++++++++++++++++++------
> >   drivers/of/fdt.c  |  4 ++--
> >   2 files changed, 24 insertions(+), 8 deletions(-)
>
> This patch landed in today's linux-next as commit 4b28a0dec185 ("of:
> WARN on deprecated #address-cells/#size-cells handling"). In my tests I
> found that it introduces warnings on almost all of my test systems. I
> took a look at the first one I got in my logs (Samsung Exynos Rinato
> board: arch/arm/boot/dts/samsung/exynos3250-rinato.dts):

Thanks for the report. Let me know if any others have a different
backtrace. Also, since it's a WARN_ONCE, fixing one case could expose
others.

> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 1 at drivers/of/base.c:107
> of_bus_n_addr_cells+0x98/0xcc
> Missing '#address-cells' in /soc/system-controller@10020000
> Modules linked in:
> CPU: 1 UID: 0 PID: 1 Comm: swapper/0 Not tainted
> 6.12.0-rc6-next-20241108 #9360
> Hardware name: Samsung Exynos (Flattened Device Tree)
> Call trace:
>   unwind_backtrace from show_stack+0x10/0x14
>   show_stack from dump_stack_lvl+0x68/0x88
>   dump_stack_lvl from __warn+0x150/0x1dc
>   __warn from warn_slowpath_fmt+0x128/0x1b0
>   warn_slowpath_fmt from of_bus_n_addr_cells+0x98/0xcc
>   of_bus_n_addr_cells from of_bus_default_flags_match+0x8/0x18
>   of_bus_default_flags_match from of_match_bus+0x28/0x58
>   of_match_bus from __of_get_address+0x3c/0x1c8
>   __of_get_address from __of_address_to_resource.constprop.2+0x3c/0x1e8
>   __of_address_to_resource.constprop.2 from of_device_alloc+0x54/0x164
>   of_device_alloc from of_platform_device_create_pdata+0x60/0xfc
>   of_platform_device_create_pdata from of_platform_bus_create+0x1b0/0x4dc
>   of_platform_bus_create from of_platform_populate+0x80/0x114
>   of_platform_populate from devm_of_platform_populate+0x50/0x98
>   devm_of_platform_populate from exynos_pmu_probe+0x12c/0x284
>   exynos_pmu_probe from platform_probe+0x80/0xc0
>   platform_probe from really_probe+0x154/0x3ac
>   really_probe from __driver_probe_device+0xa0/0x1d4
>   __driver_probe_device from driver_probe_device+0x30/0xd0
>   driver_probe_device from __device_attach_driver+0xbc/0x11c
>   __device_attach_driver from bus_for_each_drv+0x74/0xc0
>   bus_for_each_drv from __device_attach+0xec/0x1b4
>   __device_attach from bus_probe_device+0x8c/0x90
>   bus_probe_device from device_add+0x56c/0x77c
>   device_add from of_platform_device_create_pdata+0xac/0xfc
>   of_platform_device_create_pdata from of_platform_bus_create+0x1b0/0x4dc
>   of_platform_bus_create from of_platform_bus_create+0x218/0x4dc
>   of_platform_bus_create from of_platform_populate+0x80/0x114
>   of_platform_populate from of_platform_default_populate_init+0xc0/0xd0
>   of_platform_default_populate_init from do_one_initcall+0x6c/0x328
>   do_one_initcall from kernel_init_freeable+0x1c8/0x218
>   kernel_init_freeable from kernel_init+0x1c/0x12c
>   kernel_init from ret_from_fork+0x14/0x28
> Exception stack(0xe0045fb0 to 0xe0045ff8)
> ...
> ---[ end trace 0000000000000000 ]---
>
> To silence the above warning and the rest of them on this board I had to
> do the following change:
>
> diff --git a/arch/arm/boot/dts/samsung/exynos3250.dtsi
> b/arch/arm/boot/dts/samsung/exynos3250.dtsi
> index b6c3826a9424..c09afbcd1cab 100644
> --- a/arch/arm/boot/dts/samsung/exynos3250.dtsi
> +++ b/arch/arm/boot/dts/samsung/exynos3250.dtsi
> @@ -347,6 +347,8 @@ pmu_system_controller: system-controller@10020000 {
>                          reg = <0x10020000 0x4000>;
>                          interrupt-controller;
>                          #interrupt-cells = <3>;
> +                       #size-cells = <0>;
> +                       #address-cells = <0>;
>                          interrupt-parent = <&gic>;
>                          clock-names = "clkout8";
>                          clocks = <&cmu CLK_FIN_PLL>;
> @@ -615,6 +617,8 @@ pdma1: dma-controller@12690000 {
>                  adc: adc@126c0000 {
>                          compatible = "samsung,exynos3250-adc";
>                          reg = <0x126c0000 0x100>;
> +                       #size-cells = <0>;
> +                       #address-cells = <0>;
>                          interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>;
>                          clock-names = "adc", "sclk";
>                          clocks = <&cmu CLK_TSADC>, <&cmu CLK_SCLK_TSADC>;
>
> I'm not a device tree expert, but for me this size/address cells
> requirement for nodes, which don't have addressable children looks a bit
> excessive. I bet, that if it was really needed from specification point
> of view, Krzysztof would already add it to Samsung Exynos dtsi/dts, as
> he spent quite a lot of time making them conformant with the spec.
> Krzysztof, could you comment on this?

No, you shouldn't need them and that's in fact a warning if you do.

I'm going to fold in the following fix which should fix the warning:

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 824bb449e007..f21f4699df7a 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -333,7 +333,8 @@ static unsigned int of_bus_isa_get_flags(const __be32 *addr)

 static int of_bus_default_flags_match(struct device_node *np)
 {
-       return of_bus_n_addr_cells(np) == 3;
+       /* Check for presence first since of_bus_n_addr_cells() will
walk parents */
+       return of_property_present(np, "#address-cells") &&
(of_bus_n_addr_cells(np) == 3);
 }

 /*
@@ -701,16 +702,16 @@ const __be32 *__of_get_address(struct
device_node *dev, int index, int bar_no,
        if (strcmp(bus->name, "pci") && (bar_no >= 0))
                return NULL;

-       bus->count_cells(dev, &na, &ns);
-       if (!OF_CHECK_ADDR_COUNT(na))
-               return NULL;
-
        /* Get "reg" or "assigned-addresses" property */
        prop = of_get_property(dev, bus->addresses, &psize);
        if (prop == NULL)
                return NULL;
        psize /= 4;

+       bus->count_cells(dev, &na, &ns);
+       if (!OF_CHECK_ADDR_COUNT(na))
+               return NULL;
+
        onesize = na + ns;
        for (i = 0; psize >= onesize; psize -= onesize, prop += onesize, i++) {
                u32 val = be32_to_cpu(prop[0]);

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Steven Price]

On 08/11/2024 11:04, Marek Szyprowski wrote:
> Hi Rob,
> 
> On 06.11.2024 18:10, Rob Herring (Arm) wrote:
>> While OpenFirmware originally allowed walking parent nodes and default
>> root values for #address-cells and #size-cells, FDT has long required
>> explicit values. It's been a warning in dtc for the root node since the
>> beginning (2005) and for any parent node since 2007. Of course, not all
>> FDT uses dtc, but that should be the majority by far. The various
>> extracted OF devicetrees I have dating back to the 1990s (various
>> PowerMac, OLPC, PASemi Nemo) all have explicit root node properties. The
>> warning is disabled for Sparc as there are known systems relying on
>> default root node values.
>>
>> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
>> ---
>> v2:
>>   - Add a define for excluded platforms to help clarify the intent
>>     is to have an exclude list and make adding platforms easier.
>>   - Also warn when walking parent nodes.
>> ---
>>   drivers/of/base.c | 28 ++++++++++++++++++++++------
>>   drivers/of/fdt.c  |  4 ++--
>>   2 files changed, 24 insertions(+), 8 deletions(-)
> 
> This patch landed in today's linux-next as commit 4b28a0dec185 ("of: 
> WARN on deprecated #address-cells/#size-cells handling"). In my tests I 
> found that it introduces warnings on almost all of my test systems. I 
> took a look at the first one I got in my logs (Samsung Exynos Rinato 
> board: arch/arm/boot/dts/samsung/exynos3250-rinato.dts):

Just a "me too" for rk3288-firefly.dtb:

[    0.138735] WARNING: CPU: 0 PID: 1 at drivers/of/base.c:106 of_bus_n_addr_cells+0x9c/0xd8
[    0.138776] Missing '#address-cells' in /power-management@ff730000

I'm sure it's easy to fix up the DTB, but we shouldn't be breaking long existing DTBs.

Steve

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Rob Herring]

On Fri, Nov 8, 2024 at 7:26 AM Steven Price <steven.price@arm.com> wrote:
>
> On 08/11/2024 11:04, Marek Szyprowski wrote:
> > Hi Rob,
> >
> > On 06.11.2024 18:10, Rob Herring (Arm) wrote:
> >> While OpenFirmware originally allowed walking parent nodes and default
> >> root values for #address-cells and #size-cells, FDT has long required
> >> explicit values. It's been a warning in dtc for the root node since the
> >> beginning (2005) and for any parent node since 2007. Of course, not all
> >> FDT uses dtc, but that should be the majority by far. The various
> >> extracted OF devicetrees I have dating back to the 1990s (various
> >> PowerMac, OLPC, PASemi Nemo) all have explicit root node properties. The
> >> warning is disabled for Sparc as there are known systems relying on
> >> default root node values.
> >>
> >> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> >> ---
> >> v2:
> >>   - Add a define for excluded platforms to help clarify the intent
> >>     is to have an exclude list and make adding platforms easier.
> >>   - Also warn when walking parent nodes.
> >> ---
> >>   drivers/of/base.c | 28 ++++++++++++++++++++++------
> >>   drivers/of/fdt.c  |  4 ++--
> >>   2 files changed, 24 insertions(+), 8 deletions(-)
> >
> > This patch landed in today's linux-next as commit 4b28a0dec185 ("of:
> > WARN on deprecated #address-cells/#size-cells handling"). In my tests I
> > found that it introduces warnings on almost all of my test systems. I
> > took a look at the first one I got in my logs (Samsung Exynos Rinato
> > board: arch/arm/boot/dts/samsung/exynos3250-rinato.dts):
>
> Just a "me too" for rk3288-firefly.dtb:
>
> [    0.138735] WARNING: CPU: 0 PID: 1 at drivers/of/base.c:106 of_bus_n_addr_cells+0x9c/0xd8
> [    0.138776] Missing '#address-cells' in /power-management@ff730000
>
> I'm sure it's easy to fix up the DTB, but we shouldn't be breaking long existing DTBs.

What broke?

The intent here is to exclude any platforms/arch which actually need
the deprecated behavior, not change DTBs. That's spelled out at the
WARN which I assume people would read before fixing "Missing
'#address-cells' in /power-management@ff730000". I tried to make the
warn message indicate that on v1 with:

WARN_ONCE(!IS_ENABLED(CONFIG_SPARC), "Only listed platforms should
rely on default '#address-cells'\n");

But Conor thought that wasn't clear. So I'm open to suggestions...

Rob

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Steven Price]

On 08/11/2024 14:04, Rob Herring wrote:
> On Fri, Nov 8, 2024 at 7:26 AM Steven Price <steven.price@arm.com> wrote:
>>
>> On 08/11/2024 11:04, Marek Szyprowski wrote:
>>> Hi Rob,
>>>
>>> On 06.11.2024 18:10, Rob Herring (Arm) wrote:
>>>> While OpenFirmware originally allowed walking parent nodes and default
>>>> root values for #address-cells and #size-cells, FDT has long required
>>>> explicit values. It's been a warning in dtc for the root node since the
>>>> beginning (2005) and for any parent node since 2007. Of course, not all
>>>> FDT uses dtc, but that should be the majority by far. The various
>>>> extracted OF devicetrees I have dating back to the 1990s (various
>>>> PowerMac, OLPC, PASemi Nemo) all have explicit root node properties. The
>>>> warning is disabled for Sparc as there are known systems relying on
>>>> default root node values.
>>>>
>>>> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
>>>> ---
>>>> v2:
>>>>   - Add a define for excluded platforms to help clarify the intent
>>>>     is to have an exclude list and make adding platforms easier.
>>>>   - Also warn when walking parent nodes.
>>>> ---
>>>>   drivers/of/base.c | 28 ++++++++++++++++++++++------
>>>>   drivers/of/fdt.c  |  4 ++--
>>>>   2 files changed, 24 insertions(+), 8 deletions(-)
>>>
>>> This patch landed in today's linux-next as commit 4b28a0dec185 ("of:
>>> WARN on deprecated #address-cells/#size-cells handling"). In my tests I
>>> found that it introduces warnings on almost all of my test systems. I
>>> took a look at the first one I got in my logs (Samsung Exynos Rinato
>>> board: arch/arm/boot/dts/samsung/exynos3250-rinato.dts):
>>
>> Just a "me too" for rk3288-firefly.dtb:
>>
>> [    0.138735] WARNING: CPU: 0 PID: 1 at drivers/of/base.c:106 of_bus_n_addr_cells+0x9c/0xd8
>> [    0.138776] Missing '#address-cells' in /power-management@ff730000
>>
>> I'm sure it's easy to fix up the DTB, but we shouldn't be breaking long existing DTBs.
> 
> What broke?

Nothing 'broke' as such (the board continued booting) but the WARN
shouldn't be happening. My CI treats the WARN as a failure as these
shouldn't occur unless there's a programming error.

> The intent here is to exclude any platforms/arch which actually need
> the deprecated behavior, not change DTBs. That's spelled out at the
> WARN which I assume people would read before fixing "Missing
> '#address-cells' in /power-management@ff730000". I tried to make the
> warn message indicate that on v1 with:
> 
> WARN_ONCE(!IS_ENABLED(CONFIG_SPARC), "Only listed platforms should
> rely on default '#address-cells'\n");

So one possibility is to include this platform in the exclusion list -
but I'm not sure how to do that, I assume including CONFIG_ARM in the
list would rather defeat the point of the patch. But my feeling is that
it would involve a lot of playing whack-a-mole to identify individual
platforms.

One obvious idea would be to look at the DTBs in the kernel tree and see
which are affected by this currently, that might be a good place to
start with an exclusion list.

You could also downgrade the warning to a pr_warn() or similar.

> But Conor thought that wasn't clear. So I'm open to suggestions...

I don't have any particular suggestions other than above, I just wanted
to report an existing DTB that triggers this WARN. We need to resolve
this one way or another before this patch can progress. For now I've
simply reverted this patch for my CI.

Steve

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Rob Herring]

On Fri, Nov 8, 2024 at 8:33 AM Steven Price <steven.price@arm.com> wrote:
>
> On 08/11/2024 14:04, Rob Herring wrote:
> > On Fri, Nov 8, 2024 at 7:26 AM Steven Price <steven.price@arm.com> wrote:
> >>
> >> On 08/11/2024 11:04, Marek Szyprowski wrote:
> >>> Hi Rob,
> >>>
> >>> On 06.11.2024 18:10, Rob Herring (Arm) wrote:
> >>>> While OpenFirmware originally allowed walking parent nodes and default
> >>>> root values for #address-cells and #size-cells, FDT has long required
> >>>> explicit values. It's been a warning in dtc for the root node since the
> >>>> beginning (2005) and for any parent node since 2007. Of course, not all
> >>>> FDT uses dtc, but that should be the majority by far. The various
> >>>> extracted OF devicetrees I have dating back to the 1990s (various
> >>>> PowerMac, OLPC, PASemi Nemo) all have explicit root node properties. The
> >>>> warning is disabled for Sparc as there are known systems relying on
> >>>> default root node values.
> >>>>
> >>>> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> >>>> ---
> >>>> v2:
> >>>>   - Add a define for excluded platforms to help clarify the intent
> >>>>     is to have an exclude list and make adding platforms easier.
> >>>>   - Also warn when walking parent nodes.
> >>>> ---
> >>>>   drivers/of/base.c | 28 ++++++++++++++++++++++------
> >>>>   drivers/of/fdt.c  |  4 ++--
> >>>>   2 files changed, 24 insertions(+), 8 deletions(-)
> >>>
> >>> This patch landed in today's linux-next as commit 4b28a0dec185 ("of:
> >>> WARN on deprecated #address-cells/#size-cells handling"). In my tests I
> >>> found that it introduces warnings on almost all of my test systems. I
> >>> took a look at the first one I got in my logs (Samsung Exynos Rinato
> >>> board: arch/arm/boot/dts/samsung/exynos3250-rinato.dts):
> >>
> >> Just a "me too" for rk3288-firefly.dtb:
> >>
> >> [    0.138735] WARNING: CPU: 0 PID: 1 at drivers/of/base.c:106 of_bus_n_addr_cells+0x9c/0xd8
> >> [    0.138776] Missing '#address-cells' in /power-management@ff730000
> >>
> >> I'm sure it's easy to fix up the DTB, but we shouldn't be breaking long existing DTBs.
> >
> > What broke?
>
> Nothing 'broke' as such (the board continued booting) but the WARN
> shouldn't be happening. My CI treats the WARN as a failure as these
> shouldn't occur unless there's a programming error.
>
> > The intent here is to exclude any platforms/arch which actually need
> > the deprecated behavior, not change DTBs. That's spelled out at the
> > WARN which I assume people would read before fixing "Missing
> > '#address-cells' in /power-management@ff730000". I tried to make the
> > warn message indicate that on v1 with:
> >
> > WARN_ONCE(!IS_ENABLED(CONFIG_SPARC), "Only listed platforms should
> > rely on default '#address-cells'\n");
>
> So one possibility is to include this platform in the exclusion list -
> but I'm not sure how to do that, I assume including CONFIG_ARM in the
> list would rather defeat the point of the patch. But my feeling is that
> it would involve a lot of playing whack-a-mole to identify individual
> platforms.

Please see my posted fix in this thread. Things "broke" quite a bit
more widely than anticipated.

> One obvious idea would be to look at the DTBs in the kernel tree and see
> which are affected by this currently, that might be a good place to
> start with an exclusion list.

It's been a dtc warning since 2007, so I can say all of the in tree
dts's are fine. The problem for these reported platforms is the
kernel, not the DT.

> You could also downgrade the warning to a pr_warn() or similar.

I find that pr_warn() may or may not get noticed, but WARN for sure
will which is what I want here.

Rob

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Marek Szyprowski]

Hi Rob,

On 08.11.2024 14:25, Rob Herring wrote:
> On Fri, Nov 8, 2024 at 5:04 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 06.11.2024 18:10, Rob Herring (Arm) wrote:
>>> While OpenFirmware originally allowed walking parent nodes and default
>>> root values for #address-cells and #size-cells, FDT has long required
>>> explicit values. It's been a warning in dtc for the root node since the
>>> beginning (2005) and for any parent node since 2007. Of course, not all
>>> FDT uses dtc, but that should be the majority by far. The various
>>> extracted OF devicetrees I have dating back to the 1990s (various
>>> PowerMac, OLPC, PASemi Nemo) all have explicit root node properties. The
>>> warning is disabled for Sparc as there are known systems relying on
>>> default root node values.
>>>
>>> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
>>> ---
>>> v2:
>>>    - Add a define for excluded platforms to help clarify the intent
>>>      is to have an exclude list and make adding platforms easier.
>>>    - Also warn when walking parent nodes.
>>> ---
>>>    drivers/of/base.c | 28 ++++++++++++++++++++++------
>>>    drivers/of/fdt.c  |  4 ++--
>>>    2 files changed, 24 insertions(+), 8 deletions(-)
>> This patch landed in today's linux-next as commit 4b28a0dec185 ("of:
>> WARN on deprecated #address-cells/#size-cells handling"). In my tests I
>> found that it introduces warnings on almost all of my test systems. I
>> took a look at the first one I got in my logs (Samsung Exynos Rinato
>> board: arch/arm/boot/dts/samsung/exynos3250-rinato.dts):
> Thanks for the report. Let me know if any others have a different
> backtrace. Also, since it's a WARN_ONCE, fixing one case could expose
> others.

> >...

> I'm going to fold in the following fix which should fix the warning:

This fixes all the warnings I've observed.

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>


> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 824bb449e007..f21f4699df7a 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -333,7 +333,8 @@ static unsigned int of_bus_isa_get_flags(const __be32 *addr)
>
>   static int of_bus_default_flags_match(struct device_node *np)
>   {
> -       return of_bus_n_addr_cells(np) == 3;
> +       /* Check for presence first since of_bus_n_addr_cells() will
> walk parents */
> +       return of_property_present(np, "#address-cells") &&
> (of_bus_n_addr_cells(np) == 3);
>   }
>
>   /*
> @@ -701,16 +702,16 @@ const __be32 *__of_get_address(struct
> device_node *dev, int index, int bar_no,
>          if (strcmp(bus->name, "pci") && (bar_no >= 0))
>                  return NULL;
>
> -       bus->count_cells(dev, &na, &ns);
> -       if (!OF_CHECK_ADDR_COUNT(na))
> -               return NULL;
> -
>          /* Get "reg" or "assigned-addresses" property */
>          prop = of_get_property(dev, bus->addresses, &psize);
>          if (prop == NULL)
>                  return NULL;
>          psize /= 4;
>
> +       bus->count_cells(dev, &na, &ns);
> +       if (!OF_CHECK_ADDR_COUNT(na))
> +               return NULL;
> +
>          onesize = na + ns;
>          for (i = 0; psize >= onesize; psize -= onesize, prop += onesize, i++) {
>                  u32 val = be32_to_cpu(prop[0]);
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Steven Price]

On 08/11/2024 14:58, Rob Herring wrote:
> On Fri, Nov 8, 2024 at 8:33 AM Steven Price <steven.price@arm.com> wrote:
>>
>> On 08/11/2024 14:04, Rob Herring wrote:
>>> On Fri, Nov 8, 2024 at 7:26 AM Steven Price <steven.price@arm.com> wrote:
>>>>
>>>> On 08/11/2024 11:04, Marek Szyprowski wrote:
>>>>> Hi Rob,
>>>>>
>>>>> On 06.11.2024 18:10, Rob Herring (Arm) wrote:
>>>>>> While OpenFirmware originally allowed walking parent nodes and default
>>>>>> root values for #address-cells and #size-cells, FDT has long required
>>>>>> explicit values. It's been a warning in dtc for the root node since the
>>>>>> beginning (2005) and for any parent node since 2007. Of course, not all
>>>>>> FDT uses dtc, but that should be the majority by far. The various
>>>>>> extracted OF devicetrees I have dating back to the 1990s (various
>>>>>> PowerMac, OLPC, PASemi Nemo) all have explicit root node properties. The
>>>>>> warning is disabled for Sparc as there are known systems relying on
>>>>>> default root node values.
>>>>>>
>>>>>> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
>>>>>> ---
>>>>>> v2:
>>>>>>   - Add a define for excluded platforms to help clarify the intent
>>>>>>     is to have an exclude list and make adding platforms easier.
>>>>>>   - Also warn when walking parent nodes.
>>>>>> ---
>>>>>>   drivers/of/base.c | 28 ++++++++++++++++++++++------
>>>>>>   drivers/of/fdt.c  |  4 ++--
>>>>>>   2 files changed, 24 insertions(+), 8 deletions(-)
>>>>>
>>>>> This patch landed in today's linux-next as commit 4b28a0dec185 ("of:
>>>>> WARN on deprecated #address-cells/#size-cells handling"). In my tests I
>>>>> found that it introduces warnings on almost all of my test systems. I
>>>>> took a look at the first one I got in my logs (Samsung Exynos Rinato
>>>>> board: arch/arm/boot/dts/samsung/exynos3250-rinato.dts):
>>>>
>>>> Just a "me too" for rk3288-firefly.dtb:
>>>>
>>>> [    0.138735] WARNING: CPU: 0 PID: 1 at drivers/of/base.c:106 of_bus_n_addr_cells+0x9c/0xd8
>>>> [    0.138776] Missing '#address-cells' in /power-management@ff730000
>>>>
>>>> I'm sure it's easy to fix up the DTB, but we shouldn't be breaking long existing DTBs.
>>>
>>> What broke?
>>
>> Nothing 'broke' as such (the board continued booting) but the WARN
>> shouldn't be happening. My CI treats the WARN as a failure as these
>> shouldn't occur unless there's a programming error.
>>
>>> The intent here is to exclude any platforms/arch which actually need
>>> the deprecated behavior, not change DTBs. That's spelled out at the
>>> WARN which I assume people would read before fixing "Missing
>>> '#address-cells' in /power-management@ff730000". I tried to make the
>>> warn message indicate that on v1 with:
>>>
>>> WARN_ONCE(!IS_ENABLED(CONFIG_SPARC), "Only listed platforms should
>>> rely on default '#address-cells'\n");
>>
>> So one possibility is to include this platform in the exclusion list -
>> but I'm not sure how to do that, I assume including CONFIG_ARM in the
>> list would rather defeat the point of the patch. But my feeling is that
>> it would involve a lot of playing whack-a-mole to identify individual
>> platforms.
> 
> Please see my posted fix in this thread. Things "broke" quite a bit
> more widely than anticipated.

Thanks for the pointer. Yes that fix seems to work for my board!

Thanks,
Steve

>> One obvious idea would be to look at the DTBs in the kernel tree and see
>> which are affected by this currently, that might be a good place to
>> start with an exclusion list.
> 
> It's been a dtc warning since 2007, so I can say all of the in tree
> dts's are fine. The problem for these reported platforms is the
> kernel, not the DT.
> 
>> You could also downgrade the warning to a pr_warn() or similar.
> 
> I find that pr_warn() may or may not get noticed, but WARN for sure
> will which is what I want here.
> 
> Rob

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Michael Ellerman]

Geert Uytterhoeven <geert@linux-m68k.org> writes:
> On Thu, Nov 7, 2024 at 12:37 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>> "Rob Herring (Arm)" <robh@kernel.org> writes:
>> > While OpenFirmware originally allowed walking parent nodes and default
>> > root values for #address-cells and #size-cells, FDT has long required
>> > explicit values. It's been a warning in dtc for the root node since the
>> > beginning (2005) and for any parent node since 2007. Of course, not all
>> > FDT uses dtc, but that should be the majority by far. The various
>> > extracted OF devicetrees I have dating back to the 1990s (various
>> > PowerMac, OLPC, PASemi Nemo) all have explicit root node properties.
>>
>> I have various old device trees that have been given to me over the
>> years, and as far as I can tell they all have these properties (some of
>> them are partial trees so it's hard to be 100% sure).
>
> Apparently CHRP LongTrail only had #address-cells in the root node.
> Interestingly, /cpus does have a (zero) @size-cells property.
> http://g33rt.be/migrated/Linux/PPC/root.html
> http://g33rt.be/migrated/Linux/PPC/DeviceTree.html
>
> No idea if any of them are still alive.

OK. We could fix that up in prom_init() if necessary - there's already a
bunch of workarounds in there for longtrail.

cheers

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Segher Boessenkool]

On Thu, Nov 07, 2024 at 10:35:58PM +1100, Michael Ellerman wrote:
> "Rob Herring (Arm)" <robh@kernel.org> writes:
> > While OpenFirmware originally allowed walking parent nodes and default
> > root values for #address-cells and #size-cells, FDT has long required
> > explicit values. It's been a warning in dtc for the root node since the
> > beginning (2005) and for any parent node since 2007. Of course, not all
> > FDT uses dtc, but that should be the majority by far. The various
> > extracted OF devicetrees I have dating back to the 1990s (various
> > PowerMac, OLPC, PASemi Nemo) all have explicit root node properties.
> 
> I have various old device trees that have been given to me over the
> years, and as far as I can tell they all have these properties (some of
> them are partial trees so it's hard to be 100% sure).

Many SUN systems won't have such superfluous properties.  But does
anyone use such systems at all anymore, and do people use dtc with
those :-)


Segher

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Rob Herring]

On Thu, Nov 14, 2024 at 6:59 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Thu, Nov 07, 2024 at 10:35:58PM +1100, Michael Ellerman wrote:
> > "Rob Herring (Arm)" <robh@kernel.org> writes:
> > > While OpenFirmware originally allowed walking parent nodes and default
> > > root values for #address-cells and #size-cells, FDT has long required
> > > explicit values. It's been a warning in dtc for the root node since the
> > > beginning (2005) and for any parent node since 2007. Of course, not all
> > > FDT uses dtc, but that should be the majority by far. The various
> > > extracted OF devicetrees I have dating back to the 1990s (various
> > > PowerMac, OLPC, PASemi Nemo) all have explicit root node properties.
> >
> > I have various old device trees that have been given to me over the
> > years, and as far as I can tell they all have these properties (some of
> > them are partial trees so it's hard to be 100% sure).
>
> Many SUN systems won't have such superfluous properties.  But does
> anyone use such systems at all anymore, and do people use dtc with
> those :-)

There's still a few presumably. Sparc is omitted from this warning
already because I suspected a problem which was confirmed on v1 thanks
to the DT dumps here[1].

Rob

[1] https://git.kernel.org/pub/scm/linux/kernel/git/davem/prtconfs.git/

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Michael Ellerman]

Michael Ellerman <mpe@ellerman.id.au> writes:
> "Rob Herring (Arm)" <robh@kernel.org> writes:
>> While OpenFirmware originally allowed walking parent nodes and default
>> root values for #address-cells and #size-cells, FDT has long required
>> explicit values. It's been a warning in dtc for the root node since the
>> beginning (2005) and for any parent node since 2007. Of course, not all
>> FDT uses dtc, but that should be the majority by far. The various
>> extracted OF devicetrees I have dating back to the 1990s (various
>> PowerMac, OLPC, PASemi Nemo) all have explicit root node properties.
>
> I have various old device trees that have been given to me over the
> years, and as far as I can tell they all have these properties (some of
> them are partial trees so it's hard to be 100% sure).
>
> So LGTM.

Turns out I was wrong.

The warning about #size-cells hits on some powermacs, possible fixup
patch here:

  https://lore.kernel.org/linuxppc-dev/20241126025710.591683-1-mpe@ellerman.id.au/

cheers

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Segher Boessenkool]

On Tue, Nov 26, 2024 at 02:36:32PM +1100, Michael Ellerman wrote:
> Michael Ellerman <mpe@ellerman.id.au> writes:
> > "Rob Herring (Arm)" <robh@kernel.org> writes:
> >> While OpenFirmware originally allowed walking parent nodes and default
> >> root values for #address-cells and #size-cells, FDT has long required
> >> explicit values. It's been a warning in dtc for the root node since the
> >> beginning (2005) and for any parent node since 2007. Of course, not all
> >> FDT uses dtc, but that should be the majority by far. The various
> >> extracted OF devicetrees I have dating back to the 1990s (various
> >> PowerMac, OLPC, PASemi Nemo) all have explicit root node properties.
> >
> > I have various old device trees that have been given to me over the
> > years, and as far as I can tell they all have these properties (some of
> > them are partial trees so it's hard to be 100% sure).
> >
> > So LGTM.
> 
> Turns out I was wrong.
> 
> The warning about #size-cells hits on some powermacs, possible fixup
> patch here:
> 
>   https://lore.kernel.org/linuxppc-dev/20241126025710.591683-1-mpe@ellerman.id.au/

The Open Firmware specification is extremely clear that a "missing"
"#size-cells" property means this bus has the default value of 1.

https://www.openfirmware.info/data/docs/of1275.pdf (page 186).

DTC or FDT might want to do things differently, but expecting decades
older stuff to conform to its ill-conceived unnecessarily super wordy
stuff is, well, not a plan that is likely to work very well :-)


Segher

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Rob Herring]

On Wed, Nov 27, 2024 at 3:47 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Tue, Nov 26, 2024 at 02:36:32PM +1100, Michael Ellerman wrote:
> > Michael Ellerman <mpe@ellerman.id.au> writes:
> > > "Rob Herring (Arm)" <robh@kernel.org> writes:
> > >> While OpenFirmware originally allowed walking parent nodes and default
> > >> root values for #address-cells and #size-cells, FDT has long required
> > >> explicit values. It's been a warning in dtc for the root node since the
> > >> beginning (2005) and for any parent node since 2007. Of course, not all
> > >> FDT uses dtc, but that should be the majority by far. The various
> > >> extracted OF devicetrees I have dating back to the 1990s (various
> > >> PowerMac, OLPC, PASemi Nemo) all have explicit root node properties.
> > >
> > > I have various old device trees that have been given to me over the
> > > years, and as far as I can tell they all have these properties (some of
> > > them are partial trees so it's hard to be 100% sure).
> > >
> > > So LGTM.
> >
> > Turns out I was wrong.
> >
> > The warning about #size-cells hits on some powermacs, possible fixup
> > patch here:
> >
> >   https://lore.kernel.org/linuxppc-dev/20241126025710.591683-1-mpe@ellerman.id.au/
>
> The Open Firmware specification is extremely clear that a "missing"
> "#size-cells" property means this bus has the default value of 1.

And the default for #address-cells is 2, but yet every architecture
except Sparc has that wrong.

If I have a node without #size-cells, is the default of 1 used or do
we check parent nodes? My read of the spec would be the former, but
the kernel does the latter.

> https://www.openfirmware.info/data/docs/of1275.pdf (page 186).
>
> DTC or FDT might want to do things differently, but expecting decades
> older stuff to conform to its ill-conceived unnecessarily super wordy
> stuff is, well, not a plan that is likely to work very well :-)

That is not the intention. The intention is to identify what doesn't
conform and exclude those systems from this check (or apply a fixup if
that works).

Rob

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Segher Boessenkool]

On Mon, Dec 02, 2024 at 08:18:22AM -0600, Rob Herring wrote:
> On Wed, Nov 27, 2024 at 3:47 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> > On Tue, Nov 26, 2024 at 02:36:32PM +1100, Michael Ellerman wrote:
> > > Michael Ellerman <mpe@ellerman.id.au> writes:
> > > > "Rob Herring (Arm)" <robh@kernel.org> writes:
> > > >> While OpenFirmware originally allowed walking parent nodes and default
> > > >> root values for #address-cells and #size-cells, FDT has long required
> > > >> explicit values. It's been a warning in dtc for the root node since the
> > > >> beginning (2005) and for any parent node since 2007. Of course, not all
> > > >> FDT uses dtc, but that should be the majority by far. The various
> > > >> extracted OF devicetrees I have dating back to the 1990s (various
> > > >> PowerMac, OLPC, PASemi Nemo) all have explicit root node properties.
> > > >
> > > > I have various old device trees that have been given to me over the
> > > > years, and as far as I can tell they all have these properties (some of
> > > > them are partial trees so it's hard to be 100% sure).
> > > >
> > > > So LGTM.
> > >
> > > Turns out I was wrong.
> > >
> > > The warning about #size-cells hits on some powermacs, possible fixup
> > > patch here:
> > >
> > >   https://lore.kernel.org/linuxppc-dev/20241126025710.591683-1-mpe@ellerman.id.au/
> >
> > The Open Firmware specification is extremely clear that a "missing"
> > "#size-cells" property means this bus has the default value of 1.
> 
> And the default for #address-cells is 2, but yet every architecture
> except Sparc has that wrong.

?

Almost all architectures (that run Linux) use 64-bit addressing, both
32-bit and 64-bit architectures.

> If I have a node without #size-cells, is the default of 1 used or do
> we check parent nodes? My read of the spec would be the former, but
> the kernel does the latter.

The former is correct.  The latter makes no sense at all!  The whole
point of the "bus" abstraction is that you get a new addressing domain
there.

Yes, these days you numerically find it most often with PCI sub-domains,
but those are boring.  In most cases you *do* have different adressing
on your child busses, and even if the addressing is the same, addresses
on the child bus are not normally a subset of those on the parent bus.

> > https://www.openfirmware.info/data/docs/of1275.pdf (page 186).
> >
> > DTC or FDT might want to do things differently, but expecting decades
> > older stuff to conform to its ill-conceived unnecessarily super wordy
> > stuff is, well, not a plan that is likely to work very well :-)
> 
> That is not the intention. The intention is to identify what doesn't
> conform and exclude those systems from this check (or apply a fixup if
> that works).

So *always* use the OF definition, at least on OF systems?  Where
everything is meant to conform, but conform to OF, not conform to this
"OF-like-but-very-different-in-crucial-spots" thing :-)


Segher

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Rob Herring]

On Mon, Dec 2, 2024 at 4:09 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Mon, Dec 02, 2024 at 08:18:22AM -0600, Rob Herring wrote:
> > On Wed, Nov 27, 2024 at 3:47 PM Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > > On Tue, Nov 26, 2024 at 02:36:32PM +1100, Michael Ellerman wrote:
> > > > Michael Ellerman <mpe@ellerman.id.au> writes:
> > > > > "Rob Herring (Arm)" <robh@kernel.org> writes:
> > > > >> While OpenFirmware originally allowed walking parent nodes and default
> > > > >> root values for #address-cells and #size-cells, FDT has long required
> > > > >> explicit values. It's been a warning in dtc for the root node since the
> > > > >> beginning (2005) and for any parent node since 2007. Of course, not all
> > > > >> FDT uses dtc, but that should be the majority by far. The various
> > > > >> extracted OF devicetrees I have dating back to the 1990s (various
> > > > >> PowerMac, OLPC, PASemi Nemo) all have explicit root node properties.
> > > > >
> > > > > I have various old device trees that have been given to me over the
> > > > > years, and as far as I can tell they all have these properties (some of
> > > > > them are partial trees so it's hard to be 100% sure).
> > > > >
> > > > > So LGTM.
> > > >
> > > > Turns out I was wrong.
> > > >
> > > > The warning about #size-cells hits on some powermacs, possible fixup
> > > > patch here:
> > > >
> > > >   https://lore.kernel.org/linuxppc-dev/20241126025710.591683-1-mpe@ellerman.id.au/
> > >
> > > The Open Firmware specification is extremely clear that a "missing"
> > > "#size-cells" property means this bus has the default value of 1.
> >
> > And the default for #address-cells is 2, but yet every architecture
> > except Sparc has that wrong.
>
> ?
>
> Almost all architectures (that run Linux) use 64-bit addressing, both
> 32-bit and 64-bit architectures.

I'm just telling you what Linux uses for defaults for at least 20 years.

> > If I have a node without #size-cells, is the default of 1 used or do
> > we check parent nodes? My read of the spec would be the former, but
> > the kernel does the latter.
>
> The former is correct.  The latter makes no sense at all!  The whole
> point of the "bus" abstraction is that you get a new addressing domain
> there.

I agree, but that's what the kernel does (again, for 20+ years).
Walking the parents is really what I want to get rid of here. My
choices were drop that behavior and see who I break, or add a warning
and see who notices. I went the nicer route of a warning.

> Yes, these days you numerically find it most often with PCI sub-domains,
> but those are boring.  In most cases you *do* have different adressing
> on your child busses, and even if the addressing is the same, addresses
> on the child bus are not normally a subset of those on the parent bus.
>
> > > https://www.openfirmware.info/data/docs/of1275.pdf (page 186).
> > >
> > > DTC or FDT might want to do things differently, but expecting decades
> > > older stuff to conform to its ill-conceived unnecessarily super wordy
> > > stuff is, well, not a plan that is likely to work very well :-)
> >
> > That is not the intention. The intention is to identify what doesn't
> > conform and exclude those systems from this check (or apply a fixup if
> > that works).
>
> So *always* use the OF definition, at least on OF systems?  Where
> everything is meant to conform, but conform to OF, not conform to this
> "OF-like-but-very-different-in-crucial-spots" thing :-)

I'm pretty sure there are OF systems that don't conform, so it is not
that simple. There's this comment in of_irq_parse_raw() for example:

        /* Look for this #address-cells. We have to implement the old linux
         * trick of looking for the parent here as some device-trees rely on it
         */

Maybe that's from some system long dropped and we don't need it
anymore. I have no idea. That's what I'm trying to find out with this
patch.

We also don't really have a way to distinguish OF from FDT (where we'd
need to). It is somewhat just by arch, but PPC always passes an FDT to
the kernel for both FDT and OF systems.

Rob

Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling

[Herrenschmidt, Benjamin]

On Mon, 2024-12-02 at 16:55 -0600, Rob Herring wrote:
>         /* Look for this #address-cells. We have to implement the old linux
>          * trick of looking for the parent here as some device-trees rely on it
>          */
> 
> Maybe that's from some system long dropped and we don't need it
> anymore. I have no idea. That's what I'm trying to find out with this
> patch.
> 
> We also don't really have a way to distinguish OF from FDT (where we'd
> need to). It is somewhat just by arch, but PPC always passes an FDT to
> the kernel for both FDT and OF systems.

I probably wrote that :-)

The little I can remember, I think is there are many cases of old DTs
(esp. old Apple ones) missing the "empty" ranges property in some nodes
to indicate that the parent and child address spaces are identical.

I honestly don't remember the specifics, but this is something I did
hit a few times in the past.

Cheers,
Ben.