[RFC PATCH 0/5] Macb power management support for ZynqMP

[RFC PATCH 0/5] Macb power management support for ZynqMP

[harinikatakamlinux]

From: Harini Katakam <harinik@xilinx.com>

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 (5):
  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
  net: macb: Add WOL support with ARP

 drivers/net/ethernet/cadence/macb.h      |   9 +-
 drivers/net/ethernet/cadence/macb_main.c | 349 ++++++++++++++++++++++++++++---
 2 files changed, 332 insertions(+), 26 deletions(-)

-- 
2.7.4

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

[harinikatakamlinux]

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: Harini Katakam <harinik@xilinx.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 54 ++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index d09bd43..f4030c1 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -321,6 +321,21 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 {
 	struct macb *bp = bus->priv;
 	int value;
+	ulong timeout;
+
+	timeout = jiffies + msecs_to_jiffies(1000);
+	/* wait for end of transfer */
+	do {
+		if (MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
+			break;
+
+		cpu_relax();
+	} while (!time_after_eq(jiffies, timeout));
+
+	if (time_after_eq(jiffies, timeout)) {
+		netdev_err(bp->dev, "wait for end of transfer timed out\n");
+		return -ETIMEDOUT;
+	}
 
 	macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
 			      | MACB_BF(RW, MACB_MAN_READ)
@@ -328,9 +343,19 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 			      | MACB_BF(REGA, regnum)
 			      | MACB_BF(CODE, MACB_MAN_CODE)));
 
+	timeout = jiffies + msecs_to_jiffies(1000);
 	/* wait for end of transfer */
-	while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
+	do {
+		if (MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
+			break;
+
 		cpu_relax();
+	} while (!time_after_eq(jiffies, timeout));
+
+	if (time_after_eq(jiffies, timeout)) {
+		netdev_err(bp->dev, "wait for end of transfer timed out\n");
+		return -ETIMEDOUT;
+	}
 
 	value = MACB_BFEXT(DATA, macb_readl(bp, MAN));
 
@@ -341,6 +366,21 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 			   u16 value)
 {
 	struct macb *bp = bus->priv;
+	ulong timeout;
+
+	timeout = jiffies + msecs_to_jiffies(1000);
+	/* wait for end of transfer */
+	do {
+		if (MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
+			break;
+
+		cpu_relax();
+	} while (!time_after_eq(jiffies, timeout));
+
+	if (time_after_eq(jiffies, timeout)) {
+		netdev_err(bp->dev, "wait for end of transfer timed out\n");
+		return -ETIMEDOUT;
+	}
 
 	macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
 			      | MACB_BF(RW, MACB_MAN_WRITE)
@@ -349,9 +389,19 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 			      | MACB_BF(CODE, MACB_MAN_CODE)
 			      | MACB_BF(DATA, value)));
 
+	timeout = jiffies + msecs_to_jiffies(1000);
 	/* wait for end of transfer */
-	while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
+	do {
+		if (MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
+			break;
+
 		cpu_relax();
+	} while (!time_after_eq(jiffies, timeout));
+
+	if (time_after_eq(jiffies, timeout)) {
+		netdev_err(bp->dev, "wait for end of transfer timed out\n");
+		return -ETIMEDOUT;
+	}
 
 	return 0;
 }
-- 
2.7.4

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

[Andrew Lunn]

On Thu, Mar 22, 2018 at 07:21:36PM +0530, harinikatakamlinux@gmail.com 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: Harini Katakam <harinik@xilinx.com>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 54 ++++++++++++++++++++++++++++++--
>  1 file changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index d09bd43..f4030c1 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -321,6 +321,21 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>  {
>  	struct macb *bp = bus->priv;
>  	int value;
> +	ulong timeout;
> +
> +	timeout = jiffies + msecs_to_jiffies(1000);
> +	/* wait for end of transfer */
> +	do {
> +		if (MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
> +			break;
> +
> +		cpu_relax();
> +	} while (!time_after_eq(jiffies, timeout));
> +
> +	if (time_after_eq(jiffies, timeout)) {
> +		netdev_err(bp->dev, "wait for end of transfer timed out\n");
> +		return -ETIMEDOUT;
> +	}

Hi Harini

It looks like you have repeated the same code 4 times. Please move it
into a helper function.

     Andrew

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

[Claudiu Beznea]

On 22.03.2018 15:51, harinikatakamlinux@gmail.com 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: Harini Katakam <harinik@xilinx.com>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 54 ++++++++++++++++++++++++++++++--
>  1 file changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index d09bd43..f4030c1 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -321,6 +321,21 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>  {
>  	struct macb *bp = bus->priv;
>  	int value;
> +	ulong timeout;
> +
> +	timeout = jiffies + msecs_to_jiffies(1000);
> +	/* wait for end of transfer */
> +	do {
> +		if (MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
> +			break;
> +
> +		cpu_relax();
> +	} while (!time_after_eq(jiffies, timeout));
> +
> +	if (time_after_eq(jiffies, timeout)) {
> +		netdev_err(bp->dev, "wait for end of transfer timed out\n");
> +		return -ETIMEDOUT;
> +	}

Wouldn't be cleaner to keep it in this way:

	while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR))) {
		if (time_after_eq(jiffies, timeout) {
			netdev_err(bp->dev, "wait for end of transfer timed out\n");
			return -ETIMEDOUT;
		}
		cpu_relax();
	}

>  
>  	macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
>  			      | MACB_BF(RW, MACB_MAN_READ)
> @@ -328,9 +343,19 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>  			      | MACB_BF(REGA, regnum)
>  			      | MACB_BF(CODE, MACB_MAN_CODE)));
>  
> +	timeout = jiffies + msecs_to_jiffies(1000);
>  	/* wait for end of transfer */
> -	while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
> +	do {
> +		if (MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
> +			break;
> +
>  		cpu_relax();
> +	} while (!time_after_eq(jiffies, timeout));
> +
> +	if (time_after_eq(jiffies, timeout)) {
> +		netdev_err(bp->dev, "wait for end of transfer timed out\n");
> +		return -ETIMEDOUT;
> +	}
>  
>  	value = MACB_BFEXT(DATA, macb_readl(bp, MAN));
>  
> @@ -341,6 +366,21 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>  			   u16 value)
>  {
>  	struct macb *bp = bus->priv;
> +	ulong timeout;
> +
> +	timeout = jiffies + msecs_to_jiffies(1000);
> +	/* wait for end of transfer */
> +	do {
> +		if (MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
> +			break;
> +
> +		cpu_relax();
> +	} while (!time_after_eq(jiffies, timeout));
> +
> +	if (time_after_eq(jiffies, timeout)) {
> +		netdev_err(bp->dev, "wait for end of transfer timed out\n");
> +		return -ETIMEDOUT;
> +	}
>  
>  	macb_writel(bp, MAN, (MACB_BF(SOF, MACB_MAN_SOF)
>  			      | MACB_BF(RW, MACB_MAN_WRITE)
> @@ -349,9 +389,19 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>  			      | MACB_BF(CODE, MACB_MAN_CODE)
>  			      | MACB_BF(DATA, value)));
>  
> +	timeout = jiffies + msecs_to_jiffies(1000);
>  	/* wait for end of transfer */
> -	while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
> +	do {
> +		if (MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
> +			break;
> +
>  		cpu_relax();
> +	} while (!time_after_eq(jiffies, timeout));
> +
> +	if (time_after_eq(jiffies, timeout)) {
> +		netdev_err(bp->dev, "wait for end of transfer timed out\n");
> +		return -ETIMEDOUT;
> +	}
>  
>  	return 0;
>  }
> 

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

[Harini Katakam]

Hi Claudiu,

On Thu, May 3, 2018 at 3:38 PM, Claudiu Beznea
<Claudiu.Beznea@microchip.com> wrote:
>
>
> On 22.03.2018 15:51, harinikatakamlinux@gmail.com wrote:
>> From: Harini Katakam <harinik@xilinx.com>
>>
<snip>
>> +     ulong timeout;
>> +
>> +     timeout = jiffies + msecs_to_jiffies(1000);
>> +     /* wait for end of transfer */
>> +     do {
>> +             if (MACB_BFEXT(IDLE, macb_readl(bp, NSR)))
>> +                     break;
>> +
>> +             cpu_relax();
>> +     } while (!time_after_eq(jiffies, timeout));
>> +
>> +     if (time_after_eq(jiffies, timeout)) {
>> +             netdev_err(bp->dev, "wait for end of transfer timed out\n");
>> +             return -ETIMEDOUT;
>> +     }
>
> Wouldn't be cleaner to keep it in this way:
>
>         while (!MACB_BFEXT(IDLE, macb_readl(bp, NSR))) {
>                 if (time_after_eq(jiffies, timeout) {
>                         netdev_err(bp->dev, "wait for end of transfer timed out\n");
>                         return -ETIMEDOUT;
>                 }
>                 cpu_relax();
>         }
>

Thanks for the review.
Sure, will update in next version.

Regards,
Harini

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

[harinikatakamlinux]

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 8665982..9e7fb14 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -1074,7 +1074,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;
 };
@@ -1154,6 +1154,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 f4030c1..ae61927 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -3245,7 +3245,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;
@@ -3279,6 +3279,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);
@@ -3303,8 +3307,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);
 
