Re: [PATCH v2 0/3] renesas: irqchip: Use WAKEUP_PATH driver PM flag

[Geert Uytterhoeven]

Hi Rafael,

On Sun, Dec 31, 2017 at 1:56 AM, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>> From: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>>
>> Changes in v2: [By Ulf Hansson]
>>         - I have picked up the series from Geert [1] and converted it into use
>>         the WAKEUP_PATH driver PM flag. This includes some minor changes to each
>>         patch and updates to the changelogs.
>>         - An important note, the WAKEUP_PATH driver PM flag is introduced in a
>>         separate series [2], not yet applied, so @subject series depends on it.
>>         - One more note, two of the patches has a checkpatch error, however I
>>         did not fix them, becuase I think that should be done separate.
>>
>> [1]
>> https://lkml.org/lkml/2017/11/9/382
>> [2]
>> https://marc.info/?l=linux-pm&m=151454744124661&w=2
>>
>> More information below, picked from Geert's previous cover letter.
>>
>> Kind regards
>> Uffe
>>
>>
>> Hi all,
>>
>> If an interrupt controller in a Renesas ARM SoC is part of a Clock
>> Domain, and it is part of the wakeup path, it must be kept active during
>> system suspend.
>>
>> Currently this is handled in all interrupt controller drivers by
>> explicitly increasing the use count of the module clock when the device
>> is part of the wakeup path.  However, this explicit clock handling is
>> merely a workaround for a failure to properly communicate wakeup
>> information to the device core.
>>
>> Hence this series fixes the affected drivers by setting the devices'
>> power.wakeup_path fields instead, to indicate they are part of the
>> wakeup path.  Depending on the PM Domain's active_wakeup configuration,
>> the genpd core code will keep the device enabled (and the clock running)
>> during system suspend when needed.
>
> However, there is a convention, documented in the kerneldoc comment of
> device_init_wakeup(), by which devices participating in system wakeup
> "passively" (like USB controllers and hubs) are expected to have it
> enabled by default.
>
> If that convention was followed by the devices in question here, the
> wakeup_path bit would be set for them and no other code changes would
> be necessary.  So is there any reason for not following it?

Yes there is.  The need to stay enabled during system suspend depends
on the consumer of the interrupt. It is controlled by the consumer using
the irq_chip.irq_set_wake() callback at runtime, and may change at runtime.

If the wakeup_path flag is always set, the interrupt controller will
never be suspended during system suspend, and thus waste power.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds