diff mbox

[1/3] dt-bindings: add DT binding for the Aardvark PCIe controller

Message ID 1464858585-10963-2-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Thomas Petazzoni June 2, 2016, 9:09 a.m. UTC
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

Comments

Arnd Bergmann June 2, 2016, 9:35 a.m. UTC | #1
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

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Lunn June 2, 2016, 12:24 p.m. UTC | #2
> +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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann June 2, 2016, 12:34 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Petazzoni June 2, 2016, 12:45 p.m. UTC | #4
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
Arnd Bergmann June 2, 2016, 1:53 p.m. UTC | #5
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

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Petazzoni June 8, 2016, 2:27 p.m. UTC | #6
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
Thomas Petazzoni June 8, 2016, 2:28 p.m. UTC | #7
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
Arnd Bergmann June 8, 2016, 3:34 p.m. UTC | #8
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas June 10, 2016, 3:44 p.m. UTC | #9
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Petazzoni June 10, 2016, 3:47 p.m. UTC | #10
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 mbox

Patch

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>;
+		};
+	};