diff mbox series

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

Message ID 1521726700-22634-6-git-send-email-harinikatakamlinux@gmail.com
State RFC, archived
Delegated to: David Miller
Headers show
Series Macb power management support for ZynqMP | expand

Commit Message

Harini Katakam March 22, 2018, 1:51 p.m. UTC
From: Harini Katakam <harinik@xilinx.com>

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

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

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

Comments

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

Why is this different for magic packet vs ARP packet?

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

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

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

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

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


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

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

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

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

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

What do you think?

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

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

	if (bp->wol & MACB_WOL_ENABLED) {
		macb_writel(bp, IER, MACB_BIT(WOL));
		macb_writel(bp, WOL, MACB_BIT(MAG));
		enable_irq_wake(bp->queues[0].irq);
		netif_device_detach(netdev);
	}

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

	u32 wolmask, arpipmask = 0;

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

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

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

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

Thank you,
Claudiu

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

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

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

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

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

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

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

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

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

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

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

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

Regards,
Harini
Claudiu Beznea May 15, 2018, 8:39 a.m. UTC | #3
Hi Harini,

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

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

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

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

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

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

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

I have user guide revision 15
.

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

Patch

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