Commit 4f2e2280862a ("RFC: arm: pci: Add PCI cam support to PCI-E ecam driver")

[Mark Kettenis]

> Date: Wed, 9 Feb 2022 17:09:50 +0100
> From: Pali Rohár <pali at kernel.org>
> 
> On Monday 07 February 2022 11:48:29 Alistair Delva wrote:
> > On Mon, Feb 7, 2022 at 11:39 AM Pali Rohár <pali at kernel.org> wrote:
> > >
> > > PING! Could you look at this email?
> > >
> > > On Thursday 20 January 2022 14:48:34 Pali Rohár wrote:
> > > > Hello Alistair!
> > > >
> > > > On Wednesday 19 January 2022 14:48:21 Alistair Delva wrote:
> > > > > Hi Pali,
> > > > >
> > > > > Sorry for the late reply..
> > > > >
> > > > > On Thu, Jan 13, 2022 at 4:34 AM Pali Rohár <pali at kernel.org> wrote:
> > > > > >
> > > > > > Hello!
> > > > > >
> > > > > > Now I see that you have merged commit 4f2e2280862a ("RFC: arm: pci: Add
> > > > > > PCI cam support to PCI-E ecam driver"). It adds some "PCI cam support"
> > > > > > with generic DT binding "pci-host-cam-generic".
> > > > > >
> > > > > > I have tried to find some information about it, but in PCIe
> > > > > > specification there is nothing like PCI CAM. And neither in old PCI
> > > > > > local bus 2.x or 3.x specs.
> > > > >
> > > > > I can't really help you with documentation, but "pci-host-cam-generic"
> > > > > isn't something we made up, it is the same name used upstream by
> > > > > Linux: https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/pci-host-generic.c#L60
> > > >
> > > > Ou, I did not know about it.
> > > >
> > > > > We don't have specs, we just reverse engineered what was happening in
> > > > > the crosvm vm manager emulation of this device
> > > > > (https://chromium.googlesource.com/chromiumos/platform/crosvm/+/refs/heads/master/aarch64/src/fdt.rs).
> > > >
> > > > Hm... And could you in Google contact authors of this code (as it is
> > > > hosted in Google too) if they could provide specification for it? It
> > > > could really help to understand how it is suppose to do...
> > 
> > I will try, but as noted by Mark, we are not the sole user of this
> > device and we certainly don't own the definition of it. It just
> > happens to be the device that was chosen for emulation by crosvm.
> 
> I'm still sceptical that there is any other user... I checked all U-Boot
> drivers and there was no such HW, no such driver. And because
> PCI Mechanism #2 has some kind of memory mapped config space access I'm
> in impression that it was #2 what was used on real HW. As I was
> confirmed that there was real HW with #2 support. And this could explain
> possible mix of #1 and #2 which I described in previous emails.

Real hardware with configuration mechanism #2 probably hasn't been
produced since the mid-90's.  It was a thing on i386 machines and and
uses io space; not memory mapped io.  And it still required you to
poke some other register in io space before you could access PCI
config space for a particular PCI bus.  The PCI/PCIe standards only
ever standardized the configuration mechanisms on x86 hardware.  Other
hardware architectures have always done their own thing,

The "pci-host-cam-generic" binding was introduced in 2013 (commit
ce292991d88b in the Linux tree) and the commit references some ARM
specific code.  There is no way this was ever intended to be used for
hardware that use configuration mechanism #2.  But if you're not
convinced, you should ask Will Deacon.  He is still around.

> > I'm afraid I don't agree. The DT binding for U-Boot should be the same
> > as the DT binding for Linux, and as Linux calls this
> > "pci-host-cam-generic", and this device has really nothing to do with
> > Google, I don't think it should be changed.

Indeed.

> Well, if bindings is incorrect then it should be fixed. And argument
> that somebody use something incorrect is not the reason to expand its
> usage.

How can the binding be incorrect.  It clearly documents how this
works.
 
> I was really trying to find some other user of such thing, but seems
> that this google crosvm is the only current user.
> 
> Lets start doing it properly. You are targeting support for google
> crossvm (virtual hardware), so name it according to scheme like it is
> used for other vendors.

Which would mean submitting a new binding or a change to the existing
generic binding to the Linux kernel folks.  I'm fairly confident this
would end up with being rejected.

> There are many PCIe hardware parts based on Synopsys Designware IPs, and
> probably this is the most common usage nowadays. But even Designware
> drivers do not use "generic" bindings and instead are named
> specifically.

Well the Synopsys stuff is it own category of fail.  But under some
circumstances it can be integrated in a way that is fully ECAM
compatible and "pci-host-ecam-generic" has been used in cases where
the firmware fully configures the PCI host bridge such that the OS can
just use it.

> So if something seems to be marked as generic, it is Designware used by
> many HW, and not something which is used currently only by google
> crossvm.
> 
> I saw in past that Google wanted and tried people to force their
> technology as a standard even nobody except google used it. And this
> stuff looks very similar to those attempts.
> 
> In my opinion, "generic" should be used for something which is really
> generic, something which is standardized and defined. And this google
> crossvm stuff is not in any PCIe spec.

It is standardized and defined by the Linux community in:

  Documentation/devicetree/bindings/pci/host-generic-pci.yaml

That in my opinipon is more valuable than a document produced by an
industry consortium that ask serious money to be able to play.

> > > > And also please use new U-Boot PCI_CONF1_ADDRESS macro for constructing
> > > > "PCI Configuration Mechanism #1"-like values, what I did recently for
> > > > all U-Boot pci drivers.
> > 
> > We'll take a look at this too.
> > 
> > > > And for future, I would really suggest to implement standard PCIe ECAM
> > > > mechanism (as defined in PCIe standards) into crosvm project. For PCIe
> > > > devices it is really required to be able to access also config registers
> > > > above offset 255. Limitation for 0..255 is there just for compatibility
> > > > with old Conventional PCI local bus hardware (and possibility to connect
> > > > PCI-to-PCIe switches to these old hardware).
> > 
> > I agree. Once we have migrated to PCIe ECAM emulation, we can move our
> > U-Boot configuration to that as well, and potentially remove the code
> > added to support the PCI CAM version. However, this will take some
> > time, and the impact of this code in the eCAM file is very limited. I
> > think with the fixups you have requested, we can live with it for a
> > little longer.
>