Message-ID: <b9c4182d-38c2-4173-a35a-0e1773c8f2ed@samsung.com>
Date: Wed, 16 Apr 2025 15:32:35 +0200
From: Michal Wilczynski <m.wilczynski@...sung.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: 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>, Ulf Hansson
	<ulf.hansson@...aro.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>, Maxime Ripard
	<mripard@...nel.org>, 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 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 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'.

[1] - https://lore.kernel.org/all/a3142259-1c72-45b9-b148-5e5e6bef87f9@samsung.com/

> 
>> This flag enables device drivers to cooperate with SoC-specific PM
>> domains by conditionally skipping management of clocks and resets when
>> the platform owns them.
>>
>> This idea was discussed on the mailing list [1].
>>
>> [1] - https://lore.kernel.org/all/CAPDyKFq=BF5f2i_Sr1cmVqtVAMgr=0FqsksL7RHZLKn++y0uwg@mail.gmail.com/
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@...sung.com>
>> ---
>>  include/linux/device.h | 11 +++++++++++
>>  include/linux/pm.h     |  1 +
>>  2 files changed, 12 insertions(+)
>>
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 79e49fe494b7c4c70d902886db63c4cfe5b4de4f..3e7a36dd874cfb6b98e2451c7a876989aa9f1913 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -881,6 +881,17 @@ static inline bool device_async_suspend_enabled(struct device *dev)
>>         return !!dev->power.async_suspend;
>>  }
>>
>> +static inline bool device_platform_resources_pm_managed(struct device *dev)
> 
> Could this function name be shorter?

Maybe:

static inline bool dev_is_runtime_pm_delegated(struct device *dev);
static inline void dev_set_runtime_pm_delegated(struct device *dev, bool val);

Regards,
Michał

> 
>> +{
>> +       return dev->power.platform_resources_managed;
>> +}
>> +
>> +static inline void device_platform_resources_set_pm_managed(struct device *dev,
>> +                                                           bool val)
> 
> Ditto?
> 
>> +{
>> +       dev->power.platform_resources_managed = val;
>> +}
>> +
>>  static inline bool device_pm_not_required(struct device *dev)
>>  {
>>         return dev->power.no_pm;
>> diff --git a/include/linux/pm.h b/include/linux/pm.h
>> index f0bd8fbae4f2c09c63d780bb2528693acf2d2da1..cd6cb59686e4a5e9eaa2701d1e44af2abbfd88d1 100644
>> --- a/include/linux/pm.h
>> +++ b/include/linux/pm.h
>> @@ -670,6 +670,7 @@ struct dev_pm_info {
>>         bool                    no_pm:1;
>>         bool                    early_init:1;   /* Owned by the PM core */
>>         bool                    direct_complete:1;      /* Owned by the PM core */
>> +       bool                    platform_resources_managed:1;
>>         u32                     driver_flags;
>>         spinlock_t              lock;
>>  #ifdef CONFIG_PM_SLEEP
>>
>> --
>> 2.34.1
>>
>>
> 

Powered by blists - more mailing lists