[PATCH 1/3 v2] net: smsc911x: augment device tree bindings

This adds device tree bindings for:

- An optional GPIO line for releasing the RESET signal to the
  SMSC911x devices

- An optional PME (power management event) interrupt line that
  can be utilized to wake up the system on network activity.
  This signal exist on all the SMSC911x devices, it is just not
  very often routed.

Both these lines are routed to the SoC on the Qualcomm APQ8060
Dragonboard and thus needs to be bound in the device tree.

Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Linus Walleij <redacted>
---
ChangeLog v1->v2:
- Document for "interrupts", skip mentioning "interrupts-extended"
  as both are always supported.
---
 Documentation/devicetree/bindings/net/smsc911x.txt | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/smsc911x.txt b/Documentation/devicetree/bindings/net/smsc911x.txt
index 3fed3c124411..11c4a46ba682 100644
--- a/Documentation/devicetree/bindings/net/smsc911x.txt
+++ b/Documentation/devicetree/bindings/net/smsc911x.txt

@@ -3,9 +3,12 @@
 Required properties:
 - compatible : Should be "smsc,lan<model>", "smsc,lan9115"
 - reg : Address and length of the io space for SMSC LAN
-- interrupts : Should contain SMSC LAN interrupt line
-- interrupt-parent : Should be the phandle for the interrupt controller
-  that services interrupts for this device
+- interrupts : Should contain the SMSC LAN
+  interrupt line as cell 0, cell 1 is an OPTIONAL PME (power
+  management event) interrupt that is able to wake up the host
+  system with a 50ms pulse on network activity
+  For generic bindings for interrupt controller parents, refer to
+  interrupt-controller/interrupts.txt
 - phy-mode : See ethernet.txt file in the same directory
 
 Optional properties:

@@ -21,6 +24,10 @@ Optional properties:
   external PHY
 - smsc,save-mac-address : Indicates that mac address needs to be saved
   before resetting the controller
+- reset-gpios : a GPIO line connected to the RESET (active low) signal
+  of the device. On many systems this is wired high so the device goes
+  out of reset at power-on, but if it is under program control, this
+  optional GPIO can wake up in response to it.
 
 Examples:
 

@@ -29,7 +36,8 @@ lan9220@f4000000 {
 	reg = <0xf4000000 0x2000000>;
 	phy-mode = "mii";
 	interrupt-parent = <&gpio1>;
-	interrupts = <31>;
+	interrupts = <31>, <32>;
+	reset-gpios = <&gpio1 30 GPIO_ACTIVE_LOW>;
 	reg-io-width = <4>;
 	smsc,irq-push-pull;
 };

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

The SMSC911x have a line out of the chip called "PME",
Power Management Event. When connected to an asynchronous
interrupt controller this is able to wake the system up
from sleep in response to certain network events.

This is the first attempt to support this in the Linux
driver: the Qualcomm APQ8060 Dragonboard has this line
routed to a GPIO line on the primary SoC padring, and as
such it can be armed as a wakeup interrupt.

The patch is inspired by the wakeup code in the RTC
subsystem.

The code looks for an additional interrupt - apart from the
ordinary device interrupt - and in case that is present,
we register an interrupt handler to respons to this,
and flag the device and this interrupt as a wakeup.

Cc: Sudeep Holla <redacted>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Linus Walleij <redacted>
---
ChangeLog v1->v2:
- Call pm_wakeup_event() in the wakeup IRQ thread to
  account for the wakeup event.
- Drop the enable/disable_irq_wake() calls from suspend/resume:
  this is handled from the irq core when you call
  dev_pm_set_wake_irq() as we do.
---
 drivers/net/ethernet/smsc/smsc911x.c | 42 ++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index 8ab8d4b9614b..8fffc1dc2bdd 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c

@@ -63,6 +63,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
 #include <linux/gpio/consumer.h>
+#include <linux/pm_wakeirq.h>
 
 #include "smsc911x.h"
 

@@ -151,6 +152,9 @@ struct smsc911x_data {
 	/* Reset GPIO */
 	struct gpio_desc *reset_gpiod;
 
+	/* PME interrupt */
+	int pme_irq;
+
 	/* clock */
 	struct clk *clk;
 };

@@ -1881,6 +1885,19 @@ static irqreturn_t smsc911x_irqhandler(int irq, void *dev_id)
 	return serviced;
 }
 
+static irqreturn_t smsc911x_pme_irq_thread(int irq, void *dev_id)
+{
+	struct net_device *dev = dev_id;
+	struct smsc911x_data *pdata __maybe_unused = netdev_priv(dev);
+
+	SMSC_TRACE(pdata, pm, "wakeup event");
+	pm_wakeup_event(&dev->dev, 50);
+	/* This signal is active for 50 ms, wait for it to deassert */
+	usleep_range(50000, 100000);
+
+	return IRQ_HANDLED;
+}
+
 #ifdef CONFIG_NET_POLL_CONTROLLER
 static void smsc911x_poll_controller(struct net_device *dev)
 {

@@ -2501,6 +2518,31 @@ static int smsc911x_drv_probe(struct platform_device *pdev)
 		goto out_disable_resources;
 	}
 
