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!
On Thu, Aug 31, 2023 at 11:28:00AM +0100, Russell King (Oracle) wrote:
> 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.
Indeed, it is broken. I'll send a fix for this one.
MICREL_KSZ8_P1_ERRATA should be BIT(3)
and MICREL_NO_EEE BIT(4)
Thx!
Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |