On Fri, 25 Apr 2025 at 09:09, Maxime Ripard <mrip...@kernel.org> wrote: > > Hi, > > On Thu, Apr 24, 2025 at 06:51:00PM +0200, Ulf Hansson wrote: > > On Thu, 17 Apr 2025 at 18:19, Michal Wilczynski > > <m.wilczyn...@samsung.com> wrote: > > > On 4/16/25 16:48, Rafael J. Wysocki wrote: > > > > On Wed, Apr 16, 2025 at 3:32 PM Michal Wilczynski > > > > <m.wilczyn...@samsung.com> wrote: > > > >> > > > >> On 4/15/25 18:42, Rafael J. Wysocki wrote: > > > >>> On Mon, Apr 14, 2025 at 8:53 PM Michal Wilczynski > > > >>> <m.wilczyn...@samsung.com> wrote: > > > >>>> > > > >>>> Introduce a new dev_pm_info flag - platform_resources_managed, to > > > >>>> indicate whether platform PM resources such as clocks or resets are > > > >>>> managed externally (e.g. by a generic power domain driver) instead of > > > >>>> directly by the consumer device driver. > > > >>> > > > >>> I think that this is genpd-specific and so I don't think it belongs in > > > >>> struct dev_pm_info. > > > >>> > > > >>> There is dev->power.subsys_data->domain_data, why not use it for this? > > > >> > > > >> Hi Rafael, > > > >> > > > >> Thanks for the feedback. > > > >> > > > >> You're right — this behavior is specific to genpd, so embedding the > > > >> flag > > > >> directly in struct dev_pm_info may not be the best choice. Using > > > >> dev->power.subsys_data->domain_data makes more sense and avoids > > > >> bloating > > > >> the core PM structure. > > > >> > > > >>> > > > >>> Also, it should be documented way more comprehensively IMV. > > > >>> > > > >>> Who is supposed to set it and when? What does it mean when it is set? > > > >> > > > >> To clarify the intended usage, I would propose adding the following > > > >> explanation to the commit message: > > > >> > > > >> "This flag is intended to be set by a generic PM domain driver (e.g., > > > >> from within its attach_dev callback) to indicate that it will manage > > > >> platform specific runtime power management resources — such as clocks > > > >> and resets — on behalf of the consumer device. This implies a > > > >> delegation > > > >> of runtime PM control to the PM domain, typically implemented through > > > >> its start and stop callbacks. > > > >> > > > >> When this flag is set, the consumer driver (e.g., drm/imagination) can > > > >> check it and skip managing such resources in its runtime PM callbacks > > > >> (runtime_suspend, runtime_resume), avoiding conflicts or redundant > > > >> operations." > > > > > > > > This sounds good and I would also put it into a code comment somewhere. > > > > > > > > I guess you'll need helpers for setting and testing this flag, so > > > > their kerneldoc comments can be used for that. > > > > > > > >> This could also be included as a code comment near the flag definition > > > >> if you think that’s appropriate. > > > >> > > > >> Also, as discussed earlier with Maxime and Matt [1], this is not about > > > >> full "resource ownership," but more about delegating runtime control of > > > >> PM resources like clocks/resets to the genpd. That nuance may be worth > > > >> reflecting in the flag name as well, I would rename it to let's say > > > >> 'runtime_pm_platform_res_delegated', or more concise > > > >> 'runtime_pm_delegated'. > > > > > > > > Or just "rpm_delegated" I suppose. > > > > > > > > But if the genpd driver is going to set that flag, it will rather mean > > > > that this driver will now control the resources in question, so the > > > > driver should not attempt to manipulate them directly. Is my > > > > understanding correct? > > > > > > Yes, your understanding is correct — with one minor clarification. > > > > > > When the genpd driver sets the flag, it indicates that it will take over > > > control of the relevant PM resources in the context of runtime PM, i.e., > > > via its start() and stop() callbacks. As a result, the device driver > > > should not manipulate those resources from within its RUNTIME_PM_OPS > > > (e.g., runtime_suspend, runtime_resume) to avoid conflicts. > > > > > > However, outside of the runtime PM callbacks, the consumer device driver > > > may still access or use those resources if needed e.g for devfreq. > > > > > > > > > > > Assuming that it is correct, how is the device driver going to know > > > > which resources in particular are now controlled by the genpd driver? > > > > > > Good question — to allow finer-grained control, we could replace the > > > current single boolean flag with a u32 bitmask field. Each bit would > > > correspond to a specific category of platform managed resources. For > > > example: > > > > > > #define RPM_TAKEOVER_CLK BIT(0) > > > #define RPM_TAKEOVER_RESET BIT(1) > > > > > > This would allow a PM domain driver to selectively declare which > > > resources it is taking over and let the consumer driver query only the > > > relevant parts. > > > > Assuming we are targeting device specific resources for runtime PM; > > why would we want the driver to be responsible for some resources and > > the genpd provider for some others? I would assume we want to handle > > all these RPM-resources from the genpd provider, if/when possible, > > right? > > > > The tricky part though (maybe Stephen had some ideas in his talk [a] > > at OSS), is to teach the genpd provider about what resources it should > > handle. In principle the genpd provider will need some kind of device > > specific knowledge, perhaps based on the device's compatible-string > > and description in DT. > > > > My point is, using a bitmask doesn't scale as it would end up having > > one bit for each clock (a device may have multiple clocks), regulator, > > pinctrl, phy, etc. In principle, reflecting the description in DT. > > My understanding is that it's to address a situation where a "generic" > driver interacts with some platform specific code. I think it's tied to > the discussion with the imagination GPU driver handling his clocks, and > the platform genpd clocks overlapping a bit. > > But then, my question is: does it matter? clocks are refcounted, and > resets are as well iirc, so why do we need a transition at all? Can't we > just let the platform genpd code take a reference on the clock, the GPU > driver take one as well, and it's all good, right?
The problem is the power-sequencing that is needed to initialize the GPU. Have a look at patch3's commit message - and I think it will be clearer what is needed. In my last reply for patch 3/4, I am suggesting this whole thing may be better modeled as a real power-sequence, using the new subsystem in drivers/power/sequencing/* Kind regards Uffe
