Hi Geert,
Thanks for your feedback. Sorry for following up a tad late on this
getting hold of and getting started with the APE6 took some time (thanks
again for your help in that area too!).
On 2019-01-10 17:23:46 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
>
> On Wed, Jan 9, 2019 at 11:36 PM Niklas Söderlund
> <niklas.soderlund+renesas@xxxxxxxxxxxx> wrote:
> > Both the Renesas and Uniphier implementations perform actions which
> > effects runtime PM before calling into the core tmio_mmc_probe() which
> > enabled runtime PM. Move pm_runtime_enable() from the core and
> > tmio_mmc_probe() into each drivers probe() so it can be called before
> > any clocks or other resources are switched on.
> >
> > Reported-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
>
> Thanks for your patch!
>
> /sys/kernel/debug/clk/clk_summary after boot (without your patch):
>
> ape6evm (R-Mobile APE6):
>
> mmc1 0 0 0 12500000 0 0 50000
> mmcif1 0 0 0 12500000 0 0 50000
> mmc0 1 1 0 100000000 0 0 50000
> mmcif0 2 2 0 100000000 0 0 50000
> sdhi2ck 0 0 0 12500000 0 0 50000
> sdhi2 0 0 0 12500000 0 0 50000
> sdhi1ck 1 1 0 12500000 0 0 50000
> sdhi1 2 2 0 12500000 0 0 50000
> sdhi0ck 1 1 0 88888888 0 0 50000
> sdhi0 1 2 0 88888888 0 0 50000
>
> kzm9g (SH-Mobile AG5):
>
> sdhi2ck 1 1 0 69333333 0 0 50000
> sdhi2 2 2 0 69333333 0 0 50000
> sdhi1ck 0 0 0 69333333 0 0 50000
> sdhi1 0 0 0 69333333 0 0 50000
> sdhi0ck 1 1 0 69333333 0 0 50000
> sdhi0 1 2 0 69333333 0 0 50000
>
> m3-n-salvator-xs (R-Car M3-N):
>
> .sdsrc 3 3 0 798720000 0 0 50000
> sd3 1 1 0 12480000 0 0 50000
> sdif3 1 2 0 12480000 0 0 50000
> sd2 1 1 0 199680000 0 0 50000
> sdif2 1 2 0 199680000 0 0 50000
> sd1 0 0 0 199680000 0 0 50000
> sdif1 0 0 0 199680000 0 0 50000
> sd0 1 1 0 12480000 0 0 50000
> sdif0 1 2 0 12480000 0 0 50000
>
>
> /sys/kernel/debug/clk/clk_summary after boot (with your patch):
>
> ape6evm:
>
> mmc1 0 0 0 12500000 0 0 50000
> mmcif1 0 0 0 12500000 0 0 50000
> mmc0 1 1 0 100000000 0 0 50000
> mmcif0 2 2 0 100000000 0 0 50000
> sdhi2ck 0 0 0 12500000 0 0 50000
> sdhi2 0 0 0 12500000 0 0 50000
> sdhi1ck 1 1 0 12500000 0 0 50000
> sdhi1 3 3 0 12500000 0 0 50000
> sdhi0ck 1 1 0 88888888 0 0 50000
> sdhi0 3 3 0 88888888 0 0 50000
>
> kzm9g:
>
> sdhi2ck 1 1 0 69333333 0 0 50000
> sdhi2 3 3 0 69333333 0 0 50000
> sdhi1ck 0 0 0 69333333 0 0 50000
> sdhi1 0 0 0 69333333 0 0 50000
> sdhi0ck 1 1 0 69333333 0 0 50000
> sdhi0 3 3 0 69333333 0 0 50000
>
> m3-n-salvator-xs:
>
> .sdsrc 3 3 0 798720000 0 0 50000
> sd3 1 1 0 12480000 0 0 50000
> sdif3 3 3 0 12480000 0 0 50000
> sd2 1 1 0 199680000 0 0 50000
> sdif2 3 3 0 199680000 0 0 50000
> sd1 0 0 0 199680000 0 0 50000
> sdif1 0 0 0 199680000 0 0 50000
> sd0 1 1 0 12480000 0 0 50000
> sdif0 3 3 0 12480000 0 0 50000
>
>
> Unfortunately the clock imbalance isn't gone.
> After resume from s2ram (with your patch):
>
> ape6evm:
>
> mmc1 0 0 0 12500000 0 0 50000
> mmcif1 0 0 0 12500000 0 0 50000
> mmc0 0 1 0 100000000 0 0 50000
> mmcif0 0 1 0 100000000 0 0 50000
These clocks are handled by the sh_mmcif driver and I observe the same
thing. I suspect however that this is the correct behavior as the driver
uses dev_pm_qos_expose_latency_limit() and trying to use the mmc changes
this to 1/1 and it seems to work fine.
My knowledge in this area of PM is not good so I might be wrong but I'm
not trying to deal with this clock in this patch.
> sdhi2ck 0 0 0 12500000 0 0 50000
> sdhi2 0 0 0 12500000 0 0 50000
> sdhi1ck 1 1 0 12500000 0 0 50000
> sdhi1 1 2 0 12500000 0 0 50000
> sdhi0ck 1 1 0 88888888 0 0 50000
> sdhi0 3 3 0 88888888 0 0 50000
I can not reproduce this with this patch on top of v4.20 which the
original patch was based on nor on today's mmc/next. For me clocks are
the same as before suspending. I do however get this result if I run on
ether base without this patch.
>
> kzm9g:
>
> sdhi2ck 1 1 0 69333333 0 0 50000
> sdhi2 1 2 0 69333333 0 0 50000
> sdhi1ck 0 0 0 69333333 0 0 50000
> sdhi1 0 0 0 69333333 0 0 50000
> sdhi0ck 1 1 0 69333333 0 0 50000
> sdhi0 3 3 0 69333333 0 0 50000
>
> And my debug prints in genpd_st{art,op}_dev() still tell me the device
> is cycled continuously after resume from s2ram:
>
> ape6evm:
>
> [ 761.928322] ==== a3sp/ee120000.sd: start
> [ 762.017086] ==== a3sp/ee120000.sd: stop
> [ 762.968418] ==== a3sp/ee120000.sd: start
> [ 763.057139] ==== a3sp/ee120000.sd: stop
> [ 764.008535] ==== a3sp/ee120000.sd: start
> [ 764.097261] ==== a3sp/ee120000.sd: stop
> [ 765.048621] ==== a3sp/ee120000.sd: start
> [ 765.137359] ==== a3sp/ee120000.sd: stop
> ...
I tried the same on my end and I can observe the start and stop as I
expected and can't observe this ping-pong.
>
> kzm9g:
>
> [ 403.676197] ==== a3sp/ee140000.sd: start
> [ 403.764984] ==== a3sp/ee140000.sd: stop
> [ 404.716462] ==== a3sp/ee140000.sd: start
> [ 404.805208] ==== a3sp/ee140000.sd: stop
> [ 405.756708] ==== a3sp/ee140000.sd: start
> [ 405.836766] ==== a3sp/ee140000.sd: stop
> ...
>
> Further s2ram cycles make no difference, as before.
>
> On R-Car M3-N, there is no such imbalance. This seems to happen
> on older SH/R-Mobile SoCs only.
I will send out a v2 of 2/2 addressing Wolframs comments, I would
appreciate if you could test that as well as I'm not able to reproduce
the same issue as you. I'm using the same configuration you sent me when
you helped me to get started with the APE6EVM.
--
Regards,
Niklas Söderlund
