[RFC PATCH 0/6] Add SiFive FU740 PCIe host controller driver support

[RFC PATCH 0/6] Add SiFive FU740 PCIe host controller driver support

[Greentime Hu]

This patchset includes SiFive FU740 PCIe host controller driver. We also
add pcie_aux clock and pcie_power_on_reset controller to prci driver for
PCIe driver to use it.

This is tested with e1000e: Intel(R) PRO/1000 Network Card and SP M.2 PCIe
Gen 3 SSD in SiFive Unmatched.

Greentime Hu (5):
  clk: sifive: Add pcie_aux clock in prci driver for PCIe driver
  clk: sifive: Use reset-simple in prci driver for PCIe driver
  MAINTAINERS: Add maintainers for SiFive FU740 PCIe driver
  dt-bindings: PCI: Add SiFive FU740 PCIe host controller
  riscv: dts: Add PCIe support for the SiFive FU740-C000 SoC

Paul Walmsley (1):
  PCI: designware: Add SiFive FU740 PCIe host controller driver

 .../bindings/pci/sifive,fu740-pcie.yaml       | 119 +++++
 MAINTAINERS                                   |   8 +
 arch/riscv/boot/dts/sifive/fu740-c000.dtsi    |  34 ++
 drivers/clk/sifive/Kconfig                    |   2 +
 drivers/clk/sifive/fu740-prci.c               |  11 +
 drivers/clk/sifive/fu740-prci.h               |   2 +-
 drivers/clk/sifive/sifive-prci.c              |  55 +++
 drivers/clk/sifive/sifive-prci.h              |  13 +
 drivers/pci/controller/dwc/Kconfig            |   9 +
 drivers/pci/controller/dwc/Makefile           |   1 +
 drivers/pci/controller/dwc/pcie-fu740.c       | 455 ++++++++++++++++++
 drivers/reset/Kconfig                         |   3 +-
 include/dt-bindings/clock/sifive-fu740-prci.h |   1 +
 13 files changed, 711 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
 create mode 100644 drivers/pci/controller/dwc/pcie-fu740.c

-- 
2.30.0

[RFC PATCH 1/6] clk: sifive: Add pcie_aux clock in prci driver for PCIe driver

[Greentime Hu]

We add pcie_aux clock in this patch so that pcie driver can use
clk_prepare_enable() and clk_disable_unprepare() to enable and disable
pcie_aux clock.

Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
---
 drivers/clk/sifive/fu740-prci.c               | 11 +++++
 drivers/clk/sifive/fu740-prci.h               |  2 +-
 drivers/clk/sifive/sifive-prci.c              | 41 +++++++++++++++++++
 drivers/clk/sifive/sifive-prci.h              |  9 ++++
 include/dt-bindings/clock/sifive-fu740-prci.h |  1 +
 5 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/sifive/fu740-prci.c b/drivers/clk/sifive/fu740-prci.c
index 764d1097aa51..53f6e00a03b9 100644
--- a/drivers/clk/sifive/fu740-prci.c
+++ b/drivers/clk/sifive/fu740-prci.c
@@ -72,6 +72,12 @@ static const struct clk_ops sifive_fu740_prci_hfpclkplldiv_clk_ops = {
 	.recalc_rate = sifive_prci_hfpclkplldiv_recalc_rate,
 };
 
+static const struct clk_ops sifive_fu740_prci_pcie_aux_clk_ops = {
+	.enable = sifive_prci_pcie_aux_clock_enable,
+	.disable = sifive_prci_pcie_aux_clock_disable,
+	.is_enabled = sifive_prci_pcie_aux_clock_is_enabled,
+};
+
 /* List of clock controls provided by the PRCI */
 struct __prci_clock __prci_init_clocks_fu740[] = {
 	[PRCI_CLK_COREPLL] = {
@@ -120,4 +126,9 @@ struct __prci_clock __prci_init_clocks_fu740[] = {
 		.parent_name = "hfpclkpll",
 		.ops = &sifive_fu740_prci_hfpclkplldiv_clk_ops,
 	},
+	[PRCI_CLK_PCIE_AUX] = {
+		.name = "pcie_aux",
+		.parent_name = "hfclk",
+		.ops = &sifive_fu740_prci_pcie_aux_clk_ops,
+	},
 };
diff --git a/drivers/clk/sifive/fu740-prci.h b/drivers/clk/sifive/fu740-prci.h
index 13ef971f7764..511a0bf7ba2b 100644
--- a/drivers/clk/sifive/fu740-prci.h
+++ b/drivers/clk/sifive/fu740-prci.h
@@ -9,7 +9,7 @@
 
 #include "sifive-prci.h"
 
-#define NUM_CLOCK_FU740	8
+#define NUM_CLOCK_FU740	9
 
 extern struct __prci_clock __prci_init_clocks_fu740[NUM_CLOCK_FU740];
 
diff --git a/drivers/clk/sifive/sifive-prci.c b/drivers/clk/sifive/sifive-prci.c
index c78b042750e2..baf7313dac92 100644
--- a/drivers/clk/sifive/sifive-prci.c
+++ b/drivers/clk/sifive/sifive-prci.c
@@ -448,6 +448,47 @@ void sifive_prci_hfpclkpllsel_use_hfpclkpll(struct __prci_data *pd)
 	r = __prci_readl(pd, PRCI_HFPCLKPLLSEL_OFFSET);	/* barrier */
 }
 
+/* PCIE AUX clock APIs for enable, disable. */
+int sifive_prci_pcie_aux_clock_is_enabled(struct clk_hw *hw)
+{
+	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
+	struct __prci_data *pd = pc->pd;
+	u32 r;
+
+	r = __prci_readl(pd, PRCI_PCIE_AUX_OFFSET);
+
+	if (r & PRCI_PCIE_AUX_EN_MASK)
+		return 1;
+	else
+		return 0;
+}
+
+int sifive_prci_pcie_aux_clock_enable(struct clk_hw *hw)
+{
+	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
+	struct __prci_data *pd = pc->pd;
+	u32 r;
+
+	if (sifive_prci_pcie_aux_clock_is_enabled(hw))
+		return 0;
+
+	__prci_writel(1, PRCI_PCIE_AUX_OFFSET, pd);
+	r = __prci_readl(pd, PRCI_PCIE_AUX_OFFSET);	/* barrier */
+
+	return 0;
+}
+
+void sifive_prci_pcie_aux_clock_disable(struct clk_hw *hw)
+{
+	struct __prci_clock *pc = clk_hw_to_prci_clock(hw);
+	struct __prci_data *pd = pc->pd;
+	u32 r;
+
+	__prci_writel(0, PRCI_PCIE_AUX_OFFSET, pd);
+	r = __prci_readl(pd, PRCI_PCIE_AUX_OFFSET);	/* barrier */
+
+}
+
 /**
  * __prci_register_clocks() - register clock controls in the PRCI
  * @dev: Linux struct device
diff --git a/drivers/clk/sifive/sifive-prci.h b/drivers/clk/sifive/sifive-prci.h
index dbdbd1722688..022c67cf053c 100644
--- a/drivers/clk/sifive/sifive-prci.h
+++ b/drivers/clk/sifive/sifive-prci.h
@@ -67,6 +67,11 @@
 #define PRCI_DDRPLLCFG1_CKE_SHIFT	31
 #define PRCI_DDRPLLCFG1_CKE_MASK	(0x1 << PRCI_DDRPLLCFG1_CKE_SHIFT)
 
+/* PCIEAUX */
+#define PRCI_PCIE_AUX_OFFSET		0x14
+#define PRCI_PCIE_AUX_EN_SHIFT		0
+#define PRCI_PCIE_AUX_EN_MASK		(0x1 << PRCI_PCIE_AUX_EN_SHIFT)
+
 /* GEMGXLPLLCFG0 */
 #define PRCI_GEMGXLPLLCFG0_OFFSET	0x1c
 #define PRCI_GEMGXLPLLCFG0_DIVR_SHIFT	0
