* [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-20 10:16 ` Simon Xue 0 siblings, 0 replies; 20+ messages in thread From: Simon Xue @ 2021-01-20 10:16 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 | 140 ++++++++++++++++++ 1 file changed, 140 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml new file mode 100644 index 000000000000..9d3a57f5305e --- /dev/null +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml @@ -0,0 +1,140 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: DesignWare based PCIe RC controller on Rockchip SoCs + +maintainers: + - Shawn Lin <shawn.lin@rock-chips.com> + - Simon Xue <xxm@rock-chips.com> + +description: |+ + RK3568 SoC PCIe host controller is based on the Synopsys DesignWare + PCIe IP and thus inherits all the common properties defined in + designware-pcie.txt. + +allOf: + - $ref: /schemas/pci/pci-bus.yaml# + +# We need a select here so we don't match all nodes with 'snps,dw-pcie' +select: + properties: + compatible: + contains: + const: rockchip,rk3568-pcie + required: + - compatible + +properties: + compatible: + item: + - const: rockchip,rk3568-pcie + - const: snps,dw-pcie + reg: + maxItems: 1 + + interrupt: + - description: system information + - description: power management control + - description: PCIe message + - description: legacy interrupt + - description: error report + + interrupt-names: + items: + - const: sys + - const: pmc + - const: msg + - const: legacy + - const: err + + clocks: + items: + - description: AHB clock for PCIe master + - description: AHB clock for PCIe slave + - description: AHB clock for PCIe dbi + - description: APB clock for PCIe + - description: Auxiliary clock for PCIe + + clock-names: + items: + - const: aclk_mst + - const: aclk_slv + - const: aclk_dbi + - const: pclk + - const: aux + + msi-map: true + + power-domains: + maxItems: 1 + + resets: + maxItems: 1 + + reset-names: + items: + - const: pipe + +required: + - compatible + - bus-range + - reg + - reg-names + - clocks + - clock-names + - msi-map + - num-lanes + - phys + - phy-names + - power-domains + - resets + - reset-names + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/clock/rk3568-cru.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/power/rk3568-power.h> + + bus { + #address-cells = <2>; + #size-cells = <2>; + + pcie3x2: pcie@fe280000 { + compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; + #address-cells = <3>; + #size-cells = <2>; + bus-range = <0x20 0x2f>; + reg = <0x3 0xc0800000 0x0 0x400000>, + <0x0 0xfe280000 0x0 0x10000>; + reg-names = "pcie-dbi", "pcie-apb"; + interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "sys", "pmc", "msg", "legacy", "err"; + clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>, + <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>, + <&cru CLK_PCIE30X2_AUX_NDFT>; + clock-names = "aclk_mst", "aclk_slv", + "aclk_dbi", "pclk", + "aux"; + msi-map = <0x2000 &its 0x2000 0x1000>; + num-lanes = <2>; + phys = <&pcie30phy>; + phy-names = "pcie-phy"; + power-domains = <&power RK3568_PD_PIPE>; + ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000 + 0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000 + 0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>; + resets = <&cru SRST_PCIE30X2_POWERUP>; + reset-names = "pipe"; + }; + }; +... -- 2.25.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-20 10:16 ` Simon Xue 0 siblings, 0 replies; 20+ messages in thread From: Simon Xue @ 2021-01-20 10:16 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 | 140 ++++++++++++++++++ 1 file changed, 140 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml new file mode 100644 index 000000000000..9d3a57f5305e --- /dev/null +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml @@ -0,0 +1,140 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: DesignWare based PCIe RC controller on Rockchip SoCs + +maintainers: + - Shawn Lin <shawn.lin@rock-chips.com> + - Simon Xue <xxm@rock-chips.com> + +description: |+ + RK3568 SoC PCIe host controller is based on the Synopsys DesignWare + PCIe IP and thus inherits all the common properties defined in + designware-pcie.txt. + +allOf: + - $ref: /schemas/pci/pci-bus.yaml# + +# We need a select here so we don't match all nodes with 'snps,dw-pcie' +select: + properties: + compatible: + contains: + const: rockchip,rk3568-pcie + required: + - compatible + +properties: + compatible: + item: + - const: rockchip,rk3568-pcie + - const: snps,dw-pcie + reg: + maxItems: 1 + + interrupt: + - description: system information + - description: power management control + - description: PCIe message + - description: legacy interrupt + - description: error report + + interrupt-names: + items: + - const: sys + - const: pmc + - const: msg + - const: legacy + - const: err + + clocks: + items: + - description: AHB clock for PCIe master + - description: AHB clock for PCIe slave + - description: AHB clock for PCIe dbi + - description: APB clock for PCIe + - description: Auxiliary clock for PCIe + + clock-names: + items: + - const: aclk_mst + - const: aclk_slv + - const: aclk_dbi + - const: pclk + - const: aux + + msi-map: true + + power-domains: + maxItems: 1 + + resets: + maxItems: 1 + + reset-names: + items: + - const: pipe + +required: + - compatible + - bus-range + - reg + - reg-names + - clocks + - clock-names + - msi-map + - num-lanes + - phys + - phy-names + - power-domains + - resets + - reset-names + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/clock/rk3568-cru.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/power/rk3568-power.h> + + bus { + #address-cells = <2>; + #size-cells = <2>; + + pcie3x2: pcie@fe280000 { + compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; + #address-cells = <3>; + #size-cells = <2>; + bus-range = <0x20 0x2f>; + reg = <0x3 0xc0800000 0x0 0x400000>, + <0x0 0xfe280000 0x0 0x10000>; + reg-names = "pcie-dbi", "pcie-apb"; + interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "sys", "pmc", "msg", "legacy", "err"; + clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>, + <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>, + <&cru CLK_PCIE30X2_AUX_NDFT>; + clock-names = "aclk_mst", "aclk_slv", + "aclk_dbi", "pclk", + "aux"; + msi-map = <0x2000 &its 0x2000 0x1000>; + num-lanes = <2>; + phys = <&pcie30phy>; + phy-names = "pcie-phy"; + power-domains = <&power RK3568_PD_PIPE>; + ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000 + 0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000 + 0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>; + resets = <&cru SRST_PCIE30X2_POWERUP>; + reset-names = "pipe"; + }; + }; +... -- 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] 20+ messages in thread
* [PATCH v2 2/2] PCI: rockchip: add DesignWare based PCIe controller 2021-01-20 10:16 ` Simon Xue @ 2021-01-20 10:16 ` Simon Xue -1 siblings, 0 replies; 20+ messages in thread From: Simon Xue @ 2021-01-20 10:16 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 | 370 ++++++++++++++++++ 4 files changed, 382 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..e41cd6041c3f --- /dev/null +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c @@ -0,0 +1,370 @@ +// 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/gpio/consumer.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of_device.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 rockchip_pcie { + struct dw_pcie *pci; + void __iomem *apb_base; + struct phy *phy; + struct clk_bulk_data *clks; + unsigned int clk_cnt; + struct reset_control *rst; + 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; + + pp->ops = &rockchip_pcie_host_ops; + + 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_unprepare(rockchip->clk_cnt, rockchip->clks); +} + +static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip) +{ + struct device *dev = rockchip->pci->dev; + int ret; + + ret = devm_clk_bulk_get_all(dev, &rockchip->clks); + if (ret < 0) + return ret; + + rockchip->clk_cnt = ret; + + ret = clk_bulk_prepare_enable(rockchip->clk_cnt, rockchip->clks); + if (ret) + return ret; + + return 0; +} + +static int rockchip_pcie_resource_get(struct platform_device *pdev, + struct rockchip_pcie *rockchip) +{ + struct dw_pcie *pci = rockchip->pci; + + pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "pcie-dbi"); + if (IS_ERR(pci->dbi_base)) + return PTR_ERR(pci->dbi_base); + + rockchip->apb_base = devm_platform_ioremap_resource_byname(pdev, + "pcie-apb"); + 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)) + return dev_err_probe(dev, PTR_ERR(rockchip->phy), + "missing phy\n"); + + 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; + int ret; + + rockchip->rst = devm_reset_control_array_get_exclusive(dev); + if (IS_ERR(rockchip->rst)) + return dev_err_probe(dev, PTR_ERR(rockchip->rst), + "failed to get reset lines\n"); + + ret = reset_control_deassert(rockchip->rst); + + return ret; +} + +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] 20+ messages in thread
* [PATCH v2 2/2] PCI: rockchip: add DesignWare based PCIe controller @ 2021-01-20 10:16 ` Simon Xue 0 siblings, 0 replies; 20+ messages in thread From: Simon Xue @ 2021-01-20 10:16 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 | 370 ++++++++++++++++++ 4 files changed, 382 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..e41cd6041c3f --- /dev/null +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c @@ -0,0 +1,370 @@ +// 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/gpio/consumer.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of_device.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 rockchip_pcie { + struct dw_pcie *pci; + void __iomem *apb_base; + struct phy *phy; + struct clk_bulk_data *clks; + unsigned int clk_cnt; + struct reset_control *rst; + 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; + + pp->ops = &rockchip_pcie_host_ops; + + 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_unprepare(rockchip->clk_cnt, rockchip->clks); +} + +static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip) +{ + struct device *dev = rockchip->pci->dev; + int ret; + + ret = devm_clk_bulk_get_all(dev, &rockchip->clks); + if (ret < 0) + return ret; + + rockchip->clk_cnt = ret; + + ret = clk_bulk_prepare_enable(rockchip->clk_cnt, rockchip->clks); + if (ret) + return ret; + + return 0; +} + +static int rockchip_pcie_resource_get(struct platform_device *pdev, + struct rockchip_pcie *rockchip) +{ + struct dw_pcie *pci = rockchip->pci; + + pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "pcie-dbi"); + if (IS_ERR(pci->dbi_base)) + return PTR_ERR(pci->dbi_base); + + rockchip->apb_base = devm_platform_ioremap_resource_byname(pdev, + "pcie-apb"); + 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)) + return dev_err_probe(dev, PTR_ERR(rockchip->phy), + "missing phy\n"); + + 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; + int ret; + + rockchip->rst = devm_reset_control_array_get_exclusive(dev); + if (IS_ERR(rockchip->rst)) + return dev_err_probe(dev, PTR_ERR(rockchip->rst), + "failed to get reset lines\n"); + + ret = reset_control_deassert(rockchip->rst); + + return ret; +} + +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] 20+ messages in thread
* Re: [PATCH v2 2/2] PCI: rockchip: add DesignWare based PCIe controller 2021-01-20 10:16 ` Simon Xue @ 2021-01-20 14:55 ` Rob Herring -1 siblings, 0 replies; 20+ messages in thread From: Rob Herring @ 2021-01-20 14:55 UTC (permalink / raw) To: Simon Xue Cc: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, linux-rockchip, Shawn Lin On Wed, Jan 20, 2021 at 06:16:58PM +0800, 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 s/another/Cadence/ There's been a lot of reworking of the DWC drivers. You need to update this based on that. Details inline. > 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 | 370 ++++++++++++++++++ > 4 files changed, 382 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. I would make these changes a separate patch. I have a pending patch to move this driver to the Cadence directory. > > 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 PCIE_ROCKCHIP_DW_HOST to be more inline with other DWC config options. > + bool "Rockchip DesignWare PCIe controller" > + select PCIE_DW > + select PCIE_DW_HOST > + depends on ARCH_ROCKCHIP || COMPILE_TEST > + 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..e41cd6041c3f > --- /dev/null > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > @@ -0,0 +1,370 @@ > +// 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/gpio/consumer.h> > +#include <linux/mfd/syscon.h> > +#include <linux/module.h> > +#include <linux/of_device.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 rockchip_pcie { > + struct dw_pcie *pci; Make this a struct, not a pointer. Then 1 alloc instead of 2. > + void __iomem *apb_base; > + struct phy *phy; > + struct clk_bulk_data *clks; > + unsigned int clk_cnt; > + struct reset_control *rst; > + struct gpio_desc *rst_gpio; > + struct pcie_port pp; Move this up to 2nd element. > + struct regulator *vpcie3v3; > +}; > + > +static inline int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, > + u32 reg) You can drop inline on all of these. The compiler does that anyways with static functions. > +{ > + return readl(rockchip->apb_base + reg); > +} > + > +static inline void rockchip_pcie_writel_apb(struct rockchip_pcie *rockchip, > + u32 reg, u32 val) Use the same order as writel. val then reg. > +{ > + writel(val, rockchip->apb_base + reg); > +} > + > +static inline void rockchip_pcie_set_mode(struct rockchip_pcie *rockchip) Set what mode? Just drop this wrapper. The register access tells me more than the function name does. > +{ > + 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; Just to confirm, the PORT_LOGIC registers for this don't work? > + > + return 0; > +} > + > +static void rockchip_pcie_establish_link(struct dw_pcie *pci) s/establish/start/ > +{ > + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > + > + if (dw_pcie_link_up(pci)) { The DWC core does this for you now. > + 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)); > +} Please document what this debug stuff does. I tend to think it should be removed. > + > +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); The DWC core does this now after host_init. > + > + rockchip_pcie_enable_debug(rockchip); > + > + rockchip_pcie_establish_link(pci); > + dw_pcie_wait_for_link(pci); The DWC core does link handling now. > + > + 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) Remove this wrapper and move to probe. > +{ > + int ret; > + struct dw_pcie *pci = rockchip->pci; > + struct pcie_port *pp = &pci->pp; > + > + pp->ops = &rockchip_pcie_host_ops; > + > + 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) > +{ Another pointless wrapper. > + clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks); > +} > + > +static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip) > +{ > + struct device *dev = rockchip->pci->dev; > + int ret; > + > + ret = devm_clk_bulk_get_all(dev, &rockchip->clks); > + if (ret < 0) > + return ret; > + > + rockchip->clk_cnt = ret; > + > + ret = clk_bulk_prepare_enable(rockchip->clk_cnt, rockchip->clks); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int rockchip_pcie_resource_get(struct platform_device *pdev, > + struct rockchip_pcie *rockchip) > +{ > + struct dw_pcie *pci = rockchip->pci; > + > + pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "pcie-dbi"); The established name is 'dbi' and the DWC core handles it for you. > + if (IS_ERR(pci->dbi_base)) > + return PTR_ERR(pci->dbi_base); > + > + rockchip->apb_base = devm_platform_ioremap_resource_byname(pdev, > + "pcie-apb"); Just 'apb' for the name. > + 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)) > + return dev_err_probe(dev, PTR_ERR(rockchip->phy), > + "missing phy\n"); > + > + 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; > + int ret; > + > + rockchip->rst = devm_reset_control_array_get_exclusive(dev); > + if (IS_ERR(rockchip->rst)) > + return dev_err_probe(dev, PTR_ERR(rockchip->rst), > + "failed to get reset lines\n"); > + > + ret = reset_control_deassert(rockchip->rst); > + > + return ret; > +} > + > +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 */ }, > +}; Move this next to MODULE_DEVICE_TABLE. > + > +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) I think that cannot happen since it is optional. > + return PTR_ERR(rockchip->vpcie3v3); Use dev_err_probe() > + dev_info(dev, "no vpcie3v3 regulator found\n"); So? It's optional. > + } > + > + if (!IS_ERR(rockchip->vpcie3v3)) { You shouldn't need this check. > + 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); Move this up next to 'rockchip' alloc. > + > + 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)) Shouldn't need this check. > + 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 [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] PCI: rockchip: add DesignWare based PCIe controller @ 2021-01-20 14:55 ` Rob Herring 0 siblings, 0 replies; 20+ messages in thread From: Rob Herring @ 2021-01-20 14:55 UTC (permalink / raw) To: Simon Xue Cc: Bjorn Helgaas, linux-pci, Shawn Lin, Lorenzo Pieralisi, linux-rockchip On Wed, Jan 20, 2021 at 06:16:58PM +0800, 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 s/another/Cadence/ There's been a lot of reworking of the DWC drivers. You need to update this based on that. Details inline. > 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 | 370 ++++++++++++++++++ > 4 files changed, 382 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. I would make these changes a separate patch. I have a pending patch to move this driver to the Cadence directory. > > 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 PCIE_ROCKCHIP_DW_HOST to be more inline with other DWC config options. > + bool "Rockchip DesignWare PCIe controller" > + select PCIE_DW > + select PCIE_DW_HOST > + depends on ARCH_ROCKCHIP || COMPILE_TEST > + 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..e41cd6041c3f > --- /dev/null > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > @@ -0,0 +1,370 @@ > +// 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/gpio/consumer.h> > +#include <linux/mfd/syscon.h> > +#include <linux/module.h> > +#include <linux/of_device.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 rockchip_pcie { > + struct dw_pcie *pci; Make this a struct, not a pointer. Then 1 alloc instead of 2. > + void __iomem *apb_base; > + struct phy *phy; > + struct clk_bulk_data *clks; > + unsigned int clk_cnt; > + struct reset_control *rst; > + struct gpio_desc *rst_gpio; > + struct pcie_port pp; Move this up to 2nd element. > + struct regulator *vpcie3v3; > +}; > + > +static inline int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, > + u32 reg) You can drop inline on all of these. The compiler does that anyways with static functions. > +{ > + return readl(rockchip->apb_base + reg); > +} > + > +static inline void rockchip_pcie_writel_apb(struct rockchip_pcie *rockchip, > + u32 reg, u32 val) Use the same order as writel. val then reg. > +{ > + writel(val, rockchip->apb_base + reg); > +} > + > +static inline void rockchip_pcie_set_mode(struct rockchip_pcie *rockchip) Set what mode? Just drop this wrapper. The register access tells me more than the function name does. > +{ > + 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; Just to confirm, the PORT_LOGIC registers for this don't work? > + > + return 0; > +} > + > +static void rockchip_pcie_establish_link(struct dw_pcie *pci) s/establish/start/ > +{ > + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > + > + if (dw_pcie_link_up(pci)) { The DWC core does this for you now. > + 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)); > +} Please document what this debug stuff does. I tend to think it should be removed. > + > +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); The DWC core does this now after host_init. > + > + rockchip_pcie_enable_debug(rockchip); > + > + rockchip_pcie_establish_link(pci); > + dw_pcie_wait_for_link(pci); The DWC core does link handling now. > + > + 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) Remove this wrapper and move to probe. > +{ > + int ret; > + struct dw_pcie *pci = rockchip->pci; > + struct pcie_port *pp = &pci->pp; > + > + pp->ops = &rockchip_pcie_host_ops; > + > + 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) > +{ Another pointless wrapper. > + clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks); > +} > + > +static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip) > +{ > + struct device *dev = rockchip->pci->dev; > + int ret; > + > + ret = devm_clk_bulk_get_all(dev, &rockchip->clks); > + if (ret < 0) > + return ret; > + > + rockchip->clk_cnt = ret; > + > + ret = clk_bulk_prepare_enable(rockchip->clk_cnt, rockchip->clks); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int rockchip_pcie_resource_get(struct platform_device *pdev, > + struct rockchip_pcie *rockchip) > +{ > + struct dw_pcie *pci = rockchip->pci; > + > + pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "pcie-dbi"); The established name is 'dbi' and the DWC core handles it for you. > + if (IS_ERR(pci->dbi_base)) > + return PTR_ERR(pci->dbi_base); > + > + rockchip->apb_base = devm_platform_ioremap_resource_byname(pdev, > + "pcie-apb"); Just 'apb' for the name. > + 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)) > + return dev_err_probe(dev, PTR_ERR(rockchip->phy), > + "missing phy\n"); > + > + 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; > + int ret; > + > + rockchip->rst = devm_reset_control_array_get_exclusive(dev); > + if (IS_ERR(rockchip->rst)) > + return dev_err_probe(dev, PTR_ERR(rockchip->rst), > + "failed to get reset lines\n"); > + > + ret = reset_control_deassert(rockchip->rst); > + > + return ret; > +} > + > +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 */ }, > +}; Move this next to MODULE_DEVICE_TABLE. > + > +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) I think that cannot happen since it is optional. > + return PTR_ERR(rockchip->vpcie3v3); Use dev_err_probe() > + dev_info(dev, "no vpcie3v3 regulator found\n"); So? It's optional. > + } > + > + if (!IS_ERR(rockchip->vpcie3v3)) { You shouldn't need this check. > + 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); Move this up next to 'rockchip' alloc. > + > + 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)) Shouldn't need this check. > + 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 [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] PCI: rockchip: add DesignWare based PCIe controller【请注意,邮件由robherring2@gmail.com代发】 2021-01-20 14:55 ` Rob Herring @ 2021-01-21 7:57 ` xxm -1 siblings, 0 replies; 20+ messages in thread From: xxm @ 2021-01-21 7:57 UTC (permalink / raw) To: Rob Herring Cc: Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, linux-rockchip, Shawn Lin Hi Rob, Thanks for your review, some replies inline. All you pointed out will be fixed in v3. 在 2021/1/20 22:55, Rob Herring 写道: > On Wed, Jan 20, 2021 at 06:16:58PM +0800, 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 > s/another/Cadence/ > > There's been a lot of reworking of the DWC drivers. You need to update > this based on that. Details inline. DWC core really cover a lot, I think I need to refactor some codes. It's more reasonable to start link after all relative registers have set up. So, dw_pcie_host_ops->host_init just do some set up works, link start should postpone to dw_pcie_ops->start_link, and place rockchip_pcie_start_link in the dw_pcie_ops->start_link call back. > >> 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 | 370 ++++++++++++++++++ >> 4 files changed, 382 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. > I would make these changes a separate patch. I have a pending patch to > move this driver to the Cadence directory. Ok, will remove this. >> >> 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 > PCIE_ROCKCHIP_DW_HOST to be more inline with other DWC config options. > >> + bool "Rockchip DesignWare PCIe controller" >> + select PCIE_DW >> + select PCIE_DW_HOST >> + depends on ARCH_ROCKCHIP > || COMPILE_TEST > >> + 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..e41cd6041c3f >> --- /dev/null >> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c >> @@ -0,0 +1,370 @@ >> +// 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/gpio/consumer.h> >> +#include <linux/mfd/syscon.h> >> +#include <linux/module.h> >> +#include <linux/of_device.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 rockchip_pcie { >> + struct dw_pcie *pci; > Make this a struct, not a pointer. Then 1 alloc instead of 2. > >> + void __iomem *apb_base; >> + struct phy *phy; >> + struct clk_bulk_data *clks; >> + unsigned int clk_cnt; >> + struct reset_control *rst; >> + struct gpio_desc *rst_gpio; >> + struct pcie_port pp; > Move this up to 2nd element. No need to define pp, struct dw_pcie already has one, will remove this. > >> + struct regulator *vpcie3v3; >> +}; >> + >> +static inline int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, >> + u32 reg) > You can drop inline on all of these. The compiler does that anyways with > static functions. > >> +{ >> + return readl(rockchip->apb_base + reg); >> +} >> + >> +static inline void rockchip_pcie_writel_apb(struct rockchip_pcie *rockchip, >> + u32 reg, u32 val) > Use the same order as writel. val then reg. > >> +{ >> + writel(val, rockchip->apb_base + reg); >> +} >> + >> +static inline void rockchip_pcie_set_mode(struct rockchip_pcie *rockchip) > Set what mode? Just drop this wrapper. The register access tells me more > than the function name does. > >> +{ >> + 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; > Just to confirm, the PORT_LOGIC registers for this don't work? Yes, we need to check the link status from our own SoC's register. > >> + >> + return 0; >> +} >> + >> +static void rockchip_pcie_establish_link(struct dw_pcie *pci) > s/establish/start/ > >> +{ >> + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); >> + >> + if (dw_pcie_link_up(pci)) { > The DWC core does this for you now. > >> + 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)); >> +} > Please document what this debug stuff does. I tend to think it should be > removed. Ok, will remove this. >> + >> +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); > The DWC core does this now after host_init. > >> + >> + rockchip_pcie_enable_debug(rockchip); >> + >> + rockchip_pcie_establish_link(pci); >> + dw_pcie_wait_for_link(pci); > The DWC core does link handling now. > >> + >> + 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) > Remove this wrapper and move to probe. > >> +{ >> + int ret; >> + struct dw_pcie *pci = rockchip->pci; >> + struct pcie_port *pp = &pci->pp; >> + >> + pp->ops = &rockchip_pcie_host_ops; >> + >> + 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) >> +{ > Another pointless wrapper. > >> + clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks); >> +} >> + >> +static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip) >> +{ >> + struct device *dev = rockchip->pci->dev; >> + int ret; >> + >> + ret = devm_clk_bulk_get_all(dev, &rockchip->clks); >> + if (ret < 0) >> + return ret; >> + >> + rockchip->clk_cnt = ret; >> + >> + ret = clk_bulk_prepare_enable(rockchip->clk_cnt, rockchip->clks); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> + >> +static int rockchip_pcie_resource_get(struct platform_device *pdev, >> + struct rockchip_pcie *rockchip) >> +{ >> + struct dw_pcie *pci = rockchip->pci; >> + >> + pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "pcie-dbi"); > The established name is 'dbi' and the DWC core handles it for you. > >> + if (IS_ERR(pci->dbi_base)) >> + return PTR_ERR(pci->dbi_base); >> + >> + rockchip->apb_base = devm_platform_ioremap_resource_byname(pdev, >> + "pcie-apb"); > Just 'apb' for the name. > >> + 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)) >> + return dev_err_probe(dev, PTR_ERR(rockchip->phy), >> + "missing phy\n"); >> + >> + 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; >> + int ret; >> + >> + rockchip->rst = devm_reset_control_array_get_exclusive(dev); >> + if (IS_ERR(rockchip->rst)) >> + return dev_err_probe(dev, PTR_ERR(rockchip->rst), >> + "failed to get reset lines\n"); >> + >> + ret = reset_control_deassert(rockchip->rst); >> + >> + return ret; >> +} >> + >> +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 */ }, >> +}; > Move this next to MODULE_DEVICE_TABLE. > >> + >> +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) > I think that cannot happen since it is optional. > >> + return PTR_ERR(rockchip->vpcie3v3); > Use dev_err_probe() > >> + dev_info(dev, "no vpcie3v3 regulator found\n"); > So? It's optional. Yes, vpcie3v3 is enabled by software or by hardware directly. If get NULL, means enabled by hardware directly. >> + } >> + >> + if (!IS_ERR(rockchip->vpcie3v3)) { > You shouldn't need this check. > >> + 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); > Move this up next to 'rockchip' alloc. > >> + >> + 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)) > Shouldn't need this check. > >> + 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 [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] PCI: rockchip: add DesignWare based PCIe controller【请注意,邮件由robherring2@gmail.com代发】 @ 2021-01-21 7:57 ` xxm 0 siblings, 0 replies; 20+ messages in thread From: xxm @ 2021-01-21 7:57 UTC (permalink / raw) To: Rob Herring Cc: Bjorn Helgaas, linux-pci, Shawn Lin, Lorenzo Pieralisi, linux-rockchip Hi Rob, Thanks for your review, some replies inline. All you pointed out will be fixed in v3. 在 2021/1/20 22:55, Rob Herring 写道: > On Wed, Jan 20, 2021 at 06:16:58PM +0800, 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 > s/another/Cadence/ > > There's been a lot of reworking of the DWC drivers. You need to update > this based on that. Details inline. DWC core really cover a lot, I think I need to refactor some codes. It's more reasonable to start link after all relative registers have set up. So, dw_pcie_host_ops->host_init just do some set up works, link start should postpone to dw_pcie_ops->start_link, and place rockchip_pcie_start_link in the dw_pcie_ops->start_link call back. > >> 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 | 370 ++++++++++++++++++ >> 4 files changed, 382 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. > I would make these changes a separate patch. I have a pending patch to > move this driver to the Cadence directory. Ok, will remove this. >> >> 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 > PCIE_ROCKCHIP_DW_HOST to be more inline with other DWC config options. > >> + bool "Rockchip DesignWare PCIe controller" >> + select PCIE_DW >> + select PCIE_DW_HOST >> + depends on ARCH_ROCKCHIP > || COMPILE_TEST > >> + 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..e41cd6041c3f >> --- /dev/null >> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c >> @@ -0,0 +1,370 @@ >> +// 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/gpio/consumer.h> >> +#include <linux/mfd/syscon.h> >> +#include <linux/module.h> >> +#include <linux/of_device.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 rockchip_pcie { >> + struct dw_pcie *pci; > Make this a struct, not a pointer. Then 1 alloc instead of 2. > >> + void __iomem *apb_base; >> + struct phy *phy; >> + struct clk_bulk_data *clks; >> + unsigned int clk_cnt; >> + struct reset_control *rst; >> + struct gpio_desc *rst_gpio; >> + struct pcie_port pp; > Move this up to 2nd element. No need to define pp, struct dw_pcie already has one, will remove this. > >> + struct regulator *vpcie3v3; >> +}; >> + >> +static inline int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, >> + u32 reg) > You can drop inline on all of these. The compiler does that anyways with > static functions. > >> +{ >> + return readl(rockchip->apb_base + reg); >> +} >> + >> +static inline void rockchip_pcie_writel_apb(struct rockchip_pcie *rockchip, >> + u32 reg, u32 val) > Use the same order as writel. val then reg. > >> +{ >> + writel(val, rockchip->apb_base + reg); >> +} >> + >> +static inline void rockchip_pcie_set_mode(struct rockchip_pcie *rockchip) > Set what mode? Just drop this wrapper. The register access tells me more > than the function name does. > >> +{ >> + 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; > Just to confirm, the PORT_LOGIC registers for this don't work? Yes, we need to check the link status from our own SoC's register. > >> + >> + return 0; >> +} >> + >> +static void rockchip_pcie_establish_link(struct dw_pcie *pci) > s/establish/start/ > >> +{ >> + struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); >> + >> + if (dw_pcie_link_up(pci)) { > The DWC core does this for you now. > >> + 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)); >> +} > Please document what this debug stuff does. I tend to think it should be > removed. Ok, will remove this. >> + >> +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); > The DWC core does this now after host_init. > >> + >> + rockchip_pcie_enable_debug(rockchip); >> + >> + rockchip_pcie_establish_link(pci); >> + dw_pcie_wait_for_link(pci); > The DWC core does link handling now. > >> + >> + 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) > Remove this wrapper and move to probe. > >> +{ >> + int ret; >> + struct dw_pcie *pci = rockchip->pci; >> + struct pcie_port *pp = &pci->pp; >> + >> + pp->ops = &rockchip_pcie_host_ops; >> + >> + 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) >> +{ > Another pointless wrapper. > >> + clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks); >> +} >> + >> +static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip) >> +{ >> + struct device *dev = rockchip->pci->dev; >> + int ret; >> + >> + ret = devm_clk_bulk_get_all(dev, &rockchip->clks); >> + if (ret < 0) >> + return ret; >> + >> + rockchip->clk_cnt = ret; >> + >> + ret = clk_bulk_prepare_enable(rockchip->clk_cnt, rockchip->clks); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> + >> +static int rockchip_pcie_resource_get(struct platform_device *pdev, >> + struct rockchip_pcie *rockchip) >> +{ >> + struct dw_pcie *pci = rockchip->pci; >> + >> + pci->dbi_base = devm_platform_ioremap_resource_byname(pdev, "pcie-dbi"); > The established name is 'dbi' and the DWC core handles it for you. > >> + if (IS_ERR(pci->dbi_base)) >> + return PTR_ERR(pci->dbi_base); >> + >> + rockchip->apb_base = devm_platform_ioremap_resource_byname(pdev, >> + "pcie-apb"); > Just 'apb' for the name. > >> + 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)) >> + return dev_err_probe(dev, PTR_ERR(rockchip->phy), >> + "missing phy\n"); >> + >> + 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; >> + int ret; >> + >> + rockchip->rst = devm_reset_control_array_get_exclusive(dev); >> + if (IS_ERR(rockchip->rst)) >> + return dev_err_probe(dev, PTR_ERR(rockchip->rst), >> + "failed to get reset lines\n"); >> + >> + ret = reset_control_deassert(rockchip->rst); >> + >> + return ret; >> +} >> + >> +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 */ }, >> +}; > Move this next to MODULE_DEVICE_TABLE. > >> + >> +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) > I think that cannot happen since it is optional. > >> + return PTR_ERR(rockchip->vpcie3v3); > Use dev_err_probe() > >> + dev_info(dev, "no vpcie3v3 regulator found\n"); > So? It's optional. Yes, vpcie3v3 is enabled by software or by hardware directly. If get NULL, means enabled by hardware directly. >> + } >> + >> + if (!IS_ERR(rockchip->vpcie3v3)) { > You shouldn't need this check. > >> + 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); > Move this up next to 'rockchip' alloc. > >> + >> + 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)) > Shouldn't need this check. > >> + 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 [flat|nested] 20+ messages in thread
* [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-20 10:15 ` Simon Xue 0 siblings, 0 replies; 20+ messages in thread From: Simon Xue @ 2021-01-20 10:15 UTC (permalink / raw) To: Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, linux-rockchip, devicetree, robh+dt, Johan Jonker, Simon Xue Signed-off-by: Simon Xue <xxm@rock-chips.com> --- .../bindings/pci/rockchip-dw-pcie.yaml | 140 ++++++++++++++++++ 1 file changed, 140 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml new file mode 100644 index 000000000000..9d3a57f5305e --- /dev/null +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml @@ -0,0 +1,140 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: DesignWare based PCIe RC controller on Rockchip SoCs + +maintainers: + - Shawn Lin <shawn.lin@rock-chips.com> + - Simon Xue <xxm@rock-chips.com> + +description: |+ + RK3568 SoC PCIe host controller is based on the Synopsys DesignWare + PCIe IP and thus inherits all the common properties defined in + designware-pcie.txt. + +allOf: + - $ref: /schemas/pci/pci-bus.yaml# + +# We need a select here so we don't match all nodes with 'snps,dw-pcie' +select: + properties: + compatible: + contains: + const: rockchip,rk3568-pcie + required: + - compatible + +properties: + compatible: + item: + - const: rockchip,rk3568-pcie + - const: snps,dw-pcie + reg: + maxItems: 1 + + interrupt: + - description: system information + - description: power management control + - description: PCIe message + - description: legacy interrupt + - description: error report + + interrupt-names: + items: + - const: sys + - const: pmc + - const: msg + - const: legacy + - const: err + + clocks: + items: + - description: AHB clock for PCIe master + - description: AHB clock for PCIe slave + - description: AHB clock for PCIe dbi + - description: APB clock for PCIe + - description: Auxiliary clock for PCIe + + clock-names: + items: + - const: aclk_mst + - const: aclk_slv + - const: aclk_dbi + - const: pclk + - const: aux + + msi-map: true + + power-domains: + maxItems: 1 + + resets: + maxItems: 1 + + reset-names: + items: + - const: pipe + +required: + - compatible + - bus-range + - reg + - reg-names + - clocks + - clock-names + - msi-map + - num-lanes + - phys + - phy-names + - power-domains + - resets + - reset-names + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/clock/rk3568-cru.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/power/rk3568-power.h> + + bus { + #address-cells = <2>; + #size-cells = <2>; + + pcie3x2: pcie@fe280000 { + compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; + #address-cells = <3>; + #size-cells = <2>; + bus-range = <0x20 0x2f>; + reg = <0x3 0xc0800000 0x0 0x400000>, + <0x0 0xfe280000 0x0 0x10000>; + reg-names = "pcie-dbi", "pcie-apb"; + interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "sys", "pmc", "msg", "legacy", "err"; + clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>, + <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>, + <&cru CLK_PCIE30X2_AUX_NDFT>; + clock-names = "aclk_mst", "aclk_slv", + "aclk_dbi", "pclk", + "aux"; + msi-map = <0x2000 &its 0x2000 0x1000>; + num-lanes = <2>; + phys = <&pcie30phy>; + phy-names = "pcie-phy"; + power-domains = <&power RK3568_PD_PIPE>; + ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000 + 0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000 + 0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>; + resets = <&cru SRST_PCIE30X2_POWERUP>; + reset-names = "pipe"; + }; + }; +... -- 2.25.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-20 10:15 ` Simon Xue 0 siblings, 0 replies; 20+ messages in thread From: Simon Xue @ 2021-01-20 10:15 UTC (permalink / raw) To: Bjorn Helgaas, Lorenzo Pieralisi Cc: devicetree, Simon Xue, linux-pci, linux-rockchip, robh+dt, Johan Jonker Signed-off-by: Simon Xue <xxm@rock-chips.com> --- .../bindings/pci/rockchip-dw-pcie.yaml | 140 ++++++++++++++++++ 1 file changed, 140 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml new file mode 100644 index 000000000000..9d3a57f5305e --- /dev/null +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml @@ -0,0 +1,140 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: DesignWare based PCIe RC controller on Rockchip SoCs + +maintainers: + - Shawn Lin <shawn.lin@rock-chips.com> + - Simon Xue <xxm@rock-chips.com> + +description: |+ + RK3568 SoC PCIe host controller is based on the Synopsys DesignWare + PCIe IP and thus inherits all the common properties defined in + designware-pcie.txt. + +allOf: + - $ref: /schemas/pci/pci-bus.yaml# + +# We need a select here so we don't match all nodes with 'snps,dw-pcie' +select: + properties: + compatible: + contains: + const: rockchip,rk3568-pcie + required: + - compatible + +properties: + compatible: + item: + - const: rockchip,rk3568-pcie + - const: snps,dw-pcie + reg: + maxItems: 1 + + interrupt: + - description: system information + - description: power management control + - description: PCIe message + - description: legacy interrupt + - description: error report + + interrupt-names: + items: + - const: sys + - const: pmc + - const: msg + - const: legacy + - const: err + + clocks: + items: + - description: AHB clock for PCIe master + - description: AHB clock for PCIe slave + - description: AHB clock for PCIe dbi + - description: APB clock for PCIe + - description: Auxiliary clock for PCIe + + clock-names: + items: + - const: aclk_mst + - const: aclk_slv + - const: aclk_dbi + - const: pclk + - const: aux + + msi-map: true + + power-domains: + maxItems: 1 + + resets: + maxItems: 1 + + reset-names: + items: + - const: pipe + +required: + - compatible + - bus-range + - reg + - reg-names + - clocks + - clock-names + - msi-map + - num-lanes + - phys + - phy-names + - power-domains + - resets + - reset-names + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/clock/rk3568-cru.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/power/rk3568-power.h> + + bus { + #address-cells = <2>; + #size-cells = <2>; + + pcie3x2: pcie@fe280000 { + compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; + #address-cells = <3>; + #size-cells = <2>; + bus-range = <0x20 0x2f>; + reg = <0x3 0xc0800000 0x0 0x400000>, + <0x0 0xfe280000 0x0 0x10000>; + reg-names = "pcie-dbi", "pcie-apb"; + interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "sys", "pmc", "msg", "legacy", "err"; + clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>, + <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>, + <&cru CLK_PCIE30X2_AUX_NDFT>; + clock-names = "aclk_mst", "aclk_slv", + "aclk_dbi", "pclk", + "aux"; + msi-map = <0x2000 &its 0x2000 0x1000>; + num-lanes = <2>; + phys = <&pcie30phy>; + phy-names = "pcie-phy"; + power-domains = <&power RK3568_PD_PIPE>; + ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000 + 0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000 + 0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>; + resets = <&cru SRST_PCIE30X2_POWERUP>; + reset-names = "pipe"; + }; + }; +... -- 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] 20+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller 2021-01-20 10:15 ` Simon Xue @ 2021-01-20 14:07 ` Rob Herring -1 siblings, 0 replies; 20+ messages in thread From: Rob Herring @ 2021-01-20 14:07 UTC (permalink / raw) To: Simon Xue Cc: Johan Jonker, devicetree, linux-pci, Bjorn Helgaas, Lorenzo Pieralisi, robh+dt, linux-rockchip On Wed, 20 Jan 2021 18:15:53 +0800, Simon Xue wrote: > Signed-off-by: Simon Xue <xxm@rock-chips.com> > --- > .../bindings/pci/rockchip-dw-pcie.yaml | 140 ++++++++++++++++++ > 1 file changed, 140 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: ./Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml:39:7: [warning] wrong indentation: expected 4 but found 6 (indentation) dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml: properties:interrupt: [{'description': 'system information'}, {'description': 'power management control'}, {'description': 'PCIe message'}, {'description': 'legacy interrupt'}, {'description': 'error report'}] is not of type 'object', 'boolean' /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml: properties:compatible: Additional properties are not allowed ('item' was unexpected) /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml: properties:compatible: 'item' is not one of ['$ref', 'additionalItems', 'additionalProperties', 'allOf', 'anyOf', 'const', 'contains', 'default', 'dependencies', 'deprecated', 'description', 'else', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'items', 'if', 'minItems', 'minimum', 'maxItems', 'maximum', 'multipleOf', 'not', 'oneOf', 'pattern', 'patternProperties', 'properties', 'required', 'then', 'type', 'typeSize', 'unevaluatedProperties', 'uniqueItems'] /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml: properties:compatible: Additional properties are not allowed ('item' was unexpected) /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml: ignoring, error in schema: properties: interrupt warning: no schema found in file: ./Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml Documentation/devicetree/bindings/pci/rockchip-dw-pcie.example.dts:19:18: fatal error: dt-bindings/clock/rk3568-cru.h: No such file or directory 19 | #include <dt-bindings/clock/rk3568-cru.h> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. make[1]: *** [scripts/Makefile.lib:344: Documentation/devicetree/bindings/pci/rockchip-dw-pcie.example.dt.yaml] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1370: dt_binding_check] Error 2 See https://patchwork.ozlabs.org/patch/1429132 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-20 14:07 ` Rob Herring 0 siblings, 0 replies; 20+ messages in thread From: Rob Herring @ 2021-01-20 14:07 UTC (permalink / raw) To: Simon Xue Cc: devicetree, Lorenzo Pieralisi, linux-pci, linux-rockchip, robh+dt, Bjorn Helgaas, Johan Jonker On Wed, 20 Jan 2021 18:15:53 +0800, Simon Xue wrote: > Signed-off-by: Simon Xue <xxm@rock-chips.com> > --- > .../bindings/pci/rockchip-dw-pcie.yaml | 140 ++++++++++++++++++ > 1 file changed, 140 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: ./Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml:39:7: [warning] wrong indentation: expected 4 but found 6 (indentation) dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml: properties:interrupt: [{'description': 'system information'}, {'description': 'power management control'}, {'description': 'PCIe message'}, {'description': 'legacy interrupt'}, {'description': 'error report'}] is not of type 'object', 'boolean' /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml: properties:compatible: Additional properties are not allowed ('item' was unexpected) /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml: properties:compatible: 'item' is not one of ['$ref', 'additionalItems', 'additionalProperties', 'allOf', 'anyOf', 'const', 'contains', 'default', 'dependencies', 'deprecated', 'description', 'else', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'items', 'if', 'minItems', 'minimum', 'maxItems', 'maximum', 'multipleOf', 'not', 'oneOf', 'pattern', 'patternProperties', 'properties', 'required', 'then', 'type', 'typeSize', 'unevaluatedProperties', 'uniqueItems'] /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml: properties:compatible: Additional properties are not allowed ('item' was unexpected) /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml: ignoring, error in schema: properties: interrupt warning: no schema found in file: ./Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml Documentation/devicetree/bindings/pci/rockchip-dw-pcie.example.dts:19:18: fatal error: dt-bindings/clock/rk3568-cru.h: No such file or directory 19 | #include <dt-bindings/clock/rk3568-cru.h> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. make[1]: *** [scripts/Makefile.lib:344: Documentation/devicetree/bindings/pci/rockchip-dw-pcie.example.dt.yaml] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1370: dt_binding_check] Error 2 See https://patchwork.ozlabs.org/patch/1429132 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit. _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller 2021-01-20 10:15 ` Simon Xue @ 2021-01-20 17:07 ` Johan Jonker -1 siblings, 0 replies; 20+ messages in thread From: Johan Jonker @ 2021-01-20 17:07 UTC (permalink / raw) To: Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, linux-rockchip, devicetree, robh+dt, Heiko Stuebner Hi Simon, Thanks you for version 2. A few comments, have a look if it is useful or that you disagree. This patch has no commit message. Add one in version 3. Submit all patches in one batch with the same sort message ID to all maintainers including Heiko. Heiko Stuebner <heiko@sntech.de> Example message ID: 20210120101554.241029-1-xxm@rock-chips.com ///// Included is a copy of the Rockchip pcie nodes in a sort of test.dts below. Could you confirm that the properties in that dts are the one that we can expect for Linux mainline and can base our YAML document on? With rk3568-cru.h and rk3568-power.h manualy added we do some tests with the following commands: make ARCH=arm64 dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=~/.local/lib/python3.5/site-packages/dtschema/schemas/pci/pci-bus.yaml ///// Example notifications: /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: reg: [[3, 3225419776, 0, 4194304], [0, 4263968768, 0, 65536]] is too long /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: ranges: 'oneOf' conditional failed, one must be fixed: Before you submit version 3 make sure that all warnings gone as much as possible. On 1/20/21 11:15 AM, Simon Xue wrote: > Signed-off-by: Simon Xue <xxm@rock-chips.com> > --- > .../bindings/pci/rockchip-dw-pcie.yaml | 140 ++++++++++++++++++ > 1 file changed, 140 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > new file mode 100644 > index 000000000000..9d3a57f5305e > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > @@ -0,0 +1,140 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: DesignWare based PCIe RC controller on Rockchip SoCs > + > +maintainers: > + - Shawn Lin <shawn.lin@rock-chips.com> > + - Simon Xue <xxm@rock-chips.com> - Heiko Stuebner <heiko@sntech.de> ;) > + > +description: |+ > + RK3568 SoC PCIe host controller is based on the Synopsys DesignWare > + PCIe IP and thus inherits all the common properties defined in > + designware-pcie.txt. > + > +allOf: > + - $ref: /schemas/pci/pci-bus.yaml# > + > +# We need a select here so we don't match all nodes with 'snps,dw-pcie' > +select: > + properties: > + compatible: > + contains: > + const: rockchip,rk3568-pcie > + required: > + - compatible > + > +properties: > + compatible: > + item: items: > + - const: rockchip,rk3568-pcie > + - const: snps,dw-pcie Add empty line > + reg: items: - description: - description: Add some description for regs. > + maxItems: 1 remove This reg maxItems gives errors. > + > + interrupt: interrupts: items: > + - description: system information > + - description: power management control > + - description: PCIe message > + - description: legacy interrupt > + - description: error report > + > + interrupt-names: > + items: > + - const: sys > + - const: pmc > + - const: msg > + - const: legacy > + - const: err > + > + clocks: > + items: > + - description: AHB clock for PCIe master > + - description: AHB clock for PCIe slave > + - description: AHB clock for PCIe dbi > + - description: APB clock for PCIe > + - description: Auxiliary clock for PCIe > + > + clock-names: > + items: > + - const: aclk_mst > + - const: aclk_slv > + - const: aclk_dbi > + - const: pclk > + - const: aux > + > + msi-map: true > + > + power-domains: > + maxItems: 1 ///// These properties come from designware-pcie.txt Maybe add them here for now till there's a common yaml? num-ib-windows: number of inbound address translation windows num-ob-windows: number of outbound address translation windows Optional properties: num-lanes: ///// phys: maxItems: 1 phy-names: const: pcie-phy ranges: ...... ...... This ranges needs a fix so that it doesn't generate notifications. See above example. > + > + resets: > + maxItems: 1 > + > + reset-names: const: pipe > + items: > + - const: pipe remove > + > +required: > + - compatible > + - bus-range > + - reg > + - reg-names > + - clocks > + - clock-names > + - msi-map > + - num-lanes > + - phys > + - phy-names > + - power-domains > + - resets > + - reset-names Add also all the other properties that are defined in this binding and are required. But not the ones from pci-bus.yaml. > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/rk3568-cru.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/power/rk3568-power.h> If #include is not possible for now replace all defines by there original numbers. Just to get this binding pass for mainline. > + > + bus { > + #address-cells = <2>; > + #size-cells = <2>; > + > + pcie3x2: pcie@fe280000 { > + compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; dts sort order is: compatible reg interrupts the rest things with # > + reg = <0x3 0xc0800000 0x0 0x400000>, > + <0x0 0xfe280000 0x0 0x10000>; > + reg-names = "pcie-dbi", "pcie-apb"; > + interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "sys", "pmc", "msg", "legacy", "err"; Could you confirm that these "interrupt-names" are used in your driver? Else remove. bus-range = <0x20 0x2f>; > + clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>, > + <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>, > + <&cru CLK_PCIE30X2_AUX_NDFT>; > + clock-names = "aclk_mst", "aclk_slv", > + "aclk_dbi", "pclk", > + "aux"; device_type = "pci"; linux,pci-domain = <0>; num-ib-windows = <6>; num-ob-windows = <2>; max-link-speed = <2>; Maybe give a complete example? > + msi-map = <0x2000 &its 0x2000 0x1000>; > + num-lanes = <2>; > + phys = <&pcie30phy>; > + phy-names = "pcie-phy"; > + power-domains = <&power RK3568_PD_PIPE>; > + ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000 > + 0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000 > + 0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>; > + resets = <&cru SRST_PCIE30X2_POWERUP>; > + reset-names = "pipe"; #address-cells = <3>; #size-cells = <2>; > + }; > + }; > +... > /////////// test.dts /////////// /dts-v1/; #include <dt-bindings/clock/rk3568-cru.h> #include <dt-bindings/interrupt-controller/arm-gic.h> #include <dt-bindings/interrupt-controller/irq.h> #include <dt-bindings/phy/phy.h> #include <dt-bindings/power/rk3568-power.h> / { compatible = "rockchip,rk3568"; #address-cells = <2>; #size-cells = <2>; cru: clock-controller@fdd20000 { compatible = "rockchip,rk3568-cru"; reg = <0x0 0xfdd20000 0x0 0x1000>; #clock-cells = <1>; #reset-cells = <1>; }; pcie30phy: phy@fe8c0000 { compatible = "rockchip,rk3568-pcie3-phy"; reg = <0x0 0xfe8c0000 0x0 0x20000>; #phy-cells = <0>; }; combphy2_psq: phy@fe840000 { compatible = "rockchip,rk3568-naneng-combphy"; reg = <0x0 0xfe840000 0x0 0x100>; #phy-cells = <1>; }; gic: interrupt-controller@fd400000 { compatible = "arm,gic-v3"; #interrupt-cells = <3>; #address-cells = <2>; #size-cells = <2>; ranges; interrupt-controller; reg = <0x0 0xfd400000 0 0x10000>, <0x0 0xfd460000 0 0xc0000>; interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>; its: interrupt-controller@fd440000 { compatible = "arm,gic-v3-its"; msi-controller; #msi-cells = <1>; reg = <0x0 0xfd440000 0x0 0x20000>; status = "disabled"; }; }; pmu: power-management@fdd90000 { compatible = "rockchip,rk3568-pmu", "syscon", "simple-mfd"; reg = <0x0 0xfdd90000 0x0 0x1000>; power: power-controller { compatible = "rockchip,rk3568-power-controller"; #power-domain-cells = <1>; #address-cells = <1>; #size-cells = <0>; status = "okay"; }; }; pcie2x1: pcie@fe260000 { compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; reg = <0x3 0xc0000000 0x0 0x400000>, <0x0 0xfe260000 0x0 0x10000>; reg-names = "pcie-dbi", "pcie-apb"; interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "sys", "pmc", "msg", "legacy", "err"; bus-range = <0x0 0xf>; clocks = <&cru ACLK_PCIE20_MST>, <&cru ACLK_PCIE20_SLV>, <&cru ACLK_PCIE20_DBI>, <&cru PCLK_PCIE20>, <&cru CLK_PCIE20_AUX_NDFT>; clock-names = "aclk_mst", "aclk_slv", "aclk_dbi", "pclk", "aux"; device_type = "pci"; linux,pci-domain = <0>; num-ib-windows = <6>; num-ob-windows = <2>; max-link-speed = <2>; msi-map = <0x0 &its 0x0 0x1000>; num-lanes = <1>; phys = <&combphy2_psq PHY_TYPE_PCIE>; phy-names = "pcie-phy"; power-domains = <&power RK3568_PD_PIPE>; ranges = <0x00000800 0x0 0x00000000 0x3 0x00000000 0x0 0x800000 0x81000000 0x0 0x00800000 0x3 0x00800000 0x0 0x100000 0x83000000 0x0 0x00900000 0x3 0x00900000 0x0 0x3f700000>; resets = <&cru SRST_PCIE20_POWERUP>; reset-names = "pipe"; #address-cells = <3>; #size-cells = <2>; status = "disabled"; }; pcie3x1: pcie@fe270000 { compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; reg = <0x3 0xc0400000 0x0 0x400000>, <0x0 0xfe270000 0x0 0x10000>; reg-names = "pcie-dbi", "pcie-apb"; interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "sys", "pmc", "msg", "legacy", "err"; bus-range = <0x10 0x1f>; clocks = <&cru ACLK_PCIE30X1_MST>, <&cru ACLK_PCIE30X1_SLV>, <&cru ACLK_PCIE30X1_DBI>, <&cru PCLK_PCIE30X1>, <&cru CLK_PCIE30X1_AUX_NDFT>; clock-names = "aclk_mst", "aclk_slv", "aclk_dbi", "pclk", "aux"; device_type = "pci"; linux,pci-domain = <1>; num-ib-windows = <6>; num-ob-windows = <2>; max-link-speed = <3>; msi-map = <0x1000 &its 0x1000 0x1000>; num-lanes = <1>; phys = <&pcie30phy>; phy-names = "pcie-phy"; power-domains = <&power RK3568_PD_PIPE>; ranges = <0x00000800 0x0 0x40000000 0x3 0x40000000 0x0 0x800000 0x81000000 0x0 0x40800000 0x3 0x40800000 0x0 0x100000 0x83000000 0x0 0x40900000 0x3 0x40900000 0x0 0x3f700000>; resets = <&cru SRST_PCIE30X1_POWERUP>; reset-names = "pipe"; #address-cells = <3>; #size-cells = <2>; status = "disabled"; }; pcie3x2: pcie@fe280000 { compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; reg = <0x3 0xc0800000 0x0 0x400000>, <0x0 0xfe280000 0x0 0x10000>; reg-names = "pcie-dbi", "pcie-apb"; interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "sys", "pmc", "msg", "legacy", "err"; bus-range = <0x20 0x2f>; clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>, <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>, <&cru CLK_PCIE30X2_AUX_NDFT>; clock-names = "aclk_mst", "aclk_slv", "aclk_dbi", "pclk", "aux"; device_type = "pci"; linux,pci-domain = <2>; num-ib-windows = <6>; num-ob-windows = <2>; max-link-speed = <3>; msi-map = <0x2000 &its 0x2000 0x1000>; num-lanes = <2>; phys = <&pcie30phy>; phy-names = "pcie-phy"; power-domains = <&power RK3568_PD_PIPE>; ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000 0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000 0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>; resets = <&cru SRST_PCIE30X2_POWERUP>; reset-names = "pipe"; #address-cells = <3>; #size-cells = <2>; status = "disabled"; }; }; ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-20 17:07 ` Johan Jonker 0 siblings, 0 replies; 20+ messages in thread From: Johan Jonker @ 2021-01-20 17:07 UTC (permalink / raw) To: Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, devicetree, robh+dt, Heiko Stuebner, linux-rockchip Hi Simon, Thanks you for version 2. A few comments, have a look if it is useful or that you disagree. This patch has no commit message. Add one in version 3. Submit all patches in one batch with the same sort message ID to all maintainers including Heiko. Heiko Stuebner <heiko@sntech.de> Example message ID: 20210120101554.241029-1-xxm@rock-chips.com ///// Included is a copy of the Rockchip pcie nodes in a sort of test.dts below. Could you confirm that the properties in that dts are the one that we can expect for Linux mainline and can base our YAML document on? With rk3568-cru.h and rk3568-power.h manualy added we do some tests with the following commands: make ARCH=arm64 dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=~/.local/lib/python3.5/site-packages/dtschema/schemas/pci/pci-bus.yaml ///// Example notifications: /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: reg: [[3, 3225419776, 0, 4194304], [0, 4263968768, 0, 65536]] is too long /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: ranges: 'oneOf' conditional failed, one must be fixed: Before you submit version 3 make sure that all warnings gone as much as possible. On 1/20/21 11:15 AM, Simon Xue wrote: > Signed-off-by: Simon Xue <xxm@rock-chips.com> > --- > .../bindings/pci/rockchip-dw-pcie.yaml | 140 ++++++++++++++++++ > 1 file changed, 140 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > new file mode 100644 > index 000000000000..9d3a57f5305e > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > @@ -0,0 +1,140 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: DesignWare based PCIe RC controller on Rockchip SoCs > + > +maintainers: > + - Shawn Lin <shawn.lin@rock-chips.com> > + - Simon Xue <xxm@rock-chips.com> - Heiko Stuebner <heiko@sntech.de> ;) > + > +description: |+ > + RK3568 SoC PCIe host controller is based on the Synopsys DesignWare > + PCIe IP and thus inherits all the common properties defined in > + designware-pcie.txt. > + > +allOf: > + - $ref: /schemas/pci/pci-bus.yaml# > + > +# We need a select here so we don't match all nodes with 'snps,dw-pcie' > +select: > + properties: > + compatible: > + contains: > + const: rockchip,rk3568-pcie > + required: > + - compatible > + > +properties: > + compatible: > + item: items: > + - const: rockchip,rk3568-pcie > + - const: snps,dw-pcie Add empty line > + reg: items: - description: - description: Add some description for regs. > + maxItems: 1 remove This reg maxItems gives errors. > + > + interrupt: interrupts: items: > + - description: system information > + - description: power management control > + - description: PCIe message > + - description: legacy interrupt > + - description: error report > + > + interrupt-names: > + items: > + - const: sys > + - const: pmc > + - const: msg > + - const: legacy > + - const: err > + > + clocks: > + items: > + - description: AHB clock for PCIe master > + - description: AHB clock for PCIe slave > + - description: AHB clock for PCIe dbi > + - description: APB clock for PCIe > + - description: Auxiliary clock for PCIe > + > + clock-names: > + items: > + - const: aclk_mst > + - const: aclk_slv > + - const: aclk_dbi > + - const: pclk > + - const: aux > + > + msi-map: true > + > + power-domains: > + maxItems: 1 ///// These properties come from designware-pcie.txt Maybe add them here for now till there's a common yaml? num-ib-windows: number of inbound address translation windows num-ob-windows: number of outbound address translation windows Optional properties: num-lanes: ///// phys: maxItems: 1 phy-names: const: pcie-phy ranges: ...... ...... This ranges needs a fix so that it doesn't generate notifications. See above example. > + > + resets: > + maxItems: 1 > + > + reset-names: const: pipe > + items: > + - const: pipe remove > + > +required: > + - compatible > + - bus-range > + - reg > + - reg-names > + - clocks > + - clock-names > + - msi-map > + - num-lanes > + - phys > + - phy-names > + - power-domains > + - resets > + - reset-names Add also all the other properties that are defined in this binding and are required. But not the ones from pci-bus.yaml. > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/rk3568-cru.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/power/rk3568-power.h> If #include is not possible for now replace all defines by there original numbers. Just to get this binding pass for mainline. > + > + bus { > + #address-cells = <2>; > + #size-cells = <2>; > + > + pcie3x2: pcie@fe280000 { > + compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; dts sort order is: compatible reg interrupts the rest things with # > + reg = <0x3 0xc0800000 0x0 0x400000>, > + <0x0 0xfe280000 0x0 0x10000>; > + reg-names = "pcie-dbi", "pcie-apb"; > + interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "sys", "pmc", "msg", "legacy", "err"; Could you confirm that these "interrupt-names" are used in your driver? Else remove. bus-range = <0x20 0x2f>; > + clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>, > + <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>, > + <&cru CLK_PCIE30X2_AUX_NDFT>; > + clock-names = "aclk_mst", "aclk_slv", > + "aclk_dbi", "pclk", > + "aux"; device_type = "pci"; linux,pci-domain = <0>; num-ib-windows = <6>; num-ob-windows = <2>; max-link-speed = <2>; Maybe give a complete example? > + msi-map = <0x2000 &its 0x2000 0x1000>; > + num-lanes = <2>; > + phys = <&pcie30phy>; > + phy-names = "pcie-phy"; > + power-domains = <&power RK3568_PD_PIPE>; > + ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000 > + 0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000 > + 0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>; > + resets = <&cru SRST_PCIE30X2_POWERUP>; > + reset-names = "pipe"; #address-cells = <3>; #size-cells = <2>; > + }; > + }; > +... > /////////// test.dts /////////// /dts-v1/; #include <dt-bindings/clock/rk3568-cru.h> #include <dt-bindings/interrupt-controller/arm-gic.h> #include <dt-bindings/interrupt-controller/irq.h> #include <dt-bindings/phy/phy.h> #include <dt-bindings/power/rk3568-power.h> / { compatible = "rockchip,rk3568"; #address-cells = <2>; #size-cells = <2>; cru: clock-controller@fdd20000 { compatible = "rockchip,rk3568-cru"; reg = <0x0 0xfdd20000 0x0 0x1000>; #clock-cells = <1>; #reset-cells = <1>; }; pcie30phy: phy@fe8c0000 { compatible = "rockchip,rk3568-pcie3-phy"; reg = <0x0 0xfe8c0000 0x0 0x20000>; #phy-cells = <0>; }; combphy2_psq: phy@fe840000 { compatible = "rockchip,rk3568-naneng-combphy"; reg = <0x0 0xfe840000 0x0 0x100>; #phy-cells = <1>; }; gic: interrupt-controller@fd400000 { compatible = "arm,gic-v3"; #interrupt-cells = <3>; #address-cells = <2>; #size-cells = <2>; ranges; interrupt-controller; reg = <0x0 0xfd400000 0 0x10000>, <0x0 0xfd460000 0 0xc0000>; interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>; its: interrupt-controller@fd440000 { compatible = "arm,gic-v3-its"; msi-controller; #msi-cells = <1>; reg = <0x0 0xfd440000 0x0 0x20000>; status = "disabled"; }; }; pmu: power-management@fdd90000 { compatible = "rockchip,rk3568-pmu", "syscon", "simple-mfd"; reg = <0x0 0xfdd90000 0x0 0x1000>; power: power-controller { compatible = "rockchip,rk3568-power-controller"; #power-domain-cells = <1>; #address-cells = <1>; #size-cells = <0>; status = "okay"; }; }; pcie2x1: pcie@fe260000 { compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; reg = <0x3 0xc0000000 0x0 0x400000>, <0x0 0xfe260000 0x0 0x10000>; reg-names = "pcie-dbi", "pcie-apb"; interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "sys", "pmc", "msg", "legacy", "err"; bus-range = <0x0 0xf>; clocks = <&cru ACLK_PCIE20_MST>, <&cru ACLK_PCIE20_SLV>, <&cru ACLK_PCIE20_DBI>, <&cru PCLK_PCIE20>, <&cru CLK_PCIE20_AUX_NDFT>; clock-names = "aclk_mst", "aclk_slv", "aclk_dbi", "pclk", "aux"; device_type = "pci"; linux,pci-domain = <0>; num-ib-windows = <6>; num-ob-windows = <2>; max-link-speed = <2>; msi-map = <0x0 &its 0x0 0x1000>; num-lanes = <1>; phys = <&combphy2_psq PHY_TYPE_PCIE>; phy-names = "pcie-phy"; power-domains = <&power RK3568_PD_PIPE>; ranges = <0x00000800 0x0 0x00000000 0x3 0x00000000 0x0 0x800000 0x81000000 0x0 0x00800000 0x3 0x00800000 0x0 0x100000 0x83000000 0x0 0x00900000 0x3 0x00900000 0x0 0x3f700000>; resets = <&cru SRST_PCIE20_POWERUP>; reset-names = "pipe"; #address-cells = <3>; #size-cells = <2>; status = "disabled"; }; pcie3x1: pcie@fe270000 { compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; reg = <0x3 0xc0400000 0x0 0x400000>, <0x0 0xfe270000 0x0 0x10000>; reg-names = "pcie-dbi", "pcie-apb"; interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "sys", "pmc", "msg", "legacy", "err"; bus-range = <0x10 0x1f>; clocks = <&cru ACLK_PCIE30X1_MST>, <&cru ACLK_PCIE30X1_SLV>, <&cru ACLK_PCIE30X1_DBI>, <&cru PCLK_PCIE30X1>, <&cru CLK_PCIE30X1_AUX_NDFT>; clock-names = "aclk_mst", "aclk_slv", "aclk_dbi", "pclk", "aux"; device_type = "pci"; linux,pci-domain = <1>; num-ib-windows = <6>; num-ob-windows = <2>; max-link-speed = <3>; msi-map = <0x1000 &its 0x1000 0x1000>; num-lanes = <1>; phys = <&pcie30phy>; phy-names = "pcie-phy"; power-domains = <&power RK3568_PD_PIPE>; ranges = <0x00000800 0x0 0x40000000 0x3 0x40000000 0x0 0x800000 0x81000000 0x0 0x40800000 0x3 0x40800000 0x0 0x100000 0x83000000 0x0 0x40900000 0x3 0x40900000 0x0 0x3f700000>; resets = <&cru SRST_PCIE30X1_POWERUP>; reset-names = "pipe"; #address-cells = <3>; #size-cells = <2>; status = "disabled"; }; pcie3x2: pcie@fe280000 { compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; reg = <0x3 0xc0800000 0x0 0x400000>, <0x0 0xfe280000 0x0 0x10000>; reg-names = "pcie-dbi", "pcie-apb"; interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "sys", "pmc", "msg", "legacy", "err"; bus-range = <0x20 0x2f>; clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>, <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>, <&cru CLK_PCIE30X2_AUX_NDFT>; clock-names = "aclk_mst", "aclk_slv", "aclk_dbi", "pclk", "aux"; device_type = "pci"; linux,pci-domain = <2>; num-ib-windows = <6>; num-ob-windows = <2>; max-link-speed = <3>; msi-map = <0x2000 &its 0x2000 0x1000>; num-lanes = <2>; phys = <&pcie30phy>; phy-names = "pcie-phy"; power-domains = <&power RK3568_PD_PIPE>; ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000 0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000 0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>; resets = <&cru SRST_PCIE30X2_POWERUP>; reset-names = "pipe"; #address-cells = <3>; #size-cells = <2>; status = "disabled"; }; }; _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller 2021-01-20 17:07 ` Johan Jonker @ 2021-01-22 2:22 ` xxm -1 siblings, 0 replies; 20+ messages in thread From: xxm @ 2021-01-22 2:22 UTC (permalink / raw) To: Johan Jonker, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, linux-rockchip, devicetree, robh+dt, Heiko Stuebner Hi Johan, Thanks for your review, I have some questions, see inline. 在 2021/1/21 1:07, Johan Jonker 写道: > Hi Simon, > > Thanks you for version 2. > A few comments, have a look if it is useful or that you disagree. > > This patch has no commit message. Add one in version 3. > > Submit all patches in one batch with the same sort message ID to all > maintainers including Heiko. > > Heiko Stuebner <heiko@sntech.de> > > Example message ID: > 20210120101554.241029-1-xxm@rock-chips.com > > ///// > > Included is a copy of the Rockchip pcie nodes in a sort of test.dts below. > Could you confirm that the properties in that dts are the one that we > can expect for Linux mainline and can base our YAML document on? > > With rk3568-cru.h and rk3568-power.h manualy added we do some tests with > the following commands: > > make ARCH=arm64 dt_binding_check > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > make ARCH=arm64 dtbs_check > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > make ARCH=arm64 dtbs_check > DT_SCHEMA_FILES=~/.local/lib/python3.5/site-packages/dtschema/schemas/pci/pci-bus.yaml > > ///// > > Example notifications: > > /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: reg: [[3, > 3225419776, 0, 4194304], [0, 4263968768, 0, 65536]] is too long > > /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: ranges: > 'oneOf' conditional failed, one must be fixed: > > Before you submit version 3 make sure that all warnings gone as much as > possible. > > On 1/20/21 11:15 AM, Simon Xue wrote: >> Signed-off-by: Simon Xue <xxm@rock-chips.com> >> --- >> .../bindings/pci/rockchip-dw-pcie.yaml | 140 ++++++++++++++++++ >> 1 file changed, 140 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >> >> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >> new file mode 100644 >> index 000000000000..9d3a57f5305e >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >> @@ -0,0 +1,140 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: DesignWare based PCIe RC controller on Rockchip SoCs >> + >> +maintainers: >> + - Shawn Lin <shawn.lin@rock-chips.com> >> + - Simon Xue <xxm@rock-chips.com> > - Heiko Stuebner <heiko@sntech.de> ;) >> + >> +description: |+ >> + RK3568 SoC PCIe host controller is based on the Synopsys DesignWare >> + PCIe IP and thus inherits all the common properties defined in >> + designware-pcie.txt. >> + >> +allOf: >> + - $ref: /schemas/pci/pci-bus.yaml# >> + >> +# We need a select here so we don't match all nodes with 'snps,dw-pcie' >> +select: >> + properties: >> + compatible: >> + contains: >> + const: rockchip,rk3568-pcie >> + required: >> + - compatible >> + >> +properties: >> + compatible: >> + item: > items: > >> + - const: rockchip,rk3568-pcie >> + - const: snps,dw-pcie > Add empty line > >> + reg: items: > - description: > - description: > > Add some description for regs. > >> + maxItems: 1 > remove > > This reg maxItems gives errors. > >> + >> + interrupt: > interrupts: > items: > >> + - description: system information >> + - description: power management control >> + - description: PCIe message >> + - description: legacy interrupt >> + - description: error report >> + >> + interrupt-names: >> + items: >> + - const: sys >> + - const: pmc >> + - const: msg >> + - const: legacy >> + - const: err >> + >> + clocks: >> + items: >> + - description: AHB clock for PCIe master >> + - description: AHB clock for PCIe slave >> + - description: AHB clock for PCIe dbi >> + - description: APB clock for PCIe >> + - description: Auxiliary clock for PCIe >> + >> + clock-names: >> + items: >> + - const: aclk_mst >> + - const: aclk_slv >> + - const: aclk_dbi >> + - const: pclk >> + - const: aux >> + >> + msi-map: true >> + >> + power-domains: >> + maxItems: 1 > ///// > These properties come from designware-pcie.txt > Maybe add them here for now till there's a common yaml? > > num-ib-windows: number of inbound address translation windows > num-ob-windows: number of outbound address translation windows No plan to upstream EP function at the moment, I think no need to add EP's properties > Optional properties: > num-lanes: > ///// > > phys: > maxItems: 1 > > phy-names: > const: pcie-phy > > ranges: > ...... > ...... > This ranges needs a fix so that it doesn't generate notifications. > See above example. Do you mean : ranges: maxItems: 3 >> + >> + resets: >> + maxItems: 1 >> + >> + reset-names: > const: pipe > >> + items: >> + - const: pipe > remove > >> + >> +required: >> + - compatible >> + - bus-range >> + - reg >> + - reg-names >> + - clocks >> + - clock-names >> + - msi-map >> + - num-lanes >> + - phys >> + - phy-names >> + - power-domains >> + - resets >> + - reset-names > Add also all the other properties that are defined in this binding and > are required. But not the ones from pci-bus.yaml. I can't find pci-bus.yaml, do you mean Documentation/devicetree/bindings/pci/pci.txt ? > >> + >> +unevaluatedProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/rk3568-cru.h> >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + #include <dt-bindings/power/rk3568-power.h> > If #include is not possible for now replace all defines by there > original numbers. > Just to get this binding pass for mainline. Replace ACLK_PCIE30X2_MST by original number ? also the others. >> + >> + bus { >> + #address-cells = <2>; >> + #size-cells = <2>; >> + >> + pcie3x2: pcie@fe280000 { >> + compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; > dts sort order is: > > compatible > reg > interrupts > the rest > things with # > >> + reg = <0x3 0xc0800000 0x0 0x400000>, >> + <0x0 0xfe280000 0x0 0x10000>; >> + reg-names = "pcie-dbi", "pcie-apb"; >> + interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>, >> + <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>, >> + <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>, >> + <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>, >> + <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-names = "sys", "pmc", "msg", "legacy", "err"; > Could you confirm that these "interrupt-names" are used in your driver? > Else remove. Will remove > > bus-range = <0x20 0x2f>; > >> + clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>, >> + <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>, >> + <&cru CLK_PCIE30X2_AUX_NDFT>; >> + clock-names = "aclk_mst", "aclk_slv", >> + "aclk_dbi", "pclk", >> + "aux"; > device_type = "pci"; > linux,pci-domain = <0>; > num-ib-windows = <6>; > num-ob-windows = <2>; > max-link-speed = <2>; > > Maybe give a complete example? As mentioned above, no need to add EP's properties here Simon Xue. > >> + msi-map = <0x2000 &its 0x2000 0x1000>; >> + num-lanes = <2>; >> + phys = <&pcie30phy>; >> + phy-names = "pcie-phy"; >> + power-domains = <&power RK3568_PD_PIPE>; >> + ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000 >> + 0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000 >> + 0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>; >> + resets = <&cru SRST_PCIE30X2_POWERUP>; >> + reset-names = "pipe"; > #address-cells = <3>; > #size-cells = <2>; > >> + }; >> + }; >> +... >> > /////////// > > test.dts > > /////////// > /dts-v1/; > #include <dt-bindings/clock/rk3568-cru.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/interrupt-controller/irq.h> > #include <dt-bindings/phy/phy.h> > #include <dt-bindings/power/rk3568-power.h> > > / { > compatible = "rockchip,rk3568"; > > #address-cells = <2>; > #size-cells = <2>; > > cru: clock-controller@fdd20000 { > compatible = "rockchip,rk3568-cru"; > reg = <0x0 0xfdd20000 0x0 0x1000>; > #clock-cells = <1>; > #reset-cells = <1>; > }; > > pcie30phy: phy@fe8c0000 { > compatible = "rockchip,rk3568-pcie3-phy"; > reg = <0x0 0xfe8c0000 0x0 0x20000>; > #phy-cells = <0>; > }; > > combphy2_psq: phy@fe840000 { > compatible = "rockchip,rk3568-naneng-combphy"; > reg = <0x0 0xfe840000 0x0 0x100>; > #phy-cells = <1>; > }; > > gic: interrupt-controller@fd400000 { > compatible = "arm,gic-v3"; > #interrupt-cells = <3>; > #address-cells = <2>; > #size-cells = <2>; > ranges; > interrupt-controller; > > reg = <0x0 0xfd400000 0 0x10000>, > <0x0 0xfd460000 0 0xc0000>; > interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>; > its: interrupt-controller@fd440000 { > compatible = "arm,gic-v3-its"; > msi-controller; > #msi-cells = <1>; > reg = <0x0 0xfd440000 0x0 0x20000>; > status = "disabled"; > }; > }; > > pmu: power-management@fdd90000 { > compatible = "rockchip,rk3568-pmu", "syscon", "simple-mfd"; > reg = <0x0 0xfdd90000 0x0 0x1000>; > > power: power-controller { > compatible = "rockchip,rk3568-power-controller"; > #power-domain-cells = <1>; > #address-cells = <1>; > #size-cells = <0>; > status = "okay"; > }; > }; > > pcie2x1: pcie@fe260000 { > compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; > reg = <0x3 0xc0000000 0x0 0x400000>, > <0x0 0xfe260000 0x0 0x10000>; > reg-names = "pcie-dbi", "pcie-apb"; > interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>; > interrupt-names = "sys", "pmc", "msg", "legacy", "err"; > bus-range = <0x0 0xf>; > clocks = <&cru ACLK_PCIE20_MST>, <&cru ACLK_PCIE20_SLV>, > <&cru ACLK_PCIE20_DBI>, <&cru PCLK_PCIE20>, > <&cru CLK_PCIE20_AUX_NDFT>; > clock-names = "aclk_mst", "aclk_slv", > "aclk_dbi", "pclk", "aux"; > device_type = "pci"; > linux,pci-domain = <0>; > num-ib-windows = <6>; > num-ob-windows = <2>; > max-link-speed = <2>; > msi-map = <0x0 &its 0x0 0x1000>; > num-lanes = <1>; > phys = <&combphy2_psq PHY_TYPE_PCIE>; > phy-names = "pcie-phy"; > power-domains = <&power RK3568_PD_PIPE>; > ranges = <0x00000800 0x0 0x00000000 0x3 0x00000000 0x0 0x800000 > 0x81000000 0x0 0x00800000 0x3 0x00800000 0x0 0x100000 > 0x83000000 0x0 0x00900000 0x3 0x00900000 0x0 0x3f700000>; > resets = <&cru SRST_PCIE20_POWERUP>; > reset-names = "pipe"; > #address-cells = <3>; > #size-cells = <2>; > status = "disabled"; > }; > > pcie3x1: pcie@fe270000 { > compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; > reg = <0x3 0xc0400000 0x0 0x400000>, > <0x0 0xfe270000 0x0 0x10000>; > reg-names = "pcie-dbi", "pcie-apb"; > interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>; > interrupt-names = "sys", "pmc", "msg", "legacy", "err"; > bus-range = <0x10 0x1f>; > clocks = <&cru ACLK_PCIE30X1_MST>, <&cru ACLK_PCIE30X1_SLV>, > <&cru ACLK_PCIE30X1_DBI>, <&cru PCLK_PCIE30X1>, > <&cru CLK_PCIE30X1_AUX_NDFT>; > clock-names = "aclk_mst", "aclk_slv", > "aclk_dbi", "pclk", "aux"; > device_type = "pci"; > linux,pci-domain = <1>; > num-ib-windows = <6>; > num-ob-windows = <2>; > max-link-speed = <3>; > msi-map = <0x1000 &its 0x1000 0x1000>; > num-lanes = <1>; > phys = <&pcie30phy>; > phy-names = "pcie-phy"; > power-domains = <&power RK3568_PD_PIPE>; > ranges = <0x00000800 0x0 0x40000000 0x3 0x40000000 0x0 0x800000 > 0x81000000 0x0 0x40800000 0x3 0x40800000 0x0 0x100000 > 0x83000000 0x0 0x40900000 0x3 0x40900000 0x0 0x3f700000>; > resets = <&cru SRST_PCIE30X1_POWERUP>; > reset-names = "pipe"; > #address-cells = <3>; > #size-cells = <2>; > status = "disabled"; > }; > > pcie3x2: pcie@fe280000 { > compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; > reg = <0x3 0xc0800000 0x0 0x400000>, > <0x0 0xfe280000 0x0 0x10000>; > reg-names = "pcie-dbi", "pcie-apb"; > interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>; > interrupt-names = "sys", "pmc", "msg", "legacy", "err"; > bus-range = <0x20 0x2f>; > clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>, > <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>, > <&cru CLK_PCIE30X2_AUX_NDFT>; > clock-names = "aclk_mst", "aclk_slv", > "aclk_dbi", "pclk", "aux"; > device_type = "pci"; > linux,pci-domain = <2>; > num-ib-windows = <6>; > num-ob-windows = <2>; > max-link-speed = <3>; > msi-map = <0x2000 &its 0x2000 0x1000>; > num-lanes = <2>; > phys = <&pcie30phy>; > phy-names = "pcie-phy"; > power-domains = <&power RK3568_PD_PIPE>; > ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000 > 0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000 > 0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>; > resets = <&cru SRST_PCIE30X2_POWERUP>; > reset-names = "pipe"; > #address-cells = <3>; > #size-cells = <2>; > status = "disabled"; > }; > }; > > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-22 2:22 ` xxm 0 siblings, 0 replies; 20+ messages in thread From: xxm @ 2021-01-22 2:22 UTC (permalink / raw) To: Johan Jonker, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, devicetree, robh+dt, Heiko Stuebner, linux-rockchip Hi Johan, Thanks for your review, I have some questions, see inline. 在 2021/1/21 1:07, Johan Jonker 写道: > Hi Simon, > > Thanks you for version 2. > A few comments, have a look if it is useful or that you disagree. > > This patch has no commit message. Add one in version 3. > > Submit all patches in one batch with the same sort message ID to all > maintainers including Heiko. > > Heiko Stuebner <heiko@sntech.de> > > Example message ID: > 20210120101554.241029-1-xxm@rock-chips.com > > ///// > > Included is a copy of the Rockchip pcie nodes in a sort of test.dts below. > Could you confirm that the properties in that dts are the one that we > can expect for Linux mainline and can base our YAML document on? > > With rk3568-cru.h and rk3568-power.h manualy added we do some tests with > the following commands: > > make ARCH=arm64 dt_binding_check > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > make ARCH=arm64 dtbs_check > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > make ARCH=arm64 dtbs_check > DT_SCHEMA_FILES=~/.local/lib/python3.5/site-packages/dtschema/schemas/pci/pci-bus.yaml > > ///// > > Example notifications: > > /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: reg: [[3, > 3225419776, 0, 4194304], [0, 4263968768, 0, 65536]] is too long > > /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: ranges: > 'oneOf' conditional failed, one must be fixed: > > Before you submit version 3 make sure that all warnings gone as much as > possible. > > On 1/20/21 11:15 AM, Simon Xue wrote: >> Signed-off-by: Simon Xue <xxm@rock-chips.com> >> --- >> .../bindings/pci/rockchip-dw-pcie.yaml | 140 ++++++++++++++++++ >> 1 file changed, 140 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >> >> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >> new file mode 100644 >> index 000000000000..9d3a57f5305e >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml >> @@ -0,0 +1,140 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: DesignWare based PCIe RC controller on Rockchip SoCs >> + >> +maintainers: >> + - Shawn Lin <shawn.lin@rock-chips.com> >> + - Simon Xue <xxm@rock-chips.com> > - Heiko Stuebner <heiko@sntech.de> ;) >> + >> +description: |+ >> + RK3568 SoC PCIe host controller is based on the Synopsys DesignWare >> + PCIe IP and thus inherits all the common properties defined in >> + designware-pcie.txt. >> + >> +allOf: >> + - $ref: /schemas/pci/pci-bus.yaml# >> + >> +# We need a select here so we don't match all nodes with 'snps,dw-pcie' >> +select: >> + properties: >> + compatible: >> + contains: >> + const: rockchip,rk3568-pcie >> + required: >> + - compatible >> + >> +properties: >> + compatible: >> + item: > items: > >> + - const: rockchip,rk3568-pcie >> + - const: snps,dw-pcie > Add empty line > >> + reg: items: > - description: > - description: > > Add some description for regs. > >> + maxItems: 1 > remove > > This reg maxItems gives errors. > >> + >> + interrupt: > interrupts: > items: > >> + - description: system information >> + - description: power management control >> + - description: PCIe message >> + - description: legacy interrupt >> + - description: error report >> + >> + interrupt-names: >> + items: >> + - const: sys >> + - const: pmc >> + - const: msg >> + - const: legacy >> + - const: err >> + >> + clocks: >> + items: >> + - description: AHB clock for PCIe master >> + - description: AHB clock for PCIe slave >> + - description: AHB clock for PCIe dbi >> + - description: APB clock for PCIe >> + - description: Auxiliary clock for PCIe >> + >> + clock-names: >> + items: >> + - const: aclk_mst >> + - const: aclk_slv >> + - const: aclk_dbi >> + - const: pclk >> + - const: aux >> + >> + msi-map: true >> + >> + power-domains: >> + maxItems: 1 > ///// > These properties come from designware-pcie.txt > Maybe add them here for now till there's a common yaml? > > num-ib-windows: number of inbound address translation windows > num-ob-windows: number of outbound address translation windows No plan to upstream EP function at the moment, I think no need to add EP's properties > Optional properties: > num-lanes: > ///// > > phys: > maxItems: 1 > > phy-names: > const: pcie-phy > > ranges: > ...... > ...... > This ranges needs a fix so that it doesn't generate notifications. > See above example. Do you mean : ranges: maxItems: 3 >> + >> + resets: >> + maxItems: 1 >> + >> + reset-names: > const: pipe > >> + items: >> + - const: pipe > remove > >> + >> +required: >> + - compatible >> + - bus-range >> + - reg >> + - reg-names >> + - clocks >> + - clock-names >> + - msi-map >> + - num-lanes >> + - phys >> + - phy-names >> + - power-domains >> + - resets >> + - reset-names > Add also all the other properties that are defined in this binding and > are required. But not the ones from pci-bus.yaml. I can't find pci-bus.yaml, do you mean Documentation/devicetree/bindings/pci/pci.txt ? > >> + >> +unevaluatedProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/rk3568-cru.h> >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + #include <dt-bindings/power/rk3568-power.h> > If #include is not possible for now replace all defines by there > original numbers. > Just to get this binding pass for mainline. Replace ACLK_PCIE30X2_MST by original number ? also the others. >> + >> + bus { >> + #address-cells = <2>; >> + #size-cells = <2>; >> + >> + pcie3x2: pcie@fe280000 { >> + compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; > dts sort order is: > > compatible > reg > interrupts > the rest > things with # > >> + reg = <0x3 0xc0800000 0x0 0x400000>, >> + <0x0 0xfe280000 0x0 0x10000>; >> + reg-names = "pcie-dbi", "pcie-apb"; >> + interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>, >> + <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>, >> + <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>, >> + <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>, >> + <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-names = "sys", "pmc", "msg", "legacy", "err"; > Could you confirm that these "interrupt-names" are used in your driver? > Else remove. Will remove > > bus-range = <0x20 0x2f>; > >> + clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>, >> + <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>, >> + <&cru CLK_PCIE30X2_AUX_NDFT>; >> + clock-names = "aclk_mst", "aclk_slv", >> + "aclk_dbi", "pclk", >> + "aux"; > device_type = "pci"; > linux,pci-domain = <0>; > num-ib-windows = <6>; > num-ob-windows = <2>; > max-link-speed = <2>; > > Maybe give a complete example? As mentioned above, no need to add EP's properties here Simon Xue. > >> + msi-map = <0x2000 &its 0x2000 0x1000>; >> + num-lanes = <2>; >> + phys = <&pcie30phy>; >> + phy-names = "pcie-phy"; >> + power-domains = <&power RK3568_PD_PIPE>; >> + ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000 >> + 0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000 >> + 0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>; >> + resets = <&cru SRST_PCIE30X2_POWERUP>; >> + reset-names = "pipe"; > #address-cells = <3>; > #size-cells = <2>; > >> + }; >> + }; >> +... >> > /////////// > > test.dts > > /////////// > /dts-v1/; > #include <dt-bindings/clock/rk3568-cru.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/interrupt-controller/irq.h> > #include <dt-bindings/phy/phy.h> > #include <dt-bindings/power/rk3568-power.h> > > / { > compatible = "rockchip,rk3568"; > > #address-cells = <2>; > #size-cells = <2>; > > cru: clock-controller@fdd20000 { > compatible = "rockchip,rk3568-cru"; > reg = <0x0 0xfdd20000 0x0 0x1000>; > #clock-cells = <1>; > #reset-cells = <1>; > }; > > pcie30phy: phy@fe8c0000 { > compatible = "rockchip,rk3568-pcie3-phy"; > reg = <0x0 0xfe8c0000 0x0 0x20000>; > #phy-cells = <0>; > }; > > combphy2_psq: phy@fe840000 { > compatible = "rockchip,rk3568-naneng-combphy"; > reg = <0x0 0xfe840000 0x0 0x100>; > #phy-cells = <1>; > }; > > gic: interrupt-controller@fd400000 { > compatible = "arm,gic-v3"; > #interrupt-cells = <3>; > #address-cells = <2>; > #size-cells = <2>; > ranges; > interrupt-controller; > > reg = <0x0 0xfd400000 0 0x10000>, > <0x0 0xfd460000 0 0xc0000>; > interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_HIGH>; > its: interrupt-controller@fd440000 { > compatible = "arm,gic-v3-its"; > msi-controller; > #msi-cells = <1>; > reg = <0x0 0xfd440000 0x0 0x20000>; > status = "disabled"; > }; > }; > > pmu: power-management@fdd90000 { > compatible = "rockchip,rk3568-pmu", "syscon", "simple-mfd"; > reg = <0x0 0xfdd90000 0x0 0x1000>; > > power: power-controller { > compatible = "rockchip,rk3568-power-controller"; > #power-domain-cells = <1>; > #address-cells = <1>; > #size-cells = <0>; > status = "okay"; > }; > }; > > pcie2x1: pcie@fe260000 { > compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; > reg = <0x3 0xc0000000 0x0 0x400000>, > <0x0 0xfe260000 0x0 0x10000>; > reg-names = "pcie-dbi", "pcie-apb"; > interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>; > interrupt-names = "sys", "pmc", "msg", "legacy", "err"; > bus-range = <0x0 0xf>; > clocks = <&cru ACLK_PCIE20_MST>, <&cru ACLK_PCIE20_SLV>, > <&cru ACLK_PCIE20_DBI>, <&cru PCLK_PCIE20>, > <&cru CLK_PCIE20_AUX_NDFT>; > clock-names = "aclk_mst", "aclk_slv", > "aclk_dbi", "pclk", "aux"; > device_type = "pci"; > linux,pci-domain = <0>; > num-ib-windows = <6>; > num-ob-windows = <2>; > max-link-speed = <2>; > msi-map = <0x0 &its 0x0 0x1000>; > num-lanes = <1>; > phys = <&combphy2_psq PHY_TYPE_PCIE>; > phy-names = "pcie-phy"; > power-domains = <&power RK3568_PD_PIPE>; > ranges = <0x00000800 0x0 0x00000000 0x3 0x00000000 0x0 0x800000 > 0x81000000 0x0 0x00800000 0x3 0x00800000 0x0 0x100000 > 0x83000000 0x0 0x00900000 0x3 0x00900000 0x0 0x3f700000>; > resets = <&cru SRST_PCIE20_POWERUP>; > reset-names = "pipe"; > #address-cells = <3>; > #size-cells = <2>; > status = "disabled"; > }; > > pcie3x1: pcie@fe270000 { > compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; > reg = <0x3 0xc0400000 0x0 0x400000>, > <0x0 0xfe270000 0x0 0x10000>; > reg-names = "pcie-dbi", "pcie-apb"; > interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>; > interrupt-names = "sys", "pmc", "msg", "legacy", "err"; > bus-range = <0x10 0x1f>; > clocks = <&cru ACLK_PCIE30X1_MST>, <&cru ACLK_PCIE30X1_SLV>, > <&cru ACLK_PCIE30X1_DBI>, <&cru PCLK_PCIE30X1>, > <&cru CLK_PCIE30X1_AUX_NDFT>; > clock-names = "aclk_mst", "aclk_slv", > "aclk_dbi", "pclk", "aux"; > device_type = "pci"; > linux,pci-domain = <1>; > num-ib-windows = <6>; > num-ob-windows = <2>; > max-link-speed = <3>; > msi-map = <0x1000 &its 0x1000 0x1000>; > num-lanes = <1>; > phys = <&pcie30phy>; > phy-names = "pcie-phy"; > power-domains = <&power RK3568_PD_PIPE>; > ranges = <0x00000800 0x0 0x40000000 0x3 0x40000000 0x0 0x800000 > 0x81000000 0x0 0x40800000 0x3 0x40800000 0x0 0x100000 > 0x83000000 0x0 0x40900000 0x3 0x40900000 0x0 0x3f700000>; > resets = <&cru SRST_PCIE30X1_POWERUP>; > reset-names = "pipe"; > #address-cells = <3>; > #size-cells = <2>; > status = "disabled"; > }; > > pcie3x2: pcie@fe280000 { > compatible = "rockchip,rk3568-pcie", "snps,dw-pcie"; > reg = <0x3 0xc0800000 0x0 0x400000>, > <0x0 0xfe280000 0x0 0x10000>; > reg-names = "pcie-dbi", "pcie-apb"; > interrupts = <GIC_SPI 165 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 164 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 163 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 162 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 161 IRQ_TYPE_LEVEL_HIGH>; > interrupt-names = "sys", "pmc", "msg", "legacy", "err"; > bus-range = <0x20 0x2f>; > clocks = <&cru ACLK_PCIE30X2_MST>, <&cru ACLK_PCIE30X2_SLV>, > <&cru ACLK_PCIE30X2_DBI>, <&cru PCLK_PCIE30X2>, > <&cru CLK_PCIE30X2_AUX_NDFT>; > clock-names = "aclk_mst", "aclk_slv", > "aclk_dbi", "pclk", "aux"; > device_type = "pci"; > linux,pci-domain = <2>; > num-ib-windows = <6>; > num-ob-windows = <2>; > max-link-speed = <3>; > msi-map = <0x2000 &its 0x2000 0x1000>; > num-lanes = <2>; > phys = <&pcie30phy>; > phy-names = "pcie-phy"; > power-domains = <&power RK3568_PD_PIPE>; > ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000 > 0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000 > 0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>; > resets = <&cru SRST_PCIE30X2_POWERUP>; > reset-names = "pipe"; > #address-cells = <3>; > #size-cells = <2>; > status = "disabled"; > }; > }; > > > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller 2021-01-22 2:22 ` xxm @ 2021-01-22 14:02 ` Johan Jonker -1 siblings, 0 replies; 20+ messages in thread From: Johan Jonker @ 2021-01-22 14:02 UTC (permalink / raw) To: xxm, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, linux-rockchip, devicetree, robh+dt, Heiko Stuebner Hi Simon, A few comments, have a look if it is useful or that you disagree. ///// The format of ranges in your pcie node in rk3568.dtsi and your example is wrong! ranges: oneOf: - $ref: "/schemas/types.yaml#/definitions/flag" - minItems: 1 maxItems: 32 # Should be enough items: minItems: 5 maxItems: 7 additionalItems: true items: - enum: - 0x01000000 - 0x02000000 - 0x03000000 - 0x42000000 - 0x43000000 - 0x81000000 - 0x82000000 - 0x83000000 - 0xc2000000 - 0xc3000000 The array size is 3: ==> maxItems: 3 in a array a range of 5 to 7 u64 is expected. They must start with one of the enums above! yaml dt_check expects <> around every array member! Old example: ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000 0x00000800 is not in the list of above, please recheck! 0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000 0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>; New example: FICTION example with correct first element, but maybe NOT good for Rockchip pcie! ranges = <0x81000000 0x0 0x80000000 0x3 0x80000000 0x0 0x800000>, <0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000>, <0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>; Change this also in rk3568.dtsi! ///// rockchip_add_pcie_port() For FTRACE filters it is needed that all functions start with the same function prefix. Maybe use: rockchip_pcie_add_port() ///// The function prefix of pcie-dw-rockchip.c is identical with functions in pcie-rockchip-host.c Is that OK for the maintainers? ///// +static struct platform_driver rockchip_pcie_driver = { + .driver = { + .name = "rk-pcie", Maybe change to: + .name = "rockchip-dw-pcie", This name shows up in the kernel log. Could you keep it in line with other Rockchip names, so we can filter more easy? dmesg | grep rockchip rockchip-vop rochchip-drm etc. + .of_match_table = rockchip_pcie_of_match, + .suppress_bind_attrs = true, + }, + .probe = rockchip_pcie_probe, +}; ///// ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-22 14:02 ` Johan Jonker 0 siblings, 0 replies; 20+ messages in thread From: Johan Jonker @ 2021-01-22 14:02 UTC (permalink / raw) To: xxm, Bjorn Helgaas, Lorenzo Pieralisi Cc: linux-pci, devicetree, robh+dt, Heiko Stuebner, linux-rockchip Hi Simon, A few comments, have a look if it is useful or that you disagree. ///// The format of ranges in your pcie node in rk3568.dtsi and your example is wrong! ranges: oneOf: - $ref: "/schemas/types.yaml#/definitions/flag" - minItems: 1 maxItems: 32 # Should be enough items: minItems: 5 maxItems: 7 additionalItems: true items: - enum: - 0x01000000 - 0x02000000 - 0x03000000 - 0x42000000 - 0x43000000 - 0x81000000 - 0x82000000 - 0x83000000 - 0xc2000000 - 0xc3000000 The array size is 3: ==> maxItems: 3 in a array a range of 5 to 7 u64 is expected. They must start with one of the enums above! yaml dt_check expects <> around every array member! Old example: ranges = <0x00000800 0x0 0x80000000 0x3 0x80000000 0x0 0x800000 0x00000800 is not in the list of above, please recheck! 0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000 0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>; New example: FICTION example with correct first element, but maybe NOT good for Rockchip pcie! ranges = <0x81000000 0x0 0x80000000 0x3 0x80000000 0x0 0x800000>, <0x81000000 0x0 0x80800000 0x3 0x80800000 0x0 0x100000>, <0x83000000 0x0 0x80900000 0x3 0x80900000 0x0 0x3f700000>; Change this also in rk3568.dtsi! ///// rockchip_add_pcie_port() For FTRACE filters it is needed that all functions start with the same function prefix. Maybe use: rockchip_pcie_add_port() ///// The function prefix of pcie-dw-rockchip.c is identical with functions in pcie-rockchip-host.c Is that OK for the maintainers? ///// +static struct platform_driver rockchip_pcie_driver = { + .driver = { + .name = "rk-pcie", Maybe change to: + .name = "rockchip-dw-pcie", This name shows up in the kernel log. Could you keep it in line with other Rockchip names, so we can filter more easy? dmesg | grep rockchip rockchip-vop rochchip-drm etc. + .of_match_table = rockchip_pcie_of_match, + .suppress_bind_attrs = true, + }, + .probe = rockchip_pcie_probe, +}; ///// _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller 2021-01-20 17:07 ` Johan Jonker @ 2021-01-22 15:55 ` Rob Herring -1 siblings, 0 replies; 20+ messages in thread From: Rob Herring @ 2021-01-22 15:55 UTC (permalink / raw) To: Johan Jonker Cc: Simon Xue, Bjorn Helgaas, Lorenzo Pieralisi, linux-pci, linux-rockchip, devicetree, Heiko Stuebner On Wed, Jan 20, 2021 at 06:07:29PM +0100, Johan Jonker wrote: > Hi Simon, > > Thanks you for version 2. > A few comments, have a look if it is useful or that you disagree. > > This patch has no commit message. Add one in version 3. > > Submit all patches in one batch with the same sort message ID to all > maintainers including Heiko. > > Heiko Stuebner <heiko@sntech.de> > > Example message ID: > 20210120101554.241029-1-xxm@rock-chips.com > > ///// > > Included is a copy of the Rockchip pcie nodes in a sort of test.dts below. > Could you confirm that the properties in that dts are the one that we > can expect for Linux mainline and can base our YAML document on? > > With rk3568-cru.h and rk3568-power.h manualy added we do some tests with > the following commands: > > make ARCH=arm64 dt_binding_check > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > make ARCH=arm64 dtbs_check > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > make ARCH=arm64 dtbs_check > DT_SCHEMA_FILES=~/.local/lib/python3.5/site-packages/dtschema/schemas/pci/pci-bus.yaml > > ///// > > Example notifications: > > /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: reg: [[3, > 3225419776, 0, 4194304], [0, 4263968768, 0, 65536]] is too long > > /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: ranges: > 'oneOf' conditional failed, one must be fixed: > > Before you submit version 3 make sure that all warnings gone as much as > possible. > > On 1/20/21 11:15 AM, Simon Xue wrote: > > Signed-off-by: Simon Xue <xxm@rock-chips.com> > > --- > > .../bindings/pci/rockchip-dw-pcie.yaml | 140 ++++++++++++++++++ > > 1 file changed, 140 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > new file mode 100644 > > index 000000000000..9d3a57f5305e > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > @@ -0,0 +1,140 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: DesignWare based PCIe RC controller on Rockchip SoCs > > + > > +maintainers: > > + - Shawn Lin <shawn.lin@rock-chips.com> > > + - Simon Xue <xxm@rock-chips.com> > - Heiko Stuebner <heiko@sntech.de> ;) > > + > > +description: |+ > > + RK3568 SoC PCIe host controller is based on the Synopsys DesignWare > > + PCIe IP and thus inherits all the common properties defined in > > + designware-pcie.txt. > > + > > +allOf: > > + - $ref: /schemas/pci/pci-bus.yaml# > > + > > +# We need a select here so we don't match all nodes with 'snps,dw-pcie' > > +select: > > + properties: > > + compatible: > > + contains: > > + const: rockchip,rk3568-pcie > > + required: > > + - compatible > > + > > +properties: > > + compatible: > > > + item: > > items: > > > + - const: rockchip,rk3568-pcie > > + - const: snps,dw-pcie > > Add empty line > > > + reg: items: > - description: > - description: > > Add some description for regs. > > > + maxItems: 1 > remove > > This reg maxItems gives errors. > > > + > > > + interrupt: > interrupts: > items: > > > + - description: system information > > + - description: power management control > > + - description: PCIe message > > + - description: legacy interrupt > > + - description: error report > > + > > + interrupt-names: > > + items: > > + - const: sys > > + - const: pmc > > + - const: msg MSI? If so, use 'msi'. The DWC core will handle setting it up now. > > + - const: legacy > > + - const: err > > + > > + clocks: > > + items: > > + - description: AHB clock for PCIe master > > + - description: AHB clock for PCIe slave > > + - description: AHB clock for PCIe dbi > > + - description: APB clock for PCIe > > + - description: Auxiliary clock for PCIe > > + > > + clock-names: > > + items: > > + - const: aclk_mst > > + - const: aclk_slv > > + - const: aclk_dbi > > + - const: pclk > > + - const: aux > > + > > + msi-map: true > > + > > + power-domains: > > + maxItems: 1 > > ///// > These properties come from designware-pcie.txt > Maybe add them here for now till there's a common yaml? > > num-ib-windows: number of inbound address translation windows > num-ob-windows: number of outbound address translation windows These can be and are now detected at runtime. Rob ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller @ 2021-01-22 15:55 ` Rob Herring 0 siblings, 0 replies; 20+ messages in thread From: Rob Herring @ 2021-01-22 15:55 UTC (permalink / raw) To: Johan Jonker Cc: devicetree, Lorenzo Pieralisi, Simon Xue, linux-pci, linux-rockchip, Bjorn Helgaas, Heiko Stuebner On Wed, Jan 20, 2021 at 06:07:29PM +0100, Johan Jonker wrote: > Hi Simon, > > Thanks you for version 2. > A few comments, have a look if it is useful or that you disagree. > > This patch has no commit message. Add one in version 3. > > Submit all patches in one batch with the same sort message ID to all > maintainers including Heiko. > > Heiko Stuebner <heiko@sntech.de> > > Example message ID: > 20210120101554.241029-1-xxm@rock-chips.com > > ///// > > Included is a copy of the Rockchip pcie nodes in a sort of test.dts below. > Could you confirm that the properties in that dts are the one that we > can expect for Linux mainline and can base our YAML document on? > > With rk3568-cru.h and rk3568-power.h manualy added we do some tests with > the following commands: > > make ARCH=arm64 dt_binding_check > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > make ARCH=arm64 dtbs_check > DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > make ARCH=arm64 dtbs_check > DT_SCHEMA_FILES=~/.local/lib/python3.5/site-packages/dtschema/schemas/pci/pci-bus.yaml > > ///// > > Example notifications: > > /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: reg: [[3, > 3225419776, 0, 4194304], [0, 4263968768, 0, 65536]] is too long > > /arch/arm64/boot/dts/rockchip/test.dt.yaml: pcie@fe270000: ranges: > 'oneOf' conditional failed, one must be fixed: > > Before you submit version 3 make sure that all warnings gone as much as > possible. > > On 1/20/21 11:15 AM, Simon Xue wrote: > > Signed-off-by: Simon Xue <xxm@rock-chips.com> > > --- > > .../bindings/pci/rockchip-dw-pcie.yaml | 140 ++++++++++++++++++ > > 1 file changed, 140 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > new file mode 100644 > > index 000000000000..9d3a57f5305e > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > @@ -0,0 +1,140 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: DesignWare based PCIe RC controller on Rockchip SoCs > > + > > +maintainers: > > + - Shawn Lin <shawn.lin@rock-chips.com> > > + - Simon Xue <xxm@rock-chips.com> > - Heiko Stuebner <heiko@sntech.de> ;) > > + > > +description: |+ > > + RK3568 SoC PCIe host controller is based on the Synopsys DesignWare > > + PCIe IP and thus inherits all the common properties defined in > > + designware-pcie.txt. > > + > > +allOf: > > + - $ref: /schemas/pci/pci-bus.yaml# > > + > > +# We need a select here so we don't match all nodes with 'snps,dw-pcie' > > +select: > > + properties: > > + compatible: > > + contains: > > + const: rockchip,rk3568-pcie > > + required: > > + - compatible > > + > > +properties: > > + compatible: > > > + item: > > items: > > > + - const: rockchip,rk3568-pcie > > + - const: snps,dw-pcie > > Add empty line > > > + reg: items: > - description: > - description: > > Add some description for regs. > > > + maxItems: 1 > remove > > This reg maxItems gives errors. > > > + > > > + interrupt: > interrupts: > items: > > > + - description: system information > > + - description: power management control > > + - description: PCIe message > > + - description: legacy interrupt > > + - description: error report > > + > > + interrupt-names: > > + items: > > + - const: sys > > + - const: pmc > > + - const: msg MSI? If so, use 'msi'. The DWC core will handle setting it up now. > > + - const: legacy > > + - const: err > > + > > + clocks: > > + items: > > + - description: AHB clock for PCIe master > > + - description: AHB clock for PCIe slave > > + - description: AHB clock for PCIe dbi > > + - description: APB clock for PCIe > > + - description: Auxiliary clock for PCIe > > + > > + clock-names: > > + items: > > + - const: aclk_mst > > + - const: aclk_slv > > + - const: aclk_dbi > > + - const: pclk > > + - const: aux > > + > > + msi-map: true > > + > > + power-domains: > > + maxItems: 1 > > ///// > These properties come from designware-pcie.txt > Maybe add them here for now till there's a common yaml? > > num-ib-windows: number of inbound address translation windows > num-ob-windows: number of outbound address translation windows These can be and are now detected at runtime. Rob _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2021-01-22 15:56 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-20 10:16 [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller Simon Xue 2021-01-20 10:16 ` Simon Xue 2021-01-20 10:16 ` [PATCH v2 2/2] PCI: rockchip: add " Simon Xue 2021-01-20 10:16 ` Simon Xue 2021-01-20 14:55 ` Rob Herring 2021-01-20 14:55 ` Rob Herring 2021-01-21 7:57 ` [PATCH v2 2/2] PCI: rockchip: add DesignWare based PCIe controller【请注意,邮件由robherring2@gmail.com代发】 xxm 2021-01-21 7:57 ` xxm -- strict thread matches above, loose matches on Subject: below -- 2021-01-20 10:15 [PATCH v2 1/2] dt-bindings: rockchip: Add DesignWare based PCIe controller Simon Xue 2021-01-20 10:15 ` Simon Xue 2021-01-20 14:07 ` Rob Herring 2021-01-20 14:07 ` Rob Herring 2021-01-20 17:07 ` Johan Jonker 2021-01-20 17:07 ` Johan Jonker 2021-01-22 2:22 ` xxm 2021-01-22 2:22 ` xxm 2021-01-22 14:02 ` Johan Jonker 2021-01-22 14:02 ` Johan Jonker 2021-01-22 15:55 ` Rob Herring 2021-01-22 15:55 ` Rob Herring
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.