On 05.04.2019 23:38, Heiner Kallweit wrote: > 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. > I just see that we can reuse is_gigabit_capable also in genphy_config_advert().
And when checking occurrences of BMSR_ESTATEN there seems to be more work waiting: In swphy BMSR_ESTATEN is set, but MII_ESTATUS isn't configured. It just works by chance becausing reading this register returns the default 0xffff. >> Andrew >> > Heiner >
