diff mbox series

[net-next,v1,02/21] net: usb: lan78xx: Remove KSZ9031 PHY fixup

Message ID 20241203072154.2440034-3-o.rempel@pengutronix.de (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series lan78xx: Preparations for PHYlink | expand

Checks

Context Check Description
netdev/series_format fail Series longer than 15 patches
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 3 this patch: 4
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: linux-usb@vger.kernel.org
netdev/build_clang fail Errors and warnings before: 3 this patch: 5
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 306 this patch: 307
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 68 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Oleksij Rempel Dec. 3, 2024, 7:21 a.m. UTC
Remove the KSZ9031RNX PHY fixup from the lan78xx driver. The fixup applied
specific RGMII pad skew configurations globally, but these settings violate the
RGMII specification and cause more harm than benefit.

Key issues with the fixup:
1. **Non-Compliant Timing**: The fixup's delay settings fall outside the RGMII
   specification requirements of 1.5 ns to 2.0 ns:
   - RX Path: Total delay of **2.16 ns** (PHY internal delay of 1.2 ns + 0.96
     ns skew).
   - TX Path: Total delay of **0.96 ns**, significantly below the RGMII minimum
     of 1.5 ns.

2. **Redundant or Incorrect Configurations**:
   - The RGMII skew registers written by the fixup do not meaningfully alter
     the PHY's default behavior and fail to account for its internal delays.
   - The TX_DATA pad skew was not configured, relying on power-on defaults
     that are insufficient for RGMII compliance.

3. **Micrel Driver Support**: By setting `PHY_INTERFACE_MODE_RGMII_ID`, the
   Micrel driver can calculate and assign appropriate skew values for the
   KSZ9031 PHY.  This ensures better timing configurations without relying on
   external fixups.

4. **System Interference**: The fixup applied globally, reconfiguring all
   KSZ9031 PHYs in the system, even those unrelated to the LAN78xx adapter.
   This could lead to unintended and harmful behavior on unrelated interfaces.

While the fixup is removed, a better mechanism is still needed to dynamically
determine the optimal combination of PHY and MAC delays to fully meet RGMII
requirements without relying on Device Tree or global fixups. This would allow
for robust operation across different hardware configurations.

The Micrel driver is capable of using the interface mode value to calculate and
apply better skew values, providing a configuration much closer to the RGMII
specification than the fixup. Removing the fixup ensures better default
behavior and prevents harm to other system interfaces.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/usb/lan78xx.c | 38 +++++---------------------------------
 1 file changed, 5 insertions(+), 33 deletions(-)

Comments

Andrew Lunn Dec. 3, 2024, 8:29 p.m. UTC | #1
On Tue, Dec 03, 2024 at 08:21:35AM +0100, Oleksij Rempel wrote:
> Remove the KSZ9031RNX PHY fixup from the lan78xx driver. The fixup applied
> specific RGMII pad skew configurations globally, but these settings violate the
> RGMII specification and cause more harm than benefit.
> 
> Key issues with the fixup:
> 1. **Non-Compliant Timing**: The fixup's delay settings fall outside the RGMII
>    specification requirements of 1.5 ns to 2.0 ns:
>    - RX Path: Total delay of **2.16 ns** (PHY internal delay of 1.2 ns + 0.96
>      ns skew).
>    - TX Path: Total delay of **0.96 ns**, significantly below the RGMII minimum
>      of 1.5 ns.
> 
> 2. **Redundant or Incorrect Configurations**:
>    - The RGMII skew registers written by the fixup do not meaningfully alter
>      the PHY's default behavior and fail to account for its internal delays.
>    - The TX_DATA pad skew was not configured, relying on power-on defaults
>      that are insufficient for RGMII compliance.
> 
> 3. **Micrel Driver Support**: By setting `PHY_INTERFACE_MODE_RGMII_ID`, the
>    Micrel driver can calculate and assign appropriate skew values for the
>    KSZ9031 PHY.  This ensures better timing configurations without relying on
>    external fixups.
> 
> 4. **System Interference**: The fixup applied globally, reconfiguring all
>    KSZ9031 PHYs in the system, even those unrelated to the LAN78xx adapter.
>    This could lead to unintended and harmful behavior on unrelated interfaces.
> 
> While the fixup is removed, a better mechanism is still needed to dynamically
> determine the optimal combination of PHY and MAC delays to fully meet RGMII
> requirements without relying on Device Tree or global fixups. This would allow
> for robust operation across different hardware configurations.
> 
> The Micrel driver is capable of using the interface mode value to calculate and
> apply better skew values, providing a configuration much closer to the RGMII
> specification than the fixup. Removing the fixup ensures better default
> behavior and prevents harm to other system interfaces.

PHY_INTERFACE_MODE_RGMII_ID should be good enough for most
hardware. It seems like USB dongle developers don't really understand
phylib.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Simon Horman Dec. 5, 2024, 5:12 p.m. UTC | #2
On Tue, Dec 03, 2024 at 08:21:35AM +0100, Oleksij Rempel wrote:
> Remove the KSZ9031RNX PHY fixup from the lan78xx driver. The fixup applied
> specific RGMII pad skew configurations globally, but these settings violate the
> RGMII specification and cause more harm than benefit.
> 
> Key issues with the fixup:
> 1. **Non-Compliant Timing**: The fixup's delay settings fall outside the RGMII
>    specification requirements of 1.5 ns to 2.0 ns:
>    - RX Path: Total delay of **2.16 ns** (PHY internal delay of 1.2 ns + 0.96
>      ns skew).
>    - TX Path: Total delay of **0.96 ns**, significantly below the RGMII minimum
>      of 1.5 ns.
> 
> 2. **Redundant or Incorrect Configurations**:
>    - The RGMII skew registers written by the fixup do not meaningfully alter
>      the PHY's default behavior and fail to account for its internal delays.
>    - The TX_DATA pad skew was not configured, relying on power-on defaults
>      that are insufficient for RGMII compliance.
> 
> 3. **Micrel Driver Support**: By setting `PHY_INTERFACE_MODE_RGMII_ID`, the
>    Micrel driver can calculate and assign appropriate skew values for the
>    KSZ9031 PHY.  This ensures better timing configurations without relying on
>    external fixups.
> 
> 4. **System Interference**: The fixup applied globally, reconfiguring all
>    KSZ9031 PHYs in the system, even those unrelated to the LAN78xx adapter.
>    This could lead to unintended and harmful behavior on unrelated interfaces.
> 
> While the fixup is removed, a better mechanism is still needed to dynamically
> determine the optimal combination of PHY and MAC delays to fully meet RGMII
> requirements without relying on Device Tree or global fixups. This would allow
> for robust operation across different hardware configurations.
> 
> The Micrel driver is capable of using the interface mode value to calculate and
> apply better skew values, providing a configuration much closer to the RGMII
> specification than the fixup. Removing the fixup ensures better default
> behavior and prevents harm to other system interfaces.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/usb/lan78xx.c | 38 +++++---------------------------------
>  1 file changed, 5 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c

...

> @@ -2283,14 +2263,11 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
>  			netdev_err(dev->net, "no PHY driver found\n");
>  			return NULL;
>  		}
> -		dev->interface = PHY_INTERFACE_MODE_RGMII;
> -		/* external PHY fixup for KSZ9031RNX */
> -		ret = phy_register_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0,
> -						 ksz9031rnx_fixup);
> -		if (ret < 0) {
> -			netdev_err(dev->net, "Failed to register fixup for PHY_KSZ9031RNX\n");
> -			return NULL;
> -		}
> +		dev->interface = PHY_INTERFACE_MODE_RGMII_ID;
> +		/* The PHY driver is responsible to configure proper RGMII
> +		 * interface delays. Disable RGMII delays on MAC side.
> +		 */
> +		lan78xx_write_reg(dev, MAC_RGMII_ID, 0);
>  
>  		phydev->is_internal = false;
>  	}

