From: Ben Peled <[email protected]>
The following patches implement the required procedure to handle and recover from asynchronous PCIE link down events on Armada SoCs.
The procedure is defined as the following:
1) Prevent new access to the PCI-E I/F by disabling the LTSSM
2) Flush all pending transaction/access to the PCI-E I/F
3) HW reset the PCIE end point device (based on board support)
4) Reset the PCIE MAC
5) Reinitialize the PCIE root complex and enable the LTSSM
The execution of this procedure is triggered by the PCIE RST_LINK_DOWN interrupt
v1 --> v2
- Add missing device reset to link-down handle
Ben Peled (5):
PCI: armada8k: Disable LTSSM on link down interrupts
PCI: armada8k: Add link-down handle
dt-bindings: pci: add system controller and MAC reset bit to
Armada 7K/8K controller bindings
arm64: dts: marvell: add pcie mac reset to pcie
PCI: armada8k: add device reset to link-down handle
Documentation/devicetree/bindings/pci/pci-armada8k.txt | 6 +
arch/arm64/boot/dts/marvell/armada-cp11x.dtsi | 7 ++
drivers/pci/controller/dwc/pcie-armada8k.c | 127 ++++++++++++++++++++
3 files changed, 140 insertions(+)
--
2.7.4
From: Ben Peled <[email protected]>
Add system controller and reset bit to each pcie to enable pcie mac reset
Signed-off-by: Ben Peled <[email protected]>
---
arch/arm64/boot/dts/marvell/armada-cp11x.dtsi | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
index 9dcf16b..eb60e73 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
@@ -11,6 +11,7 @@
#include "armada-common.dtsi"
#define CP11X_PCIEx_CONF_BASE(iface) (CP11X_PCIEx_MEM_BASE(iface) + CP11X_PCIEx_MEM_SIZE(iface))
+#define CP11X_PCIEx_MAC_RESET_BIT_MASK(n) (0x1 << 11 + ((n + 2) % 3))
/ {
/*
@@ -513,6 +514,8 @@
num-lanes = <1>;
clock-names = "core", "reg";
clocks = <&CP11X_LABEL(clk) 1 13>, <&CP11X_LABEL(clk) 1 14>;
+ marvell,system-controller = <&CP11X_LABEL(syscon0)>;
+ marvell,mac-reset-bit-mask = <CP11X_PCIEx_MAC_RESET_BIT_MASK(0)>;
status = "disabled";
};
@@ -538,6 +541,8 @@
num-lanes = <1>;
clock-names = "core", "reg";
clocks = <&CP11X_LABEL(clk) 1 11>, <&CP11X_LABEL(clk) 1 14>;
+ marvell,system-controller = <&CP11X_LABEL(syscon0)>;
+ marvell,mac-reset-bit-mask = <CP11X_PCIEx_MAC_RESET_BIT_MASK(1)>;
status = "disabled";
};
@@ -563,6 +568,8 @@
num-lanes = <1>;
clock-names = "core", "reg";
clocks = <&CP11X_LABEL(clk) 1 12>, <&CP11X_LABEL(clk) 1 14>;
+ marvell,system-controller = <&CP11X_LABEL(syscon0)>;
+ marvell,mac-reset-bit-mask = <CP11X_PCIEx_MAC_RESET_BIT_MASK(2)>;
status = "disabled";
};
};
--
2.7.4
From: Ben Peled <[email protected]>
Added pcie reset via gpio support as described in the
designware-pcie.txt DT binding document.
In cases link down cause still exist in device.
The device need to be reset to reestablish the link.
If reset-gpio pin provided in the device tree, then the linkdown
handle resets the device before reestablishing link.
Signed-off-by: Ben Peled <[email protected]>
---
drivers/pci/controller/dwc/pcie-armada8k.c | 24 ++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
index 34b253c..04bba97 100644
--- a/drivers/pci/controller/dwc/pcie-armada8k.c
+++ b/drivers/pci/controller/dwc/pcie-armada8k.c
@@ -24,6 +24,7 @@
#include <linux/of_irq.h>
#include <linux/mfd/syscon.h>
#include <linux/regmap.h>
+#include <linux/of_gpio.h>
#include "pcie-designware.h"
@@ -38,6 +39,8 @@ struct armada8k_pcie {
struct regmap *sysctrl_base;
u32 mac_rest_bitmask;
struct work_struct recover_link_work;
+ enum of_gpio_flags flags;
+ struct gpio_desc *reset_gpio;
};
#define PCIE_VENDOR_REGS_OFFSET 0x8000
@@ -247,9 +250,18 @@ static void armada8k_pcie_recover_link(struct work_struct *ws)
}
pci_lock_rescan_remove();
pci_stop_and_remove_bus_device(root_port);
+ /* Reset device if reset gpio is set */
+ if (pcie->reset_gpio) {
+ /* assert and then deassert the reset signal */
+ gpiod_set_value_cansleep(pcie->reset_gpio, 0);
+ msleep(100);
+ gpiod_set_value_cansleep(pcie->reset_gpio,
+ (pcie->flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1);
+ }
/*
- * Sleep needed to make sure all pcie transactions and access
- * are flushed before resetting the mac
+ * Sleep used for two reasons.
+ * First make sure all pcie transactions and access are flushed before resetting the mac
+ * and second to make sure pci device is ready in case we reset the device
*/
msleep(100);
@@ -369,6 +381,7 @@ static int armada8k_pcie_probe(struct platform_device *pdev)
struct armada8k_pcie *pcie;
struct device *dev = &pdev->dev;
struct resource *base;
+ int reset_gpio;
int ret;
pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
@@ -413,6 +426,13 @@ static int armada8k_pcie_probe(struct platform_device *pdev)
goto fail_clkreg;
}
+ /* Config reset gpio for pcie if the reset connected to gpio */
+ reset_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+ "reset-gpios", 0,
+ &pcie->flags);
+ if (gpio_is_valid(reset_gpio))
+ pcie->reset_gpio = gpio_to_desc(reset_gpio);
+
pcie->sysctrl_base = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
"marvell,system-controller");
if (IS_ERR(pcie->sysctrl_base)) {
--
2.7.4
From: Ben Peled <[email protected]>
When a PCI link down condition is detected, the link training state
machine must be disabled immediately.
Signed-off-by: Marc St-Amand <[email protected]>
Signed-off-by: Konstantin Porotchkin <[email protected]>
Signed-off-by: Ben Peled <[email protected]>
---
drivers/pci/controller/dwc/pcie-armada8k.c | 38 ++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
index 13901f3..b2278b1 100644
--- a/drivers/pci/controller/dwc/pcie-armada8k.c
+++ b/drivers/pci/controller/dwc/pcie-armada8k.c
@@ -54,6 +54,10 @@ struct armada8k_pcie {
#define PCIE_INT_C_ASSERT_MASK BIT(11)
#define PCIE_INT_D_ASSERT_MASK BIT(12)
+#define PCIE_GLOBAL_INT_CAUSE2_REG (PCIE_VENDOR_REGS_OFFSET + 0x24)
+#define PCIE_GLOBAL_INT_MASK2_REG (PCIE_VENDOR_REGS_OFFSET + 0x28)
+#define PCIE_INT2_PHY_RST_LINK_DOWN BIT(1)
+
#define PCIE_ARCACHE_TRC_REG (PCIE_VENDOR_REGS_OFFSET + 0x50)
#define PCIE_AWCACHE_TRC_REG (PCIE_VENDOR_REGS_OFFSET + 0x54)
#define PCIE_ARUSER_REG (PCIE_VENDOR_REGS_OFFSET + 0x5C)
@@ -193,6 +197,11 @@ static void armada8k_pcie_establish_link(struct armada8k_pcie *pcie)
PCIE_INT_C_ASSERT_MASK | PCIE_INT_D_ASSERT_MASK;
dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK1_REG, reg);
+ /* Also enable link down interrupts */
+ reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG);
+ reg |= PCIE_INT2_PHY_RST_LINK_DOWN;
+ dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG, reg);
+
if (!dw_pcie_link_up(pci)) {
/* Configuration done. Start LTSSM */
reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_CONTROL_REG);
@@ -230,6 +239,35 @@ static irqreturn_t armada8k_pcie_irq_handler(int irq, void *arg)
val = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_CAUSE1_REG);
dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_CAUSE1_REG, val);
+ val = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_CAUSE2_REG);
+
+ if (PCIE_INT2_PHY_RST_LINK_DOWN & val) {
+ u32 ctrl_reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_CONTROL_REG);
+ /*
+ * The link went down. Disable LTSSM immediately. This
+ * unlocks the root complex config registers. Downstream
+ * device accesses will return all-Fs
+ */
+ ctrl_reg &= ~(PCIE_APP_LTSSM_EN);
+ dw_pcie_writel_dbi(pci, PCIE_GLOBAL_CONTROL_REG, ctrl_reg);
+ /*
+ * Mask link down interrupts. They can be re-enabled once
+ * the link is retrained.
+ */
+ ctrl_reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG);
+ ctrl_reg &= ~PCIE_INT2_PHY_RST_LINK_DOWN;
+ dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG, ctrl_reg);
+ /*
+ * At this point a worker thread can be triggered to
+ * initiate a link retrain. If link retrains were
+ * possible, that is.
+ */
+ dev_dbg(pci->dev, "%s: link went down\n", __func__);
+ }
+
+ /* Now clear the second interrupt cause. */
+ dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_CAUSE2_REG, val);
+
return IRQ_HANDLED;
}
--
2.7.4
From: Ben Peled <[email protected]>
Adding optional system-controller and mac-reset-bit-mask
needed for linkdown procedure.
Signed-off-by: Ben Peled <[email protected]>
---
Documentation/devicetree/bindings/pci/pci-armada8k.txt | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/pci-armada8k.txt b/Documentation/devicetree/bindings/pci/pci-armada8k.txt
index 7a813d0..2696e79 100644
--- a/Documentation/devicetree/bindings/pci/pci-armada8k.txt
+++ b/Documentation/devicetree/bindings/pci/pci-armada8k.txt
@@ -24,6 +24,10 @@ Optional properties:
- phy-names: names of the PHYs corresponding to the number of lanes.
Must be "cp0-pcie0-x4-lane0-phy", "cp0-pcie0-x4-lane1-phy" for
2 PHYs.
+- marvell,system-controller: address of system controller needed
+ in order to reset MAC used by link-down handle
+- marvell,mac-reset-bit-mask: MAC reset bit of system controller
+ needed in order to reset MAC used by link-down handle
Example:
@@ -45,4 +49,6 @@ Example:
interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
num-lanes = <1>;
clocks = <&cpm_syscon0 1 13>;
+ marvell,system-controller = <&CP11X_LABEL(syscon0)>;
+ marvell,mac-reset-bit-mask = <CP11X_PCIEx_MAC_RESET_BIT_MASK(1)>;
};
--
2.7.4
On Wed, Apr 14, 2021 at 04:20:52PM +0300, [email protected] wrote:
> From: Ben Peled <[email protected]>
>
> Adding optional system-controller and mac-reset-bit-mask
> needed for linkdown procedure.
Same comment as v1.
BTW, it's PATCH not "PATCH". Don't do anything and git will do the right
thing here.
>
> Signed-off-by: Ben Peled <[email protected]>
> ---
> Documentation/devicetree/bindings/pci/pci-armada8k.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/pci-armada8k.txt b/Documentation/devicetree/bindings/pci/pci-armada8k.txt
> index 7a813d0..2696e79 100644
> --- a/Documentation/devicetree/bindings/pci/pci-armada8k.txt
> +++ b/Documentation/devicetree/bindings/pci/pci-armada8k.txt
> @@ -24,6 +24,10 @@ Optional properties:
> - phy-names: names of the PHYs corresponding to the number of lanes.
> Must be "cp0-pcie0-x4-lane0-phy", "cp0-pcie0-x4-lane1-phy" for
> 2 PHYs.
> +- marvell,system-controller: address of system controller needed
> + in order to reset MAC used by link-down handle
> +- marvell,mac-reset-bit-mask: MAC reset bit of system controller
> + needed in order to reset MAC used by link-down handle
>
> Example:
>
> @@ -45,4 +49,6 @@ Example:
> interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>;
> num-lanes = <1>;
> clocks = <&cpm_syscon0 1 13>;
> + marvell,system-controller = <&CP11X_LABEL(syscon0)>;
> + marvell,mac-reset-bit-mask = <CP11X_PCIEx_MAC_RESET_BIT_MASK(1)>;
> };
> --
> 2.7.4
>
On Wednesday 14 April 2021 16:20:50 [email protected] wrote:
> From: Ben Peled <[email protected]>
>
> When a PCI link down condition is detected, the link training state
> machine must be disabled immediately.
>
> Signed-off-by: Marc St-Amand <[email protected]>
> Signed-off-by: Konstantin Porotchkin <[email protected]>
> Signed-off-by: Ben Peled <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-armada8k.c | 38 ++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-armada8k.c b/drivers/pci/controller/dwc/pcie-armada8k.c
> index 13901f3..b2278b1 100644
> --- a/drivers/pci/controller/dwc/pcie-armada8k.c
> +++ b/drivers/pci/controller/dwc/pcie-armada8k.c
> @@ -54,6 +54,10 @@ struct armada8k_pcie {
> #define PCIE_INT_C_ASSERT_MASK BIT(11)
> #define PCIE_INT_D_ASSERT_MASK BIT(12)
>
> +#define PCIE_GLOBAL_INT_CAUSE2_REG (PCIE_VENDOR_REGS_OFFSET + 0x24)
> +#define PCIE_GLOBAL_INT_MASK2_REG (PCIE_VENDOR_REGS_OFFSET + 0x28)
> +#define PCIE_INT2_PHY_RST_LINK_DOWN BIT(1)
> +
> #define PCIE_ARCACHE_TRC_REG (PCIE_VENDOR_REGS_OFFSET + 0x50)
> #define PCIE_AWCACHE_TRC_REG (PCIE_VENDOR_REGS_OFFSET + 0x54)
> #define PCIE_ARUSER_REG (PCIE_VENDOR_REGS_OFFSET + 0x5C)
> @@ -193,6 +197,11 @@ static void armada8k_pcie_establish_link(struct armada8k_pcie *pcie)
> PCIE_INT_C_ASSERT_MASK | PCIE_INT_D_ASSERT_MASK;
> dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK1_REG, reg);
>
> + /* Also enable link down interrupts */
> + reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG);
> + reg |= PCIE_INT2_PHY_RST_LINK_DOWN;
> + dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG, reg);
> +
> if (!dw_pcie_link_up(pci)) {
> /* Configuration done. Start LTSSM */
> reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_CONTROL_REG);
> @@ -230,6 +239,35 @@ static irqreturn_t armada8k_pcie_irq_handler(int irq, void *arg)
> val = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_CAUSE1_REG);
> dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_CAUSE1_REG, val);
>
> + val = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_CAUSE2_REG);
> +
> + if (PCIE_INT2_PHY_RST_LINK_DOWN & val) {
> + u32 ctrl_reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_CONTROL_REG);
> + /*
> + * The link went down. Disable LTSSM immediately. This
> + * unlocks the root complex config registers. Downstream
> + * device accesses will return all-Fs
> + */
Hello! This looks like some issue with PCIe HW. Does it mean that if
LTSSM is not disabled then Root Complex stuck or crash during processing
config requests from CPU? Or what else happens?
Could you provide more details what is the exact issue described in this
comment? And is there any Marvell errata about this particular "disable
LTSSM immediately" issue?
> + ctrl_reg &= ~(PCIE_APP_LTSSM_EN);
> + dw_pcie_writel_dbi(pci, PCIE_GLOBAL_CONTROL_REG, ctrl_reg);
> + /*
> + * Mask link down interrupts. They can be re-enabled once
> + * the link is retrained.
> + */
> + ctrl_reg = dw_pcie_readl_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG);
> + ctrl_reg &= ~PCIE_INT2_PHY_RST_LINK_DOWN;
> + dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_MASK2_REG, ctrl_reg);
> + /*
> + * At this point a worker thread can be triggered to
> + * initiate a link retrain. If link retrains were
> + * possible, that is.
> + */
> + dev_dbg(pci->dev, "%s: link went down\n", __func__);
> + }
> +
> + /* Now clear the second interrupt cause. */
> + dw_pcie_writel_dbi(pci, PCIE_GLOBAL_INT_CAUSE2_REG, val);
> +
> return IRQ_HANDLED;
> }
>
> --
> 2.7.4
>