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.
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?
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?
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!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
