[RFC/RFT PATCH 0/7] gpio: omap: rework and fixes

[RFC/RFT PATCH 0/7] gpio: omap: rework and fixes

[Grygorii Strashko]

Hi Tony,

As I promised in [1] I've prepared a new series for OMAP GPIO driver.

Patches 1-2 are bug fixes.

Patches 3-6 is attempt (RFC/RFT) to rework OMAP GPIO driver taking into account
that GPIO Chip and GPIO IRQ Chip functionality are mostly orthogonal.

Patch 7 is second attempt (RFC/RFT) to Runtime PM APIs without checking current
GPIO bank's state with BANK_USED() macro.

Based on top of:
 030bbdb Linux 4.1-rc3

Tested on
  dra7-evm, gpiosysfs, GPIO IRQ only

Refs:
[1] gpio: omap: Fix PM runtime issue and remove most BANK_USED macros 
    http://www.spinics.net/lists/linux-gpio/msg05308.html
[2] [RFC/RFT PATCH 2/2] gpio: omap: ensure that runtime pm will disable unused gpio banks
    http://marc.info/?l=linux-gpio&m=142567003515626&w=2

Grygorii Strashko (7):
  gpio: omap: fix omap_gpio_free to not clean up irq configuration
  gpio: omap: fix error handling in omap_gpio_irq_type
  gpio: omap: rework omap_x_irq_shutdown to touch only irqs specific registers
  gpio: omap: rework omap_gpio_request to touch only gpio specific registers
  gpio: omap: rework omap_gpio_irq_startup to handle current pin state properly
  gpio: omap: clean up omap_gpio_irq_type
  gpio: omap: ensure that runtime pm will disable unused gpio banks

 drivers/gpio/gpio-omap.c | 85 ++++++++++++++++++++----------------------------
 1 file changed, 36 insertions(+), 49 deletions(-)

-- 
1.9.1

[PATCH 1/7] gpio: omap: fix omap_gpio_free to not clean up irq configuration

[Grygorii Strashko]

This patch fixes following issue:
- GPIOn is used as IRQ by some dev, for example PCF8575.INT ->  gpio6.11
- PCFx driver knows nothing about type of IRQ line (GPIO or not)
  so it doesn't request gpio and just do request_irq()
- If gpio6.11 will be exported through the sysfs and then un-xeported
then IRQs from PCFx will not be received any more, because
IRQ configuration for gpio6.11 will be cleaned up unconditionally
in omap_gpio_free.

Fix this by removing all GPIO IRQ specific code from omap_gpio_free()
and also do GPIO clean up (change direction to 'in' and disable debounce)
only if corresponding GPIO is not used as IRQ too.
GPIO IRQ will be properly cleaned up by GPIO irqchip code.

Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
---
 drivers/gpio/gpio-omap.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index b232397..bb60cbc 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -690,8 +690,11 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
 
 	spin_lock_irqsave(&bank->lock, flags);
 	bank->mod_usage &= ~(BIT(offset));
+	if (!LINE_USED(bank->irq_usage, offset)) {
+		omap_set_gpio_direction(bank, offset, 1);
+		omap_clear_gpio_debounce(bank, offset);
+	}
 	omap_disable_gpio_module(bank, offset);
-	omap_reset_gpio(bank, offset);
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	/*
-- 
1.9.1

[PATCH 2/7] gpio: omap: fix error handling in omap_gpio_irq_type

[Grygorii Strashko]

The GPIO bank will be kept powered in case if input parameters
are invalid or error occurred in omap_gpio_irq_type.

Hence, fix it by ensuring that GPIO bank will be unpowered
in case of errors and add additional check of value returned
from omap_set_gpio_triggering().

Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
---
 drivers/gpio/gpio-omap.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index bb60cbc..f6cc638 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -488,9 +488,6 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
 	unsigned long flags;
 	unsigned offset = d->hwirq;
 
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
-
 	if (type & ~IRQ_TYPE_SENSE_MASK)
 		return -EINVAL;
 
@@ -498,12 +495,18 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
 		(type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
 		return -EINVAL;
 
+	if (!BANK_USED(bank))
+		pm_runtime_get_sync(bank->dev);
+
 	spin_lock_irqsave(&bank->lock, flags);
 	retval = omap_set_gpio_triggering(bank, offset, type);
+	if (retval)
+		goto error;
 	omap_gpio_init_irq(bank, offset);
 	if (!omap_gpio_is_input(bank, offset)) {
 		spin_unlock_irqrestore(&bank->lock, flags);
-		return -EINVAL;
+		retval = -EINVAL;
+		goto error;
 	}
 	spin_unlock_irqrestore(&bank->lock, flags);
 
@@ -512,6 +515,11 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
 	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
 		__irq_set_handler_locked(d->irq, handle_edge_irq);
 
+	return 0;
+
+error:
+	if (!BANK_USED(bank))
+		pm_runtime_put(bank->dev);
 	return retval;
 }
 
-- 
1.9.1

[RFC/RFT PATCH 3/7] gpio: omap: rework omap_x_irq_shutdown to touch only irqs specific registers

[Grygorii Strashko]

The GPIO Chip and GPIO IRQ Chip functionality are essentially orthogonal,
so GPIO IRQ Chip implementation shouldn't touch GPIO specific
registers and vise versa.

Hence, rework omap_gpio_irq_shutdown and try to touch only irqs specific
registers:
- don't configure GPIO as input (it, actually, should be already configured
  as input).
- don't clear debounce configuration if GPIO is still used as GPIO.
  We need to take in to account here commit c9c55d921115
  ("gpio/omap: fix off-mode bug: clear debounce settings on free/reset").

Also remove omap_reset_gpio() function as it is not used any more.

Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
---
 drivers/gpio/gpio-omap.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index f6cc638..d933b99 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -646,15 +646,6 @@ static int omap_set_gpio_wakeup(struct gpio_bank *bank, unsigned offset,
 	return 0;
 }
 
-static void omap_reset_gpio(struct gpio_bank *bank, unsigned offset)
-{
-	omap_set_gpio_direction(bank, offset, 1);
-	omap_set_gpio_irqenable(bank, offset, 0);
-	omap_clear_gpio_irqstatus(bank, offset);
-	omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
-	omap_clear_gpio_debounce(bank, offset);
-}
-
 /* Use disable_irq_wake() and enable_irq_wake() functions from drivers */
 static int omap_gpio_wake_enable(struct irq_data *d, unsigned int enable)
 {
@@ -821,8 +812,12 @@ static void omap_gpio_irq_shutdown(struct irq_data *d)
 
 	spin_lock_irqsave(&bank->lock, flags);
 	bank->irq_usage &= ~(BIT(offset));
+	omap_set_gpio_irqenable(bank, offset, 0);
+	omap_clear_gpio_irqstatus(bank, offset);
+	omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
+	if (!LINE_USED(bank->mod_usage, offset))
+		omap_clear_gpio_debounce(bank, offset);
 	omap_disable_gpio_module(bank, offset);
-	omap_reset_gpio(bank, offset);
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	/*
-- 
1.9.1

[RFC/RFT PATCH 4/7] gpio: omap: rework omap_gpio_request to touch only gpio specific registers

[Grygorii Strashko]

The GPIO Chip and GPIO IRQ Chip functionality are essentially orthogonal,
so GPIO Chip implementation shouldn't touch GPIO IRQ specific registers
and vise versa.

Hence, rework omap_gpio_request:
- don't reset GPIO IRQ triggering type to IRQ_TYPE_NONE, because
  GPIO irqchip should be responsible for that;
- call directly omap_enable_gpio_module as all needed checks are already
  present inside it.

Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
---
 drivers/gpio/gpio-omap.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index d933b99..2fbd569 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -668,14 +668,7 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
 		pm_runtime_get_sync(bank->dev);
 
 	spin_lock_irqsave(&bank->lock, flags);
-	/* Set trigger to none. You need to enable the desired trigger with
-	 * request_irq() or set_irq_type(). Only do this if the IRQ line has
-	 * not already been requested.
-	 */
-	if (!LINE_USED(bank->irq_usage, offset)) {
-		omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
-		omap_enable_gpio_module(bank, offset);
-	}
+	omap_enable_gpio_module(bank, offset);
 	bank->mod_usage |= BIT(offset);
 	spin_unlock_irqrestore(&bank->lock, flags);
 
-- 
1.9.1

[RFC/RFT PATCH 5/7] gpio: omap: rework omap_gpio_irq_startup to handle current pin state properly

[Grygorii Strashko]

The omap_gpio_irq_startup() can be called at time when:
- corresponding GPIO has been requested already and in this case
it has to be configured as input already. If not - return with -EINVAL
and do not try to re-configure it as it could be unsafe.
- corresponding GPIO is free: reconfigure GPIO as input.

In addition, call omap_enable_gpio_module directly as all needed
checks are already present inside it.

Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
---
 drivers/gpio/gpio-omap.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 2fbd569..1c226f1 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -790,11 +790,23 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d)
 		pm_runtime_get_sync(bank->dev);
 
 	spin_lock_irqsave(&bank->lock, flags);
-	omap_gpio_init_irq(bank, offset);
+
+	if (!LINE_USED(bank->mod_usage, offset))
+		omap_set_gpio_direction(bank, offset, 1);
+	else if (!omap_gpio_is_input(bank, offset))
+		goto err;
+	omap_enable_gpio_module(bank, offset);
+	bank->irq_usage |= BIT(offset);
+
 	spin_unlock_irqrestore(&bank->lock, flags);
 	omap_gpio_unmask_irq(d);
 
 	return 0;
+err:
+	spin_unlock_irqrestore(&bank->lock, flags);
+	if (!BANK_USED(bank))
+		pm_runtime_put(bank->dev);
+	return -EINVAL;
 }
 
 static void omap_gpio_irq_shutdown(struct irq_data *d)
-- 
1.9.1

[RFC/RFT PATCH 6/7] gpio: omap: clean up omap_gpio_irq_type

[Grygorii Strashko]

The omap_gpio_irq_type() can do only configuration of GPIO IRQ
triggering type, because now OMAP GPIO driver has implemented
.irq_startup()/.irq_shutdown() which are responsible for
GPIO bank enabling and pin direction configuration.

Hence, remove redundant code and omap_gpio_init_irq() which is
not used any more.

Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
---
 drivers/gpio/gpio-omap.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 1c226f1..f02b3fa 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -472,15 +472,6 @@ static int omap_gpio_is_input(struct gpio_bank *bank, unsigned offset)
 	return readl_relaxed(reg) & BIT(offset);
 }
 
-static void omap_gpio_init_irq(struct gpio_bank *bank, unsigned offset)
-{
-	if (!LINE_USED(bank->mod_usage, offset)) {
-		omap_enable_gpio_module(bank, offset);
-		omap_set_gpio_direction(bank, offset, 1);
-	}
-	bank->irq_usage |= BIT(offset);
-}
-
 static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
 {
 	struct gpio_bank *bank = omap_irq_data_get_bank(d);
@@ -502,12 +493,6 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
 	retval = omap_set_gpio_triggering(bank, offset, type);
 	if (retval)
 		goto error;
-	omap_gpio_init_irq(bank, offset);
-	if (!omap_gpio_is_input(bank, offset)) {
-		spin_unlock_irqrestore(&bank->lock, flags);
-		retval = -EINVAL;
-		goto error;
-	}
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
-- 
1.9.1

[RFC/RFT PATCH v2 7/7] gpio: omap: ensure that runtime pm will disable unused gpio banks

[Grygorii Strashko]

Now there are some points related to Runtime PM usage:
1) bank state doesn't need to be checked in places where
Rintime PM is used, bacause Runtime PM maintains its
own usage counter:
      if (!BANK_USED(bank))
               pm_runtime_get_sync(bank->dev);
so, it's safe to drop such checks.

2) Such implementation is racy, because check !BANK_USED(bank)
   is not protected, like:
 CPU0				CPU1
 gpio_request(bankX.A)
 ...
 gpio_free(bankX.A)		gpio_request(bankX.Y)

 and bankX can be unpowered in the middle of processing
 gpio_request(bankX.Y)

3) There is a call of pm_runtime_get_sync() in omap_gpio_irq_type(),
but no corresponding put. As result, GPIO devices could be
powered on forever if at least one GPIO was used as IRQ.
Hence, allow powering off GPIO banks by adding missed
pm_runtime_put(bank->dev) at the end of omap_gpio_irq_type().

As, part of this change update omap2_gpio_xxxx_idle() functions
to use pm_runtime_force_suspend()/pm_runtime_force_resume().

Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
---
Changes in v2:
 - omap2_gpio_xxxx_idle() functions switched to use 
   pm_runtime_force_suspend()/pm_runtime_force_resume() API.

v1:
    http://marc.info/?l=linux-gpio&m=142567003515626&w=2

 drivers/gpio/gpio-omap.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index f02b3fa..e26dc40 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -486,8 +486,7 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
 		(type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
 		return -EINVAL;
 
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
+	pm_runtime_get_sync(bank->dev);
 
 	spin_lock_irqsave(&bank->lock, flags);
 	retval = omap_set_gpio_triggering(bank, offset, type);
@@ -500,11 +499,8 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
 	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
 		__irq_set_handler_locked(d->irq, handle_edge_irq);
 
-	return 0;
-
 error:
-	if (!BANK_USED(bank))
-		pm_runtime_put(bank->dev);
+	pm_runtime_put(bank->dev);
 	return retval;
 }
 
@@ -649,8 +645,7 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
 	 * If this is the first gpio_request for the bank,
 	 * enable the bank module.
 	 */
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
+	pm_runtime_get_sync(bank->dev);
 
 	spin_lock_irqsave(&bank->lock, flags);
 	omap_enable_gpio_module(bank, offset);
@@ -678,8 +673,7 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
 	 * If this is the last gpio to be freed in the bank,
 	 * disable the bank module.
 	 */