@@ -296,4 +301,8 @@ unsigned long sifive_prci_tlclksel_recalc_rate(struct clk_hw *hw,
 unsigned long sifive_prci_hfpclkplldiv_recalc_rate(struct clk_hw *hw,
 						   unsigned long parent_rate);
 
+int sifive_prci_pcie_aux_clock_is_enabled(struct clk_hw *hw);
+int sifive_prci_pcie_aux_clock_enable(struct clk_hw *hw);
+void sifive_prci_pcie_aux_clock_disable(struct clk_hw *hw);
+
 #endif /* __SIFIVE_CLK_SIFIVE_PRCI_H */
diff --git a/include/dt-bindings/clock/sifive-fu740-prci.h b/include/dt-bindings/clock/sifive-fu740-prci.h
index cd7706ea5677..7899b7fee7db 100644
--- a/include/dt-bindings/clock/sifive-fu740-prci.h
+++ b/include/dt-bindings/clock/sifive-fu740-prci.h
@@ -19,5 +19,6 @@
 #define PRCI_CLK_CLTXPLL	       5
 #define PRCI_CLK_TLCLK		       6
 #define PRCI_CLK_PCLK		       7
+#define PRCI_CLK_PCIE_AUX	       8
 
 #endif	/* __DT_BINDINGS_CLOCK_SIFIVE_FU740_PRCI_H */
-- 
2.30.0

[RFC PATCH 2/6] clk: sifive: Use reset-simple in prci driver for PCIe driver

[Greentime Hu]

We use reset-simple in this patch so that pcie driver can use
devm_reset_control_get() to get this reset data structure and use
reset_control_deassert() to deassert pcie_power_up_rst_n.

Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
---
 drivers/clk/sifive/Kconfig       |  2 ++
 drivers/clk/sifive/sifive-prci.c | 14 ++++++++++++++
 drivers/clk/sifive/sifive-prci.h |  4 ++++
 drivers/reset/Kconfig            |  3 ++-
 4 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/sifive/Kconfig b/drivers/clk/sifive/Kconfig
index 1c14eb20c066..9132c3c4aa86 100644
--- a/drivers/clk/sifive/Kconfig
+++ b/drivers/clk/sifive/Kconfig
@@ -10,6 +10,8 @@ if CLK_SIFIVE
 
 config CLK_SIFIVE_PRCI
 	bool "PRCI driver for SiFive SoCs"
+	select RESET_CONTROLLER
+	select RESET_SIMPLE
 	select CLK_ANALOGBITS_WRPLL_CLN28HPC
 	help
 	  Supports the Power Reset Clock interface (PRCI) IP block found in
diff --git a/drivers/clk/sifive/sifive-prci.c b/drivers/clk/sifive/sifive-prci.c
index baf7313dac92..925affc6de55 100644
--- a/drivers/clk/sifive/sifive-prci.c
+++ b/drivers/clk/sifive/sifive-prci.c
@@ -583,7 +583,21 @@ static int sifive_prci_probe(struct platform_device *pdev)
 	if (IS_ERR(pd->va))
 		return PTR_ERR(pd->va);
 
+	pd->reset.rcdev.owner = THIS_MODULE;
+	pd->reset.rcdev.nr_resets = PRCI_RST_NR;
+	pd->reset.rcdev.ops = &reset_simple_ops;
+	pd->reset.rcdev.of_node = pdev->dev.of_node;
+	pd->reset.active_low = true;
+	pd->reset.membase = pd->va + PRCI_DEVICESRESETREG_OFFSET;
+	spin_lock_init(&pd->reset.lock);
+
+	r = devm_reset_controller_register(&pdev->dev, &pd->reset.rcdev);
+	if (r) {
+		dev_err(dev, "could not register reset controller: %d\n", r);
+		return r;
+	}
 	r = __prci_register_clocks(dev, pd, desc);
+
 	if (r) {
 		dev_err(dev, "could not register clocks: %d\n", r);
 		return r;
diff --git a/drivers/clk/sifive/sifive-prci.h b/drivers/clk/sifive/sifive-prci.h
index 022c67cf053c..91658a88af4e 100644
--- a/drivers/clk/sifive/sifive-prci.h
+++ b/drivers/clk/sifive/sifive-prci.h
@@ -11,6 +11,7 @@
 
 #include <linux/clk/analogbits-wrpll-cln28hpc.h>
 #include <linux/clk-provider.h>
+#include <linux/reset/reset-simple.h>
 #include <linux/platform_device.h>
 
 /*
@@ -121,6 +122,8 @@
 #define PRCI_DEVICESRESETREG_CHIPLINK_RST_N_MASK			\
 		(0x1 << PRCI_DEVICESRESETREG_CHIPLINK_RST_N_SHIFT)
 
+#define PRCI_RST_NR						7
+
 /* CLKMUXSTATUSREG */
 #define PRCI_CLKMUXSTATUSREG_OFFSET				0x2c
 #define PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_SHIFT		1
@@ -221,6 +224,7 @@
  */
 struct __prci_data {
 	void __iomem *va;
+	struct reset_simple_data reset;
 	struct clk_hw_onecell_data hw_clks;
 };
 
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 71ab75a46491..f094df93d911 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -173,7 +173,7 @@ config RESET_SCMI
 
 config RESET_SIMPLE
 	bool "Simple Reset Controller Driver" if COMPILE_TEST
-	default ARCH_AGILEX || ARCH_ASPEED || ARCH_BITMAIN || ARCH_REALTEK || ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX || ARC
+	default ARCH_AGILEX || ARCH_ASPEED || ARCH_BITMAIN || ARCH_REALTEK || ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX || ARC || RISCV
 	help
 	  This enables a simple reset controller driver for reset lines that
 	  that can be asserted and deasserted by toggling bits in a contiguous,
@@ -187,6 +187,7 @@ config RESET_SIMPLE
 	   - RCC reset controller in STM32 MCUs
 	   - Allwinner SoCs
 	   - ZTE's zx2967 family
+	   - SiFive FU740 SoCs
 
 config RESET_STM32MP157
 	bool "STM32MP157 Reset Driver" if COMPILE_TEST
-- 
2.30.0

[RFC PATCH 3/6] MAINTAINERS: Add maintainers for SiFive FU740 PCIe driver

[Greentime Hu]

Here add maintainer information for SiFive FU740 PCIe driver.

Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index bfc1b86e3e73..4da888be6e80 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13592,6 +13592,14 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
 F:	drivers/pci/controller/dwc/*imx6*
 
+PCI DRIVER FOR FU740
+M:	Paul Walmsley <paul.walmsley@sifive.com>
+M:	Greentime Hu <greentime.hu@sifive.com>
+L:	linux-pci@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
+F:	drivers/pci/controller/dwc/pcie-fu740.c
+
 PCI DRIVER FOR INTEL VOLUME MANAGEMENT DEVICE (VMD)
 M:	Jonathan Derrick <jonathan.derrick@intel.com>
 L:	linux-pci@vger.kernel.org
-- 
2.30.0

[RFC PATCH 4/6] dt-bindings: PCI: Add SiFive FU740 PCIe host controller

[Greentime Hu]

Add PCIe host controller DT bindings of SiFive FU740.

Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
---
 .../bindings/pci/sifive,fu740-pcie.yaml       | 119 ++++++++++++++++++
 1 file changed, 119 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml

diff --git a/Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml b/Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
new file mode 100644
index 000000000000..879ab4f80456
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
@@ -0,0 +1,119 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/sifive,fu740-pcie.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SiFive fu740 PCIe host controller
+
+description: |
+  SiFive fu740 PCIe host controller is based on the Synopsys DesignWare
+  PCI core. It shares common features with the PCIe DesignWare core and
+  inherits common properties defined in
+  Documentation/devicetree/bindings/pci/designware-pcie.txt.
+
+maintainers:
+  - Paul Walmsley <paul.walmsley@sifive.com>
+  - Greentime Hu <greentime.hu@sifive.com>
+
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+
+properties:
+  compatible:
+    const: sifive,fu740-pcie
+
+  reg:
+    maxItems: 4
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: config
+      - const: mgmt
+
+  device_type:
+    const: pci
+
+  dma-coherent:
+    description: Indicates that the PCIe IP block can ensure the coherency
+
+  bus-range:
+    description: Range of bus numbers associated with this controller.
+
+  num-lanes: true
+
+  msi-parent: true
+
+  interrupt-names:
+    items:
+      - const: msi
+      - const: inta
+      - const: intb
+      - const: intc
+      - const: intd
+
+  resets:
+    description: A phandle to the PCIe power up reset line
+
+  pwren-gpios:
+    description: Should specify the GPIO for controlling the PCI bus device power on
+
+  perstn-gpios:
+    description: Should specify the GPIO for controlling the PCI bus device reset
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - device_type
+  - dma-coherent
+  - bus-range
+  - ranges
+  - num-lanes
+  - interrupts
+  - interrupt-names
+  - interrupt-parent
+  - interrupt-map-mask
+  - interrupt-map
+  - clock-names
+  - clocks
+  - resets
+  - pwren-gpios
+  - perstn-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    pcie@e00000000 {
+        #address-cells = <3>;
+        #interrupt-cells = <1>;
+        #size-cells = <2>;
+        compatible = "sifive,fu740-pcie";
+        reg = <0xe 0x00000000 0x1 0x0
+               0xd 0xf0000000 0x0 0x10000000
+               0x0 0x100d0000 0x0 0x1000>;
+        reg-names = "dbi", "config", "mgmt";
+        device_type = "pci";
+        dma-coherent;
+        bus-range = <0x0 0xff>;
+        ranges = <0x81000000  0x0 0x60080000  0x0 0x60080000 0x0 0x10000        /* I/O */
+                  0x82000000  0x0 0x60090000  0x0 0x60090000 0x0 0xff70000      /* mem */
+                  0x82000000  0x0 0x70000000  0x0 0x70000000 0x0 0x1000000      /* mem */
+                  0xc3000000 0x20 0x00000000 0x20 0x00000000 0x20 0x00000000>;  /* mem prefetchable */
+        num-lanes = <0x8>;
+        interrupts = <56 57 58 59 60 61 62 63 64>;
+        interrupt-names = "msi", "inta", "intb", "intc", "intd";
+        interrupt-parent = <&plic0>;
+        interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+        interrupt-map = <0x0 0x0 0x0 0x1 &plic0 57>,
+                        <0x0 0x0 0x0 0x2 &plic0 58>,
+                        <0x0 0x0 0x0 0x3 &plic0 59>,
+                        <0x0 0x0 0x0 0x4 &plic0 60>;
+	clock-names = "pcie_aux";
+        clocks = <&prci PRCI_CLK_PCIE_AUX>;
+        resets = <&prci 4>;
+        pwren-gpios = <&gpio 5 0>;
+        perstn-gpios = <&gpio 8 0>;
+    };
-- 
2.30.0

[RFC PATCH 5/6] PCI: designware: Add SiFive FU740 PCIe host controller driver

[Greentime Hu]

From: Paul Walmsley <paul.walmsley@sifive.com>

Add driver for the SiFive FU740 PCIe host controller.
This controller is based on the DesignWare PCIe core.

Co-developed-by: Henry Styles <hes@sifive.com>
Signed-off-by: Henry Styles <hes@sifive.com>
Co-developed-by: Erik Danie <erik.danie@sifive.com>
Signed-off-by: Erik Danie <erik.danie@sifive.com>
Co-developed-by: Greentime Hu <greentime.hu@sifive.com>
Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
---
 drivers/pci/controller/dwc/Kconfig      |   9 +
 drivers/pci/controller/dwc/Makefile     |   1 +
 drivers/pci/controller/dwc/pcie-fu740.c | 455 ++++++++++++++++++++++++
 3 files changed, 465 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-fu740.c

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 22c5529e9a65..0a37d21ed64e 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -318,4 +318,13 @@ config PCIE_AL
 	  required only for DT-based platforms. ACPI platforms with the
 	  Annapurna Labs PCIe controller don't need to enable this.
 
+config PCIE_FU740
+	bool "SiFive FU740 PCIe host controller"
+	depends on PCI_MSI_IRQ_DOMAIN
+	depends on SOC_SIFIVE || COMPILE_TEST
+	select PCIE_DW_HOST
+	help
+	  Say Y here if you want PCIe controller support for the SiFive
+	  FU740.
+
 endmenu
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index a751553fa0db..625f6aaeb5b8 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
 obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
 obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
 obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
+obj-$(CONFIG_PCIE_FU740) += pcie-fu740.o
 obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
 obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
 obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone.o
diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
new file mode 100644
index 000000000000..6916eea40ea5
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-fu740.c
@@ -0,0 +1,455 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * FU740 DesignWare PCIe Controller integration
+ * Copyright (C) 2019-2021 SiFive, Inc.
+ * Paul Walmsley
+ * Greentime Hu
+ *
+ * Based in part on the i.MX6 PCIe host controller shim which is:
+ *
+ * Copyright (C) 2013 Kosagi
+ *		https://www.kosagi.com
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/of_device.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/resource.h>
+#include <linux/signal.h>
+#include <linux/types.h>
+#include <linux/interrupt.h>
+#include <linux/reset.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
+
+#include "pcie-designware.h"
+
+#define to_fu740_pcie(x)	dev_get_drvdata((x)->dev)
+
+struct fu740_pcie {
+	struct dw_pcie *pci;
+	void __iomem *mgmt_base;
+	int perstn_gpio;
+	int pwren_gpio;
+	struct clk *pcie_aux;
+	struct reset_control *rst;
+};
+
+#define SIFIVE_DEVICESRESETREG			0x28
+
+#define PCIEX8MGMT_PERST_N			0x0
+#define PCIEX8MGMT_APP_LTSSM_ENABLE		0x10
+#define PCIEX8MGMT_APP_HOLD_PHY_RST		0x18
+#define PCIEX8MGMT_DEVICE_TYPE			0x708
+#define PCIEX8MGMT_PHY0_CR_PARA_ADDR		0x860
+#define PCIEX8MGMT_PHY0_CR_PARA_RD_EN		0x870
+#define PCIEX8MGMT_PHY0_CR_PARA_RD_DATA		0x878
+#define PCIEX8MGMT_PHY0_CR_PARA_SEL		0x880
+#define PCIEX8MGMT_PHY0_CR_PARA_WR_DATA		0x888
+#define PCIEX8MGMT_PHY0_CR_PARA_WR_EN		0x890
+#define PCIEX8MGMT_PHY0_CR_PARA_ACK		0x898
+#define PCIEX8MGMT_PHY1_CR_PARA_ADDR		0x8a0
+#define PCIEX8MGMT_PHY1_CR_PARA_RD_EN		0x8b0
+#define PCIEX8MGMT_PHY1_CR_PARA_RD_DATA		0x8b8
+#define PCIEX8MGMT_PHY1_CR_PARA_SEL		0x8c0
+#define PCIEX8MGMT_PHY1_CR_PARA_WR_DATA		0x8c8
+#define PCIEX8MGMT_PHY1_CR_PARA_WR_EN		0x8d0
+#define PCIEX8MGMT_PHY1_CR_PARA_ACK		0x8d8
+
+/* PCIe Port Logic registers (memory-mapped) */
+#define PL_OFFSET				0x700
+#define PCIE_PL_GEN2_CTRL_OFF			(PL_OFFSET + 0x10c)
+#define PCIE_PL_DIRECTED_SPEED_CHANGE_OFF	0x20000
+
+#define PCIE_PHY_MAX_RETRY_CNT			1000
+
+static void fu740_pcie_assert_perstn(struct fu740_pcie *afp)
+{
+	/* PERST_N GPIO */
+	if (gpio_is_valid(afp->perstn_gpio))
+		gpio_direction_output(afp->perstn_gpio, 0);
+
+	/* Controller PERST_N */
+	__raw_writel(0x0, afp->mgmt_base + PCIEX8MGMT_PERST_N);
+}
+
+static void fu740_pcie_deassert_perstn(struct fu740_pcie *afp)
+{
+	/* Controller PERST_N */
+	__raw_writel(0x1, afp->mgmt_base + PCIEX8MGMT_PERST_N);
+	/* PERST_N GPIO */
+	if (gpio_is_valid(afp->perstn_gpio))
+		gpio_direction_output(afp->perstn_gpio, 1);
+}
+
+static void fu740_pcie_power_on(struct fu740_pcie *afp)
+{
+	if (gpio_is_valid(afp->pwren_gpio)) {
+		gpio_direction_output(afp->pwren_gpio, 1);
+		mdelay(100);
+	}
+}
+
+static void fu740_pcie_drive_perstn(struct fu740_pcie *afp)
+{
+	fu740_pcie_assert_perstn(afp);
+	fu740_pcie_power_on(afp);
+	fu740_pcie_deassert_perstn(afp);
+}
+
+static void fu740_phyregreadwrite(const uint8_t phy, const uint8_t write,
+				const uint16_t addr,
+				const uint16_t wrdata, uint16_t *rddata,
+				struct fu740_pcie *afp)
+{
+	unsigned char ack = 0;
+	unsigned int cnt = 0;
+	struct device *dev = afp->pci->dev;
+
+	/* setup */
+	__raw_writel(addr,
+		     afp->mgmt_base +
+		     (phy ? PCIEX8MGMT_PHY1_CR_PARA_ADDR :
+		      PCIEX8MGMT_PHY0_CR_PARA_ADDR));
+	if (write)
+		__raw_writel(wrdata,
+			     afp->mgmt_base +
+			     (phy ? PCIEX8MGMT_PHY1_CR_PARA_WR_DATA :
+			      PCIEX8MGMT_PHY0_CR_PARA_WR_DATA));
+	if (write)
+		__raw_writel(1,
+			     afp->mgmt_base +
+			     (phy ? PCIEX8MGMT_PHY1_CR_PARA_WR_EN :
+			      PCIEX8MGMT_PHY0_CR_PARA_WR_EN));
+	else
+		__raw_writel(1,
+			     afp->mgmt_base +
+			     (phy ? PCIEX8MGMT_PHY1_CR_PARA_RD_EN :
+			      PCIEX8MGMT_PHY0_CR_PARA_RD_EN));
+
+	/* wait for wait_idle */
+	do {
+		if (__raw_readl
+		    (afp->mgmt_base +
+		     (phy ? PCIEX8MGMT_PHY1_CR_PARA_ACK :
+		      PCIEX8MGMT_PHY0_CR_PARA_ACK))) {
+			ack = 1;
+			if (!write)
+				__raw_readl(afp->mgmt_base +
+					    (phy ?
+					     PCIEX8MGMT_PHY1_CR_PARA_RD_DATA :
+					     PCIEX8MGMT_PHY0_CR_PARA_RD_DATA));
+		}
+	} while (!ack);
+
+	/* clear */
+	if (write)
+		__raw_writel(0,
+			     afp->mgmt_base +
+			     (phy ? PCIEX8MGMT_PHY1_CR_PARA_WR_EN :
+			      PCIEX8MGMT_PHY0_CR_PARA_WR_EN));
+	else
+		__raw_writel(0,
+			     afp->mgmt_base +
+			     (phy ? PCIEX8MGMT_PHY1_CR_PARA_RD_EN :
+			      PCIEX8MGMT_PHY0_CR_PARA_RD_EN));
+
+	/* wait for ~wait_idle */
+	while (__raw_readl
+	       (afp->mgmt_base +
+		(phy ? PCIEX8MGMT_PHY1_CR_PARA_ACK :
+		 PCIEX8MGMT_PHY0_CR_PARA_ACK))) {
+		cpu_relax();
+		cnt++;
+		if (cnt > PCIE_PHY_MAX_RETRY_CNT) {
+			dev_err(dev, "PCIE phy doesn't enter idle state.\n");
+			break;
+		}
+	}
+}
+
+static void fu740_pcie_init_phy(struct fu740_pcie *afp)
+{
+	int lane;
+
+	/* enable phy cr_para_sel interfaces */
+	__raw_writel(0x1, afp->mgmt_base + PCIEX8MGMT_PHY0_CR_PARA_SEL);
+	__raw_writel(0x1, afp->mgmt_base + PCIEX8MGMT_PHY1_CR_PARA_SEL);
+
+	/* wait 10 cr_para cycles */
+	msleep(1);
+
+	/* set PHY AC termination mode */
+	for (lane = 0; lane < 4; lane++) {
+		fu740_phyregreadwrite(0, 1,
+				     0x1008 + (0x100 * lane),
+				     0x0e21, NULL, afp);
+		fu740_phyregreadwrite(1, 1,
+				     0x1008 + (0x100 * lane),
+				     0x0e21, NULL, afp);
+	}
+
+}
+
+static void fu740_pcie_ltssm_enable(struct device *dev)
+{
+	struct fu740_pcie *afp = dev_get_drvdata(dev);
+
+	/* Enable LTSSM */
+	__raw_writel(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE);
+}
+
+static int fu740_pcie_start_link(struct dw_pcie *pci)
+{
+	struct device *dev = pci->dev;
+	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
+	u32 tmp;
+	int ret;
+
+	/*
+	 * Force Gen1 operation when starting the link.  In case the link is
+	 * started in Gen2 mode, there is a possibility the devices on the
+	 * bus will not be detected at all.  This happens with PCIe switches.
+	 */
+	dw_pcie_dbi_ro_wr_en(pci);
+	tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
+	tmp &= ~PCI_EXP_LNKCAP_SLS;
+	tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
+	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
+	dw_pcie_dbi_ro_wr_dis(pci);
+
+	/* Start LTSSM. */
+	fu740_pcie_ltssm_enable(dev);
+
+	ret = dw_pcie_wait_for_link(pci);
+	if (ret)
+		goto err_reset_phy;
+
+	/* Now set it to operate in Gen3 */
+	dw_pcie_dbi_ro_wr_en(pci);
+	tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
+	tmp &= ~PCI_EXP_LNKCAP_SLS;
+	tmp |= PCI_EXP_LNKCAP_SLS_8_0GB;
+	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
+	/* Enable DIRECTED SPEED CHANGE bit of GEN2_CTRL_OFF register */
+	tmp = dw_pcie_readl_dbi(pci, PCIE_PL_GEN2_CTRL_OFF);
+	tmp |= PCIE_PL_DIRECTED_SPEED_CHANGE_OFF;
+	dw_pcie_writel_dbi(pci, PCIE_PL_GEN2_CTRL_OFF, tmp);
+	dw_pcie_dbi_ro_wr_dis(pci);
+
+	ret = dw_pcie_wait_for_link(pci);
+	if (ret)
+		goto err_reset_phy;
+
+	/*
+	 * Reenable DIRECTED SPEED CHANGE.
+	 *
+	 * You need to set this bit after each speed change, but after
+	 * reaching G1, setting it once doesn't seem to work (it reaches G3
+	 * equalization states and then times out, falls back to G1). But
+	 * If after that, you set it again, it then reaches G3 perfectly
+	 * fine.
+	 */
+	dw_pcie_dbi_ro_wr_en(pci);
+	tmp = dw_pcie_readl_dbi(pci, PCIE_PL_GEN2_CTRL_OFF);
+	tmp |= PCIE_PL_DIRECTED_SPEED_CHANGE_OFF;
+	dw_pcie_writel_dbi(pci, PCIE_PL_GEN2_CTRL_OFF, tmp);
+	dw_pcie_dbi_ro_wr_dis(pci);
+
+	ret = dw_pcie_wait_for_link(pci);
+	if (ret)
+		goto err_reset_phy;
+
+	tmp = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
+	dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS);
+	return 0;
+
+ err_reset_phy:
+	dev_err(dev, "Failed to bring link up!\n"
+		"PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
+		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
+		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
+	return ret;
+}
+
+static int fu740_pcie_host_init(struct pcie_port *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct fu740_pcie *afp = to_fu740_pcie(pci);
+	struct device *dev = pci->dev;
+	int ret = 0;
+
+	/* power on reset */
+	fu740_pcie_drive_perstn(afp);
+
+	/* enable pcieauxclk */
+	ret = clk_prepare_enable(afp->pcie_aux);
+	if (ret)
+		dev_err(dev, "unable to enable pcie_aux clock\n");
+
+	/*
+	 * assert hold_phy_rst (hold the controller LTSSM in reset after
+	 * power_up_rst_n
+	 * for register programming with cr_para)
+	 */
+	__raw_writel(0x1, afp->mgmt_base + PCIEX8MGMT_APP_HOLD_PHY_RST);
+
+	/* deassert power_up_rst_n */
+	ret = reset_control_deassert(afp->rst);
+	if (ret)
+		dev_err(dev, "unable to deassert pcie_power_up_rst_n\n");
+
+	fu740_pcie_init_phy(afp);
+
+	/* disable pcieauxclk */
+	clk_disable_unprepare(afp->pcie_aux);
+	/* clear hold_phy_rst */
+	__raw_writel(0x0, afp->mgmt_base + PCIEX8MGMT_APP_HOLD_PHY_RST);
+	/* enable pcieauxclk */
+	ret = clk_prepare_enable(afp->pcie_aux);
+	/* set RC mode */
+	__raw_writel(0x4, afp->mgmt_base + PCIEX8MGMT_DEVICE_TYPE);
+
+	return 0;
+}
+
+static const struct dw_pcie_host_ops fu740_pcie_host_ops = {
+	.host_init = fu740_pcie_host_init,
+};
+
+static const struct dw_pcie_ops dw_pcie_ops = {
+	.start_link = fu740_pcie_start_link,
+};
+
+static const struct dev_pm_ops fu740_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(fu740_pcie_suspend_noirq,
+				      fu740_pcie_resume_noirq)
+};
+
+static int fu740_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct dw_pcie *pci;
+	struct fu740_pcie *afp;
+	struct resource *mgmt_res;
+	struct device_node *node = dev->of_node;
+	int ret;
+	u16 val;
+
+	afp = devm_kzalloc(dev, sizeof(*afp), GFP_KERNEL);
+	if (!afp)
+		return -ENOMEM;
+
+	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
+	if (!pci)
+		return -ENOMEM;
+
+	pci->dev = dev;
+	pci->ops = &dw_pcie_ops;
+	pci->pp.ops = &fu740_pcie_host_ops;
+
+	afp->pci = pci;
+
+	mgmt_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mgmt");
+	if (!mgmt_res) {
+		dev_warn(dev, "missing required mgmt address range");
+		return -ENOENT;
+	}
+	afp->mgmt_base = devm_ioremap_resource(dev, mgmt_res);
+	if (IS_ERR(afp->mgmt_base))
+		return PTR_ERR(afp->mgmt_base);
+
+	/* Fetch GPIOs */
+	afp->perstn_gpio = of_get_named_gpio(node, "perstn-gpios", 0);
+	if (gpio_is_valid(afp->perstn_gpio)) {
+		ret = devm_gpio_request_one(dev, afp->perstn_gpio,
+					    GPIOF_OUT_INIT_LOW, "perstn-gpios");
+		if (ret) {
+			dev_err(dev, "unable to get perstn gpio\n");
+			return ret;
+		}
+	} else if (afp->perstn_gpio == -EPROBE_DEFER) {
+		dev_err(dev, "perst-gpios EPROBE_DEFER\n");
+		return afp->perstn_gpio;
+	}
+
+	afp->pwren_gpio = of_get_named_gpio(node, "pwren-gpios", 0);
+	if (gpio_is_valid(afp->pwren_gpio)) {
+		ret = devm_gpio_request_one(dev, afp->pwren_gpio,
+					    GPIOF_OUT_INIT_LOW, "pwren-gpios");
+		if (ret) {
+			dev_err(dev, "unable to get pwren gpio\n");
+			return ret;
+		}
+	} else if (afp->pwren_gpio == -EPROBE_DEFER) {
+		dev_err(dev, "pwren-gpios EPROBE_DEFER\n");
+		return afp->pwren_gpio;
+	}
+
+	/* Fetch clocks */
+	afp->pcie_aux = devm_clk_get(dev, "pcie_aux");
+	if (IS_ERR(afp->pcie_aux))
+		return dev_err_probe(dev, PTR_ERR(afp->pcie_aux),
+					     "pcie_aux clock source missing or invalid\n");
+
+	/* Fetch reset */
+	afp->rst = devm_reset_control_get(dev, NULL);
+	if (IS_ERR(afp->rst))
+		return dev_err_probe(dev, PTR_ERR(afp->rst), "unable to get reset\n");
+
+	platform_set_drvdata(pdev, afp);
+
+	ret = dw_pcie_host_init(&pci->pp);
+	if (ret < 0)
+		return ret;
+
+	if (pci_msi_enabled()) {
+		u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
+
+		val = dw_pcie_readw_dbi(pci, offset + PCI_MSI_FLAGS);
+		val |= PCI_MSI_FLAGS_ENABLE;
+		dw_pcie_writew_dbi(pci, offset + PCI_MSI_FLAGS, val);
+	}
+
+	return 0;
+}
+
+static void fu740_pcie_shutdown(struct platform_device *pdev)
+{
+	struct fu740_pcie *afp = platform_get_drvdata(pdev);
+
+	/* bring down link, so bootloader gets clean state in case of reboot */
+	fu740_pcie_assert_perstn(afp);
+}
+
+static const struct of_device_id fu740_pcie_of_match[] = {
+	{.compatible = "sifive,fu740-pcie"},
+	{},
+};
+
+static struct platform_driver fu740_pcie_driver = {
+	.driver = {
+		   .name = "fu740-pcie",
+		   .of_match_table = fu740_pcie_of_match,
+		   .suppress_bind_attrs = true,
+		   .pm = &fu740_pcie_pm_ops,
+		   },
+	.probe = fu740_pcie_probe,
+	.shutdown = fu740_pcie_shutdown,
+};
+
+static int __init fu740_pcie_init(void)
+{
+	return platform_driver_register(&fu740_pcie_driver);
+}
+
+device_initcall(fu740_pcie_init);
-- 
2.30.0

