Re: [PATCH 2/5] dt-bindings: PCI: cadence: Add DT bindings for Cadence PCIe host controller

[Rob Herring]

On Thu, Nov 23, 2017 at 04:01:47PM +0100, Cyrille Pitchen wrote:
> From: Scott Telford <stelford@xxxxxxxxxxx>
> 
> This patch adds documentation for the DT bindings of the Cadence PCIe
> controller when configured in host (Root Complex) mode.
> 
> Signed-off-by: Scott Telford <stelford@xxxxxxxxxxx>
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@xxxxxxxxxxxxxxxxxx>
> 
> dt-bindings: PCI: cadence: host fixup
> ---
>  .../bindings/pci/cdns,cdns-pcie-host.txt           | 54 ++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt

A few nits, otherwise:

Acked-by: Rob Herring <robh@xxxxxxxxxx>

> 
> diff --git a/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
> new file mode 100644
> index 000000000000..4b3df8ffd5e1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/cdns,cdns-pcie-host.txt
> @@ -0,0 +1,54 @@
> +* Cadence PCIe host controller
> +
> +This PCIe controller inherits the base properties defined in
> +host-generic-pci.txt.
> +
> +Required properties:
> +- compatible: should contain "cdns,cdns-pcie-host" to identify the IP used.
> +- reg: Should contain the PCIe configuration window base address, controller
> +  register base address, and AXI interface region base address respectively.
> +- reg-names: Must be "cfg", "reg" and "mem" respectively.
> +- #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.
> +
> +Example:
> +
> +	pci@fb000000 {

pcie@...

Usually the first address is the unit-address, so either change the 
order in reg or change this.

> +		compatible = "cdns,cdns-pcie-host";
> +		device_type = "pci";
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		bus-range = <0x0 0xff>;
> +		linux,pci-domain = <0>;
> +
> +		// CPU_PHYSICAL(2)  SIZE(2)

Drop the comment. If you know what 'reg' is then it is obvious. Also, 
don't use C++ style comments.

> +		reg = <0x0 0x41000000  0x0 0x00001000>, /* Config space */
> +		      <0x0 0xfb000000  0x0 0x01000000>, /* Base Address */
> +		      <0x0 0x40000000  0x0 0x04000000>; /* AXI region */
> +		reg-names = "cfg", "reg", "mem";
> +
> +		// BUS_ADDRESS(3)  CPU_PHYSICAL(1)  SIZE(2)

ditto

> +		ranges =
> +			<0x02000000 0x0 0x42000000  0x0 0x42000000  0x0 0x1000000>,
> +			<0x01000000 0x0 0x43000000  0x0 0x43000000  0x0 0x0010000>;
> +
> +
> +		#interrupt-cells = <0x1>;
> +
> +		// PCI_DEVICE(3)  INT#(1)  CONTROLLER(PHANDLE)  CONTROLLER_DATA(5)
> +		interrupt-map = <0x0 0x0 0x0  0x1  &gic  0x0 0x0 0x0 14 0x1
> +				 0x0 0x0 0x0  0x2  &gic  0x0 0x0 0x0 15 0x1
> +				 0x0 0x0 0x0  0x3  &gic  0x0 0x0 0x0 16 0x1
> +				 0x0 0x0 0x0  0x4  &gic  0x0 0x0 0x0 17 0x1>;
> +
> +		// PCI_DEVICE(3)  INT#(1)
> +		interrupt-map-mask = <0x0000 0x0 0x0  0x7>;
> +
> +		msi-parent = <&its_pci>;
> +	};
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html