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

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

[Matthias Schiffer]

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

[PATCH net-next 2/2] net: phy: dp83867: use 2ns delay if not specified in DTB

[Matthias Schiffer]

Most PHY drivers default to a 2ns delay if internal delay is requested
and no value is specified. Having a default value makes sense, as it
allows a Device Tree to only care about board design (whether there are
delays on the PCB or not), and not whether the delay is added on the MAC
or the PHY side when needed.

Whether the delays are actually applied is controlled by the
DP83867_RGMII_*_CLK_DELAY_EN flags, so the behavior is only changed in
configurations that would previously be rejected with -EINVAL.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/net/phy/dp83867.c | 44 ++++-----------------------------------
 1 file changed, 4 insertions(+), 40 deletions(-)

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index e5b0c1b7be13f..deeefb9625664 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -106,10 +106,8 @@
 /* RGMIIDCTL bits */
 #define DP83867_RGMII_TX_CLK_DELAY_MAX		0xf
 #define DP83867_RGMII_TX_CLK_DELAY_SHIFT	4
-#define DP83867_RGMII_TX_CLK_DELAY_INV	(DP83867_RGMII_TX_CLK_DELAY_MAX + 1)
 #define DP83867_RGMII_RX_CLK_DELAY_MAX		0xf
 #define DP83867_RGMII_RX_CLK_DELAY_SHIFT	0
-#define DP83867_RGMII_RX_CLK_DELAY_INV	(DP83867_RGMII_RX_CLK_DELAY_MAX + 1)
 
 /* IO_MUX_CFG bits */
 #define DP83867_IO_MUX_CFG_IO_IMPEDANCE_MASK	0x1f
@@ -501,29 +499,6 @@ static int dp83867_config_port_mirroring(struct phy_device *phydev)
 	return 0;
 }
 
-static int dp83867_verify_rgmii_cfg(struct phy_device *phydev)
-{
-	struct dp83867_private *dp83867 = phydev->priv;
-
-	/* 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) &&
-	     dp83867->rx_id_delay == DP83867_RGMII_RX_CLK_DELAY_INV) {
-		phydev_err(phydev, "ti,rx-internal-delay must be specified\n");
-		return -EINVAL;
-	}
-
-	/* TX delay *must* be specified if internal delay of TX is used. */
-	if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
-	     phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) &&
-	     dp83867->tx_id_delay == DP83867_RGMII_TX_CLK_DELAY_INV) {
-		phydev_err(phydev, "ti,tx-internal-delay must be specified\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 #if IS_ENABLED(CONFIG_OF_MDIO)
 static int dp83867_of_init_io_impedance(struct phy_device *phydev)
 {
@@ -607,7 +582,7 @@ static int dp83867_of_init(struct phy_device *phydev)
 	dp83867->sgmii_ref_clk_en = of_property_read_bool(of_node,
 							  "ti,sgmii-ref-clock-output-enable");
 
-	dp83867->rx_id_delay = DP83867_RGMII_RX_CLK_DELAY_INV;
+	dp83867->rx_id_delay = DP83867_RGMIIDCTL_2_00_NS;
 	ret = of_property_read_u32(of_node, "ti,rx-internal-delay",
 				   &dp83867->rx_id_delay);
 	if (!ret && dp83867->rx_id_delay > DP83867_RGMII_RX_CLK_DELAY_MAX) {
@@ -617,7 +592,7 @@ static int dp83867_of_init(struct phy_device *phydev)
 		return -EINVAL;
 	}
 
-	dp83867->tx_id_delay = DP83867_RGMII_TX_CLK_DELAY_INV;
+	dp83867->tx_id_delay = DP83867_RGMIIDCTL_2_00_NS;
 	ret = of_property_read_u32(of_node, "ti,tx-internal-delay",
 				   &dp83867->tx_id_delay);
 	if (!ret && dp83867->tx_id_delay > DP83867_RGMII_TX_CLK_DELAY_MAX) {
@@ -737,7 +712,6 @@ static int dp83867_config_init(struct phy_device *phydev)
 {
 	struct dp83867_private *dp83867 = phydev->priv;
 	int ret, val, bs;
-	u16 delay;
 
 	/* Force speed optimization for the PHY even if it strapped */
 	ret = phy_modify(phydev, DP83867_CFG2, DP83867_DOWNSHIFT_EN,
@@ -745,10 +719,6 @@ static int dp83867_config_init(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	ret = dp83867_verify_rgmii_cfg(phydev);
-	if (ret)
-		return ret;
-
 	/* RX_DV/RX_CTRL strapped in mode 1 or mode 2 workaround */
 	if (dp83867->rxctrl_strap_quirk)
 		phy_clear_bits_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4,
@@ -827,15 +797,9 @@ static int dp83867_config_init(struct phy_device *phydev)
 
 		phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_RGMIICTL, val);
 
-		delay = 0;
-		if (dp83867->rx_id_delay != DP83867_RGMII_RX_CLK_DELAY_INV)
-			delay |= dp83867->rx_id_delay;
-		if (dp83867->tx_id_delay != DP83867_RGMII_TX_CLK_DELAY_INV)
-			delay |= dp83867->tx_id_delay <<
-				 DP83867_RGMII_TX_CLK_DELAY_SHIFT;
-
 		phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_RGMIIDCTL,
-			      delay);
+			      dp83867->rx_id_delay |
+			      (dp83867->tx_id_delay << DP83867_RGMII_TX_CLK_DELAY_SHIFT));
 	}
 
 	/* If specified, set io impedance */
-- 
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/

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

[Matthias Schiffer]

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/

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