[PATCH v2 0/3] PM / Domain: renesas: Fix active wakeup behavior

[PATCH v2 0/3] PM / Domain: renesas: Fix active wakeup behavior

[Geert Uytterhoeven]

	Hi Rafael, Ulf, Kevin,

If a device in a Renesas ARM SoC is part of a Clock Domain, and it is
used as a wakeup source, it must be kept active during system suspend.

Currently this is handled in device-specific drivers by explicitly
increasing the use count of the module clock when the device is
configured as a wakeup source, or if it is part of the wakeup path.

However, this is merely a workaround.  The proper way to prevent the
device from being stopped is to inform this requirement to the genpd
core, using the new GENPD_FLAG_ACTIVE_WAKEUP flag introduced in commit
95a20ef6f7e54c6a ("PM / Domains: Allow genpd users to specify default
active wakeup behavior").

Hence this series does that for PM Domain drivers used on R-Car, RZ/A1,
RZ/G1 SoCs, mimicking what is already done succesfully on SH/R-Mobile
SoCs.  This will allow for the workarounds can be removed later.

This series was extracted from "[PATCH 00/10] PM / Domain: renesas: Fix
active wakeup behavior", and retains only fixes for Renesas PM Domain
drivers.

Changes compared to v1:
  - Integrate follow-up patches to use GENPD_FLAG_ACTIVE_WAKEUP instead
    of adding an "always true" callback.

As GENPD_FLAG_ACTIVE_WAKEUP exists in pm/linux-next only, and this
series is a dependency for the removal of workarounds in drivers of
multiple subsystems (net, irqchip, and gpio), I think it is a good idea
to still queue this for v4.15 in the PM tree, if possible.

This has been tested on r8a73a4/ape6evm, r8a7740/armadillo,
r8a7791/koelsch, r8a7795/salvator-x and -xs, r8a7795/salvator-x, and
sh73a0/kzm9g.

Thanks for applying!

Geert Uytterhoeven (3):
  clk: renesas: mstp: Keep wakeup sources active during system suspend
  clk: renesas: cpg-mssr: Keep wakeup sources active during system
    suspend
  soc: renesas: rcar-sysc: Keep wakeup sources active during system
    suspend

 drivers/clk/renesas/clk-mstp.c         | 2 +-
 drivers/clk/renesas/renesas-cpg-mssr.c | 2 +-
 drivers/soc/renesas/rcar-sysc.c        | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.7.4

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

[PATCH v2 1/3] clk: renesas: mstp: Keep wakeup sources active during system suspend

[Geert Uytterhoeven]

If a device is part of the CPG/MSTP Clock Domain and to be used as a
wakeup source, it must be kept active during system suspend.

Currently this is handled in device-specific drivers by explicitly
increasing the use count of the module clock when the device is
configured as a wakeup source.  However, the proper way to prevent the
device from being stopped is to inform this requirement to the genpd
core, by setting the GENPD_FLAG_ACTIVE_WAKEUP flag.

Note that this will only affect devices configured as wakeup sources.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Integrate "clk: renesas: mstp: Use GENPD_FLAG_ACTIVE_WAKEUP",
---
 drivers/clk/renesas/clk-mstp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
index 500a9e4e03c48957..0b222197a4576fd7 100644
--- a/drivers/clk/renesas/clk-mstp.c
+++ b/drivers/clk/renesas/clk-mstp.c
@@ -344,7 +344,7 @@ void __init cpg_mstp_add_clk_domain(struct device_node *np)
 		return;
 
 	pd->name = np->name;
-	pd->flags = GENPD_FLAG_PM_CLK;
+	pd->flags = GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP;
 	pd->attach_dev = cpg_mstp_attach_dev;
 	pd->detach_dev = cpg_mstp_detach_dev;
 	pm_genpd_init(pd, &pm_domain_always_on_gov, false);
-- 
2.7.4

[PATCH v2 2/3] clk: renesas: cpg-mssr: Keep wakeup sources active during system suspend

[Geert Uytterhoeven]

If a device is part of the CPG/MSSR Clock Domain and to be used as a
wakeup source, it must be kept active during system suspend.

Currently this is handled in device-specific drivers by explicitly
increasing the use count of the module clock when the device is
configured as a wakeup source.  However, the proper way to prevent the
device from being stopped is to inform this requirement to the genpd
core, by setting the GENPD_FLAG_ACTIVE_WAKEUP flag.

Note that this will only affect devices configured as wakeup sources.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Integrate "clk: renesas: cpg-mssr: Use GENPD_FLAG_ACTIVE_WAKEUP",
---
 drivers/clk/renesas/renesas-cpg-mssr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
index e580a5e6346c2533..99699b715d7cf2de 100644
--- a/drivers/clk/renesas/renesas-cpg-mssr.c
+++ b/drivers/clk/renesas/renesas-cpg-mssr.c
@@ -500,7 +500,7 @@ static int __init cpg_mssr_add_clk_domain(struct device *dev,
 
 	genpd = &pd->genpd;
 	genpd->name = np->name;
-	genpd->flags = GENPD_FLAG_PM_CLK;
+	genpd->flags = GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP;
 	genpd->attach_dev = cpg_mssr_attach_dev;
 	genpd->detach_dev = cpg_mssr_detach_dev;
 	pm_genpd_init(genpd, &pm_domain_always_on_gov, false);
-- 
2.7.4

[PATCH v2 3/3] soc: renesas: rcar-sysc: Keep wakeup sources active during system suspend

[Geert Uytterhoeven]

If an R-Car SYSC slave device is part of the CPG/MSTP or CPG/MSSR Clock
Domain and to be used as a wakeup source, it must be kept active during
system suspend.

Currently this is handled in device-specific drivers by explicitly
increasing the use count of the module clock when the device is
configured as a wakeup source.  However, the proper way to prevent the
device from being stopped is to inform this requirement to the genpd
core, by setting the GENPD_FLAG_ACTIVE_WAKEUP flag.

Note that this will only affect devices configured as wakeup sources.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Integrate "soc: renesas: rcar-sysc: Use GENPD_FLAG_ACTIVE_WAKEUP",
---
 drivers/soc/renesas/rcar-sysc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/renesas/rcar-sysc.c b/drivers/soc/renesas/rcar-sysc.c
index c8406e81640f6560..ac4df1c43b2fbdbe 100644
--- a/drivers/soc/renesas/rcar-sysc.c
+++ b/drivers/soc/renesas/rcar-sysc.c
@@ -224,7 +224,7 @@ static void __init rcar_sysc_pd_setup(struct rcar_sysc_pd *pd)
 
 	if (!(pd->flags & (PD_CPU | PD_SCU))) {
 		/* Enable Clock Domain for I/O devices */
-		genpd->flags |= GENPD_FLAG_PM_CLK;
+		genpd->flags |= GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP;
 		if (has_cpg_mstp) {
 			genpd->attach_dev = cpg_mstp_attach_dev;
 			genpd->detach_dev = cpg_mstp_detach_dev;
-- 
2.7.4

Re: [PATCH v2 0/3] PM / Domain: renesas: Fix active wakeup behavior

[Ulf Hansson]

On 9 November 2017 at 14:26, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>         Hi Rafael, Ulf, Kevin,
>
> If a device in a Renesas ARM SoC is part of a Clock Domain, and it is
> used as a wakeup source, it must be kept active during system suspend.

Geert, I think we discussed this a bit already. I wonder if the above
is a correct statement for all devices in these PM domains, that has
wakeups?

Don't these SoCs make use of any external logic (out-of-band IRQ) to
deal with the wakeup IRQs?

For example, how is GPIO irqs dealt with in this regards?

If that is the case, you should really avoid using the big hammer
method of setting the GENPD_FLAG_ACTIVE_WAKEUP.

>
> Currently this is handled in device-specific drivers by explicitly
> increasing the use count of the module clock when the device is
> configured as a wakeup source, or if it is part of the wakeup path.
>
> However, this is merely a workaround.  The proper way to prevent the
> device from being stopped is to inform this requirement to the genpd
> core, using the new GENPD_FLAG_ACTIVE_WAKEUP flag introduced in commit
> 95a20ef6f7e54c6a ("PM / Domains: Allow genpd users to specify default
> active wakeup behavior").
>
> Hence this series does that for PM Domain drivers used on R-Car, RZ/A1,
> RZ/G1 SoCs, mimicking what is already done succesfully on SH/R-Mobile
> SoCs.  This will allow for the workarounds can be removed later.
>

[...]

Kind regards
Uffe

Re: [PATCH v2 0/3] PM / Domain: renesas: Fix active wakeup behavior

[Geert Uytterhoeven]

Hi Ulf,

On Fri, Nov 10, 2017 at 10:57 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 9 November 2017 at 14:26, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>> If a device in a Renesas ARM SoC is part of a Clock Domain, and it is
>> used as a wakeup source, it must be kept active during system suspend.
>
> Geert, I think we discussed this a bit already. I wonder if the above
> is a correct statement for all devices in these PM domains, that has
> wakeups?

It is true for all wakeup sources (e.g. Ethernet and serial).

> Don't these SoCs make use of any external logic (out-of-band IRQ) to
> deal with the wakeup IRQs?
>
> For example, how is GPIO irqs dealt with in this regards?

Interrupt controllers (incl. GPIO) may, depending on SoC type:
  - be located in a controllable power area (SH/R-Mobile),
  - may run from a controllable module clock (SH/R-Mobile, R-Car Gen2/Gen3,
    RZ/A1, RZ/G1).

So there are no out-of-band IRQs in the wakeup path, unless on SoCs where
the interrupt controllers are in an always-on power area, AND run from a
fixed clock. I think only R-Car Gen1 falls in that category.
Still, R-Car Gen1 needs active_wakeup for wakeup sources.

See series "[PATCH/RFC 0/3] renesas: irqchip: Use wakeup_path i.s.o.
explicit clock handling", which includes GPIO interrupts.

> If that is the case, you should really avoid using the big hammer
> method of setting the GENPD_FLAG_ACTIVE_WAKEUP.

So I think I do need the big hammer ;-)

>> Currently this is handled in device-specific drivers by explicitly
>> increasing the use count of the module clock when the device is
>> configured as a wakeup source, or if it is part of the wakeup path.
>>
>> However, this is merely a workaround.  The proper way to prevent the
>> device from being stopped is to inform this requirement to the genpd
>> core, using the new GENPD_FLAG_ACTIVE_WAKEUP flag introduced in commit
>> 95a20ef6f7e54c6a ("PM / Domains: Allow genpd users to specify default
>> active wakeup behavior").
>>
>> Hence this series does that for PM Domain drivers used on R-Car, RZ/A1,
>> RZ/G1 SoCs, mimicking what is already done succesfully on SH/R-Mobile
>> SoCs.  This will allow for the workarounds can be removed later.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 0/3] PM / Domain: renesas: Fix active wakeup behavior

[Ulf Hansson]

On 10 November 2017 at 11:22, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Fri, Nov 10, 2017 at 10:57 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 9 November 2017 at 14:26, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>>> If a device in a Renesas ARM SoC is part of a Clock Domain, and it is
>>> used as a wakeup source, it must be kept active during system suspend.
>>
>> Geert, I think we discussed this a bit already. I wonder if the above
>> is a correct statement for all devices in these PM domains, that has
>> wakeups?
>
> It is true for all wakeup sources (e.g. Ethernet and serial).
>
>> Don't these SoCs make use of any external logic (out-of-band IRQ) to
>> deal with the wakeup IRQs?
>>
>> For example, how is GPIO irqs dealt with in this regards?
>
> Interrupt controllers (incl. GPIO) may, depending on SoC type:
>   - be located in a controllable power area (SH/R-Mobile),
>   - may run from a controllable module clock (SH/R-Mobile, R-Car Gen2/Gen3,
>     RZ/A1, RZ/G1).
>
> So there are no out-of-band IRQs in the wakeup path, unless on SoCs where
> the interrupt controllers are in an always-on power area, AND run from a
> fixed clock. I think only R-Car Gen1 falls in that category.
> Still, R-Car Gen1 needs active_wakeup for wakeup sources.

Perhaps out-of-band IRQ is a too vague term. :-) Anyway, let me
elaborate a bit more.

Apologize for being so persistent, but I really want to get to bottom with this.

According to the statement above, it seems like IRQs (including GPIOs)
is controllable from a separate "power area" (or module clock). In
many ARM SoCs that "power area" is a separate piece of external logic,
sometimes it may even consist a small co-processor. I guess you
already know that, but wanted to point it out for clarity.

For that reason, some serial drivers re-routs its serial rx pin to a
GPIO IRQ to deal with wakeup, instead of keeping the serial device
always powered. This enables the GPIO IRQ to be managed by the
external logic, thus allowing the serial device and the PM domain it
is attached to, to be powered off. Especially during system suspend,
that may avoid wasting lots of power.

Another example, where I think a more fine grained method is preferred
over using GENPD_FLAG_ACTIVE_WAKEUP, is when an SD card controller has
an card detect pin hooked up to a GPIO. Thus the device_may_wakeup()
would return true for the SD card controller's struct device, but that
does not mean that the device needs to stay powered during system
suspend.

>
> See series "[PATCH/RFC 0/3] renesas: irqchip: Use wakeup_path i.s.o.
> explicit clock handling", which includes GPIO interrupts.
>
>> If that is the case, you should really avoid using the big hammer
>> method of setting the GENPD_FLAG_ACTIVE_WAKEUP.
>
> So I think I do need the big hammer ;-)

>From a hypothetical point of view, if you were to use the more fine
grained method I proposed [1] (or something very similar), how many
drivers would you need to change, to be able to remove the current
workaround?

[...]

Kind regards
Uffe

[1]
https://www.spinics.net/lists/linux-renesas-soc/msg19911.html

Re: [PATCH v2 0/3] PM / Domain: renesas: Fix active wakeup behavior

[Geert Uytterhoeven]

Hi Ulf,

On Fri, Nov 10, 2017 at 1:49 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 10 November 2017 at 11:22, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Fri, Nov 10, 2017 at 10:57 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 9 November 2017 at 14:26, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>>>> If a device in a Renesas ARM SoC is part of a Clock Domain, and it is
>>>> used as a wakeup source, it must be kept active during system suspend.
>>>
>>> Geert, I think we discussed this a bit already. I wonder if the above
>>> is a correct statement for all devices in these PM domains, that has
>>> wakeups?
>>
>> It is true for all wakeup sources (e.g. Ethernet and serial).
>>
>>> Don't these SoCs make use of any external logic (out-of-band IRQ) to
>>> deal with the wakeup IRQs?
>>>
>>> For example, how is GPIO irqs dealt with in this regards?
>>
>> Interrupt controllers (incl. GPIO) may, depending on SoC type:
>>   - be located in a controllable power area (SH/R-Mobile),
>>   - may run from a controllable module clock (SH/R-Mobile, R-Car Gen2/Gen3,
>>     RZ/A1, RZ/G1).
>>
>> So there are no out-of-band IRQs in the wakeup path, unless on SoCs where
>> the interrupt controllers are in an always-on power area, AND run from a
>> fixed clock. I think only R-Car Gen1 falls in that category.
>> Still, R-Car Gen1 needs active_wakeup for wakeup sources.
>
> Perhaps out-of-band IRQ is a too vague term. :-) Anyway, let me
> elaborate a bit more.
>
> Apologize for being so persistent, but I really want to get to bottom with this.
>
> According to the statement above, it seems like IRQs (including GPIOs)
> is controllable from a separate "power area" (or module clock). In

There are two ways to save power: disabling the module clock, and powering
down the module logic.

> many ARM SoCs that "power area" is a separate piece of external logic,
> sometimes it may even consist a small co-processor. I guess you
> already know that, but wanted to point it out for clarity.

Sure.

I think on some older SH/R-Mobile SoCs you can do something similar,
but even there you have to keep the interrupt controller's power area powered.
This makes sense, given those SoCs also have an SH core, which could be used
as the small co-processor that powers down the whole ARM subsystem and
handles wakeup through the SH subsystem's interrupt controller.

> For that reason, some serial drivers re-routs its serial rx pin to a
> GPIO IRQ to deal with wakeup, instead of keeping the serial device
> always powered. This enables the GPIO IRQ to be managed by the
> external logic, thus allowing the serial device and the PM domain it
> is attached to, to be powered off. Especially during system suspend,
> that may avoid wasting lots of power.

Doesn't re-routing a pin to a GPIO require using pinctrl?

Here the serial device is the wakeup source, but it doesn't handle wakeup
itself...

> Another example, where I think a more fine grained method is preferred
> over using GENPD_FLAG_ACTIVE_WAKEUP, is when an SD card controller has
> an card detect pin hooked up to a GPIO. Thus the device_may_wakeup()
> would return true for the SD card controller's struct device, but that
> does not mean that the device needs to stay powered during system
> suspend.

This one is more tricky, as I think we're already using a GPIO for CD on
many boards.
Then the SD device is the wakeup source, but it doesn't handle wakeup
itself...

So you not only need a flag to opt-in, but also a flag to opt-out?

>> See series "[PATCH/RFC 0/3] renesas: irqchip: Use wakeup_path i.s.o.
>> explicit clock handling", which includes GPIO interrupts.
>>
>>> If that is the case, you should really avoid using the big hammer
>>> method of setting the GENPD_FLAG_ACTIVE_WAKEUP.
>>
>> So I think I do need the big hammer ;-)
>
> From a hypothetical point of view, if you were to use the more fine
> grained method I proposed [1] (or something very similar), how many
> drivers would you need to change, to be able to remove the current
> workaround?

