How to correctly define memory range of PCIe config space

How to correctly define memory range of PCIe config space

[Pali Rohár]

Hello!

Together with Mauri we are working on extending pci-mvebu.c driver to
support Orion PCIe controllers as these controllers are same as mvebu
controller.

There is just one big difference: Config space access on Orion is
different. mvebu uses classic Intel CFC/CF8 registers for indirect
config space access but Orion has direct memory mapped config space.
So Orion DTS files need to have this memory range for config space and
pci-mvebu.c driver have to read this range from DTS and properly map it.

So my question is: How to properly define config space range in device
tree file? In which device tree property and in which format? Please
note that this memory range of config space is PCIe root port specific
and it requires its own MBUS_ID() like memory range of PCIe MEM and PCIe
IO mapping. Please look e.g. at armada-385.dtsi how are MBUS_ID() used:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/armada-385.dtsi

Krzysztof, would you be able to help with proper definition of this
property, so it would be fine also for schema checkers or other
automatic testing tools?

Re: How to correctly define memory range of PCIe config space

[Pali Rohár]

Gentle reminder...

On Monday 11 July 2022 00:51:08 Pali Rohár wrote:
> Hello!
> 
> Together with Mauri we are working on extending pci-mvebu.c driver to
> support Orion PCIe controllers as these controllers are same as mvebu
> controller.
> 
> There is just one big difference: Config space access on Orion is
> different. mvebu uses classic Intel CFC/CF8 registers for indirect
> config space access but Orion has direct memory mapped config space.
> So Orion DTS files need to have this memory range for config space and
> pci-mvebu.c driver have to read this range from DTS and properly map it.
> 
> So my question is: How to properly define config space range in device
> tree file? In which device tree property and in which format? Please
> note that this memory range of config space is PCIe root port specific
> and it requires its own MBUS_ID() like memory range of PCIe MEM and PCIe
> IO mapping. Please look e.g. at armada-385.dtsi how are MBUS_ID() used:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/armada-385.dtsi
> 
> Krzysztof, would you be able to help with proper definition of this
> property, so it would be fine also for schema checkers or other
> automatic testing tools?

Re: How to correctly define memory range of PCIe config space

[Pali Rohár]

Gentle reminder...

On Saturday 23 July 2022 11:05:06 Pali Rohár wrote:
> Gentle reminder...
> 
> On Monday 11 July 2022 00:51:08 Pali Rohár wrote:
> > Hello!
> > 
> > Together with Mauri we are working on extending pci-mvebu.c driver to
> > support Orion PCIe controllers as these controllers are same as mvebu
> > controller.
> > 
> > There is just one big difference: Config space access on Orion is
> > different. mvebu uses classic Intel CFC/CF8 registers for indirect
> > config space access but Orion has direct memory mapped config space.
> > So Orion DTS files need to have this memory range for config space and
> > pci-mvebu.c driver have to read this range from DTS and properly map it.
> > 
> > So my question is: How to properly define config space range in device
> > tree file? In which device tree property and in which format? Please
> > note that this memory range of config space is PCIe root port specific
> > and it requires its own MBUS_ID() like memory range of PCIe MEM and PCIe
> > IO mapping. Please look e.g. at armada-385.dtsi how are MBUS_ID() used:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/armada-385.dtsi
> > 
> > Krzysztof, would you be able to help with proper definition of this
> > property, so it would be fine also for schema checkers or other
> > automatic testing tools?

Re: How to correctly define memory range of PCIe config space

[Manivannan Sadhasivam]

Hi Pali,

On Mon, Jul 11, 2022 at 12:51:08AM +0200, Pali Rohár wrote:
> Hello!
> 
> Together with Mauri we are working on extending pci-mvebu.c driver to
> support Orion PCIe controllers as these controllers are same as mvebu
> controller.
> 
> There is just one big difference: Config space access on Orion is
> different. mvebu uses classic Intel CFC/CF8 registers for indirect
> config space access but Orion has direct memory mapped config space.
> So Orion DTS files need to have this memory range for config space and
> pci-mvebu.c driver have to read this range from DTS and properly map it.
> 
> So my question is: How to properly define config space range in device
> tree file? In which device tree property and in which format? Please
> note that this memory range of config space is PCIe root port specific
> and it requires its own MBUS_ID() like memory range of PCIe MEM and PCIe
> IO mapping. Please look e.g. at armada-385.dtsi how are MBUS_ID() used:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/armada-385.dtsi
> 

On most of the platforms, the standard "reg" property is used to specify the
config space together with other device specific memory regions. For instance,
on the Qcom platforms based on Designware IP, we have below regions:

      reg = <0xfc520000 0x2000>,
            <0xff000000 0x1000>,
            <0xff001000 0x1000>,
            <0xff002000 0x2000>;
      reg-names = "parf", "dbi", "elbi", "config";

Where "parf" and "elbi" are Qcom controller specific regions, while "dbi" and
"config" (config space) are common to all Designware IPs.

These properties are documented in: Documentation/devicetree/bindings/pci/qcom,pcie.yaml

Hope this helps!

Thanks,
Mani

> Krzysztof, would you be able to help with proper definition of this
> property, so it would be fine also for schema checkers or other
> automatic testing tools?

-- 
மணிவண்ணன் சதாசிவம்

Re: How to correctly define memory range of PCIe config space

[Pali Rohár]

On Saturday 06 August 2022 16:36:13 Manivannan Sadhasivam wrote:
> Hi Pali,
> 
> On Mon, Jul 11, 2022 at 12:51:08AM +0200, Pali Rohár wrote:
> > Hello!
> > 
> > Together with Mauri we are working on extending pci-mvebu.c driver to
> > support Orion PCIe controllers as these controllers are same as mvebu
> > controller.
> > 
> > There is just one big difference: Config space access on Orion is
> > different. mvebu uses classic Intel CFC/CF8 registers for indirect
> > config space access but Orion has direct memory mapped config space.
> > So Orion DTS files need to have this memory range for config space and
> > pci-mvebu.c driver have to read this range from DTS and properly map it.
> > 
> > So my question is: How to properly define config space range in device
> > tree file? In which device tree property and in which format? Please
> > note that this memory range of config space is PCIe root port specific
> > and it requires its own MBUS_ID() like memory range of PCIe MEM and PCIe
> > IO mapping. Please look e.g. at armada-385.dtsi how are MBUS_ID() used:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/armada-385.dtsi
> > 
> 
> On most of the platforms, the standard "reg" property is used to specify the
> config space together with other device specific memory regions. For instance,
> on the Qcom platforms based on Designware IP, we have below regions:
> 
>       reg = <0xfc520000 0x2000>,
>             <0xff000000 0x1000>,
>             <0xff001000 0x1000>,
>             <0xff002000 0x2000>;
>       reg-names = "parf", "dbi", "elbi", "config";
> 
> Where "parf" and "elbi" are Qcom controller specific regions, while "dbi" and
> "config" (config space) are common to all Designware IPs.
> 
> These properties are documented in: Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> 
> Hope this helps!

Hello! I have already looked at this. But as I pointed in above
armada-385.dtsi file, mvebu is quite complicated. First it does not use
explicit address ranges, but rather macros MBUS_ID() which assign
addresses at kernel runtime by mbus driver. Second issue is that config
space range (like any other resources) are pcie root port specific. So
it cannot be in pcie controller node and in pcie devices is "reg"
property reserved for pci bdf address.

In last few days, I spent some time on this issue and after reading lot
of pcie dts files, including bindings and other documents (including
open firmware pci2_1.pdf) and I'm proposing following definition:

soc {
  pcie-mem-aperture = <0xe0000000 0x08000000>; /* 128 MiB memory space */
  pcie-cfg-aperture = <0xf0000000 0x01000000>; /*  16 MiB config space */
  pcie-io-aperture  = <0xf2000000 0x00100000>; /*   1 MiB I/O space */

  pcie {
    ranges = <0x82000000 0 0x40000     MBUS_ID(0xf0, 0x01) 0x40000  0x0 0x2000>,    /* Port 0.0 Internal registers */
             <0x82000000 0 0xf0000000  MBUS_ID(0x04, 0x79) 0        0x0 0x1000000>, /* Port 0.0 Config space */
             <0x82000000 1 0x0         MBUS_ID(0x04, 0x59) 0        0x1 0x0>,       /* Port 0.0 Mem */
             <0x81000000 1 0x0         MBUS_ID(0x04, 0x51) 0        0x1 0x0>,       /* Port 0.0 I/O */

    pcie@1,0 {
      reg = <0x0800 0 0 0 0>; /* BDF 0:1.0 */
      assigned-addresses =     <0x82000800 0 0x40000     0x0 0x2000>,     /* Port 0.0 Internal registers */
                               <0x82000800 0 0xf0000000  0x0 0x1000000>;  /* Port 0.0 Config space */
      ranges = <0x82000000 0 0  0x82000000 1 0           0x1 0x0>,        /* Port 0.0 Mem */
                0x81000000 0 0  0x81000000 1 0           0x1 0x0>;        /* Port 0.0 I/O */
    };
  };
};

So the pci config space address range would be defined in
"assigned-addresses" property as the _second_ value. First value is
already used for specifying internal registers (similar what is "parf"
for qcom).

config space is currently limited to 16 MB (without extended PCIe), but
after we find free continuous physical address window of size 256MB we
can extend it to full PCIe config space range.

Any objections to above device tree definition?

> Thanks,
> Mani
> 
> > Krzysztof, would you be able to help with proper definition of this
> > property, so it would be fine also for schema checkers or other
> > automatic testing tools?
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Re: How to correctly define memory range of PCIe config space

