[PATCH] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs

[PATCH] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs

[Pavel.Parkhomenko]

It is mandatory for a software to issue a reset upon modifying RGMII
Receive Timing Control and RGMII Transmit Timing Control bit fields of MAC
Specific Control register 2 (page 2, register 21) otherwise the changes
won't be perceived by the PHY (the same is applicable for a lot of other
registers). Not setting the RGMII delays on the platforms that imply
it's being done on the PHY side will consequently cause the traffic loss.
We discovered that the denoted soft-reset is missing in the
m88e1121_config_aneg() method for the case if the RGMII delays are
modified but the MDIx polarity isn't changed or the auto-negotiation is
left enabled, thus causing the traffic loss on our platform with Marvell
Alaska 88E1510 installed. Let's fix that by issuing the soft-reset if the
delays have been actually set in the m88e1121_config_aneg_rgmii_delays()
method.

Fixes: d6ab93364734 ("net: phy: marvell: Avoid unnecessary soft reset")
Signed-off-by: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
Reviewed-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
 drivers/net/phy/marvell.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 4fcfca4e1702..a4f685927a64 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -551,9 +551,9 @@ static int m88e1121_config_aneg_rgmii_delays(struct
phy_device *phydev)
 	else
 		mscr = 0;
 
-	return phy_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
-				MII_88E1121_PHY_MSCR_REG,
-				MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);
+	return phy_modify_paged_changed(phydev, MII_MARVELL_MSCR_PAGE,
+					MII_88E1121_PHY_MSCR_REG,
+					MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);
 }
 
 static int m88e1121_config_aneg(struct phy_device *phydev)
@@ -567,11 +567,13 @@ static int m88e1121_config_aneg(struct phy_device *phydev)
 			return err;
 	}
 
+	changed = err;
+
 	err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
 	if (err < 0)
 		return err;
 
-	changed = err;
+	changed |= err;
 
 	err = genphy_config_aneg(phydev);
 	if (err < 0)
-- 
2.34.1

Re: [PATCH] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs

[Serge Semin]

+Cc: Heiner, Russel, David, Jakub

-Sergey

On Fri, Feb 04, 2022 at 08:29:11AM +0300, Пархоменко Павел Кириллович wrote:
> It is mandatory for a software to issue a reset upon modifying RGMII
> Receive Timing Control and RGMII Transmit Timing Control bit fields of MAC
> Specific Control register 2 (page 2, register 21) otherwise the changes
> won't be perceived by the PHY (the same is applicable for a lot of other
> registers). Not setting the RGMII delays on the platforms that imply
> it's being done on the PHY side will consequently cause the traffic loss.
> We discovered that the denoted soft-reset is missing in the
> m88e1121_config_aneg() method for the case if the RGMII delays are
> modified but the MDIx polarity isn't changed or the auto-negotiation is
> left enabled, thus causing the traffic loss on our platform with Marvell
> Alaska 88E1510 installed. Let's fix that by issuing the soft-reset if the
> delays have been actually set in the m88e1121_config_aneg_rgmii_delays()
> method.
> 
> Fixes: d6ab93364734 ("net: phy: marvell: Avoid unnecessary soft reset")
> Signed-off-by: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Reviewed-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
> ---
>  drivers/net/phy/marvell.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 4fcfca4e1702..a4f685927a64 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -551,9 +551,9 @@ static int m88e1121_config_aneg_rgmii_delays(struct
> phy_device *phydev)
>  	else
>  		mscr = 0;
>  
> -	return phy_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
> -				MII_88E1121_PHY_MSCR_REG,
> -				MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);
> +	return phy_modify_paged_changed(phydev, MII_MARVELL_MSCR_PAGE,
> +					MII_88E1121_PHY_MSCR_REG,
> +					MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);
>  }
>  
>  static int m88e1121_config_aneg(struct phy_device *phydev)
> @@ -567,11 +567,13 @@ static int m88e1121_config_aneg(struct phy_device *phydev)
>  			return err;
>  	}
>  
> +	changed = err;
> +
>  	err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
>  	if (err < 0)
>  		return err;
>  
> -	changed = err;
> +	changed |= err;
>  
>  	err = genphy_config_aneg(phydev);
>  	if (err < 0)
> -- 
> 2.34.1
> 
> 