I think five:

  - drivers/gpio/gpio-rcar.c
  - drivers/irqchip/irq-renesas-intc-irqpin.c
  - drivers/irqchip/irq-renesas-irqc.c
  - drivers/net/ethernet/renesas/ravb_main.c
  - drivers/net/ethernet/renesas/sh_eth.c

Note that while setting the flag wouldn't harm, it would not be necessary for
gpio-rcar and renesas-intc-irqpin when running on R-Car Gen1.  Not setting it
means using platform knowledge in a device driver, which is what genpd was
supposed to solve in the first place, and thus IMHO a layering violation.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 0/3] PM / Domain: renesas: Fix active wakeup behavior

[Ulf Hansson]

On 10 November 2017 at 14:22, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Fri, Nov 10, 2017 at 1:49 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 10 November 2017 at 11:22, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Fri, Nov 10, 2017 at 10:57 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> On 9 November 2017 at 14:26, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>>>>> If a device in a Renesas ARM SoC is part of a Clock Domain, and it is
>>>>> used as a wakeup source, it must be kept active during system suspend.
>>>>
>>>> Geert, I think we discussed this a bit already. I wonder if the above
>>>> is a correct statement for all devices in these PM domains, that has
>>>> wakeups?
>>>
>>> It is true for all wakeup sources (e.g. Ethernet and serial).
>>>
>>>> Don't these SoCs make use of any external logic (out-of-band IRQ) to
>>>> deal with the wakeup IRQs?
>>>>
>>>> For example, how is GPIO irqs dealt with in this regards?
>>>
>>> Interrupt controllers (incl. GPIO) may, depending on SoC type:
>>>   - be located in a controllable power area (SH/R-Mobile),
>>>   - may run from a controllable module clock (SH/R-Mobile, R-Car Gen2/Gen3,
>>>     RZ/A1, RZ/G1).
>>>
>>> So there are no out-of-band IRQs in the wakeup path, unless on SoCs where
>>> the interrupt controllers are in an always-on power area, AND run from a
>>> fixed clock. I think only R-Car Gen1 falls in that category.
>>> Still, R-Car Gen1 needs active_wakeup for wakeup sources.
>>
>> Perhaps out-of-band IRQ is a too vague term. :-) Anyway, let me
>> elaborate a bit more.
>>
>> Apologize for being so persistent, but I really want to get to bottom with this.
>>
>> According to the statement above, it seems like IRQs (including GPIOs)
>> is controllable from a separate "power area" (or module clock). In
>
> There are two ways to save power: disabling the module clock, and powering
> down the module logic.
>
>> many ARM SoCs that "power area" is a separate piece of external logic,
>> sometimes it may even consist a small co-processor. I guess you
>> already know that, but wanted to point it out for clarity.
>
> Sure.
>
> I think on some older SH/R-Mobile SoCs you can do something similar,
> but even there you have to keep the interrupt controller's power area powered.
> This makes sense, given those SoCs also have an SH core, which could be used
> as the small co-processor that powers down the whole ARM subsystem and
> handles wakeup through the SH subsystem's interrupt controller.
>
>> For that reason, some serial drivers re-routs its serial rx pin to a
>> GPIO IRQ to deal with wakeup, instead of keeping the serial device
>> always powered. This enables the GPIO IRQ to be managed by the
>> external logic, thus allowing the serial device and the PM domain it
>> is attached to, to be powered off. Especially during system suspend,
>> that may avoid wasting lots of power.
>
> Doesn't re-routing a pin to a GPIO require using pinctrl?

