[PATCH v2 0/4] Add GPU clock/reset management for TH1520 in genpd

[PATCH v2 0/4] Add GPU clock/reset management for TH1520 in genpd

[Michal Wilczynski]

This small patch series adds clock and reset management for the GPU in
the T-HEAD TH1520 SoC through the generic power domain (genpd)
framework.

The TH1520 GPU requires a special sequence involving multiple clocks and
resets to safely bring it up. Coordinating this sequence properly is
necessary for correct GPU operation. Following discussions on the
mailing list with kernel maintainers [1], the recommended approach is to
model this complexity inside a power domain driver, keeping SoC specific
details out of the GPU driver, clock framework, and reset framework.

This series consists of four patches:
- Patch 1 introduces a new dev_pm_info flag to allow device drivers
  to detect when platform PM domains manage clocks or resets
- Patch 2 updates the AON firmware bindings to describe the GPU clkgen
  reset
- Patch 3 extends the TH1520 PM domain driver to handle GPU-specific
  clock and reset sequencing at runtime, using genpd start/stop and
  attach/detach callbacks
- Patch 4 updates the Imagination DRM driver to skip direct clock
  management when platform PM ownership is detected

This approach aligns with recent efforts to treat PM domains as
SoC-specific power management drivers, as presented at OSSEU 2024 [2].

This patchset continues the work started in bigger series [3] by moving
the GPU initialization sequence for the TH1520 SoC into a generic PM
domain driver, specifically handling clock and reset management as part
of GPU bring-up.

v2:

Extended the series by adding two new commits:
 - introduced a new platform_resources_managed flag in dev_pm_info along
   with helper functions, allowing drivers to detect when clocks and resets
   are managed by the platform
 - updated the DRM Imagination driver to skip claiming clocks when
   platform_resources_managed is set

Split the original bindings update:
 - the AON firmware bindings now only add the GPU clkgen reset (the GPU
   core reset remains handled by the GPU node)

Reworked the TH1520 PM domain driver to:
 - acquire GPU clocks and reset dynamically using attach_dev/detach_dev
   callbacks
 - handle clkgen reset internally, while GPU core reset is obtained from
   the consumer device node
 - added a check to enforce that only a single device can be attached to
   the GPU PM domain

[1] - https://lore.kernel.org/all/CAPDyKFqsJaTrF0tBSY-TjpqdVt5=6aPQHYfnDebtphfRZSU=-Q@mail.gmail.com/
[2] - https://osseu2024.sched.com/event/1ej38/the-case-for-an-soc-power-management-driver-stephen-boyd-google
[3] - https://lore.kernel.org/all/20250219140239.1378758-1-m.wilczynski@samsung.com/

---
Michal Wilczynski (4):
      PM: device: Introduce platform_resources_managed flag
      dt-bindings: firmware: thead,th1520: Add resets for GPU clkgen
      pmdomain: thead: Add GPU-specific clock and reset handling for TH1520
      drm/imagination: Skip clocks if platform PM manages resources

 .../bindings/firmware/thead,th1520-aon.yaml        |  11 ++
 drivers/gpu/drm/imagination/pvr_device.c           |  14 +-
 drivers/pmdomain/thead/th1520-pm-domains.c         | 199 +++++++++++++++++++++
 include/linux/device.h                             |  11 ++
 include/linux/pm.h                                 |   1 +
 5 files changed, 232 insertions(+), 4 deletions(-)
---
base-commit: 7c89da246c1268c8dbfc1c7f1edc55aabce45b43
change-id: 20250414-apr_14_for_sending-5b3917817acc

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>

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

[Michal Wilczynski]

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(+)

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

-- 
2.34.1

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

[Rafael J. Wysocki]

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
>
>

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

[Michal Wilczynski]

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
>>
>>
> 

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

[Rafael J. Wysocki]

On Wed, Apr 16, 2025 at 3:32 PM Michal Wilczynski
<m.wilczynski@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.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 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?

Assuming that it is correct, how is the device driver going to know
which resources in particular are now controlled by the genpd driver?

Also "rpm_takeover" may be a better name for the flag in that case.

> [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);

What about

dev_pm_genpd_rpm_delegated()
dev_pm_genpd_set_rpm_delegated()

respectively (or analogously with the "takeover" variant)?

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

[Michal Wilczynski]

On 4/16/25 16:48, Rafael J. Wysocki wrote:
> On Wed, Apr 16, 2025 at 3:32 PM Michal Wilczynski
> <m.wilczynski@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.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 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.

> 
> Also "rpm_takeover" may be a better name for the flag in that case.

Sounds good, bitmask could also be named like that

> 
>> [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);
> 
> What about
> 
> dev_pm_genpd_rpm_delegated()
> dev_pm_genpd_set_rpm_delegated()
> 
> respectively (or analogously with the "takeover" variant)?

Right sounds good, so if you also like a bitmask idea they could look
like this:

static inline bool dev_pm_genpd_rpm_takeover(struct device *dev, u32 flags);
static inline void dev_pm_genpd_set_rpm_takeover(struct device *dev, u32 flags);

Regards,
Michał

> 

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

[Ulf Hansson]

+ Stephen

On Thu, 17 Apr 2025 at 18:19, Michal Wilczynski
<m.wilczynski@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.wilczynski@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.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 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.

>
> >
> > Also "rpm_takeover" may be a better name for the flag in that case.
>
> Sounds good, bitmask could also be named like that
>
> >
> >> [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);
> >
> > What about
> >
> > dev_pm_genpd_rpm_delegated()
> > dev_pm_genpd_set_rpm_delegated()
> >
> > respectively (or analogously with the "takeover" variant)?
>
> Right sounds good, so if you also like a bitmask idea they could look
> like this:
>
> static inline bool dev_pm_genpd_rpm_takeover(struct device *dev, u32 flags);
> static inline void dev_pm_genpd_set_rpm_takeover(struct device *dev, u32 flags);

These names sound fine to me. Although, as stated, I am not sure how
useful the 'flags' would really be.

>
> Regards,
> Michał
>
> >

[a] - https://osseu2024.sched.com/event/1ej38/the-case-for-an-soc-power-management-driver-stephen-boyd-google

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6057 bytes --]

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@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.wilczynski@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.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 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?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Ulf Hansson]

On Fri, 25 Apr 2025 at 09:09, Maxime Ripard <mripard@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.wilczynski@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.wilczynski@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.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 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

[PATCH v2 2/4] dt-bindings: firmware: thead,th1520: Add resets for GPU clkgen

[Michal Wilczynski]

Extend the TH1520 AON firmware bindings to describe the GPU clkgen reset
line, required for proper GPU clock and reset sequencing.

The T-HEAD TH1520 GPU requires coordinated management of two clocks
(core and sys) and two resets (GPU core reset and GPU clkgen
reset).  Only the clkgen reset is exposed at the AON level, to support
SoC-specific initialization handled through a generic PM domain. The GPU
core reset remains described in the GPU device node, as from the GPU
driver's perspective, there is only a single reset line [1].

This follows upstream maintainers' recommendations [2] to abstract
SoC specific details into the PM domain layer rather than exposing them
to drivers directly.

[1] - https://lore.kernel.org/all/816db99d-7088-4c1a-af03-b9a825ac09dc@imgtec.com/
[2] - https://lore.kernel.org/all/38d9650fc11a674c8b689d6bab937acf@kernel.org/

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 .../devicetree/bindings/firmware/thead,th1520-aon.yaml        | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
index bbc183200400de7aadbb21fea21911f6f4227b09..6ea3029c222df9ba6ea7d423b92ba248cfb02cc0 100644
--- a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
+++ b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
@@ -32,6 +32,13 @@ properties:
     items:
       - const: aon
 
+  resets:
+    maxItems: 1
+
+  reset-names:
+    items:
+      - const: gpu-clkgen
+
   "#power-domain-cells":
     const: 1
 
@@ -39,6 +46,8 @@ required:
   - compatible
   - mboxes
   - mbox-names
+  - resets
+  - reset-names
   - "#power-domain-cells"
 
 additionalProperties: false
@@ -49,5 +58,7 @@ examples:
         compatible = "thead,th1520-aon";
         mboxes = <&mbox_910t 1>;
         mbox-names = "aon";
+        resets = <&rst 0>;
+        reset-names = "gpu-clkgen";
         #power-domain-cells = <1>;
     };

-- 
2.34.1

