From: Nicolas Ferre <[email protected]>
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
From: Nicolas Ferre <[email protected]>
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 <[email protected]>
Cc: Harini Katakam <[email protected]>
Cc: Rafal Ozieblo <[email protected]>
Cc: Sergio Prado <[email protected]>
Signed-off-by: Nicolas Ferre <[email protected]>
---
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
From: Nicolas Ferre <[email protected]>
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 <[email protected]>
Cc: Harini Katakam <[email protected]>
Cc: Rafal Ozieblo <[email protected]>
Cc: Antoine Tenart <[email protected]>
Signed-off-by: Nicolas Ferre <[email protected]>
---
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
From: Nicolas Ferre <[email protected]>
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 <[email protected]>
---
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
From: Nicolas Ferre <[email protected]>
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 <[email protected]>
Cc: Harini Katakam <[email protected]>
Cc: Rafal Ozieblo <[email protected]>
Signed-off-by: Nicolas Ferre <[email protected]>
---
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
From: Nicolas Ferre <[email protected]>
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 <[email protected]>, thanks!
Signed-off-by: Nicolas Ferre <[email protected]>
---
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
On 4/16/2020 10:44 AM, [email protected] wrote:
> From: Nicolas Ferre <[email protected]>
>
> 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 <[email protected]>
> Cc: Harini Katakam <[email protected]>
> Cc: Rafal Ozieblo <[email protected]>
> Signed-off-by: Nicolas Ferre <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 4/16/2020 10:44 AM, [email protected] wrote:
> From: Nicolas Ferre <[email protected]>
>
> 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 <[email protected]>, thanks!
>
> Signed-off-by: Nicolas Ferre <[email protected]>
> ---
[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
On 4/16/2020 10:44 AM, [email protected] wrote:
> From: Nicolas Ferre <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
Hi Nicolas,
> -----Original Message-----
> From: [email protected] [mailto:[email protected]]
> Sent: Thursday, April 16, 2020 11:14 PM
> To: [email protected]; [email protected]; Claudiu
> Beznea <[email protected]>; Harini Katakam
> <[email protected]>
> Cc: [email protected]; David S. Miller <[email protected]>;
> Alexandre Belloni <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Michal Simek
> <[email protected]>; Nicolas Ferre <[email protected]>; Rafal
> Ozieblo <[email protected]>
> Subject: [PATCH 1/5] net: macb: fix wakeup test in runtime suspend/resume
> routines
>
> From: Nicolas Ferre <[email protected]>
>
> 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
On 4/16/2020 10:44 AM, [email protected] wrote:
> From: Nicolas Ferre <[email protected]>
>
> 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 <[email protected]>
> Cc: Harini Katakam <[email protected]>
> Cc: Rafal Ozieblo <[email protected]>
> Cc: Sergio Prado <[email protected]>
> Signed-off-by: Nicolas Ferre <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 4/16/2020 10:44 AM, [email protected] wrote:
> From: Nicolas Ferre <[email protected]>
>
> 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 <[email protected]>
> Cc: Harini Katakam <[email protected]>
> Cc: Rafal Ozieblo <[email protected]>
> Cc: Antoine Tenart <[email protected]>
> Signed-off-by: Nicolas Ferre <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
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: [email protected] [mailto:[email protected]]
>> Sent: Thursday, April 16, 2020 11:14 PM
>> To: [email protected]; [email protected]; Claudiu
>> Beznea <[email protected]>; Harini Katakam
>> <[email protected]>
>> Cc: [email protected]; David S. Miller <[email protected]>;
>> Alexandre Belloni <[email protected]>; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected]; Michal Simek
>> <[email protected]>; Nicolas Ferre <[email protected]>; Rafal
>> Ozieblo <[email protected]>
>> Subject: [PATCH 1/5] net: macb: fix wakeup test in runtime suspend/resume
>> routines
>>
>> From: Nicolas Ferre <[email protected]>
>>
>> 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
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, [email protected] wrote:
>> From: Nicolas Ferre <[email protected]>
>>
>> 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 <[email protected]>, thanks!
>>
>> Signed-off-by: Nicolas Ferre <[email protected]>
>> ---
>
> [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
On 16/04/2020 at 19:44, [email protected] wrote:
> From: Nicolas Ferre <[email protected]>
>
> 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 <[email protected]>, thanks!
>
> Signed-off-by: Nicolas Ferre <[email protected]>
> ---
> 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
On 17/04/2020 at 19:14, Nicolas Ferre wrote:
> On 16/04/2020 at 19:44, [email protected] wrote:
>> From: Nicolas Ferre <[email protected]>
>>
>> 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 <[email protected]>, thanks!
>>
>> Signed-off-by: Nicolas Ferre <[email protected]>
>> ---
>> 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
On Tue, Apr 21, 2020 at 1:55 PM <[email protected]> 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