Fix Armada 38x mvneta lockups when switching speeds
Subsystems (2)
cover Fix Armada 38x mvneta lockups when switching speeds
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.
I've mentioned this to Thomas Petazzoni on #mvlinux, but that has not yet lead anywhere.
.../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(-)
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
PATCH 1/2 dt: update Marvell Armada 38x COMPHY binding
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.
--- .../devicetree/bindings/phy/phy-armada38x-comphy.txt | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/phy/phy-armada38x-comphy.txt +9 -1
@@ -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>;
Signature · 1 line
2.20.1
PATCH 2/2 phy: armada-38x: fix NETA lockup when repeatedly switching speeds
In reply to Russell King - ARM Linux adminThe 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.
--- arch/arm/boot/dts/armada-38x.dtsi | 3 +- drivers/phy/marvell/phy-armada38x-comphy.c | 45 ++++++++++++++++++---- 2 files changed, 40 insertions(+), 8 deletions(-)
arch/arm/boot/dts/armada-38x.dtsi +2 -1
@@ -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>;
drivers/phy/marvell/phy-armada38x-comphy.c +38 -7
83 lines not on this page · raw message
Signature · 1 line
2.20.1
Re: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds
Quoted 17 lines · 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.
Quoted 7 lines
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.
Signed-off-by Russell King <[email protected]>
I forgot...
Quoted 115 lines
--- arch/arm/boot/dts/armada-38x.dtsi | 3 +- drivers/phy/marvell/phy-armada38x-comphy.c | 45 ++++++++++++++++++---- 2 files changed, 40 insertions(+), 8 deletions(-)arch/arm/boot/dts/armada-38x.dtsi +2
-1@@ -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>;drivers/phy/marvell/phy-armada38x-comphy.c +38
-7@@ -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;Signature · 1 line
2.20.1
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Vinod Koul
maintainerRe: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds
In reply to Russell KingQuoted 21 lines · 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.
Quoted 5 lines
This problem was found while script-testing phylink.
Signed-off-by Russell King <[email protected]>--- arch/arm/boot/dts/armada-38x.dtsi | 3 +-
lgtm, i need ack for dts parts before I can apply this
Quoted 111 lines
drivers/phy/marvell/phy-armada38x-comphy.c | 45 ++++++++++++++++++---- 2 files changed, 40 insertions(+), 8 deletions(-)arch/arm/boot/dts/armada-38x.dtsi +2
-1@@ -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>;drivers/phy/marvell/phy-armada38x-comphy.c +38
-7@@ -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;Signature · 1 line
2.20.1
Signature · 1 line
~Vinod
Re: [PATCH 1/2] dt: update Marvell Armada 38x COMPHY binding
In reply to Russell KingRob?
Quoted 53 lines · 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.
Signed-off-by Russell King <[email protected]>--- .../devicetree/bindings/phy/phy-armada38x-comphy.txt | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)Documentation/devicetree/bindings/phy/phy-armada38x-comphy.txt +9
-1@@ -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>;Signature · 1 line
2.20.1
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds
In reply to Vinod KoulQuoted 22 lines · Vinod Koul
Quoted 21 lines · 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.
Quoted 7 lines
Quoted 5 lines
This problem was found while script-testing phylink.
Signed-off-by Russell King <[email protected]>--- arch/arm/boot/dts/armada-38x.dtsi | 3 +-lgtm, i need ack for dts parts before I can apply this
I'm not sure what the situation is for Bootlin, but they don't seem to be very responsive right now (covid related?)
What I know from what I've been party to on netdev is that Bootlin sent a patch for the MVPP2 driver, and the very next day someone reported that the patch caused a bug. Unfortunately, the patch got picked up anyway, but there was no response from Bootlin. After a month or so, -final was released containing this patch, so now it had become a regression - and still no response from Bootlin.
Eventually the bug got fixed - not because Bootlin fixed it, but because I ended up spending the time researching how that part of the network driver worked, diagnosing what was going on, and eventually fixing it in the most obvious way - but it's not clear that the fix was the right approach. Bootlin never commented. See 3138a07ce219 ("net: mvpp2: fix RX hashing for non-10G ports").
So, I think we have to assume that Bootlin are struggling right now, and as it's been over a week, it's unlikely that they are going to respond soon. What do you think we should do?
I also note that Rob has not responded to the DT binding change either, despite me gently prodding, and Rob processing a whole raft of DT binding stuff yesterday.
I can split the DTS change from the rest of the patch, but I don't think that really helps without at least the binding change being agreed.
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Vinod Koul
maintainerRe: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds
Quoted 52 lines · Russell King - ARM Linux admin
Quoted 22 lines · Vinod Koul
Quoted 21 lines · 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.
Quoted 7 lines
Quoted 5 lines
This problem was found while script-testing phylink.
Signed-off-by Russell King <[email protected]>--- arch/arm/boot/dts/armada-38x.dtsi | 3 +-lgtm, i need ack for dts parts before I can apply this
I'm not sure what the situation is for Bootlin, but they don't seem to be very responsive right now (covid related?)
What I know from what I've been party to on netdev is that Bootlin sent a patch for the MVPP2 driver, and the very next day someone reported that the patch caused a bug. Unfortunately, the patch got picked up anyway, but there was no response from Bootlin. After a month or so, -final was released containing this patch, so now it had become a regression - and still no response from Bootlin.
Eventually the bug got fixed - not because Bootlin fixed it, but because I ended up spending the time researching how that part of the network driver worked, diagnosing what was going on, and eventually fixing it in the most obvious way - but it's not clear that the fix was the right approach. Bootlin never commented. See
3138a07ce219("net: mvpp2: fix RX hashing for non-10G ports").So, I think we have to assume that Bootlin are struggling right now, and as it's been over a week, it's unlikely that they are going to respond soon. What do you think we should do?
Quoted 7 lines
I also note that Rob has not responded to the DT binding change either, despite me gently prodding, and Rob processing a whole raft of DT binding stuff yesterday.
I can split the DTS change from the rest of the patch, but I don't think that really helps without at least the binding change being agreed.
I would prefer splitting, you may sent the DTS to arm arch folks if no response from subarch folks
Signature · 1 line
~Vinod
Re: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds
Hello,
Quoted 57 lines · Russell King - ARM Linux admin
Quoted 56 lines · Vinod Koul
Quoted 22 lines · Russell King
Quoted 21 lines
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.
Quoted 7 lines
Quoted 5 lines
This problem was found while script-testing phylink.
Signed-off-by Russell King <[email protected]>--- arch/arm/boot/dts/armada-38x.dtsi | 3 +-lgtm, i need ack for dts parts before I can apply this
I'm not sure what the situation is for Bootlin, but they don't seem to be very responsive right now (covid related?)
What I know from what I've been party to on netdev is that Bootlin sent a patch for the MVPP2 driver, and the very next day someone reported that the patch caused a bug. Unfortunately, the patch got picked up anyway, but there was no response from Bootlin. After a month or so, -final was released containing this patch, so now it had become a regression - and still no response from Bootlin.
Eventually the bug got fixed - not because Bootlin fixed it, but because I ended up spending the time researching how that part of the network driver worked, diagnosing what was going on, and eventually fixing it in the most obvious way - but it's not clear that the fix was the right approach. Bootlin never commented. See
3138a07ce219("net: mvpp2: fix RX hashing for non-10G ports").So, I think we have to assume that Bootlin are struggling right now, and as it's been over a week, it's unlikely that they are going to respond soon. What do you think we should do?
I also note that Rob has not responded to the DT binding change either, despite me gently prodding, and Rob processing a whole raft of DT binding stuff yesterday.
Quoted 6 lines
Quoted 3 lines
I can split the DTS change from the rest of the patch, but I don't think that really helps without at least the binding change being agreed.
I would prefer splitting, you may sent the DTS to arm arch folks if no response from subarch folks
Yes please could you split the patch to put the dts apart ? And if the binding is accepted we will apply it.
Thanks,
Gregory
Quoted 3 lines
Signature · 2 lines
-- ~Vinod
Signature · 3 lines
Gregory Clement, Bootlin Embedded Linux and Kernel engineering http://bootlin.com
Re: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds
Quoted 63 lines · Gregory CLEMENT
Hello,
Quoted 61 lines · Russell King - ARM Linux admin
Quoted 59 lines · Vinod Koul
Quoted 22 lines · Russell King
Quoted 21 lines
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.
Quoted 7 lines
Quoted 5 lines
This problem was found while script-testing phylink.
Signed-off-by Russell King <[email protected]>--- arch/arm/boot/dts/armada-38x.dtsi | 3 +-lgtm, i need ack for dts parts before I can apply this
I'm not sure what the situation is for Bootlin, but they don't seem to be very responsive right now (covid related?)
What I know from what I've been party to on netdev is that Bootlin sent a patch for the MVPP2 driver, and the very next day someone reported that the patch caused a bug. Unfortunately, the patch got picked up anyway, but there was no response from Bootlin. After a month or so, -final was released containing this patch, so now it had become a regression - and still no response from Bootlin.
Eventually the bug got fixed - not because Bootlin fixed it, but because I ended up spending the time researching how that part of the network driver worked, diagnosing what was going on, and eventually fixing it in the most obvious way - but it's not clear that the fix was the right approach. Bootlin never commented. See
3138a07ce219("net: mvpp2: fix RX hashing for non-10G ports").So, I think we have to assume that Bootlin are struggling right now, and as it's been over a week, it's unlikely that they are going to respond soon. What do you think we should do?
I also note that Rob has not responded to the DT binding change either, despite me gently prodding, and Rob processing a whole raft of DT binding stuff yesterday.
I can split the DTS change from the rest of the patch, but I don't think that really helps without at least the binding change being agreed.
Quoted 5 lines
Quoted 2 lines
I would prefer splitting, you may sent the DTS to arm arch folks if no response from subarch folks
Yes please could you split the patch to put the dts apart ? And if the binding is accepted we will apply it.
I don't see any sign that Rob will ever review the DTS part, so I'm at the point of just not caring about this anymore. I will carry it in my tree, but I'm going to do nothing further.
That means that switching speed on mvneta on the Armada 38x is can cause the network to die, but hey, if people can't be bothered to review, and wish to impose rules such as "you can't change anything with DT without my express say so" which have the effect of blocking fixes, that's really not my problem.
So, shrug, I'm giving up with these patches. Sorry.
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds
Quoted 74 lines · Russell King - ARM Linux admin
Quoted 63 lines · Gregory CLEMENT
Hello,
Quoted 61 lines · Russell King - ARM Linux admin
Quoted 59 lines · Vinod Koul
Quoted 22 lines · Russell King
Quoted 21 lines
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.
Quoted 7 lines
Quoted 5 lines
This problem was found while script-testing phylink.
Signed-off-by Russell King <[email protected]>--- arch/arm/boot/dts/armada-38x.dtsi | 3 +-lgtm, i need ack for dts parts before I can apply this
I'm not sure what the situation is for Bootlin, but they don't seem to be very responsive right now (covid related?)
What I know from what I've been party to on netdev is that Bootlin sent a patch for the MVPP2 driver, and the very next day someone reported that the patch caused a bug. Unfortunately, the patch got picked up anyway, but there was no response from Bootlin. After a month or so, -final was released containing this patch, so now it had become a regression - and still no response from Bootlin.
Eventually the bug got fixed - not because Bootlin fixed it, but because I ended up spending the time researching how that part of the network driver worked, diagnosing what was going on, and eventually fixing it in the most obvious way - but it's not clear that the fix was the right approach. Bootlin never commented. See
3138a07ce219("net: mvpp2: fix RX hashing for non-10G ports").So, I think we have to assume that Bootlin are struggling right now, and as it's been over a week, it's unlikely that they are going to respond soon. What do you think we should do?
I also note that Rob has not responded to the DT binding change either, despite me gently prodding, and Rob processing a whole raft of DT binding stuff yesterday.
I can split the DTS change from the rest of the patch, but I don't think that really helps without at least the binding change being agreed.
Quoted 5 lines
Quoted 2 lines
I would prefer splitting, you may sent the DTS to arm arch folks if no response from subarch folks
Yes please could you split the patch to put the dts apart ? And if the binding is accepted we will apply it.
I don't see any sign that Rob will ever review the DTS part, so I'm at the point of just not caring about this anymore. I will carry it in my tree, but I'm going to do nothing further.
Quoted 7 lines
That means that switching speed on mvneta on the Armada 38x is can cause the network to die, but hey, if people can't be bothered to review, and wish to impose rules such as "you can't change anything with DT without my express say so" which have the effect of blocking fixes, that's really not my problem.
So, shrug, I'm giving up with these patches. Sorry.
To be clear, this is not aimed at either Vinod or Gregory.
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH 1/2] dt: update Marvell Armada 38x COMPHY binding
In reply to Russell KingQuoted 13 lines · 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.
Quoted 8 lines
However, at the end of the day, due to the undocumented nature of this register, we can only guess.
Signed-off-by Russell King <[email protected]>--- .../devicetree/bindings/phy/phy-armada38x-comphy.txt | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
Vinod Koul
maintainerRe: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds
In reply to Russell King - ARM Linux adminQuoted 81 lines · Russell King - ARM Linux admin
Quoted 80 lines · Russell King - ARM Linux admin
Quoted 63 lines · Gregory CLEMENT
Hello,
Quoted 61 lines · Russell King - ARM Linux admin
Quoted 59 lines · Vinod Koul
Quoted 22 lines · Russell King
Quoted 21 lines
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.
Quoted 7 lines
Quoted 5 lines
This problem was found while script-testing phylink.
Signed-off-by Russell King <[email protected]>--- arch/arm/boot/dts/armada-38x.dtsi | 3 +-lgtm, i need ack for dts parts before I can apply this
I'm not sure what the situation is for Bootlin, but they don't seem to be very responsive right now (covid related?)
What I know from what I've been party to on netdev is that Bootlin sent a patch for the MVPP2 driver, and the very next day someone reported that the patch caused a bug. Unfortunately, the patch got picked up anyway, but there was no response from Bootlin. After a month or so, -final was released containing this patch, so now it had become a regression - and still no response from Bootlin.
Eventually the bug got fixed - not because Bootlin fixed it, but because I ended up spending the time researching how that part of the network driver worked, diagnosing what was going on, and eventually fixing it in the most obvious way - but it's not clear that the fix was the right approach. Bootlin never commented. See
3138a07ce219("net: mvpp2: fix RX hashing for non-10G ports").So, I think we have to assume that Bootlin are struggling right now, and as it's been over a week, it's unlikely that they are going to respond soon. What do you think we should do?
I also note that Rob has not responded to the DT binding change either, despite me gently prodding, and Rob processing a whole raft of DT binding stuff yesterday.
I can split the DTS change from the rest of the patch, but I don't think that really helps without at least the binding change being agreed.
Quoted 5 lines
Quoted 2 lines
I would prefer splitting, you may sent the DTS to arm arch folks if no response from subarch folks
Yes please could you split the patch to put the dts apart ? And if the binding is accepted we will apply it.
I don't see any sign that Rob will ever review the DTS part, so I'm at the point of just not caring about this anymore. I will carry it in my tree, but I'm going to do nothing further.
That means that switching speed on mvneta on the Armada 38x is can cause the network to die, but hey, if people can't be bothered to review, and wish to impose rules such as "you can't change anything with DT without my express say so" which have the effect of blocking fixes, that's really not my problem.
Quoted 3 lines
Quoted 1 line
So, shrug, I'm giving up with these patches. Sorry.
To be clear, this is not aimed at either Vinod or Gregory.
Rob has acked, so if you can respin and split, I can apply
Signature · 1 line
~Vinod