[RFC PATCH 6/6] riscv: dts: Add PCIe support for the SiFive FU740-C000 SoC

[Greentime Hu]

Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
---
 arch/riscv/boot/dts/sifive/fu740-c000.dtsi | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/riscv/boot/dts/sifive/fu740-c000.dtsi b/arch/riscv/boot/dts/sifive/fu740-c000.dtsi
index d1bb22b11920..d0839739b425 100644
--- a/arch/riscv/boot/dts/sifive/fu740-c000.dtsi
+++ b/arch/riscv/boot/dts/sifive/fu740-c000.dtsi
@@ -158,6 +158,7 @@ prci: clock-controller@10000000 {
 			reg = <0x0 0x10000000 0x0 0x1000>;
 			clocks = <&hfclk>, <&rtcclk>;
 			#clock-cells = <1>;
+			#reset-cells = <1>;
 		};
 		uart0: serial@10010000 {
 			compatible = "sifive,fu740-c000-uart", "sifive,uart0";
@@ -288,5 +289,38 @@ gpio: gpio@10060000 {
 			clocks = <&prci PRCI_CLK_PCLK>;
 			status = "disabled";
 		};
+		pcie@e00000000 {
+			#address-cells = <3>;
+			#interrupt-cells = <1>;
+			#num-lanes = <8>;
+			#size-cells = <2>;
+			compatible = "sifive,fu740-pcie";
+			reg = <0xe 0x00000000 0x1 0x0
+			       0xd 0xf0000000 0x0 0x10000000
+			       0x0 0x100d0000 0x0 0x1000>;
+			reg-names = "dbi", "config", "mgmt";
+			device_type = "pci";
+			dma-coherent;
+			bus-range = <0x0 0xff>;
+			ranges = <0x81000000  0x0 0x60080000  0x0 0x60080000 0x0 0x10000        /* I/O */
+				  0x82000000  0x0 0x60090000  0x0 0x60090000 0x0 0xff70000      /* mem */
+				  0x82000000  0x0 0x70000000  0x0 0x70000000 0x0 0x1000000      /* mem */
+				  0xc3000000 0x20 0x00000000 0x20 0x00000000 0x20 0x00000000>;  /* mem prefetchable */
+			num-lanes = <0x8>;
+			interrupts = <56 57 58 59 60 61 62 63 64>;
+			interrupt-names = "msi", "inta", "intb", "intc", "intd";
+			interrupt-parent = <&plic0>;
+			interrupt-map-mask = <0x0 0x0 0x0 0x7>;
+			interrupt-map = <0x0 0x0 0x0 0x1 &plic0 57>,
+					<0x0 0x0 0x0 0x2 &plic0 58>,
+					<0x0 0x0 0x0 0x3 &plic0 59>,
+					<0x0 0x0 0x0 0x4 &plic0 60>;
+			clock-names = "pcie_aux";
+			clocks = <&prci PRCI_CLK_PCIE_AUX>;
+			pwren-gpios = <&gpio 5 0>;
+			perstn-gpios = <&gpio 8 0>;
+			resets = <&prci 4>;
+			status = "okay";
+		};
 	};
 };
