[PATCH] ARM: dts: zynq: Add address-cells property to interrupt controllers

[PATCH] ARM: dts: zynq: Add address-cells property to interrupt controllers

[Michal Simek]

The commit 3eb619b2f7d8 ("scripts/dtc: Update to upstream version
v1.6.0-11-g9d7888cbf19c") updated dtc version which also contained DTC
commit
"81e0919a3e21 checks: Add interrupt provider test"
where reasons for this checking are mentioned as
"A missing #address-cells property is less critical, but creates
ambiguities when used in interrupt-map properties, so warn about this as
well now."

Add address-cells property to gic and gpio nodes to get rid of this warning.
The similar change has been done for ZynqMP too.

CC: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 arch/arm/boot/dts/zynq-7000.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
index df9ad831cf05..c4810d58540b 100644
--- a/arch/arm/boot/dts/zynq-7000.dtsi
+++ b/arch/arm/boot/dts/zynq-7000.dtsi
@@ -133,6 +133,7 @@ can1: can@e0009000 {
 
 		gpio0: gpio@e000a000 {
 			compatible = "xlnx,zynq-gpio-1.0";
+			#address-cells = <0>;
 			#gpio-cells = <2>;
 			clocks = <&clkc 42>;
 			gpio-controller;
@@ -168,6 +169,7 @@ i2c1: i2c@e0005000 {
 		intc: interrupt-controller@f8f01000 {
 			compatible = "arm,cortex-a9-gic";
 			#interrupt-cells = <3>;
+			#address-cells = <0>;
 			interrupt-controller;
 			reg = <0xF8F01000 0x1000>,
 			      <0xF8F00100 0x100>;
-- 
2.30.0

Re: [PATCH] ARM: dts: zynq: Add address-cells property to interrupt controllers

[Rob Herring]

On Mon, Feb 1, 2021 at 8:27 AM Michal Simek <michal.simek@xilinx.com> wrote:
>
> The commit 3eb619b2f7d8 ("scripts/dtc: Update to upstream version
> v1.6.0-11-g9d7888cbf19c") updated dtc version which also contained DTC
> commit
> "81e0919a3e21 checks: Add interrupt provider test"
> where reasons for this checking are mentioned as
> "A missing #address-cells property is less critical, but creates
> ambiguities when used in interrupt-map properties, so warn about this as
> well now."
>
> Add address-cells property to gic and gpio nodes to get rid of this warning.
> The similar change has been done for ZynqMP too.

FYI, we're going to make this check dependent on having an
interrupt-map property. So adding these isn't necessary.

>
> CC: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
>  arch/arm/boot/dts/zynq-7000.dtsi | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
> index df9ad831cf05..c4810d58540b 100644
> --- a/arch/arm/boot/dts/zynq-7000.dtsi
> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> @@ -133,6 +133,7 @@ can1: can@e0009000 {
>
>                 gpio0: gpio@e000a000 {
>                         compatible = "xlnx,zynq-gpio-1.0";
> +                       #address-cells = <0>;
>                         #gpio-cells = <2>;
>                         clocks = <&clkc 42>;
>                         gpio-controller;
> @@ -168,6 +169,7 @@ i2c1: i2c@e0005000 {
>                 intc: interrupt-controller@f8f01000 {
>                         compatible = "arm,cortex-a9-gic";
>                         #interrupt-cells = <3>;
> +                       #address-cells = <0>;
>                         interrupt-controller;
>                         reg = <0xF8F01000 0x1000>,
>                               <0xF8F00100 0x100>;
> --
> 2.30.0
>

Re: [PATCH] ARM: dts: zynq: Add address-cells property to interrupt controllers

[Michal Simek]

On 2/1/21 6:41 PM, Rob Herring wrote:
> On Mon, Feb 1, 2021 at 8:27 AM Michal Simek <michal.simek@xilinx.com> wrote:
>>
>> The commit 3eb619b2f7d8 ("scripts/dtc: Update to upstream version
>> v1.6.0-11-g9d7888cbf19c") updated dtc version which also contained DTC
>> commit
>> "81e0919a3e21 checks: Add interrupt provider test"
>> where reasons for this checking are mentioned as
>> "A missing #address-cells property is less critical, but creates
>> ambiguities when used in interrupt-map properties, so warn about this as
>> well now."
>>
>> Add address-cells property to gic and gpio nodes to get rid of this warning.
>> The similar change has been done for ZynqMP too.
> 
> FYI, we're going to make this check dependent on having an
> interrupt-map property. So adding these isn't necessary.

Good to know. Is there going to be report if interrupt-map doesn't
exist? Which can end up with reverting these changes?

Thanks,
Michal

Re: [PATCH] ARM: dts: zynq: Add address-cells property to interrupt controllers

[Rob Herring]

On Wed, Feb 3, 2021 at 1:01 AM Michal Simek <michal.simek@xilinx.com> wrote:
>
>
>
> On 2/1/21 6:41 PM, Rob Herring wrote:
> > On Mon, Feb 1, 2021 at 8:27 AM Michal Simek <michal.simek@xilinx.com> wrote:
> >>
> >> The commit 3eb619b2f7d8 ("scripts/dtc: Update to upstream version
> >> v1.6.0-11-g9d7888cbf19c") updated dtc version which also contained DTC
> >> commit
> >> "81e0919a3e21 checks: Add interrupt provider test"
> >> where reasons for this checking are mentioned as
> >> "A missing #address-cells property is less critical, but creates
> >> ambiguities when used in interrupt-map properties, so warn about this as
> >> well now."
> >>
> >> Add address-cells property to gic and gpio nodes to get rid of this warning.
> >> The similar change has been done for ZynqMP too.
> >
> > FYI, we're going to make this check dependent on having an
> > interrupt-map property. So adding these isn't necessary.
>
> Good to know. Is there going to be report if interrupt-map doesn't
> exist? Which can end up with reverting these changes?

You mean a warning if '#address-cells' is present and interrupt-map is
not? No, that would cause lots of warnings.

Rob

Re: [PATCH] ARM: dts: zynq: Add address-cells property to interrupt controllers

[Michal Simek]

On 2/3/21 3:12 PM, Rob Herring wrote:
> On Wed, Feb 3, 2021 at 1:01 AM Michal Simek <michal.simek@xilinx.com> wrote:
>>
>>
>>
>> On 2/1/21 6:41 PM, Rob Herring wrote:
>>> On Mon, Feb 1, 2021 at 8:27 AM Michal Simek <michal.simek@xilinx.com> wrote:
>>>>
>>>> The commit 3eb619b2f7d8 ("scripts/dtc: Update to upstream version
>>>> v1.6.0-11-g9d7888cbf19c") updated dtc version which also contained DTC
>>>> commit
>>>> "81e0919a3e21 checks: Add interrupt provider test"
>>>> where reasons for this checking are mentioned as
>>>> "A missing #address-cells property is less critical, but creates
>>>> ambiguities when used in interrupt-map properties, so warn about this as
>>>> well now."
>>>>
>>>> Add address-cells property to gic and gpio nodes to get rid of this warning.
>>>> The similar change has been done for ZynqMP too.
>>>
>>> FYI, we're going to make this check dependent on having an
>>> interrupt-map property. So adding these isn't necessary.
>>
>> Good to know. Is there going to be report if interrupt-map doesn't
>> exist? Which can end up with reverting these changes?
> 
> You mean a warning if '#address-cells' is present and interrupt-map is
> not? No, that would cause lots of warnings.

yep. What's the timeline for this? I mean I want to get to state that
all current warnings are gone that it will be much easier to add stuff
which we have in soc tree.

Thanks,
Michal

Re: [PATCH] ARM: dts: zynq: Add address-cells property to interrupt controllers

[Andre Przywara]

On Wed, 3 Feb 2021 15:15:19 +0100
Michal Simek <michal.simek@xilinx.com> wrote:

> On 2/3/21 3:12 PM, Rob Herring wrote:
> > On Wed, Feb 3, 2021 at 1:01 AM Michal Simek <michal.simek@xilinx.com> wrote:  
> >>
> >>
> >>
> >> On 2/1/21 6:41 PM, Rob Herring wrote:  
> >>> On Mon, Feb 1, 2021 at 8:27 AM Michal Simek <michal.simek@xilinx.com> wrote:  
> >>>>
> >>>> The commit 3eb619b2f7d8 ("scripts/dtc: Update to upstream version
> >>>> v1.6.0-11-g9d7888cbf19c") updated dtc version which also contained DTC
> >>>> commit
> >>>> "81e0919a3e21 checks: Add interrupt provider test"
> >>>> where reasons for this checking are mentioned as
> >>>> "A missing #address-cells property is less critical, but creates
> >>>> ambiguities when used in interrupt-map properties, so warn about this as
> >>>> well now."
> >>>>
> >>>> Add address-cells property to gic and gpio nodes to get rid of this warning.
> >>>> The similar change has been done for ZynqMP too.  
> >>>
> >>> FYI, we're going to make this check dependent on having an
> >>> interrupt-map property. So adding these isn't necessary.  
> >>
> >> Good to know. Is there going to be report if interrupt-map doesn't
> >> exist? Which can end up with reverting these changes?  
> > 
> > You mean a warning if '#address-cells' is present and interrupt-map is
> > not? No, that would cause lots of warnings.  
> 
> yep. 

Why would we do that? That sounds dangerous and would be broken if the
IRQ controller is in a generic .dtsi (as it usually is), but the
interrupt map is only in *some* of the board .dts files.

What is the problem of just putting #address-cells = <0>; in the
IRQ controller node, after checking that there currently no interrupt
maps in use and no IRQ children? And be safe for good? That's 16 bytes
in the DTB, IIUC.

Because otherwise we have that lovely ambiguity between the
implicit default #address-cells = 2; and the assumed default of 0.

And that's why I think we also cannot *automatically* add an #ac = <0>;
property, because that would change behaviour.

Cheers,
Andre


> What's the timeline for this? I mean I want to get to state that
> all current warnings are gone that it will be much easier to add stuff
> which we have in soc tree.
> 
> Thanks,
> Michal
> 

Re: [PATCH] ARM: dts: zynq: Add address-cells property to interrupt controllers

[Rob Herring]

On Wed, Feb 3, 2021 at 8:44 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Wed, 3 Feb 2021 15:15:19 +0100
> Michal Simek <michal.simek@xilinx.com> wrote:
>
> > On 2/3/21 3:12 PM, Rob Herring wrote:
> > > On Wed, Feb 3, 2021 at 1:01 AM Michal Simek <michal.simek@xilinx.com> wrote:
> > >>
> > >>
> > >>
> > >> On 2/1/21 6:41 PM, Rob Herring wrote:
> > >>> On Mon, Feb 1, 2021 at 8:27 AM Michal Simek <michal.simek@xilinx.com> wrote:
> > >>>>
> > >>>> The commit 3eb619b2f7d8 ("scripts/dtc: Update to upstream version
> > >>>> v1.6.0-11-g9d7888cbf19c") updated dtc version which also contained DTC
> > >>>> commit
> > >>>> "81e0919a3e21 checks: Add interrupt provider test"
> > >>>> where reasons for this checking are mentioned as
> > >>>> "A missing #address-cells property is less critical, but creates
> > >>>> ambiguities when used in interrupt-map properties, so warn about this as
> > >>>> well now."
> > >>>>
> > >>>> Add address-cells property to gic and gpio nodes to get rid of this warning.
> > >>>> The similar change has been done for ZynqMP too.
> > >>>
> > >>> FYI, we're going to make this check dependent on having an
> > >>> interrupt-map property. So adding these isn't necessary.
> > >>
> > >> Good to know. Is there going to be report if interrupt-map doesn't
> > >> exist? Which can end up with reverting these changes?
> > >
> > > You mean a warning if '#address-cells' is present and interrupt-map is
> > > not? No, that would cause lots of warnings.
> >
> > yep.
>
> Why would we do that? That sounds dangerous and would be broken if the
> IRQ controller is in a generic .dtsi (as it usually is), but the
> interrupt map is only in *some* of the board .dts files.
>
> What is the problem of just putting #address-cells = <0>; in the
> IRQ controller node, after checking that there currently no interrupt
> maps in use and no IRQ children? And be safe for good? That's 16 bytes
> in the DTB, IIUC.

Because I don't think we need a bunch of warning fix patches to add
these everywhere. Also, the need for #address-cells pretty much makes
no sense on any modern system. It is a relic from days when the bus
(address) topology and interrupt topology were related.

> Because otherwise we have that lovely ambiguity between the
> implicit default #address-cells = 2; and the assumed default of 0.
>
> And that's why I think we also cannot *automatically* add an #ac = <0>;
> property, because that would change behaviour.

I'd rather try to limit where we assume the default of 2. My guess is
that's only some combination of old PowerPC and/or Sparc and no FDT
based DT.

Rob

Re: [PATCH] ARM: dts: zynq: Add address-cells property to interrupt controllers

[Rob Herring]

On Wed, Feb 3, 2021 at 10:49 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Wed, Feb 3, 2021 at 8:44 AM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > On Wed, 3 Feb 2021 15:15:19 +0100
> > Michal Simek <michal.simek@xilinx.com> wrote:
> >
> > > On 2/3/21 3:12 PM, Rob Herring wrote:
> > > > On Wed, Feb 3, 2021 at 1:01 AM Michal Simek <michal.simek@xilinx.com> wrote:
> > > >>
> > > >>
> > > >>
> > > >> On 2/1/21 6:41 PM, Rob Herring wrote:
> > > >>> On Mon, Feb 1, 2021 at 8:27 AM Michal Simek <michal.simek@xilinx.com> wrote:
> > > >>>>
> > > >>>> The commit 3eb619b2f7d8 ("scripts/dtc: Update to upstream version
> > > >>>> v1.6.0-11-g9d7888cbf19c") updated dtc version which also contained DTC
> > > >>>> commit
> > > >>>> "81e0919a3e21 checks: Add interrupt provider test"
> > > >>>> where reasons for this checking are mentioned as
> > > >>>> "A missing #address-cells property is less critical, but creates
> > > >>>> ambiguities when used in interrupt-map properties, so warn about this as
> > > >>>> well now."
> > > >>>>
> > > >>>> Add address-cells property to gic and gpio nodes to get rid of this warning.
> > > >>>> The similar change has been done for ZynqMP too.
> > > >>>
> > > >>> FYI, we're going to make this check dependent on having an
> > > >>> interrupt-map property. So adding these isn't necessary.
> > > >>
> > > >> Good to know. Is there going to be report if interrupt-map doesn't
> > > >> exist? Which can end up with reverting these changes?
> > > >
> > > > You mean a warning if '#address-cells' is present and interrupt-map is
> > > > not? No, that would cause lots of warnings.
> > >
> > > yep.
> >
> > Why would we do that? That sounds dangerous and would be broken if the
> > IRQ controller is in a generic .dtsi (as it usually is), but the
> > interrupt map is only in *some* of the board .dts files.
> >
> > What is the problem of just putting #address-cells = <0>; in the
> > IRQ controller node, after checking that there currently no interrupt
> > maps in use and no IRQ children? And be safe for good? That's 16 bytes
> > in the DTB, IIUC.
>
> Because I don't think we need a bunch of warning fix patches to add
> these everywhere. Also, the need for #address-cells pretty much makes
> no sense on any modern system. It is a relic from days when the bus
> (address) topology and interrupt topology were related.
>
> > Because otherwise we have that lovely ambiguity between the
> > implicit default #address-cells = 2; and the assumed default of 0.
> >
> > And that's why I think we also cannot *automatically* add an #ac = <0>;
> > property, because that would change behaviour.
>
> I'd rather try to limit where we assume the default of 2. My guess is
> that's only some combination of old PowerPC and/or Sparc and no FDT
> based DT.

Actually, after reviewing of_irq_parse_raw() again, I think you're
mixing the 2 different #address-cells involved. Let's review which
#*-cells applies to parts of interrupt-map:

interrupt-map = <[ac current node or parent] [ic current node] [parent
intc phandle] [ac parent intc] [ic parent intc]>;

For [ac current node or parent], we start in the 'interrupt-map' node
(because it's the interrupt parent). From there, we walk up the tree
to find #address-cells. Worst case is we find none and take the
default of 2. First, dtc has pretty much always made no root
#address-cells a warning. Second, Linux has notion of a default and
that varies by arch and isn't used here. Only Sparc defaults to 2 (see
of_private.h) which means we should never hit the default on PowerPC
or Arm (or anything else).

The #address-cells the fix here addresses is the [parent intc
phandle]'s for [ac parent intc] cells. This default is 0 (see
newaddrsize in of_irq_parse_raw()). So really, we only need to be
checking for #address-cells in nodes with interrupt-map.

Rob

Re: [PATCH] ARM: dts: zynq: Add address-cells property to interrupt controllers

[Rob Herring]

On Wed, Feb 3, 2021 at 12:03 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Wed, Feb 3, 2021 at 10:49 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Wed, Feb 3, 2021 at 8:44 AM Andre Przywara <andre.przywara@arm.com> wrote:
> > >
> > > On Wed, 3 Feb 2021 15:15:19 +0100
> > > Michal Simek <michal.simek@xilinx.com> wrote:
> > >
> > > > On 2/3/21 3:12 PM, Rob Herring wrote:
> > > > > On Wed, Feb 3, 2021 at 1:01 AM Michal Simek <michal.simek@xilinx.com> wrote:
> > > > >>
> > > > >>
> > > > >>
> > > > >> On 2/1/21 6:41 PM, Rob Herring wrote:
> > > > >>> On Mon, Feb 1, 2021 at 8:27 AM Michal Simek <michal.simek@xilinx.com> wrote:
> > > > >>>>
> > > > >>>> The commit 3eb619b2f7d8 ("scripts/dtc: Update to upstream version
> > > > >>>> v1.6.0-11-g9d7888cbf19c") updated dtc version which also contained DTC
> > > > >>>> commit
> > > > >>>> "81e0919a3e21 checks: Add interrupt provider test"
> > > > >>>> where reasons for this checking are mentioned as
> > > > >>>> "A missing #address-cells property is less critical, but creates
> > > > >>>> ambiguities when used in interrupt-map properties, so warn about this as
> > > > >>>> well now."
> > > > >>>>
> > > > >>>> Add address-cells property to gic and gpio nodes to get rid of this warning.
> > > > >>>> The similar change has been done for ZynqMP too.
> > > > >>>
> > > > >>> FYI, we're going to make this check dependent on having an
> > > > >>> interrupt-map property. So adding these isn't necessary.
> > > > >>
> > > > >> Good to know. Is there going to be report if interrupt-map doesn't
> > > > >> exist? Which can end up with reverting these changes?
> > > > >
> > > > > You mean a warning if '#address-cells' is present and interrupt-map is
> > > > > not? No, that would cause lots of warnings.
> > > >
> > > > yep.
> > >
> > > Why would we do that? That sounds dangerous and would be broken if the
> > > IRQ controller is in a generic .dtsi (as it usually is), but the
> > > interrupt map is only in *some* of the board .dts files.
> > >
> > > What is the problem of just putting #address-cells = <0>; in the
> > > IRQ controller node, after checking that there currently no interrupt
> > > maps in use and no IRQ children? And be safe for good? That's 16 bytes
> > > in the DTB, IIUC.
> >
> > Because I don't think we need a bunch of warning fix patches to add
> > these everywhere. Also, the need for #address-cells pretty much makes
> > no sense on any modern system. It is a relic from days when the bus
> > (address) topology and interrupt topology were related.
> >
> > > Because otherwise we have that lovely ambiguity between the
> > > implicit default #address-cells = 2; and the assumed default of 0.
> > >
> > > And that's why I think we also cannot *automatically* add an #ac = <0>;
> > > property, because that would change behaviour.
> >
> > I'd rather try to limit where we assume the default of 2. My guess is
> > that's only some combination of old PowerPC and/or Sparc and no FDT
> > based DT.
>
> Actually, after reviewing of_irq_parse_raw() again, I think you're
> mixing the 2 different #address-cells involved. Let's review which
> #*-cells applies to parts of interrupt-map:
>
> interrupt-map = <[ac current node or parent] [ic current node] [parent
> intc phandle] [ac parent intc] [ic parent intc]>;
>
> For [ac current node or parent], we start in the 'interrupt-map' node
> (because it's the interrupt parent). From there, we walk up the tree
> to find #address-cells. Worst case is we find none and take the
> default of 2. First, dtc has pretty much always made no root
> #address-cells a warning. Second, Linux has notion of a default and
> that varies by arch and isn't used here. Only Sparc defaults to 2 (see
> of_private.h) which means we should never hit the default on PowerPC
> or Arm (or anything else).

Actually, Sparc doesn't even use this code. Turns out PowerPC is a bit
more complicated.

I traced where the '2' in this code came from. PowerPC had a mixture
of the default being 1 or 2. For the interrupt parsing code, it was 1
(from prom_n_addr_cells()) before commit 0ebfff1491ef and 2
(hardcoded) after it. That's not the only place that a default was
set. The early_init_dt_scan_root() function at that time defaulted to
2. Now it's 1 as we added per arch default defines which used the '1'
from prom_n_addr_cells() (now of_n_addr_cells()). So in conclusion,
PowerPC has had a mixture of defaults and no one cared since 2006 when
it changed. I'm inclined to rip out these defaults and just fail.

Rob