@@ -3754,13 +3767,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))
@@ -3898,11 +3912,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;
@@ -3930,7 +3945,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;
 
@@ -3967,6 +3982,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;
 
@@ -4064,6 +4080,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;
 }
@@ -4091,6 +4108,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);
 	}
@@ -4117,6 +4135,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;
 }
@@ -4137,6 +4156,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

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

[harinikatakamlinux]

From: Harini Katakam <harinik@xilinx.com>

Add runtime pm functions and move clock handling there.
Enable clocks in mdio read/write functions.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Signed-off-by: Harini Katakam <harinik@xilinx.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 105 ++++++++++++++++++++++++++-----
 1 file changed, 90 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index ae61927..ce75088 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -35,6 +35,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
@@ -77,6 +78,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:
@@ -321,8 +323,13 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 {
 	struct macb *bp = bus->priv;
 	int value;
+	int err;
 	ulong timeout;
 
+	err = pm_runtime_get_sync(&bp->pdev->dev);
+	if (err < 0)
+		return err;
+
 	timeout = jiffies + msecs_to_jiffies(1000);
 	/* wait for end of transfer */
 	do {
@@ -334,6 +341,8 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 
 	if (time_after_eq(jiffies, timeout)) {
 		netdev_err(bp->dev, "wait for end of transfer timed out\n");
+		pm_runtime_mark_last_busy(&bp->pdev->dev);
+		pm_runtime_put_autosuspend(&bp->pdev->dev);
 		return -ETIMEDOUT;
 	}
 
@@ -354,11 +363,15 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 
 	if (time_after_eq(jiffies, timeout)) {
 		netdev_err(bp->dev, "wait for end of transfer timed out\n");
+		pm_runtime_mark_last_busy(&bp->pdev->dev);
+		pm_runtime_put_autosuspend(&bp->pdev->dev);
 		return -ETIMEDOUT;
 	}
 
 	value = MACB_BFEXT(DATA, macb_readl(bp, MAN));
 
+	pm_runtime_mark_last_busy(&bp->pdev->dev);
+	pm_runtime_put_autosuspend(&bp->pdev->dev);
 	return value;
 }
 
@@ -366,8 +379,13 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 			   u16 value)
 {
 	struct macb *bp = bus->priv;
+	int err;
 	ulong timeout;
 
+	err = pm_runtime_get_sync(&bp->pdev->dev);
+	if (err < 0)
+		return err;
+
 	timeout = jiffies + msecs_to_jiffies(1000);
 	/* wait for end of transfer */
 	do {
@@ -379,6 +397,8 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 
 	if (time_after_eq(jiffies, timeout)) {
 		netdev_err(bp->dev, "wait for end of transfer timed out\n");
+		pm_runtime_mark_last_busy(&bp->pdev->dev);
+		pm_runtime_put_autosuspend(&bp->pdev->dev);
 		return -ETIMEDOUT;
 	}
 
@@ -400,9 +420,13 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 
 	if (time_after_eq(jiffies, timeout)) {
 		netdev_err(bp->dev, "wait for end of transfer timed out\n");
+		pm_runtime_mark_last_busy(&bp->pdev->dev);
+		pm_runtime_put_autosuspend(&bp->pdev->dev);
 		return -ETIMEDOUT;
 	}
 
+	pm_runtime_mark_last_busy(&bp->pdev->dev);
+	pm_runtime_put_autosuspend(&bp->pdev->dev);
 	return 0;
 }
 
@@ -2338,6 +2362,10 @@ 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)
+		return err;
+
 	/* carrier starts down */
 	netif_carrier_off(dev);
 
@@ -2397,6 +2425,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;
 }
 
@@ -3949,6 +3979,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);
@@ -4062,6 +4097,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:
@@ -4081,6 +4119,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;
 }
@@ -4104,11 +4145,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);
 	}
@@ -4129,13 +4175,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;
 }
@@ -4146,11 +4188,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);
@@ -4158,12 +4232,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: [RFC PATCH 3/5] net: macb: Add pm runtime support

[Claudiu Beznea]

On 22.03.2018 15:51, harinikatakamlinux@gmail.com wrote:
> From: Harini Katakam <harinik@xilinx.com>
> 
> Add runtime pm functions and move clock handling there.
> Enable clocks in mdio read/write functions.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 105 ++++++++++++++++++++++++++-----
>  1 file changed, 90 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index ae61927..ce75088 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -35,6 +35,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
> @@ -77,6 +78,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:
> @@ -321,8 +323,13 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>  {
>  	struct macb *bp = bus->priv;
>  	int value;
> +	int err;
>  	ulong timeout;
>  
> +	err = pm_runtime_get_sync(&bp->pdev->dev);
> +	if (err < 0)

You have to call pm_runtime_put_noidle() or something similar to decrement the
dev->power.usage_count.

> +		return err;
> +
>  	timeout = jiffies + msecs_to_jiffies(1000);
>  	/* wait for end of transfer */
>  	do {
> @@ -334,6 +341,8 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>  
>  	if (time_after_eq(jiffies, timeout)) {
>  		netdev_err(bp->dev, "wait for end of transfer timed out\n");

For this:

> +		pm_runtime_mark_last_busy(&bp->pdev->dev);
> +		pm_runtime_put_autosuspend(&bp->pdev->dev);>  		return -ETIMEDOUT;
>  	}
>  
> @@ -354,11 +363,15 @@ static int macb_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
>  
>  	if (time_after_eq(jiffies, timeout)) {
>  		netdev_err(bp->dev, "wait for end of transfer timed out\n");

And this:

> +		pm_runtime_mark_last_busy(&bp->pdev->dev);
> +		pm_runtime_put_autosuspend(&bp->pdev->dev);
>  		return -ETIMEDOUT;

I would use a "goto" instruction, e.g.:
  		value = -ETIMEDOUT;
		goto out;

>  	}
>  
>  	value = MACB_BFEXT(DATA, macb_readl(bp, MAN));
>  

out:
> +	pm_runtime_mark_last_busy(&bp->pdev->dev);
> +	pm_runtime_put_autosuspend(&bp->pdev->dev);
>  	return value;
>  }
>  
> @@ -366,8 +379,13 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>  			   u16 value)
>  {
>  	struct macb *bp = bus->priv;
> +	int err;
>  	ulong timeout;
>  
> +	err = pm_runtime_get_sync(&bp->pdev->dev);> +	if (err < 0)

Ditto

> +		return err;
> +
>  	timeout = jiffies + msecs_to_jiffies(1000);
>  	/* wait for end of transfer */
>  	do {
> @@ -379,6 +397,8 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>  
>  	if (time_after_eq(jiffies, timeout)) {
>  		netdev_err(bp->dev, "wait for end of transfer timed out\n");

Ditto

> +		pm_runtime_mark_last_busy(&bp->pdev->dev);
> +		pm_runtime_put_autosuspend(&bp->pdev->dev);
>  		return -ETIMEDOUT;
>  	}
>  
> @@ -400,9 +420,13 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>  
>  	if (time_after_eq(jiffies, timeout)) {
>  		netdev_err(bp->dev, "wait for end of transfer timed out\n");
> +		pm_runtime_mark_last_busy(&bp->pdev->dev);
> +		pm_runtime_put_autosuspend(&bp->pdev->dev);

Ditto

>  		return -ETIMEDOUT;
>  	}
>  
> +	pm_runtime_mark_last_busy(&bp->pdev->dev);
> +	pm_runtime_put_autosuspend(&bp->pdev->dev);
>  	return 0;
>  }
>  
> @@ -2338,6 +2362,10 @@ 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)

