[PATCH net-next 00/10] r8169: add phylib support

[PATCH net-next 00/10] r8169: add phylib support

[Heiner Kallweit]

Now that all the basic refactoring has been done we can add phylib
support. This patch series was successfully tested on:
RTL8168h
RTL8168evl
RTL8169sb

Heiner Kallweit (10):
  r8169: add basic phylib support
  r8169: use phy_resume/phy_suspend
  r8169: replace open-coded PHY soft reset with genphy_soft_reset
  r8169: use phy_ethtool_(g|s)et_link_ksettings
  r8169: use phy_ethtool_nway_reset
  r8169: use phy_mii_ioctl
  r8169: migrate speed_down function to phylib
  r8169: remove rtl8169_set_speed_xmii
  r8169: remove mii_if_info member from struct rtl8169_private
  r8169: don't read chip phy status register

 drivers/net/ethernet/realtek/Kconfig |   2 +-
 drivers/net/ethernet/realtek/r8169.c | 454 +++++++++------------------
 2 files changed, 152 insertions(+), 304 deletions(-)

-- 
2.18.0

[PATCH net-next 01/10] r8169: add basic phylib support

[Heiner Kallweit]

Add basic phylib support to r8169. All now unneeded old PHY handling code
will be removed in subsequent patches.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/Kconfig |   1 +
 drivers/net/ethernet/realtek/r8169.c | 146 +++++++++++++++++++++------
 2 files changed, 115 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig
index 7c69f4c8..7fb1af1f 100644
--- a/drivers/net/ethernet/realtek/Kconfig
+++ b/drivers/net/ethernet/realtek/Kconfig
@@ -99,6 +99,7 @@ config R8169
 	depends on PCI
 	select FW_LOADER
 	select CRC32
+	select PHYLIB
 	select MII
 	---help---
 	  Say Y here if you have a Realtek 8169 PCI Gigabit Ethernet adapter.
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index f80ac894..7443b230 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -16,6 +16,7 @@
 #include <linux/delay.h>
 #include <linux/ethtool.h>
 #include <linux/mii.h>
+#include <linux/phy.h>
 #include <linux/if_vlan.h>
 #include <linux/crc32.h>
 #include <linux/in.h>
@@ -754,6 +755,7 @@ struct rtl8169_private {
 	} wk;
 
 	struct mii_if_info mii;
+	struct mii_bus *mii_bus;
 	dma_addr_t counters_phys_addr;
 	struct rtl8169_counters *counters;
 	struct rtl8169_tc_offsets tc_offset;
@@ -1444,11 +1446,6 @@ static unsigned int rtl8169_xmii_reset_pending(struct rtl8169_private *tp)
 	return rtl_readphy(tp, MII_BMCR) & BMCR_RESET;
 }
 
-static unsigned int rtl8169_xmii_link_ok(struct rtl8169_private *tp)
-{
-	return RTL_R8(tp, PHYstatus) & LinkStatus;
-}
-
 static void rtl8169_xmii_reset_enable(struct rtl8169_private *tp)
 {
 	unsigned int val;
@@ -1513,25 +1510,6 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp)
 	}
 }
 
-static void rtl8169_check_link_status(struct net_device *dev,
-				      struct rtl8169_private *tp)
-{
-	struct device *d = tp_to_dev(tp);
-
-	if (rtl8169_xmii_link_ok(tp)) {
-		rtl_link_chg_patch(tp);
-		/* This is to cancel a scheduled suspend if there's one. */
-		pm_request_resume(d);
-		netif_carrier_on(dev);
-		if (net_ratelimit())
-			netif_info(tp, ifup, dev, "link up\n");
-	} else {
-		netif_carrier_off(dev);
-		netif_info(tp, ifdown, dev, "link down\n");
-		pm_runtime_idle(d);
-	}
-}
-
 #define WAKE_ANY (WAKE_PHY | WAKE_MAGIC | WAKE_UCAST | WAKE_BCAST | WAKE_MCAST)
 
 /* Currently we only enable WoL if explicitly told by userspace to circumvent
@@ -6228,7 +6206,6 @@ static void rtl_reset_work(struct rtl8169_private *tp)
 	napi_enable(&tp->napi);
 	rtl_hw_start(tp);
 	netif_wake_queue(dev);
-	rtl8169_check_link_status(dev, tp);
 }
 
 static void rtl8169_tx_timeout(struct net_device *dev)
@@ -6845,7 +6822,7 @@ static void rtl_slow_event_work(struct rtl8169_private *tp)
 		rtl8169_pcierr_interrupt(dev);
 
 	if (status & LinkChg)
-		rtl8169_check_link_status(dev, tp);
+		phy_mac_interrupt(dev->phydev);
 
 	rtl_irq_enable_all(tp);
 }
@@ -6927,10 +6904,53 @@ static void rtl8169_rx_missed(struct net_device *dev)
 	RTL_W32(tp, RxMissed, 0);
 }
 
+static void r8169_phylink_handler(struct net_device *ndev)
+{
+	struct rtl8169_private *tp = netdev_priv(ndev);
+
+	if (netif_carrier_ok(ndev)) {
+		rtl_link_chg_patch(tp);
+		pm_request_resume(&tp->pci_dev->dev);
+	} else {
+		pm_runtime_idle(&tp->pci_dev->dev);
+	}
+
+	if (net_ratelimit())
+		phy_print_status(ndev->phydev);
+}
+
+static int r8169_phy_connect(struct rtl8169_private *tp)
+{
+	struct phy_device *phydev;
+	phy_interface_t phy_mode;
+	int ret;
+
+	phy_mode = tp->mii.supports_gmii ? PHY_INTERFACE_MODE_GMII :
+		   PHY_INTERFACE_MODE_MII;
+
+	phydev = mdiobus_get_phy(tp->mii_bus, 0);
+	if (!phydev)
+		return -ENODEV;
+
+	if (!tp->mii.supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
+		netif_info(tp, probe, tp->dev, "Restrict PHY to 100Mbit because MAC doesn't support 1GBit\n");
+		phy_set_max_speed(phydev, SPEED_100);
+	}
+
+	ret = phy_connect_direct(tp->dev, phydev, r8169_phylink_handler,
+				 phy_mode);
+	if (!ret)
+		phy_attached_info(phydev);
+
+	return ret;
+}
+
 static void rtl8169_down(struct net_device *dev)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
 
+	phy_stop(dev->phydev);
+
 	napi_disable(&tp->napi);
 	netif_stop_queue(dev);
 
@@ -6970,6 +6990,8 @@ static int rtl8169_close(struct net_device *dev)
 
 	cancel_work_sync(&tp->wk.work);
 
+	phy_disconnect(dev->phydev);
+
 	pci_free_irq(pdev, 0, tp);
 
 	dma_free_coherent(&pdev->dev, R8169_RX_RING_BYTES, tp->RxDescArray,
@@ -7030,6 +7052,10 @@ static int rtl_open(struct net_device *dev)
 	if (retval < 0)
 		goto err_release_fw_2;
 
+	retval = r8169_phy_connect(tp);
+	if (retval)
+		goto err_free_irq;
+
 	rtl_lock_work(tp);
 
 	set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
@@ -7045,16 +7071,17 @@ static int rtl_open(struct net_device *dev)
 	if (!rtl8169_init_counter_offsets(tp))
 		netif_warn(tp, hw, dev, "counter reset/update failed\n");
 
+	phy_start(dev->phydev);
 	netif_start_queue(dev);
 
 	rtl_unlock_work(tp);
 
 	pm_runtime_put_sync(&pdev->dev);
-
-	rtl8169_check_link_status(dev, tp);
 out:
 	return retval;
 
+err_free_irq:
+	pci_free_irq(pdev, 0, tp);
 err_release_fw_2:
 	rtl_release_firmware(tp);
 	rtl8169_rx_clear(tp);
@@ -7133,6 +7160,7 @@ static void rtl8169_net_suspend(struct net_device *dev)
 	if (!netif_running(dev))
 		return;
 
+	phy_stop(dev->phydev);
 	netif_device_detach(dev);
 	netif_stop_queue(dev);
 
@@ -7165,6 +7193,8 @@ static void __rtl8169_resume(struct net_device *dev)
 	rtl_pll_power_up(tp);
 	rtl8169_init_phy(dev, tp);
 
+	phy_start(tp->dev->phydev);
+
 	rtl_lock_work(tp);
 	napi_enable(&tp->napi);
 	set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
@@ -7310,6 +7340,7 @@ static void rtl_remove_one(struct pci_dev *pdev)
 	netif_napi_del(&tp->napi);
 
 	unregister_netdev(dev);
+	mdiobus_unregister(tp->mii_bus);
 
 	rtl_release_firmware(tp);
 
@@ -7395,6 +7426,51 @@ DECLARE_RTL_COND(rtl_rxtx_empty_cond)
 	return (RTL_R8(tp, MCU) & RXTX_EMPTY) == RXTX_EMPTY;
 }
 
+static int r8169_mdio_read_reg(struct mii_bus *mii_bus, int phyaddr, int phyreg)
+{
+	struct rtl8169_private *tp = mii_bus->priv;
+
+	return rtl_readphy(tp, phyreg);
+}
+
+static int r8169_mdio_write_reg(struct mii_bus *mii_bus, int phyaddr,
+				int phyreg, u16 val)
+{
+	struct rtl8169_private *tp = mii_bus->priv;
+
+	rtl_writephy(tp, phyreg, val);
+
+	return 0;
+}
+
+static int r8169_mdio_register(struct rtl8169_private *tp)
+{
+	struct pci_dev *pdev = tp->pci_dev;
+	struct mii_bus *new_bus;
+	int ret;
+
+	new_bus = devm_mdiobus_alloc(&pdev->dev);
+	if (!new_bus)
+		return -ENOMEM;
+
+	new_bus->name = "r8169";
+	new_bus->phy_mask = ~1;
+	new_bus->priv = tp;
+	new_bus->parent = &pdev->dev;
+	new_bus->irq[0] = PHY_IGNORE_INTERRUPT;
+	snprintf(new_bus->id, MII_BUS_ID_SIZE, "r8169-%x",
+		 PCI_DEVID(pdev->bus->number, pdev->devfn));
+
+	new_bus->read = r8169_mdio_read_reg;
+	new_bus->write = r8169_mdio_write_reg;
+
+	ret = mdiobus_register(new_bus);
+	if (!ret)
+		tp->mii_bus = new_bus;
+
+	return ret;
+}
+
 static void rtl_hw_init_8168g(struct rtl8169_private *tp)
 {
 	u32 data;
@@ -7651,10 +7727,14 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	pci_set_drvdata(pdev, dev);
 
-	rc = register_netdev(dev);
-	if (rc < 0)
+	rc = r8169_mdio_register(tp);
+	if (rc)
 		return rc;
 
+	rc = register_netdev(dev);
+	if (rc)
+		goto err_mdio_unregister;
+
 	netif_info(tp, probe, dev, "%s, %pM, XID %08x, IRQ %d\n",
 		   rtl_chip_infos[chipset].name, dev->dev_addr,
 		   (u32)(RTL_R32(tp, TxConfig) & 0xfcf0f8ff),
@@ -7669,12 +7749,14 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (r8168_check_dash(tp))
 		rtl8168_driver_start(tp);
 
-	netif_carrier_off(dev);
-
 	if (pci_dev_run_wake(pdev))
 		pm_runtime_put_sync(&pdev->dev);
 
 	return 0;
+
+err_mdio_unregister:
+	mdiobus_unregister(tp->mii_bus);
+	return rc;
 }
 
 static struct pci_driver rtl8169_pci_driver = {
-- 
2.18.0

Re: [PATCH net-next 01/10] r8169: add basic phylib support

[Andrew Lunn]

> +static int r8169_mdio_read_reg(struct mii_bus *mii_bus, int phyaddr, int phyreg)
> +{
> +	struct rtl8169_private *tp = mii_bus->priv;
> +
> +	return rtl_readphy(tp, phyreg);

So there is no support for phyaddr?

It would be better to trap the phyaddr which are not supported and
return 0xffff.

       Andrew

Re: [PATCH net-next 01/10] r8169: add basic phylib support

[Heiner Kallweit]

On 02.07.2018 23:02, Andrew Lunn wrote:
>> +static int r8169_mdio_read_reg(struct mii_bus *mii_bus, int phyaddr, int phyreg)
>> +{
>> +	struct rtl8169_private *tp = mii_bus->priv;
>> +
>> +	return rtl_readphy(tp, phyreg);
> 
> So there is no support for phyaddr?
> 
Right, the chip can access only the one internal PHY, therefore it
doesn't support phyaddr.

> It would be better to trap the phyaddr which are not supported and
> return 0xffff.
> 
OK

>        Andrew
> 

Re: [PATCH net-next 01/10] r8169: add basic phylib support

[Florian Fainelli]

On 07/02/2018 02:15 PM, Heiner Kallweit wrote:
> On 02.07.2018 23:02, Andrew Lunn wrote:
>>> +static int r8169_mdio_read_reg(struct mii_bus *mii_bus, int phyaddr, int phyreg)
>>> +{
>>> +	struct rtl8169_private *tp = mii_bus->priv;
>>> +
>>> +	return rtl_readphy(tp, phyreg);
>>
>> So there is no support for phyaddr?
>>
> Right, the chip can access only the one internal PHY, therefore it
> doesn't support phyaddr.

Then you might also want to set mii_bus->phy_mask accordingly such that
only the internal PHY address bit is cleared there?
-- 
Florian

Re: [PATCH net-next 01/10] r8169: add basic phylib support

[Heiner Kallweit]

On 03.07.2018 18:42, Florian Fainelli wrote:
> 
> 
> On 07/02/2018 02:15 PM, Heiner Kallweit wrote:
>> On 02.07.2018 23:02, Andrew Lunn wrote:
>>>> +static int r8169_mdio_read_reg(struct mii_bus *mii_bus, int phyaddr, int phyreg)
>>>> +{
>>>> +	struct rtl8169_private *tp = mii_bus->priv;
>>>> +
>>>> +	return rtl_readphy(tp, phyreg);
>>>
>>> So there is no support for phyaddr?
>>>
>> Right, the chip can access only the one internal PHY, therefore it
>> doesn't support phyaddr.
> 
> Then you might also want to set mii_bus->phy_mask accordingly such that
> only the internal PHY address bit is cleared there?
> 
That's something I'm doing already, see following line in r8169_mdio_register():
new_bus->phy_mask = ~1;
But thanks for the hint anyway.

[PATCH net-next 02/10] r8169: use phy_resume/phy_suspend

[Heiner Kallweit]

Use phy_resume() / phy_suspend() instead of open coding this functionality.
The chip version specific differences are handled by the respective PHY
drivers.

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

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 7443b230..0fba2581 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4457,47 +4457,6 @@ static bool rtl_wol_pll_power_down(struct rtl8169_private *tp)
 	return true;
 }
 
-static void r8168_phy_power_up(struct rtl8169_private *tp)
-{
-	rtl_writephy(tp, 0x1f, 0x0000);
-	switch (tp->mac_version) {
-	case RTL_GIGA_MAC_VER_11:
-	case RTL_GIGA_MAC_VER_12:
-	case RTL_GIGA_MAC_VER_17 ... RTL_GIGA_MAC_VER_28:
-	case RTL_GIGA_MAC_VER_31:
-		rtl_writephy(tp, 0x0e, 0x0000);
-		break;
-	default:
-		break;
-	}
-	rtl_writephy(tp, MII_BMCR, BMCR_ANENABLE);
-
-	/* give MAC/PHY some time to resume */
-	msleep(20);
-}
-
-static void r8168_phy_power_down(struct rtl8169_private *tp)
-{
-	rtl_writephy(tp, 0x1f, 0x0000);
-	switch (tp->mac_version) {
-	case RTL_GIGA_MAC_VER_32:
-	case RTL_GIGA_MAC_VER_33:
-	case RTL_GIGA_MAC_VER_40:
-	case RTL_GIGA_MAC_VER_41:
-		rtl_writephy(tp, MII_BMCR, BMCR_ANENABLE | BMCR_PDOWN);
-		break;
-
-	case RTL_GIGA_MAC_VER_11:
-	case RTL_GIGA_MAC_VER_12:
-	case RTL_GIGA_MAC_VER_17 ... RTL_GIGA_MAC_VER_28:
-	case RTL_GIGA_MAC_VER_31:
-		rtl_writephy(tp, 0x0e, 0x0200);
-	default:
-		rtl_writephy(tp, MII_BMCR, BMCR_PDOWN);
-		break;
-	}
-}
-
 static void r8168_pll_power_down(struct rtl8169_private *tp)
 {
 	if (r8168_check_dash(tp))
@@ -4510,7 +4469,8 @@ static void r8168_pll_power_down(struct rtl8169_private *tp)
 	if (rtl_wol_pll_power_down(tp))
 		return;
 
-	r8168_phy_power_down(tp);
+	/* cover the case that PHY isn't connected */
+	phy_suspend(mdiobus_get_phy(tp->mii_bus, 0));
 
 	switch (tp->mac_version) {
 	case RTL_GIGA_MAC_VER_25 ... RTL_GIGA_MAC_VER_33:
@@ -4563,7 +4523,9 @@ static void r8168_pll_power_up(struct rtl8169_private *tp)
 		break;
 	}
 
-	r8168_phy_power_up(tp);
+	phy_resume(tp->dev->phydev);
+	/* give MAC/PHY some time to resume */
+	msleep(20);
 }
 
 static void rtl_pll_power_down(struct rtl8169_private *tp)
