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

Wed Feb 9 17:32:17 CET 2022

On Wednesday 09 February 2022 08:24:27 Alistair Delva wrote:
> On Wed, Feb 9, 2022 at 8:09 AM Pali Rohár <pali at kernel.org> wrote:
> >
> > 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.
> 
> Just to be clear, we're talking about this device:
> https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/host-generic-pci.yaml

This document _mainly_ describes ECAM, which is standard and heavily
used... There are just two or three mentions about CAM, but nothing
more.

> This is used by other physical hardware in the real world, supported
> by Linux.

This applies for ECAM and in that document are already such examples.
But nothing for google crossvm CAM.

> Just because U-Boot has no support for these devices doesn't
> mean the device doesn't exist.

U-Boot supports ECAM and more drivers are based on ECAM.

But here in this discussion for commit 4f2e2280862a we are talking about
something different.

> > > > > > > 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?
> > >
> > > Yes, and we're mostly using the QEMU backend in U-Boot. We're using it
> > > as a virtual device reference for Android, and U-Boot's ability to
> > > boot Android boot.imgs has been very useful.
> > >
> > > > > > > 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).
> > >
> > > Understood. We'll send a patch to fix it (soon hopefully).
> > >
> > > > > > 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.
> > >
> > > 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.
> >
> > 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.
> >
> > 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.
> >
> > 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.
> >
> > 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.
> 
> From the document I linked above, it looks like pci-host-cam-generic
> is the right name to use for the variant of the controller with no
> quirks. The other variants are used to handle quirks.

And I explained above that it is not the right name. I'm not talking
here about ECAM (which is fully correct and should stay as it is) but
about that cam. And I still have not seen usage, example, driver or
implementation where is used that "cam" except for crossvm, yet.

> > 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.
> 
> I'm sorry you feel that way. I can only speak for myself, but that
> isn't what we're trying to do here.
> 
> > 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.
> >
> > > > > 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.