-- 
2.30.0

Re: [RFC PATCH 4/6] dt-bindings: PCI: Add SiFive FU740 PCIe host controller

[Rob Herring]

On Tue, 02 Mar 2021 18:59:15 +0800, Greentime Hu wrote:
> Add PCIe host controller DT bindings of SiFive FU740.
> 
> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> ---
>  .../bindings/pci/sifive,fu740-pcie.yaml       | 119 ++++++++++++++++++
>  1 file changed, 119 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
> 

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

yamllint warnings/errors:
./Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml:114:1: [error] syntax error: found character '\t' that cannot start any token (syntax)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/pci/sifive,fu740-pcie.example.dts'
Traceback (most recent call last):
  File "/usr/local/bin/dt-extract-example", line 45, in <module>
    binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 343, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 111, in get_single_data
    node = self.composer.get_single_node()
  File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node
  File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 773, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 848, in _ruamel_yaml.CParser._compose_sequence_node
  File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event
ruamel.yaml.scanner.ScannerError: while scanning a block scalar
  in "<unicode string>", line 88, column 5
found a tab character where an indentation space is expected
  in "<unicode string>", line 114, column 1
make[1]: *** [Documentation/devicetree/bindings/Makefile:20: Documentation/devicetree/bindings/pci/sifive,fu740-pcie.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml:  while scanning a block scalar
  in "<unicode string>", line 88, column 5
found a tab character where an indentation space is expected
  in "<unicode string>", line 114, column 1
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
make: *** [Makefile:1380: dt_binding_check] Error 2

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

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.

Re: [RFC PATCH 5/6] PCI: designware: Add SiFive FU740 PCIe host controller driver

[Rob Herring]

On Tue, Mar 2, 2021 at 4:59 AM Greentime Hu <greentime.hu@sifive.com> wrote:
>
> From: Paul Walmsley <paul.walmsley@sifive.com>
>
> Add driver for the SiFive FU740 PCIe host controller.
> This controller is based on the DesignWare PCIe core.
>
> Co-developed-by: Henry Styles <hes@sifive.com>
> Signed-off-by: Henry Styles <hes@sifive.com>
> Co-developed-by: Erik Danie <erik.danie@sifive.com>
> Signed-off-by: Erik Danie <erik.danie@sifive.com>
> Co-developed-by: Greentime Hu <greentime.hu@sifive.com>
> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>

Your Sob should be last since you are sending the patch.

> Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>

And Paul's either before yours or first in the list depending on the history.

> ---
>  drivers/pci/controller/dwc/Kconfig      |   9 +
>  drivers/pci/controller/dwc/Makefile     |   1 +
>  drivers/pci/controller/dwc/pcie-fu740.c | 455 ++++++++++++++++++++++++
>  3 files changed, 465 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-fu740.c
>
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 22c5529e9a65..0a37d21ed64e 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -318,4 +318,13 @@ config PCIE_AL
>           required only for DT-based platforms. ACPI platforms with the
>           Annapurna Labs PCIe controller don't need to enable this.
>
> +config PCIE_FU740
> +       bool "SiFive FU740 PCIe host controller"
> +       depends on PCI_MSI_IRQ_DOMAIN
> +       depends on SOC_SIFIVE || COMPILE_TEST
> +       select PCIE_DW_HOST
> +       help
> +         Say Y here if you want PCIe controller support for the SiFive
> +         FU740.
> +
>  endmenu
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index a751553fa0db..625f6aaeb5b8 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
>  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
>  obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>  obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
> +obj-$(CONFIG_PCIE_FU740) += pcie-fu740.o
>  obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
>  obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
>  obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone.o
> diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
> new file mode 100644
> index 000000000000..6916eea40ea5
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-fu740.c
> @@ -0,0 +1,455 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * FU740 DesignWare PCIe Controller integration
> + * Copyright (C) 2019-2021 SiFive, Inc.
> + * Paul Walmsley
> + * Greentime Hu
> + *
> + * Based in part on the i.MX6 PCIe host controller shim which is:
> + *
> + * Copyright (C) 2013 Kosagi
> + *             https://www.kosagi.com
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_device.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/resource.h>
> +#include <linux/signal.h>
> +#include <linux/types.h>
> +#include <linux/interrupt.h>
> +#include <linux/reset.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +
> +#include "pcie-designware.h"
> +
> +#define to_fu740_pcie(x)       dev_get_drvdata((x)->dev)
> +
> +struct fu740_pcie {
> +       struct dw_pcie *pci;
> +       void __iomem *mgmt_base;
> +       int perstn_gpio;
> +       int pwren_gpio;

Use gpio descriptors and the gpiod_* api.

> +       struct clk *pcie_aux;
> +       struct reset_control *rst;
> +};
> +
> +#define SIFIVE_DEVICESRESETREG                 0x28
> +
> +#define PCIEX8MGMT_PERST_N                     0x0
> +#define PCIEX8MGMT_APP_LTSSM_ENABLE            0x10
> +#define PCIEX8MGMT_APP_HOLD_PHY_RST            0x18
> +#define PCIEX8MGMT_DEVICE_TYPE                 0x708
> +#define PCIEX8MGMT_PHY0_CR_PARA_ADDR           0x860
> +#define PCIEX8MGMT_PHY0_CR_PARA_RD_EN          0x870
> +#define PCIEX8MGMT_PHY0_CR_PARA_RD_DATA                0x878
> +#define PCIEX8MGMT_PHY0_CR_PARA_SEL            0x880
> +#define PCIEX8MGMT_PHY0_CR_PARA_WR_DATA                0x888
> +#define PCIEX8MGMT_PHY0_CR_PARA_WR_EN          0x890
> +#define PCIEX8MGMT_PHY0_CR_PARA_ACK            0x898
> +#define PCIEX8MGMT_PHY1_CR_PARA_ADDR           0x8a0
> +#define PCIEX8MGMT_PHY1_CR_PARA_RD_EN          0x8b0
> +#define PCIEX8MGMT_PHY1_CR_PARA_RD_DATA                0x8b8
> +#define PCIEX8MGMT_PHY1_CR_PARA_SEL            0x8c0
> +#define PCIEX8MGMT_PHY1_CR_PARA_WR_DATA                0x8c8
> +#define PCIEX8MGMT_PHY1_CR_PARA_WR_EN          0x8d0
> +#define PCIEX8MGMT_PHY1_CR_PARA_ACK            0x8d8

I tend to think we should split the phy part to a proper phy driver.
Just because the registers happen to be in the same register space
isn't really a good reason to combine them.

> +
> +/* PCIe Port Logic registers (memory-mapped) */
> +#define PL_OFFSET                              0x700

That's all DWC controllers.

> +#define PCIE_PL_GEN2_CTRL_OFF                  (PL_OFFSET + 0x10c)

Also common. Pretty sure there's a define already.

> +#define PCIE_PL_DIRECTED_SPEED_CHANGE_OFF      0x20000
> +
> +#define PCIE_PHY_MAX_RETRY_CNT                 1000
> +
> +static void fu740_pcie_assert_perstn(struct fu740_pcie *afp)
> +{
> +       /* PERST_N GPIO */
> +       if (gpio_is_valid(afp->perstn_gpio))
> +               gpio_direction_output(afp->perstn_gpio, 0);
> +
> +       /* Controller PERST_N */
> +       __raw_writel(0x0, afp->mgmt_base + PCIEX8MGMT_PERST_N);

writel_relaxed is the preferred variant.

> +}
> +
> +static void fu740_pcie_deassert_perstn(struct fu740_pcie *afp)
> +{
> +       /* Controller PERST_N */
> +       __raw_writel(0x1, afp->mgmt_base + PCIEX8MGMT_PERST_N);
> +       /* PERST_N GPIO */
> +       if (gpio_is_valid(afp->perstn_gpio))
> +               gpio_direction_output(afp->perstn_gpio, 1);
> +}
> +
> +static void fu740_pcie_power_on(struct fu740_pcie *afp)
> +{
> +       if (gpio_is_valid(afp->pwren_gpio)) {
> +               gpio_direction_output(afp->pwren_gpio, 1);
> +               mdelay(100);
> +       }
> +}
> +
> +static void fu740_pcie_drive_perstn(struct fu740_pcie *afp)
> +{
> +       fu740_pcie_assert_perstn(afp);
> +       fu740_pcie_power_on(afp);
> +       fu740_pcie_deassert_perstn(afp);
> +}
> +
> +static void fu740_phyregreadwrite(const uint8_t phy, const uint8_t write,
> +                               const uint16_t addr,
> +                               const uint16_t wrdata, uint16_t *rddata,
> +                               struct fu740_pcie *afp)

I think you should split this into separate read and write functions.

> +{
> +       unsigned char ack = 0;
> +       unsigned int cnt = 0;
> +       struct device *dev = afp->pci->dev;
> +
> +       /* setup */
> +       __raw_writel(addr,
> +                    afp->mgmt_base +
> +                    (phy ? PCIEX8MGMT_PHY1_CR_PARA_ADDR :
> +                     PCIEX8MGMT_PHY0_CR_PARA_ADDR));
> +       if (write)
> +               __raw_writel(wrdata,
> +                            afp->mgmt_base +
> +                            (phy ? PCIEX8MGMT_PHY1_CR_PARA_WR_DATA :
> +                             PCIEX8MGMT_PHY0_CR_PARA_WR_DATA));
> +       if (write)
> +               __raw_writel(1,
> +                            afp->mgmt_base +
> +                            (phy ? PCIEX8MGMT_PHY1_CR_PARA_WR_EN :
> +                             PCIEX8MGMT_PHY0_CR_PARA_WR_EN));
> +       else
> +               __raw_writel(1,
> +                            afp->mgmt_base +
> +                            (phy ? PCIEX8MGMT_PHY1_CR_PARA_RD_EN :
> +                             PCIEX8MGMT_PHY0_CR_PARA_RD_EN));
> +
> +       /* wait for wait_idle */
> +       do {
> +               if (__raw_readl
> +                   (afp->mgmt_base +
> +                    (phy ? PCIEX8MGMT_PHY1_CR_PARA_ACK :
> +                     PCIEX8MGMT_PHY0_CR_PARA_ACK))) {
> +                       ack = 1;
> +                       if (!write)
> +                               __raw_readl(afp->mgmt_base +
> +                                           (phy ?
> +                                            PCIEX8MGMT_PHY1_CR_PARA_RD_DATA :
> +                                            PCIEX8MGMT_PHY0_CR_PARA_RD_DATA));
> +               }
> +       } while (!ack);
> +
> +       /* clear */
> +       if (write)
> +               __raw_writel(0,
> +                            afp->mgmt_base +
> +                            (phy ? PCIEX8MGMT_PHY1_CR_PARA_WR_EN :
> +                             PCIEX8MGMT_PHY0_CR_PARA_WR_EN));
> +       else
> +               __raw_writel(0,
> +                            afp->mgmt_base +
> +                            (phy ? PCIEX8MGMT_PHY1_CR_PARA_RD_EN :
> +                             PCIEX8MGMT_PHY0_CR_PARA_RD_EN));
> +
> +       /* wait for ~wait_idle */
> +       while (__raw_readl
> +              (afp->mgmt_base +
> +               (phy ? PCIEX8MGMT_PHY1_CR_PARA_ACK :
> +                PCIEX8MGMT_PHY0_CR_PARA_ACK))) {
> +               cpu_relax();
> +               cnt++;
> +               if (cnt > PCIE_PHY_MAX_RETRY_CNT) {
> +                       dev_err(dev, "PCIE phy doesn't enter idle state.\n");
> +                       break;
> +               }
> +       }

We have helpers for this kind of loop.

> +}
> +
> +static void fu740_pcie_init_phy(struct fu740_pcie *afp)
> +{
> +       int lane;
> +
> +       /* enable phy cr_para_sel interfaces */
> +       __raw_writel(0x1, afp->mgmt_base + PCIEX8MGMT_PHY0_CR_PARA_SEL);
> +       __raw_writel(0x1, afp->mgmt_base + PCIEX8MGMT_PHY1_CR_PARA_SEL);
> +
> +       /* wait 10 cr_para cycles */
> +       msleep(1);
> +
> +       /* set PHY AC termination mode */
> +       for (lane = 0; lane < 4; lane++) {
> +               fu740_phyregreadwrite(0, 1,
> +                                    0x1008 + (0x100 * lane),
> +                                    0x0e21, NULL, afp);
> +               fu740_phyregreadwrite(1, 1,
> +                                    0x1008 + (0x100 * lane),
> +                                    0x0e21, NULL, afp);
> +       }
> +
> +}
> +
> +static void fu740_pcie_ltssm_enable(struct device *dev)
> +{
> +       struct fu740_pcie *afp = dev_get_drvdata(dev);
> +
> +       /* Enable LTSSM */
> +       __raw_writel(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE);
> +}
> +
> +static int fu740_pcie_start_link(struct dw_pcie *pci)
> +{
> +       struct device *dev = pci->dev;
> +       u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +       u32 tmp;
> +       int ret;
> +
> +       /*
> +        * Force Gen1 operation when starting the link.  In case the link is
> +        * started in Gen2 mode, there is a possibility the devices on the
> +        * bus will not be detected at all.  This happens with PCIe switches.
> +        */
> +       dw_pcie_dbi_ro_wr_en(pci);
> +       tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> +       tmp &= ~PCI_EXP_LNKCAP_SLS;
> +       tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
> +       dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
> +       dw_pcie_dbi_ro_wr_dis(pci);
> +
> +       /* Start LTSSM. */
> +       fu740_pcie_ltssm_enable(dev);
> +
> +       ret = dw_pcie_wait_for_link(pci);
> +       if (ret)
> +               goto err_reset_phy;
> +
> +       /* Now set it to operate in Gen3 */
> +       dw_pcie_dbi_ro_wr_en(pci);
> +       tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> +       tmp &= ~PCI_EXP_LNKCAP_SLS;
> +       tmp |= PCI_EXP_LNKCAP_SLS_8_0GB;

You don't support slower speeds?

We have some common DWC handling of this too, that doesn't work?

> +       dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
> +       /* Enable DIRECTED SPEED CHANGE bit of GEN2_CTRL_OFF register */
> +       tmp = dw_pcie_readl_dbi(pci, PCIE_PL_GEN2_CTRL_OFF);
> +       tmp |= PCIE_PL_DIRECTED_SPEED_CHANGE_OFF;
> +       dw_pcie_writel_dbi(pci, PCIE_PL_GEN2_CTRL_OFF, tmp);
> +       dw_pcie_dbi_ro_wr_dis(pci);
> +
> +       ret = dw_pcie_wait_for_link(pci);
> +       if (ret)
> +               goto err_reset_phy;
> +
> +       /*
> +        * Reenable DIRECTED SPEED CHANGE.
> +        *
> +        * You need to set this bit after each speed change, but after
> +        * reaching G1, setting it once doesn't seem to work (it reaches G3
> +        * equalization states and then times out, falls back to G1). But
> +        * If after that, you set it again, it then reaches G3 perfectly
> +        * fine.
> +        */
> +       dw_pcie_dbi_ro_wr_en(pci);
> +       tmp = dw_pcie_readl_dbi(pci, PCIE_PL_GEN2_CTRL_OFF);
> +       tmp |= PCIE_PL_DIRECTED_SPEED_CHANGE_OFF;
> +       dw_pcie_writel_dbi(pci, PCIE_PL_GEN2_CTRL_OFF, tmp);
> +       dw_pcie_dbi_ro_wr_dis(pci);
> +
> +       ret = dw_pcie_wait_for_link(pci);

The DWC core does this after start_link() now.

> +       if (ret)
> +               goto err_reset_phy;
> +
> +       tmp = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
> +       dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS);

If we want this, it should be common.

> +       return 0;
> +
> + err_reset_phy:
> +       dev_err(dev, "Failed to bring link up!\n"
> +               "PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
> +               dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> +               dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> +       return ret;
> +}
> +
> +static int fu740_pcie_host_init(struct pcie_port *pp)
> +{
> +       struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +       struct fu740_pcie *afp = to_fu740_pcie(pci);
> +       struct device *dev = pci->dev;
> +       int ret = 0;
> +
> +       /* power on reset */
> +       fu740_pcie_drive_perstn(afp);
> +
> +       /* enable pcieauxclk */
> +       ret = clk_prepare_enable(afp->pcie_aux);
> +       if (ret)
> +               dev_err(dev, "unable to enable pcie_aux clock\n");
> +
> +       /*
> +        * assert hold_phy_rst (hold the controller LTSSM in reset after
> +        * power_up_rst_n
> +        * for register programming with cr_para)
> +        */
> +       __raw_writel(0x1, afp->mgmt_base + PCIEX8MGMT_APP_HOLD_PHY_RST);
> +
> +       /* deassert power_up_rst_n */
> +       ret = reset_control_deassert(afp->rst);
> +       if (ret)
> +               dev_err(dev, "unable to deassert pcie_power_up_rst_n\n");
> +
> +       fu740_pcie_init_phy(afp);
> +
> +       /* disable pcieauxclk */
> +       clk_disable_unprepare(afp->pcie_aux);
> +       /* clear hold_phy_rst */
> +       __raw_writel(0x0, afp->mgmt_base + PCIEX8MGMT_APP_HOLD_PHY_RST);
> +       /* enable pcieauxclk */
> +       ret = clk_prepare_enable(afp->pcie_aux);
> +       /* set RC mode */
> +       __raw_writel(0x4, afp->mgmt_base + PCIEX8MGMT_DEVICE_TYPE);
> +
> +       return 0;
> +}
> +
> +static const struct dw_pcie_host_ops fu740_pcie_host_ops = {
> +       .host_init = fu740_pcie_host_init,
> +};
> +
> +static const struct dw_pcie_ops dw_pcie_ops = {
> +       .start_link = fu740_pcie_start_link,
> +};
> +
> +static const struct dev_pm_ops fu740_pcie_pm_ops = {
> +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(fu740_pcie_suspend_noirq,
> +                                     fu740_pcie_resume_noirq)
> +};
> +
> +static int fu740_pcie_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct dw_pcie *pci;
> +       struct fu740_pcie *afp;
> +       struct resource *mgmt_res;
> +       struct device_node *node = dev->of_node;
> +       int ret;
> +       u16 val;
> +
> +       afp = devm_kzalloc(dev, sizeof(*afp), GFP_KERNEL);
> +       if (!afp)
> +               return -ENOMEM;
> +
> +       pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);

