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
