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

[Matthias Schiffer]

On Mon, 2025-04-14 at 16:19 +0200, Matthias Schiffer wrote:
> 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

Ah, I just realized that this is not entirely accurate. The condition did not
always evaluate to false, it just incorrectly evaluated to false for rgmii-txid.
Another thing to fix in v2.


> > - 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@xxxxxxxxxxxxxxx>
> > ---
> >  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/