Embed dw_pcie in fu740_pcie and do 1 alloc.

> +       if (!pci)
> +               return -ENOMEM;
> +
> +       pci->dev = dev;
> +       pci->ops = &dw_pcie_ops;
> +       pci->pp.ops = &fu740_pcie_host_ops;
> +
> +       afp->pci = pci;
> +
> +       mgmt_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mgmt");
> +       if (!mgmt_res) {
> +               dev_warn(dev, "missing required mgmt address range");
> +               return -ENOENT;
> +       }
> +       afp->mgmt_base = devm_ioremap_resource(dev, mgmt_res);

Use devm_platform_ioremap_resource_byname


> +       if (IS_ERR(afp->mgmt_base))
> +               return PTR_ERR(afp->mgmt_base);
> +
> +       /* Fetch GPIOs */
> +       afp->perstn_gpio = of_get_named_gpio(node, "perstn-gpios", 0);
> +       if (gpio_is_valid(afp->perstn_gpio)) {
> +               ret = devm_gpio_request_one(dev, afp->perstn_gpio,
> +                                           GPIOF_OUT_INIT_LOW, "perstn-gpios");
> +               if (ret) {
> +                       dev_err(dev, "unable to get perstn gpio\n");
> +                       return ret;
> +               }
> +       } else if (afp->perstn_gpio == -EPROBE_DEFER) {
> +               dev_err(dev, "perst-gpios EPROBE_DEFER\n");
> +               return afp->perstn_gpio;
> +       }

