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