Refactoring Microchip PolarFire PCIe driver

Add PLDA XpressRICH PCIe host common properties dt-binding doc. Microchip PolarFire PCIe host using PLDA IP.
Extract properties from Microchip PolarFire PCIe host.

Signed-off-by Minda Chen <[email protected]>

Reviewed-by Hal Feng <[email protected]>

---
 .../pci/plda,xpressrich-pcie-common.yaml      | 72 +++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-common.yaml

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-common.yaml new file +6 -0

new file mode 100644
index 000000000000..3627a846c5d1
@@ -0,0 +1,72 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/plda,xpressrich-pcie-common.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+

Quoted 4 lines · Minda Chen

Add PLDA XpressRICH PCIe host common properties dt-binding doc. Microchip PolarFire PCIe host using PLDA IP.
Extract properties from Microchip PolarFire PCIe host.

Quoted 6 lines

Signed-off-by Minda Chen <[email protected]>

Reviewed-by Hal Feng <[email protected]>

---
 .../pci/plda,xpressrich-pcie-common.yaml      | 72 +++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-common.yaml

How is it related with existing plda,xpressrich3-axi?

Quoted 17 lines

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-common.yaml new file +10 -0

new file mode 100644
index 000000000000..3627a846c5d1
@@ -0,0 +1,72 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/plda,xpressrich-pcie-common.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: PLDA XpressRICH PCIe host common properties
+
+maintainers:
+  - Daire McNamara <[email protected]>

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-common.yaml line 16

Quoted 6 lines

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-common.yaml new file +6 -0

+  - Minda Chen <[email protected]>
+
+description:
+  Generic PLDA XpressRICH PCIe host common properties.
+
+select: false

This should not be needed.

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-common.yaml line 21

Quoted 5 lines

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-common.yaml new file +5 -0

+
+properties:
+  reg:
+    description:
+      At least host IP register set and configuration space are

"At least" does not fit here since you do not allow anything else.

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-common.yaml line 32

Quoted 11 lines

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-common.yaml new file +11 -0

+      required for normal controller work.
+    maxItems: 2
+
+  reg-names:
+    oneOf:
+      - items:
+          - const: cfg
+          - const: apb
+      - items:
+          - const: host
+          - const: cfg

Maybe keep similar order, so cfg followed by host?

Best regards,
Krzysztof

Add PLDA XpressRICH PCIe host common properties dt-binding doc. Microchip PolarFire PCIe host using PLDA IP.
Extract properties from Microchip PolarFire PCIe host.

Signed-off-by Minda Chen <[email protected]>

Reviewed-by Hal Feng <[email protected]>

---
 .../pci/plda,xpressrich-pcie-common.yaml      | 72 +++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-common.yaml

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-common.yaml new file +22 -0

new file mode 100644
index 000000000000..3627a846c5d1
@@ -0,0 +1,72 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/plda,xpressrich-pcie-common.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: PLDA XpressRICH PCIe host common properties
+
+maintainers:
+  - Daire McNamara <[email protected]>
+  - Minda Chen <[email protected]>
+
+description:
+  Generic PLDA XpressRICH PCIe host common properties.
+
+select: false
+
+properties:
+  reg:
+    description:
+      At least host IP register set and configuration space are
+      required for normal controller work.

Quoted 39 lines · Minda Chen

Add PLDA XpressRICH PCIe host common properties dt-binding doc. Microchip PolarFire PCIe host using PLDA IP.
Extract properties from Microchip PolarFire PCIe host.

Signed-off-by Minda Chen <[email protected]>

Reviewed-by Hal Feng <[email protected]>

---
 .../pci/plda,xpressrich-pcie-common.yaml      | 72 +++++++++++++++++++
 1 file changed, 72 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-common.yaml

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-common.yaml new file +22 -0

new file mode 100644
index 000000000000..3627a846c5d1
@@ -0,0 +1,72 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/plda,xpressrich-pcie-common.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: PLDA XpressRICH PCIe host common properties
+
+maintainers:
+  - Daire McNamara <[email protected]>
+  - Minda Chen <[email protected]>
+
+description:
+  Generic PLDA XpressRICH PCIe host common properties.
+
+select: false
+
+properties:
+  reg:
+    description:
+      At least host IP register set and configuration space are
+      required for normal controller work.

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-common.yaml line 32

Quoted 10 lines

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-common.yaml new file +10 -0

+    maxItems: 2
+
+  reg-names:
+    oneOf:
+      - items:
+          - const: cfg
+          - const: apb
+      - items:
+          - const: host
+          - const: cfg

This didn't exist before. Where's the reasoning?

There's no reason for 'cfg' to be in different spots and little reason to have different names for the host/apb space.

Rob

Add plda,xpressrich-pcie-common.yaml reference and remove the PLDA XpressRICH PCIe host common properties.

Signed-off-by Minda Chen <[email protected]>

---
 .../bindings/pci/microchip,pcie-host.yaml     | 45 +------------------
 1 file changed, 1 insertion(+), 44 deletions(-)

Move Microchip specific platform codes to PLDA directory. Including clock init, interrupt event handle and platform init codes.

Signed-off-by Minda Chen <[email protected]>

---
 MAINTAINERS                                   |   4 +-
 drivers/pci/controller/Kconfig                |   8 -
 drivers/pci/controller/Makefile               |   1 -
 drivers/pci/controller/plda/Kconfig           |  10 +-
 drivers/pci/controller/plda/Makefile          |   1 +
 .../{ => plda}/pcie-microchip-host.c          | 594 ++----------------

 6 files changed, 55 insertions(+), 563 deletions(-)
 rename drivers/pci/controller/{ => plda}/pcie-microchip-host.c (50%)

MAINTAINERS +2 -2

index 3b3c4d8808ae..f02618c2bdf5 100644
@@ -16503,7 +16503,7 @@ M:	Daire McNamara <[email protected]>
 L:	[email protected]
 S:	Supported
 F:	Documentation/devicetree/bindings/pci/microchip*
-F:	drivers/pci/controller/*microchip*
+F:	drivers/pci/controller/plda/*microchip*
 
 PCIE DRIVER FOR QUALCOMM MSM
 M:	Manivannan Sadhasivam <[email protected]>

@@ -18287,7 +18287,7 @@ F:	drivers/char/hw_random/mpfs-rng.c
 F:	drivers/clk/microchip/clk-mpfs*.c
 F:	drivers/i2c/busses/i2c-microchip-corei2c.c
 F:	drivers/mailbox/mailbox-mpfs.c
-F:	drivers/pci/controller/pcie-microchip-host.c
+F:	drivers/pci/controller/plda/pcie-microchip-host.c
 F:	drivers/pwm/pwm-microchip-core.c
 F:	drivers/reset/reset-mpfs.c
 F:	drivers/rtc/rtc-mpfs.c

drivers/pci/controller/Kconfig +0 -8

index a106dabcb8dc..107cdb69e15c 100644
@@ -216,14 +216,6 @@ config PCIE_MT7621
 	help
 	  This selects a driver for the MediaTek MT7621 PCIe Controller.
 
-config PCIE_MICROCHIP_HOST
-	bool "Microchip AXI PCIe controller"
-	depends on PCI_MSI && OF
-	select PCI_HOST_COMMON
-	help
-	  Say Y here if you want kernel to support the Microchip AXI PCIe
-	  Host Bridge driver.
-
 config PCI_HYPERV_INTERFACE
 	tristate "Microsoft Hyper-V PCI Interface"
 	depends on ((X86 && X86_64) || ARM64) && HYPERV && PCI_MSI

drivers/pci/controller/Makefile +0 -1

index aa0a691ebcbc..93236dc97b21 100644
@@ -32,7 +32,6 @@ obj-$(CONFIG_PCIE_ROCKCHIP_EP) += pcie-rockchip-ep.o
 obj-$(CONFIG_PCIE_ROCKCHIP_HOST) += pcie-rockchip-host.o
 obj-$(CONFIG_PCIE_MEDIATEK) += pcie-mediatek.o
 obj-$(CONFIG_PCIE_MEDIATEK_GEN3) += pcie-mediatek-gen3.o
-obj-$(CONFIG_PCIE_MICROCHIP_HOST) += pcie-microchip-host.o
 obj-$(CONFIG_VMD) += vmd.o
 obj-$(CONFIG_PCIE_BRCMSTB) += pcie-brcmstb.o
 obj-$(CONFIG_PCI_LOONGSON) += pci-loongson.o

drivers/pci/controller/plda/Kconfig +0 -0

index 37e17326671b..fb274976b84b 100644

drivers/pci/controller/plda/Kconfig +8 -0

 	select IRQ_DOMAIN
 
+config PCIE_MICROCHIP_HOST
+	bool "Microchip AXI PCIe controller"
+	select PCI_HOST_COMMON
+	select PCIE_PLDA_HOST
+	help
+	  Say Y here if you want kernel to support the Microchip AXI PCIe
+	  Host Bridge driver.
+
 endmenu

drivers/pci/controller/plda/Makefile +0 -0

index 79dbae655c2b..4340ab007f44 100644

Move Microchip specific platform codes to PLDA directory. Including clock init, interrupt event handle and platform init codes.

Signed-off-by Minda Chen <[email protected]>

Hey Minda,

Quoted 4 lines · Minda Chen

Move Microchip specific platform codes to PLDA directory. Including clock init, interrupt event handle and platform init codes.

Quoted 1 line

Signed-off-by Minda Chen <[email protected]>

Something else that I noticed, looking at what is not in the diff here, but is everything under the "/* PCIe Controller Phy Regs */" comment that remains in the microchip driver not also common to the plda IP?

Thanks,
Conor.

Add PLDA XpressRICH host controller dt-bindings. Both Microchip PolarFire SoC and StarFive JH7110 SoC are using PLDA XpressRICH PCIe host controller IP.

Signed-off-by Minda Chen <[email protected]>

Reviewed-by Hal Feng <[email protected]>

---
 .../pci/plda,xpressrich-pcie-host.yaml        | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-host.yaml

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-host.yaml new file +20 -0

