On Wednesday 09 February 2022 08:24:27 Alistair Delva wrote: > On Wed, Feb 9, 2022 at 8:09 AM Pali Rohár <p...@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 <p...@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 <p...@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.