On 05.04.2019 22:27, Andrew Lunn wrote: > On Fri, Apr 05, 2019 at 10:04:22PM +0200, Heiner Kallweit wrote: >> On 05.04.2019 21:48, Andrew Lunn wrote: >>> On Fri, Apr 05, 2019 at 09:23:13PM +0200, Heiner Kallweit wrote: >>>> genphy_read_status() so far checks phydev->supported, not the actual >>>> PHY capabilities. This can make a difference if the supported speeds >>>> have been limited by of_set_phy_supported() or phy_set_max_speed(). >>>> >>>> It seems that this issue only affects the link partner advertisements >>>> as displayed by ethtool. Also this patch wouldn't apply to older >>>> kernels because linkmode bitmaps have been introduced recently. >>>> Therefore net-next. >>> >>> Hi Heiner >>> >> Hi Andrew, >> >>> Are you saying, if the local PHY/MAC does not support 1000Base-T, we >>> should not check if the peer is advertising 1000Base-T? That seems >>> wrong. We should report everything it offers. The fact we cannot make >>> use of 1000Base-T should not matter, and resolving of the auto-get >>> should do the right thing when presented with modes it cannot use. >>> >> Link partner gigabit advertisement is reported in register MII_STAT1000. >> If we have a 100MBit PHY w/o this register, then we will never know >> whether the link partner supports gigabit. > > Hi Heiner > > There seems to be two uses cases: > > A Fast MAC connected to a Fast PHY. In that case, MII_STAT1000 > probably does not exist, so we should not read it. Hopefully .features > says BASIC, or if we ask the PHY what is it, it reports it is a Fast > PHY. > Right. BMSR_ESTATEN should not be set on a Fast PHY. Handling of this case didn't change.
> A Fast MAC connected to a Giga PHY. The MAC driver will of used > phy_set_max_speed() to indicate its limits. In that case, MII_STAT1000 > does exist and we should report what the peer is advertising. > That's what we're doing now with this patch. > Andrew > Heiner