Re: [PATCH v2 2/4] dt-bindings: firmware: thead,th1520: Add resets for GPU clkgen

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2560 bytes --]

On Mon, Apr 14, 2025 at 08:52:56PM +0200, Michal Wilczynski wrote:
> Extend the TH1520 AON firmware bindings to describe the GPU clkgen reset
> line, required for proper GPU clock and reset sequencing.
> 
> The T-HEAD TH1520 GPU requires coordinated management of two clocks
> (core and sys) and two resets (GPU core reset and GPU clkgen
> reset).  Only the clkgen reset is exposed at the AON level, to support
> SoC-specific initialization handled through a generic PM domain. The GPU
> core reset remains described in the GPU device node, as from the GPU
> driver's perspective, there is only a single reset line [1].
> 
> This follows upstream maintainers' recommendations [2] to abstract
> SoC specific details into the PM domain layer rather than exposing them
> to drivers directly.
> 
> [1] - https://lore.kernel.org/all/816db99d-7088-4c1a-af03-b9a825ac09dc@imgtec.com/
> [2] - https://lore.kernel.org/all/38d9650fc11a674c8b689d6bab937acf@kernel.org/
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../devicetree/bindings/firmware/thead,th1520-aon.yaml        | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> index bbc183200400de7aadbb21fea21911f6f4227b09..6ea3029c222df9ba6ea7d423b92ba248cfb02cc0 100644
> --- a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> +++ b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> @@ -32,6 +32,13 @@ properties:
>      items:
>        - const: aon
>  
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    items:
> +      - const: gpu-clkgen
> +
>    "#power-domain-cells":
>      const: 1
>  
> @@ -39,6 +46,8 @@ required:
>    - compatible
>    - mboxes
>    - mbox-names
> +  - resets
> +  - reset-names

Given these are new required properties, have you made sure in the
driver that their absence will not cause problems with older
devicetrees? I took a brief look at the driver, and it _looked_ like you
were failing if they were not there? It was a brief look though, tbf.

>    - "#power-domain-cells"
>  
>  additionalProperties: false
> @@ -49,5 +58,7 @@ examples:
>          compatible = "thead,th1520-aon";
>          mboxes = <&mbox_910t 1>;
>          mbox-names = "aon";
> +        resets = <&rst 0>;
> +        reset-names = "gpu-clkgen";
>          #power-domain-cells = <1>;
>      };
> 
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 2/4] dt-bindings: firmware: thead,th1520: Add resets for GPU clkgen

[Michal Wilczynski]

On 4/15/25 18:38, Conor Dooley wrote:
> On Mon, Apr 14, 2025 at 08:52:56PM +0200, Michal Wilczynski wrote:
>> Extend the TH1520 AON firmware bindings to describe the GPU clkgen reset
>> line, required for proper GPU clock and reset sequencing.
>>
>> The T-HEAD TH1520 GPU requires coordinated management of two clocks
>> (core and sys) and two resets (GPU core reset and GPU clkgen
>> reset).  Only the clkgen reset is exposed at the AON level, to support
>> SoC-specific initialization handled through a generic PM domain. The GPU
>> core reset remains described in the GPU device node, as from the GPU
>> driver's perspective, there is only a single reset line [1].
>>
>> This follows upstream maintainers' recommendations [2] to abstract
>> SoC specific details into the PM domain layer rather than exposing them
>> to drivers directly.
>>
>> [1] - https://lore.kernel.org/all/816db99d-7088-4c1a-af03-b9a825ac09dc@imgtec.com/
>> [2] - https://lore.kernel.org/all/38d9650fc11a674c8b689d6bab937acf@kernel.org/
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  .../devicetree/bindings/firmware/thead,th1520-aon.yaml        | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
>> index bbc183200400de7aadbb21fea21911f6f4227b09..6ea3029c222df9ba6ea7d423b92ba248cfb02cc0 100644
>> --- a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
>> +++ b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
>> @@ -32,6 +32,13 @@ properties:
>>      items:
>>        - const: aon
>>  
>> +  resets:
>> +    maxItems: 1
>> +
>> +  reset-names:
>> +    items:
>> +      - const: gpu-clkgen
>> +
>>    "#power-domain-cells":
>>      const: 1
>>  
>> @@ -39,6 +46,8 @@ required:
>>    - compatible
>>    - mboxes
>>    - mbox-names
>> +  - resets
>> +  - reset-names
> 
> Given these are new required properties, have you made sure in the
> driver that their absence will not cause problems with older
> devicetrees? I took a brief look at the driver, and it _looked_ like you
> were failing if they were not there? It was a brief look though, tbf.

Hi Conor,

Good point — but in this case, the devicetrees compatible with the
driver haven’t been merged upstream yet. In fact, the TH1520 PM domains
driver currently doesn’t even compile against mainline, since the
required commit [1] didn’t make it into 6.15.

That said, Drew has queued the DT changes for the next release [2], and
you’ve queued [1], so assuming this series lands in 6.16, there won’t be
any older devicetrees to support. As a result, I haven’t added a
fallback path in the driver for missing properties.

If, however this series doesn’t make it in for 6.16, then yes — we’d
need to revisit the driver and add a failure safe path for cases where
these properties aren’t present.

Thanks,
Michał

[1] - https://lore.kernel.org/all/20250407-synergy-staff-b1cec90ffe72@spud/
[2] - https://lore.kernel.org/all/Z%2F6p6MQDS8ZlQv5r@x1/

> 
>>    - "#power-domain-cells"
>>  
>>  additionalProperties: false
>> @@ -49,5 +58,7 @@ examples:
>>          compatible = "thead,th1520-aon";
>>          mboxes = <&mbox_910t 1>;
>>          mbox-names = "aon";
>> +        resets = <&rst 0>;
>> +        reset-names = "gpu-clkgen";
>>          #power-domain-cells = <1>;
>>      };
>>
>> -- 
>> 2.34.1
>>

Re: [PATCH v2 2/4] dt-bindings: firmware: thead,th1520: Add resets for GPU clkgen

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3307 bytes --]

On Wed, Apr 16, 2025 at 01:40:15PM +0200, Michal Wilczynski wrote:
> 
> 
> On 4/15/25 18:38, Conor Dooley wrote:
> > On Mon, Apr 14, 2025 at 08:52:56PM +0200, Michal Wilczynski wrote:
> >> Extend the TH1520 AON firmware bindings to describe the GPU clkgen reset
> >> line, required for proper GPU clock and reset sequencing.
> >>
> >> The T-HEAD TH1520 GPU requires coordinated management of two clocks
> >> (core and sys) and two resets (GPU core reset and GPU clkgen
> >> reset).  Only the clkgen reset is exposed at the AON level, to support
> >> SoC-specific initialization handled through a generic PM domain. The GPU
> >> core reset remains described in the GPU device node, as from the GPU
> >> driver's perspective, there is only a single reset line [1].
> >>
> >> This follows upstream maintainers' recommendations [2] to abstract
> >> SoC specific details into the PM domain layer rather than exposing them
> >> to drivers directly.
> >>
> >> [1] - https://lore.kernel.org/all/816db99d-7088-4c1a-af03-b9a825ac09dc@imgtec.com/
> >> [2] - https://lore.kernel.org/all/38d9650fc11a674c8b689d6bab937acf@kernel.org/
> >>
> >> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> >> ---
> >>  .../devicetree/bindings/firmware/thead,th1520-aon.yaml        | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> >> index bbc183200400de7aadbb21fea21911f6f4227b09..6ea3029c222df9ba6ea7d423b92ba248cfb02cc0 100644
> >> --- a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> >> +++ b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> >> @@ -32,6 +32,13 @@ properties:
> >>      items:
> >>        - const: aon
> >>  
> >> +  resets:
> >> +    maxItems: 1
> >> +
> >> +  reset-names:
> >> +    items:
> >> +      - const: gpu-clkgen
> >> +
> >>    "#power-domain-cells":
> >>      const: 1
> >>  
> >> @@ -39,6 +46,8 @@ required:
> >>    - compatible
> >>    - mboxes
> >>    - mbox-names
> >> +  - resets
> >> +  - reset-names
> > 
> > Given these are new required properties, have you made sure in the
> > driver that their absence will not cause problems with older
> > devicetrees? I took a brief look at the driver, and it _looked_ like you
> > were failing if they were not there? It was a brief look though, tbf.
> 
> Hi Conor,
> 
> Good point — but in this case, the devicetrees compatible with the
> driver haven’t been merged upstream yet. In fact, the TH1520 PM domains
> driver currently doesn’t even compile against mainline, since the
> required commit [1] didn’t make it into 6.15.
> 
> That said, Drew has queued the DT changes for the next release [2], and
> you’ve queued [1], so assuming this series lands in 6.16, there won’t be
> any older devicetrees to support. As a result, I haven’t added a
> fallback path in the driver for missing properties.
> 
> If, however this series doesn’t make it in for 6.16, then yes — we’d
> need to revisit the driver and add a failure safe path for cases where
> these properties aren’t present.

