Hi,

On Mon, 2021-02-01 at 13:32 +0200, Mika Westerberg wrote:

Hi,

On Fri, Jan 29, 2021 at 03:11:37PM +0800, mingchuang.qiao@mediatek.com wrote:

quoted

From: Mingchuang Qiao <redacted>

In bus scan flow, the "LTR Mechanism Enable" bit of DEVCTL2 register is
configured in pci_configure_ltr(). If device and bridge both support LTR
mechanism, the "LTR Mechanism Enable" bit of device and bridge will be
enabled in DEVCTL2 register. And pci_dev->ltr_path will be set as 1.

If PCIe link goes down when device resets, the "LTR Mechanism Enable" bit
of bridge will change to 0 according to PCIe r5.0, sec 7.5.3.16. However,
the pci_dev->ltr_path value of bridge is still 1.

For following conditions, check and re-configure "LTR Mechanism Enable" bit
of bridge to make "LTR Mechanism Enable" bit mtach ltr_path value.

Typo mtach -> match.

quoted

   -before configuring device's LTR for hot-remove/hot-add
   -before restoring device's DEVCTL2 register when restore device state

Signed-off-by: Mingchuang Qiao <redacted>
---
changes of v2
 -modify patch description
 -reconfigure bridge's LTR before restoring device DEVCTL2 register
changes of v3
 -call pci_reconfigure_bridge_ltr() in probe.c

Hmm, which part of this patch takes care of the reset path? It is not
entirely clear to me at least.

When device resets and link goes down, there seems to have two methods
to recover for software. 
   -One is that trigger device removal and rescan.
   -The other is that restore device with pci_restore_state() after link
comes back up.
For above both scenarios, we need check and reconfigure "LTR Mechanism
Enable" bit of bridge. It's also this patch intends to do.
   -For the rescan scenario, it's done in pci_configure_ltr().
   -For the restore scenario, it's done in pci_restore_pcie_state().

quoted

---
 drivers/pci/pci.c   | 25 +++++++++++++++++++++++++
 drivers/pci/pci.h   |  1 +
 drivers/pci/probe.c | 13 ++++++++++---
 3 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b9fecc25d213..12b557c8f062 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c

@@ -1437,6 +1437,24 @@ static int pci_save_pcie_state(struct pci_dev *dev)
 	return 0;
 }
 
+void pci_reconfigure_bridge_ltr(struct pci_dev *dev)
+{
+#ifdef CONFIG_PCIEASPM
+	struct pci_dev *bridge;
+	u32 ctl;
+
+	bridge = pci_upstream_bridge(dev);
+	if (bridge && bridge->ltr_path) {
+		pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2, &ctl);
+		if (!(ctl & PCI_EXP_DEVCTL2_LTR_EN)) {
+			pci_dbg(bridge, "re-enabling LTR\n");
+			pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
+						 PCI_EXP_DEVCTL2_LTR_EN);
+		}
+	}
+#endif
+}
+
 static void pci_restore_pcie_state(struct pci_dev *dev)
 {
 	int i = 0;

@@ -1447,6 +1465,13 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
 	if (!save_state)
 		return;
 
+	/*
+	 * Downstream ports reset the LTR enable bit when link goes down.
+	 * Check and re-configure the bit here before restoring device.
+	 * PCIe r5.0, sec 7.5.3.16.
+	 */
+	pci_reconfigure_bridge_ltr(dev);
+
 	cap = (u16 *)&save_state->cap.data[0];
 	pcie_capability_write_word(dev, PCI_EXP_DEVCTL, cap[i++]);
 	pcie_capability_write_word(dev, PCI_EXP_LNKCTL, cap[i++]);

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 5c59365092fa..a660a01358c5 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h

@@ -111,6 +111,7 @@ void pci_free_cap_save_buffers(struct pci_dev *dev);
 bool pci_bridge_d3_possible(struct pci_dev *dev);
 void pci_bridge_d3_update(struct pci_dev *dev);
 void pci_bridge_wait_for_secondary_bus(struct pci_dev *dev);
+void pci_reconfigure_bridge_ltr(struct pci_dev *dev);

Nit: calling it pci_bridge_reconfigure_ltr() would match better with the
other function names.

Thanks for the suggestion. I will update the name in next patch :)

quoted

 
 static inline void pci_wakeup_event(struct pci_dev *dev)
 {

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 953f15abc850..fa6075093f3b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c

@@ -2132,9 +2132,16 @@ static void pci_configure_ltr(struct pci_dev *dev)
 	 * Complex and all intermediate Switches indicate support for LTR.
 	 * PCIe r4.0, sec 6.18.
 	 */
-	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
-	    ((bridge = pci_upstream_bridge(dev)) &&
-	      bridge->ltr_path)) {
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
+		pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
+					 PCI_EXP_DEVCTL2_LTR_EN);
+		dev->ltr_path = 1;
+		return;
+	}
+
+	bridge = pci_upstream_bridge(dev);
+	if (bridge && bridge->ltr_path) {
+		pci_reconfigure_bridge_ltr(dev);
 		pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
 					 PCI_EXP_DEVCTL2_LTR_EN);
 		dev->ltr_path = 1;

-- 
2.18.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel