Add support for Hikey 970 PCIe

On HiKey970, there's a PEX 8606 PCI bridge on its PHY with 6 lanes. Only 4 lanes are connected:

	lane 0 - connected to Kirin 970;
	lane 4 - M.2 slot;
	lane 5 - mini PCIe slot;
	lane 6 - in-board Ethernet controller.

Each lane has its own PERST# gpio pin, and needs a clock request.

Add support to parse a DT schema containing the above data.

Cc Kishon Vijay Abraham I <[email protected]>

Acked-by Xiaowei Song <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/[email protected]/

 drivers/pci/controller/dwc/pcie-kirin.c | 262 +++++++++++++++++++++---
 1 file changed, 231 insertions(+), 31 deletions(-)

drivers/pci/controller/dwc/pcie-kirin.c +27 -0

index 86c13661e02d..de375795a3b8 100644
@@ -52,6 +52,19 @@
 #define PCIE_DEBOUNCE_PARAM	0xF0F400
 #define PCIE_OE_BYPASS		(0x3 << 28)
 
+/*
+ * Max number of connected PCI slots at an external PCI bridge
+ *
+ * This is used on HiKey 970, which has a PEX 8606 bridge with has
+ * 4 connected lanes (lane 0 upstream, and the other tree lanes,
+ * one connected to an in-board Ethernet adapter and the other two
+ * connected to M.2 and mini PCI slots.
+ *
+ * Each slot has a different clock source and uses a separate PERST#
+ * pin.
...
 
+static int kirin_pcie_add_bus(struct pci_bus *bus)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata);
+	struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
+	int i, ret;
+
+	if (!kirin_pcie->num_slots)
+		return 0;
+
+	/* Send PERST# to each slot */
+	for (i = 0; i < kirin_pcie->num_slots; i++) {
+		ret = gpio_direction_output(kirin_pcie->gpio_id_reset[i], 1);
+		if (ret) {
+			dev_err(pci->dev, "PERST# %s error: %d\n",
+				kirin_pcie->reset_names[i], ret);
+		}
+	}

Quoted 60 lines · Mauro Carvalho Chehab

On HiKey970, there's a PEX 8606 PCI bridge on its PHY with 6 lanes. Only 4 lanes are connected:

	lane 0 - connected to Kirin 970;
	lane 4 - M.2 slot;
	lane 5 - mini PCIe slot;
	lane 6 - in-board Ethernet controller.

Each lane has its own PERST# gpio pin, and needs a clock request.

Add support to parse a DT schema containing the above data.

Cc Kishon Vijay Abraham I <[email protected]>

Acked-by Xiaowei Song <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/[email protected]/

 drivers/pci/controller/dwc/pcie-kirin.c | 262 +++++++++++++++++++++---
 1 file changed, 231 insertions(+), 31 deletions(-)

drivers/pci/controller/dwc/pcie-kirin.c +27 -0

index 86c13661e02d..de375795a3b8 100644
@@ -52,6 +52,19 @@
 #define PCIE_DEBOUNCE_PARAM	0xF0F400
 #define PCIE_OE_BYPASS		(0x3 << 28)
 
+/*
+ * Max number of connected PCI slots at an external PCI bridge
+ *
+ * This is used on HiKey 970, which has a PEX 8606 bridge with has
+ * 4 connected lanes (lane 0 upstream, and the other tree lanes,
+ * one connected to an in-board Ethernet adapter and the other two
+ * connected to M.2 and mini PCI slots.
+ *
+ * Each slot has a different clock source and uses a separate PERST#
+ * pin.
...
 
+static int kirin_pcie_add_bus(struct pci_bus *bus)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata);
+	struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
+	int i, ret;
+
+	if (!kirin_pcie->num_slots)
+		return 0;
+
+	/* Send PERST# to each slot */
+	for (i = 0; i < kirin_pcie->num_slots; i++) {
+		ret = gpio_direction_output(kirin_pcie->gpio_id_reset[i], 1);
+		if (ret) {
+			dev_err(pci->dev, "PERST# %s error: %d\n",
+				kirin_pcie->reset_names[i], ret);
+		}
+	}

drivers/pci/controller/dwc/pcie-kirin.c

Quoted 10 lines

drivers/pci/controller/dwc/pcie-kirin.c +7 -0

