Message-ID: <f48b0978-7891-487b-d2b1-3f23b269578c@gmail.com>
Date:   Thu, 31 May 2018 22:28:09 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume

On 31.05.2018 20:30, Andrew Lunn wrote:
>> By the way: The problem is related to an experimental patch series for
>> splitting r8169/r8168 drivers and switching r8168 to phylib.
>> Therefore the change to r8168.c won't apply to existing kernel code.
> 
> Hi Heiner
> 
> I still think you are trying to fix the wrong problem.
> 
> Lets take a look at these patches, particularly your code for
> interfacing to phylib.
> 
> Thanks
> 	Andrew
> 
Hi Andrew,

I skip some intermediate patches and in the following just list the
patch adding basic phylib support. The code shouldn't be a big
surprise ..

The network chip integrates subsystems like MAC and PHY, and the
driver takes care that in suspend / resume the different components
are suspended / resumed in the right order.
This includes actions like speeding down the PHY from 1GB to
preferably 10MB (to save energy) in case WoL is configured.

It causes issues like the one I face now, if subsystems like the
MDIO bus suddenly trigger own suspend / resume actions w/o knowing
about status of the other chip subsystems.
Main issue I think is that we loose control over what is done in
which order.

To provide just one example of typical issues:
Configuring the different WoL options isn't handled by writing to
the PHY registers but by writing to chip / MAC registers.
Therefore phy_suspend() isn't able to figure out whether WoL is
enabled or not. Only the parent has the full picture.

To consider dependencies and ensure the right order of suspend /
resume actions I think a parent should be allowed to request that
it takes care of PM of its subsystems.

Regards,
Heiner

---
 drivers/net/ethernet/realtek/Kconfig |   1 +
 drivers/net/ethernet/realtek/r8168.c | 140 +++++++++++++++++++++------
 drivers/net/ethernet/realtek/r8169.h |   2 +
 3 files changed, 116 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig
index 404e1f288..f1e9e8cc2 100644
--- a/drivers/net/ethernet/realtek/Kconfig
+++ b/drivers/net/ethernet/realtek/Kconfig
@@ -105,6 +105,7 @@ config R8168
 	select R8169_COMMON
 	select FW_LOADER
 	select CRC32
+	select PHYLIB
 	select MII
 	help
 	  Say Y here if you have a Realtek 8168 PCI Gigabit Ethernet adapter.
diff --git a/drivers/net/ethernet/realtek/r8168.c b/drivers/net/ethernet/realtek/r8168.c
index 546115344..473a147ec 100644
--- a/drivers/net/ethernet/realtek/r8168.c
+++ b/drivers/net/ethernet/realtek/r8168.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/in.h>
 #include <linux/ip.h>
@@ -868,25 +869,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)
@@ -4444,8 +4426,8 @@ static void rtl_reset_work(struct rtl8169_private *tp)
 
 	napi_enable(&tp->napi);
 	rtl8169_hw_start(tp);
+	phy_start(dev->phydev);
 	netif_wake_queue(dev);
-	rtl8169_check_link_status(dev, tp);
 }
 
 static bool rtl_test_hw_pad_bug(struct rtl8169_private *tp, struct sk_buff *skb)
@@ -4615,7 +4597,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);
 }
@@ -4658,6 +4640,8 @@ static void rtl8169_down(struct net_device *dev)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
 
+	phy_stop(dev->phydev);
+
 	del_timer_sync(&tp->timer);
 
 	napi_disable(&tp->napi);
@@ -4762,14 +4746,13 @@ 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);
 
 	tp->saved_wolopts = 0;
 	pm_runtime_put_sync(&pdev->dev);
-
-	rtl8169_check_link_status(dev, tp);
 out:
 	return retval;
 
@@ -4796,6 +4779,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);
 
@@ -4827,6 +4811,8 @@ static void __rtl8169_resume(struct net_device *dev)
 
 	rtl_pll_power_up(tp);
 
+	phy_start(tp->dev->phydev);
+
 	rtl_lock_work(tp);
 	napi_enable(&tp->napi);
 	set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
@@ -4979,6 +4965,8 @@ static void rtl_remove_one(struct pci_dev *pdev)
 	netif_napi_del(&tp->napi);
 
 	unregister_netdev(dev);
+	phy_disconnect(dev->phydev);
+	mdiobus_unregister(tp->mii_bus);
 
 	rtl_release_firmware(tp);
 
@@ -5041,6 +5029,92 @@ DECLARE_RTL_COND(rtl_rxtx_empty_cond)
 	return (RTL_R8(tp, MCU) & RXTX_EMPTY) == RXTX_EMPTY;
 }
 
+static int r8168_mdio_read_reg(struct mii_bus *mii_bus, int phyaddr, int phyreg)
+{
+	struct rtl8169_private *tp = mii_bus->priv;
+
+	return tp->mdio_ops.read(tp, phyreg);
+}
+
+static int r8168_mdio_write_reg(struct mii_bus *mii_bus, int phyaddr,
+				int phyreg, u16 val)
+{
+	struct rtl8169_private *tp = mii_bus->priv;
+
+	tp->mdio_ops.write(tp, phyreg, val);
+
+	return 0;
+}
+
+static int r8168_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 = "r8168";
+	new_bus->priv = tp;
+	new_bus->phy_mask = ~1;
+	new_bus->parent = &pdev->dev;
+	new_bus->irq[0] = PHY_IGNORE_INTERRUPT;
+	snprintf(new_bus->id, MII_BUS_ID_SIZE, "r8168-%x",
+		 PCI_DEVID(pdev->bus->number, pdev->devfn));
+
+	new_bus->read = r8168_mdio_read_reg;
+	new_bus->write = r8168_mdio_write_reg;
+
+	ret = mdiobus_register(new_bus);
+	if (!ret)
+		tp->mii_bus = new_bus;
+
+	return ret;
+}
+
+static void r8168_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 r8168_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 = phy_find_first(tp->mii_bus);
+	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, r8168_phylink_handler,
+				 phy_mode);
+	if (!ret)
+		phy_attached_info(phydev);
+
+	return ret;
+}
+
 static void rtl_hw_init_8168g(struct rtl8169_private *tp)
 {
 	u32 data;
@@ -5287,10 +5361,18 @@ 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 = r8168_mdio_register(tp);
+	if (rc)
 		return rc;
 
+	rc = r8168_phy_connect(tp);
+	if (rc)
+		goto err_mdio_unregister;
+
+	rc = register_netdev(dev);
+	if (rc)
+		goto err_phy_disconnect;
+
 	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),
@@ -5303,12 +5385,16 @@ 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_phy_disconnect:
+	phy_disconnect(dev->phydev);
+err_mdio_unregister:
+	mdiobus_unregister(tp->mii_bus);
+	return rc;
 }
 
 static struct pci_driver rtl8168_pci_driver = {
diff --git a/drivers/net/ethernet/realtek/r8169.h b/drivers/net/ethernet/realtek/r8169.h
index 355bbcd0f..2f18e5dee 100644
--- a/drivers/net/ethernet/realtek/r8169.h
+++ b/drivers/net/ethernet/realtek/r8169.h
@@ -14,6 +14,7 @@
 #include <linux/delay.h>
 #include <linux/ethtool.h>
 #include <linux/mii.h>
+#include <linux/phy.h>
 #include <linux/if_vlan.h>
 #include <linux/in.h>
 #include <linux/ip.h>
@@ -475,6 +476,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;
-- 
2.17.0

Powered by blists - more mailing lists