GPIOs are optional? That's not what the binding says.

> +
> +       afp->pwren_gpio = of_get_named_gpio(node, "pwren-gpios", 0);
> +       if (gpio_is_valid(afp->pwren_gpio)) {
> +               ret = devm_gpio_request_one(dev, afp->pwren_gpio,
> +                                           GPIOF_OUT_INIT_LOW, "pwren-gpios");
> +               if (ret) {
> +                       dev_err(dev, "unable to get pwren gpio\n");
> +                       return ret;
> +               }
> +       } else if (afp->pwren_gpio == -EPROBE_DEFER) {
> +               dev_err(dev, "pwren-gpios EPROBE_DEFER\n");
> +               return afp->pwren_gpio;
> +       }
> +
> +       /* Fetch clocks */
> +       afp->pcie_aux = devm_clk_get(dev, "pcie_aux");
> +       if (IS_ERR(afp->pcie_aux))
> +               return dev_err_probe(dev, PTR_ERR(afp->pcie_aux),
> +                                            "pcie_aux clock source missing or invalid\n");
> +
> +       /* Fetch reset */
> +       afp->rst = devm_reset_control_get(dev, NULL);
> +       if (IS_ERR(afp->rst))
> +               return dev_err_probe(dev, PTR_ERR(afp->rst), "unable to get reset\n");
> +
> +       platform_set_drvdata(pdev, afp);
> +
> +       ret = dw_pcie_host_init(&pci->pp);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (pci_msi_enabled()) {
> +               u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> +
> +               val = dw_pcie_readw_dbi(pci, offset + PCI_MSI_FLAGS);
> +               val |= PCI_MSI_FLAGS_ENABLE;
> +               dw_pcie_writew_dbi(pci, offset + PCI_MSI_FLAGS, val);

Are you sure this is needed? It's suspect that only i.MX6 needs it.

> +       }
> +
> +       return 0;
> +}
> +
> +static void fu740_pcie_shutdown(struct platform_device *pdev)
> +{
> +       struct fu740_pcie *afp = platform_get_drvdata(pdev);
> +
> +       /* bring down link, so bootloader gets clean state in case of reboot */
> +       fu740_pcie_assert_perstn(afp);
> +}
> +
> +static const struct of_device_id fu740_pcie_of_match[] = {
> +       {.compatible = "sifive,fu740-pcie"},
> +       {},
> +};
> +
> +static struct platform_driver fu740_pcie_driver = {
> +       .driver = {
> +                  .name = "fu740-pcie",
> +                  .of_match_table = fu740_pcie_of_match,
> +                  .suppress_bind_attrs = true,
> +                  .pm = &fu740_pcie_pm_ops,
> +                  },
> +       .probe = fu740_pcie_probe,
> +       .shutdown = fu740_pcie_shutdown,
> +};
> +
> +static int __init fu740_pcie_init(void)
> +{
> +       return platform_driver_register(&fu740_pcie_driver);
> +}
> +
> +device_initcall(fu740_pcie_init);

Needs to be early? Why?

Re: [RFC PATCH 2/6] clk: sifive: Use reset-simple in prci driver for PCIe driver

[Philipp Zabel]

On Tue, 2021-03-02 at 18:59 +0800, Greentime Hu wrote:
> We use reset-simple in this patch so that pcie driver can use
> devm_reset_control_get() to get this reset data structure and use
> reset_control_deassert() to deassert pcie_power_up_rst_n.
> 
> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> ---
>  drivers/clk/sifive/Kconfig       |  2 ++
>  drivers/clk/sifive/sifive-prci.c | 14 ++++++++++++++
>  drivers/clk/sifive/sifive-prci.h |  4 ++++
>  drivers/reset/Kconfig            |  3 ++-
>  4 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/sifive/Kconfig b/drivers/clk/sifive/Kconfig
> index 1c14eb20c066..9132c3c4aa86 100644
> --- a/drivers/clk/sifive/Kconfig
> +++ b/drivers/clk/sifive/Kconfig
> @@ -10,6 +10,8 @@ if CLK_SIFIVE
>  
>  config CLK_SIFIVE_PRCI
>  	bool "PRCI driver for SiFive SoCs"
> +	select RESET_CONTROLLER
> +	select RESET_SIMPLE
>  	select CLK_ANALOGBITS_WRPLL_CLN28HPC
>  	help
>  	  Supports the Power Reset Clock interface (PRCI) IP block found in
> diff --git a/drivers/clk/sifive/sifive-prci.c b/drivers/clk/sifive/sifive-prci.c
> index baf7313dac92..925affc6de55 100644
> --- a/drivers/clk/sifive/sifive-prci.c
> +++ b/drivers/clk/sifive/sifive-prci.c
> @@ -583,7 +583,21 @@ static int sifive_prci_probe(struct platform_device *pdev)
>  	if (IS_ERR(pd->va))
>  		return PTR_ERR(pd->va);
>  
> +	pd->reset.rcdev.owner = THIS_MODULE;
> +	pd->reset.rcdev.nr_resets = PRCI_RST_NR;
> +	pd->reset.rcdev.ops = &reset_simple_ops;
> +	pd->reset.rcdev.of_node = pdev->dev.of_node;
> +	pd->reset.active_low = true;
> +	pd->reset.membase = pd->va + PRCI_DEVICESRESETREG_OFFSET;
> +	spin_lock_init(&pd->reset.lock);
> +
> +	r = devm_reset_controller_register(&pdev->dev, &pd->reset.rcdev);
> +	if (r) {
> +		dev_err(dev, "could not register reset controller: %d\n", r);
> +		return r;
> +	}
>  	r = __prci_register_clocks(dev, pd, desc);
> +

Accidental whitespace?

Otherwise,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [RFC PATCH 5/6] PCI: designware: Add SiFive FU740 PCIe host controller driver

[Philipp Zabel]