Yes.

>
> Here the serial device is the wakeup source, but it doesn't handle wakeup
> itself...

Exactly.

>
>> Another example, where I think a more fine grained method is preferred
>> over using GENPD_FLAG_ACTIVE_WAKEUP, is when an SD card controller has
>> an card detect pin hooked up to a GPIO. Thus the device_may_wakeup()
>> would return true for the SD card controller's struct device, but that
>> does not mean that the device needs to stay powered during system
>> suspend.
>
> This one is more tricky, as I think we're already using a GPIO for CD on
> many boards.
> Then the SD device is the wakeup source, but it doesn't handle wakeup
> itself...

Exactly!

>
> So you not only need a flag to opt-in, but also a flag to opt-out?

>From a genpd point of view, the default method is to trusts the driver
to deal with the wakeup, then it treats the device (and thus the PM
domain) as it can be powered down, even if it is configured as a
wakeup source.

Therefore, I have so far, only seen a reason to enable an opt-in
method, to let drivers instruct genpd that it needs to change its
default behavior.

>
>>> See series "[PATCH/RFC 0/3] renesas: irqchip: Use wakeup_path i.s.o.
>>> explicit clock handling", which includes GPIO interrupts.
>>>
>>>> If that is the case, you should really avoid using the big hammer
>>>> method of setting the GENPD_FLAG_ACTIVE_WAKEUP.
>>>
>>> So I think I do need the big hammer ;-)
>>
>> From a hypothetical point of view, if you were to use the more fine
>> grained method I proposed [1] (or something very similar), how many
>> drivers would you need to change, to be able to remove the current
>> workaround?
>
> I think five:
>
>   - drivers/gpio/gpio-rcar.c
>   - drivers/irqchip/irq-renesas-intc-irqpin.c
>   - drivers/irqchip/irq-renesas-irqc.c
>   - drivers/net/ethernet/renesas/ravb_main.c
>   - drivers/net/ethernet/renesas/sh_eth.c

