* [PATCH 1/3] PCI: dwc: Skip allocating own MSI domain if using external MSI domain @ 2021-01-18 9:17 ` Simon Xue 0 siblings, 0 replies; 32+ messages in thread From: Simon Xue @ 2021-01-18 9:17 UTC (permalink / raw) To: Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, linux-rockchip, Shawn Lin, Simon Xue From: Shawn Lin <shawn.lin@rock-chips.com> On some platform, external MSI domain is using instead of the one created by designware driver. For instance, if using GIC-V3-ITS as a MSI domain, we only need set msi-map in the devicetree but never need any bit in the designware driver to handle MSI stuff. So skip allocating its own MSI domain for that case. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> Signed-off-by: Simon Xue <xxm@rock-chips.com> --- drivers/pci/controller/dwc/pcie-designware-host.c | 10 +++++++++- drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 8a84c005f32b..d9d93cab970a 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -235,6 +235,10 @@ int dw_pcie_allocate_domains(struct pcie_port *pp) struct dw_pcie *pci = to_dw_pcie_from_pp(pp); struct fwnode_handle *fwnode = of_node_to_fwnode(pci->dev->of_node); + /* Rely on the external MSI domain */ + if (pp->msi_ext) + return 0; + pp->irq_domain = irq_domain_create_linear(fwnode, pp->num_vectors, &dw_pcie_msi_domain_ops, pp); if (!pp->irq_domain) { @@ -258,6 +262,9 @@ int dw_pcie_allocate_domains(struct pcie_port *pp) static void dw_pcie_free_msi(struct pcie_port *pp) { + if (pp->msi_ext) + return; + if (pp->msi_irq) { irq_set_chained_handler(pp->msi_irq, NULL); irq_set_handler_data(pp->msi_irq, NULL); @@ -359,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp) if (pci->link_gen < 1) pci->link_gen = of_pci_get_max_link_speed(np); - if (pci_msi_enabled()) { + if (pci_msi_enabled() && + !pp->msi_ext) { pp->has_msi_ctrl = !(pp->ops->msi_host_init || of_property_read_bool(np, "msi-parent") || of_property_read_bool(np, "msi-map")); diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 0207840756c4..cf3b0664302a 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -195,6 +195,7 @@ struct pcie_port { u32 irq_mask[MAX_MSI_CTRLS]; struct pci_host_bridge *bridge; raw_spinlock_t lock; + int msi_ext; DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); }; -- 2.25.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 1/3] PCI: dwc: Skip allocating own MSI domain if using external MSI domain @ 2021-01-18 9:17 ` Simon Xue 0 siblings, 0 replies; 32+ messages in thread From: Simon Xue @ 2021-01-18 9:17 UTC (permalink / raw) To: Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, Shawn Lin, Simon Xue, linux-rockchip From: Shawn Lin <shawn.lin@rock-chips.com> On some platform, external MSI domain is using instead of the one created by designware driver. For instance, if using GIC-V3-ITS as a MSI domain, we only need set msi-map in the devicetree but never need any bit in the designware driver to handle MSI stuff. So skip allocating its own MSI domain for that case. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> Signed-off-by: Simon Xue <xxm@rock-chips.com> --- drivers/pci/controller/dwc/pcie-designware-host.c | 10 +++++++++- drivers/pci/controller/dwc/pcie-designware.h | 1 + 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c index 8a84c005f32b..d9d93cab970a 100644 --- a/drivers/pci/controller/dwc/pcie-designware-host.c +++ b/drivers/pci/controller/dwc/pcie-designware-host.c @@ -235,6 +235,10 @@ int dw_pcie_allocate_domains(struct pcie_port *pp) struct dw_pcie *pci = to_dw_pcie_from_pp(pp); struct fwnode_handle *fwnode = of_node_to_fwnode(pci->dev->of_node); + /* Rely on the external MSI domain */ + if (pp->msi_ext) + return 0; + pp->irq_domain = irq_domain_create_linear(fwnode, pp->num_vectors, &dw_pcie_msi_domain_ops, pp); if (!pp->irq_domain) { @@ -258,6 +262,9 @@ int dw_pcie_allocate_domains(struct pcie_port *pp) static void dw_pcie_free_msi(struct pcie_port *pp) { + if (pp->msi_ext) + return; + if (pp->msi_irq) { irq_set_chained_handler(pp->msi_irq, NULL); irq_set_handler_data(pp->msi_irq, NULL); @@ -359,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp) if (pci->link_gen < 1) pci->link_gen = of_pci_get_max_link_speed(np); - if (pci_msi_enabled()) { + if (pci_msi_enabled() && + !pp->msi_ext) { pp->has_msi_ctrl = !(pp->ops->msi_host_init || of_property_read_bool(np, "msi-parent") || of_property_read_bool(np, "msi-map")); diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 0207840756c4..cf3b0664302a 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -195,6 +195,7 @@ struct pcie_port { u32 irq_mask[MAX_MSI_CTRLS]; struct pci_host_bridge *bridge; raw_spinlock_t lock; + int msi_ext; DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); }; -- 2.25.1 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller 2021-01-18 9:17 ` Simon Xue @ 2021-01-18 9:17 ` Simon Xue -1 siblings, 0 replies; 32+ messages in thread From: Simon Xue @ 2021-01-18 9:17 UTC (permalink / raw) To: Bjorn Helgaas, Lorenzo Pieralisi; +Cc: linux-pci, linux-rockchip, Simon Xue Signed-off-by: Simon Xue <xxm@rock-chips.com> --- .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ 1 file changed, 101 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..fa664cfffb29 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml @@ -0,0 +1,101 @@ +# 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> + +# 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: + enum: + - rockchip,rk3568-pcie + - snps,dw-pcie + + reg: + maxItems: 1 + + 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 + + resets: + items: + - description: PCIe pipe reset line + + reset-names: + items: + - const: pipe + +required: + - compatible + - "#address-cells" + - "#size-cells" + - bus-range + - reg + - reg-names + - clocks + - clock-names + - msi-map + - num-lanes + - phys + - phy-names + - ranges + - resets + - reset-names + +additionalProperties: false + +examples: + - | + 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"; + 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"; + 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"; + }; + +... -- 2.25.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-18 9:17 ` Simon Xue 0 siblings, 0 replies; 32+ messages in thread From: Simon Xue @ 2021-01-18 9:17 UTC (permalink / raw) To: Bjorn Helgaas, Lorenzo Pieralisi; +Cc: linux-pci, Simon Xue, linux-rockchip Signed-off-by: Simon Xue <xxm@rock-chips.com> --- .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ 1 file changed, 101 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..fa664cfffb29 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml @@ -0,0 +1,101 @@ +# 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> + +# 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: + enum: + - rockchip,rk3568-pcie + - snps,dw-pcie + + reg: + maxItems: 1 + + 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 + + resets: + items: + - description: PCIe pipe reset line + + reset-names: + items: + - const: pipe + +required: + - compatible + - "#address-cells" + - "#size-cells" + - bus-range + - reg + - reg-names + - clocks + - clock-names + - msi-map + - num-lanes + - phys + - phy-names + - ranges + - resets + - reset-names + +additionalProperties: false + +examples: + - | + 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"; + 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"; + 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"; + }; + +... -- 2.25.1 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller 2021-01-18 9:17 ` Simon Xue @ 2021-01-19 13:07 ` Johan Jonker -1 siblings, 0 replies; 32+ messages in thread From: Johan Jonker @ 2021-01-19 13:07 UTC (permalink / raw) To: Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, linux-rockchip, Heiko Stuebner, Rob Herring, devicetree Hi Simon, Thank you for this patch for rk3568 pcie. Include the Rockchip device tree maintainer and all other people/lists to the CC list. ./scripts/checkpatch.pl --strict <patch1> <patch2> ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback --nogit <patch1> <patch2> git send-email --suppress-cc all --dry-run --annotate --to heiko@sntech.de --cc <..> <patch1> <patch2> This SoC has no support in mainline linux kernel yet. In all the following yaml documents for rk3568 we need headers with defines for clocks and power domains, etc. For example: #include <dt-bindings/clock/rk3568-cru.h> #include <dt-bindings/power/rk3568-power.h> Could Rockchip submit first clocks and power drivers entries and a basic rk3568.dtsi + evb dts? Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. A dtbs_check only works with a complete dtsi and evb dts. make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml On 1/18/21 10:17 AM, Simon Xue wrote: > Signed-off-by: Simon Xue <xxm@rock-chips.com> > --- > .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ > 1 file changed, 101 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..fa664cfffb29 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > @@ -0,0 +1,101 @@ > +# 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> maintainers: - Heiko Stuebner <heiko@sntech.de> Add only people with maintainer rights. allOf: - $ref: /schemas/pci/pci-bus.yaml# designware-pcie.txt is in need for conversion to yaml. Include the things that are needed in this document for now. > + > +# 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: > + enum: > + - rockchip,rk3568-pcie > + - snps,dw-pcie items: - const: rockchip,rk3568-pcie - const: snps,dw-pcie > + > + reg: > + maxItems: 1 interrupts: - description: - description: - description: - description: - description: 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 > + items: > + - description: PCIe pipe reset line remove > + > + reset-names: const: pipe > + items: > + - const: pipe remove > + > +required: > + - compatible > + - "#address-cells" > + - "#size-cells" already required in pci-bus.yaml > + - bus-range > + - reg > + - reg-names > + - clocks > + - clock-names > + - msi-map not defined in pci-bus.yaml and designware-pcie.txt add to document > + - num-lanes > + - phys > + - phy-names not defined in pci-bus.yaml and designware-pcie.txt add to document - power-domains > + - ranges already required in pci-bus.yaml > + - resets > + - reset-names > + > +additionalProperties: false unevaluatedProperties: false If other documents are included use unevaluatedProperties. > + > +examples: > + - | #include <dt-bindings/clock/rk3568-cru.h> #include <dt-bindings/interrupt-controller/arm-gic.h> #include <dt-bindings/power/rk3568-power.h> Missing defines bus { #address-cells = <2>; #size-cells = <2>; dt-check uses standard 32 bit regs this example is for 64 bit > + pcie3x2: pcie@fe280000 { > + compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; > + #address-cells = <3>; > + #size-cells = <2>; sort things that start with # below > + bus-range = <0x20 0x2f>; sort order is: compatible reg interrupt the rest in alphabetical order things with # no status in yaml examples > + reg = <0x3 0xc0800000 0x0 0x400000>, > + <0x0 0xfe280000 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"; > + 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"; > + }; }; > + > +... > Make sure that all properties that show up in this node are checked! pcie2x1: pcie@fe260000 { compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; #address-cells = <3>; #size-cells = <2>; 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"; 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"; 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>; reg = <0x3 0xc0000000 0x0 0x400000>, <0x0 0xfe260000 0x0 0x10000>; reg-names = "pcie-dbi", "pcie-apb"; resets = <&cru SRST_PCIE20_POWERUP>; reset-names = "pipe"; status = "disabled"; }; ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-19 13:07 ` Johan Jonker 0 siblings, 0 replies; 32+ messages in thread From: Johan Jonker @ 2021-01-19 13:07 UTC (permalink / raw) To: Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, devicetree, Rob Herring, Heiko Stuebner, linux-rockchip Hi Simon, Thank you for this patch for rk3568 pcie. Include the Rockchip device tree maintainer and all other people/lists to the CC list. ./scripts/checkpatch.pl --strict <patch1> <patch2> ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback --nogit <patch1> <patch2> git send-email --suppress-cc all --dry-run --annotate --to heiko@sntech.de --cc <..> <patch1> <patch2> This SoC has no support in mainline linux kernel yet. In all the following yaml documents for rk3568 we need headers with defines for clocks and power domains, etc. For example: #include <dt-bindings/clock/rk3568-cru.h> #include <dt-bindings/power/rk3568-power.h> Could Rockchip submit first clocks and power drivers entries and a basic rk3568.dtsi + evb dts? Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. A dtbs_check only works with a complete dtsi and evb dts. make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml On 1/18/21 10:17 AM, Simon Xue wrote: > Signed-off-by: Simon Xue <xxm@rock-chips.com> > --- > .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ > 1 file changed, 101 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..fa664cfffb29 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > @@ -0,0 +1,101 @@ > +# 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> maintainers: - Heiko Stuebner <heiko@sntech.de> Add only people with maintainer rights. allOf: - $ref: /schemas/pci/pci-bus.yaml# designware-pcie.txt is in need for conversion to yaml. Include the things that are needed in this document for now. > + > +# 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: > + enum: > + - rockchip,rk3568-pcie > + - snps,dw-pcie items: - const: rockchip,rk3568-pcie - const: snps,dw-pcie > + > + reg: > + maxItems: 1 interrupts: - description: - description: - description: - description: - description: 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 > + items: > + - description: PCIe pipe reset line remove > + > + reset-names: const: pipe > + items: > + - const: pipe remove > + > +required: > + - compatible > + - "#address-cells" > + - "#size-cells" already required in pci-bus.yaml > + - bus-range > + - reg > + - reg-names > + - clocks > + - clock-names > + - msi-map not defined in pci-bus.yaml and designware-pcie.txt add to document > + - num-lanes > + - phys > + - phy-names not defined in pci-bus.yaml and designware-pcie.txt add to document - power-domains > + - ranges already required in pci-bus.yaml > + - resets > + - reset-names > + > +additionalProperties: false unevaluatedProperties: false If other documents are included use unevaluatedProperties. > + > +examples: > + - | #include <dt-bindings/clock/rk3568-cru.h> #include <dt-bindings/interrupt-controller/arm-gic.h> #include <dt-bindings/power/rk3568-power.h> Missing defines bus { #address-cells = <2>; #size-cells = <2>; dt-check uses standard 32 bit regs this example is for 64 bit > + pcie3x2: pcie@fe280000 { > + compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; > + #address-cells = <3>; > + #size-cells = <2>; sort things that start with # below > + bus-range = <0x20 0x2f>; sort order is: compatible reg interrupt the rest in alphabetical order things with # no status in yaml examples > + reg = <0x3 0xc0800000 0x0 0x400000>, > + <0x0 0xfe280000 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"; > + 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"; > + }; }; > + > +... > Make sure that all properties that show up in this node are checked! pcie2x1: pcie@fe260000 { compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; #address-cells = <3>; #size-cells = <2>; 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"; 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"; 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>; reg = <0x3 0xc0000000 0x0 0x400000>, <0x0 0xfe260000 0x0 0x10000>; reg-names = "pcie-dbi", "pcie-apb"; resets = <&cru SRST_PCIE20_POWERUP>; reset-names = "pipe"; status = "disabled"; }; _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller 2021-01-19 13:07 ` Johan Jonker @ 2021-01-19 13:14 ` Heiko Stübner -1 siblings, 0 replies; 32+ messages in thread From: Heiko Stübner @ 2021-01-19 13:14 UTC (permalink / raw) To: Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi, Johan Jonker Cc: linux-pci, linux-rockchip, Rob Herring, devicetree Hi Johan, Am Dienstag, 19. Januar 2021, 14:07:41 CET schrieb Johan Jonker: > Hi Simon, > > Thank you for this patch for rk3568 pcie. > > Include the Rockchip device tree maintainer and all other people/lists > to the CC list. > > ./scripts/checkpatch.pl --strict <patch1> <patch2> > > ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback > --nogit <patch1> <patch2> > > git send-email --suppress-cc all --dry-run --annotate --to > heiko@sntech.de --cc <..> <patch1> <patch2> > > This SoC has no support in mainline linux kernel yet. > In all the following yaml documents for rk3568 we need headers with > defines for clocks and power domains, etc. > > For example: > #include <dt-bindings/clock/rk3568-cru.h> > #include <dt-bindings/power/rk3568-power.h> > > Could Rockchip submit first clocks and power drivers entries and a basic > rk3568.dtsi + evb dts? > Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. > > A dtbs_check only works with a complete dtsi and evb dts. > > make ARCH=arm64 dtbs_check > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > On 1/18/21 10:17 AM, Simon Xue wrote: > > Signed-off-by: Simon Xue <xxm@rock-chips.com> > > --- > > .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ > > 1 file changed, 101 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..fa664cfffb29 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > @@ -0,0 +1,101 @@ > > +# 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> > > maintainers: > - Heiko Stuebner <heiko@sntech.de> > > Add only people with maintainer rights. I'd disagree on this ;-) The maintainer for individual drivers should be the persons who are actually know the hardware. We have individual Rockchip developers taking care of other drivers as well already. And normally scripts/get_maintainer.pl should already include me due to the wildcard for things having "rockchip" in the name. Heiko ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-19 13:14 ` Heiko Stübner 0 siblings, 0 replies; 32+ messages in thread From: Heiko Stübner @ 2021-01-19 13:14 UTC (permalink / raw) To: Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi, Johan Jonker Cc: linux-pci, devicetree, Rob Herring, linux-rockchip Hi Johan, Am Dienstag, 19. Januar 2021, 14:07:41 CET schrieb Johan Jonker: > Hi Simon, > > Thank you for this patch for rk3568 pcie. > > Include the Rockchip device tree maintainer and all other people/lists > to the CC list. > > ./scripts/checkpatch.pl --strict <patch1> <patch2> > > ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback > --nogit <patch1> <patch2> > > git send-email --suppress-cc all --dry-run --annotate --to > heiko@sntech.de --cc <..> <patch1> <patch2> > > This SoC has no support in mainline linux kernel yet. > In all the following yaml documents for rk3568 we need headers with > defines for clocks and power domains, etc. > > For example: > #include <dt-bindings/clock/rk3568-cru.h> > #include <dt-bindings/power/rk3568-power.h> > > Could Rockchip submit first clocks and power drivers entries and a basic > rk3568.dtsi + evb dts? > Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. > > A dtbs_check only works with a complete dtsi and evb dts. > > make ARCH=arm64 dtbs_check > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > On 1/18/21 10:17 AM, Simon Xue wrote: > > Signed-off-by: Simon Xue <xxm@rock-chips.com> > > --- > > .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ > > 1 file changed, 101 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..fa664cfffb29 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > @@ -0,0 +1,101 @@ > > +# 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> > > maintainers: > - Heiko Stuebner <heiko@sntech.de> > > Add only people with maintainer rights. I'd disagree on this ;-) The maintainer for individual drivers should be the persons who are actually know the hardware. We have individual Rockchip developers taking care of other drivers as well already. And normally scripts/get_maintainer.pl should already include me due to the wildcard for things having "rockchip" in the name. Heiko _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller 2021-01-19 13:14 ` Heiko Stübner @ 2021-01-19 15:11 ` Johan Jonker -1 siblings, 0 replies; 32+ messages in thread From: Johan Jonker @ 2021-01-19 15:11 UTC (permalink / raw) To: Heiko Stübner, Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, linux-rockchip, Rob Herring, devicetree Hi Simon, Heiko, On 1/19/21 2:14 PM, Heiko Stübner wrote: > Hi Johan, > > Am Dienstag, 19. Januar 2021, 14:07:41 CET schrieb Johan Jonker: >> Hi Simon, >> >> Thank you for this patch for rk3568 pcie. >> >> Include the Rockchip device tree maintainer and all other people/lists >> to the CC list. >> >> ./scripts/checkpatch.pl --strict <patch1> <patch2> >> >> ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback >> --nogit <patch1> <patch2> >> >> git send-email --suppress-cc all --dry-run --annotate --to >> heiko@sntech.de --cc <..> <patch1> <patch2> >> >> This SoC has no support in mainline linux kernel yet. >> In all the following yaml documents for rk3568 we need headers with >> defines for clocks and power domains, etc. >> >> For example: >> #include <dt-bindings/clock/rk3568-cru.h> >> #include <dt-bindings/power/rk3568-power.h> >> >> Could Rockchip submit first clocks and power drivers entries and a basic >> rk3568.dtsi + evb dts? >> Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. >> >> A dtbs_check only works with a complete dtsi and evb dts. >> >> make ARCH=arm64 dtbs_check >> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >> >> On 1/18/21 10:17 AM, Simon Xue wrote: >>> Signed-off-by: Simon Xue <xxm@rock-chips.com> >>> --- >>> .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ >>> 1 file changed, 101 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..fa664cfffb29 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >>> @@ -0,0 +1,101 @@ >>> +# 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> >> >> maintainers: >> - Heiko Stuebner <heiko@sntech.de> >> >> Add only people with maintainer rights. > > I'd disagree on this ;-) All roads leads to Heiko... ;) It takes long term commitment. Year in, year out. Keeping yourself up to date with the latest pcei development. Communicate in English. Be able to submit patches without errors... ;) Review other peoples patches. Respond in short time. Bug fixing. If that's what you really want, then you must include a patch to this serie for MAINTAINERS. Check patch with: ./scripts/parse-maintainers.pl --input=MAINTAINERS --output=MAINTAINERS --order Otherwise it's safe to include that person mentioned above. > > The maintainer for individual drivers should be the persons who are > actually know the hardware. We have individual Rockchip developers > taking care of other drivers as well already. > > And normally scripts/get_maintainer.pl should already include me > due to the wildcard for things having "rockchip" in the name. > > > Heiko > > > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-19 15:11 ` Johan Jonker 0 siblings, 0 replies; 32+ messages in thread From: Johan Jonker @ 2021-01-19 15:11 UTC (permalink / raw) To: Heiko Stübner, Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, devicetree, Rob Herring, linux-rockchip Hi Simon, Heiko, On 1/19/21 2:14 PM, Heiko Stübner wrote: > Hi Johan, > > Am Dienstag, 19. Januar 2021, 14:07:41 CET schrieb Johan Jonker: >> Hi Simon, >> >> Thank you for this patch for rk3568 pcie. >> >> Include the Rockchip device tree maintainer and all other people/lists >> to the CC list. >> >> ./scripts/checkpatch.pl --strict <patch1> <patch2> >> >> ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback >> --nogit <patch1> <patch2> >> >> git send-email --suppress-cc all --dry-run --annotate --to >> heiko@sntech.de --cc <..> <patch1> <patch2> >> >> This SoC has no support in mainline linux kernel yet. >> In all the following yaml documents for rk3568 we need headers with >> defines for clocks and power domains, etc. >> >> For example: >> #include <dt-bindings/clock/rk3568-cru.h> >> #include <dt-bindings/power/rk3568-power.h> >> >> Could Rockchip submit first clocks and power drivers entries and a basic >> rk3568.dtsi + evb dts? >> Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. >> >> A dtbs_check only works with a complete dtsi and evb dts. >> >> make ARCH=arm64 dtbs_check >> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >> >> On 1/18/21 10:17 AM, Simon Xue wrote: >>> Signed-off-by: Simon Xue <xxm@rock-chips.com> >>> --- >>> .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ >>> 1 file changed, 101 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..fa664cfffb29 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >>> @@ -0,0 +1,101 @@ >>> +# 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> >> >> maintainers: >> - Heiko Stuebner <heiko@sntech.de> >> >> Add only people with maintainer rights. > > I'd disagree on this ;-) All roads leads to Heiko... ;) It takes long term commitment. Year in, year out. Keeping yourself up to date with the latest pcei development. Communicate in English. Be able to submit patches without errors... ;) Review other peoples patches. Respond in short time. Bug fixing. If that's what you really want, then you must include a patch to this serie for MAINTAINERS. Check patch with: ./scripts/parse-maintainers.pl --input=MAINTAINERS --output=MAINTAINERS --order Otherwise it's safe to include that person mentioned above. > > The maintainer for individual drivers should be the persons who are > actually know the hardware. We have individual Rockchip developers > taking care of other drivers as well already. > > And normally scripts/get_maintainer.pl should already include me > due to the wildcard for things having "rockchip" in the name. > > > Heiko > > > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller 2021-01-19 15:11 ` Johan Jonker @ 2021-01-19 18:40 ` Robin Murphy -1 siblings, 0 replies; 32+ messages in thread From: Robin Murphy @ 2021-01-19 18:40 UTC (permalink / raw) To: Johan Jonker, Heiko Stübner, Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, devicetree, Rob Herring, linux-rockchip On 2021-01-19 15:11, Johan Jonker wrote: > Hi Simon, Heiko, > > On 1/19/21 2:14 PM, Heiko Stübner wrote: >> Hi Johan, >> >> Am Dienstag, 19. Januar 2021, 14:07:41 CET schrieb Johan Jonker: >>> Hi Simon, >>> >>> Thank you for this patch for rk3568 pcie. >>> >>> Include the Rockchip device tree maintainer and all other people/lists >>> to the CC list. >>> >>> ./scripts/checkpatch.pl --strict <patch1> <patch2> >>> >>> ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback >>> --nogit <patch1> <patch2> >>> >>> git send-email --suppress-cc all --dry-run --annotate --to >>> heiko@sntech.de --cc <..> <patch1> <patch2> >>> >>> This SoC has no support in mainline linux kernel yet. >>> In all the following yaml documents for rk3568 we need headers with >>> defines for clocks and power domains, etc. >>> >>> For example: >>> #include <dt-bindings/clock/rk3568-cru.h> >>> #include <dt-bindings/power/rk3568-power.h> >>> >>> Could Rockchip submit first clocks and power drivers entries and a basic >>> rk3568.dtsi + evb dts? >>> Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. >>> >>> A dtbs_check only works with a complete dtsi and evb dts. >>> >>> make ARCH=arm64 dtbs_check >>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >>> >>> On 1/18/21 10:17 AM, Simon Xue wrote: >>>> Signed-off-by: Simon Xue <xxm@rock-chips.com> >>>> --- >>>> .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ >>>> 1 file changed, 101 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..fa664cfffb29 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >>>> @@ -0,0 +1,101 @@ >>>> +# 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> >>> >>> maintainers: >>> - Heiko Stuebner <heiko@sntech.de> >>> >>> Add only people with maintainer rights. >> >> I'd disagree on this ;-) > > All roads leads to Heiko... ;) > > It takes long term commitment. > Year in, year out. > Keeping yourself up to date with the latest pcei development. > Communicate in English. > Be able to submit patches without errors... ;) > Review other peoples patches. > Respond in short time. > Bug fixing. Crikey, it's only a DT binding... :/ > If that's what you really want, then you must include a patch to this > serie for MAINTAINERS. I think if Bjorn and Lorenzo want a specifically named sub-maintainer for the driver itself, we can let them say so rather than presume. Robin. > > Check patch with: > > ./scripts/parse-maintainers.pl --input=MAINTAINERS --output=MAINTAINERS > --order > > Otherwise it's safe to include that person mentioned above. > >> >> The maintainer for individual drivers should be the persons who are >> actually know the hardware. We have individual Rockchip developers >> taking care of other drivers as well already. >> >> And normally scripts/get_maintainer.pl should already include me >> due to the wildcard for things having "rockchip" in the name. >> >> >> Heiko >> >> >> > > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-19 18:40 ` Robin Murphy 0 siblings, 0 replies; 32+ messages in thread From: Robin Murphy @ 2021-01-19 18:40 UTC (permalink / raw) To: Johan Jonker, Heiko Stübner, Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, linux-rockchip, Rob Herring, devicetree On 2021-01-19 15:11, Johan Jonker wrote: > Hi Simon, Heiko, > > On 1/19/21 2:14 PM, Heiko Stübner wrote: >> Hi Johan, >> >> Am Dienstag, 19. Januar 2021, 14:07:41 CET schrieb Johan Jonker: >>> Hi Simon, >>> >>> Thank you for this patch for rk3568 pcie. >>> >>> Include the Rockchip device tree maintainer and all other people/lists >>> to the CC list. >>> >>> ./scripts/checkpatch.pl --strict <patch1> <patch2> >>> >>> ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback >>> --nogit <patch1> <patch2> >>> >>> git send-email --suppress-cc all --dry-run --annotate --to >>> heiko@sntech.de --cc <..> <patch1> <patch2> >>> >>> This SoC has no support in mainline linux kernel yet. >>> In all the following yaml documents for rk3568 we need headers with >>> defines for clocks and power domains, etc. >>> >>> For example: >>> #include <dt-bindings/clock/rk3568-cru.h> >>> #include <dt-bindings/power/rk3568-power.h> >>> >>> Could Rockchip submit first clocks and power drivers entries and a basic >>> rk3568.dtsi + evb dts? >>> Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. >>> >>> A dtbs_check only works with a complete dtsi and evb dts. >>> >>> make ARCH=arm64 dtbs_check >>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >>> >>> On 1/18/21 10:17 AM, Simon Xue wrote: >>>> Signed-off-by: Simon Xue <xxm@rock-chips.com> >>>> --- >>>> .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ >>>> 1 file changed, 101 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..fa664cfffb29 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >>>> @@ -0,0 +1,101 @@ >>>> +# 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> >>> >>> maintainers: >>> - Heiko Stuebner <heiko@sntech.de> >>> >>> Add only people with maintainer rights. >> >> I'd disagree on this ;-) > > All roads leads to Heiko... ;) > > It takes long term commitment. > Year in, year out. > Keeping yourself up to date with the latest pcei development. > Communicate in English. > Be able to submit patches without errors... ;) > Review other peoples patches. > Respond in short time. > Bug fixing. Crikey, it's only a DT binding... :/ > If that's what you really want, then you must include a patch to this > serie for MAINTAINERS. I think if Bjorn and Lorenzo want a specifically named sub-maintainer for the driver itself, we can let them say so rather than presume. Robin. > > Check patch with: > > ./scripts/parse-maintainers.pl --input=MAINTAINERS --output=MAINTAINERS > --order > > Otherwise it's safe to include that person mentioned above. > >> >> The maintainer for individual drivers should be the persons who are >> actually know the hardware. We have individual Rockchip developers >> taking care of other drivers as well already. >> >> And normally scripts/get_maintainer.pl should already include me >> due to the wildcard for things having "rockchip" in the name. >> >> >> Heiko >> >> >> > > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller 2021-01-19 18:40 ` Robin Murphy @ 2021-01-20 14:16 ` Rob Herring -1 siblings, 0 replies; 32+ messages in thread From: Rob Herring @ 2021-01-20 14:16 UTC (permalink / raw) To: Robin Murphy Cc: Johan Jonker, Heiko Stübner, Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi, PCI, devicetree, open list:ARM/Rockchip SoC... On Tue, Jan 19, 2021 at 12:40 PM Robin Murphy <robin.murphy@arm.com> wrote: > > On 2021-01-19 15:11, Johan Jonker wrote: > > Hi Simon, Heiko, > > > > On 1/19/21 2:14 PM, Heiko Stübner wrote: > >> Hi Johan, > >> > >> Am Dienstag, 19. Januar 2021, 14:07:41 CET schrieb Johan Jonker: > >>> Hi Simon, > >>> > >>> Thank you for this patch for rk3568 pcie. > >>> > >>> Include the Rockchip device tree maintainer and all other people/lists > >>> to the CC list. > >>> > >>> ./scripts/checkpatch.pl --strict <patch1> <patch2> > >>> > >>> ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback > >>> --nogit <patch1> <patch2> > >>> > >>> git send-email --suppress-cc all --dry-run --annotate --to > >>> heiko@sntech.de --cc <..> <patch1> <patch2> > >>> > >>> This SoC has no support in mainline linux kernel yet. > >>> In all the following yaml documents for rk3568 we need headers with > >>> defines for clocks and power domains, etc. > >>> > >>> For example: > >>> #include <dt-bindings/clock/rk3568-cru.h> > >>> #include <dt-bindings/power/rk3568-power.h> > >>> > >>> Could Rockchip submit first clocks and power drivers entries and a basic > >>> rk3568.dtsi + evb dts? > >>> Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. > >>> > >>> A dtbs_check only works with a complete dtsi and evb dts. > >>> > >>> make ARCH=arm64 dtbs_check > >>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > >>> > >>> On 1/18/21 10:17 AM, Simon Xue wrote: > >>>> Signed-off-by: Simon Xue <xxm@rock-chips.com> > >>>> --- > >>>> .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ > >>>> 1 file changed, 101 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..fa664cfffb29 > >>>> --- /dev/null > >>>> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > >>>> @@ -0,0 +1,101 @@ > >>>> +# 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> > >>> > >>> maintainers: > >>> - Heiko Stuebner <heiko@sntech.de> > >>> > >>> Add only people with maintainer rights. > >> > >> I'd disagree on this ;-) > > > > All roads leads to Heiko... ;) > > > > It takes long term commitment. > > Year in, year out. > > Keeping yourself up to date with the latest pcei development. > > Communicate in English. > > Be able to submit patches without errors... ;) > > Review other peoples patches. > > Respond in short time. > > Bug fixing. > > Crikey, it's only a DT binding... :/ > > > If that's what you really want, then you must include a patch to this > > serie for MAINTAINERS. > > I think if Bjorn and Lorenzo want a specifically named sub-maintainer > for the driver itself, we can let them say so rather than presume. For the binding it's my call. :) This should be someone who cares and knows the h/w. IOW, if I want to delete the binding, someone who will object. Of course, I'd like that someone to have all the above qualities too. Rob ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-20 14:16 ` Rob Herring 0 siblings, 0 replies; 32+ messages in thread From: Rob Herring @ 2021-01-20 14:16 UTC (permalink / raw) To: Robin Murphy Cc: devicetree, Lorenzo Pieralisi, Heiko Stübner, PCI, open list:ARM/Rockchip SoC..., Bjorn Helgaas, Johan Jonker, Simon Xue On Tue, Jan 19, 2021 at 12:40 PM Robin Murphy <robin.murphy@arm.com> wrote: > > On 2021-01-19 15:11, Johan Jonker wrote: > > Hi Simon, Heiko, > > > > On 1/19/21 2:14 PM, Heiko Stübner wrote: > >> Hi Johan, > >> > >> Am Dienstag, 19. Januar 2021, 14:07:41 CET schrieb Johan Jonker: > >>> Hi Simon, > >>> > >>> Thank you for this patch for rk3568 pcie. > >>> > >>> Include the Rockchip device tree maintainer and all other people/lists > >>> to the CC list. > >>> > >>> ./scripts/checkpatch.pl --strict <patch1> <patch2> > >>> > >>> ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback > >>> --nogit <patch1> <patch2> > >>> > >>> git send-email --suppress-cc all --dry-run --annotate --to > >>> heiko@sntech.de --cc <..> <patch1> <patch2> > >>> > >>> This SoC has no support in mainline linux kernel yet. > >>> In all the following yaml documents for rk3568 we need headers with > >>> defines for clocks and power domains, etc. > >>> > >>> For example: > >>> #include <dt-bindings/clock/rk3568-cru.h> > >>> #include <dt-bindings/power/rk3568-power.h> > >>> > >>> Could Rockchip submit first clocks and power drivers entries and a basic > >>> rk3568.dtsi + evb dts? > >>> Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. > >>> > >>> A dtbs_check only works with a complete dtsi and evb dts. > >>> > >>> make ARCH=arm64 dtbs_check > >>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > >>> > >>> On 1/18/21 10:17 AM, Simon Xue wrote: > >>>> Signed-off-by: Simon Xue <xxm@rock-chips.com> > >>>> --- > >>>> .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ > >>>> 1 file changed, 101 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..fa664cfffb29 > >>>> --- /dev/null > >>>> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > >>>> @@ -0,0 +1,101 @@ > >>>> +# 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> > >>> > >>> maintainers: > >>> - Heiko Stuebner <heiko@sntech.de> > >>> > >>> Add only people with maintainer rights. > >> > >> I'd disagree on this ;-) > > > > All roads leads to Heiko... ;) > > > > It takes long term commitment. > > Year in, year out. > > Keeping yourself up to date with the latest pcei development. > > Communicate in English. > > Be able to submit patches without errors... ;) > > Review other peoples patches. > > Respond in short time. > > Bug fixing. > > Crikey, it's only a DT binding... :/ > > > If that's what you really want, then you must include a patch to this > > serie for MAINTAINERS. > > I think if Bjorn and Lorenzo want a specifically named sub-maintainer > for the driver itself, we can let them say so rather than presume. For the binding it's my call. :) This should be someone who cares and knows the h/w. IOW, if I want to delete the binding, someone who will object. Of course, I'd like that someone to have all the above qualities too. Rob _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller 2021-01-20 14:16 ` Rob Herring @ 2021-01-20 14:54 ` Heiko Stübner -1 siblings, 0 replies; 32+ messages in thread From: Heiko Stübner @ 2021-01-20 14:54 UTC (permalink / raw) To: Robin Murphy, Rob Herring Cc: Johan Jonker, Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi, PCI, devicetree, open list:ARM/Rockchip SoC... Am Mittwoch, 20. Januar 2021, 15:16:25 CET schrieb Rob Herring: > On Tue, Jan 19, 2021 at 12:40 PM Robin Murphy <robin.murphy@arm.com> wrote: > > > > On 2021-01-19 15:11, Johan Jonker wrote: > > > Hi Simon, Heiko, > > > > > > On 1/19/21 2:14 PM, Heiko Stübner wrote: > > >> Hi Johan, > > >> > > >> Am Dienstag, 19. Januar 2021, 14:07:41 CET schrieb Johan Jonker: > > >>> Hi Simon, > > >>> > > >>> Thank you for this patch for rk3568 pcie. > > >>> > > >>> Include the Rockchip device tree maintainer and all other people/lists > > >>> to the CC list. > > >>> > > >>> ./scripts/checkpatch.pl --strict <patch1> <patch2> > > >>> > > >>> ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback > > >>> --nogit <patch1> <patch2> > > >>> > > >>> git send-email --suppress-cc all --dry-run --annotate --to > > >>> heiko@sntech.de --cc <..> <patch1> <patch2> > > >>> > > >>> This SoC has no support in mainline linux kernel yet. > > >>> In all the following yaml documents for rk3568 we need headers with > > >>> defines for clocks and power domains, etc. > > >>> > > >>> For example: > > >>> #include <dt-bindings/clock/rk3568-cru.h> > > >>> #include <dt-bindings/power/rk3568-power.h> > > >>> > > >>> Could Rockchip submit first clocks and power drivers entries and a basic > > >>> rk3568.dtsi + evb dts? > > >>> Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. > > >>> > > >>> A dtbs_check only works with a complete dtsi and evb dts. > > >>> > > >>> make ARCH=arm64 dtbs_check > > >>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > >>> > > >>> On 1/18/21 10:17 AM, Simon Xue wrote: > > >>>> Signed-off-by: Simon Xue <xxm@rock-chips.com> > > >>>> --- > > >>>> .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ > > >>>> 1 file changed, 101 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..fa664cfffb29 > > >>>> --- /dev/null > > >>>> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > >>>> @@ -0,0 +1,101 @@ > > >>>> +# 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> > > >>> > > >>> maintainers: > > >>> - Heiko Stuebner <heiko@sntech.de> > > >>> > > >>> Add only people with maintainer rights. > > >> > > >> I'd disagree on this ;-) > > > > > > All roads leads to Heiko... ;) > > > > > > It takes long term commitment. > > > Year in, year out. > > > Keeping yourself up to date with the latest pcei development. > > > Communicate in English. > > > Be able to submit patches without errors... ;) > > > Review other peoples patches. > > > Respond in short time. > > > Bug fixing. > > > > Crikey, it's only a DT binding... :/ > > > > > If that's what you really want, then you must include a patch to this > > > serie for MAINTAINERS. > > > > I think if Bjorn and Lorenzo want a specifically named sub-maintainer > > for the driver itself, we can let them say so rather than presume. > > For the binding it's my call. :) > > This should be someone who cares and knows the h/w. IOW, if I want to > delete the binding, someone who will object. > > Of course, I'd like that someone to have all the above qualities too. I guess that would be separate entites then ... Shawn and Simon know the hardware way better, though I'm not sure if their work commitments will allow them to keep track of binding deletions So maybe all 3 of us ;-) Heiko ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-20 14:54 ` Heiko Stübner 0 siblings, 0 replies; 32+ messages in thread From: Heiko Stübner @ 2021-01-20 14:54 UTC (permalink / raw) To: Robin Murphy, Rob Herring Cc: devicetree, Lorenzo Pieralisi, Simon Xue, PCI, open list:ARM/Rockchip SoC..., Bjorn Helgaas, Johan Jonker Am Mittwoch, 20. Januar 2021, 15:16:25 CET schrieb Rob Herring: > On Tue, Jan 19, 2021 at 12:40 PM Robin Murphy <robin.murphy@arm.com> wrote: > > > > On 2021-01-19 15:11, Johan Jonker wrote: > > > Hi Simon, Heiko, > > > > > > On 1/19/21 2:14 PM, Heiko Stübner wrote: > > >> Hi Johan, > > >> > > >> Am Dienstag, 19. Januar 2021, 14:07:41 CET schrieb Johan Jonker: > > >>> Hi Simon, > > >>> > > >>> Thank you for this patch for rk3568 pcie. > > >>> > > >>> Include the Rockchip device tree maintainer and all other people/lists > > >>> to the CC list. > > >>> > > >>> ./scripts/checkpatch.pl --strict <patch1> <patch2> > > >>> > > >>> ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback > > >>> --nogit <patch1> <patch2> > > >>> > > >>> git send-email --suppress-cc all --dry-run --annotate --to > > >>> heiko@sntech.de --cc <..> <patch1> <patch2> > > >>> > > >>> This SoC has no support in mainline linux kernel yet. > > >>> In all the following yaml documents for rk3568 we need headers with > > >>> defines for clocks and power domains, etc. > > >>> > > >>> For example: > > >>> #include <dt-bindings/clock/rk3568-cru.h> > > >>> #include <dt-bindings/power/rk3568-power.h> > > >>> > > >>> Could Rockchip submit first clocks and power drivers entries and a basic > > >>> rk3568.dtsi + evb dts? > > >>> Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. > > >>> > > >>> A dtbs_check only works with a complete dtsi and evb dts. > > >>> > > >>> make ARCH=arm64 dtbs_check > > >>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > >>> > > >>> On 1/18/21 10:17 AM, Simon Xue wrote: > > >>>> Signed-off-by: Simon Xue <xxm@rock-chips.com> > > >>>> --- > > >>>> .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ > > >>>> 1 file changed, 101 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..fa664cfffb29 > > >>>> --- /dev/null > > >>>> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > >>>> @@ -0,0 +1,101 @@ > > >>>> +# 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> > > >>> > > >>> maintainers: > > >>> - Heiko Stuebner <heiko@sntech.de> > > >>> > > >>> Add only people with maintainer rights. > > >> > > >> I'd disagree on this ;-) > > > > > > All roads leads to Heiko... ;) > > > > > > It takes long term commitment. > > > Year in, year out. > > > Keeping yourself up to date with the latest pcei development. > > > Communicate in English. > > > Be able to submit patches without errors... ;) > > > Review other peoples patches. > > > Respond in short time. > > > Bug fixing. > > > > Crikey, it's only a DT binding... :/ > > > > > If that's what you really want, then you must include a patch to this > > > serie for MAINTAINERS. > > > > I think if Bjorn and Lorenzo want a specifically named sub-maintainer > > for the driver itself, we can let them say so rather than presume. > > For the binding it's my call. :) > > This should be someone who cares and knows the h/w. IOW, if I want to > delete the binding, someone who will object. > > Of course, I'd like that someone to have all the above qualities too. I guess that would be separate entites then ... Shawn and Simon know the hardware way better, though I'm not sure if their work commitments will allow them to keep track of binding deletions So maybe all 3 of us ;-) Heiko _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller 2021-01-20 14:54 ` Heiko Stübner @ 2021-01-20 16:20 ` Robin Murphy -1 siblings, 0 replies; 32+ messages in thread From: Robin Murphy @ 2021-01-20 16:20 UTC (permalink / raw) To: Heiko Stübner, Rob Herring Cc: Johan Jonker, Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi, PCI, devicetree, open list:ARM/Rockchip SoC... On 2021-01-20 14:54, Heiko Stübner wrote: > Am Mittwoch, 20. Januar 2021, 15:16:25 CET schrieb Rob Herring: >> On Tue, Jan 19, 2021 at 12:40 PM Robin Murphy <robin.murphy@arm.com> wrote: >>> >>> On 2021-01-19 15:11, Johan Jonker wrote: >>>> Hi Simon, Heiko, >>>> >>>> On 1/19/21 2:14 PM, Heiko Stübner wrote: >>>>> Hi Johan, >>>>> >>>>> Am Dienstag, 19. Januar 2021, 14:07:41 CET schrieb Johan Jonker: >>>>>> Hi Simon, >>>>>> >>>>>> Thank you for this patch for rk3568 pcie. >>>>>> >>>>>> Include the Rockchip device tree maintainer and all other people/lists >>>>>> to the CC list. >>>>>> >>>>>> ./scripts/checkpatch.pl --strict <patch1> <patch2> >>>>>> >>>>>> ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback >>>>>> --nogit <patch1> <patch2> >>>>>> >>>>>> git send-email --suppress-cc all --dry-run --annotate --to >>>>>> heiko@sntech.de --cc <..> <patch1> <patch2> >>>>>> >>>>>> This SoC has no support in mainline linux kernel yet. >>>>>> In all the following yaml documents for rk3568 we need headers with >>>>>> defines for clocks and power domains, etc. >>>>>> >>>>>> For example: >>>>>> #include <dt-bindings/clock/rk3568-cru.h> >>>>>> #include <dt-bindings/power/rk3568-power.h> >>>>>> >>>>>> Could Rockchip submit first clocks and power drivers entries and a basic >>>>>> rk3568.dtsi + evb dts? >>>>>> Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. >>>>>> >>>>>> A dtbs_check only works with a complete dtsi and evb dts. >>>>>> >>>>>> make ARCH=arm64 dtbs_check >>>>>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >>>>>> >>>>>> On 1/18/21 10:17 AM, Simon Xue wrote: >>>>>>> Signed-off-by: Simon Xue <xxm@rock-chips.com> >>>>>>> --- >>>>>>> .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ >>>>>>> 1 file changed, 101 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..fa664cfffb29 >>>>>>> --- /dev/null >>>>>>> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >>>>>>> @@ -0,0 +1,101 @@ >>>>>>> +# 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> >>>>>> >>>>>> maintainers: >>>>>> - Heiko Stuebner <heiko@sntech.de> >>>>>> >>>>>> Add only people with maintainer rights. >>>>> >>>>> I'd disagree on this ;-) >>>> >>>> All roads leads to Heiko... ;) >>>> >>>> It takes long term commitment. >>>> Year in, year out. >>>> Keeping yourself up to date with the latest pcei development. >>>> Communicate in English. >>>> Be able to submit patches without errors... ;) >>>> Review other peoples patches. >>>> Respond in short time. >>>> Bug fixing. >>> >>> Crikey, it's only a DT binding... :/ >>> >>>> If that's what you really want, then you must include a patch to this >>>> serie for MAINTAINERS. >>> >>> I think if Bjorn and Lorenzo want a specifically named sub-maintainer >>> for the driver itself, we can let them say so rather than presume. >> >> For the binding it's my call. :) >> >> This should be someone who cares and knows the h/w. IOW, if I want to >> delete the binding, someone who will object. >> >> Of course, I'd like that someone to have all the above qualities too. > > I guess that would be separate entites then ... Yup, that's the point I was trying to clarify - binding maintainer and driver maintainer are technically distinct roles. Of course if someone is a driver maintainer then it's usual - and preferable - for them to maintain the relevant bindings as well, so MAINTAINERS entries for drivers typically also cover those for convenience and pre-schema historical reasons. However, someone can take responsibility for a binding without signing up to explicitly maintain a corresponding driver (I know I have), and in that case the schema makes the binding self-documenting already - we don't have MAINTAINERS entries that *only* cover bindings, other than the top-level one that says Rob's in charge overall :) Robin. > Shawn and Simon know the hardware way better, though I'm not sure if their > work commitments will allow them to keep track of binding deletions > > So maybe all 3 of us ;-) > > Heiko > > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-20 16:20 ` Robin Murphy 0 siblings, 0 replies; 32+ messages in thread From: Robin Murphy @ 2021-01-20 16:20 UTC (permalink / raw) To: Heiko Stübner, Rob Herring Cc: devicetree, Lorenzo Pieralisi, Simon Xue, PCI, open list:ARM/Rockchip SoC..., Bjorn Helgaas, Johan Jonker On 2021-01-20 14:54, Heiko Stübner wrote: > Am Mittwoch, 20. Januar 2021, 15:16:25 CET schrieb Rob Herring: >> On Tue, Jan 19, 2021 at 12:40 PM Robin Murphy <robin.murphy@arm.com> wrote: >>> >>> On 2021-01-19 15:11, Johan Jonker wrote: >>>> Hi Simon, Heiko, >>>> >>>> On 1/19/21 2:14 PM, Heiko Stübner wrote: >>>>> Hi Johan, >>>>> >>>>> Am Dienstag, 19. Januar 2021, 14:07:41 CET schrieb Johan Jonker: >>>>>> Hi Simon, >>>>>> >>>>>> Thank you for this patch for rk3568 pcie. >>>>>> >>>>>> Include the Rockchip device tree maintainer and all other people/lists >>>>>> to the CC list. >>>>>> >>>>>> ./scripts/checkpatch.pl --strict <patch1> <patch2> >>>>>> >>>>>> ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback >>>>>> --nogit <patch1> <patch2> >>>>>> >>>>>> git send-email --suppress-cc all --dry-run --annotate --to >>>>>> heiko@sntech.de --cc <..> <patch1> <patch2> >>>>>> >>>>>> This SoC has no support in mainline linux kernel yet. >>>>>> In all the following yaml documents for rk3568 we need headers with >>>>>> defines for clocks and power domains, etc. >>>>>> >>>>>> For example: >>>>>> #include <dt-bindings/clock/rk3568-cru.h> >>>>>> #include <dt-bindings/power/rk3568-power.h> >>>>>> >>>>>> Could Rockchip submit first clocks and power drivers entries and a basic >>>>>> rk3568.dtsi + evb dts? >>>>>> Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. >>>>>> >>>>>> A dtbs_check only works with a complete dtsi and evb dts. >>>>>> >>>>>> make ARCH=arm64 dtbs_check >>>>>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >>>>>> >>>>>> On 1/18/21 10:17 AM, Simon Xue wrote: >>>>>>> Signed-off-by: Simon Xue <xxm@rock-chips.com> >>>>>>> --- >>>>>>> .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ >>>>>>> 1 file changed, 101 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..fa664cfffb29 >>>>>>> --- /dev/null >>>>>>> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >>>>>>> @@ -0,0 +1,101 @@ >>>>>>> +# 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> >>>>>> >>>>>> maintainers: >>>>>> - Heiko Stuebner <heiko@sntech.de> >>>>>> >>>>>> Add only people with maintainer rights. >>>>> >>>>> I'd disagree on this ;-) >>>> >>>> All roads leads to Heiko... ;) >>>> >>>> It takes long term commitment. >>>> Year in, year out. >>>> Keeping yourself up to date with the latest pcei development. >>>> Communicate in English. >>>> Be able to submit patches without errors... ;) >>>> Review other peoples patches. >>>> Respond in short time. >>>> Bug fixing. >>> >>> Crikey, it's only a DT binding... :/ >>> >>>> If that's what you really want, then you must include a patch to this >>>> serie for MAINTAINERS. >>> >>> I think if Bjorn and Lorenzo want a specifically named sub-maintainer >>> for the driver itself, we can let them say so rather than presume. >> >> For the binding it's my call. :) >> >> This should be someone who cares and knows the h/w. IOW, if I want to >> delete the binding, someone who will object. >> >> Of course, I'd like that someone to have all the above qualities too. > > I guess that would be separate entites then ... Yup, that's the point I was trying to clarify - binding maintainer and driver maintainer are technically distinct roles. Of course if someone is a driver maintainer then it's usual - and preferable - for them to maintain the relevant bindings as well, so MAINTAINERS entries for drivers typically also cover those for convenience and pre-schema historical reasons. However, someone can take responsibility for a binding without signing up to explicitly maintain a corresponding driver (I know I have), and in that case the schema makes the binding self-documenting already - we don't have MAINTAINERS entries that *only* cover bindings, other than the top-level one that says Rob's in charge overall :) Robin. > Shawn and Simon know the hardware way better, though I'm not sure if their > work commitments will allow them to keep track of binding deletions > > So maybe all 3 of us ;-) > > Heiko > > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller 2021-01-19 13:07 ` Johan Jonker @ 2021-01-19 19:58 ` Robin Murphy -1 siblings, 0 replies; 32+ messages in thread From: Robin Murphy @ 2021-01-19 19:58 UTC (permalink / raw) To: Johan Jonker, Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, devicetree, Rob Herring, Heiko Stuebner, linux-rockchip On 2021-01-19 13:07, Johan Jonker wrote: > Hi Simon, > > Thank you for this patch for rk3568 pcie. > > Include the Rockchip device tree maintainer and all other people/lists > to the CC list. > > ./scripts/checkpatch.pl --strict <patch1> <patch2> > > ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback > --nogit <patch1> <patch2> > > git send-email --suppress-cc all --dry-run --annotate --to > heiko@sntech.de --cc <..> <patch1> <patch2> > > This SoC has no support in mainline linux kernel yet. > In all the following yaml documents for rk3568 we need headers with > defines for clocks and power domains, etc. > > For example: > #include <dt-bindings/clock/rk3568-cru.h> > #include <dt-bindings/power/rk3568-power.h> > > Could Rockchip submit first clocks and power drivers entries and a basic > rk3568.dtsi + evb dts? > Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. > > A dtbs_check only works with a complete dtsi and evb dts. This is a bizarrely circular argument - we can't have a binding without a DT to try validating against it? That's not how it usually works. We review and merge bindings first, then drivers that consume them and DTs that implement them. Examples within a binding itself do not need to represent a whole SoC, in fact that would usually just make them unnecessarily complicated. They do need to compile correctly in the `make dt_binding_check` environment, but if anything it makes more sense to strip down and simplify the example to fit that environment than to pile in enough other crap to realistically model all the supporting components from a particular SoC. Not least because the more extra stuff has to be crammed in, the more it distracts from the fundamental purpose of demonstrating *that specific binding* in use. For instance, a "clocks" property can quite happily use arbitrary numeric specifiers targeting a dummy clock controller node; requiring that some device binding depend on a particular clock controller binding being upstreamed just so that SoC-specific identifiers can then be used in an example would be rather absurd. Robin. > make ARCH=arm64 dtbs_check > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > On 1/18/21 10:17 AM, Simon Xue wrote: >> Signed-off-by: Simon Xue <xxm@rock-chips.com> >> --- >> .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ >> 1 file changed, 101 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..fa664cfffb29 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >> @@ -0,0 +1,101 @@ >> +# 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> > > maintainers: > - Heiko Stuebner <heiko@sntech.de> > > Add only people with maintainer rights. > > > allOf: > - $ref: /schemas/pci/pci-bus.yaml# > > designware-pcie.txt is in need for conversion to yaml. > Include the things that are needed in this document for now. > >> + >> +# 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: > >> + enum: >> + - rockchip,rk3568-pcie >> + - snps,dw-pcie > > items: > - const: rockchip,rk3568-pcie > - const: snps,dw-pcie > >> + >> + reg: >> + maxItems: 1 > > interrupts: > - description: > - description: > - description: > - description: > - description: > > 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 > >> + items: >> + - description: PCIe pipe reset line > > remove > >> + >> + reset-names: > const: pipe > >> + items: >> + - const: pipe > > remove > >> + >> +required: >> + - compatible > >> + - "#address-cells" >> + - "#size-cells" > > already required in pci-bus.yaml > >> + - bus-range >> + - reg >> + - reg-names >> + - clocks >> + - clock-names > >> + - msi-map > > not defined in pci-bus.yaml and designware-pcie.txt > add to document > >> + - num-lanes > >> + - phys >> + - phy-names > > not defined in pci-bus.yaml and designware-pcie.txt > add to document > > - power-domains > >> + - ranges > > already required in pci-bus.yaml > >> + - resets >> + - reset-names >> + > >> +additionalProperties: false > > unevaluatedProperties: false > > If other documents are included use unevaluatedProperties. > >> + >> +examples: >> + - | > > #include <dt-bindings/clock/rk3568-cru.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/power/rk3568-power.h> > > Missing defines > > > bus { > #address-cells = <2>; > #size-cells = <2>; > > dt-check uses standard 32 bit regs > this example is for 64 bit > >> + pcie3x2: pcie@fe280000 { >> + compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; > >> + #address-cells = <3>; >> + #size-cells = <2>; > > sort things that start with # below > >> + bus-range = <0x20 0x2f>; > > sort order is: > compatible > reg > interrupt > the rest in alphabetical order > things with # > no status in yaml examples > > >> + reg = <0x3 0xc0800000 0x0 0x400000>, >> + <0x0 0xfe280000 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"; > >> + 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"; >> + }; > > }; > >> + >> +... >> > > Make sure that all properties that show up in this node are checked! > > pcie2x1: pcie@fe260000 { > compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; > #address-cells = <3>; > #size-cells = <2>; > 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"; > 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"; > 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>; > reg = <0x3 0xc0000000 0x0 0x400000>, > <0x0 0xfe260000 0x0 0x10000>; > reg-names = "pcie-dbi", "pcie-apb"; > resets = <&cru SRST_PCIE20_POWERUP>; > reset-names = "pipe"; > status = "disabled"; > }; > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-19 19:58 ` Robin Murphy 0 siblings, 0 replies; 32+ messages in thread From: Robin Murphy @ 2021-01-19 19:58 UTC (permalink / raw) To: Johan Jonker, Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, linux-rockchip, Rob Herring, Heiko Stuebner, devicetree On 2021-01-19 13:07, Johan Jonker wrote: > Hi Simon, > > Thank you for this patch for rk3568 pcie. > > Include the Rockchip device tree maintainer and all other people/lists > to the CC list. > > ./scripts/checkpatch.pl --strict <patch1> <patch2> > > ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback > --nogit <patch1> <patch2> > > git send-email --suppress-cc all --dry-run --annotate --to > heiko@sntech.de --cc <..> <patch1> <patch2> > > This SoC has no support in mainline linux kernel yet. > In all the following yaml documents for rk3568 we need headers with > defines for clocks and power domains, etc. > > For example: > #include <dt-bindings/clock/rk3568-cru.h> > #include <dt-bindings/power/rk3568-power.h> > > Could Rockchip submit first clocks and power drivers entries and a basic > rk3568.dtsi + evb dts? > Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. > > A dtbs_check only works with a complete dtsi and evb dts. This is a bizarrely circular argument - we can't have a binding without a DT to try validating against it? That's not how it usually works. We review and merge bindings first, then drivers that consume them and DTs that implement them. Examples within a binding itself do not need to represent a whole SoC, in fact that would usually just make them unnecessarily complicated. They do need to compile correctly in the `make dt_binding_check` environment, but if anything it makes more sense to strip down and simplify the example to fit that environment than to pile in enough other crap to realistically model all the supporting components from a particular SoC. Not least because the more extra stuff has to be crammed in, the more it distracts from the fundamental purpose of demonstrating *that specific binding* in use. For instance, a "clocks" property can quite happily use arbitrary numeric specifiers targeting a dummy clock controller node; requiring that some device binding depend on a particular clock controller binding being upstreamed just so that SoC-specific identifiers can then be used in an example would be rather absurd. Robin. > make ARCH=arm64 dtbs_check > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > On 1/18/21 10:17 AM, Simon Xue wrote: >> Signed-off-by: Simon Xue <xxm@rock-chips.com> >> --- >> .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ >> 1 file changed, 101 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..fa664cfffb29 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >> @@ -0,0 +1,101 @@ >> +# 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> > > maintainers: > - Heiko Stuebner <heiko@sntech.de> > > Add only people with maintainer rights. > > > allOf: > - $ref: /schemas/pci/pci-bus.yaml# > > designware-pcie.txt is in need for conversion to yaml. > Include the things that are needed in this document for now. > >> + >> +# 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: > >> + enum: >> + - rockchip,rk3568-pcie >> + - snps,dw-pcie > > items: > - const: rockchip,rk3568-pcie > - const: snps,dw-pcie > >> + >> + reg: >> + maxItems: 1 > > interrupts: > - description: > - description: > - description: > - description: > - description: > > 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 > >> + items: >> + - description: PCIe pipe reset line > > remove > >> + >> + reset-names: > const: pipe > >> + items: >> + - const: pipe > > remove > >> + >> +required: >> + - compatible > >> + - "#address-cells" >> + - "#size-cells" > > already required in pci-bus.yaml > >> + - bus-range >> + - reg >> + - reg-names >> + - clocks >> + - clock-names > >> + - msi-map > > not defined in pci-bus.yaml and designware-pcie.txt > add to document > >> + - num-lanes > >> + - phys >> + - phy-names > > not defined in pci-bus.yaml and designware-pcie.txt > add to document > > - power-domains > >> + - ranges > > already required in pci-bus.yaml > >> + - resets >> + - reset-names >> + > >> +additionalProperties: false > > unevaluatedProperties: false > > If other documents are included use unevaluatedProperties. > >> + >> +examples: >> + - | > > #include <dt-bindings/clock/rk3568-cru.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/power/rk3568-power.h> > > Missing defines > > > bus { > #address-cells = <2>; > #size-cells = <2>; > > dt-check uses standard 32 bit regs > this example is for 64 bit > >> + pcie3x2: pcie@fe280000 { >> + compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; > >> + #address-cells = <3>; >> + #size-cells = <2>; > > sort things that start with # below > >> + bus-range = <0x20 0x2f>; > > sort order is: > compatible > reg > interrupt > the rest in alphabetical order > things with # > no status in yaml examples > > >> + reg = <0x3 0xc0800000 0x0 0x400000>, >> + <0x0 0xfe280000 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"; > >> + 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"; >> + }; > > }; > >> + >> +... >> > > Make sure that all properties that show up in this node are checked! > > pcie2x1: pcie@fe260000 { > compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; > #address-cells = <3>; > #size-cells = <2>; > 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"; > 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"; > 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>; > reg = <0x3 0xc0000000 0x0 0x400000>, > <0x0 0xfe260000 0x0 0x10000>; > reg-names = "pcie-dbi", "pcie-apb"; > resets = <&cru SRST_PCIE20_POWERUP>; > reset-names = "pipe"; > status = "disabled"; > }; > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller 2021-01-19 13:07 ` Johan Jonker @ 2021-01-20 10:06 ` xxm -1 siblings, 0 replies; 32+ messages in thread From: xxm @ 2021-01-20 10:06 UTC (permalink / raw) To: Johan Jonker, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, linux-rockchip, Heiko Stuebner, Rob Herring, devicetree Hi Johan, Thanks for your review, I try to fix what you point out and get some consultations from others, please help to have a look at the patch V2 Simon Xue. 在 2021/1/19 21:07, Johan Jonker 写道: > Hi Simon, > > Thank you for this patch for rk3568 pcie. > > Include the Rockchip device tree maintainer and all other people/lists > to the CC list. > > ./scripts/checkpatch.pl --strict <patch1> <patch2> > > ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback > --nogit <patch1> <patch2> > > git send-email --suppress-cc all --dry-run --annotate --to > heiko@sntech.de --cc <..> <patch1> <patch2> > > This SoC has no support in mainline linux kernel yet. > In all the following yaml documents for rk3568 we need headers with > defines for clocks and power domains, etc. > > For example: > #include <dt-bindings/clock/rk3568-cru.h> > #include <dt-bindings/power/rk3568-power.h> > > Could Rockchip submit first clocks and power drivers entries and a basic > rk3568.dtsi + evb dts? > Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. > > A dtbs_check only works with a complete dtsi and evb dts. > > make ARCH=arm64 dtbs_check > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > On 1/18/21 10:17 AM, Simon Xue wrote: >> Signed-off-by: Simon Xue <xxm@rock-chips.com> >> --- >> .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ >> 1 file changed, 101 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..fa664cfffb29 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >> @@ -0,0 +1,101 @@ >> +# 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> > maintainers: > - Heiko Stuebner <heiko@sntech.de> > > Add only people with maintainer rights. > > > allOf: > - $ref: /schemas/pci/pci-bus.yaml# > > designware-pcie.txt is in need for conversion to yaml. > Include the things that are needed in this document for now. > >> + >> +# 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: >> + enum: >> + - rockchip,rk3568-pcie >> + - snps,dw-pcie > items: > - const: rockchip,rk3568-pcie > - const: snps,dw-pcie > >> + >> + reg: >> + maxItems: 1 > interrupts: > - description: > - description: > - description: > - description: > - description: > > 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 >> + items: >> + - description: PCIe pipe reset line > remove > >> + >> + reset-names: > const: pipe > >> + items: >> + - const: pipe > remove > >> + >> +required: >> + - compatible >> + - "#address-cells" >> + - "#size-cells" > already required in pci-bus.yaml > >> + - bus-range >> + - reg >> + - reg-names >> + - clocks >> + - clock-names >> + - msi-map > not defined in pci-bus.yaml and designware-pcie.txt > add to document > >> + - num-lanes >> + - phys >> + - phy-names > not defined in pci-bus.yaml and designware-pcie.txt > add to document > > - power-domains > >> + - ranges > already required in pci-bus.yaml > >> + - resets >> + - reset-names >> + >> +additionalProperties: false > unevaluatedProperties: false > > If other documents are included use unevaluatedProperties. > >> + >> +examples: >> + - | > #include <dt-bindings/clock/rk3568-cru.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/power/rk3568-power.h> > > Missing defines > > > bus { > #address-cells = <2>; > #size-cells = <2>; > > dt-check uses standard 32 bit regs > this example is for 64 bit > >> + pcie3x2: pcie@fe280000 { >> + compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; >> + #address-cells = <3>; >> + #size-cells = <2>; > sort things that start with # below > >> + bus-range = <0x20 0x2f>; > sort order is: > compatible > reg > interrupt > the rest in alphabetical order > things with # > no status in yaml examples > > >> + reg = <0x3 0xc0800000 0x0 0x400000>, >> + <0x0 0xfe280000 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"; > >> + 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"; >> + }; > }; > >> + >> +... >> > Make sure that all properties that show up in this node are checked! > > pcie2x1: pcie@fe260000 { > compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; > #address-cells = <3>; > #size-cells = <2>; > 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"; > 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"; > 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>; > reg = <0x3 0xc0000000 0x0 0x400000>, > <0x0 0xfe260000 0x0 0x10000>; > reg-names = "pcie-dbi", "pcie-apb"; > resets = <&cru SRST_PCIE20_POWERUP>; > reset-names = "pipe"; > status = "disabled"; > }; > > > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-20 10:06 ` xxm 0 siblings, 0 replies; 32+ messages in thread From: xxm @ 2021-01-20 10:06 UTC (permalink / raw) To: Johan Jonker, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, devicetree, Rob Herring, Heiko Stuebner, linux-rockchip Hi Johan, Thanks for your review, I try to fix what you point out and get some consultations from others, please help to have a look at the patch V2 Simon Xue. 在 2021/1/19 21:07, Johan Jonker 写道: > Hi Simon, > > Thank you for this patch for rk3568 pcie. > > Include the Rockchip device tree maintainer and all other people/lists > to the CC list. > > ./scripts/checkpatch.pl --strict <patch1> <patch2> > > ./scripts/get_maintainer.pl --noroles --norolestats --nogit-fallback > --nogit <patch1> <patch2> > > git send-email --suppress-cc all --dry-run --annotate --to > heiko@sntech.de --cc <..> <patch1> <patch2> > > This SoC has no support in mainline linux kernel yet. > In all the following yaml documents for rk3568 we need headers with > defines for clocks and power domains, etc. > > For example: > #include <dt-bindings/clock/rk3568-cru.h> > #include <dt-bindings/power/rk3568-power.h> > > Could Rockchip submit first clocks and power drivers entries and a basic > rk3568.dtsi + evb dts? > Include a patch to this serie with 3 pcie nodes added to rk3568.dtsi. > > A dtbs_check only works with a complete dtsi and evb dts. > > make ARCH=arm64 dtbs_check > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > On 1/18/21 10:17 AM, Simon Xue wrote: >> Signed-off-by: Simon Xue <xxm@rock-chips.com> >> --- >> .../bindings/pci/rockchip-dw-pcie.yaml | 101 ++++++++++++++++++ >> 1 file changed, 101 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..fa664cfffb29 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >> @@ -0,0 +1,101 @@ >> +# 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> > maintainers: > - Heiko Stuebner <heiko@sntech.de> > > Add only people with maintainer rights. > > > allOf: > - $ref: /schemas/pci/pci-bus.yaml# > > designware-pcie.txt is in need for conversion to yaml. > Include the things that are needed in this document for now. > >> + >> +# 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: >> + enum: >> + - rockchip,rk3568-pcie >> + - snps,dw-pcie > items: > - const: rockchip,rk3568-pcie > - const: snps,dw-pcie > >> + >> + reg: >> + maxItems: 1 > interrupts: > - description: > - description: > - description: > - description: > - description: > > 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 >> + items: >> + - description: PCIe pipe reset line > remove > >> + >> + reset-names: > const: pipe > >> + items: >> + - const: pipe > remove > >> + >> +required: >> + - compatible >> + - "#address-cells" >> + - "#size-cells" > already required in pci-bus.yaml > >> + - bus-range >> + - reg >> + - reg-names >> + - clocks >> + - clock-names >> + - msi-map > not defined in pci-bus.yaml and designware-pcie.txt > add to document > >> + - num-lanes >> + - phys >> + - phy-names > not defined in pci-bus.yaml and designware-pcie.txt > add to document > > - power-domains > >> + - ranges > already required in pci-bus.yaml > >> + - resets >> + - reset-names >> + >> +additionalProperties: false > unevaluatedProperties: false > > If other documents are included use unevaluatedProperties. > >> + >> +examples: >> + - | > #include <dt-bindings/clock/rk3568-cru.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/power/rk3568-power.h> > > Missing defines > > > bus { > #address-cells = <2>; > #size-cells = <2>; > > dt-check uses standard 32 bit regs > this example is for 64 bit > >> + pcie3x2: pcie@fe280000 { >> + compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; >> + #address-cells = <3>; >> + #size-cells = <2>; > sort things that start with # below > >> + bus-range = <0x20 0x2f>; > sort order is: > compatible > reg > interrupt > the rest in alphabetical order > things with # > no status in yaml examples > > >> + reg = <0x3 0xc0800000 0x0 0x400000>, >> + <0x0 0xfe280000 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"; > >> + 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"; >> + }; > }; > >> + >> +... >> > Make sure that all properties that show up in this node are checked! > > pcie2x1: pcie@fe260000 { > compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; > #address-cells = <3>; > #size-cells = <2>; > 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"; > 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"; > 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>; > reg = <0x3 0xc0000000 0x0 0x400000>, > <0x0 0xfe260000 0x0 0x10000>; > reg-names = "pcie-dbi", "pcie-apb"; > resets = <&cru SRST_PCIE20_POWERUP>; > reset-names = "pipe"; > status = "disabled"; > }; > > > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 3/3] PCI: rockchip: add DesignWare based PCIe controller 2021-01-18 9:17 ` Simon Xue @ 2021-01-18 9:17 ` Simon Xue -1 siblings, 0 replies; 32+ messages in thread From: Simon Xue @ 2021-01-18 9:17 UTC (permalink / raw) To: Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, linux-rockchip, Simon Xue, Shawn Lin pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host is another IP which is only used for RK3399. So all the following non-RK3399 SoCs should use this driver. Signed-off-by: Simon Xue <xxm@rock-chips.com> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/pci/controller/Kconfig | 4 +- drivers/pci/controller/dwc/Kconfig | 9 + drivers/pci/controller/dwc/Makefile | 1 + drivers/pci/controller/dwc/pcie-dw-rockchip.c | 454 ++++++++++++++++++ 4 files changed, 466 insertions(+), 2 deletions(-) create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig index 64e2f5e379aa..48a7d62f97f3 100644 --- a/drivers/pci/controller/Kconfig +++ b/drivers/pci/controller/Kconfig @@ -219,7 +219,7 @@ config PCIE_ROCKCHIP_HOST help Say Y here if you want internal PCI support on Rockchip SoC. There is 1 internal PCIe port available to support GEN2 with - 4 slots. + 4 slots. Only for RK3399. config PCIE_ROCKCHIP_EP bool "Rockchip PCIe endpoint controller" @@ -231,7 +231,7 @@ config PCIE_ROCKCHIP_EP help Say Y here if you want to support Rockchip PCIe controller in endpoint mode on Rockchip SoC. There is 1 internal PCIe port - available to support GEN2 with 4 slots. + available to support GEN2 with 4 slots. Only for RK3399. config PCIE_MEDIATEK tristate "MediaTek PCIe controller" diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 22c5529e9a65..110733d0c241 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -214,6 +214,15 @@ config PCIE_ARTPEC6_EP Enables support for the PCIe controller in the ARTPEC-6 SoC to work in endpoint mode. This uses the DesignWare core. +config PCIE_DW_ROCKCHIP_HOST + bool "Rockchip DesignWare PCIe controller" + select PCIE_DW + select PCIE_DW_HOST + depends on ARCH_ROCKCHIP + depends on OF + help + Enables support for the DW PCIe controller in the Rockchip SoC. + config PCIE_INTEL_GW bool "Intel Gateway PCIe host controller support" depends on OF && (X86 || COMPILE_TEST) diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile index a751553fa0db..9c7048d2be17 100644 --- a/drivers/pci/controller/dwc/Makefile +++ b/drivers/pci/controller/dwc/Makefile @@ -13,6 +13,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o +obj-$(CONFIG_PCIE_DW_ROCKCHIP_HOST) += pcie-dw-rockchip.o obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c new file mode 100644 index 000000000000..5997fbc675d3 --- /dev/null +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c @@ -0,0 +1,454 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * PCIe host controller driver for Rockchip SoCs + * + * Copyright (C) 2021 Rockchip Electronics Co., Ltd. + * http://www.rock-chips.com + * + * Author: Simon Xue <xxm@rock-chips.com> + */ + +#include <linux/clk.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/of_gpio.h> +#include <linux/phy/phy.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/reset.h> + +#include "pcie-designware.h" + +/* + * The upper 16 bits of PCIE_CLIENT_CONFIG are a write + * mask for the lower 16 bits. This allows atomic updates + * of the register without locking. + */ +#define HIWORD_UPDATE(mask, val) (((mask) << 16) | (val)) +#define HIWORD_UPDATE_BIT(val) HIWORD_UPDATE(val, val) + +#define to_rockchip_pcie(x) dev_get_drvdata((x)->dev) + +#define PCIE_CLIENT_RC_MODE HIWORD_UPDATE_BIT(0x40) +#define PCIE_CLIENT_ENABLE_LTSSM HIWORD_UPDATE_BIT(0xc) +#define PCIE_SMLH_LINKUP BIT(16) +#define PCIE_RDLH_LINKUP BIT(17) +#define PCIE_L0S_ENTRY 0x11 +#define PCIE_CLIENT_GENERAL_CONTROL 0x0 +#define PCIE_CLIENT_GENERAL_DEBUG 0x104 +#define PCIE_CLIENT_HOT_RESET_CTRL 0x180 +#define PCIE_CLIENT_LTSSM_STATUS 0x300 +#define PCIE_CLIENT_DBG_FIFO_MODE_CON 0x310 +#define PCIE_CLIENT_DBG_FIFO_PTN_HIT_D0 0x320 +#define PCIE_CLIENT_DBG_FIFO_PTN_HIT_D1 0x324 +#define PCIE_CLIENT_DBG_FIFO_TRN_HIT_D0 0x328 +#define PCIE_CLIENT_DBG_FIFO_TRN_HIT_D1 0x32c +#define PCIE_CLIENT_DBG_FIFO_STATUS 0x350 +#define PCIE_CLIENT_DBG_TRANSITION_DATA 0xffff0000 +#define PCIE_CLIENT_DBF_EN 0xffff0003 +#define PCIE_LTSSM_ENABLE_ENHANCE BIT(4) + +struct reset_bulk_data { + const char *id; + struct reset_control *rst; +}; + +struct rockchip_pcie { + struct dw_pcie *pci; + void __iomem *dbi_base; + void __iomem *apb_base; + struct phy *phy; + struct clk_bulk_data *clks; + unsigned int clk_cnt; + struct reset_bulk_data *rsts; + struct gpio_desc *rst_gpio; + struct pcie_port pp; + struct regulator *vpcie3v3; +}; + +static inline int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, + u32 reg) +{ + return readl(rockchip->apb_base + reg); +} + +static inline void rockchip_pcie_writel_apb(struct rockchip_pcie *rockchip, + u32 reg, u32 val) +{ + writel(val, rockchip->apb_base + reg); +} + +static inline void rockchip_pcie_set_mode(struct rockchip_pcie *rockchip) +{ + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_GENERAL_CONTROL, + PCIE_CLIENT_RC_MODE); +} + +static inline void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip) +{ + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_GENERAL_CONTROL, + PCIE_CLIENT_ENABLE_LTSSM); +} + +static int rockchip_pcie_link_up(struct dw_pcie *pci) +{ + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); + u32 val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS); + + if ((val & (PCIE_RDLH_LINKUP | PCIE_SMLH_LINKUP)) == 0x30000 && + (val & GENMASK(5, 0)) == PCIE_L0S_ENTRY) + return 1; + + return 0; +} + +static void rockchip_pcie_establish_link(struct dw_pcie *pci) +{ + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); + + if (dw_pcie_link_up(pci)) { + dev_err(pci->dev, "link already up\n"); + return; + } + + /* Reset device */ + gpiod_set_value_cansleep(rockchip->rst_gpio, 0); + msleep(100); + gpiod_set_value_cansleep(rockchip->rst_gpio, 1); + + rockchip_pcie_enable_ltssm(rockchip); +} + +static void rockchip_pcie_enable_debug(struct rockchip_pcie *rockchip) +{ + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_PTN_HIT_D0, + PCIE_CLIENT_DBG_TRANSITION_DATA); + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_PTN_HIT_D1, + PCIE_CLIENT_DBG_TRANSITION_DATA); + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_TRN_HIT_D0, + PCIE_CLIENT_DBG_TRANSITION_DATA); + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_TRN_HIT_D1, + PCIE_CLIENT_DBG_TRANSITION_DATA); + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_MODE_CON, + PCIE_CLIENT_DBF_EN); +} + +static void rockchip_pcie_debug_dump(struct rockchip_pcie *rockchip) +{ + u32 loop; + struct dw_pcie *pci = rockchip->pci; + + dev_dbg(pci->dev, "ltssm = 0x%x\n", + rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS)); + for (loop = 0; loop < 64; loop++) + dev_dbg(pci->dev, "fifo_status = 0x%x\n", + rockchip_pcie_readl_apb(rockchip, + PCIE_CLIENT_DBG_FIFO_STATUS)); +} + +static int rockchip_pcie_host_init(struct pcie_port *pp) +{ + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); + + dw_pcie_setup_rc(pp); + + rockchip_pcie_enable_debug(rockchip); + + rockchip_pcie_establish_link(pci); + dw_pcie_wait_for_link(pci); + + rockchip_pcie_debug_dump(rockchip); + + return 0; +} + +static const struct dw_pcie_host_ops rockchip_pcie_host_ops = { + .host_init = rockchip_pcie_host_init, +}; + +static int rockchip_add_pcie_port(struct rockchip_pcie *rockchip) +{ + int ret; + struct dw_pcie *pci = rockchip->pci; + struct pcie_port *pp = &pci->pp; + struct device *dev = pci->dev; + + pp->ops = &rockchip_pcie_host_ops; + + if (device_property_read_bool(dev, "msi-map")) + pp->msi_ext = 1; + + rockchip_pcie_set_mode(rockchip); + + ret = dw_pcie_host_init(pp); + if (ret) + return ret; + + return 0; +} + +static void rockchip_pcie_clk_deinit(struct rockchip_pcie *rockchip) +{ + clk_bulk_disable(rockchip->clk_cnt, rockchip->clks); + clk_bulk_unprepare(rockchip->clk_cnt, rockchip->clks); +} + +static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip) +{ + struct device *dev = rockchip->pci->dev; + struct property *prop; + const char *name; + int i = 0, ret, count; + + count = of_property_count_strings(dev->of_node, "clock-names"); + if (count < 1) + return -ENODEV; + + rockchip->clks = devm_kcalloc(dev, count, + sizeof(struct clk_bulk_data), + GFP_KERNEL); + if (!rockchip->clks) + return -ENOMEM; + + rockchip->clk_cnt = count; + + of_property_for_each_string(dev->of_node, "clock-names", + prop, name) { + rockchip->clks[i].id = name; + if (!rockchip->clks[i].id) + return -ENOMEM; + i++; + } + + ret = devm_clk_bulk_get(dev, count, rockchip->clks); + if (ret) + return ret; + + ret = clk_bulk_prepare(count, rockchip->clks); + if (ret) + return ret; + + ret = clk_bulk_enable(count, rockchip->clks); + if (ret) { + clk_bulk_unprepare(count, rockchip->clks); + return ret; + } + + return 0; +} + +static int rockchip_pcie_resource_get(struct platform_device *pdev, + struct rockchip_pcie *rockchip) +{ + struct resource *dbi_base; + struct resource *apb_base; + + dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, + "pcie-dbi"); + if (!dbi_base) + return -ENODEV; + + rockchip->dbi_base = devm_ioremap_resource(&pdev->dev, dbi_base); + if (IS_ERR(rockchip->dbi_base)) + return PTR_ERR(rockchip->dbi_base); + + rockchip->pci->dbi_base = rockchip->dbi_base; + + apb_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, + "pcie-apb"); + if (!apb_base) + return -ENODEV; + + rockchip->apb_base = devm_ioremap_resource(&pdev->dev, apb_base); + if (IS_ERR(rockchip->apb_base)) + return PTR_ERR(rockchip->apb_base); + + rockchip->rst_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", + GPIOD_OUT_HIGH); + if (IS_ERR(rockchip->rst_gpio)) + return PTR_ERR(rockchip->rst_gpio); + + return 0; +} + +static int rockchip_pcie_phy_init(struct rockchip_pcie *rockchip) +{ + int ret; + struct device *dev = rockchip->pci->dev; + + rockchip->phy = devm_phy_get(dev, "pcie-phy"); + if (IS_ERR(rockchip->phy)) { + if (PTR_ERR(rockchip->phy) != -EPROBE_DEFER) + dev_info(dev, "missing phy\n"); + return PTR_ERR(rockchip->phy); + } + + ret = phy_init(rockchip->phy); + if (ret < 0) + return ret; + + phy_power_on(rockchip->phy); + + return 0; +} + +static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip) +{ + phy_exit(rockchip->phy); + phy_power_off(rockchip->phy); +} + +static int rockchip_pcie_reset_control_release(struct rockchip_pcie *rockchip) +{ + struct device *dev = rockchip->pci->dev; + struct property *prop; + const char *name; + int ret, count, i = 0; + + count = of_property_count_strings(dev->of_node, "reset-names"); + if (count < 1) + return -ENODEV; + + rockchip->rsts = devm_kcalloc(dev, count, + sizeof(struct reset_bulk_data), + GFP_KERNEL); + if (!rockchip->rsts) + return -ENOMEM; + + of_property_for_each_string(dev->of_node, "reset-names", + prop, name) { + rockchip->rsts[i].id = name; + if (!rockchip->rsts[i].id) + return -ENOMEM; + i++; + } + + for (i = 0; i < count; i++) { + rockchip->rsts[i].rst = devm_reset_control_get_exclusive(dev, + rockchip->rsts[i].id); + if (IS_ERR(rockchip->rsts[i].rst)) { + dev_err(dev, "failed to get %s\n", + rockchip->clks[i].id); + return PTR_ERR(rockchip->rsts[i].rst); + } + } + + for (i = 0; i < count; i++) { + ret = reset_control_deassert(rockchip->rsts[i].rst); + if (ret) { + dev_err(dev, "failed to release %s\n", + rockchip->rsts[i].id); + return ret; + } + } + + return 0; +} + +static void rockchip_pcie_fast_link_setup(struct rockchip_pcie *rockchip) +{ + u32 val; + + /* LTSSM EN ctrl mode */ + val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_HOT_RESET_CTRL); + val |= PCIE_LTSSM_ENABLE_ENHANCE | (PCIE_LTSSM_ENABLE_ENHANCE << 16); + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_HOT_RESET_CTRL, val); +} + + +static const struct of_device_id rockchip_pcie_of_match[] = { + { .compatible = "rockchip,rk3568-pcie", }, + { /* sentinel */ }, +}; + +static const struct dw_pcie_ops dw_pcie_ops = { + .link_up = rockchip_pcie_link_up, +}; + +static int rockchip_pcie_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct rockchip_pcie *rockchip; + struct dw_pcie *pci; + int ret; + + rockchip = devm_kzalloc(dev, sizeof(*rockchip), GFP_KERNEL); + if (!rockchip) + return -ENOMEM; + + pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); + if (!pci) + return -ENOMEM; + + pci->dev = dev; + pci->ops = &dw_pcie_ops; + + rockchip->pci = pci; + + ret = rockchip_pcie_resource_get(pdev, rockchip); + if (ret) + return ret; + + /* DON'T MOVE ME: must be enable before phy init */ + rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3"); + if (IS_ERR(rockchip->vpcie3v3)) { + if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV) + return PTR_ERR(rockchip->vpcie3v3); + dev_info(dev, "no vpcie3v3 regulator found\n"); + } + + if (!IS_ERR(rockchip->vpcie3v3)) { + ret = regulator_enable(rockchip->vpcie3v3); + if (ret) { + dev_err(pci->dev, "fail to enable vpcie3v3 regulator\n"); + return ret; + } + } + + ret = rockchip_pcie_phy_init(rockchip); + if (ret) + goto disable_regulator; + + ret = rockchip_pcie_reset_control_release(rockchip); + if (ret) + goto deinit_phy; + + ret = rockchip_pcie_clk_init(rockchip); + if (ret) + goto deinit_phy; + + rockchip_pcie_fast_link_setup(rockchip); + + platform_set_drvdata(pdev, rockchip); + + ret = rockchip_add_pcie_port(rockchip); + if (ret) + goto deinit_clk; + + return 0; + +deinit_clk: + rockchip_pcie_clk_deinit(rockchip); +deinit_phy: + rockchip_pcie_phy_deinit(rockchip); +disable_regulator: + if (!IS_ERR(rockchip->vpcie3v3)) + regulator_disable(rockchip->vpcie3v3); + + return ret; +} + +MODULE_DEVICE_TABLE(of, rockchip_pcie_of_match); + +static struct platform_driver rockchip_pcie_driver = { + .driver = { + .name = "rk-pcie", + .of_match_table = rockchip_pcie_of_match, + .suppress_bind_attrs = true, + }, + .probe = rockchip_pcie_probe, +}; + +builtin_platform_driver(rockchip_pcie_driver); -- 2.25.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 3/3] PCI: rockchip: add DesignWare based PCIe controller @ 2021-01-18 9:17 ` Simon Xue 0 siblings, 0 replies; 32+ messages in thread From: Simon Xue @ 2021-01-18 9:17 UTC (permalink / raw) To: Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, Shawn Lin, Simon Xue, linux-rockchip pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host is another IP which is only used for RK3399. So all the following non-RK3399 SoCs should use this driver. Signed-off-by: Simon Xue <xxm@rock-chips.com> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/pci/controller/Kconfig | 4 +- drivers/pci/controller/dwc/Kconfig | 9 + drivers/pci/controller/dwc/Makefile | 1 + drivers/pci/controller/dwc/pcie-dw-rockchip.c | 454 ++++++++++++++++++ 4 files changed, 466 insertions(+), 2 deletions(-) create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig index 64e2f5e379aa..48a7d62f97f3 100644 --- a/drivers/pci/controller/Kconfig +++ b/drivers/pci/controller/Kconfig @@ -219,7 +219,7 @@ config PCIE_ROCKCHIP_HOST help Say Y here if you want internal PCI support on Rockchip SoC. There is 1 internal PCIe port available to support GEN2 with - 4 slots. + 4 slots. Only for RK3399. config PCIE_ROCKCHIP_EP bool "Rockchip PCIe endpoint controller" @@ -231,7 +231,7 @@ config PCIE_ROCKCHIP_EP help Say Y here if you want to support Rockchip PCIe controller in endpoint mode on Rockchip SoC. There is 1 internal PCIe port - available to support GEN2 with 4 slots. + available to support GEN2 with 4 slots. Only for RK3399. config PCIE_MEDIATEK tristate "MediaTek PCIe controller" diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig index 22c5529e9a65..110733d0c241 100644 --- a/drivers/pci/controller/dwc/Kconfig +++ b/drivers/pci/controller/dwc/Kconfig @@ -214,6 +214,15 @@ config PCIE_ARTPEC6_EP Enables support for the PCIe controller in the ARTPEC-6 SoC to work in endpoint mode. This uses the DesignWare core. +config PCIE_DW_ROCKCHIP_HOST + bool "Rockchip DesignWare PCIe controller" + select PCIE_DW + select PCIE_DW_HOST + depends on ARCH_ROCKCHIP + depends on OF + help + Enables support for the DW PCIe controller in the Rockchip SoC. + config PCIE_INTEL_GW bool "Intel Gateway PCIe host controller support" depends on OF && (X86 || COMPILE_TEST) diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile index a751553fa0db..9c7048d2be17 100644 --- a/drivers/pci/controller/dwc/Makefile +++ b/drivers/pci/controller/dwc/Makefile @@ -13,6 +13,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o +obj-$(CONFIG_PCIE_DW_ROCKCHIP_HOST) += pcie-dw-rockchip.o obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c new file mode 100644 index 000000000000..5997fbc675d3 --- /dev/null +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c @@ -0,0 +1,454 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * PCIe host controller driver for Rockchip SoCs + * + * Copyright (C) 2021 Rockchip Electronics Co., Ltd. + * http://www.rock-chips.com + * + * Author: Simon Xue <xxm@rock-chips.com> + */ + +#include <linux/clk.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/of_gpio.h> +#include <linux/phy/phy.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/reset.h> + +#include "pcie-designware.h" + +/* + * The upper 16 bits of PCIE_CLIENT_CONFIG are a write + * mask for the lower 16 bits. This allows atomic updates + * of the register without locking. + */ +#define HIWORD_UPDATE(mask, val) (((mask) << 16) | (val)) +#define HIWORD_UPDATE_BIT(val) HIWORD_UPDATE(val, val) + +#define to_rockchip_pcie(x) dev_get_drvdata((x)->dev) + +#define PCIE_CLIENT_RC_MODE HIWORD_UPDATE_BIT(0x40) +#define PCIE_CLIENT_ENABLE_LTSSM HIWORD_UPDATE_BIT(0xc) +#define PCIE_SMLH_LINKUP BIT(16) +#define PCIE_RDLH_LINKUP BIT(17) +#define PCIE_L0S_ENTRY 0x11 +#define PCIE_CLIENT_GENERAL_CONTROL 0x0 +#define PCIE_CLIENT_GENERAL_DEBUG 0x104 +#define PCIE_CLIENT_HOT_RESET_CTRL 0x180 +#define PCIE_CLIENT_LTSSM_STATUS 0x300 +#define PCIE_CLIENT_DBG_FIFO_MODE_CON 0x310 +#define PCIE_CLIENT_DBG_FIFO_PTN_HIT_D0 0x320 +#define PCIE_CLIENT_DBG_FIFO_PTN_HIT_D1 0x324 +#define PCIE_CLIENT_DBG_FIFO_TRN_HIT_D0 0x328 +#define PCIE_CLIENT_DBG_FIFO_TRN_HIT_D1 0x32c +#define PCIE_CLIENT_DBG_FIFO_STATUS 0x350 +#define PCIE_CLIENT_DBG_TRANSITION_DATA 0xffff0000 +#define PCIE_CLIENT_DBF_EN 0xffff0003 +#define PCIE_LTSSM_ENABLE_ENHANCE BIT(4) + +struct reset_bulk_data { + const char *id; + struct reset_control *rst; +}; + +struct rockchip_pcie { + struct dw_pcie *pci; + void __iomem *dbi_base; + void __iomem *apb_base; + struct phy *phy; + struct clk_bulk_data *clks; + unsigned int clk_cnt; + struct reset_bulk_data *rsts; + struct gpio_desc *rst_gpio; + struct pcie_port pp; + struct regulator *vpcie3v3; +}; + +static inline int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, + u32 reg) +{ + return readl(rockchip->apb_base + reg); +} + +static inline void rockchip_pcie_writel_apb(struct rockchip_pcie *rockchip, + u32 reg, u32 val) +{ + writel(val, rockchip->apb_base + reg); +} + +static inline void rockchip_pcie_set_mode(struct rockchip_pcie *rockchip) +{ + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_GENERAL_CONTROL, + PCIE_CLIENT_RC_MODE); +} + +static inline void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip) +{ + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_GENERAL_CONTROL, + PCIE_CLIENT_ENABLE_LTSSM); +} + +static int rockchip_pcie_link_up(struct dw_pcie *pci) +{ + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); + u32 val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS); + + if ((val & (PCIE_RDLH_LINKUP | PCIE_SMLH_LINKUP)) == 0x30000 && + (val & GENMASK(5, 0)) == PCIE_L0S_ENTRY) + return 1; + + return 0; +} + +static void rockchip_pcie_establish_link(struct dw_pcie *pci) +{ + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); + + if (dw_pcie_link_up(pci)) { + dev_err(pci->dev, "link already up\n"); + return; + } + + /* Reset device */ + gpiod_set_value_cansleep(rockchip->rst_gpio, 0); + msleep(100); + gpiod_set_value_cansleep(rockchip->rst_gpio, 1); + + rockchip_pcie_enable_ltssm(rockchip); +} + +static void rockchip_pcie_enable_debug(struct rockchip_pcie *rockchip) +{ + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_PTN_HIT_D0, + PCIE_CLIENT_DBG_TRANSITION_DATA); + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_PTN_HIT_D1, + PCIE_CLIENT_DBG_TRANSITION_DATA); + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_TRN_HIT_D0, + PCIE_CLIENT_DBG_TRANSITION_DATA); + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_TRN_HIT_D1, + PCIE_CLIENT_DBG_TRANSITION_DATA); + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_MODE_CON, + PCIE_CLIENT_DBF_EN); +} + +static void rockchip_pcie_debug_dump(struct rockchip_pcie *rockchip) +{ + u32 loop; + struct dw_pcie *pci = rockchip->pci; + + dev_dbg(pci->dev, "ltssm = 0x%x\n", + rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS)); + for (loop = 0; loop < 64; loop++) + dev_dbg(pci->dev, "fifo_status = 0x%x\n", + rockchip_pcie_readl_apb(rockchip, + PCIE_CLIENT_DBG_FIFO_STATUS)); +} + +static int rockchip_pcie_host_init(struct pcie_port *pp) +{ + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); + + dw_pcie_setup_rc(pp); + + rockchip_pcie_enable_debug(rockchip); + + rockchip_pcie_establish_link(pci); + dw_pcie_wait_for_link(pci); + + rockchip_pcie_debug_dump(rockchip); + + return 0; +} + +static const struct dw_pcie_host_ops rockchip_pcie_host_ops = { + .host_init = rockchip_pcie_host_init, +}; + +static int rockchip_add_pcie_port(struct rockchip_pcie *rockchip) +{ + int ret; + struct dw_pcie *pci = rockchip->pci; + struct pcie_port *pp = &pci->pp; + struct device *dev = pci->dev; + + pp->ops = &rockchip_pcie_host_ops; + + if (device_property_read_bool(dev, "msi-map")) + pp->msi_ext = 1; + + rockchip_pcie_set_mode(rockchip); + + ret = dw_pcie_host_init(pp); + if (ret) + return ret; + + return 0; +} + +static void rockchip_pcie_clk_deinit(struct rockchip_pcie *rockchip) +{ + clk_bulk_disable(rockchip->clk_cnt, rockchip->clks); + clk_bulk_unprepare(rockchip->clk_cnt, rockchip->clks); +} + +static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip) +{ + struct device *dev = rockchip->pci->dev; + struct property *prop; + const char *name; + int i = 0, ret, count; + + count = of_property_count_strings(dev->of_node, "clock-names"); + if (count < 1) + return -ENODEV; + + rockchip->clks = devm_kcalloc(dev, count, + sizeof(struct clk_bulk_data), + GFP_KERNEL); + if (!rockchip->clks) + return -ENOMEM; + + rockchip->clk_cnt = count; + + of_property_for_each_string(dev->of_node, "clock-names", + prop, name) { + rockchip->clks[i].id = name; + if (!rockchip->clks[i].id) + return -ENOMEM; + i++; + } + + ret = devm_clk_bulk_get(dev, count, rockchip->clks); + if (ret) + return ret; + + ret = clk_bulk_prepare(count, rockchip->clks); + if (ret) + return ret; + + ret = clk_bulk_enable(count, rockchip->clks); + if (ret) { + clk_bulk_unprepare(count, rockchip->clks); + return ret; + } + + return 0; +} + +static int rockchip_pcie_resource_get(struct platform_device *pdev, + struct rockchip_pcie *rockchip) +{ + struct resource *dbi_base; + struct resource *apb_base; + + dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, + "pcie-dbi"); + if (!dbi_base) + return -ENODEV; + + rockchip->dbi_base = devm_ioremap_resource(&pdev->dev, dbi_base); + if (IS_ERR(rockchip->dbi_base)) + return PTR_ERR(rockchip->dbi_base); + + rockchip->pci->dbi_base = rockchip->dbi_base; + + apb_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, + "pcie-apb"); + if (!apb_base) + return -ENODEV; + + rockchip->apb_base = devm_ioremap_resource(&pdev->dev, apb_base); + if (IS_ERR(rockchip->apb_base)) + return PTR_ERR(rockchip->apb_base); + + rockchip->rst_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", + GPIOD_OUT_HIGH); + if (IS_ERR(rockchip->rst_gpio)) + return PTR_ERR(rockchip->rst_gpio); + + return 0; +} + +static int rockchip_pcie_phy_init(struct rockchip_pcie *rockchip) +{ + int ret; + struct device *dev = rockchip->pci->dev; + + rockchip->phy = devm_phy_get(dev, "pcie-phy"); + if (IS_ERR(rockchip->phy)) { + if (PTR_ERR(rockchip->phy) != -EPROBE_DEFER) + dev_info(dev, "missing phy\n"); + return PTR_ERR(rockchip->phy); + } + + ret = phy_init(rockchip->phy); + if (ret < 0) + return ret; + + phy_power_on(rockchip->phy); + + return 0; +} + +static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip) +{ + phy_exit(rockchip->phy); + phy_power_off(rockchip->phy); +} + +static int rockchip_pcie_reset_control_release(struct rockchip_pcie *rockchip) +{ + struct device *dev = rockchip->pci->dev; + struct property *prop; + const char *name; + int ret, count, i = 0; + + count = of_property_count_strings(dev->of_node, "reset-names"); + if (count < 1) + return -ENODEV; + + rockchip->rsts = devm_kcalloc(dev, count, + sizeof(struct reset_bulk_data), + GFP_KERNEL); + if (!rockchip->rsts) + return -ENOMEM; + + of_property_for_each_string(dev->of_node, "reset-names", + prop, name) { + rockchip->rsts[i].id = name; + if (!rockchip->rsts[i].id) + return -ENOMEM; + i++; + } + + for (i = 0; i < count; i++) { + rockchip->rsts[i].rst = devm_reset_control_get_exclusive(dev, + rockchip->rsts[i].id); + if (IS_ERR(rockchip->rsts[i].rst)) { + dev_err(dev, "failed to get %s\n", + rockchip->clks[i].id); + return PTR_ERR(rockchip->rsts[i].rst); + } + } + + for (i = 0; i < count; i++) { + ret = reset_control_deassert(rockchip->rsts[i].rst); + if (ret) { + dev_err(dev, "failed to release %s\n", + rockchip->rsts[i].id); + return ret; + } + } + + return 0; +} + +static void rockchip_pcie_fast_link_setup(struct rockchip_pcie *rockchip) +{ + u32 val; + + /* LTSSM EN ctrl mode */ + val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_HOT_RESET_CTRL); + val |= PCIE_LTSSM_ENABLE_ENHANCE | (PCIE_LTSSM_ENABLE_ENHANCE << 16); + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_HOT_RESET_CTRL, val); +} + + +static const struct of_device_id rockchip_pcie_of_match[] = { + { .compatible = "rockchip,rk3568-pcie", }, + { /* sentinel */ }, +}; + +static const struct dw_pcie_ops dw_pcie_ops = { + .link_up = rockchip_pcie_link_up, +}; + +static int rockchip_pcie_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct rockchip_pcie *rockchip; + struct dw_pcie *pci; + int ret; + + rockchip = devm_kzalloc(dev, sizeof(*rockchip), GFP_KERNEL); + if (!rockchip) + return -ENOMEM; + + pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); + if (!pci) + return -ENOMEM; + + pci->dev = dev; + pci->ops = &dw_pcie_ops; + + rockchip->pci = pci; + + ret = rockchip_pcie_resource_get(pdev, rockchip); + if (ret) + return ret; + + /* DON'T MOVE ME: must be enable before phy init */ + rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3"); + if (IS_ERR(rockchip->vpcie3v3)) { + if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV) + return PTR_ERR(rockchip->vpcie3v3); + dev_info(dev, "no vpcie3v3 regulator found\n"); + } + + if (!IS_ERR(rockchip->vpcie3v3)) { + ret = regulator_enable(rockchip->vpcie3v3); + if (ret) { + dev_err(pci->dev, "fail to enable vpcie3v3 regulator\n"); + return ret; + } + } + + ret = rockchip_pcie_phy_init(rockchip); + if (ret) + goto disable_regulator; + + ret = rockchip_pcie_reset_control_release(rockchip); + if (ret) + goto deinit_phy; + + ret = rockchip_pcie_clk_init(rockchip); + if (ret) + goto deinit_phy; + + rockchip_pcie_fast_link_setup(rockchip); + + platform_set_drvdata(pdev, rockchip); + + ret = rockchip_add_pcie_port(rockchip); + if (ret) + goto deinit_clk; + + return 0; + +deinit_clk: + rockchip_pcie_clk_deinit(rockchip); +deinit_phy: + rockchip_pcie_phy_deinit(rockchip); +disable_regulator: + if (!IS_ERR(rockchip->vpcie3v3)) + regulator_disable(rockchip->vpcie3v3); + + return ret; +} + +MODULE_DEVICE_TABLE(of, rockchip_pcie_of_match); + +static struct platform_driver rockchip_pcie_driver = { + .driver = { + .name = "rk-pcie", + .of_match_table = rockchip_pcie_of_match, + .suppress_bind_attrs = true, + }, + .probe = rockchip_pcie_probe, +}; + +builtin_platform_driver(rockchip_pcie_driver); -- 2.25.1 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 3/3] PCI: rockchip: add DesignWare based PCIe controller 2021-01-18 9:17 ` Simon Xue @ 2021-01-19 20:28 ` Robin Murphy -1 siblings, 0 replies; 32+ messages in thread From: Robin Murphy @ 2021-01-19 20:28 UTC (permalink / raw) To: Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, Shawn Lin, linux-rockchip On 2021-01-18 09:17, Simon Xue wrote: > pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host > is another IP which is only used for RK3399. So all the following > non-RK3399 SoCs should use this driver. > > Signed-off-by: Simon Xue <xxm@rock-chips.com> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > drivers/pci/controller/Kconfig | 4 +- > drivers/pci/controller/dwc/Kconfig | 9 + > drivers/pci/controller/dwc/Makefile | 1 + > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 454 ++++++++++++++++++ > 4 files changed, 466 insertions(+), 2 deletions(-) > create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig > index 64e2f5e379aa..48a7d62f97f3 100644 > --- a/drivers/pci/controller/Kconfig > +++ b/drivers/pci/controller/Kconfig > @@ -219,7 +219,7 @@ config PCIE_ROCKCHIP_HOST > help > Say Y here if you want internal PCI support on Rockchip SoC. > There is 1 internal PCIe port available to support GEN2 with > - 4 slots. > + 4 slots. Only for RK3399. > > config PCIE_ROCKCHIP_EP > bool "Rockchip PCIe endpoint controller" > @@ -231,7 +231,7 @@ config PCIE_ROCKCHIP_EP > help > Say Y here if you want to support Rockchip PCIe controller in > endpoint mode on Rockchip SoC. There is 1 internal PCIe port > - available to support GEN2 with 4 slots. > + available to support GEN2 with 4 slots. Only for RK3399. > > config PCIE_MEDIATEK > tristate "MediaTek PCIe controller" > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig > index 22c5529e9a65..110733d0c241 100644 > --- a/drivers/pci/controller/dwc/Kconfig > +++ b/drivers/pci/controller/dwc/Kconfig > @@ -214,6 +214,15 @@ config PCIE_ARTPEC6_EP > Enables support for the PCIe controller in the ARTPEC-6 SoC to work in > endpoint mode. This uses the DesignWare core. > > +config PCIE_DW_ROCKCHIP_HOST > + bool "Rockchip DesignWare PCIe controller" > + select PCIE_DW > + select PCIE_DW_HOST > + depends on ARCH_ROCKCHIP > + depends on OF > + help > + Enables support for the DW PCIe controller in the Rockchip SoC. > + > config PCIE_INTEL_GW > bool "Intel Gateway PCIe host controller support" > depends on OF && (X86 || COMPILE_TEST) > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile > index a751553fa0db..9c7048d2be17 100644 > --- a/drivers/pci/controller/dwc/Makefile > +++ b/drivers/pci/controller/dwc/Makefile > @@ -13,6 +13,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o > obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o > obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o > obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o > +obj-$(CONFIG_PCIE_DW_ROCKCHIP_HOST) += pcie-dw-rockchip.o > obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o > obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o > obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > new file mode 100644 > index 000000000000..5997fbc675d3 > --- /dev/null > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > @@ -0,0 +1,454 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * PCIe host controller driver for Rockchip SoCs > + * > + * Copyright (C) 2021 Rockchip Electronics Co., Ltd. > + * http://www.rock-chips.com > + * > + * Author: Simon Xue <xxm@rock-chips.com> > + */ > + > +#include <linux/clk.h> > +#include <linux/mfd/syscon.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/of_gpio.h> I believe you're meant to include linux/gpio/consumer.h rather than any other GPIO-related header. > +#include <linux/phy/phy.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/reset.h> > + > +#include "pcie-designware.h" > + > +/* > + * The upper 16 bits of PCIE_CLIENT_CONFIG are a write > + * mask for the lower 16 bits. This allows atomic updates > + * of the register without locking. > + */ > +#define HIWORD_UPDATE(mask, val) (((mask) << 16) | (val)) > +#define HIWORD_UPDATE_BIT(val) HIWORD_UPDATE(val, val) > + > +#define to_rockchip_pcie(x) dev_get_drvdata((x)->dev) > + > +#define PCIE_CLIENT_RC_MODE HIWORD_UPDATE_BIT(0x40) > +#define PCIE_CLIENT_ENABLE_LTSSM HIWORD_UPDATE_BIT(0xc) > +#define PCIE_SMLH_LINKUP BIT(16) > +#define PCIE_RDLH_LINKUP BIT(17) > +#define PCIE_L0S_ENTRY 0x11 > +#define PCIE_CLIENT_GENERAL_CONTROL 0x0 > +#define PCIE_CLIENT_GENERAL_DEBUG 0x104 > +#define PCIE_CLIENT_HOT_RESET_CTRL 0x180 > +#define PCIE_CLIENT_LTSSM_STATUS 0x300 > +#define PCIE_CLIENT_DBG_FIFO_MODE_CON 0x310 > +#define PCIE_CLIENT_DBG_FIFO_PTN_HIT_D0 0x320 > +#define PCIE_CLIENT_DBG_FIFO_PTN_HIT_D1 0x324 > +#define PCIE_CLIENT_DBG_FIFO_TRN_HIT_D0 0x328 > +#define PCIE_CLIENT_DBG_FIFO_TRN_HIT_D1 0x32c > +#define PCIE_CLIENT_DBG_FIFO_STATUS 0x350 > +#define PCIE_CLIENT_DBG_TRANSITION_DATA 0xffff0000 > +#define PCIE_CLIENT_DBF_EN 0xffff0003 > +#define PCIE_LTSSM_ENABLE_ENHANCE BIT(4) > + > +struct reset_bulk_data { > + const char *id; > + struct reset_control *rst; > +}; > + > +struct rockchip_pcie { > + struct dw_pcie *pci; > + void __iomem *dbi_base; > + void __iomem *apb_base; > + struct phy *phy; > + struct clk_bulk_data *clks; > + unsigned int clk_cnt; > + struct reset_bulk_data *rsts; > + struct gpio_desc *rst_gpio; > + struct pcie_port pp; > + struct regulator *vpcie3v3; > +}; > + > +static inline int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, > + u32 reg) > +{ > + return readl(rockchip->apb_base + reg); > +} > + > +static inline void rockchip_pcie_writel_apb(struct rockchip_pcie *rockchip, > + u32 reg, u32 val) > +{ > + writel(val, rockchip->apb_base + reg); > +} > + > +static inline void rockchip_pcie_set_mode(struct rockchip_pcie *rockchip) > +{ > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_GENERAL_CONTROL, > + PCIE_CLIENT_RC_MODE); > +} > + > +static inline void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip) > +{ > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_GENERAL_CONTROL, > + PCIE_CLIENT_ENABLE_LTSSM); > +} > + > +static int rockchip_pcie_link_up(struct dw_pcie *pci) > +{ > + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > + u32 val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS); > + > + if ((val & (PCIE_RDLH_LINKUP | PCIE_SMLH_LINKUP)) == 0x30000 && > + (val & GENMASK(5, 0)) == PCIE_L0S_ENTRY) > + return 1; > + > + return 0; > +} > + > +static void rockchip_pcie_establish_link(struct dw_pcie *pci) > +{ > + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > + > + if (dw_pcie_link_up(pci)) { > + dev_err(pci->dev, "link already up\n"); > + return; > + } > + > + /* Reset device */ > + gpiod_set_value_cansleep(rockchip->rst_gpio, 0); > + msleep(100); > + gpiod_set_value_cansleep(rockchip->rst_gpio, 1); > + > + rockchip_pcie_enable_ltssm(rockchip); > +} > + > +static void rockchip_pcie_enable_debug(struct rockchip_pcie *rockchip) > +{ > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_PTN_HIT_D0, > + PCIE_CLIENT_DBG_TRANSITION_DATA); > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_PTN_HIT_D1, > + PCIE_CLIENT_DBG_TRANSITION_DATA); > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_TRN_HIT_D0, > + PCIE_CLIENT_DBG_TRANSITION_DATA); > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_TRN_HIT_D1, > + PCIE_CLIENT_DBG_TRANSITION_DATA); > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_MODE_CON, > + PCIE_CLIENT_DBF_EN); > +} > + > +static void rockchip_pcie_debug_dump(struct rockchip_pcie *rockchip) > +{ > + u32 loop; > + struct dw_pcie *pci = rockchip->pci; > + > + dev_dbg(pci->dev, "ltssm = 0x%x\n", > + rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS)); > + for (loop = 0; loop < 64; loop++) > + dev_dbg(pci->dev, "fifo_status = 0x%x\n", > + rockchip_pcie_readl_apb(rockchip, > + PCIE_CLIENT_DBG_FIFO_STATUS)); > +} > + > +static int rockchip_pcie_host_init(struct pcie_port *pp) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > + > + dw_pcie_setup_rc(pp); > + > + rockchip_pcie_enable_debug(rockchip); > + > + rockchip_pcie_establish_link(pci); > + dw_pcie_wait_for_link(pci); > + > + rockchip_pcie_debug_dump(rockchip); > + > + return 0; > +} > + > +static const struct dw_pcie_host_ops rockchip_pcie_host_ops = { > + .host_init = rockchip_pcie_host_init, > +}; > + > +static int rockchip_add_pcie_port(struct rockchip_pcie *rockchip) > +{ > + int ret; > + struct dw_pcie *pci = rockchip->pci; > + struct pcie_port *pp = &pci->pp; > + struct device *dev = pci->dev; > + > + pp->ops = &rockchip_pcie_host_ops; > + > + if (device_property_read_bool(dev, "msi-map")) > + pp->msi_ext = 1; As per patch #1, I don't think this is necessary. > + > + rockchip_pcie_set_mode(rockchip); > + > + ret = dw_pcie_host_init(pp); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static void rockchip_pcie_clk_deinit(struct rockchip_pcie *rockchip) > +{ > + clk_bulk_disable(rockchip->clk_cnt, rockchip->clks); > + clk_bulk_unprepare(rockchip->clk_cnt, rockchip->clks); There's a clk_bulk_disable_unprepare() helper, you know ;) > +} > + > +static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip) > +{ > + struct device *dev = rockchip->pci->dev; > + struct property *prop; > + const char *name; > + int i = 0, ret, count; > + > + count = of_property_count_strings(dev->of_node, "clock-names"); > + if (count < 1) > + return -ENODEV; > + > + rockchip->clks = devm_kcalloc(dev, count, > + sizeof(struct clk_bulk_data), > + GFP_KERNEL); > + if (!rockchip->clks) > + return -ENOMEM; > + > + rockchip->clk_cnt = count; > + > + of_property_for_each_string(dev->of_node, "clock-names", > + prop, name) { > + rockchip->clks[i].id = name; > + if (!rockchip->clks[i].id) > + return -ENOMEM; > + i++; > + } of_clk_bulk_get_all() has existed for a while; no need to open-code it. > + > + ret = devm_clk_bulk_get(dev, count, rockchip->clks); > + if (ret) > + return ret; > + > + ret = clk_bulk_prepare(count, rockchip->clks); > + if (ret) > + return ret; > + > + ret = clk_bulk_enable(count, rockchip->clks); > + if (ret) { > + clk_bulk_unprepare(count, rockchip->clks); > + return ret; > + } Similarly, clk_bulk_prepare_enable() also exists. > + return 0; > +} > + > +static int rockchip_pcie_resource_get(struct platform_device *pdev, > + struct rockchip_pcie *rockchip) > +{ > + struct resource *dbi_base; > + struct resource *apb_base; > + > + dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, > + "pcie-dbi"); > + if (!dbi_base) > + return -ENODEV; > + > + rockchip->dbi_base = devm_ioremap_resource(&pdev->dev, dbi_base); > + if (IS_ERR(rockchip->dbi_base)) > + return PTR_ERR(rockchip->dbi_base); It still has a ridiculously impractical name that I detest, but if I don't mention devm_platform_ioremap_resource_byname() I'm sure somebody else would come along and suggest it. > + rockchip->pci->dbi_base = rockchip->dbi_base; > + > + apb_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, > + "pcie-apb"); > + if (!apb_base) > + return -ENODEV; > + > + rockchip->apb_base = devm_ioremap_resource(&pdev->dev, apb_base); > + if (IS_ERR(rockchip->apb_base)) > + return PTR_ERR(rockchip->apb_base); > + > + rockchip->rst_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", > + GPIOD_OUT_HIGH); > + if (IS_ERR(rockchip->rst_gpio)) > + return PTR_ERR(rockchip->rst_gpio); > + > + return 0; > +} > + > +static int rockchip_pcie_phy_init(struct rockchip_pcie *rockchip) > +{ > + int ret; > + struct device *dev = rockchip->pci->dev; > + > + rockchip->phy = devm_phy_get(dev, "pcie-phy"); > + if (IS_ERR(rockchip->phy)) { > + if (PTR_ERR(rockchip->phy) != -EPROBE_DEFER) > + dev_info(dev, "missing phy\n"); > + return PTR_ERR(rockchip->phy); Use dev_err_probe() for this pattern now. > + } > + > + ret = phy_init(rockchip->phy); > + if (ret < 0) > + return ret; > + > + phy_power_on(rockchip->phy); > + > + return 0; > +} > + > +static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip) > +{ > + phy_exit(rockchip->phy); > + phy_power_off(rockchip->phy); > +} > + > +static int rockchip_pcie_reset_control_release(struct rockchip_pcie *rockchip) > +{ > + struct device *dev = rockchip->pci->dev; > + struct property *prop; > + const char *name; > + int ret, count, i = 0; > + > + count = of_property_count_strings(dev->of_node, "reset-names"); > + if (count < 1) > + return -ENODEV; > + > + rockchip->rsts = devm_kcalloc(dev, count, > + sizeof(struct reset_bulk_data), > + GFP_KERNEL); > + if (!rockchip->rsts) > + return -ENOMEM; > + > + of_property_for_each_string(dev->of_node, "reset-names", > + prop, name) { > + rockchip->rsts[i].id = name; > + if (!rockchip->rsts[i].id) > + return -ENOMEM; > + i++; > + } > + > + for (i = 0; i < count; i++) { > + rockchip->rsts[i].rst = devm_reset_control_get_exclusive(dev, > + rockchip->rsts[i].id); > + if (IS_ERR(rockchip->rsts[i].rst)) { > + dev_err(dev, "failed to get %s\n", > + rockchip->clks[i].id); > + return PTR_ERR(rockchip->rsts[i].rst); > + } > + } > + > + for (i = 0; i < count; i++) { > + ret = reset_control_deassert(rockchip->rsts[i].rst); > + if (ret) { > + dev_err(dev, "failed to release %s\n", > + rockchip->rsts[i].id); > + return ret; > + } > + } Does the hardware require that the resets are deasserted in the specific order of "whatever happens to be in the DT", or could you do this much more simply with the reset_control_array APIs? Robin. > + > + return 0; > +} > + > +static void rockchip_pcie_fast_link_setup(struct rockchip_pcie *rockchip) > +{ > + u32 val; > + > + /* LTSSM EN ctrl mode */ > + val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_HOT_RESET_CTRL); > + val |= PCIE_LTSSM_ENABLE_ENHANCE | (PCIE_LTSSM_ENABLE_ENHANCE << 16); > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_HOT_RESET_CTRL, val); > +} > + > + > +static const struct of_device_id rockchip_pcie_of_match[] = { > + { .compatible = "rockchip,rk3568-pcie", }, > + { /* sentinel */ }, > +}; > + > +static const struct dw_pcie_ops dw_pcie_ops = { > + .link_up = rockchip_pcie_link_up, > +}; > + > +static int rockchip_pcie_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct rockchip_pcie *rockchip; > + struct dw_pcie *pci; > + int ret; > + > + rockchip = devm_kzalloc(dev, sizeof(*rockchip), GFP_KERNEL); > + if (!rockchip) > + return -ENOMEM; > + > + pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > + if (!pci) > + return -ENOMEM; > + > + pci->dev = dev; > + pci->ops = &dw_pcie_ops; > + > + rockchip->pci = pci; > + > + ret = rockchip_pcie_resource_get(pdev, rockchip); > + if (ret) > + return ret; > + > + /* DON'T MOVE ME: must be enable before phy init */ > + rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3"); > + if (IS_ERR(rockchip->vpcie3v3)) { > + if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV) > + return PTR_ERR(rockchip->vpcie3v3); > + dev_info(dev, "no vpcie3v3 regulator found\n"); > + } > + > + if (!IS_ERR(rockchip->vpcie3v3)) { > + ret = regulator_enable(rockchip->vpcie3v3); > + if (ret) { > + dev_err(pci->dev, "fail to enable vpcie3v3 regulator\n"); > + return ret; > + } > + } > + > + ret = rockchip_pcie_phy_init(rockchip); > + if (ret) > + goto disable_regulator; > + > + ret = rockchip_pcie_reset_control_release(rockchip); > + if (ret) > + goto deinit_phy; > + > + ret = rockchip_pcie_clk_init(rockchip); > + if (ret) > + goto deinit_phy; > + > + rockchip_pcie_fast_link_setup(rockchip); > + > + platform_set_drvdata(pdev, rockchip); > + > + ret = rockchip_add_pcie_port(rockchip); > + if (ret) > + goto deinit_clk; > + > + return 0; > + > +deinit_clk: > + rockchip_pcie_clk_deinit(rockchip); > +deinit_phy: > + rockchip_pcie_phy_deinit(rockchip); > +disable_regulator: > + if (!IS_ERR(rockchip->vpcie3v3)) > + regulator_disable(rockchip->vpcie3v3); > + > + return ret; > +} > + > +MODULE_DEVICE_TABLE(of, rockchip_pcie_of_match); > + > +static struct platform_driver rockchip_pcie_driver = { > + .driver = { > + .name = "rk-pcie", > + .of_match_table = rockchip_pcie_of_match, > + .suppress_bind_attrs = true, > + }, > + .probe = rockchip_pcie_probe, > +}; > + > +builtin_platform_driver(rockchip_pcie_driver); > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/3] PCI: rockchip: add DesignWare based PCIe controller @ 2021-01-19 20:28 ` Robin Murphy 0 siblings, 0 replies; 32+ messages in thread From: Robin Murphy @ 2021-01-19 20:28 UTC (permalink / raw) To: Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, Shawn Lin, linux-rockchip On 2021-01-18 09:17, Simon Xue wrote: > pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host > is another IP which is only used for RK3399. So all the following > non-RK3399 SoCs should use this driver. > > Signed-off-by: Simon Xue <xxm@rock-chips.com> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > drivers/pci/controller/Kconfig | 4 +- > drivers/pci/controller/dwc/Kconfig | 9 + > drivers/pci/controller/dwc/Makefile | 1 + > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 454 ++++++++++++++++++ > 4 files changed, 466 insertions(+), 2 deletions(-) > create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig > index 64e2f5e379aa..48a7d62f97f3 100644 > --- a/drivers/pci/controller/Kconfig > +++ b/drivers/pci/controller/Kconfig > @@ -219,7 +219,7 @@ config PCIE_ROCKCHIP_HOST > help > Say Y here if you want internal PCI support on Rockchip SoC. > There is 1 internal PCIe port available to support GEN2 with > - 4 slots. > + 4 slots. Only for RK3399. > > config PCIE_ROCKCHIP_EP > bool "Rockchip PCIe endpoint controller" > @@ -231,7 +231,7 @@ config PCIE_ROCKCHIP_EP > help > Say Y here if you want to support Rockchip PCIe controller in > endpoint mode on Rockchip SoC. There is 1 internal PCIe port > - available to support GEN2 with 4 slots. > + available to support GEN2 with 4 slots. Only for RK3399. > > config PCIE_MEDIATEK > tristate "MediaTek PCIe controller" > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig > index 22c5529e9a65..110733d0c241 100644 > --- a/drivers/pci/controller/dwc/Kconfig > +++ b/drivers/pci/controller/dwc/Kconfig > @@ -214,6 +214,15 @@ config PCIE_ARTPEC6_EP > Enables support for the PCIe controller in the ARTPEC-6 SoC to work in > endpoint mode. This uses the DesignWare core. > > +config PCIE_DW_ROCKCHIP_HOST > + bool "Rockchip DesignWare PCIe controller" > + select PCIE_DW > + select PCIE_DW_HOST > + depends on ARCH_ROCKCHIP > + depends on OF > + help > + Enables support for the DW PCIe controller in the Rockchip SoC. > + > config PCIE_INTEL_GW > bool "Intel Gateway PCIe host controller support" > depends on OF && (X86 || COMPILE_TEST) > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile > index a751553fa0db..9c7048d2be17 100644 > --- a/drivers/pci/controller/dwc/Makefile > +++ b/drivers/pci/controller/dwc/Makefile > @@ -13,6 +13,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o > obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o > obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o > obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o > +obj-$(CONFIG_PCIE_DW_ROCKCHIP_HOST) += pcie-dw-rockchip.o > obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o > obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o > obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > new file mode 100644 > index 000000000000..5997fbc675d3 > --- /dev/null > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > @@ -0,0 +1,454 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * PCIe host controller driver for Rockchip SoCs > + * > + * Copyright (C) 2021 Rockchip Electronics Co., Ltd. > + * http://www.rock-chips.com > + * > + * Author: Simon Xue <xxm@rock-chips.com> > + */ > + > +#include <linux/clk.h> > +#include <linux/mfd/syscon.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/of_gpio.h> I believe you're meant to include linux/gpio/consumer.h rather than any other GPIO-related header. > +#include <linux/phy/phy.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/reset.h> > + > +#include "pcie-designware.h" > + > +/* > + * The upper 16 bits of PCIE_CLIENT_CONFIG are a write > + * mask for the lower 16 bits. This allows atomic updates > + * of the register without locking. > + */ > +#define HIWORD_UPDATE(mask, val) (((mask) << 16) | (val)) > +#define HIWORD_UPDATE_BIT(val) HIWORD_UPDATE(val, val) > + > +#define to_rockchip_pcie(x) dev_get_drvdata((x)->dev) > + > +#define PCIE_CLIENT_RC_MODE HIWORD_UPDATE_BIT(0x40) > +#define PCIE_CLIENT_ENABLE_LTSSM HIWORD_UPDATE_BIT(0xc) > +#define PCIE_SMLH_LINKUP BIT(16) > +#define PCIE_RDLH_LINKUP BIT(17) > +#define PCIE_L0S_ENTRY 0x11 > +#define PCIE_CLIENT_GENERAL_CONTROL 0x0 > +#define PCIE_CLIENT_GENERAL_DEBUG 0x104 > +#define PCIE_CLIENT_HOT_RESET_CTRL 0x180 > +#define PCIE_CLIENT_LTSSM_STATUS 0x300 > +#define PCIE_CLIENT_DBG_FIFO_MODE_CON 0x310 > +#define PCIE_CLIENT_DBG_FIFO_PTN_HIT_D0 0x320 > +#define PCIE_CLIENT_DBG_FIFO_PTN_HIT_D1 0x324 > +#define PCIE_CLIENT_DBG_FIFO_TRN_HIT_D0 0x328 > +#define PCIE_CLIENT_DBG_FIFO_TRN_HIT_D1 0x32c > +#define PCIE_CLIENT_DBG_FIFO_STATUS 0x350 > +#define PCIE_CLIENT_DBG_TRANSITION_DATA 0xffff0000 > +#define PCIE_CLIENT_DBF_EN 0xffff0003 > +#define PCIE_LTSSM_ENABLE_ENHANCE BIT(4) > + > +struct reset_bulk_data { > + const char *id; > + struct reset_control *rst; > +}; > + > +struct rockchip_pcie { > + struct dw_pcie *pci; > + void __iomem *dbi_base; > + void __iomem *apb_base; > + struct phy *phy; > + struct clk_bulk_data *clks; > + unsigned int clk_cnt; > + struct reset_bulk_data *rsts; > + struct gpio_desc *rst_gpio; > + struct pcie_port pp; > + struct regulator *vpcie3v3; > +}; > + > +static inline int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, > + u32 reg) > +{ > + return readl(rockchip->apb_base + reg); > +} > + > +static inline void rockchip_pcie_writel_apb(struct rockchip_pcie *rockchip, > + u32 reg, u32 val) > +{ > + writel(val, rockchip->apb_base + reg); > +} > + > +static inline void rockchip_pcie_set_mode(struct rockchip_pcie *rockchip) > +{ > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_GENERAL_CONTROL, > + PCIE_CLIENT_RC_MODE); > +} > + > +static inline void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip) > +{ > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_GENERAL_CONTROL, > + PCIE_CLIENT_ENABLE_LTSSM); > +} > + > +static int rockchip_pcie_link_up(struct dw_pcie *pci) > +{ > + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > + u32 val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS); > + > + if ((val & (PCIE_RDLH_LINKUP | PCIE_SMLH_LINKUP)) == 0x30000 && > + (val & GENMASK(5, 0)) == PCIE_L0S_ENTRY) > + return 1; > + > + return 0; > +} > + > +static void rockchip_pcie_establish_link(struct dw_pcie *pci) > +{ > + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > + > + if (dw_pcie_link_up(pci)) { > + dev_err(pci->dev, "link already up\n"); > + return; > + } > + > + /* Reset device */ > + gpiod_set_value_cansleep(rockchip->rst_gpio, 0); > + msleep(100); > + gpiod_set_value_cansleep(rockchip->rst_gpio, 1); > + > + rockchip_pcie_enable_ltssm(rockchip); > +} > + > +static void rockchip_pcie_enable_debug(struct rockchip_pcie *rockchip) > +{ > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_PTN_HIT_D0, > + PCIE_CLIENT_DBG_TRANSITION_DATA); > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_PTN_HIT_D1, > + PCIE_CLIENT_DBG_TRANSITION_DATA); > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_TRN_HIT_D0, > + PCIE_CLIENT_DBG_TRANSITION_DATA); > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_TRN_HIT_D1, > + PCIE_CLIENT_DBG_TRANSITION_DATA); > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_MODE_CON, > + PCIE_CLIENT_DBF_EN); > +} > + > +static void rockchip_pcie_debug_dump(struct rockchip_pcie *rockchip) > +{ > + u32 loop; > + struct dw_pcie *pci = rockchip->pci; > + > + dev_dbg(pci->dev, "ltssm = 0x%x\n", > + rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS)); > + for (loop = 0; loop < 64; loop++) > + dev_dbg(pci->dev, "fifo_status = 0x%x\n", > + rockchip_pcie_readl_apb(rockchip, > + PCIE_CLIENT_DBG_FIFO_STATUS)); > +} > + > +static int rockchip_pcie_host_init(struct pcie_port *pp) > +{ > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > + > + dw_pcie_setup_rc(pp); > + > + rockchip_pcie_enable_debug(rockchip); > + > + rockchip_pcie_establish_link(pci); > + dw_pcie_wait_for_link(pci); > + > + rockchip_pcie_debug_dump(rockchip); > + > + return 0; > +} > + > +static const struct dw_pcie_host_ops rockchip_pcie_host_ops = { > + .host_init = rockchip_pcie_host_init, > +}; > + > +static int rockchip_add_pcie_port(struct rockchip_pcie *rockchip) > +{ > + int ret; > + struct dw_pcie *pci = rockchip->pci; > + struct pcie_port *pp = &pci->pp; > + struct device *dev = pci->dev; > + > + pp->ops = &rockchip_pcie_host_ops; > + > + if (device_property_read_bool(dev, "msi-map")) > + pp->msi_ext = 1; As per patch #1, I don't think this is necessary. > + > + rockchip_pcie_set_mode(rockchip); > + > + ret = dw_pcie_host_init(pp); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static void rockchip_pcie_clk_deinit(struct rockchip_pcie *rockchip) > +{ > + clk_bulk_disable(rockchip->clk_cnt, rockchip->clks); > + clk_bulk_unprepare(rockchip->clk_cnt, rockchip->clks); There's a clk_bulk_disable_unprepare() helper, you know ;) > +} > + > +static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip) > +{ > + struct device *dev = rockchip->pci->dev; > + struct property *prop; > + const char *name; > + int i = 0, ret, count; > + > + count = of_property_count_strings(dev->of_node, "clock-names"); > + if (count < 1) > + return -ENODEV; > + > + rockchip->clks = devm_kcalloc(dev, count, > + sizeof(struct clk_bulk_data), > + GFP_KERNEL); > + if (!rockchip->clks) > + return -ENOMEM; > + > + rockchip->clk_cnt = count; > + > + of_property_for_each_string(dev->of_node, "clock-names", > + prop, name) { > + rockchip->clks[i].id = name; > + if (!rockchip->clks[i].id) > + return -ENOMEM; > + i++; > + } of_clk_bulk_get_all() has existed for a while; no need to open-code it. > + > + ret = devm_clk_bulk_get(dev, count, rockchip->clks); > + if (ret) > + return ret; > + > + ret = clk_bulk_prepare(count, rockchip->clks); > + if (ret) > + return ret; > + > + ret = clk_bulk_enable(count, rockchip->clks); > + if (ret) { > + clk_bulk_unprepare(count, rockchip->clks); > + return ret; > + } Similarly, clk_bulk_prepare_enable() also exists. > + return 0; > +} > + > +static int rockchip_pcie_resource_get(struct platform_device *pdev, > + struct rockchip_pcie *rockchip) > +{ > + struct resource *dbi_base; > + struct resource *apb_base; > + > + dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, > + "pcie-dbi"); > + if (!dbi_base) > + return -ENODEV; > + > + rockchip->dbi_base = devm_ioremap_resource(&pdev->dev, dbi_base); > + if (IS_ERR(rockchip->dbi_base)) > + return PTR_ERR(rockchip->dbi_base); It still has a ridiculously impractical name that I detest, but if I don't mention devm_platform_ioremap_resource_byname() I'm sure somebody else would come along and suggest it. > + rockchip->pci->dbi_base = rockchip->dbi_base; > + > + apb_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, > + "pcie-apb"); > + if (!apb_base) > + return -ENODEV; > + > + rockchip->apb_base = devm_ioremap_resource(&pdev->dev, apb_base); > + if (IS_ERR(rockchip->apb_base)) > + return PTR_ERR(rockchip->apb_base); > + > + rockchip->rst_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", > + GPIOD_OUT_HIGH); > + if (IS_ERR(rockchip->rst_gpio)) > + return PTR_ERR(rockchip->rst_gpio); > + > + return 0; > +} > + > +static int rockchip_pcie_phy_init(struct rockchip_pcie *rockchip) > +{ > + int ret; > + struct device *dev = rockchip->pci->dev; > + > + rockchip->phy = devm_phy_get(dev, "pcie-phy"); > + if (IS_ERR(rockchip->phy)) { > + if (PTR_ERR(rockchip->phy) != -EPROBE_DEFER) > + dev_info(dev, "missing phy\n"); > + return PTR_ERR(rockchip->phy); Use dev_err_probe() for this pattern now. > + } > + > + ret = phy_init(rockchip->phy); > + if (ret < 0) > + return ret; > + > + phy_power_on(rockchip->phy); > + > + return 0; > +} > + > +static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip) > +{ > + phy_exit(rockchip->phy); > + phy_power_off(rockchip->phy); > +} > + > +static int rockchip_pcie_reset_control_release(struct rockchip_pcie *rockchip) > +{ > + struct device *dev = rockchip->pci->dev; > + struct property *prop; > + const char *name; > + int ret, count, i = 0; > + > + count = of_property_count_strings(dev->of_node, "reset-names"); > + if (count < 1) > + return -ENODEV; > + > + rockchip->rsts = devm_kcalloc(dev, count, > + sizeof(struct reset_bulk_data), > + GFP_KERNEL); > + if (!rockchip->rsts) > + return -ENOMEM; > + > + of_property_for_each_string(dev->of_node, "reset-names", > + prop, name) { > + rockchip->rsts[i].id = name; > + if (!rockchip->rsts[i].id) > + return -ENOMEM; > + i++; > + } > + > + for (i = 0; i < count; i++) { > + rockchip->rsts[i].rst = devm_reset_control_get_exclusive(dev, > + rockchip->rsts[i].id); > + if (IS_ERR(rockchip->rsts[i].rst)) { > + dev_err(dev, "failed to get %s\n", > + rockchip->clks[i].id); > + return PTR_ERR(rockchip->rsts[i].rst); > + } > + } > + > + for (i = 0; i < count; i++) { > + ret = reset_control_deassert(rockchip->rsts[i].rst); > + if (ret) { > + dev_err(dev, "failed to release %s\n", > + rockchip->rsts[i].id); > + return ret; > + } > + } Does the hardware require that the resets are deasserted in the specific order of "whatever happens to be in the DT", or could you do this much more simply with the reset_control_array APIs? Robin. > + > + return 0; > +} > + > +static void rockchip_pcie_fast_link_setup(struct rockchip_pcie *rockchip) > +{ > + u32 val; > + > + /* LTSSM EN ctrl mode */ > + val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_HOT_RESET_CTRL); > + val |= PCIE_LTSSM_ENABLE_ENHANCE | (PCIE_LTSSM_ENABLE_ENHANCE << 16); > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_HOT_RESET_CTRL, val); > +} > + > + > +static const struct of_device_id rockchip_pcie_of_match[] = { > + { .compatible = "rockchip,rk3568-pcie", }, > + { /* sentinel */ }, > +}; > + > +static const struct dw_pcie_ops dw_pcie_ops = { > + .link_up = rockchip_pcie_link_up, > +}; > + > +static int rockchip_pcie_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct rockchip_pcie *rockchip; > + struct dw_pcie *pci; > + int ret; > + > + rockchip = devm_kzalloc(dev, sizeof(*rockchip), GFP_KERNEL); > + if (!rockchip) > + return -ENOMEM; > + > + pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > + if (!pci) > + return -ENOMEM; > + > + pci->dev = dev; > + pci->ops = &dw_pcie_ops; > + > + rockchip->pci = pci; > + > + ret = rockchip_pcie_resource_get(pdev, rockchip); > + if (ret) > + return ret; > + > + /* DON'T MOVE ME: must be enable before phy init */ > + rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3"); > + if (IS_ERR(rockchip->vpcie3v3)) { > + if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV) > + return PTR_ERR(rockchip->vpcie3v3); > + dev_info(dev, "no vpcie3v3 regulator found\n"); > + } > + > + if (!IS_ERR(rockchip->vpcie3v3)) { > + ret = regulator_enable(rockchip->vpcie3v3); > + if (ret) { > + dev_err(pci->dev, "fail to enable vpcie3v3 regulator\n"); > + return ret; > + } > + } > + > + ret = rockchip_pcie_phy_init(rockchip); > + if (ret) > + goto disable_regulator; > + > + ret = rockchip_pcie_reset_control_release(rockchip); > + if (ret) > + goto deinit_phy; > + > + ret = rockchip_pcie_clk_init(rockchip); > + if (ret) > + goto deinit_phy; > + > + rockchip_pcie_fast_link_setup(rockchip); > + > + platform_set_drvdata(pdev, rockchip); > + > + ret = rockchip_add_pcie_port(rockchip); > + if (ret) > + goto deinit_clk; > + > + return 0; > + > +deinit_clk: > + rockchip_pcie_clk_deinit(rockchip); > +deinit_phy: > + rockchip_pcie_phy_deinit(rockchip); > +disable_regulator: > + if (!IS_ERR(rockchip->vpcie3v3)) > + regulator_disable(rockchip->vpcie3v3); > + > + return ret; > +} > + > +MODULE_DEVICE_TABLE(of, rockchip_pcie_of_match); > + > +static struct platform_driver rockchip_pcie_driver = { > + .driver = { > + .name = "rk-pcie", > + .of_match_table = rockchip_pcie_of_match, > + .suppress_bind_attrs = true, > + }, > + .probe = rockchip_pcie_probe, > +}; > + > +builtin_platform_driver(rockchip_pcie_driver); > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/3] PCI: rockchip: add DesignWare based PCIe controller 2021-01-19 20:28 ` Robin Murphy @ 2021-01-20 1:58 ` xxm -1 siblings, 0 replies; 32+ messages in thread From: xxm @ 2021-01-20 1:58 UTC (permalink / raw) To: Robin Murphy, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, Shawn Lin, linux-rockchip Hi Robin, Thanks for your review, all you mentioned will be fixed in next patch set Simon Xue. 在 2021/1/20 4:28, Robin Murphy 写道: > On 2021-01-18 09:17, Simon Xue wrote: >> pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host >> is another IP which is only used for RK3399. So all the following >> non-RK3399 SoCs should use this driver. >> >> Signed-off-by: Simon Xue <xxm@rock-chips.com> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> drivers/pci/controller/Kconfig | 4 +- >> drivers/pci/controller/dwc/Kconfig | 9 + >> drivers/pci/controller/dwc/Makefile | 1 + >> drivers/pci/controller/dwc/pcie-dw-rockchip.c | 454 ++++++++++++++++++ >> 4 files changed, 466 insertions(+), 2 deletions(-) >> create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c >> >> diff --git a/drivers/pci/controller/Kconfig >> b/drivers/pci/controller/Kconfig >> index 64e2f5e379aa..48a7d62f97f3 100644 >> --- a/drivers/pci/controller/Kconfig >> +++ b/drivers/pci/controller/Kconfig >> @@ -219,7 +219,7 @@ config PCIE_ROCKCHIP_HOST >> help >> Say Y here if you want internal PCI support on Rockchip SoC. >> There is 1 internal PCIe port available to support GEN2 with >> - 4 slots. >> + 4 slots. Only for RK3399. >> config PCIE_ROCKCHIP_EP >> bool "Rockchip PCIe endpoint controller" >> @@ -231,7 +231,7 @@ config PCIE_ROCKCHIP_EP >> help >> Say Y here if you want to support Rockchip PCIe controller in >> endpoint mode on Rockchip SoC. There is 1 internal PCIe port >> - available to support GEN2 with 4 slots. >> + available to support GEN2 with 4 slots. Only for RK3399. >> config PCIE_MEDIATEK >> tristate "MediaTek PCIe controller" >> diff --git a/drivers/pci/controller/dwc/Kconfig >> b/drivers/pci/controller/dwc/Kconfig >> index 22c5529e9a65..110733d0c241 100644 >> --- a/drivers/pci/controller/dwc/Kconfig >> +++ b/drivers/pci/controller/dwc/Kconfig >> @@ -214,6 +214,15 @@ config PCIE_ARTPEC6_EP >> Enables support for the PCIe controller in the ARTPEC-6 SoC >> to work in >> endpoint mode. This uses the DesignWare core. >> +config PCIE_DW_ROCKCHIP_HOST >> + bool "Rockchip DesignWare PCIe controller" >> + select PCIE_DW >> + select PCIE_DW_HOST >> + depends on ARCH_ROCKCHIP >> + depends on OF >> + help >> + Enables support for the DW PCIe controller in the Rockchip SoC. >> + >> config PCIE_INTEL_GW >> bool "Intel Gateway PCIe host controller support" >> depends on OF && (X86 || COMPILE_TEST) >> diff --git a/drivers/pci/controller/dwc/Makefile >> b/drivers/pci/controller/dwc/Makefile >> index a751553fa0db..9c7048d2be17 100644 >> --- a/drivers/pci/controller/dwc/Makefile >> +++ b/drivers/pci/controller/dwc/Makefile >> @@ -13,6 +13,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o >> obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o >> obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o >> obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o >> +obj-$(CONFIG_PCIE_DW_ROCKCHIP_HOST) += pcie-dw-rockchip.o >> obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o >> obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o >> obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o >> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c >> b/drivers/pci/controller/dwc/pcie-dw-rockchip.c >> new file mode 100644 >> index 000000000000..5997fbc675d3 >> --- /dev/null >> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c >> @@ -0,0 +1,454 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * PCIe host controller driver for Rockchip SoCs >> + * >> + * Copyright (C) 2021 Rockchip Electronics Co., Ltd. >> + * http://www.rock-chips.com >> + * >> + * Author: Simon Xue <xxm@rock-chips.com> >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/mfd/syscon.h> >> +#include <linux/module.h> >> +#include <linux/of_device.h> >> +#include <linux/of_gpio.h> > > I believe you're meant to include linux/gpio/consumer.h rather than > any other GPIO-related header. > >> +#include <linux/phy/phy.h> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> >> +#include <linux/reset.h> >> + >> +#include "pcie-designware.h" >> + >> +/* >> + * The upper 16 bits of PCIE_CLIENT_CONFIG are a write >> + * mask for the lower 16 bits. This allows atomic updates >> + * of the register without locking. >> + */ >> +#define HIWORD_UPDATE(mask, val) (((mask) << 16) | (val)) >> +#define HIWORD_UPDATE_BIT(val) HIWORD_UPDATE(val, val) >> + >> +#define to_rockchip_pcie(x) dev_get_drvdata((x)->dev) >> + >> +#define PCIE_CLIENT_RC_MODE HIWORD_UPDATE_BIT(0x40) >> +#define PCIE_CLIENT_ENABLE_LTSSM HIWORD_UPDATE_BIT(0xc) >> +#define PCIE_SMLH_LINKUP BIT(16) >> +#define PCIE_RDLH_LINKUP BIT(17) >> +#define PCIE_L0S_ENTRY 0x11 >> +#define PCIE_CLIENT_GENERAL_CONTROL 0x0 >> +#define PCIE_CLIENT_GENERAL_DEBUG 0x104 >> +#define PCIE_CLIENT_HOT_RESET_CTRL 0x180 >> +#define PCIE_CLIENT_LTSSM_STATUS 0x300 >> +#define PCIE_CLIENT_DBG_FIFO_MODE_CON 0x310 >> +#define PCIE_CLIENT_DBG_FIFO_PTN_HIT_D0 0x320 >> +#define PCIE_CLIENT_DBG_FIFO_PTN_HIT_D1 0x324 >> +#define PCIE_CLIENT_DBG_FIFO_TRN_HIT_D0 0x328 >> +#define PCIE_CLIENT_DBG_FIFO_TRN_HIT_D1 0x32c >> +#define PCIE_CLIENT_DBG_FIFO_STATUS 0x350 >> +#define PCIE_CLIENT_DBG_TRANSITION_DATA 0xffff0000 >> +#define PCIE_CLIENT_DBF_EN 0xffff0003 >> +#define PCIE_LTSSM_ENABLE_ENHANCE BIT(4) >> + >> +struct reset_bulk_data { >> + const char *id; >> + struct reset_control *rst; >> +}; >> + >> +struct rockchip_pcie { >> + struct dw_pcie *pci; >> + void __iomem *dbi_base; >> + void __iomem *apb_base; >> + struct phy *phy; >> + struct clk_bulk_data *clks; >> + unsigned int clk_cnt; >> + struct reset_bulk_data *rsts; >> + struct gpio_desc *rst_gpio; >> + struct pcie_port pp; >> + struct regulator *vpcie3v3; >> +}; >> + >> +static inline int rockchip_pcie_readl_apb(struct rockchip_pcie >> *rockchip, >> + u32 reg) >> +{ >> + return readl(rockchip->apb_base + reg); >> +} >> + >> +static inline void rockchip_pcie_writel_apb(struct rockchip_pcie >> *rockchip, >> + u32 reg, u32 val) >> +{ >> + writel(val, rockchip->apb_base + reg); >> +} >> + >> +static inline void rockchip_pcie_set_mode(struct rockchip_pcie >> *rockchip) >> +{ >> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_GENERAL_CONTROL, >> + PCIE_CLIENT_RC_MODE); >> +} >> + >> +static inline void rockchip_pcie_enable_ltssm(struct rockchip_pcie >> *rockchip) >> +{ >> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_GENERAL_CONTROL, >> + PCIE_CLIENT_ENABLE_LTSSM); >> +} >> + >> +static int rockchip_pcie_link_up(struct dw_pcie *pci) >> +{ >> + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); >> + u32 val = rockchip_pcie_readl_apb(rockchip, >> PCIE_CLIENT_LTSSM_STATUS); >> + >> + if ((val & (PCIE_RDLH_LINKUP | PCIE_SMLH_LINKUP)) == 0x30000 && >> + (val & GENMASK(5, 0)) == PCIE_L0S_ENTRY) >> + return 1; >> + >> + return 0; >> +} >> + >> +static void rockchip_pcie_establish_link(struct dw_pcie *pci) >> +{ >> + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); >> + >> + if (dw_pcie_link_up(pci)) { >> + dev_err(pci->dev, "link already up\n"); >> + return; >> + } >> + >> + /* Reset device */ >> + gpiod_set_value_cansleep(rockchip->rst_gpio, 0); >> + msleep(100); >> + gpiod_set_value_cansleep(rockchip->rst_gpio, 1); >> + >> + rockchip_pcie_enable_ltssm(rockchip); >> +} >> + >> +static void rockchip_pcie_enable_debug(struct rockchip_pcie *rockchip) >> +{ >> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_PTN_HIT_D0, >> + PCIE_CLIENT_DBG_TRANSITION_DATA); >> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_PTN_HIT_D1, >> + PCIE_CLIENT_DBG_TRANSITION_DATA); >> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_TRN_HIT_D0, >> + PCIE_CLIENT_DBG_TRANSITION_DATA); >> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_TRN_HIT_D1, >> + PCIE_CLIENT_DBG_TRANSITION_DATA); >> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_MODE_CON, >> + PCIE_CLIENT_DBF_EN); >> +} >> + >> +static void rockchip_pcie_debug_dump(struct rockchip_pcie *rockchip) >> +{ >> + u32 loop; >> + struct dw_pcie *pci = rockchip->pci; >> + >> + dev_dbg(pci->dev, "ltssm = 0x%x\n", >> + rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS)); >> + for (loop = 0; loop < 64; loop++) >> + dev_dbg(pci->dev, "fifo_status = 0x%x\n", >> + rockchip_pcie_readl_apb(rockchip, >> + PCIE_CLIENT_DBG_FIFO_STATUS)); >> +} >> + >> +static int rockchip_pcie_host_init(struct pcie_port *pp) >> +{ >> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >> + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); >> + >> + dw_pcie_setup_rc(pp); >> + >> + rockchip_pcie_enable_debug(rockchip); >> + >> + rockchip_pcie_establish_link(pci); >> + dw_pcie_wait_for_link(pci); >> + >> + rockchip_pcie_debug_dump(rockchip); >> + >> + return 0; >> +} >> + >> +static const struct dw_pcie_host_ops rockchip_pcie_host_ops = { >> + .host_init = rockchip_pcie_host_init, >> +}; >> + >> +static int rockchip_add_pcie_port(struct rockchip_pcie *rockchip) >> +{ >> + int ret; >> + struct dw_pcie *pci = rockchip->pci; >> + struct pcie_port *pp = &pci->pp; >> + struct device *dev = pci->dev; >> + >> + pp->ops = &rockchip_pcie_host_ops; >> + >> + if (device_property_read_bool(dev, "msi-map")) >> + pp->msi_ext = 1; > > As per patch #1, I don't think this is necessary. > >> + >> + rockchip_pcie_set_mode(rockchip); >> + >> + ret = dw_pcie_host_init(pp); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> + >> +static void rockchip_pcie_clk_deinit(struct rockchip_pcie *rockchip) >> +{ >> + clk_bulk_disable(rockchip->clk_cnt, rockchip->clks); >> + clk_bulk_unprepare(rockchip->clk_cnt, rockchip->clks); > > There's a clk_bulk_disable_unprepare() helper, you know ;) > >> +} >> + >> +static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip) >> +{ >> + struct device *dev = rockchip->pci->dev; >> + struct property *prop; >> + const char *name; >> + int i = 0, ret, count; >> + >> + count = of_property_count_strings(dev->of_node, "clock-names"); >> + if (count < 1) >> + return -ENODEV; >> + >> + rockchip->clks = devm_kcalloc(dev, count, >> + sizeof(struct clk_bulk_data), >> + GFP_KERNEL); >> + if (!rockchip->clks) >> + return -ENOMEM; >> + >> + rockchip->clk_cnt = count; >> + >> + of_property_for_each_string(dev->of_node, "clock-names", >> + prop, name) { >> + rockchip->clks[i].id = name; >> + if (!rockchip->clks[i].id) >> + return -ENOMEM; >> + i++; >> + } > > of_clk_bulk_get_all() has existed for a while; no need to open-code it. > >> + >> + ret = devm_clk_bulk_get(dev, count, rockchip->clks); >> + if (ret) >> + return ret; >> + >> + ret = clk_bulk_prepare(count, rockchip->clks); >> + if (ret) >> + return ret; >> + >> + ret = clk_bulk_enable(count, rockchip->clks); >> + if (ret) { >> + clk_bulk_unprepare(count, rockchip->clks); >> + return ret; >> + } > > Similarly, clk_bulk_prepare_enable() also exists. > >> + return 0; >> +} >> + >> +static int rockchip_pcie_resource_get(struct platform_device *pdev, >> + struct rockchip_pcie *rockchip) >> +{ >> + struct resource *dbi_base; >> + struct resource *apb_base; >> + >> + dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, >> + "pcie-dbi"); >> + if (!dbi_base) >> + return -ENODEV; >> + >> + rockchip->dbi_base = devm_ioremap_resource(&pdev->dev, dbi_base); >> + if (IS_ERR(rockchip->dbi_base)) >> + return PTR_ERR(rockchip->dbi_base); > > It still has a ridiculously impractical name that I detest, but if I > don't mention devm_platform_ioremap_resource_byname() I'm sure > somebody else would come along and suggest it. > >> + rockchip->pci->dbi_base = rockchip->dbi_base; >> + >> + apb_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, >> + "pcie-apb"); >> + if (!apb_base) >> + return -ENODEV; >> + >> + rockchip->apb_base = devm_ioremap_resource(&pdev->dev, apb_base); >> + if (IS_ERR(rockchip->apb_base)) >> + return PTR_ERR(rockchip->apb_base); >> + >> + rockchip->rst_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", >> + GPIOD_OUT_HIGH); >> + if (IS_ERR(rockchip->rst_gpio)) >> + return PTR_ERR(rockchip->rst_gpio); >> + >> + return 0; >> +} >> + >> +static int rockchip_pcie_phy_init(struct rockchip_pcie *rockchip) >> +{ >> + int ret; >> + struct device *dev = rockchip->pci->dev; >> + >> + rockchip->phy = devm_phy_get(dev, "pcie-phy"); >> + if (IS_ERR(rockchip->phy)) { >> + if (PTR_ERR(rockchip->phy) != -EPROBE_DEFER) >> + dev_info(dev, "missing phy\n"); >> + return PTR_ERR(rockchip->phy); > > Use dev_err_probe() for this pattern now. > >> + } >> + >> + ret = phy_init(rockchip->phy); >> + if (ret < 0) >> + return ret; >> + >> + phy_power_on(rockchip->phy); >> + >> + return 0; >> +} >> + >> +static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip) >> +{ >> + phy_exit(rockchip->phy); >> + phy_power_off(rockchip->phy); >> +} >> + >> +static int rockchip_pcie_reset_control_release(struct rockchip_pcie >> *rockchip) >> +{ >> + struct device *dev = rockchip->pci->dev; >> + struct property *prop; >> + const char *name; >> + int ret, count, i = 0; >> + >> + count = of_property_count_strings(dev->of_node, "reset-names"); >> + if (count < 1) >> + return -ENODEV; >> + >> + rockchip->rsts = devm_kcalloc(dev, count, >> + sizeof(struct reset_bulk_data), >> + GFP_KERNEL); >> + if (!rockchip->rsts) >> + return -ENOMEM; >> + >> + of_property_for_each_string(dev->of_node, "reset-names", >> + prop, name) { >> + rockchip->rsts[i].id = name; >> + if (!rockchip->rsts[i].id) >> + return -ENOMEM; >> + i++; >> + } >> + >> + for (i = 0; i < count; i++) { >> + rockchip->rsts[i].rst = devm_reset_control_get_exclusive(dev, >> + rockchip->rsts[i].id); >> + if (IS_ERR(rockchip->rsts[i].rst)) { >> + dev_err(dev, "failed to get %s\n", >> + rockchip->clks[i].id); >> + return PTR_ERR(rockchip->rsts[i].rst); >> + } >> + } >> + >> + for (i = 0; i < count; i++) { >> + ret = reset_control_deassert(rockchip->rsts[i].rst); >> + if (ret) { >> + dev_err(dev, "failed to release %s\n", >> + rockchip->rsts[i].id); >> + return ret; >> + } >> + } > > Does the hardware require that the resets are deasserted in the > specific order of "whatever happens to be in the DT", or could you do > this much more simply with the reset_control_array APIs? > > Robin. > >> + >> + return 0; >> +} >> + >> +static void rockchip_pcie_fast_link_setup(struct rockchip_pcie >> *rockchip) >> +{ >> + u32 val; >> + >> + /* LTSSM EN ctrl mode */ >> + val = rockchip_pcie_readl_apb(rockchip, >> PCIE_CLIENT_HOT_RESET_CTRL); >> + val |= PCIE_LTSSM_ENABLE_ENHANCE | (PCIE_LTSSM_ENABLE_ENHANCE << >> 16); >> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_HOT_RESET_CTRL, >> val); >> +} >> + >> + >> +static const struct of_device_id rockchip_pcie_of_match[] = { >> + { .compatible = "rockchip,rk3568-pcie", }, >> + { /* sentinel */ }, >> +}; >> + >> +static const struct dw_pcie_ops dw_pcie_ops = { >> + .link_up = rockchip_pcie_link_up, >> +}; >> + >> +static int rockchip_pcie_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct rockchip_pcie *rockchip; >> + struct dw_pcie *pci; >> + int ret; >> + >> + rockchip = devm_kzalloc(dev, sizeof(*rockchip), GFP_KERNEL); >> + if (!rockchip) >> + return -ENOMEM; >> + >> + pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); >> + if (!pci) >> + return -ENOMEM; >> + >> + pci->dev = dev; >> + pci->ops = &dw_pcie_ops; >> + >> + rockchip->pci = pci; >> + >> + ret = rockchip_pcie_resource_get(pdev, rockchip); >> + if (ret) >> + return ret; >> + >> + /* DON'T MOVE ME: must be enable before phy init */ >> + rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3"); >> + if (IS_ERR(rockchip->vpcie3v3)) { >> + if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV) >> + return PTR_ERR(rockchip->vpcie3v3); >> + dev_info(dev, "no vpcie3v3 regulator found\n"); >> + } >> + >> + if (!IS_ERR(rockchip->vpcie3v3)) { >> + ret = regulator_enable(rockchip->vpcie3v3); >> + if (ret) { >> + dev_err(pci->dev, "fail to enable vpcie3v3 regulator\n"); >> + return ret; >> + } >> + } >> + >> + ret = rockchip_pcie_phy_init(rockchip); >> + if (ret) >> + goto disable_regulator; >> + >> + ret = rockchip_pcie_reset_control_release(rockchip); >> + if (ret) >> + goto deinit_phy; >> + >> + ret = rockchip_pcie_clk_init(rockchip); >> + if (ret) >> + goto deinit_phy; >> + >> + rockchip_pcie_fast_link_setup(rockchip); >> + >> + platform_set_drvdata(pdev, rockchip); >> + >> + ret = rockchip_add_pcie_port(rockchip); >> + if (ret) >> + goto deinit_clk; >> + >> + return 0; >> + >> +deinit_clk: >> + rockchip_pcie_clk_deinit(rockchip); >> +deinit_phy: >> + rockchip_pcie_phy_deinit(rockchip); >> +disable_regulator: >> + if (!IS_ERR(rockchip->vpcie3v3)) >> + regulator_disable(rockchip->vpcie3v3); >> + >> + return ret; >> +} >> + >> +MODULE_DEVICE_TABLE(of, rockchip_pcie_of_match); >> + >> +static struct platform_driver rockchip_pcie_driver = { >> + .driver = { >> + .name = "rk-pcie", >> + .of_match_table = rockchip_pcie_of_match, >> + .suppress_bind_attrs = true, >> + }, >> + .probe = rockchip_pcie_probe, >> +}; >> + >> +builtin_platform_driver(rockchip_pcie_driver); >> > > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/3] PCI: rockchip: add DesignWare based PCIe controller @ 2021-01-20 1:58 ` xxm 0 siblings, 0 replies; 32+ messages in thread From: xxm @ 2021-01-20 1:58 UTC (permalink / raw) To: Robin Murphy, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, Shawn Lin, linux-rockchip Hi Robin, Thanks for your review, all you mentioned will be fixed in next patch set Simon Xue. 在 2021/1/20 4:28, Robin Murphy 写道: > On 2021-01-18 09:17, Simon Xue wrote: >> pcie-dw-rockchip is based on DWC IP. But pcie-rockchip-host >> is another IP which is only used for RK3399. So all the following >> non-RK3399 SoCs should use this driver. >> >> Signed-off-by: Simon Xue <xxm@rock-chips.com> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> drivers/pci/controller/Kconfig | 4 +- >> drivers/pci/controller/dwc/Kconfig | 9 + >> drivers/pci/controller/dwc/Makefile | 1 + >> drivers/pci/controller/dwc/pcie-dw-rockchip.c | 454 ++++++++++++++++++ >> 4 files changed, 466 insertions(+), 2 deletions(-) >> create mode 100644 drivers/pci/controller/dwc/pcie-dw-rockchip.c >> >> diff --git a/drivers/pci/controller/Kconfig >> b/drivers/pci/controller/Kconfig >> index 64e2f5e379aa..48a7d62f97f3 100644 >> --- a/drivers/pci/controller/Kconfig >> +++ b/drivers/pci/controller/Kconfig >> @@ -219,7 +219,7 @@ config PCIE_ROCKCHIP_HOST >> help >> Say Y here if you want internal PCI support on Rockchip SoC. >> There is 1 internal PCIe port available to support GEN2 with >> - 4 slots. >> + 4 slots. Only for RK3399. >> config PCIE_ROCKCHIP_EP >> bool "Rockchip PCIe endpoint controller" >> @@ -231,7 +231,7 @@ config PCIE_ROCKCHIP_EP >> help >> Say Y here if you want to support Rockchip PCIe controller in >> endpoint mode on Rockchip SoC. There is 1 internal PCIe port >> - available to support GEN2 with 4 slots. >> + available to support GEN2 with 4 slots. Only for RK3399. >> config PCIE_MEDIATEK >> tristate "MediaTek PCIe controller" >> diff --git a/drivers/pci/controller/dwc/Kconfig >> b/drivers/pci/controller/dwc/Kconfig >> index 22c5529e9a65..110733d0c241 100644 >> --- a/drivers/pci/controller/dwc/Kconfig >> +++ b/drivers/pci/controller/dwc/Kconfig >> @@ -214,6 +214,15 @@ config PCIE_ARTPEC6_EP >> Enables support for the PCIe controller in the ARTPEC-6 SoC >> to work in >> endpoint mode. This uses the DesignWare core. >> +config PCIE_DW_ROCKCHIP_HOST >> + bool "Rockchip DesignWare PCIe controller" >> + select PCIE_DW >> + select PCIE_DW_HOST >> + depends on ARCH_ROCKCHIP >> + depends on OF >> + help >> + Enables support for the DW PCIe controller in the Rockchip SoC. >> + >> config PCIE_INTEL_GW >> bool "Intel Gateway PCIe host controller support" >> depends on OF && (X86 || COMPILE_TEST) >> diff --git a/drivers/pci/controller/dwc/Makefile >> b/drivers/pci/controller/dwc/Makefile >> index a751553fa0db..9c7048d2be17 100644 >> --- a/drivers/pci/controller/dwc/Makefile >> +++ b/drivers/pci/controller/dwc/Makefile >> @@ -13,6 +13,7 @@ obj-$(CONFIG_PCI_LAYERSCAPE_EP) += pci-layerscape-ep.o >> obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o >> obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o >> obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o >> +obj-$(CONFIG_PCIE_DW_ROCKCHIP_HOST) += pcie-dw-rockchip.o >> obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o >> obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o >> obj-$(CONFIG_PCIE_HISI_STB) += pcie-histb.o >> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c >> b/drivers/pci/controller/dwc/pcie-dw-rockchip.c >> new file mode 100644 >> index 000000000000..5997fbc675d3 >> --- /dev/null >> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c >> @@ -0,0 +1,454 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * PCIe host controller driver for Rockchip SoCs >> + * >> + * Copyright (C) 2021 Rockchip Electronics Co., Ltd. >> + * http://www.rock-chips.com >> + * >> + * Author: Simon Xue <xxm@rock-chips.com> >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/mfd/syscon.h> >> +#include <linux/module.h> >> +#include <linux/of_device.h> >> +#include <linux/of_gpio.h> > > I believe you're meant to include linux/gpio/consumer.h rather than > any other GPIO-related header. > >> +#include <linux/phy/phy.h> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> >> +#include <linux/reset.h> >> + >> +#include "pcie-designware.h" >> + >> +/* >> + * The upper 16 bits of PCIE_CLIENT_CONFIG are a write >> + * mask for the lower 16 bits. This allows atomic updates >> + * of the register without locking. >> + */ >> +#define HIWORD_UPDATE(mask, val) (((mask) << 16) | (val)) >> +#define HIWORD_UPDATE_BIT(val) HIWORD_UPDATE(val, val) >> + >> +#define to_rockchip_pcie(x) dev_get_drvdata((x)->dev) >> + >> +#define PCIE_CLIENT_RC_MODE HIWORD_UPDATE_BIT(0x40) >> +#define PCIE_CLIENT_ENABLE_LTSSM HIWORD_UPDATE_BIT(0xc) >> +#define PCIE_SMLH_LINKUP BIT(16) >> +#define PCIE_RDLH_LINKUP BIT(17) >> +#define PCIE_L0S_ENTRY 0x11 >> +#define PCIE_CLIENT_GENERAL_CONTROL 0x0 >> +#define PCIE_CLIENT_GENERAL_DEBUG 0x104 >> +#define PCIE_CLIENT_HOT_RESET_CTRL 0x180 >> +#define PCIE_CLIENT_LTSSM_STATUS 0x300 >> +#define PCIE_CLIENT_DBG_FIFO_MODE_CON 0x310 >> +#define PCIE_CLIENT_DBG_FIFO_PTN_HIT_D0 0x320 >> +#define PCIE_CLIENT_DBG_FIFO_PTN_HIT_D1 0x324 >> +#define PCIE_CLIENT_DBG_FIFO_TRN_HIT_D0 0x328 >> +#define PCIE_CLIENT_DBG_FIFO_TRN_HIT_D1 0x32c >> +#define PCIE_CLIENT_DBG_FIFO_STATUS 0x350 >> +#define PCIE_CLIENT_DBG_TRANSITION_DATA 0xffff0000 >> +#define PCIE_CLIENT_DBF_EN 0xffff0003 >> +#define PCIE_LTSSM_ENABLE_ENHANCE BIT(4) >> + >> +struct reset_bulk_data { >> + const char *id; >> + struct reset_control *rst; >> +}; >> + >> +struct rockchip_pcie { >> + struct dw_pcie *pci; >> + void __iomem *dbi_base; >> + void __iomem *apb_base; >> + struct phy *phy; >> + struct clk_bulk_data *clks; >> + unsigned int clk_cnt; >> + struct reset_bulk_data *rsts; >> + struct gpio_desc *rst_gpio; >> + struct pcie_port pp; >> + struct regulator *vpcie3v3; >> +}; >> + >> +static inline int rockchip_pcie_readl_apb(struct rockchip_pcie >> *rockchip, >> + u32 reg) >> +{ >> + return readl(rockchip->apb_base + reg); >> +} >> + >> +static inline void rockchip_pcie_writel_apb(struct rockchip_pcie >> *rockchip, >> + u32 reg, u32 val) >> +{ >> + writel(val, rockchip->apb_base + reg); >> +} >> + >> +static inline void rockchip_pcie_set_mode(struct rockchip_pcie >> *rockchip) >> +{ >> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_GENERAL_CONTROL, >> + PCIE_CLIENT_RC_MODE); >> +} >> + >> +static inline void rockchip_pcie_enable_ltssm(struct rockchip_pcie >> *rockchip) >> +{ >> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_GENERAL_CONTROL, >> + PCIE_CLIENT_ENABLE_LTSSM); >> +} >> + >> +static int rockchip_pcie_link_up(struct dw_pcie *pci) >> +{ >> + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); >> + u32 val = rockchip_pcie_readl_apb(rockchip, >> PCIE_CLIENT_LTSSM_STATUS); >> + >> + if ((val & (PCIE_RDLH_LINKUP | PCIE_SMLH_LINKUP)) == 0x30000 && >> + (val & GENMASK(5, 0)) == PCIE_L0S_ENTRY) >> + return 1; >> + >> + return 0; >> +} >> + >> +static void rockchip_pcie_establish_link(struct dw_pcie *pci) >> +{ >> + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); >> + >> + if (dw_pcie_link_up(pci)) { >> + dev_err(pci->dev, "link already up\n"); >> + return; >> + } >> + >> + /* Reset device */ >> + gpiod_set_value_cansleep(rockchip->rst_gpio, 0); >> + msleep(100); >> + gpiod_set_value_cansleep(rockchip->rst_gpio, 1); >> + >> + rockchip_pcie_enable_ltssm(rockchip); >> +} >> + >> +static void rockchip_pcie_enable_debug(struct rockchip_pcie *rockchip) >> +{ >> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_PTN_HIT_D0, >> + PCIE_CLIENT_DBG_TRANSITION_DATA); >> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_PTN_HIT_D1, >> + PCIE_CLIENT_DBG_TRANSITION_DATA); >> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_TRN_HIT_D0, >> + PCIE_CLIENT_DBG_TRANSITION_DATA); >> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_TRN_HIT_D1, >> + PCIE_CLIENT_DBG_TRANSITION_DATA); >> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DBG_FIFO_MODE_CON, >> + PCIE_CLIENT_DBF_EN); >> +} >> + >> +static void rockchip_pcie_debug_dump(struct rockchip_pcie *rockchip) >> +{ >> + u32 loop; >> + struct dw_pcie *pci = rockchip->pci; >> + >> + dev_dbg(pci->dev, "ltssm = 0x%x\n", >> + rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS)); >> + for (loop = 0; loop < 64; loop++) >> + dev_dbg(pci->dev, "fifo_status = 0x%x\n", >> + rockchip_pcie_readl_apb(rockchip, >> + PCIE_CLIENT_DBG_FIFO_STATUS)); >> +} >> + >> +static int rockchip_pcie_host_init(struct pcie_port *pp) >> +{ >> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >> + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); >> + >> + dw_pcie_setup_rc(pp); >> + >> + rockchip_pcie_enable_debug(rockchip); >> + >> + rockchip_pcie_establish_link(pci); >> + dw_pcie_wait_for_link(pci); >> + >> + rockchip_pcie_debug_dump(rockchip); >> + >> + return 0; >> +} >> + >> +static const struct dw_pcie_host_ops rockchip_pcie_host_ops = { >> + .host_init = rockchip_pcie_host_init, >> +}; >> + >> +static int rockchip_add_pcie_port(struct rockchip_pcie *rockchip) >> +{ >> + int ret; >> + struct dw_pcie *pci = rockchip->pci; >> + struct pcie_port *pp = &pci->pp; >> + struct device *dev = pci->dev; >> + >> + pp->ops = &rockchip_pcie_host_ops; >> + >> + if (device_property_read_bool(dev, "msi-map")) >> + pp->msi_ext = 1; > > As per patch #1, I don't think this is necessary. > >> + >> + rockchip_pcie_set_mode(rockchip); >> + >> + ret = dw_pcie_host_init(pp); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> + >> +static void rockchip_pcie_clk_deinit(struct rockchip_pcie *rockchip) >> +{ >> + clk_bulk_disable(rockchip->clk_cnt, rockchip->clks); >> + clk_bulk_unprepare(rockchip->clk_cnt, rockchip->clks); > > There's a clk_bulk_disable_unprepare() helper, you know ;) > >> +} >> + >> +static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip) >> +{ >> + struct device *dev = rockchip->pci->dev; >> + struct property *prop; >> + const char *name; >> + int i = 0, ret, count; >> + >> + count = of_property_count_strings(dev->of_node, "clock-names"); >> + if (count < 1) >> + return -ENODEV; >> + >> + rockchip->clks = devm_kcalloc(dev, count, >> + sizeof(struct clk_bulk_data), >> + GFP_KERNEL); >> + if (!rockchip->clks) >> + return -ENOMEM; >> + >> + rockchip->clk_cnt = count; >> + >> + of_property_for_each_string(dev->of_node, "clock-names", >> + prop, name) { >> + rockchip->clks[i].id = name; >> + if (!rockchip->clks[i].id) >> + return -ENOMEM; >> + i++; >> + } > > of_clk_bulk_get_all() has existed for a while; no need to open-code it. > >> + >> + ret = devm_clk_bulk_get(dev, count, rockchip->clks); >> + if (ret) >> + return ret; >> + >> + ret = clk_bulk_prepare(count, rockchip->clks); >> + if (ret) >> + return ret; >> + >> + ret = clk_bulk_enable(count, rockchip->clks); >> + if (ret) { >> + clk_bulk_unprepare(count, rockchip->clks); >> + return ret; >> + } > > Similarly, clk_bulk_prepare_enable() also exists. > >> + return 0; >> +} >> + >> +static int rockchip_pcie_resource_get(struct platform_device *pdev, >> + struct rockchip_pcie *rockchip) >> +{ >> + struct resource *dbi_base; >> + struct resource *apb_base; >> + >> + dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, >> + "pcie-dbi"); >> + if (!dbi_base) >> + return -ENODEV; >> + >> + rockchip->dbi_base = devm_ioremap_resource(&pdev->dev, dbi_base); >> + if (IS_ERR(rockchip->dbi_base)) >> + return PTR_ERR(rockchip->dbi_base); > > It still has a ridiculously impractical name that I detest, but if I > don't mention devm_platform_ioremap_resource_byname() I'm sure > somebody else would come along and suggest it. > >> + rockchip->pci->dbi_base = rockchip->dbi_base; >> + >> + apb_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, >> + "pcie-apb"); >> + if (!apb_base) >> + return -ENODEV; >> + >> + rockchip->apb_base = devm_ioremap_resource(&pdev->dev, apb_base); >> + if (IS_ERR(rockchip->apb_base)) >> + return PTR_ERR(rockchip->apb_base); >> + >> + rockchip->rst_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", >> + GPIOD_OUT_HIGH); >> + if (IS_ERR(rockchip->rst_gpio)) >> + return PTR_ERR(rockchip->rst_gpio); >> + >> + return 0; >> +} >> + >> +static int rockchip_pcie_phy_init(struct rockchip_pcie *rockchip) >> +{ >> + int ret; >> + struct device *dev = rockchip->pci->dev; >> + >> + rockchip->phy = devm_phy_get(dev, "pcie-phy"); >> + if (IS_ERR(rockchip->phy)) { >> + if (PTR_ERR(rockchip->phy) != -EPROBE_DEFER) >> + dev_info(dev, "missing phy\n"); >> + return PTR_ERR(rockchip->phy); > > Use dev_err_probe() for this pattern now. > >> + } >> + >> + ret = phy_init(rockchip->phy); >> + if (ret < 0) >> + return ret; >> + >> + phy_power_on(rockchip->phy); >> + >> + return 0; >> +} >> + >> +static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip) >> +{ >> + phy_exit(rockchip->phy); >> + phy_power_off(rockchip->phy); >> +} >> + >> +static int rockchip_pcie_reset_control_release(struct rockchip_pcie >> *rockchip) >> +{ >> + struct device *dev = rockchip->pci->dev; >> + struct property *prop; >> + const char *name; >> + int ret, count, i = 0; >> + >> + count = of_property_count_strings(dev->of_node, "reset-names"); >> + if (count < 1) >> + return -ENODEV; >> + >> + rockchip->rsts = devm_kcalloc(dev, count, >> + sizeof(struct reset_bulk_data), >> + GFP_KERNEL); >> + if (!rockchip->rsts) >> + return -ENOMEM; >> + >> + of_property_for_each_string(dev->of_node, "reset-names", >> + prop, name) { >> + rockchip->rsts[i].id = name; >> + if (!rockchip->rsts[i].id) >> + return -ENOMEM; >> + i++; >> + } >> + >> + for (i = 0; i < count; i++) { >> + rockchip->rsts[i].rst = devm_reset_control_get_exclusive(dev, >> + rockchip->rsts[i].id); >> + if (IS_ERR(rockchip->rsts[i].rst)) { >> + dev_err(dev, "failed to get %s\n", >> + rockchip->clks[i].id); >> + return PTR_ERR(rockchip->rsts[i].rst); >> + } >> + } >> + >> + for (i = 0; i < count; i++) { >> + ret = reset_control_deassert(rockchip->rsts[i].rst); >> + if (ret) { >> + dev_err(dev, "failed to release %s\n", >> + rockchip->rsts[i].id); >> + return ret; >> + } >> + } > > Does the hardware require that the resets are deasserted in the > specific order of "whatever happens to be in the DT", or could you do > this much more simply with the reset_control_array APIs? > > Robin. > >> + >> + return 0; >> +} >> + >> +static void rockchip_pcie_fast_link_setup(struct rockchip_pcie >> *rockchip) >> +{ >> + u32 val; >> + >> + /* LTSSM EN ctrl mode */ >> + val = rockchip_pcie_readl_apb(rockchip, >> PCIE_CLIENT_HOT_RESET_CTRL); >> + val |= PCIE_LTSSM_ENABLE_ENHANCE | (PCIE_LTSSM_ENABLE_ENHANCE << >> 16); >> + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_HOT_RESET_CTRL, >> val); >> +} >> + >> + >> +static const struct of_device_id rockchip_pcie_of_match[] = { >> + { .compatible = "rockchip,rk3568-pcie", }, >> + { /* sentinel */ }, >> +}; >> + >> +static const struct dw_pcie_ops dw_pcie_ops = { >> + .link_up = rockchip_pcie_link_up, >> +}; >> + >> +static int rockchip_pcie_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct rockchip_pcie *rockchip; >> + struct dw_pcie *pci; >> + int ret; >> + >> + rockchip = devm_kzalloc(dev, sizeof(*rockchip), GFP_KERNEL); >> + if (!rockchip) >> + return -ENOMEM; >> + >> + pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); >> + if (!pci) >> + return -ENOMEM; >> + >> + pci->dev = dev; >> + pci->ops = &dw_pcie_ops; >> + >> + rockchip->pci = pci; >> + >> + ret = rockchip_pcie_resource_get(pdev, rockchip); >> + if (ret) >> + return ret; >> + >> + /* DON'T MOVE ME: must be enable before phy init */ >> + rockchip->vpcie3v3 = devm_regulator_get_optional(dev, "vpcie3v3"); >> + if (IS_ERR(rockchip->vpcie3v3)) { >> + if (PTR_ERR(rockchip->vpcie3v3) != -ENODEV) >> + return PTR_ERR(rockchip->vpcie3v3); >> + dev_info(dev, "no vpcie3v3 regulator found\n"); >> + } >> + >> + if (!IS_ERR(rockchip->vpcie3v3)) { >> + ret = regulator_enable(rockchip->vpcie3v3); >> + if (ret) { >> + dev_err(pci->dev, "fail to enable vpcie3v3 regulator\n"); >> + return ret; >> + } >> + } >> + >> + ret = rockchip_pcie_phy_init(rockchip); >> + if (ret) >> + goto disable_regulator; >> + >> + ret = rockchip_pcie_reset_control_release(rockchip); >> + if (ret) >> + goto deinit_phy; >> + >> + ret = rockchip_pcie_clk_init(rockchip); >> + if (ret) >> + goto deinit_phy; >> + >> + rockchip_pcie_fast_link_setup(rockchip); >> + >> + platform_set_drvdata(pdev, rockchip); >> + >> + ret = rockchip_add_pcie_port(rockchip); >> + if (ret) >> + goto deinit_clk; >> + >> + return 0; >> + >> +deinit_clk: >> + rockchip_pcie_clk_deinit(rockchip); >> +deinit_phy: >> + rockchip_pcie_phy_deinit(rockchip); >> +disable_regulator: >> + if (!IS_ERR(rockchip->vpcie3v3)) >> + regulator_disable(rockchip->vpcie3v3); >> + >> + return ret; >> +} >> + >> +MODULE_DEVICE_TABLE(of, rockchip_pcie_of_match); >> + >> +static struct platform_driver rockchip_pcie_driver = { >> + .driver = { >> + .name = "rk-pcie", >> + .of_match_table = rockchip_pcie_of_match, >> + .suppress_bind_attrs = true, >> + }, >> + .probe = rockchip_pcie_probe, >> +}; >> + >> +builtin_platform_driver(rockchip_pcie_driver); >> > > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/3] PCI: dwc: Skip allocating own MSI domain if using external MSI domain 2021-01-18 9:17 ` Simon Xue @ 2021-01-19 19:47 ` Robin Murphy -1 siblings, 0 replies; 32+ messages in thread From: Robin Murphy @ 2021-01-19 19:47 UTC (permalink / raw) To: Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, Shawn Lin, linux-rockchip On 2021-01-18 09:17, Simon Xue wrote: > From: Shawn Lin <shawn.lin@rock-chips.com> > > On some platform, external MSI domain is using instead of the one > created by designware driver. For instance, if using GIC-V3-ITS > as a MSI domain, we only need set msi-map in the devicetree but > never need any bit in the designware driver to handle MSI stuff. > So skip allocating its own MSI domain for that case. How is this different from the existing pp->has_msi_ctrl? AFAICS, dw_pcie_host_init() won't call dw_pcie_allocate_domains() anyway if an external MSI controller is present. Robin. > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > Signed-off-by: Simon Xue <xxm@rock-chips.com> > --- > drivers/pci/controller/dwc/pcie-designware-host.c | 10 +++++++++- > drivers/pci/controller/dwc/pcie-designware.h | 1 + > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 8a84c005f32b..d9d93cab970a 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -235,6 +235,10 @@ int dw_pcie_allocate_domains(struct pcie_port *pp) > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > struct fwnode_handle *fwnode = of_node_to_fwnode(pci->dev->of_node); > > + /* Rely on the external MSI domain */ > + if (pp->msi_ext) > + return 0; > + > pp->irq_domain = irq_domain_create_linear(fwnode, pp->num_vectors, > &dw_pcie_msi_domain_ops, pp); > if (!pp->irq_domain) { > @@ -258,6 +262,9 @@ int dw_pcie_allocate_domains(struct pcie_port *pp) > > static void dw_pcie_free_msi(struct pcie_port *pp) > { > + if (pp->msi_ext) > + return; > + > if (pp->msi_irq) { > irq_set_chained_handler(pp->msi_irq, NULL); > irq_set_handler_data(pp->msi_irq, NULL); > @@ -359,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp) > if (pci->link_gen < 1) > pci->link_gen = of_pci_get_max_link_speed(np); > > - if (pci_msi_enabled()) { > + if (pci_msi_enabled() && > + !pp->msi_ext) { > pp->has_msi_ctrl = !(pp->ops->msi_host_init || > of_property_read_bool(np, "msi-parent") || > of_property_read_bool(np, "msi-map")); > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 0207840756c4..cf3b0664302a 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -195,6 +195,7 @@ struct pcie_port { > u32 irq_mask[MAX_MSI_CTRLS]; > struct pci_host_bridge *bridge; > raw_spinlock_t lock; > + int msi_ext; > DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); > }; > > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/3] PCI: dwc: Skip allocating own MSI domain if using external MSI domain @ 2021-01-19 19:47 ` Robin Murphy 0 siblings, 0 replies; 32+ messages in thread From: Robin Murphy @ 2021-01-19 19:47 UTC (permalink / raw) To: Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, Shawn Lin, linux-rockchip On 2021-01-18 09:17, Simon Xue wrote: > From: Shawn Lin <shawn.lin@rock-chips.com> > > On some platform, external MSI domain is using instead of the one > created by designware driver. For instance, if using GIC-V3-ITS > as a MSI domain, we only need set msi-map in the devicetree but > never need any bit in the designware driver to handle MSI stuff. > So skip allocating its own MSI domain for that case. How is this different from the existing pp->has_msi_ctrl? AFAICS, dw_pcie_host_init() won't call dw_pcie_allocate_domains() anyway if an external MSI controller is present. Robin. > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > Signed-off-by: Simon Xue <xxm@rock-chips.com> > --- > drivers/pci/controller/dwc/pcie-designware-host.c | 10 +++++++++- > drivers/pci/controller/dwc/pcie-designware.h | 1 + > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index 8a84c005f32b..d9d93cab970a 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -235,6 +235,10 @@ int dw_pcie_allocate_domains(struct pcie_port *pp) > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > struct fwnode_handle *fwnode = of_node_to_fwnode(pci->dev->of_node); > > + /* Rely on the external MSI domain */ > + if (pp->msi_ext) > + return 0; > + > pp->irq_domain = irq_domain_create_linear(fwnode, pp->num_vectors, > &dw_pcie_msi_domain_ops, pp); > if (!pp->irq_domain) { > @@ -258,6 +262,9 @@ int dw_pcie_allocate_domains(struct pcie_port *pp) > > static void dw_pcie_free_msi(struct pcie_port *pp) > { > + if (pp->msi_ext) > + return; > + > if (pp->msi_irq) { > irq_set_chained_handler(pp->msi_irq, NULL); > irq_set_handler_data(pp->msi_irq, NULL); > @@ -359,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp) > if (pci->link_gen < 1) > pci->link_gen = of_pci_get_max_link_speed(np); > > - if (pci_msi_enabled()) { > + if (pci_msi_enabled() && > + !pp->msi_ext) { > pp->has_msi_ctrl = !(pp->ops->msi_host_init || > of_property_read_bool(np, "msi-parent") || > of_property_read_bool(np, "msi-map")); > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h > index 0207840756c4..cf3b0664302a 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.h > +++ b/drivers/pci/controller/dwc/pcie-designware.h > @@ -195,6 +195,7 @@ struct pcie_port { > u32 irq_mask[MAX_MSI_CTRLS]; > struct pci_host_bridge *bridge; > raw_spinlock_t lock; > + int msi_ext; > DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); > }; > > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/3] PCI: dwc: Skip allocating own MSI domain if using external MSI domain 2021-01-19 19:47 ` Robin Murphy @ 2021-01-20 1:53 ` xxm -1 siblings, 0 replies; 32+ messages in thread From: xxm @ 2021-01-20 1:53 UTC (permalink / raw) To: Robin Murphy, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, Shawn Lin, linux-rockchip Hi Robin, 在 2021/1/20 3:47, Robin Murphy 写道: > On 2021-01-18 09:17, Simon Xue wrote: >> From: Shawn Lin <shawn.lin@rock-chips.com> >> >> On some platform, external MSI domain is using instead of the one >> created by designware driver. For instance, if using GIC-V3-ITS >> as a MSI domain, we only need set msi-map in the devicetree but >> never need any bit in the designware driver to handle MSI stuff. >> So skip allocating its own MSI domain for that case. > > How is this different from the existing pp->has_msi_ctrl? AFAICS, > dw_pcie_host_init() won't call dw_pcie_allocate_domains() anyway if an > external MSI controller is present. Thanks for your review, I didn't notice pp->has_msi_ctrl already exist, will abandon this patch. Simon Xue. > > Robin. > >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> Signed-off-by: Simon Xue <xxm@rock-chips.com> >> --- >> drivers/pci/controller/dwc/pcie-designware-host.c | 10 +++++++++- >> drivers/pci/controller/dwc/pcie-designware.h | 1 + >> 2 files changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c >> b/drivers/pci/controller/dwc/pcie-designware-host.c >> index 8a84c005f32b..d9d93cab970a 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware-host.c >> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c >> @@ -235,6 +235,10 @@ int dw_pcie_allocate_domains(struct pcie_port *pp) >> struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >> struct fwnode_handle *fwnode = >> of_node_to_fwnode(pci->dev->of_node); >> + /* Rely on the external MSI domain */ >> + if (pp->msi_ext) >> + return 0; >> + >> pp->irq_domain = irq_domain_create_linear(fwnode, pp->num_vectors, >> &dw_pcie_msi_domain_ops, pp); >> if (!pp->irq_domain) { >> @@ -258,6 +262,9 @@ int dw_pcie_allocate_domains(struct pcie_port *pp) >> static void dw_pcie_free_msi(struct pcie_port *pp) >> { >> + if (pp->msi_ext) >> + return; >> + >> if (pp->msi_irq) { >> irq_set_chained_handler(pp->msi_irq, NULL); >> irq_set_handler_data(pp->msi_irq, NULL); >> @@ -359,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp) >> if (pci->link_gen < 1) >> pci->link_gen = of_pci_get_max_link_speed(np); >> - if (pci_msi_enabled()) { >> + if (pci_msi_enabled() && >> + !pp->msi_ext) { >> pp->has_msi_ctrl = !(pp->ops->msi_host_init || >> of_property_read_bool(np, "msi-parent") || >> of_property_read_bool(np, "msi-map")); >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h >> b/drivers/pci/controller/dwc/pcie-designware.h >> index 0207840756c4..cf3b0664302a 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.h >> +++ b/drivers/pci/controller/dwc/pcie-designware.h >> @@ -195,6 +195,7 @@ struct pcie_port { >> u32 irq_mask[MAX_MSI_CTRLS]; >> struct pci_host_bridge *bridge; >> raw_spinlock_t lock; >> + int msi_ext; >> DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); >> }; >> > > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/3] PCI: dwc: Skip allocating own MSI domain if using external MSI domain @ 2021-01-20 1:53 ` xxm 0 siblings, 0 replies; 32+ messages in thread From: xxm @ 2021-01-20 1:53 UTC (permalink / raw) To: Robin Murphy, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, Shawn Lin, linux-rockchip Hi Robin, 在 2021/1/20 3:47, Robin Murphy 写道: > On 2021-01-18 09:17, Simon Xue wrote: >> From: Shawn Lin <shawn.lin@rock-chips.com> >> >> On some platform, external MSI domain is using instead of the one >> created by designware driver. For instance, if using GIC-V3-ITS >> as a MSI domain, we only need set msi-map in the devicetree but >> never need any bit in the designware driver to handle MSI stuff. >> So skip allocating its own MSI domain for that case. > > How is this different from the existing pp->has_msi_ctrl? AFAICS, > dw_pcie_host_init() won't call dw_pcie_allocate_domains() anyway if an > external MSI controller is present. Thanks for your review, I didn't notice pp->has_msi_ctrl already exist, will abandon this patch. Simon Xue. > > Robin. > >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> Signed-off-by: Simon Xue <xxm@rock-chips.com> >> --- >> drivers/pci/controller/dwc/pcie-designware-host.c | 10 +++++++++- >> drivers/pci/controller/dwc/pcie-designware.h | 1 + >> 2 files changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c >> b/drivers/pci/controller/dwc/pcie-designware-host.c >> index 8a84c005f32b..d9d93cab970a 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware-host.c >> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c >> @@ -235,6 +235,10 @@ int dw_pcie_allocate_domains(struct pcie_port *pp) >> struct dw_pcie *pci = to_dw_pcie_from_pp(pp); >> struct fwnode_handle *fwnode = >> of_node_to_fwnode(pci->dev->of_node); >> + /* Rely on the external MSI domain */ >> + if (pp->msi_ext) >> + return 0; >> + >> pp->irq_domain = irq_domain_create_linear(fwnode, pp->num_vectors, >> &dw_pcie_msi_domain_ops, pp); >> if (!pp->irq_domain) { >> @@ -258,6 +262,9 @@ int dw_pcie_allocate_domains(struct pcie_port *pp) >> static void dw_pcie_free_msi(struct pcie_port *pp) >> { >> + if (pp->msi_ext) >> + return; >> + >> if (pp->msi_irq) { >> irq_set_chained_handler(pp->msi_irq, NULL); >> irq_set_handler_data(pp->msi_irq, NULL); >> @@ -359,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp) >> if (pci->link_gen < 1) >> pci->link_gen = of_pci_get_max_link_speed(np); >> - if (pci_msi_enabled()) { >> + if (pci_msi_enabled() && >> + !pp->msi_ext) { >> pp->has_msi_ctrl = !(pp->ops->msi_host_init || >> of_property_read_bool(np, "msi-parent") || >> of_property_read_bool(np, "msi-map")); >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h >> b/drivers/pci/controller/dwc/pcie-designware.h >> index 0207840756c4..cf3b0664302a 100644 >> --- a/drivers/pci/controller/dwc/pcie-designware.h >> +++ b/drivers/pci/controller/dwc/pcie-designware.h >> @@ -195,6 +195,7 @@ struct pcie_port { >> u32 irq_mask[MAX_MSI_CTRLS]; >> struct pci_host_bridge *bridge; >> raw_spinlock_t lock; >> + int msi_ext; >> DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); >> }; >> > > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2021-01-20 17:23 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-18 9:17 [PATCH 1/3] PCI: dwc: Skip allocating own MSI domain if using external MSI domain Simon Xue 2021-01-18 9:17 ` Simon Xue 2021-01-18 9:17 ` [PATCH 2/3] dt-bindings: rockchip: Add DesignWare based PCIe controller Simon Xue 2021-01-18 9:17 ` Simon Xue 2021-01-19 13:07 ` Johan Jonker 2021-01-19 13:07 ` Johan Jonker 2021-01-19 13:14 ` Heiko Stübner 2021-01-19 13:14 ` Heiko Stübner 2021-01-19 15:11 ` Johan Jonker 2021-01-19 15:11 ` Johan Jonker 2021-01-19 18:40 ` Robin Murphy 2021-01-19 18:40 ` Robin Murphy 2021-01-20 14:16 ` Rob Herring 2021-01-20 14:16 ` Rob Herring 2021-01-20 14:54 ` Heiko Stübner 2021-01-20 14:54 ` Heiko Stübner 2021-01-20 16:20 ` Robin Murphy 2021-01-20 16:20 ` Robin Murphy 2021-01-19 19:58 ` Robin Murphy 2021-01-19 19:58 ` Robin Murphy 2021-01-20 10:06 ` xxm 2021-01-20 10:06 ` xxm 2021-01-18 9:17 ` [PATCH 3/3] PCI: rockchip: add " Simon Xue 2021-01-18 9:17 ` Simon Xue 2021-01-19 20:28 ` Robin Murphy 2021-01-19 20:28 ` Robin Murphy 2021-01-20 1:58 ` xxm 2021-01-20 1:58 ` xxm 2021-01-19 19:47 ` [PATCH 1/3] PCI: dwc: Skip allocating own MSI domain if using external MSI domain Robin Murphy 2021-01-19 19:47 ` Robin Murphy 2021-01-20 1:53 ` xxm 2021-01-20 1:53 ` xxm
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.