[PATCH] gpio: omap: fix irq triggering in smart-idle wakeup mode

[PATCH] gpio: omap: fix irq triggering in smart-idle wakeup mode

[Grygorii Strashko]

Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle
in the following case:
  extcon_usb1(id_irq) ->  pcf8575.gpio1 -> omapgpio6.gpio11 -> gic

the extcon_usb1 is wake up source and it enables IRQ wake up for
id_irq by calling enable/disable_irq_wake() during suspend/resume
which, in turn, causes execution of omap_gpio_wake_enable(). And
omap_gpio_wake_enable() will set/clear corresponding bit in
GPIO_IRQWAKEN_x register.

omapgpio6 configuration after boot - wakeup is enabled for GPIO IRQs
by default from  omap_gpio_irq_type:
GPIO_IRQSTATUS_SET_0    | 0x00000400
GPIO_IRQSTATUS_CLR_0    | 0x00000400
GPIO_IRQWAKEN_0         | 0x00000400
GPIO_RISINGDETECT       | 0x00000000
GPIO_FALLINGDETECT      | 0x00000400

omapgpio6 configuration after after suspend/resume cycle:
GPIO_IRQSTATUS_SET_0    | 0x00000400
GPIO_IRQSTATUS_CLR_0    | 0x00000400
GPIO_IRQWAKEN_0         | 0x00000000 <---
GPIO_RISINGDETECT       | 0x00000000
GPIO_FALLINGDETECT      | 0x00000400

As result, system will start to lose interrupts from pcf8575 GPIO
expander, because when OMAP GPIO IP is in smart-idle wakeup mode, there
is no guarantee that transition(s) on input non wake up GPIO pin will
trigger asynchronous wake-up request to PRCM and then IRQ generation.
IRQ will be generated when GPIO is in active mode - for example, some
time after accessing GPIO bank registers IRQs will be generated
normally, but issue will happen again once PRCM will put GPIO in low
power smart-idle wakeup mode.

Note 1. Issue is not reproduced if debounce clk is enabled for GPIO
bank.

