Message-ID: <CAPDyKFp-Bguqukn0my9mVDdSyG2eQ3EPP+diD-BBg-P_E8S9=A@mail.gmail.com>
Date: Fri, 25 Apr 2025 12:10:45 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Maxime Ripard <mripard@...nel.org>
Cc: Michal Wilczynski <m.wilczynski@...sung.com>, Stephen Boyd <sboyd@...nel.org>, 
	"Rafael J. Wysocki" <rafael@...nel.org>, Danilo Krummrich <dakr@...nel.org>, Pavel Machek <pavel@...nel.org>, 
	Drew Fustini <drew@...7.com>, Guo Ren <guoren@...nel.org>, Fu Wei <wefu@...hat.com>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Philipp Zabel <p.zabel@...gutronix.de>, Frank Binns <frank.binns@...tec.com>, 
	Matt Coster <matt.coster@...tec.com>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, m.szyprowski@...sung.com, 
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org, 
	linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 1/4] PM: device: Introduce platform_resources_managed flag

On Fri, 25 Apr 2025 at 09:09, Maxime Ripard <mripard@...nel.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.wilczynski@...sung.com> wrote:
> > > On 4/16/25 16:48, Rafael J. Wysocki wrote:
> > > > On Wed, Apr 16, 2025 at 3:32 PM Michal Wilczynski
> > > > <m.wilczynski@...sung.com> wrote:
> > > >>
> > > >> On 4/15/25 18:42, Rafael J. Wysocki wrote:
> > > >>> On Mon, Apr 14, 2025 at 8:53 PM Michal Wilczynski
> > > >>> <m.wilczynski@...sung.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

Powered by blists - more mailing lists