Great, that sounds quite limited. :-)

>
> Note that while setting the flag wouldn't harm, it would not be necessary for
> gpio-rcar and renesas-intc-irqpin when running on R-Car Gen1.  Not setting it
> means using platform knowledge in a device driver, which is what genpd was
> supposed to solve in the first place, and thus IMHO a layering violation.

The flag (assuming you mean WAKEUP_POWERED), is to allow drivers to
instruct the bus type and PM domain about that it think it uses
"in-band-wakeup". If the bus type and PM domain has additional
knowledge about wakeup configurations, it's free to override that
behavior. So there should be no "layering violation. :-)

In other words, genpd should continue to respect the
GENPD_FLAG_ACTIVE_WAKEUP, but my point is, that I think the flag
should be more carefully set by genpd clients.

Does it make sense?

Kind regards
Uffe

Re: [PATCH v2 1/3] clk: renesas: mstp: Keep wakeup sources active during system suspend

[Ulf Hansson]

On 9 November 2017 at 14:27, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> If a device is part of the CPG/MSTP Clock Domain and to be used as a
> wakeup source, it must be kept active during system suspend.
>
> Currently this is handled in device-specific drivers by explicitly
> increasing the use count of the module clock when the device is
> configured as a wakeup source.  However, the proper way to prevent the
> device from being stopped is to inform this requirement to the genpd
> core, by setting the GENPD_FLAG_ACTIVE_WAKEUP flag.
>
> Note that this will only affect devices configured as wakeup sources.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
> v2:
>   - Integrate "clk: renesas: mstp: Use GENPD_FLAG_ACTIVE_WAKEUP",
> ---
>  drivers/clk/renesas/clk-mstp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
> index 500a9e4e03c48957..0b222197a4576fd7 100644
> --- a/drivers/clk/renesas/clk-mstp.c
> +++ b/drivers/clk/renesas/clk-mstp.c
> @@ -344,7 +344,7 @@ void __init cpg_mstp_add_clk_domain(struct device_node *np)
>                 return;
>
>         pd->name = np->name;
> -       pd->flags = GENPD_FLAG_PM_CLK;
> +       pd->flags = GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP;
>         pd->attach_dev = cpg_mstp_attach_dev;
>         pd->detach_dev = cpg_mstp_detach_dev;
>         pm_genpd_init(pd, &pm_domain_always_on_gov, false);
> --
> 2.7.4
>

