On Fri, Mar 08, 2024 at 03:16:06PM +0530, Manivannan Sadhasivam wrote:

On Fri, Mar 08, 2024 at 09:56:33AM +0100, Niklas Cassel wrote:

quoted

On Fri, Mar 08, 2024 at 11:11:52AM +0530, Manivannan Sadhasivam wrote:

quoted

On Thu, Mar 07, 2024 at 10:43:19PM +0100, Niklas Cassel wrote:

quoted

On Mon, Mar 04, 2024 at 02:52:20PM +0530, Manivannan Sadhasivam wrote:

quoted

The PCIe link can go to LINK_DOWN state in one of the following scenarios:

1. Fundamental (PERST#)/hot/warm reset
2. Link transition from L2/L3 to L0

In those cases, LINK_DOWN causes some non-sticky DWC registers to loose the
state (like REBAR, PTM_CAP etc...). So the drivers need to reinitialize
them to function properly once the link comes back again.

This is not a problem for drivers supporting PERST# IRQ, since they can
reinitialize the registers in the PERST# IRQ callback. But for the drivers
not supporting PERST#, there is no way they can reinitialize the registers
other than relying on LINK_DOWN IRQ received when the link goes down. So
let's add a DWC generic API dw_pcie_ep_linkdown() that reinitializes the
non-sticky registers and also notifies the EPF drivers about link going
down.

This API can also be used by the drivers supporting PERST# to handle the
scenario (2) mentioned above.

Signed-off-by: Manivannan Sadhasivam <redacted>
---
 drivers/pci/controller/dwc/pcie-designware-ep.c | 111 ++++++++++++++----------
 drivers/pci/controller/dwc/pcie-designware.h    |   5 ++
 2 files changed, 72 insertions(+), 44 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 278bdc9b2269..fed4c2936c78 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c

@@ -14,14 +14,6 @@
 #include <linux/pci-epc.h>
 #include <linux/pci-epf.h>
 
-void dw_pcie_ep_linkup(struct dw_pcie_ep *ep)
-{
-	struct pci_epc *epc = ep->epc;
-
-	pci_epc_linkup(epc);
-}
-EXPORT_SYMBOL_GPL(dw_pcie_ep_linkup);
-
 void dw_pcie_ep_init_notify(struct dw_pcie_ep *ep)
 {
 	struct pci_epc *epc = ep->epc;

@@ -603,19 +595,56 @@ static unsigned int dw_pcie_ep_find_ext_capability(struct dw_pcie *pci, int cap)
 	return 0;
 }
 
+static void dw_pcie_ep_init_non_sticky_registers(struct dw_pcie *pci)
+{
+	unsigned int offset, ptm_cap_base;
+	unsigned int nbars;
+	u32 reg, i;
+
+	offset = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_REBAR);
+	ptm_cap_base = dw_pcie_ep_find_ext_capability(pci, PCI_EXT_CAP_ID_PTM);
+
+	dw_pcie_dbi_ro_wr_en(pci);
+
+	if (offset) {
+		reg = dw_pcie_readl_dbi(pci, offset + PCI_REBAR_CTRL);
+		nbars = (reg & PCI_REBAR_CTRL_NBAR_MASK) >>
+			PCI_REBAR_CTRL_NBAR_SHIFT;
+
+		for (i = 0; i < nbars; i++, offset += PCI_REBAR_CTRL)
+			dw_pcie_writel_dbi(pci, offset + PCI_REBAR_CAP, 0x0);

If you look at PCI_REBAR_CAP, you will see that it is sticky,
but you have to actually read the databook to see that:

"The RESBAR_CTRL_REG_BAR_SIZE field is automatically updated
when you write to RESBAR_CAP_REG_0_REG through the DBI."

So the reason why we need to write this register, even though
it is sticky, is to update the RESBAR_CTRL_REG_BAR_SIZE register,
which is not sticky :)

(Perhaps we should add that as a comment?)

Yeah, makes sense.

Note that I add a (unrelated) comment related to REBAR_CAP in this patch:
https://lore.kernel.org/linux-pci/20240307111520.3303774-1-cassel@kernel.org/T/#u (local)

But once we move/add code to dw_pcie_ep_init_non_sticky_registers(), I think
that it might be a good "rule" to have a small comment for each write in
dw_pcie_ep_init_non_sticky_registers() which explains why the code should be
in dw_pcie_ep_init_non_sticky_registers() instead of dw_pcie_ep_init_registers(),
even if it just a small:

/* Field PCI_XXX_YYY.ZZZ is non-sticky */
writel_dbi(pci, offset + PCI_XXX_YYY, 0);

Why? The function name itself suggests that we are reinitializing non-sticky
registers. So a comment for each write is overkill.

So that you know which field it is in the register that you are writing which
you care about (which field it is that is non-sticky).

But I see your point, perhaps it is overkill.


Kind regards,
Niklas

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