[PATCH v2 0/3] Fix Armada 38x mvneta lockups when switching speeds

[PATCH v2 0/3] Fix Armada 38x mvneta lockups when switching speeds

[Russell King - ARM Linux admin]

Hi,

While testing phylink over the weekend, I found it was possible to
cause the mvneta hardware to lockup in various weird and wonderful
ways by switching the interface speed between 1G and 2.5G repeatedly.
It didn't require a rapid switching, but one switch every few seconds.

Symptoms included one or more of:
- Timeout while trying to stop transmit (seen once)
- 2500BASE-X link negotiation failure (fails to exchange link word.)
- Detects lack of sync, but fails to flag 10ms of sync failure.
- SyncOk bit randomly toggles.

Once the hardware gets into a "bad" state, trying to recover it by
using the mvneta GMAC port reset fails to resolve the issue.
Disabling the port also fails to recover it.  The only way to
recover seemed to be via a reboot.

Many solutions to solve this were tried in various combinations -
while changing the COMPHY configuration:
- putting the GMAC into reset
- disabling the GMAC port
- augmenting the COMPHY configuration to try to "cleanly" disable
  the COMPHY via phy_power_down() and reconfigure it via
  phy_power_up(), including resetting parts of the COMPHY and
  re-running the RX initialisation.

None of that worked.  It was then discovered from the u-boot sources
that there is an undocumented register that has a lane-specific bit
set at the end of COMPHY initialisation, once the loosely documented
COMPHY setup has completed.

Experimentation with that showed that if the lane specific bit is
cleared before changing the COMPHY "GEN" configuration, and set
afterwards, mvneta no longer locks up.

Unfortunately, this undocumented register is not part of the COMPHY
register set that we map - it is located in a region of "System
Registers" which are shared between multiple different devices.

Who should be responsible for mapping this register (mvneta or
COMPHY) was considered; the register is only present on Armada 38x
systems, and seemingly not on Armada 37x or Armada 37xx systems.
It seems that it is a system-level register.  The COMPHYs seem to
be system specific, so let's make it part of the COMPHY.

With no real information on this register, all we can do is guess
about it's function and how to fit it into the system.

 .../bindings/phy/phy-armada38x-comphy.txt          | 10 ++++-
 arch/arm/boot/dts/armada-38x.dtsi                  |  3 +-
 drivers/phy/marvell/phy-armada38x-comphy.c         | 45 ++++++++++++++++++----
 3 files changed, 49 insertions(+), 9 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH v2 0/3] Fix Armada 38x mvneta lockups when switching speeds

[Russell King - ARM Linux admin]

Hi,

While testing phylink over the weekend, I found it was possible to
cause the mvneta hardware to lockup in various weird and wonderful
ways by switching the interface speed between 1G and 2.5G repeatedly.
It didn't require a rapid switching, but one switch every few seconds.

Symptoms included one or more of:
- Timeout while trying to stop transmit (seen once)
- 2500BASE-X link negotiation failure (fails to exchange link word.)
- Detects lack of sync, but fails to flag 10ms of sync failure.
- SyncOk bit randomly toggles.

Once the hardware gets into a "bad" state, trying to recover it by
using the mvneta GMAC port reset fails to resolve the issue.
Disabling the port also fails to recover it.  The only way to
recover seemed to be via a reboot.

Many solutions to solve this were tried in various combinations -
while changing the COMPHY configuration:
- putting the GMAC into reset
- disabling the GMAC port
- augmenting the COMPHY configuration to try to "cleanly" disable
  the COMPHY via phy_power_down() and reconfigure it via
  phy_power_up(), including resetting parts of the COMPHY and
  re-running the RX initialisation.

None of that worked.  It was then discovered from the u-boot sources
that there is an undocumented register that has a lane-specific bit
set at the end of COMPHY initialisation, once the loosely documented
COMPHY setup has completed.

Experimentation with that showed that if the lane specific bit is
cleared before changing the COMPHY "GEN" configuration, and set
afterwards, mvneta no longer locks up.

