[PATCH 0/4] Macb power management support for ZynqMP

[PATCH 0/4] Macb power management support for ZynqMP

[Harini Katakam]

This series adds support for macb suspend/resume with system power down
and wake on LAN with ARP packets.
In relation to the above, this series also updates mdio_read/write
function for PM and adds tsu clock management.

Harini Katakam (4):
  net: macb: Check MDIO state before read/write and use timeouts
  net: macb: Support clock management for tsu_clk
  net: macb: Add pm runtime support
  net: macb: Add support for suspend/resume with full power down

 drivers/net/ethernet/cadence/macb.h      |   3 +-
 drivers/net/ethernet/cadence/macb_main.c | 207 +++++++++++++++++++++++++++----
 2 files changed, 182 insertions(+), 28 deletions(-)

-- 
2.7.4

[PATCH 1/4] net: macb: Check MDIO state before read/write and use timeouts

[Harini Katakam]

From: Harini Katakam <harinik@xilinx.com>

Replace the while loop in MDIO read/write functions with a timeout.
In addition, add a check for MDIO bus busy before initiating a new
operation as well to make sure there is no ongoing MDIO operation.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
Signed-off-by: Harini Katakam <harinik@xilinx.com>
---
Changes form RFC:
Cleaned up timeout implementation and moved it to a helper.

 drivers/net/ethernet/cadence/macb_main.c | 44 +++++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 0acaef3..b4e26c1 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -318,10 +318,36 @@ static void macb_get_hwaddr(struct macb *bp)
 	eth_hw_addr_random(bp->dev);
 }
 
+static int macb_mdio_wait_for_idle(struct macb *bp)
+{
+	ulong timeout;
+
+	timeout = jiffies + msecs_to_jiffies(1000);
+	/* wait for end of transfer */
+	while (1) {
+		if (MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
+			break;
+
+		if (time_after_eq(jiffies, timeout)) {
+			netdev_err(bp->dev, "wait for end of transfer timed out\n");
+			return -ETIMEDOUT;
+		}
+
+		cpu_relax();
+	}
+
+	return 0;
+}
+
 static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 {
 	struct macb *bp = bus->priv;
 	int value;
+	int err;
+
+	err = macb_mdio_wait_for_idle(bp);
+	if (err < 0)
+		return err;
 
 	macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
 			      | MACB_BF(RW, MACB_MAN_READ)
@@ -329,9 +355,9 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 			      | MACB_BF(REGA, regnum)
 			      | MACB_BF(CODE, MACB_MAN_CODE)));
 
-	/* wait for end of transfer */
-	while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
-		cpu_relax();
+	err = macb_mdio_wait_for_idle(bp);
+	if (err < 0)
+		return err;
 
 	value = MACB_BFEXT(DATA, macb_readl(bp, MAN));
 
@@ -342,6 +368,12 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 			   u16 value)
 {
 	struct macb *bp = bus->priv;
+	int err;
+
+
+	err = macb_mdio_wait_for_idle(bp);
+	if (err < 0)
+		return err;
 
 	macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
 			      | MACB_BF(RW, MACB_MAN_WRITE)
@@ -350,9 +382,9 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 			      | MACB_BF(CODE, MACB_MAN_CODE)
 			      | MACB_BF(DATA, value)));
 
-	/* wait for end of transfer */
-	while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
-		cpu_relax();
+	err = macb_mdio_wait_for_idle(bp);
+	if (err < 0)
+		return err;
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH 1/4] net: macb: Check MDIO state before read/write and use timeouts

[Andrew Lunn]

On Wed, Oct 31, 2018 at 09:10:20AM +0530, Harini Katakam wrote:
> From: Harini Katakam <harinik@xilinx.com>
> 
> Replace the while loop in MDIO read/write functions with a timeout.
> In addition, add a check for MDIO bus busy before initiating a new
> operation as well to make sure there is no ongoing MDIO operation.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
> ---
> Changes form RFC:
> Cleaned up timeout implementation and moved it to a helper.
> 
>  drivers/net/ethernet/cadence/macb_main.c | 44 +++++++++++++++++++++++++++-----
>  1 file changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 0acaef3..b4e26c1 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -318,10 +318,36 @@ static void macb_get_hwaddr(struct macb *bp)
>  	eth_hw_addr_random(bp->dev);
>  }
>  
> +static int macb_mdio_wait_for_idle(struct macb *bp)
> +{
> +	ulong timeout;
> +
> +	timeout = jiffies + msecs_to_jiffies(1000);
> +	/* wait for end of transfer */
> +	while (1) {
> +		if (MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
> +			break;
> +
> +		if (time_after_eq(jiffies, timeout)) {
> +			netdev_err(bp->dev, "wait for end of transfer timed out\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		cpu_relax();
> +	}
> +
> +	return 0;
> +}

Hi Harini

Could you make use of readx_poll_timeout(). You will need to add a
helper for the read of the register, since readx_poll_timeout() only
allows one parameter.

Otherwise, this looks O.K.

	   Andrew

Re: [PATCH 1/4] net: macb: Check MDIO state before read/write and use timeouts

[Harini Katakam]

Hi Andrew,

>
> Hi Harini
>
> Could you make use of readx_poll_timeout(). You will need to add a
> helper for the read of the register, since readx_poll_timeout() only
> allows one parameter.
>
> Otherwise, this looks O.K.

Sure, will do. Thanks.

Regards,
Harini

[PATCH 2/4] net: macb: Support clock management for tsu_clk

[Harini Katakam]

From: Harini Katakam <harinik@xilinx.com>

TSU clock needs to be enabled/disabled as per support in devicetree
and it should also be controlled during suspend/resume (WOL has no
dependency on this clock).

Signed-off-by: Harini Katakam <harinik@xilinx.com>
---
 drivers/net/ethernet/cadence/macb.h      |  3 ++-
 drivers/net/ethernet/cadence/macb_main.c | 30 +++++++++++++++++++++++++-----
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 3d45f4c..6f2b4b0 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -1082,7 +1082,7 @@ struct macb_config {
 	unsigned int		dma_burst_length;
 	int	(*clk_init)(struct platform_device *pdev, struct clk **pclk,
 			    struct clk **hclk, struct clk **tx_clk,
-			    struct clk **rx_clk);
+			    struct clk **rx_clk, struct clk **tsu_clk);
 	int	(*init)(struct platform_device *pdev);
 	int	jumbo_max_len;
 };
@@ -1162,6 +1162,7 @@ struct macb {
 	struct clk		*hclk;
 	struct clk		*tx_clk;
 	struct clk		*rx_clk;
+	struct clk		*tsu_clk;
 	struct net_device	*dev;
 	union {
 		struct macb_stats	macb;
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index b4e26c1..7ae8d731 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3304,7 +3304,7 @@ static void macb_probe_queues(void __iomem *mem,
 
 static int macb_clk_init(struct platform_device *pdev, struct clk **pclk,
 			 struct clk **hclk, struct clk **tx_clk,
-			 struct clk **rx_clk)
+			 struct clk **rx_clk, struct clk **tsu_clk)
 {
 	struct macb_platform_data *pdata;
 	int err;
@@ -3338,6 +3338,10 @@ static int macb_clk_init(struct platform_device *pdev, struct clk **pclk,
 	if (IS_ERR(*rx_clk))
 		*rx_clk = NULL;
 
+	*tsu_clk = devm_clk_get(&pdev->dev, "tsu_clk");
+	if (IS_ERR(*tsu_clk))
+		*tsu_clk = NULL;
+
 	err = clk_prepare_enable(*pclk);
 	if (err) {
 		dev_err(&pdev->dev, "failed to enable pclk (%u)\n", err);
@@ -3362,8 +3366,17 @@ static int macb_clk_init(struct platform_device *pdev, struct clk **pclk,
 		goto err_disable_txclk;
 	}
 
+	err = clk_prepare_enable(*tsu_clk);
+	if (err) {
+		dev_err(&pdev->dev, "failed to enable tsu_clk (%u)\n", err);
+		goto err_disable_rxclk;
+	}
+
 	return 0;
 
+err_disable_rxclk:
+	clk_disable_unprepare(*rx_clk);
+
 err_disable_txclk:
 	clk_disable_unprepare(*tx_clk);
 
@@ -3814,13 +3827,14 @@ static const struct net_device_ops at91ether_netdev_ops = {
 
 static int at91ether_clk_init(struct platform_device *pdev, struct clk **pclk,
 			      struct clk **hclk, struct clk **tx_clk,
-			      struct clk **rx_clk)
+			      struct clk **rx_clk, struct clk **tsu_clk)
 {
 	int err;
 
 	*hclk = NULL;
 	*tx_clk = NULL;
 	*rx_clk = NULL;
+	*tsu_clk = NULL;
 
 	*pclk = devm_clk_get(&pdev->dev, "ether_clk");
 	if (IS_ERR(*pclk))
@@ -3968,11 +3982,12 @@ static int macb_probe(struct platform_device *pdev)
 {
 	const struct macb_config *macb_config = &default_gem_config;
 	int (*clk_init)(struct platform_device *, struct clk **,
-			struct clk **, struct clk **,  struct clk **)
-					      = macb_config->clk_init;
+			struct clk **, struct clk **,  struct clk **,
+			struct clk **) = macb_config->clk_init;
 	int (*init)(struct platform_device *) = macb_config->init;
 	struct device_node *np = pdev->dev.of_node;
 	struct clk *pclk, *hclk = NULL, *tx_clk = NULL, *rx_clk = NULL;
+	struct clk *tsu_clk = NULL;
 	unsigned int queue_mask, num_queues;
 	struct macb_platform_data *pdata;
 	bool native_io;
@@ -4000,7 +4015,7 @@ static int macb_probe(struct platform_device *pdev)
 		}
 	}
 
-	err = clk_init(pdev, &pclk, &hclk, &tx_clk, &rx_clk);
+	err = clk_init(pdev, &pclk, &hclk, &tx_clk, &rx_clk, &tsu_clk);
 	if (err)
 		return err;
 
@@ -4037,6 +4052,7 @@ static int macb_probe(struct platform_device *pdev)
 	bp->hclk = hclk;
 	bp->tx_clk = tx_clk;
 	bp->rx_clk = rx_clk;
+	bp->tsu_clk = tsu_clk;
 	if (macb_config)
 		bp->jumbo_max_len = macb_config->jumbo_max_len;
 
@@ -4152,6 +4168,7 @@ static int macb_probe(struct platform_device *pdev)
 	clk_disable_unprepare(hclk);
 	clk_disable_unprepare(pclk);
 	clk_disable_unprepare(rx_clk);
+	clk_disable_unprepare(tsu_clk);
 
 	return err;
 }
@@ -4179,6 +4196,7 @@ static int macb_remove(struct platform_device *pdev)
 		clk_disable_unprepare(bp->hclk);
 		clk_disable_unprepare(bp->pclk);
 		clk_disable_unprepare(bp->rx_clk);
+		clk_disable_unprepare(bp->tsu_clk);
 		of_node_put(bp->phy_node);
 		free_netdev(dev);
 	}
@@ -4205,6 +4223,7 @@ static int __maybe_unused macb_suspend(struct device *dev)
 		clk_disable_unprepare(bp->pclk);
 		clk_disable_unprepare(bp->rx_clk);
 	}
+	clk_disable_unprepare(bp->tsu_clk);
 
 	return 0;
 }
@@ -4225,6 +4244,7 @@ static int __maybe_unused macb_resume(struct device *dev)
 		clk_prepare_enable(bp->tx_clk);
 		clk_prepare_enable(bp->rx_clk);
 	}
+	clk_prepare_enable(bp->tsu_clk);
 
 	netif_device_attach(netdev);
 
-- 
2.7.4

[PATCH 3/4] net: macb: Add pm runtime support

[Harini Katakam]

From: Harini Katakam <harinik@xilinx.com>

Add runtime pm functions and move clock handling there.
If device is suspended and not a wake device, then return from
mdio read/write functions without performing any action because
the clocks are not active.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Signed-off-by: Harini Katakam <harinik@xilinx.com>
---
Changes from RFC:
Updated pm get sync/put sync calls.
Removed unecessary clk up in mdio helpers.

 drivers/net/ethernet/cadence/macb_main.c | 103 +++++++++++++++++++++++++------
 1 file changed, 85 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 7ae8d731..09cb4bb 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -36,6 +36,7 @@
 #include <linux/ip.h>
 #include <linux/udp.h>
 #include <linux/tcp.h>
+#include <linux/pm_runtime.h>
 #include "macb.h"
 
 #define MACB_RX_BUFFER_SIZE	128
@@ -78,6 +79,7 @@
  * 1 frame time (10 Mbits/s, full-duplex, ignoring collisions)
  */
 #define MACB_HALT_TIMEOUT	1230
+#define MACB_PM_TIMEOUT  100 /* ms */
 
 /* DMA buffer descriptor might be different size
  * depends on hardware configuration:
@@ -345,6 +347,10 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 	int value;
 	int err;
 
+	if (pm_runtime_status_suspended(&bp->pdev->dev) &&
+	    !device_may_wakeup(&bp->dev->dev))
+		return -EAGAIN;
+
 	err = macb_mdio_wait_for_idle(bp);
 	if (err < 0)
 		return err;
@@ -370,6 +376,9 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	struct macb *bp = bus->priv;
 	int err;
 
+	if (pm_runtime_status_suspended(&bp->pdev->dev) &&
+	    !device_may_wakeup(&bp->dev->dev))
+		return -EAGAIN;
 
 	err = macb_mdio_wait_for_idle(bp);
 	if (err < 0)
@@ -2397,12 +2406,18 @@ static int macb_open(struct net_device *dev)
 
 	netdev_dbg(bp->dev, "open\n");
 
+	err = pm_runtime_get_sync(&bp->pdev->dev);
+	if (err < 0)
+		goto pm_exit;
+
 	/* carrier starts down */
 	netif_carrier_off(dev);
 
 	/* if the phy is not yet register, retry later*/
-	if (!dev->phydev)
-		return -EAGAIN;
+	if (!dev->phydev) {
+		err = -EAGAIN;
+		goto pm_exit;
+	}
 
 	/* RX buffers initialization */
 	macb_init_rx_buffer_size(bp, bufsz);
@@ -2411,7 +2426,7 @@ static int macb_open(struct net_device *dev)
 	if (err) {
 		netdev_err(dev, "Unable to allocate DMA memory (error %d)\n",
 			   err);
-		return err;
+		goto pm_exit;
 	}
 
 	bp->macbgem_ops.mog_init_rings(bp);
@@ -2428,6 +2443,11 @@ static int macb_open(struct net_device *dev)
 	if (bp->ptp_info)
 		bp->ptp_info->ptp_init(dev);
 
+pm_exit:
+	if (err) {
+		pm_runtime_put_sync(&bp->pdev->dev);
+		return err;
+	}
 	return 0;
 }
 
