Re: [PATCH net v2 2/2] phy: aquantia: Determine rate adaptation support from registers

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Nov 28, 2022 at 07:21:56PM -0500, Sean Anderson wrote:
> On 11/28/22 18:22, Russell King (Oracle) wrote:
> > This doesn't make any sense. priv->supported_speeds is the set of speeds
> > read from the PMAPMD. The only bits that are valid for this are the
> > MDIO_PMA_SPEED_* definitions, but teh above switch makes use of the
> > MDIO_PCS_SPEED_* definitions. To see why this is wrong, look at these
> > two definitions:
> > 
> > #define MDIO_PMA_SPEED_10               0x0040  /* 10M capable */
> > #define MDIO_PCS_SPEED_2_5G             0x0040  /* 2.5G capable */
> > 
> > Note that they are the same value, yet above, you're testing for bit 6
> > being clear effectively for both 10M and 2.5G speeds. I suspect this
> > is *not* what you want.
> > 
> > MDIO_PMA_SPEED_* are only valid for the PMAPMD MMD (MMD 1).
> > MDIO_PCS_SPEED_* are only valid for the PCS MMD (MMD 3).
> 
> Ugh. I almost noticed this from the register naming...
> 
> Part of the problem is that all the defines are right next to each other
> with no indication of what you just described.

That's because they all refer to the speed register which is at the same
address, but for some reason the 802.3 committees decided to make the
register bits mean different things depending on the MMD. That's why the
definition states the MMD name in it.

This is true of all definitions in mdio.h - the naming convention is of
the format "MDIO_mmd_register_bit" where the bit is specific to a MMD,
or "MDIO_register_bit" where it is non-specific (e.g. the status
register 1 link status bit.)

> Anyway, what I want are the PCS/PMA speeds from the 2018 revision, which
> this phy seems to follow.

I think we should add further entries for the PMA/PMD speed register.
For example, 2.5G is bit 13 and 5G is bit 14. (vs bits 6 and 7 for the
PCS MMD version of the speed register.)

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