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

[PATCH net-next v2 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

Changes in v2:
- return error in mdio ops if phyaddr > 0
- advertise pause modes
- added reviewed-by for several patches

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 | 466 ++++++++++-----------------
 2 files changed, 164 insertions(+), 304 deletions(-)


-- 
2.18.0

[PATCH net-next v2 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>
---
v2:
- return error in mdio ops if phyaddr > 0
- advertise pause modes
---
 drivers/net/ethernet/realtek/Kconfig |   1 +
 drivers/net/ethernet/realtek/r8169.c | 158 +++++++++++++++++++++------
 2 files changed, 127 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 d598fdf0..d1951f32 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)
 
 static u32 __rtl8169_get_wol(struct rtl8169_private *tp)
@@ -6221,7 +6199,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)
@@ -6838,7 +6815,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);
 }
@@ -6920,10 +6897,59 @@ 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;
+
+	ret = phy_connect_direct(tp->dev, phydev, r8169_phylink_handler,
+				 phy_mode);
+	if (ret)
+		return ret;
+
+	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);
+	}
+
+	/* Ensure to advertise everything, incl. pause */
+	phydev->advertising = phydev->supported;
+
+	phy_attached_info(phydev);
+
+	return 0;
+}
+
 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);
 
@@ -6963,6 +6989,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,
@@ -7023,6 +7051,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);
@@ -7038,16 +7070,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);
@@ -7126,6 +7159,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);
 
@@ -7158,6 +7192,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);
@@ -7303,6 +7339,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);
 
@@ -7388,6 +7425,57 @@ 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;
+
+	if (phyaddr > 0)
+		return -EINVAL;
+
+	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;
+
+	if (phyaddr > 0)
+		return -EINVAL;
+
+	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;
@@ -7644,10 +7732,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),
@@ -7662,12 +7754,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

[PATCH net-next v2 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>
---
v2:
- no changes
---
 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 d1951f32..e22d4e64 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4450,47 +4450,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))
@@ -4503,7 +4462,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:
@@ -4556,7 +4516,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

[PATCH net-next v2 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>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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 e22d4e64..d5133fde 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;
@@ -4252,18 +4239,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) &&
@@ -4294,7 +4269,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

[PATCH net-next v2 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>
---
v2:
- no changes
---
 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 d5133fde..5282e413 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1809,35 +1809,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)
 {
@@ -2092,7 +2063,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);
 
@@ -2251,8 +2222,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 v2 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>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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 5282e413..f0aa1b54 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1984,13 +1984,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
  *
@@ -2221,7 +2214,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

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

[Heiner Kallweit]

Switch to using phy_mii_ioctl().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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 f0aa1b54..deed477e 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4283,31 +4283,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

[PATCH net-next v2 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 deed477e..1eb761a8 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4233,6 +4233,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,
@@ -4316,28 +4320,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

[PATCH net-next v2 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>
---
v2:
- no changes
---
 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 1eb761a8..3a13c1da 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1663,89 +1663,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)
 {
@@ -4238,13 +4155,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

[PATCH net-next v2 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>
---
v2:
- no changes
--
 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 3a13c1da..6600edfb 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;
@@ -2246,15 +2230,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;
 	}
@@ -6707,7 +6691,7 @@ 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);
@@ -6719,7 +6703,7 @@ static int r8169_phy_connect(struct rtl8169_private *tp)
 	if (ret)
 		return ret;
 
-	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);
@@ -7322,7 +7306,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;
@@ -7342,14 +7325,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 v2 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>
---
v2:
- no changes
---
 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 6600edfb..25a834fa 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

Re: [PATCH net-next v2 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;
> +
> +	if (phyaddr > 0)
> +		return -EINVAL;

Please use ENODEV.

The mdio bus is scanned for devices in __mdiobus_register(). If
mdiobus_scan() returns -ENODEV, it is not considered an error, and it
will continue scanning other addresses on the bus. Any other error is
a real error, and will abort the scan. That will probably abort the
bus registration.

> +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;

Once your handling of addr > 0 is correct, you don't need this.  Let
is scan all addresses, just like a normal MDIO bus. The more we can
keep it normal, the better.

     Andrew

Re: [PATCH net-next v2 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))
> @@ -4503,7 +4462,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));

I don't particularly like this, because no other MAC driver does it.

Why is it powered up, but not connected? Is it powered down before it
is disconnected? Is it the bootloader which is powering it up?

   Andrew

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

[Heiner Kallweit]

