diff mbox series

[v2,1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller

Message ID 20210120101554.241029-1-xxm@rock-chips.com (mailing list archive)
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series [v2,1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller | expand

Commit Message

Simon Xue Jan. 20, 2021, 10:15 a.m. UTC 
Signed-off-by: Simon Xue <xxm@rock-chips.com>
---
 .../bindings/pci/rockchip-dw-pcie.yaml        | 140 ++++++++++++++++++
 1 file changed, 140 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml

Comments

Rob Herring (Arm) Jan. 20, 2021, 2:07 p.m. UTC | #1 
On Wed, 20 Jan 2021 18:15:53 +0800, Simon Xue wrote:
> Signed-off-by: Simon Xue <xxm@rock-chips.com>
> ---
>  .../bindings/pci/rockchip-dw-pcie.yaml        | 140 ++++++++++++++++++
>  1 file changed, 140 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml:39:7: [warning] wrong indentation: expected 4 but found 6 (indentation)

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml: properties:interrupt: [{'description': 'system information'}, {'description': 'power management control'}, {'description': 'PCIe message'}, {'description': 'legacy interrupt'}, {'description': 'error report'}] is not of type 'object', 'boolean'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml: properties:compatible: Additional properties are not allowed ('item' was unexpected)
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml: properties:compatible: 'item' is not one of ['$ref', 'additionalItems', 'additionalProperties', 'allOf', 'anyOf', 'const', 'contains', 'default', 'dependencies', 'deprecated', 'description', 'else', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'items', 'if', 'minItems', 'minimum', 'maxItems', 'maximum', 'multipleOf', 'not', 'oneOf', 'pattern', 'patternProperties', 'properties', 'required', 'then', 'type', 'typeSize', 'unevaluatedProperties', 'uniqueItems']
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml: properties:compatible: Additional properties are not allowed ('item' was unexpected)
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml: ignoring, error in schema: properties: interrupt
warning: no schema found in file: ./Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
Documentation/devicetree/bindings/pci/rockchip-dw-pcie.example.dts:19:18: fatal error: dt-bindings/clock/rk3568-cru.h: No such file or directory
   19 |         #include <dt-bindings/clock/rk3568-cru.h>
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[1]: *** [scripts/Makefile.lib:344: Documentation/devicetree/bindings/pci/rockchip-dw-pcie.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1370: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1429132

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
Johan Jonker Jan. 20, 2021, 5:07 p.m. UTC | #2 
Hi Simon,

Thanks you for version 2.
A few comments, have a look if it is useful or that you disagree.

This patch has no commit message. Add one in version 3.

Submit all patches in one batch with the same sort message ID to all
maintainers including Heiko.

Heiko Stuebner <heiko@sntech.de>

Example message ID:
20210120101554.241029-1-xxm@rock-chips.com

/////

Included is a copy of the Rockchip pcie nodes in a sort of test.dts below.
Could you confirm that the properties in that dts are the one that we
can expect for Linux mainline and can base our YAML document on?

With rk3568-cru.h and rk3568-power.h manualy added we do some tests with
the following commands:

make ARCH=arm64 dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml

make ARCH=arm64 dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml

make ARCH=arm64 dtbs_check
DT_SCHEMA_FILES=~/.local/lib/python3.5/site-packages/dtschema/schemas/pci/pci-bus.yaml

/////

Example notifications:

/arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: reg: [[3,
3225419776, 0, 4194304], [0, 4263968768, 0, 65536]] is too long

/arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: ranges:
'oneOf' conditional failed, one must be fixed:

Before you submit version 3 make sure that all warnings gone as much as
possible.

On 1/20/21 11:15 AM, Simon Xue wrote:
> Signed-off-by: Simon Xue <xxm@rock-chips.com>
> ---
>  .../bindings/pci/rockchip-dw-pcie.yaml        | 140 ++++++++++++++++++
>  1 file changed, 140 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> new file mode 100644
> index 000000000000..9d3a57f5305e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> @@ -0,0 +1,140 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: DesignWare based PCIe RC controller on Rockchip SoCs
> +
> +maintainers:
> +  - Shawn Lin <shawn.lin@rock-chips.com>
> +  - Simon Xue <xxm@rock-chips.com>
     - Heiko Stuebner <heiko@sntech.de> ;)
> +
> +description: |+
> +  RK3568 SoC PCIe host controller is based on the Synopsys DesignWare
> +  PCIe IP and thus inherits all the common properties defined in
> +  designware-pcie.txt.
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +
> +# We need a select here so we don't match all nodes with 'snps,dw-pcie'
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        const: rockchip,rk3568-pcie
> +  required:
> +    - compatible
> +
> +properties:
> +  compatible:

> +    item:

    items:

> +      - const: rockchip,rk3568-pcie
> +      - const: snps,dw-pcie

Add empty line

> +  reg:    items:
      - description:
      - description:

Add some description for regs.

> +    maxItems: 1
remove

This reg maxItems gives errors.

> +

> +  interrupt:
interrupts:
   items:

> +      - description: system information
> +      - description: power management control
> +      - description: PCIe message
> +      - description: legacy interrupt
> +      - description: error report
> +
> +  interrupt-names:
> +    items:
> +      - const: sys
> +      - const: pmc
> +      - const: msg
> +      - const: legacy
> +      - const: err
> +
> +  clocks:
> +    items:
> +      - description: AHB clock for PCIe master
> +      - description: AHB clock for PCIe slave
> +      - description: AHB clock for PCIe dbi
> +      - description: APB clock for PCIe
> +      - description: Auxiliary clock for PCIe
> +
> +  clock-names:
> +    items:
> +      - const: aclk_mst
> +      - const: aclk_slv
> +      - const: aclk_dbi
> +      - const: pclk
> +      - const: aux
> +
> +  msi-map: true
> +
> +  power-domains:
> +    maxItems: 1

/////
These properties come from designware-pcie.txt
Maybe add them here for now till there's a common yaml?

  num-ib-windows: number of inbound address translation windows
  num-ob-windows: number of outbound address translation windows

Optional properties:
  num-lanes:

/////

  phys:
    maxItems: 1

  phy-names:
    const: pcie-phy

ranges:
     ......
     ......
This ranges needs a fix so that it doesn't generate notifications.
See above example.

> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:

        const: pipe

> +    items:
> +      - const: pipe

remove

> +
> +required:
> +  - compatible
> +  - bus-range
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - msi-map
> +  - num-lanes
> +  - phys
> +  - phy-names
> +  - power-domains
> +  - resets
> +  - reset-names

Add also all the other properties that are defined in this binding and
are required. But not the ones from pci-bus.yaml.

> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |

> +    #include <dt-bindings/clock/rk3568-cru.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/power/rk3568-power.h>

If #include is not possible for now replace all defines by there
original numbers.
Just to get this binding pass for mainline.

> +
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pcie3x2: pcie@fe280000 {
> +            compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";

dts sort order is:

compatible
reg
interrupts
the rest
things with #

> +            reg = <0x3 0xc0800000 0x0 0x400000>,
> +                  <0x0 0xfe280000 0x0 0x10000>;
> +            reg-names = "pcie-dbi", "pcie-apb";
> +            interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>;

> +            interrupt-names = "sys", "pmc", "msg", "legacy", "err";

Could you confirm that these "interrupt-names" are used in your driver?
Else remove.

            bus-range = <0x20 0x2f>;

> +            clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>,
> +                     <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>,
> +                     <&cru CLK_PCIE30X2_AUX_NDFT>;
> +            clock-names = "aclk_mst", "aclk_slv",
> +                          "aclk_dbi", "pclk",
> +                          "aux";

		device_type = "pci";
		linux,pci-domain = <0>;
		num-ib-windows = <6>;
		num-ob-windows = <2>;
		max-link-speed = <2>;

Maybe give a complete example?

> +            msi-map = <0x2000 &its 0x2000 0x1000>;
> +            num-lanes = <2>;
> +            phys = <&pcie30phy>;
> +            phy-names = "pcie-phy";
> +            power-domains = <&power RK3568_PD_PIPE>;
> +            ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000
> +                      0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000
> +                      0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>;
> +            resets = <&cru SRST_PCIE30X2_POWERUP>;
> +            reset-names = "pipe";

            #address-cells = <3>;
            #size-cells = <2>;

> +        };
> +    };
> +...
> 

///////////

test.dts

///////////
/dts-v1/;
#include <dt-bindings/clock/rk3568-cru.h>
#include <dt-bindings/interrupt-controller/arm-gic.h>
#include <dt-bindings/interrupt-controller/irq.h>
#include <dt-bindings/phy/phy.h>
#include <dt-bindings/power/rk3568-power.h>

/ {
	compatible = "rockchip,rk3568";

	#address-cells = <2>;
	#size-cells = <2>;

	cru: clock-controller@fdd20000 {
		compatible = "rockchip,rk3568-cru";
		reg = <0x0 0xfdd20000 0x0 0x1000>;
		#clock-cells = <1>;
		#reset-cells = <1>;
	};

	pcie30phy: phy@fe8c0000 {
		compatible = "rockchip,rk3568-pcie3-phy";
		reg = <0x0 0xfe8c0000 0x0 0x20000>;
		#phy-cells = <0>;
	};

	combphy2_psq: phy@fe840000 {
		compatible = "rockchip,rk3568-naneng-combphy";
		reg = <0x0 0xfe840000 0x0 0x100>;
		#phy-cells = <1>;
	};

	gic: interrupt-controller@fd400000 {
		compatible = "arm,gic-v3";
		#interrupt-cells = <3>;
		#address-cells = <2>;
		#size-cells = <2>;
		ranges;
		interrupt-controller;

		reg = <0x0 0xfd400000 0 0x10000>,
		      <0x0 0xfd460000 0 0xc0000>;
		interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
		its: interrupt-controller@fd440000 {
			compatible = "arm,gic-v3-its";
			msi-controller;
			#msi-cells = <1>;
			reg = <0x0 0xfd440000 0x0 0x20000>;
			status = "disabled";
		};
	};

	pmu: power-management@fdd90000 {
		compatible = "rockchip,rk3568-pmu", "syscon", "simple-mfd";
		reg = <0x0 0xfdd90000 0x0 0x1000>;

		power: power-controller {
			compatible = "rockchip,rk3568-power-controller";
			#power-domain-cells = <1>;
			#address-cells = <1>;
			#size-cells = <0>;
			status = "okay";
		};
	};

	pcie2x1: pcie@fe260000 {
		compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";
		reg = <0x3 0xc0000000 0x0 0x400000>,
		      <0x0 0xfe260000 0x0 0x10000>;
		reg-names = "pcie-dbi", "pcie-apb";
		interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
		interrupt-names = "sys", "pmc", "msg", "legacy", "err";
		bus-range = <0x0 0xf>;
		clocks = <&cru ACLK_PCIE20_MST>, <&cru ACLK_PCIE20_SLV>,
			 <&cru ACLK_PCIE20_DBI>, <&cru PCLK_PCIE20>,
			 <&cru CLK_PCIE20_AUX_NDFT>;
		clock-names = "aclk_mst", "aclk_slv",
			      "aclk_dbi", "pclk", "aux";
		device_type = "pci";
		linux,pci-domain = <0>;
		num-ib-windows = <6>;
		num-ob-windows = <2>;
		max-link-speed = <2>;
		msi-map = <0x0 &its 0x0 0x1000>;
		num-lanes = <1>;
		phys = <&combphy2_psq PHY_TYPE_PCIE>;
		phy-names = "pcie-phy";
		power-domains = <&power RK3568_PD_PIPE>;
		ranges = <0x00000800 0x0 0x00000000 0x3 0x00000000 0x0 0x800000
			  0x81000000 0x0 0x00800000 0x3 0x00800000 0x0 0x100000
			  0x83000000 0x0 0x00900000 0x3 0x00900000 0x0 0x3f700000>;
		resets = <&cru SRST_PCIE20_POWERUP>;
		reset-names = "pipe";
		#address-cells = <3>;
		#size-cells = <2>;
		status = "disabled";
	};

	pcie3x1: pcie@fe270000 {
		compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";
		reg = <0x3 0xc0400000 0x0 0x400000>,
		      <0x0 0xfe270000 0x0 0x10000>;
		reg-names = "pcie-dbi", "pcie-apb";
		interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>;
		interrupt-names = "sys", "pmc", "msg", "legacy", "err";
		bus-range = <0x10 0x1f>;
		clocks = <&cru ACLK_PCIE30X1_MST>, <&cru ACLK_PCIE30X1_SLV>,
			 <&cru ACLK_PCIE30X1_DBI>, <&cru PCLK_PCIE30X1>,
			 <&cru CLK_PCIE30X1_AUX_NDFT>;
		clock-names = "aclk_mst", "aclk_slv",
			      "aclk_dbi", "pclk", "aux";
		device_type = "pci";
		linux,pci-domain = <1>;
		num-ib-windows = <6>;
		num-ob-windows = <2>;
		max-link-speed = <3>;
		msi-map = <0x1000 &its 0x1000 0x1000>;
		num-lanes = <1>;
		phys = <&pcie30phy>;
		phy-names = "pcie-phy";
		power-domains = <&power RK3568_PD_PIPE>;
		ranges = <0x00000800 0x0 0x40000000 0x3 0x40000000 0x0 0x800000
			  0x81000000 0x0 0x40800000 0x3 0x40800000 0x0 0x100000
			  0x83000000 0x0 0x40900000 0x3 0x40900000 0x0 0x3f700000>;
		resets = <&cru SRST_PCIE30X1_POWERUP>;
		reset-names = "pipe";
		#address-cells = <3>;
		#size-cells = <2>;
		status = "disabled";
	};

	pcie3x2: pcie@fe280000 {
		compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";
		reg = <0x3 0xc0800000 0x0 0x400000>,
		      <0x0 0xfe280000 0x0 0x10000>;
		reg-names = "pcie-dbi", "pcie-apb";
		interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>,
			     <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>;
		interrupt-names = "sys", "pmc", "msg", "legacy", "err";
		bus-range = <0x20 0x2f>;
		clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>,
			 <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>,
			 <&cru CLK_PCIE30X2_AUX_NDFT>;
		clock-names = "aclk_mst", "aclk_slv",
			      "aclk_dbi", "pclk", "aux";
		device_type = "pci";
		linux,pci-domain = <2>;
		num-ib-windows = <6>;
		num-ob-windows = <2>;
		max-link-speed = <3>;
		msi-map = <0x2000 &its 0x2000 0x1000>;
		num-lanes = <2>;
		phys = <&pcie30phy>;
		phy-names = "pcie-phy";
		power-domains = <&power RK3568_PD_PIPE>;
		ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000
			  0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000
			  0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>;
		resets = <&cru SRST_PCIE30X2_POWERUP>;
		reset-names = "pipe";
		#address-cells = <3>;
		#size-cells = <2>;
		status = "disabled";
	};
};
Simon Xue Jan. 22, 2021, 2:22 a.m. UTC | #3 
Hi Johan,

Thanks for your review, I have some questions, see inline.

在 2021/1/21 1:07, Johan Jonker 写道:
> Hi Simon,
>
> Thanks you for version 2.
> A few comments, have a look if it is useful or that you disagree.
>
> This patch has no commit message. Add one in version 3.
>
> Submit all patches in one batch with the same sort message ID to all
> maintainers including Heiko.
>
> Heiko Stuebner <heiko@sntech.de>
>
> Example message ID:
> 20210120101554.241029-1-xxm@rock-chips.com
>
> /////
>
> Included is a copy of the Rockchip pcie nodes in a sort of test.dts below.
> Could you confirm that the properties in that dts are the one that we
> can expect for Linux mainline and can base our YAML document on?
>
> With rk3568-cru.h and rk3568-power.h manualy added we do some tests with
> the following commands:
>
> make ARCH=arm64 dt_binding_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>
> make ARCH=arm64 dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>
> make ARCH=arm64 dtbs_check
> DT_SCHEMA_FILES=~/.local/lib/python3.5/site-packages/dtschema/schemas/pci/pci-bus.yaml
>
> /////
>
> Example notifications:
>
> /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: reg: [[3,
> 3225419776, 0, 4194304], [0, 4263968768, 0, 65536]] is too long
>
> /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: ranges:
> 'oneOf' conditional failed, one must be fixed:
>
> Before you submit version 3 make sure that all warnings gone as much as
> possible.
>
> On 1/20/21 11:15 AM, Simon Xue wrote:
>> Signed-off-by: Simon Xue <xxm@rock-chips.com>
>> ---
>>   .../bindings/pci/rockchip-dw-pcie.yaml        | 140 ++++++++++++++++++
>>   1 file changed, 140 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>> new file mode 100644
>> index 000000000000..9d3a57f5305e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>> @@ -0,0 +1,140 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: DesignWare based PCIe RC controller on Rockchip SoCs
>> +
>> +maintainers:
>> +  - Shawn Lin <shawn.lin@rock-chips.com>
>> +  - Simon Xue <xxm@rock-chips.com>
>       - Heiko Stuebner <heiko@sntech.de> ;)
>> +
>> +description: |+
>> +  RK3568 SoC PCIe host controller is based on the Synopsys DesignWare
>> +  PCIe IP and thus inherits all the common properties defined in
>> +  designware-pcie.txt.
>> +
>> +allOf:
>> +  - $ref: /schemas/pci/pci-bus.yaml#
>> +
>> +# We need a select here so we don't match all nodes with 'snps,dw-pcie'
>> +select:
>> +  properties:
>> +    compatible:
>> +      contains:
>> +        const: rockchip,rk3568-pcie
>> +  required:
>> +    - compatible
>> +
>> +properties:
>> +  compatible:
>> +    item:
>      items:
>
>> +      - const: rockchip,rk3568-pcie
>> +      - const: snps,dw-pcie
> Add empty line
>
>> +  reg:    items:
>        - description:
>        - description:
>
> Add some description for regs.
>
>> +    maxItems: 1
> remove
>
> This reg maxItems gives errors.
>
>> +
>> +  interrupt:
> interrupts:
>     items:
>
>> +      - description: system information
>> +      - description: power management control
>> +      - description: PCIe message
>> +      - description: legacy interrupt
>> +      - description: error report
>> +
>> +  interrupt-names:
>> +    items:
>> +      - const: sys
>> +      - const: pmc
>> +      - const: msg
>> +      - const: legacy
>> +      - const: err
>> +
>> +  clocks:
>> +    items:
>> +      - description: AHB clock for PCIe master
>> +      - description: AHB clock for PCIe slave
>> +      - description: AHB clock for PCIe dbi
>> +      - description: APB clock for PCIe
>> +      - description: Auxiliary clock for PCIe
>> +
>> +  clock-names:
>> +    items:
>> +      - const: aclk_mst
>> +      - const: aclk_slv
>> +      - const: aclk_dbi
>> +      - const: pclk
>> +      - const: aux
>> +
>> +  msi-map: true
>> +
>> +  power-domains:
>> +    maxItems: 1
> /////
> These properties come from designware-pcie.txt
> Maybe add them here for now till there's a common yaml?
>
>    num-ib-windows: number of inbound address translation windows
>    num-ob-windows: number of outbound address translation windows
No plan to upstream EP function at the moment, I think no need to add 
EP's properties
> Optional properties:
>    num-lanes:
> /////
>
>    phys:
>      maxItems: 1
>
>    phy-names:
>      const: pcie-phy
>
> ranges:
>       ......
>       ......
> This ranges needs a fix so that it doesn't generate notifications.
> See above example.