@@ -2456,6 +2476,8 @@ static int macb_close(struct net_device *dev)
 	if (bp->ptp_info)
 		bp->ptp_info->ptp_remove(dev);
 
+	pm_runtime_put(&bp->pdev->dev);
+
 	return 0;
 }
 
@@ -4019,6 +4041,11 @@ static int macb_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	pm_runtime_set_autosuspend_delay(&pdev->dev, MACB_PM_TIMEOUT);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_get_noresume(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
 	native_io = hw_is_native_io(mem);
 
 	macb_probe_queues(mem, native_io, &queue_mask, &num_queues);
@@ -4150,6 +4177,9 @@ static int macb_probe(struct platform_device *pdev)
 		    macb_is_gem(bp) ? "GEM" : "MACB", macb_readl(bp, MID),
 		    dev->base_addr, dev->irq, dev->dev_addr);
 
+	pm_runtime_mark_last_busy(&bp->pdev->dev);
+	pm_runtime_put_autosuspend(&bp->pdev->dev);
+
 	return 0;
 
 err_out_unregister_mdio:
@@ -4169,6 +4199,9 @@ static int macb_probe(struct platform_device *pdev)
 	clk_disable_unprepare(pclk);
 	clk_disable_unprepare(rx_clk);
 	clk_disable_unprepare(tsu_clk);
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
 
 	return err;
 }
@@ -4192,11 +4225,16 @@ static int macb_remove(struct platform_device *pdev)
 		mdiobus_free(bp->mii_bus);
 
 		unregister_netdev(dev);
-		clk_disable_unprepare(bp->tx_clk);
-		clk_disable_unprepare(bp->hclk);
-		clk_disable_unprepare(bp->pclk);
-		clk_disable_unprepare(bp->rx_clk);
-		clk_disable_unprepare(bp->tsu_clk);
+		pm_runtime_disable(&pdev->dev);
+		pm_runtime_dont_use_autosuspend(&pdev->dev);
+		if (!pm_runtime_suspended(&pdev->dev)) {
+			clk_disable_unprepare(bp->tx_clk);
+			clk_disable_unprepare(bp->hclk);
+			clk_disable_unprepare(bp->pclk);
+			clk_disable_unprepare(bp->rx_clk);
+			clk_disable_unprepare(bp->tsu_clk);
+			pm_runtime_set_suspended(&pdev->dev);
+		}
 		of_node_put(bp->phy_node);
 		free_netdev(dev);
 	}
@@ -4217,13 +4255,9 @@ static int __maybe_unused macb_suspend(struct device *dev)
 		macb_writel(bp, IER, MACB_BIT(WOL));
 		macb_writel(bp, WOL, MACB_BIT(MAG));
 		enable_irq_wake(bp->queues[0].irq);
-	} else {
-		clk_disable_unprepare(bp->tx_clk);
-		clk_disable_unprepare(bp->hclk);
-		clk_disable_unprepare(bp->pclk);
-		clk_disable_unprepare(bp->rx_clk);
 	}
-	clk_disable_unprepare(bp->tsu_clk);
+
+	pm_runtime_force_suspend(dev);
 
 	return 0;
 }
@@ -4234,11 +4268,43 @@ static int __maybe_unused macb_resume(struct device *dev)
 	struct net_device *netdev = platform_get_drvdata(pdev);
 	struct macb *bp = netdev_priv(netdev);
 
+	pm_runtime_force_resume(dev);
+
 	if (bp->wol & MACB_WOL_ENABLED) {
 		macb_writel(bp, IDR, MACB_BIT(WOL));
 		macb_writel(bp, WOL, 0);
 		disable_irq_wake(bp->queues[0].irq);
-	} else {
+	}
+
+	netif_device_attach(netdev);
+
+	return 0;
+}
+
+static int __maybe_unused macb_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct net_device *netdev = platform_get_drvdata(pdev);
+	struct macb *bp = netdev_priv(netdev);
+
+	if (!(device_may_wakeup(&bp->dev->dev))) {
+		clk_disable_unprepare(bp->tx_clk);
+		clk_disable_unprepare(bp->hclk);
+		clk_disable_unprepare(bp->pclk);
+		clk_disable_unprepare(bp->rx_clk);
+	}
+	clk_disable_unprepare(bp->tsu_clk);
+
+	return 0;
+}
+
+static int __maybe_unused macb_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct net_device *netdev = platform_get_drvdata(pdev);
+	struct macb *bp = netdev_priv(netdev);
+
+	if (!(device_may_wakeup(&bp->dev->dev))) {
 		clk_prepare_enable(bp->pclk);
 		clk_prepare_enable(bp->hclk);
 		clk_prepare_enable(bp->tx_clk);
@@ -4246,12 +4312,13 @@ static int __maybe_unused macb_resume(struct device *dev)
 	}
 	clk_prepare_enable(bp->tsu_clk);
 
-	netif_device_attach(netdev);
-
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(macb_pm_ops, macb_suspend, macb_resume);
+static const struct dev_pm_ops macb_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(macb_suspend, macb_resume)
+	SET_RUNTIME_PM_OPS(macb_runtime_suspend, macb_runtime_resume, NULL)
+};
 
 static struct platform_driver macb_driver = {
 	.probe		= macb_probe,
-- 
2.7.4

Re: [PATCH 3/4] net: macb: Add pm runtime support

[Andrew Lunn]

On Wed, Oct 31, 2018 at 09:10:22AM +0530, Harini Katakam wrote:
> From: Harini Katakam <harinik@xilinx.com>
> 
> Add runtime pm functions and move clock handling there.
> If device is suspended and not a wake device, then return from
> mdio read/write functions without performing any action because
> the clocks are not active.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
> ---
> Changes from RFC:
> Updated pm get sync/put sync calls.
> Removed unecessary clk up in mdio helpers.

This last bit has me worried.

The MDIO bus is a shared bus with a life of its own.  You can have
multiple PHYs and switches on it. The PHYs can for a different
Ethernet MAC. Switch drivers will expect to be able to address the
switch when the interface is down.

The FEC driver did something similar for a while. I had to make MDIO
read/write runtime PM aware, otherwise i could not access the Ethernet
switch hanging of its MDIO bus.

	Andrew

Re: [PATCH 3/4] net: macb: Add pm runtime support

[Harini Katakam]

Hi Andrew,
On Wed, Oct 31, 2018 at 8:24 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Oct 31, 2018 at 09:10:22AM +0530, Harini Katakam wrote:
> > From: Harini Katakam <harinik@xilinx.com>
> >
> > Add runtime pm functions and move clock handling there.
> > If device is suspended and not a wake device, then return from
> > mdio read/write functions without performing any action because
> > the clocks are not active.
> >
> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> > Signed-off-by: Harini Katakam <harinik@xilinx.com>
> > ---
> > Changes from RFC:
> > Updated pm get sync/put sync calls.
> > Removed unecessary clk up in mdio helpers.
>
> This last bit has me worried.
>
> The MDIO bus is a shared bus with a life of its own.  You can have
> multiple PHYs and switches on it. The PHYs can for a different
> Ethernet MAC. Switch drivers will expect to be able to address the
> switch when the interface is down.
>
> The FEC driver did something similar for a while. I had to make MDIO
> read/write runtime PM aware, otherwise i could not access the Ethernet
> switch hanging of its MDIO bus.

Yes, I understand. But I thought I'd handle it when we have a separate
MDIO bus driver. As of now, with macb, I do not see a way, for ex.,
for MAC1 to access MDIO0 when MAC0 is suspended.
However, I will work a bit more on this solution.

With the clk up, I've noticed that there is atleast one PHY status poll
that ends up bringing the clock up after the MAC has been suspended.
Of course phy is supended immediately after that but it just delays
this full power down process.

Regards,
Harini

Re: [PATCH 3/4] net: macb: Add pm runtime support

[Andrew Lunn]

> Yes, I understand. But I thought I'd handle it when we have a separate
> MDIO bus driver. As of now, with macb, I do not see a way, for ex.,
> for MAC1 to access MDIO0 when MAC0 is suspended.
> However, I will work a bit more on this solution.

You could look at the FEC driver. Its runtime suspend works well for
me, and i've some complex setups, with two FEC interfaces, PHY and
switches hanging off various MDIO busses etc.

	 Andrew

Re: [PATCH 3/4] net: macb: Add pm runtime support

[Harini Katakam]

Hi Andrew,
On Wed, Oct 31, 2018 at 9:18 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Yes, I understand. But I thought I'd handle it when we have a separate
> > MDIO bus driver. As of now, with macb, I do not see a way, for ex.,
> > for MAC1 to access MDIO0 when MAC0 is suspended.
> > However, I will work a bit more on this solution.
>
> You could look at the FEC driver. Its runtime suspend works well for
> me, and i've some complex setups, with two FEC interfaces, PHY and
> switches hanging off various MDIO busses etc.

OK sure, thanks.

Regards,
Harini

[PATCH 4/4] net: macb: Add support for suspend/resume with full power down

[Harini Katakam]

When macb device is suspended and system is powered down, the clocks
are removed and hence macb should be closed gracefully and restored
upon resume. This patch does the same by switching off the net device,
suspending phy and performing necessary cleanup of interrupts and BDs.
Upon resume, all these are reinitialized again.

Reset of macb device is done only when GEM is not a wake device.
Even when gem is a wake device, tx queues can be stopped and ptp device
can be closed (tsu clock will be disabled in pm_runtime_suspend) as
wake event detection has no dependency on this.

Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
---
Notes:
I was unable to do a full macb_close/open in this patch as suggested
because it was freeing and allocating the full RX/TX buffers and
this time consuming, also leading to a crash when done continuously
in stress tests. 

 drivers/net/ethernet/cadence/macb_main.c | 38 ++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 09cb4bb..0d1acb4 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4247,16 +4247,33 @@ static int __maybe_unused macb_suspend(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct net_device *netdev = platform_get_drvdata(pdev);
 	struct macb *bp = netdev_priv(netdev);
+	struct macb_queue *queue = bp->queues;
+	unsigned long flags;
+	unsigned int q;
+
+	if (!netif_running(netdev))
+		return 0;
 
-	netif_carrier_off(netdev);
-	netif_device_detach(netdev);
 
 	if (bp->wol & MACB_WOL_ENABLED) {
 		macb_writel(bp, IER, MACB_BIT(WOL));
 		macb_writel(bp, WOL, MACB_BIT(MAG));
 		enable_irq_wake(bp->queues[0].irq);
+		netif_device_detach(netdev);
+	} else {
+		netif_device_detach(netdev);
+		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
+			napi_disable(&queue->napi);
+		phy_stop(bp->phy_dev);
+		phy_suspend(bp->phy_dev);
+		spin_lock_irqsave(&bp->lock, flags);
+		macb_reset_hw(bp);
+		spin_unlock_irqrestore(&bp->lock, flags);
 	}
 
+	netif_carrier_off(netdev);
+	if (bp->ptp_info)
+		bp->ptp_info->ptp_remove(netdev);
 	pm_runtime_force_suspend(dev);
 
 	return 0;
@@ -4267,6 +4284,11 @@ static int __maybe_unused macb_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct net_device *netdev = platform_get_drvdata(pdev);
 	struct macb *bp = netdev_priv(netdev);
+	struct macb_queue *queue = bp->queues;
+	unsigned int q;
+
+	if (!netif_running(netdev))
+		return 0;
 
 	pm_runtime_force_resume(dev);
 
@@ -4274,9 +4296,21 @@ static int __maybe_unused macb_resume(struct device *dev)
 		macb_writel(bp, IDR, MACB_BIT(WOL));
 		macb_writel(bp, WOL, 0);
 		disable_irq_wake(bp->queues[0].irq);
+	} else {
+		macb_writel(bp, NCR, MACB_BIT(MPE));
+		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
+			napi_enable(&queue->napi);
+		phy_resume(bp->phy_dev);
+		phy_init_hw(bp->phy_dev);
+		phy_start(bp->phy_dev);
 	}
 
+	bp->macbgem_ops.mog_init_rings(bp);
+	macb_init_hw(bp);
+	macb_set_rx_mode(netdev);
 	netif_device_attach(netdev);
+	if (bp->ptp_info)
+		bp->ptp_info->ptp_init(netdev);
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH 4/4] net: macb: Add support for suspend/resume with full power down

[Andrew Lunn]

On Wed, Oct 31, 2018 at 09:10:23AM +0530, Harini Katakam wrote:
> When macb device is suspended and system is powered down, the clocks
> are removed and hence macb should be closed gracefully and restored
> upon resume. This patch does the same by switching off the net device,
> suspending phy and performing necessary cleanup of interrupts and BDs.
> Upon resume, all these are reinitialized again.
> 
> Reset of macb device is done only when GEM is not a wake device.
> Even when gem is a wake device, tx queues can be stopped and ptp device
> can be closed (tsu clock will be disabled in pm_runtime_suspend) as
> wake event detection has no dependency on this.
> 
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> ---
> Notes:
> I was unable to do a full macb_close/open in this patch as suggested
> because it was freeing and allocating the full RX/TX buffers and
> this time consuming, also leading to a crash when done continuously
> in stress tests. 

Hi Harini

Did you try stress testing just plain up/down, which will call
macb_open/macb_close? It could be it is broken already, and
suspend/resume just makes it more obvious it is broken.

	       Andrew

Re: [PATCH 4/4] net: macb: Add support for suspend/resume with full power down

[Harini Katakam]

Hi Andrew,
On Wed, Oct 31, 2018 at 8:28 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Wed, Oct 31, 2018 at 09:10:23AM +0530, Harini Katakam wrote:
> > When macb device is suspended and system is powered down, the clocks
> > are removed and hence macb should be closed gracefully and restored
> > upon resume. This patch does the same by switching off the net device,
> > suspending phy and performing necessary cleanup of interrupts and BDs.
> > Upon resume, all these are reinitialized again.
> >
> > Reset of macb device is done only when GEM is not a wake device.
> > Even when gem is a wake device, tx queues can be stopped and ptp device
> > can be closed (tsu clock will be disabled in pm_runtime_suspend) as
> > wake event detection has no dependency on this.
> >
> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> > Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> > ---
> > Notes:
> > I was unable to do a full macb_close/open in this patch as suggested
> > because it was freeing and allocating the full RX/TX buffers and
> > this time consuming, also leading to a crash when done continuously
> > in stress tests.
>
> Hi Harini
>
> Did you try stress testing just plain up/down, which will call
> macb_open/macb_close? It could be it is broken already, and
> suspend/resume just makes it more obvious it is broken.

Yes, I did. Plain up/down stress tests do not show any problems.

Regards,
Harini

Re: [PATCH 4/4] net: macb: Add support for suspend/resume with full power down

[Claudiu.Beznea]