-- 
2.18.0

Re: [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend

[Andrew Lunn]

>  static void r8168_pll_power_down(struct rtl8169_private *tp)
>  {
>  	if (r8168_check_dash(tp))
> @@ -4510,7 +4469,8 @@ static void r8168_pll_power_down(struct rtl8169_private *tp)
>  	if (rtl_wol_pll_power_down(tp))
>  		return;
>  
> -	r8168_phy_power_down(tp);
> +	/* cover the case that PHY isn't connected */
> +	phy_suspend(mdiobus_get_phy(tp->mii_bus, 0));

This could do some more explanation. Why would it not be connected?

     Andrew

Re: [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend

[Heiner Kallweit]

On 02.07.2018 23:06, Andrew Lunn wrote:
>>  static void r8168_pll_power_down(struct rtl8169_private *tp)
>>  {
>>  	if (r8168_check_dash(tp))
>> @@ -4510,7 +4469,8 @@ static void r8168_pll_power_down(struct rtl8169_private *tp)
>>  	if (rtl_wol_pll_power_down(tp))
>>  		return;
>>  
>> -	r8168_phy_power_down(tp);
>> +	/* cover the case that PHY isn't connected */
>> +	phy_suspend(mdiobus_get_phy(tp->mii_bus, 0));
> 
> This could do some more explanation. Why would it not be connected?
> 
The PHY gets connected when the net_device is opened. If a network
port isn't used then it will be runtime-suspended a few seconds after
boot. In this case we call r8168_pll_power_down() with the PHY not
being connected. 

>      Andrew
> 

Re: [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend

[Florian Fainelli]

On 07/02/2018 02:24 PM, Heiner Kallweit wrote:
> On 02.07.2018 23:06, Andrew Lunn wrote:
>>>  static void r8168_pll_power_down(struct rtl8169_private *tp)
>>>  {
>>>  	if (r8168_check_dash(tp))
>>> @@ -4510,7 +4469,8 @@ static void r8168_pll_power_down(struct rtl8169_private *tp)
>>>  	if (rtl_wol_pll_power_down(tp))
>>>  		return;
>>>  
>>> -	r8168_phy_power_down(tp);
>>> +	/* cover the case that PHY isn't connected */
>>> +	phy_suspend(mdiobus_get_phy(tp->mii_bus, 0));
>>
>> This could do some more explanation. Why would it not be connected?
>>
> The PHY gets connected when the net_device is opened. If a network
> port isn't used then it will be runtime-suspended a few seconds after
> boot. In this case we call r8168_pll_power_down() with the PHY not
> being connected. 

Would not the !netif_running() check in rtl8169_net_suspend() take care
of that though?
-- 
Florian

Re: [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend

[Heiner Kallweit]

On 03.07.2018 18:44, Florian Fainelli wrote:
> 
> 
> On 07/02/2018 02:24 PM, Heiner Kallweit wrote:
>> On 02.07.2018 23:06, Andrew Lunn wrote:
>>>>  static void r8168_pll_power_down(struct rtl8169_private *tp)
>>>>  {
>>>>  	if (r8168_check_dash(tp))
>>>> @@ -4510,7 +4469,8 @@ static void r8168_pll_power_down(struct rtl8169_private *tp)
>>>>  	if (rtl_wol_pll_power_down(tp))
>>>>  		return;
>>>>  
>>>> -	r8168_phy_power_down(tp);
>>>> +	/* cover the case that PHY isn't connected */
>>>> +	phy_suspend(mdiobus_get_phy(tp->mii_bus, 0));
>>>
>>> This could do some more explanation. Why would it not be connected?
>>>
>> The PHY gets connected when the net_device is opened. If a network
>> port isn't used then it will be runtime-suspended a few seconds after
>> boot. In this case we call r8168_pll_power_down() with the PHY not
>> being connected. 
> 
> Would not the !netif_running() check in rtl8169_net_suspend() take care
> of that though?
> 
We don't come that far ..
If the interface is down then tp->TxDescArray is NULL. Means we call
rtl_pll_power_down() in rtl8169_runtime_suspend() before reaching
rtl8169_net_suspend().

[PATCH net-next 03/10] r8169: replace open-coded PHY soft reset with genphy_soft_reset

[Heiner Kallweit]

Use genphy_soft_reset() instead of open-coding a PHY soft reset. We have
to do an explicit PHY soft reset because some chips use the genphy driver
which uses a no-op as soft_reset callback.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 27 +--------------------------
 1 file changed, 1 insertion(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 0fba2581..a466647e 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1441,19 +1441,6 @@ static void rtl8169_irq_mask_and_ack(struct rtl8169_private *tp)
 	RTL_R8(tp, ChipCmd);
 }
 
-static unsigned int rtl8169_xmii_reset_pending(struct rtl8169_private *tp)
-{
-	return rtl_readphy(tp, MII_BMCR) & BMCR_RESET;
-}
-
-static void rtl8169_xmii_reset_enable(struct rtl8169_private *tp)
-{
-	unsigned int val;
-
-	val = rtl_readphy(tp, MII_BMCR) | BMCR_RESET;
-	rtl_writephy(tp, MII_BMCR, val & 0xffff);
-}
-
 static void rtl_link_chg_patch(struct rtl8169_private *tp)
 {
 	struct net_device *dev = tp->dev;
@@ -4259,18 +4246,6 @@ static void rtl_schedule_task(struct rtl8169_private *tp, enum rtl_flag flag)
 		schedule_work(&tp->wk.work);
 }
 
-DECLARE_RTL_COND(rtl_phy_reset_cond)
-{
-	return rtl8169_xmii_reset_pending(tp);
-}
-
-static void rtl8169_phy_reset(struct net_device *dev,
-			      struct rtl8169_private *tp)
-{
-	rtl8169_xmii_reset_enable(tp);
-	rtl_msleep_loop_wait_low(tp, &rtl_phy_reset_cond, 1, 100);
-}
-
 static bool rtl_tbi_enabled(struct rtl8169_private *tp)
 {
 	return (tp->mac_version == RTL_GIGA_MAC_VER_01) &&
@@ -4301,7 +4276,7 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
 		rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
 	}
 
-	rtl8169_phy_reset(dev, tp);
+	genphy_soft_reset(dev->phydev);
 
 	rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
 			  ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
-- 
2.18.0

Re: [PATCH net-next 03/10] r8169: replace open-coded PHY soft reset with genphy_soft_reset

[Andrew Lunn]

On Mon, Jul 02, 2018 at 09:36:58PM +0200, Heiner Kallweit wrote:
> Use genphy_soft_reset() instead of open-coding a PHY soft reset. We have
> to do an explicit PHY soft reset because some chips use the genphy driver
> which uses a no-op as soft_reset callback.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 04/10] r8169: use phy_ethtool_(g|s)et_link_ksettings

[Heiner Kallweit]

Use phy_ethtool_(g|s)et_link_ksettings() for the respective ethtool_ops
callbacks.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 35 +++-------------------------
 1 file changed, 3 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index a466647e..d3a909fb 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1816,35 +1816,6 @@ static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb)
 		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), swab16(opts2 & 0xffff));
 }
 
-static int rtl8169_get_link_ksettings(struct net_device *dev,
-				      struct ethtool_link_ksettings *cmd)
-{
-	struct rtl8169_private *tp = netdev_priv(dev);
-
-	mii_ethtool_get_link_ksettings(&tp->mii, cmd);
-
-	return 0;
-}
-
-static int rtl8169_set_link_ksettings(struct net_device *dev,
-				      const struct ethtool_link_ksettings *cmd)
-{
-	struct rtl8169_private *tp = netdev_priv(dev);
-	int rc;
-	u32 advertising;
-
-	if (!ethtool_convert_link_mode_to_legacy_u32(&advertising,
-	    cmd->link_modes.advertising))
-		return -EINVAL;
-
-	rtl_lock_work(tp);
-	rc = rtl8169_set_speed(dev, cmd->base.autoneg, cmd->base.speed,
-			       cmd->base.duplex, advertising);
-	rtl_unlock_work(tp);
-
-	return rc;
-}
-
 static void rtl8169_get_regs(struct net_device *dev, struct ethtool_regs *regs,
 			     void *p)
 {
@@ -2099,7 +2070,7 @@ static const struct rtl_coalesce_info *rtl_coalesce_info(struct net_device *dev)
 	const struct rtl_coalesce_info *ci;
 	int rc;
 
-	rc = rtl8169_get_link_ksettings(dev, &ecmd);
+	rc = phy_ethtool_get_link_ksettings(dev, &ecmd);
 	if (rc < 0)
 		return ERR_PTR(rc);
 
@@ -2258,8 +2229,8 @@ static const struct ethtool_ops rtl8169_ethtool_ops = {
 	.get_ethtool_stats	= rtl8169_get_ethtool_stats,
 	.get_ts_info		= ethtool_op_get_ts_info,
 	.nway_reset		= rtl8169_nway_reset,
-	.get_link_ksettings	= rtl8169_get_link_ksettings,
-	.set_link_ksettings	= rtl8169_set_link_ksettings,
+	.get_link_ksettings	= phy_ethtool_get_link_ksettings,
+	.set_link_ksettings	= phy_ethtool_set_link_ksettings,
 };
 
 static void rtl8169_get_mac_version(struct rtl8169_private *tp,
-- 
2.18.0

[PATCH net-next 05/10] r8169: use phy_ethtool_nway_reset

[Heiner Kallweit]

Switch to using phy_ethtool_nway_reset().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/Kconfig | 1 -
 drivers/net/ethernet/realtek/r8169.c | 9 +--------
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig
index 7fb1af1f..e1cd934c 100644
--- a/drivers/net/ethernet/realtek/Kconfig
+++ b/drivers/net/ethernet/realtek/Kconfig
@@ -100,7 +100,6 @@ config R8169
 	select FW_LOADER
 	select CRC32
 	select PHYLIB
-	select MII
 	---help---
 	  Say Y here if you have a Realtek 8169 PCI Gigabit Ethernet adapter.
 
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index d3a909fb..6006676b 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1991,13 +1991,6 @@ static void rtl8169_get_strings(struct net_device *dev, u32 stringset, u8 *data)
 	}
 }
 
-static int rtl8169_nway_reset(struct net_device *dev)
-{
-	struct rtl8169_private *tp = netdev_priv(dev);
-
-	return mii_nway_restart(&tp->mii);
-}
-
 /*
  * Interrupt coalescing
  *
@@ -2228,7 +2221,7 @@ static const struct ethtool_ops rtl8169_ethtool_ops = {
 	.get_sset_count		= rtl8169_get_sset_count,
 	.get_ethtool_stats	= rtl8169_get_ethtool_stats,
 	.get_ts_info		= ethtool_op_get_ts_info,
-	.nway_reset		= rtl8169_nway_reset,
+	.nway_reset		= phy_ethtool_nway_reset,
 	.get_link_ksettings	= phy_ethtool_get_link_ksettings,
 	.set_link_ksettings	= phy_ethtool_set_link_ksettings,
 };
-- 
2.18.0

Re: [PATCH net-next 05/10] r8169: use phy_ethtool_nway_reset

[Andrew Lunn]

On Mon, Jul 02, 2018 at 09:37:03PM +0200, Heiner Kallweit wrote:
> Switch to using phy_ethtool_nway_reset().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 06/10] r8169: use phy_mii_ioctl

[Heiner Kallweit]

Switch to using phy_mii_ioctl().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 25 +++----------------------
 1 file changed, 3 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 6006676b..311321ee 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4290,31 +4290,12 @@ static int rtl_set_mac_address(struct net_device *dev, void *p)
 	return 0;
 }
 
-static int rtl_xmii_ioctl(struct rtl8169_private *tp,
-			  struct mii_ioctl_data *data, int cmd)
-{
-	switch (cmd) {
-	case SIOCGMIIPHY:
-		data->phy_id = 32; /* Internal PHY */
-		return 0;
-
-	case SIOCGMIIREG:
-		data->val_out = rtl_readphy(tp, data->reg_num & 0x1f);
-		return 0;
-
-	case SIOCSMIIREG:
-		rtl_writephy(tp, data->reg_num & 0x1f, data->val_in);
-		return 0;
-	}
-	return -EOPNOTSUPP;
-}
-
 static int rtl8169_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 {
-	struct rtl8169_private *tp = netdev_priv(dev);
-	struct mii_ioctl_data *data = if_mii(ifr);
+	if (!netif_running(dev))
+		return -ENODEV;
 
-	return netif_running(dev) ? rtl_xmii_ioctl(tp, data, cmd) : -ENODEV;
+	return phy_mii_ioctl(dev->phydev, ifr, cmd);
 }
 
 static void rtl_init_mdio_ops(struct rtl8169_private *tp)
-- 
2.18.0

Re: [PATCH net-next 06/10] r8169: use phy_mii_ioctl

[Andrew Lunn]

On Mon, Jul 02, 2018 at 09:37:05PM +0200, Heiner Kallweit wrote:
> Switch to using phy_mii_ioctl().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next 07/10] r8169: migrate speed_down function to phylib