Can you point this reason out in the commit message please?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v2 3/4] pmdomain: thead: Add GPU-specific clock and reset handling for TH1520

[Michal Wilczynski]

Extend the TH1520 power domain driver to manage GPU related clocks and
resets via generic PM domain start/stop callbacks.

The TH1520 GPU requires a special sequence to correctly initialize:
- Enable the GPU clocks
- Deassert the GPU clkgen reset
- Delay for a few cycles to satisfy hardware requirements
- Deassert the GPU core reset

This sequence is SoC-specific and must be abstracted away from the
Imagination GPU driver, which expects only a standard single reset
interface. Following discussions with kernel maintainers [1], this
logic is placed inside a PM domain, rather than polluting the clock or
reset frameworks, or the GPU driver itself.

To support this, the TH1520 PM domain implements `attach_dev` and
`detach_dev` callbacks, allowing it to dynamically acquire clock and
reset resources from the GPU device tree node at runtime. This allows to
maintain the separation between generic drivers and SoC-specific
integration logic.

As a result, the PM domain not only handles power sequencing but also
effectively acts as the SoC specific "glue driver" for the GPU device,
encapsulating all TH1520-specific clock and reset management.

This approach improves maintainability and aligns with the broader
direction of treating PM domains as lightweight SoC-specific power
management drivers [2].

[1] - https://lore.kernel.org/all/CAPDyKFqsJaTrF0tBSY-TjpqdVt5=6aPQHYfnDebtphfRZSU=-Q@mail.gmail.com/
[2] - https://osseu2024.sched.com/event/1ej38/the-case-for-an-soc-power-management-driver-stephen-boyd-google

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 drivers/pmdomain/thead/th1520-pm-domains.c | 199 +++++++++++++++++++++++++++++
 1 file changed, 199 insertions(+)

diff --git a/drivers/pmdomain/thead/th1520-pm-domains.c b/drivers/pmdomain/thead/th1520-pm-domains.c
index f702e20306f469aeb0ed15e54bd4f8309f28018c..75412efb195eb534c2e8ff10ced65ed4c4d2452c 100644
--- a/drivers/pmdomain/thead/th1520-pm-domains.c
+++ b/drivers/pmdomain/thead/th1520-pm-domains.c
@@ -5,10 +5,13 @@
  * Author: Michal Wilczynski <m.wilczynski@samsung.com>
  */
 
+#include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/firmware/thead/thead,th1520-aon.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
 #include <linux/pm_domain.h>
+#include <linux/reset.h>
 
 #include <dt-bindings/power/thead,th1520-power.h>
 
@@ -16,6 +19,15 @@ struct th1520_power_domain {
 	struct th1520_aon_chan *aon_chan;
 	struct generic_pm_domain genpd;
 	u32 rsrc;
+
+	/* PM-owned reset */
+	struct reset_control *clkgen_reset;
+
+	/* Device-specific resources */
+	struct device *attached_dev;
+	struct clk_bulk_data *clks;
+	int num_clks;
+	struct reset_control *gpu_reset;
 };
 
 struct th1520_power_info {
@@ -61,6 +73,177 @@ static int th1520_pd_power_off(struct generic_pm_domain *domain)
 	return th1520_aon_power_update(pd->aon_chan, pd->rsrc, false);
 }
 
