[PATCH 0/5] net: macb: Wake-on-Lan magic packet fixes and GEM handling

[PATCH 0/5] net: macb: Wake-on-Lan magic packet fixes and GEM handling

[nicolas.ferre]

From: Nicolas Ferre <nicolas.ferre@microchip.com>

Hi,
Here are some of my patches in order to fix WoL magic-packet on the current
macb driver.
I also add, in the second part of this series the feature to GEM types of IPs.
Please tell me if they should be separated; but the two last patches cannot go
without the 3 fixes first ones.

MACB and GEM code must co-exist and as they don't share exactly the same
register layout, I had to specialize a bit the suspend/resume paths and plug a
specific IRQ handler in order to avoid overloading the "normal" IRQ hot path.

The use of dumb buffers for RX that Harini implemented in [1] might
need to be considered for a follow-up patch series in order to address
lower-power modes on some of the platforms.
For instance, I didn't have to implement dumb buffers for some of the simpler
ARM9 platforms using MACB+FIFO types of controllers.

Please give feedback. Best regards,
  Nicolas

[1]:
https://github.com/Xilinx/linux-xlnx/commit/e9648006e8d9132db2594e50e700af362b3c9226#diff-41909d180431659ccc1229aa30fd4e5a
https://github.com/Xilinx/linux-xlnx/commit/60a21c686f7e4e50489ae04b9bb1980b145e52ef


Nicolas Ferre (5):
  net: macb: fix wakeup test in runtime suspend/resume routines
  net: macb: mark device wake capable when "magic-packet" property
    present
  net: macb: fix macb_get/set_wol() when moving to phylink
  net: macb: WoL support for GEM type of Ethernet controller
  net: macb: Add WoL interrupt support for MACB type of Ethernet
    controller

 drivers/net/ethernet/cadence/macb.h      |   3 +
 drivers/net/ethernet/cadence/macb_main.c | 181 +++++++++++++++++++----
 2 files changed, 158 insertions(+), 26 deletions(-)

-- 
2.20.1

[PATCH 1/5] net: macb: fix wakeup test in runtime suspend/resume routines

[nicolas.ferre]

From: Nicolas Ferre <nicolas.ferre@microchip.com>

Use the proper struct device pointer to check if the wakeup flag
and wakeup source are positioned.
Use the one passed by function call which is equivalent to
&bp->dev->dev.parent.

It's preventing the trigger of a spurious interrupt in case the
Wake-on-Lan feature is used.

Fixes: bc1109d04c39 ("net: macb: Add pm runtime support")
Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
Cc: Harini Katakam <harini.katakam@xilinx.com>
Cc: Rafal Ozieblo <rafalo@cadence.com>
Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index a0e8c5bbabc0..d1b4d6b6d7c8 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4616,7 +4616,7 @@ static int __maybe_unused macb_runtime_suspend(struct device *dev)
 	struct net_device *netdev = dev_get_drvdata(dev);
 	struct macb *bp = netdev_priv(netdev);
 
