| Message ID | 20250414-apr_14_for_sending-v2-1-70c5af2af96c@samsung.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/4] PM: device: Introduce platform_resources_managed flag | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | success | Success |
| bjorn/build-rv32-defconfig | success | build-rv32-defconfig |
| bjorn/build-rv64-clang-allmodconfig | success | build-rv64-clang-allmodconfig |
| bjorn/build-rv64-gcc-allmodconfig | success | build-rv64-gcc-allmodconfig |
| bjorn/build-rv64-nommu-k210-defconfig | success | build-rv64-nommu-k210-defconfig |
| bjorn/build-rv64-nommu-k210-virt | success | build-rv64-nommu-k210-virt |
| bjorn/checkpatch | success | checkpatch |
| bjorn/dtb-warn-rv64 | success | dtb-warn-rv64 |
| bjorn/header-inline | success | header-inline |
| bjorn/kdoc | success | kdoc |
| bjorn/module-param | success | module-param |
| bjorn/verify-fixes | success | verify-fixes |
| bjorn/verify-signedoff | success | verify-signedoff |
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? 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? > 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? > +{ > + 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 > >
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 >> >> >
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) +{ + return dev->power.platform_resources_managed; +} + +static inline void device_platform_resources_set_pm_managed(struct device *dev, + bool val) +{ + 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
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. 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(+)