[PATCH 0/3] gpio: zynq: Fix suspend/wake

[PATCH 0/3] gpio: zynq: Fix suspend/wake

[Soren Brinkmann]

Hi Linus,

after our discussion during the push of the gpio-zynq driver, we took a
closer look at the LED and gpio_keys drivers.

For LEDs and other simple use-cases where you need to control an output,
that driver is great and a patch to get the LEDs on our boards added to
DT is pending.

For the keys, and especially wake, things didn't look that good. Testing
revealed a couple of flaws in the driver's wake-related implementation
which should be fixed with this series.

I'm still not fully convinced that the gpio_keys are the best
replacement for the sysfs interface when it comes to inputs. For that
reason and to have a way to do some quick wake testing, I'd like to
propose adding the ability to control wake through the sysfs interface
(patch 3).

The series is based on Linus T's current tip + the Zynq-related patches
from gpio/for-next + Lars' pending patch 'gpio: zynq: Take bank
offset into account when reporting a IRQ'.
A branch with all those patches can be found here:
https://github.com/sorenb-xlnx/linux-xlnx/commits/gpio

	Thanks,
	Sören

Ezra Savard (2):
  gpio: zynq: Mask non-wakeup GPIO interrupts on suspend
  gpio: zynq: Fixed broken wakeup implementation

Soren Brinkmann (1):
  gpio: lib-sysfs: Add 'wakeup' attribute

 drivers/gpio/gpio-zynq.c     | 38 +++++++++++++---------
 drivers/gpio/gpiolib-sysfs.c | 75 ++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 92 insertions(+), 21 deletions(-)

-- 
2.1.0.1.g27b9230

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

[PATCH 1/3] gpio: zynq: Mask non-wakeup GPIO interrupts on suspend

[Soren Brinkmann]

From: Ezra Savard <ezra.savard@xilinx.com>

Added flag to the GPIO chip so that IRQ from non-wakeup GPIO will
not wake the system.