[Manivannan Sadhasivam]

On Sat, Aug 06, 2022 at 01:17:02PM +0200, Pali Rohár wrote:
> On Saturday 06 August 2022 16:36:13 Manivannan Sadhasivam wrote:
> > Hi Pali,
> > 
> > On Mon, Jul 11, 2022 at 12:51:08AM +0200, Pali Rohár wrote:
> > > Hello!
> > > 
> > > Together with Mauri we are working on extending pci-mvebu.c driver to
> > > support Orion PCIe controllers as these controllers are same as mvebu
> > > controller.
> > > 
> > > There is just one big difference: Config space access on Orion is
> > > different. mvebu uses classic Intel CFC/CF8 registers for indirect
> > > config space access but Orion has direct memory mapped config space.
> > > So Orion DTS files need to have this memory range for config space and
> > > pci-mvebu.c driver have to read this range from DTS and properly map it.
> > > 
> > > So my question is: How to properly define config space range in device
> > > tree file? In which device tree property and in which format? Please
> > > note that this memory range of config space is PCIe root port specific
> > > and it requires its own MBUS_ID() like memory range of PCIe MEM and PCIe
> > > IO mapping. Please look e.g. at armada-385.dtsi how are MBUS_ID() used:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/armada-385.dtsi
> > > 
> > 
> > On most of the platforms, the standard "reg" property is used to specify the
> > config space together with other device specific memory regions. For instance,
> > on the Qcom platforms based on Designware IP, we have below regions:
> > 
> >       reg = <0xfc520000 0x2000>,
> >             <0xff000000 0x1000>,
> >             <0xff001000 0x1000>,
> >             <0xff002000 0x2000>;
> >       reg-names = "parf", "dbi", "elbi", "config";
> > 
> > Where "parf" and "elbi" are Qcom controller specific regions, while "dbi" and
> > "config" (config space) are common to all Designware IPs.
> > 
> > These properties are documented in: Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > 
> > Hope this helps!
> 
> Hello! I have already looked at this. But as I pointed in above
> armada-385.dtsi file, mvebu is quite complicated. First it does not use
> explicit address ranges, but rather macros MBUS_ID() which assign
> addresses at kernel runtime by mbus driver. Second issue is that config
> space range (like any other resources) are pcie root port specific. So
> it cannot be in pcie controller node and in pcie devices is "reg"
> property reserved for pci bdf address.
> 
> In last few days, I spent some time on this issue and after reading lot
> of pcie dts files, including bindings and other documents (including
> open firmware pci2_1.pdf) and I'm proposing following definition:
> 
> soc {
>   pcie-mem-aperture = <0xe0000000 0x08000000>; /* 128 MiB memory space */
>   pcie-cfg-aperture = <0xf0000000 0x01000000>; /*  16 MiB config space */
>   pcie-io-aperture  = <0xf2000000 0x00100000>; /*   1 MiB I/O space */
> 
>   pcie {
>     ranges = <0x82000000 0 0x40000     MBUS_ID(0xf0, 0x01) 0x40000  0x0 0x2000>,    /* Port 0.0 Internal registers */
>              <0x82000000 0 0xf0000000  MBUS_ID(0x04, 0x79) 0        0x0 0x1000000>, /* Port 0.0 Config space */
>              <0x82000000 1 0x0         MBUS_ID(0x04, 0x59) 0        0x1 0x0>,       /* Port 0.0 Mem */
>              <0x81000000 1 0x0         MBUS_ID(0x04, 0x51) 0        0x1 0x0>,       /* Port 0.0 I/O */
> 
>     pcie@1,0 {
>       reg = <0x0800 0 0 0 0>; /* BDF 0:1.0 */
>       assigned-addresses =     <0x82000800 0 0x40000     0x0 0x2000>,     /* Port 0.0 Internal registers */
>                                <0x82000800 0 0xf0000000  0x0 0x1000000>;  /* Port 0.0 Config space */
>       ranges = <0x82000000 0 0  0x82000000 1 0           0x1 0x0>,        /* Port 0.0 Mem */
>                 0x81000000 0 0  0x81000000 1 0           0x1 0x0>;        /* Port 0.0 I/O */
>     };
>   };
> };
> 
> So the pci config space address range would be defined in
> "assigned-addresses" property as the _second_ value. First value is
> already used for specifying internal registers (similar what is "parf"
> for qcom).
> 

Sounds reasonable to me. Another option would be to introduce a mvebu specific
property but that would be the least preferred option I guess.

But the fact that "assigned-addresses" property is described as "MMIO registers"
also adds up to the justification IMO.

Rob/Krzysztof could always correct that during binding review.

> config space is currently limited to 16 MB (without extended PCIe), but
> after we find free continuous physical address window of size 256MB we
> can extend it to full PCIe config space range.
> 
> Any objections to above device tree definition?
> 

Are you also converting the binding to YAML for validation?

Thanks,
Mani

> > Thanks,
> > Mani
> > 
> > > Krzysztof, would you be able to help with proper definition of this
> > > property, so it would be fine also for schema checkers or other
> > > automatic testing tools?
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்

-- 
மணிவண்ணன் சதாசிவம்

Re: How to correctly define memory range of PCIe config space

[Pali Rohár]

On Saturday 06 August 2022 17:46:14 Manivannan Sadhasivam wrote:
> On Sat, Aug 06, 2022 at 01:17:02PM +0200, Pali Rohár wrote:
> > On Saturday 06 August 2022 16:36:13 Manivannan Sadhasivam wrote:
> > > Hi Pali,
> > > 
> > > On Mon, Jul 11, 2022 at 12:51:08AM +0200, Pali Rohár wrote:
> > > > Hello!
> > > > 
> > > > Together with Mauri we are working on extending pci-mvebu.c driver to
> > > > support Orion PCIe controllers as these controllers are same as mvebu
> > > > controller.
> > > > 
> > > > There is just one big difference: Config space access on Orion is
> > > > different. mvebu uses classic Intel CFC/CF8 registers for indirect
> > > > config space access but Orion has direct memory mapped config space.
> > > > So Orion DTS files need to have this memory range for config space and
> > > > pci-mvebu.c driver have to read this range from DTS and properly map it.
> > > > 
> > > > So my question is: How to properly define config space range in device
> > > > tree file? In which device tree property and in which format? Please
> > > > note that this memory range of config space is PCIe root port specific
> > > > and it requires its own MBUS_ID() like memory range of PCIe MEM and PCIe
> > > > IO mapping. Please look e.g. at armada-385.dtsi how are MBUS_ID() used:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/armada-385.dtsi
> > > > 
> > > 
> > > On most of the platforms, the standard "reg" property is used to specify the
> > > config space together with other device specific memory regions. For instance,
> > > on the Qcom platforms based on Designware IP, we have below regions:
> > > 
> > >       reg = <0xfc520000 0x2000>,
> > >             <0xff000000 0x1000>,
> > >             <0xff001000 0x1000>,
> > >             <0xff002000 0x2000>;
> > >       reg-names = "parf", "dbi", "elbi", "config";
> > > 
> > > Where "parf" and "elbi" are Qcom controller specific regions, while "dbi" and
> > > "config" (config space) are common to all Designware IPs.
> > > 
> > > These properties are documented in: Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > > 
> > > Hope this helps!
> > 
> > Hello! I have already looked at this. But as I pointed in above
> > armada-385.dtsi file, mvebu is quite complicated. First it does not use
> > explicit address ranges, but rather macros MBUS_ID() which assign
> > addresses at kernel runtime by mbus driver. Second issue is that config
> > space range (like any other resources) are pcie root port specific. So
> > it cannot be in pcie controller node and in pcie devices is "reg"
> > property reserved for pci bdf address.
> > 
> > In last few days, I spent some time on this issue and after reading lot
> > of pcie dts files, including bindings and other documents (including
> > open firmware pci2_1.pdf) and I'm proposing following definition:
> > 
> > soc {
> >   pcie-mem-aperture = <0xe0000000 0x08000000>; /* 128 MiB memory space */
> >   pcie-cfg-aperture = <0xf0000000 0x01000000>; /*  16 MiB config space */
> >   pcie-io-aperture  = <0xf2000000 0x00100000>; /*   1 MiB I/O space */
> > 
> >   pcie {
> >     ranges = <0x82000000 0 0x40000     MBUS_ID(0xf0, 0x01) 0x40000  0x0 0x2000>,    /* Port 0.0 Internal registers */
> >              <0x82000000 0 0xf0000000  MBUS_ID(0x04, 0x79) 0        0x0 0x1000000>, /* Port 0.0 Config space */
> >              <0x82000000 1 0x0         MBUS_ID(0x04, 0x59) 0        0x1 0x0>,       /* Port 0.0 Mem */
> >              <0x81000000 1 0x0         MBUS_ID(0x04, 0x51) 0        0x1 0x0>,       /* Port 0.0 I/O */
> > 
> >     pcie@1,0 {
> >       reg = <0x0800 0 0 0 0>; /* BDF 0:1.0 */
> >       assigned-addresses =     <0x82000800 0 0x40000     0x0 0x2000>,     /* Port 0.0 Internal registers */
> >                                <0x82000800 0 0xf0000000  0x0 0x1000000>;  /* Port 0.0 Config space */
> >       ranges = <0x82000000 0 0  0x82000000 1 0           0x1 0x0>,        /* Port 0.0 Mem */
> >                 0x81000000 0 0  0x81000000 1 0           0x1 0x0>;        /* Port 0.0 I/O */
> >     };
> >   };
> > };
> > 
> > So the pci config space address range would be defined in
> > "assigned-addresses" property as the _second_ value. First value is
> > already used for specifying internal registers (similar what is "parf"
> > for qcom).
> > 
> 
> Sounds reasonable to me. Another option would be to introduce a mvebu specific
> property but that would be the least preferred option I guess.
> 
> But the fact that "assigned-addresses" property is described as "MMIO registers"
> also adds up to the justification IMO.
> 
> Rob/Krzysztof could always correct that during binding review.

