On Fri, 13 Sep 2019 at 08:37, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi all,
>
> On Thu, Sep 12, 2019 at 10:04 PM Niklas Soderlund
> <niklas.soderlund@xxxxxxxxxxxx> wrote:
> > On 2019-09-12 19:05:47 +0100, Wolfram Sang wrote:
> > > On Wed, Sep 11, 2019 at 04:16:56PM +0200, Ulf Hansson wrote:
> > > > On Mon, 9 Sep 2019 at 12:46, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> > > > >
> > > > > During probe, tmio variant drivers calls pm_runtime_enable() before they
> > > > > call tmio_mmc_host_probe(). This doesn't work as expected, because
> > > > > tmio_mmc_host_probe() calls pm_runtime_set_active(), which fails to set the
> > > > > status to RPM_ACTIVE for the device, when its been enabled for runtime PM.
> > > > >
> > > > > Fix this by calling pm_runtime_enable() from tmio_mmc_host_probe() instead.
> > > > > To avoid the device from being runtime suspended during the probe phase,
> > > > > let's also increase the runtime PM usage count in tmio_mmc_host_probe().
> > > > > Consequentially, each tmio variant driver can decide themselves when to
> > > > > call pm_runtime_put(), to allow the device to become runtime suspended.
> > > > >
> > > > > Additionally, if the tmio variant driver decided to call pm_runtime_put()
> > > > > during probe, it's is expected that it also calls pm_runtime_get_sync() to
> > > > > restore the usage count, before it calls tmio_mmc_host_remove().
> > > > >
> > > > > Fixes: 7ff213193310 ("mmc: tmio: move runtime PM enablement to the driver implementations")
> > > > > Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > > >
> > > > So I decided to apply this for my fixes branch, as to get it tested
> > > > for a few days.
> > > >
> > > > If you have any objections, please tell.
> > >
> > > Sadly, I can't test until next week because I am still on the road. Yet,
> > > I recall Niklas said at LPC that the patch looks good to him, at least.
> > >
> >
> > Yes I think it looks good and was planing to test it. Unfortunately I'm
> > also on the road until the end of next week ;-(
>
> So I decided to give it a try on my boards. Note that apart from eMMC,
> I do not have any SD cards inserted.
Thanks for testing!
>
> The first thing I noticed, on R-Car Gen2/Gen3:
>
> sh_mobile_sdhi ee100000.sd: using device tree for GPIO lookup
> of_get_named_gpiod_flags: parsed 'cd-gpios' property of node
> '/soc/sd@ee100000[0]' - status (0)
> gpio-812 (cd): gpiod_request: status -16
> -PM: ==== always-on/ee100000.sd: start
> gpio_rcar e6055400.gpio: sense irq = 6, type = 3
> sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 max clock rate 195 MHz
> sh_mobile_sdhi ee140000.sd: adding to PM domain always-on
> @@ -964,7 +961,6 @@
> sh_mobile_sdhi ee140000.sd: using device tree for GPIO lookup
> of_get_named_gpiod_flags: parsed 'cd-gpios' property of node
> '/soc/sd@ee140000[0]' - status (0)
> gpio-820 (cd): gpiod_request: status -16
> -PM: ==== always-on/ee140000.sd: start
> gpio_rcar e6055400.gpio: sense irq = 14, type = 3
> sh_mobile_sdhi ee140000.sd: mmc1 base at 0xee140000 max clock rate 97 MHz
> sh_mobile_sdhi ee160000.sd: adding to PM domain always-on
> @@ -984,7 +980,6 @@
> sh_mobile_sdhi ee160000.sd: using device tree for GPIO lookup
> of_get_named_gpiod_flags: parsed 'cd-gpios' property of node
> '/soc/sd@ee160000[0]' - status (0)
> gpio-828 (cd): gpiod_request: status -16
> -PM: ==== always-on/ee160000.sd: start
> gpio_rcar e6055400.gpio: sense irq = 22, type = 3
> sh_mobile_sdhi ee160000.sd: mmc2 base at 0xee160000 max clock rate 97 MHz
>
> The "PM: ==== always-on/ee100000.sd: start" are from debug prints
> in genpd_start_dev(). Likewise, I have a similar print in genpd_stop_dev().
>
> So the device is no longer started? Or does it starts in started state?
Right. That is the behaviour I expect at this point. In other words,
more fixes on top is needed.
I assume the above problem is what Niklas tried to fix in commit
7ff213193310 ("mmc: tmio: move runtime PM enablement to the driver
implementations"). However, that commit introduced even more problems.
For example, in case the device was not having genpd attached, there
was no problem, but after commit 7ff213193310, there is always a clock
imbalance problem.
Hmm, maybe we should simply revert Niklas patch and make a proper fix
on top instead, as that would mean we can tag that fix for stable more
easily.
>
> On SH/R-Mobile, the same happens:
>
> sh_mobile_sdhi ee100000.sd: adding to PM domain a3sp
> PM: genpd_add_device: Add ee100000.sd to a3sp
> -PM: ==== a3sp/ee100000.sd: start
> sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 max clock rate 88 MHz
> sh_mobile_sdhi ee120000.sd: adding to PM domain a3sp
> PM: genpd_add_device: Add ee120000.sd to a3sp
> -PM: ==== a3sp/ee120000.sd: start
> sh_mobile_sdhi ee120000.sd: mmc1 base at 0xee120000 max clock rate 12 MHz
>
> But later:
>
> +PM: ==== a3sp/ee120000.sd: stop
> ...
> +PM: ==== a3sp/ee120000.sd: start
>
> So it is stopped without being started first, and restarted later.
> This fuels the theory that the device starts in started state.
> Is that expected?
See above.
>
> On R-Car Gen2/Gen3, and some instances on SH/R-Mobile, clk_summary
> has changed, compared to before this patch:
> -clk_summary: sdhi0 3 3 0
> 97500000 0 0 50000
> +clk_summary: sdhi0 1 2 0
> 97500000 0 0 50000
>
> On other instances on SH/R-Mobile:
> -clk_summary: sdhi1 3 3 0
> 12500000 0 0 50000
> +clk_summary: sdhi1 2 2 0
> 12500000 0 0 50000
>
> After s2ram, clk_summary has changed, like:
>
> -clk_summary: sdhi0 1 2 0
> 97500000 0 0 50000
> +clk_summary: sdhi0 2 2 0
> 97500000 0 0 50000
>
> Recently I started tracking regulator_summary changes, too.
> S2ram also impacts regulator usecounts, but that may be a pre-existing issue,
> as I didn't compare them before:
>
> -regulator_summary: fixed-1.8V 2 1 0
> unknown 1800mV 0mA 1800mV 1800mV
> -regulator_summary: ee140000.sd 1
> 0mA 1800mV 1950mV
> -regulator_summary: fixed-3.3V 2 1 0
> unknown 3300mV 0mA 3300mV 3300mV
> -regulator_summary: ee140000.sd 1
> 0mA 3300mV 3400mV
> +regulator_summary: fixed-1.8V 1 1 0
> unknown 1800mV 0mA 1800mV 1800mV
> +regulator_summary: ee140000.sd 0
> 0mA 1800mV 1950mV
> +regulator_summary: fixed-3.3V 1 1 0
> unknown 3300mV 0mA 3300mV 3300mV
> +regulator_summary: ee140000.sd 0
> 0mA 3300mV 3400mV
>
> Regardless, the eMMC on Salvator-XS is still working.
>
> Does this makes sense?
Thanks for providing the details. In regards to regulators, I would
expect all of them to become disabled when the system suspends, then
re-enabled during system resume.
Let's have a look at that in the next steps though and fix the probe
problems first. I can post a patch or two in an hour or so, have you
the possibility to test this today?
Kind regards
Uffe
