Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
device/pci_device: Add notion of "hidden" PCI devices
On some SoCs, there are PCI devices that may get hidden from PCI enumeration by platform firmware. Because the Vendor ID reads back as 0xffffffff, it appears that there is no PCI device located at that BDF. However, because the device does exist, designers may wish to hang its PCI resources off of a real __pci_driver, as well as have it participate in ACPI table generation.
This patch extends the semantics of the 'hidden' keyword in devicetree.cb. If a device now uses 'hidden' instead of 'on', then it will be assumed during PCI enumeration that the device indeed does exist, and it will not be removed as a "leftover device." This allows child devices to be enumerated correctly and also PCI resources can be designated from the {read,set}_resources callbacks.
It should be noted that as of this commit, there are precisely 0 devices using 'hidden' in their devicetree.cb files, so this should be a safe thing to do.
Later patches will begin moving PCI resources from random places (typically hung off of fixed SA and LPC) into the PMC device (procedure will vary per- platform).
Change-Id: I16c2d3e1d1433343e63dfc16856cff69cd815e2a Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/device/pci_device.c 1 file changed, 57 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41384/1
diff --git a/src/device/pci_device.c b/src/device/pci_device.c index 0584871..6350e29 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -783,6 +783,18 @@ .ops_pci = &pci_bus_ops_pci, };
+/** Default device operations for hidden PCI devices */ +static struct pci_operations hidden_pci_dev_ops = { + .set_subsystem = 0, +}; + +static struct device_operations default_hidden_pci_ops_dev = { + .read_resources = noop_read_resources, + .set_resources = noop_set_resources, + .scan_bus = scan_static_bus, + .ops_pci = &hidden_pci_dev_ops, +}; + /** * Check for compatibility to route legacy VGA cycles through a bridge. * @@ -1195,6 +1207,45 @@ /* First thing setup the device structure. */ dev = pci_scan_get_dev(bus, devfn);
+ /* + * Devices that are marked as "hidden" do not get + * probed. However, the same initialization logic is still + * performed as if it were. This is useful when devices would + * like to be described in the devicetree.cb file, and/or + * present static PCI resources to the allocator, but the + * platform firmware hides the device (makes the device + * invisible to PCI enumeration) before PCI enumeration takes + * place. + * + * The semantics of hidden devices: + * 1) The device is actually present under the specified BDF + * 2) The device has an RO config space and returns its Vendor + * ID as if there were no device there (0xffffffff). + * 3) The device may still consume PCI resources. Typically, + * these would have been hardcoded elsewhere. + */ + if (dev && dev->hidden) { + /* This would normally be run from pci_probe_dev() */ + if (dev->chip_ops && dev->chip_ops->enable_dev) + dev->chip_ops->enable_dev(dev); + + /* + * If chip_ops->enable_dev did not set dev->ops, then + * set to a default .ops, because PCI enumeration is + * effectively being skipped, therefore no PCI driver + * will bind to this device. + */ + if (dev->ops == NULL) + dev->ops = &default_hidden_pci_ops_dev; + + /* This would normally be run from pci_probe_dev() */ + if (dev->ops->enable) + dev->ops->enable(dev); + + /* Skip pci_probe_dev, go to next devfn */ + continue; + } + /* See if a device is present and setup the device structure. */ dev = pci_probe_dev(dev, bus, devfn);
@@ -1218,8 +1269,12 @@
prev = &bus->children; for (dev = bus->children; dev; dev = dev->sibling) { - /* If we read valid vendor id, it is not leftover device. */ - if (dev->vendor != 0) { + /* + * The device is only considered leftover if it is not hidden + * and it has a Vendor ID of 0 (the default for a device that + * could not be probed). + */ + if (dev->vendor != 0 || dev->hidden) { prev = &dev->sibling; continue; }
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Paul Menzel, Duncan Laurie, Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41384
to look at the new patch set (#2).
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
device/pci_device: Add notion of "hidden" PCI devices
On some SoCs, there are PCI devices that may get hidden from PCI enumeration by platform firmware. Because the Vendor ID reads back as 0xffffffff, it appears that there is no PCI device located at that BDF. However, because the device does exist, designers may wish to hang its PCI resources off of a real __pci_driver, as well as have it participate in ACPI table generation.
This patch extends the semantics of the 'hidden' keyword in devicetree.cb. If a device now uses 'hidden' instead of 'on', then it will be assumed during PCI enumeration that the device indeed does exist, and it will not be removed as a "leftover device." This allows child devices to be enumerated correctly and also PCI resources can be designated from the {read,set}_resources callbacks.
It should be noted that as of this commit, there are precisely 0 devices using 'hidden' in their devicetree.cb files, so this should be a safe thing to do.
Later patches will begin moving PCI resources from random places (typically hung off of fixed SA and LPC) into the PMC device (procedure will vary per- platform).
Change-Id: I16c2d3e1d1433343e63dfc16856cff69cd815e2a Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/device/pci_device.c 1 file changed, 57 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41384/2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 2: Code-Review+2
How to handle devices that can be unhidden by coreboot? For example the P2SB can be made visible again, removing the need for such a change. Would I mark this device as hidden, too?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41384/2/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/41384/2/src/device/pci_device.c@788 PS2, Line 788: 0 We should be using NULL for pointers.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2: Code-Review+2
How to handle devices that can be unhidden by coreboot? For example the P2SB can be made visible again, removing the need for such a change. Would I mark this device as hidden, too?
That depends on when it's hidden / unhidden. If it is hidden during PCI enumeration, and that causes problems (you still want to do something with the struct device), you can mark it hidden, and then you can still use device_operations callbacks to perform work during ramstage. If it is hidden during enumeration and we would still like to hang its PCI resources off of that device, we could certainly look at marking it hidden as well.
https://review.coreboot.org/c/coreboot/+/41384/2/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/41384/2/src/device/pci_device.c@788 PS2, Line 788: 0
We should be using NULL for pointers.
Ack, will also fix pci_bus_ops_pci above.
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Paul Menzel, Duncan Laurie, Angel Pons, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41384
to look at the new patch set (#3).
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
device/pci_device: Add notion of "hidden" PCI devices
On some SoCs, there are PCI devices that may get hidden from PCI enumeration by platform firmware. Because the Vendor ID reads back as 0xffffffff, it appears that there is no PCI device located at that BDF. However, because the device does exist, designers may wish to hang its PCI resources off of a real __pci_driver, as well as have it participate in ACPI table generation.
This patch extends the semantics of the 'hidden' keyword in devicetree.cb. If a device now uses 'hidden' instead of 'on', then it will be assumed during PCI enumeration that the device indeed does exist, and it will not be removed as a "leftover device." This allows child devices to be enumerated correctly and also PCI resources can be designated from the {read,set}_resources callbacks.
It should be noted that as of this commit, there are precisely 0 devices using 'hidden' in their devicetree.cb files, so this should be a safe thing to do.
Later patches will begin moving PCI resources from random places (typically hung off of fixed SA and LPC) into the PMC device (procedure will vary per- platform).
Change-Id: I16c2d3e1d1433343e63dfc16856cff69cd815e2a Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/device/pci_device.c 1 file changed, 58 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41384/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 3: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/41384/3/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/41384/3/src/device/pci_device.c@124... PS3, Line 1244: Probably we should add a print here just like pci_probe_dev() does at the end showing that the device is enabled and has ops associated with it.
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Paul Menzel, Duncan Laurie, Angel Pons, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41384
to look at the new patch set (#4).
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
device/pci_device: Add notion of "hidden" PCI devices
On some SoCs, there are PCI devices that may get hidden from PCI enumeration by platform firmware. Because the Vendor ID reads back as 0xffffffff, it appears that there is no PCI device located at that BDF. However, because the device does exist, designers may wish to hang its PCI resources off of a real __pci_driver, as well as have it participate in ACPI table generation.
This patch extends the semantics of the 'hidden' keyword in devicetree.cb. If a device now uses 'hidden' instead of 'on', then it will be assumed during PCI enumeration that the device indeed does exist, and it will not be removed as a "leftover device." This allows child devices to be enumerated correctly and also PCI resources can be designated from the {read,set}_resources callbacks.
It should be noted that as of this commit, there are precisely 0 devices using 'hidden' in their devicetree.cb files, so this should be a safe thing to do.
Later patches will begin moving PCI resources from random places (typically hung off of fixed SA and LPC) into the PMC device (procedure will vary per- platform).
Change-Id: I16c2d3e1d1433343e63dfc16856cff69cd815e2a Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/device/pci_device.c 1 file changed, 62 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41384/4
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41384/3/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/41384/3/src/device/pci_device.c@124... PS3, Line 1244:
Probably we should add a print here just like pci_probe_dev() does at the end showing that the devic […]
Good call, will add.
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Paul Menzel, Duncan Laurie, Angel Pons, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41384
to look at the new patch set (#5).
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
device/pci_device: Add notion of "hidden" PCI devices
On some SoCs, there are PCI devices that may get hidden from PCI enumeration by platform firmware. Because the Vendor ID reads back as 0xffffffff, it appears that there is no PCI device located at that BDF. However, because the device does exist, designers may wish to hang its PCI resources off of a real __pci_driver, as well as have it participate in ACPI table generation.
This patch extends the semantics of the 'hidden' keyword in devicetree.cb. If a device now uses 'hidden' instead of 'on', then it will be assumed during PCI enumeration that the device indeed does exist, and it will not be removed as a "leftover device." This allows child devices to be enumerated correctly and also PCI resources can be designated from the {read,set}_resources callbacks.
It should be noted that as of this commit, there are precisely 0 devices using 'hidden' in their devicetree.cb files, so this should be a safe thing to do.
Later patches will begin moving PCI resources from random places (typically hung off of fixed SA and LPC) into the PMC device (procedure will vary per- platform).
Change-Id: I16c2d3e1d1433343e63dfc16856cff69cd815e2a Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/device/pci_device.c 1 file changed, 62 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41384/5
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Paul Menzel, Duncan Laurie, Angel Pons, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41384
to look at the new patch set (#6).
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
device/pci_device: Add notion of "hidden" PCI devices
On some SoCs, there are PCI devices that may get hidden from PCI enumeration by platform firmware. Because the Vendor ID reads back as 0xffffffff, it appears that there is no PCI device located at that BDF. However, because the device does exist, designers may wish to hang its PCI resources off of a real __pci_driver, as well as have it participate in ACPI table generation.
This patch extends the semantics of the 'hidden' keyword in devicetree.cb. If a device now uses 'hidden' instead of 'on', then it will be assumed during PCI enumeration that the device indeed does exist, and it will not be removed as a "leftover device." This allows child devices to be enumerated correctly and also PCI resources can be designated from the {read,set}_resources callbacks.
It should be noted that as of this commit, there are precisely 0 devices using 'hidden' in their devicetree.cb files, so this should be a safe thing to do.
Later patches will begin moving PCI resources from random places (typically hung off of fixed SA and LPC) into the PMC device (procedure will vary per- platform).
Change-Id: I16c2d3e1d1433343e63dfc16856cff69cd815e2a Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/device/pci_device.c 1 file changed, 62 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41384/6
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Paul Menzel, Duncan Laurie, Angel Pons, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41384
to look at the new patch set (#7).
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
device/pci_device: Add notion of "hidden" PCI devices
On some SoCs, there are PCI devices that may get hidden from PCI enumeration by platform firmware. Because the Vendor ID reads back as 0xffffffff, it appears that there is no PCI device located at that BDF. However, because the device does exist, designers may wish to hang its PCI resources off of a real __pci_driver, as well as have it participate in ACPI table generation.
This patch extends the semantics of the 'hidden' keyword in devicetree.cb. If a device now uses 'hidden' instead of 'on', then it will be assumed during PCI enumeration that the device indeed does exist, and it will not be removed as a "leftover device." This allows child devices to be enumerated correctly and also PCI resources can be designated from the {read,set}_resources callbacks.
It should be noted that as of this commit, there are precisely 0 devices using 'hidden' in their devicetree.cb files, so this should be a safe thing to do.
Later patches will begin moving PCI resources from random places (typically hung off of fixed SA and LPC) into the PMC device (procedure will vary per- platform).
Change-Id: I16c2d3e1d1433343e63dfc16856cff69cd815e2a Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/device/pci_device.c 1 file changed, 62 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41384/7
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 7: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c@795 PS7, Line 795: hidden_pci_dev_ops Is this needed? Usually, we check all the pointers for NULL.
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c@122... PS7, Line 1223: * ID as if there were no device there (0xffffffff). This is probably only true for a single device. IIRC, Intel has at least 4 different semantics for hidden but still active devices.
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c@125... PS7, Line 1251: } If it resembles what another function would do, why not put it into a function of its own? The flow would need less comments and were easier to follow, I guess.
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Paul Menzel, Duncan Laurie, Angel Pons, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41384
to look at the new patch set (#8).
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
device/pci_device: Add notion of "hidden" PCI devices
On some SoCs, there are PCI devices that may get hidden from PCI enumeration by platform firmware. Because the Vendor ID reads back as 0xffffffff, it appears that there is no PCI device located at that BDF. However, because the device does exist, designers may wish to hang its PCI resources off of a real __pci_driver, as well as have it participate in ACPI table generation.
This patch extends the semantics of the 'hidden' keyword in devicetree.cb. If a device now uses 'hidden' instead of 'on', then it will be assumed during PCI enumeration that the device indeed does exist, and it will not be removed as a "leftover device." This allows child devices to be enumerated correctly and also PCI resources can be designated from the {read,set}_resources callbacks.
It should be noted that as of this commit, there are precisely 0 devices using 'hidden' in their devicetree.cb files, so this should be a safe thing to do.
Later patches will begin moving PCI resources from random places (typically hung off of fixed SA and LPC) into the PMC device (procedure will vary per- platform).
Change-Id: I16c2d3e1d1433343e63dfc16856cff69cd815e2a Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/device/pci_device.c 1 file changed, 62 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41384/8
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c@795 PS7, Line 795: hidden_pci_dev_ops
Is this needed? Usually, we check all the pointers for NULL.
Hmm, maybe not. With the NULL checks, I suppose it is up to the person using 'hidden' to provide an appropriate ops_pci if desired. That would also imply pci_bus_ops_pci on line 773 isn't necessary either.
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c@122... PS7, Line 1223: * ID as if there were no device there (0xffffffff).
This is probably only true for a single device. IIRC, Intel has at least […]
Fair point. In this case, I meant the semantics of using the 'hidden' keyword in the devicetree. I'll try to make that more clear.
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c@125... PS7, Line 1251: }
If it resembles what another function would do, why not put it into a […]
Good call, will do.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 8: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c@795 PS7, Line 795: hidden_pci_dev_ops
That would also imply pci_bus_ops_pci on line 773 isn't necessary either.
Looks like it. It's also odd that it initializes only one out of two pointers explicitly to NULL. Probably some very stale piece of code.
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c@122... PS7, Line 1223: * ID as if there were no device there (0xffffffff).
Fair point. In this case, I meant the semantics of using the 'hidden' keyword in the devicetree. […]
Understood. But it seems a bit too limited. Would it suffice to say that "The device' config space can still be accessed somehow."? I guess that's the point to keep a hidden device on the PCI bus?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c@795 PS7, Line 795: hidden_pci_dev_ops
That would also imply pci_bus_ops_pci on line 773 isn't necessary either. […]
Yeah... I'll remove that in a later patch.
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c@122... PS7, Line 1223: * ID as if there were no device there (0xffffffff).
Understood. But it seems a bit too limited. Would it suffice to say that […]
Sure, SGTM. For the PMC case, I believe the FSP uses the config space during FSP-S, and then disables it so no one else can touch it. I'm not sure why it's still even a PCI device, except that it has previously been?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c@122... PS7, Line 1223: * ID as if there were no device there (0xffffffff).
Sure, SGTM. […]
I would prefer to _not_ treat hidden devices as PCI devices.
However, if there is still a way to access their config space and coreboot has to access it, the mechanism here seems reasonable. Is that the case for the PMC? I just assumed because you metioned the readability of the config space.
If there is no need to treat it as a PCI device any more, I would suggest a "generic" device instead.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c@122... PS7, Line 1223: * ID as if there were no device there (0xffffffff).
I would prefer to _not_ treat hidden devices as PCI devices. […]
My goal of this is to make it simpler for us to deal with the PCI devices that the FSP decides to hide (i.e., config space makes it look like nobody's home). Currently, because the device is invisible to enumeration, we are forced to include its PCI resources elsewhere, and we also cannot enable any SSDT generation for these devices. This method seemed like a fairly clean way to enable us to do both of these. Right now, the PMC's MMIO resource is exposed through the SA's fixed resources (MCHBAR, etc.), and the I/O resource lives off of the eSPI/LPC device.
The problem with making it a generic device is that direct children of a domain can only be PCI devices. For SSDT generation, that also makes it trickier to get a proper hierarchy of ACPI devices, because the kernel expects _SB.PCI0.PMC for the device's path.
This is a "real" PCI device; at some point in the boot flow it exposes the config space, and it does occupy the BDF.
The other way way to handle it (although I prefer this patch's approach) would be to have a special even earlier PCI enumeration for these devices.
Hello build bot (Jenkins), Nico Huber, Furquan Shaikh, Paul Menzel, Duncan Laurie, Angel Pons, Aaron Durbin, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41384
to look at the new patch set (#9).
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
device/pci_device: Add notion of "hidden" PCI devices
On some SoCs, there are PCI devices that may get hidden from PCI enumeration by platform firmware. Because the Vendor ID reads back as 0xffffffff, it appears that there is no PCI device located at that BDF. However, because the device does exist, designers may wish to hang its PCI resources off of a real __pci_driver, as well as have it participate in ACPI table generation.
This patch extends the semantics of the 'hidden' keyword in devicetree.cb. If a device now uses 'hidden' instead of 'on', then it will be assumed during PCI enumeration that the device indeed does exist, and it will not be removed as a "leftover device." This allows child devices to be enumerated correctly and also PCI resources can be designated from the {read,set}_resources callbacks.
It should be noted that as of this commit, there are precisely 0 devices using 'hidden' in their devicetree.cb files, so this should be a safe thing to do.
Later patches will begin moving PCI resources from random places (typically hung off of fixed SA and LPC) into the PMC device (procedure will vary per- platform).
Change-Id: I16c2d3e1d1433343e63dfc16856cff69cd815e2a Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/device/pci_device.c 1 file changed, 61 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41384/9
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/41384/7/src/device/pci_device.c@122... PS7, Line 1223: * ID as if there were no device there (0xffffffff).
The problem with making it a generic device is that direct children of a domain can only be PCI devices.
Ah, that's it. I didn't anticipate this limitation. It seems just wrong. (I guess part of the problem is that we'd drop it as a leftover below, while we should only consider PCI devices for that.)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 9: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 9:
Does documentation need to be updated, and it’s is important enough to announce to the mailing list?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 9:
Patch Set 9:
Does documentation need to be updated, and it’s is important enough to announce to the mailing list?
Sure, I'll look through our devicetree documentation and try to find an appropriate place to document this. I can sure email everyone too.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 10:
Patch Set 9:
Patch Set 9:
Does documentation need to be updated, and it’s is important enough to announce to the mailing list?
Sure, I'll look through our devicetree documentation and try to find an appropriate place to document this. I can sure email everyone too.
I think the documentation in the code and commit message is a good start, even if the use case is still a bit implicit there (I needed the discussion in here to fully get it).
Maybe add something to the 4.13 release notes (in Documentation/releases) about this new capability and when to use it so that people are aware of it? That can be done in a follow-up commit though.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
device/pci_device: Add notion of "hidden" PCI devices
On some SoCs, there are PCI devices that may get hidden from PCI enumeration by platform firmware. Because the Vendor ID reads back as 0xffffffff, it appears that there is no PCI device located at that BDF. However, because the device does exist, designers may wish to hang its PCI resources off of a real __pci_driver, as well as have it participate in ACPI table generation.
This patch extends the semantics of the 'hidden' keyword in devicetree.cb. If a device now uses 'hidden' instead of 'on', then it will be assumed during PCI enumeration that the device indeed does exist, and it will not be removed as a "leftover device." This allows child devices to be enumerated correctly and also PCI resources can be designated from the {read,set}_resources callbacks.
It should be noted that as of this commit, there are precisely 0 devices using 'hidden' in their devicetree.cb files, so this should be a safe thing to do.
Later patches will begin moving PCI resources from random places (typically hung off of fixed SA and LPC) into the PMC device (procedure will vary per- platform).
Change-Id: I16c2d3e1d1433343e63dfc16856cff69cd815e2a Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/41384 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/device/pci_device.c 1 file changed, 61 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/device/pci_device.c b/src/device/pci_device.c index 689325d..5f50a31 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -778,6 +778,13 @@ .reset_bus = pci_bus_reset, };
+/** Default device operations for PCI devices marked 'hidden' */ +static struct device_operations default_hidden_pci_ops_dev = { + .read_resources = noop_read_resources, + .set_resources = noop_set_resources, + .scan_bus = scan_static_bus, +}; + /** * Check for compatibility to route legacy VGA cycles through a bridge. * @@ -1147,6 +1154,46 @@ }
/** + * PCI devices that are marked as "hidden" do not get probed. However, the same + * initialization logic is still performed as if it were. This is useful when + * devices would like to be described in the devicetree.cb file, and/or present + * static PCI resources to the allocator, but the platform firmware hides the + * device (makes the device invisible to PCI enumeration) before PCI enumeration + * takes place. + * + * The expected semantics of PCI devices marked as 'hidden': + * 1) The device is actually present under the specified BDF + * 2) The device config space can still be accessed somehow, but the Vendor ID + * indicates there is no device there (it reads as 0xffffffff). + * 3) The device may still consume PCI resources. Typically, these would have + * been hardcoded elsewhere. + * + * @param dev Pointer to the device structure. + */ +static void pci_scan_hidden_device(struct device *dev) +{ + if (dev->chip_ops && dev->chip_ops->enable_dev) + dev->chip_ops->enable_dev(dev); + + /* + * If chip_ops->enable_dev did not set dev->ops, then set to a default + * .ops, because PCI enumeration is effectively being skipped, therefore + * no PCI driver will bind to this device. However, children may want to + * be enumerated, so this provides scan_static_bus for the .scan_bus + * callback. + */ + if (dev->ops == NULL) + dev->ops = &default_hidden_pci_ops_dev; + + if (dev->ops->enable) + dev->ops->enable(dev); + + /* Display the device almost as if it were probed normally */ + printk(BIOS_DEBUG, "%s [0000/%04x] hidden%s\n", dev_path(dev), + dev->device, dev->ops ? "" : " No operations"); +} + +/** * Scan a PCI bus. * * Determine the existence of devices and bridges on a PCI bus. If there are @@ -1190,6 +1237,14 @@ /* First thing setup the device structure. */ dev = pci_scan_get_dev(bus, devfn);
+ /* Devices marked 'hidden' do not get probed */ + if (dev && dev->hidden) { + pci_scan_hidden_device(dev); + + /* Skip pci_probe_dev, go to next devfn */ + continue; + } + /* See if a device is present and setup the device structure. */ dev = pci_probe_dev(dev, bus, devfn);
@@ -1213,8 +1268,12 @@
prev = &bus->children; for (dev = bus->children; dev; dev = dev->sibling) { - /* If we read valid vendor id, it is not leftover device. */ - if (dev->vendor != 0) { + /* + * The device is only considered leftover if it is not hidden + * and it has a Vendor ID of 0 (the default for a device that + * could not be probed). + */ + if (dev->vendor != 0 || dev->hidden) { prev = &dev->sibling; continue; }
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41384 )
Change subject: device/pci_device: Add notion of "hidden" PCI devices ......................................................................
Patch Set 11:
Patch Set 10:
Patch Set 9:
Patch Set 9:
Does documentation need to be updated, and it’s is important enough to announce to the mailing list?
Sure, I'll look through our devicetree documentation and try to find an appropriate place to document this. I can sure email everyone too.
I think the documentation in the code and commit message is a good start, even if the use case is still a bit implicit there (I needed the discussion in here to fully get it).
Maybe add something to the 4.13 release notes (in Documentation/releases) about this new capability and when to use it so that people are aware of it? That can be done in a follow-up commit though.
SGTM, will add that.