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

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

[Pali Rohár]

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.

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?


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.

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.

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

[Alistair Delva]

Hi Pali,

Sorry for the late reply..

On Thu, Jan 13, 2022 at 4:34 AM Pali Rohár <pali@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

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

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

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

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.

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

[Mark Kettenis]

> From: Alistair Delva <adelva@google.com>
> Date: Wed, 19 Jan 2022 14:48:21 -0800
> 
> Hi Pali,
> 
> Sorry for the late reply..
> 
> On Thu, Jan 13, 2022 at 4:34 AM Pali Rohár <pali@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
> 
> 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).
> 
> > 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)
> 
> > 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.
> 
> 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.

CAM is just a version of ECAM that only gives you access to the
classic PCI config space (register offsets < 256).  This has very
little to do with the classic "mode 1" and "mode 2" config space
access methods of the x86 PCI host bridges.  I don't think there is a
CAM standard at all, but some of the PCI host bridges found on PowerPC
and SPARC hardware implemented a straight mapping of PCI config space
into mmio space like that.

It is a little bit strange that crosvm implements CAM instead of ECAM,
but I guess they don't care about passthrough of arbitrary PCIe
devices.  And as long as all (emulated) PCIe devices only have
registers with offsets < 256, this will work just fine.

And yes, you should check that the register offset is limited to
0..255.

Re: 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@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).

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

[Pali Rohár]

Hello Mark!

On Thursday 20 January 2022 00:23:02 Mark Kettenis wrote:
> CAM is just a version of ECAM that only gives you access to the
> classic PCI config space (register offsets < 256).  This has very
> little to do with the classic "mode 1" and "mode 2" config space
> access methods of the x86 PCI host bridges.

Interesting... as I have not found anything about CAM in specs... That
is why I thought it must be some proprietary, vendor-specific or
non-standard access method.

"mode 1" is indirect access method and "mode 2" has mapped config space
into (io) memory space. But this "CAM" is neither "mode 1" nor "mode 2".
That is why it looked suspicious to me.

> I don't think there is a
> CAM standard at all, but some of the PCI host bridges found on PowerPC
> and SPARC hardware implemented a straight mapping of PCI config space
> into mmio space like that.

Interesting... But at least it looks like that U-Boot does not have
support for these kind of hardware.

Anyway, is not that mapping in that old hardware of PCI config space
into mmio space according to "mode 2" layout? I know that both "mode 1"
and "mode 2" are IO-space orientated, but on non-x86 HW there probably
does not have to be IO-space and same layout can be used also for
memory-space mapping.

> It is a little bit strange that crosvm implements CAM instead of ECAM,
> but I guess they don't care about passthrough of arbitrary PCIe
> devices.  And as long as all (emulated) PCIe devices only have
> registers with offsets < 256, this will work just fine.
> 
> And yes, you should check that the register offset is limited to
> 0..255.

Re: 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@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).

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

[Alistair Delva]

On Mon, Feb 7, 2022 at 11:39 AM Pali Rohár <pali@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@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.

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

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

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

[Pali Rohár]

On Monday 07 February 2022 11:48:29 Alistair Delva wrote:
> On Mon, Feb 7, 2022 at 11:39 AM Pali Rohár <pali@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@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.

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

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.

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

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

[Alistair Delva]

On Wed, Feb 9, 2022 at 8:09 AM Pali Rohár <pali@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@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@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 is used by other physical hardware in the real world, supported
by Linux. Just because U-Boot has no support for these devices doesn't
mean the device doesn't exist.

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

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

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

[Pali Rohár]

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

Re: 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@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@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@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.
>