On Tue, 4 Apr 2023 at 17:02, Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> wrote: > > 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.) Right, that part is changing the reference count. It doesn't really enable/disable runtime PM for the device, so it shouldn't really matter. Unless I am missing something, of course. Kind regards Uffe