[Heiner Kallweit]

Change rtl_speed_down() to use phylib.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 33 +++++++++++++---------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 311321ee..807fbc75 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4240,6 +4240,10 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
 		rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
 	}
 
+	/* We may have called rtl_speed_down before */
+	dev->phydev->advertising = dev->phydev->supported;
+	genphy_config_aneg(dev->phydev);
+
 	genphy_soft_reset(dev->phydev);
 
 	rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
@@ -4323,28 +4327,21 @@ static void rtl_init_mdio_ops(struct rtl8169_private *tp)
 	}
 }
 
+#define BASET10		(ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full)
+#define BASET100	(ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full)
+#define BASET1000	(ADVERTISED_1000baseT_Half | ADVERTISED_1000baseT_Full)
+
 static void rtl_speed_down(struct rtl8169_private *tp)
 {
-	u32 adv;
-	int lpa;
+	struct phy_device *phydev = tp->dev->phydev;
+	u32 adv = phydev->lp_advertising & phydev->supported;
 
-	rtl_writephy(tp, 0x1f, 0x0000);
-	lpa = rtl_readphy(tp, MII_LPA);
+	if (adv & BASET10)
+		phydev->advertising &= ~(BASET100 | BASET1000);
+	else if (adv & BASET100)
+		phydev->advertising &= ~BASET1000;
 
-	if (lpa & (LPA_10HALF | LPA_10FULL))
-		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full;
-	else if (lpa & (LPA_100HALF | LPA_100FULL))
-		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
-		      ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full;
-	else
-		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
-		      ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
-		      (tp->mii.supports_gmii ?
-		       ADVERTISED_1000baseT_Half |
-		       ADVERTISED_1000baseT_Full : 0);
-
-	rtl8169_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
-			  adv);
+	genphy_config_aneg(phydev);
 }
 
 static void rtl_wol_suspend_quirk(struct rtl8169_private *tp)
