On 12/4/23 15:03, Rob Herring wrote:
On Mon, Dec 4, 2023 at 9:30 AM Herve Codina <herve.codina@xxxxxxxxxxx> wrote:
Hi Rob,
On Mon, 4 Dec 2023 07:59:09 -0600
Rob Herring <robh@xxxxxxxxxx> wrote:
[...]
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 9c2137dae429..46b252bbe500 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -342,8 +342,6 @@ void pci_bus_add_device(struct pci_dev *dev)
*/
pcibios_bus_add_device(dev);
pci_fixup_device(pci_fixup_final, dev);
- if (pci_is_bridge(dev))
- of_pci_make_dev_node(dev);
pci_create_sysfs_dev_files(dev);
pci_proc_attach_device(dev);
pci_bridge_d3_update(dev);
diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 51e3dd0ea5ab..e15eaf0127fc 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -31,6 +31,8 @@ int pci_set_of_node(struct pci_dev *dev)
return 0;
node = of_pci_find_child_device(dev->bus->dev.of_node, dev->devfn);
+ if (!node && pci_is_bridge(dev))
+ of_pci_make_dev_node(dev);
if (!node)
return 0;
Maybe it is too early.
of_pci_make_dev_node() creates a node and fills some properties based on
some already set values available in the PCI device such as its struct resource
values.
We need to have some values set by the PCI infra in order to create our DT node
with correct values.
Indeed, that's probably the issue I'm having. In that case,
DECLARE_PCI_FIXUP_HEADER should work. That's later, but still before
device_add().
I think modifying sysfs after device_add() is going to race with
userspace. Userspace is notified of a new device, and then the of_node
link may or may not be there when it reads sysfs. Also, not sure if
we'll need DT modaliases with PCI devices, but they won't work if the
DT node is not set before device_add().
Ok, we can try using DECLARE_PCI_FIXUP_HEADER.
On your side, is moving from DECLARE_PCI_FIXUP_EARLY to DECLARE_PCI_FIXUP_HEADER
fix your QEMU unittest ?
No...
And testing the bridge part crashes. That's because there's a
dependency on the bridge->subordinate to write out bus-range and
interrupt-map. I think the fix there is we should just not write those
properties. The bus range isn't needed because the kernel does its own
assignments. For interrupt-map, it is only needed if "interrupts" is
Without 'bus-range', dtc will output a warning while compiling the
generated node.
present in the child devices. If not present, then the standard PCI
swizzling is used. Alternatively, I think the interrupt mapping could
be simplified to just implement the standard swizzling at each level
which isn't dependent on any of the devices on the bus. I gave that a
go where each interrupt-map just points to the parent bridge, but ran
into an issue that the bridge nodes don't have a phandle. That should
be fixable, but I'd rather go with the first option. I suppose that
Do you mean it might be something similar with I posted in V12?
https://lore.kernel.org/lkml/97ae2eda-f712-0b83-dc90-0f29edd5db8b@xxxxxxx/
Thanks,
Lizhi
depends on how the interrupts downstream of the PCI device need to get
resolved. It could be that the PCI device serves as the interrupt
controller and can resolve the parent interrupt on its own (which may
be needed for ACPI host anyways).
We have to note that between the pci_fixup_device(pci_fixup_header, dev) call
and the device_add() call, the call to pci_set_msi_domain() is present.
MSIs are not supported currently but in the future ...
MSI's aren't ever described in PCI nodes. Only the host bridge. So I
don't think we should have problems there.
Related to DT modaliases, I don't think they are needed.
All drivers related to PCI device should be declared as pci_driver.
Correct me if I am wrong but I think that the core PCI will load the correct
module without any DT modalias.
Yes, you are probably right.
Rob