Ditto

> +		return err;
> +

Below, in macb_open() you have a return err; case:
	err = macb_alloc_consistent(bp);
	if (err) {
		netdev_err(dev, "Unable to allocate DMA memory (error %d)\n",
			   err);
		return err;
	}

You have to undo pm_runtime_get_sync() with pm_runtime_put_sync() or something
similar to decrement dev->power.usage_count.
	
>  	/* carrier starts down */
>  	netif_carrier_off(dev);
>  
> @@ -2397,6 +2425,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;
>  }
>  
> @@ -3949,6 +3979,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);
> @@ -4062,6 +4097,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:
> @@ -4081,6 +4119,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;
>  }
> @@ -4104,11 +4145,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);

This is driver remove function. Shouldn't clocks be removed?

> +		}>  		of_node_put(bp->phy_node);
>  		free_netdev(dev);
>  	}
> @@ -4129,13 +4175,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;
>  }
> @@ -4146,11 +4188,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);
> @@ -4158,12 +4232,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,
> 

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

[Harini Katakam]

Hi Claudiu,

On Thu, May 3, 2018 at 3:39 PM, Claudiu Beznea
<Claudiu.Beznea@microchip.com> wrote:
>
>
> On 22.03.2018 15:51, harinikatakamlinux@gmail.com wrote:
>> From: Harini Katakam <harinik@xilinx.com>
<snip>
> I would use a "goto" instruction, e.g.:
>                 value = -ETIMEDOUT;
>                 goto out;
>

Will do

>
> Below, in macb_open() you have a return err; case:
>         err = macb_alloc_consistent(bp);
>         if (err) {
>                 netdev_err(dev, "Unable to allocate DMA memory (error %d)\n",
>                            err);
>                 return err;
>         }
>
> You have to undo pm_runtime_get_sync() with pm_runtime_put_sync() or something
> similar to decrement dev->power.usage_count.

Will do

<snip>
>> @@ -4104,11 +4145,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);
>
> This is driver remove function. Shouldn't clocks be removed?

clk_disable_unprepare IS being done here.
The check for !pm_runtime_suspended is just to make sure the
clocks are not already removed (in runtime_suspend).

Regards,
Harini

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

[Claudiu Beznea]

On 03.05.2018 14:13, Harini Katakam wrote:
> Hi Claudiu,
> 
> On Thu, May 3, 2018 at 3:39 PM, Claudiu Beznea
> <Claudiu.Beznea@microchip.com> wrote:
>>
>>
>> On 22.03.2018 15:51, harinikatakamlinux@gmail.com wrote:
>>> From: Harini Katakam <harinik@xilinx.com>
> <snip>
>> I would use a "goto" instruction, e.g.:
>>                 value = -ETIMEDOUT;
>>                 goto out;
>>
> 
> Will do
> 
>>
>> Below, in macb_open() you have a return err; case:
>>         err = macb_alloc_consistent(bp);
>>         if (err) {
>>                 netdev_err(dev, "Unable to allocate DMA memory (error %d)\n",
>>                            err);
>>                 return err;
>>         }
>>
>> You have to undo pm_runtime_get_sync() with pm_runtime_put_sync() or something
>> similar to decrement dev->power.usage_count.
> 
> Will do
> 
> <snip>
>>> @@ -4104,11 +4145,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);
>>
>> This is driver remove function. Shouldn't clocks be removed?
> 
> clk_disable_unprepare IS being done here.
> The check for !pm_runtime_suspended is just to make sure the
> clocks are not already removed (in runtime_suspend).

Could this happen? Looking over code, starting with 
platform_driver_unregister() it looks to me that the runtime resume
is called just before driver remove is called.

platform_driver_unregister() ->
driver_unregister() ->
bus_remove_driver() ->
driver_detach() ->
device_release_driver_internal() ->
__device_release_driver()
{
	// ...
        pm_runtime_get_sync(dev);		// runtime resume                                       
        pm_runtime_clean_up_links(dev);                                 
 
	// ...

	pm_runtime_put_sync(dev);                                       
                                                                                
        if (dev->bus && dev->bus->remove)                               
        	dev->bus->remove(dev);                                  
        else if (drv->remove)                                           
                drv->remove(dev);                                       

	// ...
}

Thank you,
Claudiu

> 
> Regards,
> Harini
> 

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

[harinikatakamlinux]

From: Harini Katakam <harinik@xilinx.com>

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 <harinik@xilinx.com>
---
 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 ce75088..bca91bd 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4167,16 +4167,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(netdev->phydev);
+		phy_suspend(netdev->phydev);
+		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;
@@ -4187,6 +4204,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);
 
@@ -4194,9 +4216,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);
+		netif_carrier_on(netdev);
+		phy_resume(netdev->phydev);
+		phy_start(netdev->phydev);
 	}
 
+	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: [RFC PATCH 4/5] net: macb: Add support for suspend/resume with full power down

[Claudiu Beznea]

On 22.03.2018 15:51, harinikatakamlinux@gmail.com wrote:
> From: Harini Katakam <harinik@xilinx.com>
> 
> 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. 

Is this a power saving mode which shut down the core?

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 <harinik@xilinx.com>
> ---
>  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 ce75088..bca91bd 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4167,16 +4167,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(netdev->phydev);
> +		phy_suspend(netdev->phydev);
> +		spin_lock_irqsave(&bp->lock, flags);
> +		macb_reset_hw(bp);
> +		spin_unlock_irqrestore(&bp->lock, flags);

Wouldn't be simple to just call macb_close() here?

>  	}
>  
> +	netif_carrier_off(netdev);
> +	if (bp->ptp_info)
> +		bp->ptp_info->ptp_remove(netdev);
>  	pm_runtime_force_suspend(dev);
>  
>  	return 0;
> @@ -4187,6 +4204,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);
>  
> @@ -4194,9 +4216,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);
> +		netif_carrier_on(netdev);
> +		phy_resume(netdev->phydev);
> +		phy_start(netdev->phydev);
>  	}
>  
> +	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);

Wouln't be simpler to call macb_open() here?

>  
>  	return 0;
>  }
> 

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

[Harini Katakam]

Hi Claudiu,

On Thu, May 3, 2018 at 3:39 PM, Claudiu Beznea
<Claudiu.Beznea@microchip.com> wrote:
>
>
> On 22.03.2018 15:51, harinikatakamlinux@gmail.com wrote:
>> From: Harini Katakam <harinik@xilinx.com>
>>
>> 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.
>
> Is this a power saving mode which shut down the core?

The Ethernet IP is suspended and a majority of the SoC is shut down, yes.

<snip>
>> +             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(netdev->phydev);
>> +             phy_suspend(netdev->phydev);
>> +             spin_lock_irqsave(&bp->lock, flags);
>> +             macb_reset_hw(bp);
>> +             spin_unlock_irqrestore(&bp->lock, flags);
>
> Wouldn't be simple to just call macb_close() here?

