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

[Pali Rohár]

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

> > This access looks like a mix of "PCI Configuration Mechanism #1" and
> > "PCI Configuration Mechanism #2" from PCI Local Bus Specification
> > (rev 2.1, sections 3.7.4.1 and 3.7.4.2) and incompatible with both of
> > them. It has layout similar to Mechanism #1 and access similar to #2.
> >
> > PCI Configuration Mechanism #1 uses two registers, one which select
> > config address and second for accessing config space (selected address).
> > But that U-Boot "PCI CAM" is implemented as memory mapped address space,
> > something similar to PCI Configuration Mechanism #2 but with different
> > layout. Also that "PCI CAM" does not set "enable" bit which is per PCI
> > Configuration Mechanism #1 required to access PCI config space.
> >
> > Recently I converted all PCI drivers in U-Boot which uses PCI
> > Configuration Mechanism #1 to use PCI_CONF1_ADDRESS() macro for
> > accessing PCI config space. Basically every HW which uses PCI
> > Configuration Mechanism #1 requires to set "enable" bit like it is
> > described in PCI local bus spec. There is only one exception pci_msc01.c
> > which requires to have "enable" bit unset. And I'm not sure if this is
> > not rather bug in U-Boot driver (but it is in U-Boot in this state for a
> > long time).
> >
> > Do you have some references to this "PCI CAM" specification? Because for
> > me it looks like some vendor/proprietary undocumented API and
> > incompatible with everything which I saw.
> >
> > Therefore I would suggest to not call it "pci-host-cam-generic" or
> > TYPE_PCI as it is not generic for sure (like PCIe ECAM which is
> > documented in PCIe base spec) and also because it is not PCI type (does
> > not match neither PCI Mechanism #1 nor Mechanism #2).
> >
> > Anyway, I would like to know, which hardware uses this unusual PCI
> > config space access?
> 
> I don't know what real hardware uses it, but it is used by crosvm
> (https://chromium.googlesource.com/chromiumos/platform/crosvm)

I did not know about crosvm project. But seems that this is really used,
but just only in emulated hardware?

> > Btw, that commit probably does not work. It uses construction:
> >
> >   (PCI_FUNC(bdf) << 8) | offset
> >
> > offset passed by U-Boot is number between 0..4095 and therefore it
> > overlaps with PCI function number. Either shift by 8 is wrong and it
> > should be shift by 12 or offset needs to be limited just to 0..255. But
> > then there would be no access to PCIe extended space (256..4095), only
> > PCI and I doubt that somebody in 2022 is still doing new development for
> > Conventional PCI local bus hardware.
> 
> I think that's the case for this device, unfortunately. Perhaps we
> should cap offset between 0..255.
> 
> Our change does work; without it, U-Boot can't see any PCI devices.
> With it, they are all shown.

Well, in that commit is overlapping offset and function. So accessing
offsets above 255 would overwrite also function number and so, register
access goes into different function/device. U-Boot would see PCI device
but content in registers above 255 would be just garbage.

If your (emulated) hw has really function number starting at bit 8, and
not 12 then offset has to be limited just to 0..255. Meaning that for
offsets above 255, u-boot driver has to return fabricated value zeros
for any read attempts (and ignores write attempts).

> The other shifts in the change look the same as the Linux driver which
> adjusts the shift from 20 to 16 here:
> https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/pci-host-generic.c#L18
> 
> I admit, the added logic looks different though:
> https://elixir.bootlin.com/linux/latest/source/drivers/pci/ecam.c#L187
> 
> > Also in my opinion as this "PCI CAM" does not set "enable" bit, there is
> > needed some other code which sets it via vendor-specific API.
> 
> What should we do for now? Do you need any help getting set up with
> this environment? I think we could look at adding the pcie ecam device
> to crosvm in parallel.

As this access mechanism is already used by HW (it is only emulator, but
that does not matter), it makes sense to have u-boot driver. But as this
access mechanism is custom / non-standard, I would suggest to call it
e.g. "google,pci-crosvm" or something like that. For sure not
"pci-host-cam-generic" as this is not generic (or unless somebody points
to the PCI specification where it is documented that is is really
generic). Term "generic" is not really correct here.

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.

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