Hello Ulf, On Tue, Apr 04, 2023 at 02:41:33PM +0200, Ulf Hansson wrote: > On Mon, 3 Apr 2023 at 17:17, Uwe Kleine-König > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote: > > > > On Mon, Apr 03, 2023 at 12:19:20PM +0200, Ulf Hansson wrote: > > > On Sat, 1 Apr 2023 at 15:16, Uwe Kleine-König > > > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote: > > > > > > > > Hello, > > > > > > > > I looked at the sdhci_am654's probe function and concerning runtime-pm > > > > this is at least unconvential: > > > > > > > > It does > > > > > > > > clk_xin = devm_clk_get(dev, "clk_xin"); > > > > if (IS_ERR(clk_xin)) > > > > ... > > > > > > > > /* Clocks are enabled using pm_runtime */ > > > > pm_runtime_enable(dev); > > > > ret = pm_runtime_resume_and_get(dev); > > > > ... > > > > > > > > I'm not fluent in runtime-pm stuff (so I added Rafael to Cc), but I > > > > thought it is to be used the other way around, i.e. put the device in > > > > operational state and then runtime-pm cares to suspend the device under > > > > some conditions (e.g. CONFIG_PM being enabled). > > > > > > > > With CONFIG_PM unset the driver is broken for sure, as then > > > > pm_runtime_enable() and pm_runtime_resume_and_get() have no effect. So > > > > the clk stays off. > > > > > > In principle, you are correct. I wouldn't recommend the above pattern > > > in general, but it doesn't mean that it can't work. > > > > > > Some platforms are selecting "PM" from some of their toplevel Kconfig, > > > as they simply can't work without it. That means that the code you > > > refer to above, doesn't have to be broken. > > > > If I understand correctly the driver under discussion is supposed to be > > used on SoCs in the CONFIG_ARCH_K3 family. This isn't one of those that > > enforce PM. > > Right. Thanks for checking this. > > In fact, I have just queued up a patch that fixes the behaviour [1]. > Maybe we should consider to backport it for stable kernels too, at > least if there are some reports about errors!? > > > But yes, adding a depends on PM would be an action that improves the > > situation. Having said that even on such platforms you can disable > > runtime PM, so even a select on PM isn't a complete fix. > > Note sure I understand this. In what way do you mean that runtime PM > can be disabled? Normally, it's the bus/driver that decide whether > runtime PM should be enabled or not. Right? You can disable runtime PM per device via sysfs. (Something like: echo on > /sys/devices/.../power/control .) The docs say "Changing this attribute to "on" [...] while the device is suspended causes it to be woken up.", so the right thing seems to happen. (I didn't test what happens if that file is written before the driver is bound.) Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