Do you mean :

ranges:

     maxItems: 3
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +  reset-names:
>          const: pipe
>
>> +    items:
>> +      - const: pipe
> remove
>
>> +
>> +required:
>> +  - compatible
>> +  - bus-range
>> +  - reg
>> +  - reg-names
>> +  - clocks
>> +  - clock-names
>> +  - msi-map
>> +  - num-lanes
>> +  - phys
>> +  - phy-names
>> +  - power-domains
>> +  - resets
>> +  - reset-names
> Add also all the other properties that are defined in this binding and
> are required. But not the ones from pci-bus.yaml.
I can't find pci-bus.yaml, do you mean 
Documentation/devicetree/bindings/pci/pci.txt ?
>
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/rk3568-cru.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/power/rk3568-power.h>
> If #include is not possible for now replace all defines by there
> original numbers.
> Just to get this binding pass for mainline.
Replace  ACLK_PCIE30X2_MST by original number ? also the others.
>> +
>> +    bus {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        pcie3x2: pcie@fe280000 {
>> +            compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";
> dts sort order is:
>
> compatible
> reg
> interrupts
> the rest
> things with #
>
>> +            reg = <0x3 0xc0800000 0x0 0x400000>,
>> +                  <0x0 0xfe280000 0x0 0x10000>;
>> +            reg-names = "pcie-dbi", "pcie-apb";
>> +            interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>,
>> +                         <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>;
>> +            interrupt-names = "sys", "pmc", "msg", "legacy", "err";
> Could you confirm that these "interrupt-names" are used in your driver?
> Else remove.
Will remove
>
>              bus-range = <0x20 0x2f>;
>
>> +            clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>,
>> +                     <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>,
>> +                     <&cru CLK_PCIE30X2_AUX_NDFT>;
>> +            clock-names = "aclk_mst", "aclk_slv",
>> +                          "aclk_dbi", "pclk",
>> +                          "aux";
> 		device_type = "pci";
> 		linux,pci-domain = <0>;
> 		num-ib-windows = <6>;
> 		num-ob-windows = <2>;
> 		max-link-speed = <2>;
>
> Maybe give a complete example?

As mentioned above, no need to add EP's properties here

Simon Xue.

>
>> +            msi-map = <0x2000 &its 0x2000 0x1000>;
>> +            num-lanes = <2>;
>> +            phys = <&pcie30phy>;
>> +            phy-names = "pcie-phy";
>> +            power-domains = <&power RK3568_PD_PIPE>;
>> +            ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000
>> +                      0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000
>> +                      0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>;
>> +            resets = <&cru SRST_PCIE30X2_POWERUP>;
>> +            reset-names = "pipe";
>              #address-cells = <3>;
>              #size-cells = <2>;
>
>> +        };
>> +    };
>> +...
>>
> ///////////
>
> test.dts
>
> ///////////
> /dts-v1/;
> #include <dt-bindings/clock/rk3568-cru.h>
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> #include <dt-bindings/interrupt-controller/irq.h>
> #include <dt-bindings/phy/phy.h>
> #include <dt-bindings/power/rk3568-power.h>
>
> / {
> 	compatible = "rockchip,rk3568";
>
> 	#address-cells = <2>;
> 	#size-cells = <2>;
>
> 	cru: clock-controller@fdd20000 {
> 		compatible = "rockchip,rk3568-cru";
> 		reg = <0x0 0xfdd20000 0x0 0x1000>;
> 		#clock-cells = <1>;
> 		#reset-cells = <1>;
> 	};
>
> 	pcie30phy: phy@fe8c0000 {
> 		compatible = "rockchip,rk3568-pcie3-phy";
> 		reg = <0x0 0xfe8c0000 0x0 0x20000>;
> 		#phy-cells = <0>;
> 	};
>
> 	combphy2_psq: phy@fe840000 {
> 		compatible = "rockchip,rk3568-naneng-combphy";
> 		reg = <0x0 0xfe840000 0x0 0x100>;
> 		#phy-cells = <1>;
> 	};
>
> 	gic: interrupt-controller@fd400000 {
> 		compatible = "arm,gic-v3";
> 		#interrupt-cells = <3>;
> 		#address-cells = <2>;
> 		#size-cells = <2>;
> 		ranges;
> 		interrupt-controller;
>
> 		reg = <0x0 0xfd400000 0 0x10000>,
> 		      <0x0 0xfd460000 0 0xc0000>;
> 		interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>;
> 		its: interrupt-controller@fd440000 {
> 			compatible = "arm,gic-v3-its";
> 			msi-controller;
> 			#msi-cells = <1>;
> 			reg = <0x0 0xfd440000 0x0 0x20000>;
> 			status = "disabled";
> 		};
> 	};
>
> 	pmu: power-management@fdd90000 {
> 		compatible = "rockchip,rk3568-pmu", "syscon", "simple-mfd";
> 		reg = <0x0 0xfdd90000 0x0 0x1000>;
>
> 		power: power-controller {
> 			compatible = "rockchip,rk3568-power-controller";
> 			#power-domain-cells = <1>;
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 			status = "okay";
> 		};
> 	};
>
> 	pcie2x1: pcie@fe260000 {
> 		compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";
> 		reg = <0x3 0xc0000000 0x0 0x400000>,
> 		      <0x0 0xfe260000 0x0 0x10000>;
> 		reg-names = "pcie-dbi", "pcie-apb";
> 		interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
> 		interrupt-names = "sys", "pmc", "msg", "legacy", "err";
> 		bus-range = <0x0 0xf>;
> 		clocks = <&cru ACLK_PCIE20_MST>, <&cru ACLK_PCIE20_SLV>,
> 			 <&cru ACLK_PCIE20_DBI>, <&cru PCLK_PCIE20>,
> 			 <&cru CLK_PCIE20_AUX_NDFT>;
> 		clock-names = "aclk_mst", "aclk_slv",
> 			      "aclk_dbi", "pclk", "aux";
> 		device_type = "pci";
> 		linux,pci-domain = <0>;
> 		num-ib-windows = <6>;
> 		num-ob-windows = <2>;
> 		max-link-speed = <2>;
> 		msi-map = <0x0 &its 0x0 0x1000>;
> 		num-lanes = <1>;
> 		phys = <&combphy2_psq PHY_TYPE_PCIE>;
> 		phy-names = "pcie-phy";
> 		power-domains = <&power RK3568_PD_PIPE>;
> 		ranges = <0x00000800 0x0 0x00000000 0x3 0x00000000 0x0 0x800000
> 			  0x81000000 0x0 0x00800000 0x3 0x00800000 0x0 0x100000
> 			  0x83000000 0x0 0x00900000 0x3 0x00900000 0x0 0x3f700000>;
> 		resets = <&cru SRST_PCIE20_POWERUP>;
> 		reset-names = "pipe";
> 		#address-cells = <3>;
> 		#size-cells = <2>;
> 		status = "disabled";
> 	};
>
> 	pcie3x1: pcie@fe270000 {
> 		compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";
> 		reg = <0x3 0xc0400000 0x0 0x400000>,
> 		      <0x0 0xfe270000 0x0 0x10000>;
> 		reg-names = "pcie-dbi", "pcie-apb";
> 		interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>;
> 		interrupt-names = "sys", "pmc", "msg", "legacy", "err";
> 		bus-range = <0x10 0x1f>;
> 		clocks = <&cru ACLK_PCIE30X1_MST>, <&cru ACLK_PCIE30X1_SLV>,
> 			 <&cru ACLK_PCIE30X1_DBI>, <&cru PCLK_PCIE30X1>,
> 			 <&cru CLK_PCIE30X1_AUX_NDFT>;
> 		clock-names = "aclk_mst", "aclk_slv",
> 			      "aclk_dbi", "pclk", "aux";
> 		device_type = "pci";
> 		linux,pci-domain = <1>;
> 		num-ib-windows = <6>;
> 		num-ob-windows = <2>;
> 		max-link-speed = <3>;
> 		msi-map = <0x1000 &its 0x1000 0x1000>;
> 		num-lanes = <1>;
> 		phys = <&pcie30phy>;
> 		phy-names = "pcie-phy";
> 		power-domains = <&power RK3568_PD_PIPE>;
> 		ranges = <0x00000800 0x0 0x40000000 0x3 0x40000000 0x0 0x800000
> 			  0x81000000 0x0 0x40800000 0x3 0x40800000 0x0 0x100000
> 			  0x83000000 0x0 0x40900000 0x3 0x40900000 0x0 0x3f700000>;
> 		resets = <&cru SRST_PCIE30X1_POWERUP>;
> 		reset-names = "pipe";
> 		#address-cells = <3>;
> 		#size-cells = <2>;
> 		status = "disabled";
> 	};
>
> 	pcie3x2: pcie@fe280000 {
> 		compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";
> 		reg = <0x3 0xc0800000 0x0 0x400000>,
> 		      <0x0 0xfe280000 0x0 0x10000>;
> 		reg-names = "pcie-dbi", "pcie-apb";
> 		interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>,
> 			     <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>;
> 		interrupt-names = "sys", "pmc", "msg", "legacy", "err";
> 		bus-range = <0x20 0x2f>;
> 		clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>,
> 			 <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>,
> 			 <&cru CLK_PCIE30X2_AUX_NDFT>;
> 		clock-names = "aclk_mst", "aclk_slv",
> 			      "aclk_dbi", "pclk", "aux";
> 		device_type = "pci";
> 		linux,pci-domain = <2>;
> 		num-ib-windows = <6>;
> 		num-ob-windows = <2>;
> 		max-link-speed = <3>;
> 		msi-map = <0x2000 &its 0x2000 0x1000>;
> 		num-lanes = <2>;
> 		phys = <&pcie30phy>;
> 		phy-names = "pcie-phy";
> 		power-domains = <&power RK3568_PD_PIPE>;
> 		ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000
> 			  0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000
> 			  0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>;
> 		resets = <&cru SRST_PCIE30X2_POWERUP>;
> 		reset-names = "pipe";
> 		#address-cells = <3>;
> 		#size-cells = <2>;
> 		status = "disabled";
> 	};
> };
>
>
>
Johan Jonker Jan. 22, 2021, 2:02 p.m. UTC | #4 
Hi Simon,