-	if (!(device_may_wakeup(&bp->dev->dev))) {
+	if (!(device_may_wakeup(dev))) {
 		clk_disable_unprepare(bp->tx_clk);
 		clk_disable_unprepare(bp->hclk);
 		clk_disable_unprepare(bp->pclk);
@@ -4632,7 +4632,7 @@ static int __maybe_unused macb_runtime_resume(struct device *dev)
 	struct net_device *netdev = dev_get_drvdata(dev);
 	struct macb *bp = netdev_priv(netdev);
 
-	if (!(device_may_wakeup(&bp->dev->dev))) {
+	if (!(device_may_wakeup(dev))) {
 		clk_prepare_enable(bp->pclk);
 		clk_prepare_enable(bp->hclk);
 		clk_prepare_enable(bp->tx_clk);
-- 
2.20.1

RE: [PATCH 1/5] net: macb: fix wakeup test in runtime suspend/resume routines

[Harini Katakam]

Hi Nicolas,

> -----Original Message-----
> From: nicolas.ferre@microchip.com [mailto:nicolas.ferre@microchip.com]
> Sent: Thursday, April 16, 2020 11:14 PM
> To: linux-arm-kernel@lists.infradead.org; netdev@vger.kernel.org; Claudiu
> Beznea <claudiu.beznea@microchip.com>; Harini Katakam
> <harinik@xilinx.com>
> Cc: linux-kernel@vger.kernel.org; David S. Miller <davem@davemloft.net>;
> Alexandre Belloni <alexandre.belloni@bootlin.com>; pthombar@cadence.com;
> sergio.prado@e-labworks.com; antoine.tenart@bootlin.com;
> f.fainelli@gmail.com; linux@armlinux.org.uk; andrew@lunn.ch; Michal Simek
> <michals@xilinx.com>; Nicolas Ferre <nicolas.ferre@microchip.com>; Rafal
> Ozieblo <rafalo@cadence.com>
> Subject: [PATCH 1/5] net: macb: fix wakeup test in runtime suspend/resume
> routines
> 
> From: Nicolas Ferre <nicolas.ferre@microchip.com>
> 
> Use the proper struct device pointer to check if the wakeup flag and wakeup
> source are positioned.
> Use the one passed by function call which is equivalent to &bp->dev-
> >dev.parent.
> 
> It's preventing the trigger of a spurious interrupt in case the Wake-on-Lan
> feature is used.

Sorry I have some mail issues; meant to reply earlier.
Tested patches 1, 2, 3 in this set and they work for me.

I'll try patch 4; it looks similar to what I'm using locally but I'll add whatever
tie-off queue handling is required on top of your series, thanks.

Regards,
Harini

Re: [PATCH 1/5] net: macb: fix wakeup test in runtime suspend/resume routines

[Nicolas Ferre]

On 16/04/2020 at 20:26, Harini Katakam wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Nicolas,
> 
>> -----Original Message-----
>> From: nicolas.ferre@microchip.com [mailto:nicolas.ferre@microchip.com]
>> Sent: Thursday, April 16, 2020 11:14 PM
>> To: linux-arm-kernel@lists.infradead.org; netdev@vger.kernel.org; Claudiu
>> Beznea <claudiu.beznea@microchip.com>; Harini Katakam
>> <harinik@xilinx.com>
>> Cc: linux-kernel@vger.kernel.org; David S. Miller <davem@davemloft.net>;
>> Alexandre Belloni <alexandre.belloni@bootlin.com>; pthombar@cadence.com;
>> sergio.prado@e-labworks.com; antoine.tenart@bootlin.com;
>> f.fainelli@gmail.com; linux@armlinux.org.uk; andrew@lunn.ch; Michal Simek
>> <michals@xilinx.com>; Nicolas Ferre <nicolas.ferre@microchip.com>; Rafal
>> Ozieblo <rafalo@cadence.com>
>> Subject: [PATCH 1/5] net: macb: fix wakeup test in runtime suspend/resume
>> routines
>>
>> From: Nicolas Ferre <nicolas.ferre@microchip.com>
>>
>> Use the proper struct device pointer to check if the wakeup flag and wakeup
>> source are positioned.
>> Use the one passed by function call which is equivalent to &bp->dev-
>>> dev.parent.
>>
>> It's preventing the trigger of a spurious interrupt in case the Wake-on-Lan
>> feature is used.
> 
> Sorry I have some mail issues; meant to reply earlier.
> Tested patches 1, 2, 3 in this set and they work for me.

Brilliant! Thanks for the feedback.

> I'll try patch 4; it looks similar to what I'm using locally but I'll add whatever
> tie-off queue handling is required on top of your series, thanks.

Alright, I'll hold my v2 for a few days then. Thanks. Best regards,
   Nicolas


-- 
Nicolas Ferre

Re: [PATCH 1/5] net: macb: fix wakeup test in runtime suspend/resume routines

[Florian Fainelli]

On 4/16/2020 10:44 AM, nicolas.ferre@microchip.com wrote:
> From: Nicolas Ferre <nicolas.ferre@microchip.com>
> 
> Use the proper struct device pointer to check if the wakeup flag
> and wakeup source are positioned.
> Use the one passed by function call which is equivalent to
> &bp->dev->dev.parent.
> 
> It's preventing the trigger of a spurious interrupt in case the
> Wake-on-Lan feature is used.
> 
> Fixes: bc1109d04c39 ("net: macb: Add pm runtime support")
> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
> Cc: Harini Katakam <harini.katakam@xilinx.com>
> Cc: Rafal Ozieblo <rafalo@cadence.com>
> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>

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

[PATCH 2/5] net: macb: mark device wake capable when "magic-packet" property present

[nicolas.ferre]

From: Nicolas Ferre <nicolas.ferre@microchip.com>

Change the way the "magic-packet" DT property is handled in the
macb_probe() function, matching DT binding documentation.
Now we mark the device as "wakeup capable" instead of calling the
device_init_wakeup() function that would enable the wakeup source.

For Ethernet WoL, enabling the wakeup_source is done by
using ethtool and associated macb_set_wol() function that
already calls device_set_wakeup_enable() for this purpose.

That would reduce power consumption by cutting more clocks if
"magic-packet" property is set but WoL is not configured by ethtool.

Fixes: 3e2a5e153906 ("net: macb: add wake-on-lan support via magic packet")
Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
Cc: Harini Katakam <harini.katakam@xilinx.com>
Cc: Rafal Ozieblo <rafalo@cadence.com>
Cc: Sergio Prado <sergio.prado@e-labworks.com>
Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index d1b4d6b6d7c8..629660d9f17e 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -4384,7 +4384,7 @@ static int macb_probe(struct platform_device *pdev)
 	bp->wol = 0;
 	if (of_get_property(np, "magic-packet", NULL))
 		bp->wol |= MACB_WOL_HAS_MAGIC_PACKET;
-	device_init_wakeup(&pdev->dev, bp->wol & MACB_WOL_HAS_MAGIC_PACKET);
+	device_set_wakeup_capable(&pdev->dev, bp->wol & MACB_WOL_HAS_MAGIC_PACKET);
 
 	spin_lock_init(&bp->lock);
 
-- 
2.20.1

Re: [PATCH 2/5] net: macb: mark device wake capable when "magic-packet" property present

[Florian Fainelli]

On 4/16/2020 10:44 AM, nicolas.ferre@microchip.com wrote:
> From: Nicolas Ferre <nicolas.ferre@microchip.com>
> 
> Change the way the "magic-packet" DT property is handled in the
> macb_probe() function, matching DT binding documentation.
> Now we mark the device as "wakeup capable" instead of calling the
> device_init_wakeup() function that would enable the wakeup source.
> 
> For Ethernet WoL, enabling the wakeup_source is done by
> using ethtool and associated macb_set_wol() function that
> already calls device_set_wakeup_enable() for this purpose.
> 
> That would reduce power consumption by cutting more clocks if
> "magic-packet" property is set but WoL is not configured by ethtool.
> 
> Fixes: 3e2a5e153906 ("net: macb: add wake-on-lan support via magic packet")
> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
> Cc: Harini Katakam <harini.katakam@xilinx.com>
> Cc: Rafal Ozieblo <rafalo@cadence.com>
> Cc: Sergio Prado <sergio.prado@e-labworks.com>
> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>

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

[PATCH 3/5] net: macb: fix macb_get/set_wol() when moving to phylink

[nicolas.ferre]

From: Nicolas Ferre <nicolas.ferre@microchip.com>

Keep previous function goals and integrate phylink actions to them.

phylink_ethtool_get_wol() is not enough to figure out if Ethernet driver
supports Wake-on-Lan.
Initialization of "supported" and "wolopts" members is done in phylink
function, no need to keep them in calling function.

phylink_ethtool_set_wol() return value is not enough to determine
if WoL is enabled for the calling Ethernet driver. Call if first
but don't rely on its return value as most of simple PHY drivers
don't implement a set_wol() function.

Fixes: 7897b071ac3b ("net: macb: convert to phylink")
Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
Cc: Harini Katakam <harini.katakam@xilinx.com>
Cc: Rafal Ozieblo <rafalo@cadence.com>
Cc: Antoine Tenart <antoine.tenart@bootlin.com>
Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 629660d9f17e..b17a33c60cd4 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2813,21 +2813,23 @@ static void macb_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 {
 	struct macb *bp = netdev_priv(netdev);
 
-	wol->supported = 0;
-	wol->wolopts = 0;
-
-	if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET)
+	if (bp->wol & MACB_WOL_HAS_MAGIC_PACKET) {
 		phylink_ethtool_get_wol(bp->phylink, wol);
+		wol->supported |= WAKE_MAGIC;
+
+		if (bp->wol & MACB_WOL_ENABLED)
+			wol->wolopts |= WAKE_MAGIC;
+	}
 }
 
 static int macb_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
 {
 	struct macb *bp = netdev_priv(netdev);
-	int ret;
 
-	ret = phylink_ethtool_set_wol(bp->phylink, wol);
-	if (!ret)
-		return 0;
+	/* Pass the order to phylink layer.
+	 * Don't test return value as set_wol() is often not supported.
+	 */
+	phylink_ethtool_set_wol(bp->phylink, wol);
 
 	if (!(bp->wol & MACB_WOL_HAS_MAGIC_PACKET) ||
 	    (wol->wolopts & ~WAKE_MAGIC))
-- 
2.20.1

Re: [PATCH 3/5] net: macb: fix macb_get/set_wol() when moving to phylink

[Florian Fainelli]

On 4/16/2020 10:44 AM, nicolas.ferre@microchip.com wrote:
> From: Nicolas Ferre <nicolas.ferre@microchip.com>
> 
> Keep previous function goals and integrate phylink actions to them.
> 
> phylink_ethtool_get_wol() is not enough to figure out if Ethernet driver
> supports Wake-on-Lan.
> Initialization of "supported" and "wolopts" members is done in phylink
> function, no need to keep them in calling function.
> 
> phylink_ethtool_set_wol() return value is not enough to determine
> if WoL is enabled for the calling Ethernet driver. Call if first
> but don't rely on its return value as most of simple PHY drivers
> don't implement a set_wol() function.
> 
> Fixes: 7897b071ac3b ("net: macb: convert to phylink")
> Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
> Cc: Harini Katakam <harini.katakam@xilinx.com>
> Cc: Rafal Ozieblo <rafalo@cadence.com>
> Cc: Antoine Tenart <antoine.tenart@bootlin.com>
> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>

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

[PATCH 4/5] net: macb: WoL support for GEM type of Ethernet controller

[nicolas.ferre]

From: Nicolas Ferre <nicolas.ferre@microchip.com>

Adapt the Wake-on-Lan feature to the Cadence GEM Ethernet controller.
This controller has different register layout and cannot be handled by
previous code.
We disable completely interrupts on all the queues but the queue 0.
Handling of WoL interrupt is done in another interrupt handler
positioned depending on the controller version used, just between
suspend() and resume() calls.
It allows to lower pressure on the generic interrupt hot path by
removing the need to handle 2 tests for each IRQ: the first figuring out
the controller revision, the second for actually knowing if the WoL bit
is set.

Queue management in suspend()/resume() functions inspired from RFC patch
by Harini Katakam <harinik@xilinx.com>, thanks!

Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
 drivers/net/ethernet/cadence/macb.h      |   3 +
 drivers/net/ethernet/cadence/macb_main.c | 121 ++++++++++++++++++++---
 2 files changed, 109 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index ab827fb4b6b9..4f1b41569260 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -90,6 +90,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 */
@@ -396,6 +397,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 */
+#define GEM_WOL_SIZE		1
 
 /* Timer increment fields */
 #define MACB_TI_CNS_OFFSET	0
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index b17a33c60cd4..71e6afbdfb47 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1513,6 +1513,34 @@ static void macb_tx_restart(struct macb_queue *queue)
 	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
 }
 
+static irqreturn_t gem_wol_interrupt(int irq, void *dev_id)
+{
+	struct macb_queue *queue = dev_id;
+	struct macb *bp = queue->bp;
+	u32 status;
+
+	status = queue_readl(queue, ISR);
+
+	if (unlikely(!status))
+		return IRQ_NONE;
+
+	spin_lock(&bp->lock);
+
+	if (status & GEM_BIT(WOL)) {
+		queue_writel(queue, IDR, GEM_BIT(WOL));
+		gem_writel(bp, WOL, 0);
+		netdev_vdbg(bp->dev, "GEM WoL: queue = %u, isr = 0x%08lx\n",
+			    (unsigned int)(queue - bp->queues),
+			    (unsigned long)status);
+		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+			queue_writel(queue, ISR, GEM_BIT(WOL));
+	}
+
+	spin_unlock(&bp->lock);
+
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t macb_interrupt(int irq, void *dev_id)
 {
 	struct macb_queue *queue = dev_id;
@@ -3306,6 +3334,8 @@ static const struct ethtool_ops macb_ethtool_ops = {
 static const struct ethtool_ops gem_ethtool_ops = {
 	.get_regs_len		= macb_get_regs_len,
 	.get_regs		= macb_get_regs,
+	.get_wol		= macb_get_wol,
+	.set_wol		= macb_set_wol,
 	.get_link		= ethtool_op_get_link,
 	.get_ts_info		= macb_get_ts_info,
 	.get_ethtool_stats	= gem_get_ethtool_stats,
@@ -4534,23 +4564,56 @@ static int __maybe_unused macb_suspend(struct device *dev)
 	struct macb_queue *queue = bp->queues;
 	unsigned long flags;
 	unsigned int q;
+	int err;
 
 	if (!netif_running(netdev))
 		return 0;
 
 	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);
+		spin_lock_irqsave(&bp->lock, flags);
+		/* Flush all status bits */
+		macb_writel(bp, TSR, -1);
+		macb_writel(bp, RSR, -1);
 		for (q = 0, queue = bp->queues; q < bp->num_queues;
-		     ++q, ++queue)
-			napi_disable(&queue->napi);
-		rtnl_lock();
-		phylink_stop(bp->phylink);
-		rtnl_unlock();
+		     ++q, ++queue) {
+			/* Disable all interrupts */
+			queue_writel(queue, IDR, -1);
+			queue_readl(queue, ISR);
+			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+				queue_writel(queue, ISR, -1);
+		}
+		/* Change interrupt handler and
+		 * Enable WoL IRQ on queue 0
+		 */
+		if (macb_is_gem(bp)) {
+			devm_free_irq(dev, bp->queues[0].irq, bp->queues);
+			err = devm_request_irq(dev, bp->queues[0].irq, gem_wol_interrupt,
+					       IRQF_SHARED, netdev->name, bp->queues);
+			if (err) {
+				dev_err(dev,
+					"Unable to request IRQ %d (error %d)\n",
+					bp->queues[0].irq, err);
+				return err;
+			}
+			queue_writel(bp->queues, IER, GEM_BIT(WOL));
+			gem_writel(bp, WOL, MACB_BIT(MAG));
+		} else {
+			queue_writel(bp->queues, IER, MACB_BIT(WOL));
+			macb_writel(bp, WOL, MACB_BIT(MAG));
+		}
+		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);
+
+	if (!(bp->wol & MACB_WOL_ENABLED)) {
+		phy_stop(netdev->phydev);
+		phy_suspend(netdev->phydev);
 		spin_lock_irqsave(&bp->lock, flags);
 		macb_reset_hw(bp);
 		spin_unlock_irqrestore(&bp->lock, flags);
@@ -4575,20 +4638,48 @@ static int __maybe_unused macb_resume(struct device *dev)
 	struct net_device *netdev = dev_get_drvdata(dev);
 	struct macb *bp = netdev_priv(netdev);
 	struct macb_queue *queue = bp->queues;
+	unsigned long flags;
 	unsigned int q;
+	int err;
 
 	if (!netif_running(netdev))
 		return 0;
 
 	pm_runtime_force_resume(dev);
 
+	macb_writel(bp, NCR, MACB_BIT(MPE));
+
 	if (bp->wol & MACB_WOL_ENABLED) {
-		macb_writel(bp, IDR, MACB_BIT(WOL));
-		macb_writel(bp, WOL, 0);
+		spin_lock_irqsave(&bp->lock, flags);
+		/* Disable WoL */
+		if (macb_is_gem(bp)) {
+			queue_writel(bp->queues, IDR, GEM_BIT(WOL));
+			gem_writel(bp, WOL, 0);
+		} else {
+			queue_writel(bp->queues, IDR, MACB_BIT(WOL));
+			macb_writel(bp, WOL, 0);
+		}
+		/* Clear ISR on queue 0 */
+		queue_readl(bp->queues, ISR);
+		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+			queue_writel(bp->queues, ISR, -1);
+		/* Replace interrupt handler on queue 0 */
+		devm_free_irq(dev, bp->queues[0].irq, bp->queues);
+		err = devm_request_irq(dev, bp->queues[0].irq, macb_interrupt,
+				       IRQF_SHARED, netdev->name, bp->queues);
+		if (err) {
+			dev_err(dev,
+				"Unable to request IRQ %d (error %d)\n",
+				bp->queues[0].irq, err);
+			return err;
+		}
+		spin_unlock_irqrestore(&bp->lock, flags);
+
 		disable_irq_wake(bp->queues[0].irq);
+		for (q = 0, queue = bp->queues; q < bp->num_queues;
+		     ++q, ++queue)
+			napi_enable(&queue->napi);
 	} else {
-		macb_writel(bp, NCR, MACB_BIT(MPE));
-
 		if (netdev->hw_features & NETIF_F_NTUPLE)
 			gem_writel_n(bp, ETHT, SCRT2_ETHT, bp->pm_data.scrt2);
 
-- 
2.20.1

Re: [PATCH 4/5] net: macb: WoL support for GEM type of Ethernet controller

[Florian Fainelli]

On 4/16/2020 10:44 AM, nicolas.ferre@microchip.com wrote:
> From: Nicolas Ferre <nicolas.ferre@microchip.com>
> 
> Adapt the Wake-on-Lan feature to the Cadence GEM Ethernet controller.
> This controller has different register layout and cannot be handled by
> previous code.
> We disable completely interrupts on all the queues but the queue 0.
> Handling of WoL interrupt is done in another interrupt handler
> positioned depending on the controller version used, just between
> suspend() and resume() calls.
> It allows to lower pressure on the generic interrupt hot path by
> removing the need to handle 2 tests for each IRQ: the first figuring out
> the controller revision, the second for actually knowing if the WoL bit
> is set.
> 
> Queue management in suspend()/resume() functions inspired from RFC patch
> by Harini Katakam <harinik@xilinx.com>, thanks!
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> ---

[snip]

>   
> +static irqreturn_t gem_wol_interrupt(int irq, void *dev_id)
> +{
> +	struct macb_queue *queue = dev_id;
> +	struct macb *bp = queue->bp;
> +	u32 status;
> +
> +	status = queue_readl(queue, ISR);
> +
> +	if (unlikely(!status))
> +		return IRQ_NONE;
> +
> +	spin_lock(&bp->lock);
> +
> +	if (status & GEM_BIT(WOL)) {
> +		queue_writel(queue, IDR, GEM_BIT(WOL));
> +		gem_writel(bp, WOL, 0);
> +		netdev_vdbg(bp->dev, "GEM WoL: queue = %u, isr = 0x%08lx\n",
> +			    (unsigned int)(queue - bp->queues),
> +			    (unsigned long)status);
> +		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
> +			queue_writel(queue, ISR, GEM_BIT(WOL));

You would also need a pm_wakeup_event() call here to record that this 
device did wake-up the system.
-- 
Florian

Re: [PATCH 4/5] net: macb: WoL support for GEM type of Ethernet controller

[Nicolas Ferre]

Florian,

Thank you for your review of the series!


On 16/04/2020 at 21:25, Florian Fainelli wrote:
> On 4/16/2020 10:44 AM, nicolas.ferre@microchip.com wrote:
>> From: Nicolas Ferre <nicolas.ferre@microchip.com>
>>
>> Adapt the Wake-on-Lan feature to the Cadence GEM Ethernet controller.
>> This controller has different register layout and cannot be handled by
>> previous code.
>> We disable completely interrupts on all the queues but the queue 0.
>> Handling of WoL interrupt is done in another interrupt handler
>> positioned depending on the controller version used, just between
>> suspend() and resume() calls.
>> It allows to lower pressure on the generic interrupt hot path by
>> removing the need to handle 2 tests for each IRQ: the first figuring out
>> the controller revision, the second for actually knowing if the WoL bit
>> is set.
>>
>> Queue management in suspend()/resume() functions inspired from RFC patch
>> by Harini Katakam <harinik@xilinx.com>, thanks!
>>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
>> ---
> 
> [snip]
> 
>>
>> +static irqreturn_t gem_wol_interrupt(int irq, void *dev_id)
>> +{
>> +     struct macb_queue *queue = dev_id;
>> +     struct macb *bp = queue->bp;
>> +     u32 status;
>> +
>> +     status = queue_readl(queue, ISR);
>> +
>> +     if (unlikely(!status))
>> +             return IRQ_NONE;
>> +
>> +     spin_lock(&bp->lock);
>> +
>> +     if (status & GEM_BIT(WOL)) {
>> +             queue_writel(queue, IDR, GEM_BIT(WOL));
>> +             gem_writel(bp, WOL, 0);
>> +             netdev_vdbg(bp->dev, "GEM WoL: queue = %u, isr = 0x%08lx\n",
>> +                         (unsigned int)(queue - bp->queues),
>> +                         (unsigned long)status);
>> +             if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>> +                     queue_writel(queue, ISR, GEM_BIT(WOL));
> 
> You would also need a pm_wakeup_event() call here to record that this
> device did wake-up the system.

Oh yes, indeed that's missing. I'll add it to my v2.

Thanks. Best regards,
   Nicolas


-- 
Nicolas Ferre

Re: [PATCH 4/5] net: macb: WoL support for GEM type of Ethernet controller

[Nicolas Ferre]

On 16/04/2020 at 19:44, nicolas.ferre@microchip.com wrote:
> From: Nicolas Ferre <nicolas.ferre@microchip.com>
> 
> Adapt the Wake-on-Lan feature to the Cadence GEM Ethernet controller.
> This controller has different register layout and cannot be handled by
> previous code.
> We disable completely interrupts on all the queues but the queue 0.
> Handling of WoL interrupt is done in another interrupt handler
> positioned depending on the controller version used, just between
> suspend() and resume() calls.
> It allows to lower pressure on the generic interrupt hot path by
> removing the need to handle 2 tests for each IRQ: the first figuring out
> the controller revision, the second for actually knowing if the WoL bit
> is set.
> 
> Queue management in suspend()/resume() functions inspired from RFC patch
> by Harini Katakam <harinik@xilinx.com>, thanks!
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> ---
>   drivers/net/ethernet/cadence/macb.h      |   3 +
>   drivers/net/ethernet/cadence/macb_main.c | 121 ++++++++++++++++++++---
>   2 files changed, 109 insertions(+), 15 deletions(-)

[..]

> @@ -4534,23 +4564,56 @@ static int __maybe_unused macb_suspend(struct device *dev)
>   	struct macb_queue *queue = bp->queues;
>   	unsigned long flags;
>   	unsigned int q;
> +	int err;
>   
>   	if (!netif_running(netdev))
>   		return 0;
>   
>   	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);
> +		spin_lock_irqsave(&bp->lock, flags);
> +		/* Flush all status bits */
> +		macb_writel(bp, TSR, -1);
> +		macb_writel(bp, RSR, -1);
>   		for (q = 0, queue = bp->queues; q < bp->num_queues;
> -		     ++q, ++queue)
> -			napi_disable(&queue->napi);
> -		rtnl_lock();
> -		phylink_stop(bp->phylink);
> -		rtnl_unlock();
> +		     ++q, ++queue) {
> +			/* Disable all interrupts */
> +			queue_writel(queue, IDR, -1);
> +			queue_readl(queue, ISR);
> +			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
> +				queue_writel(queue, ISR, -1);
> +		}
> +		/* Change interrupt handler and
> +		 * Enable WoL IRQ on queue 0
> +		 */
> +		if (macb_is_gem(bp)) {
> +			devm_free_irq(dev, bp->queues[0].irq, bp->queues);
> +			err = devm_request_irq(dev, bp->queues[0].irq, gem_wol_interrupt,
> +					       IRQF_SHARED, netdev->name, bp->queues);
> +			if (err) {
> +				dev_err(dev,
> +					"Unable to request IRQ %d (error %d)\n",
> +					bp->queues[0].irq, err);
> +				return err;
> +			}
> +			queue_writel(bp->queues, IER, GEM_BIT(WOL));
> +			gem_writel(bp, WOL, MACB_BIT(MAG));
> +		} else {
> +			queue_writel(bp->queues, IER, MACB_BIT(WOL));
> +			macb_writel(bp, WOL, MACB_BIT(MAG));
> +		}
> +		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);
> +
> +	if (!(bp->wol & MACB_WOL_ENABLED)) {
> +		phy_stop(netdev->phydev);
> +		phy_suspend(netdev->phydev);

Bug here: you must read:

		rtnl_lock();
		phylink_stop(bp->phylink);
		rtnl_unlock();

Instead of the 2 previous lines. I'll correct in v2.

Sorry for the regression.


>   		spin_lock_irqsave(&bp->lock, flags);
>   		macb_reset_hw(bp);
>   		spin_unlock_irqrestore(&bp->lock, flags);
> @@ -4575,20 +4638,48 @@ static int __maybe_unused macb_resume(struct device *dev)

[..]
BTW: I have issue having a real resume event from the phy with this 
series. I'm investigating that but didn't find anything for now.

Observation #1: when the WoL is not enabled, I don't have link issue. 
But the path in suspend/resume is far more intrusive in phy state.

Observation #2: when WoL is enabled, I need to do a full ifdown/ifup 
sequence for gain access again to the link:

ip link show eth0
2: eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc pfifo_fast 
state DOWN mode DEFAULT group default qlen 1000
     link/ether 54:10:ec:be:50:b0 brd ff:ff:ff:ff:ff:ff

ifdown eth0 && ifup eth0

ip link show eth0
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast 
state UP mode DEFAULT group default qlen 1000
     link/ether 54:10:ec:be:50:b0 brd ff:ff:ff:ff:ff:ff

Observation #3: I didn't experience this behavior while playing with the 
WoL on my 4.19 kernel before porting to Mainline.

Best regards,
-- 
Nicolas Ferre

Re: [PATCH 4/5] net: macb: WoL support for GEM type of Ethernet controller

[Nicolas.Ferre]

On 17/04/2020 at 19:14, Nicolas Ferre wrote:
> On 16/04/2020 at 19:44, nicolas.ferre@microchip.com wrote:
>> From: Nicolas Ferre <nicolas.ferre@microchip.com>
>>
>> Adapt the Wake-on-Lan feature to the Cadence GEM Ethernet controller.
>> This controller has different register layout and cannot be handled by
>> previous code.
>> We disable completely interrupts on all the queues but the queue 0.
>> Handling of WoL interrupt is done in another interrupt handler
>> positioned depending on the controller version used, just between
>> suspend() and resume() calls.
>> It allows to lower pressure on the generic interrupt hot path by
>> removing the need to handle 2 tests for each IRQ: the first figuring out
>> the controller revision, the second for actually knowing if the WoL bit
>> is set.
>>
>> Queue management in suspend()/resume() functions inspired from RFC patch
>> by Harini Katakam <harinik@xilinx.com>, thanks!
>>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
>> ---
>>    drivers/net/ethernet/cadence/macb.h      |   3 +
>>    drivers/net/ethernet/cadence/macb_main.c | 121 ++++++++++++++++++++---
>>    2 files changed, 109 insertions(+), 15 deletions(-)
> 
> [..]
> 
>> @@ -4534,23 +4564,56 @@ static int __maybe_unused macb_suspend(struct device *dev)
>>    	struct macb_queue *queue = bp->queues;
>>    	unsigned long flags;
>>    	unsigned int q;
>> +	int err;
>>    
>>    	if (!netif_running(netdev))
>>    		return 0;
>>    
>>    	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);
>> +		spin_lock_irqsave(&bp->lock, flags);
>> +		/* Flush all status bits */
>> +		macb_writel(bp, TSR, -1);
>> +		macb_writel(bp, RSR, -1);
>>    		for (q = 0, queue = bp->queues; q < bp->num_queues;
>> -		     ++q, ++queue)
>> -			napi_disable(&queue->napi);
>> -		rtnl_lock();
>> -		phylink_stop(bp->phylink);
>> -		rtnl_unlock();
>> +		     ++q, ++queue) {
>> +			/* Disable all interrupts */
>> +			queue_writel(queue, IDR, -1);
>> +			queue_readl(queue, ISR);
>> +			if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
>> +				queue_writel(queue, ISR, -1);
>> +		}
>> +		/* Change interrupt handler and
>> +		 * Enable WoL IRQ on queue 0
>> +		 */
>> +		if (macb_is_gem(bp)) {
>> +			devm_free_irq(dev, bp->queues[0].irq, bp->queues);
>> +			err = devm_request_irq(dev, bp->queues[0].irq, gem_wol_interrupt,
>> +					       IRQF_SHARED, netdev->name, bp->queues);
>> +			if (err) {
>> +				dev_err(dev,
>> +					"Unable to request IRQ %d (error %d)\n",
>> +					bp->queues[0].irq, err);
>> +				return err;
>> +			}
>> +			queue_writel(bp->queues, IER, GEM_BIT(WOL));
>> +			gem_writel(bp, WOL, MACB_BIT(MAG));
>> +		} else {
>> +			queue_writel(bp->queues, IER, MACB_BIT(WOL));
>> +			macb_writel(bp, WOL, MACB_BIT(MAG));
>> +		}
>> +		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);
>> +
>> +	if (!(bp->wol & MACB_WOL_ENABLED)) {
>> +		phy_stop(netdev->phydev);
>> +		phy_suspend(netdev->phydev);
> 
> Bug here: you must read:
> 
> 		rtnl_lock();
> 		phylink_stop(bp->phylink);
> 		rtnl_unlock();
> 
> Instead of the 2 previous lines. I'll correct in v2.
> 
> Sorry for the regression.
> 
> 
>>    		spin_lock_irqsave(&bp->lock, flags);
>>    		macb_reset_hw(bp);
>>    		spin_unlock_irqrestore(&bp->lock, flags);
>> @@ -4575,20 +4638,48 @@ static int __maybe_unused macb_resume(struct device *dev)
> 
> [..]
> BTW: I have issue having a real resume event from the phy with this
> series. I'm investigating that but didn't find anything for now.
> 
> Observation #1: when the WoL is not enabled, I don't have link issue.
> But the path in suspend/resume is far more intrusive in phy state.
> 
> Observation #2: when WoL is enabled, I need to do a full ifdown/ifup
> sequence for gain access again to the link:
> 
> ip link show eth0
> 2: eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc pfifo_fast
> state DOWN mode DEFAULT group default qlen 1000
>       link/ether 54:10:ec:be:50:b0 brd ff:ff:ff:ff:ff:ff
> 
> ifdown eth0 && ifup eth0
> 
> ip link show eth0
> 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast
> state UP mode DEFAULT group default qlen 1000
>       link/ether 54:10:ec:be:50:b0 brd ff:ff:ff:ff:ff:ff
> 
> Observation #3: I didn't experience this behavior while playing with the
> WoL on my 4.19 kernel before porting to Mainline.

I've reviewed this series to fix this last issues. It's was a 
combination of runtime_pm not handled properly and a mix of 
netif_carrier_* call with phylink calls not well positioned nor balanced 
between suspend and resume.

I have a v2 series that I'm preparing for today: Harini, I prefer to 
post it now so it could avoid that you hit the same issues as me while 
testing on your platform.

Best regards,
    Nicolas


-- 
Nicolas Ferre

Re: [PATCH 4/5] net: macb: WoL support for GEM type of Ethernet controller

[Harini Katakam]

On Tue, Apr 21, 2020 at 1:55 PM <Nicolas.Ferre@microchip.com> wrote:
<snip>
> I've reviewed this series to fix this last issues. It's was a
> combination of runtime_pm not handled properly and a mix of
> netif_carrier_* call with phylink calls not well positioned nor balanced
> between suspend and resume.
>
> I have a v2 series that I'm preparing for today: Harini, I prefer to
> post it now so it could avoid that you hit the same issues as me while
> testing on your platform.

OK thanks Nicolas.

Regards,
Harini

[PATCH 5/5] net: macb: Add WoL interrupt support for MACB type of Ethernet controller

[nicolas.ferre]

From: Nicolas Ferre <nicolas.ferre@microchip.com>

Handle the Wake-on-Lan interrupt for the Cadence MACB Ethernet
controller.
As we do for the GEM version, we handle of WoL interrupt in a
specialized interrupt handler for MACB version that is positionned
just between suspend() and resume() calls.

Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 38 +++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 71e6afbdfb47..6d535e3e803c 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1513,6 +1513,34 @@ static void macb_tx_restart(struct macb_queue *queue)
 	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
 }
 
+static irqreturn_t macb_wol_interrupt(int irq, void *dev_id)
+{
+	struct macb_queue *queue = dev_id;
+	struct macb *bp = queue->bp;
+	u32 status;
+
+	status = queue_readl(queue, ISR);
+
+	if (unlikely(!status))
+		return IRQ_NONE;
+
+	spin_lock(&bp->lock);
+
+	if (status & MACB_BIT(WOL)) {
+		queue_writel(queue, IDR, MACB_BIT(WOL));
+		macb_writel(bp, WOL, 0);
+		netdev_vdbg(bp->dev, "MACB WoL: queue = %u, isr = 0x%08lx\n",
+			    (unsigned int)(queue - bp->queues),
+			    (unsigned long)status);
+		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
+			queue_writel(queue, ISR, MACB_BIT(WOL));
+	}
+
+	spin_unlock(&bp->lock);
+
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t gem_wol_interrupt(int irq, void *dev_id)
 {
 	struct macb_queue *queue = dev_id;
@@ -4585,8 +4613,8 @@ static int __maybe_unused macb_suspend(struct device *dev)
 		/* Change interrupt handler and
 		 * Enable WoL IRQ on queue 0
 		 */
+		devm_free_irq(dev, bp->queues[0].irq, bp->queues);
 		if (macb_is_gem(bp)) {
-			devm_free_irq(dev, bp->queues[0].irq, bp->queues);
 			err = devm_request_irq(dev, bp->queues[0].irq, gem_wol_interrupt,
 					       IRQF_SHARED, netdev->name, bp->queues);
 			if (err) {
@@ -4598,6 +4626,14 @@ static int __maybe_unused macb_suspend(struct device *dev)
 			queue_writel(bp->queues, IER, GEM_BIT(WOL));
 			gem_writel(bp, WOL, MACB_BIT(MAG));
 		} else {
+			err = devm_request_irq(dev, bp->queues[0].irq, macb_wol_interrupt,
+					       IRQF_SHARED, netdev->name, bp->queues);
+			if (err) {
+				dev_err(dev,
+					"Unable to request IRQ %d (error %d)\n",
+					bp->queues[0].irq, err);
+				return err;
+			}
 			queue_writel(bp->queues, IER, MACB_BIT(WOL));
 			macb_writel(bp, WOL, MACB_BIT(MAG));
 		}
-- 
2.20.1

Re: [PATCH 5/5] net: macb: Add WoL interrupt support for MACB type of Ethernet controller

[Florian Fainelli]

On 4/16/2020 10:44 AM, nicolas.ferre@microchip.com wrote:
> From: Nicolas Ferre <nicolas.ferre@microchip.com>
> 
> Handle the Wake-on-Lan interrupt for the Cadence MACB Ethernet
> controller.
> As we do for the GEM version, we handle of WoL interrupt in a
> specialized interrupt handler for MACB version that is positionned
> just between suspend() and resume() calls.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> ---
>   drivers/net/ethernet/cadence/macb_main.c | 38 +++++++++++++++++++++++-
>   1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 71e6afbdfb47..6d535e3e803c 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1513,6 +1513,34 @@ static void macb_tx_restart(struct macb_queue *queue)
>   	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
>   }
>   
> +static irqreturn_t macb_wol_interrupt(int irq, void *dev_id)
> +{
> +	struct macb_queue *queue = dev_id;
> +	struct macb *bp = queue->bp;
> +	u32 status;
> +
> +	status = queue_readl(queue, ISR);
> +
> +	if (unlikely(!status))
> +		return IRQ_NONE;
> +
> +	spin_lock(&bp->lock);
> +
> +	if (status & MACB_BIT(WOL)) {
> +		queue_writel(queue, IDR, MACB_BIT(WOL));
> +		macb_writel(bp, WOL, 0);
> +		netdev_vdbg(bp->dev, "MACB WoL: queue = %u, isr = 0x%08lx\n",
> +			    (unsigned int)(queue - bp->queues),
> +			    (unsigned long)status);
> +		if (bp->caps & MACB_CAPS_ISR_CLEAR_ON_WRITE)
> +			queue_writel(queue, ISR, MACB_BIT(WOL));
> +	}

Likewise, this would need a call to pm_wakeup_event() to record the 
wake-up event associated with this device.
-- 
Florian