Re: [PATCH] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs

[Russell King (Oracle)]

On Fri, Feb 04, 2022 at 12:56:21PM +0300, Serge Semin wrote:
> +Cc: Heiner, Russel, David, Jakub
> 
> -Sergey
> 
> On Fri, Feb 04, 2022 at 08:29:11AM +0300, Пархоменко Павел Кириллович wrote:
> > It is mandatory for a software to issue a reset upon modifying RGMII
> > Receive Timing Control and RGMII Transmit Timing Control bit fields of MAC
> > Specific Control register 2 (page 2, register 21) otherwise the changes
> > won't be perceived by the PHY (the same is applicable for a lot of other
> > registers). Not setting the RGMII delays on the platforms that imply
> > it's being done on the PHY side will consequently cause the traffic loss.
> > We discovered that the denoted soft-reset is missing in the
> > m88e1121_config_aneg() method for the case if the RGMII delays are
> > modified but the MDIx polarity isn't changed or the auto-negotiation is
> > left enabled, thus causing the traffic loss on our platform with Marvell
> > Alaska 88E1510 installed. Let's fix that by issuing the soft-reset if the
> > delays have been actually set in the m88e1121_config_aneg_rgmii_delays()
> > method.

Thanks. Indeed, this appears to be correct for the 88E151x PHYs.

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

> > Fixes: d6ab93364734 ("net: phy: marvell: Avoid unnecessary soft reset")
> > Signed-off-by: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> > Reviewed-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Cc: netdev@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > 
> > ---
> >  drivers/net/phy/marvell.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> > index 4fcfca4e1702..a4f685927a64 100644
> > --- a/drivers/net/phy/marvell.c
> > +++ b/drivers/net/phy/marvell.c
> > @@ -551,9 +551,9 @@ static int m88e1121_config_aneg_rgmii_delays(struct
> > phy_device *phydev)
> >  	else
> >  		mscr = 0;
> >  
> > -	return phy_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
> > -				MII_88E1121_PHY_MSCR_REG,
> > -				MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);
> > +	return phy_modify_paged_changed(phydev, MII_MARVELL_MSCR_PAGE,
> > +					MII_88E1121_PHY_MSCR_REG,
> > +					MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);
> >  }
> >  
> >  static int m88e1121_config_aneg(struct phy_device *phydev)
> > @@ -567,11 +567,13 @@ static int m88e1121_config_aneg(struct phy_device *phydev)
> >  			return err;
> >  	}
> >  
> > +	changed = err;
> > +
> >  	err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
> >  	if (err < 0)
> >  		return err;
> >  
> > -	changed = err;
> > +	changed |= err;
> >  
> >  	err = genphy_config_aneg(phydev);
> >  	if (err < 0)
> > -- 
> > 2.34.1
> > 
> > 
> 

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

Re: [PATCH] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs

[Andrew Lunn]

On Fri, Feb 04, 2022 at 05:29:11AM +0000, Pavel.Parkhomenko@baikalelectronics.ru wrote:
> It is mandatory for a software to issue a reset upon modifying RGMII
> Receive Timing Control and RGMII Transmit Timing Control bit fields of MAC
> Specific Control register 2 (page 2, register 21) otherwise the changes
> won't be perceived by the PHY (the same is applicable for a lot of other
> registers). Not setting the RGMII delays on the platforms that imply
> it's being done on the PHY side will consequently cause the traffic loss.
> We discovered that the denoted soft-reset is missing in the
> m88e1121_config_aneg() method for the case if the RGMII delays are
> modified but the MDIx polarity isn't changed or the auto-negotiation is
> left enabled, thus causing the traffic loss on our platform with Marvell
> Alaska 88E1510 installed. Let's fix that by issuing the soft-reset if the
> delays have been actually set in the m88e1121_config_aneg_rgmii_delays()
> method.

