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