* [PATCH net-next v1 1/3] net: phy: micrel: Extend KSZ886X PHY Special Ctrl/Status Reg definitions @ 2023-10-11 12:38 Oleksij Rempel 2023-10-11 12:38 ` [PATCH net-next v1 2/3] net: dsa: microchip: ksz8: Enable MIIM PHY Control reg access Oleksij Rempel 2023-10-11 12:38 ` [PATCH net-next v1 3/3] net: phy: micrel: Fix forced link mode for KSZ886X switches Oleksij Rempel 0 siblings, 2 replies; 9+ messages in thread From: Oleksij Rempel @ 2023-10-11 12:38 UTC (permalink / raw) To: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Heiner Kallweit Cc: Oleksij Rempel, kernel, linux-kernel, Russell King, netdev Extend 'micrel_phy.h' with additional definitions for KSZ886X PHY Special Control/Status Register (Reg 31), for upcoming usage in subsequent patches. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- include/linux/micrel_phy.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h index 4e27ca7c49de..591bf5b5e8dc 100644 --- a/include/linux/micrel_phy.h +++ b/include/linux/micrel_phy.h @@ -64,6 +64,10 @@ #define KSZ886X_BMCR_DISABLE_TRANSMIT BIT(1) #define KSZ886X_BMCR_DISABLE_LED BIT(0) +/* PHY Special Control/Status Register (Reg 31) */ #define KSZ886X_CTRL_MDIX_STAT BIT(4) +#define KSZ886X_CTRL_FORCE_LINK BIT(3) +#define KSZ886X_CTRL_PWRSAVE BIT(2) +#define KSZ886X_CTRL_REMOTE_LOOPBACK BIT(1) #endif /* _MICREL_PHY_H */ -- 2.39.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net-next v1 2/3] net: dsa: microchip: ksz8: Enable MIIM PHY Control reg access 2023-10-11 12:38 [PATCH net-next v1 1/3] net: phy: micrel: Extend KSZ886X PHY Special Ctrl/Status Reg definitions Oleksij Rempel @ 2023-10-11 12:38 ` Oleksij Rempel 2023-10-11 14:35 ` kernel test robot 2023-10-13 15:47 ` Simon Horman 2023-10-11 12:38 ` [PATCH net-next v1 3/3] net: phy: micrel: Fix forced link mode for KSZ886X switches Oleksij Rempel 1 sibling, 2 replies; 9+ messages in thread From: Oleksij Rempel @ 2023-10-11 12:38 UTC (permalink / raw) To: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Heiner Kallweit Cc: Oleksij Rempel, kernel, linux-kernel, Russell King, netdev Provide access to MIIM PHY Control register (Reg. 31) through ksz8_r_phy_ctrl() and ksz8_w_phy_ctrl() functions. Necessary for upcoming micrel.c patch to address forced link mode configuration. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/dsa/microchip/ksz8795.c | 81 +++++++++++++++++++++++++++-- 1 file changed, 78 insertions(+), 3 deletions(-) diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c index 91aba470fb2f..11cb054cb54f 100644 --- a/drivers/net/dsa/microchip/ksz8795.c +++ b/drivers/net/dsa/microchip/ksz8795.c @@ -632,6 +632,47 @@ static void ksz8_w_vlan_table(struct ksz_device *dev, u16 vid, u16 vlan) ksz8_w_table(dev, TABLE_VLAN, addr, buf); } +/** + * ksz8_r_phy_ctrl - Translates and reads from the SMI interface to a MIIM PHY + * Control register (Reg. 31). + * @dev: The KSZ device instance. + * @port: The port number to be read. + * + * This function reads the SMI interface and translates the hardware register + * bit values into their corresponding control settings for a MIIM PHY Control + * register. + */ +static int ksz8_r_phy_ctrl(struct ksz_device *dev, int port, u16 *val) +{ + const u16 *regs = dev->info->regs; + u8 reg_val; + int ret; + + *val = 0; + + ret = ksz_pread8(dev, port, regs[P_LINK_STATUS], ®_val); + if (ret < 0) + return ret; + + if (reg_val & PORT_MDIX_STATUS) + *val |= KSZ886X_CTRL_MDIX_STAT; + + ret = ksz_pread8(dev, port, REG_PORT_LINK_MD_CTRL, ®_val); + if (ret < 0) + return ret; + + if (reg_val & PORT_FORCE_LINK) + *val |= KSZ886X_CTRL_FORCE_LINK; + + if (reg_val & PORT_POWER_SAVING) + *val |= KSZ886X_CTRL_PWRSAVE; + + if (reg_val & PORT_PHY_REMOTE_LOOPBACK) + *val |= KSZ886X_CTRL_REMOTE_LOOPBACK; + + return 0; +} + int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val) { u8 restart, speed, ctrl, link; @@ -769,12 +810,10 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val) FIELD_GET(PORT_CABLE_FAULT_COUNTER_L, val2)); break; case PHY_REG_PHY_CTRL: - ret = ksz_pread8(dev, p, regs[P_LINK_STATUS], &link); + ret = ksz8_r_phy_ctrl(dev, p, &data); if (ret) return ret; - if (link & PORT_MDIX_STATUS) - data |= KSZ886X_CTRL_MDIX_STAT; break; default: processed = false; @@ -786,6 +825,36 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val) return 0; } +/** + * ksz8_w_phy_ctrl - Translates and writes to the SMI interface from a MIIM PHY + * Control register (Reg. 31). + * @dev: The KSZ device instance. + * @port: The port number to be configured. + * @val: The register value to be written. + * + * This function translates control settings from a MIIM PHY Control register + * into their corresponding hardware register bit values for the SMI + * interface. + */ +static int ksz8_w_phy_ctrl(struct ksz_device *dev, int port, u16 val) +{ + u8 reg_val = 0; + int ret; + + if (val & KSZ886X_CTRL_FORCE_LINK) + reg_val |= PORT_FORCE_LINK; + + if (val & KSZ886X_CTRL_PWRSAVE) + reg_val |= PORT_POWER_SAVING; + + if (val & KSZ886X_CTRL_REMOTE_LOOPBACK) + reg_val |= PORT_PHY_REMOTE_LOOPBACK; + + ret = ksz_prmw8(dev, port, REG_PORT_LINK_MD_CTRL, PORT_FORCE_LINK | + PORT_POWER_SAVING | PORT_PHY_REMOTE_LOOPBACK, reg_val); + return ret; +} + int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val) { u8 restart, speed, ctrl, data; @@ -926,6 +995,12 @@ int ksz8_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val) if (val & PHY_START_CABLE_DIAG) ksz_port_cfg(dev, p, REG_PORT_LINK_MD_CTRL, PORT_START_CABLE_DIAG, true); break; + + case PHY_REG_PHY_CTRL: + ret = ksz8_w_phy_ctrl(dev, p, val); + if (ret) + return ret; + break; default: break; } -- 2.39.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v1 2/3] net: dsa: microchip: ksz8: Enable MIIM PHY Control reg access 2023-10-11 12:38 ` [PATCH net-next v1 2/3] net: dsa: microchip: ksz8: Enable MIIM PHY Control reg access Oleksij Rempel @ 2023-10-11 14:35 ` kernel test robot 2023-10-13 15:47 ` Simon Horman 1 sibling, 0 replies; 9+ messages in thread From: kernel test robot @ 2023-10-11 14:35 UTC (permalink / raw) To: Oleksij Rempel, Woojung Huh, UNGLinuxDriver, Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Heiner Kallweit Cc: oe-kbuild-all, netdev, Oleksij Rempel, kernel, linux-kernel, Russell King Hi Oleksij, kernel test robot noticed the following build warnings: [auto build test WARNING on net-next/main] url: https://github.com/intel-lab-lkp/linux/commits/Oleksij-Rempel/net-dsa-microchip-ksz8-Enable-MIIM-PHY-Control-reg-access/20231011-204502 base: net-next/main patch link: https://lore.kernel.org/r/20231011123856.1443308-2-o.rempel%40pengutronix.de patch subject: [PATCH net-next v1 2/3] net: dsa: microchip: ksz8: Enable MIIM PHY Control reg access config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231011/202310112224.iYgvjBUy-lkp@intel.com/config) compiler: m68k-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231011/202310112224.iYgvjBUy-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202310112224.iYgvjBUy-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/net/dsa/microchip/ksz8795.c:646: warning: Function parameter or member 'val' not described in 'ksz8_r_phy_ctrl' vim +646 drivers/net/dsa/microchip/ksz8795.c 634 635 /** 636 * ksz8_r_phy_ctrl - Translates and reads from the SMI interface to a MIIM PHY 637 * Control register (Reg. 31). 638 * @dev: The KSZ device instance. 639 * @port: The port number to be read. 640 * 641 * This function reads the SMI interface and translates the hardware register 642 * bit values into their corresponding control settings for a MIIM PHY Control 643 * register. 644 */ 645 static int ksz8_r_phy_ctrl(struct ksz_device *dev, int port, u16 *val) > 646 { 647 const u16 *regs = dev->info->regs; 648 u8 reg_val; 649 int ret; 650 651 *val = 0; 652 653 ret = ksz_pread8(dev, port, regs[P_LINK_STATUS], ®_val); 654 if (ret < 0) 655 return ret; 656 657 if (reg_val & PORT_MDIX_STATUS) 658 *val |= KSZ886X_CTRL_MDIX_STAT; 659 660 ret = ksz_pread8(dev, port, REG_PORT_LINK_MD_CTRL, ®_val); 661 if (ret < 0) 662 return ret; 663 664 if (reg_val & PORT_FORCE_LINK) 665 *val |= KSZ886X_CTRL_FORCE_LINK; 666 667 if (reg_val & PORT_POWER_SAVING) 668 *val |= KSZ886X_CTRL_PWRSAVE; 669 670 if (reg_val & PORT_PHY_REMOTE_LOOPBACK) 671 *val |= KSZ886X_CTRL_REMOTE_LOOPBACK; 672 673 return 0; 674 } 675 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v1 2/3] net: dsa: microchip: ksz8: Enable MIIM PHY Control reg access 2023-10-11 12:38 ` [PATCH net-next v1 2/3] net: dsa: microchip: ksz8: Enable MIIM PHY Control reg access Oleksij Rempel 2023-10-11 14:35 ` kernel test robot @ 2023-10-13 15:47 ` Simon Horman 1 sibling, 0 replies; 9+ messages in thread From: Simon Horman @ 2023-10-13 15:47 UTC (permalink / raw) To: Oleksij Rempel Cc: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Heiner Kallweit, kernel, linux-kernel, Russell King, netdev On Wed, Oct 11, 2023 at 02:38:55PM +0200, Oleksij Rempel wrote: > Provide access to MIIM PHY Control register (Reg. 31) through > ksz8_r_phy_ctrl() and ksz8_w_phy_ctrl() functions. Necessary for > upcoming micrel.c patch to address forced link mode configuration. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/dsa/microchip/ksz8795.c | 81 +++++++++++++++++++++++++++-- > 1 file changed, 78 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c > index 91aba470fb2f..11cb054cb54f 100644 > --- a/drivers/net/dsa/microchip/ksz8795.c > +++ b/drivers/net/dsa/microchip/ksz8795.c > @@ -632,6 +632,47 @@ static void ksz8_w_vlan_table(struct ksz_device *dev, u16 vid, u16 vlan) > ksz8_w_table(dev, TABLE_VLAN, addr, buf); > } > > +/** > + * ksz8_r_phy_ctrl - Translates and reads from the SMI interface to a MIIM PHY > + * Control register (Reg. 31). > + * @dev: The KSZ device instance. > + * @port: The port number to be read. nit: please include an entry for @val here > + * > + * This function reads the SMI interface and translates the hardware register > + * bit values into their corresponding control settings for a MIIM PHY Control > + * register. > + */ > +static int ksz8_r_phy_ctrl(struct ksz_device *dev, int port, u16 *val) ... ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next v1 3/3] net: phy: micrel: Fix forced link mode for KSZ886X switches 2023-10-11 12:38 [PATCH net-next v1 1/3] net: phy: micrel: Extend KSZ886X PHY Special Ctrl/Status Reg definitions Oleksij Rempel 2023-10-11 12:38 ` [PATCH net-next v1 2/3] net: dsa: microchip: ksz8: Enable MIIM PHY Control reg access Oleksij Rempel @ 2023-10-11 12:38 ` Oleksij Rempel 2023-10-11 13:29 ` Alexander Stein 2023-10-13 15:48 ` Simon Horman 1 sibling, 2 replies; 9+ messages in thread From: Oleksij Rempel @ 2023-10-11 12:38 UTC (permalink / raw) To: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Heiner Kallweit Cc: Oleksij Rempel, kernel, linux-kernel, Russell King, netdev Address a link speed detection issue in KSZ886X PHY driver when in forced link mode. Previously, link partners like "ASIX AX88772B" with KSZ8873 could fall back to 10Mbit instead of configured 100Mbit. The issue arises as KSZ886X PHY continues sending Fast Link Pulses (FLPs) even with autonegotiation off, misleading link partners in autoneg mode, leading to incorrect link speed detection. Now, when autonegotiation is disabled, the driver sets the link state forcefully using KSZ886X_CTRL_FORCE_LINK bit. This action, beyond just disabling autonegotiation, makes the PHY state more reliably detected by link partners using parallel detection, thus fixing the link speed misconfiguration. With autonegotiation enabled, link state is not forced, allowing proper autonegotiation process participation. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/phy/micrel.c | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 927d3d54658e..12f093aed4ff 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -1729,9 +1729,35 @@ static int ksz886x_config_aneg(struct phy_device *phydev) { int ret; - ret = genphy_config_aneg(phydev); - if (ret) - return ret; + if (phydev->autoneg != AUTONEG_ENABLE) { + ret = genphy_setup_forced(phydev); + if (ret) + return ret; + + /* When autonegotation is disabled, we need to manually force + * the link state. If we don't do this, the PHY will keep + * sending Fast Link Pulses (FLPs) which are part of the + * autonegotiation process. This is not desired when + * autonegotiation is off. + */ + ret = phy_set_bits(phydev, MII_KSZPHY_CTRL, + KSZ886X_CTRL_FORCE_LINK); + if (ret) + return ret; + } else { + /* Make sure, the link state is not forced. + * Otherwise, the PHY we create a link by skipping the + * autonegotiation process. + */ + ret = phy_clear_bits(phydev, MII_KSZPHY_CTRL, + KSZ886X_CTRL_FORCE_LINK); + if (ret) + return ret; + + ret = genphy_config_aneg(phydev); + if (ret) + return ret; + } /* The MDI-X configuration is automatically changed by the PHY after * switching from autoneg off to on. So, take MDI-X configuration under -- 2.39.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v1 3/3] net: phy: micrel: Fix forced link mode for KSZ886X switches 2023-10-11 12:38 ` [PATCH net-next v1 3/3] net: phy: micrel: Fix forced link mode for KSZ886X switches Oleksij Rempel @ 2023-10-11 13:29 ` Alexander Stein 2023-10-11 14:25 ` Oleksij Rempel 2023-10-13 16:04 ` Russell King (Oracle) 2023-10-13 15:48 ` Simon Horman 1 sibling, 2 replies; 9+ messages in thread From: Alexander Stein @ 2023-10-11 13:29 UTC (permalink / raw) To: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Heiner Kallweit, Oleksij Rempel Cc: Oleksij Rempel, kernel, linux-kernel, Russell King, netdev Hi Oleksij, Am Mittwoch, 11. Oktober 2023, 14:38:56 CEST schrieb Oleksij Rempel: > Address a link speed detection issue in KSZ886X PHY driver when in > forced link mode. Previously, link partners like "ASIX AX88772B" > with KSZ8873 could fall back to 10Mbit instead of configured 100Mbit. > > The issue arises as KSZ886X PHY continues sending Fast Link Pulses (FLPs) > even with autonegotiation off, misleading link partners in autoneg mode, > leading to incorrect link speed detection. > > Now, when autonegotiation is disabled, the driver sets the link state > forcefully using KSZ886X_CTRL_FORCE_LINK bit. This action, beyond just > disabling autonegotiation, makes the PHY state more reliably detected by > link partners using parallel detection, thus fixing the link speed > misconfiguration. > > With autonegotiation enabled, link state is not forced, allowing proper > autonegotiation process participation. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/phy/micrel.c | 32 +++++++++++++++++++++++++++++--- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 927d3d54658e..12f093aed4ff 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -1729,9 +1729,35 @@ static int ksz886x_config_aneg(struct phy_device > *phydev) { > int ret; > > - ret = genphy_config_aneg(phydev); > - if (ret) > - return ret; > + if (phydev->autoneg != AUTONEG_ENABLE) { > + ret = genphy_setup_forced(phydev); > + if (ret) > + return ret; > + > + /* When autonegotation is disabled, we need to manually force > + * the link state. If we don't do this, the PHY will keep > + * sending Fast Link Pulses (FLPs) which are part of the > + * autonegotiation process. This is not desired when > + * autonegotiation is off. > + */ > + ret = phy_set_bits(phydev, MII_KSZPHY_CTRL, > + KSZ886X_CTRL_FORCE_LINK); > + if (ret) > + return ret; > + } else { > + /* Make sure, the link state is not forced. > + * Otherwise, the PHY we create a link by skipping the PHY will create? > + * autonegotiation process. > + */ > + ret = phy_clear_bits(phydev, MII_KSZPHY_CTRL, > + KSZ886X_CTRL_FORCE_LINK); > + if (ret) > + return ret; Isn't this call to phy_clear_bits() a fix for autonegotiation mode? This should be a separate patch then. Best regards, Alexander > + > + ret = genphy_config_aneg(phydev); > + if (ret) > + return ret; > + } > > /* The MDI-X configuration is automatically changed by the PHY after > * switching from autoneg off to on. So, take MDI-X configuration under -- 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 http://www.tq-group.com/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v1 3/3] net: phy: micrel: Fix forced link mode for KSZ886X switches 2023-10-11 13:29 ` Alexander Stein @ 2023-10-11 14:25 ` Oleksij Rempel 2023-10-13 16:04 ` Russell King (Oracle) 1 sibling, 0 replies; 9+ messages in thread From: Oleksij Rempel @ 2023-10-11 14:25 UTC (permalink / raw) To: Alexander Stein Cc: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Heiner Kallweit, netdev, linux-kernel, kernel, Russell King Hi Alexander, thank you for review! On Wed, Oct 11, 2023 at 03:29:49PM +0200, Alexander Stein wrote: > Hi Oleksij, > > Am Mittwoch, 11. Oktober 2023, 14:38:56 CEST schrieb Oleksij Rempel: > > Address a link speed detection issue in KSZ886X PHY driver when in > > forced link mode. Previously, link partners like "ASIX AX88772B" > > with KSZ8873 could fall back to 10Mbit instead of configured 100Mbit. > > > > The issue arises as KSZ886X PHY continues sending Fast Link Pulses (FLPs) > > even with autonegotiation off, misleading link partners in autoneg mode, > > leading to incorrect link speed detection. > > > > Now, when autonegotiation is disabled, the driver sets the link state > > forcefully using KSZ886X_CTRL_FORCE_LINK bit. This action, beyond just > > disabling autonegotiation, makes the PHY state more reliably detected by > > link partners using parallel detection, thus fixing the link speed > > misconfiguration. > > > > With autonegotiation enabled, link state is not forced, allowing proper > > autonegotiation process participation. > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > --- > > drivers/net/phy/micrel.c | 32 +++++++++++++++++++++++++++++--- > > 1 file changed, 29 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > > index 927d3d54658e..12f093aed4ff 100644 > > --- a/drivers/net/phy/micrel.c > > +++ b/drivers/net/phy/micrel.c > > @@ -1729,9 +1729,35 @@ static int ksz886x_config_aneg(struct phy_device > > *phydev) { > > int ret; > > > > - ret = genphy_config_aneg(phydev); > > - if (ret) > > - return ret; > > + if (phydev->autoneg != AUTONEG_ENABLE) { > > + ret = genphy_setup_forced(phydev); > > + if (ret) > > + return ret; > > + > > + /* When autonegotation is disabled, we need to manually > force > > + * the link state. If we don't do this, the PHY will keep > > + * sending Fast Link Pulses (FLPs) which are part of the > > + * autonegotiation process. This is not desired when > > + * autonegotiation is off. > > + */ > > + ret = phy_set_bits(phydev, MII_KSZPHY_CTRL, > > + KSZ886X_CTRL_FORCE_LINK); > > + if (ret) > > + return ret; > > + } else { > > + /* Make sure, the link state is not forced. > > + * Otherwise, the PHY we create a link by skipping the > > PHY will create? > > > + * autonegotiation process. > > + */ > > + ret = phy_clear_bits(phydev, MII_KSZPHY_CTRL, > > + KSZ886X_CTRL_FORCE_LINK); > > + if (ret) > > + return ret; > > Isn't this call to phy_clear_bits() a fix for autonegotiation mode? This > should be a separate patch then. First time this bit is set by this patch, I assume this problem would not exist without fixed link fix. Regards, Oleksij -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v1 3/3] net: phy: micrel: Fix forced link mode for KSZ886X switches 2023-10-11 13:29 ` Alexander Stein 2023-10-11 14:25 ` Oleksij Rempel @ 2023-10-13 16:04 ` Russell King (Oracle) 1 sibling, 0 replies; 9+ messages in thread From: Russell King (Oracle) @ 2023-10-13 16:04 UTC (permalink / raw) To: Alexander Stein Cc: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Heiner Kallweit, Oleksij Rempel, kernel, linux-kernel, netdev On Wed, Oct 11, 2023 at 03:29:49PM +0200, Alexander Stein wrote: > Hi Oleksij, > > Am Mittwoch, 11. Oktober 2023, 14:38:56 CEST schrieb Oleksij Rempel: > > + if (phydev->autoneg != AUTONEG_ENABLE) { > > + ret = genphy_setup_forced(phydev); > > + if (ret) > > + return ret; > > + ... > > + ret = phy_set_bits(phydev, MII_KSZPHY_CTRL, > > + KSZ886X_CTRL_FORCE_LINK); > > + if (ret) > > + return ret; > > + } else { ... > > + ret = phy_clear_bits(phydev, MII_KSZPHY_CTRL, > > + KSZ886X_CTRL_FORCE_LINK); > > + if (ret) > > + return ret; > > Isn't this call to phy_clear_bits() a fix for autonegotiation mode? This > should be a separate patch then. No, I don't think that is the case. Compare the two paths above, noting that patch 1 introduces the definition for KSZ886X_CTRL_FORCE_LINK. If autoneg is disabled, then this bit is then set, which forces the link. Clearly, if autoneg is then re-enabled, this bit has to be cleared to allow the effects of the autoneg-disabled path to be undone. So both of these, the phy_set_bits() and the phy_clear_bits() belong in the same patch. Splitting them up, so we introduce phy_set_bits() first will create a regression - which we don't want. These two belong logically together. What does concern me, however, is that the autoneg-disabled path avoids calling genphy_setup_master_slave(), and since establishing which end of the link is part of the fundamentals of a 1000base-T link, I wonder whether both paths should still call genphy_config_aneg(). Apart from that, I think the patch is otherwise fine. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next v1 3/3] net: phy: micrel: Fix forced link mode for KSZ886X switches 2023-10-11 12:38 ` [PATCH net-next v1 3/3] net: phy: micrel: Fix forced link mode for KSZ886X switches Oleksij Rempel 2023-10-11 13:29 ` Alexander Stein @ 2023-10-13 15:48 ` Simon Horman 1 sibling, 0 replies; 9+ messages in thread From: Simon Horman @ 2023-10-13 15:48 UTC (permalink / raw) To: Oleksij Rempel Cc: Woojung Huh, UNGLinuxDriver, Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Heiner Kallweit, kernel, linux-kernel, Russell King, netdev On Wed, Oct 11, 2023 at 02:38:56PM +0200, Oleksij Rempel wrote: > Address a link speed detection issue in KSZ886X PHY driver when in > forced link mode. Previously, link partners like "ASIX AX88772B" > with KSZ8873 could fall back to 10Mbit instead of configured 100Mbit. > > The issue arises as KSZ886X PHY continues sending Fast Link Pulses (FLPs) > even with autonegotiation off, misleading link partners in autoneg mode, > leading to incorrect link speed detection. > > Now, when autonegotiation is disabled, the driver sets the link state > forcefully using KSZ886X_CTRL_FORCE_LINK bit. This action, beyond just > disabling autonegotiation, makes the PHY state more reliably detected by > link partners using parallel detection, thus fixing the link speed > misconfiguration. > > With autonegotiation enabled, link state is not forced, allowing proper > autonegotiation process participation. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/phy/micrel.c | 32 +++++++++++++++++++++++++++++--- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 927d3d54658e..12f093aed4ff 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -1729,9 +1729,35 @@ static int ksz886x_config_aneg(struct phy_device *phydev) > { > int ret; > > - ret = genphy_config_aneg(phydev); > - if (ret) > - return ret; > + if (phydev->autoneg != AUTONEG_ENABLE) { > + ret = genphy_setup_forced(phydev); > + if (ret) > + return ret; > + > + /* When autonegotation is disabled, we need to manually force nit: autonegotiation > + * the link state. If we don't do this, the PHY will keep > + * sending Fast Link Pulses (FLPs) which are part of the > + * autonegotiation process. This is not desired when > + * autonegotiation is off. > + */ ... ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-10-13 16:05 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-11 12:38 [PATCH net-next v1 1/3] net: phy: micrel: Extend KSZ886X PHY Special Ctrl/Status Reg definitions Oleksij Rempel 2023-10-11 12:38 ` [PATCH net-next v1 2/3] net: dsa: microchip: ksz8: Enable MIIM PHY Control reg access Oleksij Rempel 2023-10-11 14:35 ` kernel test robot 2023-10-13 15:47 ` Simon Horman 2023-10-11 12:38 ` [PATCH net-next v1 3/3] net: phy: micrel: Fix forced link mode for KSZ886X switches Oleksij Rempel 2023-10-11 13:29 ` Alexander Stein 2023-10-11 14:25 ` Oleksij Rempel 2023-10-13 16:04 ` Russell King (Oracle) 2023-10-13 15:48 ` Simon Horman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).