Re: [PATCH v3] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Russell King (Oracle)]

On Thu, Aug 31, 2023 at 09:25:27AM +0200, Lukasz Majewski wrote:
> diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
> index 8bef1ab62bba..eed474fc7308 100644
> --- a/include/linux/micrel_phy.h
> +++ b/include/linux/micrel_phy.h
> @@ -44,6 +44,7 @@
>  #define MICREL_PHY_50MHZ_CLK	0x00000001
>  #define MICREL_PHY_FXEN		0x00000002
>  #define MICREL_KSZ8_P1_ERRATA	0x00000003
> +#define MICREL_NO_EEE	0x00000004

Erm... Maybe someone should clarify this... we have in the code the
following tests for these "flags":

	/* Support legacy board-file configuration */
	if (phydev->dev_flags & MICREL_PHY_50MHZ_CLK) {
	        priv->rmii_ref_clk_sel = true;
	        priv->rmii_ref_clk_sel_val = true;
	}

	/* Skip auto-negotiation in fiber mode */
	if (phydev->dev_flags & MICREL_PHY_FXEN) {
	        phydev->speed = SPEED_100;
	        return 0;
	}

	if (phydev->dev_flags & MICREL_KSZ8_P1_ERRATA)
		return -EOPNOTSUPP;

	/* 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;

Is it intentional that setting MICREL_PHY_50MHZ_CLK on its own also
activates the MICREL_KSZ8_P1_ERRATA and vice versa? Is it intentional
that setting MICREL_PHY_FXEN also activates MICREL_KSZ8_P1_ERRATA and
vice versa?

To me, this looks horribly broken, and this patch just perpetuates the
brokenness (but at least 0x4 doesn't overlap with the other flags.)

If it is intentional, then MICREL_KSZ8_P1_ERRATA should be defined to
make it explicit - in other words, as
(MICREL_PHY_FXEN|MICREL_PHY_50MHZ_CLK). If not, all these flags should
be defined using (1 << n) or BIT() to make it explicit that they're a
bit, and not just a hex number that gets incremented when the next flag
is added.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!