Message-ID: <20190327113911.GB8331@e107981-ln.cambridge.arm.com>
Date:   Wed, 27 Mar 2019 11:39:11 +0000
From:   Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:     David Woodhouse <dwmw@...zon.co.uk>
Cc:     Jonathan Chocron <jonnyc@...zon.com>, linux-pci@...r.kernel.org,
        bhelgaas@...gle.com, linux-kernel@...r.kernel.org,
        vaerov@...zon.com, benh@...nel.crashing.org, alisaidi@...zon.com,
        zeev@...zon.com, ronenk@...zon.com, barakw@...zon.com,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Zhou Wang <wangzhou1@...ilicon.com>
Subject: Re: [PATCH v2] PCI: al: Add Amazon Annapurna Labs PCIe host
 controller driver

On Wed, Mar 27, 2019 at 09:43:26AM +0000, David Woodhouse wrote:
> 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.

The pci_ecam_ops.init function can be certainly reused but I agree
duplicating it is not a big deal either - I just noticed it and asked.

we can merge code as-is and think about writing a common init function
if/when needed.

> 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).

I missed this one - thanks for clarifying.

> 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.

DT PCI host bridge bootstrap is a different story and on that
consolidation is all but impossible.

I just want to keep MCFG ECAM quirks as simple as possible, code as it
stands is horrible enough and I wish I could remove the mechanism in
the future rather than adding more to it, hopefully we are getting
there.

> 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.

That's fine, no problems with that.

Thanks,
Lorenzo

Powered by blists - more mailing lists