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

[Pali Rohár]

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