-	if (!BANK_USED(bank))
-		pm_runtime_put(bank->dev);
+	pm_runtime_put(bank->dev);
 }
 
 /*
@@ -771,8 +765,7 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d)
 	unsigned long flags;
 	unsigned offset = d->hwirq;
 
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
+	pm_runtime_get_sync(bank->dev);
 
 	spin_lock_irqsave(&bank->lock, flags);
 
@@ -789,8 +782,7 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d)
 	return 0;
 err:
 	spin_unlock_irqrestore(&bank->lock, flags);
-	if (!BANK_USED(bank))
-		pm_runtime_put(bank->dev);
+	pm_runtime_put(bank->dev);
 	return -EINVAL;
 }
 
@@ -814,8 +806,7 @@ static void omap_gpio_irq_shutdown(struct irq_data *d)
 	 * If this is the last IRQ to be freed in the bank,
 	 * disable the bank module.
 	 */
-	if (!BANK_USED(bank))
-		pm_runtime_put(bank->dev);
+	pm_runtime_put(bank->dev);
 }
 
 static void omap_gpio_ack_irq(struct irq_data *d)
@@ -1419,12 +1410,12 @@ void omap2_gpio_prepare_for_idle(int pwr_mode)
 	struct gpio_bank *bank;
 
 	list_for_each_entry(bank, &omap_gpio_list, node) {
-		if (!BANK_USED(bank) || !bank->loses_context)
+		if (!bank->loses_context)
 			continue;
 
 		bank->power_mode = pwr_mode;
 
-		pm_runtime_put_sync_suspend(bank->dev);
+		pm_runtime_force_suspend(bank->dev);
 	}
 }
 
@@ -1433,10 +1424,10 @@ void omap2_gpio_resume_after_idle(void)
 	struct gpio_bank *bank;
 
 	list_for_each_entry(bank, &omap_gpio_list, node) {
-		if (!BANK_USED(bank) || !bank->loses_context)
+		if (!bank->loses_context)
 			continue;
 
-		pm_runtime_get_sync(bank->dev);
+		pm_runtime_force_resume(bank->dev);
 	}
 }
 
-- 
1.9.1

Re: [RFC/RFT PATCH 6/7] gpio: omap: clean up omap_gpio_irq_type

[Tony Lindgren]

Hi,

* Grygorii Strashko <grygorii.strashko@linaro.org> [150522 07:37]:
> The omap_gpio_irq_type() can do only configuration of GPIO IRQ
> triggering type, because now OMAP GPIO driver has implemented
> .irq_startup()/.irq_shutdown() which are responsible for
> GPIO bank enabling and pin direction configuration.
> 
> Hence, remove redundant code and omap_gpio_init_irq() which is
> not used any more.
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
> ---
>  drivers/gpio/gpio-omap.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 1c226f1..f02b3fa 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -472,15 +472,6 @@ static int omap_gpio_is_input(struct gpio_bank *bank, unsigned offset)
>  	return readl_relaxed(reg) & BIT(offset);
>  }
>  
> -static void omap_gpio_init_irq(struct gpio_bank *bank, unsigned offset)
> -{
> -	if (!LINE_USED(bank->mod_usage, offset)) {
> -		omap_enable_gpio_module(bank, offset);
> -		omap_set_gpio_direction(bank, offset, 1);
> -	}
> -	bank->irq_usage |= BIT(offset);
> -}
> -
>  static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
>  {
>  	struct gpio_bank *bank = omap_irq_data_get_bank(d);
> @@ -502,12 +493,6 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
>  	retval = omap_set_gpio_triggering(bank, offset, type);
>  	if (retval)
>  		goto error;
> -	omap_gpio_init_irq(bank, offset);
> -	if (!omap_gpio_is_input(bank, offset)) {
> -		spin_unlock_irqrestore(&bank->lock, flags);
> -		retval = -EINVAL;
> -		goto error;
> -	}
>  	spin_unlock_irqrestore(&bank->lock, flags);
>  
>  	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))

This one seems to break smsc911x GPIO interrupt somehow and
nfsroot becomes very slow and system becomes unresponsive.
The Ethernet conroller probably runs mostly in polling mode
or something with this patch.

Regards,

Tony

Re: [RFC/RFT PATCH v2 7/7] gpio: omap: ensure that runtime pm will disable unused gpio banks

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@linaro.org> [150522 07:37]:
> Now there are some points related to Runtime PM usage:
> 1) bank state doesn't need to be checked in places where
> Rintime PM is used, bacause Runtime PM maintains its
> own usage counter:
>       if (!BANK_USED(bank))
>                pm_runtime_get_sync(bank->dev);
> so, it's safe to drop such checks.
> 
> 2) Such implementation is racy, because check !BANK_USED(bank)
>    is not protected, like:
>  CPU0				CPU1
>  gpio_request(bankX.A)
>  ...
>  gpio_free(bankX.A)		gpio_request(bankX.Y)
> 
>  and bankX can be unpowered in the middle of processing
>  gpio_request(bankX.Y)
> 
> 3) There is a call of pm_runtime_get_sync() in omap_gpio_irq_type(),
> but no corresponding put. As result, GPIO devices could be
> powered on forever if at least one GPIO was used as IRQ.
> Hence, allow powering off GPIO banks by adding missed
> pm_runtime_put(bank->dev) at the end of omap_gpio_irq_type().
> 
> As, part of this change update omap2_gpio_xxxx_idle() functions
> to use pm_runtime_force_suspend()/pm_runtime_force_resume().
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
> ---
> Changes in v2:
>  - omap2_gpio_xxxx_idle() functions switched to use 
>    pm_runtime_force_suspend()/pm_runtime_force_resume() API.
> 
> v1:
>     http://marc.info/?l=linux-gpio&m=142567003515626&w=2

This one causes an abort during boot on at least omap3.

Maybe try to get a beagleboard xm to test with? It has Ethernet
over USB though, so that does not work with PM over NFSroot.

If you want something to test with PM with mainline over
NFSroot, maybe you can get hold of some of the better supported
ones out of these boards:

