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

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

[Tim Pambor]

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>
---
 drivers/net/phy/dp83822.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index b7cb71817780..b061036f654a 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -398,16 +398,15 @@ static int dp83822_config_init(struct phy_device *phydev)
 		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;
+		else
+			rgmii_delay &= ~DP83822_TX_CLK_SHIFT;
 
-		if (rgmii_delay) {
-			err = phy_set_bits_mmd(phydev, DP83822_DEVADDR,
-					       MII_DP83822_RCSR, rgmii_delay);
-			if (err)
-				return err;
-		}
+		err = phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83822_RCSR,
+				     DP83822_RX_CLK_SHIFT | DP83822_TX_CLK_SHIFT, rgmii_delay);
+
+		if (err < 0)
+			return err;
 
 		phy_set_bits_mmd(phydev, DP83822_DEVADDR,
 					MII_DP83822_RCSR, DP83822_RGMII_MODE_EN);
-- 
2.43.0

Re: [PATCH] net: phy: dp83822: Fix RGMII TX delay configuration

[Russell King (Oracle)]

On Sat, Feb 03, 2024 at 02:11:51PM +0100, Tim Pambor wrote:
> diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
> index b7cb71817780..b061036f654a 100644
> --- a/drivers/net/phy/dp83822.c
> +++ b/drivers/net/phy/dp83822.c
> @@ -398,16 +398,15 @@ static int dp83822_config_init(struct phy_device *phydev)
>  		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;
> +		else
> +			rgmii_delay &= ~DP83822_TX_CLK_SHIFT;

Further cleanup is possible here:

                rx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0,
                                                      true);

                if (rx_int_delay <= 0)
                        rgmii_delay = 0;
                else
                        rgmii_delay = DP83822_RX_CLK_SHIFT;

At this point, rgmii_delay can only contain one of two possible values.
Zero, and bit 12 set.

The the code above modifies this value by either setting bit 11, or
clearing the already guaranteed to be clear bit 11. So, the only thing
that has any effect is setting bit 12, so we can omit the code path that
clears bit 11. Therefore, this can become:

		// TX_CLK_SHIFT disables the delay
                if (tx_int_delay <= 0)
                        rgmii_delay |= DP83822_TX_CLK_SHIFT;

I would also add a comment that RX_CLK_SHIFT enables the delay.

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