+	usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
+
+	return 0;
+}
+
+
 static struct pci_ops kirin_pci_ops = {
 	.read = kirin_pcie_rd_own_conf,
 	.write = kirin_pcie_wr_own_conf,
+	.add_bus = kirin_pcie_add_bus,

This seems a little weird. Can you educate me?

From [1], it looks like the topology here is:

00:00.0 Root Port
01:00.0 PEX 8606 Upstream Port
02:01.0 PEX 8606 Downstream Port
02:04.0 PEX 8606 Downstream Port
02:05.0 PEX 8606 Downstream Port
02:07.0 PEX 8606 Downstream Port
02:09.0 PEX 8606 Downstream Port
06:00.0 RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller

The .add_bus() callback will be called for *every* child bus we want to enumerate. So if any of those PEX 8606 Downstream Ports are connected to another switch, when we enumerate the secondary buses of that other switch, it looks like we'll send PERST# to all the slots again, which doesn't sound right. Am I missing something?

Bjorn

[1] https://lore.kernel.org/all/[email protected]/

Quoted 60 lines

On HiKey970, there's a PEX 8606 PCI bridge on its PHY with 6 lanes. Only 4 lanes are connected:

	lane 0 - connected to Kirin 970;
	lane 4 - M.2 slot;
	lane 5 - mini PCIe slot;
	lane 6 - in-board Ethernet controller.

Each lane has its own PERST# gpio pin, and needs a clock request.

Add support to parse a DT schema containing the above data.

Cc Kishon Vijay Abraham I <[email protected]>

Acked-by Xiaowei Song <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/[email protected]/

 drivers/pci/controller/dwc/pcie-kirin.c | 262 +++++++++++++++++++++---
 1 file changed, 231 insertions(+), 31 deletions(-)

drivers/pci/controller/dwc/pcie-kirin.c +27 -0

index 86c13661e02d..de375795a3b8 100644
@@ -52,6 +52,19 @@
 #define PCIE_DEBOUNCE_PARAM	0xF0F400
 #define PCIE_OE_BYPASS		(0x3 << 28)
 
+/*
+ * Max number of connected PCI slots at an external PCI bridge
+ *
+ * This is used on HiKey 970, which has a PEX 8606 bridge with has
+ * 4 connected lanes (lane 0 upstream, and the other tree lanes,
+ * one connected to an in-board Ethernet adapter and the other two
+ * connected to M.2 and mini PCI slots.
+ *
+ * Each slot has a different clock source and uses a separate PERST#
+ * pin.
...
 
+static int kirin_pcie_add_bus(struct pci_bus *bus)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata);
+	struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
+	int i, ret;
+
+	if (!kirin_pcie->num_slots)
+		return 0;
+
+	/* Send PERST# to each slot */
+	for (i = 0; i < kirin_pcie->num_slots; i++) {
+		ret = gpio_direction_output(kirin_pcie->gpio_id_reset[i], 1);
+		if (ret) {
+			dev_err(pci->dev, "PERST# %s error: %d\n",
+				kirin_pcie->reset_names[i], ret);
+		}
+	}

drivers/pci/controller/dwc/pcie-kirin.c

Quoted 10 lines

drivers/pci/controller/dwc/pcie-kirin.c +7 -0

+	usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
+
+	return 0;
+}
+
+
 static struct pci_ops kirin_pci_ops = {
 	.read = kirin_pcie_rd_own_conf,
 	.write = kirin_pcie_wr_own_conf,
+	.add_bus = kirin_pcie_add_bus,  

This seems a little weird. Can you educate me?

From [1], it looks like the topology here is:

00:00.0 Root Port
01:00.0 PEX 8606 Upstream Port
02:01.0 PEX 8606 Downstream Port
02:04.0 PEX 8606 Downstream Port
02:05.0 PEX 8606 Downstream Port
02:07.0 PEX 8606 Downstream Port
02:09.0 PEX 8606 Downstream Port
06:00.0 RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller

The .add_bus() callback will be called for *every* child bus we want to enumerate. So if any of those PEX 8606 Downstream Ports are connected to another switch, when we enumerate the secondary buses of that other switch, it looks like we'll send PERST# to all the slots again, which doesn't sound right. Am I missing something?

Em Tue, 24 May 2022 12:19:47 -0500
Bjorn Helgaas <[email protected]> escreveu:

Quoted 85 lines · Mauro Carvalho Chehab

Quoted 60 lines

On HiKey970, there's a PEX 8606 PCI bridge on its PHY with 6 lanes. Only 4 lanes are connected:

	lane 0 - connected to Kirin 970;
	lane 4 - M.2 slot;
	lane 5 - mini PCIe slot;
	lane 6 - in-board Ethernet controller.

Each lane has its own PERST# gpio pin, and needs a clock request.

Add support to parse a DT schema containing the above data.

Cc Kishon Vijay Abraham I <[email protected]>

Acked-by Xiaowei Song <[email protected]>

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/[email protected]/

 drivers/pci/controller/dwc/pcie-kirin.c | 262 +++++++++++++++++++++---
 1 file changed, 231 insertions(+), 31 deletions(-)

drivers/pci/controller/dwc/pcie-kirin.c +27 -0

index 86c13661e02d..de375795a3b8 100644
@@ -52,6 +52,19 @@
 #define PCIE_DEBOUNCE_PARAM	0xF0F400
 #define PCIE_OE_BYPASS		(0x3 << 28)
 
+/*
+ * Max number of connected PCI slots at an external PCI bridge
+ *
+ * This is used on HiKey 970, which has a PEX 8606 bridge with has
+ * 4 connected lanes (lane 0 upstream, and the other tree lanes,
+ * one connected to an in-board Ethernet adapter and the other two
+ * connected to M.2 and mini PCI slots.
+ *
+ * Each slot has a different clock source and uses a separate PERST#
+ * pin.
...
 
+static int kirin_pcie_add_bus(struct pci_bus *bus)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(bus->sysdata);
+	struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
+	int i, ret;
+
+	if (!kirin_pcie->num_slots)
+		return 0;
+
+	/* Send PERST# to each slot */
+	for (i = 0; i < kirin_pcie->num_slots; i++) {
+		ret = gpio_direction_output(kirin_pcie->gpio_id_reset[i], 1);
+		if (ret) {
+			dev_err(pci->dev, "PERST# %s error: %d\n",
+				kirin_pcie->reset_names[i], ret);
+		}
+	}

drivers/pci/controller/dwc/pcie-kirin.c

Quoted 10 lines

drivers/pci/controller/dwc/pcie-kirin.c +7 -0

+	usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
+
+	return 0;
+}
+
+
 static struct pci_ops kirin_pci_ops = {
 	.read = kirin_pcie_rd_own_conf,
 	.write = kirin_pcie_wr_own_conf,
+	.add_bus = kirin_pcie_add_bus,  

This seems a little weird. Can you educate me?

From [1], it looks like the topology here is:

00:00.0 Root Port
01:00.0 PEX 8606 Upstream Port
02:01.0 PEX 8606 Downstream Port
02:04.0 PEX 8606 Downstream Port
02:05.0 PEX 8606 Downstream Port
02:07.0 PEX 8606 Downstream Port
02:09.0 PEX 8606 Downstream Port
06:00.0 RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller

Quoted 5 lines

The .add_bus() callback will be called for *every* child bus we want to enumerate. So if any of those PEX 8606 Downstream Ports are connected to another switch, when we enumerate the secondary buses of that other switch, it looks like we'll send PERST# to all the slots again, which doesn't sound right. Am I missing something?

The implementation made on Kirin 960/970 for PCI is to not have a PERST# bus. Instead, it has one independent GPIO driving the PERST# signal for each single individual port that is physically connected.

See the schematics at:

- https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf
- https://github.com/96boards/documentation/blob/master/consumer/hikey/hikey960/hardware-docs/HiKey960_Schematics.pdf

Yet, my first proposal were to send all GPIOs altogether. See, for instance:

	https://lore.kernel.org/all/3acf2c073e8ea67efaae91074dda0763bf7a2ab9.1626768323.git.mchehab+huawei@kernel.org/

There, the PERST# signals are initialized altogether, at the end of hi3670_pcie_phy_power_on():

	/* perst assert Endpoints */
	usleep_range(21000, 23000);
	for (i = 0; i < phy->n_gpio_resets; i++) {
		ret = gpio_direction_output(phy->gpio_id_reset[i], 1);
		if (ret)
			return ret;
	}
	usleep_range(10000, 11000);

	ret = is_pipe_clk_stable(phy);
	if (!ret)
		goto disable_clks;

	hi3670_pcie_set_eyeparam(phy);

	ret = hi3670_pcie_noc_power(phy, false);
	if (ret)
		goto disable_clks;

During the review process, I was requested to change it in order to do it via .add_bus.

On my tests at the boards, I didn't see the same PERST# signal to hit more than once, and the driver is working fine. So, this wasn't an actual issue, as far as I can tell.

Before code refactor, the PERST# signals were sent at the end of the power_on logic. Then, the PCI core would probe for the buses and add them.

The new logic changed it to send PERST# signals during add_bus operation. That altered the timings.

Also, HiKey 970 require a little more waiting time for the PCI bridge - which is outside the SoC - to finish the PERST# reset, and then initialize the eye diagram.

So, increase the waiting time for the PERST# signals to what's required for it to also work with HiKey 970.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/[email protected]/

 drivers/pci/controller/dwc/pcie-kirin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/pci/controller/dwc/pcie-kirin.c +1 -1

index de375795a3b8..bc329673632a 100644
@@ -113,7 +113,7 @@ struct kirin_pcie {
 #define CRGCTRL_PCIE_ASSERT_BIT		0x8c000000
 
 /* Time for delay */
-#define REF_2_PERST_MIN		20000
+#define REF_2_PERST_MIN		21000
 #define REF_2_PERST_MAX		25000
 #define PERST_2_ACCESS_MIN	10000
 #define PERST_2_ACCESS_MAX	12000

Signature · 1 line

2.31.1

Quoted 11 lines

Before code refactor, the PERST# signals were sent at the end of the power_on logic. Then, the PCI core would probe for the buses and add them.

The new logic changed it to send PERST# signals during add_bus operation. That altered the timings.

Also, HiKey 970 require a little more waiting time for the PCI bridge - which is outside the SoC - to finish the PERST# reset, and then initialize the eye diagram.

Ok, now you explained it and we should move this explanation in the commit log that this change is affecting (I mean we should squash this patch with the patch that actually requires it
- I am not sure whether it is patch 6 or another one).

I can do it for you; I thought it would be a standalone change but it actually isn't, because it is brought about by the changes you are making and therefore there it belongs.

Thanks for explaining it and apologies for the churn.

Lorenzo

Quoted 27 lines

So, increase the waiting time for the PERST# signals to what's required for it to also work with HiKey 970.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/[email protected]/

 drivers/pci/controller/dwc/pcie-kirin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/pci/controller/dwc/pcie-kirin.c +1 -1

index de375795a3b8..bc329673632a 100644
@@ -113,7 +113,7 @@ struct kirin_pcie {
 #define CRGCTRL_PCIE_ASSERT_BIT		0x8c000000
 
 /* Time for delay */
-#define REF_2_PERST_MIN		20000
+#define REF_2_PERST_MIN		21000
 #define REF_2_PERST_MAX		25000
 #define PERST_2_ACCESS_MIN	10000
 #define PERST_2_ACCESS_MAX	12000

Signature · 1 line

2.31.1

Before code refactor, the PERST# signals were sent at the end of the power_on logic. Then, the PCI core would probe for the buses and add them.

The new logic changed it to send PERST# signals during add_bus operation. That altered the timings.

Also, HiKey 970 require a little more waiting time for the PCI bridge - which is outside the SoC - to finish the PERST# reset, and then initialize the eye diagram.

So, increase the waiting time for the PERST# signals to what's required for it to also work with HiKey 970.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/[email protected]/

 drivers/pci/controller/dwc/pcie-kirin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/pci/controller/dwc/pcie-kirin.c +1 -1

index de375795a3b8..bc329673632a 100644
@@ -113,7 +113,7 @@ struct kirin_pcie {
 #define CRGCTRL_PCIE_ASSERT_BIT		0x8c000000
 
 /* Time for delay */
-#define REF_2_PERST_MIN		20000
+#define REF_2_PERST_MIN		21000
 #define REF_2_PERST_MAX		25000
 #define PERST_2_ACCESS_MIN	10000
 #define PERST_2_ACCESS_MAX	12000

Signature · 1 line

2.31.1

Quoted 10 lines

Before code refactor, the PERST# signals were sent at the end of the power_on logic. Then, the PCI core would probe for the buses and add them.

The new logic changed it to send PERST# signals during add_bus operation. That altered the timings.

Also, HiKey 970 require a little more waiting time for the PCI bridge - which is outside the SoC - to finish the PERST# reset, and then initialize the eye diagram.

Hello! Which PCIe port do you mean by PCI bridge device? Do you mean PCIe Root Port? Or upstream port on some external PCIe switch connected via PCIe bus to the PCIe Root Port? Because all of these (virtual) PCIe devices are presented as PCI bridge devices, so it is not clear to which device it refers.

Normally PERST# signal is used to reset endpoint card, other end of PCIe link and so PERST# signal should not affect PCIe Root Port at all.

Quoted 2 lines

So, increase the waiting time for the PERST# signals to what's required for it to also work with HiKey 970.

Because PERST# signal resets endpoint card, this reset timeout should not be driver or controller specific.

Mauro, if you understand this issue more deeply, could you look at my email? https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/

I think that kernel PCI subsystem does not properly handle PCIe Warm Reset and correct initialization of endpoint cards. Because similar "random PERST# timeout patches" were applied to lot of native controller drivers.

PS: I'm not opposing this patch, I'm just trying to understand what is happening here and why particular number "21000" was chosen. It is defined in some standard? Or was it just randomly chosen and measures that with this number is initialization working fine?

Quoted 25 lines

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/[email protected]/

 drivers/pci/controller/dwc/pcie-kirin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/pci/controller/dwc/pcie-kirin.c +1 -1

index de375795a3b8..bc329673632a 100644
@@ -113,7 +113,7 @@ struct kirin_pcie {
 #define CRGCTRL_PCIE_ASSERT_BIT		0x8c000000
 
 /* Time for delay */
-#define REF_2_PERST_MIN		20000
+#define REF_2_PERST_MIN		21000
 #define REF_2_PERST_MAX		25000
 #define PERST_2_ACCESS_MIN	10000
 #define PERST_2_ACCESS_MAX	12000

Signature · 1 line

2.31.1

Hi Pali,

Em Fri, 22 Oct 2021 17:16:24 +0200
Pali Rohár <[email protected]> escreveu:

Quoted 12 lines · On Tuesday 19 October 2021 07:06:42 Mauro Carvalho Chehab wrote:

Quoted 10 lines

Before code refactor, the PERST# signals were sent at the end of the power_on logic. Then, the PCI core would probe for the buses and add them.

The new logic changed it to send PERST# signals during add_bus operation. That altered the timings.

Also, HiKey 970 require a little more waiting time for the PCI bridge - which is outside the SoC - to finish the PERST# reset, and then initialize the eye diagram.

Quoted 5 lines

Hello! Which PCIe port do you mean by PCI bridge device? Do you mean PCIe Root Port? Or upstream port on some external PCIe switch connected via PCIe bus to the PCIe Root Port? Because all of these (virtual) PCIe devices are presented as PCI bridge devices, so it is not clear to which device it refers.

HiKey 970 uses an external PCI bridge chipset (a Broadcom PEX 8606[1]),

with 3 elements connected to the bus: an Ethernet card, a M.2 slot and a mini PCIe slot. It seems HiKey 970 is unique with regards to PERST# signal, as there are 4 independent PERST# signals there:

- one for PEX 8606 (the PCIe root port);
- one for Ethernet;
- one for M.2;
- one for mini-PCIe.

After sending the PCIe PERST# signals, the device has to wait for 21 ms before adjusting the eye diagram.

[1] https://docs.broadcom.com/docs/PEX_8606_AIC_RDK_HRM_v1.3_06Aug10.pdf

Quoted 2 lines

Normally PERST# signal is used to reset endpoint card, other end of PCIe link and so PERST# signal should not affect PCIe Root Port at all.

That's not the case, as PEX 8606 needs to complete its reset sequence for the rest of the devices to be visible. If the wait time is reduced or removed, the devices behind it won't be detected.

Quoted 5 lines

Quoted 2 lines

So, increase the waiting time for the PERST# signals to what's required for it to also work with HiKey 970.

Because PERST# signal resets endpoint card, this reset timeout should not be driver or controller specific.

Not sure if it would be possible to implement it at the core without breaking devices like this one where there's a separate chip to actually implement the PCIe bus.

Quoted 7 lines

Mauro, if you understand this issue more deeply, could you look at my email? https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/

I think that kernel PCI subsystem does not properly handle PCIe Warm Reset and correct initialization of endpoint cards. Because similar "random PERST# timeout patches" were applied to lot of native controller drivers.

I don't know enough about PCIe documentation in order to help with that. Yet, if the PCI/PCIe specs doesn't define a maximum time for PERST# to finish, hardware manufacturers will do whatever they please. So, finding a common value is impossible.

Well, even if specs define it, vendors may still violate that. So, whatever implementation is done, some quirks may be needed.

Sending PERST# signals to the devices connected to the bridge too early will cause the bridge to not detect the devices behind it. That's what happens with HiKey 970: lower reset values cause it to miss devices.

Looking from harware perspective, I'd say that the reset time pretty much depends on how the PCIe bridges are implemented: if it is FPGA, it is probably slower than if it is a dedicated hardware. It can be even slower if the bridge uses a microcontroller and needs to read the firmware from some place.

Quoted 4 lines

PS: I'm not opposing this patch, I'm just trying to understand what is happening here and why particular number "21000" was chosen. It is defined in some standard? Or was it just randomly chosen and measures that with this number is initialization working fine?

It is the value used by the HiKey 970 PCIe out-of-tree driver. The patch which added support for it at the pcie-kirin increased the time out there.

I tried to preserve the previous value, but that cause some devices to be missed during PCI probe time.

Btw, PEX 8606 datasheet says:

`Reset Circuit

The PEX 8606BA-AIC1U1D RDK accepts a PERST# from the host PC via card edge connector P1. This signal is OR’d with a manual reset circuit. The manual reset circuit consists of a pushbutton (SW7, upper left corner) that feeds into a reset timer. The reset timer monitors its power rail and reset input. If the reset input is low or the supply rail is out of range, the reset output is held. Once both conditions no longer exist, the reset output will de- assert after a programmable reset timeout period (capacitor adjustable, default value 128 msec). The OR’d reset signal goes to the PEX 8606 device’s PEX_PERST# input pin, and the downstream slots’ PERST# connector pins. PERST# to Slot J1 can be controlled by the PEX 8606 device’s Hot-Plug interface.'

If I understood it well, the PERST# time is hardware-configurable, by changing the value of a capacitor.

Regards,
Mauro

Quoted 25 lines

Quoted 25 lines

Signed-off-by Mauro Carvalho Chehab <[email protected]>

---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/[email protected]/

 drivers/pci/controller/dwc/pcie-kirin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/pci/controller/dwc/pcie-kirin.c +1 -1

index de375795a3b8..bc329673632a 100644
@@ -113,7 +113,7 @@ struct kirin_pcie {
 #define CRGCTRL_PCIE_ASSERT_BIT		0x8c000000
 
 /* Time for delay */
-#define REF_2_PERST_MIN		20000
+#define REF_2_PERST_MIN		21000
 #define REF_2_PERST_MAX		25000
 #define PERST_2_ACCESS_MIN	10000
 #define PERST_2_ACCESS_MAX	12000

Signature · 1 line

2.31.1

Hello!

Quoted 23 lines · On Saturday 23 October 2021 10:30:59 Mauro Carvalho Chehab wrote:

Hi Pali,

Em Fri, 22 Oct 2021 17:16:24 +0200
Pali Rohár <[email protected]> escreveu:

Quoted 17 lines · On Tuesday 19 October 2021 07:06:42 Mauro Carvalho Chehab wrote:

Quoted 10 lines

Before code refactor, the PERST# signals were sent at the end of the power_on logic. Then, the PCI core would probe for the buses and add them.

The new logic changed it to send PERST# signals during add_bus operation. That altered the timings.

Also, HiKey 970 require a little more waiting time for the PCI bridge - which is outside the SoC - to finish the PERST# reset, and then initialize the eye diagram.

Hello! Which PCIe port do you mean by PCI bridge device? Do you mean PCIe Root Port? Or upstream port on some external PCIe switch connected via PCIe bus to the PCIe Root Port? Because all of these (virtual) PCIe devices are presented as PCI bridge devices, so it is not clear to which device it refers.

Quoted 1 line

HiKey 970 uses an external PCI bridge chipset (a Broadcom PEX 8606[1]),

Ok! Now I understood. You have probably one PCIe Root Port on your board and to this port you have connected (external) PCIe switch card from Broadcom to increase number of PCIe ports for endpoint cards.

It is classic setup for boards with just one PCIe port.

Quoted 8 lines

with 3 elements connected to the bus: an Ethernet card, a M.2 slot and a mini PCIe slot. It seems HiKey 970 is unique with regards to PERST# signal, as there are 4 independent PERST# signals there:

- one for PEX 8606 (the PCIe root port);
- one for Ethernet;
- one for M.2;
- one for mini-PCIe.

This is not unique setup, its pretty normal. Every PCIe card has (own) PERST# pin and obviously you want to control each pin separately via SW. And because PCIe switch is also (upstream) PCIe device it has also PERST# pin.

Quoted 3 lines

After sending the PCIe PERST# signals, the device has to wait for 21 ms before adjusting the eye diagram.

"the device" which has to wait is HiKey970 or PEX8606?

Quoted 8 lines

[1] https://docs.broadcom.com/docs/PEX_8606_AIC_RDK_HRM_v1.3_06Aug10.pdf

Quoted 2 lines

Normally PERST# signal is used to reset endpoint card, other end of PCIe link and so PERST# signal should not affect PCIe Root Port at all.

That's not the case, as PEX 8606 needs to complete its reset sequence for the rest of the devices to be visible. If the wait time is reduced or removed, the devices behind it won't be detected.

Well, "endpoint card" for HiKey970 PCIe link is here PEX8606. And if you connect PEX8606 to any other board (which could have totally different PCIe controller), it means that same wait timeouts are required for that other board.

So this wait timeout 21 ms is not HiKey970 specific, but rather PEX8606 specific, right?

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

So, increase the waiting time for the PERST# signals to what's required for it to also work with HiKey 970.

Because PERST# signal resets endpoint card, this reset timeout should not be driver or controller specific.

Not sure if it would be possible to implement it at the core without breaking devices like this one where there's a separate chip to actually implement the PCIe bus.

I think it should be possible. Probably not so easy, would need more testing, etc... But as I wrote above, this setup is not unique, it is really normal and kernel is prepared to work PCI and PCIe topologies when one or more PCIe switches, PCIe-to-PCI bridges or even more PCI-to-PCI bridges are used and connected to system board.

I send email with proposal / idea how could be PCI subsystem extended to handle initialization of native PCIe controller drivers:
https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/
(if you have some more points, feel free to reply)

Quoted 8 lines

Quoted 7 lines

Mauro, if you understand this issue more deeply, could you look at my email? https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/

I think that kernel PCI subsystem does not properly handle PCIe Warm Reset and correct initialization of endpoint cards. Because similar "random PERST# timeout patches" were applied to lot of native controller drivers.

Quoted 4 lines

I don't know enough about PCIe documentation in order to help with that. Yet, if the PCI/PCIe specs doesn't define a maximum time for PERST# to finish, hardware manufacturers will do whatever they please. So, finding a common value is impossible.

Well, it is possible that just I was not able to "find and decode" this timeout from specifications. So I'm just asking if somebody else was able to do it :-)

Quoted 2 lines

Well, even if specs define it, vendors may still violate that. So, whatever implementation is done, some quirks may be needed.

Of course, we know it and kernel has hooks and corrections for such situation. Fixes are in most cases in one place: drivers/pci/quirks.c

Quoted 3 lines

Sending PERST# signals to the devices connected to the bridge too early will cause the bridge to not detect the devices behind it. That's what happens with HiKey 970: lower reset values cause it to miss devices.

Just to make sure, that I understand your problem. Is your setup looks like this?

          +-------------------------------------PERST#--+---+
          |                                             |eth|
          |                 +------PERST#--+  +--PCIe---+---+
          |                 |              |  |
  +-------------+ +-------------+        +-------+      +---+
  |GPIO-HiKey970| |PCIe-HiKey970|--PCIe--|PEX8606|-PCIe-|m.2|
  +-------------+ +-------------+        +-------+      +---+
          |           |          +-----+        |         |
          |           +--PERST#--|mPCIe|--PCIe--+         |
          |                      +-----+                  |
          +---------------------------------------PERST#--+

And if yes, in which order you need to assert individual PERST# signals and in which order to de-assert them?

Quoted 29 lines

Looking from harware perspective, I'd say that the reset time pretty much depends on how the PCIe bridges are implemented: if it is FPGA, it is probably slower than if it is a dedicated hardware. It can be even slower if the bridge uses a microcontroller and needs to read the firmware from some place.

Quoted 4 lines

PS: I'm not opposing this patch, I'm just trying to understand what is happening here and why particular number "21000" was chosen. It is defined in some standard? Or was it just randomly chosen and measures that with this number is initialization working fine?

It is the value used by the HiKey 970 PCIe out-of-tree driver. The patch which added support for it at the pcie-kirin increased the time out there.

I tried to preserve the previous value, but that cause some devices to be missed during PCI probe time.

Btw, PEX 8606 datasheet says:

`Reset Circuit

The PEX 8606BA-AIC1U1D RDK accepts a PERST# from the host PC via card edge connector P1. This signal is OR’d with a manual reset circuit. The manual reset circuit consists of a pushbutton (SW7, upper left corner) that feeds into a reset timer. The reset timer monitors its power rail and reset input. If the reset input is low or the supply rail is out of range, the reset output is held. Once both conditions no longer exist, the reset output will de- assert after a programmable reset timeout period (capacitor adjustable, default value 128 msec). The OR’d reset signal goes to the PEX 8606 device’s PEX_PERST# input pin, and the downstream slots’ PERST# connector pins. PERST# to Slot J1 can be controlled by the PEX 8606 device’s Hot-Plug interface.'

Quoted 2 lines

If I understood it well, the PERST# time is hardware-configurable, by changing the value of a capacitor.

Hm... this is something different. It says: "Once both conditions no longer exist, the reset output will de-assert after a programmable reset timeout period (capacitor adjustable, default value 128 msec)."

I understand this part that if signal is no longer in reset that then this capacitor cause that reset is held for another 128 ms. So if host stops reset signal then it has to wait 128 ms prior doing something (to ensure that reset finished), right?

Em Sat, 23 Oct 2021 12:40:11 +0200
Pali Rohár <[email protected]> escreveu:

Quoted 11 lines

It is classic setup for boards with just one PCIe port.

Quoted 8 lines

with 3 elements connected to the bus: an Ethernet card, a M.2 slot and a mini PCIe slot. It seems HiKey 970 is unique with regards to PERST# signal, as there are 4 independent PERST# signals there:

- one for PEX 8606 (the PCIe root port);
- one for Ethernet;
- one for M.2;
- one for mini-PCIe.

Quoted 4 lines

This is not unique setup, its pretty normal. Every PCIe card has (own) PERST# pin and obviously you want to control each pin separately via SW. And because PCIe switch is also (upstream) PCIe device it has also PERST# pin.

Based on the discussions we had to add per-port DT PERST# gpios, it sounded to me that this is was not a typical setup ;-)

It seems that the typical setup is to have a single PERST# connected to all devices inside the bus.

Hi Pali,

Em Fri, 22 Oct 2021 17:16:24 +0200
Pali Rohár <[email protected]> escreveu:

Quoted 12 lines · On Tuesday 19 October 2021 07:06:42 Mauro Carvalho Chehab wrote:

Quoted 10 lines

Before code refactor, the PERST# signals were sent at the end of the power_on logic. Then, the PCI core would probe for the buses and add them.

The new logic changed it to send PERST# signals during add_bus operation. That altered the timings.

Also, HiKey 970 require a little more waiting time for the PCI bridge - which is outside the SoC - to finish the PERST# reset, and then initialize the eye diagram.

Quoted 5 lines

Hello! Which PCIe port do you mean by PCI bridge device? Do you mean PCIe Root Port? Or upstream port on some external PCIe switch connected via PCIe bus to the PCIe Root Port? Because all of these (virtual) PCIe devices are presented as PCI bridge devices, so it is not clear to which device it refers.

HiKey 970 uses an external PCI bridge chipset (a Broadcom PEX 8606[1]), with 3 elements connected to the bus: an Ethernet card, a M.2 slot and a mini PCIe slot. It seems HiKey 970 is unique with regards to PERST# signal, as there are 4 independent PERST# signals there:

- one for PEX 8606 (the PCIe root port);
- one for Ethernet;
- one for M.2;
- one for mini-PCIe.

After sending the PCIe PERST# signals, the device has to wait for 21 ms before adjusting the eye diagram.

[1] https://docs.broadcom.com/docs/PEX_8606_AIC_RDK_HRM_v1.3_06Aug10.pdf

Quoted 2 lines

Normally PERST# signal is used to reset endpoint card, other end of PCIe link and so PERST# signal should not affect PCIe Root Port at all.

That's not the case, as PEX 8606 needs to complete its reset sequence for the rest of the devices to be visible. If the wait time is reduced or removed, the devices behind it won't be detected.

Quoted 36 lines

Hi Pali,

Em Fri, 22 Oct 2021 17:16:24 +0200
Pali Rohár <[email protected]> escreveu:

Quoted 12 lines · On Tuesday 19 October 2021 07:06:42 Mauro Carvalho Chehab wrote:

Quoted 10 lines

Before code refactor, the PERST# signals were sent at the end of the power_on logic. Then, the PCI core would probe for the buses and add them.

The new logic changed it to send PERST# signals during add_bus operation. That altered the timings.

Also, HiKey 970 require a little more waiting time for the PCI bridge - which is outside the SoC - to finish the PERST# reset, and then initialize the eye diagram.

Quoted 5 lines

Hello! Which PCIe port do you mean by PCI bridge device? Do you mean PCIe Root Port? Or upstream port on some external PCIe switch connected via PCIe bus to the PCIe Root Port? Because all of these (virtual) PCIe devices are presented as PCI bridge devices, so it is not clear to which device it refers.

HiKey 970 uses an external PCI bridge chipset (a Broadcom PEX 8606[1]), with 3 elements connected to the bus: an Ethernet card, a M.2 slot and a mini PCIe slot. It seems HiKey 970 is unique with regards to PERST# signal, as there are 4 independent PERST# signals there:

- one for PEX 8606 (the PCIe root port);
- one for Ethernet;
- one for M.2;
- one for mini-PCIe.

After sending the PCIe PERST# signals, the device has to wait for 21 ms before adjusting the eye diagram.

Quoted 8 lines

[1] https://docs.broadcom.com/docs/PEX_8606_AIC_RDK_HRM_v1.3_06Aug10.pdf

Quoted 2 lines

Normally PERST# signal is used to reset endpoint card, other end of PCIe link and so PERST# signal should not affect PCIe Root Port at all.

That's not the case, as PEX 8606 needs to complete its reset sequence for the rest of the devices to be visible. If the wait time is reduced or removed, the devices behind it won't be detected.

These pieces of information should go into the commit log (or I can add a Link: tag to this discussion) - it is fundamental to understand these changes.

I believe we can merge this series but we have to document this discussion appropriately.

Em Mon, 25 Oct 2021 11:25:11 +0100
Lorenzo Pieralisi <[email protected]> escreveu:

Quoted 46 lines · Mauro Carvalho Chehab

Quoted 36 lines

Hi Pali,

Em Fri, 22 Oct 2021 17:16:24 +0200
Pali Rohár <[email protected]> escreveu:

Quoted 12 lines · On Tuesday 19 October 2021 07:06:42 Mauro Carvalho Chehab wrote:

Quoted 10 lines

Before code refactor, the PERST# signals were sent at the end of the power_on logic. Then, the PCI core would probe for the buses and add them.

The new logic changed it to send PERST# signals during add_bus operation. That altered the timings.

Also, HiKey 970 require a little more waiting time for the PCI bridge - which is outside the SoC - to finish the PERST# reset, and then initialize the eye diagram.

Quoted 5 lines

Hello! Which PCIe port do you mean by PCI bridge device? Do you mean PCIe Root Port? Or upstream port on some external PCIe switch connected via PCIe bus to the PCIe Root Port? Because all of these (virtual) PCIe devices are presented as PCI bridge devices, so it is not clear to which device it refers.

HiKey 970 uses an external PCI bridge chipset (a Broadcom PEX 8606[1]), with 3 elements connected to the bus: an Ethernet card, a M.2 slot and a mini PCIe slot. It seems HiKey 970 is unique with regards to PERST# signal, as there are 4 independent PERST# signals there:

- one for PEX 8606 (the PCIe root port);
- one for Ethernet;
- one for M.2;
- one for mini-PCIe.

After sending the PCIe PERST# signals, the device has to wait for 21 ms before adjusting the eye diagram.

Quoted 8 lines

[1] https://docs.broadcom.com/docs/PEX_8606_AIC_RDK_HRM_v1.3_06Aug10.pdf

Quoted 2 lines

Normally PERST# signal is used to reset endpoint card, other end of PCIe link and so PERST# signal should not affect PCIe Root Port at all.

That's not the case, as PEX 8606 needs to complete its reset sequence for the rest of the devices to be visible. If the wait time is reduced or removed, the devices behind it won't be detected.

Quoted 6 lines

These pieces of information should go into the commit log (or I can add a Link: tag to this discussion) - it is fundamental to understand these changes.

I believe we can merge this series but we have to document this discussion appropriately.

IMO, the best is to add a Link: to the discussion:

Link https://lore.kernel.org/all/9a365cffe5af9ec5a1f79638968c3a2efa979b65.1634622716.git.mchehab+huawei@kernel.org/ open here

But if you prefer otherwise and want me to re-submit the series, please let me know.

Mauro Carvalho Chehab (11):
PCI: kirin: Reorganize the PHY logic inside the driver
PCI: kirin: Add support for a PHY layer
PCI: kirin: Use regmap for APB registers

PCI: kirin: Add support for bridge slot DT schema
PCI: kirin: give more time for PERST# reset to finish
PCI: kirin: Add Kirin 970 compatible
PCI: kirin: Add MODULE_* macros
PCI: kirin: Allow building it as a module
PCI: kirin: Add power_off support for Kirin 960 PHY
PCI: kirin: fix poweroff sequence
PCI: kirin: Allow removing the driver

PCI: kirin: give more time for PERST# reset to finish
PCI: kirin: fix poweroff sequence

Quoted 13 lines

Mauro Carvalho Chehab (11):
PCI: kirin: Reorganize the PHY logic inside the driver
PCI: kirin: Add support for a PHY layer
PCI: kirin: Use regmap for APB registers
PCI: kirin: Add support for bridge slot DT schema
PCI: kirin: give more time for PERST# reset to finish
PCI: kirin: Add Kirin 970 compatible
PCI: kirin: Add MODULE_* macros
PCI: kirin: Allow building it as a module
PCI: kirin: Add power_off support for Kirin 960 PHY
PCI: kirin: fix poweroff sequence
PCI: kirin: Allow removing the driver

Don't repost for this, but if you have occasion to repost for other reasons, two of these are not capitalized like the others:

Quoted 2 lines

PCI: kirin: give more time for PERST# reset to finish
PCI: kirin: fix poweroff sequence

These are write-once for you, but I'll be reading them many times in the future and they're minor distractions.

Em Tue, 19 Oct 2021 14:27:58 -0500
Bjorn Helgaas <[email protected]> escreveu:

Quoted 21 lines · Mauro Carvalho Chehab

Quoted 13 lines

Mauro Carvalho Chehab (11):
PCI: kirin: Reorganize the PHY logic inside the driver
PCI: kirin: Add support for a PHY layer
PCI: kirin: Use regmap for APB registers
PCI: kirin: Add support for bridge slot DT schema
PCI: kirin: give more time for PERST# reset to finish
PCI: kirin: Add Kirin 970 compatible
PCI: kirin: Add MODULE_* macros
PCI: kirin: Allow building it as a module
PCI: kirin: Add power_off support for Kirin 960 PHY
PCI: kirin: fix poweroff sequence
PCI: kirin: Allow removing the driver

Don't repost for this, but if you have occasion to repost for other reasons, two of these are not capitalized like the others:

Quoted 2 lines

PCI: kirin: give more time for PERST# reset to finish
PCI: kirin: fix poweroff sequence

Quoted 2 lines

These are write-once for you, but I'll be reading them many times in the future and they're minor distractions.

Ok, changed on my working repository. On media and on other subsystems I contribute regularly, the practice is just the opposite: to use lowercase after colons.