Signed-off-by: Ezra Savard <ezra.savard@xilinx.com>
Reviewed-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/gpio/gpio-zynq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index dccadea9d830..d80d722529ad 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -451,7 +451,8 @@ static struct irq_chip zynq_gpio_level_irqchip = {
 	.irq_unmask	= zynq_gpio_irq_unmask,
 	.irq_set_type	= zynq_gpio_set_irq_type,
 	.irq_set_wake	= zynq_gpio_set_wake,
-	.flags		= IRQCHIP_EOI_THREADED | IRQCHIP_EOI_IF_HANDLED,
+	.flags		= IRQCHIP_EOI_THREADED | IRQCHIP_EOI_IF_HANDLED |
+			  IRQCHIP_MASK_ON_SUSPEND,
 };
 
 static struct irq_chip zynq_gpio_edge_irqchip = {
@@ -462,6 +463,7 @@ static struct irq_chip zynq_gpio_edge_irqchip = {
 	.irq_unmask	= zynq_gpio_irq_unmask,
 	.irq_set_type	= zynq_gpio_set_irq_type,
 	.irq_set_wake	= zynq_gpio_set_wake,
+	.flags		= IRQCHIP_MASK_ON_SUSPEND,
 };
 
 static void zynq_gpio_handle_bank_irq(struct zynq_gpio *gpio,
-- 
2.1.0.1.g27b9230

Re: [PATCH 1/3] gpio: zynq: Mask non-wakeup GPIO interrupts on suspend

[Linus Walleij]

On Fri, Aug 29, 2014 at 7:58 PM, Soren Brinkmann
<soren.brinkmann@xilinx.com> wrote:

> From: Ezra Savard <ezra.savard@xilinx.com>
>
> Added flag to the GPIO chip so that IRQ from non-wakeup GPIO will
> not wake the system.
>
> Signed-off-by: Ezra Savard <ezra.savard@xilinx.com>
> Reviewed-by: Soren Brinkmann <soren.brinkmann@xilinx.com>

Patch applied.

Yours,
Linus Walleij

[PATCH 2/3] gpio: zynq: Fixed broken wakeup implementation

[Soren Brinkmann]

From: Ezra Savard <ezra.savard@xilinx.com>

Use of unmask/mask in set_wake was an incorrect implementation. The new
implementation correctly sets wakeup for the gpio chip's IRQ so the gpio chip
will not sleep while wakeup-enabled gpio are in use.

Signed-off-by: Ezra Savard <ezra.savard@xilinx.com>
Reviewed-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/gpio/gpio-zynq.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index d80d722529ad..1e6f19a07454 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -88,16 +88,17 @@
  * @chip:	instance of the gpio_chip
  * @base_addr:	base address of the GPIO device
  * @clk:	clock resource for this controller
+ * @irq:	interrupt for the GPIO device
  */
 struct zynq_gpio {
 	struct gpio_chip chip;
 	void __iomem *base_addr;
 	struct clk *clk;
+	int irq;
 };
 
 static struct irq_chip zynq_gpio_level_irqchip;
 static struct irq_chip zynq_gpio_edge_irqchip;
-
 /**
  * zynq_gpio_get_bank_pin - Get the bank number and pin number within that bank
  * for a given pin in the GPIO device
@@ -434,10 +435,9 @@ static int zynq_gpio_set_irq_type(struct irq_data *irq_data, unsigned int type)
 
 static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
 {
-	if (on)
-		zynq_gpio_irq_unmask(data);
-	else
-		zynq_gpio_irq_mask(data);
+	struct zynq_gpio *gpio = irq_data_get_irq_chip_data(data);
+
+	irq_set_irq_wake(gpio->irq, on);
 
 	return 0;
 }
@@ -518,7 +518,11 @@ static void zynq_gpio_irqhandler(unsigned int irq, struct irq_desc *desc)
 
 static int __maybe_unused zynq_gpio_suspend(struct device *dev)
 {
-	if (!device_may_wakeup(dev))
+	struct platform_device *pdev = to_platform_device(dev);
+	int irq = platform_get_irq(pdev, 0);
+	struct irq_data *data = irq_get_irq_data(irq);
+
+	if (!irqd_is_wakeup_set(data))
 		return pm_runtime_force_suspend(dev);
 
 	return 0;
@@ -526,7 +530,11 @@ static int __maybe_unused zynq_gpio_suspend(struct device *dev)
 
 static int __maybe_unused zynq_gpio_resume(struct device *dev)
 {
-	if (!device_may_wakeup(dev))
+	struct platform_device *pdev = to_platform_device(dev);
+	int irq = platform_get_irq(pdev, 0);
+	struct irq_data *data = irq_get_irq_data(irq);
+
+	if (!irqd_is_wakeup_set(data))
 		return pm_runtime_force_resume(dev);
 
 	return 0;
@@ -587,7 +595,7 @@ static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
  */
 static int zynq_gpio_probe(struct platform_device *pdev)
 {
-	int ret, bank_num, irq;
+	int ret, bank_num;
 	struct zynq_gpio *gpio;
 	struct gpio_chip *chip;
 	struct resource *res;
@@ -603,10 +611,10 @@ static int zynq_gpio_probe(struct platform_device *pdev)
 	if (IS_ERR(gpio->base_addr))
 		return PTR_ERR(gpio->base_addr);
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0) {
+	gpio->irq = platform_get_irq(pdev, 0);
+	if (gpio->irq < 0) {
 		dev_err(&pdev->dev, "invalid IRQ\n");
-		return irq;
+		return gpio->irq;
 	}
 
 	/* configure the gpio chip */
@@ -654,14 +662,12 @@ static int zynq_gpio_probe(struct platform_device *pdev)
 		goto err_rm_gpiochip;
 	}
 
-	gpiochip_set_chained_irqchip(chip, &zynq_gpio_edge_irqchip, irq,
+	gpiochip_set_chained_irqchip(chip, &zynq_gpio_edge_irqchip, gpio->irq,
 				     zynq_gpio_irqhandler);
 
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
-	device_set_wakeup_capable(&pdev->dev, 1);
-
 	return 0;
 
 err_rm_gpiochip:
-- 
2.1.0.1.g27b9230

Re: [PATCH 2/3] gpio: zynq: Fixed broken wakeup implementation

[Linus Walleij]

On Fri, Aug 29, 2014 at 7:58 PM, Soren Brinkmann
<soren.brinkmann@xilinx.com> wrote:

> From: Ezra Savard <ezra.savard@xilinx.com>
>
> Use of unmask/mask in set_wake was an incorrect implementation. The new
> implementation correctly sets wakeup for the gpio chip's IRQ so the gpio chip
> will not sleep while wakeup-enabled gpio are in use.
>
> Signed-off-by: Ezra Savard <ezra.savard@xilinx.com>
> Reviewed-by: Soren Brinkmann <soren.brinkmann@xilinx.com>

Patch applied.

However the problems seems quite generic.

Do you see this kind of error in other GPIO drivers?

IRQchip semantics always make me nervous.

Yours,
Linus Walleij

Re: [PATCH 2/3] gpio: zynq: Fixed broken wakeup implementation

[Sören Brinkmann]

On Thu, 2014-09-04 at 06:27PM +0200, Linus Walleij wrote:
> On Fri, Aug 29, 2014 at 7:58 PM, Soren Brinkmann
> <soren.brinkmann@xilinx.com> wrote:
> 
> > From: Ezra Savard <ezra.savard@xilinx.com>
> >
> > Use of unmask/mask in set_wake was an incorrect implementation. The new
> > implementation correctly sets wakeup for the gpio chip's IRQ so the gpio chip
> > will not sleep while wakeup-enabled gpio are in use.
> >
> > Signed-off-by: Ezra Savard <ezra.savard@xilinx.com>
> > Reviewed-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> 
> Patch applied.
> 
> However the problems seems quite generic.
> 
> Do you see this kind of error in other GPIO drivers?
> 
> IRQchip semantics always make me nervous.

Our implementation was just completely flawed. It did work with our
limited tests using the sysfs interface. But once we started with the
gpio_keys things fell apart. The set_wake did mask/unmask IRQs, which
is clearly the job of the respective mask/unmask function of a gpiochip.
After we found that, we looked at a few other drivers and designed this
following gpio-mxs.
So, the core part was to do the right thing in set_wake. Once that was
done, the runtime PM callbacks needed some realignment to determine
whether GPIO is a wakeup device or not.

So, this was really just a gpio-zynq problem, not really generic.

Thanks for applying. I'll post the gpiolib-sysfs patch on its own in a
separate submission.

	Sören
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] gpio: lib-sysfs: Add 'wakeup' attribute

[Soren Brinkmann]

Add an attribute 'wakeup' to the GPIO sysfs interface which allows
marking/unmarking a GPIO as wake IRQ.
The file 'wakeup' is created in each exported GPIOs directory, if an IRQ
is associated with that GPIO and the irqchip implements set_wake().
Writing 'enabled' to that file will enable wake for that GPIO, while
writing 'disabled' will disable wake.
Reading that file will return either 'disabled' or 'enabled' depening on
the currently set flag for the GPIO's IRQ.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/gpio/gpiolib-sysfs.c | 75 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 69 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 5f2150b619a7..aaf021eaaff5 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -286,6 +286,56 @@ found:
 
 static DEVICE_ATTR(edge, 0644, gpio_edge_show, gpio_edge_store);
 
+static ssize_t gpio_wakeup_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	ssize_t			status;
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	int			irq = gpiod_to_irq(desc);
+	struct irq_desc		*irq_desc = irq_to_desc(irq);
+
+	mutex_lock(&sysfs_lock);
+
+	if (irqd_is_wakeup_set(&irq_desc->irq_data))
+		status = sprintf(buf, "enabled\n");
+	else
+		status = sprintf(buf, "disabled\n");
+
+	mutex_unlock(&sysfs_lock);
+
+	return status;
+}
+
+static ssize_t gpio_wakeup_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	int			ret;
+	unsigned int		on;
+	struct gpio_desc	*desc = dev_get_drvdata(dev);
+	int			irq = gpiod_to_irq(desc);
+
+	mutex_lock(&sysfs_lock);
+
+	if (sysfs_streq("enabled", buf))
+		on = true;
+	else if (sysfs_streq("disabled", buf))
+		on = false;
+	else
+		return -EINVAL;
+
+	ret = irq_set_irq_wake(irq, on);
+
+	mutex_unlock(&sysfs_lock);
+
+	if (ret)
+		pr_warn("%s: failed to %s wake\n", __func__,
+				on ? "enable" : "disable");
+
+	return size;
+}
+
+static DEVICE_ATTR(wakeup, 0644, gpio_wakeup_show, gpio_wakeup_store);
+
 static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
 				int value)
 {
@@ -526,7 +576,7 @@ static struct class gpio_class = {
 int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 {
 	unsigned long		flags;
-	int			status;
+	int			status, irq;
 	const char		*ioname = NULL;
 	struct device		*dev;
 	int			offset;
@@ -582,11 +632,24 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 			goto fail_unregister_device;
 	}
 
-	if (gpiod_to_irq(desc) >= 0 && (direction_may_change ||
-				       !test_bit(FLAG_IS_OUT, &desc->flags))) {
-		status = device_create_file(dev, &dev_attr_edge);
-		if (status)
-			goto fail_unregister_device;
+	irq = gpiod_to_irq(desc);
+	if (irq >= 0) {
+		struct irq_desc *irq_desc = irq_to_desc(irq);
+		struct irq_chip	*irqchip = irq_desc_get_chip(irq_desc);
+
+		if (direction_may_change ||
+				!test_bit(FLAG_IS_OUT, &desc->flags)) {
+			status = device_create_file(dev, &dev_attr_edge);
+			if (status)
+				goto fail_unregister_device;
+		}
+
+		if (irqchip->flags & IRQCHIP_SKIP_SET_WAKE ||
+				irqchip->irq_set_wake) {
+			status = device_create_file(dev, &dev_attr_wakeup);
+			if (status)
+				goto fail_unregister_device;
+		}
 	}
 
 	set_bit(FLAG_EXPORT, &desc->flags);
-- 
2.1.0.1.g27b9230

Re: [PATCH 3/3] gpio: lib-sysfs: Add 'wakeup' attribute

[Linus Walleij]

On Fri, Aug 29, 2014 at 7:58 PM, Soren Brinkmann
<soren.brinkmann@xilinx.com> wrote:

> Add an attribute 'wakeup' to the GPIO sysfs interface which allows
> marking/unmarking a GPIO as wake IRQ.
> The file 'wakeup' is created in each exported GPIOs directory, if an IRQ
> is associated with that GPIO and the irqchip implements set_wake().
> Writing 'enabled' to that file will enable wake for that GPIO, while
> writing 'disabled' will disable wake.
> Reading that file will return either 'disabled' or 'enabled' depening on
> the currently set flag for the GPIO's IRQ.
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>

While I do see the utility of this patch, any kind of interrupt and
related handling in userspace makes me super-stressed and
nervous.

I need wide consent for this and please post this patch separately
from the zynq-related patches.

Yours,
Linus Walleij