Re: [PATCH net-next 1/2] net: phy: dp83867: remove check of delay strap configuration

[Matthias Schiffer <matthias.schiffer@ew.tq-group.com>]

On Mon, 2025-04-14 at 16:13 +0200, Matthias Schiffer wrote:
> The check that intended to handle "rgmii" PHY mode differently from the
> other RGMII modes never worked as intended:
> 
> - added in commit 2a10154abcb7 ("net: phy: dp83867: Add TI dp83867 phy"):
>   logic error caused the condition to always evaluate to true
> - changed in commit a46fa260f6f5 ("net: phy: dp83867: Fix warning check
>   for setting the internal delay"): now the condition always evaluates
>   to false
> - removed in commit 2b892649254f ("net: phy: dp83867: Set up RGMII TX
>   delay")
> 
> Around the time of the removal, commit c11669a2757e ("net: phy: dp83867:
> Rework delay rgmii delay handling") started clearing the delay enable
> flags in RGMIICTL (or it would have, if the condition ever evaluated to
> true at that time). The change attempted to preserve the historical
> behavior of not disabling internal delays with "rgmii" PHY mode and also
> documented this in a comment, but due to a conflict between "Set up
> RGMII TX delay" and "Rework delay rgmii delay handling", the behavior
> dp83867_verify_rgmii_cfg() warned about (and that was also described in
> a commit in dp83867_config_init()) disappeared in the following merge

Ugh, of course I find a mistake in the commit message right after submitting the
patch - this should read "a comment in ...". I'm going to wait for review and
then fix this in v2.


> of net into net-next in commit b4b12b0d2f02
> ("Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net").
> 
> While is doesn't appear that this breaking change was intentional, it
> has been like this since 2019, and the new behavior to disable the delays
> with "rgmii" PHY mode is generally desirable - in particular with MAC
> drivers that have to fix up the delay mode, resulting in the PHY driver
> not even seeing the same mode that was specified in the Device Tree.
> 
> Remove the obsolete check and comment.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/net/phy/dp83867.c | 32 +-------------------------------
>  1 file changed, 1 insertion(+), 31 deletions(-)
> 
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index 063266cafe9c7..e5b0c1b7be13f 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -92,11 +92,6 @@
>  #define DP83867_STRAP_STS1_RESERVED		BIT(11)
>  
>  /* STRAP_STS2 bits */
> -#define DP83867_STRAP_STS2_CLK_SKEW_TX_MASK	GENMASK(6, 4)
> -#define DP83867_STRAP_STS2_CLK_SKEW_TX_SHIFT	4
> -#define DP83867_STRAP_STS2_CLK_SKEW_RX_MASK	GENMASK(2, 0)
> -#define DP83867_STRAP_STS2_CLK_SKEW_RX_SHIFT	0
> -#define DP83867_STRAP_STS2_CLK_SKEW_NONE	BIT(2)
>  #define DP83867_STRAP_STS2_STRAP_FLD		BIT(10)
>  
>  /* PHY CTRL bits */
> @@ -510,25 +505,6 @@ static int dp83867_verify_rgmii_cfg(struct phy_device *phydev)
>  {
>  	struct dp83867_private *dp83867 = phydev->priv;
>  
> -	/* Existing behavior was to use default pin strapping delay in rgmii
> -	 * mode, but rgmii should have meant no delay.  Warn existing users.
> -	 */
> -	if (phydev->interface == PHY_INTERFACE_MODE_RGMII) {
> -		const u16 val = phy_read_mmd(phydev, DP83867_DEVADDR,
> -					     DP83867_STRAP_STS2);
> -		const u16 txskew = (val & DP83867_STRAP_STS2_CLK_SKEW_TX_MASK) >>
> -				   DP83867_STRAP_STS2_CLK_SKEW_TX_SHIFT;
> -		const u16 rxskew = (val & DP83867_STRAP_STS2_CLK_SKEW_RX_MASK) >>
> -				   DP83867_STRAP_STS2_CLK_SKEW_RX_SHIFT;
> -
> -		if (txskew != DP83867_STRAP_STS2_CLK_SKEW_NONE ||
> -		    rxskew != DP83867_STRAP_STS2_CLK_SKEW_NONE)
> -			phydev_warn(phydev,
> -				    "PHY has delays via pin strapping, but phy-mode = 'rgmii'\n"
> -				    "Should be 'rgmii-id' to use internal delays txskew:%x rxskew:%x\n",
> -				    txskew, rxskew);
> -	}
> -
>  	/* RX delay *must* be specified if internal delay of RX is used. */
>  	if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
>  	     phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) &&
> @@ -836,13 +812,7 @@ static int dp83867_config_init(struct phy_device *phydev)
>  		if (ret)
>  			return ret;
>  
> -		/* If rgmii mode with no internal delay is selected, we do NOT use
> -		 * aligned mode as one might expect.  Instead we use the PHY's default
> -		 * based on pin strapping.  And the "mode 0" default is to *use*
> -		 * internal delay with a value of 7 (2.00 ns).
> -		 *
> -		 * Set up RGMII delays
> -		 */
> +		/* Set up RGMII delays */
>  		val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_RGMIICTL);
>  
>  		val &= ~(DP83867_RGMII_TX_CLK_DELAY_EN | DP83867_RGMII_RX_CLK_DELAY_EN);

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/