[PATCH 0/2] Fix dtc warnings when building the LAN966x device tree overlay

[PATCH 0/2] Fix dtc warnings when building the LAN966x device tree overlay

[Herve Codina]

dtc generates 3 kinds of warnings when it builds the LAN966x dtso.

- missing or empty reg/ranges property
    .../pci-ep-bus@0/cpu_clk: missing or empty reg/ranges property
    .../pci-ep-bus@0/ddr_clk: missing or empty reg/ranges property
    .../pci-ep-bus@0/sys_clk: missing or empty reg/ranges property
  Patch 1 in this series fixes these warnings

- Missing interrupt-parent
   .../pci-ep-bus@0/oic@e00c0120: Missing interrupt-parent
  This warning was quickly silenced by Philipp [1].
  Patch 2 in this series fixes the warning and should replace the patch
  applied by Philipp to silence the warning.

- Warning (avoid_unnecessary_addr_size)
   /fragment@0/__overlay__: unnecessary #address-cells/#size-cells without "ranges", "dma-ranges" or child "reg" property
  This warning should be fixed in dtc.
  A patch has already be sent by Philip to fix it [2].

[1] https://lore.kernel.org/all/57793bb01e02f03e215dfa6f8783df18034ae2ea.camel@pengutronix.de/
[2] https://lore.kernel.org/devicetree-compiler/20241025161307.3629901-1-p.zabel@pengutronix.de/T/#u

Best regards,
Hervé

Herve Codina (2):
  misc: lan966x_pci: Fix dtc warns 'missing or empty reg/ranges
    property'
  misc: lan966x_pci: Fix dtc warn 'Missing interrupt-parent'

 drivers/misc/lan966x_pci.dtso | 46 +++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 18 deletions(-)

-- 
2.46.2

[PATCH 1/2] misc: lan966x_pci: Fix dtc warns 'missing or empty reg/ranges property'

[Herve Codina]

dtc generates the following warnings when building the LAN966x device
tree overlay (lan966x_pci.dtso):
  Warning (simple_bus_reg): /fragment@0/__overlay__/pci-ep-bus@0/cpu_clk: missing or empty reg/ranges property
  Warning (simple_bus_reg): /fragment@0/__overlay__/pci-ep-bus@0/ddr_clk: missing or empty reg/ranges property
  Warning (simple_bus_reg): /fragment@0/__overlay__/pci-ep-bus@0/sys_clk: missing or empty reg/ranges property

Indeed, related nodes are under the pci-ep-bus (simple-bus) which is not
correct.

Put them outside this node.

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Closes: https://lore.kernel.org/all/20241025110919.64b1cffb@canb.auug.org.au/
Fixes: 185686beb464 ("misc: Add support for LAN966x PCI device")
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
The referenced commit is in the reset tree
---
 drivers/misc/lan966x_pci.dtso | 36 +++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/misc/lan966x_pci.dtso b/drivers/misc/lan966x_pci.dtso