-- 
2.18.0

Re: [PATCH net-next 07/10] r8169: migrate speed_down function to phylib

[Andrew Lunn]

 On Mon, Jul 02, 2018 at 09:37:08PM +0200, Heiner Kallweit wrote:
> Change rtl_speed_down() to use phylib.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/ethernet/realtek/r8169.c | 33 +++++++++++++---------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 311321ee..807fbc75 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -4240,6 +4240,10 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
>  		rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
>  	}
>  
> +	/* We may have called rtl_speed_down before */
> +	dev->phydev->advertising = dev->phydev->supported;
> +	genphy_config_aneg(dev->phydev);
> +
>  	genphy_soft_reset(dev->phydev);
>  
>  	rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
> @@ -4323,28 +4327,21 @@ static void rtl_init_mdio_ops(struct rtl8169_private *tp)
>  	}
>  }
>  
> +#define BASET10		(ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full)
> +#define BASET100	(ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full)
> +#define BASET1000	(ADVERTISED_1000baseT_Half | ADVERTISED_1000baseT_Full)
> +
>  static void rtl_speed_down(struct rtl8169_private *tp)
>  {
> -	u32 adv;
> -	int lpa;
> +	struct phy_device *phydev = tp->dev->phydev;
> +	u32 adv = phydev->lp_advertising & phydev->supported;
>  
> -	rtl_writephy(tp, 0x1f, 0x0000);
> -	lpa = rtl_readphy(tp, MII_LPA);
> +	if (adv & BASET10)
> +		phydev->advertising &= ~(BASET100 | BASET1000);
> +	else if (adv & BASET100)
> +		phydev->advertising &= ~BASET1000;
>  
> -	if (lpa & (LPA_10HALF | LPA_10FULL))
> -		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full;
> -	else if (lpa & (LPA_100HALF | LPA_100FULL))
> -		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
> -		      ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full;
> -	else
> -		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
> -		      ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
> -		      (tp->mii.supports_gmii ?
> -		       ADVERTISED_1000baseT_Half |
> -		       ADVERTISED_1000baseT_Full : 0);
> -
> -	rtl8169_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
> -			  adv);
> +	genphy_config_aneg(phydev);
>  }

It probably it is me being too tired, but i don't get what this is
doing? Changing the local advertisement based on what the remote is
advertising. Why?

	Andrew

Re: [PATCH net-next 07/10] r8169: migrate speed_down function to phylib

[Heiner Kallweit]

On 02.07.2018 23:20, Andrew Lunn wrote:
>  On Mon, Jul 02, 2018 at 09:37:08PM +0200, Heiner Kallweit wrote:
>> Change rtl_speed_down() to use phylib.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/ethernet/realtek/r8169.c | 33 +++++++++++++---------------
>>  1 file changed, 15 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
>> index 311321ee..807fbc75 100644
>> --- a/drivers/net/ethernet/realtek/r8169.c
>> +++ b/drivers/net/ethernet/realtek/r8169.c
>> @@ -4240,6 +4240,10 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
>>  		rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
>>  	}
>>  
>> +	/* We may have called rtl_speed_down before */
>> +	dev->phydev->advertising = dev->phydev->supported;
>> +	genphy_config_aneg(dev->phydev);
>> +
>>  	genphy_soft_reset(dev->phydev);
>>  
>>  	rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
>> @@ -4323,28 +4327,21 @@ static void rtl_init_mdio_ops(struct rtl8169_private *tp)
>>  	}
>>  }
>>  
>> +#define BASET10		(ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full)
>> +#define BASET100	(ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full)
>> +#define BASET1000	(ADVERTISED_1000baseT_Half | ADVERTISED_1000baseT_Full)
>> +
>>  static void rtl_speed_down(struct rtl8169_private *tp)
>>  {
>> -	u32 adv;
>> -	int lpa;
>> +	struct phy_device *phydev = tp->dev->phydev;
>> +	u32 adv = phydev->lp_advertising & phydev->supported;
>>  
>> -	rtl_writephy(tp, 0x1f, 0x0000);
>> -	lpa = rtl_readphy(tp, MII_LPA);
>> +	if (adv & BASET10)
>> +		phydev->advertising &= ~(BASET100 | BASET1000);
>> +	else if (adv & BASET100)
>> +		phydev->advertising &= ~BASET1000;
>>  
>> -	if (lpa & (LPA_10HALF | LPA_10FULL))
>> -		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full;
>> -	else if (lpa & (LPA_100HALF | LPA_100FULL))
>> -		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
>> -		      ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full;
>> -	else
>> -		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
>> -		      ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
>> -		      (tp->mii.supports_gmii ?
>> -		       ADVERTISED_1000baseT_Half |
>> -		       ADVERTISED_1000baseT_Full : 0);
>> -
>> -	rtl8169_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
>> -			  adv);
>> +	genphy_config_aneg(phydev);
>>  }
> 
> It probably it is me being too tired, but i don't get what this is
> doing? Changing the local advertisement based on what the remote is
> advertising. Why?
> 
It also took me some time to understand what this speed_down is doing.
If we suspend and wait for a WoL packet, then we don't have to burn all
the energy for a GBit connection. Therefore we switch to the lowest
speed supported by chip and link partner. This is done by removing
higher speeds from the advertised modes and restarting an autonego.

> 	Andrew
> 

Re: [PATCH net-next 07/10] r8169: migrate speed_down function to phylib

[Florian Fainelli]

On 07/02/2018 02:31 PM, Heiner Kallweit wrote:
> On 02.07.2018 23:20, Andrew Lunn wrote:
>>  On Mon, Jul 02, 2018 at 09:37:08PM +0200, Heiner Kallweit wrote:
>>> Change rtl_speed_down() to use phylib.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/net/ethernet/realtek/r8169.c | 33 +++++++++++++---------------
>>>  1 file changed, 15 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
>>> index 311321ee..807fbc75 100644
>>> --- a/drivers/net/ethernet/realtek/r8169.c
>>> +++ b/drivers/net/ethernet/realtek/r8169.c
>>> @@ -4240,6 +4240,10 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
>>>  		rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
>>>  	}
>>>  
>>> +	/* We may have called rtl_speed_down before */
>>> +	dev->phydev->advertising = dev->phydev->supported;
>>> +	genphy_config_aneg(dev->phydev);
>>> +
>>>  	genphy_soft_reset(dev->phydev);
>>>  
>>>  	rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
>>> @@ -4323,28 +4327,21 @@ static void rtl_init_mdio_ops(struct rtl8169_private *tp)
>>>  	}
>>>  }
>>>  
>>> +#define BASET10		(ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full)
>>> +#define BASET100	(ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full)
>>> +#define BASET1000	(ADVERTISED_1000baseT_Half | ADVERTISED_1000baseT_Full)
>>> +
>>>  static void rtl_speed_down(struct rtl8169_private *tp)
>>>  {
>>> -	u32 adv;
>>> -	int lpa;
>>> +	struct phy_device *phydev = tp->dev->phydev;
>>> +	u32 adv = phydev->lp_advertising & phydev->supported;
>>>  
>>> -	rtl_writephy(tp, 0x1f, 0x0000);
>>> -	lpa = rtl_readphy(tp, MII_LPA);
>>> +	if (adv & BASET10)
>>> +		phydev->advertising &= ~(BASET100 | BASET1000);
>>> +	else if (adv & BASET100)
>>> +		phydev->advertising &= ~BASET1000;
>>>  
>>> -	if (lpa & (LPA_10HALF | LPA_10FULL))
>>> -		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full;
>>> -	else if (lpa & (LPA_100HALF | LPA_100FULL))
>>> -		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
>>> -		      ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full;
>>> -	else
>>> -		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
>>> -		      ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
>>> -		      (tp->mii.supports_gmii ?
>>> -		       ADVERTISED_1000baseT_Half |
>>> -		       ADVERTISED_1000baseT_Full : 0);
>>> -
>>> -	rtl8169_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
>>> -			  adv);
>>> +	genphy_config_aneg(phydev);
>>>  }
>>
>> It probably it is me being too tired, but i don't get what this is
>> doing? Changing the local advertisement based on what the remote is
>> advertising. Why?
>>
> It also took me some time to understand what this speed_down is doing.
> If we suspend and wait for a WoL packet, then we don't have to burn all
> the energy for a GBit connection. Therefore we switch to the lowest
> speed supported by chip and link partner. This is done by removing
> higher speeds from the advertised modes and restarting an autonego.

This is something that the tg3 driver also does, we should probably
consider doing this as part of a generic PHY library helpers since I was
told by several HW engineers that usually 10Mbits for WoL is much more
energy efficient.

One thing that bothers me a bit is that this should ideally be offered
as both blocking and non-blocking options, because we might want to make
sure that at the time we suspend, and we already had a link established,
we successfully re-negotiate the link with the partner. I agree that
there could be any sort of link disruption happening at any point though..
-- 
Florian

Re: [PATCH net-next 07/10] r8169: migrate speed_down function to phylib

[Heiner Kallweit]

On 03.07.2018 18:48, Florian Fainelli wrote:
> 
> 
> On 07/02/2018 02:31 PM, Heiner Kallweit wrote:
>> On 02.07.2018 23:20, Andrew Lunn wrote:
>>>  On Mon, Jul 02, 2018 at 09:37:08PM +0200, Heiner Kallweit wrote:
>>>> Change rtl_speed_down() to use phylib.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>>  drivers/net/ethernet/realtek/r8169.c | 33 +++++++++++++---------------
>>>>  1 file changed, 15 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
>>>> index 311321ee..807fbc75 100644
>>>> --- a/drivers/net/ethernet/realtek/r8169.c
>>>> +++ b/drivers/net/ethernet/realtek/r8169.c
>>>> @@ -4240,6 +4240,10 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
>>>>  		rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
>>>>  	}
>>>>  
>>>> +	/* We may have called rtl_speed_down before */
>>>> +	dev->phydev->advertising = dev->phydev->supported;
>>>> +	genphy_config_aneg(dev->phydev);
>>>> +
>>>>  	genphy_soft_reset(dev->phydev);
>>>>  
>>>>  	rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
>>>> @@ -4323,28 +4327,21 @@ static void rtl_init_mdio_ops(struct rtl8169_private *tp)
>>>>  	}
>>>>  }
>>>>  
>>>> +#define BASET10		(ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full)
>>>> +#define BASET100	(ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full)
>>>> +#define BASET1000	(ADVERTISED_1000baseT_Half | ADVERTISED_1000baseT_Full)
>>>> +
>>>>  static void rtl_speed_down(struct rtl8169_private *tp)
>>>>  {
>>>> -	u32 adv;
>>>> -	int lpa;
>>>> +	struct phy_device *phydev = tp->dev->phydev;
>>>> +	u32 adv = phydev->lp_advertising & phydev->supported;
>>>>  
>>>> -	rtl_writephy(tp, 0x1f, 0x0000);
>>>> -	lpa = rtl_readphy(tp, MII_LPA);
>>>> +	if (adv & BASET10)
>>>> +		phydev->advertising &= ~(BASET100 | BASET1000);
>>>> +	else if (adv & BASET100)
>>>> +		phydev->advertising &= ~BASET1000;
>>>>  
>>>> -	if (lpa & (LPA_10HALF | LPA_10FULL))
>>>> -		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full;
>>>> -	else if (lpa & (LPA_100HALF | LPA_100FULL))
>>>> -		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
>>>> -		      ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full;
>>>> -	else
>>>> -		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
>>>> -		      ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
>>>> -		      (tp->mii.supports_gmii ?
>>>> -		       ADVERTISED_1000baseT_Half |
>>>> -		       ADVERTISED_1000baseT_Full : 0);
>>>> -
>>>> -	rtl8169_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
>>>> -			  adv);
>>>> +	genphy_config_aneg(phydev);
>>>>  }
>>>
>>> It probably it is me being too tired, but i don't get what this is
>>> doing? Changing the local advertisement based on what the remote is
>>> advertising. Why?
>>>
>> It also took me some time to understand what this speed_down is doing.
>> If we suspend and wait for a WoL packet, then we don't have to burn all
>> the energy for a GBit connection. Therefore we switch to the lowest
>> speed supported by chip and link partner. This is done by removing
>> higher speeds from the advertised modes and restarting an autonego.
> 
> This is something that the tg3 driver also does, we should probably
> consider doing this as part of a generic PHY library helpers since I was
> told by several HW engineers that usually 10Mbits for WoL is much more
> energy efficient.
> 
Yes, I agree this should become part of phylib. I'd prefer to do it
after this r8169 patch series, will spend a few thoughts on how to
do it best, also considering your remark below.

Heiner

> One thing that bothers me a bit is that this should ideally be offered
> as both blocking and non-blocking options, because we might want to make
> sure that at the time we suspend, and we already had a link established,
> we successfully re-negotiate the link with the partner. I agree that
> there could be any sort of link disruption happening at any point though..
> 

[PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii

[Heiner Kallweit]

We can remove rtl8169_set_speed_xmii() now that phylib handles all this.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 90 ----------------------------
 1 file changed, 90 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 807fbc75..b696b83d 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1670,89 +1670,6 @@ static int rtl8169_get_regs_len(struct net_device *dev)
 	return R8169_REGS_SIZE;
 }
 
-static int rtl8169_set_speed_xmii(struct net_device *dev,
-				  u8 autoneg, u16 speed, u8 duplex, u32 adv)
-{
-	struct rtl8169_private *tp = netdev_priv(dev);
-	int giga_ctrl, bmcr;
-	int rc = -EINVAL;
-
-	rtl_writephy(tp, 0x1f, 0x0000);
-
-	if (autoneg == AUTONEG_ENABLE) {
-		int auto_nego;
-
-		auto_nego = rtl_readphy(tp, MII_ADVERTISE);
-		auto_nego &= ~(ADVERTISE_10HALF | ADVERTISE_10FULL |
-				ADVERTISE_100HALF | ADVERTISE_100FULL);
-
-		if (adv & ADVERTISED_10baseT_Half)
-			auto_nego |= ADVERTISE_10HALF;
-		if (adv & ADVERTISED_10baseT_Full)
-			auto_nego |= ADVERTISE_10FULL;
-		if (adv & ADVERTISED_100baseT_Half)
-			auto_nego |= ADVERTISE_100HALF;
-		if (adv & ADVERTISED_100baseT_Full)
-			auto_nego |= ADVERTISE_100FULL;
-
-		auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
-
-		giga_ctrl = rtl_readphy(tp, MII_CTRL1000);
-		giga_ctrl &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
-
-		/* The 8100e/8101e/8102e do Fast Ethernet only. */
-		if (tp->mii.supports_gmii) {
-			if (adv & ADVERTISED_1000baseT_Half)
-				giga_ctrl |= ADVERTISE_1000HALF;
-			if (adv & ADVERTISED_1000baseT_Full)
-				giga_ctrl |= ADVERTISE_1000FULL;
-		} else if (adv & (ADVERTISED_1000baseT_Half |
-				  ADVERTISED_1000baseT_Full)) {
-			netif_info(tp, link, dev,
-				   "PHY does not support 1000Mbps\n");
-			goto out;
-		}
-
-		bmcr = BMCR_ANENABLE | BMCR_ANRESTART;
-
-		rtl_writephy(tp, MII_ADVERTISE, auto_nego);
-		rtl_writephy(tp, MII_CTRL1000, giga_ctrl);
-	} else {
-		if (speed == SPEED_10)
-			bmcr = 0;
-		else if (speed == SPEED_100)
-			bmcr = BMCR_SPEED100;
-		else
-			goto out;
-
-		if (duplex == DUPLEX_FULL)
-			bmcr |= BMCR_FULLDPLX;
-	}
-
-	rtl_writephy(tp, MII_BMCR, bmcr);
-
-	if (tp->mac_version == RTL_GIGA_MAC_VER_02 ||
-	    tp->mac_version == RTL_GIGA_MAC_VER_03) {
-		if ((speed == SPEED_100) && (autoneg != AUTONEG_ENABLE)) {
-			rtl_writephy(tp, 0x17, 0x2138);
-			rtl_writephy(tp, 0x0e, 0x0260);
-		} else {
-			rtl_writephy(tp, 0x17, 0x2108);
-			rtl_writephy(tp, 0x0e, 0x0000);
-		}
-	}
-
-	rc = 0;
-out:
-	return rc;
-}
-
-static int rtl8169_set_speed(struct net_device *dev,
-			     u8 autoneg, u16 speed, u8 duplex, u32 advertising)
-{
-	return rtl8169_set_speed_xmii(dev, autoneg, speed, duplex, advertising);
-}
-
 static netdev_features_t rtl8169_fix_features(struct net_device *dev,
 	netdev_features_t features)
 {
@@ -4245,13 +4162,6 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
 	genphy_config_aneg(dev->phydev);
 
 	genphy_soft_reset(dev->phydev);
-
-	rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
-			  ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
-			  ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
-			  (tp->mii.supports_gmii ?
-			   ADVERTISED_1000baseT_Half |
-			   ADVERTISED_1000baseT_Full : 0));
 }
 
 static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
-- 
2.18.0

Re: [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii

[Andrew Lunn]

> -		auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;

This bit you probably want to keep. The PHY never says it support
Pause. The MAC needs to enable pause if the MAC supports pause.

       Andrew

Re: [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii

[Heiner Kallweit]

On 02.07.2018 23:21, Andrew Lunn wrote:
>> -		auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
> 
> This bit you probably want to keep. The PHY never says it support
> Pause. The MAC needs to enable pause if the MAC supports pause.
> 
Actually I assumed that phylib would do this for me. But:
In phy_probe() first phydev->supported is copied to
phydev->advertising, and only after this both pause flags are added
to phydev->supported. Therefore I think they are not advertised.
Is this intentional? It sounds a little weird to me to add the
pause flags to the supported features per default, but not
advertise them.
Except e.g. we call by chance phy_set_max_speed(), which copies
phydev->supported to phydev->advertising after having adjusted
the supported speeds.

If this is not a bug, then where would be the right place to add
the pause flags to phydev->advertising?

>        Andrew
> 
Heiner

Re: [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii

[Andrew Lunn]

On Mon, Jul 02, 2018 at 11:54:54PM +0200, Heiner Kallweit wrote:
> On 02.07.2018 23:21, Andrew Lunn wrote:
> >> -		auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
> > 
> > This bit you probably want to keep. The PHY never says it support
> > Pause. The MAC needs to enable pause if the MAC supports pause.
> > 
> Actually I assumed that phylib would do this for me. But:
> In phy_probe() first phydev->supported is copied to
> phydev->advertising, and only after this both pause flags are added
> to phydev->supported. Therefore I think they are not advertised.
> Is this intentional? It sounds a little weird to me to add the
> pause flags to the supported features per default, but not
> advertise them.

phylib has no idea if the MAC supports Pause. So it should not enable
it by default. The MAC needs to enable it. And a lot of MAC drivers
get this wrong...

> Except e.g. we call by chance phy_set_max_speed(), which copies
> phydev->supported to phydev->advertising after having adjusted
> the supported speeds.

As you correctly pointed out, phy_set_max_speed() is masking out too
much.

> If this is not a bug, then where would be the right place to add
> the pause flags to phydev->advertising?

Before you call phy_start().

       Andrew

Re: [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii

[Heiner Kallweit]

On 04.07.2018 16:46, Andrew Lunn wrote:
> On Mon, Jul 02, 2018 at 11:54:54PM +0200, Heiner Kallweit wrote:
>> On 02.07.2018 23:21, Andrew Lunn wrote:
>>>> -		auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
>>>
>>> This bit you probably want to keep. The PHY never says it support
>>> Pause. The MAC needs to enable pause if the MAC supports pause.
>>>
>> Actually I assumed that phylib would do this for me. But:
>> In phy_probe() first phydev->supported is copied to
>> phydev->advertising, and only after this both pause flags are added
>> to phydev->supported. Therefore I think they are not advertised.
>> Is this intentional? It sounds a little weird to me to add the
>> pause flags to the supported features per default, but not
>> advertise them.
> 
> phylib has no idea if the MAC supports Pause. So it should not enable
> it by default. The MAC needs to enable it. And a lot of MAC drivers
> get this wrong...
> 
>> Except e.g. we call by chance phy_set_max_speed(), which copies
>> phydev->supported to phydev->advertising after having adjusted
>> the supported speeds.
> 
> As you correctly pointed out, phy_set_max_speed() is masking out too
> much.
> 
>> If this is not a bug, then where would be the right place to add
>> the pause flags to phydev->advertising?
> 
> Before you call phy_start().
> 
Thanks for the clarification. I think beginning of next week I can
provide a v2 of the patch series.

Heiner


>        Andrew
> 

[PATCH net-next 09/10] r8169: remove mii_if_info member from struct rtl8169_private

[Heiner Kallweit]

The only remaining usage of the struct mii_if_info member is to store the
information whether the chip is GMII-capable. So we can replace it with
a simple flag.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 38 +++++-----------------------
 1 file changed, 7 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index b696b83d..48c0e77c 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -15,7 +15,6 @@
 #include <linux/etherdevice.h>
 #include <linux/delay.h>
 #include <linux/ethtool.h>
-#include <linux/mii.h>
 #include <linux/phy.h>
 #include <linux/if_vlan.h>
 #include <linux/crc32.h>
@@ -754,7 +753,7 @@ struct rtl8169_private {
 		struct work_struct work;
 	} wk;
 
-	struct mii_if_info mii;
+	unsigned supports_gmii:1;
 	struct mii_bus *mii_bus;
 	dma_addr_t counters_phys_addr;
 	struct rtl8169_counters *counters;
@@ -1106,21 +1105,6 @@ static void rtl_w0w1_phy(struct rtl8169_private *tp, int reg_addr, int p, int m)
 	rtl_writephy(tp, reg_addr, (val & ~m) | p);
 }
 
-static void rtl_mdio_write(struct net_device *dev, int phy_id, int location,
-			   int val)
-{
-	struct rtl8169_private *tp = netdev_priv(dev);
-
-	rtl_writephy(tp, location, val);
-}
-
-static int rtl_mdio_read(struct net_device *dev, int phy_id, int location)
-{
-	struct rtl8169_private *tp = netdev_priv(dev);
-
-	return rtl_readphy(tp, location);
-}
-
 DECLARE_RTL_COND(rtl_ephyar_cond)
 {
 	return RTL_R32(tp, EPHYAR) & EPHYAR_FLAG;
@@ -2253,15 +2237,15 @@ static void rtl8169_get_mac_version(struct rtl8169_private *tp,
 			   "unknown MAC, using family default\n");
 		tp->mac_version = default_version;
 	} else if (tp->mac_version == RTL_GIGA_MAC_VER_42) {
-		tp->mac_version = tp->mii.supports_gmii ?
+		tp->mac_version = tp->supports_gmii ?
 				  RTL_GIGA_MAC_VER_42 :
 				  RTL_GIGA_MAC_VER_43;
 	} else if (tp->mac_version == RTL_GIGA_MAC_VER_45) {
-		tp->mac_version = tp->mii.supports_gmii ?
+		tp->mac_version = tp->supports_gmii ?
 				  RTL_GIGA_MAC_VER_45 :
 				  RTL_GIGA_MAC_VER_47;
 	} else if (tp->mac_version == RTL_GIGA_MAC_VER_46) {
-		tp->mac_version = tp->mii.supports_gmii ?
+		tp->mac_version = tp->supports_gmii ?
 				  RTL_GIGA_MAC_VER_46 :
 				  RTL_GIGA_MAC_VER_48;
 	}
@@ -6714,14 +6698,14 @@ static int r8169_phy_connect(struct rtl8169_private *tp)
 	phy_interface_t phy_mode;
 	int ret;
 
-	phy_mode = tp->mii.supports_gmii ? PHY_INTERFACE_MODE_GMII :
+	phy_mode = tp->supports_gmii ? PHY_INTERFACE_MODE_GMII :
 		   PHY_INTERFACE_MODE_MII;
 
 	phydev = mdiobus_get_phy(tp->mii_bus, 0);
 	if (!phydev)
 		return -ENODEV;
 
-	if (!tp->mii.supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
+	if (!tp->supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
 		netif_info(tp, probe, tp->dev, "Restrict PHY to 100Mbit because MAC doesn't support 1GBit\n");
 		phy_set_max_speed(phydev, SPEED_100);
 	}
@@ -7317,7 +7301,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data;
 	struct rtl8169_private *tp;
-	struct mii_if_info *mii;
 	struct net_device *dev;
 	int chipset, region, i;
 	int rc;
@@ -7337,14 +7320,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	tp->dev = dev;
 	tp->pci_dev = pdev;
 	tp->msg_enable = netif_msg_init(debug.msg_enable, R8169_MSG_DEFAULT);
-
-	mii = &tp->mii;
-	mii->dev = dev;
-	mii->mdio_read = rtl_mdio_read;
-	mii->mdio_write = rtl_mdio_write;
-	mii->phy_id_mask = 0x1f;
-	mii->reg_num_mask = 0x1f;
-	mii->supports_gmii = cfg->has_gmii;
+	tp->supports_gmii = cfg->has_gmii;
 
 	/* enable device (incl. PCI PM wakeup and hotplug setup) */
 	rc = pcim_enable_device(pdev);
-- 
2.18.0

[PATCH net-next 10/10] r8169: don't read chip phy status register

[Heiner Kallweit]

Instead of accessing the PHYstatus register we can use the information
phylib stores in the phy_device structure.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 48c0e77c..7b7de596 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1428,18 +1428,19 @@ static void rtl8169_irq_mask_and_ack(struct rtl8169_private *tp)
 static void rtl_link_chg_patch(struct rtl8169_private *tp)
 {
 	struct net_device *dev = tp->dev;
+	struct phy_device *phydev = dev->phydev;
 
 	if (!netif_running(dev))
 		return;
 
 	if (tp->mac_version == RTL_GIGA_MAC_VER_34 ||
 	    tp->mac_version == RTL_GIGA_MAC_VER_38) {
-		if (RTL_R8(tp, PHYstatus) & _1000bpsF) {
+		if (phydev->speed == SPEED_1000) {
 			rtl_eri_write(tp, 0x1bc, ERIAR_MASK_1111, 0x00000011,
 				      ERIAR_EXGMAC);
 			rtl_eri_write(tp, 0x1dc, ERIAR_MASK_1111, 0x00000005,
 				      ERIAR_EXGMAC);
-		} else if (RTL_R8(tp, PHYstatus) & _100bps) {
+		} else if (phydev->speed == SPEED_100) {
 			rtl_eri_write(tp, 0x1bc, ERIAR_MASK_1111, 0x0000001f,
 				      ERIAR_EXGMAC);
 			rtl_eri_write(tp, 0x1dc, ERIAR_MASK_1111, 0x00000005,
@@ -1457,7 +1458,7 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp)
 			     ERIAR_EXGMAC);
 	} else if (tp->mac_version == RTL_GIGA_MAC_VER_35 ||
 		   tp->mac_version == RTL_GIGA_MAC_VER_36) {
-		if (RTL_R8(tp, PHYstatus) & _1000bpsF) {
+		if (phydev->speed == SPEED_1000) {
 			rtl_eri_write(tp, 0x1bc, ERIAR_MASK_1111, 0x00000011,
 				      ERIAR_EXGMAC);
 			rtl_eri_write(tp, 0x1dc, ERIAR_MASK_1111, 0x00000005,
@@ -1469,7 +1470,7 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp)
 				      ERIAR_EXGMAC);
 		}
 	} else if (tp->mac_version == RTL_GIGA_MAC_VER_37) {
-		if (RTL_R8(tp, PHYstatus) & _10bps) {
+		if (phydev->speed == SPEED_10) {
 			rtl_eri_write(tp, 0x1d0, ERIAR_MASK_0011, 0x4d02,
 				      ERIAR_EXGMAC);
 			rtl_eri_write(tp, 0x1dc, ERIAR_MASK_0011, 0x0060,
-- 
2.18.0