Re: [PATCH v2] of: WARN on deprecated #address-cells/#size-cells handling
From: Rob Herring <robh@kernel.org>
Date: 2024-11-08 13:25:57
Also in:
linux-devicetree, linux-samsung-soc, lkml
Subsystem:
open firmware and flattened device tree, the rest · Maintainers:
Rob Herring, Saravana Kannan, Linus Torvalds
On Fri, Nov 8, 2024 at 5:04 AM Marek Szyprowski [off-list ref] wrote:
Hi Rob, On 06.11.2024 18:10, Rob Herring (Arm) wrote:quoted
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.
quoted hunk ↗ jump to hunk
------------[ 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.dtsib/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(structdevice_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]);