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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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/
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 <[email protected]>
> > ---
> > 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 |
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/[email protected]/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231011/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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 <[email protected]>
> ---
> 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)
...
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 <[email protected]>
> ---
> 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.
> + */
...
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!