| Message ID | 1464858585-10963-2-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday, June 2, 2016 11:09:43 AM CEST Thomas Petazzoni wrote: > + ranges = <0x82000000 0 0xe8000000 0 0xe8000000 0 0x1000000 /* Port 0 MEM */ > + 0x81000000 0 0xe9000000 0 0xe9000000 0 0x10000>; /* Port 0 IO*/ > Any reason for not having a 64-bit MEM prefetchable area in the example? Does the host not support that? Arnd
> +In addition, the Device Tree describing an Aardvark PCIe controller > +must include a sub-node that describes the legacy interrupt controller > +built into the PCIe controller. This sub-node must have the following > +properties: > + > + - interrupt-controller > + - #interrupt-cells: set to <1> > + > +Example: > + > + pcie0: pcie@d0070000 { > + compatible = "marvell,armada-3700-pcie"; > + device_type = "pci"; > + status = "disabled"; > + reg = <0 0xd0070000 0 0x20000>; > + #address-cells = <3>; > + #size-cells = <2>; > + bus-range = <0x00 0xff>; > + interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>; > + #interrupt-cells = <1>; > + ranges = <0x82000000 0 0xe8000000 0 0xe8000000 0 0x1000000 /* Port 0 MEM */ > + 0x81000000 0 0xe9000000 0 0xe9000000 0 0x10000>; /* Port 0 IO*/ > + interrupt-map-mask = <0 0 0 7>; > + interrupt-map = <0 0 0 1 &pcie_intc 0>, > + <0 0 0 2 &pcie_intc 1>, > + <0 0 0 3 &pcie_intc 2>, > + <0 0 0 4 &pcie_intc 3>; > + pcie_intc: interrupt-controller { > + interrupt-controller; > + #interrupt-cells = <1>; > + }; Hi Thomas It is possible to list PCIe devices on the bus here as child nodes. I've done this when i needed a phandle to an intel ethernet controller on the PCIe bus, which i know is soldered onto the board. I think your current implementation simply uses the first child node. It would be good to document that ordering is important. It must be the first child node, and any pcie devices children must come afterwards. Andrew
On Thursday, June 2, 2016 2:24:05 PM CEST Andrew Lunn wrote: > > It is possible to list PCIe devices on the bus here as child > nodes. I've done this when i needed a phandle to an intel ethernet > controller on the PCIe bus, which i know is soldered onto the board. > > I think your current implementation simply uses the first child > node. It would be good to document that ordering is important. It must > be the first child node, and any pcie devices children must come > afterwards. I think some other PCI hosts just move the interrupt-controller and #interrupt-cells properties into the PCI host node itself, which avoids the ambiguity here. Arnd
Hello, On Thu, 02 Jun 2016 14:34:14 +0200, Arnd Bergmann wrote: > I think some other PCI hosts just move the interrupt-controller > and #interrupt-cells properties into the PCI host node itself, > which avoids the ambiguity here. Do you have an example of this? I have modeled my DT binding after the one used by the pci-dra7xx driver, which is in the same situation as I am in terms of interrupt handling. But I can indeed try to make the top-level PCIe controller node the interrupt controller. Let me try that quickly. Best regards, Thomas
On Thursday, June 2, 2016 2:45:42 PM CEST Thomas Petazzoni wrote: > > On Thu, 02 Jun 2016 14:34:14 +0200, Arnd Bergmann wrote: > > > I think some other PCI hosts just move the interrupt-controller > > and #interrupt-cells properties into the PCI host node itself, > > which avoids the ambiguity here. > > Do you have an example of this? I have modeled my DT binding after the > one used by the pci-dra7xx driver, which is in the same situation as I > am in terms of interrupt handling. > > But I can indeed try to make the top-level PCIe controller node the > interrupt controller. Let me try that quickly. > Looking at the binding files, I see only this one: Documentation/devicetree/bindings/pci/altera-pcie.txt and a couple of others using a child node. Arnd
Hello, Thanks for your review! On Thu, 02 Jun 2016 11:35:38 +0200, Arnd Bergmann wrote: > On Thursday, June 2, 2016 11:09:43 AM CEST Thomas Petazzoni wrote: > > + ranges = <0x82000000 0 0xe8000000 0 0xe8000000 0 0x1000000 /* Port 0 MEM */ > > + 0x81000000 0 0xe9000000 0 0xe9000000 0 0x10000>; /* Port 0 IO*/ > > > > Any reason for not having a 64-bit MEM prefetchable area in the example? > Does the host not support that? I'll have to admit I am not sure how to find this out from the datasheet. My datasheet says about the PCIe controller: """ 64-bit PCIe address and system address space for outbound transactions """ So I guess this would indicate that a 64-bit MEM area is possible. However, since anyway the area used above is at 0xe8000000 for a length of 0x1000000, what would be the benefit of declaring this range as a 64-bit one ? Regarding the prefetchable aspect, I couldn't find any reference in the datasheet. However, the original driver code explicitly errors out if there is no non-prefetchable memory area, so I guess prefetchable areas is not supported. In of_bus_pci_get_flags(), both the 32-bit and 64-bit cases are handled in the same way, so is this distinction actually being used by the kernel? Thomas
Hello, On Thu, 02 Jun 2016 14:34:14 +0200, Arnd Bergmann wrote: > I think some other PCI hosts just move the interrupt-controller > and #interrupt-cells properties into the PCI host node itself, > which avoids the ambiguity here. I've tried that, and it works fine. Makes the code simpler, the binding simpler, so: adopted! Thanks for suggesting! Thomas
On Wednesday, June 8, 2016 4:27:50 PM CEST Thomas Petazzoni wrote: > Hello, > > Thanks for your review! > > On Thu, 02 Jun 2016 11:35:38 +0200, Arnd Bergmann wrote: > > On Thursday, June 2, 2016 11:09:43 AM CEST Thomas Petazzoni wrote: > > > + ranges = <0x82000000 0 0xe8000000 0 0xe8000000 0 0x1000000 /* Port 0 MEM */ > > > + 0x81000000 0 0xe9000000 0 0xe9000000 0 0x10000>; /* Port 0 IO*/ > > > > > > > Any reason for not having a 64-bit MEM prefetchable area in the example? > > Does the host not support that? > > I'll have to admit I am not sure how to find this out from the > datasheet. My datasheet says about the PCIe controller: > > """ > 64-bit PCIe address and system address space for outbound transactions > """ > > So I guess this would indicate that a 64-bit MEM area is possible. > However, since anyway the area used above is at 0xe8000000 for a length > of 0x1000000, what would be the benefit of declaring this range as a > 64-bit one ? > > Regarding the prefetchable aspect, I couldn't find any reference in the > datasheet. However, the original driver code explicitly errors out if > there is no non-prefetchable memory area, so I guess prefetchable > areas is not supported. > > In of_bus_pci_get_flags(), both the 32-bit and 64-bit cases are handled > in the same way, so is this distinction actually being used by the > kernel? Each device needs to have at least one non-prefetcheable range, which is why some drivers check for that. Non-prefetchable BARs always use 32-bit addressing, so 64-bit BARs are by definition prefetchable, but you can also have prefetchable registers in the first 4GB in some cases. Some devices require large prefetchable BARs (hundreds of MB, or more), so if you have the option, you should enable that in the host, as the 16MB you have available for non-prefetchable devices is not going to be sufficient. Arnd
On Thu, Jun 02, 2016 at 11:09:43AM +0200, Thomas Petazzoni wrote: > This commit adds the documentation for the Device Tree binding used to > describe the Aardvark PCIe controller, found on Marvell Armada 3700 > ARM64 SoCs. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Hi Thomas, If you post a v2 of these, can you align the subject lines of these binding, DT, and driver patches so it's easier to find the related pieces, e.g., add "Armada 3700" to this one or "Aardvark" to the others? It looks like maybe Aardvark could be used in several different systems, so maybe this patch should stay the same and the drivers/pci patch should mention Aardvark instead of only "Marvell Armada 3700"? Bjorn
Hello, On Fri, 10 Jun 2016 10:44:14 -0500, Bjorn Helgaas wrote: > If you post a v2 of these, can you align the subject lines of these > binding, DT, and driver patches so it's easier to find the related > pieces, e.g., add "Armada 3700" to this one or "Aardvark" to the > others? It looks like maybe Aardvark could be used in several > different systems, so maybe this patch should stay the same and the > drivers/pci patch should mention Aardvark instead of only "Marvell > Armada 3700"? Sure thing. I was about to send a v2, so I've fixed up the commit titles so that they all include Aardvark. Thomas
diff --git a/Documentation/devicetree/bindings/pci/aardvark-pci.txt b/Documentation/devicetree/bindings/pci/aardvark-pci.txt new file mode 100644 index 0000000..3517234 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/aardvark-pci.txt @@ -0,0 +1,51 @@ +Aardvark PCIe controller + +This PCIe controller is used on the Marvell Armada 3700 ARM64 SoC. + +The Device Tree node describing an Aardvark PCIe controller must +contain the following properties: + + - compatible: Should be "marvell,armada-3700-pcie" + - reg: range of registers for the PCIe controller + - interrupts: the interrupt line of the PCIe controller + - #address-cells: set to <3> + - #size-cells: set to <2> + - device_type: set to "pci" + - ranges: ranges for the PCI memory and I/O regions + - #interrupt-cells: set to <1> + - interrupt-map-mask and interrupt-map: standard PCI properties to + define the mapping of the PCIe interface to interrupt numbers. + - bus-range: PCI bus numbers covered + +In addition, the Device Tree describing an Aardvark PCIe controller +must include a sub-node that describes the legacy interrupt controller +built into the PCIe controller. This sub-node must have the following +properties: + + - interrupt-controller + - #interrupt-cells: set to <1> + +Example: + + pcie0: pcie@d0070000 { + compatible = "marvell,armada-3700-pcie"; + device_type = "pci"; + status = "disabled"; + reg = <0 0xd0070000 0 0x20000>; + #address-cells = <3>; + #size-cells = <2>; + bus-range = <0x00 0xff>; + interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>; + #interrupt-cells = <1>; + ranges = <0x82000000 0 0xe8000000 0 0xe8000000 0 0x1000000 /* Port 0 MEM */ + 0x81000000 0 0xe9000000 0 0xe9000000 0 0x10000>; /* Port 0 IO*/ + interrupt-map-mask = <0 0 0 7>; + interrupt-map = <0 0 0 1 &pcie_intc 0>, + <0 0 0 2 &pcie_intc 1>, + <0 0 0 3 &pcie_intc 2>, + <0 0 0 4 &pcie_intc 3>; + pcie_intc: interrupt-controller { + interrupt-controller; + #interrupt-cells = <1>; + }; + };
This commit adds the documentation for the Device Tree binding used to describe the Aardvark PCIe controller, found on Marvell Armada 3700 ARM64 SoCs. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- .../devicetree/bindings/pci/aardvark-pci.txt | 51 ++++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/aardvark-pci.txt