Unfortunately, this undocumented register is not part of the COMPHY
register set that we map - it is located in a region of "System
Registers" which are shared between multiple different devices.

Who should be responsible for mapping this register (mvneta or
COMPHY) was considered; the register is only present on Armada 38x
systems, and seemingly not on Armada 37x or Armada 37xx systems.
It seems that it is a system-level register.  The COMPHYs seem to
be system specific, so let's make it part of the COMPHY.

With no real information on this register, all we can do is guess
about it's function and how to fit it into the system.

 .../bindings/phy/phy-armada38x-comphy.txt          | 10 ++++-
 arch/arm/boot/dts/armada-38x.dtsi                  |  3 +-
 drivers/phy/marvell/phy-armada38x-comphy.c         | 45 ++++++++++++++++++----
 3 files changed, 49 insertions(+), 9 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

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

[PATCH v2 1/3] dt: update Marvell Armada 38x COMPHY binding

[Russell King]

Update the Marvell Armada 38x COMPHY binding with an additional
optional register pair describing the location of an undocumented
system register controlling something to do with the Gigabit Ethernet
and COMPHY.  There is one bit for each COMPHY lane that may be using
the serdes, but exactly what this register does is completely unknown.

This register only appears to exist on Armada 38x devices, and not
other SoCs using the NETA ethernet block, so it seems logical that it
should be part of the COMPHY.

This is also how u-boot groups this register; it is dealt with as part
of the COMPHY initialisation there.

However, at the end of the day, due to the undocumented nature of this
register, we can only guess.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 .../devicetree/bindings/phy/phy-armada38x-comphy.txt   | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/phy/phy-armada38x-comphy.txt b/Documentation/devicetree/bindings/phy/phy-armada38x-comphy.txt
index ad49e5c01334..8b5a7a28a35b 100644
--- a/Documentation/devicetree/bindings/phy/phy-armada38x-comphy.txt
+++ b/Documentation/devicetree/bindings/phy/phy-armada38x-comphy.txt
@@ -12,6 +12,13 @@ PCIe...).
 - #address-cells: should be 1.
 - #size-cells: should be 0.
 
+Optional properties:
+
+- reg-names: must be "comphy" as the first name, and "conf".
+- reg: must contain the comphy register location and length as the first
+    pair, followed by an optional configuration register address and
+    length pair.
+
 A sub-node is required for each comphy lane provided by the comphy.
 
 Required properties (child nodes):
@@ -24,7 +31,8 @@ A sub-node is required for each comphy lane provided by the comphy.
 
 	comphy: phy@18300 {
 		compatible = "marvell,armada-380-comphy";
-		reg = <0x18300 0x100>;
+		reg-names = "comphy", "conf";
+		reg = <0x18300 0x100>, <0x18460 4>;
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-- 
2.20.1

[PATCH v2 1/3] dt: update Marvell Armada 38x COMPHY binding

[Russell King]

Update the Marvell Armada 38x COMPHY binding with an additional
optional register pair describing the location of an undocumented
system register controlling something to do with the Gigabit Ethernet
and COMPHY.  There is one bit for each COMPHY lane that may be using
the serdes, but exactly what this register does is completely unknown.

This register only appears to exist on Armada 38x devices, and not
other SoCs using the NETA ethernet block, so it seems logical that it
should be part of the COMPHY.

This is also how u-boot groups this register; it is dealt with as part
of the COMPHY initialisation there.

However, at the end of the day, due to the undocumented nature of this
register, we can only guess.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 .../devicetree/bindings/phy/phy-armada38x-comphy.txt   | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/phy/phy-armada38x-comphy.txt b/Documentation/devicetree/bindings/phy/phy-armada38x-comphy.txt
index ad49e5c01334..8b5a7a28a35b 100644
--- a/Documentation/devicetree/bindings/phy/phy-armada38x-comphy.txt
+++ b/Documentation/devicetree/bindings/phy/phy-armada38x-comphy.txt
@@ -12,6 +12,13 @@ PCIe...).
 - #address-cells: should be 1.
 - #size-cells: should be 0.
 