Ok!

> > config space is currently limited to 16 MB (without extended PCIe), but
> > after we find free continuous physical address window of size 256MB we
> > can extend it to full PCIe config space range.
> > 
> > Any objections to above device tree definition?
> > 
> 
> Are you also converting the binding to YAML for validation?

I still have an issue to understand YAML scheme declaration and do not
know how to express all those properties in this scheme language
correctly. Also I was not able to setup infrastructure for running
scheme binding tests. So I'm currently not planning to do this.

It would be really a good idea to provide some web service where people
could upload their work-in-progress DTS files and YAML schemes for
automatic validation.

> Thanks,
> Mani
> 
> > > Thanks,
> > > Mani
> > > 
> > > > Krzysztof, would you be able to help with proper definition of this
> > > > property, so it would be fine also for schema checkers or other
> > > > automatic testing tools?
> > > 
> > > -- 
> > > மணிவண்ணன் சதாசிவம்
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Re: How to correctly define memory range of PCIe config space

[Manivannan Sadhasivam]

On Sat, Aug 06, 2022 at 02:23:30PM +0200, Pali Rohár wrote:
> On Saturday 06 August 2022 17:46:14 Manivannan Sadhasivam wrote:
> > On Sat, Aug 06, 2022 at 01:17:02PM +0200, Pali Rohár wrote:
> > > On Saturday 06 August 2022 16:36:13 Manivannan Sadhasivam wrote:
> > > > Hi Pali,
> > > > 
> > > > On Mon, Jul 11, 2022 at 12:51:08AM +0200, Pali Rohár wrote:
> > > > > Hello!
> > > > > 
> > > > > Together with Mauri we are working on extending pci-mvebu.c driver to
> > > > > support Orion PCIe controllers as these controllers are same as mvebu
> > > > > controller.
> > > > > 
> > > > > There is just one big difference: Config space access on Orion is
> > > > > different. mvebu uses classic Intel CFC/CF8 registers for indirect
> > > > > config space access but Orion has direct memory mapped config space.
> > > > > So Orion DTS files need to have this memory range for config space and
> > > > > pci-mvebu.c driver have to read this range from DTS and properly map it.
> > > > > 
> > > > > So my question is: How to properly define config space range in device
> > > > > tree file? In which device tree property and in which format? Please
> > > > > note that this memory range of config space is PCIe root port specific
> > > > > and it requires its own MBUS_ID() like memory range of PCIe MEM and PCIe
> > > > > IO mapping. Please look e.g. at armada-385.dtsi how are MBUS_ID() used:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/armada-385.dtsi
> > > > > 
> > > > 
> > > > On most of the platforms, the standard "reg" property is used to specify the
> > > > config space together with other device specific memory regions. For instance,
> > > > on the Qcom platforms based on Designware IP, we have below regions:
> > > > 
> > > >       reg = <0xfc520000 0x2000>,
> > > >             <0xff000000 0x1000>,
> > > >             <0xff001000 0x1000>,
> > > >             <0xff002000 0x2000>;
> > > >       reg-names = "parf", "dbi", "elbi", "config";
> > > > 
> > > > Where "parf" and "elbi" are Qcom controller specific regions, while "dbi" and
> > > > "config" (config space) are common to all Designware IPs.
> > > > 
> > > > These properties are documented in: Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > > > 
> > > > Hope this helps!
> > > 
> > > Hello! I have already looked at this. But as I pointed in above
> > > armada-385.dtsi file, mvebu is quite complicated. First it does not use
> > > explicit address ranges, but rather macros MBUS_ID() which assign
> > > addresses at kernel runtime by mbus driver. Second issue is that config
> > > space range (like any other resources) are pcie root port specific. So
> > > it cannot be in pcie controller node and in pcie devices is "reg"
> > > property reserved for pci bdf address.
> > > 
> > > In last few days, I spent some time on this issue and after reading lot
> > > of pcie dts files, including bindings and other documents (including
> > > open firmware pci2_1.pdf) and I'm proposing following definition:
> > > 
> > > soc {
> > >   pcie-mem-aperture = <0xe0000000 0x08000000>; /* 128 MiB memory space */
> > >   pcie-cfg-aperture = <0xf0000000 0x01000000>; /*  16 MiB config space */
> > >   pcie-io-aperture  = <0xf2000000 0x00100000>; /*   1 MiB I/O space */
> > > 
> > >   pcie {
> > >     ranges = <0x82000000 0 0x40000     MBUS_ID(0xf0, 0x01) 0x40000  0x0 0x2000>,    /* Port 0.0 Internal registers */
> > >              <0x82000000 0 0xf0000000  MBUS_ID(0x04, 0x79) 0        0x0 0x1000000>, /* Port 0.0 Config space */
> > >              <0x82000000 1 0x0         MBUS_ID(0x04, 0x59) 0        0x1 0x0>,       /* Port 0.0 Mem */
> > >              <0x81000000 1 0x0         MBUS_ID(0x04, 0x51) 0        0x1 0x0>,       /* Port 0.0 I/O */
> > > 
> > >     pcie@1,0 {
> > >       reg = <0x0800 0 0 0 0>; /* BDF 0:1.0 */
> > >       assigned-addresses =     <0x82000800 0 0x40000     0x0 0x2000>,     /* Port 0.0 Internal registers */
> > >                                <0x82000800 0 0xf0000000  0x0 0x1000000>;  /* Port 0.0 Config space */
> > >       ranges = <0x82000000 0 0  0x82000000 1 0           0x1 0x0>,        /* Port 0.0 Mem */
> > >                 0x81000000 0 0  0x81000000 1 0           0x1 0x0>;        /* Port 0.0 I/O */
> > >     };
> > >   };
> > > };
> > > 
> > > So the pci config space address range would be defined in
> > > "assigned-addresses" property as the _second_ value. First value is
> > > already used for specifying internal registers (similar what is "parf"
> > > for qcom).
> > > 
> > 
> > Sounds reasonable to me. Another option would be to introduce a mvebu specific
> > property but that would be the least preferred option I guess.
> > 
> > But the fact that "assigned-addresses" property is described as "MMIO registers"
> > also adds up to the justification IMO.
> > 
> > Rob/Krzysztof could always correct that during binding review.
> 
> Ok!
> 
> > > config space is currently limited to 16 MB (without extended PCIe), but
> > > after we find free continuous physical address window of size 256MB we
> > > can extend it to full PCIe config space range.
> > > 
> > > Any objections to above device tree definition?
> > > 
> > 
> > Are you also converting the binding to YAML for validation?
> 
> I still have an issue to understand YAML scheme declaration and do not
> know how to express all those properties in this scheme language
> correctly. Also I was not able to setup infrastructure for running
> scheme binding tests. So I'm currently not planning to do this.
> 

Okay!

> It would be really a good idea to provide some web service where people
> could upload their work-in-progress DTS files and YAML schemes for
> automatic validation.
> 

Not sure what issues you are facing, but I find it easy to install and validate
the devicetree files and schemas using the "dt-schema" tool. And I always run it
together with "make dtbs" so that I know if my changes are violating the
binding or not.

I'm aware that folks have been using the validator on popular distributions like
Ubuntu, Debian, Fedora, Kali etc...

If you have a specific issue, you can raise the Github issue on the repo to
get some help: https://github.com/devicetree-org/dt-schema/issues

Thanks,
Mani

> > Thanks,
> > Mani
> > 
> > > > Thanks,
> > > > Mani
> > > > 
> > > > > Krzysztof, would you be able to help with proper definition of this
> > > > > property, so it would be fine also for schema checkers or other
> > > > > automatic testing tools?
> > > > 
> > > > -- 
> > > > மணிவண்ணன் சதாசிவம்
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்

-- 
மணிவண்ணன் சதாசிவம்

Re: How to correctly define memory range of PCIe config space

[Rob Herring]

