From: Michal Simek <hidden> Date: 2021-02-01 14:29:13
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 <redacted>
---
arch/arm/boot/dts/zynq-7000.dtsi | 2 ++
1 file changed, 2 insertions(+)
From: Rob Herring <robh+dt@kernel.org> Date: 2021-02-01 17:43:01
On Mon, Feb 1, 2021 at 8:27 AM Michal Simek [off-list ref] 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.
quoted hunk
CC: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Michal Simek <redacted>
---
arch/arm/boot/dts/zynq-7000.dtsi | 2 ++
1 file changed, 2 insertions(+)
From: Michal Simek <hidden> Date: 2021-02-03 07:02:26
On 2/1/21 6:41 PM, Rob Herring wrote:
On Mon, Feb 1, 2021 at 8:27 AM Michal Simek [off-list ref] wrote:
quoted
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
From: Rob Herring <robh+dt@kernel.org> Date: 2021-02-03 14:13:46
On Wed, Feb 3, 2021 at 1:01 AM Michal Simek [off-list ref] wrote:
On 2/1/21 6:41 PM, Rob Herring wrote:
quoted
On Mon, Feb 1, 2021 at 8:27 AM Michal Simek [off-list ref] wrote:
quoted
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
From: Michal Simek <hidden> Date: 2021-02-03 14:17:02
On 2/3/21 3:12 PM, Rob Herring wrote:
On Wed, Feb 3, 2021 at 1:01 AM Michal Simek [off-list ref] wrote:
quoted
On 2/1/21 6:41 PM, Rob Herring wrote:
quoted
On Mon, Feb 1, 2021 at 8:27 AM Michal Simek [off-list ref] wrote:
quoted
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
From: Andre Przywara <andre.przywara@arm.com> Date: 2021-02-03 14:45:45
On Wed, 3 Feb 2021 15:15:19 +0100
Michal Simek [off-list ref] wrote:
On 2/3/21 3:12 PM, Rob Herring wrote:
quoted
On Wed, Feb 3, 2021 at 1:01 AM Michal Simek [off-list ref] wrote:
quoted
On 2/1/21 6:41 PM, Rob Herring wrote:
quoted
On Mon, Feb 1, 2021 at 8:27 AM Michal Simek [off-list ref] wrote:
quoted
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
From: Rob Herring <robh+dt@kernel.org> Date: 2021-02-03 16:50:45
On Wed, Feb 3, 2021 at 8:44 AM Andre Przywara [off-list ref] wrote:
On Wed, 3 Feb 2021 15:15:19 +0100
Michal Simek [off-list ref] wrote:
quoted
On 2/3/21 3:12 PM, Rob Herring wrote:
quoted
On Wed, Feb 3, 2021 at 1:01 AM Michal Simek [off-list ref] wrote:
quoted
On 2/1/21 6:41 PM, Rob Herring wrote:
quoted
On Mon, Feb 1, 2021 at 8:27 AM Michal Simek [off-list ref] wrote:
quoted
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
From: Rob Herring <robh+dt@kernel.org> Date: 2021-02-03 18:05:26
On Wed, Feb 3, 2021 at 10:49 AM Rob Herring [off-list ref] wrote:
On Wed, Feb 3, 2021 at 8:44 AM Andre Przywara [off-list ref] wrote:
quoted
On Wed, 3 Feb 2021 15:15:19 +0100
Michal Simek [off-list ref] wrote:
quoted
On 2/3/21 3:12 PM, Rob Herring wrote:
quoted
On Wed, Feb 3, 2021 at 1:01 AM Michal Simek [off-list ref] wrote:
quoted
On 2/1/21 6:41 PM, Rob Herring wrote:
quoted
On Mon, Feb 1, 2021 at 8:27 AM Michal Simek [off-list ref] wrote:
quoted
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.
quoted
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
From: Rob Herring <robh+dt@kernel.org> Date: 2021-02-04 20:20:31
On Wed, Feb 3, 2021 at 12:03 PM Rob Herring [off-list ref] wrote:
On Wed, Feb 3, 2021 at 10:49 AM Rob Herring [off-list ref] wrote:
quoted
On Wed, Feb 3, 2021 at 8:44 AM Andre Przywara [off-list ref] wrote:
quoted
On Wed, 3 Feb 2021 15:15:19 +0100
Michal Simek [off-list ref] wrote:
quoted
On 2/3/21 3:12 PM, Rob Herring wrote:
quoted
On Wed, Feb 3, 2021 at 1:01 AM Michal Simek [off-list ref] wrote:
quoted
On 2/1/21 6:41 PM, Rob Herring wrote:
quoted
On Mon, Feb 1, 2021 at 8:27 AM Michal Simek [off-list ref] wrote:
quoted
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.
quoted
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