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@xxxxxxxxx> Example message ID: 20210120101554.241029-1-xxm@xxxxxxxxxxxxxx ///// 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@xxxxxxxxxxxxxx> > --- > .../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@xxxxxxxxxxxxxx> > + - Simon Xue <xxm@xxxxxxxxxxxxxx> - Heiko Stuebner <heiko@xxxxxxxxx> ;) > + > +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"; }; };