+	irq = platform_get_irq(pdev, 1);
+	if (irq == -EPROBE_DEFER) {
+		retval = -EPROBE_DEFER;
+		goto out_disable_resources;
+	/* It's perfectly fine to not have a PME IRQ */
+	} else if (irq > 0) {
+		/*
+		 * The Power Management Event (PME) IRQ appears as
+		 * a pulse waking up the system from sleep in response to  a
+		 * network event.
+		 */
+		retval = request_threaded_irq(irq, NULL,
+					      smsc911x_pme_irq_thread,
+					      IRQF_ONESHOT, "smsc911x-pme",
+					      dev);
+		if (retval) {
+			SMSC_WARN(pdata, probe,
+			"Unable to claim requested PME irq: %d", irq);
+			goto out_disable_resources;
+		}
+		pdata->pme_irq = irq;
+		device_init_wakeup(&pdev->dev, true);
+		dev_pm_set_wake_irq(&pdev->dev, irq);
+	}
+
 	netif_carrier_off(dev);
 
 	retval = register_netdev(dev);

-- 
2.7.4

On some systems (such as the Qualcomm APQ8060 Dragonboard) the
RESET signal of the SMSC911x is not pulled up by a resistor but
connected to a GPIO line, so that the operating system must
explicitly deassert RESET before use.

Support this in the SMSC911x driver so this ethernet connector
can be used on such targets.

Signed-off-by: Linus Walleij <redacted>
---
ChangeLog v1->v2:
- Use devm_gpiod_request_optiona() and request the line with
  GPIOD_OUT_LOW so it is deasserted immediately if active.
---
 drivers/net/ethernet/smsc/smsc911x.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
index ca3134540d2d..8ab8d4b9614b 100644
--- a/drivers/net/ethernet/smsc/smsc911x.c
+++ b/drivers/net/ethernet/smsc/smsc911x.c

@@ -62,6 +62,7 @@
 #include <linux/acpi.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
+#include <linux/gpio/consumer.h>
 
 #include "smsc911x.h"
 

@@ -147,6 +148,9 @@ struct smsc911x_data {
 	/* regulators */
 	struct regulator_bulk_data supplies[SMSC911X_NUM_SUPPLIES];
 
+	/* Reset GPIO */
+	struct gpio_desc *reset_gpiod;
+
 	/* clock */
 	struct clk *clk;
 };

@@ -438,6 +442,11 @@ static int smsc911x_request_resources(struct platform_device *pdev)
 		netdev_err(ndev, "couldn't get regulators %d\n",
 				ret);
 
+	/* Request optional RESET GPIO */
+	pdata->reset_gpiod = devm_gpiod_get_optional(&pdev->dev,
+						     "reset",
+						     GPIOD_OUT_LOW);
+
 	/* Request clock */
 	pdata->clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(pdata->clk))

-- 
2.7.4

On Wednesday, August 24, 2016 2:59:40 PM CEST Linus Walleij wrote:

+- interrupts : Should contain the SMSC LAN
+  interrupt line as cell 0, cell 1 is an OPTIONAL PME (power
+  management event) interrupt that is able to wake up the host
+  system with a 50ms pulse on network activity
+  For generic bindings for interrupt controller parents, refer to
+  interrupt-controller/interrupts.txt

I think you should (slightly) reword this to avoid using the
term "cell", which refers to a 32-bit word in the property,
not the interrupt specifier that is often made up of two or
three cells.

Maybe something like

- interrupts: one or two interrupt specifiers:
	- The first interrupt is the SMSC LAN interrupt line.
	- The second interrupt (if present) is the power management
	  event ...

	Arnd

* Linus Walleij [off-list ref] [160824 06:00]:

The SMSC911x have a line out of the chip called "PME",
Power Management Event. When connected to an asynchronous
interrupt controller this is able to wake the system up
from sleep in response to certain network events.

This is the first attempt to support this in the Linux
driver: the Qualcomm APQ8060 Dragonboard has this line
routed to a GPIO line on the primary SoC padring, and as
such it can be armed as a wakeup interrupt.

The patch is inspired by the wakeup code in the RTC
subsystem.

The code looks for an additional interrupt - apart from the
ordinary device interrupt - and in case that is present,
we register an interrupt handler to respons to this,
and flag the device and this interrupt as a wakeup.

Cc: Sudeep Holla <redacted>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Linus Walleij <redacted>
---
ChangeLog v1->v2:
- Call pm_wakeup_event() in the wakeup IRQ thread to
  account for the wakeup event.
