RE: [PATCH V7 00/11] Support for generic ACPI based PCI host controller

[Gabriele Paoloni]

Hi Lorenzo

> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi@xxxxxxx]
> Sent: 23 May 2016 11:57
> To: Ard Biesheuvel
> Cc: Gabriele Paoloni; Jon Masters; Tomasz Nowicki; helgaas@xxxxxxxxxx;
> arnd@xxxxxxxx; will.deacon@xxxxxxx; catalin.marinas@xxxxxxx;
> rafael@xxxxxxxxxx; hanjun.guo@xxxxxxxxxx; okaya@xxxxxxxxxxxxxx;
> jchandra@xxxxxxxxxxxx; linaro-acpi@xxxxxxxxxxxxxxxx; linux-
> pci@xxxxxxxxxxxxxxx; dhdang@xxxxxxx; Liviu.Dudau@xxxxxxx;
> ddaney@xxxxxxxxxxxxxxxxxx; jeremy.linton@xxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx;
> robert.richter@xxxxxxxxxxxxxxxxxx; Suravee.Suthikulpanit@xxxxxxx;
> msalter@xxxxxxxxxx; Wangyijing; mw@xxxxxxxxxxxx;
> andrea.gallo@xxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH V7 00/11] Support for generic ACPI based PCI host
> controller
> 
> On Fri, May 20, 2016 at 11:14:03AM +0200, Ard Biesheuvel wrote:
> > On 20 May 2016 at 10:40, Gabriele Paoloni
> <gabriele.paoloni@xxxxxxxxxx> wrote:
> > > Hi Ard
> > >
> > >> -----Original Message-----
> > >> From: Ard Biesheuvel [mailto:ard.biesheuvel@xxxxxxxxxx]
> > [...]
> > >>
> > >> Is the PCIe root complex so special that you cannot simply
> describe an
> > >> implementation that is not PNP0408 compatible as something else,
> under
> > >> its own unique HID? If everybody is onboard with using ACPI, how
> is
> > >> this any different from describing other parts of the platform
> > >> topology? Even if the SBSA mandates generic PCI, they already
> deviated
> > >> from that when they built the hardware, so pretending that it is a
> > >> PNP0408 with quirks really does not buy us anything.
> > >
> > > From my understanding we want to avoid this as this would allow
> each
> > > vendor to come up with his own code and it would be much more
> effort
> > > for the PCI maintainer to rework the PCI framework to accommodate
> X86
> > > and "all" ARM64 Host Controllers...
> > >
> > > I guess this approach is too risky and we want to avoid this.
> Through
> > > standardization we can more easily maintain the code and scale it
> to
> > > multiple SoCs...
> > >
> > > So this is my understanding; maybe Jon, Tomasz or Lorenzo can give
> > > a bit more explanation...
> > >
> >
> > OK, so that boils down to recommending to vendors to represent known
> > non-compliant hardware as compliant, just so that we don't have to
> > change the code to support additional flavors of ECAM ? It's fine to
> > be pragmatic, but that sucks.
> >
> > We keep confusing the x86 case with the ARM case here: for x86, they
> > needed to deal with broken hardware *after* the fact, and all they
> > could do is find /some/ distinguishing feature in order to guess
> which
> > exact hardware they might be running on. For arm64, it is the
> opposite
> > case. We are currently in a position where we can demand vendors to
> > comply with the standards they endorsed themselves, and (ab)using
> ACPI
> > + DMI as a de facto platform description rather than plain ACPI makes
> > me think the DT crowd were actually right from the beginning. It
> > *directly* violates the standardization principle, since it requires
> a
> > priori knowledge inside the OS that a certain 'generic' device must
> be
> > driven in a special way.
> >
> > So can anyone comment on the feasibility of adding support for
> devices
> > with vendor specific HIDs (and no generic CIDs) to the current ACPI
> > ECAM driver in Linux?
> 
> Host bridges in ACPI are handled through PNP0A08/PNP0A03 ids, and
> most of the arch specific code is handled in the respective arch
> directories (X86 and IA64, even though IA64 does not rely on ECAM/MCFG
> for
> PCI ops), it is not a driver per-se, PNP0A08/PNP0A03 are detected
> through
> ACPI scan handlers and the respective arch code (ie pci_acpi_scan_root)
> sets-up resources AND config space on an arch specific basis.
> 
> X86 deals with that with code in arch/x86 that sets-up the pci_raw_ops
> on a platform specific basis (and it is not nice, but it works because
> as you all know the number of platforms in X86 world is contained).
> 
> Will this happen for ARM64 in arch/arm64 based on vendor specific
> HIDs ?
> 
> No.
> 
> So given the current state of play (we were requested to move the
> arch/arm64 specific ACPI PCI bits to arch/arm64), we would end up
> with arch/arm64 code requiring code in /drivers to set-up pci_ops
> in a platform specific way, it is horrible, if feasible at all.
> 
> The only way this can be implemented is by pretending that the
> ACPI/PCI arch/arm64 implementation is generic code (that's what this
> series does), move it to /drivers (where it is in this series), and
> implement _DSD vendor specific bindings (per HID) to set-up the pci
> operations; whether this solution should go upstream, given that it
> is just a short-term solution for early platforms bugs, it is another
> story and my personal answer is no.

I think it shouldn't be too bad to move quirk handling mechanism to
arch/arm64. Effectively we would not move platform specific code into
arch/arm64 but just the mechanism checking if there is any quirk that
is defined.

i.e.:

extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[];
extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[];

static struct pci_ecam_ops *pci_acpi_get_ops(struct acpi_pci_root *root)
{
        int bus_num = root->secondary.start;
        int domain = root->segment;
        struct pci_cfg_fixup *f;

        /*
         * Match against platform specific quirks and return corresponding
         * CAM ops.
         *
         * First match against PCI topology <domain:bus> then use DMI or
         * custom match handler.
         */
        for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; f++) {
                if ((f->domain == domain || f->domain == PCI_MCFG_DOMAIN_ANY) &&
                    (f->bus_num == bus_num || f->bus_num == PCI_MCFG_BUS_ANY) &&
                    (f->system ? dmi_check_system(f->system) : 1) &&
                    (f->match ? f->match(f, root) : 1))
                        return f->ops;
        }
        /* No quirks, use ECAM */
        return &pci_generic_ecam_ops;
}

Such quirks will be defined anyway in drivers/pci/host/ in the vendor
specific quirk implementations.

e.g. in HiSilicon case we would have

DECLARE_ACPI_MCFG_FIXUP(NULL, hisi_pcie_match, &hisi_pcie_ecam_ops,
			PCI_MCFG_DOMAIN_ANY, PCI_MCFG_BUS_ANY);

in "drivers/pci/host/pcie-hisi-acpi.c "

Thanks

Gab

> 
> Lorenzo