index 7282687df25f..5466d013da7d 100644
--- a/drivers/misc/lan966x_pci.dtso
+++ b/drivers/misc/lan966x_pci.dtso
@@ -19,6 +19,24 @@ __overlay__ {
 			#address-cells = <3>;
 			#size-cells = <2>;
 
+			cpu_clk: cpu_clk {
+				compatible = "fixed-clock";
+				#clock-cells = <0>;
+				clock-frequency = <600000000>;  // CPU clock = 600MHz
+			};
+
+			ddr_clk: ddr_clk {
+				compatible = "fixed-clock";
+				#clock-cells = <0>;
+				clock-frequency = <30000000>;  // Fabric clock = 30MHz
+			};
+
+			sys_clk: sys_clk {
+				compatible = "fixed-clock";
+				#clock-cells = <0>;
+				clock-frequency = <15625000>;  // System clock = 15.625MHz
+			};
+
 			pci-ep-bus@0 {
 				compatible = "simple-bus";
 				#address-cells = <1>;
@@ -39,24 +57,6 @@ oic: oic@e00c0120 {
 					reg = <0xe00c0120 0x190>;
 				};
 
-				cpu_clk: cpu_clk {
-					compatible = "fixed-clock";
-					#clock-cells = <0>;
-					clock-frequency = <600000000>;  // CPU clock = 600MHz
-				};
-
-				ddr_clk: ddr_clk {
-					compatible = "fixed-clock";
-					#clock-cells = <0>;
-					clock-frequency = <30000000>;  // Fabric clock = 30MHz
-				};
-
-				sys_clk: sys_clk {
-					compatible = "fixed-clock";
-					#clock-cells = <0>;
-					clock-frequency = <15625000>;  // System clock = 15.625MHz
-				};
-
 				cpu_ctrl: syscon@e00c0000 {
 					compatible = "microchip,lan966x-cpu-syscon", "syscon";
 					reg = <0xe00c0000 0xa8>;
-- 
2.46.2

[PATCH 2/2] misc: lan966x_pci: Fix dtc warn 'Missing interrupt-parent'

[Herve Codina]

dtc generates the following warning when building the LAN966x devicetree
overlay (lan966x_pci.dtso):
  Warning (interrupts_property): /fragment@0/__overlay__/pci-ep-bus@0/oic@e00c0120: Missing interrupt-parent

The oic interrupt parent is the PCI device itself. The PCI device node
is the node on which the dtbo will be applied and this node already has
properties needed.

In order to remove the warning, add the missing properties in the
overlay fragment node. Properties in this node will not be added when
the overlay is applied (which is what we expect) but makes dtc happy.

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Closes: https://lore.kernel.org/all/20241025110919.64b1cffb@canb.auug.org.au/
Fixes: 185686beb464 ("misc: Add support for LAN966x PCI device")
Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
The referenced commit is in the reset tree
---
 drivers/misc/lan966x_pci.dtso | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/misc/lan966x_pci.dtso b/drivers/misc/lan966x_pci.dtso
index 5466d013da7d..ed5c35809b2f 100644
--- a/drivers/misc/lan966x_pci.dtso
+++ b/drivers/misc/lan966x_pci.dtso
@@ -15,6 +15,16 @@
 / {
 	fragment@0 {
 		target-path = "";
+
+		/*
+		 * These properties allow to avoid a dtc warnings.
+		 * The real interrupt controller is the PCI device itself. It
+		 * is the node on which the device tree overlay will be applied.
+		 * This node has those properties.
+		 */
+		#interrupt-cells = <1>;
+		interrupt-controller;
+
 		__overlay__ {
 			#address-cells = <3>;
 			#size-cells = <2>;
-- 
2.46.2

Re: [PATCH 1/2] misc: lan966x_pci: Fix dtc warns 'missing or empty reg/ranges property'

[Rob Herring]

On Mon, Oct 28, 2024 at 7:24 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> dtc generates the following warnings when building the LAN966x device
> tree overlay (lan966x_pci.dtso):
>   Warning (simple_bus_reg): /fragment@0/__overlay__/pci-ep-bus@0/cpu_clk: missing or empty reg/ranges property
>   Warning (simple_bus_reg): /fragment@0/__overlay__/pci-ep-bus@0/ddr_clk: missing or empty reg/ranges property
>   Warning (simple_bus_reg): /fragment@0/__overlay__/pci-ep-bus@0/sys_clk: missing or empty reg/ranges property
>
> Indeed, related nodes are under the pci-ep-bus (simple-bus) which is not
> correct.
>
> Put them outside this node.
>
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Closes: https://lore.kernel.org/all/20241025110919.64b1cffb@canb.auug.org.au/
> Fixes: 185686beb464 ("misc: Add support for LAN966x PCI device")
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
> The referenced commit is in the reset tree
> ---
>  drivers/misc/lan966x_pci.dtso | 36 +++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/misc/lan966x_pci.dtso b/drivers/misc/lan966x_pci.dtso
> index 7282687df25f..5466d013da7d 100644
> --- a/drivers/misc/lan966x_pci.dtso
> +++ b/drivers/misc/lan966x_pci.dtso
> @@ -19,6 +19,24 @@ __overlay__ {
>                         #address-cells = <3>;
>                         #size-cells = <2>;
>
> +                       cpu_clk: cpu_clk {

Preferred node name is "clock-<freq-in-hz>"

Also, as a general rule, don't use "_" in node names (and properties).

Isn't there a schema for the device which needs these nodes added to
it? If not, there should be.

Rob

Re: [PATCH 2/2] misc: lan966x_pci: Fix dtc warn 'Missing interrupt-parent'

[Rob Herring]

On Mon, Oct 28, 2024 at 7:24 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> dtc generates the following warning when building the LAN966x devicetree
> overlay (lan966x_pci.dtso):
>   Warning (interrupts_property): /fragment@0/__overlay__/pci-ep-bus@0/oic@e00c0120: Missing interrupt-parent
>
> The oic interrupt parent is the PCI device itself. The PCI device node
> is the node on which the dtbo will be applied and this node already has
> properties needed.
>
> In order to remove the warning, add the missing properties in the
> overlay fragment node. Properties in this node will not be added when
> the overlay is applied (which is what we expect) but makes dtc happy.
>
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Closes: https://lore.kernel.org/all/20241025110919.64b1cffb@canb.auug.org.au/
> Fixes: 185686beb464 ("misc: Add support for LAN966x PCI device")
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
> The referenced commit is in the reset tree
> ---
>  drivers/misc/lan966x_pci.dtso | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH 1/2] misc: lan966x_pci: Fix dtc warns 'missing or empty reg/ranges property'

[Herve Codina]

Hi Rob,

On Mon, 28 Oct 2024 08:55:24 -0500
Rob Herring <robh@kernel.org> wrote:

> On Mon, Oct 28, 2024 at 7:24 AM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > dtc generates the following warnings when building the LAN966x device
> > tree overlay (lan966x_pci.dtso):
> >   Warning (simple_bus_reg): /fragment@0/__overlay__/pci-ep-bus@0/cpu_clk: missing or empty reg/ranges property
> >   Warning (simple_bus_reg): /fragment@0/__overlay__/pci-ep-bus@0/ddr_clk: missing or empty reg/ranges property
> >   Warning (simple_bus_reg): /fragment@0/__overlay__/pci-ep-bus@0/sys_clk: missing or empty reg/ranges property
> >
> > Indeed, related nodes are under the pci-ep-bus (simple-bus) which is not
> > correct.
> >
> > Put them outside this node.
> >
> > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > Closes: https://lore.kernel.org/all/20241025110919.64b1cffb@canb.auug.org.au/
> > Fixes: 185686beb464 ("misc: Add support for LAN966x PCI device")
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> > The referenced commit is in the reset tree
> > ---
> >  drivers/misc/lan966x_pci.dtso | 36 +++++++++++++++++------------------
> >  1 file changed, 18 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/misc/lan966x_pci.dtso b/drivers/misc/lan966x_pci.dtso
> > index 7282687df25f..5466d013da7d 100644
> > --- a/drivers/misc/lan966x_pci.dtso
> > +++ b/drivers/misc/lan966x_pci.dtso
> > @@ -19,6 +19,24 @@ __overlay__ {
> >                         #address-cells = <3>;
> >                         #size-cells = <2>;
> >
> > +                       cpu_clk: cpu_clk {  
> 
> Preferred node name is "clock-<freq-in-hz>"

I based the name on the lan966x.dtsi
https://elixir.bootlin.com/linux/v6.12-rc1/source/arch/arm/boot/dts/microchip/lan966x.dtsi#L38

Of course, I can rename the cpu_clk, ddr_clk and sys_clk nodes but this will create
a difference against lan966x.dtsi on some points that should be identical.

Let me know with that in mind if I need to rename those nodes in this series.

> 
> Also, as a general rule, don't use "_" in node names (and properties).
> 
> Isn't there a schema for the device which needs these nodes added to
> it? If not, there should be.
> 

No, there is no schema yet for this device.

How can we describe schema for this kind of devices that are using
device-tree overlays?
I mean, this overlay is applied on a PCI device DT node. This DT node is
computed at runtime. It is, in the end, available in the base DT before
applying the overlay.
The compatible string that could be used to check the dtso against schema
cannot be set in the overlay (at least not at the correct place in the
hierarchy) without causing a property memory leak at runtime. An overlay
cannot add a property in a base DT node without generating a memory leak
and so, we avoid adding such properties in the base DT from the overlay.

Is this missing schema blocking for this series ?

Best regards,
Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/2] misc: lan966x_pci: Fix dtc warns 'missing or empty reg/ranges property'

[Rob Herring]

On Mon, Oct 28, 2024 at 12:43 PM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Hi Rob,
>
> On Mon, 28 Oct 2024 08:55:24 -0500
> Rob Herring <robh@kernel.org> wrote:
>
> > On Mon, Oct 28, 2024 at 7:24 AM Herve Codina <herve.codina@bootlin.com> wrote:
> > >
> > > dtc generates the following warnings when building the LAN966x device
> > > tree overlay (lan966x_pci.dtso):
> > >   Warning (simple_bus_reg): /fragment@0/__overlay__/pci-ep-bus@0/cpu_clk: missing or empty reg/ranges property
> > >   Warning (simple_bus_reg): /fragment@0/__overlay__/pci-ep-bus@0/ddr_clk: missing or empty reg/ranges property
> > >   Warning (simple_bus_reg): /fragment@0/__overlay__/pci-ep-bus@0/sys_clk: missing or empty reg/ranges property
> > >
> > > Indeed, related nodes are under the pci-ep-bus (simple-bus) which is not
> > > correct.
> > >
> > > Put them outside this node.
> > >
> > > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > > Closes: https://lore.kernel.org/all/20241025110919.64b1cffb@canb.auug.org.au/
> > > Fixes: 185686beb464 ("misc: Add support for LAN966x PCI device")
> > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > ---
> > > The referenced commit is in the reset tree
> > > ---
> > >  drivers/misc/lan966x_pci.dtso | 36 +++++++++++++++++------------------
> > >  1 file changed, 18 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/misc/lan966x_pci.dtso b/drivers/misc/lan966x_pci.dtso
> > > index 7282687df25f..5466d013da7d 100644
> > > --- a/drivers/misc/lan966x_pci.dtso
> > > +++ b/drivers/misc/lan966x_pci.dtso
> > > @@ -19,6 +19,24 @@ __overlay__ {
> > >                         #address-cells = <3>;
> > >                         #size-cells = <2>;
> > >
> > > +                       cpu_clk: cpu_clk {
> >
> > Preferred node name is "clock-<freq-in-hz>"
>
> I based the name on the lan966x.dtsi
> https://elixir.bootlin.com/linux/v6.12-rc1/source/arch/arm/boot/dts/microchip/lan966x.dtsi#L38

That should be fixed too.

> Of course, I can rename the cpu_clk, ddr_clk and sys_clk nodes but this will create
> a difference against lan966x.dtsi on some points that should be identical.

Then maybe they should be sharing a .dtsi?

> Let me know with that in mind if I need to rename those nodes in this series.

Yes, easier now than later.

> > Also, as a general rule, don't use "_" in node names (and properties).
> >
> > Isn't there a schema for the device which needs these nodes added to
> > it? If not, there should be.
> >
>
> No, there is no schema yet for this device.
>
> How can we describe schema for this kind of devices that are using
> device-tree overlays?

Describing is not the issue. Running the checks is. Though you can run
the checks at runtime.

> I mean, this overlay is applied on a PCI device DT node. This DT node is
> computed at runtime. It is, in the end, available in the base DT before
> applying the overlay.
> The compatible string that could be used to check the dtso against schema
> cannot be set in the overlay (at least not at the correct place in the
> hierarchy) without causing a property memory leak at runtime. An overlay
> cannot add a property in a base DT node without generating a memory leak
> and so, we avoid adding such properties in the base DT from the overlay.

That's a problem with overlays in general which we need to solve at some point.

> Is this missing schema blocking for this series ?

No.

Rob

Re: [PATCH 1/2] misc: lan966x_pci: Fix dtc warns 'missing or empty reg/ranges property'

[Herve Codina]

On Mon, 28 Oct 2024 14:52:09 -0500
Rob Herring <robh@kernel.org> wrote:

> On Mon, Oct 28, 2024 at 12:43 PM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > Hi Rob,
> >
> > On Mon, 28 Oct 2024 08:55:24 -0500
> > Rob Herring <robh@kernel.org> wrote:
> >  
> > > On Mon, Oct 28, 2024 at 7:24 AM Herve Codina <herve.codina@bootlin.com> wrote:  
> > > >
> > > > dtc generates the following warnings when building the LAN966x device
> > > > tree overlay (lan966x_pci.dtso):
> > > >   Warning (simple_bus_reg): /fragment@0/__overlay__/pci-ep-bus@0/cpu_clk: missing or empty reg/ranges property
> > > >   Warning (simple_bus_reg): /fragment@0/__overlay__/pci-ep-bus@0/ddr_clk: missing or empty reg/ranges property
> > > >   Warning (simple_bus_reg): /fragment@0/__overlay__/pci-ep-bus@0/sys_clk: missing or empty reg/ranges property
> > > >
> > > > Indeed, related nodes are under the pci-ep-bus (simple-bus) which is not
> > > > correct.
> > > >
> > > > Put them outside this node.
> > > >
> > > > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > > > Closes: https://lore.kernel.org/all/20241025110919.64b1cffb@canb.auug.org.au/
> > > > Fixes: 185686beb464 ("misc: Add support for LAN966x PCI device")
> > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > > ---
> > > > The referenced commit is in the reset tree
> > > > ---
> > > >  drivers/misc/lan966x_pci.dtso | 36 +++++++++++++++++------------------
> > > >  1 file changed, 18 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/misc/lan966x_pci.dtso b/drivers/misc/lan966x_pci.dtso
> > > > index 7282687df25f..5466d013da7d 100644
> > > > --- a/drivers/misc/lan966x_pci.dtso
> > > > +++ b/drivers/misc/lan966x_pci.dtso
> > > > @@ -19,6 +19,24 @@ __overlay__ {
> > > >                         #address-cells = <3>;
> > > >                         #size-cells = <2>;
> > > >
> > > > +                       cpu_clk: cpu_clk {  
> > >
> > > Preferred node name is "clock-<freq-in-hz>"  
> >
> > I based the name on the lan966x.dtsi
> > https://elixir.bootlin.com/linux/v6.12-rc1/source/arch/arm/boot/dts/microchip/lan966x.dtsi#L38  
> 
> That should be fixed too.
> 
> > Of course, I can rename the cpu_clk, ddr_clk and sys_clk nodes but this will create
> > a difference against lan966x.dtsi on some points that should be identical.  
> 
> Then maybe they should be sharing a .dtsi?
> 
> > Let me know with that in mind if I need to rename those nodes in this series.  
> 
> Yes, easier now than later.

Right, I will rename in v2.

Best regards,
Hervé