Re: [PATCH v2] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver

From: David Woodhouse <dwmw@amazon.co.uk>
To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Jonathan Chocron <jonnyc@amazon.com>
Cc: <linux-pci@vger.kernel.org>, <bhelgaas@google.com>,
	<linux-kernel@vger.kernel.org>, <vaerov@amazon.com>,
	<benh@kernel.crashing.org>, <alisaidi@amazon.com>,
	<zeev@amazon.com>, <ronenk@amazon.com>, <barakw@amazon.com>,
	Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
	Zhou Wang <wangzhou1@hisilicon.com>
Subject: Re: [PATCH v2] PCI: al: Add Amazon Annapurna Labs PCIe host controller driver
Date: Wed, 27 Mar 2019 09:43:26 +0000	[thread overview]
Message-ID: <7bab98421e2144a934d369e49454f0424c2a3758.camel@amazon.co.uk> (raw)
In-Reply-To: <20190326121727.GA4171@e107981-ln.cambridge.arm.com>

[-- Attachment #1: Type: text/plain, Size: 2312 bytes --]

On Tue, 2019-03-26 at 12:17 +0000, Lorenzo Pieralisi wrote:
> This code is basically identical to (apart from the string matching
> the DBI resource)
> 
> drivers/pci/controller/pcie-hisi.c
> 
> because, as you said, that's a DW quirk that is really not
> platform specific AFAICS.
> 
> Not that I am really fond of the idea but in practice we can
> create a quirk that applies to all host controllers DW based,
> in case they want to boot with ACPI, this patch is basically
> code duplication.

Having chatted to Jonny in a little more detail... this isn't quite
duplicate code. On the Annapurna implementation we have fixed the
alignment constraints so we can just use pci_generic_config_read()
directly instead of forcing alignment. We only need to filter out the
"ghost" devices on bus zero.

There might eventually be scope for some form of consolidation, but it
doesn't really seem clear at this point that it would be worth it.

There are three separate quirks needed for different versions of the DW
controller.

One is that the config space of the controller itself shows up multiple
times in all slots of bus zero.

The second is that bus zero cannot be accessed through ECAM and must be
accessed through a separate MMIO region.

The third is the requirement for 32-bit alignment of the host
controller's config space (through the special MMIO region).

Some vendors have managed to work around some of these issues, for
example Annapurna fixing the alignment one. It looks like the three DT
matches in pci-host-generic.c which use pci_dw_ecam_bus_ops are
assuming the hardware suffers only issue #1 from the list above, and
not the other two.

The Annapurna hardware gives us a new combination, and that's why it
isn't quite a duplicate. Again, there may be scope for consolidation in
the future but it's not clear what it should look like. Especially as
when exposed through DT, both the hisi and al versions seem to do
things quite differently and need their own specific code there anyway.

There will be a DT variant of the AL driver coming in the near future,
but when it's exposed via ACPI in the "as close to ECAM compliant as we
could make it in this iteration of the hardware" mode, it's relatively
simple so we did that patch first.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5210 bytes --]