<snip>
>
> Wouln't be simpler to call macb_open() here?

No, I think that would be excessive for suspend. This does just
enough to put the IP into suspend and cut off clocks.
For ex., the RX and TX buffers are not freed and allocated again
in this cycle, just the buffer descriptors.

Regards,
Harini

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

[Claudiu Beznea]

On 03.05.2018 14:20, Harini Katakam wrote:
> Hi Claudiu,
> 
> On Thu, May 3, 2018 at 3:39 PM, Claudiu Beznea
> <Claudiu.Beznea@microchip.com> wrote:
>>
>>
>> On 22.03.2018 15:51, harinikatakamlinux@gmail.com wrote:
>>> From: Harini Katakam <harinik@xilinx.com>
>>>
>>> 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.
>>
>> Is this a power saving mode which shut down the core?
> 
> The Ethernet IP is suspended and a majority of the SoC is shut down, yes.
> 
> <snip>
>>> +             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(netdev->phydev);
>>> +             phy_suspend(netdev->phydev);
>>> +             spin_lock_irqsave(&bp->lock, flags);
>>> +             macb_reset_hw(bp);
>>> +             spin_unlock_irqrestore(&bp->lock, flags);
>>
>> Wouldn't be simple to just call macb_close() here?
> 
> <snip>
>>
>> Wouln't be simpler to call macb_open() here?
> 
> No, I think that would be excessive for suspend. This does just
> enough to put the IP into suspend and cut off clocks.
> For ex., the RX and TX buffers are not freed and allocated again
> in this cycle, just the buffer descriptors.
> 

For me looks simpler to just call macb_close()/macb_open() in suspend/resume
hooks. Maybe this doesn't fit to your needs... But most of the code you
added in suspend/resume hooks is already present in macb_open()/macb_close(),
except the TX/RX buffers alloc/free.

SAMA5D2 also uses this IP. SAMA5d2 has a power saving mode where core power is cut
off (including this IP). We are using macb_open()/macb_close() + restoring all
few other registers after this (by calling macb_set_rx_mode() and a variant of
macb_set_features() and few other registers). This is not yet mainlined. I was
intending to send soon a version.

Thank you,
Claudiu

> Regards,
> Harini
> 

[RFC PATCH 5/5] net: macb: Add WOL support with ARP

[harinikatakamlinux]

From: Harini Katakam <harinik@xilinx.com>

This patch enables ARP wake event support in GEM through the following:

-> WOL capability can be selected based on the SoC/GEM IP version rather
than a devictree property alone. Hence add a new capability property and
set device as "wakeup capable" in probe in this case.
-> Wake source selection can be done via ethtool or by enabling wakeup
in /sys/devices/platform/..../ethx/power/
This patch adds default wake source as ARP and the existing selection of
WOL using magic packet remains unchanged.
-> When GEM is the wake device with ARP as the wake event, the current
IP address to match is written to WOL register along with other
necessary confuguration required for MAC to recognize an ARP event.
-> While RX needs to remain enabled, there is no need to process the
actual wake packet - hence tie off all RX queues to avoid unnecessary
processing by DMA in the background. This tie off is done using a
dummy buffer descriptor with used bit set. (There is no other provision
to disable RX DMA in the GEM IP version in ZynqMP)
-> TX is disabled and all interrupts except WOL on Q0 are disabled.
Clear the WOL interrupt as no other action is required from driver.
Power management of the SoC will already have got the event and will
take care of initiating resume.
-> Upon resume ARP WOL config is cleared and macb is reinitialized.

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

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 9e7fb14..e18ff34 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -93,6 +93,7 @@
 #define GEM_SA3T		0x009C /* Specific3 Top */
 #define GEM_SA4B		0x00A0 /* Specific4 Bottom */
 #define GEM_SA4T		0x00A4 /* Specific4 Top */
+#define GEM_WOL			0x00B8 /* Wake on LAN */
 #define GEM_EFTSH		0x00e8 /* PTP Event Frame Transmitted Seconds Register 47:32 */
 #define GEM_EFRSH		0x00ec /* PTP Event Frame Received Seconds Register 47:32 */
 #define GEM_PEFTSH		0x00f0 /* PTP Peer Event Frame Transmitted Seconds Register 47:32 */
@@ -398,6 +399,8 @@
 #define MACB_PDRSFT_SIZE	1
 #define MACB_SRI_OFFSET		26 /* TSU Seconds Register Increment */
 #define MACB_SRI_SIZE		1
+#define GEM_WOL_OFFSET		28 /* Enable wake-on-lan interrupt in GEM */
+#define GEM_WOL_SIZE		1
 
 /* Timer increment fields */
 #define MACB_TI_CNS_OFFSET	0
@@ -635,6 +638,7 @@
 #define MACB_CAPS_USRIO_DISABLED		0x00000010
 #define MACB_CAPS_JUMBO				0x00000020
 #define MACB_CAPS_GEM_HAS_PTP			0x00000040
+#define MACB_CAPS_WOL				0x00000080
 #define MACB_CAPS_FIFO_MODE			0x10000000
 #define MACB_CAPS_GIGABIT_MODE_AVAILABLE	0x20000000
 #define MACB_CAPS_SG_DISABLED			0x40000000
@@ -1147,6 +1151,8 @@ struct macb {
 	unsigned int		num_queues;
 	unsigned int		queue_mask;
 	struct macb_queue	queues[MACB_MAX_QUEUES];
+	dma_addr_t		rx_ring_tieoff_dma;
+	struct macb_dma_desc	*rx_ring_tieoff;
 
 	spinlock_t		lock;
 	struct platform_device	*pdev;
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index bca91bd..9902654 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -36,6 +36,7 @@
 #include <linux/udp.h>
 #include <linux/tcp.h>
 #include <linux/pm_runtime.h>
+#include <linux/inetdevice.h>
 #include "macb.h"
 
 #define MACB_RX_BUFFER_SIZE	128
@@ -1400,6 +1401,12 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 	spin_lock(&bp->lock);
 
 	while (status) {
+		if (status & GEM_BIT(WOL)) {
+			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+				queue_writel(queue, ISR, GEM_BIT(WOL));
+			break;
+		}
+
 		/* close possible race with dev_close */
 		if (unlikely(!netif_running(dev))) {
 			queue_writel(queue, IDR, -1);
@@ -1900,6 +1907,12 @@ static void macb_free_consistent(struct macb *bp)
 		queue->rx_ring = NULL;
 	}
 
+	if (bp->rx_ring_tieoff) {
+		dma_free_coherent(&bp->pdev->dev, macb_dma_desc_get_size(bp),
+				  bp->rx_ring_tieoff, bp->rx_ring_tieoff_dma);
+		bp->rx_ring_tieoff = NULL;
+	}
+
 	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
 		kfree(queue->tx_skb);
 		queue->tx_skb = NULL;
@@ -1979,6 +1992,14 @@ static int macb_alloc_consistent(struct macb *bp)
 			   "Allocated RX ring of %d bytes at %08lx (mapped %p)\n",
 			   size, (unsigned long)queue->rx_ring_dma, queue->rx_ring);
 	}
+	/* Allocate one dummy descriptor to tie off RX queues when required */
+	bp->rx_ring_tieoff = dma_alloc_coherent(&bp->pdev->dev,
+						macb_dma_desc_get_size(bp),
+						&bp->rx_ring_tieoff_dma,
+						GFP_KERNEL);
+	if (!bp->rx_ring_tieoff)
+		goto out_err;
+
 	if (bp->macbgem_ops.mog_alloc_rx_buffers(bp))
 		goto out_err;
 
@@ -1989,6 +2010,34 @@ static int macb_alloc_consistent(struct macb *bp)
 	return -ENOMEM;
 }
 