On Tue, 2021-03-02 at 18:59 +0800, Greentime Hu wrote:
[...]
> +static int fu740_pcie_probe(struct platform_device *pdev)
> +{
[...]
> +       /* Fetch reset */
> +       afp->rst = devm_reset_control_get(dev, NULL);

Please use
       afp->rst = devm_reset_control_get_exclusive(dev, NULL);	

regards
Philipp

Re: [RFC PATCH 5/6] PCI: designware: Add SiFive FU740 PCIe host controller driver

[Bjorn Helgaas]

Make the subject like this:

  PCI: fu740: Add SiFive FU740 PCIe host controller driver

since you're adding a "fu740" driver, not a "designware" driver.
Future commits will then look like:

  PCI: fu740: ...

On Tue, Mar 02, 2021 at 06:59:16PM +0800, Greentime Hu wrote:
> From: Paul Walmsley <paul.walmsley@sifive.com>
> 
> Add driver for the SiFive FU740 PCIe host controller.
> This controller is based on the DesignWare PCIe core.
> 
> Co-developed-by: Henry Styles <hes@sifive.com>
> Signed-off-by: Henry Styles <hes@sifive.com>
> Co-developed-by: Erik Danie <erik.danie@sifive.com>
> Signed-off-by: Erik Danie <erik.danie@sifive.com>
> Co-developed-by: Greentime Hu <greentime.hu@sifive.com>
> Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> ---
>  drivers/pci/controller/dwc/Kconfig      |   9 +
>  drivers/pci/controller/dwc/Makefile     |   1 +
>  drivers/pci/controller/dwc/pcie-fu740.c | 455 ++++++++++++++++++++++++
>  3 files changed, 465 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-fu740.c
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 22c5529e9a65..0a37d21ed64e 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -318,4 +318,13 @@ config PCIE_AL
>  	  required only for DT-based platforms. ACPI platforms with the
>  	  Annapurna Labs PCIe controller don't need to enable this.
>  
> +config PCIE_FU740
> +	bool "SiFive FU740 PCIe host controller"
> +	depends on PCI_MSI_IRQ_DOMAIN
> +	depends on SOC_SIFIVE || COMPILE_TEST
> +	select PCIE_DW_HOST
> +	help
> +	  Say Y here if you want PCIe controller support for the SiFive
> +	  FU740.
> +
>  endmenu
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index a751553fa0db..625f6aaeb5b8 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
>  obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
>  obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
>  obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
> +obj-$(CONFIG_PCIE_FU740) += pcie-fu740.o
>  obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
>  obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o
>  obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone.o
> diff --git a/drivers/pci/controller/dwc/pcie-fu740.c b/drivers/pci/controller/dwc/pcie-fu740.c
> new file mode 100644
> index 000000000000..6916eea40ea5
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-fu740.c
> @@ -0,0 +1,455 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * FU740 DesignWare PCIe Controller integration
> + * Copyright (C) 2019-2021 SiFive, Inc.
> + * Paul Walmsley
> + * Greentime Hu
> + *
> + * Based in part on the i.MX6 PCIe host controller shim which is:
> + *
> + * Copyright (C) 2013 Kosagi
> + *		https://www.kosagi.com
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_device.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/resource.h>
> +#include <linux/signal.h>
> +#include <linux/types.h>
> +#include <linux/interrupt.h>
> +#include <linux/reset.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +
> +#include "pcie-designware.h"
> +
> +#define to_fu740_pcie(x)	dev_get_drvdata((x)->dev)
> +
> +struct fu740_pcie {
> +	struct dw_pcie *pci;
> +	void __iomem *mgmt_base;
> +	int perstn_gpio;
> +	int pwren_gpio;
> +	struct clk *pcie_aux;
> +	struct reset_control *rst;
> +};
> +
> +#define SIFIVE_DEVICESRESETREG			0x28
> +
> +#define PCIEX8MGMT_PERST_N			0x0
> +#define PCIEX8MGMT_APP_LTSSM_ENABLE		0x10
> +#define PCIEX8MGMT_APP_HOLD_PHY_RST		0x18
> +#define PCIEX8MGMT_DEVICE_TYPE			0x708
> +#define PCIEX8MGMT_PHY0_CR_PARA_ADDR		0x860
> +#define PCIEX8MGMT_PHY0_CR_PARA_RD_EN		0x870
> +#define PCIEX8MGMT_PHY0_CR_PARA_RD_DATA		0x878
> +#define PCIEX8MGMT_PHY0_CR_PARA_SEL		0x880
> +#define PCIEX8MGMT_PHY0_CR_PARA_WR_DATA		0x888
> +#define PCIEX8MGMT_PHY0_CR_PARA_WR_EN		0x890
> +#define PCIEX8MGMT_PHY0_CR_PARA_ACK		0x898
> +#define PCIEX8MGMT_PHY1_CR_PARA_ADDR		0x8a0
> +#define PCIEX8MGMT_PHY1_CR_PARA_RD_EN		0x8b0
> +#define PCIEX8MGMT_PHY1_CR_PARA_RD_DATA		0x8b8
> +#define PCIEX8MGMT_PHY1_CR_PARA_SEL		0x8c0
> +#define PCIEX8MGMT_PHY1_CR_PARA_WR_DATA		0x8c8
> +#define PCIEX8MGMT_PHY1_CR_PARA_WR_EN		0x8d0
> +#define PCIEX8MGMT_PHY1_CR_PARA_ACK		0x8d8
> +
> +/* PCIe Port Logic registers (memory-mapped) */
> +#define PL_OFFSET				0x700
> +#define PCIE_PL_GEN2_CTRL_OFF			(PL_OFFSET + 0x10c)
> +#define PCIE_PL_DIRECTED_SPEED_CHANGE_OFF	0x20000
> +
> +#define PCIE_PHY_MAX_RETRY_CNT			1000
> +
> +static void fu740_pcie_assert_perstn(struct fu740_pcie *afp)
> +{
> +	/* PERST_N GPIO */
> +	if (gpio_is_valid(afp->perstn_gpio))
> +		gpio_direction_output(afp->perstn_gpio, 0);
> +
> +	/* Controller PERST_N */
> +	__raw_writel(0x0, afp->mgmt_base + PCIEX8MGMT_PERST_N);
> +}
> +
> +static void fu740_pcie_deassert_perstn(struct fu740_pcie *afp)
> +{
> +	/* Controller PERST_N */
> +	__raw_writel(0x1, afp->mgmt_base + PCIEX8MGMT_PERST_N);
> +	/* PERST_N GPIO */
> +	if (gpio_is_valid(afp->perstn_gpio))
> +		gpio_direction_output(afp->perstn_gpio, 1);
> +}
> +
> +static void fu740_pcie_power_on(struct fu740_pcie *afp)
> +{
> +	if (gpio_is_valid(afp->pwren_gpio)) {
> +		gpio_direction_output(afp->pwren_gpio, 1);
> +		mdelay(100);

Can you add a note about where the 100ms value comes from?

> +	}
> +}
> +
> +static void fu740_pcie_drive_perstn(struct fu740_pcie *afp)
> +{
> +	fu740_pcie_assert_perstn(afp);
> +	fu740_pcie_power_on(afp);
> +	fu740_pcie_deassert_perstn(afp);
> +}
> +
> +static void fu740_phyregreadwrite(const uint8_t phy, const uint8_t write,
> +				const uint16_t addr,
> +				const uint16_t wrdata, uint16_t *rddata,
> +				struct fu740_pcie *afp)
> +{
> +	unsigned char ack = 0;
> +	unsigned int cnt = 0;
> +	struct device *dev = afp->pci->dev;
> +
> +	/* setup */

Most of your single-line comments start with a capital letter.  It's
nice if they all use the same style.

> +	__raw_writel(addr,
> +		     afp->mgmt_base +
> +		     (phy ? PCIEX8MGMT_PHY1_CR_PARA_ADDR :
> +		      PCIEX8MGMT_PHY0_CR_PARA_ADDR));

All these "phy ? x : y" expressions are really ugly and maybe could be
factored out ahead of time.

> +	if (write)
> +		__raw_writel(wrdata,
> +			     afp->mgmt_base +
> +			     (phy ? PCIEX8MGMT_PHY1_CR_PARA_WR_DATA :
> +			      PCIEX8MGMT_PHY0_CR_PARA_WR_DATA));
> +	if (write)
> +		__raw_writel(1,
> +			     afp->mgmt_base +
> +			     (phy ? PCIEX8MGMT_PHY1_CR_PARA_WR_EN :
> +			      PCIEX8MGMT_PHY0_CR_PARA_WR_EN));
> +	else
> +		__raw_writel(1,
> +			     afp->mgmt_base +
> +			     (phy ? PCIEX8MGMT_PHY1_CR_PARA_RD_EN :
> +			      PCIEX8MGMT_PHY0_CR_PARA_RD_EN));
> +
> +	/* wait for wait_idle */
> +	do {
> +		if (__raw_readl
> +		    (afp->mgmt_base +
> +		     (phy ? PCIEX8MGMT_PHY1_CR_PARA_ACK :
> +		      PCIEX8MGMT_PHY0_CR_PARA_ACK))) {
> +			ack = 1;
> +			if (!write)
> +				__raw_readl(afp->mgmt_base +
> +					    (phy ?
> +					     PCIEX8MGMT_PHY1_CR_PARA_RD_DATA :
> +					     PCIEX8MGMT_PHY0_CR_PARA_RD_DATA));
> +		}
> +	} while (!ack);

Possible infinite loop.  We should try to avoid depending on hardware
doing the right thing.  If the hardware breaks or is removed, we don't
want to hang.

> +	/* clear */
> +	if (write)
> +		__raw_writel(0,
> +			     afp->mgmt_base +
> +			     (phy ? PCIEX8MGMT_PHY1_CR_PARA_WR_EN :
> +			      PCIEX8MGMT_PHY0_CR_PARA_WR_EN));
> +	else
> +		__raw_writel(0,
> +			     afp->mgmt_base +
> +			     (phy ? PCIEX8MGMT_PHY1_CR_PARA_RD_EN :
> +			      PCIEX8MGMT_PHY0_CR_PARA_RD_EN));
> +
> +	/* wait for ~wait_idle */
> +	while (__raw_readl
> +	       (afp->mgmt_base +
> +		(phy ? PCIEX8MGMT_PHY1_CR_PARA_ACK :
> +		 PCIEX8MGMT_PHY0_CR_PARA_ACK))) {
> +		cpu_relax();
> +		cnt++;
> +		if (cnt > PCIE_PHY_MAX_RETRY_CNT) {
> +			dev_err(dev, "PCIE phy doesn't enter idle state.\n");
> +			break;
> +		}
> +	}
> +}
> +
> +static void fu740_pcie_init_phy(struct fu740_pcie *afp)
> +{
> +	int lane;
> +
> +	/* enable phy cr_para_sel interfaces */
> +	__raw_writel(0x1, afp->mgmt_base + PCIEX8MGMT_PHY0_CR_PARA_SEL);
> +	__raw_writel(0x1, afp->mgmt_base + PCIEX8MGMT_PHY1_CR_PARA_SEL);
> +
> +	/* wait 10 cr_para cycles */
> +	msleep(1);

A reference to the spec section that requires the 1ms would be
helpful.

> +	/* set PHY AC termination mode */
> +	for (lane = 0; lane < 4; lane++) {
> +		fu740_phyregreadwrite(0, 1,
> +				     0x1008 + (0x100 * lane),
> +				     0x0e21, NULL, afp);
> +		fu740_phyregreadwrite(1, 1,
> +				     0x1008 + (0x100 * lane),
> +				     0x0e21, NULL, afp);

Rewrap to use more of the 80-column line.  Each will fit on two
lines.

Maybe add #defines for the 0x1008, 0x100, 0x0e21 magic numbers?  Could
compute the "0x1008 + (0x100 * lane)" expression once instead of
repeating it.

> +	}
> +
> +}
> +
> +static void fu740_pcie_ltssm_enable(struct device *dev)
> +{
> +	struct fu740_pcie *afp = dev_get_drvdata(dev);
> +
> +	/* Enable LTSSM */
> +	__raw_writel(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE);
> +}
> +
> +static int fu740_pcie_start_link(struct dw_pcie *pci)
> +{
> +	struct device *dev = pci->dev;
> +	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	u32 tmp;
> +	int ret;
> +
> +	/*
> +	 * Force Gen1 operation when starting the link.  In case the link is
> +	 * started in Gen2 mode, there is a possibility the devices on the
> +	 * bus will not be detected at all.  This happens with PCIe switches.
> +	 */
> +	dw_pcie_dbi_ro_wr_en(pci);
> +	tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> +	tmp &= ~PCI_EXP_LNKCAP_SLS;
> +	tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;
> +	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +
> +	/* Start LTSSM. */
> +	fu740_pcie_ltssm_enable(dev);
> +
> +	ret = dw_pcie_wait_for_link(pci);
> +	if (ret)
> +		goto err_reset_phy;
> +
> +	/* Now set it to operate in Gen3 */
> +	dw_pcie_dbi_ro_wr_en(pci);
> +	tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);
> +	tmp &= ~PCI_EXP_LNKCAP_SLS;
> +	tmp |= PCI_EXP_LNKCAP_SLS_8_0GB;
> +	dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);
> +	/* Enable DIRECTED SPEED CHANGE bit of GEN2_CTRL_OFF register */
> +	tmp = dw_pcie_readl_dbi(pci, PCIE_PL_GEN2_CTRL_OFF);
> +	tmp |= PCIE_PL_DIRECTED_SPEED_CHANGE_OFF;
> +	dw_pcie_writel_dbi(pci, PCIE_PL_GEN2_CTRL_OFF, tmp);
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +
> +	ret = dw_pcie_wait_for_link(pci);
> +	if (ret)
> +		goto err_reset_phy;
> +
> +	/*
> +	 * Reenable DIRECTED SPEED CHANGE.
> +	 *
> +	 * You need to set this bit after each speed change, but after
> +	 * reaching G1, setting it once doesn't seem to work (it reaches G3
> +	 * equalization states and then times out, falls back to G1). But
> +	 * If after that, you set it again, it then reaches G3 perfectly

s/If after/if after/

> +	 * fine.
> +	 */
> +	dw_pcie_dbi_ro_wr_en(pci);
> +	tmp = dw_pcie_readl_dbi(pci, PCIE_PL_GEN2_CTRL_OFF);
> +	tmp |= PCIE_PL_DIRECTED_SPEED_CHANGE_OFF;
> +	dw_pcie_writel_dbi(pci, PCIE_PL_GEN2_CTRL_OFF, tmp);
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +
> +	ret = dw_pcie_wait_for_link(pci);
> +	if (ret)
> +		goto err_reset_phy;
> +
> +	tmp = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA);
> +	dev_info(dev, "Link up, Gen%i\n", tmp & PCI_EXP_LNKSTA_CLS);
> +	return 0;
> +
> + err_reset_phy:
> +	dev_err(dev, "Failed to bring link up!\n"
> +		"PHY DEBUG_R0=0x%08x DEBUG_R1=0x%08x\n",
> +		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0),
> +		dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1));
> +	return ret;
> +}
> +
> +static int fu740_pcie_host_init(struct pcie_port *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct fu740_pcie *afp = to_fu740_pcie(pci);
> +	struct device *dev = pci->dev;
> +	int ret = 0;

Unnecessary init of "ret".

> +
> +	/* power on reset */
> +	fu740_pcie_drive_perstn(afp);
> +
> +	/* enable pcieauxclk */
> +	ret = clk_prepare_enable(afp->pcie_aux);
> +	if (ret)
> +		dev_err(dev, "unable to enable pcie_aux clock\n");
> +
> +	/*
> +	 * assert hold_phy_rst (hold the controller LTSSM in reset after
> +	 * power_up_rst_n
> +	 * for register programming with cr_para)

Rewrap text to fill lines.

> +	 */
> +	__raw_writel(0x1, afp->mgmt_base + PCIEX8MGMT_APP_HOLD_PHY_RST);
> +
> +	/* deassert power_up_rst_n */
> +	ret = reset_control_deassert(afp->rst);
> +	if (ret)
> +		dev_err(dev, "unable to deassert pcie_power_up_rst_n\n");
> +
> +	fu740_pcie_init_phy(afp);
> +
> +	/* disable pcieauxclk */
> +	clk_disable_unprepare(afp->pcie_aux);
> +	/* clear hold_phy_rst */
> +	__raw_writel(0x0, afp->mgmt_base + PCIEX8MGMT_APP_HOLD_PHY_RST);
> +	/* enable pcieauxclk */
> +	ret = clk_prepare_enable(afp->pcie_aux);
> +	/* set RC mode */
> +	__raw_writel(0x4, afp->mgmt_base + PCIEX8MGMT_DEVICE_TYPE);
> +
> +	return 0;
> +}
> +
> +static const struct dw_pcie_host_ops fu740_pcie_host_ops = {
> +	.host_init = fu740_pcie_host_init,
> +};
> +
> +static const struct dw_pcie_ops dw_pcie_ops = {
> +	.start_link = fu740_pcie_start_link,
> +};
> +
> +static const struct dev_pm_ops fu740_pcie_pm_ops = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(fu740_pcie_suspend_noirq,
> +				      fu740_pcie_resume_noirq)