- Drop the enable/disable_irq_wake() calls from suspend/resume:
  this is handled from the irq core when you call
  dev_pm_set_wake_irq() as we do.

Looks OK to me:

Acked-by: Tony Lindgren <tony@atomide.com>

Hi Linus,

On 08/24/2016 07:59 AM, Linus Walleij wrote:

On some systems (such as the Qualcomm APQ8060 Dragonboard) the
RESET signal of the SMSC911x is not pulled up by a resistor but
connected to a GPIO line, so that the operating system must
explicitly deassert RESET before use.

Support this in the SMSC911x driver so this ethernet connector
can be used on such targets.

Hmm, at least in our hardware case AFAIK (juno/lan9118) the hardware 
reset line on the lan9118 is active low, but the chip itself is 
documented as having internal pullups so that it may be left unconnected.

Which microchip/smsc chip are we talking about? because it seems that 
you probably want the GPIO pin to be in any state (hi-Z, or just high) 
besides the one you selected here.

Beyond that, is it not possible for the firmware to get the reset pin in 
the correct configuration, so that linux doesn't have to mess with it?

Hi Linus,


On 08/24/2016 07:59 AM, Linus Walleij wrote:

The cleanup code in drv_remove seems to be missing, am I missing something?

Also, do you want the wake-up to be active if the interface is downed?


Thanks,

Hi,

On 08/24/2016 07:59 AM, Linus Walleij wrote:

The SMSC911x have a line out of the chip called "PME",
Power Management Event. When connected to an asynchronous
interrupt controller this is able to wake the system up
from sleep in response to certain network events.

This is the first attempt to support this in the Linux
driver: the Qualcomm APQ8060 Dragonboard has this line
routed to a GPIO line on the primary SoC padring, and as
such it can be armed as a wakeup interrupt.

The patch is inspired by the wakeup code in the RTC
subsystem.

The code looks for an additional interrupt - apart from the
ordinary device interrupt - and in case that is present,
we register an interrupt handler to respons to this,
and flag the device and this interrupt as a wakeup.

Having looked at a couple of the supported smsc chips, it seems they can 
route the wakeup through the chip's interrupt as well. If you add code 
to support this, it should work on a lot of the smsc911x devices rather 
than just the dragonboard.


Thanks,

On Wed, Aug 31, 2016 at 5:08 PM, Jeremy Linton [off-list ref] wrote:

On 08/24/2016 07:59 AM, Linus Walleij wrote:

quoted

On some systems (such as the Qualcomm APQ8060 Dragonboard) the
RESET signal of the SMSC911x is not pulled up by a resistor but
connected to a GPIO line, so that the operating system must
explicitly deassert RESET before use.

Support this in the SMSC911x driver so this ethernet connector
can be used on such targets.

Hmm, at least in our hardware case AFAIK (juno/lan9118) the hardware reset
line on the lan9118 is active low, but the chip itself is documented as
having internal pullups so that it may be left unconnected.

I guess this is only a comment about the contents of the commit message,
I'll check it up and augment.

This:

+       /* Request optional RESET GPIO */
+       pdata->reset_gpiod = devm_gpiod_get_optional(&pdev->dev,
+                                                    "reset",
+                                                    GPIOD_OUT_LOW);

Is Linux-internal way of saying "deassert the RESET" line, it does
*not* mean "drive it low".

GPIO lines are configured in the device tree for the platform in
question, and what you say about it being active low is true, and
that is why the device tree for the DragonBoard looks like so:

+ reset-gpios = <&tlmm 30 GPIO_ACTIVE_LOW>;

Flagging it active low in the device tree make sure it is driven
high by the statement in the code.

Beyond that, is it not possible for the firmware to get the reset pin in the
correct configuration, so that linux doesn't have to mess with it?

That is always possible, and in that case you simply do not specify
the GPIO line for RESET. But this firmware for the APQ8060 DragonBoard
does not do it and Qualcomm is not going to update it, and I need
to deal with it.

Yours,
Linus Walleij

On Wed, Aug 31, 2016 at 5:27 PM, Jeremy Linton [off-list ref] wrote:

The cleanup code in drv_remove seems to be missing, am I missing something?

No you're right it's just my bad coding. Fixing it and thanks for noticing.

Also, do you want the wake-up to be active if the interface is downed?

Hm! Good point.

It seems most other drivers call
device_set_wakeup_enable() on the device inside a
foo_set_wol() (wake-on-LAN) from the
.set_wol() callback in struct ethtool_ops.

This is in response to the ethtool calls from userspace.

So we need to implement this too.

I don't know whether that has anything to do with whether
the interface is up or not, it seems orthogonal actually.

I will hold this patch back until I can investigate and test
and just resend patches 1+2 right now. (Bindings should
still be OK to merge I guess, and I think the RESET patch
is sorted out.)

Yours,
Linus Walleij