+static void macb_init_tieoff(struct macb *bp)
+{
+	struct macb_dma_desc *d = bp->rx_ring_tieoff;
+
+	/* Setup a wrapping descriptor with no free slots
+	 * (WRAP and USED) to tie off/disable unused RX queues.
+	 */
+	macb_set_addr(bp, d, MACB_BIT(RX_WRAP) | MACB_BIT(RX_USED));
+	d->ctrl = 0;
+}
+
+static inline void macb_rx_tieoff(struct macb *bp)
+{
+	struct macb_queue *queue = bp->queues;
+	unsigned int q;
+
+	for (q = 0, queue = bp->queues; q < bp->num_queues;
+	     ++q, ++queue) {
+		queue_writel(queue, RBQP,
+			     lower_32_bits(bp->rx_ring_tieoff_dma));
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+		if (bp->hw_dma_cap & HW_DMA_CAP_64B)
+			queue_writel(queue, RBQPH,
+				     upper_32_bits(bp->rx_ring_tieoff_dma));
+#endif
+	}
+}
+
 static void gem_init_rings(struct macb *bp)
 {
 	struct macb_queue *queue;
@@ -2011,6 +2060,7 @@ static void gem_init_rings(struct macb *bp)
 
 		gem_rx_refill(queue);
 	}
+	macb_init_tieoff(bp);
 
 }
 
@@ -2653,6 +2703,13 @@ static void macb_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 		if (bp->wol & MACB_WOL_ENABLED)
 			wol->wolopts |= WAKE_MAGIC;
 	}
+
+	if (bp->caps & MACB_CAPS_WOL) {
+		wol->supported = WAKE_ARP;
+
+		if (bp->wol & MACB_WOL_ENABLED)
+			wol->wolopts |= WAKE_ARP;
+	}
 }
 
 static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
@@ -2660,10 +2717,11 @@ static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 	struct macb *bp = netdev_priv(netdev);
 
 	if (!(bp->wol & MACB_WOL_HAS_MAGIC_PACKET) ||
-	    (wol->wolopts & ~WAKE_MAGIC))
+	    !(bp->caps & MACB_CAPS_WOL) ||
+	    (wol->wolopts & ~WAKE_MAGIC) || (wol->wolopts & ~WAKE_ARP))
 		return -EOPNOTSUPP;
 
-	if (wol->wolopts & WAKE_MAGIC)
+	if (wol->wolopts & (WAKE_MAGIC | WAKE_ARP))
 		bp->wol |= MACB_WOL_ENABLED;
 	else
 		bp->wol &= ~MACB_WOL_ENABLED;
