Hi Lorenzo

-----Original Message-----
From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
Sent: 23 May 2016 11:57
To: Ard Biesheuvel
Cc: Gabriele Paoloni; Jon Masters; Tomasz Nowicki; helgaas at kernel.org;
arnd at arndb.de; will.deacon at arm.com; catalin.marinas at arm.com;
rafael at kernel.org; hanjun.guo at linaro.org; okaya at codeaurora.org;
jchandra at broadcom.com; linaro-acpi at lists.linaro.org; linux-
pci at vger.kernel.org; dhdang at apm.com; Liviu.Dudau at arm.com;
ddaney at caviumnetworks.com; jeremy.linton at arm.com; linux-
kernel at vger.kernel.org; linux-acpi at vger.kernel.org;
robert.richter at caviumnetworks.com; Suravee.Suthikulpanit at amd.com;
msalter at redhat.com; Wangyijing; mw at semihalf.com;
andrea.gallo at linaro.org; linux-arm-kernel at lists.infradead.org
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:

quoted

On 20 May 2016 at 10:40, Gabriele Paoloni

[off-list ref] wrote:

quoted

quoted

Hi Ard

quoted

-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel at linaro.org]

[...]

quoted

quoted

Is the PCIe root complex so special that you cannot simply

describe an

quoted

quoted

quoted

implementation that is not PNP0408 compatible as something else,

under

quoted

quoted

quoted

its own unique HID? If everybody is onboard with using ACPI, how

is

quoted

quoted

quoted

this any different from describing other parts of the platform
topology? Even if the SBSA mandates generic PCI, they already

deviated

quoted

quoted

quoted

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

quoted

quoted

vendor to come up with his own code and it would be much more

effort

quoted

quoted

for the PCI maintainer to rework the PCI framework to accommodate

X86

quoted

quoted

and "all" ARM64 Host Controllers...

I guess this approach is too risky and we want to avoid this.

Through

quoted

quoted

standardization we can more easily maintain the code and scale it

to

quoted

quoted

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

quoted

exact hardware they might be running on. For arm64, it is the

opposite

quoted

case. We are currently in a position where we can demand vendors to
comply with the standards they endorsed themselves, and (ab)using

ACPI

quoted

+ 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

quoted

priori knowledge inside the OS that a certain 'generic' device must

be

quoted

driven in a special way.

So can anyone comment on the feasibility of adding support for

devices

quoted

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