new file mode 100644
index 000000000000..10a10862a078
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/plda,xpressrich-pcie-host.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: PLDA XpressRICH PCIe host controller
+
+maintainers:
+  - Daire McNamara <[email protected]>
+  - Minda Chen <[email protected]>
+
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+  - $ref: plda,xpressrich-pcie-common.yaml#
+  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
+
+properties:
+  compatible:
+    const: plda,xpressrich-pcie-host

Add PLDA XpressRICH host controller dt-bindings. Both Microchip PolarFire SoC and StarFive JH7110 SoC are using PLDA XpressRICH PCIe host controller IP.

Signed-off-by Minda Chen <[email protected]>

Reviewed-by Hal Feng <[email protected]>

---
 .../pci/plda,xpressrich-pcie-host.yaml        | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-host.yaml

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-host.yaml new file +10 -0

new file mode 100644
index 000000000000..10a10862a078
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/plda,xpressrich-pcie-host.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: PLDA XpressRICH PCIe host controller
+
+maintainers:
+  - Daire McNamara <[email protected]>

Quoted 27 lines · Minda Chen

Add PLDA XpressRICH host controller dt-bindings. Both Microchip PolarFire SoC and StarFive JH7110 SoC are using PLDA XpressRICH PCIe host controller IP.

Signed-off-by Minda Chen <[email protected]>

Reviewed-by Hal Feng <[email protected]>

---
 .../pci/plda,xpressrich-pcie-host.yaml        | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-host.yaml

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-host.yaml new file +10 -0

new file mode 100644
index 000000000000..10a10862a078
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/plda,xpressrich-pcie-host.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: PLDA XpressRICH PCIe host controller
+
+maintainers:
+  - Daire McNamara <[email protected]>

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-host.yaml line 20

Quoted 10 lines

Documentation/devicetree/bindings/pci/plda,xpressrich-pcie-host.yaml new file +10 -0

+  - Minda Chen <[email protected]>
+
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+  - $ref: plda,xpressrich-pcie-common.yaml#
+  - $ref: /schemas/interrupt-controller/msi-controller.yaml#
+
+properties:
+  compatible:
+    const: plda,xpressrich-pcie-host

What h/w is this in? I don't see why this is needed. You have the common schema for the IP block and then the Microchip and Starfive schemas for the 2 implementations.

Rob

Add StarFive JH7110 SoC PCIe controller dt-bindings. JH7110 using PLDA XpressRICH PCIe host controller IP.

Signed-off-by Minda Chen <[email protected]>

Reviewed-by Hal Feng <[email protected]>

---

Documentation/devicetree/bindings/pci/starfive,jh7110-pcie.yaml new file +4 -0

+
+required:
+  - compatible
+  - reg

Add StarFive JH7110 SoC PCIe controller platform
driver codes.

drivers/pci/controller/plda/pcie-starfive.c new file +4 -0

