Subject: [RFC PATCH 3/3] dt-bindings: pci: xgene pcie device tree bindings

From: [email-masked] (Arnd Bergmann)
Date: Fri, 3 Jan 2014 10:49:20 +0100

On Monday 23 December 2013, Tanmay Inamdar wrote:
> This patch adds the bindings for X-Gene PCIe driver. The driver resides
> under 'drivers/pci/host/pcie-xgene.c' file.
> 
> Signed-off-by: Tanmay Inamdar <[email-masked]>
> ---
>  .../devicetree/bindings/pci/xgene-pcie.txt         |   43 ++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/xgene-pcie.txt
> 
> diff --git a/Documentation/devicetree/bindings/pci/xgene-pcie.txt b/Documentation/devicetree/bindings/pci/xgene-pcie.txt
> new file mode 100644
> index 0000000..d92da4f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/xgene-pcie.txt
> @@ -0,0 +1,43 @@
> +* AppliedMicro X-Gene PCIe interface
> +
> +Required properties:
> +- status: Either "ok" or "disabled".
> +- device_type: set to "pci"
> +- compatible: should contain "xgene,pcie" to identify the core.
> +- reg: base addresses and lengths of the pcie controller configuration
> +	space register.
> +- #address-cells: set to <3>
> +- #size-cells: set to <2>
> +- ranges: ranges for the PCI memory, I/O regions, config and MSI 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.
> +- clocks: from common clock binding: handle to pci clock.
> +- clock-names: from common clock binding. Should be "pcieclk".
> +

Better use an anonymous clock?

The driver also seems to use a phy that is not defined here.

> +Example:
> +
> +SoC specific DT Entry:
> +	pcie0: pcie at 1f2b0000 {
> +		status = "disabled";
> +		device_type = "pci";
> +		compatible = "xgene,pcie";
> +		#interrupt-cells = <1>;
> +		#size-cells = <2>;
> +		#address-cells = <3>;
> +		reg = < 0x00 0x1f2b0000 0x0 0x00010000>;
> +		ranges = <0x02000000 0x0 0x00000000 0xe0 0x00000000 0x0 0x10000000 /* mem*/

This is an awfully small memory space for a 64 bit machine. Is that a hardware bug you
are working around? If not, please make it as large as you can to allow for arbitrary
extension cards with large BARs,@least 4GB.

Also, do you support no prefetchable memory?

> +			  0x01000000 0x0 0x80000000 0xe0 0x80000000 0x0 0x00010000 /* io */

I/O space at 0x80000000? That won't work with a lot of devices. Can you make it start
at bus address 0?

> +			  0x00000000 0x0 0xd0000000 0xe0 0xd0000000 0x0 0x00200000 /* cfg */

config space is not normally in the ranges property, and I think you will need
it in the pcie node itself as a 'reg' property so the code can access it.

> +			  0x00000000 0x0 0x79000000 0x00 0x79000000 0x0 0x00800000>; /* msi */

Same here. Also I suspect this is a separate irqchip that isn't actually part
of the PCIe host. If they are separate devices, I'd strongly recommend modeling
them as separate device-nodes in DT to make reuse easier.

> +		interrupt-map-mask = <0x0 0x0 0x0 0x7>;
> +		interrupt-map = <0x0 0x0 0x0 0x1 &gic 0x0 0xc2 0x1>;

Only one IRQ for all devices?

> +		clocks = <&pcie0clk 0>;
> +		clock-names = "pcieclk"
> +	};
> +
> +Board specific DT Entry:
> +	&pcie0 {
> +		status = "ok";
> +	};

	Arnd