On 2 June 2017 at 10:47, Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
> Hi Wolfram, Ulf, Simon,
>
> While investigating suspend/resume for the R-Car Gen3 clock driver, I
> noticed a clock imbalance for SDHI on Salvator-X.
>
> After boot:
>
> # head -2 /sys/kernel/debug/clk/clk_summary
>    clock         enable_cnt prepare_cnt  rate  accuracy phase
> -------------------------------------------------------------
> # grep sd /sys/kernel/debug/clk/clk_summary
>          .sdsrc           3        3 399999984      0 0
>         sd3               1        1     6250000      0 0
>            sdif3          1        2     6250000      0 0
>         sd2               1        1   199999992      0 0
>            sdif2          1        2   199999992      0 0
>         sd1               0        0   199999992      0 0
>            sdif1          0        0   199999992      0 0
>         sd0               1        1     6250000      0 0
>            sdif0          1        2     6250000      0 0
>
> After s2idle suspend/resume:
>
> # grep sd /sys/kernel/debug/clk/clk_summary
>          .sdsrc           3        3   399999984      0 0
>         sd3               1        1     6250000      0 0
>            sdif3          2        2     6250000      0 0
>         sd2               1        1   199999992      0 0
>            sdif2          2        2   199999992      0 0
>         sd1               0        0   199999992      0 0
>            sdif1          0        0   199999992      0 0
>         sd0               1        1     6250000      0 0
>            sdif0          2        2     6250000      0 0
>
> Enable counts are 1 before suspend, and 2 after resume.
>
>
> Boot: Enabled once (also at hardware level):
>
>     platform_drv_probe
>         renesas_sdhi_sys_dmac_probe
>             renesas_sdhi_probe
>                 tmio_mmc_host_probe
>                     renesas_sdhi_clk_enable

The driver calls pm_runtime_set_active() during ->probe(), which means
genpd's ->runtime_resume() callback isn't invoked during that point.
In other words, the clocks managed by the clock domain isn't enabled
during ->probe() as genpd's doesn't get to run pm_clk_resume() from
its ->runtime_resume() callback. Unless I am missing something. :-)

I think this is the reason to the following problems. How to fix it?

The driver needs to call pm_runtime_get_sync() instead of
pm_runtime_set_active(), however that may requires some careful
changes if one wants the driver to be able to enable clocks also when
CONFIG_PM is unset. If not, it's pretty easy, else I would do
something like below.

Add a "init" flag to host struct, and set that flag before calling
pm_runtime_get_sync() in ->probe(). When the driver's
->runtime_resume() callbacks get called when the "init" flag is set,
just do an early return 0, as t means ->probe() is running and has
already taken care of the enabling the clock. When probe is done, and
before dropping runtime usage count with pm_runtime_put(), reset the
"init" flag.

>
>
> Suspend: Disabled once (also at hardware level):
>
>     suspend_devices_and_enter
>         dpm_suspend_start
>             dpm_suspend
>                 __device_suspend
>                     dpm_run_callback
>                         pm_runtime_force_suspend
>                             genpd_runtime_suspend
>                                 pm_generic_runtime_suspend
>                                     tmio_mmc_host_runtime_suspend
>                                         renesas_sdhi_clk_disable
>
>
> Resume: Enabled twice (first one enables at hardware level):
>
>     dpm_resume_noirq
>         device_resume_noirq
>             dpm_run_callback
>                 pm_genpd_resume_noirq
>                     pm_runtime_force_resume
>                         genpd_runtime_resume
>                             genpd_start_dev
>                                 pm_clk_resume                   (1)
>                             __genpd_runtime_resume
>                                 pm_generic_runtime_resume
>                                     tmio_mmc_host_runtime_resume
>                                         renesas_sdhi_clk_enable (2)
>
>
> During subsequent suspends, the clock is disabled twice (last one disables
> at hardware level), as expected:
>
>     suspend_devices_and_enter
>         dpm_suspend_start
>             dpm_suspend
>                 __device_suspend
>                     dpm_run_callback
>                         pm_runtime_force_suspend
>                             genpd_runtime_suspend
>                                 pm_generic_runtime_suspend
>                                     tmio_mmc_host_runtime_suspend
>                                         renesas_sdhi_clk_disable (1)
>                                 genpd_stop_dev
>                                     pm_clk_suspend               (2)
>
>
> From now on, the imbalance is gone.
>
> Note that at boot and initial suspend, genpd does not seem to call into the
> clock domain operations at all.
> Presumable some call to pm_runtime_get_sync() is missing?
>
> Thanks.
>
> 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kind regards
Uffe

Reply via email to