On 05.04.2019 23:27, Andrew Lunn wrote: >> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, >> + phydev->supported)) >> + phydev->is_gigabit_capable = 1; >> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, >> + phydev->supported)) >> + phydev->is_gigabit_capable = 1; >> + > > What i'm trying to get at is, why do we need this bit of the patch? > Why do we need this flag? The hardware should tell us if it can do > gigabit. > The code to query BMSR_ESTATEN and MII_ESTATUS is in genphy_read_status. However we also have to cover the case that this function isn't used. Therefore I query phydev->supported before the speed could be limited. (relying on the PHY driver not lying about gigabit capability) This part of the patch is directly before of_set_phy_supported().
I just see that we can re-use is_gigabit_capable also in genphy_config_advert. Of course I can read in every place the hardware for gigabit support. But IMO this creates unnecessary code duplication. > Andrew > Heiner
