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? Kind regards Uffe [1] https://lore.kernel.org/lkml/CAPDyKFpXfRqx4WLuiU6m=rgM9A=21KfDTuEb5TboaOC+w_hMwg@xxxxxxxxxxxxxx/