nit: With this change ret is now set but otherwise unused in this function.

     I would suggest a new patch, prior to this one, that drops the
     other places where it is set. They seem to have no effect.
     And then remove ret from this function in this patch.

> @@ -2349,9 +2326,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  			if (phy_is_pseudo_fixed_link(phydev)) {
>  				fixed_phy_unregister(phydev);
>  				phy_device_free(phydev);
> -			} else {
> -				phy_unregister_fixup_for_uid(PHY_KSZ9031RNX,
> -							     0xfffffff0);
>  			}
>  		}
>  		return -EIO;

...
Oleksij Rempel Dec. 6, 2024, 8:56 a.m. UTC | #3
Hi Simon,

On Thu, Dec 05, 2024 at 05:12:03PM +0000, Simon Horman wrote:
> > -		dev->interface = PHY_INTERFACE_MODE_RGMII;
> > -		/* external PHY fixup for KSZ9031RNX */
> > -		ret = phy_register_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0,
> > -						 ksz9031rnx_fixup);
> > -		if (ret < 0) {
> > -			netdev_err(dev->net, "Failed to register fixup for PHY_KSZ9031RNX\n");
> > -			return NULL;
> > -		}
> > +		dev->interface = PHY_INTERFACE_MODE_RGMII_ID;
> > +		/* The PHY driver is responsible to configure proper RGMII
> > +		 * interface delays. Disable RGMII delays on MAC side.
> > +		 */
> > +		lan78xx_write_reg(dev, MAC_RGMII_ID, 0);
> >  
> >  		phydev->is_internal = false;
> >  	}
> 
> nit: With this change ret is now set but otherwise unused in this function.
> 
>      I would suggest a new patch, prior to this one, that drops the
>      other places where it is set. They seem to have no effect.
>      And then remove ret from this function in this patch.
> 

