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

[Kevin Hilman]

Grygorii Strashko <grygorii.strashko@xxxxxx> writes:

> On 06/04/2013 08:46 PM, Kevin Hilman wrote:
>> Grygorii Strashko <grygorii.strashko@xxxxxx> 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@xxxxxxxxxxxx>
>>>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>>>> Cc: Tony Lindgren <tony@xxxxxxxxxxx>
>>>> Signed-off-by: Kevin Hilman <khilman@xxxxxxxxxx>
>>>> ---
>>>>    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

You're right for the parent TWL IRQ, but the child interrupts (e.g. RTC)
would still abort suspend if wakeup enabled.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/