A few comments, have a look if it is useful or that you disagree.

/////

The format of ranges in your pcie node in rk3568.dtsi and your example
is wrong!

  ranges:
    oneOf:
      - $ref: "/schemas/types.yaml#/definitions/flag"
      - minItems: 1
        maxItems: 32    # Should be enough
        items:
          minItems: 5
          maxItems: 7
          additionalItems: true
          items:
            - enum:
                - 0x01000000
                - 0x02000000
                - 0x03000000
                - 0x42000000
                - 0x43000000
                - 0x81000000
                - 0x82000000
                - 0x83000000
                - 0xc2000000
                - 0xc3000000

The array size is 3: ==> maxItems: 3
in a array a range of 5 to 7 u64 is expected.
They must start with one of the enums above!
yaml dt_check expects <> around every array member!

Old example:

ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000

0x00000800 is not in the list of above, please recheck!

          0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000
          0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>;

New example:
FICTION example with correct first element, but maybe NOT good for
Rockchip pcie!

ranges = <0x81000000 0x0 0x80000000 0x3 0x80000000 0x0 0x800000>,
         <0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000>,
         <0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>;
	
Change this also in rk3568.dtsi!

/////

rockchip_add_pcie_port()

For FTRACE filters it is needed that all functions start with the same
function prefix.

