On 8/13/25 01:38, Lorenzo Pieralisi wrote:
On Tue, Aug 12, 2025 at 11:59:06AM -0500, Rob Herring wrote:
On Tue, Aug 12, 2025 at 10:53 AM Lizhi Hou <lizhi.hou@xxxxxxx> wrote:
On 8/12/25 00:42, Lorenzo Pieralisi wrote:
On Mon, Aug 11, 2025 at 08:26:11PM -0700, Lizhi Hou wrote:
On 8/11/25 01:42, Lorenzo Pieralisi wrote:
Hi Lizhi, Rob,
while debugging something unrelated I noticed two issues
(related) caused by the automatic generation of device nodes
for PCI bridges.
GICv5 interrupt controller DT top level node [1] does not have a "reg"
property, because it represents the top level node, children (IRSes and ITSs)
are nested.
It does provide #address-cells since it has child nodes, so it has to
have a "ranges" property as well.
You have added code to automatically generate properties for PCI bridges
and in particular this code [2] creates an interrupt-map property for
the PCI bridges (other than the host bridge if it has got an OF node
already).
That code fails on GICv5, because the interrupt controller node does not
have a "reg" property (and AFAIU it does not have to - as a matter of
fact, INTx mapping works on GICv5 with the interrupt-map in the
host bridge node containing zeros in the parent unit interrupt
specifier #address-cells).
Does GICv5 have 'interrupt-controller' but not 'interrupt-map'? I think
of_irq_parse_raw will not check its parent in this case.
But that's not the problem. GICv5 does not have an interrupt-map,
the issue here is that GICv5 _is_ the parent and does not have
a "reg" property. Why does the code in [2] check the reg property
for the parent node while building the interrupt-map property for
the PCI bridge ?
Based on the document, if #address-cells is not zero, it needs to get
parent unit address. Maybe there is way to get the parent unit address
other than read 'reg'? Or maybe zero should be used as parent unit
address if 'reg' does not exist?
Rob, Could you give us some advise on this?
If there's no 'reg', then 'ranges' parent address can be used. If
'ranges' is boolean (i.e. 1:1), then shrug. Just use 0. Probably, 0
should just always be used because I don't think it ever matters.
From my read of the kernel's interrupt parsing code, only the original
device's node (i.e. the one with 'interrupts') address is ever used in
the parsing and matching. So the values in the parent's address cells
don't matter. If a subsequent 'interrupt-map' is the parent, then the
code would compare the original address with the parent's
interrupt-map entries (if not masked). That kind of seems wrong to me,
but also unlikely to ever occur if it hasn't already. And that code is
something I don't want to touch because we tend to break platforms
when we do. The addresses are intertwined with the interrupt
translating because interrupts used to be part of the buses (e.g ISA).
That hasn't been the case for any h/w in the last 20 years.
If the parent address values don't matter I think we can just leave
them as zeroes and be done with it (explaining why obviously).
Something like this (with a big fat comment added summarizing this
thread):
Lizhi are you able to test it please at least to check it does not break
anything before I make it a patch for the MLs ?
I tested it and did not see any issue on my side.
Thanks,
Lizhi
Any concerns ?
-- >8 --
diff --git i/drivers/pci/of_property.c w/drivers/pci/of_property.c
index 506fcd507113..dd12691fe43c 100644
--- i/drivers/pci/of_property.c
+++ w/drivers/pci/of_property.c
@@ -279,13 +279,6 @@ static int of_pci_prop_intr_map(struct pci_dev *pdev, struct of_changeset *ocs,
mapp++;
*mapp = out_irq[i].np->phandle;
mapp++;
- if (addr_sz[i]) {
- ret = of_property_read_u32_array(out_irq[i].np,
- "reg", mapp,
- addr_sz[i]);
- if (ret)
- goto failed;
- }
mapp += addr_sz[i];
memcpy(mapp, out_irq[i].args,
out_irq[i].args_count * sizeof(u32));
