| Message ID | 56bb7a742093cec160c4465c808778a14b2607e7.1587058078.git.nicolas.ferre@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: macb: Wake-on-Lan magic packet fixes and GEM handling | expand |
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, 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
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,
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
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
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);