Re: [PATCH v2 1/4] PM: device: Introduce platform_resources_managed flag

[Michal Wilczynski <m.wilczynski@samsung.com>]

On 4/15/25 18:42, Rafael J. Wysocki wrote:
> On Mon, Apr 14, 2025 at 8:53 PM Michal Wilczynski
> <m.wilczynski@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 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@samsung.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
>>
>>
>