$ git grep "ethernet@gpmc" arch/arm/boot/dts/*.dts*

For those Ethernet has a GPIO interrupt ;)

Regards,

Tony


[    6.150238] Unhandled fault: external abort on non-linefetch (0x1028) at 0xfb05601c
[    6.158355] pgd = c0004000
[    6.161224] [fb05601c] *pgd=49011452(bad)
[    6.165496] Internal error: : 1028 [#1] SMP ARM
[    6.170288] Modules linked in:
[    6.173522] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.1.0-rc4-next-20150522-00021-g6d8613e #94
[    6.182800] Hardware name: Generic OMAP36xx (Flattened Device Tree)
[    6.189422] task: c09b0678 ti: c09aa000 task.ti: c09aa000
[    6.195129] PC is at omap_gpio_irq_handler+0x9c/0x234
[    6.200469] LR is at trace_hardirqs_off+0x14/0x18
[    6.205444] pc : [<c03d9b68>]    lr : [<c0095924>]    psr: a0000193
[    6.205444] sp : c09abcf0  ip : c09abca8  fp : c09abd2c
[    6.217529] r10: c06786c0  r9 : fb056000  r8 : 00000000
[    6.223052] r7 : c59fa010  r6 : c5816300  r5 : 00000001  r4 : c59fa084
[    6.229949] r3 : ffffffff  r2 : fb05601c  r1 : c0a2461c  r0 : 0000001c
[    6.236846] Flags: NzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
[    6.244628] Control: 10c5387d  Table: 80004019  DAC: 00000015
[    6.250701] Process swapper/0 (pid: 0, stack limit = 0xc09aa218)
[    6.257049] Stack: (0xc09abcf0 to 0xc09ac000)
[    6.261657] bce0:                                     c09abd1c c5816300 c5802c64 fb056018
[    6.270294] bd00: 00000000 00000031 00000031 c09adacc 00000000 00000001 c5802000 c06786c0
[    6.278900] bd20: c09abd44 c09abd30 c00a674c c03d9ad8 00000177 c09a6110 c09abd6c c09abd48
[    6.287536] bd40: c00a6aa0 c00a6720 c12536c0 c09abd98 c0a22f40 00000021 00000001 00000001
[    6.296173] bd60: c09abd94 c09abd70 c0009510 c00a6a38 00001bb6 c0671c0c 20000113 ffffffff
[    6.304809] bd80: c09abdcc 00000001 c09abdf4 c09abd98 c06725e4 c0009464 00000001 00000001
[    6.313446] bda0: 00000000 00000000 a0000113 c09c610c a0000113 00000001 00000001 00000001
[    6.322082] bdc0: c06786c0 c09abdf4 c09abdb0 c09abde0 c0098d44 c0671c0c 20000113 ffffffff
[    6.330718] bde0: c09c60ac c09c610c c09abe14 c09abdf8 c002cd60 c0671bd4 c59ff200 00000000
[    6.339355] be00: c59ff240 00000000 c09abe2c c09abe18 c002e21c c002cd2c 00000000 c5a03010
[    6.347961] be20: c09abe44 c09abe30 c002e288 c002e1e0 c5a03010 c0a245dc c09abe5c c09abe48
[    6.356597] be40: c0446588 c002e268 c59fa010 c0a245dc c09abe7c c09abe60 c03da610 c044654c
[    6.365234] be60: fa306ae0 c0a64d28 00000003 00000000 c09abea4 c09abe80 c0030d4c c03da5cc
[    6.373870] be80: 00000001 00000008 00000002 c0a64d68 c06786b8 00000001 c09abedc c09abea8
[    6.382507] bea0: c00323a4 c0030c1c 65a0bc00 00000001 c0513e94 00000000 c09b3af4 6b46e923
[    6.391143] bec0: 00000001 c6e75380 00000003 00000000 c09abf2c c09abee0 c0512550 c003229c
[    6.399780] bee0: c09b3af4 000014a4 6b46e923 00000001 00000000 c09abf00 6b46e923 00000001
[    6.408386] bf00: 00000000 c09b3af4 c6e75380 00000003 c6e75380 c09b3af4 c09adacc c0679fa8
[    6.417022] bf20: c09abf4c c09abf30 c0512854 c05124c0 00000000 00000003 c0a5f8c8 c09ac988
[    6.425659] bf40: c09abf64 c09abf50 c008dc40 c051281c 00007530 c09ac9ec c09abf84 c09abf68
[    6.434295] bf60: c008de70 c008dc14 c09a7378 c09a34c4 c066cc98 c09ac8c0 c09abfac c09abf88
[    6.442932] bf80: c06653d4 c008dc74 00000000 00000000 c06652a0 ffffffff c0a64050 c0a64000
[    6.451568] bfa0: c09abff4 c09abfb0 c0921ccc c06652ac ffffffff ffffffff 00000000 c09216d4
[    6.460205] bfc0: 00000000 c0975368 00000000 c0a64214 c09ac96c c0975364 c09b20f0 80004059
[    6.468841] bfe0: 413fc082 00000000 00000000 c09abff8 8000807c c0921954 00000000 00000000
[    6.477508] [<c03d9b68>] (omap_gpio_irq_handler) from [<c00a674c>] (generic_handle_irq+0x38/0x4c)
[    6.486877] [<c00a674c>] (generic_handle_irq) from [<c00a6aa0>] (__handle_domain_irq+0x74/0xf0)
[    6.496063] [<c00a6aa0>] (__handle_domain_irq) from [<c0009510>] (omap_intc_handle_irq+0xb8/0xc8)
[    6.505432] [<c0009510>] (omap_intc_handle_irq) from [<c06725e4>] (__irq_svc+0x44/0x5c)
[    6.513854] Exception stack(0xc09abd98 to 0xc09abde0)
[    6.519195] bd80:                                                       00000001 00000001
[    6.527832] bda0: 00000000 00000000 a0000113 c09c610c a0000113 00000001 00000001 00000001
[    6.536468] bdc0: c06786c0 c09abdf4 c09abdb0 c09abde0 c0098d44 c0671c0c 20000113 ffffffff
[    6.545104] [<c06725e4>] (__irq_svc) from [<c0671c0c>] (_raw_spin_unlock_irqrestore+0x44/0x54)
[    6.554229] [<c0671c0c>] (_raw_spin_unlock_irqrestore) from [<c002cd60>] (omap_hwmod_idle+0x40/0x50)
[    6.563873] [<c002cd60>] (omap_hwmod_idle) from [<c002e21c>] (omap_device_idle+0x48/0x88)
[    6.572509] [<c002e21c>] (omap_device_idle) from [<c002e288>] (_od_runtime_suspend+0x2c/0x34)
[    6.581512] [<c002e288>] (_od_runtime_suspend) from [<c0446588>] (pm_runtime_force_suspend+0x48/0x84)
[    6.591247] [<c0446588>] (pm_runtime_force_suspend) from [<c03da610>] (omap2_gpio_prepare_for_idle+0x50/0x64)
[    6.601745] [<c03da610>] (omap2_gpio_prepare_for_idle) from [<c0030d4c>] (omap_sram_idle+0x13c/0x24c)
[    6.611480] [<c0030d4c>] (omap_sram_idle) from [<c00323a4>] (omap3_enter_idle_bm+0x114/0x228)
[    6.620483] [<c00323a4>] (omap3_enter_idle_bm) from [<c0512550>] (cpuidle_enter_state+0x9c/0x330)
[    6.629852] [<c0512550>] (cpuidle_enter_state) from [<c0512854>] (cpuidle_enter+0x44/0x50)
[    6.638580] [<c0512854>] (cpuidle_enter) from [<c008dc40>] (call_cpuidle+0x38/0x60)
[    6.646667] [<c008dc40>] (call_cpuidle) from [<c008de70>] (cpu_startup_entry+0x208/0x38c)
[    6.655303] [<c008de70>] (cpu_startup_entry) from [<c06653d4>] (rest_init+0x134/0x170)
[    6.663665] [<c06653d4>] (rest_init) from [<c0921ccc>] (start_kernel+0x384/0x3fc)
[    6.671569] [<c0921ccc>] (start_kernel) from [<8000807c>] (0x8000807c)
[    6.678466] Code: e1d101b4 e1a03315 e0822000 e2433001 (e5926000) 
[    6.684936] ---[ end trace 56ea2e3fa23d8248 ]---

Re: [RFC/RFT PATCH 0/7] gpio: omap: rework and fixes

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@linaro.org> [150522 07:37]:
> Hi Tony,
> 
> As I promised in [1] I've prepared a new series for OMAP GPIO driver.

Great!
 
> Patches 1-2 are bug fixes.
> 
> Patches 3-6 is attempt (RFC/RFT) to rework OMAP GPIO driver taking into account
> that GPIO Chip and GPIO IRQ Chip functionality are mostly orthogonal.

Patches 1-5 seem to work for me, patch 6 does not.
So for patches 1-5, please feel free to add:

Tested-by: Tony Lindgren <tony@atomide.com>
 
> Patch 7 is second attempt (RFC/RFT) to Runtime PM APIs without checking current
> GPIO bank's state with BANK_USED() macro.

Made some comments on patches 6-7 on what I saw.

Regards,

Tony

Re: [RFC/RFT PATCH v2 7/7] gpio: omap: ensure that runtime pm will disable unused gpio banks

[Grygorii.Strashko@linaro.org]

Hi Tony,

On 05/22/2015 09:10 PM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@linaro.org> [150522 07:37]:
>> Now there are some points related to Runtime PM usage:
>> 1) bank state doesn't need to be checked in places where
>> Rintime PM is used, bacause Runtime PM maintains its
>> own usage counter:
>>        if (!BANK_USED(bank))
>>                 pm_runtime_get_sync(bank->dev);
>> so, it's safe to drop such checks.
>>
>> 2) Such implementation is racy, because check !BANK_USED(bank)
>>     is not protected, like:
>>   CPU0				CPU1
>>   gpio_request(bankX.A)
>>   ...
>>   gpio_free(bankX.A)		gpio_request(bankX.Y)
>>
>>   and bankX can be unpowered in the middle of processing
>>   gpio_request(bankX.Y)
>>
>> 3) There is a call of pm_runtime_get_sync() in omap_gpio_irq_type(),
>> but no corresponding put. As result, GPIO devices could be
>> powered on forever if at least one GPIO was used as IRQ.
>> Hence, allow powering off GPIO banks by adding missed
>> pm_runtime_put(bank->dev) at the end of omap_gpio_irq_type().
>>
>> As, part of this change update omap2_gpio_xxxx_idle() functions
>> to use pm_runtime_force_suspend()/pm_runtime_force_resume().
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
>> ---
>> Changes in v2:
>>   - omap2_gpio_xxxx_idle() functions switched to use
>>     pm_runtime_force_suspend()/pm_runtime_force_resume() API.
>>
>> v1:
>>      http://marc.info/?l=linux-gpio&m=142567003515626&w=2
>
> This one causes an abort during boot on at least omap3.
>
> Maybe try to get a beagleboard xm to test with? It has Ethernet
> over USB though, so that does not work with PM over NFSroot.
>
> If you want something to test with PM with mainline over
> NFSroot, maybe you can get hold of some of the better supported
> ones out of these boards:
>
> $ git grep "ethernet@gpmc" arch/arm/boot/dts/*.dts*
>
> For those Ethernet has a GPIO interrupt ;)

Sure, I'll try get beagleboard.

But, this log is very interesting :( What I can see
from it is that GPIO IRQ is triggered in the middle of
omap_sram_idle() - which shouldn't happen, because this is
cpuidle path and local IRQs should be disabled.

Am I missing smth?

>
> [    6.150238] Unhandled fault: external abort on non-linefetch (0x1028) at 0xfb05601c
> [    6.158355] pgd = c0004000
> [    6.161224] [fb05601c] *pgd=49011452(bad)
> [    6.165496] Internal error: : 1028 [#1] SMP ARM
> [    6.170288] Modules linked in:
> [    6.173522] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.1.0-rc4-next-20150522-00021-g6d8613e #94
> [    6.182800] Hardware name: Generic OMAP36xx (Flattened Device Tree)
> [    6.189422] task: c09b0678 ti: c09aa000 task.ti: c09aa000
> [    6.195129] PC is at omap_gpio_irq_handler+0x9c/0x234
> [    6.200469] LR is at trace_hardirqs_off+0x14/0x18
> [    6.205444] pc : [<c03d9b68>]    lr : [<c0095924>]    psr: a0000193
> [    6.205444] sp : c09abcf0  ip : c09abca8  fp : c09abd2c
> [    6.217529] r10: c06786c0  r9 : fb056000  r8 : 00000000
> [    6.223052] r7 : c59fa010  r6 : c5816300  r5 : 00000001  r4 : c59fa084
> [    6.229949] r3 : ffffffff  r2 : fb05601c  r1 : c0a2461c  r0 : 0000001c
> [    6.236846] Flags: NzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
> [    6.244628] Control: 10c5387d  Table: 80004019  DAC: 00000015
> [    6.250701] Process swapper/0 (pid: 0, stack limit = 0xc09aa218)
> [    6.257049] Stack: (0xc09abcf0 to 0xc09ac000)
> [    6.261657] bce0:                                     c09abd1c c5816300 c5802c64 fb056018
> [    6.270294] bd00: 00000000 00000031 00000031 c09adacc 00000000 00000001 c5802000 c06786c0
> [    6.278900] bd20: c09abd44 c09abd30 c00a674c c03d9ad8 00000177 c09a6110 c09abd6c c09abd48
> [    6.287536] bd40: c00a6aa0 c00a6720 c12536c0 c09abd98 c0a22f40 00000021 00000001 00000001
> [    6.296173] bd60: c09abd94 c09abd70 c0009510 c00a6a38 00001bb6 c0671c0c 20000113 ffffffff
> [    6.304809] bd80: c09abdcc 00000001 c09abdf4 c09abd98 c06725e4 c0009464 00000001 00000001
> [    6.313446] bda0: 00000000 00000000 a0000113 c09c610c a0000113 00000001 00000001 00000001
> [    6.322082] bdc0: c06786c0 c09abdf4 c09abdb0 c09abde0 c0098d44 c0671c0c 20000113 ffffffff
> [    6.330718] bde0: c09c60ac c09c610c c09abe14 c09abdf8 c002cd60 c0671bd4 c59ff200 00000000
> [    6.339355] be00: c59ff240 00000000 c09abe2c c09abe18 c002e21c c002cd2c 00000000 c5a03010
> [    6.347961] be20: c09abe44 c09abe30 c002e288 c002e1e0 c5a03010 c0a245dc c09abe5c c09abe48
> [    6.356597] be40: c0446588 c002e268 c59fa010 c0a245dc c09abe7c c09abe60 c03da610 c044654c
> [    6.365234] be60: fa306ae0 c0a64d28 00000003 00000000 c09abea4 c09abe80 c0030d4c c03da5cc
> [    6.373870] be80: 00000001 00000008 00000002 c0a64d68 c06786b8 00000001 c09abedc c09abea8
> [    6.382507] bea0: c00323a4 c0030c1c 65a0bc00 00000001 c0513e94 00000000 c09b3af4 6b46e923
> [    6.391143] bec0: 00000001 c6e75380 00000003 00000000 c09abf2c c09abee0 c0512550 c003229c
> [    6.399780] bee0: c09b3af4 000014a4 6b46e923 00000001 00000000 c09abf00 6b46e923 00000001
> [    6.408386] bf00: 00000000 c09b3af4 c6e75380 00000003 c6e75380 c09b3af4 c09adacc c0679fa8
> [    6.417022] bf20: c09abf4c c09abf30 c0512854 c05124c0 00000000 00000003 c0a5f8c8 c09ac988
> [    6.425659] bf40: c09abf64 c09abf50 c008dc40 c051281c 00007530 c09ac9ec c09abf84 c09abf68
> [    6.434295] bf60: c008de70 c008dc14 c09a7378 c09a34c4 c066cc98 c09ac8c0 c09abfac c09abf88
> [    6.442932] bf80: c06653d4 c008dc74 00000000 00000000 c06652a0 ffffffff c0a64050 c0a64000
> [    6.451568] bfa0: c09abff4 c09abfb0 c0921ccc c06652ac ffffffff ffffffff 00000000 c09216d4
> [    6.460205] bfc0: 00000000 c0975368 00000000 c0a64214 c09ac96c c0975364 c09b20f0 80004059
> [    6.468841] bfe0: 413fc082 00000000 00000000 c09abff8 8000807c c0921954 00000000 00000000
> [    6.477508] [<c03d9b68>] (omap_gpio_irq_handler) from [<c00a674c>] (generic_handle_irq+0x38/0x4c)
> [    6.486877] [<c00a674c>] (generic_handle_irq) from [<c00a6aa0>] (__handle_domain_irq+0x74/0xf0)
> [    6.496063] [<c00a6aa0>] (__handle_domain_irq) from [<c0009510>] (omap_intc_handle_irq+0xb8/0xc8)
> [    6.505432] [<c0009510>] (omap_intc_handle_irq) from [<c06725e4>] (__irq_svc+0x44/0x5c)
> [    6.513854] Exception stack(0xc09abd98 to 0xc09abde0)
> [    6.519195] bd80:                                                       00000001 00000001
> [    6.527832] bda0: 00000000 00000000 a0000113 c09c610c a0000113 00000001 00000001 00000001
> [    6.536468] bdc0: c06786c0 c09abdf4 c09abdb0 c09abde0 c0098d44 c0671c0c 20000113 ffffffff
> [    6.545104] [<c06725e4>] (__irq_svc) from [<c0671c0c>] (_raw_spin_unlock_irqrestore+0x44/0x54)
> [    6.554229] [<c0671c0c>] (_raw_spin_unlock_irqrestore) from [<c002cd60>] (omap_hwmod_idle+0x40/0x50)
> [    6.563873] [<c002cd60>] (omap_hwmod_idle) from [<c002e21c>] (omap_device_idle+0x48/0x88)
> [    6.572509] [<c002e21c>] (omap_device_idle) from [<c002e288>] (_od_runtime_suspend+0x2c/0x34)
> [    6.581512] [<c002e288>] (_od_runtime_suspend) from [<c0446588>] (pm_runtime_force_suspend+0x48/0x84)
> [    6.591247] [<c0446588>] (pm_runtime_force_suspend) from [<c03da610>] (omap2_gpio_prepare_for_idle+0x50/0x64)
> [    6.601745] [<c03da610>] (omap2_gpio_prepare_for_idle) from [<c0030d4c>] (omap_sram_idle+0x13c/0x24c)
> [    6.611480] [<c0030d4c>] (omap_sram_idle) from [<c00323a4>] (omap3_enter_idle_bm+0x114/0x228)
> [    6.620483] [<c00323a4>] (omap3_enter_idle_bm) from [<c0512550>] (cpuidle_enter_state+0x9c/0x330)
> [    6.629852] [<c0512550>] (cpuidle_enter_state) from [<c0512854>] (cpuidle_enter+0x44/0x50)
> [    6.638580] [<c0512854>] (cpuidle_enter) from [<c008dc40>] (call_cpuidle+0x38/0x60)
> [    6.646667] [<c008dc40>] (call_cpuidle) from [<c008de70>] (cpu_startup_entry+0x208/0x38c)
> [    6.655303] [<c008de70>] (cpu_startup_entry) from [<c06653d4>] (rest_init+0x134/0x170)
> [    6.663665] [<c06653d4>] (rest_init) from [<c0921ccc>] (start_kernel+0x384/0x3fc)
> [    6.671569] [<c0921ccc>] (start_kernel) from [<8000807c>] (0x8000807c)
> [    6.678466] Code: e1d101b4 e1a03315 e0822000 e2433001 (e5926000)
> [    6.684936] ---[ end trace 56ea2e3fa23d8248 ]---
>


-- 
regards,
-grygorii

Re: [RFC/RFT PATCH v2 7/7] gpio: omap: ensure that runtime pm will disable unused gpio banks

[Grygorii.Strashko@linaro.org]

On 05/25/2015 05:46 PM, Grygorii.Strashko@linaro.org wrote:
> Hi Tony,
> 
> On 05/22/2015 09:10 PM, Tony Lindgren wrote:
>> * Grygorii Strashko <grygorii.strashko@linaro.org> [150522 07:37]:
>>> Now there are some points related to Runtime PM usage:
>>> 1) bank state doesn't need to be checked in places where
>>> Rintime PM is used, bacause Runtime PM maintains its
>>> own usage counter:
>>>        if (!BANK_USED(bank))
>>>                 pm_runtime_get_sync(bank->dev);
>>> so, it's safe to drop such checks.
>>>
>>> 2) Such implementation is racy, because check !BANK_USED(bank)
>>>     is not protected, like:
>>>   CPU0                CPU1
>>>   gpio_request(bankX.A)
>>>   ...
>>>   gpio_free(bankX.A)        gpio_request(bankX.Y)
>>>
>>>   and bankX can be unpowered in the middle of processing
>>>   gpio_request(bankX.Y)
>>>
>>> 3) There is a call of pm_runtime_get_sync() in omap_gpio_irq_type(),
>>> but no corresponding put. As result, GPIO devices could be
>>> powered on forever if at least one GPIO was used as IRQ.
>>> Hence, allow powering off GPIO banks by adding missed
>>> pm_runtime_put(bank->dev) at the end of omap_gpio_irq_type().
>>>
>>> As, part of this change update omap2_gpio_xxxx_idle() functions
>>> to use pm_runtime_force_suspend()/pm_runtime_force_resume().
>>>
>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
>>> ---
>>> Changes in v2:
>>>   - omap2_gpio_xxxx_idle() functions switched to use
>>>     pm_runtime_force_suspend()/pm_runtime_force_resume() API.
>>>
>>> v1:
>>>      http://marc.info/?l=linux-gpio&m=142567003515626&w=2
>>
>> This one causes an abort during boot on at least omap3.
>>
>> Maybe try to get a beagleboard xm to test with? It has Ethernet
>> over USB though, so that does not work with PM over NFSroot.
>>
>> If you want something to test with PM with mainline over
>> NFSroot, maybe you can get hold of some of the better supported
>> ones out of these boards:
>>
>> $ git grep "ethernet@gpmc" arch/arm/boot/dts/*.dts*
>>
>> For those Ethernet has a GPIO interrupt ;)
> 
> Sure, I'll try get beagleboard.
> 
> But, this log is very interesting :( What I can see
> from it is that GPIO IRQ is triggered in the middle of
> omap_sram_idle() - which shouldn't happen, because this is
> cpuidle path and local IRQs should be disabled.
> 
> Am I missing smth?
> 

Yep. I've missed this :(  
pm_runtime_force_suspend
 |- pm_runtime_disable
    |-__pm_runtime_disable
       |- spin_unlock_irq(&dev->power.lock);

So, Runtime PM forced API can't be used in cpuidle path :(

Sorry.

-- 
regards,
-grygorii

Re: [PATCH 1/7] gpio: omap: fix omap_gpio_free to not clean up irq configuration

[Linus Walleij]

On Fri, May 22, 2015 at 4:35 PM, Grygorii Strashko
<grygorii.strashko@linaro.org> wrote:

> This patch fixes following issue:
> - GPIOn is used as IRQ by some dev, for example PCF8575.INT ->  gpio6.11
> - PCFx driver knows nothing about type of IRQ line (GPIO or not)
>   so it doesn't request gpio and just do request_irq()
> - If gpio6.11 will be exported through the sysfs and then un-xeported
> then IRQs from PCFx will not be received any more, because
> IRQ configuration for gpio6.11 will be cleaned up unconditionally
> in omap_gpio_free.
>
> Fix this by removing all GPIO IRQ specific code from omap_gpio_free()
> and also do GPIO clean up (change direction to 'in' and disable debounce)
> only if corresponding GPIO is not used as IRQ too.
> GPIO IRQ will be properly cleaned up by GPIO irqchip code.
>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>

Can I get an ACK or comment from one of the three (!) maintainers
on atleast these non-RFC patches?

Yours,
Linus Walleij

Re: [RFC/RFT PATCH 0/7] gpio: omap: rework and fixes

[Linus Walleij]

On Fri, May 22, 2015 at 9:03 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Grygorii Strashko <grygorii.strashko@linaro.org> [150522 07:37]:

> Patches 1-5 seem to work for me, patch 6 does not.
> So for patches 1-5, please feel free to add:
>
> Tested-by: Tony Lindgren <tony@atomide.com>

OK I take this as an excuse to apply patches 1-5 with
Tony's test tag.

The maintainers can cheer in if they want, I will
anyway take the OMAP maintainers test tag as
a good quality indication.

Yours,
Linus Walleij

Re: [RFC/RFT PATCH 0/7] gpio: omap: rework and fixes

[santosh shilimkar]

On 6/1/2015 6:15 AM, Linus Walleij wrote:
> On Fri, May 22, 2015 at 9:03 PM, Tony Lindgren <tony@atomide.com> wrote:
>> * Grygorii Strashko <grygorii.strashko@linaro.org> [150522 07:37]:
>
>> Patches 1-5 seem to work for me, patch 6 does not.
>> So for patches 1-5, please feel free to add:
>>
>> Tested-by: Tony Lindgren <tony@atomide.com>
>
> OK I take this as an excuse to apply patches 1-5 with
> Tony's test tag.
>
> The maintainers can cheer in if they want, I will
> anyway take the OMAP maintainers test tag as
> a good quality indication.
>
Cheers for patch 1-5 ;-)

Acked-by: Santosh Shilimkar <ssantosh@kernel.org>

Re: [PATCH 1/7] gpio: omap: fix omap_gpio_free to not clean up irq configuration

[Javier Martinez Canillas]

Hello Linus,

On Mon, Jun 1, 2015 at 3:11 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, May 22, 2015 at 4:35 PM, Grygorii Strashko
> <grygorii.strashko@linaro.org> wrote:
>
>> This patch fixes following issue:
>> - GPIOn is used as IRQ by some dev, for example PCF8575.INT ->  gpio6.11
>> - PCFx driver knows nothing about type of IRQ line (GPIO or not)
>>   so it doesn't request gpio and just do request_irq()
>> - If gpio6.11 will be exported through the sysfs and then un-xeported
>> then IRQs from PCFx will not be received any more, because
>> IRQ configuration for gpio6.11 will be cleaned up unconditionally
>> in omap_gpio_free.
>>
>> Fix this by removing all GPIO IRQ specific code from omap_gpio_free()
>> and also do GPIO clean up (change direction to 'in' and disable debounce)
>> only if corresponding GPIO is not used as IRQ too.
>> GPIO IRQ will be properly cleaned up by GPIO irqchip code.
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
>
> Can I get an ACK or comment from one of the three (!) maintainers
> on atleast these non-RFC patches?
>

Sorry for the delay, the patch looks good to me.

Acked-by: Javier Martinez Canillas <javier@dowhile0.org>

> Yours,
> Linus Walleij

Best regards,
Javier

Re: [PATCH 2/7] gpio: omap: fix error handling in omap_gpio_irq_type

[Javier Martinez Canillas]

Hello Grygorii,

On Fri, May 22, 2015 at 4:35 PM, Grygorii Strashko
<grygorii.strashko@linaro.org> wrote:
> The GPIO bank will be kept powered in case if input parameters
> are invalid or error occurred in omap_gpio_irq_type.
>
> Hence, fix it by ensuring that GPIO bank will be unpowered
> in case of errors and add additional check of value returned
> from omap_set_gpio_triggering().
>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
> ---
>  drivers/gpio/gpio-omap.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index bb60cbc..f6cc638 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -488,9 +488,6 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
>         unsigned long flags;
>         unsigned offset = d->hwirq;
>
> -       if (!BANK_USED(bank))
> -               pm_runtime_get_sync(bank->dev);
> -
>         if (type & ~IRQ_TYPE_SENSE_MASK)
>                 return -EINVAL;
>
> @@ -498,12 +495,18 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
>                 (type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
>                 return -EINVAL;
>
> +       if (!BANK_USED(bank))
> +               pm_runtime_get_sync(bank->dev);
> +
>         spin_lock_irqsave(&bank->lock, flags);
>         retval = omap_set_gpio_triggering(bank, offset, type);
> +       if (retval)

At this point the bank->lock spinlock is held so you need to add a
spin_unlock_irqrestore() in the error path.

> +               goto error;
>         omap_gpio_init_irq(bank, offset);
>         if (!omap_gpio_is_input(bank, offset)) {
>                 spin_unlock_irqrestore(&bank->lock, flags);
> -               return -EINVAL;
> +               retval = -EINVAL;
> +               goto error;
>         }
>         spin_unlock_irqrestore(&bank->lock, flags);
>
> @@ -512,6 +515,11 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
>         else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
>                 __irq_set_handler_locked(d->irq, handle_edge_irq);
>
> +       return 0;
> +
> +error:
> +       if (!BANK_USED(bank))
> +               pm_runtime_put(bank->dev);
>         return retval;
>  }
>
> --

But you are correct about the runtime PM bug so after addressing the
above comment, feel free to add:

Acked-by: Javier Martinez Canillas <javier@dowhile0.org>

Best regards,
Javier

Re: [PATCH 2/7] gpio: omap: fix error handling in omap_gpio_irq_type

[Grygorii.Strashko@linaro.org]

On 06/02/2015 12:40 PM, Javier Martinez Canillas wrote:
> Hello Grygorii,
>
> On Fri, May 22, 2015 at 4:35 PM, Grygorii Strashko
> <grygorii.strashko@linaro.org> wrote:
>> The GPIO bank will be kept powered in case if input parameters
>> are invalid or error occurred in omap_gpio_irq_type.
>>
>> Hence, fix it by ensuring that GPIO bank will be unpowered
>> in case of errors and add additional check of value returned
>> from omap_set_gpio_triggering().
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
>> ---
>>   drivers/gpio/gpio-omap.c | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index bb60cbc..f6cc638 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -488,9 +488,6 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
>>          unsigned long flags;
>>          unsigned offset = d->hwirq;
>>
>> -       if (!BANK_USED(bank))
>> -               pm_runtime_get_sync(bank->dev);
>> -
>>          if (type & ~IRQ_TYPE_SENSE_MASK)
>>                  return -EINVAL;
>>
>> @@ -498,12 +495,18 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
>>                  (type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
>>                  return -EINVAL;
>>
>> +       if (!BANK_USED(bank))
>> +               pm_runtime_get_sync(bank->dev);
>> +
>>          spin_lock_irqsave(&bank->lock, flags);
>>          retval = omap_set_gpio_triggering(bank, offset, type);
>> +       if (retval)
>
> At this point the bank->lock spinlock is held so you need to add a
> spin_unlock_irqrestore() in the error path.

Ops. Thanks for catching it.
>
>> +               goto error;
>>          omap_gpio_init_irq(bank, offset);
>>          if (!omap_gpio_is_input(bank, offset)) {
>>                  spin_unlock_irqrestore(&bank->lock, flags);
>> -               return -EINVAL;
>> +               retval = -EINVAL;
>> +               goto error;
>>          }
>>          spin_unlock_irqrestore(&bank->lock, flags);
>>
>> @@ -512,6 +515,11 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
>>          else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
>>                  __irq_set_handler_locked(d->irq, handle_edge_irq);
>>
>> +       return 0;
>> +
>> +error:
>> +       if (!BANK_USED(bank))
>> +               pm_runtime_put(bank->dev);
>>          return retval;
>>   }
>>
>> --
>
> But you are correct about the runtime PM bug so after addressing the
> above comment, feel free to add:
>
> Acked-by: Javier Martinez Canillas <javier@dowhile0.org>
>

Linus, How do prefer me to fix it?
Resend whole patch or send additional fix on top of patch 5.


-- 
regards,
-grygorii

Re: [PATCH 2/7] gpio: omap: fix error handling in omap_gpio_irq_type

[Grygorii.Strashko@linaro.org]

Hi Linus,

On 06/02/2015 05:27 PM, Grygorii.Strashko@linaro.org wrote:
> On 06/02/2015 12:40 PM, Javier Martinez Canillas wrote:
>> Hello Grygorii,
>>
>> On Fri, May 22, 2015 at 4:35 PM, Grygorii Strashko
>> <grygorii.strashko@linaro.org> wrote:
>>> The GPIO bank will be kept powered in case if input parameters
>>> are invalid or error occurred in omap_gpio_irq_type.
>>>
>>> Hence, fix it by ensuring that GPIO bank will be unpowered
>>> in case of errors and add additional check of value returned
>>> from omap_set_gpio_triggering().
>>>
>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
>>> ---
>>>   drivers/gpio/gpio-omap.c | 16 ++++++++++++----
>>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>> index bb60cbc..f6cc638 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -488,9 +488,6 @@ static int omap_gpio_irq_type(struct irq_data *d, 
>>> unsigned type)
>>>          unsigned long flags;
>>>          unsigned offset = d->hwirq;
>>>
>>> -       if (!BANK_USED(bank))
>>> -               pm_runtime_get_sync(bank->dev);
>>> -
>>>          if (type & ~IRQ_TYPE_SENSE_MASK)
>>>                  return -EINVAL;
>>>
>>> @@ -498,12 +495,18 @@ static int omap_gpio_irq_type(struct irq_data 
>>> *d, unsigned type)
>>>                  (type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
>>>                  return -EINVAL;
>>>
>>> +       if (!BANK_USED(bank))
>>> +               pm_runtime_get_sync(bank->dev);
>>> +
>>>          spin_lock_irqsave(&bank->lock, flags);
>>>          retval = omap_set_gpio_triggering(bank, offset, type);
>>> +       if (retval)
>>
>> At this point the bank->lock spinlock is held so you need to add a
>> spin_unlock_irqrestore() in the error path.
> 
> Ops. Thanks for catching it.
>>
>>> +               goto error;
>>>          omap_gpio_init_irq(bank, offset);
>>>          if (!omap_gpio_is_input(bank, offset)) {
>>>                  spin_unlock_irqrestore(&bank->lock, flags);
>>> -               return -EINVAL;
>>> +               retval = -EINVAL;
>>> +               goto error;
>>>          }
>>>          spin_unlock_irqrestore(&bank->lock, flags);
>>>
>>> @@ -512,6 +515,11 @@ static int omap_gpio_irq_type(struct irq_data 
>>> *d, unsigned type)
>>>          else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
>>>                  __irq_set_handler_locked(d->irq, handle_edge_irq);
>>>
>>> +       return 0;
>>> +
>>> +error:
>>> +       if (!BANK_USED(bank))
>>> +               pm_runtime_put(bank->dev);
>>>          return retval;
>>>   }
>>>
>>> -- 
>>
>> But you are correct about the runtime PM bug so after addressing the
>> above comment, feel free to add:
>>
>> Acked-by: Javier Martinez Canillas <javier@dowhile0.org>
>>
> 
> Linus, How do prefer me to fix it?
> Resend whole patch or send additional fix on top of patch 5.
> 

Below is the additional patch to fix issue reported by Javier.
Pls. inform me if you would like me to send v2 of this patch instead.

-- 
regards,
-grygorii

------ <
>From 611cdfe480f095136e55f155bb71aeb09ea994b0 Mon Sep 17 00:00:00 2001
From: Grygorii Strashko <grygorii.strashko@linaro.org>
Date: Wed, 3 Jun 2015 19:15:19 +0300
Subject: [PATCH] gpio: omap: add missed spin_unlock_irqrestore in omap_gpio_irq_type

Add missed spin_unlock_irqrestore in omap_gpio_irq_type when
omap_set_gpio_triggering() is failed.

This fixes patch 'gpio: omap: fix error handling in omap_gpio_irq_type'

Reported-by: Javier Martinez Canillas <javier@dowhile0.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
---
 drivers/gpio/gpio-omap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index e26dc40..e3f8205 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -490,8 +490,10 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
 
 	spin_lock_irqsave(&bank->lock, flags);
 	retval = omap_set_gpio_triggering(bank, offset, type);
-	if (retval)
+	if (retval) {
+		spin_unlock_irqrestore(&bank->lock, flags);
 		goto error;
+	}
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
-- 
1.9.1