Re: [PATCH 3/3] PCI/ACPI: Add new quirk detection, enable bcm2711

[Jeremy Linton]

Hi,

Thanks for looking at this.

On 8/9/21 10:27 AM, Rob Herring wrote:
> On Fri, Aug 6, 2021 at 6:35 PM Jeremy Linton <jeremy.linton@xxxxxxx> wrote:
> > Hi,
> >
> > Thanks for looking at this.
> >
> > On 8/6/21 5:12 PM, Bjorn Helgaas wrote:
> > > In subject, this or similar would match history:
> > >
> > >     PCI/ACPI: Add Broadcom bcm2711 MCFG quirk
> > >
> > > On Thu, Aug 05, 2021 at 04:12:00PM -0500, Jeremy Linton wrote:
> > > > Now that we have a bcm2711 quirk, we need to be able to
> > > > detect it when the MCFG is missing. Use a namespace
> > > > property as an alternative to the MCFG OEM.
> > >
> > > Rewrap to use ~75 columns.
> > >
> > > Mention the DT namespace property here.
> > >
> > > > Signed-off-by: Jeremy Linton <jeremy.linton@xxxxxxx>
> > > > ---
> > > >    drivers/acpi/pci_mcfg.c | 14 ++++++++++++++
> > > >    1 file changed, 14 insertions(+)
> > > >
> > > > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c
> > > > index 53cab975f612..7d77fc72c2a4 100644
> > > > --- a/drivers/acpi/pci_mcfg.c
> > > > +++ b/drivers/acpi/pci_mcfg.c
> > > > @@ -169,6 +169,9 @@ static struct mcfg_fixup mcfg_quirks[] = {
> > > >       ALTRA_ECAM_QUIRK(1, 13),
> > > >       ALTRA_ECAM_QUIRK(1, 14),
> > > >       ALTRA_ECAM_QUIRK(1, 15),
> > > > +
> > > > +    { "bcm2711", "", 0, 0, MCFG_BUS_ANY, &bcm2711_pcie_ops,
> > > > +      DEFINE_RES_MEM(0xFD500000, 0xA000) },
> > > >    };
> > > >
> > > >    static char mcfg_oem_id[ACPI_OEM_ID_SIZE];
> > > > @@ -198,8 +201,19 @@ static void pci_mcfg_apply_quirks(struct acpi_pci_root *root,
> > > >       u16 segment = root->segment;
> > > >       struct resource *bus_range = &root->secondary;
> > > >       struct mcfg_fixup *f;
> > > > +    const char *soc;
> > > >       int i;
> > > >
> > > > +    /*
> > > > +     * This could be a machine with a PCI/SMC conduit,
> > > > +     * which means it doens't have MCFG. Get the machineid from
> > > > +     * the namespace definition instead.
> > >
> > > s/SMC/SMCCC/ ?  Cover letter uses SMCCC (not sure it's relevant anyway)
> > > s/doens't/doesn't/
> > >
> > > Rewrap comment to use ~80 columns.
> > >
> > > Seems pretty reasonable that a platform without standard ECAM might
> > > not have MCFG, since MCFG basically implies ECAM.
> >
> >
> > Sure, on all the above comments.
> >
> > > Is "linux,pcie-quirk" the right property to look for?  It doesn't
> > > sound very generic, and it doesn't sound like anything related to
> > > ECAM.  Is it new?  I don't see it in the tree yet.  Should it be in
> > > Documentation/devicetree/bindings/pci/pci.txt so we don't get a
> > > different property name for every new platform?
>
> No, it should not be in pci.txt. Nothing to do with DT and I don't
> review ACPI bindings (someone should) if I can help it.

That is the intention of the uefi properties registry I referred to 
earlier. It has a code first model too, which allows us to review it 
here and then update the document with the property and the intended 
behavior.

> > Yes, I made it up. Someone else commented about the "linux," partially
> > because it should be "linux-" to conform with
> > https://github.com/UEFI/DSD-Guide. But also in the same context of it
> > being linux specific.  I think that guide is where it should end up,
> > rather than the devicetree bindings.
> >
> > I guess we can request addition to the uefi- but that seems like a
> > mistake this is really (hopefully?) a Linux specific properly as other
> > OS's will simply use the SMC. I think we could request another prefix if
> > we come up with a good one and think it belongs in that guide.
>
> It's only Linux specific until it's not.
>
> What happens when there's a second PCIe quirk here? Say what the quirk
> is (and not in terms of Linux policy).

This is really just a stand in for the existing MCFG oem id, its an 
identifier to key off, nothing else.  Maybe a better name is 
"linux-ecam-quirk-id" or something to that effect?

> Don't you know the device here and can imply all this (like implying
> off of 'compatible' in DT)? Adding properties to address issues
> creates compatibility issues. Maybe not an issue in this case if the
> firmware is not stable, but not a good practice to be in.

Yes, and no, I think there was some discussion a few years back about 
using non standard HID's for these nonstandard implementations, but that 
was discouraged at the time in favor of using the standard identifiers 
and and keying off a Soc/platform specific ID to enable a "quirk". Given 
we are a bit far down that path and the decision was made to continue 
down it (rather than solving much of it with a new firmware interface) 
this seems like the straightforward solution. The vast majority of these 
are SoC specific, and just minor tweaks for alignment/etc. Given its an 
ACPI/UEFI machine we are already hiding a lot of the platform specific 
behavior for configuring the bridge/etc that might require DT like 
properties in the firmware.


Thanks again.