Re: [PATCH] rtc: rtc-twl: ensure IRQ is wakeup enabled

[Grygorii Strashko <grygorii.strashko@ti.com>]

On 06/04/2013 08:46 PM, Kevin Hilman wrote:
> Grygorii Strashko <grygorii.strashko@ti.com> writes:
>
>> Hi Kevin,
>>
>> On 06/01/2013 01:37 AM, Kevin Hilman wrote:
>>> Currently, the RTC IRQ is never wakeup-enabled so is not capable of
>>> bringing the system out of suspend.
>>>
>>> On OMAP platforms, we have gotten by without this because the TWL RTC
>>> is on an I2C-connected chip which is capable of waking up the OMAP via
>>> the IO ring when the OMAP is in low-power states.
>>>
>>> However, if the OMAP suspends without hitting the low-power states
>>> (and the IO ring is not enabled), RTC wakeups will not work because
>>> the IRQ is not wakeup enabled.
>> As I understand, IRQ wake up capabilities are set/clear simultaneously with
>> IRQ unmasking/masking on OMAP4+ in omap-wakeupgen.c.
>> So, it should work without this patch on OMAP4+.
> It might work on OMAP4 for wakeup from suspend, but without properly
> declaring the IRQ as a wakeup source, it will not abort suspend if the
> RTC fires during the suspend process.  To abort suspend, the IRQ must be
> declared as a wakeup IRQ.
>
>> But if TWL is used on non OMAP4+ platform then it is needed.  (OMAP3:
>> I haven't found the place where IRQ wakeup capabilities are
>> configured, would be appreciate if you can point me on)
> IRQ wakeup is a genirq feature that trickles into the irq_chip (in OMAP3
> case, it's the twl4030 irq_chip.)
>
> On OMAP3, as mentioned in the changelog, RTC wake has been working fine
> without this because we default to CORE retention, so wakeup happens via
> the IO ring.  However, if you prevent retention during suspend, then
> this IRQ will not wake the system.
>
> Kevin
>
>>> To fix, ensure the RTC IRQ is wakeup enabled whenever the RTC alarm is
>>> set.
>>>
>>> Cc: Alessandro Zummo <a.zummo@towertech.it>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Tony Lindgren <tony@atomide.com>
>>> Signed-off-by: Kevin Hilman <khilman@linaro.org>
>>> ---
>>>    drivers/rtc/rtc-twl.c | 16 ++++++++++++++--
>>>    1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/rtc/rtc-twl.c b/drivers/rtc/rtc-twl.c
>>> index 8751a52..bbda0fd 100644
>>> --- a/drivers/rtc/rtc-twl.c
>>> +++ b/drivers/rtc/rtc-twl.c
>>> @@ -213,12 +213,24 @@ static int mask_rtc_irq_bit(unsigned char bit)
>>>      static int twl_rtc_alarm_irq_enable(struct device *dev, unsigned
>>> enabled)
>>>    {
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>> +	int irq = platform_get_irq(pdev, 0);
>>> +	static bool twl_rtc_wake_enabled;
>>>    	int ret;
>>>    -	if (enabled)
>>> +	if (enabled) {
>>>    		ret = set_rtc_irq_bit(BIT_RTC_INTERRUPTS_REG_IT_ALARM_M);
>>> -	else
>>> +		if (device_can_wakeup(dev) && !twl_rtc_wake_enabled) {
>>> +			enable_irq_wake(irq);
>>> +			twl_rtc_wake_enabled = true;
>>> +		}
>>> +	} else {
>>>    		ret = mask_rtc_irq_bit(BIT_RTC_INTERRUPTS_REG_IT_ALARM_M);
>>> +		if (twl_rtc_wake_enabled) {
>>> +			disable_irq_wake(irq);
>>> +			twl_rtc_wake_enabled = false;
>>> +		}
>>> +	}
>>>      	return ret;
>>>    }
>> twl-rtc has suspend/resume callbacks implemented, so I think it's the
>> better place
>> for this code and twl_rtc_wake_enabled can be dropped.
> In theory, that might be the better place (and that's where I put these
> at first), but unfortunately, it doesn't work that way because the
> twl6030-irq core enables/diables the parent IRQ wake feature using PM
> notifiers (which was done to avoid potential lock recursion[1].)
>
> During suspend, the notifier runs at suspend_prepare() time, which is
> well before the driver's ->suspend() method is called.  The result is
> that the parents IRQ wakeup capabilies are never set.
Sorry, forget about this patch - have no questions for this patch anymore.

Thanks.

Just FYI. It seems, The suspend will never be aborted on OMAP4 by SYSN_IRQ
because of these two patches:
782baa2 mfd: Disable twl6030 IRQ during suspend
9c6079a genirq: Do not consider disabled wakeup irqs

-grygorii
>
> Kevin
>
>
> [1]
> commit ab2b9260df67e29d5bd69d989f2f84f8c2ed4238
> Author: Todd Poynor <toddpoynor@google.com>
> Date:   Tue Oct 4 11:52:29 2011 +0200
>
>      mfd: Fix twl6030 lockdep recursion warning on setting wake IRQs
>      
>      LOCKDEP explicitly sets all irq_desc locks as a single lock-class,
>      causing "possible recursive locking detected" when the TWL RTC
>      driver calls through enable_irq_wake to twl6030_irq_set_wake,
>      which recursively calls irq_set_irq_wake.  Although the
>      irq_desc and lock are different, LOCKDEP treats these as
>      equivalent, presumably due to problems that can be incurred
>      when locking more than one irq_desc, so best to avoid this.
>      
>      Suspend/resume actions implemented as PM notifiers to avoid
>      touch the TWL core for this.
>      
>      Signed-off-by: Todd Poynor <toddpoynor@google.com>
>      Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>      Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>