On Wed, 2 Nov 2022 at 04:16, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote: > > Quoting Doug Anderson (2022-11-01 17:45:03) > > > > One small nit is that the kernel doc for "@dev" in "struct gdsc" is > > incorrect after your patch. It still says this even though we're not > > using it for pm_runtime calls anymore: > > > > * @dev: the device holding the GDSC, used for pm_runtime calls > > Good catch! I can remove the part after the comma. > > > > > Other than that, this seems OK to me. I don't feel like I have a lot > > of good intuition around PM Clocks and genpd and all the topics talked > > about here, but I tried to look at the diff from before all the > > "recent" patches to "drivers/clk/qcom/gdsc.c" till the state after > > your patch. In other words the combined diff of these 4 patches: > > > > clk: qcom: gdsc: Remove direct runtime PM calls > > clk: qcom: gdsc: add missing error handling > > clk: qcom: gdsc: Bump parent usage count when GDSC is found enabled > > clk: qcom: gdsc: enable optional power domain support > > > > That basically shows a combined change that does two things: > > > > a) Adds error handling if pm_genpd_init() returns an error. > > > > b) Says that if "scs[i]->parent" wasn't provided that we can imply a > > parent from "dev->pm_domain". > > > > That seems to make sense, but one thing I'm wondering about for "b)" > > is how you know that "dev->pm_domain" can be safely upcast to a genpd. > > In other words, I'm hesitant about the "pd_to_genpd(dev->pm_domain)" > > call. I'll assume that "dev->pm_domain" isn't 100% guaranteed to be a > > genpd or else (presumably) we would have stored a genpd. Is there > > something about the "dev" that's passed in with "struct gdsc_desc" > > that gives the stronger guarantee about this being a genpd? > > Not really any stronger guarantee. The guarantee is pretty strong > already though. You can look at the callers of dev_pm_domain_set() and > see that nothing is calling that really besides the genpd attachment > logic when a driver is bound to a device (follow dev_pm_domain_attach() > from platform_probe()). The dev->pm_domain is going to be assigned to a > genpd assuming the 'dev' pointer is a platform device and has > 'power-domains' in DT. > > It's not great, but it works for now. Certainly if we ever want to > replace the pm_domain with something that isn't a genpd then we'll be in > trouble. I'm not sure it will ever happen. Ulf, can you provide more > assurances here? I think the call to pd_to_genpd() should be considered as safe, as long as the call is made in a controlled way from within a genpd provider. However, in some cases, we want to pick up a genpd from the dev->pm_domain, that isn't a genpd provider. Internally in genpd we use dev_to_genpd_safe(). Is that something that would be valuable to use here? If so, I don't see any issues with exporting that as a new genpd helper function. [...] Kind regards Uffe