Hi Pavel

There appears to be another path which has the same issue.

m88e1118_config_aneg() calls marvell_set_polarity(), which also needs
a reset afterwards.

Could you fix this case as well?

Thanks
	Andrew

Re: [PATCH] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs

[Jakub Kicinski]

On Fri, 4 Feb 2022 05:29:11 +0000
Pavel.Parkhomenko@baikalelectronics.ru wrote:
> It is mandatory for a software to issue a reset upon modifying RGMII
> Receive Timing Control and RGMII Transmit Timing Control bit fields of MAC
> Specific Control register 2 (page 2, register 21) otherwise the changes
> won't be perceived by the PHY (the same is applicable for a lot of other
> registers). Not setting the RGMII delays on the platforms that imply
> it's being done on the PHY side will consequently cause the traffic loss.
> We discovered that the denoted soft-reset is missing in the
> m88e1121_config_aneg() method for the case if the RGMII delays are
> modified but the MDIx polarity isn't changed or the auto-negotiation is
> left enabled, thus causing the traffic loss on our platform with Marvell
> Alaska 88E1510 installed. Let's fix that by issuing the soft-reset if the
> delays have been actually set in the m88e1121_config_aneg_rgmii_delays()
> method.
> 
> Fixes: d6ab93364734 ("net: phy: marvell: Avoid unnecessary soft reset")
> Signed-off-by: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Reviewed-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Appears not to apply to net, please rebase on top of:

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/

and repost (do keep Russell's tag and address Andrew's question).

Re: [PATCH] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs

[Pavel.Parkhomenko]

On Fri, 04/02/2022 at 14:10 +0100, Andrew Lunn wrote:
> Hi Pavel
> 
> There appears to be another path which has the same issue.
> 
> m88e1118_config_aneg() calls marvell_set_polarity(), which also needs
> a reset afterwards.
> 
> Could you fix this case as well?
> 
> Thanks
>         Andrew

m88e1118_config_aneg() was added back in 2008 and has unconditional
genphy_soft_reset() at the very beginning. I haven't got 88E1118R or
88E11149R by the hand and the full documentation is also not available.
I believe that in this case it would be safe to still issue reset
unconditionally, but do it at the very end of m88e1118_config_aneg().
Anyways, I'd like to post it as a separate patch as I cannot test the fix
properly, unlike previous patch regarding 88E1510.

-- 
Best regards,
Pavel Parkhomenko

Re: [PATCH] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs

[Andrew Lunn]

On Sat, Feb 05, 2022 at 03:22:44PM +0000, Pavel.Parkhomenko@baikalelectronics.ru wrote:
> On Fri, 04/02/2022 at 14:10 +0100, Andrew Lunn wrote:
> > Hi Pavel
> > 
> > There appears to be another path which has the same issue.
> > 
> > m88e1118_config_aneg() calls marvell_set_polarity(), which also needs
> > a reset afterwards.
> > 
> > Could you fix this case as well?
> > 
> > Thanks
> >         Andrew
> 
> m88e1118_config_aneg() was added back in 2008 and has unconditional
> genphy_soft_reset() at the very beginning. I haven't got 88E1118R or
> 88E11149R by the hand and the full documentation is also not available.
> I believe that in this case it would be safe to still issue reset
> unconditionally, but do it at the very end of m88e1118_config_aneg().
> Anyways, I'd like to post it as a separate patch as I cannot test the fix
> properly, unlike previous patch regarding 88E1510.

All the datasheets i have follow the same scheme, you need to perform
a reset at some point to get changes to actually apply. So i doubt
1118r is any different.

Posting a separate patch is fine.

Thanks
	Andrew

[PATCH net v2] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs

[Pavel Parkhomenko]

It is mandatory for a software to issue a reset upon modifying RGMII
Receive Timing Control and RGMII Transmit Timing Control bit fields of MAC
Specific Control register 2 (page 2, register 21) otherwise the changes
won't be perceived by the PHY (the same is applicable for a lot of other
registers). Not setting the RGMII delays on the platforms that imply it'
being done on the PHY side will consequently cause the traffic loss. We
discovered that the denoted soft-reset is missing in the
m88e1121_config_aneg() method for the case if the RGMII delays are
modified but the MDIx polarity isn't changed or the auto-negotiation is
left enabled, thus causing the traffic loss on our platform with Marvell
Alaska 88E1510 installed. Let's fix that by issuing the soft-reset if the
delays have been actually set in the m88e1121_config_aneg_rgmii_delays()
method.

Fixes: d6ab93364734 ("net: phy: marvell: Avoid unnecessary soft reset")
Signed-off-by: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Reviewed-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: stable@vger.kernel.org

---

Link: https://lore.kernel.org/netdev/96759fee7240fd095cb9cc1f6eaf2d9113b57cf0.camel@baikalelectronics.ru/
Changelog v2:
- Add "net" suffix into the PATCH-clause of the subject.
- Cc the patch to the stable tree list.
- Rebase onto the latset netdev/net branch with the top commit
---
 drivers/net/phy/marvell.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index fa71fb7a66b5..4b8f822f8c51 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -553,9 +553,9 @@ static int m88e1121_config_aneg_rgmii_delays(struct phy_device *phydev)
 	else
 		mscr = 0;
 
-	return phy_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
-				MII_88E1121_PHY_MSCR_REG,
-				MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);
+	return phy_modify_paged_changed(phydev, MII_MARVELL_MSCR_PAGE,
+					MII_88E1121_PHY_MSCR_REG,
+					MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);
 }
 
 static int m88e1121_config_aneg(struct phy_device *phydev)
@@ -569,11 +569,13 @@ static int m88e1121_config_aneg(struct phy_device *phydev)
 			return err;
 	}
 
+	changed = err;
+
 	err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
 	if (err < 0)
 		return err;
 
-	changed = err;
+	changed |= err;
 
 	err = genphy_config_aneg(phydev);
 	if (err < 0)
@@ -1211,17 +1213,22 @@ static int m88e1510_config_init(struct phy_device *phydev)
 
 static int m88e1118_config_aneg(struct phy_device *phydev)
 {
+	int changed = 0;
 	int err;
 
-	err = genphy_soft_reset(phydev);
+	err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
 	if (err < 0)
 		return err;
 
-	err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
+	changed = err;
+
+	err = genphy_config_aneg(phydev);
 	if (err < 0)
 		return err;
 
-	err = genphy_config_aneg(phydev);
+	if (phydev->autoneg != AUTONEG_ENABLE || changed)
+		return genphy_soft_reset(phydev);
+
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH net v2] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs

[Serge Semin]

Folks,

Ah, sorry for the noise. A wrong version of the patch was by mistake sent to
you, Russel, and to the netdev stable list only. Please just ignore
it. There is another patch, which we are going to re-send shortly to
all the required addressee.

-Serge