Re: [PATCH v2 2/3] clk: renesas: cpg-mssr: Keep wakeup sources active during system suspend

[Ulf Hansson]

On 9 November 2017 at 14:27, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> If a device is part of the CPG/MSSR Clock Domain and to be used as a
> wakeup source, it must be kept active during system suspend.
>
> Currently this is handled in device-specific drivers by explicitly
> increasing the use count of the module clock when the device is
> configured as a wakeup source.  However, the proper way to prevent the
> device from being stopped is to inform this requirement to the genpd
> core, by setting the GENPD_FLAG_ACTIVE_WAKEUP flag.
>
> Note that this will only affect devices configured as wakeup sources.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe


> ---
> v2:
>   - Integrate "clk: renesas: cpg-mssr: Use GENPD_FLAG_ACTIVE_WAKEUP",
> ---
>  drivers/clk/renesas/renesas-cpg-mssr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> index e580a5e6346c2533..99699b715d7cf2de 100644
> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> @@ -500,7 +500,7 @@ static int __init cpg_mssr_add_clk_domain(struct device *dev,
>
>         genpd = &pd->genpd;
>         genpd->name = np->name;
> -       genpd->flags = GENPD_FLAG_PM_CLK;
> +       genpd->flags = GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP;
>         genpd->attach_dev = cpg_mssr_attach_dev;
>         genpd->detach_dev = cpg_mssr_detach_dev;
>         pm_genpd_init(genpd, &pm_domain_always_on_gov, false);
> --
> 2.7.4
>

Re: [PATCH v2 3/3] soc: renesas: rcar-sysc: Keep wakeup sources active during system suspend

[Ulf Hansson]

On 9 November 2017 at 14:27, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> If an R-Car SYSC slave device is part of the CPG/MSTP or CPG/MSSR Clock
> Domain and to be used as a wakeup source, it must be kept active during
> system suspend.
>
> Currently this is handled in device-specific drivers by explicitly
> increasing the use count of the module clock when the device is
> configured as a wakeup source.  However, the proper way to prevent the
> device from being stopped is to inform this requirement to the genpd
> core, by setting the GENPD_FLAG_ACTIVE_WAKEUP flag.
>
> Note that this will only affect devices configured as wakeup sources.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
> v2:
>   - Integrate "soc: renesas: rcar-sysc: Use GENPD_FLAG_ACTIVE_WAKEUP",
> ---
>  drivers/soc/renesas/rcar-sysc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/soc/renesas/rcar-sysc.c b/drivers/soc/renesas/rcar-sysc.c
> index c8406e81640f6560..ac4df1c43b2fbdbe 100644
> --- a/drivers/soc/renesas/rcar-sysc.c
> +++ b/drivers/soc/renesas/rcar-sysc.c
> @@ -224,7 +224,7 @@ static void __init rcar_sysc_pd_setup(struct rcar_sysc_pd *pd)
>
>         if (!(pd->flags & (PD_CPU | PD_SCU))) {
>                 /* Enable Clock Domain for I/O devices */
> -               genpd->flags |= GENPD_FLAG_PM_CLK;
> +               genpd->flags |= GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP;
>                 if (has_cpg_mstp) {
>                         genpd->attach_dev = cpg_mstp_attach_dev;
>                         genpd->detach_dev = cpg_mstp_detach_dev;
> --
> 2.7.4
>

Re: [PATCH v2 1/3] clk: renesas: mstp: Keep wakeup sources active during system suspend

[Geert Uytterhoeven]

On Thu, Dec 14, 2017 at 3:10 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 9 November 2017 at 14:27, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>> If a device is part of the CPG/MSTP Clock Domain and to be used as a
>> wakeup source, it must be kept active during system suspend.
>>
>> Currently this is handled in device-specific drivers by explicitly
>> increasing the use count of the module clock when the device is
>> configured as a wakeup source.  However, the proper way to prevent the
>> device from being stopped is to inform this requirement to the genpd
>> core, by setting the GENPD_FLAG_ACTIVE_WAKEUP flag.
>>
>> Note that this will only affect devices configured as wakeup sources.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Thank you, queued in clk-renesas-for-v4.16.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 2/3] clk: renesas: cpg-mssr: Keep wakeup sources active during system suspend

[Geert Uytterhoeven]

On Thu, Dec 14, 2017 at 3:11 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 9 November 2017 at 14:27, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>> If a device is part of the CPG/MSSR Clock Domain and to be used as a
>> wakeup source, it must be kept active during system suspend.
>>
>> Currently this is handled in device-specific drivers by explicitly
>> increasing the use count of the module clock when the device is
>> configured as a wakeup source.  However, the proper way to prevent the
>> device from being stopped is to inform this requirement to the genpd
>> core, by setting the GENPD_FLAG_ACTIVE_WAKEUP flag.
>>
>> Note that this will only affect devices configured as wakeup sources.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Thank you, queued in clk-renesas-for-v4.16.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 3/3] soc: renesas: rcar-sysc: Keep wakeup sources active during system suspend

[Geert Uytterhoeven]

On Thu, Dec 14, 2017 at 3:11 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 9 November 2017 at 14:27, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>> If an R-Car SYSC slave device is part of the CPG/MSTP or CPG/MSSR Clock
>> Domain and to be used as a wakeup source, it must be kept active during
>> system suspend.
>>
>> Currently this is handled in device-specific drivers by explicitly
>> increasing the use count of the module clock when the device is
>> configured as a wakeup source.  However, the proper way to prevent the
>> device from being stopped is to inform this requirement to the genpd
>> core, by setting the GENPD_FLAG_ACTIVE_WAKEUP flag.
>>
>> Note that this will only affect devices configured as wakeup sources.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Thank you!

Simon: can you please queue this up for v4.16?
I have queued the other 2 clock patches in clk-renesas-for-v4.16, and sent
a PR for it.
Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 3/3] soc: renesas: rcar-sysc: Keep wakeup sources active during system suspend

