diff mbox series

[RFC,net-next,v2,3/3] net: dsa: microchip: implement PHY loopback configuration for KSZ8794 and KSZ8873

Message ID 20240109095753.1872010-4-o.rempel@pengutronix.de
State New
Headers
Series net: dsa: microchip: implement PHY loopback |

Commit Message

Oleksij Rempel Jan. 9, 2024, 9:57 a.m. UTC 
  Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and
ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the ksz8795
driver. Previously, the code erroneously used Bit 7 of port register 0xD
for both chip variants, which is actually for LED configuration. This
update ensures the correct registers and bits are used for the PHY
loopback feature:

- For KSZ8794: Use 0xF / Bit 7.
- For KSZ8873: Use 0xD / Bit 0.

The lack of loopback support was seen on KSZ8873 system by using
"ethtool -t lanX". After this patch, the ethtool selftest will work,
but only if port is not part of a bridge.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c     | 36 ++++++++++++++++++++-----
 drivers/net/dsa/microchip/ksz8795_reg.h |  1 +
 2 files changed, 30 insertions(+), 7 deletions(-)

Comments

Arun Ramadoss Jan. 9, 2024, 2:33 p.m. UTC | #1 
On Tue, 2024-01-09 at 10:57 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Correct the PHY loopback bit handling in the ksz8_w_phy_bmcr and
> ksz8_r_phy_bmcr functions for KSZ8794 and KSZ8873 variants in the
> ksz8795
> driver. Previously, the code erroneously used Bit 7 of port register
> 0xD
> for both chip variants, which is actually for LED configuration. This
> update ensures the correct registers and bits are used for the PHY
> loopback feature:
> 
> - For KSZ8794: Use 0xF / Bit 7.
> - For KSZ8873: Use 0xD / Bit 0.
> 
> The lack of loopback support was seen on KSZ8873 system by using
> "ethtool -t lanX". After this patch, the ethtool selftest will work,
> but only if port is not part of a bridge.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz8795.c     | 36 ++++++++++++++++++++---
> --
>  drivers/net/dsa/microchip/ksz8795_reg.h |  1 +
>  2 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> index 51e0194453df..1d8377640a3d 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -731,16 +731,25 @@ static int ksz8_r_phy_bmcr(struct ksz_device
> *dev, u16 port, u16 *val)
>         if (ret)
>                 return ret;
> 
> -       if (restart & PORT_PHY_LOOPBACK)
> -               *val |= BMCR_LOOPBACK;
> -
>         if (ctrl & PORT_FORCE_100_MBIT)
>                 *val |= BMCR_SPEED100;
> 
>         if (ksz_is_ksz88x3(dev)) {
> +               if (restart & KSZ8873_PORT_PHY_LOOPBACK)
> +                       *val |= BMCR_LOOPBACK;
> +
>                 if ((ctrl & PORT_AUTO_NEG_ENABLE))
>                         *val |= BMCR_ANENABLE;
>         } else {
> +               u8 stat3;
> +
> +               ret = ksz_pread8(dev, port, REG_PORT_STATUS_3,
> &stat3);
> +               if (ret)
> +                       return ret;
> +
> +               if (stat3 & PORT_PHY_LOOPBACK)
> +                       *val |= BMCR_LOOPBACK;
> +
>                 if (!(ctrl & PORT_AUTO_NEG_DISABLE))
>                         *val |= BMCR_ANENABLE;
>         }
> @@ -1001,8 +1010,7 @@ static int ksz8_w_phy_bmcr(struct ksz_device
> *dev, u16 port, u16 val)
> 
>         restart = 0;
>         restart_mask = PORT_LED_OFF | PORT_TX_DISABLE |
> PORT_AUTO_NEG_RESTART |
> -               PORT_POWER_DOWN | PORT_AUTO_MDIX_DISABLE |
> PORT_FORCE_MDIX |
> -               PORT_PHY_LOOPBACK;
> +               PORT_POWER_DOWN | PORT_AUTO_MDIX_DISABLE |
> PORT_FORCE_MDIX;
> 
>         if (val & KSZ886X_BMCR_DISABLE_LED)
>                 restart |= PORT_LED_OFF;
> @@ -1022,8 +1030,22 @@ static int ksz8_w_phy_bmcr(struct ksz_device
> *dev, u16 port, u16 val)
>         if (val & KSZ886X_BMCR_FORCE_MDI)
>                 restart |= PORT_FORCE_MDIX;
> 
> -       if (val & BMCR_LOOPBACK)
> -               restart |= PORT_PHY_LOOPBACK;
> +       if (ksz_is_ksz88x3(dev)) {
> +               restart_mask |= KSZ8873_PORT_PHY_LOOPBACK;
> +
> +               if (val & BMCR_LOOPBACK)
> +                       restart |= KSZ8873_PORT_PHY_LOOPBACK;
> +       } else {
> +               u8 stat3 = 0;
> +
> +               if (val & BMCR_LOOPBACK)
> +                       stat3 |= PORT_PHY_LOOPBACK;
> +
> +               ret = ksz_prmw8(dev, port, REG_PORT_STATUS_3,
> PORT_PHY_LOOPBACK,
> +                               stat3);
> +               if (ret)
> +                       return ret;

nitpick: As it is independent thing, it will be good to have wrapper
function for writing and reading phy loopback in stat3 register. 

> +       }
> 
>         return ksz_prmw8(dev, port, regs[P_NEG_RESTART_CTRL],
> restart_mask,
>                          restart);
> diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h
> b/drivers/net/dsa/microchip/ksz8795_reg.h
> index beca974e0171..7c9341ef73b0 100644
> --- a/drivers/net/dsa/microchip/ksz8795_reg.h
> +++ b/drivers/net/dsa/microchip/ksz8795_reg.h
> @@ -265,6 +265,7 @@
>  #define PORT_AUTO_MDIX_DISABLE         BIT(2)
>  #define PORT_FORCE_MDIX                        BIT(1)
>  #define PORT_MAC_LOOPBACK              BIT(0)
> +#define KSZ8873_PORT_PHY_LOOPBACK      BIT(0)
> 
>  #define REG_PORT_1_STATUS_2            0x1E
>  #define REG_PORT_2_STATUS_2            0x2E
> --
> 2.39.2
>
diff mbox series

Patch

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 51e0194453df..1d8377640a3d 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -731,16 +731,25 @@  static int ksz8_r_phy_bmcr(struct ksz_device *dev, u16 port, u16 *val)
 	if (ret)
 		return ret;
 
-	if (restart & PORT_PHY_LOOPBACK)
-		*val |= BMCR_LOOPBACK;
-
 	if (ctrl & PORT_FORCE_100_MBIT)
 		*val |= BMCR_SPEED100;
 
 	if (ksz_is_ksz88x3(dev)) {
+		if (restart & KSZ8873_PORT_PHY_LOOPBACK)
+			*val |= BMCR_LOOPBACK;
+
 		if ((ctrl & PORT_AUTO_NEG_ENABLE))
 			*val |= BMCR_ANENABLE;
 	} else {
+		u8 stat3;
+
+		ret = ksz_pread8(dev, port, REG_PORT_STATUS_3, &stat3);
+		if (ret)
+			return ret;
+
+		if (stat3 & PORT_PHY_LOOPBACK)
+			*val |= BMCR_LOOPBACK;
+
 		if (!(ctrl & PORT_AUTO_NEG_DISABLE))
 			*val |= BMCR_ANENABLE;
 	}
@@ -1001,8 +1010,7 @@  static int ksz8_w_phy_bmcr(struct ksz_device *dev, u16 port, u16 val)
 
 	restart = 0;
 	restart_mask = PORT_LED_OFF | PORT_TX_DISABLE | PORT_AUTO_NEG_RESTART |
-		PORT_POWER_DOWN | PORT_AUTO_MDIX_DISABLE | PORT_FORCE_MDIX |
-		PORT_PHY_LOOPBACK;
+		PORT_POWER_DOWN | PORT_AUTO_MDIX_DISABLE | PORT_FORCE_MDIX;
 
 	if (val & KSZ886X_BMCR_DISABLE_LED)
 		restart |= PORT_LED_OFF;
@@ -1022,8 +1030,22 @@  static int ksz8_w_phy_bmcr(struct ksz_device *dev, u16 port, u16 val)
 	if (val & KSZ886X_BMCR_FORCE_MDI)
 		restart |= PORT_FORCE_MDIX;
 
-	if (val & BMCR_LOOPBACK)
-		restart |= PORT_PHY_LOOPBACK;
+	if (ksz_is_ksz88x3(dev)) {
+		restart_mask |= KSZ8873_PORT_PHY_LOOPBACK;
+
+		if (val & BMCR_LOOPBACK)
+			restart |= KSZ8873_PORT_PHY_LOOPBACK;
+	} else {
+		u8 stat3 = 0;
+
+		if (val & BMCR_LOOPBACK)
+			stat3 |= PORT_PHY_LOOPBACK;
+
+		ret = ksz_prmw8(dev, port, REG_PORT_STATUS_3, PORT_PHY_LOOPBACK,
+				stat3);
+		if (ret)
+			return ret;
+	}
 
 	return ksz_prmw8(dev, port, regs[P_NEG_RESTART_CTRL], restart_mask,
 			 restart);
diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h
index beca974e0171..7c9341ef73b0 100644
--- a/drivers/net/dsa/microchip/ksz8795_reg.h
+++ b/drivers/net/dsa/microchip/ksz8795_reg.h
@@ -265,6 +265,7 @@ 
 #define PORT_AUTO_MDIX_DISABLE		BIT(2)
 #define PORT_FORCE_MDIX			BIT(1)
 #define PORT_MAC_LOOPBACK		BIT(0)
+#define KSZ8873_PORT_PHY_LOOPBACK	BIT(0)
 
 #define REG_PORT_1_STATUS_2		0x1E
 #define REG_PORT_2_STATUS_2		0x2E