+Optional properties:
+
+- reg-names: must be "comphy" as the first name, and "conf".
+- reg: must contain the comphy register location and length as the first
+    pair, followed by an optional configuration register address and
+    length pair.
+
 A sub-node is required for each comphy lane provided by the comphy.
 
 Required properties (child nodes):
@@ -24,7 +31,8 @@ A sub-node is required for each comphy lane provided by the comphy.
 
 	comphy: phy@18300 {
 		compatible = "marvell,armada-380-comphy";
-		reg = <0x18300 0x100>;
+		reg-names = "comphy", "conf";
+		reg = <0x18300 0x100>, <0x18460 4>;
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-- 
2.20.1


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

[PATCH v2 2/3] ARM: dts: armada-38x: fix NETA lockup when repeatedly switching speeds

[Russell King]

To support the change in "phy: armada-38x: fix NETA lockup when
repeatedly switching speeds" we need to update the DT with the
additional register.

Fixes: 14dc100b4411 ("phy: armada38x: add common phy support")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/boot/dts/armada-38x.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/armada-38x.dtsi b/arch/arm/boot/dts/armada-38x.dtsi
index e038abc0c6b4..420ae26e846b 100644
--- a/arch/arm/boot/dts/armada-38x.dtsi
+++ b/arch/arm/boot/dts/armada-38x.dtsi
@@ -344,7 +344,8 @@
 
 			comphy: phy@18300 {
 				compatible = "marvell,armada-380-comphy";
-				reg = <0x18300 0x100>;
+				reg-names = "comphy", "conf";
+				reg = <0x18300 0x100>, <0x18460 4>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 
-- 
2.20.1

[PATCH v2 2/3] ARM: dts: armada-38x: fix NETA lockup when repeatedly switching speeds

[Russell King]

To support the change in "phy: armada-38x: fix NETA lockup when
repeatedly switching speeds" we need to update the DT with the
additional register.

Fixes: 14dc100b4411 ("phy: armada38x: add common phy support")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/boot/dts/armada-38x.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/armada-38x.dtsi b/arch/arm/boot/dts/armada-38x.dtsi
index e038abc0c6b4..420ae26e846b 100644
--- a/arch/arm/boot/dts/armada-38x.dtsi
+++ b/arch/arm/boot/dts/armada-38x.dtsi
@@ -344,7 +344,8 @@
 
 			comphy: phy@18300 {
 				compatible = "marvell,armada-380-comphy";
-				reg = <0x18300 0x100>;
+				reg-names = "comphy", "conf";
+				reg = <0x18300 0x100>, <0x18460 4>;
 				#address-cells = <1>;
 				#size-cells = <0>;
 
-- 
2.20.1


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

[PATCH v2 3/3] phy: armada-38x: fix NETA lockup when repeatedly switching speeds

[Russell King]

The mvneta hardware appears to lock up in various random ways when
repeatedly switching speeds between 1G and 2.5G, which involves
reprogramming the COMPHY.  It is not entirely clear why this happens,
but best guess is that reprogramming the COMPHY glitches mvneta clocks
causing the hardware to fail.  It seems that rebooting resolves the
failure, but not down/up cycling the interface alone.

Various other approaches have been tried, such as trying to cleanly
power down the COMPHY and then take it back through the power up
initialisation, but this does not seem to help.

It was finally noticed that u-boot's last step when configuring a
COMPHY for "SGMII" mode was to poke at a register described as
"GBE_CONFIGURATION_REG", which is undocumented in any external
documentation.  All that we have is the fact that u-boot sets a bit
corresponding to the "SGMII" lane at the end of COMPHY initialisation.

Experimentation shows that if we clear this bit prior to changing the
speed, and then set it afterwards, mvneta does not suffer this problem
on the SolidRun Clearfog when switching speeds between 1G and 2.5G.

This problem was found while script-testing phylink.

This fix also requires the corresponding change to DT to be effective.
See "ARM: dts: armada-38x: fix NETA lockup when repeatedly switching
speeds".

Fixes: 14dc100b4411 ("phy: armada38x: add common phy support")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/phy/marvell/phy-armada38x-comphy.c | 45 ++++++++++++++++++----
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/phy/marvell/phy-armada38x-comphy.c b/drivers/phy/marvell/phy-armada38x-comphy.c
index 6960dfd8ad8c..0fe408964334 100644
--- a/drivers/phy/marvell/phy-armada38x-comphy.c
+++ b/drivers/phy/marvell/phy-armada38x-comphy.c
@@ -41,6 +41,7 @@ struct a38x_comphy_lane {
 
 struct a38x_comphy {
 	void __iomem *base;
+	void __iomem *conf;
 	struct device *dev;
 	struct a38x_comphy_lane lane[MAX_A38X_COMPHY];
 };
@@ -54,6 +55,21 @@ static const u8 gbe_mux[MAX_A38X_COMPHY][MAX_A38X_PORTS] = {
 	{ 0, 0, 3 },
 };
 
+static void a38x_set_conf(struct a38x_comphy_lane *lane, bool enable)
+{
+	struct a38x_comphy *priv = lane->priv;
+	u32 conf;
+
+	if (priv->conf) {
+		conf = readl_relaxed(priv->conf);
+		if (enable)
+			conf |= BIT(lane->port);
+		else
+			conf &= ~BIT(lane->port);
+		writel(conf, priv->conf);
+	}
+}
+
 static void a38x_comphy_set_reg(struct a38x_comphy_lane *lane,
 				unsigned int offset, u32 mask, u32 value)
 {
@@ -97,6 +113,7 @@ static int a38x_comphy_set_mode(struct phy *phy, enum phy_mode mode, int sub)
 {
 	struct a38x_comphy_lane *lane = phy_get_drvdata(phy);
 	unsigned int gen;
+	int ret;
 
 	if (mode != PHY_MODE_ETHERNET)
 		return -EINVAL;
@@ -115,13 +132,20 @@ static int a38x_comphy_set_mode(struct phy *phy, enum phy_mode mode, int sub)
 		return -EINVAL;
 	}
 
+	a38x_set_conf(lane, false);
+
 	a38x_comphy_set_speed(lane, gen, gen);
 
-	return a38x_comphy_poll(lane, COMPHY_STAT1,
-				COMPHY_STAT1_PLL_RDY_TX |
-				COMPHY_STAT1_PLL_RDY_RX,
-				COMPHY_STAT1_PLL_RDY_TX |
-				COMPHY_STAT1_PLL_RDY_RX);
+	ret = a38x_comphy_poll(lane, COMPHY_STAT1,
+			       COMPHY_STAT1_PLL_RDY_TX |
+			       COMPHY_STAT1_PLL_RDY_RX,
+			       COMPHY_STAT1_PLL_RDY_TX |
+			       COMPHY_STAT1_PLL_RDY_RX);
+
+	if (ret == 0)
+		a38x_set_conf(lane, true);
+
+	return ret;
 }
 
 static const struct phy_ops a38x_comphy_ops = {
@@ -174,14 +198,21 @@ static int a38x_comphy_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	base = devm_ioremap_resource(&pdev->dev, res);
+	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
 	priv->dev = &pdev->dev;
 	priv->base = base;
 
+	/* Optional */
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "conf");
+	if (res) {
+		priv->conf = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(priv->conf))
+			return PTR_ERR(priv->conf);
+	}
+
 	for_each_available_child_of_node(pdev->dev.of_node, child) {
 		struct phy *phy;
 		int ret;
-- 
2.20.1

[PATCH v2 3/3] phy: armada-38x: fix NETA lockup when repeatedly switching speeds

[Russell King]

The mvneta hardware appears to lock up in various random ways when
repeatedly switching speeds between 1G and 2.5G, which involves
reprogramming the COMPHY.  It is not entirely clear why this happens,
but best guess is that reprogramming the COMPHY glitches mvneta clocks
causing the hardware to fail.  It seems that rebooting resolves the
failure, but not down/up cycling the interface alone.

Various other approaches have been tried, such as trying to cleanly
power down the COMPHY and then take it back through the power up
initialisation, but this does not seem to help.

It was finally noticed that u-boot's last step when configuring a
COMPHY for "SGMII" mode was to poke at a register described as
"GBE_CONFIGURATION_REG", which is undocumented in any external
documentation.  All that we have is the fact that u-boot sets a bit
corresponding to the "SGMII" lane at the end of COMPHY initialisation.

Experimentation shows that if we clear this bit prior to changing the
speed, and then set it afterwards, mvneta does not suffer this problem
on the SolidRun Clearfog when switching speeds between 1G and 2.5G.

This problem was found while script-testing phylink.

This fix also requires the corresponding change to DT to be effective.
See "ARM: dts: armada-38x: fix NETA lockup when repeatedly switching
speeds".

Fixes: 14dc100b4411 ("phy: armada38x: add common phy support")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/phy/marvell/phy-armada38x-comphy.c | 45 ++++++++++++++++++----
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/phy/marvell/phy-armada38x-comphy.c b/drivers/phy/marvell/phy-armada38x-comphy.c
index 6960dfd8ad8c..0fe408964334 100644
--- a/drivers/phy/marvell/phy-armada38x-comphy.c
+++ b/drivers/phy/marvell/phy-armada38x-comphy.c
@@ -41,6 +41,7 @@ struct a38x_comphy_lane {
 
 struct a38x_comphy {
 	void __iomem *base;
+	void __iomem *conf;
 	struct device *dev;
 	struct a38x_comphy_lane lane[MAX_A38X_COMPHY];
 };
@@ -54,6 +55,21 @@ static const u8 gbe_mux[MAX_A38X_COMPHY][MAX_A38X_PORTS] = {
 	{ 0, 0, 3 },
 };
 
+static void a38x_set_conf(struct a38x_comphy_lane *lane, bool enable)
+{
+	struct a38x_comphy *priv = lane->priv;
+	u32 conf;
+
+	if (priv->conf) {
+		conf = readl_relaxed(priv->conf);
+		if (enable)
+			conf |= BIT(lane->port);
+		else
+			conf &= ~BIT(lane->port);
+		writel(conf, priv->conf);
+	}
+}
+
 static void a38x_comphy_set_reg(struct a38x_comphy_lane *lane,
 				unsigned int offset, u32 mask, u32 value)
 {
@@ -97,6 +113,7 @@ static int a38x_comphy_set_mode(struct phy *phy, enum phy_mode mode, int sub)
 {
 	struct a38x_comphy_lane *lane = phy_get_drvdata(phy);
 	unsigned int gen;
+	int ret;
 
 	if (mode != PHY_MODE_ETHERNET)
 		return -EINVAL;
@@ -115,13 +132,20 @@ static int a38x_comphy_set_mode(struct phy *phy, enum phy_mode mode, int sub)
 		return -EINVAL;
 	}
 
+	a38x_set_conf(lane, false);
+
 	a38x_comphy_set_speed(lane, gen, gen);
 
-	return a38x_comphy_poll(lane, COMPHY_STAT1,
-				COMPHY_STAT1_PLL_RDY_TX |
-				COMPHY_STAT1_PLL_RDY_RX,
-				COMPHY_STAT1_PLL_RDY_TX |
-				COMPHY_STAT1_PLL_RDY_RX);
+	ret = a38x_comphy_poll(lane, COMPHY_STAT1,
+			       COMPHY_STAT1_PLL_RDY_TX |
+			       COMPHY_STAT1_PLL_RDY_RX,
+			       COMPHY_STAT1_PLL_RDY_TX |
+			       COMPHY_STAT1_PLL_RDY_RX);
+
+	if (ret == 0)
+		a38x_set_conf(lane, true);
+
+	return ret;
 }
 
 static const struct phy_ops a38x_comphy_ops = {
@@ -174,14 +198,21 @@ static int a38x_comphy_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	base = devm_ioremap_resource(&pdev->dev, res);
+	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
 	priv->dev = &pdev->dev;
 	priv->base = base;
 
+	/* Optional */
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "conf");
+	if (res) {
+		priv->conf = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(priv->conf))
+			return PTR_ERR(priv->conf);
+	}
+
 	for_each_available_child_of_node(pdev->dev.of_node, child) {
 		struct phy *phy;
 		int ret;
-- 
2.20.1


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

Re: [PATCH v2 2/3] ARM: dts: armada-38x: fix NETA lockup when repeatedly switching speeds

[Andrew Lunn]

On Tue, Jul 21, 2020 at 03:40:38PM +0100, Russell King wrote:
> To support the change in "phy: armada-38x: fix NETA lockup when
> repeatedly switching speeds" we need to update the DT with the
> additional register.
> 
> Fixes: 14dc100b4411 ("phy: armada38x: add common phy support")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 2/3] ARM: dts: armada-38x: fix NETA lockup when repeatedly switching speeds

[Andrew Lunn]

On Tue, Jul 21, 2020 at 03:40:38PM +0100, Russell King wrote:
> To support the change in "phy: armada-38x: fix NETA lockup when
> repeatedly switching speeds" we need to update the DT with the
> additional register.
> 
> Fixes: 14dc100b4411 ("phy: armada38x: add common phy support")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

Re: [PATCH v2 3/3] phy: armada-38x: fix NETA lockup when repeatedly switching speeds

[Andrew Lunn]

On Tue, Jul 21, 2020 at 03:40:43PM +0100, Russell King wrote:
> The mvneta hardware appears to lock up in various random ways when
> repeatedly switching speeds between 1G and 2.5G, which involves
> reprogramming the COMPHY.  It is not entirely clear why this happens,
> but best guess is that reprogramming the COMPHY glitches mvneta clocks
> causing the hardware to fail.  It seems that rebooting resolves the
> failure, but not down/up cycling the interface alone.
> 
> Various other approaches have been tried, such as trying to cleanly
> power down the COMPHY and then take it back through the power up
> initialisation, but this does not seem to help.
> 
> It was finally noticed that u-boot's last step when configuring a
> COMPHY for "SGMII" mode was to poke at a register described as
> "GBE_CONFIGURATION_REG", which is undocumented in any external
> documentation.  All that we have is the fact that u-boot sets a bit
> corresponding to the "SGMII" lane at the end of COMPHY initialisation.
> 
> Experimentation shows that if we clear this bit prior to changing the
> speed, and then set it afterwards, mvneta does not suffer this problem
> on the SolidRun Clearfog when switching speeds between 1G and 2.5G.
> 
> This problem was found while script-testing phylink.
> 
> This fix also requires the corresponding change to DT to be effective.
> See "ARM: dts: armada-38x: fix NETA lockup when repeatedly switching
> speeds".
> 
> Fixes: 14dc100b4411 ("phy: armada38x: add common phy support")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 3/3] phy: armada-38x: fix NETA lockup when repeatedly switching speeds

[Andrew Lunn]

On Tue, Jul 21, 2020 at 03:40:43PM +0100, Russell King wrote:
> The mvneta hardware appears to lock up in various random ways when
> repeatedly switching speeds between 1G and 2.5G, which involves
> reprogramming the COMPHY.  It is not entirely clear why this happens,
> but best guess is that reprogramming the COMPHY glitches mvneta clocks
> causing the hardware to fail.  It seems that rebooting resolves the
> failure, but not down/up cycling the interface alone.
> 
> Various other approaches have been tried, such as trying to cleanly
> power down the COMPHY and then take it back through the power up
> initialisation, but this does not seem to help.
> 
> It was finally noticed that u-boot's last step when configuring a
> COMPHY for "SGMII" mode was to poke at a register described as
> "GBE_CONFIGURATION_REG", which is undocumented in any external
> documentation.  All that we have is the fact that u-boot sets a bit
> corresponding to the "SGMII" lane at the end of COMPHY initialisation.
> 
> Experimentation shows that if we clear this bit prior to changing the
> speed, and then set it afterwards, mvneta does not suffer this problem
> on the SolidRun Clearfog when switching speeds between 1G and 2.5G.
> 
> This problem was found while script-testing phylink.
> 
> This fix also requires the corresponding change to DT to be effective.
> See "ARM: dts: armada-38x: fix NETA lockup when repeatedly switching
> speeds".
> 
> Fixes: 14dc100b4411 ("phy: armada38x: add common phy support")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

Re: [PATCH v2 0/3] Fix Armada 38x mvneta lockups when switching speeds

[Vinod Koul]

On 21-07-20, 15:37, Russell King - ARM Linux admin wrote:
> Hi,
> 
> While testing phylink over the weekend, I found it was possible to
> cause the mvneta hardware to lockup in various weird and wonderful
> ways by switching the interface speed between 1G and 2.5G repeatedly.
> It didn't require a rapid switching, but one switch every few seconds.
> 
> Symptoms included one or more of:
> - Timeout while trying to stop transmit (seen once)
> - 2500BASE-X link negotiation failure (fails to exchange link word.)
> - Detects lack of sync, but fails to flag 10ms of sync failure.
> - SyncOk bit randomly toggles.
> 
> Once the hardware gets into a "bad" state, trying to recover it by
> using the mvneta GMAC port reset fails to resolve the issue.
> Disabling the port also fails to recover it.  The only way to
> recover seemed to be via a reboot.
> 
> Many solutions to solve this were tried in various combinations -
> while changing the COMPHY configuration:
> - putting the GMAC into reset
> - disabling the GMAC port
> - augmenting the COMPHY configuration to try to "cleanly" disable
>   the COMPHY via phy_power_down() and reconfigure it via
>   phy_power_up(), including resetting parts of the COMPHY and
>   re-running the RX initialisation.
> 
> None of that worked.  It was then discovered from the u-boot sources
> that there is an undocumented register that has a lane-specific bit
> set at the end of COMPHY initialisation, once the loosely documented
> COMPHY setup has completed.
> 
> Experimentation with that showed that if the lane specific bit is
> cleared before changing the COMPHY "GEN" configuration, and set
> afterwards, mvneta no longer locks up.
> 
> Unfortunately, this undocumented register is not part of the COMPHY
> register set that we map - it is located in a region of "System
> Registers" which are shared between multiple different devices.
> 
> Who should be responsible for mapping this register (mvneta or
> COMPHY) was considered; the register is only present on Armada 38x
> systems, and seemingly not on Armada 37x or Armada 37xx systems.
> It seems that it is a system-level register.  The COMPHYs seem to
> be system specific, so let's make it part of the COMPHY.
> 
> With no real information on this register, all we can do is guess
> about it's function and how to fit it into the system.

Applied 1 & 3, thanks
-- 
~Vinod

Re: [PATCH v2 0/3] Fix Armada 38x mvneta lockups when switching speeds

[Vinod Koul]

On 21-07-20, 15:37, Russell King - ARM Linux admin wrote:
> Hi,
> 
> While testing phylink over the weekend, I found it was possible to
> cause the mvneta hardware to lockup in various weird and wonderful
> ways by switching the interface speed between 1G and 2.5G repeatedly.
> It didn't require a rapid switching, but one switch every few seconds.
> 
> Symptoms included one or more of:
> - Timeout while trying to stop transmit (seen once)
> - 2500BASE-X link negotiation failure (fails to exchange link word.)
> - Detects lack of sync, but fails to flag 10ms of sync failure.
> - SyncOk bit randomly toggles.
> 
> Once the hardware gets into a "bad" state, trying to recover it by
> using the mvneta GMAC port reset fails to resolve the issue.
> Disabling the port also fails to recover it.  The only way to
> recover seemed to be via a reboot.
> 
> Many solutions to solve this were tried in various combinations -
> while changing the COMPHY configuration:
> - putting the GMAC into reset
> - disabling the GMAC port
> - augmenting the COMPHY configuration to try to "cleanly" disable
>   the COMPHY via phy_power_down() and reconfigure it via
>   phy_power_up(), including resetting parts of the COMPHY and
>   re-running the RX initialisation.
> 
> None of that worked.  It was then discovered from the u-boot sources
> that there is an undocumented register that has a lane-specific bit
> set at the end of COMPHY initialisation, once the loosely documented
> COMPHY setup has completed.
> 
> Experimentation with that showed that if the lane specific bit is
> cleared before changing the COMPHY "GEN" configuration, and set
> afterwards, mvneta no longer locks up.
> 
> Unfortunately, this undocumented register is not part of the COMPHY
> register set that we map - it is located in a region of "System
> Registers" which are shared between multiple different devices.
> 
> Who should be responsible for mapping this register (mvneta or
> COMPHY) was considered; the register is only present on Armada 38x
> systems, and seemingly not on Armada 37x or Armada 37xx systems.
> It seems that it is a system-level register.  The COMPHYs seem to
> be system specific, so let's make it part of the COMPHY.
> 
> With no real information on this register, all we can do is guess
> about it's function and how to fit it into the system.

Applied 1 & 3, thanks
-- 
~Vinod

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

Re: [PATCH v2 2/3] ARM: dts: armada-38x: fix NETA lockup when repeatedly switching speeds

[Gregory CLEMENT]

Russell King <rmk+kernel@armlinux.org.uk> writes:

> To support the change in "phy: armada-38x: fix NETA lockup when
> repeatedly switching speeds" we need to update the DT with the
> additional register.
>
> Fixes: 14dc100b4411 ("phy: armada38x: add common phy support")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Applied on mvebu/fixes

Thanks,

Gregory
> ---
>  arch/arm/boot/dts/armada-38x.dtsi | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/armada-38x.dtsi b/arch/arm/boot/dts/armada-38x.dtsi
> index e038abc0c6b4..420ae26e846b 100644
> --- a/arch/arm/boot/dts/armada-38x.dtsi
> +++ b/arch/arm/boot/dts/armada-38x.dtsi
> @@ -344,7 +344,8 @@
>  
>  			comphy: phy@18300 {
>  				compatible = "marvell,armada-380-comphy";
> -				reg = <0x18300 0x100>;
> +				reg-names = "comphy", "conf";
> +				reg = <0x18300 0x100>, <0x18460 4>;
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  
> -- 
> 2.20.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v2 2/3] ARM: dts: armada-38x: fix NETA lockup when repeatedly switching speeds

[Gregory CLEMENT]

Russell King <rmk+kernel@armlinux.org.uk> writes:

> To support the change in "phy: armada-38x: fix NETA lockup when
> repeatedly switching speeds" we need to update the DT with the
> additional register.
>
> Fixes: 14dc100b4411 ("phy: armada38x: add common phy support")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Applied on mvebu/fixes

Thanks,

Gregory
> ---
>  arch/arm/boot/dts/armada-38x.dtsi | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/armada-38x.dtsi b/arch/arm/boot/dts/armada-38x.dtsi
> index e038abc0c6b4..420ae26e846b 100644
> --- a/arch/arm/boot/dts/armada-38x.dtsi
> +++ b/arch/arm/boot/dts/armada-38x.dtsi
> @@ -344,7 +344,8 @@
>  
>  			comphy: phy@18300 {
>  				compatible = "marvell,armada-380-comphy";
> -				reg = <0x18300 0x100>;
> +				reg-names = "comphy", "conf";
> +				reg = <0x18300 0x100>, <0x18460 4>;
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  
> -- 
> 2.20.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

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