Re: [PATCH 7/7] gianfar: add support for wake-on-packet
From: Scott Wood <hidden>
Date: 2011-11-04 21:11:59
Also in:
netdev
On 11/04/2011 07:40 AM, Zhao Chenhui wrote:
quoted hunk ↗ jump to hunk
diff --git a/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt b/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt index 2c6be03..543e36c 100644 --- a/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt +++ b/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt@@ -56,6 +56,9 @@ Properties: hardware. - fsl,magic-packet : If present, indicates that the hardware supports waking up via magic packet. + - fsl,wake-on-filer : If present, indicates that the hardware supports + waking up via arp request to local ip address or unicast packet to + local mac address.
Is there any way to determine this at runtime via the device's registers? I think TSEC_ID2[TSEC_CFG] can be used. The manual describes it awkwardly, but it looks like 0x20 is the bit for the filer.
quoted hunk ↗ jump to hunk
@@ -751,7 +764,6 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev) FSL_GIANFAR_DEV_HAS_PADDING | FSL_GIANFAR_DEV_HAS_CSUM | FSL_GIANFAR_DEV_HAS_VLAN | - FSL_GIANFAR_DEV_HAS_MAGIC_PACKET | FSL_GIANFAR_DEV_HAS_EXTENDED_HASH | FSL_GIANFAR_DEV_HAS_TIMER;
This is an unrelated change. Are there any eTSECs that don't support magic packet?
+static int gfar_get_ip(struct net_device *dev)
+{
+ struct gfar_private *priv = netdev_priv(dev);
+ struct in_device *in_dev = (struct in_device *)dev->ip_ptr;
+ struct in_ifaddr *ifa;
+
+ if (in_dev != NULL) {
+ ifa = (struct in_ifaddr *)in_dev->ifa_list;
+ if (ifa != NULL) {
+ memcpy(priv->ip_addr, &ifa->ifa_address, 4);
+ return 0;
+ }
+ }
+ return -ENOENT;
+}Unnecessary cast, ifa_list is already struct in_ifaddr *. Better, use for_primary_ifa(), and document that you won't wake on ARP packets for secondary IP addresses.
quoted hunk ↗ jump to hunk
static int gfar_suspend(struct device *dev) {@@ -1268,9 +1443,17 @@ static int gfar_suspend(struct device *dev) struct gfar __iomem *regs = priv->gfargrp[0].regs; unsigned long flags; u32 tempval; - int magic_packet = priv->wol_en && - (priv->device_flags & FSL_GIANFAR_DEV_HAS_MAGIC_PACKET); + (priv->wol_opts & GIANFAR_WOL_MAGIC); + int arp_packet = priv->wol_en && + (priv->wol_opts & GIANFAR_WOL_ARP); + + if (arp_packet) { + pmc_enable_wake(priv->ofdev, PM_SUSPEND_MEM, 1); + pmc_enable_lossless(1); + gfar_arp_suspend(ndev); + return 0; + }
How do we know this isn't standby?
quoted hunk ↗ jump to hunk
@@ -577,11 +578,18 @@ static void gfar_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol) { struct gfar_private *priv = netdev_priv(dev); + wol->supported = 0; + wol->wolopts = 0; + if (priv->device_flags & FSL_GIANFAR_DEV_HAS_MAGIC_PACKET) { - wol->supported = WAKE_MAGIC; - wol->wolopts = priv->wol_en ? WAKE_MAGIC : 0; - } else { - wol->supported = wol->wolopts = 0; + wol->supported |= WAKE_MAGIC; + wol->wolopts |= (priv->wol_opts & GIANFAR_WOL_MAGIC) ? + WAKE_MAGIC : 0; + } + if (priv->device_flags & FSL_GIANFAR_DEV_HAS_ARP_PACKET) { + wol->supported |= WAKE_ARP; + wol->wolopts |= (priv->wol_opts & GIANFAR_WOL_ARP) ? + WAKE_ARP : 0; } }
Shouldn't we just make sure we don't set a bit in priv->wol_opts if we don't support it? Maybe create the "supported" mask at init time, so we can use logical bit ops rather than a bunch of if statements?
quoted hunk ↗ jump to hunk
@@ -591,16 +599,21 @@ static int gfar_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) unsigned long flags; if (!(priv->device_flags & FSL_GIANFAR_DEV_HAS_MAGIC_PACKET) && - wol->wolopts != 0) - return -EINVAL; - - if (wol->wolopts & ~WAKE_MAGIC) + !(priv->device_flags & FSL_GIANFAR_DEV_HAS_ARP_PACKET)) return -EINVAL; - device_set_wakeup_enable(&dev->dev, wol->wolopts & WAKE_MAGIC); - spin_lock_irqsave(&priv->bflock, flags); - priv->wol_en = !!device_may_wakeup(&dev->dev); + if (wol->wolopts & WAKE_MAGIC) { + priv->wol_en = 1; + priv->wol_opts = GIANFAR_WOL_MAGIC; + } else if (wol->wolopts & WAKE_ARP) { + priv->wol_en = 1; + priv->wol_opts = GIANFAR_WOL_ARP;
What if both WAKE_MAGIC and WAKE_ARP are set? And shouldn't you make sure we actually support the one being requested, rather than just making sure that we support one of the wake modes? -Scott