On 10.07.2018 21:10, 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;
>> +
>> +	if (phyaddr > 0)
>> +		return -EINVAL;
> 
> Please use ENODEV.
> 
> The mdio bus is scanned for devices in __mdiobus_register(). If
> mdiobus_scan() returns -ENODEV, it is not considered an error, and it
> will continue scanning other addresses on the bus. Any other error is
> a real error, and will abort the scan. That will probably abort the
> bus registration.
> 
OK

>> +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;
> 
> Once your handling of addr > 0 is correct, you don't need this.  Let
> is scan all addresses, just like a normal MDIO bus. The more we can
> keep it normal, the better.
> 
Sounds good. I'll wait for feedback on other patches of the
series and then prepare a v3.

Heiner

>      Andrew
> 

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

[Heiner Kallweit]

On 10.07.2018 21:15, Andrew Lunn wrote:
>>  static void r8168_pll_power_down(struct rtl8169_private *tp)
>>  {
>>  	if (r8168_check_dash(tp))
>> @@ -4503,7 +4462,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));
> 
> I don't particularly like this, because no other MAC driver does it.
> 
I have to agree, it doesn't look too nice.
In general quite few network drivers seem to use runtime pm.

> Why is it powered up, but not connected? Is it powered down before it
> is disconnected? Is it the bootloader which is powering it up?
> 
Exactly, if the device is active when driver is loaded and the
interface isn't used and therefore not brought up, then, when runtime-
suspending, we face this situation.

This could be changed by connecting the PHY in probe() already instead
of doing it in open(). I had this in the beginning, based on a
recommendation from Florian or you (don't remember) I changed this to
connect in open() only.

Do you have any preference or see a good way to deal with the situation?

Heiner

>    Andrew
> 

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

[Andrew Lunn]

On Tue, Jul 10, 2018 at 08:39:32PM +0200, Heiner Kallweit wrote:
> Use phy_ethtool_(g|s)et_link_ksettings() for the respective ethtool_ops
> callbacks.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

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

    Andrew

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

[Andrew Lunn]

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

Please use phy_set_max_speed(). We need MAC drivers to stop directly
accessing phydev members. Otherwise we are going to have problems
supporting 2.5G PHYs.

	   Andrew

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

[Andrew Lunn]

> > Why is it powered up, but not connected? Is it powered down before it
> > is disconnected? Is it the bootloader which is powering it up?
> > 
> Exactly, if the device is active when driver is loaded and the
> interface isn't used and therefore not brought up, then, when runtime-
> suspending, we face this situation.

Well, it is more complex than that. If the interface is not used, why
bother even loading the MAC driver?

If you are trying to make a really low power system, the bootloader
also needs to be involved. It needs to shut down anything it starts
before handing over control the linux.

Another approach is to handle this in phylib. When the mdio bus is
suspended, look for any PHYs which are not connected and power them
down. The assumption being, any MAC driver using WoL via a PHY does
not disconnect the PHY before suspending.

    Andrew

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

[Andrew Lunn]

On Tue, Jul 10, 2018 at 08:40:00PM +0200, Heiner Kallweit wrote:
> 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>

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

    Andrew

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

[Andrew Lunn]

> @@ -6719,7 +6703,7 @@ static int r8169_phy_connect(struct rtl8169_private *tp)
>  	if (ret)
>  		return ret;
>  
> -	if (!tp->mii.supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
> +	if (!tp->supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {

It is better to use phy_set_max_speed() with SPEED_100 if
tp->supports_gmii is not set.

	  Andrew

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

[Heiner Kallweit]

On 10.07.2018 22:44, Andrew Lunn wrote:
>>  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;
> 
> Please use phy_set_max_speed(). We need MAC drivers to stop directly
> accessing phydev members. Otherwise we are going to have problems
> supporting 2.5G PHYs.
> 
The issue I see with phy_set_max_speed() is that it also adjusts
phydev->supported and I don't think that's what we want.
What the chip supports doesn't change, just what we want to
advertise.

> 	   Andrew
> 

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

[Heiner Kallweit]

On 10.07.2018 23:00, Andrew Lunn wrote:
>> @@ -6719,7 +6703,7 @@ static int r8169_phy_connect(struct rtl8169_private *tp)
>>  	if (ret)
>>  		return ret;
>>  
>> -	if (!tp->mii.supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
>> +	if (!tp->supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
> 
> It is better to use phy_set_max_speed() with SPEED_100 if
> tp->supports_gmii is not set.
> 
Then the info message wouldn't fit any longer, it's meaningful in case
1GBit PHY + 100MBit MAC only.

> 	  Andrew
> 

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

[Andrew Lunn]

On Tue, Jul 10, 2018 at 11:32:36PM +0200, Heiner Kallweit wrote:
> On 10.07.2018 23:00, Andrew Lunn wrote:
> >> @@ -6719,7 +6703,7 @@ static int r8169_phy_connect(struct rtl8169_private *tp)
> >>  	if (ret)
> >>  		return ret;
> >>  
> >> -	if (!tp->mii.supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
> >> +	if (!tp->supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
> > 
> > It is better to use phy_set_max_speed() with SPEED_100 if
> > tp->supports_gmii is not set.
> > 
> Then the info message wouldn't fit any longer, it's meaningful in case
> 1GBit PHY + 100MBit MAC only.

What you should avoid is this part:

phydev->supported & PHY_1000BT_FEATURES

Because phydev->supported is going to go away sometime soon. 

Does anybody care about knowing they have a 1Gbit PHY connected to the
100Mbit MAC? So long as it works, i think not.

	Andrew

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

[Andrew Lunn]

On Tue, Jul 10, 2018 at 11:26:13PM +0200, Heiner Kallweit wrote:
> On 10.07.2018 22:44, Andrew Lunn wrote:
> >>  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;
> > 
> > Please use phy_set_max_speed(). We need MAC drivers to stop directly
> > accessing phydev members. Otherwise we are going to have problems
> > supporting 2.5G PHYs.
> > 
> The issue I see with phy_set_max_speed() is that it also adjusts
> phydev->supported and I don't think that's what we want.
> What the chip supports doesn't change, just what we want to
> advertise.

True.

Which is one more reason to do it inside phylib.

      Andrew

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

[Heiner Kallweit]

On 10.07.2018 22:52, Andrew Lunn wrote:
>>> Why is it powered up, but not connected? Is it powered down before it
>>> is disconnected? Is it the bootloader which is powering it up?
>>>
>> Exactly, if the device is active when driver is loaded and the
>> interface isn't used and therefore not brought up, then, when runtime-
>> suspending, we face this situation.
> 
> Well, it is more complex than that. If the interface is not used, why
> bother even loading the MAC driver?
> 
It would be nice to be able to detect at driver load time whether
interface is used. But driver loading is based on existence of the
PCI device and we don't know about any netctl- or whatever-based
network configuration.

> If you are trying to make a really low power system, the bootloader
> also needs to be involved. It needs to shut down anything it starts
> before handing over control the linux.
> 
I agree but in most cases wed don't have any influence on the
bootloader / BIOS.

> Another approach is to handle this in phylib. When the mdio bus is
> suspended, look for any PHYs which are not connected and power them
> down. The assumption being, any MAC driver using WoL via a PHY does
> not disconnect the PHY before suspending.
> 
>     Andrew
> 

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

[Heiner Kallweit]

On 10.07.2018 22:52, Andrew Lunn wrote:
>>> Why is it powered up, but not connected? Is it powered down before it
>>> is disconnected? Is it the bootloader which is powering it up?
>>>
>> Exactly, if the device is active when driver is loaded and the
>> interface isn't used and therefore not brought up, then, when runtime-
>> suspending, we face this situation.
> 
> Well, it is more complex than that. If the interface is not used, why
> bother even loading the MAC driver?
> 
> If you are trying to make a really low power system, the bootloader
> also needs to be involved. It needs to shut down anything it starts
> before handing over control the linux.
> 
> Another approach is to handle this in phylib. When the mdio bus is
> suspended, look for any PHYs which are not connected and power them
> down. The assumption being, any MAC driver using WoL via a PHY does
> not disconnect the PHY before suspending.
> 
Tricky part may be the differentiation between system- and runtime-
suspend. IIRC the MDIO bus doesn't have runtime pm ops (yet) and
most likely we would have to add them.

>     Andrew
> 

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

[Florian Fainelli]

On 07/10/2018 11:39 AM, 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>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

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

[Florian Fainelli]

On 07/10/2018 11:39 AM, Heiner Kallweit wrote:
> Use phy_ethtool_(g|s)et_link_ksettings() for the respective ethtool_ops
> callbacks.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

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

[Florian Fainelli]

On 07/10/2018 11:39 AM, 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>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

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

[Florian Fainelli]

On 07/10/2018 11:39 AM, Heiner Kallweit wrote:
> Switch to using phy_mii_ioctl().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

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

[Florian Fainelli]

On 07/10/2018 11:40 AM, Heiner Kallweit wrote:
> 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>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian