| Message ID | c5a137b1-d9d3-070c-55a1-938d6b77bdbc@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: realtek: map vendor-specific EEE registers to standard MMD registers | expand |
On Thu, Aug 15, 2019 at 11:47:33AM +0200, Heiner Kallweit wrote: > Now that the Realtek PHY driver maps the vendor-specific EEE registers > to the standard MMD registers, we can remove all special handling and > use the generic functions phy_ethtool_get/set_eee. Hi Heiner I think you should also add a call the phy_init_eee()? Andrew
On 15.08.2019 14:35, Andrew Lunn wrote: > On Thu, Aug 15, 2019 at 11:47:33AM +0200, Heiner Kallweit wrote: >> Now that the Realtek PHY driver maps the vendor-specific EEE registers >> to the standard MMD registers, we can remove all special handling and >> use the generic functions phy_ethtool_get/set_eee. > > Hi Heiner > Hi Andrew, > I think you should also add a call the phy_init_eee()? > I think it's not strictly needed. And few things regarding phy_init_eee are not fully clear to me: - When is it supposed to be called? Before each call to phy_ethtool_set_eee? Or once in the drivers init path? - The name is a little bit misleading as it's mainly a validity check. An actual "init" is done only if parameter clk_stop_enable is set. - It returns -EPROTONOSUPPORT if at least one link partner doesn't advertise EEE for current speed/duplex. To me this seems to be too restrictive. Example: We're at 1Gbps/full and link partner advertises EEE for 100Mbps only. Then phy_init_eee returns -EPROTONOSUPPORT. This keeps me from controlling 100Mbps EEE advertisement. > Andrew > Heiner
On 8/15/2019 6:02 AM, Heiner Kallweit wrote: > On 15.08.2019 14:35, Andrew Lunn wrote: >> On Thu, Aug 15, 2019 at 11:47:33AM +0200, Heiner Kallweit wrote: >>> Now that the Realtek PHY driver maps the vendor-specific EEE registers >>> to the standard MMD registers, we can remove all special handling and >>> use the generic functions phy_ethtool_get/set_eee. >> >> Hi Heiner >> > Hi Andrew, > >> I think you should also add a call the phy_init_eee()? >> > I think it's not strictly needed. And few things regarding > phy_init_eee are not fully clear to me: > > - When is it supposed to be called? Before each call to > phy_ethtool_set_eee? Or once in the drivers init path? > > - The name is a little bit misleading as it's mainly a > validity check. An actual "init" is done only if > parameter clk_stop_enable is set. > > - It returns -EPROTONOSUPPORT if at least one link partner > doesn't advertise EEE for current speed/duplex. To me this > seems to be too restrictive. Example: > We're at 1Gbps/full and link partner advertises EEE for > 100Mbps only. Then phy_init_eee returns -EPROTONOSUPPORT. > This keeps me from controlling 100Mbps EEE advertisement. That function needs a complete rework, it does not say what its name implies, and there is an assumption that you have already locally advertised EEE for it to work properly, that does not make any sense since the whole purpose is to see whether EEE can/will be active with the link partner (that's how I read it at least). Regarding whether the clock stop enable can be turned on or off is also a bit suspicious, because a MAC driver does not know whether the PHY supports doing that, I had started something in that area years ago: https://github.com/ffainelli/linux/commits/phy-eee-tx-clk
On 15.08.2019 17:43, Florian Fainelli wrote: > > > On 8/15/2019 6:02 AM, Heiner Kallweit wrote: >> On 15.08.2019 14:35, Andrew Lunn wrote: >>> On Thu, Aug 15, 2019 at 11:47:33AM +0200, Heiner Kallweit wrote: >>>> Now that the Realtek PHY driver maps the vendor-specific EEE registers >>>> to the standard MMD registers, we can remove all special handling and >>>> use the generic functions phy_ethtool_get/set_eee. >>> >>> Hi Heiner >>> >> Hi Andrew, >> >>> I think you should also add a call the phy_init_eee()? >>> >> I think it's not strictly needed. And few things regarding >> phy_init_eee are not fully clear to me: >> >> - When is it supposed to be called? Before each call to >> phy_ethtool_set_eee? Or once in the drivers init path? >> >> - The name is a little bit misleading as it's mainly a >> validity check. An actual "init" is done only if >> parameter clk_stop_enable is set. >> >> - It returns -EPROTONOSUPPORT if at least one link partner >> doesn't advertise EEE for current speed/duplex. To me this >> seems to be too restrictive. Example: >> We're at 1Gbps/full and link partner advertises EEE for >> 100Mbps only. Then phy_init_eee returns -EPROTONOSUPPORT. >> This keeps me from controlling 100Mbps EEE advertisement. > > That function needs a complete rework, it does not say what its name > implies, and there is an assumption that you have already locally > advertised EEE for it to work properly, that does not make any sense > since the whole purpose is to see whether EEE can/will be active with > the link partner (that's how I read it at least). > > Regarding whether the clock stop enable can be turned on or off is also > a bit suspicious, because a MAC driver does not know whether the PHY > supports doing that, I had started something in that area years ago: > > https://github.com/ffainelli/linux/commits/phy-eee-tx-clk > Not related to this patch, but to EEE support in general: There's something in the back of my mind to create linkmodes for all EEE modes. They could be used with the normal supported, advertising, and lp_advertising bitmaps. Means: - extend genphy_read_abilities to read supported EEE modes - extend genphy_read_status to read lp-advertised EEE modes - let genphy_config_aneg handle EEE advertising - .. This should help to make EEE mode handling consistent with link mode handling. Also still missing is support for the new C45 registers for 2.5Gbps and 5Gbps EEE (3.21, 7.62, 7.63). Open for me is how to deal best with the scenario that older PHY's use the C22 MMD registers for proprietary purposes. Writing to the MMD address register then may cause misbehavior of the PHY (that was the case for RTL8211B), and MMD reads may return random data. Maybe we need a flag to explicitly state whether MMD is supported or not. Heiner
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 7dd75c047..bd9077f85 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -758,6 +758,13 @@ static bool rtl_is_8168evl_up(struct rtl8169_private *tp) tp->mac_version <= RTL_GIGA_MAC_VER_51; } +static bool rtl_supports_eee(struct rtl8169_private *tp) +{ + return tp->mac_version >= RTL_GIGA_MAC_VER_34 && + tp->mac_version != RTL_GIGA_MAC_VER_37 && + tp->mac_version != RTL_GIGA_MAC_VER_39; +} + static void rtl_read_mac_from_reg(struct rtl8169_private *tp, u8 *mac, int reg) { int i; @@ -2014,144 +2021,40 @@ static int rtl_set_coalesce(struct net_device *dev, struct ethtool_coalesce *ec) return 0; } -static int rtl_get_eee_supp(struct rtl8169_private *tp) -{ - struct phy_device *phydev = tp->phydev; - int ret; - - switch (tp->mac_version) { - case RTL_GIGA_MAC_VER_34: - case RTL_GIGA_MAC_VER_35: - case RTL_GIGA_MAC_VER_36: - case RTL_GIGA_MAC_VER_38: - ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE); - break; - case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_51: - ret = phy_read_paged(phydev, 0x0a5c, 0x12); - break; - default: - ret = -EPROTONOSUPPORT; - break; - } - - return ret; -} - -static int rtl_get_eee_lpadv(struct rtl8169_private *tp) -{ - struct phy_device *phydev = tp->phydev; - int ret; - - switch (tp->mac_version) { - case RTL_GIGA_MAC_VER_34: - case RTL_GIGA_MAC_VER_35: - case RTL_GIGA_MAC_VER_36: - case RTL_GIGA_MAC_VER_38: - ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_LPABLE); - break; - case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_51: - ret = phy_read_paged(phydev, 0x0a5d, 0x11); - break; - default: - ret = -EPROTONOSUPPORT; - break; - } - - return ret; -} - -static int rtl_get_eee_adv(struct rtl8169_private *tp) -{ - struct phy_device *phydev = tp->phydev; - int ret; - - switch (tp->mac_version) { - case RTL_GIGA_MAC_VER_34: - case RTL_GIGA_MAC_VER_35: - case RTL_GIGA_MAC_VER_36: - case RTL_GIGA_MAC_VER_38: - ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV); - break; - case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_51: - ret = phy_read_paged(phydev, 0x0a5d, 0x10); - break; - default: - ret = -EPROTONOSUPPORT; - break; - } - - return ret; -} - -static int rtl_set_eee_adv(struct rtl8169_private *tp, int val) -{ - struct phy_device *phydev = tp->phydev; - int ret = 0; - - switch (tp->mac_version) { - case RTL_GIGA_MAC_VER_34: - case RTL_GIGA_MAC_VER_35: - case RTL_GIGA_MAC_VER_36: - case RTL_GIGA_MAC_VER_38: - ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, val); - break; - case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_51: - phy_write_paged(phydev, 0x0a5d, 0x10, val); - break; - default: - ret = -EPROTONOSUPPORT; - break; - } - - return ret; -} - static int rtl8169_get_eee(struct net_device *dev, struct ethtool_eee *data) { struct rtl8169_private *tp = netdev_priv(dev); struct device *d = tp_to_dev(tp); int ret; + if (!rtl_supports_eee(tp)) + return -EOPNOTSUPP; + pm_runtime_get_noresume(d); if (!pm_runtime_active(d)) { ret = -EOPNOTSUPP; - goto out; + } else { + ret = phy_ethtool_get_eee(tp->phydev, data); } - /* Get Supported EEE */ - ret = rtl_get_eee_supp(tp); - if (ret < 0) - goto out; - data->supported = mmd_eee_cap_to_ethtool_sup_t(ret); - - /* Get advertisement EEE */ - ret = rtl_get_eee_adv(tp); - if (ret < 0) - goto out; - data->advertised = mmd_eee_adv_to_ethtool_adv_t(ret); - data->eee_enabled = !!data->advertised; - - /* Get LP advertisement EEE */ - ret = rtl_get_eee_lpadv(tp); - if (ret < 0) - goto out; - data->lp_advertised = mmd_eee_adv_to_ethtool_adv_t(ret); - data->eee_active = !!(data->advertised & data->lp_advertised); -out: pm_runtime_put_noidle(d); - return ret < 0 ? ret : 0; + + return ret; } static int rtl8169_set_eee(struct net_device *dev, struct ethtool_eee *data) { struct rtl8169_private *tp = netdev_priv(dev); struct device *d = tp_to_dev(tp); - int old_adv, adv = 0, cap, ret; + int ret; + + if (!rtl_supports_eee(tp)) + return -EOPNOTSUPP; pm_runtime_get_noresume(d); - if (!dev->phydev || !pm_runtime_active(d)) { + if (!pm_runtime_active(d)) { ret = -EOPNOTSUPP; goto out; } @@ -2162,38 +2065,10 @@ static int rtl8169_set_eee(struct net_device *dev, struct ethtool_eee *data) goto out; } - /* Get Supported EEE */ - ret = rtl_get_eee_supp(tp); - if (ret < 0) - goto out; - cap = ret; - - ret = rtl_get_eee_adv(tp); - if (ret < 0) - goto out; - old_adv = ret; - - if (data->eee_enabled) { - adv = !data->advertised ? cap : - ethtool_adv_to_mmd_eee_adv_t(data->advertised) & cap; - /* Mask prohibited EEE modes */ - adv &= ~dev->phydev->eee_broken_modes; - } - - if (old_adv != adv) { - ret = rtl_set_eee_adv(tp, adv); - if (ret < 0) - goto out; - - /* Restart autonegotiation so the new modes get sent to the - * link partner. - */ - ret = phy_restart_aneg(dev->phydev); - } - + ret = phy_ethtool_set_eee(tp->phydev, data); out: pm_runtime_put_noidle(d); - return ret < 0 ? ret : 0; + return ret; } static const struct ethtool_ops rtl8169_ethtool_ops = { @@ -2220,10 +2095,11 @@ static const struct ethtool_ops rtl8169_ethtool_ops = { static void rtl_enable_eee(struct rtl8169_private *tp) { - int supported = rtl_get_eee_supp(tp); + struct phy_device *phydev = tp->phydev; + int supported = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE); if (supported > 0) - rtl_set_eee_adv(tp, supported); + phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, supported); } static void rtl8169_get_mac_version(struct rtl8169_private *tp)
Now that the Realtek PHY driver maps the vendor-specific EEE registers to the standard MMD registers, we can remove all special handling and use the generic functions phy_ethtool_get/set_eee. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/realtek/r8169_main.c | 172 +++------------------- 1 file changed, 24 insertions(+), 148 deletions(-)