+struct starfive_jh7110_pcie {
+	struct plda_pcie	plda;
+	struct reset_control *resets;
+	struct clk_bulk_data *clks;

+ * The BAR0/1 of bridge should be hidden during enumeration to
+ * avoid the sizing and resource allocation by PCIe core.
+ */

drivers/pci/controller/plda/pcie-starfive.c new file +5 -0

+static bool starfive_pcie_hide_rc_bar(struct pci_bus *bus, unsigned int  devfn,
+				      int offset)
+{
+	if (pci_is_root_bus(bus) && !devfn &&
+	    (offset == PCI_BASE_ADDRESS_0 || offset == PCI_BASE_ADDRESS_1))

drivers/pci/controller/plda/pcie-starfive.c new file +11 -0

+static int starfive_pcie_is_link_up(struct starfive_jh7110_pcie *pcie)
+{
+	struct device *dev = pcie->plda.dev;
+	int ret;
+	u32 stg_reg_val;
+
+	/* 100ms timeout value should be enough for Gen1/2 training */
+	ret = regmap_read_poll_timeout(pcie->reg_syscon,
+				       pcie->stg_lnksta,
+				       stg_reg_val,
+				       stg_reg_val & DATA_LINK_ACTIVE,

Quoted 2 lines · Minda Chen

Add StarFive JH7110 SoC PCIe controller platform
driver codes.

Rewrap all the commit logs to fill 75 columns or so.

drivers/pci/controller/plda/pcie-plda.h line 23

Quoted 4 lines

#define PCIE_PCI_IDS_DW1 0x9c

drivers/pci/controller/plda/pcie-plda.h +2 -1

-
+#define  IDS_CLASS_CODE_SHIFT		16
+#define PCI_MISC			0xB4

Surrounding code uses lower-case hex. Make it all match.

drivers/pci/controller/plda/pcie-starfive.c line 37

Quoted 2 lines

drivers/pci/controller/plda/pcie-starfive.c new file +2 -0

+#define STG_SYSCON_AXI4_SLVL_ARFUNC_MASK	GENMASK(22, 8)
+#define STG_SYSCON_AXI4_SLVL_ARFUNC_SHIFT	8

When practical, use FIELD_GET() and FIELD_PREP() to avoid the need for *_SHIFT macros.

Quoted 9 lines

drivers/pci/controller/plda/pcie-starfive.c new file +9 -0

+struct starfive_jh7110_pcie {
+	struct plda_pcie	plda;
+	struct reset_control *resets;
+	struct clk_bulk_data *clks;
+	struct regmap *reg_syscon;
+	struct gpio_desc *power_gpio;
+	struct gpio_desc *reset_gpio;
+
+	u32 stg_arfun;

drivers/pci/controller/plda/pcie-starfive.c line 56

Quoted 5 lines

drivers/pci/controller/plda/pcie-starfive.c new file +5 -0

+	u32 stg_awfun;
+	u32 stg_rp_nep;
+	u32 stg_lnksta;
+
+	int num_clks;

If you indent one member with tabs, e.g., "struct plda_pcie plda",
they should all be indented to match.

Quoted 16 lines

+ * The BAR0/1 of bridge should be hidden during enumeration to
+ * avoid the sizing and resource allocation by PCIe core.
+ */

drivers/pci/controller/plda/pcie-starfive.c new file +13 -0

+static bool starfive_pcie_hide_rc_bar(struct pci_bus *bus, unsigned int  devfn,
+				      int offset)
+{
+	if (pci_is_root_bus(bus) && !devfn &&
+	    (offset == PCI_BASE_ADDRESS_0 || offset == PCI_BASE_ADDRESS_1))
+		return true;
+
+	return false;
+}
+
+int starfive_pcie_config_write(struct pci_bus *bus, unsigned int devfn,
+			       int where, int size, u32 value)
+{

drivers/pci/controller/plda/pcie-starfive.c line 77

Quoted 2 lines

drivers/pci/controller/plda/pcie-starfive.c new file +2 -0

+	if (starfive_pcie_hide_rc_bar(bus, devfn, where))
+		return PCIBIOS_BAD_REGISTER_NUMBER;

I think you are trying present BARs 0 & 1 as unimplemented. Such BARs are hardwired to zero, so you should make them behave that way (both read and write). Many callers of config accessors don't check the return value, so I don't think it's reliable to just return PCIBIOS_BAD_REGISTER_NUMBER.

Quoted 11 lines

drivers/pci/controller/plda/pcie-starfive.c new file +11 -0

+static int starfive_pcie_is_link_up(struct starfive_jh7110_pcie *pcie)
+{
+	struct device *dev = pcie->plda.dev;
+	int ret;
+	u32 stg_reg_val;
+
+	/* 100ms timeout value should be enough for Gen1/2 training */
+	ret = regmap_read_poll_timeout(pcie->reg_syscon,
+				       pcie->stg_lnksta,
+				       stg_reg_val,
+				       stg_reg_val & DATA_LINK_ACTIVE,

drivers/pci/controller/plda/pcie-starfive.c line 176

Quoted 10 lines

drivers/pci/controller/plda/pcie-starfive.c new file +10 -0

+				       10 * 1000, 100 * 1000);
+
+	/* If the link is down (no device in slot), then exit. */
+	if (ret == -ETIMEDOUT) {
+		dev_info(dev, "Port link down, exit.\n");
+		return 0;
+	} else if (ret == 0) {
+		dev_info(dev, "Port link up.\n");
+		return 1;
+	}

Please copy the naming and style of the "*_pcie_link_up()" functions in other drivers. These are boolean functions with no side effects, including no timeouts.

Some drivers have "*wait_for_link()" functions if polling is needed.

drivers/pci/controller/plda/pcie-starfive.c line 191

Quoted 2 lines

drivers/pci/controller/plda/pcie-starfive.c new file +2 -0

+		return dev_err_probe(dev, ret,
+			"failed to initialize pcie phy\n");

Driver messages should match (all capitalized or none capitalized).

drivers/pci/controller/plda/pcie-starfive.c line 280

Quoted 1 line

drivers/pci/controller/plda/pcie-starfive.c new file +1 -0

+	/* Enable root port */

Superfluous comment, since the function name says the same.

drivers/pci/controller/plda/pcie-starfive.c line 301

Quoted 5 lines

drivers/pci/controller/plda/pcie-starfive.c new file +4 -0

+	plda_pcie_enable_root_port(plda);
 
+	/* Ensure that PERST has been asserted for at least 100 ms */
+	msleep(300);
+	gpiod_set_value_cansleep(pcie->reset_gpio, 0);

At least 100 ms, but you sleep *300* ms? This is probably related to
https://lore.kernel.org/r/20230718155515.GA483233@bhelgaas

Please include a comment with the source of the delay value. I assume it's T_PVPERL and T_PERST-CLK from the PCIe CEM spec. This way we can someday share those #defines across drivers.

drivers/pci/controller/plda/pcie-starfive.c line 354

Quoted 2 lines

drivers/pci/controller/plda/pcie-starfive.c new file +2 -0

+#ifdef CONFIG_PM_SLEEP
+static int __maybe_unused starfive_pcie_suspend_noirq(struct device *dev)

I think you can dispense with some of these #ifdefs and the __maybe_unused as in
https://lore.kernel.org/all/20220720224829.GA1667002@bhelgaas/

drivers/pci/controller/plda/pcie-starfive.c line 359

Quoted 5 lines

drivers/pci/controller/plda/pcie-starfive.c new file +5 -0

+{
+	struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev);
+
+	if (!pcie)
+		return 0;

How can this happen? If we're only detecting memory corruption, it's not worth it.

Bjorn

Quoted 20 lines · Bjorn Helgaas

Quoted 19 lines · Minda Chen

Add StarFive JH7110 SoC PCIe controller platform
driver codes.

+ * The BAR0/1 of bridge should be hidden during enumeration to
+ * avoid the sizing and resource allocation by PCIe core.
+ */

drivers/pci/controller/plda/pcie-starfive.c new file +13 -0

+static bool starfive_pcie_hide_rc_bar(struct pci_bus *bus, unsigned int  devfn,
+				      int offset)
+{
+	if (pci_is_root_bus(bus) && !devfn &&
+	    (offset == PCI_BASE_ADDRESS_0 || offset == PCI_BASE_ADDRESS_1))
+		return true;
+
+	return false;
+}
+
+int starfive_pcie_config_write(struct pci_bus *bus, unsigned int devfn,
+			       int where, int size, u32 value)
+{

Quoted 8 lines

drivers/pci/controller/plda/pcie-starfive.c line 77

Quoted 2 lines

drivers/pci/controller/plda/pcie-starfive.c new file +2 -0

+	if (starfive_pcie_hide_rc_bar(bus, devfn, where))
+		return PCIBIOS_BAD_REGISTER_NUMBER;

I think you are trying present BARs 0 & 1 as unimplemented. Such BARs are hardwired to zero, so you should make them behave that way (both read and write). Many callers of config accessors don't check the return value, so I don't think it's reliable to just return PCIBIOS_BAD_REGISTER_NUMBER.

This is a hardware defect that we did not hardwired those BARs to zero, and it is configurable for software now. We have to add this filter function for workaround.

Quoted 10 lines

drivers/pci/controller/plda/pcie-starfive.c line 301

Quoted 3 lines

drivers/pci/controller/plda/pcie-starfive.c new file +3 -0

+	/* Ensure that PERST has been asserted for at least 100 ms */
+	msleep(300);
+	gpiod_set_value_cansleep(pcie->reset_gpio, 0);

At least 100 ms, but you sleep *300* ms? This is probably related to
https://lore.kernel.org/r/20230718155515.GA483233@bhelgaas

Please include a comment with the source of the delay value. I assume it's T_PVPERL and T_PERST-CLK from the PCIe CEM spec. This way we can someday share those #defines across drivers.

Yes, the delay value here is T_PVPERL from PCIe CEM spec r2.0 (Table 2-4). At the first time we set 100ms delay according to sector 2.2 of the spec: "After there has been time (TPVPERL) for the power and clock to become stable, PERST# is deasserted high and the PCI Express functions can start up."

However, in the compatibility testing with several NVMe SSD, we found that Lenovo Thinklife ST8000 NVMe can not get ready in 100ms, and it actually needs almost 200ms. Thus, we increased the T_PVPERL value to 300ms for the better device compatibility.

We will use a macro to define T_PVPERL, and add comments for the source of it. If the compatibility delay of 300ms is not reasonable, we can revert it to 100ms.

Quoted 30 lines · Kevin Xie

Quoted 20 lines · Bjorn Helgaas

Quoted 19 lines · Minda Chen

Add StarFive JH7110 SoC PCIe controller platform
driver codes.

+ * The BAR0/1 of bridge should be hidden during enumeration to
+ * avoid the sizing and resource allocation by PCIe core.
+ */

drivers/pci/controller/plda/pcie-starfive.c new file +13 -0

+static bool starfive_pcie_hide_rc_bar(struct pci_bus *bus, unsigned int  devfn,
+				      int offset)
+{
+	if (pci_is_root_bus(bus) && !devfn &&
+	    (offset == PCI_BASE_ADDRESS_0 || offset == PCI_BASE_ADDRESS_1))
+		return true;
+
+	return false;
+}
+
+int starfive_pcie_config_write(struct pci_bus *bus, unsigned int devfn,
+			       int where, int size, u32 value)
+{

Quoted 8 lines

drivers/pci/controller/plda/pcie-starfive.c line 77

Quoted 2 lines

drivers/pci/controller/plda/pcie-starfive.c new file +2 -0

+	if (starfive_pcie_hide_rc_bar(bus, devfn, where))
+		return PCIBIOS_BAD_REGISTER_NUMBER;

I think you are trying present BARs 0 & 1 as unimplemented. Such BARs are hardwired to zero, so you should make them behave that way (both read and write). Many callers of config accessors don't check the return value, so I don't think it's reliable to just return PCIBIOS_BAD_REGISTER_NUMBER.

Quoted 3 lines

This is a hardware defect that we did not hardwired those BARs to zero, and it is configurable for software now. We have to add this filter function for workaround.

Yes. My point is that this only affects the write path, and the read probably does not read 0 as it should. This means lspci will show the wrong thing, and the PCI core will try to size the BAR when it doesn't need to. I haven't looked at the BAR sizing code; it might even come up with a bogus size and address, when it *should* just conclude the BAR doesn't exist at all.

Quoted 17 lines

Quoted 10 lines

drivers/pci/controller/plda/pcie-starfive.c line 301

Quoted 3 lines

drivers/pci/controller/plda/pcie-starfive.c new file +3 -0

+	/* Ensure that PERST has been asserted for at least 100 ms */
+	msleep(300);
+	gpiod_set_value_cansleep(pcie->reset_gpio, 0);

At least 100 ms, but you sleep *300* ms? This is probably related to
https://lore.kernel.org/r/20230718155515.GA483233@bhelgaas

Please include a comment with the source of the delay value. I assume it's T_PVPERL and T_PERST-CLK from the PCIe CEM spec. This way we can someday share those #defines across drivers.

Yes, the delay value here is T_PVPERL from PCIe CEM spec r2.0 (Table 2-4). At the first time we set 100ms delay according to sector 2.2 of the spec: "After there has been time (TPVPERL) for the power and clock to become stable, PERST# is deasserted high and the PCI Express functions can start up."

Quoted 8 lines

However, in the compatibility testing with several NVMe SSD, we found that Lenovo Thinklife ST8000 NVMe can not get ready in 100ms, and it actually needs almost 200ms. Thus, we increased the T_PVPERL value to 300ms for the better device compatibility.

We will use a macro to define T_PVPERL, and add comments for the source of it. If the compatibility delay of 300ms is not reasonable, we can revert it to 100ms.

Thanks for this valuable information! This NVMe issue potentially affects many similar drivers, and we may need a more generic fix so this device works well with all of them.

T_PVPERL is defined to start when power is stable. Do you have a way to accurately determine that point? I'm guessing this:

gpiod_set_value_cansleep(pcie->power_gpio, 1)

turns the power on? But of course that doesn't mean it is instantly stable. Maybe your testing is telling you that your driver should have a hardware-specific 200ms delay to wait for power to become stable, followed by the standard 100ms for T_PVPERL?

Bjorn

Quoted 19 lines · Bjorn Helgaas

Quoted 9 lines · Kevin Xie

Quoted 3 lines · Bjorn Helgaas

Quoted 2 lines · Minda Chen

Add StarFive JH7110 SoC PCIe controller platform
driver codes.

However, in the compatibility testing with several NVMe SSD, we found that Lenovo Thinklife ST8000 NVMe can not get ready in 100ms, and it actually needs almost 200ms. Thus, we increased the T_PVPERL value to 300ms for the better device compatibility.

...

Thanks for this valuable information! This NVMe issue potentially affects many similar drivers, and we may need a more generic fix so this device works well with all of them.

T_PVPERL is defined to start when power is stable. Do you have a way to accurately determine that point? I'm guessing this:

Quoted 6 lines

gpiod_set_value_cansleep(pcie->power_gpio, 1)

turns the power on? But of course that doesn't mean it is instantly stable. Maybe your testing is telling you that your driver should have a hardware-specific 200ms delay to wait for power to become stable, followed by the standard 100ms for T_PVPERL?

You are right, we did not take the power stable cost into account. T_PVPERL is enough for Lenovo Thinklife ST8000 NVMe SSD to get ready, and the extra cost is from the power circuit of a PCIe to M.2 connector, which is used to verify M.2 SSD with our EVB at early stage.

As the Thinklife NVMe SSD may be a halted product, and the onboard power circuit of VisionFive V2 is no problem, we decided revert the sleep time to be 100ms.

Quoted 27 lines · Kevin Xie

Quoted 19 lines · Bjorn Helgaas

Quoted 9 lines · Kevin Xie

Quoted 3 lines · Bjorn Helgaas

Quoted 2 lines · Minda Chen

Add StarFive JH7110 SoC PCIe controller platform
driver codes.

However, in the compatibility testing with several NVMe SSD, we found that Lenovo Thinklife ST8000 NVMe can not get ready in 100ms, and it actually needs almost 200ms. Thus, we increased the T_PVPERL value to 300ms for the better device compatibility.

...

Thanks for this valuable information! This NVMe issue potentially affects many similar drivers, and we may need a more generic fix so this device works well with all of them.

T_PVPERL is defined to start when power is stable. Do you have a way to accurately determine that point? I'm guessing this:

Quoted 6 lines

gpiod_set_value_cansleep(pcie->power_gpio, 1)

turns the power on? But of course that doesn't mean it is instantly stable. Maybe your testing is telling you that your driver should have a hardware-specific 200ms delay to wait for power to become stable, followed by the standard 100ms for T_PVPERL?

Quoted 4 lines

You are right, we did not take the power stable cost into account. T_PVPERL is enough for Lenovo Thinklife ST8000 NVMe SSD to get ready, and the extra cost is from the power circuit of a PCIe to M.2 connector, which is used to verify M.2 SSD with our EVB at early stage.

Hmm. That sounds potentially interesting. I assume you're talking about something like this: https://www.amazon.com/dp/B07JKH5VTL

I'm not familiar with the timing requirements for something like this. There is a PCIe M.2 spec with some timing requirements, but I don't know whether or how software is supposed to manage this. There is a T_PVPGL (power valid to PERST# inactive) parameter, but it's implementation specific, so I don't know what the point of that is. And I don't see a way for software to even detect the presence of such an adapter.

[+cc Mika, Maciej since they've worked on similar delays recently]

Quoted 36 lines · Bjorn Helgaas

Quoted 27 lines · Kevin Xie

Quoted 19 lines · Bjorn Helgaas

Quoted 9 lines · Kevin Xie

Quoted 3 lines · Bjorn Helgaas

Quoted 2 lines · Minda Chen

Add StarFive JH7110 SoC PCIe controller platform
driver codes.

However, in the compatibility testing with several NVMe SSD, we found that Lenovo Thinklife ST8000 NVMe can not get ready in 100ms, and it actually needs almost 200ms. Thus, we increased the T_PVPERL value to 300ms for the better device compatibility.

...

Thanks for this valuable information! This NVMe issue potentially affects many similar drivers, and we may need a more generic fix so this device works well with all of them.

T_PVPERL is defined to start when power is stable. Do you have a way to accurately determine that point? I'm guessing this:

Quoted 6 lines

gpiod_set_value_cansleep(pcie->power_gpio, 1)

turns the power on? But of course that doesn't mean it is instantly stable. Maybe your testing is telling you that your driver should have a hardware-specific 200ms delay to wait for power to become stable, followed by the standard 100ms for T_PVPERL?

Quoted 4 lines

You are right, we did not take the power stable cost into account. T_PVPERL is enough for Lenovo Thinklife ST8000 NVMe SSD to get ready, and the extra cost is from the power circuit of a PCIe to M.2 connector, which is used to verify M.2 SSD with our EVB at early stage.

Hmm. That sounds potentially interesting. I assume you're talking about something like this: https://www.amazon.com/dp/B07JKH5VTL

Quoted 7 lines

I'm not familiar with the timing requirements for something like this. There is a PCIe M.2 spec with some timing requirements, but I don't know whether or how software is supposed to manage this. There is a T_PVPGL (power valid to PERST# inactive) parameter, but it's implementation specific, so I don't know what the point of that is. And I don't see a way for software to even detect the presence of such an adapter.

I intended to ask about this on the PCI-SIG forum, but after reading this thread [1], I don't think we would learn anything. The question was:

The M.2 device has 5 voltage rails generated from the 3.3V input supply voltage
-------------------------------------------
This is re. Table 17 in PCI Express M.2 Specification Revision 1.1 Power Valid* to PERST# input inactive : Implementation specific; recommended 50 ms

What exactly does this mean ?

The Note says

*Power Valid when all the voltage supply rails have reached their respective Vmin.

Does this mean that the 50ms to PERSTn is counted from the instant when all *5 voltage rails* on the M.2 device have become "good" ?

and the answer was:

You wrote;
Does this mean that the 50ms to PERSTn is counted from the instant when all 5 voltage rails on the M.2 device have become "good" ?

Reply:
This means that counting the recommended 50 ms begins from the time when the power rails coming to the device/module, from the host, are stable *at the device connector*.

As for the time it takes voltages derived inside the device from any of the host power rails (e.g., 3.3V rail) to become stable, that is part of the 50ms the host should wait before de-asserting PERST#, in order ensure that most devices will be ready by then.

Strictly speaking, nothing disastrous happens if a host violates the 50ms. If it de-asserts too soon, the device may not be ready, but most hosts will try again. If the host de-asserts too late, the device has even more time to stabilize. This is why the WG felt that an exact minimum number for >>Tpvpgl, was not valid in practice, and we made it a recommendation.

Since T_PVPGL is implementation-specific, we can't really base anything in software on the 50ms recommendation. It sounds to me like they are counting on software to retry config reads when enumerating.

I guess the delays we *can* observe are:

100ms T_PVPERL "Power stable to PERST# inactive" (CEM 2.9.2) 100ms software delay between reset and config request (Base 6.6.1)

The PCI core doesn't know how to assert PERST#, so the T_PVPERL delay definitely has to be in the host controller driver.

The PCI core observes the second 100ms delay after a reset in pci_bridge_wait_for_secondary_bus(). But this 100ms delay does not happen during initial enumeration. I think the assumption of the PCI core is that when the host controller driver calls pci_host_probe(), we can issue config requests immediately.

So I think that to be safe, we probably need to do both of those 100ms delays in the host controller driver. Maybe there's some hope of supporting the latter one in the PCI core someday, but that's not today.

Bjorn

[1] https://forum.pcisig.com/viewtopic.php?f=74&t=1037

Quoted 93 lines · Bjorn Helgaas

Quoted 36 lines · Bjorn Helgaas

Quoted 27 lines · Kevin Xie

Quoted 19 lines · Bjorn Helgaas

Quoted 9 lines · Kevin Xie

Quoted 3 lines · Bjorn Helgaas

Quoted 2 lines · Minda Chen

Add StarFive JH7110 SoC PCIe controller platform
driver codes.

However, in the compatibility testing with several NVMe SSD, we found that Lenovo Thinklife ST8000 NVMe can not get ready in 100ms, and it actually needs almost 200ms. Thus, we increased the T_PVPERL value to 300ms for the better device compatibility.

...

Thanks for this valuable information! This NVMe issue potentially affects many similar drivers, and we may need a more generic fix so this device works well with all of them.

T_PVPERL is defined to start when power is stable. Do you have a way to accurately determine that point? I'm guessing this:

Quoted 6 lines

gpiod_set_value_cansleep(pcie->power_gpio, 1)

turns the power on? But of course that doesn't mean it is instantly stable. Maybe your testing is telling you that your driver should have a hardware-specific 200ms delay to wait for power to become stable, followed by the standard 100ms for T_PVPERL?

Quoted 4 lines

You are right, we did not take the power stable cost into account. T_PVPERL is enough for Lenovo Thinklife ST8000 NVMe SSD to get ready, and the extra cost is from the power circuit of a PCIe to M.2 connector, which is used to verify M.2 SSD with our EVB at early stage.

Hmm. That sounds potentially interesting. I assume you're talking about something like this: https://www.amazon.com/dp/B07JKH5VTL

Quoted 7 lines

I'm not familiar with the timing requirements for something like this. There is a PCIe M.2 spec with some timing requirements, but I don't know whether or how software is supposed to manage this. There is a T_PVPGL (power valid to PERST# inactive) parameter, but it's implementation specific, so I don't know what the point of that is. And I don't see a way for software to even detect the presence of such an adapter.

I intended to ask about this on the PCI-SIG forum, but after reading this thread [1], I don't think we would learn anything. The question was:

The M.2 device has 5 voltage rails generated from the 3.3V input supply voltage
-------------------------------------------
This is re. Table 17 in PCI Express M.2 Specification Revision 1.1 Power Valid* to PERST# input inactive : Implementation specific; recommended 50 ms

What exactly does this mean ?

The Note says

*Power Valid when all the voltage supply rails have reached their respective Vmin.

Does this mean that the 50ms to PERSTn is counted from the instant when all *5 voltage rails* on the M.2 device have become "good" ?

and the answer was:

You wrote;
Does this mean that the 50ms to PERSTn is counted from the instant when all 5 voltage rails on the M.2 device have become "good" ?

Reply:
This means that counting the recommended 50 ms begins from the time when the power rails coming to the device/module, from the host, are stable *at the device connector*.

As for the time it takes voltages derived inside the device from any of the host power rails (e.g., 3.3V rail) to become stable, that is part of the 50ms the host should wait before de-asserting PERST#, in order ensure that most devices will be ready by then.

Strictly speaking, nothing disastrous happens if a host violates the 50ms. If it de-asserts too soon, the device may not be ready, but most hosts will try again. If the host de-asserts too late, the device has even more time to stabilize. This is why the WG felt that an exact minimum number for >>Tpvpgl, was not valid in practice, and we made it a recommendation.

Since T_PVPGL is implementation-specific, we can't really base anything in software on the 50ms recommendation. It sounds to me like they are counting on software to retry config reads when enumerating.

Quoted 4 lines

I guess the delays we *can* observe are:

100ms T_PVPERL "Power stable to PERST# inactive" (CEM 2.9.2) 100ms software delay between reset and config request (Base 6.6.1)

Refer to Figure2-10 in CEM Spec V2.0, I guess this two delays are T2 & T4? In the PATCH v2[4/4], T2 is the msleep(100) for T_PVPERL,
and T4 is done by starfive_pcie_host_wait_for_link().

Quoted 9 lines

The PCI core doesn't know how to assert PERST#, so the T_PVPERL delay definitely has to be in the host controller driver.

The PCI core observes the second 100ms delay after a reset in pci_bridge_wait_for_secondary_bus(). But this 100ms delay does not happen during initial enumeration. I think the assumption of the PCI core is that when the host controller driver calls pci_host_probe(), we can issue config requests immediately.

Quoted 8 lines

So I think that to be safe, we probably need to do both of those 100ms delays in the host controller driver. Maybe there's some hope of supporting the latter one in the PCI core someday, but that's not today.

Bjorn

[1] https://forum.pcisig.com/viewtopic.php?f=74&t=1037

[+cc Pali, Marek because I used f76b36d40bee ("PCI: aardvark: Fix link training") as an example]

Quoted 94 lines · Kevin Xie

Quoted 93 lines · Bjorn Helgaas

Quoted 36 lines · Bjorn Helgaas

Quoted 27 lines · Kevin Xie

Quoted 19 lines · Bjorn Helgaas

Quoted 9 lines · Kevin Xie

Quoted 3 lines · Bjorn Helgaas

Quoted 2 lines · Minda Chen

Add StarFive JH7110 SoC PCIe controller platform
driver codes.

However, in the compatibility testing with several NVMe SSD, we found that Lenovo Thinklife ST8000 NVMe can not get ready in 100ms, and it actually needs almost 200ms. Thus, we increased the T_PVPERL value to 300ms for the better device compatibility.

...

Thanks for this valuable information! This NVMe issue potentially affects many similar drivers, and we may need a more generic fix so this device works well with all of them.

T_PVPERL is defined to start when power is stable. Do you have a way to accurately determine that point? I'm guessing this:

Quoted 6 lines

gpiod_set_value_cansleep(pcie->power_gpio, 1)

turns the power on? But of course that doesn't mean it is instantly stable. Maybe your testing is telling you that your driver should have a hardware-specific 200ms delay to wait for power to become stable, followed by the standard 100ms for T_PVPERL?

Quoted 4 lines

You are right, we did not take the power stable cost into account. T_PVPERL is enough for Lenovo Thinklife ST8000 NVMe SSD to get ready, and the extra cost is from the power circuit of a PCIe to M.2 connector, which is used to verify M.2 SSD with our EVB at early stage.

Hmm. That sounds potentially interesting. I assume you're talking about something like this: https://www.amazon.com/dp/B07JKH5VTL

Quoted 7 lines

I'm not familiar with the timing requirements for something like this. There is a PCIe M.2 spec with some timing requirements, but I don't know whether or how software is supposed to manage this. There is a T_PVPGL (power valid to PERST# inactive) parameter, but it's implementation specific, so I don't know what the point of that is. And I don't see a way for software to even detect the presence of such an adapter.

I intended to ask about this on the PCI-SIG forum, but after reading this thread [1], I don't think we would learn anything. The question was:

The M.2 device has 5 voltage rails generated from the 3.3V input supply voltage
-------------------------------------------
This is re. Table 17 in PCI Express M.2 Specification Revision 1.1 Power Valid* to PERST# input inactive : Implementation specific; recommended 50 ms

What exactly does this mean ?

The Note says

*Power Valid when all the voltage supply rails have reached their respective Vmin.

Does this mean that the 50ms to PERSTn is counted from the instant when all *5 voltage rails* on the M.2 device have become "good" ?

and the answer was:

You wrote;
Does this mean that the 50ms to PERSTn is counted from the instant when all 5 voltage rails on the M.2 device have become "good" ?

Reply:
This means that counting the recommended 50 ms begins from the time when the power rails coming to the device/module, from the host, are stable *at the device connector*.

As for the time it takes voltages derived inside the device from any of the host power rails (e.g., 3.3V rail) to become stable, that is part of the 50ms the host should wait before de-asserting PERST#, in order ensure that most devices will be ready by then.

Strictly speaking, nothing disastrous happens if a host violates the 50ms. If it de-asserts too soon, the device may not be ready, but most hosts will try again. If the host de-asserts too late, the device has even more time to stabilize. This is why the WG felt that an exact minimum number for >>Tpvpgl, was not valid in practice, and we made it a recommendation.

Since T_PVPGL is implementation-specific, we can't really base anything in software on the 50ms recommendation. It sounds to me like they are counting on software to retry config reads when enumerating.

Quoted 8 lines

Quoted 4 lines

I guess the delays we *can* observe are:

100ms T_PVPERL "Power stable to PERST# inactive" (CEM 2.9.2) 100ms software delay between reset and config request (Base 6.6.1)

Refer to Figure2-10 in CEM Spec V2.0, I guess this two delays are T2 & T4? In the PATCH v2[4/4], T2 is the msleep(100) for T_PVPERL,
and T4 is done by starfive_pcie_host_wait_for_link().

Yes, I think "T2" is T_PVPERL. The CEM r2.0 Figure 2-10 note is "2. Minimum time from power rails within specified tolerance to PERST# inactive (T_PVPERL)."

As far as T4 ("Minimum PERST# inactive to PCI Express link out of electrical idle"), I don't see a name or a value for that parameter, and I don't think it is the delay required by PCIe r6.0, sec 6.6.1.

The delay required by sec 6.6.1 is a minimum of 100ms following exit from reset or, for fast links, 100ms after link training completes.

The comment at the call of advk_pcie_wait_for_link() [2] says it is the delay required by sec 6.6.1, but that doesn't seem right to me.

For one thing, I don't think 6.6.1 says anything about "link up" being the end of a delay. So if we want to do the delay required by 6.6.1, "wait_for_link()" doesn't seem like quite the right name.

For another, all the *_wait_for_link() functions can return success after 0ms, 90ms, 180ms, etc. They're unlikely to return after 0ms, but 90ms is quite possible. If we avoided the 0ms return and LINK_WAIT_USLEEP_MIN were 100ms instead of 90ms, that should be enough for slow links, where we need 100ms following "exit from reset."

But it's still not enough for fast links where we need 100ms "after link training completes" because we don't know when training completed. If training completed 89ms into *_wait_for_link(), we only delay 1ms after that.

Quoted 9 lines

Quoted 9 lines

The PCI core doesn't know how to assert PERST#, so the T_PVPERL delay definitely has to be in the host controller driver.

The PCI core observes the second 100ms delay after a reset in pci_bridge_wait_for_secondary_bus(). But this 100ms delay does not happen during initial enumeration. I think the assumption of the PCI core is that when the host controller driver calls pci_host_probe(), we can issue config requests immediately.

Quoted 8 lines

Quoted 8 lines

So I think that to be safe, we probably need to do both of those 100ms delays in the host controller driver. Maybe there's some hope of supporting the latter one in the PCI core someday, but that's not today.

Bjorn

[1] https://forum.pcisig.com/viewtopic.php?f=74&t=1037

[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pci-aardvark.c?id=v6.4#n433

[+cc Pali, Marek because I used f76b36d40bee ("PCI: aardvark: Fix link training") as an example]

Quoted 94 lines · Kevin Xie

Quoted 93 lines · Bjorn Helgaas

Quoted 36 lines · Bjorn Helgaas

Quoted 27 lines · Kevin Xie

Quoted 19 lines · Bjorn Helgaas

Quoted 9 lines · Kevin Xie

Quoted 3 lines · Bjorn Helgaas

Quoted 2 lines · Minda Chen

Add StarFive JH7110 SoC PCIe controller platform
driver codes.

However, in the compatibility testing with several NVMe SSD, we found that Lenovo Thinklife ST8000 NVMe can not get ready in 100ms, and it actually needs almost 200ms. Thus, we increased the T_PVPERL value to 300ms for the better device compatibility.

...

Thanks for this valuable information! This NVMe issue potentially affects many similar drivers, and we may need a more generic fix so this device works well with all of them.

T_PVPERL is defined to start when power is stable. Do you have a way to accurately determine that point? I'm guessing this:

Quoted 6 lines

gpiod_set_value_cansleep(pcie->power_gpio, 1)

turns the power on? But of course that doesn't mean it is instantly stable. Maybe your testing is telling you that your driver should have a hardware-specific 200ms delay to wait for power to become stable, followed by the standard 100ms for T_PVPERL?

Quoted 4 lines

You are right, we did not take the power stable cost into account. T_PVPERL is enough for Lenovo Thinklife ST8000 NVMe SSD to get ready, and the extra cost is from the power circuit of a PCIe to M.2 connector, which is used to verify M.2 SSD with our EVB at early stage.

Hmm. That sounds potentially interesting. I assume you're talking about something like this: https://www.amazon.com/dp/B07JKH5VTL

Quoted 7 lines

I'm not familiar with the timing requirements for something like this. There is a PCIe M.2 spec with some timing requirements, but I don't know whether or how software is supposed to manage this. There is a T_PVPGL (power valid to PERST# inactive) parameter, but it's implementation specific, so I don't know what the point of that is. And I don't see a way for software to even detect the presence of such an adapter.

I intended to ask about this on the PCI-SIG forum, but after reading this thread [1], I don't think we would learn anything. The question was:

The M.2 device has 5 voltage rails generated from the 3.3V input supply voltage
-------------------------------------------
This is re. Table 17 in PCI Express M.2 Specification Revision 1.1 Power Valid* to PERST# input inactive : Implementation specific; recommended 50 ms

What exactly does this mean ?

The Note says

*Power Valid when all the voltage supply rails have reached their respective Vmin.

Does this mean that the 50ms to PERSTn is counted from the instant when all *5 voltage rails* on the M.2 device have become "good" ?

and the answer was:

You wrote;
Does this mean that the 50ms to PERSTn is counted from the instant when all 5 voltage rails on the M.2 device have become "good" ?

Reply:
This means that counting the recommended 50 ms begins from the time when the power rails coming to the device/module, from the host, are stable *at the device connector*.

As for the time it takes voltages derived inside the device from any of the host power rails (e.g., 3.3V rail) to become stable, that is part of the 50ms the host should wait before de-asserting PERST#, in order ensure that most devices will be ready by then.

Strictly speaking, nothing disastrous happens if a host violates the 50ms. If it de-asserts too soon, the device may not be ready, but most hosts will try again. If the host de-asserts too late, the device has even more time to stabilize. This is why the WG felt that an exact minimum number for >>Tpvpgl, was not valid in practice, and we made it a recommendation.

Since T_PVPGL is implementation-specific, we can't really base anything in software on the 50ms recommendation. It sounds to me like they are counting on software to retry config reads when enumerating.

Quoted 8 lines

Quoted 4 lines

I guess the delays we *can* observe are:

100ms T_PVPERL "Power stable to PERST# inactive" (CEM 2.9.2) 100ms software delay between reset and config request (Base 6.6.1)

Refer to Figure2-10 in CEM Spec V2.0, I guess this two delays are T2 & T4? In the PATCH v2[4/4], T2 is the msleep(100) for T_PVPERL,
and T4 is done by starfive_pcie_host_wait_for_link().

Yes, I think "T2" is T_PVPERL. The CEM r2.0 Figure 2-10 note is "2. Minimum time from power rails within specified tolerance to PERST# inactive (T_PVPERL)."

As far as T4 ("Minimum PERST# inactive to PCI Express link out of electrical idle"), I don't see a name or a value for that parameter, and I don't think it is the delay required by PCIe r6.0, sec 6.6.1.

The delay required by sec 6.6.1 is a minimum of 100ms following exit from reset or, for fast links, 100ms after link training completes.

The comment at the call of advk_pcie_wait_for_link() [2] says it is the delay required by sec 6.6.1, but that doesn't seem right to me.

For one thing, I don't think 6.6.1 says anything about "link up" being the end of a delay. So if we want to do the delay required by 6.6.1, "wait_for_link()" doesn't seem like quite the right name.

For another, all the *_wait_for_link() functions can return success after 0ms, 90ms, 180ms, etc. They're unlikely to return after 0ms, but 90ms is quite possible. If we avoided the 0ms return and LINK_WAIT_USLEEP_MIN were 100ms instead of 90ms, that should be enough for slow links, where we need 100ms following "exit from reset."

But it's still not enough for fast links where we need 100ms "after link training completes" because we don't know when training completed. If training completed 89ms into *_wait_for_link(), we only delay 1ms after that.

Quoted 9 lines

Quoted 9 lines

The PCI core doesn't know how to assert PERST#, so the T_PVPERL delay definitely has to be in the host controller driver.

The PCI core observes the second 100ms delay after a reset in pci_bridge_wait_for_secondary_bus(). But this 100ms delay does not happen during initial enumeration. I think the assumption of the PCI core is that when the host controller driver calls pci_host_probe(), we can issue config requests immediately.

Quoted 8 lines

Quoted 8 lines

So I think that to be safe, we probably need to do both of those 100ms delays in the host controller driver. Maybe there's some hope of supporting the latter one in the PCI core someday, but that's not today.

Bjorn

[1] https://forum.pcisig.com/viewtopic.php?f=74&t=1037

[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pci-aardvark.c?id=v6.4#n433

Quoted 131 lines · Bjorn Helgaas

[+cc Pali, Marek because I used f76b36d40bee ("PCI: aardvark: Fix link training") as an example]

Quoted 94 lines · Kevin Xie

Quoted 93 lines · Bjorn Helgaas

Quoted 36 lines · Bjorn Helgaas

Quoted 27 lines · Kevin Xie

Quoted 19 lines · Bjorn Helgaas

Quoted 9 lines · Kevin Xie

Quoted 3 lines · Bjorn Helgaas

Quoted 2 lines · Minda Chen

Add StarFive JH7110 SoC PCIe controller platform
driver codes.

However, in the compatibility testing with several NVMe SSD, we found that Lenovo Thinklife ST8000 NVMe can not get ready in 100ms, and it actually needs almost 200ms. Thus, we increased the T_PVPERL value to 300ms for the better device compatibility.

...

Thanks for this valuable information! This NVMe issue potentially affects many similar drivers, and we may need a more generic fix so this device works well with all of them.

T_PVPERL is defined to start when power is stable. Do you have a way to accurately determine that point? I'm guessing this:

Quoted 6 lines

gpiod_set_value_cansleep(pcie->power_gpio, 1)

turns the power on? But of course that doesn't mean it is instantly stable. Maybe your testing is telling you that your driver should have a hardware-specific 200ms delay to wait for power to become stable, followed by the standard 100ms for T_PVPERL?

Quoted 4 lines

You are right, we did not take the power stable cost into account. T_PVPERL is enough for Lenovo Thinklife ST8000 NVMe SSD to get ready, and the extra cost is from the power circuit of a PCIe to M.2 connector, which is used to verify M.2 SSD with our EVB at early stage.

Hmm. That sounds potentially interesting. I assume you're talking about something like this: https://www.amazon.com/dp/B07JKH5VTL

Quoted 7 lines

I'm not familiar with the timing requirements for something like this. There is a PCIe M.2 spec with some timing requirements, but I don't know whether or how software is supposed to manage this. There is a T_PVPGL (power valid to PERST# inactive) parameter, but it's implementation specific, so I don't know what the point of that is. And I don't see a way for software to even detect the presence of such an adapter.

I intended to ask about this on the PCI-SIG forum, but after reading this thread [1], I don't think we would learn anything. The question was:

The M.2 device has 5 voltage rails generated from the 3.3V input supply voltage
-------------------------------------------
This is re. Table 17 in PCI Express M.2 Specification Revision 1.1 Power Valid* to PERST# input inactive : Implementation specific; recommended 50 ms

What exactly does this mean ?

The Note says

*Power Valid when all the voltage supply rails have reached their respective Vmin.

Does this mean that the 50ms to PERSTn is counted from the instant when all *5 voltage rails* on the M.2 device have become "good" ?

and the answer was:

You wrote;
Does this mean that the 50ms to PERSTn is counted from the instant when all 5 voltage rails on the M.2 device have become "good" ?

Reply:
This means that counting the recommended 50 ms begins from the time when the power rails coming to the device/module, from the host, are stable *at the device connector*.

As for the time it takes voltages derived inside the device from any of the host power rails (e.g., 3.3V rail) to become stable, that is part of the 50ms the host should wait before de-asserting PERST#, in order ensure that most devices will be ready by then.

Strictly speaking, nothing disastrous happens if a host violates the 50ms. If it de-asserts too soon, the device may not be ready, but most hosts will try again. If the host de-asserts too late, the device has even more time to stabilize. This is why the WG felt that an exact minimum number for >>Tpvpgl, was not valid in practice, and we made it a recommendation.

Since T_PVPGL is implementation-specific, we can't really base anything in software on the 50ms recommendation. It sounds to me like they are counting on software to retry config reads when enumerating.

Quoted 8 lines

Quoted 4 lines

I guess the delays we *can* observe are:

100ms T_PVPERL "Power stable to PERST# inactive" (CEM 2.9.2) 100ms software delay between reset and config request (Base 6.6.1)

Refer to Figure2-10 in CEM Spec V2.0, I guess this two delays are T2 & T4? In the PATCH v2[4/4], T2 is the msleep(100) for T_PVPERL,
and T4 is done by starfive_pcie_host_wait_for_link().

Yes, I think "T2" is T_PVPERL. The CEM r2.0 Figure 2-10 note is "2. Minimum time from power rails within specified tolerance to PERST# inactive (T_PVPERL)."

As far as T4 ("Minimum PERST# inactive to PCI Express link out of electrical idle"), I don't see a name or a value for that parameter, and I don't think it is the delay required by PCIe r6.0, sec 6.6.1.

The delay required by sec 6.6.1 is a minimum of 100ms following exit from reset or, for fast links, 100ms after link training completes.

The comment at the call of advk_pcie_wait_for_link() [2] says it is the delay required by sec 6.6.1, but that doesn't seem right to me.

For one thing, I don't think 6.6.1 says anything about "link up" being the end of a delay. So if we want to do the delay required by 6.6.1, "wait_for_link()" doesn't seem like quite the right name.

For another, all the *_wait_for_link() functions can return success after 0ms, 90ms, 180ms, etc. They're unlikely to return after 0ms, but 90ms is quite possible. If we avoided the 0ms return and LINK_WAIT_USLEEP_MIN were 100ms instead of 90ms, that should be enough for slow links, where we need 100ms following "exit from reset."

Quoted 5 lines

But it's still not enough for fast links where we need 100ms "after link training completes" because we don't know when training completed. If training completed 89ms into *_wait_for_link(), we only delay 1ms after that.

That's the point, we will add a extra 100ms after PERST# de-assert in the patch-v3 according to Base Spec r6.0 - 6.6.1:

        msleep(100);
        gpiod_set_value_cansleep(pcie->reset_gpio, 0);

Quoted 19 lines

Quoted 9 lines

Quoted 9 lines

The PCI core doesn't know how to assert PERST#, so the T_PVPERL delay definitely has to be in the host controller driver.

The PCI core observes the second 100ms delay after a reset in pci_bridge_wait_for_secondary_bus(). But this 100ms delay does not happen during initial enumeration. I think the assumption of the PCI core is that when the host controller driver calls pci_host_probe(), we can issue config requests immediately.

Quoted 8 lines

Quoted 8 lines

So I think that to be safe, we probably need to do both of those 100ms delays in the host controller driver. Maybe there's some hope of supporting the latter one in the PCI core someday, but that's not today.

Bjorn

[1] https://forum.pcisig.com/viewtopic.php?f=74&t=1037

[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pci-aardvark.c?id=v6.4#n433

Quoted 8 lines · On Tuesday 01 August 2023 15:05:46 Kevin Xie wrote:

On 2023/8/1 7:12, Bjorn Helgaas wrote:
...

That's the point, we will add a extra 100ms after PERST# de-assert in the patch-v3 according to Base Spec r6.0 - 6.6.1:

        msleep(100);
        gpiod_set_value_cansleep(pcie->reset_gpio, 0);

Review

Quoted 7 lines

(quoted diff) +5 -0

+       /* As the requirement in PCIe base spec r6.0, system must wait a
+        * minimum of 100 ms following exit from a Conventional Reset
+        * before sending a Configuration Request to the device.*/
+       msleep(100);
+
        if (starfive_pcie_host_wait_for_link(pcie))
                return -EIO;

Maybe this information can be useful here:
https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/

On 2023/8/1 7:12, Bjorn Helgaas wrote:
...

Quoted 16 lines

The delay required by sec 6.6.1 is a minimum of 100ms following exit from reset or, for fast links, 100ms after link training completes.

The comment at the call of advk_pcie_wait_for_link() [2] says it is the delay required by sec 6.6.1, but that doesn't seem right to me.

For one thing, I don't think 6.6.1 says anything about "link up" being the end of a delay. So if we want to do the delay required by 6.6.1, "wait_for_link()" doesn't seem like quite the right name.

For another, all the *_wait_for_link() functions can return success after 0ms, 90ms, 180ms, etc. They're unlikely to return after 0ms, but 90ms is quite possible. If we avoided the 0ms return and LINK_WAIT_USLEEP_MIN were 100ms instead of 90ms, that should be enough for slow links, where we need 100ms following "exit from reset."

Quoted 4 lines

But it's still not enough for fast links where we need 100ms "after link training completes" because we don't know when training completed. If training completed 89ms into *_wait_for_link(), we only delay 1ms after that.

That's the point, we will add a extra 100ms after PERST# de-assert in the patch-v3 according to Base Spec r6.0 - 6.6.1:

        msleep(100);
        gpiod_set_value_cansleep(pcie->reset_gpio, 0);

Quoted 29 lines · Kevin Xie

On 2023/8/1 7:12, Bjorn Helgaas wrote:
...

Quoted 16 lines

The delay required by sec 6.6.1 is a minimum of 100ms following exit from reset or, for fast links, 100ms after link training completes.

The comment at the call of advk_pcie_wait_for_link() [2] says it is the delay required by sec 6.6.1, but that doesn't seem right to me.

For one thing, I don't think 6.6.1 says anything about "link up" being the end of a delay. So if we want to do the delay required by 6.6.1, "wait_for_link()" doesn't seem like quite the right name.

For another, all the *_wait_for_link() functions can return success after 0ms, 90ms, 180ms, etc. They're unlikely to return after 0ms, but 90ms is quite possible. If we avoided the 0ms return and LINK_WAIT_USLEEP_MIN were 100ms instead of 90ms, that should be enough for slow links, where we need 100ms following "exit from reset."

Quoted 4 lines

But it's still not enough for fast links where we need 100ms "after link training completes" because we don't know when training completed. If training completed 89ms into *_wait_for_link(), we only delay 1ms after that.

That's the point, we will add a extra 100ms after PERST# de-assert in the patch-v3 according to Base Spec r6.0 - 6.6.1:

        msleep(100);
        gpiod_set_value_cansleep(pcie->reset_gpio, 0);

Review

Quoted 7 lines

(quoted diff) +5 -0

+       /* As the requirement in PCIe base spec r6.0, system must wait a
+        * minimum of 100 ms following exit from a Conventional Reset
+        * before sending a Configuration Request to the device.*/
+       msleep(100);
+
        if (starfive_pcie_host_wait_for_link(pcie))
                return -EIO;

For fast links (links that support > 5.0 GT/s), the 100ms starts *after* link training completes. The above looks OK if starfive only supports slow links, but then I'm not sure why we would need starfive_pcie_host_wait_for_link().

Bjorn

Quoted 38 lines · Bjorn Helgaas

Quoted 29 lines · Kevin Xie

On 2023/8/1 7:12, Bjorn Helgaas wrote:
...

Quoted 16 lines

The delay required by sec 6.6.1 is a minimum of 100ms following exit from reset or, for fast links, 100ms after link training completes.

The comment at the call of advk_pcie_wait_for_link() [2] says it is the delay required by sec 6.6.1, but that doesn't seem right to me.

For one thing, I don't think 6.6.1 says anything about "link up" being the end of a delay. So if we want to do the delay required by 6.6.1, "wait_for_link()" doesn't seem like quite the right name.

For another, all the *_wait_for_link() functions can return success after 0ms, 90ms, 180ms, etc. They're unlikely to return after 0ms, but 90ms is quite possible. If we avoided the 0ms return and LINK_WAIT_USLEEP_MIN were 100ms instead of 90ms, that should be enough for slow links, where we need 100ms following "exit from reset."

Quoted 4 lines

But it's still not enough for fast links where we need 100ms "after link training completes" because we don't know when training completed. If training completed 89ms into *_wait_for_link(), we only delay 1ms after that.

That's the point, we will add a extra 100ms after PERST# de-assert in the patch-v3 according to Base Spec r6.0 - 6.6.1:

        msleep(100);
        gpiod_set_value_cansleep(pcie->reset_gpio, 0);

Review

Quoted 7 lines

(quoted diff) +5 -0

+       /* As the requirement in PCIe base spec r6.0, system must wait a
+        * minimum of 100 ms following exit from a Conventional Reset
+        * before sending a Configuration Request to the device.*/
+       msleep(100);
+
        if (starfive_pcie_host_wait_for_link(pcie))
                return -EIO;

Quoted 5 lines

For fast links (links that support > 5.0 GT/s), the 100ms starts *after* link training completes. The above looks OK if starfive only supports slow links, but then I'm not sure why we would need starfive_pcie_host_wait_for_link().

Yes, the maximum speed of JH7110 PCIe is 5.0 GT/s (Gen2x1).

About starfive_pcie_host_wait_for_link():
JH7110 SoC only has one root port in each PCIe controller (2 in total) and they do not support hot-plug yet.

Thus, We add starfive_pcie_host_wait_for_link() to poll if it is a empty slot. If nothing here, we will exit the probe() of this controller, and it will not go into pci_host_probe() too.
This may not be a very standard logic, should we remove it or rewrite in a better way?

Quoted 1 line

Bjorn

Quoted 44 lines · On Thursday 03 August 2023 10:23:47 Kevin Xie wrote:

Quoted 38 lines · Bjorn Helgaas

Quoted 29 lines · Kevin Xie

On 2023/8/1 7:12, Bjorn Helgaas wrote:
...

Quoted 16 lines

The delay required by sec 6.6.1 is a minimum of 100ms following exit from reset or, for fast links, 100ms after link training completes.

The comment at the call of advk_pcie_wait_for_link() [2] says it is the delay required by sec 6.6.1, but that doesn't seem right to me.

For one thing, I don't think 6.6.1 says anything about "link up" being the end of a delay. So if we want to do the delay required by 6.6.1, "wait_for_link()" doesn't seem like quite the right name.

For another, all the *_wait_for_link() functions can return success after 0ms, 90ms, 180ms, etc. They're unlikely to return after 0ms, but 90ms is quite possible. If we avoided the 0ms return and LINK_WAIT_USLEEP_MIN were 100ms instead of 90ms, that should be enough for slow links, where we need 100ms following "exit from reset."

Quoted 4 lines

But it's still not enough for fast links where we need 100ms "after link training completes" because we don't know when training completed. If training completed 89ms into *_wait_for_link(), we only delay 1ms after that.

That's the point, we will add a extra 100ms after PERST# de-assert in the patch-v3 according to Base Spec r6.0 - 6.6.1:

        msleep(100);
        gpiod_set_value_cansleep(pcie->reset_gpio, 0);

Review

Quoted 7 lines

(quoted diff) +5 -0

+       /* As the requirement in PCIe base spec r6.0, system must wait a
+        * minimum of 100 ms following exit from a Conventional Reset
+        * before sending a Configuration Request to the device.*/
+       msleep(100);
+
        if (starfive_pcie_host_wait_for_link(pcie))
                return -EIO;

Quoted 5 lines

For fast links (links that support > 5.0 GT/s), the 100ms starts *after* link training completes. The above looks OK if starfive only supports slow links, but then I'm not sure why we would need starfive_pcie_host_wait_for_link().

Quoted 5 lines

Yes, the maximum speed of JH7110 PCIe is 5.0 GT/s (Gen2x1).

About starfive_pcie_host_wait_for_link():
JH7110 SoC only has one root port in each PCIe controller (2 in total) and they do not support hot-plug yet.

Beware that even if HW does not support hotplug, endpoint PCIe card still may drop the link down and later put it up (for example if FW in the card crashes or when card want to do internal reset, etc...; this is very common for wifi cards). So drivers for non-hotplug controllers still have to handle hotplug events generated by link up/down state.

So it means that, if endpoint PCIe card is not detected during probe time, it may be detected later. So this check to completely stop registering controller is not a good idea. Note that userspace can tell kernel (via sysfs) to rescan all PCIe buses and try to discover new PCIea devices.

Quoted 5 lines

Thus, We add starfive_pcie_host_wait_for_link() to poll if it is a empty slot. If nothing here, we will exit the probe() of this controller, and it will not go into pci_host_probe() too.
This may not be a very standard logic, should we remove it or rewrite in a better way?

Quoted 1 line

Quoted 1 line

Bjorn

Rather to remove this starfive_pcie_host_wait_for_link logic.

Better option would be to teach PCI core code to wait for the link before trying to read vendor/device ids, like I described in my old proposal.

Add StarFive JH7110 SoC PCIe controller platform
driver codes.

Signed-off-by Minda Chen <[email protected]>

Reviewed-by Mason Huo <[email protected]>

---
 MAINTAINERS                                 |   7 +
 drivers/pci/controller/plda/Kconfig         |   8 +
 drivers/pci/controller/plda/Makefile        |   1 +
 drivers/pci/controller/plda/pcie-plda.h     |  58 ++-
 drivers/pci/controller/plda/pcie-starfive.c | 415 ++++++++++++++++++++
 5 files changed, 487 insertions(+), 2 deletions(-)
 create mode 100644 drivers/pci/controller/plda/pcie-starfive.c

MAINTAINERS +7 -0

index f02618c2bdf5..b88a54a24ae5 100644
@@ -20356,6 +20356,13 @@ S:	Supported
 F:	Documentation/devicetree/bindings/watchdog/starfive*
 F:	drivers/watchdog/starfive-wdt.c
 
+STARFIVE JH71x0 PCIE DRIVER
+M:	Minda Chen <[email protected]>
+L:	[email protected]
+S:	Supported
+F:	Documentation/devicetree/bindings/pci/starfive*
+F:	drivers/pci/controller/plda/pcie-starfive.c
+
 STATIC BRANCH/CALL
 M:	Peter Zijlstra <[email protected]>
 M:	Josh Poimboeuf <[email protected]>

drivers/pci/controller/plda/Kconfig +0 -0

index a3c790545843..eaf72954da9f 100644

drivers/pci/controller/plda/Kconfig +5 -0

+	help
+	  Say Y here if you want to support the StarFive PCIe controller
+	  in host mode. StarFive PCIe controller uses PLDA PCIe
+	  core.
+
 endmenu

Quoted 40 lines · Minda Chen

Add StarFive JH7110 SoC PCIe controller platform
driver codes.

Signed-off-by Minda Chen <[email protected]>

Reviewed-by Mason Huo <[email protected]>

---
 MAINTAINERS                                 |   7 +
 drivers/pci/controller/plda/Kconfig         |   8 +
 drivers/pci/controller/plda/Makefile        |   1 +
 drivers/pci/controller/plda/pcie-plda.h     |  58 ++-
 drivers/pci/controller/plda/pcie-starfive.c | 415 ++++++++++++++++++++
 5 files changed, 487 insertions(+), 2 deletions(-)
 create mode 100644 drivers/pci/controller/plda/pcie-starfive.c

MAINTAINERS +7 -0

index f02618c2bdf5..b88a54a24ae5 100644
@@ -20356,6 +20356,13 @@ S:	Supported
 F:	Documentation/devicetree/bindings/watchdog/starfive*
 F:	drivers/watchdog/starfive-wdt.c
 
+STARFIVE JH71x0 PCIE DRIVER
+M:	Minda Chen <[email protected]>
+L:	[email protected]
+S:	Supported
+F:	Documentation/devicetree/bindings/pci/starfive*
+F:	drivers/pci/controller/plda/pcie-starfive.c
+
 STATIC BRANCH/CALL
 M:	Peter Zijlstra <[email protected]>
 M:	Josh Poimboeuf <[email protected]>

drivers/pci/controller/plda/Kconfig +0 -0

index a3c790545843..eaf72954da9f 100644
@@ -24,4 +24,12 @@ config PCIE_MICROCHIP_HOST
 	  Say Y here if you want kernel to support the Microchip AXI PCIe
 	  Host Bridge driver.
 

drivers/pci/controller/plda/Kconfig config PCIE_MICROCHIP_HOST · line 29

Quoted 3 lines

drivers/pci/controller/plda/Kconfig +3 -0

+config PCIE_STARFIVE_HOST
+	tristate "StarFive PCIe host controller"
+	select PCIE_PLDA_HOST

Ditto here, I think this suffers from the same issue, although its probably only really randconfigs that'll trigger it.

Quoted 6 lines

drivers/pci/controller/plda/Kconfig +5 -0

+	help
+	  Say Y here if you want to support the StarFive PCIe controller
+	  in host mode. StarFive PCIe controller uses PLDA PCIe
+	  core.
+
 endmenu

This patchset final purpose is add PCIe driver for StarFive JH7110 SoC.
dt-bindings: PCI: Add PLDA XpressRICH PCIe host common properties
JH7110 using PLDA XpressRICH PCIe IP. Microchip PolarFire Using the same IP and have commit their codes, which are mixed with PLDA controller codes and Microchip platform codes.

For re-use the PLDA controller codes, I request refactoring microchip codes, move PLDA common codes to PLDA files.
Desigware and Cadence is good example for refactoring codes.

So first step is extract the PLDA common codes from microchip, and refactoring the microchip codes.(patch1 - 4)
Then add the PLDA platform codes. (patch5, 6)
At last, add Starfive codes. (patch7 - 9)

This patchset is base on v6.5-rc1

dt-bindings: PCI: Add PLDA XpressRICH PCIe host common properties
dt-bindings: PCI: microchip: Remove the PLDA common properties
PCI: PLDA: Get PLDA common codes from Microchip PolarFire host
PCI: microchip: Move PCIe driver to PLDA directory
dt-bindings: PLDA: Add PLDA XpressRICH PCIe host controller
PCI: PLDA: Add host conroller platform driver

dt-bindings: PCI: Add StarFive JH7110 PCIe controller
PCI: PLDA: starfive: Add JH7110 PCIe controller
riscv: dts: starfive: add PCIe dts configuration for JH7110

Quoted 5 lines · Minda Chen

This patchset final purpose is add PCIe driver for StarFive JH7110 SoC.
dt-bindings: PCI: Add PLDA XpressRICH PCIe host common properties
JH7110 using PLDA XpressRICH PCIe IP. Microchip PolarFire Using the same IP and have commit their codes, which are mixed with PLDA controller codes and Microchip platform codes.

I guess this actually adds TWO drivers: PCIE_PLDA_PLAT_HOST (claims "plda,xpressrich-pcie-host" devices) and PCIE_STARFIVE_HOST (claims "starfive,jh7110-pcie" devices), right?

Quoted 4 lines

For re-use the PLDA controller codes, I request refactoring microchip codes, move PLDA common codes to PLDA files.
Desigware and Cadence is good example for refactoring codes.

Quoted 6 lines

So first step is extract the PLDA common codes from microchip, and refactoring the microchip codes.(patch1 - 4)
Then add the PLDA platform codes. (patch5, 6)
At last, add Starfive codes. (patch7 - 9)

This patchset is base on v6.5-rc1

Doesn't quite apply cleanly for me:

10:10:15 ~/linux (main)$ git checkout -b wip/minda-starfive-v1 v6.5-rc1
Switched to a new branch 'wip/minda-starfive-v1'
10:10:33 ~/linux (wip/minda-starfive-v1)$ git am m/20230719_minda_chen_refactoring_microchip_polarfire_pcie_driver.mbx
Applying: dt-bindings: PCI: Add PLDA XpressRICH PCIe host common properties
Applying: dt-bindings: PCI: microchip: Remove the PLDA common properties
Applying: PCI: PLDA: Get PLDA common codes from Microchip PolarFire host
Applying: PCI: microchip: Move PCIe driver to PLDA directory
Applying: dt-bindings: PLDA: Add PLDA XpressRICH PCIe host controller
Applying: PCI: PLDA: Add host conroller platform driver
Applying: dt-bindings: PCI: Add StarFive JH7110 PCIe controller
Applying: PCI: PLDA: starfive: Add JH7110 PCIe controller
Applying: riscv: dts: starfive: add PCIe dts configuration for JH7110
error: patch failed: arch/riscv/boot/dts/starfive/jh7110.dtsi:629
error: arch/riscv/boot/dts/starfive/jh7110.dtsi: patch does not apply
Patch failed at 0009 riscv: dts: starfive: add PCIe dts configuration for JH7110

Quoted 6 lines

dt-bindings: PCI: Add PLDA XpressRICH PCIe host common properties
dt-bindings: PCI: microchip: Remove the PLDA common properties
PCI: PLDA: Get PLDA common codes from Microchip PolarFire host
PCI: microchip: Move PCIe driver to PLDA directory
dt-bindings: PLDA: Add PLDA XpressRICH PCIe host controller
PCI: PLDA: Add host conroller platform driver

"controller"
ok

Quoted 3 lines

dt-bindings: PCI: Add StarFive JH7110 PCIe controller
PCI: PLDA: starfive: Add JH7110 PCIe controller
riscv: dts: starfive: add PCIe dts configuration for JH7110

Use "PCI: plda: " prefix for PLDA things that are shared across multiple drivers.

Use "PCI: starfive: " prefix for starfive-specific things.

This is the same as how drivers/pci/controller/dwc/* looks.

Bjorn

Quoted 4 lines · Bjorn Helgaas

Quoted 1 line · Minda Chen

This patchset is base on v6.5-rc1

Quoted 2 lines

Doesn't quite apply cleanly for me:

I am sorry, The driver need stg clk and syscon driver, which are have't be merge to main line. mainly dts is(patch9) rejected, Must apply this series patch first. (I forget add this link in cover letter)
https://patchwork.kernel.org/project/linux-riscv/cover/[email protected]/
and this syscon patch
https://patchwork.kernel.org/project/linux-riscv/patch/[email protected]/

Quoted 5 lines · Minda Chen

Quoted 4 lines · Bjorn Helgaas

Quoted 1 line · Minda Chen

This patchset is base on v6.5-rc1

Quoted 7 lines

Quoted 2 lines

Doesn't quite apply cleanly for me:

I am sorry, The driver need stg clk and syscon driver, which are have't be merge to main line. mainly dts is(patch9) rejected, Must apply this series patch first. (I forget add this link in cover letter)
https://patchwork.kernel.org/project/linux-riscv/cover/[email protected]/
and this syscon patch
https://patchwork.kernel.org/project/linux-riscv/patch/[email protected]/

You could detach the dts patch from the series & send it independently once everything it depends on is in place. I'm going to pick up both of the patches you've linked for v6.6 in the next day or two.

Quoted 5 lines · Minda Chen

Quoted 4 lines · Bjorn Helgaas

Quoted 1 line · Minda Chen

This patchset is base on v6.5-rc1

Quoted 7 lines

Quoted 2 lines

Doesn't quite apply cleanly for me:

I am sorry, The driver need stg clk and syscon driver, which are have't be merge to main line. mainly dts is(patch9) rejected, Must apply this series patch first. (I forget add this link in cover letter)
https://patchwork.kernel.org/project/linux-riscv/cover/[email protected]/
and this syscon patch
https://patchwork.kernel.org/project/linux-riscv/patch/[email protected]/

You could detach the dts patch from the series & send it independently once everything it depends on is in place. I'm going to pick up both of the patches you've linked for v6.6 in the next day or two.

This patchset final purpose is add PCIe driver for StarFive JH7110 SoC. JH7110 using PLDA XpressRICH PCIe IP. Microchip PolarFire Using the same IP and have commit their codes, which are mixed with PLDA controller codes and Microchip platform codes.

For re-use the PLDA controller codes, I request refactoring microchip codes, move PLDA common codes to PLDA files.
Desigware and Cadence is good example for refactoring codes.

So first step is extract the PLDA common codes from microchip, and refactoring the microchip codes.(patch1 - 4)
Then add the PLDA platform codes. (patch5, 6)
At last, add Starfive codes. (patch7 - 9)

This patchset is base on v6.5-rc1

patch1 is add PLDA XpressRICH PCIe host common properties dt-binding
docs, most are extracted from microchip,pcie-host.yaml
patch2 is add plda,xpressrich-pcie-common.yaml(patch1 file) reference
and remove the PLDA common properties.
patch3 is extracting the PLDA common codes from microchip Polarfire PCIe
codes. The change list in the commit message.
patch4 is move microchip driver to PLDA directory and remove the PLDA
common codes.
patch5 is add PLDA Xpressrich platform driver dt-binding doc. patch6 is PLDA Xpressrich platform driver.
patch7 is add StarFive JH7110 PCIe dt-binding doc.
patch8 is add StarFive JH7110 Soc PCIe platform codes. patch9 is StarFive JH7110 device tree configuration.

I have noticed that Daire have changed microchip's codes.
https://patchwork.kernel.org/project/linux-pci/cover/[email protected]/