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

[Russell King (Oracle)]

On Tue, Nov 29, 2022 at 10:56:56AM -0500, Sean Anderson wrote:
> On 11/28/22 19:42, Russell King (Oracle) wrote:
> > 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.
> 
> Well, then it's really a different register per MMD (and therefore the
> definitions should be better separated). Grouping them together implies
> that they share bits, when they do not (except for the 10G bit).

What about bits that are shared amongst the different registers.
Should we have multiple definitions for the link status bit in _all_
the different MMDs, despite it being the same across all status 1
registers?

Clause 45 is quite a trainwreck when it comes to these register
definitions.

As I've stated, there is a pattern to the naming. Understand it,
and it isn't confusing.

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