+static int th1520_gpu_init_consumer_clocks(struct device *dev,
+					   struct th1520_power_domain *pd)
+{
+	static const char *const clk_names[] = { "core", "sys" };
+	int i, ret;
+
+	pd->num_clks = ARRAY_SIZE(clk_names);
+	pd->clks = devm_kcalloc(dev, pd->num_clks, sizeof(*pd->clks), GFP_KERNEL);
+	if (!pd->clks)
+		return -ENOMEM;
+
+	for (i = 0; i < pd->num_clks; i++)
+		pd->clks[i].id = clk_names[i];
+
+	ret = devm_clk_bulk_get(dev, pd->num_clks, pd->clks);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get GPU clocks\n");
+
+	return 0;
+}
+
+static int th1520_gpu_init_consumer_reset(struct device *dev,
+					  struct th1520_power_domain *pd)
+{
+	int ret;
+
+	pd->gpu_reset = reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(pd->gpu_reset)) {
+		ret = PTR_ERR(pd->gpu_reset);
+		pd->gpu_reset = NULL;
+		return dev_err_probe(dev, ret, "Failed to get GPU reset\n");
+	}
+
+	return 0;
+}
+
+static int th1520_gpu_init_pm_reset(struct device *dev,
+				    struct th1520_power_domain *pd)
+{
+	pd->clkgen_reset = devm_reset_control_get_exclusive(dev, "gpu-clkgen");
+	if (IS_ERR(pd->clkgen_reset))
+		return dev_err_probe(dev, PTR_ERR(pd->clkgen_reset),
+				     "Failed to get GPU clkgen reset\n");
+
+	return 0;
+}
+
+static int th1520_gpu_domain_attach_dev(struct generic_pm_domain *genpd,
+					struct device *dev)
+{
+	struct th1520_power_domain *pd = to_th1520_power_domain(genpd);
+	int ret;
+
+	/* Enforce 1:1 mapping - only one device can be attached. */
+	if (pd->attached_dev)
+		return -EBUSY;
+
+	/* Initialize clocks using the consumer device */
+	ret = th1520_gpu_init_consumer_clocks(dev, pd);
+	if (ret)
+		return ret;
+
+	/* Initialize consumer reset using the consumer device */
+	ret = th1520_gpu_init_consumer_reset(dev, pd);
+	if (ret) {
+		if (pd->clks) {
+			clk_bulk_put(pd->num_clks, pd->clks);
+			kfree(pd->clks);
+			pd->clks = NULL;
+			pd->num_clks = 0;
+		}
+		return ret;
+	}
+
+	/* Mark device as platform PM driver managed */
+	device_platform_resources_set_pm_managed(dev, true);
+	pd->attached_dev = dev;
+
+	return 0;
+}
+
+static void th1520_gpu_domain_detach_dev(struct generic_pm_domain *genpd,
+					 struct device *dev)
+{
+	struct th1520_power_domain *pd = to_th1520_power_domain(genpd);
+
+	/* Ensure this is the device we have attached */
+	if (pd->attached_dev != dev) {
+		dev_warn(dev,
+			 "tried to detach from GPU domain but not attached\n");
+		return;
+	}
+
+	/* Remove PM managed flag when detaching */
+	device_platform_resources_set_pm_managed(dev, false);
+
+	/* Clean up the consumer-owned resources */
+	if (pd->gpu_reset) {
+		reset_control_put(pd->gpu_reset);
+		pd->gpu_reset = NULL;
+	}
+
+	if (pd->clks) {
+		clk_bulk_put(pd->num_clks, pd->clks);
+		kfree(pd->clks);
+		pd->clks = NULL;
+		pd->num_clks = 0;
+	}
+
+	pd->attached_dev = NULL;
+}
+
+static int th1520_gpu_domain_start(struct device *dev)
+{
+	struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
+	struct th1520_power_domain *pd = to_th1520_power_domain(genpd);
+	int ret;
+
+	/* Check if we have all required resources */
+	if (pd->attached_dev != dev || !pd->clks || !pd->gpu_reset ||
+	    !pd->clkgen_reset)
+		return -ENODEV;
+
+	ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
+	if (ret)
+		return ret;
+
+	ret = reset_control_deassert(pd->clkgen_reset);
+	if (ret)
+		goto err_disable_clks;
+
+	/*
+	 * According to the hardware manual, a delay of at least 32 clock
+	 * cycles is required between de-asserting the clkgen reset and
+	 * de-asserting the GPU reset. Assuming a worst-case scenario with
+	 * a very high GPU clock frequency, a delay of 1 microsecond is
+	 * sufficient to ensure this requirement is met across all
+	 * feasible GPU clock speeds.
+	 */
+	udelay(1);
+
+	ret = reset_control_deassert(pd->gpu_reset);
+	if (ret)
+		goto err_assert_clkgen;
+
+	return 0;
+
+err_assert_clkgen:
+	reset_control_assert(pd->clkgen_reset);
+err_disable_clks:
+	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
+	return ret;
+}
+
+static int th1520_gpu_domain_stop(struct device *dev)
+{
+	struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
+	struct th1520_power_domain *pd = to_th1520_power_domain(genpd);
+
+	/* Check if we have all required resources and if this is the attached device */
+	if (pd->attached_dev != dev || !pd->clks || !pd->gpu_reset ||
+	    !pd->clkgen_reset)
+		return -ENODEV;
+
+	reset_control_assert(pd->gpu_reset);
+	reset_control_assert(pd->clkgen_reset);
+	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
+
+	return 0;
+}
+
 static struct generic_pm_domain *th1520_pd_xlate(const struct of_phandle_args *spec,
 						 void *data)
 {
@@ -99,6 +282,22 @@ th1520_add_pm_domain(struct device *dev, const struct th1520_power_info *pi)
 	pd->genpd.power_off = th1520_pd_power_off;
 	pd->genpd.name = pi->name;
 
+	/* there are special callbacks for the GPU */
+	if (pi == &th1520_pd_ranges[TH1520_GPU_PD]) {
+		/* Initialize the PM-owned reset */
+		ret = th1520_gpu_init_pm_reset(dev, pd);
+		if (ret)
+			return ERR_PTR(ret);
+
+		/* No device attached yet */
+		pd->attached_dev = NULL;
+
+		pd->genpd.dev_ops.start = th1520_gpu_domain_start;
+		pd->genpd.dev_ops.stop = th1520_gpu_domain_stop;
+		pd->genpd.attach_dev = th1520_gpu_domain_attach_dev;
+		pd->genpd.detach_dev = th1520_gpu_domain_detach_dev;
+	}
+
 	ret = pm_genpd_init(&pd->genpd, NULL, true);
 	if (ret)
 		return ERR_PTR(ret);

-- 
2.34.1

Re: [PATCH v2 3/4] pmdomain: thead: Add GPU-specific clock and reset handling for TH1520

[Ulf Hansson]

+ Bartosz

On Mon, 14 Apr 2025 at 20:53, Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
>
> Extend the TH1520 power domain driver to manage GPU related clocks and
> resets via generic PM domain start/stop callbacks.
>
> The TH1520 GPU requires a special sequence to correctly initialize:
> - Enable the GPU clocks
> - Deassert the GPU clkgen reset
> - Delay for a few cycles to satisfy hardware requirements
> - Deassert the GPU core reset
>
> This sequence is SoC-specific and must be abstracted away from the
> Imagination GPU driver, which expects only a standard single reset
> interface. Following discussions with kernel maintainers [1], this
> logic is placed inside a PM domain, rather than polluting the clock or
> reset frameworks, or the GPU driver itself.

Speaking about special sequences for power-on/off devices like this
one, that's a known common problem. We actually have a generic
subsystem for this now, drivers/power/sequencing/*.

Perhaps it's worth having a look at that, it should allow us to
abstract things, so the GPU driver can stay more portable.

Kind regards
Uffe

>
> To support this, the TH1520 PM domain implements `attach_dev` and
> `detach_dev` callbacks, allowing it to dynamically acquire clock and
> reset resources from the GPU device tree node at runtime. This allows to
> maintain the separation between generic drivers and SoC-specific
> integration logic.
>
> As a result, the PM domain not only handles power sequencing but also
> effectively acts as the SoC specific "glue driver" for the GPU device,
> encapsulating all TH1520-specific clock and reset management.
>
> This approach improves maintainability and aligns with the broader
> direction of treating PM domains as lightweight SoC-specific power
> management drivers [2].
>
> [1] - https://lore.kernel.org/all/CAPDyKFqsJaTrF0tBSY-TjpqdVt5=6aPQHYfnDebtphfRZSU=-Q@mail.gmail.com/
> [2] - https://osseu2024.sched.com/event/1ej38/the-case-for-an-soc-power-management-driver-stephen-boyd-google
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  drivers/pmdomain/thead/th1520-pm-domains.c | 199 +++++++++++++++++++++++++++++
>  1 file changed, 199 insertions(+)
>
> diff --git a/drivers/pmdomain/thead/th1520-pm-domains.c b/drivers/pmdomain/thead/th1520-pm-domains.c
> index f702e20306f469aeb0ed15e54bd4f8309f28018c..75412efb195eb534c2e8ff10ced65ed4c4d2452c 100644
> --- a/drivers/pmdomain/thead/th1520-pm-domains.c
> +++ b/drivers/pmdomain/thead/th1520-pm-domains.c
> @@ -5,10 +5,13 @@
>   * Author: Michal Wilczynski <m.wilczynski@samsung.com>
>   */
>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/firmware/thead/thead,th1520-aon.h>
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_domain.h>
> +#include <linux/reset.h>
>
>  #include <dt-bindings/power/thead,th1520-power.h>
>
> @@ -16,6 +19,15 @@ struct th1520_power_domain {
>         struct th1520_aon_chan *aon_chan;
>         struct generic_pm_domain genpd;
>         u32 rsrc;
> +
> +       /* PM-owned reset */
> +       struct reset_control *clkgen_reset;
> +
> +       /* Device-specific resources */
> +       struct device *attached_dev;
> +       struct clk_bulk_data *clks;
> +       int num_clks;
> +       struct reset_control *gpu_reset;
>  };
>
>  struct th1520_power_info {
> @@ -61,6 +73,177 @@ static int th1520_pd_power_off(struct generic_pm_domain *domain)
>         return th1520_aon_power_update(pd->aon_chan, pd->rsrc, false);
>  }
>
> +static int th1520_gpu_init_consumer_clocks(struct device *dev,
> +                                          struct th1520_power_domain *pd)
> +{
> +       static const char *const clk_names[] = { "core", "sys" };
> +       int i, ret;
> +
> +       pd->num_clks = ARRAY_SIZE(clk_names);
> +       pd->clks = devm_kcalloc(dev, pd->num_clks, sizeof(*pd->clks), GFP_KERNEL);
> +       if (!pd->clks)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < pd->num_clks; i++)
> +               pd->clks[i].id = clk_names[i];
> +
> +       ret = devm_clk_bulk_get(dev, pd->num_clks, pd->clks);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to get GPU clocks\n");
> +
> +       return 0;
> +}
> +
> +static int th1520_gpu_init_consumer_reset(struct device *dev,
> +                                         struct th1520_power_domain *pd)
> +{
> +       int ret;
> +
> +       pd->gpu_reset = reset_control_get_exclusive(dev, NULL);
> +       if (IS_ERR(pd->gpu_reset)) {
> +               ret = PTR_ERR(pd->gpu_reset);
> +               pd->gpu_reset = NULL;
> +               return dev_err_probe(dev, ret, "Failed to get GPU reset\n");
> +       }
> +
> +       return 0;
> +}
> +
> +static int th1520_gpu_init_pm_reset(struct device *dev,
> +                                   struct th1520_power_domain *pd)
> +{
> +       pd->clkgen_reset = devm_reset_control_get_exclusive(dev, "gpu-clkgen");
> +       if (IS_ERR(pd->clkgen_reset))
> +               return dev_err_probe(dev, PTR_ERR(pd->clkgen_reset),
> +                                    "Failed to get GPU clkgen reset\n");
> +
> +       return 0;
> +}
> +
> +static int th1520_gpu_domain_attach_dev(struct generic_pm_domain *genpd,
> +                                       struct device *dev)
> +{
> +       struct th1520_power_domain *pd = to_th1520_power_domain(genpd);
> +       int ret;
> +
> +       /* Enforce 1:1 mapping - only one device can be attached. */
> +       if (pd->attached_dev)
> +               return -EBUSY;
> +
> +       /* Initialize clocks using the consumer device */
> +       ret = th1520_gpu_init_consumer_clocks(dev, pd);
> +       if (ret)
> +               return ret;
> +
> +       /* Initialize consumer reset using the consumer device */
> +       ret = th1520_gpu_init_consumer_reset(dev, pd);
> +       if (ret) {
> +               if (pd->clks) {
> +                       clk_bulk_put(pd->num_clks, pd->clks);
> +                       kfree(pd->clks);
> +                       pd->clks = NULL;
> +                       pd->num_clks = 0;
> +               }
> +               return ret;
> +       }
> +
> +       /* Mark device as platform PM driver managed */
> +       device_platform_resources_set_pm_managed(dev, true);
> +       pd->attached_dev = dev;
> +
> +       return 0;
> +}
> +
> +static void th1520_gpu_domain_detach_dev(struct generic_pm_domain *genpd,
> +                                        struct device *dev)
> +{
> +       struct th1520_power_domain *pd = to_th1520_power_domain(genpd);
> +
> +       /* Ensure this is the device we have attached */
> +       if (pd->attached_dev != dev) {
> +               dev_warn(dev,
> +                        "tried to detach from GPU domain but not attached\n");
> +               return;
> +       }
> +
> +       /* Remove PM managed flag when detaching */
> +       device_platform_resources_set_pm_managed(dev, false);
> +
> +       /* Clean up the consumer-owned resources */
> +       if (pd->gpu_reset) {
> +               reset_control_put(pd->gpu_reset);
> +               pd->gpu_reset = NULL;
> +       }
> +
> +       if (pd->clks) {
> +               clk_bulk_put(pd->num_clks, pd->clks);
> +               kfree(pd->clks);
> +               pd->clks = NULL;
> +               pd->num_clks = 0;
> +       }
> +
> +       pd->attached_dev = NULL;
> +}
> +
> +static int th1520_gpu_domain_start(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> +       struct th1520_power_domain *pd = to_th1520_power_domain(genpd);
> +       int ret;
> +
> +       /* Check if we have all required resources */
> +       if (pd->attached_dev != dev || !pd->clks || !pd->gpu_reset ||
> +           !pd->clkgen_reset)
> +               return -ENODEV;
> +
> +       ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
> +       if (ret)
> +               return ret;
> +
> +       ret = reset_control_deassert(pd->clkgen_reset);
> +       if (ret)
> +               goto err_disable_clks;
> +
> +       /*
> +        * According to the hardware manual, a delay of at least 32 clock
> +        * cycles is required between de-asserting the clkgen reset and
> +        * de-asserting the GPU reset. Assuming a worst-case scenario with
> +        * a very high GPU clock frequency, a delay of 1 microsecond is
> +        * sufficient to ensure this requirement is met across all
> +        * feasible GPU clock speeds.
> +        */
> +       udelay(1);
> +
> +       ret = reset_control_deassert(pd->gpu_reset);
> +       if (ret)
> +               goto err_assert_clkgen;
> +
> +       return 0;
> +
> +err_assert_clkgen:
> +       reset_control_assert(pd->clkgen_reset);
> +err_disable_clks:
> +       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
> +       return ret;
> +}
> +
> +static int th1520_gpu_domain_stop(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> +       struct th1520_power_domain *pd = to_th1520_power_domain(genpd);
> +
> +       /* Check if we have all required resources and if this is the attached device */
> +       if (pd->attached_dev != dev || !pd->clks || !pd->gpu_reset ||
> +           !pd->clkgen_reset)
> +               return -ENODEV;
> +
> +       reset_control_assert(pd->gpu_reset);
> +       reset_control_assert(pd->clkgen_reset);
> +       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
> +
> +       return 0;
> +}
> +
>  static struct generic_pm_domain *th1520_pd_xlate(const struct of_phandle_args *spec,
>                                                  void *data)
>  {
> @@ -99,6 +282,22 @@ th1520_add_pm_domain(struct device *dev, const struct th1520_power_info *pi)
>         pd->genpd.power_off = th1520_pd_power_off;
>         pd->genpd.name = pi->name;
>
> +       /* there are special callbacks for the GPU */
> +       if (pi == &th1520_pd_ranges[TH1520_GPU_PD]) {
> +               /* Initialize the PM-owned reset */
> +               ret = th1520_gpu_init_pm_reset(dev, pd);
> +               if (ret)
> +                       return ERR_PTR(ret);
> +
> +               /* No device attached yet */
> +               pd->attached_dev = NULL;
> +
> +               pd->genpd.dev_ops.start = th1520_gpu_domain_start;
> +               pd->genpd.dev_ops.stop = th1520_gpu_domain_stop;
> +               pd->genpd.attach_dev = th1520_gpu_domain_attach_dev;
> +               pd->genpd.detach_dev = th1520_gpu_domain_detach_dev;
> +       }
> +
>         ret = pm_genpd_init(&pd->genpd, NULL, true);
>         if (ret)
>                 return ERR_PTR(ret);
>
> --
> 2.34.1
>

Re: [PATCH v2 3/4] pmdomain: thead: Add GPU-specific clock and reset handling for TH1520

[Michal Wilczynski]

On 4/25/25 10:50, Ulf Hansson wrote:
> + Bartosz
> 
> On Mon, 14 Apr 2025 at 20:53, Michal Wilczynski
> <m.wilczynski@samsung.com> wrote:
>>
>> Extend the TH1520 power domain driver to manage GPU related clocks and
>> resets via generic PM domain start/stop callbacks.
>>
>> The TH1520 GPU requires a special sequence to correctly initialize:
>> - Enable the GPU clocks
>> - Deassert the GPU clkgen reset
>> - Delay for a few cycles to satisfy hardware requirements
>> - Deassert the GPU core reset
>>
>> This sequence is SoC-specific and must be abstracted away from the
>> Imagination GPU driver, which expects only a standard single reset
>> interface. Following discussions with kernel maintainers [1], this
>> logic is placed inside a PM domain, rather than polluting the clock or
>> reset frameworks, or the GPU driver itself.
> 
> Speaking about special sequences for power-on/off devices like this
> one, that's a known common problem. We actually have a generic
> subsystem for this now, drivers/power/sequencing/*.
> 
> Perhaps it's worth having a look at that, it should allow us to
> abstract things, so the GPU driver can stay more portable.
> 
> Kind regards
> Uffe


Hi Ulf, Bartosz,

Thank you very much for your suggestion, Ulf. I took a look at the
drivers/power/sequencing/ API and agree that it seems like a suitable
framework to model the specific power-on/off sequence required for the
TH1520 GPU, allowing for better abstraction than embedding the logic
directly in genpd callbacks.

My plan is to refactor the series based on this approach. Here's how I
envision the implementation:

1) Provider (th1520-pm-domains.c): This driver will register as both a
generic power domain provider and a power sequencer provider for the GPU
domain.

2) pwrseq target Definition: A pwrseq target will be defined within the
provider to encapsulate the required sequence (enable clocks, deassert
clkgen reset, delay, deassert GPU core reset). The target will be
named using the GPU's first compatible string with a "-power" suffix.

Example GPU DT node, adhering to convention introduced here [1].
gpu: gpu@ffef400000 {
	compatible = "thead,th1520-gpu", "img,img-bxm-4-64",
	             "img,img-bxm", "img-rogue";
};

[1] - https://lore.kernel.org/all/20250410-sets-bxs-4-64-patch-v1-v6-1-eda620c5865f@imgtec.com/#t

3) Consumer (drm/imagination): In its probe function, the driver will
read the first compatible string of the device node. It will then
attempt devm_pwrseq_get(dev, compatible_string_with_suffix) (e.g.
devm_pwrseq_get(dev, "thead,th1520-gpu-power")). The result
pvr_dev->pwrseq_desc will be stored (it will be NULL if no suitable
provider/target is found, or a valid descriptor if successful). The
driver will still acquire its necessary clock/reset handles via
devm_*_get in probe for potential use outside of RPM (like devfreq).

4) Consumer Runtime PM Callbacks
(pvr_power_device_resume/suspend): These functions will check if
pvr_dev->pwrseq_desc is valid. If valid: Call pwrseq_power_on() in
resume and pwrseq_power_off() in suspend. The driver will not perform
its own clock/reset enabling/disabling for resources managed by the
sequence. If NULL: Execute the existing fallback logic (manually
enabling/disabling clocks/resets using the handles acquired in probe).
Unconditional logic (like FW booting/shutdown) will remain within the
RPM callbacks, executed after successful power on or before power off,
respectively.

5) Resource Handling (via genpd callbacks): To allow the provider
(th1520-pm-domains.c) to access resources defined in the consumer's DT
node (specifically the clocks and gpu_reset needed in the sequence), I
plan to keep the attach_dev / detach_dev genpd callbacks as implemented
in the previous patch version. attach_dev will acquire the consumer's
resources (using the consumer_dev pointer) and store the handles in the
provider's context. The pwrseq unit callbacks will then access these
stored handles via the shared context. detach_dev will release these
resources. This seems necessary as the pwrseq API itself doesn't
currently provide a direct hook for the provider to get the consumer's
device pointer or manage its resources across the pwrseq_get/put
lifecycle.

This approach uses the pwrseq framework for the sequence logic as
suggested, keeps the generic consumer driver free of SoC-specific
sequence details (by using the compatible string lookup for this), and
retains the genpd attach/detach mechanism to handle cross-node resource
dependencies.

Please let me know if this plan sounds reasonable.

Thanks !

> 
>>
>> To support this, the TH1520 PM domain implements `attach_dev` and
>> `detach_dev` callbacks, allowing it to dynamically acquire clock and
>> reset resources from the GPU device tree node at runtime. This allows to
>> maintain the separation between generic drivers and SoC-specific
>> integration logic.
>>
>> As a result, the PM domain not only handles power sequencing but also
>> effectively acts as the SoC specific "glue driver" for the GPU device,
>> encapsulating all TH1520-specific clock and reset management.
>>
>> This approach improves maintainability and aligns with the broader
>> direction of treating PM domains as lightweight SoC-specific power
>> management drivers [2].
>>
>> [1] - https://lore.kernel.org/all/CAPDyKFqsJaTrF0tBSY-TjpqdVt5=6aPQHYfnDebtphfRZSU=-Q@mail.gmail.com/
>> [2] - https://osseu2024.sched.com/event/1ej38/the-case-for-an-soc-power-management-driver-stephen-boyd-google
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  drivers/pmdomain/thead/th1520-pm-domains.c | 199 +++++++++++++++++++++++++++++
>>  1 file changed, 199 insertions(+)
>>
>> diff --git a/drivers/pmdomain/thead/th1520-pm-domains.c b/drivers/pmdomain/thead/th1520-pm-domains.c
>> index f702e20306f469aeb0ed15e54bd4f8309f28018c..75412efb195eb534c2e8ff10ced65ed4c4d2452c 100644
>> --- a/drivers/pmdomain/thead/th1520-pm-domains.c
>> +++ b/drivers/pmdomain/thead/th1520-pm-domains.c
>> @@ -5,10 +5,13 @@
>>   * Author: Michal Wilczynski <m.wilczynski@samsung.com>
>>   */
>>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>>  #include <linux/firmware/thead/thead,th1520-aon.h>
>>  #include <linux/slab.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/pm_domain.h>
>> +#include <linux/reset.h>
>>
>>  #include <dt-bindings/power/thead,th1520-power.h>
>>
>> @@ -16,6 +19,15 @@ struct th1520_power_domain {
>>         struct th1520_aon_chan *aon_chan;
>>         struct generic_pm_domain genpd;
>>         u32 rsrc;
>> +
>> +       /* PM-owned reset */
>> +       struct reset_control *clkgen_reset;
>> +
>> +       /* Device-specific resources */
>> +       struct device *attached_dev;
>> +       struct clk_bulk_data *clks;
>> +       int num_clks;
>> +       struct reset_control *gpu_reset;
>>  };
>>
>>  struct th1520_power_info {
>> @@ -61,6 +73,177 @@ static int th1520_pd_power_off(struct generic_pm_domain *domain)
>>         return th1520_aon_power_update(pd->aon_chan, pd->rsrc, false);
>>  }
>>
>> +static int th1520_gpu_init_consumer_clocks(struct device *dev,
>> +                                          struct th1520_power_domain *pd)
>> +{
>> +       static const char *const clk_names[] = { "core", "sys" };
>> +       int i, ret;
>> +
>> +       pd->num_clks = ARRAY_SIZE(clk_names);
>> +       pd->clks = devm_kcalloc(dev, pd->num_clks, sizeof(*pd->clks), GFP_KERNEL);
>> +       if (!pd->clks)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0; i < pd->num_clks; i++)
>> +               pd->clks[i].id = clk_names[i];
>> +
>> +       ret = devm_clk_bulk_get(dev, pd->num_clks, pd->clks);
>> +       if (ret)
>> +               return dev_err_probe(dev, ret, "Failed to get GPU clocks\n");
>> +
>> +       return 0;
>> +}
>> +
>> +static int th1520_gpu_init_consumer_reset(struct device *dev,
>> +                                         struct th1520_power_domain *pd)
>> +{
>> +       int ret;
>> +
>> +       pd->gpu_reset = reset_control_get_exclusive(dev, NULL);
>> +       if (IS_ERR(pd->gpu_reset)) {
>> +               ret = PTR_ERR(pd->gpu_reset);
>> +               pd->gpu_reset = NULL;
>> +               return dev_err_probe(dev, ret, "Failed to get GPU reset\n");
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int th1520_gpu_init_pm_reset(struct device *dev,
>> +                                   struct th1520_power_domain *pd)
>> +{
>> +       pd->clkgen_reset = devm_reset_control_get_exclusive(dev, "gpu-clkgen");
>> +       if (IS_ERR(pd->clkgen_reset))
>> +               return dev_err_probe(dev, PTR_ERR(pd->clkgen_reset),
>> +                                    "Failed to get GPU clkgen reset\n");
>> +
>> +       return 0;
>> +}
>> +
>> +static int th1520_gpu_domain_attach_dev(struct generic_pm_domain *genpd,
>> +                                       struct device *dev)
>> +{
>> +       struct th1520_power_domain *pd = to_th1520_power_domain(genpd);
>> +       int ret;
>> +
>> +       /* Enforce 1:1 mapping - only one device can be attached. */
>> +       if (pd->attached_dev)
>> +               return -EBUSY;
>> +
>> +       /* Initialize clocks using the consumer device */
>> +       ret = th1520_gpu_init_consumer_clocks(dev, pd);
>> +       if (ret)
>> +               return ret;
>> +
>> +       /* Initialize consumer reset using the consumer device */
>> +       ret = th1520_gpu_init_consumer_reset(dev, pd);
>> +       if (ret) {
>> +               if (pd->clks) {
>> +                       clk_bulk_put(pd->num_clks, pd->clks);
>> +                       kfree(pd->clks);
>> +                       pd->clks = NULL;
>> +                       pd->num_clks = 0;
>> +               }
>> +               return ret;
>> +       }
>> +
>> +       /* Mark device as platform PM driver managed */
>> +       device_platform_resources_set_pm_managed(dev, true);
>> +       pd->attached_dev = dev;
>> +
>> +       return 0;
>> +}
>> +
>> +static void th1520_gpu_domain_detach_dev(struct generic_pm_domain *genpd,
>> +                                        struct device *dev)
>> +{
>> +       struct th1520_power_domain *pd = to_th1520_power_domain(genpd);
>> +
>> +       /* Ensure this is the device we have attached */
>> +       if (pd->attached_dev != dev) {
>> +               dev_warn(dev,
>> +                        "tried to detach from GPU domain but not attached\n");
>> +               return;
>> +       }
>> +
>> +       /* Remove PM managed flag when detaching */
>> +       device_platform_resources_set_pm_managed(dev, false);
>> +
>> +       /* Clean up the consumer-owned resources */
>> +       if (pd->gpu_reset) {
>> +               reset_control_put(pd->gpu_reset);
>> +               pd->gpu_reset = NULL;
>> +       }
>> +
>> +       if (pd->clks) {
>> +               clk_bulk_put(pd->num_clks, pd->clks);
>> +               kfree(pd->clks);
>> +               pd->clks = NULL;
>> +               pd->num_clks = 0;
>> +       }
>> +
>> +       pd->attached_dev = NULL;
>> +}
>> +
>> +static int th1520_gpu_domain_start(struct device *dev)
>> +{
>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>> +       struct th1520_power_domain *pd = to_th1520_power_domain(genpd);
>> +       int ret;
>> +
>> +       /* Check if we have all required resources */
>> +       if (pd->attached_dev != dev || !pd->clks || !pd->gpu_reset ||
>> +           !pd->clkgen_reset)
>> +               return -ENODEV;
>> +
>> +       ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = reset_control_deassert(pd->clkgen_reset);
>> +       if (ret)
>> +               goto err_disable_clks;
>> +
>> +       /*
>> +        * According to the hardware manual, a delay of at least 32 clock
>> +        * cycles is required between de-asserting the clkgen reset and
>> +        * de-asserting the GPU reset. Assuming a worst-case scenario with
>> +        * a very high GPU clock frequency, a delay of 1 microsecond is
>> +        * sufficient to ensure this requirement is met across all
>> +        * feasible GPU clock speeds.
>> +        */
>> +       udelay(1);
>> +
>> +       ret = reset_control_deassert(pd->gpu_reset);
>> +       if (ret)
>> +               goto err_assert_clkgen;
>> +
>> +       return 0;
>> +
>> +err_assert_clkgen:
>> +       reset_control_assert(pd->clkgen_reset);
>> +err_disable_clks:
>> +       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>> +       return ret;
>> +}
>> +
>> +static int th1520_gpu_domain_stop(struct device *dev)
>> +{
>> +       struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
>> +       struct th1520_power_domain *pd = to_th1520_power_domain(genpd);
>> +
>> +       /* Check if we have all required resources and if this is the attached device */
>> +       if (pd->attached_dev != dev || !pd->clks || !pd->gpu_reset ||
>> +           !pd->clkgen_reset)
>> +               return -ENODEV;
>> +
>> +       reset_control_assert(pd->gpu_reset);
>> +       reset_control_assert(pd->clkgen_reset);
>> +       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>> +
>> +       return 0;
>> +}
>> +
>>  static struct generic_pm_domain *th1520_pd_xlate(const struct of_phandle_args *spec,
>>                                                  void *data)
>>  {
>> @@ -99,6 +282,22 @@ th1520_add_pm_domain(struct device *dev, const struct th1520_power_info *pi)
>>         pd->genpd.power_off = th1520_pd_power_off;
>>         pd->genpd.name = pi->name;
>>
>> +       /* there are special callbacks for the GPU */
>> +       if (pi == &th1520_pd_ranges[TH1520_GPU_PD]) {
>> +               /* Initialize the PM-owned reset */
>> +               ret = th1520_gpu_init_pm_reset(dev, pd);
>> +               if (ret)
>> +                       return ERR_PTR(ret);
>> +
>> +               /* No device attached yet */
>> +               pd->attached_dev = NULL;
>> +
>> +               pd->genpd.dev_ops.start = th1520_gpu_domain_start;
>> +               pd->genpd.dev_ops.stop = th1520_gpu_domain_stop;
>> +               pd->genpd.attach_dev = th1520_gpu_domain_attach_dev;
>> +               pd->genpd.detach_dev = th1520_gpu_domain_detach_dev;
>> +       }
>> +
>>         ret = pm_genpd_init(&pd->genpd, NULL, true);
>>         if (ret)
>>                 return ERR_PTR(ret);
>>
>> --
>> 2.34.1
>>
> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>

Re: [PATCH v2 3/4] pmdomain: thead: Add GPU-specific clock and reset handling for TH1520

[Ulf Hansson]

On Wed, 30 Apr 2025 at 14:17, Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
>
>
>
> On 4/25/25 10:50, Ulf Hansson wrote:
> > + Bartosz
> >
> > On Mon, 14 Apr 2025 at 20:53, Michal Wilczynski
> > <m.wilczynski@samsung.com> wrote:
> >>
> >> Extend the TH1520 power domain driver to manage GPU related clocks and
> >> resets via generic PM domain start/stop callbacks.
> >>
> >> The TH1520 GPU requires a special sequence to correctly initialize:
> >> - Enable the GPU clocks
> >> - Deassert the GPU clkgen reset
> >> - Delay for a few cycles to satisfy hardware requirements
> >> - Deassert the GPU core reset
> >>
> >> This sequence is SoC-specific and must be abstracted away from the
> >> Imagination GPU driver, which expects only a standard single reset
> >> interface. Following discussions with kernel maintainers [1], this
> >> logic is placed inside a PM domain, rather than polluting the clock or
> >> reset frameworks, or the GPU driver itself.
> >
> > Speaking about special sequences for power-on/off devices like this
> > one, that's a known common problem. We actually have a generic
> > subsystem for this now, drivers/power/sequencing/*.
> >
> > Perhaps it's worth having a look at that, it should allow us to
> > abstract things, so the GPU driver can stay more portable.
> >
> > Kind regards
> > Uffe
>
>
> Hi Ulf, Bartosz,
>
> Thank you very much for your suggestion, Ulf. I took a look at the
> drivers/power/sequencing/ API and agree that it seems like a suitable
> framework to model the specific power-on/off sequence required for the
> TH1520 GPU, allowing for better abstraction than embedding the logic
> directly in genpd callbacks.
>
> My plan is to refactor the series based on this approach. Here's how I
> envision the implementation:
>
> 1) Provider (th1520-pm-domains.c): This driver will register as both a
> generic power domain provider and a power sequencer provider for the GPU
> domain.
>
> 2) pwrseq target Definition: A pwrseq target will be defined within the
> provider to encapsulate the required sequence (enable clocks, deassert
> clkgen reset, delay, deassert GPU core reset). The target will be
> named using the GPU's first compatible string with a "-power" suffix.
>
> Example GPU DT node, adhering to convention introduced here [1].
> gpu: gpu@ffef400000 {
>         compatible = "thead,th1520-gpu", "img,img-bxm-4-64",
>                      "img,img-bxm", "img-rogue";
> };

I don't think the power-domain provider node is the correct place for
this, from DT point of view.

Instead, I would try to follow the same approach as
Documentation/devicetree/bindings/regulator/qcom,qca6390-pmu.yaml,
which uses a separate device-node to describe the pwrseq provider.

>
> [1] - https://lore.kernel.org/all/20250410-sets-bxs-4-64-patch-v1-v6-1-eda620c5865f@imgtec.com/#t
>
> 3) Consumer (drm/imagination): In its probe function, the driver will
> read the first compatible string of the device node. It will then
> attempt devm_pwrseq_get(dev, compatible_string_with_suffix) (e.g.
> devm_pwrseq_get(dev, "thead,th1520-gpu-power")). The result

Make sense, but we should probably use a more generic target-name,
such as "gpu-power" or something along those lines.

> pvr_dev->pwrseq_desc will be stored (it will be NULL if no suitable
> provider/target is found, or a valid descriptor if successful). The
> driver will still acquire its necessary clock/reset handles via
> devm_*_get in probe for potential use outside of RPM (like devfreq).
>
> 4) Consumer Runtime PM Callbacks
> (pvr_power_device_resume/suspend): These functions will check if
> pvr_dev->pwrseq_desc is valid. If valid: Call pwrseq_power_on() in
> resume and pwrseq_power_off() in suspend. The driver will not perform
> its own clock/reset enabling/disabling for resources managed by the
> sequence. If NULL: Execute the existing fallback logic (manually
> enabling/disabling clocks/resets using the handles acquired in probe).
> Unconditional logic (like FW booting/shutdown) will remain within the
> RPM callbacks, executed after successful power on or before power off,
> respectively.
>
> 5) Resource Handling (via genpd callbacks): To allow the provider
> (th1520-pm-domains.c) to access resources defined in the consumer's DT
> node (specifically the clocks and gpu_reset needed in the sequence), I
> plan to keep the attach_dev / detach_dev genpd callbacks as implemented
> in the previous patch version. attach_dev will acquire the consumer's
> resources (using the consumer_dev pointer) and store the handles in the
> provider's context. The pwrseq unit callbacks will then access these
> stored handles via the shared context. detach_dev will release these
> resources. This seems necessary as the pwrseq API itself doesn't
> currently provide a direct hook for the provider to get the consumer's
> device pointer or manage its resources across the pwrseq_get/put
> lifecycle.

If we have a separate node describing the pwrseq, as
Documentation/devicetree/bindings/regulator/qcom,qca6390-pmu.yaml,
it's should be rather easy to hook up and pwrseq driver for it too, as
drivers/power/sequencing/pwrseq-qcom-wcn.c does it.

>
> This approach uses the pwrseq framework for the sequence logic as
> suggested, keeps the generic consumer driver free of SoC-specific
> sequence details (by using the compatible string lookup for this), and
> retains the genpd attach/detach mechanism to handle cross-node resource
> dependencies.
>
> Please let me know if this plan sounds reasonable.
>
> Thanks !

[...]

That said, Bartosz is better with guidance around pwrseq
providers/consumers. The above is just my view of it - and I might
have missed something.

Kind regards
Uffe

[PATCH v2 4/4] drm/imagination: Skip clocks if platform PM manages resources

[Michal Wilczynski]

Update the Imagination PVR driver to skip clock management during
initialization if the platform PM has indicated that it manages platform
resources.

This is necessary for platforms like the T-HEAD TH1520, where the GPU's
clocks and resets are managed via a PM domain, and should not be
manipulated directly by the GPU driver.

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 drivers/gpu/drm/imagination/pvr_device.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
index 1704c0268589bdeb65fa6535f9ec63182b0a3e94..f40468b99cf14da418aeecde086f009695ff877c 100644
--- a/drivers/gpu/drm/imagination/pvr_device.c
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -504,10 +504,16 @@ pvr_device_init(struct pvr_device *pvr_dev)
 	if (err)
 		return err;
 
-	/* Enable and initialize clocks required for the device to operate. */
-	err = pvr_device_clk_init(pvr_dev);
-	if (err)
-		return err;
+	/*
+	 * Only initialize clocks if they are not managed by the platform's
+	 * PM domain.
+	 */
+	if (!device_platform_resources_pm_managed(dev)) {
+		/* Enable and initialize clocks required for the device to operate. */
+		err = pvr_device_clk_init(pvr_dev);
+		if (err)
+			return err;
+	}
 
 	/* Explicitly power the GPU so we can access control registers before the FW is booted. */
 	err = pm_runtime_resume_and_get(dev);

-- 
2.34.1

Re: [PATCH v2 4/4] drm/imagination: Skip clocks if platform PM manages resources

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1671 bytes --]

On Mon, Apr 14, 2025 at 08:52:58PM +0200, Michal Wilczynski wrote:
> Update the Imagination PVR driver to skip clock management during
> initialization if the platform PM has indicated that it manages platform
> resources.
> 
> This is necessary for platforms like the T-HEAD TH1520, where the GPU's
> clocks and resets are managed via a PM domain, and should not be
> manipulated directly by the GPU driver.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  drivers/gpu/drm/imagination/pvr_device.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
> index 1704c0268589bdeb65fa6535f9ec63182b0a3e94..f40468b99cf14da418aeecde086f009695ff877c 100644
> --- a/drivers/gpu/drm/imagination/pvr_device.c
> +++ b/drivers/gpu/drm/imagination/pvr_device.c
> @@ -504,10 +504,16 @@ pvr_device_init(struct pvr_device *pvr_dev)
>  	if (err)
>  		return err;
>  
> -	/* Enable and initialize clocks required for the device to operate. */
> -	err = pvr_device_clk_init(pvr_dev);
> -	if (err)
> -		return err;
> +	/*
> +	 * Only initialize clocks if they are not managed by the platform's
> +	 * PM domain.
> +	 */
> +	if (!device_platform_resources_pm_managed(dev)) {
> +		/* Enable and initialize clocks required for the device to operate. */
> +		err = pvr_device_clk_init(pvr_dev);
> +		if (err)
> +			return err;
> +	}

So, how does that work for devfreq? I can understand the rationale for
resets and the sys clock, but the core clock at least should really be
handled by the driver.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 4/4] drm/imagination: Skip clocks if platform PM manages resources

[Matt Coster]

[-- Attachment #1.1: Type: text/plain, Size: 2097 bytes --]

On 15/04/2025 09:55, Maxime Ripard wrote:
> On Mon, Apr 14, 2025 at 08:52:58PM +0200, Michal Wilczynski wrote:
>> Update the Imagination PVR driver to skip clock management during
>> initialization if the platform PM has indicated that it manages platform
>> resources.
>>
>> This is necessary for platforms like the T-HEAD TH1520, where the GPU's
>> clocks and resets are managed via a PM domain, and should not be
>> manipulated directly by the GPU driver.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  drivers/gpu/drm/imagination/pvr_device.c | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
>> index 1704c0268589bdeb65fa6535f9ec63182b0a3e94..f40468b99cf14da418aeecde086f009695ff877c 100644
>> --- a/drivers/gpu/drm/imagination/pvr_device.c
>> +++ b/drivers/gpu/drm/imagination/pvr_device.c
>> @@ -504,10 +504,16 @@ pvr_device_init(struct pvr_device *pvr_dev)
>>  	if (err)
>>  		return err;
>>  
>> -	/* Enable and initialize clocks required for the device to operate. */
>> -	err = pvr_device_clk_init(pvr_dev);
>> -	if (err)
>> -		return err;
>> +	/*
>> +	 * Only initialize clocks if they are not managed by the platform's
>> +	 * PM domain.
>> +	 */
>> +	if (!device_platform_resources_pm_managed(dev)) {
>> +		/* Enable and initialize clocks required for the device to operate. */
>> +		err = pvr_device_clk_init(pvr_dev);
>> +		if (err)
>> +			return err;
>> +	}
> 
> So, how does that work for devfreq? I can understand the rationale for
> resets and the sys clock, but the core clock at least should really be
> handled by the driver.

I agree, this feels a bit "all or nothing" to me. There's only one clock
on this platform that has issues, we can still control the other two
just fine.

I thought fixed clocks were the standard mechanism for exposing
non-controllable clocks to device drivers?

Cheers,
Matt

> 
> Maxime


-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH v2 4/4] drm/imagination: Skip clocks if platform PM manages resources

[Michal Wilczynski]

On 4/15/25 11:15, Matt Coster wrote:
> On 15/04/2025 09:55, Maxime Ripard wrote:
>> On Mon, Apr 14, 2025 at 08:52:58PM +0200, Michal Wilczynski wrote:
>>> Update the Imagination PVR driver to skip clock management during
>>> initialization if the platform PM has indicated that it manages platform
>>> resources.
>>>
>>> This is necessary for platforms like the T-HEAD TH1520, where the GPU's
>>> clocks and resets are managed via a PM domain, and should not be
>>> manipulated directly by the GPU driver.
>>>
>>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>>> ---
>>>  drivers/gpu/drm/imagination/pvr_device.c | 14 ++++++++++----
>>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
>>> index 1704c0268589bdeb65fa6535f9ec63182b0a3e94..f40468b99cf14da418aeecde086f009695ff877c 100644
>>> --- a/drivers/gpu/drm/imagination/pvr_device.c
>>> +++ b/drivers/gpu/drm/imagination/pvr_device.c
>>> @@ -504,10 +504,16 @@ pvr_device_init(struct pvr_device *pvr_dev)
>>>  	if (err)
>>>  		return err;
>>>  
>>> -	/* Enable and initialize clocks required for the device to operate. */
>>> -	err = pvr_device_clk_init(pvr_dev);
>>> -	if (err)
>>> -		return err;
>>> +	/*
>>> +	 * Only initialize clocks if they are not managed by the platform's
>>> +	 * PM domain.
>>> +	 */
>>> +	if (!device_platform_resources_pm_managed(dev)) {
>>> +		/* Enable and initialize clocks required for the device to operate. */
>>> +		err = pvr_device_clk_init(pvr_dev);
>>> +		if (err)
>>> +			return err;
>>> +	}
>>
>> So, how does that work for devfreq? I can understand the rationale for
>> resets and the sys clock, but the core clock at least should really be
>> handled by the driver.

Hi Maxime, Matt,

Thanks for the feedback.

This commit is trying to prevent the pvr RUNTIME_PM_OPS from controlling the
clocks or resets, as there is a custom start/stop sequence needed for
the TH1520 SoC coded in patch 3 of this series.

static const struct dev_pm_ops pvr_pm_ops = {
	RUNTIME_PM_OPS(pvr_power_device_suspend, pvr_power_device_resume, pvr_power_device_idle)
};

So, if the core clock needs to be used for other purposes like devfreq,
we could move the device_platform_resources_pm_managed() check to the
pvr_power_* functions instead. This would prevent the clocks and resets
from being managed in runtime PM in the consumer driver, while still
allowing the GPU driver to access and control clocks like the core clock
as needed for other purposes.

That way, clocks could be safely shared between the PM domain driver and the
device driver, with generic PM driver controlling the start/stop
sequence for reset and clocks. We would probably need to find a
better name for the flag then, to more clearly reflect that it's about
delegating clock/reset PM runtime control, rather than full resource
ownership.

> 
> I agree, this feels a bit "all or nothing" to me. There's only one clock
> on this platform that has issues, we can still control the other two
> just fine.
> 
> I thought fixed clocks were the standard mechanism for exposing
> non-controllable clocks to device drivers?

That's correct — and it's not really about the MEM clock at this point.
The main goal is to ensure the custom power-up sequence for the TH1520
SoC is followed. That sequence is implemented in
th1520_gpu_domain_start() in patch 3 of this series.

Regards,
Michał

> 
> Cheers,
> Matt
> 
>>
>> Maxime
> 
>