Skip to contentSkip to conversation

Add SiFive FU740 PCIe host controller driver support

RFC PATCH Superseded Greentime Hu 5 participants 13 messages Mar 2, 2021 – Mar 9, 2021

Subsystems (9)

v1 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Greentime Hu

author maintainer
#0 lore raw

cover Add SiFive FU740 PCIe host controller driver support

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
Signature · 1 line
2.30.0
author maintainer
#1 lore raw

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

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.

---
 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(-)
drivers/clk/sifive/fu740-prci.c +11 -0
index 764d1097aa51..53f6e00a03b9 100644
@@ -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,
+	},
 };
drivers/clk/sifive/fu740-prci.h +1 -1
index 13ef971f7764..511a0bf7ba2b 100644
@@ -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];
 
drivers/clk/sifive/sifive-prci.c +41 -0

47 lines not on this page · raw message

drivers/clk/sifive/sifive-prci.h +9 -0
index dbdbd1722688..022c67cf053c 100644
@@ -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 */
include/dt-bindings/clock/sifive-fu740-prci.h +1 -0
index cd7706ea5677..7899b7fee7db 100644
@@ -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 */
Signature · 1 line
2.30.0
author maintainer
#2 lore raw

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

In reply to 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.

---
 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(-)
drivers/clk/sifive/Kconfig +2 -0
index 1c14eb20c066..9132c3c4aa86 100644
@@ -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
drivers/clk/sifive/sifive-prci.c +14 -0
index baf7313dac92..925affc6de55 100644
@@ -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;
drivers/clk/sifive/sifive-prci.h +4 -0
index 022c67cf053c..91658a88af4e 100644
@@ -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;
 };
 
drivers/reset/Kconfig +2 -1
index 71ab75a46491..f094df93d911 100644
@@ -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
Signature · 1 line
2.30.0
maintainer Reviewed-by on v1 2/6
#3 lore raw

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

Quoted 38 lines · 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.

---
 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(-)
drivers/clk/sifive/Kconfig +2 -0
index 1c14eb20c066..9132c3c4aa86 100644
@@ -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
drivers/clk/sifive/sifive-prci.c +5 -0
index baf7313dac92..925affc6de55 100644
@@ -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;

drivers/clk/sifive/sifive-prci.c static int sifive_prci_probe(struct platform_device *pdev) · line 600

Quoted 10 lines
drivers/clk/sifive/sifive-prci.c +9 -0
+	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,

regards
Philipp

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

author maintainer
#4 lore raw

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

Philipp Zabel <[email protected]> 於 2021年3月4日 週四 下午7:58寫道:

Quoted 53 lines · Greentime Hu
Quoted 38 lines

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.

---
 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(-)
drivers/clk/sifive/Kconfig +2 -0
index 1c14eb20c066..9132c3c4aa86 100644
@@ -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
drivers/clk/sifive/sifive-prci.c +5 -0
index baf7313dac92..925affc6de55 100644
@@ -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;

drivers/clk/sifive/sifive-prci.c static int sifive_prci_probe(struct platform_device *pdev) · line 600

Quoted 10 lines
drivers/clk/sifive/sifive-prci.c +9 -0
+     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?

Quoted 3 lines

Otherwise,

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

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

author maintainer
#5 lore raw

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

In reply to Greentime Hu

Here add maintainer information for SiFive FU740 PCIe driver.

---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)
MAINTAINERS +8 -0
index bfc1b86e3e73..4da888be6e80 100644
@@ -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 <[email protected]>
+M:	Greentime Hu <[email protected]>
+L:	[email protected]
+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 <[email protected]>
 L:	[email protected]
Signature · 1 line
2.30.0
author maintainer
#6 lore raw

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

In reply to Greentime Hu

Add PCIe host controller DT bindings of SiFive FU740.

---
 .../bindings/pci/sifive,fu740-pcie.yaml       | 119 ++++++++++++++++++
 1 file changed, 119 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml
Documentation/devicetree/bindings/pci/sifive,fu740-pcie.yaml new file +119 -0
new file mode 100644
index 000000000000..879ab4f80456
@@ -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 <[email protected]>
+  - Greentime Hu <[email protected]>
+
+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>;
+    };
Signature · 1 line
2.30.0
maintainer
#7 lore raw

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

Quoted 8 lines · Greentime Hu

Add PCIe host controller DT bindings of SiFive FU740.

---
 .../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.

author maintainer
#8 lore raw

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

In reply to Greentime Hu

From: Paul Walmsley <[email protected]>

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

---
 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
drivers/pci/controller/dwc/Kconfig +9 -0
index 22c5529e9a65..0a37d21ed64e 100644
@@ -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
drivers/pci/controller/dwc/Makefile +1 -0
index a751553fa0db..625f6aaeb5b8 100644
@@ -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
drivers/pci/controller/dwc/pcie-fu740.c new file +455 -0

455 lines not on this page · raw message

Signature · 1 line
2.30.0
#9 lore raw

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

Quoted 6 lines · Greentime Hu

From: Paul Walmsley <[email protected]>

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

Quoted 6 lines

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

Quoted 1 line

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

Quoted 75 lines
---
 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
drivers/pci/controller/dwc/Kconfig +9 -0
index 22c5529e9a65..0a37d21ed64e 100644
@@ -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
drivers/pci/controller/dwc/Makefile +1 -0
index a751553fa0db..625f6aaeb5b8 100644
@@ -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
drivers/pci/controller/dwc/pcie-fu740.c new file +32 -0
new file mode 100644
index 000000000000..6916eea40ea5
@@ -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>

drivers/pci/controller/dwc/pcie-fu740.c line 42

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+
+#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.

Quoted 14 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +14 -0
+       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

drivers/pci/controller/dwc/pcie-fu740.c line 66

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+#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.

drivers/pci/controller/dwc/pcie-fu740.c line 69

Quoted 3 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +3 -0
+
+/* PCIe Port Logic registers (memory-mapped) */
+#define PL_OFFSET                              0x700

That's all DWC controllers.

drivers/pci/controller/dwc/pcie-fu740.c line 70

Quoted 1 line
drivers/pci/controller/dwc/pcie-fu740.c new file +1 -0
+#define PCIE_PL_GEN2_CTRL_OFF                  (PL_OFFSET + 0x10c)

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

drivers/pci/controller/dwc/pcie-fu740.c line 82

Quoted 12 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +12 -0
+#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.

Quoted 20 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +20 -0
+}
+
+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)

drivers/pci/controller/dwc/pcie-fu740.c line 112

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+{
+       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.

Quoted 55 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +55 -0
+{
+       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

drivers/pci/controller/dwc/pcie-fu740.c line 177

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+              (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.

Quoted 54 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +54 -0
+}
+
+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);

drivers/pci/controller/dwc/pcie-fu740.c line 241

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+
+       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?

Quoted 17 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +17 -0
+       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

drivers/pci/controller/dwc/pcie-fu740.c line 268

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+        * 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.

drivers/pci/controller/dwc/pcie-fu740.c line 273

Quoted 5 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +5 -0
+       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.

Quoted 69 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +69 -0
+       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;

drivers/pci/controller/dwc/pcie-fu740.c line 352

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+       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.

Quoted 5 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +5 -0
+       if (!pci)
+               return -ENOMEM;
+
+       pci->dev = dev;
+       pci->ops = &dw_pcie_ops;

drivers/pci/controller/dwc/pcie-fu740.c line 367

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+       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

Quoted 6 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +6 -0
+       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)) {

drivers/pci/controller/dwc/pcie-fu740.c line 383

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+               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.

Quoted 27 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +27 -0
+
+       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);
+

drivers/pci/controller/dwc/pcie-fu740.c line 420

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+       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.

Quoted 25 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +25 -0
+       }
+
+       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,
+                  },

drivers/pci/controller/dwc/pcie-fu740.c line 455

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+       .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?

maintainer
#10 lore raw

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

In reply to Greentime Hu

On Tue, 2021-03-02 at 18:59 +0800, Greentime Hu wrote:
[...]

Quoted 2 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +2 -0
+static int fu740_pcie_probe(struct platform_device *pdev)
+{

[...]

drivers/pci/controller/dwc/pcie-fu740.c line 405

Quoted 2 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +2 -0
+       /* Fetch reset */
+       afp->rst = devm_reset_control_get(dev, NULL);

Please use

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

regards
Philipp

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

#11 lore raw

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

In reply to Greentime Hu

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: ...

Quoted 143 lines · Greentime Hu

From: Paul Walmsley <[email protected]>

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

---
 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
drivers/pci/controller/dwc/Kconfig +9 -0
index 22c5529e9a65..0a37d21ed64e 100644
@@ -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
drivers/pci/controller/dwc/Makefile +1 -0
index a751553fa0db..625f6aaeb5b8 100644
@@ -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
drivers/pci/controller/dwc/pcie-fu740.c new file +88 -0
new file mode 100644
index 000000000000..6916eea40ea5
@@ -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);

drivers/pci/controller/dwc/pcie-fu740.c line 98

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+	/* 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?

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+	}
+}
+
+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);
+}
+

drivers/pci/controller/dwc/pcie-fu740.c line 118

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+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.

drivers/pci/controller/dwc/pcie-fu740.c line 122

Quoted 4 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +4 -0
+	__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.

Quoted 20 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +20 -0
+	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 +

drivers/pci/controller/dwc/pcie-fu740.c line 152

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+		     (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.

Quoted 26 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +26 -0
+	/* 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;
+		}
+	}
+}
+

drivers/pci/controller/dwc/pcie-fu740.c line 189

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+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.

drivers/pci/controller/dwc/pcie-fu740.c line 198

Quoted 8 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +8 -0
+	/* 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.

Quoted 51 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +51 -0
+	}
+
+}
+
+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);

drivers/pci/controller/dwc/pcie-fu740.c line 259

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+	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/

Quoted 20 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +20 -0
+	 * 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),

drivers/pci/controller/dwc/pcie-fu740.c line 289

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+		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".

drivers/pci/controller/dwc/pcie-fu740.c line 302

Quoted 13 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +13 -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)

Rewrap text to fill lines.

Quoted 23 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +23 -0
+	 */
+	__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 = {

drivers/pci/controller/dwc/pcie-fu740.c line 335

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+	.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?

Quoted 17 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +17 -0
+};
+
+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);

drivers/pci/controller/dwc/pcie-fu740.c line 362

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+	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.

Quoted 5 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +5 -0
+	if (!mgmt_res) {
+		dev_warn(dev, "missing required mgmt address range");
+		return -ENOENT;
+	}
+	afp->mgmt_base = devm_ioremap_resource(dev, mgmt_res);

drivers/pci/controller/dwc/pcie-fu740.c line 377

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+	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"?

drivers/pci/controller/dwc/pcie-fu740.c line 383

Quoted 6 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +6 -0
+			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.

Quoted 41 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +41 -0
+	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;
+}
+

drivers/pci/controller/dwc/pcie-fu740.c line 435

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -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", },

drivers/pci/controller/dwc/pcie-fu740.c line 445

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+	{},
+};
+
+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.

drivers/pci/controller/dwc/pcie-fu740.c line 455

Quoted 10 lines
drivers/pci/controller/dwc/pcie-fu740.c new file +10 -0
+	.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.

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

author maintainer
#12 lore raw

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

In reply to Greentime Hu
---
 arch/riscv/boot/dts/sifive/fu740-c000.dtsi | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
arch/riscv/boot/dts/sifive/fu740-c000.dtsi +34 -0
index d1bb22b11920..d0839739b425 100644
@@ -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";
+		};
 	};
 };
Signature · 1 line
2.30.0