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