| Message ID | 91e3405245c89f134676449cf3822285798d2ed2.1612189652.git.michal.simek@xilinx.com |
|---|---|
| State | New |
| Headers | show |
| Series | ARM: dts: zynq: Add address-cells property to interrupt controllers | expand |
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
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
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
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 >
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
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
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
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>;
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(+)