Maybe use:
rockchip_pcie_add_port()

/////

The function prefix of pcie-dw-rockchip.c is identical with functions in
pcie-rockchip-host.c
Is that OK for the maintainers?

/////

+static struct platform_driver rockchip_pcie_driver = {
+	.driver = {
+		.name	= "rk-pcie",

Maybe change to:

+		.name	= "rockchip-dw-pcie",

This name shows up in the kernel log.
Could you keep it in line with other Rockchip names, so we can filter
more easy?

dmesg | grep rockchip

rockchip-vop
rochchip-drm
etc.

+		.of_match_table = rockchip_pcie_of_match,
+		.suppress_bind_attrs = true,
+	},
+	.probe = rockchip_pcie_probe,
+};

/////
Rob Herring (Arm) Jan. 22, 2021, 3:55 p.m. UTC | #5 
On Wed, Jan 20, 2021 at 06:07:29PM +0100, Johan Jonker wrote:
> Hi Simon,
> 
> Thanks you for version 2.
> A few comments, have a look if it is useful or that you disagree.
> 
> This patch has no commit message. Add one in version 3.
> 
> Submit all patches in one batch with the same sort message ID to all
> maintainers including Heiko.
> 
> Heiko Stuebner <heiko@sntech.de>
> 
> Example message ID:
> 20210120101554.241029-1-xxm@rock-chips.com
> 
> /////
> 
> Included is a copy of the Rockchip pcie nodes in a sort of test.dts below.
> Could you confirm that the properties in that dts are the one that we
> can expect for Linux mainline and can base our YAML document on?
> 
> With rk3568-cru.h and rk3568-power.h manualy added we do some tests with
> the following commands:
> 
> make ARCH=arm64 dt_binding_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> 
> make ARCH=arm64 dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> 
> make ARCH=arm64 dtbs_check
> DT_SCHEMA_FILES=~/.local/lib/python3.5/site-packages/dtschema/schemas/pci/pci-bus.yaml
> 
> /////
> 
> Example notifications:
> 
> /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: reg: [[3,
> 3225419776, 0, 4194304], [0, 4263968768, 0, 65536]] is too long
> 
> /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: ranges:
> 'oneOf' conditional failed, one must be fixed:
> 
> Before you submit version 3 make sure that all warnings gone as much as
> possible.
> 
> On 1/20/21 11:15 AM, Simon Xue wrote:
> > Signed-off-by: Simon Xue <xxm@rock-chips.com>
> > ---
> >  .../bindings/pci/rockchip-dw-pcie.yaml        | 140 ++++++++++++++++++
> >  1 file changed, 140 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > new file mode 100644
> > index 000000000000..9d3a57f5305e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > @@ -0,0 +1,140 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: DesignWare based PCIe RC controller on Rockchip SoCs
> > +
> > +maintainers:
> > +  - Shawn Lin <shawn.lin@rock-chips.com>
> > +  - Simon Xue <xxm@rock-chips.com>
>      - Heiko Stuebner <heiko@sntech.de> ;)
> > +
> > +description: |+
> > +  RK3568 SoC PCIe host controller is based on the Synopsys DesignWare
> > +  PCIe IP and thus inherits all the common properties defined in
> > +  designware-pcie.txt.
> > +
> > +allOf:
> > +  - $ref: /schemas/pci/pci-bus.yaml#
> > +
> > +# We need a select here so we don't match all nodes with 'snps,dw-pcie'
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        const: rockchip,rk3568-pcie
> > +  required:
> > +    - compatible
> > +
> > +properties:
> > +  compatible:
> 
> > +    item:
> 
>     items:
> 
> > +      - const: rockchip,rk3568-pcie
> > +      - const: snps,dw-pcie
> 
> Add empty line
> 
> > +  reg:    items:
>       - description:
>       - description:
> 
> Add some description for regs.
> 
> > +    maxItems: 1
> remove
> 
> This reg maxItems gives errors.
> 
> > +
> 
> > +  interrupt:
> interrupts:
>    items:
> 
> > +      - description: system information
> > +      - description: power management control
> > +      - description: PCIe message
> > +      - description: legacy interrupt
> > +      - description: error report
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: sys
> > +      - const: pmc
> > +      - const: msg