There is a patch in this patch stack which will address it too. There is
just limit to amount of patches I can upstream per one round.
diff mbox series

Patch

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 6e468e77d796..918b88bd9524 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -472,9 +472,6 @@  struct lan78xx_net {
 	struct irq_domain_data	domain_data;
 };
 
-/* define external phy id */
-#define	PHY_KSZ9031RNX			(0x00221620)
-
 /* use ethtool to change the level for any given device */
 static int msg_level = -1;
 module_param(msg_level, int, 0);
@@ -2233,23 +2230,6 @@  static void lan78xx_remove_irq_domain(struct lan78xx_net *dev)
 	dev->domain_data.irqdomain = NULL;
 }
 
-static int ksz9031rnx_fixup(struct phy_device *phydev)
-{
-	struct lan78xx_net *dev = netdev_priv(phydev->attached_dev);
-
-	/* Micrel9301RNX PHY configuration */
-	/* RGMII Control Signal Pad Skew */
-	phy_write_mmd(phydev, MDIO_MMD_WIS, 4, 0x0077);
-	/* RGMII RX Data Pad Skew */
-	phy_write_mmd(phydev, MDIO_MMD_WIS, 5, 0x7777);
-	/* RGMII RX Clock Pad Skew */
-	phy_write_mmd(phydev, MDIO_MMD_WIS, 8, 0x1FF);
-
-	dev->interface = PHY_INTERFACE_MODE_RGMII_RXID;
-
-	return 1;
-}
-
 static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
 {
 	u32 buf;
@@ -2283,14 +2263,11 @@  static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
 			netdev_err(dev->net, "no PHY driver found\n");
 			return NULL;
 		}
-		dev->interface = PHY_INTERFACE_MODE_RGMII;
-		/* external PHY fixup for KSZ9031RNX */
-		ret = phy_register_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0,
-						 ksz9031rnx_fixup);
-		if (ret < 0) {
-			netdev_err(dev->net, "Failed to register fixup for PHY_KSZ9031RNX\n");
-			return NULL;
-		}
+		dev->interface = PHY_INTERFACE_MODE_RGMII_ID;
+		/* The PHY driver is responsible to configure proper RGMII
+		 * interface delays. Disable RGMII delays on MAC side.
+		 */
+		lan78xx_write_reg(dev, MAC_RGMII_ID, 0);
 
 		phydev->is_internal = false;
 	}
@@ -2349,9 +2326,6 @@  static int lan78xx_phy_init(struct lan78xx_net *dev)
 			if (phy_is_pseudo_fixed_link(phydev)) {
 				fixed_phy_unregister(phydev);
 				phy_device_free(phydev);
-			} else {
-				phy_unregister_fixup_for_uid(PHY_KSZ9031RNX,
-							     0xfffffff0);
 			}
 		}
 		return -EIO;
@@ -4208,8 +4182,6 @@  static void lan78xx_disconnect(struct usb_interface *intf)
 
 	phydev = net->phydev;
 
-	phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);
-
 	phy_disconnect(net->phydev);
 
 	if (phy_is_pseudo_fixed_link(phydev)) {