Re: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

[Sudeep Holla <sudeep.holla@arm.com>]

On 25/08/15 15:54, Shenwei Wang wrote:
>
>
>> -----Original Message-----
>> From: Sudeep Holla [mailto:sudeep.holla@arm.com]
>> Sent: 2015年8月25日 9:46
>> To: Wang Shenwei-B38339
>> Cc: Sudeep Holla; shawn.guo@linaro.org; tglx@linutronix.de;
>> jason@lakedaemon.net; Huang Yongcai-B20788; linux-kernel@vger.kernel.org;
>> linux-arm-kernel@lists.infradead.org
>> Subject: Re: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup
>> sources
>>
>>
>>
>> On 25/08/15 15:14, Shenwei Wang wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Sudeep Holla [mailto:sudeep.holla@arm.com]
>>
>> [...]
>>
>>>> I don't see this driver doing anything extra apart from keeping
>>>> the wakeup irqs enabled. i.e. You use the same cpu*wake
>>>> register to mask/unmask the interrupt as well as set the wakeup
>>>> source. Since the wakeup interrupt will be enabled by the
>>>> driver, you just need to mark it as wake-up source and nothing
>>>> extra in the controller right ? If so, you need to set
>>>> IRQCHIP_SKIP_SET_WAKE as you are just leaving that irq enabled
>>>> and not doing any extra configuration to enable it  as wakeup source.
>>>> Please correct if that wrong, but from the code that's what I
>>>> couldinfer.
>>>
>>> There is no special for this driver. We just use the IRQCHIP
>>> driver framework to manage the wakeup sources. Why did you
>>> propose to set IRQCHIP_SKIP_SET_WAKE flag here? If you don't need
>>> the wakeup feature, you should just not enable this driver in the
>>> configuration.
>>>
>>
>> No, if the driver doesn't nothing extra to configure the wake up
>> source other than  keeping it enabled, then it fits the case of
>> SKIP_SET_WAKE. The driver using this wake would have requested
>> and enabled the irq. When it calls enable_irq_wake, you have
>> nothing extra to set(atleast  from the looks of the driver), so
>> setting SKIP_SET_WAKE will skip the call and  updated the wake
>> flags in irq core.
>>
>> I don't see the real need of 2 separate sets of irq mask being
>> saved  in either case.
>
> You don't really understand what happens after a driver calls
> enable_irq_wake. In suspend state, even the interrupt
> controller itself is powered off. How can you get the system up
> again  by just using a SKIP_SET_WAKE.
>

Sorry for that, let me try to understand aloud. So you have
irq_{un,}mask function that are called when interrupts are enabled and
disabled. So suppose you have 3 irqs that are enabled and only one of
then is set as wakeup source.

Now you call enable_irq_wake, you save that in wakeup_sources, fine.
Later when you enter suspend, you save all the 3 active irqs in
saved_irq_mask and over-write cpu2wakeup with wakeup_sources, right?

All fine, what I am saying is let irq-core know that you want to mask
the 2 non-wakeup irqs you have using MASK_ON_SUSPEND. So when
suspend_device_irqs is called in suspend path, that's done for you
automatically and the cpu2wakeup will have just 1 wakeup enabled which
is what you are doing in suspend callback, right ?

Now that it's already done for you, you need not do anything extra and
hence just set SKIP_SET_WAKE to do nothing.

Hope this clarifies, sorry if I am still missing to understand something
here, but I don't see anything. Let me know.

Regards,
Sudeep