fu740_pcie_suspend_noirq() and fu740_pcie_resume_noirq() don't exist.
Am I missing something?

> +};
> +
> +static int fu740_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct dw_pcie *pci;
> +	struct fu740_pcie *afp;
> +	struct resource *mgmt_res;
> +	struct device_node *node = dev->of_node;
> +	int ret;
> +	u16 val;
> +
> +	afp = devm_kzalloc(dev, sizeof(*afp), GFP_KERNEL);
> +	if (!afp)
> +		return -ENOMEM;
> +
> +	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> +	if (!pci)
> +		return -ENOMEM;
> +
> +	pci->dev = dev;
> +	pci->ops = &dw_pcie_ops;
> +	pci->pp.ops = &fu740_pcie_host_ops;
> +
> +	afp->pci = pci;
> +
> +	mgmt_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mgmt");

Does every driver make up a new name for this register space?  I see
"mem", "cfg", "app", "dbics", "regs", "config", "ctrl", "dbi", "dbi2",
"addr_space", "atu", "cs", "csr", breg", "cfg", "pcireg", etc.  I know
there *are* different kinds of registers that need different names,
but I don't believe there are *this many* different kinds.  It'd be
nice if different drivers could use the same name for the same
registers.

> +	if (!mgmt_res) {
> +		dev_warn(dev, "missing required mgmt address range");
> +		return -ENOENT;
> +	}
> +	afp->mgmt_base = devm_ioremap_resource(dev, mgmt_res);
> +	if (IS_ERR(afp->mgmt_base))
> +		return PTR_ERR(afp->mgmt_base);
> +
> +	/* Fetch GPIOs */
> +	afp->perstn_gpio = of_get_named_gpio(node, "perstn-gpios", 0);
> +	if (gpio_is_valid(afp->perstn_gpio)) {
> +		ret = devm_gpio_request_one(dev, afp->perstn_gpio,
> +					    GPIOF_OUT_INIT_LOW, "perstn-gpios");
> +		if (ret) {
> +			dev_err(dev, "unable to get perstn gpio\n");

Maybe make the message match the DT property, i.e., "perstn-gpios"?

> +			return ret;
> +		}
> +	} else if (afp->perstn_gpio == -EPROBE_DEFER) {
> +		dev_err(dev, "perst-gpios EPROBE_DEFER\n");
> +		return afp->perstn_gpio;
> +	}

This structure is a little awkward, partly because it makes the implicit
assumption that -EPROBE_DEFER is not a valid GPIO number.  I think
this structure would be better:

  afp->perstn_gpio = of_get_named_gpio(...);
  if (afp->perstn_gpio == -EPROBE_DEFER)
    return perstn_gpio;

  if (gpio_is_valid(afp->perstn_gpio)) {
    ret = devm_gpio_request_one(...);
    if (ret)
      return ret;
  }

Also, similar code in mvebu_pcie_parse_port() suggests that
devm_gpio_request_one() itself can return -EPROBE_DEFER, which seems
to be true.  I have no idea whether you want to do anything special
with -EPROBE_DEFER in that case here.

> +	afp->pwren_gpio = of_get_named_gpio(node, "pwren-gpios", 0);
> +	if (gpio_is_valid(afp->pwren_gpio)) {
> +		ret = devm_gpio_request_one(dev, afp->pwren_gpio,
> +					    GPIOF_OUT_INIT_LOW, "pwren-gpios");
> +		if (ret) {
> +			dev_err(dev, "unable to get pwren gpio\n");
> +			return ret;
> +		}
> +	} else if (afp->pwren_gpio == -EPROBE_DEFER) {
> +		dev_err(dev, "pwren-gpios EPROBE_DEFER\n");
> +		return afp->pwren_gpio;
> +	}
> +
> +	/* Fetch clocks */
> +	afp->pcie_aux = devm_clk_get(dev, "pcie_aux");
> +	if (IS_ERR(afp->pcie_aux))
> +		return dev_err_probe(dev, PTR_ERR(afp->pcie_aux),
> +					     "pcie_aux clock source missing or invalid\n");
> +
> +	/* Fetch reset */
> +	afp->rst = devm_reset_control_get(dev, NULL);
> +	if (IS_ERR(afp->rst))
> +		return dev_err_probe(dev, PTR_ERR(afp->rst), "unable to get reset\n");
> +
> +	platform_set_drvdata(pdev, afp);
> +
> +	ret = dw_pcie_host_init(&pci->pp);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (pci_msi_enabled()) {
> +		u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
> +
> +		val = dw_pcie_readw_dbi(pci, offset + PCI_MSI_FLAGS);
> +		val |= PCI_MSI_FLAGS_ENABLE;
> +		dw_pcie_writew_dbi(pci, offset + PCI_MSI_FLAGS, val);
> +	}
> +
> +	return 0;
> +}
> +
> +static void fu740_pcie_shutdown(struct platform_device *pdev)
> +{
> +	struct fu740_pcie *afp = platform_get_drvdata(pdev);
> +
> +	/* bring down link, so bootloader gets clean state in case of reboot */
> +	fu740_pcie_assert_perstn(afp);
> +}
> +
> +static const struct of_device_id fu740_pcie_of_match[] = {
> +	{.compatible = "sifive,fu740-pcie"},

Typically there are a couple spaces inserted here, i.e.,

  { .compatible = "sifive,fu740-pcie", },

> +	{},
> +};
> +
> +static struct platform_driver fu740_pcie_driver = {
> +	.driver = {
> +		   .name = "fu740-pcie",
> +		   .of_match_table = fu740_pcie_of_match,
> +		   .suppress_bind_attrs = true,
> +		   .pm = &fu740_pcie_pm_ops,
> +		   },

Typical indentation would remove a tab before the "}".  Match other
drivers.

> +	.probe = fu740_pcie_probe,
> +	.shutdown = fu740_pcie_shutdown,
> +};
> +
> +static int __init fu740_pcie_init(void)
> +{
> +	return platform_driver_register(&fu740_pcie_driver);
> +}
> +
> +device_initcall(fu740_pcie_init);

pci-imx6.c is the only other driver that uses device_initcall().

Use builtin_platform_driver() or similar if possible, as other drivers
do.

Re: [RFC PATCH 2/6] clk: sifive: Use reset-simple in prci driver for PCIe driver

[Greentime Hu]

Philipp Zabel <p.zabel@pengutronix.de> 於 2021年3月4日 週四 下午7:58寫道：
>
> On Tue, 2021-03-02 at 18:59 +0800, Greentime Hu wrote:
> > We use reset-simple in this patch so that pcie driver can use
> > devm_reset_control_get() to get this reset data structure and use
> > reset_control_deassert() to deassert pcie_power_up_rst_n.
> >
> > Signed-off-by: Greentime Hu <greentime.hu@sifive.com>
> > ---
> >  drivers/clk/sifive/Kconfig       |  2 ++
> >  drivers/clk/sifive/sifive-prci.c | 14 ++++++++++++++
> >  drivers/clk/sifive/sifive-prci.h |  4 ++++
> >  drivers/reset/Kconfig            |  3 ++-
> >  4 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/sifive/Kconfig b/drivers/clk/sifive/Kconfig
> > index 1c14eb20c066..9132c3c4aa86 100644
> > --- a/drivers/clk/sifive/Kconfig
> > +++ b/drivers/clk/sifive/Kconfig
> > @@ -10,6 +10,8 @@ if CLK_SIFIVE
> >
> >  config CLK_SIFIVE_PRCI
> >       bool "PRCI driver for SiFive SoCs"
> > +     select RESET_CONTROLLER
> > +     select RESET_SIMPLE
> >       select CLK_ANALOGBITS_WRPLL_CLN28HPC
> >       help
> >         Supports the Power Reset Clock interface (PRCI) IP block found in
> > diff --git a/drivers/clk/sifive/sifive-prci.c b/drivers/clk/sifive/sifive-prci.c
> > index baf7313dac92..925affc6de55 100644
> > --- a/drivers/clk/sifive/sifive-prci.c
> > +++ b/drivers/clk/sifive/sifive-prci.c
> > @@ -583,7 +583,21 @@ static int sifive_prci_probe(struct platform_device *pdev)
> >       if (IS_ERR(pd->va))
> >               return PTR_ERR(pd->va);
> >
> > +     pd->reset.rcdev.owner = THIS_MODULE;
> > +     pd->reset.rcdev.nr_resets = PRCI_RST_NR;
> > +     pd->reset.rcdev.ops = &reset_simple_ops;
> > +     pd->reset.rcdev.of_node = pdev->dev.of_node;
> > +     pd->reset.active_low = true;
> > +     pd->reset.membase = pd->va + PRCI_DEVICESRESETREG_OFFSET;
> > +     spin_lock_init(&pd->reset.lock);
> > +
> > +     r = devm_reset_controller_register(&pdev->dev, &pd->reset.rcdev);
> > +     if (r) {
> > +             dev_err(dev, "could not register reset controller: %d\n", r);
> > +             return r;
> > +     }
> >       r = __prci_register_clocks(dev, pd, desc);
> > +
>
> Accidental whitespace?
>
> Otherwise,
>
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

Thank you, Philipp.
Yes, it is an accidental whitespace. I'll remove it in my next version patch.