* Re: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds
2020-06-30 16:05 ` [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds Russell King
@ 2020-06-30 16:06 ` Russell King - ARM Linux admin
2020-07-01 6:57 ` Vinod Koul
1 sibling, 0 replies; 13+ messages in thread
From: Russell King - ARM Linux admin @ 2020-06-30 16:06 UTC (permalink / raw)
To: Andrew Lunn, Gregory Clement, Jason Cooper,
Kishon Vijay Abraham I, Rob Herring, Sebastian Hesselbarth,
Vinod Koul
Cc: devicetree, linux-arm-kernel
On Tue, Jun 30, 2020 at 05:05:38PM +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.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
I forgot...
Fixes: 14dc100b4411 ("phy: armada38x: add common phy support")
> ---
> arch/arm/boot/dts/armada-38x.dtsi | 3 +-
> drivers/phy/marvell/phy-armada38x-comphy.c | 45 ++++++++++++++++++----
> 2 files changed, 40 insertions(+), 8 deletions(-)
>
> 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>;
>
> 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
>
>
--
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds
2020-06-30 16:05 ` [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds Russell King
2020-06-30 16:06 ` Russell King - ARM Linux admin
@ 2020-07-01 6:57 ` Vinod Koul
2020-07-10 15:19 ` Russell King - ARM Linux admin
1 sibling, 1 reply; 13+ messages in thread
From: Vinod Koul @ 2020-07-01 6:57 UTC (permalink / raw)
To: Russell King
Cc: Andrew Lunn, Jason Cooper, devicetree, Gregory Clement,
Kishon Vijay Abraham I, Rob Herring, linux-arm-kernel,
Sebastian Hesselbarth
On 30-06-20, 17:05, 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.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> arch/arm/boot/dts/armada-38x.dtsi | 3 +-
lgtm, i need ack for dts parts before I can apply this
> drivers/phy/marvell/phy-armada38x-comphy.c | 45 ++++++++++++++++++----
> 2 files changed, 40 insertions(+), 8 deletions(-)
>
> 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>;
>
> 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
--
~Vinod
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds
2020-07-01 6:57 ` Vinod Koul
@ 2020-07-10 15:19 ` Russell King - ARM Linux admin
2020-07-13 6:18 ` Vinod Koul
0 siblings, 1 reply; 13+ messages in thread
From: Russell King - ARM Linux admin @ 2020-07-10 15:19 UTC (permalink / raw)
To: Vinod Koul
Cc: Andrew Lunn, Jason Cooper, devicetree, Gregory Clement,
Kishon Vijay Abraham I, Rob Herring, linux-arm-kernel,
Sebastian Hesselbarth
On Wed, Jul 01, 2020 at 12:27:27PM +0530, Vinod Koul wrote:
> On 30-06-20, 17:05, 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.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> > 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.
--
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds
2020-07-10 15:19 ` Russell King - ARM Linux admin
@ 2020-07-13 6:18 ` Vinod Koul
2020-07-13 15:36 ` Gregory CLEMENT
0 siblings, 1 reply; 13+ messages in thread
From: Vinod Koul @ 2020-07-13 6:18 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Jason Cooper, devicetree, Gregory Clement,
Kishon Vijay Abraham I, Rob Herring, linux-arm-kernel,
Sebastian Hesselbarth
On 10-07-20, 16:19, Russell King - ARM Linux admin wrote:
> On Wed, Jul 01, 2020 at 12:27:27PM +0530, Vinod Koul wrote:
> > On 30-06-20, 17:05, 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.
> > >
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > ---
> > > 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.
I would prefer splitting, you may sent the DTS to arm arch folks if no
response from subarch folks
--
~Vinod
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds
2020-07-13 6:18 ` Vinod Koul
@ 2020-07-13 15:36 ` Gregory CLEMENT
2020-07-13 17:21 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 13+ messages in thread
From: Gregory CLEMENT @ 2020-07-13 15:36 UTC (permalink / raw)
To: Vinod Koul, Russell King - ARM Linux admin
Cc: Andrew Lunn, Jason Cooper, devicetree, Kishon Vijay Abraham I,
Rob Herring, linux-arm-kernel, Sebastian Hesselbarth
Hello,
> On 10-07-20, 16:19, Russell King - ARM Linux admin wrote:
>> On Wed, Jul 01, 2020 at 12:27:27PM +0530, Vinod Koul wrote:
>> > On 30-06-20, 17:05, 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.
>> > >
>> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>> > > ---
>> > > 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.
>
> 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
>
> --
> ~Vinod
--
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds
2020-07-13 15:36 ` Gregory CLEMENT
@ 2020-07-13 17:21 ` Russell King - ARM Linux admin
2020-07-13 18:07 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 13+ messages in thread
From: Russell King - ARM Linux admin @ 2020-07-13 17:21 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Andrew Lunn, Jason Cooper, devicetree, Kishon Vijay Abraham I,
Vinod Koul, Rob Herring, linux-arm-kernel, Sebastian Hesselbarth
On Mon, Jul 13, 2020 at 05:36:54PM +0200, Gregory CLEMENT wrote:
> Hello,
>
> > On 10-07-20, 16:19, Russell King - ARM Linux admin wrote:
> >> On Wed, Jul 01, 2020 at 12:27:27PM +0530, Vinod Koul wrote:
> >> > On 30-06-20, 17:05, 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.
> >> > >
> >> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> >> > > ---
> >> > > 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.
> >
> > 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.
--
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds
2020-07-13 17:21 ` Russell King - ARM Linux admin
@ 2020-07-13 18:07 ` Russell King - ARM Linux admin
2020-07-16 5:46 ` Vinod Koul
0 siblings, 1 reply; 13+ messages in thread
From: Russell King - ARM Linux admin @ 2020-07-13 18:07 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Andrew Lunn, Jason Cooper, devicetree, Kishon Vijay Abraham I,
Vinod Koul, Rob Herring, linux-arm-kernel, Sebastian Hesselbarth
On Mon, Jul 13, 2020 at 06:21:40PM +0100, Russell King - ARM Linux admin wrote:
> On Mon, Jul 13, 2020 at 05:36:54PM +0200, Gregory CLEMENT wrote:
> > Hello,
> >
> > > On 10-07-20, 16:19, Russell King - ARM Linux admin wrote:
> > >> On Wed, Jul 01, 2020 at 12:27:27PM +0530, Vinod Koul wrote:
> > >> > On 30-06-20, 17:05, 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.
> > >> > >
> > >> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > >> > > ---
> > >> > > 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.
> > >
> > > 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.
To be clear, this is not aimed at either Vinod or Gregory.
--
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds
2020-07-13 18:07 ` Russell King - ARM Linux admin
@ 2020-07-16 5:46 ` Vinod Koul
0 siblings, 0 replies; 13+ messages in thread
From: Vinod Koul @ 2020-07-16 5:46 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Jason Cooper, devicetree, Gregory CLEMENT,
Kishon Vijay Abraham I, Rob Herring, linux-arm-kernel,
Sebastian Hesselbarth
On 13-07-20, 19:07, Russell King - ARM Linux admin wrote:
> On Mon, Jul 13, 2020 at 06:21:40PM +0100, Russell King - ARM Linux admin wrote:
> > On Mon, Jul 13, 2020 at 05:36:54PM +0200, Gregory CLEMENT wrote:
> > > Hello,
> > >
> > > > On 10-07-20, 16:19, Russell King - ARM Linux admin wrote:
> > > >> On Wed, Jul 01, 2020 at 12:27:27PM +0530, Vinod Koul wrote:
> > > >> > On 30-06-20, 17:05, 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.
> > > >> > >
> > > >> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > >> > > ---
> > > >> > > 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.
> > > >
> > > > 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.
>
> 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
--
~Vinod
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 13+ messages in thread