On Sat, Aug 6, 2022 at 5:17 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Saturday 06 August 2022 16:36:13 Manivannan Sadhasivam wrote:
> > Hi Pali,
> >
> > On Mon, Jul 11, 2022 at 12:51:08AM +0200, Pali Rohár wrote:
> > > Hello!
> > >
> > > Together with Mauri we are working on extending pci-mvebu.c driver to
> > > support Orion PCIe controllers as these controllers are same as mvebu
> > > controller.
> > >
> > > There is just one big difference: Config space access on Orion is
> > > different. mvebu uses classic Intel CFC/CF8 registers for indirect
> > > config space access but Orion has direct memory mapped config space.
> > > So Orion DTS files need to have this memory range for config space and
> > > pci-mvebu.c driver have to read this range from DTS and properly map it.
> > >
> > > So my question is: How to properly define config space range in device
> > > tree file? In which device tree property and in which format? Please
> > > note that this memory range of config space is PCIe root port specific
> > > and it requires its own MBUS_ID() like memory range of PCIe MEM and PCIe
> > > IO mapping. Please look e.g. at armada-385.dtsi how are MBUS_ID() used:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/armada-385.dtsi
> > >
> >
> > On most of the platforms, the standard "reg" property is used to specify the
> > config space together with other device specific memory regions. For instance,
> > on the Qcom platforms based on Designware IP, we have below regions:
> >
> >       reg = <0xfc520000 0x2000>,
> >             <0xff000000 0x1000>,
> >             <0xff001000 0x1000>,
> >             <0xff002000 0x2000>;
> >       reg-names = "parf", "dbi", "elbi", "config";
> >
> > Where "parf" and "elbi" are Qcom controller specific regions, while "dbi" and
> > "config" (config space) are common to all Designware IPs.
> >
> > These properties are documented in: Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> >
> > Hope this helps!
>
> Hello! I have already looked at this. But as I pointed in above
> armada-385.dtsi file, mvebu is quite complicated. First it does not use
> explicit address ranges, but rather macros MBUS_ID() which assign
> addresses at kernel runtime by mbus driver. Second issue is that config
> space range (like any other resources) are pcie root port specific. So
> it cannot be in pcie controller node and in pcie devices is "reg"
> property reserved for pci bdf address.
>
> In last few days, I spent some time on this issue and after reading lot
> of pcie dts files, including bindings and other documents (including
> open firmware pci2_1.pdf) and I'm proposing following definition:
>
> soc {
>   pcie-mem-aperture = <0xe0000000 0x08000000>; /* 128 MiB memory space */
>   pcie-cfg-aperture = <0xf0000000 0x01000000>; /*  16 MiB config space */
>   pcie-io-aperture  = <0xf2000000 0x00100000>; /*   1 MiB I/O space */
>
>   pcie {
>     ranges = <0x82000000 0 0x40000     MBUS_ID(0xf0, 0x01) 0x40000  0x0 0x2000>,    /* Port 0.0 Internal registers */
>              <0x82000000 0 0xf0000000  MBUS_ID(0x04, 0x79) 0        0x0 0x1000000>, /* Port 0.0 Config space */

IMO, this should be 0 for first cell as this is config space. What is
0xf0000000 as that's supposed to be an address in PCI address space.

>              <0x82000000 1 0x0         MBUS_ID(0x04, 0x59) 0        0x1 0x0>,       /* Port 0.0 Mem */
>              <0x81000000 1 0x0         MBUS_ID(0x04, 0x51) 0        0x1 0x0>,       /* Port 0.0 I/O */

I/O space at 4GB? It's only 32-bits. I guess this is already there
from the mvebu binding, but it seems kind of broken...

>
>     pcie@1,0 {
>       reg = <0x0800 0 0 0 0>; /* BDF 0:1.0 */
>       assigned-addresses =     <0x82000800 0 0x40000     0x0 0x2000>,     /* Port 0.0 Internal registers */
>                                <0x82000800 0 0xf0000000  0x0 0x1000000>;  /* Port 0.0 Config space */

This says it is memory space, not config space. But the PCI binding
says config space is not allowed in assigned-addresses.

I think the parent ranges needs a config space entry with the BDF for
each root port and then this entry should be dropped. It really looks
to me like the mvebu binding created these fake PCI addresses to map
root ports back to MBUS addresses when BDF could have been used
instead.

Rob

Re: How to correctly define memory range of PCIe config space

[Rob Herring]

On Sat, Aug 6, 2022 at 6:23 AM Pali Rohár <pali@kernel.org> wrote:
>
> On Saturday 06 August 2022 17:46:14 Manivannan Sadhasivam wrote:
> > On Sat, Aug 06, 2022 at 01:17:02PM +0200, Pali Rohár wrote:
> > > On Saturday 06 August 2022 16:36:13 Manivannan Sadhasivam wrote:
> > > > Hi Pali,
> > > >
> > > > On Mon, Jul 11, 2022 at 12:51:08AM +0200, Pali Rohár wrote:
> > > > > Hello!
> > > > >
> > > > > Together with Mauri we are working on extending pci-mvebu.c driver to
> > > > > support Orion PCIe controllers as these controllers are same as mvebu
> > > > > controller.
> > > > >
> > > > > There is just one big difference: Config space access on Orion is
> > > > > different. mvebu uses classic Intel CFC/CF8 registers for indirect
> > > > > config space access but Orion has direct memory mapped config space.
> > > > > So Orion DTS files need to have this memory range for config space and
> > > > > pci-mvebu.c driver have to read this range from DTS and properly map it.
> > > > >
> > > > > So my question is: How to properly define config space range in device
> > > > > tree file? In which device tree property and in which format? Please
> > > > > note that this memory range of config space is PCIe root port specific
> > > > > and it requires its own MBUS_ID() like memory range of PCIe MEM and PCIe
> > > > > IO mapping. Please look e.g. at armada-385.dtsi how are MBUS_ID() used:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/armada-385.dtsi
> > > > >
> > > >
> > > > On most of the platforms, the standard "reg" property is used to specify the
> > > > config space together with other device specific memory regions. For instance,
> > > > on the Qcom platforms based on Designware IP, we have below regions:
> > > >
> > > >       reg = <0xfc520000 0x2000>,
> > > >             <0xff000000 0x1000>,
> > > >             <0xff001000 0x1000>,
> > > >             <0xff002000 0x2000>;
> > > >       reg-names = "parf", "dbi", "elbi", "config";
> > > >
> > > > Where "parf" and "elbi" are Qcom controller specific regions, while "dbi" and
> > > > "config" (config space) are common to all Designware IPs.
> > > >
> > > > These properties are documented in: Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > > >
> > > > Hope this helps!
> > >
> > > Hello! I have already looked at this. But as I pointed in above
> > > armada-385.dtsi file, mvebu is quite complicated. First it does not use
> > > explicit address ranges, but rather macros MBUS_ID() which assign
> > > addresses at kernel runtime by mbus driver. Second issue is that config
> > > space range (like any other resources) are pcie root port specific. So
> > > it cannot be in pcie controller node and in pcie devices is "reg"
> > > property reserved for pci bdf address.
> > >
> > > In last few days, I spent some time on this issue and after reading lot
> > > of pcie dts files, including bindings and other documents (including
> > > open firmware pci2_1.pdf) and I'm proposing following definition:
> > >
> > > soc {
> > >   pcie-mem-aperture = <0xe0000000 0x08000000>; /* 128 MiB memory space */
> > >   pcie-cfg-aperture = <0xf0000000 0x01000000>; /*  16 MiB config space */
> > >   pcie-io-aperture  = <0xf2000000 0x00100000>; /*   1 MiB I/O space */
> > >
> > >   pcie {
> > >     ranges = <0x82000000 0 0x40000     MBUS_ID(0xf0, 0x01) 0x40000  0x0 0x2000>,    /* Port 0.0 Internal registers */
> > >              <0x82000000 0 0xf0000000  MBUS_ID(0x04, 0x79) 0        0x0 0x1000000>, /* Port 0.0 Config space */
> > >              <0x82000000 1 0x0         MBUS_ID(0x04, 0x59) 0        0x1 0x0>,       /* Port 0.0 Mem */
> > >              <0x81000000 1 0x0         MBUS_ID(0x04, 0x51) 0        0x1 0x0>,       /* Port 0.0 I/O */
> > >
> > >     pcie@1,0 {
> > >       reg = <0x0800 0 0 0 0>; /* BDF 0:1.0 */
> > >       assigned-addresses =     <0x82000800 0 0x40000     0x0 0x2000>,     /* Port 0.0 Internal registers */
> > >                                <0x82000800 0 0xf0000000  0x0 0x1000000>;  /* Port 0.0 Config space */
> > >       ranges = <0x82000000 0 0  0x82000000 1 0           0x1 0x0>,        /* Port 0.0 Mem */
> > >                 0x81000000 0 0  0x81000000 1 0           0x1 0x0>;        /* Port 0.0 I/O */
> > >     };
> > >   };
> > > };
> > >
> > > So the pci config space address range would be defined in
> > > "assigned-addresses" property as the _second_ value. First value is
> > > already used for specifying internal registers (similar what is "parf"
> > > for qcom).
> > >
> >
> > Sounds reasonable to me. Another option would be to introduce a mvebu specific
> > property but that would be the least preferred option I guess.
> >
> > But the fact that "assigned-addresses" property is described as "MMIO registers"
> > also adds up to the justification IMO.
> >
> > Rob/Krzysztof could always correct that during binding review.
>
> Ok!
>
> > > config space is currently limited to 16 MB (without extended PCIe), but
> > > after we find free continuous physical address window of size 256MB we
> > > can extend it to full PCIe config space range.
> > >
> > > Any objections to above device tree definition?
> > >
> >
> > Are you also converting the binding to YAML for validation?
>
> I still have an issue to understand YAML scheme declaration and do not
> know how to express all those properties in this scheme language
> correctly. Also I was not able to setup infrastructure for running
> scheme binding tests. So I'm currently not planning to do this.

What's the issue exactly with installing? 'pip install dtschema'
doesn't work for you?

> It would be really a good idea to provide some web service where people
> could upload their work-in-progress DTS files and YAML schemes for
> automatic validation.

You can send patches and they get tested. That works until I get
annoyed that you aren't testing your patches as I review the results
and testing runs on my h/w (each patch is 5-15mins). If someone wants
to donate h/w for testing, I'd be happy to provide un-reviewed, fully
automated results. I just need a gitlab runner(s) with docker to point
the CI job to.

There's already several json-schema validator websites. Standing one
up for our specific needs probably wouldn't be too hard.

Rob

Re: How to correctly define memory range of PCIe config space

[Pali Rohár]

Hello!

On Tuesday 09 August 2022 09:59:49 Rob Herring wrote:
> On Sat, Aug 6, 2022 at 5:17 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Saturday 06 August 2022 16:36:13 Manivannan Sadhasivam wrote:
> > > Hi Pali,
> > >
> > > On Mon, Jul 11, 2022 at 12:51:08AM +0200, Pali Rohár wrote:
> > > > Hello!
> > > >
> > > > Together with Mauri we are working on extending pci-mvebu.c driver to
> > > > support Orion PCIe controllers as these controllers are same as mvebu
> > > > controller.
> > > >
> > > > There is just one big difference: Config space access on Orion is
> > > > different. mvebu uses classic Intel CFC/CF8 registers for indirect
> > > > config space access but Orion has direct memory mapped config space.
> > > > So Orion DTS files need to have this memory range for config space and
> > > > pci-mvebu.c driver have to read this range from DTS and properly map it.
> > > >
> > > > So my question is: How to properly define config space range in device
> > > > tree file? In which device tree property and in which format? Please
> > > > note that this memory range of config space is PCIe root port specific
> > > > and it requires its own MBUS_ID() like memory range of PCIe MEM and PCIe
> > > > IO mapping. Please look e.g. at armada-385.dtsi how are MBUS_ID() used:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/armada-385.dtsi
> > > >
> > >
> > > On most of the platforms, the standard "reg" property is used to specify the
> > > config space together with other device specific memory regions. For instance,
> > > on the Qcom platforms based on Designware IP, we have below regions:
> > >
> > >       reg = <0xfc520000 0x2000>,
> > >             <0xff000000 0x1000>,
> > >             <0xff001000 0x1000>,
> > >             <0xff002000 0x2000>;
> > >       reg-names = "parf", "dbi", "elbi", "config";
> > >
> > > Where "parf" and "elbi" are Qcom controller specific regions, while "dbi" and
> > > "config" (config space) are common to all Designware IPs.
> > >
> > > These properties are documented in: Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > >
> > > Hope this helps!
> >
> > Hello! I have already looked at this. But as I pointed in above
> > armada-385.dtsi file, mvebu is quite complicated. First it does not use
> > explicit address ranges, but rather macros MBUS_ID() which assign
> > addresses at kernel runtime by mbus driver. Second issue is that config
> > space range (like any other resources) are pcie root port specific. So
> > it cannot be in pcie controller node and in pcie devices is "reg"
> > property reserved for pci bdf address.
> >
> > In last few days, I spent some time on this issue and after reading lot
> > of pcie dts files, including bindings and other documents (including
> > open firmware pci2_1.pdf) and I'm proposing following definition:
> >
> > soc {
> >   pcie-mem-aperture = <0xe0000000 0x08000000>; /* 128 MiB memory space */
> >   pcie-cfg-aperture = <0xf0000000 0x01000000>; /*  16 MiB config space */
> >   pcie-io-aperture  = <0xf2000000 0x00100000>; /*   1 MiB I/O space */
> >
> >   pcie {
> >     ranges = <0x82000000 0 0x40000     MBUS_ID(0xf0, 0x01) 0x40000  0x0 0x2000>,    /* Port 0.0 Internal registers */
> >              <0x82000000 0 0xf0000000  MBUS_ID(0x04, 0x79) 0        0x0 0x1000000>, /* Port 0.0 Config space */
> 
> IMO, this should be 0 for first cell as this is config space. What is
> 0xf0000000 as that's supposed to be an address in PCI address space.

Which value should be 0? I did not catch it here.

0xf0000000 is just start offset for the child address range. I chose it
"randomly" to not conflict with 0x40000 offset for child address range
which describe internal PCIe controller registers.

> >              <0x82000000 1 0x0         MBUS_ID(0x04, 0x59) 0        0x1 0x0>,       /* Port 0.0 Mem */
> >              <0x81000000 1 0x0         MBUS_ID(0x04, 0x51) 0        0x1 0x0>,       /* Port 0.0 I/O */
> 
> I/O space at 4GB? It's only 32-bits. I guess this is already there
> from the mvebu binding, but it seems kind of broken...

You have not looked how it works. mvebu is 32-bit CPU and this describe
whole address range which could possibly used for IO or MEM for any
(unspecified) PCIe controller.

Every PCIe controller has its own PCIe MEM and PCIe IO address range
which are not shared with other PCIe controllers. (non-mvebu platforms
use for this different PCI domains; mvebu not)

These "shared" ranges are then dynamically split into PCIe controllers
like their drivers ask. So at the end all 10 PCIe controllers which are
on AXP ask for maximum (64 kB) of PCIe IO and allocator then choose 10
64kB non-interleaving ranges from this "big" master range. Same for PCIe
MEM.

So basically those two lines (one for MEM and one for IO) just says that
"unspecified" amount of PCIe MEM is mapped to MBUS_ID(0x04, 0x59) and
another "unspecified" amount of PCIe IO is mapped to MBUS_ID(0x04, 0x51)
mbus and pci-mvebu.c drivers than takes care for proper allocation of
required memory from dynamic pool defined in pcie-mem-aperture and
pcie-io-aperture properties.

Note that this is not something which I invented, this is used for a
long time by mvebu drivers and device tree files.

> >
> >     pcie@1,0 {
> >       reg = <0x0800 0 0 0 0>; /* BDF 0:1.0 */
> >       assigned-addresses =     <0x82000800 0 0x40000     0x0 0x2000>,     /* Port 0.0 Internal registers */
> >                                <0x82000800 0 0xf0000000  0x0 0x1000000>;  /* Port 0.0 Config space */
> 
> This says it is memory space, not config space.

"config space" for Orion (in from driver point of view) is just ordinary
"memory space".

"memory space" is accessed by load/store instructions. I/O space is
accessed by inb/outb (IO instructions or what architecture has) and
"config space" is accessed by kernel drivers by its own drivers.

So I think it is correct to declare this range of address space as
"memory".

> But the PCI binding
> says config space is not allowed in assigned-addresses.

Exactly. And this is because it does not make sense to assign "config
space" into "assigned-addresses" as described in:
https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf

My understanding of above document is that "reg" property in following
code describes "config space".

  pcie@1,0 {
    reg = <0x0800 0 0 0 0>; /* BDF 0:1.0 */
  }

And reason is that because above document use "config space" terminology
for describing specific PCI device on the bus.

Address part after the at-char (@1,0) in device tree should match "reg"
property and by all above definitions from pci2_1.pdf this pass because:

1) that document describes @1,0 format that is "config space" for PCI
   device with BDF X:1.0 where X is the parent bus

2) 0x0800 is really describes BDF X:1.0

And then the "config space is not allowed in assigned-addresses" makes
sense as there is absolutely no need to put "pointer" to PCI device
(with BDF address) into "assigned-addresses" property. In this property
should be some memory range and not reference to some PCI device.

> I think the parent ranges needs a config space entry with the BDF for
> each root port and then this entry should be dropped. It really looks
> to me like the mvebu binding created these fake PCI addresses to map
> root ports back to MBUS addresses when BDF could have been used
> instead.
> 
> Rob

The reason is that in mvebu there are X independent single-root-port
PCIe controllers (PCIe host bridges; PCI domains; ...) and device tree
binding was defined that all root ports (with their host bridges) would
be put into one "virtual" DT node which would act as one PCI domain.

And this leads to the fact that all controller specific settings must be
defined in pci root ports.

And obviously every PCIe controller has its own internal registers and
its own way how to access config space (all is independent).

Re: How to correctly define memory range of PCIe config space

[Rob Herring]

On Tue, Aug 9, 2022 at 10:29 AM Pali Rohár <pali@kernel.org> wrote:
>
> Hello!
>
> On Tuesday 09 August 2022 09:59:49 Rob Herring wrote:
> > On Sat, Aug 6, 2022 at 5:17 AM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > On Saturday 06 August 2022 16:36:13 Manivannan Sadhasivam wrote:
> > > > Hi Pali,
> > > >
> > > > On Mon, Jul 11, 2022 at 12:51:08AM +0200, Pali Rohár wrote:
> > > > > Hello!
> > > > >
> > > > > Together with Mauri we are working on extending pci-mvebu.c driver to
> > > > > support Orion PCIe controllers as these controllers are same as mvebu
> > > > > controller.
> > > > >
> > > > > There is just one big difference: Config space access on Orion is
> > > > > different. mvebu uses classic Intel CFC/CF8 registers for indirect
> > > > > config space access but Orion has direct memory mapped config space.
> > > > > So Orion DTS files need to have this memory range for config space and
> > > > > pci-mvebu.c driver have to read this range from DTS and properly map it.
> > > > >
> > > > > So my question is: How to properly define config space range in device
> > > > > tree file? In which device tree property and in which format? Please
> > > > > note that this memory range of config space is PCIe root port specific
> > > > > and it requires its own MBUS_ID() like memory range of PCIe MEM and PCIe
> > > > > IO mapping. Please look e.g. at armada-385.dtsi how are MBUS_ID() used:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/armada-385.dtsi
> > > > >
> > > >
> > > > On most of the platforms, the standard "reg" property is used to specify the
> > > > config space together with other device specific memory regions. For instance,
> > > > on the Qcom platforms based on Designware IP, we have below regions:
> > > >
> > > >       reg = <0xfc520000 0x2000>,
> > > >             <0xff000000 0x1000>,
> > > >             <0xff001000 0x1000>,
> > > >             <0xff002000 0x2000>;
> > > >       reg-names = "parf", "dbi", "elbi", "config";
> > > >
> > > > Where "parf" and "elbi" are Qcom controller specific regions, while "dbi" and
> > > > "config" (config space) are common to all Designware IPs.
> > > >
> > > > These properties are documented in: Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > > >
> > > > Hope this helps!
> > >
> > > Hello! I have already looked at this. But as I pointed in above
> > > armada-385.dtsi file, mvebu is quite complicated. First it does not use
> > > explicit address ranges, but rather macros MBUS_ID() which assign
> > > addresses at kernel runtime by mbus driver. Second issue is that config
> > > space range (like any other resources) are pcie root port specific. So
> > > it cannot be in pcie controller node and in pcie devices is "reg"
> > > property reserved for pci bdf address.
> > >
> > > In last few days, I spent some time on this issue and after reading lot
> > > of pcie dts files, including bindings and other documents (including
> > > open firmware pci2_1.pdf) and I'm proposing following definition:
> > >
> > > soc {
> > >   pcie-mem-aperture = <0xe0000000 0x08000000>; /* 128 MiB memory space */
> > >   pcie-cfg-aperture = <0xf0000000 0x01000000>; /*  16 MiB config space */
> > >   pcie-io-aperture  = <0xf2000000 0x00100000>; /*   1 MiB I/O space */
> > >
> > >   pcie {
> > >     ranges = <0x82000000 0 0x40000     MBUS_ID(0xf0, 0x01) 0x40000  0x0 0x2000>,    /* Port 0.0 Internal registers */
> > >              <0x82000000 0 0xf0000000  MBUS_ID(0x04, 0x79) 0        0x0 0x1000000>, /* Port 0.0 Config space */
> >
> > IMO, this should be 0 for first cell as this is config space. What is
> > 0xf0000000 as that's supposed to be an address in PCI address space.
>
> Which value should be 0? I did not catch it here.

s/0x82000000/0/

But really that's 0 for the hi byte and BDF for the lower bytes.


> 0xf0000000 is just start offset for the child address range. I chose it
> "randomly" to not conflict with 0x40000 offset for child address range
> which describe internal PCIe controller registers.
>
> > >              <0x82000000 1 0x0         MBUS_ID(0x04, 0x59) 0        0x1 0x0>,       /* Port 0.0 Mem */
> > >              <0x81000000 1 0x0         MBUS_ID(0x04, 0x51) 0        0x1 0x0>,       /* Port 0.0 I/O */
> >
> > I/O space at 4GB? It's only 32-bits. I guess this is already there
> > from the mvebu binding, but it seems kind of broken...
>
> You have not looked how it works. mvebu is 32-bit CPU and this describe
> whole address range which could possibly used for IO or MEM for any
> (unspecified) PCIe controller.

I did look. It's just an intermediate virtual/random/made-up address
space. It kind of follows PCI bus addressing, but then not really.

> Every PCIe controller has its own PCIe MEM and PCIe IO address range
> which are not shared with other PCIe controllers. (non-mvebu platforms
> use for this different PCI domains; mvebu not)

Except there's this shared 'PCI like' bus each port is on.

> These "shared" ranges are then dynamically split into PCIe controllers
> like their drivers ask. So at the end all 10 PCIe controllers which are
> on AXP ask for maximum (64 kB) of PCIe IO and allocator then choose 10
> 64kB non-interleaving ranges from this "big" master range. Same for PCIe
> MEM.
>
> So basically those two lines (one for MEM and one for IO) just says that
> "unspecified" amount of PCIe MEM is mapped to MBUS_ID(0x04, 0x59) and
> another "unspecified" amount of PCIe IO is mapped to MBUS_ID(0x04, 0x51)
> mbus and pci-mvebu.c drivers than takes care for proper allocation of
> required memory from dynamic pool defined in pcie-mem-aperture and
> pcie-io-aperture properties.

That sounds shared to me...

>
> Note that this is not something which I invented, this is used for a
> long time by mvebu drivers and device tree files.
>
> > >
> > >     pcie@1,0 {
> > >       reg = <0x0800 0 0 0 0>; /* BDF 0:1.0 */
> > >       assigned-addresses =     <0x82000800 0 0x40000     0x0 0x2000>,     /* Port 0.0 Internal registers */
> > >                                <0x82000800 0 0xf0000000  0x0 0x1000000>;  /* Port 0.0 Config space */
> >
> > This says it is memory space, not config space.
>
> "config space" for Orion (in from driver point of view) is just ordinary
> "memory space".

"memory space" means PCI memory space which it is not. Almost every
Arm platform has config space in ordinary host/cpu address space.

> "memory space" is accessed by load/store instructions. I/O space is
> accessed by inb/outb (IO instructions or what architecture has) and
> "config space" is accessed by kernel drivers by its own drivers.
>
> So I think it is correct to declare this range of address space as
> "memory".

It is not if we are talking PCI addresses. If this is Marvell pseudo
virtual PCI addresses, then you can declare it whatever you want.

>
> > But the PCI binding
> > says config space is not allowed in assigned-addresses.
>
> Exactly. And this is because it does not make sense to assign "config
> space" into "assigned-addresses" as described in:
> https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf
>
> My understanding of above document is that "reg" property in following
> code describes "config space".
>
>   pcie@1,0 {
>     reg = <0x0800 0 0 0 0>; /* BDF 0:1.0 */
>   }
>
> And reason is that because above document use "config space" terminology
> for describing specific PCI device on the bus.
>
> Address part after the at-char (@1,0) in device tree should match "reg"
> property and by all above definitions from pci2_1.pdf this pass because:
>
> 1) that document describes @1,0 format that is "config space" for PCI
>    device with BDF X:1.0 where X is the parent bus
>
> 2) 0x0800 is really describes BDF X:1.0
>
> And then the "config space is not allowed in assigned-addresses" makes
> sense as there is absolutely no need to put "pointer" to PCI device
> (with BDF address) into "assigned-addresses" property. In this property
> should be some memory range and not reference to some PCI device.
>
> > I think the parent ranges needs a config space entry with the BDF for
> > each root port and then this entry should be dropped. It really looks
> > to me like the mvebu binding created these fake PCI addresses to map
> > root ports back to MBUS addresses when BDF could have been used
> > instead.
> >
> > Rob
>
> The reason is that in mvebu there are X independent single-root-port
> PCIe controllers (PCIe host bridges; PCI domains; ...) and device tree
> binding was defined that all root ports (with their host bridges) would
> be put into one "virtual" DT node which would act as one PCI domain.

If they are independent, why is there 1 mem and i/o aperture?

I don't know how a virtual DT node for purposes of creating a single
PCI domain ever passed review. Must have been explained differently at
the time.

> And this leads to the fact that all controller specific settings must be
> defined in pci root ports.

Not necessarily. There's no reason settings can't be in the parent node.

Rob

Re: How to correctly define memory range of PCIe config space

[Pali Rohár]

On Tuesday 09 August 2022 11:06:18 Rob Herring wrote:
> On Tue, Aug 9, 2022 at 10:29 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > Hello!
> >
> > On Tuesday 09 August 2022 09:59:49 Rob Herring wrote:
> > > On Sat, Aug 6, 2022 at 5:17 AM Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > On Saturday 06 August 2022 16:36:13 Manivannan Sadhasivam wrote:
> > > > > Hi Pali,
> > > > >
> > > > > On Mon, Jul 11, 2022 at 12:51:08AM +0200, Pali Rohár wrote:
> > > > > > Hello!
> > > > > >
> > > > > > Together with Mauri we are working on extending pci-mvebu.c driver to
> > > > > > support Orion PCIe controllers as these controllers are same as mvebu
> > > > > > controller.
> > > > > >
> > > > > > There is just one big difference: Config space access on Orion is
> > > > > > different. mvebu uses classic Intel CFC/CF8 registers for indirect
> > > > > > config space access but Orion has direct memory mapped config space.
> > > > > > So Orion DTS files need to have this memory range for config space and
> > > > > > pci-mvebu.c driver have to read this range from DTS and properly map it.
> > > > > >
> > > > > > So my question is: How to properly define config space range in device
> > > > > > tree file? In which device tree property and in which format? Please
> > > > > > note that this memory range of config space is PCIe root port specific
> > > > > > and it requires its own MBUS_ID() like memory range of PCIe MEM and PCIe
> > > > > > IO mapping. Please look e.g. at armada-385.dtsi how are MBUS_ID() used:
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/armada-385.dtsi
> > > > > >
> > > > >
> > > > > On most of the platforms, the standard "reg" property is used to specify the
> > > > > config space together with other device specific memory regions. For instance,
> > > > > on the Qcom platforms based on Designware IP, we have below regions:
> > > > >
> > > > >       reg = <0xfc520000 0x2000>,
> > > > >             <0xff000000 0x1000>,
> > > > >             <0xff001000 0x1000>,
> > > > >             <0xff002000 0x2000>;
> > > > >       reg-names = "parf", "dbi", "elbi", "config";
> > > > >
> > > > > Where "parf" and "elbi" are Qcom controller specific regions, while "dbi" and
> > > > > "config" (config space) are common to all Designware IPs.
> > > > >
> > > > > These properties are documented in: Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > > > >
> > > > > Hope this helps!
> > > >
> > > > Hello! I have already looked at this. But as I pointed in above
> > > > armada-385.dtsi file, mvebu is quite complicated. First it does not use
> > > > explicit address ranges, but rather macros MBUS_ID() which assign
> > > > addresses at kernel runtime by mbus driver. Second issue is that config
> > > > space range (like any other resources) are pcie root port specific. So
> > > > it cannot be in pcie controller node and in pcie devices is "reg"
> > > > property reserved for pci bdf address.
> > > >
> > > > In last few days, I spent some time on this issue and after reading lot
> > > > of pcie dts files, including bindings and other documents (including
> > > > open firmware pci2_1.pdf) and I'm proposing following definition:
> > > >
> > > > soc {
> > > >   pcie-mem-aperture = <0xe0000000 0x08000000>; /* 128 MiB memory space */
> > > >   pcie-cfg-aperture = <0xf0000000 0x01000000>; /*  16 MiB config space */
> > > >   pcie-io-aperture  = <0xf2000000 0x00100000>; /*   1 MiB I/O space */
> > > >
> > > >   pcie {
> > > >     ranges = <0x82000000 0 0x40000     MBUS_ID(0xf0, 0x01) 0x40000  0x0 0x2000>,    /* Port 0.0 Internal registers */
> > > >              <0x82000000 0 0xf0000000  MBUS_ID(0x04, 0x79) 0        0x0 0x1000000>, /* Port 0.0 Config space */
> > >
> > > IMO, this should be 0 for first cell as this is config space. What is
> > > 0xf0000000 as that's supposed to be an address in PCI address space.
> >
> > Which value should be 0? I did not catch it here.
> 
> s/0x82000000/0/
> 
> But really that's 0 for the hi byte and BDF for the lower bytes.

But then, this line would describe "PCI device", not the address range
(if we should follow that pci2_1.pdf).

Also note that kernel accept only 0xX2000000 and 0xX1000000 values (MEM
and IO).

> > 0xf0000000 is just start offset for the child address range. I chose it
> > "randomly" to not conflict with 0x40000 offset for child address range
> > which describe internal PCIe controller registers.
> >
> > > >              <0x82000000 1 0x0         MBUS_ID(0x04, 0x59) 0        0x1 0x0>,       /* Port 0.0 Mem */
> > > >              <0x81000000 1 0x0         MBUS_ID(0x04, 0x51) 0        0x1 0x0>,       /* Port 0.0 I/O */
> > >
> > > I/O space at 4GB? It's only 32-bits. I guess this is already there
> > > from the mvebu binding, but it seems kind of broken...
> >
> > You have not looked how it works. mvebu is 32-bit CPU and this describe
> > whole address range which could possibly used for IO or MEM for any
> > (unspecified) PCIe controller.
> 
> I did look. It's just an intermediate virtual/random/made-up address
> space. It kind of follows PCI bus addressing, but then not really.

Exactly!

> > Every PCIe controller has its own PCIe MEM and PCIe IO address range
> > which are not shared with other PCIe controllers. (non-mvebu platforms
> > use for this different PCI domains; mvebu not)
> 
> Except there's this shared 'PCI like' bus each port is on.

I mean that in HW.

> > These "shared" ranges are then dynamically split into PCIe controllers
> > like their drivers ask. So at the end all 10 PCIe controllers which are
> > on AXP ask for maximum (64 kB) of PCIe IO and allocator then choose 10
> > 64kB non-interleaving ranges from this "big" master range. Same for PCIe
> > MEM.
> >
> > So basically those two lines (one for MEM and one for IO) just says that
> > "unspecified" amount of PCIe MEM is mapped to MBUS_ID(0x04, 0x59) and
> > another "unspecified" amount of PCIe IO is mapped to MBUS_ID(0x04, 0x51)
> > mbus and pci-mvebu.c drivers than takes care for proper allocation of
> > required memory from dynamic pool defined in pcie-mem-aperture and
> > pcie-io-aperture properties.
> 
> That sounds shared to me...

In device tree it looks like shared, but in HW it is not shared.

> >
> > Note that this is not something which I invented, this is used for a
> > long time by mvebu drivers and device tree files.
> >
> > > >
> > > >     pcie@1,0 {
> > > >       reg = <0x0800 0 0 0 0>; /* BDF 0:1.0 */
> > > >       assigned-addresses =     <0x82000800 0 0x40000     0x0 0x2000>,     /* Port 0.0 Internal registers */
> > > >                                <0x82000800 0 0xf0000000  0x0 0x1000000>;  /* Port 0.0 Config space */
> > >
> > > This says it is memory space, not config space.
> >
> > "config space" for Orion (in from driver point of view) is just ordinary
> > "memory space".
> 
> "memory space" means PCI memory space which it is not. Almost every
> Arm platform has config space in ordinary host/cpu address space.

Yes. This is not PCI (memory space). And it applies also for internal
registers range, which is prevent in all existing mvebu dts files.

Both assigned-addresses in above example (and also in all existing mvebu
dts files) describes host/cpu address space. Not PCI address space.

> > "memory space" is accessed by load/store instructions. I/O space is
> > accessed by inb/outb (IO instructions or what architecture has) and
> > "config space" is accessed by kernel drivers by its own drivers.
> >
> > So I think it is correct to declare this range of address space as
> > "memory".
> 
> It is not if we are talking PCI addresses. If this is Marvell pseudo
> virtual PCI addresses, then you can declare it whatever you want.

Yes, lets say it is "marvell pseudo pci address" which is not pci
address at all. But is of type "memory", not "io".

> > > But the PCI binding
> > > says config space is not allowed in assigned-addresses.
> >
> > Exactly. And this is because it does not make sense to assign "config
> > space" into "assigned-addresses" as described in:
> > https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf
> >
> > My understanding of above document is that "reg" property in following
> > code describes "config space".
> >
> >   pcie@1,0 {
> >     reg = <0x0800 0 0 0 0>; /* BDF 0:1.0 */
> >   }
> >
> > And reason is that because above document use "config space" terminology
> > for describing specific PCI device on the bus.
> >
> > Address part after the at-char (@1,0) in device tree should match "reg"
> > property and by all above definitions from pci2_1.pdf this pass because:
> >
> > 1) that document describes @1,0 format that is "config space" for PCI
> >    device with BDF X:1.0 where X is the parent bus
> >
> > 2) 0x0800 is really describes BDF X:1.0
> >
> > And then the "config space is not allowed in assigned-addresses" makes
> > sense as there is absolutely no need to put "pointer" to PCI device
> > (with BDF address) into "assigned-addresses" property. In this property
> > should be some memory range and not reference to some PCI device.
> >
> > > I think the parent ranges needs a config space entry with the BDF for
> > > each root port and then this entry should be dropped. It really looks
> > > to me like the mvebu binding created these fake PCI addresses to map
> > > root ports back to MBUS addresses when BDF could have been used
> > > instead.
> > >
> > > Rob
> >
> > The reason is that in mvebu there are X independent single-root-port
> > PCIe controllers (PCIe host bridges; PCI domains; ...) and device tree
> > binding was defined that all root ports (with their host bridges) would
> > be put into one "virtual" DT node which would act as one PCI domain.
> 
> If they are independent, why is there 1 mem and i/o aperture?
> 
> I don't know how a virtual DT node for purposes of creating a single
> PCI domain ever passed review. Must have been explained differently at
> the time.

Well, I do not know. I took that pci mvebu code as it was. I spend lot
of time understanding how is that marvell pcie hw working and how it is
implemented in linux kernel drivers and how it is defined in device
tree.

And I already saw that even marvell people did not understand how their
pcie hw is working; so it possible that nobody had all architecture and
hw information about this design at the time of device tree review.

Anyway, this is something which already exists, it is used and cannot be
changed for backward compatibility.

> > And this leads to the fact that all controller specific settings must be
> > defined in pci root ports.
> 
> Not necessarily. There's no reason settings can't be in the parent node.
> 
> Rob

Well, you can put it also into the parent node, but then you need to
somehow say which internal registers range describe which pcie root port
(=pcie controller). For me the logical option is to describe it is in
root port (=pcie controller) node as it the node, which needs it in hw.

But also this is how it is already used.

Re: How to correctly define memory range of PCIe config space

[Pali Rohár]

Hello! Due to no other comments or reactions in this area for one month,
I will briefly send Orion patches with definitions as they are. config
space registers are accessed in the same way as internal registers, they
are memory based and per port specific. Trying to do it differently
hardly does not work as kernel PCI core code has expectations what can
be stored in ranges= and reg= properties - and this cannot not (and also
should not) be changed in PCI core code for good reasons.

On Tuesday 09 August 2022 19:47:16 Pali Rohár wrote:
> On Tuesday 09 August 2022 11:06:18 Rob Herring wrote:
> > On Tue, Aug 9, 2022 at 10:29 AM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > Hello!
> > >
> > > On Tuesday 09 August 2022 09:59:49 Rob Herring wrote:
> > > > On Sat, Aug 6, 2022 at 5:17 AM Pali Rohár <pali@kernel.org> wrote:
> > > > >
> > > > > On Saturday 06 August 2022 16:36:13 Manivannan Sadhasivam wrote:
> > > > > > Hi Pali,
> > > > > >
> > > > > > On Mon, Jul 11, 2022 at 12:51:08AM +0200, Pali Rohár wrote:
> > > > > > > Hello!
> > > > > > >
> > > > > > > Together with Mauri we are working on extending pci-mvebu.c driver to
> > > > > > > support Orion PCIe controllers as these controllers are same as mvebu
> > > > > > > controller.
> > > > > > >
> > > > > > > There is just one big difference: Config space access on Orion is
> > > > > > > different. mvebu uses classic Intel CFC/CF8 registers for indirect
> > > > > > > config space access but Orion has direct memory mapped config space.
> > > > > > > So Orion DTS files need to have this memory range for config space and
> > > > > > > pci-mvebu.c driver have to read this range from DTS and properly map it.
> > > > > > >
> > > > > > > So my question is: How to properly define config space range in device
> > > > > > > tree file? In which device tree property and in which format? Please
> > > > > > > note that this memory range of config space is PCIe root port specific
> > > > > > > and it requires its own MBUS_ID() like memory range of PCIe MEM and PCIe
> > > > > > > IO mapping. Please look e.g. at armada-385.dtsi how are MBUS_ID() used:
> > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/armada-385.dtsi
> > > > > > >
> > > > > >
> > > > > > On most of the platforms, the standard "reg" property is used to specify the
> > > > > > config space together with other device specific memory regions. For instance,
> > > > > > on the Qcom platforms based on Designware IP, we have below regions:
> > > > > >
> > > > > >       reg = <0xfc520000 0x2000>,
> > > > > >             <0xff000000 0x1000>,
> > > > > >             <0xff001000 0x1000>,
> > > > > >             <0xff002000 0x2000>;
> > > > > >       reg-names = "parf", "dbi", "elbi", "config";
> > > > > >
> > > > > > Where "parf" and "elbi" are Qcom controller specific regions, while "dbi" and
> > > > > > "config" (config space) are common to all Designware IPs.
> > > > > >
> > > > > > These properties are documented in: Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > > > > >
> > > > > > Hope this helps!
> > > > >
> > > > > Hello! I have already looked at this. But as I pointed in above
> > > > > armada-385.dtsi file, mvebu is quite complicated. First it does not use
> > > > > explicit address ranges, but rather macros MBUS_ID() which assign
> > > > > addresses at kernel runtime by mbus driver. Second issue is that config
> > > > > space range (like any other resources) are pcie root port specific. So
> > > > > it cannot be in pcie controller node and in pcie devices is "reg"
> > > > > property reserved for pci bdf address.
> > > > >
> > > > > In last few days, I spent some time on this issue and after reading lot
> > > > > of pcie dts files, including bindings and other documents (including
> > > > > open firmware pci2_1.pdf) and I'm proposing following definition:
> > > > >
> > > > > soc {
> > > > >   pcie-mem-aperture = <0xe0000000 0x08000000>; /* 128 MiB memory space */
> > > > >   pcie-cfg-aperture = <0xf0000000 0x01000000>; /*  16 MiB config space */
> > > > >   pcie-io-aperture  = <0xf2000000 0x00100000>; /*   1 MiB I/O space */
> > > > >
> > > > >   pcie {
> > > > >     ranges = <0x82000000 0 0x40000     MBUS_ID(0xf0, 0x01) 0x40000  0x0 0x2000>,    /* Port 0.0 Internal registers */
> > > > >              <0x82000000 0 0xf0000000  MBUS_ID(0x04, 0x79) 0        0x0 0x1000000>, /* Port 0.0 Config space */
> > > >
> > > > IMO, this should be 0 for first cell as this is config space. What is
> > > > 0xf0000000 as that's supposed to be an address in PCI address space.
> > >
> > > Which value should be 0? I did not catch it here.
> > 
> > s/0x82000000/0/
> > 
> > But really that's 0 for the hi byte and BDF for the lower bytes.
> 
> But then, this line would describe "PCI device", not the address range
> (if we should follow that pci2_1.pdf).
> 
> Also note that kernel accept only 0xX2000000 and 0xX1000000 values (MEM
> and IO).
> 
> > > 0xf0000000 is just start offset for the child address range. I chose it
> > > "randomly" to not conflict with 0x40000 offset for child address range
> > > which describe internal PCIe controller registers.
> > >
> > > > >              <0x82000000 1 0x0         MBUS_ID(0x04, 0x59) 0        0x1 0x0>,       /* Port 0.0 Mem */
> > > > >              <0x81000000 1 0x0         MBUS_ID(0x04, 0x51) 0        0x1 0x0>,       /* Port 0.0 I/O */
> > > >
> > > > I/O space at 4GB? It's only 32-bits. I guess this is already there
> > > > from the mvebu binding, but it seems kind of broken...
> > >
> > > You have not looked how it works. mvebu is 32-bit CPU and this describe
> > > whole address range which could possibly used for IO or MEM for any
> > > (unspecified) PCIe controller.
> > 
> > I did look. It's just an intermediate virtual/random/made-up address
> > space. It kind of follows PCI bus addressing, but then not really.
> 
> Exactly!
> 
> > > Every PCIe controller has its own PCIe MEM and PCIe IO address range
> > > which are not shared with other PCIe controllers. (non-mvebu platforms
> > > use for this different PCI domains; mvebu not)
> > 
> > Except there's this shared 'PCI like' bus each port is on.
> 
> I mean that in HW.
> 
> > > These "shared" ranges are then dynamically split into PCIe controllers
> > > like their drivers ask. So at the end all 10 PCIe controllers which are
> > > on AXP ask for maximum (64 kB) of PCIe IO and allocator then choose 10
> > > 64kB non-interleaving ranges from this "big" master range. Same for PCIe
> > > MEM.
> > >
> > > So basically those two lines (one for MEM and one for IO) just says that
> > > "unspecified" amount of PCIe MEM is mapped to MBUS_ID(0x04, 0x59) and
> > > another "unspecified" amount of PCIe IO is mapped to MBUS_ID(0x04, 0x51)
> > > mbus and pci-mvebu.c drivers than takes care for proper allocation of
> > > required memory from dynamic pool defined in pcie-mem-aperture and
> > > pcie-io-aperture properties.
> > 
> > That sounds shared to me...
> 
> In device tree it looks like shared, but in HW it is not shared.
> 
> > >
> > > Note that this is not something which I invented, this is used for a
> > > long time by mvebu drivers and device tree files.
> > >
> > > > >
> > > > >     pcie@1,0 {
> > > > >       reg = <0x0800 0 0 0 0>; /* BDF 0:1.0 */
> > > > >       assigned-addresses =     <0x82000800 0 0x40000     0x0 0x2000>,     /* Port 0.0 Internal registers */
> > > > >                                <0x82000800 0 0xf0000000  0x0 0x1000000>;  /* Port 0.0 Config space */
> > > >
> > > > This says it is memory space, not config space.
> > >
> > > "config space" for Orion (in from driver point of view) is just ordinary
> > > "memory space".
> > 
> > "memory space" means PCI memory space which it is not. Almost every
> > Arm platform has config space in ordinary host/cpu address space.
> 
> Yes. This is not PCI (memory space). And it applies also for internal
> registers range, which is prevent in all existing mvebu dts files.
> 
> Both assigned-addresses in above example (and also in all existing mvebu
> dts files) describes host/cpu address space. Not PCI address space.
> 
> > > "memory space" is accessed by load/store instructions. I/O space is
> > > accessed by inb/outb (IO instructions or what architecture has) and
> > > "config space" is accessed by kernel drivers by its own drivers.
> > >
> > > So I think it is correct to declare this range of address space as
> > > "memory".
> > 
> > It is not if we are talking PCI addresses. If this is Marvell pseudo
> > virtual PCI addresses, then you can declare it whatever you want.
> 
> Yes, lets say it is "marvell pseudo pci address" which is not pci
> address at all. But is of type "memory", not "io".
> 
> > > > But the PCI binding
> > > > says config space is not allowed in assigned-addresses.
> > >
> > > Exactly. And this is because it does not make sense to assign "config
> > > space" into "assigned-addresses" as described in:
> > > https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf
> > >
> > > My understanding of above document is that "reg" property in following
> > > code describes "config space".
> > >
> > >   pcie@1,0 {
> > >     reg = <0x0800 0 0 0 0>; /* BDF 0:1.0 */
> > >   }
> > >
> > > And reason is that because above document use "config space" terminology
> > > for describing specific PCI device on the bus.
> > >
> > > Address part after the at-char (@1,0) in device tree should match "reg"
> > > property and by all above definitions from pci2_1.pdf this pass because:
> > >
> > > 1) that document describes @1,0 format that is "config space" for PCI
> > >    device with BDF X:1.0 where X is the parent bus
> > >
> > > 2) 0x0800 is really describes BDF X:1.0
> > >
> > > And then the "config space is not allowed in assigned-addresses" makes
> > > sense as there is absolutely no need to put "pointer" to PCI device
> > > (with BDF address) into "assigned-addresses" property. In this property
> > > should be some memory range and not reference to some PCI device.
> > >
> > > > I think the parent ranges needs a config space entry with the BDF for
> > > > each root port and then this entry should be dropped. It really looks
> > > > to me like the mvebu binding created these fake PCI addresses to map
> > > > root ports back to MBUS addresses when BDF could have been used
> > > > instead.
> > > >
> > > > Rob
> > >
> > > The reason is that in mvebu there are X independent single-root-port
> > > PCIe controllers (PCIe host bridges; PCI domains; ...) and device tree
> > > binding was defined that all root ports (with their host bridges) would
> > > be put into one "virtual" DT node which would act as one PCI domain.
> > 
> > If they are independent, why is there 1 mem and i/o aperture?
> > 
> > I don't know how a virtual DT node for purposes of creating a single
> > PCI domain ever passed review. Must have been explained differently at
> > the time.
> 
> Well, I do not know. I took that pci mvebu code as it was. I spend lot
> of time understanding how is that marvell pcie hw working and how it is
> implemented in linux kernel drivers and how it is defined in device
> tree.
> 
> And I already saw that even marvell people did not understand how their
> pcie hw is working; so it possible that nobody had all architecture and
> hw information about this design at the time of device tree review.
> 
> Anyway, this is something which already exists, it is used and cannot be
> changed for backward compatibility.
> 
> > > And this leads to the fact that all controller specific settings must be
> > > defined in pci root ports.
> > 
> > Not necessarily. There's no reason settings can't be in the parent node.
> > 
> > Rob
> 
> Well, you can put it also into the parent node, but then you need to
> somehow say which internal registers range describe which pcie root port
> (=pcie controller). For me the logical option is to describe it is in
> root port (=pcie controller) node as it the node, which needs it in hw.
> 
> But also this is how it is already used.