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

[Arun.Ramadoss]

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