On 31.10.2018 05:40, Harini Katakam wrote:
> When macb device is suspended and system is powered down, the clocks
> are removed and hence macb should be closed gracefully and restored
> upon resume. This patch does the same by switching off the net device,
> suspending phy and performing necessary cleanup of interrupts and BDs.
> Upon resume, all these are reinitialized again.
> 
> Reset of macb device is done only when GEM is not a wake device.
> Even when gem is a wake device, tx queues can be stopped and ptp device
> can be closed (tsu clock will be disabled in pm_runtime_suspend) as
> wake event detection has no dependency on this.
> 
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> ---
> Notes:
> I was unable to do a full macb_close/open in this patch as suggested
> because it was freeing and allocating the full RX/TX buffers and
> this time consuming, also leading to a crash when done continuously
> in stress tests. 
> 
>  drivers/net/ethernet/cadence/macb_main.c | 38 ++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 09cb4bb..0d1acb4 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4247,16 +4247,33 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct net_device *netdev = platform_get_drvdata(pdev);
>  	struct macb *bp = netdev_priv(netdev);
> +	struct macb_queue *queue = bp->queues;
> +	unsigned long flags;
> +	unsigned int q;
> +
> +	if (!netif_running(netdev))
> +		return 0;
>  
> -	netif_carrier_off(netdev);
> -	netif_device_detach(netdev);
>  
>  	if (bp->wol & MACB_WOL_ENABLED) {
>  		macb_writel(bp, IER, MACB_BIT(WOL));
>  		macb_writel(bp, WOL, MACB_BIT(MAG));
>  		enable_irq_wake(bp->queues[0].irq);
> +		netif_device_detach(netdev);
> +	} else {
> +		netif_device_detach(netdev);
> +		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
> +			napi_disable(&queue->napi);
> +		phy_stop(bp->phy_dev);

Hi Harini,

I applied these patches on net-next/master cloned from [1], updated this
moment, but I don't have a phy_dev member in struct macb. Maybe you wanted
to use netdev->phydev ?

Thank you,
Claudiu Beznea

[1] https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git

> +		phy_suspend(bp->phy_dev);
> +		spin_lock_irqsave(&bp->lock, flags);
> +		macb_reset_hw(bp);
> +		spin_unlock_irqrestore(&bp->lock, flags);
>  	}
>  
> +	netif_carrier_off(netdev);
> +	if (bp->ptp_info)
> +		bp->ptp_info->ptp_remove(netdev);
>  	pm_runtime_force_suspend(dev);
>  
>  	return 0;
> @@ -4267,6 +4284,11 @@ static int __maybe_unused macb_resume(struct device *dev)
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct net_device *netdev = platform_get_drvdata(pdev);
>  	struct macb *bp = netdev_priv(netdev);
> +	struct macb_queue *queue = bp->queues;
> +	unsigned int q;
> +
> +	if (!netif_running(netdev))
> +		return 0;
>  
>  	pm_runtime_force_resume(dev);
>  
> @@ -4274,9 +4296,21 @@ static int __maybe_unused macb_resume(struct device *dev)
>  		macb_writel(bp, IDR, MACB_BIT(WOL));
>  		macb_writel(bp, WOL, 0);
>  		disable_irq_wake(bp->queues[0].irq);
> +	} else {
> +		macb_writel(bp, NCR, MACB_BIT(MPE));
> +		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
> +			napi_enable(&queue->napi);
> +		phy_resume(bp->phy_dev);
> +		phy_init_hw(bp->phy_dev);
> +		phy_start(bp->phy_dev);
>  	}
>  
> +	bp->macbgem_ops.mog_init_rings(bp);
> +	macb_init_hw(bp);
> +	macb_set_rx_mode(netdev);
>  	netif_device_attach(netdev);
> +	if (bp->ptp_info)
> +		bp->ptp_info->ptp_init(netdev);
>  
>  	return 0;
>  }
> 

Re: [PATCH 4/4] net: macb: Add support for suspend/resume with full power down

[Harini Katakam]

Hi Claudiu,

> Hi Harini,
>
> I applied these patches on net-next/master cloned from [1], updated this
> moment, but I don't have a phy_dev member in struct macb. Maybe you wanted
> to use netdev->phydev ?
>
> Thank you,
> Claudiu Beznea
>
I apologize. Yes, I'm using netdev->phydev and it was uncommitted on my branch
@@ -4264,8 +4264,8 @@ static int __maybe_unused macb_suspend(struct device *dev)
                netif_device_detach(netdev);
                for (q = 0, queue = bp->queues; q < bp->num_queues;
++q, ++queue)
                        napi_disable(&queue->napi);
-               phy_stop(bp->phy_dev);
-               phy_suspend(bp->phy_dev);
+               phy_stop(netdev->phydev);
+               phy_suspend(netdev->phydev);
                spin_lock_irqsave(&bp->lock, flags);
                macb_reset_hw(bp);
                spin_unlock_irqrestore(&bp->lock, flags);
@@ -4300,9 +4300,9 @@ static int __maybe_unused macb_resume(struct device *dev)
                macb_writel(bp, NCR, MACB_BIT(MPE));
                for (q = 0, queue = bp->queues; q < bp->num_queues;
++q, ++queue)
                        napi_enable(&queue->napi);
-               phy_resume(bp->phy_dev);
-               phy_init_hw(bp->phy_dev);
-               phy_start(bp->phy_dev);
+               phy_resume(netdev->phydev);
+               phy_init_hw(netdev->phydev);
+               phy_start(netdev->phydev);
Will fix in next set.

Regards,
Harini