MSI? If so, use 'msi'. The DWC core will handle setting it up now.

> > +      - const: legacy
> > +      - const: err
> > +
> > +  clocks:
> > +    items:
> > +      - description: AHB clock for PCIe master
> > +      - description: AHB clock for PCIe slave
> > +      - description: AHB clock for PCIe dbi
> > +      - description: APB clock for PCIe
> > +      - description: Auxiliary clock for PCIe
> > +
> > +  clock-names:
> > +    items:
> > +      - const: aclk_mst
> > +      - const: aclk_slv
> > +      - const: aclk_dbi
> > +      - const: pclk
> > +      - const: aux
> > +
> > +  msi-map: true
> > +
> > +  power-domains:
> > +    maxItems: 1
> 
> /////
> These properties come from designware-pcie.txt
> Maybe add them here for now till there's a common yaml?
> 
>   num-ib-windows: number of inbound address translation windows
>   num-ob-windows: number of outbound address translation windows

These can be and are now detected at runtime.

Rob
Simon Xue Jan. 25, 2021, 1:53 a.m. UTC | #6 
Hi Rob,

在 2021/1/22 23:55, Rob Herring 写道:
> On Wed, Jan 20, 2021 at 06:07:29PM +0100, Johan Jonker wrote:
>> Hi Simon,
>>
>> Thanks you for version 2.
>> A few comments, have a look if it is useful or that you disagree.
>>
>> This patch has no commit message. Add one in version 3.
>>
>> Submit all patches in one batch with the same sort message ID to all
>> maintainers including Heiko.
>>
>> Heiko Stuebner <heiko@sntech.de>
>>
>> Example message ID:
>> 20210120101554.241029-1-xxm@rock-chips.com
>>
>> /////
>>
>> Included is a copy of the Rockchip pcie nodes in a sort of test.dts below.
>> Could you confirm that the properties in that dts are the one that we
>> can expect for Linux mainline and can base our YAML document on?
>>
>> With rk3568-cru.h and rk3568-power.h manualy added we do some tests with
>> the following commands:
>>
>> make ARCH=arm64 dt_binding_check
>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>>
>> make ARCH=arm64 dtbs_check
>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>>
>> make ARCH=arm64 dtbs_check
>> DT_SCHEMA_FILES=~/.local/lib/python3.5/site-packages/dtschema/schemas/pci/pci-bus.yaml
>>
>> /////
>>
>> Example notifications:
>>
>> /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: reg: [[3,
>> 3225419776, 0, 4194304], [0, 4263968768, 0, 65536]] is too long
>>
>> /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: ranges:
>> 'oneOf' conditional failed, one must be fixed:
>>
>> Before you submit version 3 make sure that all warnings gone as much as
>> possible.
>>
>> On 1/20/21 11:15 AM, Simon Xue wrote:
>>> Signed-off-by: Simon Xue <xxm@rock-chips.com>
>>> ---
>>>   .../bindings/pci/rockchip-dw-pcie.yaml        | 140 ++++++++++++++++++
>>>   1 file changed, 140 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>>> new file mode 100644
>>> index 000000000000..9d3a57f5305e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
>>> @@ -0,0 +1,140 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: DesignWare based PCIe RC controller on Rockchip SoCs
>>> +
>>> +maintainers:
>>> +  - Shawn Lin <shawn.lin@rock-chips.com>
>>> +  - Simon Xue <xxm@rock-chips.com>
>>       - Heiko Stuebner <heiko@sntech.de> ;)
>>> +
>>> +description: |+
>>> +  RK3568 SoC PCIe host controller is based on the Synopsys DesignWare
>>> +  PCIe IP and thus inherits all the common properties defined in
>>> +  designware-pcie.txt.
>>> +
>>> +allOf:
>>> +  - $ref: /schemas/pci/pci-bus.yaml#
>>> +
>>> +# We need a select here so we don't match all nodes with 'snps,dw-pcie'
>>> +select:
>>> +  properties:
>>> +    compatible:
>>> +      contains:
>>> +        const: rockchip,rk3568-pcie
>>> +  required:
>>> +    - compatible
>>> +
>>> +properties:
>>> +  compatible:
>>> +    item:
>>      items:
>>
>>> +      - const: rockchip,rk3568-pcie
>>> +      - const: snps,dw-pcie
>> Add empty line
>>
>>> +  reg:    items:
>>        - description:
>>        - description:
>>
>> Add some description for regs.
>>
>>> +    maxItems: 1
>> remove
>>
>> This reg maxItems gives errors.
>>
>>> +
>>> +  interrupt:
>> interrupts:
>>     items:
>>
>>> +      - description: system information
>>> +      - description: power management control
>>> +      - description: PCIe message
>>> +      - description: legacy interrupt
>>> +      - description: error report
>>> +
>>> +  interrupt-names:
>>> +    items:
>>> +      - const: sys
>>> +      - const: pmc
>>> +      - const: msg
> MSI? If so, use 'msi'. The DWC core will handle setting it up now.

No, it is SoC designed interrupt for some purpose. Currently, we don't 
use interrupt, will remove

these.

>>> +      - const: legacy
>>> +      - const: err
>>> +
>>> +  clocks:
>>> +    items:
>>> +      - description: AHB clock for PCIe master
>>> +      - description: AHB clock for PCIe slave
>>> +      - description: AHB clock for PCIe dbi
>>> +      - description: APB clock for PCIe
>>> +      - description: Auxiliary clock for PCIe
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: aclk_mst
>>> +      - const: aclk_slv
>>> +      - const: aclk_dbi
>>> +      - const: pclk
>>> +      - const: aux
>>> +
>>> +  msi-map: true
>>> +
>>> +  power-domains:
>>> +    maxItems: 1
>> /////
>> These properties come from designware-pcie.txt
>> Maybe add them here for now till there's a common yaml?
>>
>>    num-ib-windows: number of inbound address translation windows
>>    num-ob-windows: number of outbound address translation windows
> These can be and are now detected at runtime.

Will remove.

Simon Xue

>
> Rob
>
>
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
new file mode 100644
index 000000000000..9d3a57f5305e
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
@@ -0,0 +1,140 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: DesignWare based PCIe RC controller on Rockchip SoCs
+
+maintainers:
+  - Shawn Lin <shawn.lin@rock-chips.com>
+  - Simon Xue <xxm@rock-chips.com>
+
+description: |+
+  RK3568 SoC PCIe host controller is based on the Synopsys DesignWare
+  PCIe IP and thus inherits all the common properties defined in
+  designware-pcie.txt.
+
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+
+# We need a select here so we don't match all nodes with 'snps,dw-pcie'
+select:
+  properties:
+    compatible:
+      contains:
+        const: rockchip,rk3568-pcie
+  required:
+    - compatible
+
+properties:
+  compatible:
+    item:
+      - const: rockchip,rk3568-pcie
+      - const: snps,dw-pcie
+  reg:
+    maxItems: 1
+
+  interrupt:
+      - description: system information
+      - description: power management control
+      - description: PCIe message
+      - description: legacy interrupt
+      - description: error report
+
+  interrupt-names:
+    items:
+      - const: sys
+      - const: pmc
+      - const: msg
+      - const: legacy
+      - const: err
+
+  clocks:
+    items:
+      - description: AHB clock for PCIe master
+      - description: AHB clock for PCIe slave
+      - description: AHB clock for PCIe dbi
+      - description: APB clock for PCIe
+      - description: Auxiliary clock for PCIe
+
+  clock-names:
+    items:
+      - const: aclk_mst
+      - const: aclk_slv
+      - const: aclk_dbi
+      - const: pclk
+      - const: aux
+
+  msi-map: true
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    items:
+      - const: pipe
+
+required:
+  - compatible
+  - bus-range
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - msi-map
+  - num-lanes
+  - phys
+  - phy-names
+  - power-domains
+  - resets
+  - reset-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/rk3568-cru.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/power/rk3568-power.h>
+
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pcie3x2: pcie@fe280000 {
+            compatible = "rockchip,rk3568-pcie", "snps,dw-pcie";
+            #address-cells = <3>;
+            #size-cells = <2>;
+            bus-range = <0x20 0x2f>;
+            reg = <0x3 0xc0800000 0x0 0x400000>,
+                  <0x0 0xfe280000 0x0 0x10000>;
+            reg-names = "pcie-dbi", "pcie-apb";
+            interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "sys", "pmc", "msg", "legacy", "err";
+            clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>,
+                     <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>,
+                     <&cru CLK_PCIE30X2_AUX_NDFT>;
+            clock-names = "aclk_mst", "aclk_slv",
+                          "aclk_dbi", "pclk",
+                          "aux";
+            msi-map = <0x2000 &its 0x2000 0x1000>;
+            num-lanes = <2>;
+            phys = <&pcie30phy>;
+            phy-names = "pcie-phy";
+            power-domains = <&power RK3568_PD_PIPE>;
+            ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000
+                      0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000
+                      0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>;
+            resets = <&cru SRST_PCIE30X2_POWERUP>;
+            reset-names = "pipe";
+        };
+    };
+...