[PATCH net-next 0/2] net: phy: realtek: map vendor-specific EEE registers to standard MMD registers

[PATCH net-next 0/2] net: phy: realtek: map vendor-specific EEE registers to standard MMD registers

[Heiner Kallweit]

EEE-related registers on newer integrated PHY's have the standard
layout, but are accessible not via MMD but via vendor-specific
registers. Emulating the standard MMD registers allows to use the
generic functions for EEE control and to significantly simplify
the r8169 driver.

Heiner Kallweit (2):
  net: phy: realtek: add support for EEE registers on integrated PHY's
  r8169: use the generic EEE management functions

 drivers/net/ethernet/realtek/r8169_main.c | 172 +++-------------------
 drivers/net/phy/realtek.c                 |  43 ++++++
 2 files changed, 67 insertions(+), 148 deletions(-)

-- 
2.22.1

[PATCH net-next 1/2] net: phy: realtek: add support for EEE registers on integrated PHY's

[Heiner Kallweit]

EEE-related registers on newer integrated PHY's have the standard
layout, but are accessible not via MMD but via vendor-specific
registers. Emulating the standard MMD registers allows to use the
generic functions for EEE control.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/realtek.c | 43 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index c49a1fb13..2635ad1ff 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -266,6 +266,45 @@ static int rtl8366rb_config_init(struct phy_device *phydev)
 	return ret;
 }
 
+static int rtlgen_read_mmd(struct phy_device *phydev, int devnum, u16 regnum)
+{
+	int ret;
+
+	if (devnum == MDIO_MMD_PCS && regnum == MDIO_PCS_EEE_ABLE) {
+		rtl821x_write_page(phydev, 0xa5c);
+		ret = __phy_read(phydev, 0x12);
+		rtl821x_write_page(phydev, 0);
+	} else if (devnum == MDIO_MMD_AN && regnum == MDIO_AN_EEE_ADV) {
+		rtl821x_write_page(phydev, 0xa5d);
+		ret = __phy_read(phydev, 0x10);
+		rtl821x_write_page(phydev, 0);
+	} else if (devnum == MDIO_MMD_AN && regnum == MDIO_AN_EEE_LPABLE) {
+		rtl821x_write_page(phydev, 0xa5d);
+		ret = __phy_read(phydev, 0x11);
+		rtl821x_write_page(phydev, 0);
+	} else {
+		ret = -EOPNOTSUPP;
+	}
+
+	return ret;
+}
+
+static int rtlgen_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
+			    u16 val)
+{
+	int ret;
+
+	if (devnum == MDIO_MMD_AN && regnum == MDIO_AN_EEE_ADV) {
+		rtl821x_write_page(phydev, 0xa5d);
+		ret = __phy_write(phydev, 0x10, val);
+		rtl821x_write_page(phydev, 0);
+	} else {
+		ret = -EOPNOTSUPP;
+	}
+
+	return ret;
+}
+
 static int rtl8125_get_features(struct phy_device *phydev)
 {
 	int val;
@@ -422,6 +461,8 @@ static struct phy_driver realtek_drvs[] = {
 		.resume		= genphy_resume,
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
+		.read_mmd	= rtlgen_read_mmd,
+		.write_mmd	= rtlgen_write_mmd,
 	}, {
 		.name		= "RTL8125 2.5Gbps internal",
 		.match_phy_device = rtl8125_match_phy_device,
@@ -432,6 +473,8 @@ static struct phy_driver realtek_drvs[] = {
 		.resume		= genphy_resume,
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
+		.read_mmd	= rtlgen_read_mmd,
+		.write_mmd	= rtlgen_write_mmd,
 	}, {
 		PHY_ID_MATCH_EXACT(0x001cc961),
 		.name		= "RTL8366RB Gigabit Ethernet",
-- 
2.22.1

[PATCH net-next 2/2] r8169: use the generic EEE management functions

[Heiner Kallweit]

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(-)

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)
-- 
2.22.1

Re: [PATCH net-next 2/2] r8169: use the generic EEE management functions

[Andrew Lunn]

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

Re: [PATCH net-next 2/2] r8169: use the generic EEE management functions

[Heiner Kallweit]

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

Re: [PATCH net-next 2/2] r8169: use the generic EEE management functions

[Florian Fainelli]

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
-- 
Florian

Re: [PATCH net-next 2/2] r8169: use the generic EEE management functions

[Heiner Kallweit]

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