On Fri, Sep 27, 2019 at 10:42 AM Nico Huber <nic...@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 <adur...@google.com> wrote:
> >>
> >> On Thu, Sep 26, 2019 at 10:06 AM Kyösti Mälkki <kyosti.mal...@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
>
_______________________________________________
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org