Note 2. Issue hardly reproducible if GPIO pins group contains both
wakeup/non-wakeup gpios - for example, it will be hard to reproduce
issue with pin2 if GPIO_IRQWAKEN_0=0x1 GPIO_IRQSTATUS_SET_0=0x3
GPIO_FALLINGDETECT = 0x3 (TRM "Power Saving by Grouping the Edge/Level
Detection").

Note 3. There nothing common bitween System wake up and OMAP GPIO bank
IP wake up logic - the last one defines how the GPIO bank ON-IDLE-ON
transition will happen inside SoC under control of PRCM.

Hence, fix the problem by removing omap_set_gpio_wakeup() function
completely and so keeping always in sync GPIO IRQ mask/unmask
(IRQSTATUS_SET) and wake up enable (GPIO_IRQWAKEN) bits; and adding
IRQCHIP_MASK_ON_SUSPEND flag in OMAP GPIO irqchip. That way non wakeup
GPIO IRQs will be properly masked/unmask by IRQ PM core during
suspend/resume cycle.

Cc: Roger Quadros <rogerq@ti.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/gpio/gpio-omap.c | 42 ++----------------------------------------
 1 file changed, 2 insertions(+), 40 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 551dfa9..b98ede7 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -611,51 +611,12 @@ static inline void omap_set_gpio_irqenable(struct gpio_bank *bank,
 		omap_disable_gpio_irqbank(bank, BIT(offset));
 }
 
-/*
- * Note that ENAWAKEUP needs to be enabled in GPIO_SYSCONFIG register.
- * 1510 does not seem to have a wake-up register. If JTAG is connected
- * to the target, system will wake up always on GPIO events. While
- * system is running all registered GPIO interrupts need to have wake-up
- * enabled. When system is suspended, only selected GPIO interrupts need
- * to have wake-up enabled.
- */
-static int omap_set_gpio_wakeup(struct gpio_bank *bank, unsigned offset,
-				int enable)
-{
-	u32 gpio_bit = BIT(offset);
-	unsigned long flags;
-
-	if (bank->non_wakeup_gpios & gpio_bit) {
-		dev_err(bank->chip.parent,
-			"Unable to modify wakeup on non-wakeup GPIO%d\n",
-			offset);
-		return -EINVAL;
-	}
-
-	raw_spin_lock_irqsave(&bank->lock, flags);
-	if (enable)
-		bank->context.wake_en |= gpio_bit;
-	else
-		bank->context.wake_en &= ~gpio_bit;
-
-	writel_relaxed(bank->context.wake_en, bank->base + bank->regs->wkup_en);
-	raw_spin_unlock_irqrestore(&bank->lock, flags);
-
-	return 0;
-}
-
 /* Use disable_irq_wake() and enable_irq_wake() functions from drivers */
 static int omap_gpio_wake_enable(struct irq_data *d, unsigned int enable)
 {
 	struct gpio_bank *bank = omap_irq_data_get_bank(d);
-	unsigned offset = d->hwirq;
-	int ret;
 
-	ret = omap_set_gpio_wakeup(bank, offset, enable);
-	if (!ret)
-		ret = irq_set_irq_wake(bank->irq, enable);
-
-	return ret;
+	return irq_set_irq_wake(bank->irq, enable);
 }
 
 static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
@@ -1187,6 +1148,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
 	irqc->irq_bus_lock = omap_gpio_irq_bus_lock,
 	irqc->irq_bus_sync_unlock = gpio_irq_bus_sync_unlock,
 	irqc->name = dev_name(&pdev->dev);
+	irqc->flags = IRQCHIP_MASK_ON_SUSPEND;
 
 	bank->irq = platform_get_irq(pdev, 0);
 	if (bank->irq <= 0) {
-- 
2.8.1

Re: [PATCH] gpio: omap: fix irq triggering in smart-idle wakeup mode

[santosh shilimkar]

On 4/12/2016 3:52 AM, Grygorii Strashko wrote:
> Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle
> in the following case:
>    extcon_usb1(id_irq) ->  pcf8575.gpio1 -> omapgpio6.gpio11 -> gic
>
> the extcon_usb1 is wake up source and it enables IRQ wake up for
> id_irq by calling enable/disable_irq_wake() during suspend/resume
> which, in turn, causes execution of omap_gpio_wake_enable(). And
> omap_gpio_wake_enable() will set/clear corresponding bit in
> GPIO_IRQWAKEN_x register.
>
> omapgpio6 configuration after boot - wakeup is enabled for GPIO IRQs
> by default from  omap_gpio_irq_type:
> GPIO_IRQSTATUS_SET_0    | 0x00000400
> GPIO_IRQSTATUS_CLR_0    | 0x00000400
> GPIO_IRQWAKEN_0         | 0x00000400
> GPIO_RISINGDETECT       | 0x00000000
> GPIO_FALLINGDETECT      | 0x00000400
>
> omapgpio6 configuration after after suspend/resume cycle:
> GPIO_IRQSTATUS_SET_0    | 0x00000400
> GPIO_IRQSTATUS_CLR_0    | 0x00000400
> GPIO_IRQWAKEN_0         | 0x00000000 <---
> GPIO_RISINGDETECT       | 0x00000000
> GPIO_FALLINGDETECT      | 0x00000400
>
> As result, system will start to lose interrupts from pcf8575 GPIO
> expander, because when OMAP GPIO IP is in smart-idle wakeup mode, there
> is no guarantee that transition(s) on input non wake up GPIO pin will
> trigger asynchronous wake-up request to PRCM and then IRQ generation.
> IRQ will be generated when GPIO is in active mode - for example, some
> time after accessing GPIO bank registers IRQs will be generated
> normally, but issue will happen again once PRCM will put GPIO in low
> power smart-idle wakeup mode.
>
> Note 1. Issue is not reproduced if debounce clk is enabled for GPIO
> bank.
>
> Note 2. Issue hardly reproducible if GPIO pins group contains both
> wakeup/non-wakeup gpios - for example, it will be hard to reproduce
> issue with pin2 if GPIO_IRQWAKEN_0=0x1 GPIO_IRQSTATUS_SET_0=0x3
> GPIO_FALLINGDETECT = 0x3 (TRM "Power Saving by Grouping the Edge/Level
> Detection").
>
> Note 3. There nothing common bitween System wake up and OMAP GPIO bank
> IP wake up logic - the last one defines how the GPIO bank ON-IDLE-ON
> transition will happen inside SoC under control of PRCM.
>
> Hence, fix the problem by removing omap_set_gpio_wakeup() function
> completely and so keeping always in sync GPIO IRQ mask/unmask
> (IRQSTATUS_SET) and wake up enable (GPIO_IRQWAKEN) bits; and adding
> IRQCHIP_MASK_ON_SUSPEND flag in OMAP GPIO irqchip. That way non wakeup
> GPIO IRQs will be properly masked/unmask by IRQ PM core during
> suspend/resume cycle.
>
> Cc: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
GPIO IP has two levels of controls for wakeups and you are
just removing the SYSCFG wakeup and relying on the IRQ
line wakeup. I like usage of "IRQCHIP_MASK_ON_SUSPEND" but
please be acreful this change which might break older OMAPs.

>   drivers/gpio/gpio-omap.c | 42 ++----------------------------------------
>   1 file changed, 2 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 551dfa9..b98ede7 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -611,51 +611,12 @@ static inline void omap_set_gpio_irqenable(struct gpio_bank *bank,
>   		omap_disable_gpio_irqbank(bank, BIT(offset));
>   }
>
> -/*
> - * Note that ENAWAKEUP needs to be enabled in GPIO_SYSCONFIG register.
> - * 1510 does not seem to have a wake-up register. If JTAG is connected
> - * to the target, system will wake up always on GPIO events. While
> - * system is running all registered GPIO interrupts need to have wake-up
> - * enabled. When system is suspended, only selected GPIO interrupts need
> - * to have wake-up enabled.
> - */
> -static int omap_set_gpio_wakeup(struct gpio_bank *bank, unsigned offset,
> -				int enable)
> -{
> -	u32 gpio_bit = BIT(offset);
> -	unsigned long flags;
> -
> -	if (bank->non_wakeup_gpios & gpio_bit) {
> -		dev_err(bank->chip.parent,
> -			"Unable to modify wakeup on non-wakeup GPIO%d\n",
> -			offset);
> -		return -EINVAL;
> -	}
> -
> -	raw_spin_lock_irqsave(&bank->lock, flags);
> -	if (enable)
> -		bank->context.wake_en |= gpio_bit;
> -	else
> -		bank->context.wake_en &= ~gpio_bit;
> -
> -	writel_relaxed(bank->context.wake_en, bank->base + bank->regs->wkup_en);
> -	raw_spin_unlock_irqrestore(&bank->lock, flags);
> -
> -	return 0;
> -}
> -
>   /* Use disable_irq_wake() and enable_irq_wake() functions from drivers */
>   static int omap_gpio_wake_enable(struct irq_data *d, unsigned int enable)
>   {
>   	struct gpio_bank *bank = omap_irq_data_get_bank(d);
> -	unsigned offset = d->hwirq;
> -	int ret;
>
> -	ret = omap_set_gpio_wakeup(bank, offset, enable);
> -	if (!ret)
> -		ret = irq_set_irq_wake(bank->irq, enable);
> -
> -	return ret;
> +	return irq_set_irq_wake(bank->irq, enable);
>   }
>
>   static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
> @@ -1187,6 +1148,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
>   	irqc->irq_bus_lock = omap_gpio_irq_bus_lock,
>   	irqc->irq_bus_sync_unlock = gpio_irq_bus_sync_unlock,
>   	irqc->name = dev_name(&pdev->dev);
> +	irqc->flags = IRQCHIP_MASK_ON_SUSPEND;
>
>   	bank->irq = platform_get_irq(pdev, 0);
>   	if (bank->irq <= 0) {
>

Re: [PATCH] gpio: omap: fix irq triggering in smart-idle wakeup mode

[Grygorii Strashko]

On 04/12/2016 07:44 PM, santosh shilimkar wrote:
> On 4/12/2016 3:52 AM, Grygorii Strashko wrote:
>> Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle
>> in the following case:
>>    extcon_usb1(id_irq) ->  pcf8575.gpio1 -> omapgpio6.gpio11 -> gic
>>
>> the extcon_usb1 is wake up source and it enables IRQ wake up for
>> id_irq by calling enable/disable_irq_wake() during suspend/resume
>> which, in turn, causes execution of omap_gpio_wake_enable(). And
>> omap_gpio_wake_enable() will set/clear corresponding bit in
>> GPIO_IRQWAKEN_x register.
>>
>> omapgpio6 configuration after boot - wakeup is enabled for GPIO IRQs
>> by default from  omap_gpio_irq_type:
>> GPIO_IRQSTATUS_SET_0    | 0x00000400
>> GPIO_IRQSTATUS_CLR_0    | 0x00000400
>> GPIO_IRQWAKEN_0         | 0x00000400
>> GPIO_RISINGDETECT       | 0x00000000
>> GPIO_FALLINGDETECT      | 0x00000400
>>
>> omapgpio6 configuration after after suspend/resume cycle:
>> GPIO_IRQSTATUS_SET_0    | 0x00000400
>> GPIO_IRQSTATUS_CLR_0    | 0x00000400
>> GPIO_IRQWAKEN_0         | 0x00000000 <---
>> GPIO_RISINGDETECT       | 0x00000000
>> GPIO_FALLINGDETECT      | 0x00000400
>>
>> As result, system will start to lose interrupts from pcf8575 GPIO
>> expander, because when OMAP GPIO IP is in smart-idle wakeup mode, there
>> is no guarantee that transition(s) on input non wake up GPIO pin will
>> trigger asynchronous wake-up request to PRCM and then IRQ generation.
>> IRQ will be generated when GPIO is in active mode - for example, some
>> time after accessing GPIO bank registers IRQs will be generated
>> normally, but issue will happen again once PRCM will put GPIO in low
>> power smart-idle wakeup mode.
>>
>> Note 1. Issue is not reproduced if debounce clk is enabled for GPIO
>> bank.
>>
>> Note 2. Issue hardly reproducible if GPIO pins group contains both
>> wakeup/non-wakeup gpios - for example, it will be hard to reproduce
>> issue with pin2 if GPIO_IRQWAKEN_0=0x1 GPIO_IRQSTATUS_SET_0=0x3
>> GPIO_FALLINGDETECT = 0x3 (TRM "Power Saving by Grouping the Edge/Level
>> Detection").
>>
>> Note 3. There nothing common bitween System wake up and OMAP GPIO bank
>> IP wake up logic - the last one defines how the GPIO bank ON-IDLE-ON
>> transition will happen inside SoC under control of PRCM.
>>
>> Hence, fix the problem by removing omap_set_gpio_wakeup() function
>> completely and so keeping always in sync GPIO IRQ mask/unmask
>> (IRQSTATUS_SET) and wake up enable (GPIO_IRQWAKEN) bits; 

^^^ only omap_set_gpio_wakeup() is removed, which shouldn't touch
GPIO_IRQWAKEN register (note 3). 
IRQchip .irq_set_irq_wake() in our case should just mark irq parent as System wake up source,
 so it will be kept unmasked during System suspend. GPIO Irq by itself will
 be marked by IRQ Core. 
 All other  GPIO IRQs must be masked during System suspend
  (and it is done gracefully by using IRQCHIP_MASK_ON_SUSPEND:).


>> and adding
>> IRQCHIP_MASK_ON_SUSPEND flag in OMAP GPIO irqchip. That way non wakeup
>> GPIO IRQs will be properly masked/unmask by IRQ PM core during
>> suspend/resume cycle.
>>
>> Cc: Roger Quadros <rogerq@ti.com>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
> GPIO IP has two levels of controls for wakeups and you are
> just removing the SYSCFG wakeup and relying on the IRQ
> line wakeup.

No. I don't remove SYSCFG wakeup. SYSCFG.ENAWAKEUP will be
configured by OMAP hwmod and this patch do not change that.

Also, GPIO_IRQWAKEN will also be configured properly -
It alway configured from omap_set_gpio_trigger().

In addition:
omap_gpio_mask_irq() -> omap_set_gpio_trigger(IRQ_TYPE_NONE) : 
  ---> irq masked, GPIO_IRQWAKEN bit cleared, irq type is IRQ_TYPE_NONE

omap_gpio_unmask_irq() -> omap_set_gpio_trigger(IRQ_TYPE_xxx) : 
  ---> irq unmasked, GPIO_IRQWAKEN bit is set, irq type is IRQ_TYPE_xxx



> line wakeup. I like usage of "IRQCHIP_MASK_ON_SUSPEND" but
> please be acreful this change which might break older OMAPs.

I expect no issues (only if some platforms expect to wake up from
gpio irq, but do not configure this irq as wakeup irq).
^ and this will be a bug which need to be fixed.

> 
>>   drivers/gpio/gpio-omap.c | 42 
>> ++----------------------------------------
>>   1 file changed, 2 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 551dfa9..b98ede7 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -611,51 +611,12 @@ static inline void 
>> omap_set_gpio_irqenable(struct gpio_bank *bank,
>>           omap_disable_gpio_irqbank(bank, BIT(offset));
>>   }
>>
>> -/*
>> - * Note that ENAWAKEUP needs to be enabled in GPIO_SYSCONFIG register.
>> - * 1510 does not seem to have a wake-up register. If JTAG is connected
>> - * to the target, system will wake up always on GPIO events. While
>> - * system is running all registered GPIO interrupts need to have wake-up
>> - * enabled. When system is suspended, only selected GPIO interrupts need
>> - * to have wake-up enabled.
>> - */
>> -static int omap_set_gpio_wakeup(struct gpio_bank *bank, unsigned offset,
>> -                int enable)
>> -{
>> -    u32 gpio_bit = BIT(offset);
>> -    unsigned long flags;
>> -
>> -    if (bank->non_wakeup_gpios & gpio_bit) {
>> -        dev_err(bank->chip.parent,
>> -            "Unable to modify wakeup on non-wakeup GPIO%d\n",
>> -            offset);
>> -        return -EINVAL;
>> -    }
>> -
>> -    raw_spin_lock_irqsave(&bank->lock, flags);
>> -    if (enable)
>> -        bank->context.wake_en |= gpio_bit;
>> -    else
>> -        bank->context.wake_en &= ~gpio_bit;
>> -
>> -    writel_relaxed(bank->context.wake_en, bank->base + 
>> bank->regs->wkup_en);
>> -    raw_spin_unlock_irqrestore(&bank->lock, flags);
>> -
>> -    return 0;
>> -}
>> -
>>   /* Use disable_irq_wake() and enable_irq_wake() functions from 
>> drivers */
>>   static int omap_gpio_wake_enable(struct irq_data *d, unsigned int 
>> enable)
>>   {
>>       struct gpio_bank *bank = omap_irq_data_get_bank(d);
>> -    unsigned offset = d->hwirq;
>> -    int ret;
>>
>> -    ret = omap_set_gpio_wakeup(bank, offset, enable);
>> -    if (!ret)
>> -        ret = irq_set_irq_wake(bank->irq, enable);
>> -
>> -    return ret;
>> +    return irq_set_irq_wake(bank->irq, enable);
>>   }
>>
>>   static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
>> @@ -1187,6 +1148,7 @@ static int omap_gpio_probe(struct 
>> platform_device *pdev)
>>       irqc->irq_bus_lock = omap_gpio_irq_bus_lock,
>>       irqc->irq_bus_sync_unlock = gpio_irq_bus_sync_unlock,
>>       irqc->name = dev_name(&pdev->dev);
>> +    irqc->flags = IRQCHIP_MASK_ON_SUSPEND;
>>
>>       bank->irq = platform_get_irq(pdev, 0);
>>       if (bank->irq <= 0) {
>>


-- 
regards,
-grygorii

Re: [PATCH] gpio: omap: fix irq triggering in smart-idle wakeup mode

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [160412 11:11]:
> On 04/12/2016 07:44 PM, santosh shilimkar wrote:
> I expect no issues (only if some platforms expect to wake up from
> gpio irq, but do not configure this irq as wakeup irq).
> ^ and this will be a bug which need to be fixed.

This seems like a safe assumption to me.

Regards,

Tony

Re: [PATCH] gpio: omap: fix irq triggering in smart-idle wakeup mode

[Linus Walleij]

On Tue, Apr 12, 2016 at 12:52 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle
(...)
> Cc: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Can I get some explicit ACK / Tested-by tags for this patch?

Is it a serious regression that will need to go in as a fix and
tagged for stable?

Yours,
Linus Walleij

Re: [PATCH] gpio: omap: fix irq triggering in smart-idle wakeup mode

[Grygorii Strashko]

On 04/15/2016 11:32 AM, Linus Walleij wrote:
> On Tue, Apr 12, 2016 at 12:52 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
> 
>> Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle
> (...)
>> Cc: Roger Quadros <rogerq@ti.com>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> 
> Can I get some explicit ACK / Tested-by tags for this patch?

Roger's promised to test it once suspend regression will be fixed
for dra7-evm, probably next rc.

> 
> Is it a serious regression that will need to go in as a fix and
> tagged for stable?
> 

This issue is here since 2012, so I think it's not very critical -
It seems bits combination which causing the issue is rare.

Regarding stable:
4.4 - good to have, simple merge conflict
4.1 - some merge resolution is required
older kernel - it will be hard to backport it due to significant
changes in omap gpio driver

Santosh, Tony, do you want me to perform any additional actions regarding this patch?
-- 
regards,
-grygorii

Re: [PATCH] gpio: omap: fix irq triggering in smart-idle wakeup mode

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [160415 02:27]:
> On 04/15/2016 11:32 AM, Linus Walleij wrote:
> > On Tue, Apr 12, 2016 at 12:52 PM, Grygorii Strashko
> > <grygorii.strashko@ti.com> wrote:
> > 
> >> Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle
> > (...)
> >> Cc: Roger Quadros <rogerq@ti.com>
> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> > 
> > Can I get some explicit ACK / Tested-by tags for this patch?
> 
> Roger's promised to test it once suspend regression will be fixed
> for dra7-evm, probably next rc.
> 
> > 
> > Is it a serious regression that will need to go in as a fix and
> > tagged for stable?
> > 
> 
> This issue is here since 2012, so I think it's not very critical -
> It seems bits combination which causing the issue is rare.
> 
> Regarding stable:
> 4.4 - good to have, simple merge conflict
> 4.1 - some merge resolution is required
> older kernel - it will be hard to backport it due to significant
> changes in omap gpio driver
> 
> Santosh, Tony, do you want me to perform any additional actions regarding this patch?

Well maybe update the comments regarding non-interrupt GPIO
lines and wake-up events. Santosh may have other comments.

Regards,

Tony

Re: [PATCH] gpio: omap: fix irq triggering in smart-idle wakeup mode

[santosh shilimkar]

On 4/15/2016 2:26 AM, Grygorii Strashko wrote:
> On 04/15/2016 11:32 AM, Linus Walleij wrote:
>> On Tue, Apr 12, 2016 at 12:52 PM, Grygorii Strashko
>> <grygorii.strashko@ti.com> wrote:
>>
>>> Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle
>> (...)
>>> Cc: Roger Quadros <rogerq@ti.com>
>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>>
>> Can I get some explicit ACK / Tested-by tags for this patch?
>
> Roger's promised to test it once suspend regression will be fixed
> for dra7-evm, probably next rc.
>
>>
>> Is it a serious regression that will need to go in as a fix and
>> tagged for stable?
>>
>
> This issue is here since 2012, so I think it's not very critical -
> It seems bits combination which causing the issue is rare.
>
> Regarding stable:
> 4.4 - good to have, simple merge conflict
> 4.1 - some merge resolution is required
> older kernel - it will be hard to backport it due to significant
> changes in omap gpio driver
>
> Santosh, Tony, do you want me to perform any additional actions regarding this patch?
>
This patch should be run across family of SOCs to make
sure wakeup works on all of those if not done already

Re: [PATCH] gpio: omap: fix irq triggering in smart-idle wakeup mode

[Tony Lindgren]

* santosh shilimkar <santosh.shilimkar@oracle.com> [160415 08:22]:
> On 4/15/2016 2:26 AM, Grygorii Strashko wrote:
> >
> >Santosh, Tony, do you want me to perform any additional actions regarding this patch?
> >
> This patch should be run across family of SOCs to make
> sure wakeup works on all of those if not done already

Also, I'm not sure if we can just drop this code in question.

After this patch, what function updates the GPIO wkup_en registers
depending on enable_irq_wake()/disable_irq_wake()?

Regards,

Tony

Re: [PATCH] gpio: omap: fix irq triggering in smart-idle wakeup mode

[Grygorii Strashko]

On 04/15/2016 09:54 PM, Tony Lindgren wrote:
> * santosh shilimkar <santosh.shilimkar@oracle.com> [160415 08:22]:
>> On 4/15/2016 2:26 AM, Grygorii Strashko wrote:
>>>
>>> Santosh, Tony, do you want me to perform any additional actions regarding this patch?
>>>
>> This patch should be run across family of SOCs to make
>> sure wakeup works on all of those if not done already
> 
> Also, I'm not sure if we can just drop this code in question.
> 
> After this patch, what function updates the GPIO wkup_en registers
> depending on enable_irq_wake()/disable_irq_wake()?
> 

The main purpose of this patch is to *not* modify GPIO wkup_en registers
depending of enable_irq_wake()/disable_irq_wake() :), instead all
non wake up IRQs should be masked during suspend.

The GPIO wkup_en registers should be always in sync with GPIO irq_en when
GPIO IP is in smart-idle wakeup mode. And this is done now from 
omap_gpio_unmask_irq/omap_gpio_mask_irq(). See also [1].

In general, it is more or less similar to GIC + wakeupgen:
- during normal work (including cpuidle) GIC irq_en and Wakeupgen wkup_en
should be in sync always
- during suspend - only IRQs, marked as wake up sources, should be left
unmasked.

Also, I've found old thread [2] where Santosh proposed to use IRQCHIP_MASK_ON_SUSPEND.
And it was not possible, at that time, but now IRQCHIP_MASK_ON_SUSPEND can be used :),
because OMAP GPIO driver was switched to use generic irq handler instead of chained, so
now OMAP GPIO irqs are properly handled by IRQ PM core.  
[chained irqs (and chained irq handles) are not disabled during suspend/resume and they are
 not maintained by IRQ PM core as result they can trigger way too early on resume when
 OMAP GPIO is not ready/powered.]


I've tested it on: am57x-evm, am437x-idk-evm, omap4-panda

[1] https://lkml.org/lkml/2016/4/12/676
[2] https://lkml.org/lkml/2012/8/26/1
    https://groups.google.com/forum/#!msg/linux.kernel/iXJ5Y568B3Q/hZ39bSlcs0kJ

-- 
regards,
-grygorii

Re: [PATCH] gpio: omap: fix irq triggering in smart-idle wakeup mode

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [160418 08:59]:
> On 04/15/2016 09:54 PM, Tony Lindgren wrote:
> > * santosh shilimkar <santosh.shilimkar@oracle.com> [160415 08:22]:
> >> On 4/15/2016 2:26 AM, Grygorii Strashko wrote:
> >>>
> >>> Santosh, Tony, do you want me to perform any additional actions regarding this patch?
> >>>
> >> This patch should be run across family of SOCs to make
> >> sure wakeup works on all of those if not done already
> > 
> > Also, I'm not sure if we can just drop this code in question.
> > 
> > After this patch, what function updates the GPIO wkup_en registers
> > depending on enable_irq_wake()/disable_irq_wake()?
> > 
> 
> The main purpose of this patch is to *not* modify GPIO wkup_en registers
> depending of enable_irq_wake()/disable_irq_wake() :), instead all
> non wake up IRQs should be masked during suspend.

OK that makes sense.

> The GPIO wkup_en registers should be always in sync with GPIO irq_en when
> GPIO IP is in smart-idle wakeup mode. And this is done now from 
> omap_gpio_unmask_irq/omap_gpio_mask_irq(). See also [1].
> 
> In general, it is more or less similar to GIC + wakeupgen:
> - during normal work (including cpuidle) GIC irq_en and Wakeupgen wkup_en
> should be in sync always
> - during suspend - only IRQs, marked as wake up sources, should be left
> unmasked.
> 
> Also, I've found old thread [2] where Santosh proposed to use IRQCHIP_MASK_ON_SUSPEND.
> And it was not possible, at that time, but now IRQCHIP_MASK_ON_SUSPEND can be used :),
> because OMAP GPIO driver was switched to use generic irq handler instead of chained, so
> now OMAP GPIO irqs are properly handled by IRQ PM core.  
> [chained irqs (and chained irq handles) are not disabled during suspend/resume and they are
>  not maintained by IRQ PM core as result they can trigger way too early on resume when
>  OMAP GPIO is not ready/powered.]

OK. For my tests this patch does not change anything. I noticed however
that we still have some additional bug somewhere where GPIO wake up
events work fine for omap3 PM runtime, but are flakey for suspend.

> I've tested it on: am57x-evm, am437x-idk-evm, omap4-panda

OK thanks! Based on my tests and the above:

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

Regards,

Tony

> [1] https://lkml.org/lkml/2016/4/12/676
> [2] https://lkml.org/lkml/2012/8/26/1
>     https://groups.google.com/forum/#!msg/linux.kernel/iXJ5Y568B3Q/hZ39bSlcs0kJ

Re: [PATCH] gpio: omap: fix irq triggering in smart-idle wakeup mode

[santosh shilimkar]

On 4/18/2016 4:36 PM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [160418 08:59]:
>> On 04/15/2016 09:54 PM, Tony Lindgren wrote:
>>> * santosh shilimkar <santosh.shilimkar@oracle.com> [160415 08:22]:
>>>> On 4/15/2016 2:26 AM, Grygorii Strashko wrote:
>>>>>
>>>>> Santosh, Tony, do you want me to perform any additional actions regarding this patch?
>>>>>
>>>> This patch should be run across family of SOCs to make
>>>> sure wakeup works on all of those if not done already
>>>
>>> Also, I'm not sure if we can just drop this code in question.
>>>
>>> After this patch, what function updates the GPIO wkup_en registers
>>> depending on enable_irq_wake()/disable_irq_wake()?
>>>
>>
>> The main purpose of this patch is to *not* modify GPIO wkup_en registers
>> depending of enable_irq_wake()/disable_irq_wake() :), instead all
>> non wake up IRQs should be masked during suspend.
>
> OK that makes sense.
>
>> The GPIO wkup_en registers should be always in sync with GPIO irq_en when
>> GPIO IP is in smart-idle wakeup mode. And this is done now from
>> omap_gpio_unmask_irq/omap_gpio_mask_irq(). See also [1].
>>
>> In general, it is more or less similar to GIC + wakeupgen:
>> - during normal work (including cpuidle) GIC irq_en and Wakeupgen wkup_en
>> should be in sync always
>> - during suspend - only IRQs, marked as wake up sources, should be left
>> unmasked.
>>
>> Also, I've found old thread [2] where Santosh proposed to use IRQCHIP_MASK_ON_SUSPEND.
>> And it was not possible, at that time, but now IRQCHIP_MASK_ON_SUSPEND can be used :),
>> because OMAP GPIO driver was switched to use generic irq handler instead of chained, so
>> now OMAP GPIO irqs are properly handled by IRQ PM core.
>> [chained irqs (and chained irq handles) are not disabled during suspend/resume and they are
>>   not maintained by IRQ PM core as result they can trigger way too early on resume when
>>   OMAP GPIO is not ready/powered.]
>
> OK. For my tests this patch does not change anything. I noticed however
> that we still have some additional bug somewhere where GPIO wake up
> events work fine for omap3 PM runtime, but are flakey for suspend.
>
>> I've tested it on: am57x-evm, am437x-idk-evm, omap4-panda
>
> OK thanks! Based on my tests and the above:
>
> Acked-by: Tony Lindgren <tony@atomide.com>
>
If all works then consider my ack as well :-)

Re: [PATCH] gpio: omap: fix irq triggering in smart-idle wakeup mode

[Linus Walleij]

On Tue, Apr 12, 2016 at 12:52 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle
> in the following case:
>   extcon_usb1(id_irq) ->  pcf8575.gpio1 -> omapgpio6.gpio11 -> gic
>
> the extcon_usb1 is wake up source and it enables IRQ wake up for
> id_irq by calling enable/disable_irq_wake() during suspend/resume
> which, in turn, causes execution of omap_gpio_wake_enable(). And
> omap_gpio_wake_enable() will set/clear corresponding bit in
> GPIO_IRQWAKEN_x register.
>
> omapgpio6 configuration after boot - wakeup is enabled for GPIO IRQs
> by default from  omap_gpio_irq_type:
> GPIO_IRQSTATUS_SET_0    | 0x00000400
> GPIO_IRQSTATUS_CLR_0    | 0x00000400
> GPIO_IRQWAKEN_0         | 0x00000400
> GPIO_RISINGDETECT       | 0x00000000
> GPIO_FALLINGDETECT      | 0x00000400
>
> omapgpio6 configuration after after suspend/resume cycle:
> GPIO_IRQSTATUS_SET_0    | 0x00000400
> GPIO_IRQSTATUS_CLR_0    | 0x00000400
> GPIO_IRQWAKEN_0         | 0x00000000 <---
> GPIO_RISINGDETECT       | 0x00000000
> GPIO_FALLINGDETECT      | 0x00000400
>
> As result, system will start to lose interrupts from pcf8575 GPIO
> expander, because when OMAP GPIO IP is in smart-idle wakeup mode, there
> is no guarantee that transition(s) on input non wake up GPIO pin will
> trigger asynchronous wake-up request to PRCM and then IRQ generation.
> IRQ will be generated when GPIO is in active mode - for example, some
> time after accessing GPIO bank registers IRQs will be generated
> normally, but issue will happen again once PRCM will put GPIO in low
> power smart-idle wakeup mode.
>
> Note 1. Issue is not reproduced if debounce clk is enabled for GPIO
> bank.
>
> Note 2. Issue hardly reproducible if GPIO pins group contains both
> wakeup/non-wakeup gpios - for example, it will be hard to reproduce
> issue with pin2 if GPIO_IRQWAKEN_0=0x1 GPIO_IRQSTATUS_SET_0=0x3
> GPIO_FALLINGDETECT = 0x3 (TRM "Power Saving by Grouping the Edge/Level
> Detection").
>
> Note 3. There nothing common bitween System wake up and OMAP GPIO bank
> IP wake up logic - the last one defines how the GPIO bank ON-IDLE-ON
> transition will happen inside SoC under control of PRCM.
>
> Hence, fix the problem by removing omap_set_gpio_wakeup() function
> completely and so keeping always in sync GPIO IRQ mask/unmask
> (IRQSTATUS_SET) and wake up enable (GPIO_IRQWAKEN) bits; and adding
> IRQCHIP_MASK_ON_SUSPEND flag in OMAP GPIO irqchip. That way non wakeup
> GPIO IRQs will be properly masked/unmask by IRQ PM core during
> suspend/resume cycle.
>
> Cc: Roger Quadros <rogerq@ti.com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Patch applied with the arrived ACKs

Yours,
Linus Walleij