On Fri, Nov 8, 2024 at 5:04 AM Marek Szyprowski
<m.szyprowski@xxxxxxxxxxx> 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@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):
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]);
