[net-next] net: dsa: microchip: ksz9477: follow errata sheet when applying fixups

  The errata sheets for both ksz9477 and ksz9567 begin with

  IMPORTANT NOTE

  Multiple errata workarounds in this document call for changing PHY
  registers for each PHY port. PHY registers 0x0 to 0x1F are in the
  address range 0xN100 to 0xN13F, while indirect (MMD) PHY registers
  are accessed via the PHY MMD Setup Register and the PHY MMD Data
  Register.

  Before configuring the PHY MMD registers, it is necessary to set the
  PHY to 100 Mbps speed with auto-negotiation disabled by writing to
  register 0xN100-0xN101. After writing the MMD registers, and after
  all errata workarounds that involve PHY register settings, write
  register 0xN100-0xN101 again to enable and restart auto-negotiation.

Without that explicit auto-neg restart, we do sometimes have problems
establishing link.

Rather than writing back the hardcoded 0x1340 value the errata sheet
suggests (which likely just corresponds to the most common strap
configuration), restore the original value, setting the
PORT_AUTO_NEG_RESTART bit if PORT_AUTO_NEG_ENABLE is set.

Fixes: 1fc33199185d ("net: dsa: microchip: Add PHY errata workarounds")
Cc: stable@vger.kernel.org
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
While I do believe this is a fix, I don't think it's post-rc7
material, hence targeting net-next with cc stable.

 drivers/net/dsa/microchip/ksz9477.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
  