[Simon Horman]

On Mon, Dec 18, 2017 at 12:22:17PM +0100, Geert Uytterhoeven wrote:
> On Thu, Dec 14, 2017 at 3:11 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On 9 November 2017 at 14:27, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> >> If an R-Car SYSC slave device is part of the CPG/MSTP or CPG/MSSR Clock
> >> Domain and to be used as a wakeup source, it must be kept active during
> >> system suspend.
> >>
> >> Currently this is handled in device-specific drivers by explicitly
> >> increasing the use count of the module clock when the device is
> >> configured as a wakeup source.  However, the proper way to prevent the
> >> device from being stopped is to inform this requirement to the genpd
> >> core, by setting the GENPD_FLAG_ACTIVE_WAKEUP flag.
> >>
> >> Note that this will only affect devices configured as wakeup sources.
> >>
> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> Thank you!
> 
> Simon: can you please queue this up for v4.16?
> I have queued the other 2 clock patches in clk-renesas-for-v4.16, and sent
> a PR for it.

Thanks, I have applied this patch to the soc-for-v4.16 branch.
It is currently not based v4.15-rc1. Let me know if it should
be rebased on top of your pull request for clk-renesas-for-v4.16.

Re: [PATCH v2 3/3] soc: renesas: rcar-sysc: Keep wakeup sources active during system suspend