On Sat, Feb 05, 2022 at 10:08:14PM +0300, Pavel Parkhomenko wrote:
> It is mandatory for a software to issue a reset upon modifying RGMII
> Receive Timing Control and RGMII Transmit Timing Control bit fields of MAC
> Specific Control register 2 (page 2, register 21) otherwise the changes
> won't be perceived by the PHY (the same is applicable for a lot of other
> registers). Not setting the RGMII delays on the platforms that imply it'
> being done on the PHY side will consequently cause the traffic loss. We
> discovered that the denoted soft-reset is missing in the
> m88e1121_config_aneg() method for the case if the RGMII delays are
> modified but the MDIx polarity isn't changed or the auto-negotiation is
> left enabled, thus causing the traffic loss on our platform with Marvell
> Alaska 88E1510 installed. Let's fix that by issuing the soft-reset if the
> delays have been actually set in the m88e1121_config_aneg_rgmii_delays()
> method.
> 
> Fixes: d6ab93364734 ("net: phy: marvell: Avoid unnecessary soft reset")
> Signed-off-by: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> Reviewed-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: stable@vger.kernel.org
> 
> ---
> 
> Link: https://lore.kernel.org/netdev/96759fee7240fd095cb9cc1f6eaf2d9113b57cf0.camel@baikalelectronics.ru/
> Changelog v2:
> - Add "net" suffix into the PATCH-clause of the subject.
> - Cc the patch to the stable tree list.
> - Rebase onto the latset netdev/net branch with the top commit
> ---
>  drivers/net/phy/marvell.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index fa71fb7a66b5..4b8f822f8c51 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -553,9 +553,9 @@ static int m88e1121_config_aneg_rgmii_delays(struct phy_device *phydev)
>  	else
>  		mscr = 0;
>  
> -	return phy_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
> -				MII_88E1121_PHY_MSCR_REG,
> -				MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);
> +	return phy_modify_paged_changed(phydev, MII_MARVELL_MSCR_PAGE,
> +					MII_88E1121_PHY_MSCR_REG,
> +					MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);
>  }
>  
>  static int m88e1121_config_aneg(struct phy_device *phydev)
> @@ -569,11 +569,13 @@ static int m88e1121_config_aneg(struct phy_device *phydev)
>  			return err;
>  	}
>  
> +	changed = err;
> +
>  	err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
>  	if (err < 0)
>  		return err;
>  
> -	changed = err;
> +	changed |= err;
>  
>  	err = genphy_config_aneg(phydev);
>  	if (err < 0)
> @@ -1211,17 +1213,22 @@ static int m88e1510_config_init(struct phy_device *phydev)
>  
>  static int m88e1118_config_aneg(struct phy_device *phydev)
>  {
> +	int changed = 0;
>  	int err;
>  
> -	err = genphy_soft_reset(phydev);
> +	err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
>  	if (err < 0)
>  		return err;
>  
> -	err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
> +	changed = err;
> +
> +	err = genphy_config_aneg(phydev);
>  	if (err < 0)
>  		return err;
>  
> -	err = genphy_config_aneg(phydev);
> +	if (phydev->autoneg != AUTONEG_ENABLE || changed)
> +		return genphy_soft_reset(phydev);
> +
>  	return 0;
>  }
>  
> -- 
> 2.34.1
> 

[PATCH net v2] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs

[Pavel Parkhomenko]

It is mandatory for a software to issue a reset upon modifying RGMII
Receive Timing Control and RGMII Transmit Timing Control bit fields of MAC
Specific Control register 2 (page 2, register 21) otherwise the changes
won't be perceived by the PHY (the same is applicable for a lot of other
registers). Not setting the RGMII delays on the platforms that imply it'
being done on the PHY side will consequently cause the traffic loss. We
discovered that the denoted soft-reset is missing in the
m88e1121_config_aneg() method for the case if the RGMII delays are
modified but the MDIx polarity isn't changed or the auto-negotiation is
left enabled, thus causing the traffic loss on our platform with Marvell
Alaska 88E1510 installed. Let's fix that by issuing the soft-reset if the
delays have been actually set in the m88e1121_config_aneg_rgmii_delays()
method.

Fixes: d6ab93364734 ("net: phy: marvell: Avoid unnecessary soft reset")
Signed-off-by: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
Cc: stable@vger.kernel.org

---

Link: https://lore.kernel.org/netdev/96759fee7240fd095cb9cc1f6eaf2d9113b57cf0.camel@baikalelectronics.ru/
Changelog v2:
- Add "net" suffix into the PATCH-clause of the subject.
- Cc the patch to the stable tree list.
- Rebase onto the latset netdev/net branch with the top commit 59085208e4a2
("net: mscc: ocelot: fix all IP traffic getting trapped to CPU with PTP over
IP")
---
 drivers/net/phy/marvell.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index fa71fb7a66b5..e2fd1252be48 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -553,9 +553,9 @@ static int m88e1121_config_aneg_rgmii_delays(struct phy_device *phydev)
 	else
 		mscr = 0;
 
-	return phy_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
-				MII_88E1121_PHY_MSCR_REG,
-				MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);
+	return phy_modify_paged_changed(phydev, MII_MARVELL_MSCR_PAGE,
+					MII_88E1121_PHY_MSCR_REG,
+					MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);
 }
 
 static int m88e1121_config_aneg(struct phy_device *phydev)
@@ -569,11 +569,13 @@ static int m88e1121_config_aneg(struct phy_device *phydev)
 			return err;
 	}
 
+	changed = err;
+
 	err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
 	if (err < 0)
 		return err;
 
-	changed = err;
+	changed |= err;
 
 	err = genphy_config_aneg(phydev);
 	if (err < 0)
-- 
2.32.0

Re: [PATCH net v2] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs

[Jakub Kicinski]

On Sat, 5 Feb 2022 23:39:32 +0300 Pavel Parkhomenko wrote:
> It is mandatory for a software to issue a reset upon modifying RGMII
> Receive Timing Control and RGMII Transmit Timing Control bit fields of MAC
> Specific Control register 2 (page 2, register 21) otherwise the changes
> won't be perceived by the PHY (the same is applicable for a lot of other
> registers). Not setting the RGMII delays on the platforms that imply it'
> being done on the PHY side will consequently cause the traffic loss. We
> discovered that the denoted soft-reset is missing in the
> m88e1121_config_aneg() method for the case if the RGMII delays are
> modified but the MDIx polarity isn't changed or the auto-negotiation is
> left enabled, thus causing the traffic loss on our platform with Marvell
> Alaska 88E1510 installed. Let's fix that by issuing the soft-reset if the
> delays have been actually set in the m88e1121_config_aneg_rgmii_delays()
> method.
> 
> Fixes: d6ab93364734 ("net: phy: marvell: Avoid unnecessary soft reset")
> Signed-off-by: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> Cc: stable@vger.kernel.org
> 
> ---
> 
> Link: https://lore.kernel.org/netdev/96759fee7240fd095cb9cc1f6eaf2d9113b57cf0.camel@baikalelectronics.ru/
> Changelog v2:
> - Add "net" suffix into the PATCH-clause of the subject.
> - Cc the patch to the stable tree list.
> - Rebase onto the latset netdev/net branch with the top commit 59085208e4a2
> ("net: mscc: ocelot: fix all IP traffic getting trapped to CPU with PTP over IP")

This patch is valid and waiting to be reviewed & applied, right?
I see it's marked as Superseded in patchwork, but can't track down a v3.

Re: [PATCH net v2] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs

[Serge Semin]

Hello Jakub

On Mon, Feb 07, 2022 at 09:40:39AM -0800, Jakub Kicinski wrote:
> On Sat, 5 Feb 2022 23:39:32 +0300 Pavel Parkhomenko wrote:
> > It is mandatory for a software to issue a reset upon modifying RGMII
> > Receive Timing Control and RGMII Transmit Timing Control bit fields of MAC
> > Specific Control register 2 (page 2, register 21) otherwise the changes
> > won't be perceived by the PHY (the same is applicable for a lot of other
> > registers). Not setting the RGMII delays on the platforms that imply it'
> > being done on the PHY side will consequently cause the traffic loss. We
> > discovered that the denoted soft-reset is missing in the
> > m88e1121_config_aneg() method for the case if the RGMII delays are
> > modified but the MDIx polarity isn't changed or the auto-negotiation is
> > left enabled, thus causing the traffic loss on our platform with Marvell
> > Alaska 88E1510 installed. Let's fix that by issuing the soft-reset if the
> > delays have been actually set in the m88e1121_config_aneg_rgmii_delays()
> > method.
> > 
> > Fixes: d6ab93364734 ("net: phy: marvell: Avoid unnecessary soft reset")
> > Signed-off-by: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> > Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> > Cc: stable@vger.kernel.org
> > 
> > ---
> > 
> > Link: https://lore.kernel.org/netdev/96759fee7240fd095cb9cc1f6eaf2d9113b57cf0.camel@baikalelectronics.ru/
> > Changelog v2:
> > - Add "net" suffix into the PATCH-clause of the subject.
> > - Cc the patch to the stable tree list.
> > - Rebase onto the latset netdev/net branch with the top commit 59085208e4a2
> > ("net: mscc: ocelot: fix all IP traffic getting trapped to CPU with PTP over IP")
> 

> This patch is valid and waiting to be reviewed & applied, right?

Right.

> I see it's marked as Superseded in patchwork, but can't track down a v3.

We had accidentally sent out a temporal v2 version before submitting this
one. The failed patch is here
Link: https://lore.kernel.org/stable/20220205190814.20282-1-Pavel.Parkhomenko@baikalelectronics.ru/
But the message was sent to Russel and to the stable mailing list only
with no netdev list being in Cc. I thought if the right v2 was sent
out after the failed one, then even if patchwork somehow gets to catch
both of the messages, the former patch would have at least superseded
the later one. It appears I was wrong. Sorry about that. Do you want
us to resend this patch as v3 to have a proper patchwork status?

-Sergey

Re: [PATCH net v2] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs

[Jakub Kicinski]

On Mon, 7 Feb 2022 21:33:19 +0300 Serge Semin wrote:
> > I see it's marked as Superseded in patchwork, but can't track down a v3.  
> 
> We had accidentally sent out a temporal v2 version before submitting this
> one. The failed patch is here
> Link: https://lore.kernel.org/stable/20220205190814.20282-1-Pavel.Parkhomenko@baikalelectronics.ru/
> But the message was sent to Russel and to the stable mailing list only
> with no netdev list being in Cc. I thought if the right v2 was sent
> out after the failed one, then even if patchwork somehow gets to catch
> both of the messages, the former patch would have at least superseded
> the later one. It appears I was wrong. Sorry about that. Do you want
> us to resend this patch as v3 to have a proper patchwork status?

No need, I set it back to New, thanks!

Re: [PATCH net v2] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs

[Jakub Kicinski]

On Mon, 7 Feb 2022 11:22:39 -0800 Jakub Kicinski wrote:
> On Mon, 7 Feb 2022 21:33:19 +0300 Serge Semin wrote:
> > > I see it's marked as Superseded in patchwork, but can't track down a v3.    
> > 
> > We had accidentally sent out a temporal v2 version before submitting this
> > one. The failed patch is here
> > Link: https://lore.kernel.org/stable/20220205190814.20282-1-Pavel.Parkhomenko@baikalelectronics.ru/
> > But the message was sent to Russel and to the stable mailing list only
> > with no netdev list being in Cc. I thought if the right v2 was sent
> > out after the failed one, then even if patchwork somehow gets to catch
> > both of the messages, the former patch would have at least superseded
> > the later one. It appears I was wrong. Sorry about that. Do you want
> > us to resend this patch as v3 to have a proper patchwork status?  
> 
> No need, I set it back to New, thanks!

Applied now, commit fe4f57bf7b58 ("net: phy: marvell: Fix RGMII Tx/Rx
delays setting in 88e1121-compatible PHYs"), thanks!

Re: [PATCH net v2] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Sat, 5 Feb 2022 23:39:32 +0300 you wrote:
> It is mandatory for a software to issue a reset upon modifying RGMII
> Receive Timing Control and RGMII Transmit Timing Control bit fields of MAC
> Specific Control register 2 (page 2, register 21) otherwise the changes
> won't be perceived by the PHY (the same is applicable for a lot of other
> registers). Not setting the RGMII delays on the platforms that imply it'
> being done on the PHY side will consequently cause the traffic loss. We
> discovered that the denoted soft-reset is missing in the
> m88e1121_config_aneg() method for the case if the RGMII delays are
> modified but the MDIx polarity isn't changed or the auto-negotiation is
> left enabled, thus causing the traffic loss on our platform with Marvell
> Alaska 88E1510 installed. Let's fix that by issuing the soft-reset if the
> delays have been actually set in the m88e1121_config_aneg_rgmii_delays()
> method.
> 
> [...]

Here is the summary with links:
  - [net,v2] net: phy: marvell: Fix RGMII Tx/Rx delays setting in 88e1121-compatible PHYs
    https://git.kernel.org/netdev/net/c/fe4f57bf7b58

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html