Re: [PATCH net] net: phy: micrel: fix KSZ9477 PHY issues after suspend/resume

[Robert Hancock]

On Tue, 2024-05-28 at 14:37 -0700, Tristram.Ha@xxxxxxxxxxxxx wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you recognize the sender
> and know the content is safe.
> 
> From: Tristram Ha <tristram.ha@xxxxxxxxxxxxx>
> 
> When the PHY is powered up after powered down most of the registers
> are
> reset, so the PHY setup code needs to be done again.  In addition the
> interrupt register will need to be setup again so that link status
> indication works again.
> 
> Fixes: 26dd2974c5b5 ("net: phy: micrel: Move KSZ9477 errata fixes to
> PHY driver")
> Signed-off-by: Tristram Ha <tristram.ha@xxxxxxxxxxxxx>
> ---
>  drivers/net/phy/micrel.c | 60 +++++++++++++++++++++++++++++++++++---
> --
>  1 file changed, 53 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 2b8f8b7f1517..902d9a262c8a 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -1918,7 +1918,7 @@ static const struct ksz9477_errata_write
> ksz9477_errata_writes[] = {
>         {0x01, 0x75, 0x0060},
>         {0x01, 0xd3, 0x7777},
>         {0x1c, 0x06, 0x3008},
> -       {0x1c, 0x08, 0x2000},
> +       {0x1c, 0x08, 0x2001},

This change wasn't mentioned in the commit message, but I see that the
latest errata sheet says "The value of this register may read back as
either 0x2000 or 0x2001. Bit 0 is read-only, and is not a fixed
value."

> 
>         /* Transmit waveform amplitude can be improved (1000BASE-T,
> 100BASE-TX, 10BASE-Te) */
>         {0x1c, 0x04, 0x00d0},
> @@ -1939,7 +1939,7 @@ static const struct ksz9477_errata_write
> ksz9477_errata_writes[] = {
>         {0x1c, 0x20, 0xeeee},
>  };
> 
> -static int ksz9477_config_init(struct phy_device *phydev)
> +static int ksz9477_phy_errata(struct phy_device *phydev)
>  {
>         int err;
>         int i;
> @@ -1967,16 +1967,26 @@ static int ksz9477_config_init(struct
> phy_device *phydev)
>                         return err;
>         }
> 
> +       return err;
> +}
> +
> +static int ksz9477_config_init(struct phy_device *phydev)
> +{
> +       int err;
> +
> +       /* Only KSZ9897 family of switches needs this fix. */
> +       if ((phydev->phy_id & 0xf) == 1) {
> +               err = ksz9477_phy_errata(phydev);
> +               if (err)
> +                       return err;
> +       }
> +
>         /* According to KSZ9477 Errata DS80000754C (Module 4) all EEE
> modes
>          * in this switch shall be regarded as broken.
>          */
>         if (phydev->dev_flags & MICREL_NO_EEE)
>                 phydev->eee_broken_modes = -1;
> 
> -       err = genphy_restart_aneg(phydev);
> -       if (err)
> -               return err;
> -

ksz9477_phy_errata is setting the PHY for 100 Mbps speed with auto-
negotiation off, as the errata sheet requests before applying the
errata writes, so it seems wrong to remove this auto-negotiation
restart here as it would presumably be left in that state otherwise.
Maybe it should be moved into that function instead?

>         return kszphy_config_init(phydev);
>  }
> 
> @@ -2085,6 +2095,42 @@ static int kszphy_resume(struct phy_device
> *phydev)
>         return 0;
>  }
> 
> +static int ksz9477_resume(struct phy_device *phydev)
> +{
> +       int ret;
> +
> +       /* No need to initialize registers if not powered down. */
> +       ret = phy_read(phydev, MII_BMCR);
> +       if (ret < 0)
> +               return ret;
> +       if (!(ret & BMCR_PDOWN))
> +               return 0;
> +
> +       genphy_resume(phydev);
> +
> +       /* After switching from power-down to normal mode, an
> internal global
> +        * reset is automatically generated. Wait a minimum of 1 ms
> before
> +        * read/write access to the PHY registers.
> +        */
> +       usleep_range(1000, 2000);
> +
> +       /* Only KSZ9897 family of switches needs this fix. */
> +       if ((phydev->phy_id & 0xf) == 1) {
> +               ret = ksz9477_phy_errata(phydev);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       /* Enable PHY Interrupts */
> +       if (phy_interrupt_is_valid(phydev)) {
> +               phydev->interrupts = PHY_INTERRUPT_ENABLED;
> +               if (phydev->drv->config_intr)
> +                       phydev->drv->config_intr(phydev);
> +       }
> +
> +       return 0;
> +}
> +
>  static int kszphy_probe(struct phy_device *phydev)
>  {
>         const struct kszphy_type *type = phydev->drv->driver_data;
> @@ -5493,7 +5539,7 @@ static struct phy_driver ksphy_driver[] = {
>         .config_intr    = kszphy_config_intr,
>         .handle_interrupt = kszphy_handle_interrupt,
>         .suspend        = genphy_suspend,
> -       .resume         = genphy_resume,
> +       .resume         = ksz9477_resume,
>         .get_features   = ksz9477_get_features,
>  } };
> 
> --
> 2.34.1
>