[Geert Uytterhoeven]

Hi Simon,

On Wed, Dec 20, 2017 at 11:23 AM, Simon Horman <horms@verge.net.au> wrote:
> On Mon, Dec 18, 2017 at 12:22:17PM +0100, Geert Uytterhoeven wrote:
>> On Thu, Dec 14, 2017 at 3:11 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> > On 9 November 2017 at 14:27, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>> >> If an R-Car SYSC slave device is part of the CPG/MSTP or CPG/MSSR Clock
>> >> Domain and to be used as a wakeup source, it must be kept active during
>> >> system suspend.
>> >>
>> >> Currently this is handled in device-specific drivers by explicitly
>> >> increasing the use count of the module clock when the device is
>> >> configured as a wakeup source.  However, the proper way to prevent the
>> >> device from being stopped is to inform this requirement to the genpd
>> >> core, by setting the GENPD_FLAG_ACTIVE_WAKEUP flag.
>> >>
>> >> Note that this will only affect devices configured as wakeup sources.
>> >>
>> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> >
>> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> Thank you!
>>
>> Simon: can you please queue this up for v4.16?
>> I have queued the other 2 clock patches in clk-renesas-for-v4.16, and sent
>> a PR for it.
>
> Thanks, I have applied this patch to the soc-for-v4.16 branch.
> It is currently not based v4.15-rc1. Let me know if it should
> be rebased on top of your pull request for clk-renesas-for-v4.16.

If the base is not v4.15-rc1, what is it? GENPD_FLAG_ACTIVE_WAKEUP was
introduced in v4.15-rc1, so it won't compile based on v4.14.

There's no dependency on the clock drivers, so no need to base on
top of clk-renesas-for-v4.16.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH v2 3/3] soc: renesas: rcar-sysc: Keep wakeup sources active during system suspend

[Simon Horman]

On Wed, Dec 20, 2017 at 11:28:43AM +0100, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Wed, Dec 20, 2017 at 11:23 AM, Simon Horman <horms@verge.net.au> wrote:
> > On Mon, Dec 18, 2017 at 12:22:17PM +0100, Geert Uytterhoeven wrote:
> >> On Thu, Dec 14, 2017 at 3:11 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >> > On 9 November 2017 at 14:27, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> >> >> If an R-Car SYSC slave device is part of the CPG/MSTP or CPG/MSSR Clock
> >> >> Domain and to be used as a wakeup source, it must be kept active during
> >> >> system suspend.
> >> >>
> >> >> Currently this is handled in device-specific drivers by explicitly
> >> >> increasing the use count of the module clock when the device is
> >> >> configured as a wakeup source.  However, the proper way to prevent the
> >> >> device from being stopped is to inform this requirement to the genpd
> >> >> core, by setting the GENPD_FLAG_ACTIVE_WAKEUP flag.
> >> >>
> >> >> Note that this will only affect devices configured as wakeup sources.
> >> >>
> >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >> >
> >> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> >>
> >> Thank you!
> >>
> >> Simon: can you please queue this up for v4.16?
> >> I have queued the other 2 clock patches in clk-renesas-for-v4.16, and sent
> >> a PR for it.
> >
> > Thanks, I have applied this patch to the soc-for-v4.16 branch.
> > It is currently not based v4.15-rc1. Let me know if it should
> > be rebased on top of your pull request for clk-renesas-for-v4.16.
> 
> If the base is not v4.15-rc1, what is it? GENPD_FLAG_ACTIVE_WAKEUP was
> introduced in v4.15-rc1, so it won't compile based on v4.14.

Sorry, I typed my message incorrectly. The base _is_ v4.15-rc1.

> There's no dependency on the clock drivers, so no need to base on
> top of clk-renesas-for-v4.16.

Thanks, got it.