Re: [PATCH v5 03/11] soc: renesas: rcar-sysc: Add DT support for SYSC PM domains

[Ulf Hansson]

On 20 April 2016 at 10:24, Geert Uytterhoeven  wrote:
> Hi Ulf,
>
> On Mon, Apr 18, 2016 at 4:02 PM, Ulf Hansson  wrote:
>> On 18 April 2016 at 15:39, Geert Uytterhoeven  wrote:
>>> On Mon, Apr 18, 2016 at 2:59 PM, Geert Uytterhoeven
>>>  wrote:
 On Mon, Apr 18, 2016 at 2:21 PM, Ulf Hansson  
 wrote:
> [...]
>
>> +
>> +static bool rcar_sysc_active_wakeup(struct device *dev)
>> +{
>> +   return true;
>
> I am interested to know why this is always returning true. Perhaps you
> can elaborate a bit on that?

 Too many copying from old shmobile PM Domain code?
 Honestly, I don't know...

 Perhaps Rafael still remembers the original rationale, as git history for
 commit e3e0109138376bb2 ("ARM / shmobile: Support for I/O power domains for
 SH7372 (v9)") doesn't have it.

 Google did find: https://lkml.org/lkml/2011/6/30/471

 Do we still need this at all? I.e. aren't PM Domains containing wake-up
 devices kept powered automatically during system suspend?
>>>
>>> No they aren't. So for pm-rmobile we do need it.
>>
>> I don't quite understand why genpd should need to treat all devices
>> within the same domain exactly the same, it seems suboptimal.
>>
>> I guess it would be more clever to allow this to be controlled on per
>> device basis instead, so let's say from each driver.
>
> Perhaps this can be handled through device_set_wakeup_enable()?

Yes, the pm_wakeirq API should help with all what is needed.

> Unfortunately this doesn't seem to be called from e.g. gpio-keys.
>

Okay, so it's a matter of deployment for these devices/drivers.

A list of such drivers/devices that needs to be fixed would be great
to have. :-)

Kind regards
Uffe

Re: [PATCH v5 03/11] soc: renesas: rcar-sysc: Add DT support for SYSC PM domains

[Geert Uytterhoeven]

Hi Ulf,

On Mon, Apr 18, 2016 at 4:02 PM, Ulf Hansson  wrote:
> On 18 April 2016 at 15:39, Geert Uytterhoeven  wrote:
>> On Mon, Apr 18, 2016 at 2:59 PM, Geert Uytterhoeven
>>  wrote:
>>> On Mon, Apr 18, 2016 at 2:21 PM, Ulf Hansson  wrote:
 [...]

> +
> +static bool rcar_sysc_active_wakeup(struct device *dev)
> +{
> +   return true;

 I am interested to know why this is always returning true. Perhaps you
 can elaborate a bit on that?
>>>
>>> Too many copying from old shmobile PM Domain code?
>>> Honestly, I don't know...
>>>
>>> Perhaps Rafael still remembers the original rationale, as git history for
>>> commit e3e0109138376bb2 ("ARM / shmobile: Support for I/O power domains for
>>> SH7372 (v9)") doesn't have it.
>>>
>>> Google did find: https://lkml.org/lkml/2011/6/30/471
>>>
>>> Do we still need this at all? I.e. aren't PM Domains containing wake-up
>>> devices kept powered automatically during system suspend?
>>
>> No they aren't. So for pm-rmobile we do need it.
>
> I don't quite understand why genpd should need to treat all devices
> within the same domain exactly the same, it seems suboptimal.
>
> I guess it would be more clever to allow this to be controlled on per
> device basis instead, so let's say from each driver.

Perhaps this can be handled through device_set_wakeup_enable()?
Unfortunately this doesn't seem to be called from e.g. gpio-keys.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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 v5 03/11] soc: renesas: rcar-sysc: Add DT support for SYSC PM domains

[Ulf Hansson]

On 18 April 2016 at 15:39, Geert Uytterhoeven  wrote:
> Hi Ulf,
>
> On Mon, Apr 18, 2016 at 2:59 PM, Geert Uytterhoeven
>  wrote:
>> On Mon, Apr 18, 2016 at 2:21 PM, Ulf Hansson  wrote:
>>> [...]
>>>
 +
 +static bool rcar_sysc_active_wakeup(struct device *dev)
 +{
 +   return true;
>>>
>>> I am interested to know why this is always returning true. Perhaps you
>>> can elaborate a bit on that?
>>
>> Too many copying from old shmobile PM Domain code?
>> Honestly, I don't know...
>>
>> Perhaps Rafael still remembers the original rationale, as git history for
>> commit e3e0109138376bb2 ("ARM / shmobile: Support for I/O power domains for
>> SH7372 (v9)") doesn't have it.
>>
>> Google did find: https://lkml.org/lkml/2011/6/30/471
>>
>> Do we still need this at all? I.e. aren't PM Domains containing wake-up
>> devices kept powered automatically during system suspend?
>
> No they aren't. So for pm-rmobile we do need it.

I don't quite understand why genpd should need to treat all devices
within the same domain exactly the same, it seems suboptimal.

I guess it would be more clever to allow this to be controlled on per
device basis instead, so let's say from each driver.

>
> For rcar-sysc it's different: as no PM Domain contains wake-up devices
> (all I/O devices are in the always-on power area), we don't need the callback.
> Will drop it in v6.

Okay, great!

Kind regards
Uffe

Re: [PATCH v5 03/11] soc: renesas: rcar-sysc: Add DT support for SYSC PM domains

[Geert Uytterhoeven]

Hi Ulf,

On Mon, Apr 18, 2016 at 2:21 PM, Ulf Hansson  wrote:
> [...]
>
>> +
>> +static bool rcar_sysc_active_wakeup(struct device *dev)
>> +{
>> +   return true;
>
> I am interested to know why this is always returning true. Perhaps you
> can elaborate a bit on that?

Too many copying from old shmobile PM Domain code?
Honestly, I don't know...

Perhaps Rafael still remembers the original rationale, as git history for
commit e3e0109138376bb2 ("ARM / shmobile: Support for I/O power domains for
SH7372 (v9)") doesn't have it.

Google did find: https://lkml.org/lkml/2011/6/30/471

Do we still need this at all? I.e. aren't PM Domains containing wake-up
devices kept powered automatically during system suspend?

>> +static void __init rcar_sysc_pd_setup(struct rcar_sysc_pd *pd)
>> +{
>> +   struct generic_pm_domain *genpd = >genpd;
>> +   const char *name = pd->genpd.name;
>> +   struct dev_power_governor *gov = _qos_governor;
>> +
>> +   if (pd->flags & PD_CPU) {
>> +   /*
>> +* This domain contains a CPU core and therefore it should
>> +* only be turned off if the CPU is not in use.
>> +*/
>> +   pr_debug("PM domain %s contains %s\n", name, "CPU");
>> +   pd->flags |= PD_BUSY;
>> +   gov = _domain_always_on_gov;
>> +   } else if (pd->flags & PD_SCU) {
>> +   /*
>> +* This domain contains an SCU and cache-controller, and
>> +* therefore it should only be turned off if the CPU cores 
>> are
>> +* not in use.
>> +*/
>> +   pr_debug("PM domain %s contains %s\n", name, "SCU");
>> +   pd->flags |= PD_BUSY;
>> +   gov = _domain_always_on_gov;
>> +   } else if (pd->flags & PD_NO_CR) {
>> +   /*
>> +* This domain cannot be turned off.
>> +*/
>> +   pd->flags |= PD_BUSY;
>> +   gov = _domain_always_on_gov;
>> +   }
>> +
>> +   pm_genpd_init(genpd, gov, false);
>
> This seems weird. I don't think it correct to initialize genpd and
> then continue with the below changes (assigning callbacks and power up
> the domain).

I'm quite sure I wondered the same (for pm-rmobile), but discovered that
pm_genpd_init() overwrote something. Unfortunately I can't find the commit
that changed that...

> I would recommend doing this in the reverse order.
>
>> +   genpd->dev_ops.active_wakeup = rcar_sysc_active_wakeup;
>> +   genpd->power_off = rcar_sysc_pd_power_off;
>> +   genpd->power_on = rcar_sysc_pd_power_on;

OK, will give that a try...

Thanks for your comments!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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 v5 03/11] soc: renesas: rcar-sysc: Add DT support for SYSC PM domains

[Ulf Hansson]

[...]

> +
> +static bool rcar_sysc_active_wakeup(struct device *dev)
> +{
> +   return true;

I am interested to know why this is always returning true. Perhaps you
can elaborate a bit on that?

> +}
> +
> +static int rcar_sysc_pd_power_off(struct generic_pm_domain *genpd)
> +{
> +   struct rcar_sysc_pd *pd = to_rcar_pd(genpd);
> +
> +   pr_debug("%s: %s\n", __func__, genpd->name);
> +
> +   if (pd->flags & PD_NO_CR) {
> +   pr_debug("%s: Cannot control %s\n", __func__, genpd->name);
> +   return -EBUSY;
> +   }
> +
> +   if (pd->flags & PD_BUSY) {
> +   pr_debug("%s: %s busy\n", __func__, genpd->name);
> +   return -EBUSY;
> +   }
> +
> +   return rcar_sysc_power_down(>ch);
> +}
> +
> +static int rcar_sysc_pd_power_on(struct generic_pm_domain *genpd)
> +{
> +   struct rcar_sysc_pd *pd = to_rcar_pd(genpd);
> +
> +   pr_debug("%s: %s\n", __func__, genpd->name);
> +
> +   if (pd->flags & PD_NO_CR) {
> +   pr_debug("%s: Cannot control %s\n", __func__, genpd->name);
> +   return 0;
> +   }
> +
> +   return rcar_sysc_power_up(>ch);
> +}
> +
> +static void __init rcar_sysc_pd_setup(struct rcar_sysc_pd *pd)
> +{
> +   struct generic_pm_domain *genpd = >genpd;
> +   const char *name = pd->genpd.name;
> +   struct dev_power_governor *gov = _qos_governor;
> +
> +   if (pd->flags & PD_CPU) {
> +   /*
> +* This domain contains a CPU core and therefore it should
> +* only be turned off if the CPU is not in use.
> +*/
> +   pr_debug("PM domain %s contains %s\n", name, "CPU");
> +   pd->flags |= PD_BUSY;
> +   gov = _domain_always_on_gov;
> +   } else if (pd->flags & PD_SCU) {
> +   /*
> +* This domain contains an SCU and cache-controller, and
> +* therefore it should only be turned off if the CPU cores are
> +* not in use.
> +*/
> +   pr_debug("PM domain %s contains %s\n", name, "SCU");
> +   pd->flags |= PD_BUSY;
> +   gov = _domain_always_on_gov;
> +   } else if (pd->flags & PD_NO_CR) {
> +   /*
> +* This domain cannot be turned off.
> +*/
> +   pd->flags |= PD_BUSY;
> +   gov = _domain_always_on_gov;
> +   }
> +
> +   pm_genpd_init(genpd, gov, false);

This seems weird. I don't think it correct to initialize genpd and
then continue with the below changes (assigning callbacks and power up
the domain).

I would recommend doing this in the reverse order.

> +   genpd->dev_ops.active_wakeup = rcar_sysc_active_wakeup;
> +   genpd->power_off = rcar_sysc_pd_power_off;
> +   genpd->power_on = rcar_sysc_pd_power_on;
> +
> +   if (pd->flags & (PD_CPU | PD_NO_CR)) {
> +   /* Skip CPUs (handled by SMP code) and areas without control 
> */
> +   pr_debug("%s: Not touching %s\n", __func__, genpd->name);
> +   return;
> +   }
> +
> +   if (!rcar_sysc_power_is_off(>ch)) {
> +   pr_debug("%s: %s is already powered\n", __func__, 
> genpd->name);
> +   return;
> +   }
> +
> +   rcar_sysc_power_up(>ch);
> +}

Kind regards
Uffe