On Fri, Sep 27, 2019 at 10:42 AM Nico Huber nico.h@gmx.de wrote:
On 27.09.19 15:42, Kyösti Mälkki wrote:
On Thu, Sep 26, 2019 at 7:45 PM Aaron Durbin adurbin@google.com wrote:
On Thu, Sep 26, 2019 at 10:06 AM Kyösti Mälkki kyosti.malkki@gmail.com
wrote:
Should be easy enough to implement platform hook telling to not remove PCI device node from topology links (based on BDF), even when it does not respond to ID queries.
For those who missed it: we already have a `hidden` keyword in sconfig which complements `on`/`off`. Currently it's only used to write static ACPI _STA methods. Maybe we could just use that? If a device is known to be hidden on purpose, don't detach it from the topology but report attempts to access PCI config?
Yes, we can certainly do that. However, I also think this issue and
yours and Nico's devicetree work are somewhat related as well as https://review.coreboot.org/c/coreboot/+/35621.
I'll try to rebase all my work during the weekend>
Here's some of the requirements/issues we should resolve that come to
mind:
- Easy way to directly retrieve a device's chip config object w/o
traversing the device hierarchy. Which leads to...
- Symbol alias for accessing struct device directly (no bdf lookup)
More on this in a separate email.
- Symbol alias for pci b/d/f lookup (equivalent of simple device but
cleaner) so we can perform pci operations.
IIRC, I asked this elsewhere already. Do we want to keep simple device? If we reduce `struct device` to b/d/f and a pointer to the chip info in early stages, couldn't we just use `struct device` for PCI config access everywhere?
We could give it a go. One issue we have is the use of dev->bus->dev in encoding buses.
static __always_inline pci_devfn_t pcidev_bdf(const struct device *dev) {
-------return (dev->path.pci.devfn << 12) | (dev->bus->secondary << 20);
}
If we wanted to get to more code consistency we need a better way to track bus number (even though we don't use it in practice currently). It could be as simple as us just conditionally supporting buses > 0 or not depending on the environment. I don't think this is a huge blocker, but I wanted to bring it up.
To be explicit you are thinking the following?
struct device { struct device_path path; void *config; };
- possibly hidden pci devices
Anything else I'm missing? I think a lot of the issues we're running
into could be fixed w/ the above. Let me know what you think.
- PCI coalesce can alter PCI dev.fn assignments?
That's a serious problem. I noticed that CFL FSP can reassign them without being asked to, unpredictably (e.g. if a device fails to show up in whatever timeframe FSP assumes). Solution might be simple? A chip->enable_dev() that updates b/d/f based on DID?
Ya. I think that's likely what is required.
- Devicetree in ENV SMM is problematic when chip config is mutable in
ENV_RAMSTAGE.
Do we need chip config in SMM?
I'm not sure. I don't recall us needing it at the moment.
Nico