[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 <ulf.hansson at linaro.org> 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 = &pd->genpd;
>> +       const char *name = pd->genpd.name;
>> +       struct dev_power_governor *gov = &simple_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 = &pm_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 = &pm_domain_always_on_gov;
>> +       } else if (pd->flags & PD_NO_CR) {
>> +               /*
>> +                * This domain cannot be turned off.
>> +                */
>> +               pd->flags |= PD_BUSY;
>> +               gov = &pm_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 -- geert at 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