* SDHI clock imbalance
@ 2017-06-02 8:47 Geert Uytterhoeven
2017-06-02 9:37 ` Wolfram Sang
2017-06-02 10:37 ` Ulf Hansson
0 siblings, 2 replies; 5+ messages in thread
From: Geert Uytterhoeven @ 2017-06-02 8:47 UTC (permalink / raw)
To: Wolfram Sang, Ulf Hansson, Simon Horman
Cc: Linux MMC List, Linux PM list, Linux-Renesas
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
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 -- 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: SDHI clock imbalance
2017-06-02 8:47 SDHI clock imbalance Geert Uytterhoeven
@ 2017-06-02 9:37 ` Wolfram Sang
2017-06-02 9:52 ` Geert Uytterhoeven
2017-06-02 10:37 ` Ulf Hansson
1 sibling, 1 reply; 5+ messages in thread
From: Wolfram Sang @ 2017-06-02 9:37 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Wolfram Sang, Ulf Hansson, Simon Horman, Linux MMC List,
Linux PM list, Linux-Renesas
[-- Attachment #1: Type: text/plain, Size: 305 bytes --]
Hi Geert,
> While investigating suspend/resume for the R-Car Gen3 clock driver, I
> noticed a clock imbalance for SDHI on Salvator-X.
Thanks for the heads up! Which branch did you use?
renesas-drivers/master? This is important since we had quite some
refactoring going on lately.
Regards,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: SDHI clock imbalance
2017-06-02 9:37 ` Wolfram Sang
@ 2017-06-02 9:52 ` Geert Uytterhoeven
0 siblings, 0 replies; 5+ messages in thread
From: Geert Uytterhoeven @ 2017-06-02 9:52 UTC (permalink / raw)
To: Wolfram Sang
Cc: Wolfram Sang, Ulf Hansson, Simon Horman, Linux MMC List,
Linux PM list, Linux-Renesas
Hi Wolfram,
On Fri, Jun 2, 2017 at 11:37 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> While investigating suspend/resume for the R-Car Gen3 clock driver, I
>> noticed a clock imbalance for SDHI on Salvator-X.
>
> Thanks for the heads up! Which branch did you use?
> renesas-drivers/master? This is important since we had quite some
> refactoring going on lately.
Yes, a tree based on renesas-drivers.
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: SDHI clock imbalance
2017-06-02 8:47 SDHI clock imbalance Geert Uytterhoeven
2017-06-02 9:37 ` Wolfram Sang
@ 2017-06-02 10:37 ` Ulf Hansson
2017-06-06 10:49 ` Wolfram Sang
1 sibling, 1 reply; 5+ messages in thread
From: Ulf Hansson @ 2017-06-02 10:37 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Wolfram Sang, Simon Horman, Linux MMC List, Linux PM list,
Linux-Renesas
On 2 June 2017 at 10:47, Geert Uytterhoeven <geert@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 -- 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Kind regards
Uffe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: SDHI clock imbalance
2017-06-02 10:37 ` Ulf Hansson
@ 2017-06-06 10:49 ` Wolfram Sang
0 siblings, 0 replies; 5+ messages in thread
From: Wolfram Sang @ 2017-06-06 10:49 UTC (permalink / raw)
To: Ulf Hansson
Cc: Geert Uytterhoeven, Wolfram Sang, Simon Horman, Linux MMC List,
Linux PM list, Linux-Renesas
[-- Attachment #1: Type: text/plain, Size: 1515 bytes --]
> 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. :-)
So, in my words: in that case, the device is not really active since the
clocks are not enabled?
> 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.
We have tmio_mmc_clk_enable() already for that, I'd think. Or am I
misunderstanding?
> 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.
Welllll... I hope we can avoid this ;)
Thanks for your thoughts, Ulf. And thanks to Geert for bringing up the
issue which I can reproduce here. We need to do a bigger PM
re-evaluation anyhow, I'll add it to the list...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-06-06 10:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-02 8:47 SDHI clock imbalance Geert Uytterhoeven
2017-06-02 9:37 ` Wolfram Sang
2017-06-02 9:52 ` Geert Uytterhoeven
2017-06-02 10:37 ` Ulf Hansson
2017-06-06 10:49 ` Wolfram Sang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).