Re: [PATCH v4 2/2] pci, pci-thunder-pem: Add PCIe host driver for ThunderX processors.

From: Bjorn Helgaas
Date: Fri Feb 05 2016 - 14:49:43 EST


On Fri, Feb 05, 2016 at 09:12:50AM -0800, David Daney wrote:
> On 02/04/2016 07:10 PM, Bjorn Helgaas wrote:
> >On Thu, Feb 04, 2016 at 04:28:29PM -0800, David Daney wrote:
> >>On 02/04/2016 04:04 PM, Bjorn Helgaas wrote:
> >>>On Tue, Jan 26, 2016 at 01:46:21PM -0800, David Daney wrote:
> >>>>From: David Daney <david.daney@xxxxxxxxxx>
> >>>>
> >>>>Some Cavium ThunderX processors require quirky access methods for the
> >>>>config space of the PCIe bridge. Add a driver to provide these config
> >>>>space accessor functions. The pci-host-common code is used to
> >>>>configure the PCI machinery.
> >>>>
> >>>>Signed-off-by: David Daney <david.daney@xxxxxxxxxx>
> >>>>Acked-by: Rob Herring <robh@xxxxxxxxxx>
> >>>>Acked-by: Arnd Bergmann <arnd@xxxxxxxx>
> >>>>---
> >>>> .../devicetree/bindings/pci/pci-thunder-pem.txt | 43 ++++
> >>>> MAINTAINERS | 8 +
> >>>> drivers/pci/host/Kconfig | 7 +
> >>>> drivers/pci/host/Makefile | 1 +
> >>>> drivers/pci/host/pci-thunder-pem.c | 283 +++++++++++++++++++++
> >>>
> >>>What's the significance of the "pem" part of the name? I'm wondering
> >>>if we can shorten the filenames and function names by dropping it and
> >>>referring to this simply as "thunder" or "thunderx".
> ...

> >>Since PEM and ECAM are terminology used in the hardware manuals that
> >>have specific meanings relative to the Thunder SoC family, I think
> >>it is not too inconvenient to carry them over into the file names.
> >
> >As long as PEM and ECAM are really two distinct root complexes that
> >are unrelated, I guess this is OK.
>
> They are, see above.

OK, I'm convinced.

> >>>>+ /*
> >>>>+ * 32-bit accesses only. If the write is for a size
> >>>>+ * smaller than 32-bits, we must first read the 32-bit
> >>>>+ * value and merge in the desired bits and then write
> >>>>+ * the whole 32-bits back out.
> >>>>+ */
> >>>
> >>>Ugh. Another device that only supports 32-bit writes. I guess this
> >>>only affects this single device, and maybe you "know" that it has no
> >>>registers where RW1C bits may be corrupted. Although I suppose this
> >>>device has the standard status registers (Status at 0x06, Secondary
> >>>Status at 0x1e, Device Status in PCIe capability, etc.), which do
> >>>contain RW1C bits.
> ...

> I will add a WARN_ONCE or similar. and send a new patch set.
>
> FWIW, I think I have been able to get the message through to the
> hardware architects that building root complexes that are not
> exposed as PCI standard ECAMs makes things very difficult. This was
> the original intention, but turned out not to be possible when we
> looked more closely at the hardware implementation. I am optimistic
> that subsequent generations of Thunder will be much improved in this
> area.

Great! Since there's apparently only one ThunderX device (devfn == 0
on the root bus) that has this problem, and you know exactly what that
device is and where all its RW1C bits are, you *could* fix this in the
*_config_write() functions by clearing any RW1C bits before the
"modify/write" part of any read/modify/write cycle.

BTW, minor nit, when you repost these, can you reorder the
map_bus/read/write functions in that order so they match the order
they're declared in struct pci_ops? I think both pem and ecam
versions could benefit from this.

Bjorn