@@ -3895,7 +3953,7 @@ static const struct macb_config np4_config = {
 static const struct macb_config zynqmp_config = {
 	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
 			MACB_CAPS_JUMBO |
-			MACB_CAPS_GEM_HAS_PTP,
+			MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_WOL,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,
@@ -4093,6 +4151,9 @@ static int macb_probe(struct platform_device *pdev)
 
 	phy_attached_info(phydev);
 
+	if (bp->caps & MACB_CAPS_WOL)
+		device_set_wakeup_capable(&bp->dev->dev, 1);
+
 	netdev_info(dev, "Cadence %s rev 0x%08x at 0x%08lx irq %d (%pM)\n",
 		    macb_is_gem(bp) ? "GEM" : "MACB", macb_readl(bp, MID),
 		    dev->base_addr, dev->irq, dev->dev_addr);
@@ -4170,16 +4231,58 @@ static int __maybe_unused macb_suspend(struct device *dev)
 	struct macb_queue *queue = bp->queues;
 	unsigned long flags;
 	unsigned int q;
+	u32 ctrl, arpipmask;
 
 	if (!netif_running(netdev))
 		return 0;
 
 
-	if (bp->wol & MACB_WOL_ENABLED) {
+	if ((bp->wol & MACB_WOL_ENABLED) &&
+	    (bp->wol & MACB_WOL_HAS_MAGIC_PACKET)) {
 		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 if (device_may_wakeup(&bp->dev->dev)) {
+		/* Use ARP as default wake source */
+		spin_lock_irqsave(&bp->lock, flags);
+		ctrl = macb_readl(bp, NCR);
+		/* Disable TX as is it not required;
+		 * Disable RX to change BD pointers and enable again
+		 */
+		ctrl &= ~(MACB_BIT(TE) | MACB_BIT(RE));
+		macb_writel(bp, NCR, ctrl);
+		/* Tie all RX queues */
+		macb_rx_tieoff(bp);
+		ctrl = macb_readl(bp, NCR);
+		ctrl |= MACB_BIT(RE);
+		macb_writel(bp, NCR, ctrl);
+		/* Broadcast should be enabled for ARP wake event */
+		gem_writel(bp, NCFGR, gem_readl(bp, NCFGR) & ~MACB_BIT(NBC));
+		macb_writel(bp, TSR, -1);
+		macb_writel(bp, RSR, -1);
+		macb_readl(bp, ISR);
+		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+			macb_writel(bp, ISR, -1);
+
+		/* Enable WOL (Q0 only) and disable all other interrupts */
+		queue = bp->queues;
+		queue_writel(queue, IER, GEM_BIT(WOL));
+		for (q = 0, queue = bp->queues; q < bp->num_queues;
+		     ++q, ++queue) {
+			queue_writel(queue, IDR, MACB_RX_INT_FLAGS |
+						 MACB_TX_INT_FLAGS |
+						 MACB_BIT(HRESP));
+		}
+
+		arpipmask = cpu_to_be32p(&bp->dev->ip_ptr->ifa_list->ifa_local)
+					 & 0xFFFF;
+		gem_writel(bp, WOL, MACB_BIT(ARP) | arpipmask);
+		spin_unlock_irqrestore(&bp->lock, flags);
+		enable_irq_wake(bp->queues[0].irq);
+		netif_device_detach(netdev);
+		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
+			napi_disable(&queue->napi);
 	} else {
 		netif_device_detach(netdev);
 		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
@@ -4206,16 +4309,33 @@ static int __maybe_unused macb_resume(struct device *dev)
 	struct macb *bp = netdev_priv(netdev);
 	struct macb_queue *queue = bp->queues;
 	unsigned int q;
+	unsigned long flags;
 
 	if (!netif_running(netdev))
 		return 0;
 
 	pm_runtime_force_resume(dev);
 
-	if (bp->wol & MACB_WOL_ENABLED) {
+	if ((bp->wol & MACB_WOL_ENABLED) &&
+	    (bp->wol & MACB_WOL_HAS_MAGIC_PACKET)) {
 		macb_writel(bp, IDR, MACB_BIT(WOL));
 		macb_writel(bp, WOL, 0);
 		disable_irq_wake(bp->queues[0].irq);
+	} else if (device_may_wakeup(&bp->dev->dev)) {
+		/* Resume after ARP wake event */
+		spin_lock_irqsave(&bp->lock, flags);
+		queue = bp->queues;
+		queue_writel(queue, IDR, GEM_BIT(WOL));
+		gem_writel(bp, WOL, 0);
+		/* Clear Q0 ISR as WOL was enabled on Q0 */
+		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+			macb_writel(bp, ISR, -1);
+		disable_irq_wake(bp->queues[0].irq);
+		spin_unlock_irqrestore(&bp->lock, flags);
+		macb_writel(bp, NCR, MACB_BIT(MPE));
+		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
+			napi_enable(&queue->napi);
+		netif_carrier_on(netdev);
 	} else {
 		macb_writel(bp, NCR, MACB_BIT(MPE));
 		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
-- 
2.7.4

Re: [RFC PATCH 5/5] net: macb: Add WOL support with ARP

[Claudiu Beznea]

On 22.03.2018 15:51, harinikatakamlinux@gmail.com wrote:
> From: Harini Katakam <harinik@xilinx.com>
> 
> This patch enables ARP wake event support in GEM through the following:
> 
> -> WOL capability can be selected based on the SoC/GEM IP version rather
> than a devictree property alone. Hence add a new capability property and
> set device as "wakeup capable" in probe in this case.
> -> Wake source selection can be done via ethtool or by enabling wakeup
> in /sys/devices/platform/..../ethx/power/
> This patch adds default wake source as ARP and the existing selection of
> WOL using magic packet remains unchanged.
> -> When GEM is the wake device with ARP as the wake event, the current
> IP address to match is written to WOL register along with other
> necessary confuguration required for MAC to recognize an ARP event.
> -> While RX needs to remain enabled, there is no need to process the
> actual wake packet - hence tie off all RX queues to avoid unnecessary
> processing by DMA in the background. 

Why is this different for magic packet vs ARP packet?

This tie off is done using a
> dummy buffer descriptor with used bit set. (There is no other provision
> to disable RX DMA in the GEM IP version in ZynqMP)
> -> TX is disabled and all interrupts except WOL on Q0 are disabled.
> Clear the WOL interrupt as no other action is required from driver.
> Power management of the SoC will already have got the event and will
> take care of initiating resume.
> -> Upon resume ARP WOL config is cleared and macb is reinitialized.
> 
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
> ---
>  drivers/net/ethernet/cadence/macb.h      |   6 ++
>  drivers/net/ethernet/cadence/macb_main.c | 130 +++++++++++++++++++++++++++++--
>  2 files changed, 131 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 9e7fb14..e18ff34 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -93,6 +93,7 @@
>  #define GEM_SA3T		0x009C /* Specific3 Top */
>  #define GEM_SA4B		0x00A0 /* Specific4 Bottom */
>  #define GEM_SA4T		0x00A4 /* Specific4 Top */
> +#define GEM_WOL			0x00B8 /* Wake on LAN */
>  #define GEM_EFTSH		0x00e8 /* PTP Event Frame Transmitted Seconds Register 47:32 */
>  #define GEM_EFRSH		0x00ec /* PTP Event Frame Received Seconds Register 47:32 */
>  #define GEM_PEFTSH		0x00f0 /* PTP Peer Event Frame Transmitted Seconds Register 47:32 */
> @@ -398,6 +399,8 @@
>  #define MACB_PDRSFT_SIZE	1
>  #define MACB_SRI_OFFSET		26 /* TSU Seconds Register Increment */
>  #define MACB_SRI_SIZE		1
> +#define GEM_WOL_OFFSET		28 /* Enable wake-on-lan interrupt in GEM */
> +#define GEM_WOL_SIZE		1
>  
>  /* Timer increment fields */
>  #define MACB_TI_CNS_OFFSET	0
> @@ -635,6 +638,7 @@
>  #define MACB_CAPS_USRIO_DISABLED		0x00000010
>  #define MACB_CAPS_JUMBO				0x00000020
>  #define MACB_CAPS_GEM_HAS_PTP			0x00000040
> +#define MACB_CAPS_WOL				0x00000080

I think would be better to have this as part of bp->wol and use it properly
in suspend/resume hooks.

>  #define MACB_CAPS_FIFO_MODE			0x10000000
>  #define MACB_CAPS_GIGABIT_MODE_AVAILABLE	0x20000000
>  #define MACB_CAPS_SG_DISABLED			0x40000000
> @@ -1147,6 +1151,8 @@ struct macb {
>  	unsigned int		num_queues;
>  	unsigned int		queue_mask;
>  	struct macb_queue	queues[MACB_MAX_QUEUES];
> +	dma_addr_t		rx_ring_tieoff_dma;
> +	struct macb_dma_desc	*rx_ring_tieoff;
>  
>  	spinlock_t		lock;
>  	struct platform_device	*pdev;
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index bca91bd..9902654 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -36,6 +36,7 @@
>  #include <linux/udp.h>
>  #include <linux/tcp.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/inetdevice.h>
>  #include "macb.h"
>  
>  #define MACB_RX_BUFFER_SIZE	128
> @@ -1400,6 +1401,12 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
>  	spin_lock(&bp->lock);
>  
>  	while (status) {
> +		if (status & GEM_BIT(WOL)) {
> +			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
> +				queue_writel(queue, ISR, GEM_BIT(WOL));
> +			break;
> +		}
> +
>  		/* close possible race with dev_close */
>  		if (unlikely(!netif_running(dev))) {
>  			queue_writel(queue, IDR, -1);
> @@ -1900,6 +1907,12 @@ static void macb_free_consistent(struct macb *bp)
>  		queue->rx_ring = NULL;
>  	}
>  
> +	if (bp->rx_ring_tieoff) {
> +		dma_free_coherent(&bp->pdev->dev, macb_dma_desc_get_size(bp),
> +				  bp->rx_ring_tieoff, bp->rx_ring_tieoff_dma);
> +		bp->rx_ring_tieoff = NULL;
> +	}
> +
>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>  		kfree(queue->tx_skb);
>  		queue->tx_skb = NULL;
> @@ -1979,6 +1992,14 @@ static int macb_alloc_consistent(struct macb *bp)
>  			   "Allocated RX ring of %d bytes at %08lx (mapped %p)\n",
>  			   size, (unsigned long)queue->rx_ring_dma, queue->rx_ring);
>  	}
> +	/* Allocate one dummy descriptor to tie off RX queues when required */
> +	bp->rx_ring_tieoff = dma_alloc_coherent(&bp->pdev->dev,
> +						macb_dma_desc_get_size(bp),
> +						&bp->rx_ring_tieoff_dma,
> +						GFP_KERNEL);
> +	if (!bp->rx_ring_tieoff)
> +		goto out_err;
> +
>  	if (bp->macbgem_ops.mog_alloc_rx_buffers(bp))
>  		goto out_err;
>  
> @@ -1989,6 +2010,34 @@ static int macb_alloc_consistent(struct macb *bp)
>  	return -ENOMEM;
>  }
>  
> +static void macb_init_tieoff(struct macb *bp)
> +{
> +	struct macb_dma_desc *d = bp->rx_ring_tieoff;
> +
> +	/* Setup a wrapping descriptor with no free slots
> +	 * (WRAP and USED) to tie off/disable unused RX queues.
> +	 */
> +	macb_set_addr(bp, d, MACB_BIT(RX_WRAP) | MACB_BIT(RX_USED));
> +	d->ctrl = 0;
> +}
> +
> +static inline void macb_rx_tieoff(struct macb *bp)
> +{
> +	struct macb_queue *queue = bp->queues;
> +	unsigned int q;
> +
> +	for (q = 0, queue = bp->queues; q < bp->num_queues;
> +	     ++q, ++queue) {
> +		queue_writel(queue, RBQP,
> +			     lower_32_bits(bp->rx_ring_tieoff_dma));
> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> +		if (bp->hw_dma_cap & HW_DMA_CAP_64B)
> +			queue_writel(queue, RBQPH,
> +				     upper_32_bits(bp->rx_ring_tieoff_dma));
> +#endif
> +	}
> +}
> +
>  static void gem_init_rings(struct macb *bp)
>  {
>  	struct macb_queue *queue;
> @@ -2011,6 +2060,7 @@ static void gem_init_rings(struct macb *bp)
>  
>  		gem_rx_refill(queue);
>  	}
> +	macb_init_tieoff(bp);
>  
>  }
>  
> @@ -2653,6 +2703,13 @@ static void macb_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
>  		if (bp->wol & MACB_WOL_ENABLED)
>  			wol->wolopts |= WAKE_MAGIC;
>  	}
> +
> +	if (bp->caps & MACB_CAPS_WOL) {
> +		wol->supported = WAKE_ARP;
> +
> +		if (bp->wol & MACB_WOL_ENABLED)
> +			wol->wolopts |= WAKE_ARP;
> +	}
>  }
>  
>  static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
> @@ -2660,10 +2717,11 @@ static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
>  	struct macb *bp = netdev_priv(netdev);
>  
>  	if (!(bp->wol & MACB_WOL_HAS_MAGIC_PACKET) ||
> -	    (wol->wolopts & ~WAKE_MAGIC))
> +	    !(bp->caps & MACB_CAPS_WOL) ||
> +	    (wol->wolopts & ~WAKE_MAGIC) || (wol->wolopts & ~WAKE_ARP))
>  		return -EOPNOTSUPP;

So, both flags WAKE_MAGIC and WAKE_ARP needs to be set in order to use
Wake on Lan? Shouldn't this be exclusive? I mean if only one is set to
use that one?

Also, wouldn't be better to have all Wake on LAN capabilities in the same
place? I mean either bp->wol or bp->caps??


>  
> -	if (wol->wolopts & WAKE_MAGIC)
> +	if (wol->wolopts & (WAKE_MAGIC | WAKE_ARP))
>  		bp->wol |= MACB_WOL_ENABLED;
>  	else
>  		bp->wol &= ~MACB_WOL_ENABLED;
> @@ -3895,7 +3953,7 @@ static const struct macb_config np4_config = {
>  static const struct macb_config zynqmp_config = {
>  	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
>  			MACB_CAPS_JUMBO |
> -			MACB_CAPS_GEM_HAS_PTP,
> +			MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_WOL,
>  	.dma_burst_length = 16,
>  	.clk_init = macb_clk_init,
>  	.init = macb_init,
> @@ -4093,6 +4151,9 @@ static int macb_probe(struct platform_device *pdev)
>  
>  	phy_attached_info(phydev);
>  
> +	if (bp->caps & MACB_CAPS_WOL)
> +		device_set_wakeup_capable(&bp->dev->dev, 1);
> +

I think it is better to have this in bp->wol to keep all the wakeup related
events in the same place.

>  	netdev_info(dev, "Cadence %s rev 0x%08x at 0x%08lx irq %d (%pM)\n",
>  		    macb_is_gem(bp) ? "GEM" : "MACB", macb_readl(bp, MID),
>  		    dev->base_addr, dev->irq, dev->dev_addr);
> @@ -4170,16 +4231,58 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  	struct macb_queue *queue = bp->queues;
>  	unsigned long flags;
>  	unsigned int q;
> +	u32 ctrl, arpipmask;
>  
>  	if (!netif_running(netdev))
>  		return 0;
>  
>  
> -	if (bp->wol & MACB_WOL_ENABLED) {
> +	if ((bp->wol & MACB_WOL_ENABLED) &&
> +	    (bp->wol & MACB_WOL_HAS_MAGIC_PACKET)) {

If you will also introduce the other 2 wakeup supported sources of GEM GXL you
will end up having also a new else and conditioning device_may_wakeup() below
if-else condition with a bp->caps & MACB_CAPS_WOL.

I thinking that having something like this will be simpler:
	if (bp->wol & MACB_WOL_ENABLED) {
		if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET)
			macb_configure_magic_pkt();
		if (bp->wol & MACB_WOL_HAS_ARP_PACKET)
			macb_configure_arp_pkt();
	}

What do you think?

>  		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 if (device_may_wakeup(&bp->dev->dev)) {
> +		/* Use ARP as default wake source */
> +		spin_lock_irqsave(&bp->lock, flags);
> +		ctrl = macb_readl(bp, NCR);
> +		/* Disable TX as is it not required;
> +		 * Disable RX to change BD pointers and enable again
> +		 */
> +		ctrl &= ~(MACB_BIT(TE) | MACB_BIT(RE));
> +		macb_writel(bp, NCR, ctrl);
> +		/* Tie all RX queues */
> +		macb_rx_tieoff(bp);
> +		ctrl = macb_readl(bp, NCR);
> +		ctrl |= MACB_BIT(RE);
> +		macb_writel(bp, NCR, ctrl);
> +		/* Broadcast should be enabled for ARP wake event */
> +		gem_writel(bp, NCFGR, gem_readl(bp, NCFGR) & ~MACB_BIT(NBC));
> +		macb_writel(bp, TSR, -1);
> +		macb_writel(bp, RSR, -1);
> +		macb_readl(bp, ISR);
> +		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
> +			macb_writel(bp, ISR, -1);
> +
> +		/* Enable WOL (Q0 only) and disable all other interrupts */
> +		queue = bp->queues;
> +		queue_writel(queue, IER, GEM_BIT(WOL));
> +		for (q = 0, queue = bp->queues; q < bp->num_queues;
> +		     ++q, ++queue) {
> +			queue_writel(queue, IDR, MACB_RX_INT_FLAGS |
> +						 MACB_TX_INT_FLAGS |
> +						 MACB_BIT(HRESP));
> +		}
> +
> +		arpipmask = cpu_to_be32p(&bp->dev->ip_ptr->ifa_list->ifa_local)
> +					 & 0xFFFF;
> +		gem_writel(bp, WOL, MACB_BIT(ARP) | arpipmask);
> +		spin_unlock_irqrestore(&bp->lock, flags);
> +		enable_irq_wake(bp->queues[0].irq);
> +		netif_device_detach(netdev);
> +		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
> +			napi_disable(&queue->napi);

Is all this really necessary?
I mean, wouldn't be enough the adaption of previous approach? Looking over the initial
approach we have this:

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

Wouldn't it work if you will change it in something like this:

	u32 wolmask, arpipmask = 0;

	if (bp->wol & MACB_WOL_ENABLED) {
		macb_writel(bp, IER, MACB_BIT(WOL));

		if (bp->wol & MACB_WOL_HAS_ARP_PACKET) {
			/* Enable broadcast. */
			gem_writel(bp, NCFGR, gem_readl(bp, NCFGR) & ~MACB_BIT(NBC));
			arpipmask = cpu_to_be32p(&bp->dev->ip_ptr->ifa_list->ifa_local) & 0xFFFF;
			wolmask = arpipmask | MACB_BIT(ARP);
		} else {
			wolmask = MACB_BIT(MAG);
		}

		macb_writel(bp, WOL, wolmask);
		enable_irq_wake(bp->queues[0].irq);
		netif_device_detach(netdev);
	}

I cannot find anything particular for ARP WOL events in datasheet. Also, 
I cannot find something related to DMA activity while WOL is active

Thank you,
Claudiu

>  	} else {
>  		netif_device_detach(netdev);
>  		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
> @@ -4206,16 +4309,33 @@ static int __maybe_unused macb_resume(struct device *dev)
>  	struct macb *bp = netdev_priv(netdev);
>  	struct macb_queue *queue = bp->queues;
>  	unsigned int q;
> +	unsigned long flags;
>  
>  	if (!netif_running(netdev))
>  		return 0;
>  
>  	pm_runtime_force_resume(dev);
>  
> -	if (bp->wol & MACB_WOL_ENABLED) {
> +	if ((bp->wol & MACB_WOL_ENABLED) &&
> +	    (bp->wol & MACB_WOL_HAS_MAGIC_PACKET)) {
>  		macb_writel(bp, IDR, MACB_BIT(WOL));
>  		macb_writel(bp, WOL, 0);
>  		disable_irq_wake(bp->queues[0].irq);
> +	} else if (device_may_wakeup(&bp->dev->dev)) {
> +		/* Resume after ARP wake event */
> +		spin_lock_irqsave(&bp->lock, flags);
> +		queue = bp->queues;
> +		queue_writel(queue, IDR, GEM_BIT(WOL));
> +		gem_writel(bp, WOL, 0);
> +		/* Clear Q0 ISR as WOL was enabled on Q0 */
> +		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
> +			macb_writel(bp, ISR, -1);
> +		disable_irq_wake(bp->queues[0].irq);
> +		spin_unlock_irqrestore(&bp->lock, flags);
> +		macb_writel(bp, NCR, MACB_BIT(MPE));
> +		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
> +			napi_enable(&queue->napi);
> +		netif_carrier_on(netdev);
>  	} else {
>  		macb_writel(bp, NCR, MACB_BIT(MPE));
>  		for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue)
> 

Re: [RFC PATCH 5/5] net: macb: Add WOL support with ARP

[Harini Katakam]

Hi Claudiu,

On Fri, May 4, 2018 at 5:47 PM, Claudiu Beznea
<Claudiu.Beznea@microchip.com> wrote:
>
>
> On 22.03.2018 15:51, harinikatakamlinux@gmail.com wrote:
>> From: Harini Katakam <harinik@xilinx.com>
>>
>> This patch enables ARP wake event support in GEM through the following:
>>
>> -> WOL capability can be selected based on the SoC/GEM IP version rather
>> than a devictree property alone. Hence add a new capability property and
>> set device as "wakeup capable" in probe in this case.
>> -> Wake source selection can be done via ethtool or by enabling wakeup
>> in /sys/devices/platform/..../ethx/power/
>> This patch adds default wake source as ARP and the existing selection of
>> WOL using magic packet remains unchanged.
>> -> When GEM is the wake device with ARP as the wake event, the current
>> IP address to match is written to WOL register along with other
>> necessary confuguration required for MAC to recognize an ARP event.
>> -> While RX needs to remain enabled, there is no need to process the
>> actual wake packet - hence tie off all RX queues to avoid unnecessary
>> processing by DMA in the background.
>
> Why is this different for magic packet vs ARP packet?

This should ideally be the same whether it is a magic packet or ARP on
the version of the IP we use (more details in my comment below).
I simply did not alter the magic packet code for now to avoid breaking
others' flow.

<snip>
>> +#define MACB_CAPS_WOL                                0x00000080
>
> I think would be better to have this as part of bp->wol and use it properly
> in suspend/resume hooks.

I think a capability flag as part of config structure is better
because this is clearly an SoC related feature and there is no need
to have a devicetree property.

<snip>
> Wouldn't it work if you will change it in something like this:
>
>         u32 wolmask, arpipmask = 0;
>
>         if (bp->wol & MACB_WOL_ENABLED) {
>                 macb_writel(bp, IER, MACB_BIT(WOL));
>
>                 if (bp->wol & MACB_WOL_HAS_ARP_PACKET) {
>                         /* Enable broadcast. */
>                         gem_writel(bp, NCFGR, gem_readl(bp, NCFGR) & ~MACB_BIT(NBC));
>                         arpipmask = cpu_to_be32p(&bp->dev->ip_ptr->ifa_list->ifa_local) & 0xFFFF;
>                         wolmask = arpipmask | MACB_BIT(ARP);
>                 } else {
>                         wolmask = MACB_BIT(MAG);
>                 }
>
>                 macb_writel(bp, WOL, wolmask);
>                 enable_irq_wake(bp->queues[0].irq);

The above would work. But I'd still have to add the RX BD changes
and then stop the phy, disable interrupt etc., for most optimal power
down state - the idea is to keep only is essential to detect a wake event.

>                 netif_device_detach(netdev);
>         }
>
> I cannot find anything particular for ARP WOL events in datasheet. Also,
> I cannot find something related to DMA activity while WOL is active

Can you please let me know which version you are referring to?
ZynqMP uses the IP version r1p06 or 07.

There is a clear set of rules for ARP wake event to be recognized.

About the DMA activity, it is not explicitly mentioned but we have
observed that the DMA will continue to process incoming packets
and try to write them to the memory and Cadence has confirmed
the same. Later versions of the IP may have some provision to
stop DMA activity on RX channel but unfortunately in this version,
using a dummy RX buffer descriptor is the only way.

I'm looking into your other suggestions.
Thanks, will get back to you.

Regards,
Harini

Re: [RFC PATCH 5/5] net: macb: Add WOL support with ARP

[Claudiu Beznea]

Hi Harini,

On 10.05.2018 13:37, Harini Katakam wrote:
> Hi Claudiu,
> 
> On Fri, May 4, 2018 at 5:47 PM, Claudiu Beznea
> <Claudiu.Beznea@microchip.com> wrote:
>>
>>
>> On 22.03.2018 15:51, harinikatakamlinux@gmail.com wrote:
>>> From: Harini Katakam <harinik@xilinx.com>
>>>
>>> This patch enables ARP wake event support in GEM through the following:
>>>
>>> -> WOL capability can be selected based on the SoC/GEM IP version rather
>>> than a devictree property alone. Hence add a new capability property and
>>> set device as "wakeup capable" in probe in this case.
>>> -> Wake source selection can be done via ethtool or by enabling wakeup
>>> in /sys/devices/platform/..../ethx/power/
>>> This patch adds default wake source as ARP and the existing selection of
>>> WOL using magic packet remains unchanged.
>>> -> When GEM is the wake device with ARP as the wake event, the current
>>> IP address to match is written to WOL register along with other
>>> necessary confuguration required for MAC to recognize an ARP event.
>>> -> While RX needs to remain enabled, there is no need to process the
>>> actual wake packet - hence tie off all RX queues to avoid unnecessary
>>> processing by DMA in the background.
>>
>> Why is this different for magic packet vs ARP packet?
> 
> This should ideally be the same whether it is a magic packet or ARP on
> the version of the IP we use (more details in my comment below).
> I simply did not alter the magic packet code for now to avoid breaking
> others' flow.

I see your point. But in the end the code should be nice and clean.

>
> <snip>
>>> +#define MACB_CAPS_WOL                                0x00000080
>>
>> I think would be better to have this as part of bp->wol and use it properly
>> in suspend/resume hooks.
> 
> I think a capability flag as part of config structure is better
> because this is clearly an SoC related feature and there is no need
> to have a devicetree property.

Since there is no difference b/w ARP and magic packet in term of SoC,
I mean, there is no special treatment b/w them, I think we should treat
them both in the same way. This was my point.

> 
> <snip>
>> Wouldn't it work if you will change it in something like this:
>>
>>         u32 wolmask, arpipmask = 0;
>>
>>         if (bp->wol & MACB_WOL_ENABLED) {
>>                 macb_writel(bp, IER, MACB_BIT(WOL));
>>
>>                 if (bp->wol & MACB_WOL_HAS_ARP_PACKET) {
>>                         /* Enable broadcast. */
>>                         gem_writel(bp, NCFGR, gem_readl(bp, NCFGR) & ~MACB_BIT(NBC));
>>                         arpipmask = cpu_to_be32p(&bp->dev->ip_ptr->ifa_list->ifa_local) & 0xFFFF;
>>                         wolmask = arpipmask | MACB_BIT(ARP);
>>                 } else {
>>                         wolmask = MACB_BIT(MAG);
>>                 }
>>
>>                 macb_writel(bp, WOL, wolmask);
>>                 enable_irq_wake(bp->queues[0].irq);
> 
> The above would work. But I'd still have to add the RX BD changes
> and then stop the phy, disable interrupt etc., for most optimal power
> down state - the idea is to keep only is essential to detect a wake event.
> 

Also, with your approach the suspend/resume time will be longer.

>>                 netif_device_detach(netdev);
>>         }
>>
>> I cannot find anything particular for ARP WOL events in datasheet. Also,
>> I cannot find something related to DMA activity while WOL is active
> 
> Can you please let me know which version you are referring to?
> ZynqMP uses the IP version r1p06 or 07.

I have user guide revision 15
.

> 
> There is a clear set of rules for ARP wake event to be recognized.
> 
> About the DMA activity, it is not explicitly mentioned but we have
> observed that the DMA will continue to process incoming packets
> and try to write them to the memory and Cadence has confirmed
> the same. Later versions of the IP may have some provision to
> stop DMA activity on RX channel but unfortunately in this version,
> using a dummy RX buffer descriptor is the only way.>
> I'm looking into your other suggestions.
> Thanks, will get back to you.
> 
> Regards,
> Harini
>