diff mbox series

[v2] net: phy: dp83822: Fix RGMII TX delay configuration

Message ID 20240204101128.49336-1-tp@osasysteme.de
State New
Headers
Series [v2] net: phy: dp83822: Fix RGMII TX delay configuration |

Commit Message

Tim Pambor Feb. 4, 2024, 10:11 a.m. UTC 
  The logic for enabling the TX clock shift is inverse of enabling the RX
clock shift. The TX clock shift is disabled when DP83822_TX_CLK_SHIFT is
set. Correct the current behavior and always write the delay configuration
to ensure consistent delay settings regardless of bootloader configuration.

Reference: https://www.ti.com/lit/ds/symlink/dp83822i.pdf p. 69

Fixes: 8095295292b5 ("net: phy: DP83822: Add setting the fixed internal delay")
Signed-off-by: Tim Pambor <tp@osasysteme.de>
---
Changes in v2:
  - Further cleanup of RGMII configuration
  - Check for errors setting DP83822_RGMII_MODE_EN
---
 drivers/net/phy/dp83822.c | 41 +++++++++++++--------------------------
 1 file changed, 14 insertions(+), 27 deletions(-)

Comments

Paolo Abeni Feb. 8, 2024, 10:12 a.m. UTC | #1 
On Sun, 2024-02-04 at 11:11 +0100, Tim Pambor wrote:
> The logic for enabling the TX clock shift is inverse of enabling the RX
> clock shift. The TX clock shift is disabled when DP83822_TX_CLK_SHIFT is
> set. Correct the current behavior and always write the delay configuration
> to ensure consistent delay settings regardless of bootloader configuration.
> 
> Reference: https://www.ti.com/lit/ds/symlink/dp83822i.pdf p. 69
> 
> Fixes: 8095295292b5 ("net: phy: DP83822: Add setting the fixed internal delay")
> Signed-off-by: Tim Pambor <tp@osasysteme.de>
> ---
> Changes in v2:
>   - Further cleanup of RGMII configuration

This overall makes the change more invasive, I'm unsure is in the
direction pointed by Russell

>   - Check for errors setting DP83822_RGMII_MODE_EN
> ---
>  drivers/net/phy/dp83822.c | 41 +++++++++++++--------------------------
>  1 file changed, 14 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
> index b7cb71817780..1b2c34a97396 100644
> --- a/drivers/net/phy/dp83822.c
> +++ b/drivers/net/phy/dp83822.c
> @@ -380,42 +380,29 @@ static int dp83822_config_init(struct phy_device *phydev)
>  {
>  	struct dp83822_private *dp83822 = phydev->priv;
>  	struct device *dev = &phydev->mdio.dev;
> -	int rgmii_delay;
> -	s32 rx_int_delay;
> -	s32 tx_int_delay;
> +	int rcsr_mask = DP83822_RGMII_MODE_EN;
> +	int rcsr = 0;
>  	int err = 0;
>  	int bmcr;
>  
>  	if (phy_interface_is_rgmii(phydev)) {
> -		rx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0,
> -						      true);
> +		rcsr |= DP83822_RGMII_MODE_EN;
>  
> -		if (rx_int_delay <= 0)
> -			rgmii_delay = 0;
> -		else
> -			rgmii_delay = DP83822_RX_CLK_SHIFT;
> +		/* Set DP83822_RX_CLK_SHIFT to enable rx clk internal delay */
> +		if (phy_get_internal_delay(phydev, dev, NULL, 0, true) > 0)
> +			rcsr |= DP83822_RX_CLK_SHIFT;
>  
> -		tx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0,
> -						      false);
> -		if (tx_int_delay <= 0)
> -			rgmii_delay &= ~DP83822_TX_CLK_SHIFT;
> -		else
> -			rgmii_delay |= DP83822_TX_CLK_SHIFT;
> +		/* Set DP83822_TX_CLK_SHIFT to disable tx clk internal delay */
> +		if (phy_get_internal_delay(phydev, dev, NULL, 0, false) <= 0)
> +			rcsr |= DP83822_TX_CLK_SHIFT;
>  
> -		if (rgmii_delay) {
> -			err = phy_set_bits_mmd(phydev, DP83822_DEVADDR,
> -					       MII_DP83822_RCSR, rgmii_delay);
> -			if (err)
> -				return err;
> -		}
> -
> -		phy_set_bits_mmd(phydev, DP83822_DEVADDR,
> -					MII_DP83822_RCSR, DP83822_RGMII_MODE_EN);
> -	} else {
> -		phy_clear_bits_mmd(phydev, DP83822_DEVADDR,
> -					MII_DP83822_RCSR, DP83822_RGMII_MODE_EN);
> +		rcsr_mask |= DP83822_RX_CLK_SHIFT | DP83822_TX_CLK_SHIFT;

It looks like there is a change of behavior here. The current code
clear the DP83822_RGMII_MODE_EN, while this patch will clear
DP83822_RX_CLK_SHIFT | DP83822_TX_CLK_SHIFT, leaving
DP83822_RGMII_MODE_EN unmodified.

It does not look correct to me.

Cheers,

Paolo
Tim Pambor Feb. 13, 2024, 1:07 p.m. UTC | #2 
Hi Paolo,

On Thu, 2024-02-08 at 11:12 +0100, Paolo Abeni wrote:
> On Sun, 2024-02-04 at 11:11 +0100, Tim Pambor wrote:
> > The logic for enabling the TX clock shift is inverse of enabling
> > the RX
> > clock shift. The TX clock shift is disabled when
> > DP83822_TX_CLK_SHIFT is
> > set. Correct the current behavior and always write the delay
> > configuration
> > to ensure consistent delay settings regardless of bootloader
> > configuration.
> > 
> > Reference: https://www.ti.com/lit/ds/symlink/dp83822i.pdf p. 69
> > 
> > Fixes: 8095295292b5 ("net: phy: DP83822: Add setting the fixed
> > internal delay")
> > Signed-off-by: Tim Pambor <tp@osasysteme.de>
> > ---
> > Changes in v2:
> >   - Further cleanup of RGMII configuration
> 
> This overall makes the change more invasive, I'm unsure is in the
> direction pointed by Russell
> 
> >   - Check for errors setting DP83822_RGMII_MODE_EN
> > ---
> >  drivers/net/phy/dp83822.c | 41 +++++++++++++----------------------
> > ----
> >  1 file changed, 14 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
> > index b7cb71817780..1b2c34a97396 100644
> > --- a/drivers/net/phy/dp83822.c
> > +++ b/drivers/net/phy/dp83822.c
> > @@ -380,42 +380,29 @@ static int dp83822_config_init(struct
> > phy_device *phydev)
> >  {
> >  	struct dp83822_private *dp83822 = phydev->priv;
> >  	struct device *dev = &phydev->mdio.dev;
> > -	int rgmii_delay;
> > -	s32 rx_int_delay;
> > -	s32 tx_int_delay;
> > +	int rcsr_mask = DP83822_RGMII_MODE_EN;
> > +	int rcsr = 0;
> >  	int err = 0;
> >  	int bmcr;
> >  
> >  	if (phy_interface_is_rgmii(phydev)) {
> > -		rx_int_delay = phy_get_internal_delay(phydev, dev,
> > NULL, 0,
> > -						      true);
> > +		rcsr |= DP83822_RGMII_MODE_EN;
> >  
> > -		if (rx_int_delay <= 0)
> > -			rgmii_delay = 0;
> > -		else
> > -			rgmii_delay = DP83822_RX_CLK_SHIFT;
> > +		/* Set DP83822_RX_CLK_SHIFT to enable rx clk
> > internal delay */
> > +		if (phy_get_internal_delay(phydev, dev, NULL, 0,
> > true) > 0)
> > +			rcsr |= DP83822_RX_CLK_SHIFT;
> >  
> > -		tx_int_delay = phy_get_internal_delay(phydev, dev,
> > NULL, 0,
> > -						      false);
> > -		if (tx_int_delay <= 0)
> > -			rgmii_delay &= ~DP83822_TX_CLK_SHIFT;
> > -		else
> > -			rgmii_delay |= DP83822_TX_CLK_SHIFT;
> > +		/* Set DP83822_TX_CLK_SHIFT to disable tx clk
> > internal delay */
> > +		if (phy_get_internal_delay(phydev, dev, NULL, 0,
> > false) <= 0)
> > +			rcsr |= DP83822_TX_CLK_SHIFT;
> >  
> > -		if (rgmii_delay) {
> > -			err = phy_set_bits_mmd(phydev,
> > DP83822_DEVADDR,
> > -					       MII_DP83822_RCSR,
> > rgmii_delay);
> > -			if (err)
> > -				return err;
> > -		}
> > -
> > -		phy_set_bits_mmd(phydev, DP83822_DEVADDR,
> > -					MII_DP83822_RCSR,
> > DP83822_RGMII_MODE_EN);
> > -	} else {
> > -		phy_clear_bits_mmd(phydev, DP83822_DEVADDR,
> > -					MII_DP83822_RCSR,
> > DP83822_RGMII_MODE_EN);
> > +		rcsr_mask |= DP83822_RX_CLK_SHIFT |
> > DP83822_TX_CLK_SHIFT;
> 
> It looks like there is a change of behavior here. The current code
> clear the DP83822_RGMII_MODE_EN, while this patch will clear
> DP83822_RX_CLK_SHIFT | DP83822_TX_CLK_SHIFT, leaving
> DP83822_RGMII_MODE_EN unmodified.

The behavior of DP83822_RGMII_MODE_EN should not have changed.
DP83822_RGMII_MODE_EN is always set in rcsr_mask and if the interface
is RGMII, DP83822_RX_CLK_SHIFT and DP83822_TX_CLK_SHIFT are added to
the mask as well.

I also have tested this patch on hardware with a DP83822 phy in RGMII
mode.

> 
> It does not look correct to me.
> 
> Cheers,
> 
> Paolo
>
diff mbox series

Patch

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index b7cb71817780..1b2c34a97396 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -380,42 +380,29 @@  static int dp83822_config_init(struct phy_device *phydev)
 {
 	struct dp83822_private *dp83822 = phydev->priv;
 	struct device *dev = &phydev->mdio.dev;
-	int rgmii_delay;
-	s32 rx_int_delay;
-	s32 tx_int_delay;
+	int rcsr_mask = DP83822_RGMII_MODE_EN;
+	int rcsr = 0;
 	int err = 0;
 	int bmcr;
 
 	if (phy_interface_is_rgmii(phydev)) {
-		rx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0,
-						      true);
+		rcsr |= DP83822_RGMII_MODE_EN;
 
-		if (rx_int_delay <= 0)
-			rgmii_delay = 0;
-		else
-			rgmii_delay = DP83822_RX_CLK_SHIFT;
+		/* Set DP83822_RX_CLK_SHIFT to enable rx clk internal delay */
+		if (phy_get_internal_delay(phydev, dev, NULL, 0, true) > 0)
+			rcsr |= DP83822_RX_CLK_SHIFT;
 
-		tx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0,
-						      false);
-		if (tx_int_delay <= 0)
-			rgmii_delay &= ~DP83822_TX_CLK_SHIFT;
-		else
-			rgmii_delay |= DP83822_TX_CLK_SHIFT;
+		/* Set DP83822_TX_CLK_SHIFT to disable tx clk internal delay */
+		if (phy_get_internal_delay(phydev, dev, NULL, 0, false) <= 0)
+			rcsr |= DP83822_TX_CLK_SHIFT;
 
-		if (rgmii_delay) {
-			err = phy_set_bits_mmd(phydev, DP83822_DEVADDR,
-					       MII_DP83822_RCSR, rgmii_delay);
-			if (err)
-				return err;
-		}
-
-		phy_set_bits_mmd(phydev, DP83822_DEVADDR,
-					MII_DP83822_RCSR, DP83822_RGMII_MODE_EN);
-	} else {
-		phy_clear_bits_mmd(phydev, DP83822_DEVADDR,
-					MII_DP83822_RCSR, DP83822_RGMII_MODE_EN);
+		rcsr_mask |= DP83822_RX_CLK_SHIFT | DP83822_TX_CLK_SHIFT;
 	}
 
+	err = phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RCSR, rcsr_mask, rcsr);
+	if (err < 0)
+		return err;
+
 	if (dp83822->fx_enabled) {
 		err = phy_modify(phydev, MII_DP83822_CTRL_2,
 				 DP83822_FX_ENABLE, 1);