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:
>>
>> 1. Easy way to directly retrieve a device's chip config object w/o traversing the device hierarchy. Which leads to...
>> 2. Symbol alias for accessing struct device directly (no bdf lookup)
More on this in a separate email.
>> 3. 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;
};
>> 4. 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.
>>
>
> 5. 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.
> 6. 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