[PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller
xxm
xxm at rock-chips.com
Thu Jan 21 21:22:50 EST 2021
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 at sntech.de>
>
> Example message ID:
> 20210120101554.241029-1-xxm at 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 at fe270000: reg: [[3,
> 3225419776, 0, 4194304], [0, 4263968768, 0, 65536]] is too long
>
> /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie at 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 at 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 at rock-chips.com>
>> + - Simon Xue <xxm at rock-chips.com>
> - Heiko Stuebner <heiko at 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 at 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 at fdd20000 {
> compatible = "rockchip,rk3568-cru";
> reg = <0x0 0xfdd20000 0x0 0x1000>;
> #clock-cells = <1>;
> #reset-cells = <1>;
> };
>
> pcie30phy: phy at fe8c0000 {
> compatible = "rockchip,rk3568-pcie3-phy";
> reg = <0x0 0xfe8c0000 0x0 0x20000>;
> #phy-cells = <0>;
> };
>
> combphy2_psq: phy at fe840000 {
> compatible = "rockchip,rk3568-naneng-combphy";
> reg = <0x0 0xfe840000 0x0 0x100>;
> #phy-cells = <1>;
> };
>
> gic: interrupt-controller at 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 at fd440000 {
> compatible = "arm,gic-v3-its";
> msi-controller;
> #msi-cells = <1>;
> reg = <0x0 0xfd440000 0x0 0x20000>;
> status = "disabled";
> };
> };
>
> pmu: power-management at 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 at 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 at 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 at 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";
> };
> };
>
>
>
More information about the Linux-rockchip
mailing list