>  static void ksz9477_phy_errata_setup(struct ksz_device *dev, int port)
>  {
> +	u16 cr;
> +
> +	/* Errata document says the PHY must be configured to 100Mbps
> +	 * with auto-neg disabled before configuring the PHY MMD
> +	 * registers.
> +	 */
> +	ksz_pread16(dev, port, REG_PORT_PHY_CTRL, &cr);
> +	ksz_pwrite16(dev, port, REG_PORT_PHY_CTRL,
> +		     PORT_SPEED_100MBIT | PORT_FULL_DUPLEX);
> +

For this fix, these are fine.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Looking at the values of PORT_SPEED_100MBIT and PORT_FULL_DUPLEX, they
are identical to BMCR_SPEED100 and BMCR_FULLDPLX. In fact, it looks
like for 9477 this is a standard BMCR. Please could you add a follow
up patch which replaces these #defines with the standard ones in
include/uapi/linux/mii.h. The code is then more understandable by
people who know the standard defines.

Thanks
	Andrew
  

On Mon, 2023-06-19 at 10:16 +0200, Rasmus Villemoes 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.
> 
> The errata sheets for both ksz9477 and ksz9567 begin with
> 
>   IMPORTANT NOTE
> 
>   Multiple errata workarounds in this document call for changing PHY
>   registers for each PHY port. PHY registers 0x0 to 0x1F are in the
>   address range 0xN100 to 0xN13F, while indirect (MMD) PHY registers
>   are accessed via the PHY MMD Setup Register and the PHY MMD Data
>   Register.
> 
>   Before configuring the PHY MMD registers, it is necessary to set
> the
>   PHY to 100 Mbps speed with auto-negotiation disabled by writing to
>   register 0xN100-0xN101. After writing the MMD registers, and after
>   all errata workarounds that involve PHY register settings, write
>   register 0xN100-0xN101 again to enable and restart auto-
> negotiation.
> 
> Without that explicit auto-neg restart, we do sometimes have problems
> establishing link.
> 
> Rather than writing back the hardcoded 0x1340 value the errata sheet
> suggests (which likely just corresponds to the most common strap
> configuration), restore the original value, setting the
> PORT_AUTO_NEG_RESTART bit if PORT_AUTO_NEG_ENABLE is set.
> 
> Fixes: 1fc33199185d ("net: dsa: microchip: Add PHY errata
> workarounds")
> Cc: stable@vger.kernel.org
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> While I do believe this is a fix, I don't think it's post-rc7
> material, hence targeting net-next with cc stable.

I don't think this will apply to net-next as the relevant code has been
moved to the Micrel PHY driver and removed from this one in the
following commits, and effectively the same change to disable autoneg
before the register writes and re-enable afterwards was incorporated:

commit 26dd2974c5b5caef358784530c9e72715adc8f5b
Author: Robert Hancock <robert.hancock@calian.com>
Date:   Mon Jun 5 09:39:42 2023 -0600

    net: phy: micrel: Move KSZ9477 errata fixes to PHY driver

commit 6068e6d7ba5001dfb96bb8b7b92e2ed2a5877786
Author: Robert Hancock <robert.hancock@calian.com>
Date:   Mon Jun 5 09:39:43 2023 -0600

    net: dsa: microchip: remove KSZ9477 PHY errata handling

However, your patch may be reasonable to apply to -rc7 or stable as a
more targeted change for those releases.

> 
>  drivers/net/dsa/microchip/ksz9477.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c
> b/drivers/net/dsa/microchip/ksz9477.c
> index bf13d47c26cf..9a712ea71ee7 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -902,6 +902,16 @@ static void ksz9477_port_mmd_write(struct
> ksz_device *dev, int port,
> 
>  static void ksz9477_phy_errata_setup(struct ksz_device *dev, int
> port)
>  {
> +       u16 cr;
> +
> +       /* Errata document says the PHY must be configured to 100Mbps
> +        * with auto-neg disabled before configuring the PHY MMD
> +        * registers.
> +        */
> +       ksz_pread16(dev, port, REG_PORT_PHY_CTRL, &cr);
> +       ksz_pwrite16(dev, port, REG_PORT_PHY_CTRL,
> +                    PORT_SPEED_100MBIT | PORT_FULL_DUPLEX);
> +
>         /* Apply PHY settings to address errata listed in
>          * KSZ9477, KSZ9897, KSZ9896, KSZ9567, KSZ8565
>          * Silicon Errata and Data Sheet Clarification documents:
> @@ -943,6 +953,13 @@ static void ksz9477_phy_errata_setup(struct
> ksz_device *dev, int port)
>         ksz9477_port_mmd_write(dev, port, 0x1c, 0x1d, 0xe7ff);
>         ksz9477_port_mmd_write(dev, port, 0x1c, 0x1e, 0xefff);
>         ksz9477_port_mmd_write(dev, port, 0x1c, 0x20, 0xeeee);
> +
> +       /* Restore PHY CTRL register, restart auto-negotiation if
> +        * enabled in the original value.
> +        */
> +       if (cr & PORT_AUTO_NEG_ENABLE)
> +               cr |= PORT_AUTO_NEG_RESTART;
> +       ksz_pwrite16(dev, port, REG_PORT_PHY_CTRL, cr);
>  }
> 
>  void ksz9477_get_caps(struct ksz_device *dev, int port,
> --
> 2.37.2
> 

-- 
Robert Hancock <robert.hancock@calian.com>
  

On Mon, Jun 19, 2023 at 10:16:32AM +0200, Rasmus Villemoes wrote:
> The errata sheets for both ksz9477 and ksz9567 begin with
> 
>   IMPORTANT NOTE
> 
>   Multiple errata workarounds in this document call for changing PHY
>   registers for each PHY port. PHY registers 0x0 to 0x1F are in the
>   address range 0xN100 to 0xN13F, while indirect (MMD) PHY registers
>   are accessed via the PHY MMD Setup Register and the PHY MMD Data
>   Register.
> 
>   Before configuring the PHY MMD registers, it is necessary to set the
>   PHY to 100 Mbps speed with auto-negotiation disabled by writing to
>   register 0xN100-0xN101. After writing the MMD registers, and after
>   all errata workarounds that involve PHY register settings, write
>   register 0xN100-0xN101 again to enable and restart auto-negotiation.
> 
> Without that explicit auto-neg restart, we do sometimes have problems
> establishing link.
> 
> Rather than writing back the hardcoded 0x1340 value the errata sheet
> suggests (which likely just corresponds to the most common strap
> configuration), restore the original value, setting the
> PORT_AUTO_NEG_RESTART bit if PORT_AUTO_NEG_ENABLE is set.
> 
> Fixes: 1fc33199185d ("net: dsa: microchip: Add PHY errata workarounds")
> Cc: stable@vger.kernel.org
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> While I do believe this is a fix, I don't think it's post-rc7
> material, hence targeting net-next with cc stable.

Hi Rasmus,

unfortunately this does not seem to apply to net-next.
Please consider rebasing and reposting.
Please include Andrew's Reviewed-by tag unless there
are substantial changes (seems unlikely).
  

On 19/06/2023 19.31, Robert Hancock wrote:
> On Mon, 2023-06-19 at 10:16 +0200, Rasmus Villemoes wrote:
>>
>> Fixes: 1fc33199185d ("net: dsa: microchip: Add PHY errata
>> workarounds")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>> While I do believe this is a fix, I don't think it's post-rc7
>> material, hence targeting net-next with cc stable.
> 
> I don't think this will apply to net-next as the relevant code has been
> moved to the Micrel PHY driver and removed from this one in the
> following commits, 

Ah, sorry about that. This code hadn't been touched in a very long time,
so I didn't actually think to check -next.

and effectively the same change to disable autoneg
> before the register writes and re-enable afterwards was incorporated:

Yes, except it seems to always enable autoneg, even if the phy was
strapped otherwise. That's not a problem for our use case.

> However, your patch may be reasonable to apply to -rc7 or stable as a
> more targeted change for those releases.

Well, yes, it could be backported, but then there'd be a (simple)
conflict when net-next is to be merged in the next window. So I don't
think it's worth it.

Unless the net maintainers decide otherwise, please just drop this patch.

Rasmus