[PATCH v1 0/2] Add GPU clock/reset management for TH1520 in genpd

[PATCH v1 0/2] Add GPU clock/reset management for TH1520 in genpd

[Michal Wilczynski]

This small patch series adds clock and reset support 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 out of reset. 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.

The first patch updates the AON firmware bindings to describe the clocks
and resets that the PM domain needs to control.

The second patch implements GPU-specific handling inside the TH1520 PM domain
driver, using the genpd start/stop callbacks to sequence the clocks and resets
at runtime.

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.

[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 (2):
  dt-bindings: firmware: thead,th1520: Add clocks and resets
  pmdomain: thead: Add GPU-specific clock and reset handling for TH1520

 .../bindings/firmware/thead,th1520-aon.yaml   |  28 +++++
 drivers/pmdomain/thead/th1520-pm-domains.c    | 119 ++++++++++++++++++
 2 files changed, 147 insertions(+)

-- 
2.34.1

[PATCH v1 1/2] dt-bindings: firmware: thead,th1520: Add clocks and resets

[Michal Wilczynski]

Prepare for handling GPU clock and reset sequencing through a generic
power domain by adding clock and reset properties to the TH1520 AON
firmware bindings.

The T-HEAD TH1520 GPU requires coordinated management of two clocks
(core and sys) and two resets (GPU and GPU CLKGEN). Due to SoC-specific
requirements, the CLKGEN reset must be carefully managed alongside clock
enables to ensure proper GPU operation, as discussed on the mailing list
[1].

Since the coordination is now handled through a power domain, only the
programmable clocks (core and sys) are exposed. The GPU MEM clock is
ignored, as it is not controllable on the TH1520 SoC.

This approach follows upstream maintainers' recommendations [1] to
avoid SoC-specific details leaking into the GPU driver or clock/reset
frameworks directly.

[1] - https://lore.kernel.org/all/38d9650fc11a674c8b689d6bab937acf@kernel.org/

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

diff --git a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
index bbc183200400..8075874bcd6b 100644
--- a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
+++ b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
@@ -25,6 +25,16 @@ properties:
   compatible:
     const: thead,th1520-aon
 
+  clocks:
+    items:
+      - description: GPU core clock
+      - description: GPU sys clock
+
+  clock-names:
+    items:
+      - const: gpu-core
+      - const: gpu-sys
+
   mboxes:
     maxItems: 1
 
@@ -32,13 +42,27 @@ properties:
     items:
       - const: aon
 
+  resets:
+    items:
+      - description: GPU reset
+      - description: GPU CLKGEN reset
+
+  reset-names:
+    items:
+      - const: gpu
+      - const: gpu-clkgen
+
   "#power-domain-cells":
     const: 1
 
 required:
   - compatible
+  - clocks
+  - clock-names
   - mboxes
   - mbox-names
+  - resets
+  - reset-names
   - "#power-domain-cells"
 
 additionalProperties: false
@@ -47,7 +71,11 @@ examples:
   - |
     aon: aon {
         compatible = "thead,th1520-aon";
+        clocks = <&clk_vo 0>, <&clk_vo 1>;
+        clock-names = "gpu-core", "gpu-sys";
         mboxes = <&mbox_910t 1>;
         mbox-names = "aon";
+        resets = <&rst 0>, <&rst 1>;
+        reset-names = "gpu", "gpu-clkgen";
         #power-domain-cells = <1>;
     };
-- 
2.34.1

Re: [PATCH v1 1/2] dt-bindings: firmware: thead,th1520: Add clocks and resets

[Ulf Hansson]

On Wed, 9 Apr 2025 at 11:30, Michal Wilczynski <m.wilczynski@samsung.com> wrote:
>
> Prepare for handling GPU clock and reset sequencing through a generic
> power domain by adding clock and reset properties to the TH1520 AON
> firmware bindings.
>
> The T-HEAD TH1520 GPU requires coordinated management of two clocks
> (core and sys) and two resets (GPU and GPU CLKGEN). Due to SoC-specific
> requirements, the CLKGEN reset must be carefully managed alongside clock
> enables to ensure proper GPU operation, as discussed on the mailing list
> [1].
>
> Since the coordination is now handled through a power domain, only the
> programmable clocks (core and sys) are exposed. The GPU MEM clock is
> ignored, as it is not controllable on the TH1520 SoC.
>
> This approach follows upstream maintainers' recommendations [1] to
> avoid SoC-specific details leaking into the GPU driver or clock/reset
> frameworks directly.
>
> [1] - https://lore.kernel.org/all/38d9650fc11a674c8b689d6bab937acf@kernel.org/
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  .../bindings/firmware/thead,th1520-aon.yaml   | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> index bbc183200400..8075874bcd6b 100644
> --- a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> +++ b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> @@ -25,6 +25,16 @@ properties:
>    compatible:
>      const: thead,th1520-aon
>
> +  clocks:
> +    items:
> +      - description: GPU core clock
> +      - description: GPU sys clock
> +
> +  clock-names:
> +    items:
> +      - const: gpu-core
> +      - const: gpu-sys

These clocks don't look like they belong to the power-domain node, but
rather the GPU's node.

Or is this in fact the correct description of the HW?

> +
>    mboxes:
>      maxItems: 1
>
> @@ -32,13 +42,27 @@ properties:
>      items:
>        - const: aon
>
> +  resets:
> +    items:
> +      - description: GPU reset
> +      - description: GPU CLKGEN reset
> +
> +  reset-names:
> +    items:
> +      - const: gpu
> +      - const: gpu-clkgen
> +

Ditto for the reset.

>    "#power-domain-cells":
>      const: 1
>
>  required:
>    - compatible
> +  - clocks
> +  - clock-names
>    - mboxes
>    - mbox-names
> +  - resets
> +  - reset-names
>    - "#power-domain-cells"
>
>  additionalProperties: false
> @@ -47,7 +71,11 @@ examples:
>    - |
>      aon: aon {
>          compatible = "thead,th1520-aon";
> +        clocks = <&clk_vo 0>, <&clk_vo 1>;
> +        clock-names = "gpu-core", "gpu-sys";
>          mboxes = <&mbox_910t 1>;
>          mbox-names = "aon";
> +        resets = <&rst 0>, <&rst 1>;
> +        reset-names = "gpu", "gpu-clkgen";
>          #power-domain-cells = <1>;
>      };
> --
> 2.34.1
>

That said, it's still possible to make both the clocks and reset being
managed from the genpd provider. I will comment on that separately for
patch2.

Kind regards
Uffe

Re: [PATCH v1 1/2] dt-bindings: firmware: thead,th1520: Add clocks and resets

[Michal Wilczynski]

On 4/9/25 12:41, Ulf Hansson wrote:
> On Wed, 9 Apr 2025 at 11:30, Michal Wilczynski <m.wilczynski@samsung.com> wrote:
>>
>> Prepare for handling GPU clock and reset sequencing through a generic
>> power domain by adding clock and reset properties to the TH1520 AON
>> firmware bindings.
>>
>> The T-HEAD TH1520 GPU requires coordinated management of two clocks
>> (core and sys) and two resets (GPU and GPU CLKGEN). Due to SoC-specific
>> requirements, the CLKGEN reset must be carefully managed alongside clock
>> enables to ensure proper GPU operation, as discussed on the mailing list
>> [1].
>>
>> Since the coordination is now handled through a power domain, only the
>> programmable clocks (core and sys) are exposed. The GPU MEM clock is
>> ignored, as it is not controllable on the TH1520 SoC.
>>
>> This approach follows upstream maintainers' recommendations [1] to
>> avoid SoC-specific details leaking into the GPU driver or clock/reset
>> frameworks directly.
>>
>> [1] - https://lore.kernel.org/all/38d9650fc11a674c8b689d6bab937acf@kernel.org/
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  .../bindings/firmware/thead,th1520-aon.yaml   | 28 +++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
>> index bbc183200400..8075874bcd6b 100644
>> --- a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
>> +++ b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
>> @@ -25,6 +25,16 @@ properties:
>>    compatible:
>>      const: thead,th1520-aon
>>
>> +  clocks:
>> +    items:
>> +      - description: GPU core clock
>> +      - description: GPU sys clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: gpu-core
>> +      - const: gpu-sys
> 
> These clocks don't look like they belong to the power-domain node, but
> rather the GPU's node.
> 
> Or is this in fact the correct description of the HW?

Hi,
Thank you for your input. Based on my understanding of Stephen
presentation the power-domain layer could act as a middleware layer
(like ACPI) that could own resources. That being said it was also stated
that the proposed approach should work with already existing device
trees, which implies that the DT should remain as is.

So I could get the resources using attach_dev and detach_dev, but there
are two problems with that:

1) The GPU driver will try to manage clocks/reset on it's own using those functions
   if I provide non-stub working clocks and reset:
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 obviously I should invent a way to tell the drm/imagination driver to
NOT manage. One obvious way to do this is to introduce new flag to genpd.flags
called let's say GENPD_FLAG_EXCLUSIVE_CONTROL, which would tell the consumer
driver that the power management is being done only done from the PM
middleware driver.

2) The GPU node doesn't want to own the gpu-clkgen reset. In fact nobody
   seems to want to own it, even though theoretically it should be owned by
   the clk_vo as this would describe the hardware best (it's resetting the
   GPU clocks). But then it would be trickier to get it from the PM driver,
   making the code more complex and harder to understand. Nonetheless I
   think it would work.

If this sounds good to you I will work on the code.

Regards,
Michał

> 
>> +
>>    mboxes:
>>      maxItems: 1
>>
>> @@ -32,13 +42,27 @@ properties:
>>      items:
>>        - const: aon
>>
>> +  resets:
>> +    items:
>> +      - description: GPU reset
>> +      - description: GPU CLKGEN reset
>> +
>> +  reset-names:
>> +    items:
>> +      - const: gpu
>> +      - const: gpu-clkgen
>> +
> 
> Ditto for the reset.
> 
>>    "#power-domain-cells":
>>      const: 1
>>
>>  required:
>>    - compatible
>> +  - clocks
>> +  - clock-names
>>    - mboxes
>>    - mbox-names
>> +  - resets
>> +  - reset-names
>>    - "#power-domain-cells"
>>
>>  additionalProperties: false
>> @@ -47,7 +71,11 @@ examples:
>>    - |
>>      aon: aon {
>>          compatible = "thead,th1520-aon";
>> +        clocks = <&clk_vo 0>, <&clk_vo 1>;
>> +        clock-names = "gpu-core", "gpu-sys";
>>          mboxes = <&mbox_910t 1>;
>>          mbox-names = "aon";
>> +        resets = <&rst 0>, <&rst 1>;
>> +        reset-names = "gpu", "gpu-clkgen";
>>          #power-domain-cells = <1>;
>>      };
>> --
>> 2.34.1
>>
> 
> That said, it's still possible to make both the clocks and reset being
> managed from the genpd provider. I will comment on that separately for
> patch2.
> 
> Kind regards
> Uffe
> 

Re: [PATCH v1 1/2] dt-bindings: firmware: thead,th1520: Add clocks and resets

[Ulf Hansson]

On Thu, 10 Apr 2025 at 12:42, Michal Wilczynski
<m.wilczynski@samsung.com> wrote:
>
>
>
> On 4/9/25 12:41, Ulf Hansson wrote:
> > On Wed, 9 Apr 2025 at 11:30, Michal Wilczynski <m.wilczynski@samsung.com> wrote:
> >>
> >> Prepare for handling GPU clock and reset sequencing through a generic
> >> power domain by adding clock and reset properties to the TH1520 AON
> >> firmware bindings.
> >>
> >> The T-HEAD TH1520 GPU requires coordinated management of two clocks
> >> (core and sys) and two resets (GPU and GPU CLKGEN). Due to SoC-specific
> >> requirements, the CLKGEN reset must be carefully managed alongside clock
> >> enables to ensure proper GPU operation, as discussed on the mailing list
> >> [1].
> >>
> >> Since the coordination is now handled through a power domain, only the
> >> programmable clocks (core and sys) are exposed. The GPU MEM clock is
> >> ignored, as it is not controllable on the TH1520 SoC.
> >>
> >> This approach follows upstream maintainers' recommendations [1] to
> >> avoid SoC-specific details leaking into the GPU driver or clock/reset
> >> frameworks directly.
> >>
> >> [1] - https://lore.kernel.org/all/38d9650fc11a674c8b689d6bab937acf@kernel.org/
> >>
> >> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> >> ---
> >>  .../bindings/firmware/thead,th1520-aon.yaml   | 28 +++++++++++++++++++
> >>  1 file changed, 28 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> >> index bbc183200400..8075874bcd6b 100644
> >> --- a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> >> +++ b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
> >> @@ -25,6 +25,16 @@ properties:
> >>    compatible:
> >>      const: thead,th1520-aon
> >>
> >> +  clocks:
> >> +    items:
> >> +      - description: GPU core clock
> >> +      - description: GPU sys clock
> >> +
> >> +  clock-names:
> >> +    items:
> >> +      - const: gpu-core
> >> +      - const: gpu-sys
> >
> > These clocks don't look like they belong to the power-domain node, but
> > rather the GPU's node.
> >
> > Or is this in fact the correct description of the HW?
>
> Hi,
> Thank you for your input. Based on my understanding of Stephen
> presentation the power-domain layer could act as a middleware layer
> (like ACPI) that could own resources. That being said it was also stated
> that the proposed approach should work with already existing device
> trees, which implies that the DT should remain as is.
>
> So I could get the resources using attach_dev and detach_dev, but there
> are two problems with that:
>
> 1) The GPU driver will try to manage clocks/reset on it's own using those functions
>    if I provide non-stub working clocks and reset:
> 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 obviously I should invent a way to tell the drm/imagination driver to
> NOT manage. One obvious way to do this is to introduce new flag to genpd.flags
> called let's say GENPD_FLAG_EXCLUSIVE_CONTROL, which would tell the consumer
> driver that the power management is being done only done from the PM
> middleware driver.

Something along those lines. Although, I think the below twist to the
approach would be better.

Some flag (maybe just a bool) should be set dynamically when the
->attach_dev() callback is invoked and it should be a per device flag,
not a per genpd flag. In this way, the genpd provider driver can make
runtime decisions, perhaps even based on some DT compatible string for
the device being attached to it, whether it should manage PM resources
or not.

Additionally, we need a new genpd helper function that allows the
consumer driver to check if the PM resources are managed from the PM
domain level (genpd) or not.

If it sounds complicated, just let me know I can try to help put the
pieces together.

>
> 2) The GPU node doesn't want to own the gpu-clkgen reset. In fact nobody
>    seems to want to own it, even though theoretically it should be owned by
>    the clk_vo as this would describe the hardware best (it's resetting the
>    GPU clocks). But then it would be trickier to get it from the PM driver,
>    making the code more complex and harder to understand. Nonetheless I
>    think it would work.

I guess it doesn't really matter to me. Perhaps model it as a reset
and make the GPU be the consumer of it?

>
> If this sounds good to you I will work on the code.

Sure, let's give this a try - I am here to help review and guide the best I can.

[...]

Kind regards
Uffe

Re: [PATCH v1 1/2] dt-bindings: firmware: thead,th1520: Add clocks and resets

[Michal Wilczynski]

On 4/10/25 14:34, Ulf Hansson wrote:
> On Thu, 10 Apr 2025 at 12:42, Michal Wilczynski
> <m.wilczynski@samsung.com> wrote:
>>
>>
>>
>> On 4/9/25 12:41, Ulf Hansson wrote:
>>> On Wed, 9 Apr 2025 at 11:30, Michal Wilczynski <m.wilczynski@samsung.com> wrote:
>>>>
>>>> Prepare for handling GPU clock and reset sequencing through a generic
>>>> power domain by adding clock and reset properties to the TH1520 AON
>>>> firmware bindings.
>>>>
>>>> The T-HEAD TH1520 GPU requires coordinated management of two clocks
>>>> (core and sys) and two resets (GPU and GPU CLKGEN). Due to SoC-specific
>>>> requirements, the CLKGEN reset must be carefully managed alongside clock
>>>> enables to ensure proper GPU operation, as discussed on the mailing list
>>>> [1].
>>>>
>>>> Since the coordination is now handled through a power domain, only the
>>>> programmable clocks (core and sys) are exposed. The GPU MEM clock is
>>>> ignored, as it is not controllable on the TH1520 SoC.
>>>>
>>>> This approach follows upstream maintainers' recommendations [1] to
>>>> avoid SoC-specific details leaking into the GPU driver or clock/reset
>>>> frameworks directly.
>>>>
>>>> [1] - https://lore.kernel.org/all/38d9650fc11a674c8b689d6bab937acf@kernel.org/
>>>>
>>>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>>>> ---
>>>>  .../bindings/firmware/thead,th1520-aon.yaml   | 28 +++++++++++++++++++
>>>>  1 file changed, 28 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
>>>> index bbc183200400..8075874bcd6b 100644
>>>> --- a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
>>>> +++ b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
>>>> @@ -25,6 +25,16 @@ properties:
>>>>    compatible:
>>>>      const: thead,th1520-aon
>>>>
>>>> +  clocks:
>>>> +    items:
>>>> +      - description: GPU core clock
>>>> +      - description: GPU sys clock
>>>> +
>>>> +  clock-names:
>>>> +    items:
>>>> +      - const: gpu-core
>>>> +      - const: gpu-sys
>>>
>>> These clocks don't look like they belong to the power-domain node, but
>>> rather the GPU's node.
>>>
>>> Or is this in fact the correct description of the HW?
>>
>> Hi,
>> Thank you for your input. Based on my understanding of Stephen
>> presentation the power-domain layer could act as a middleware layer
>> (like ACPI) that could own resources. That being said it was also stated
>> that the proposed approach should work with already existing device
>> trees, which implies that the DT should remain as is.
>>
>> So I could get the resources using attach_dev and detach_dev, but there
>> are two problems with that:
>>
>> 1) The GPU driver will try to manage clocks/reset on it's own using those functions
>>    if I provide non-stub working clocks and reset:
>> 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 obviously I should invent a way to tell the drm/imagination driver to
>> NOT manage. One obvious way to do this is to introduce new flag to genpd.flags
>> called let's say GENPD_FLAG_EXCLUSIVE_CONTROL, which would tell the consumer
>> driver that the power management is being done only done from the PM
>> middleware driver.
> 
> Something along those lines. Although, I think the below twist to the
> approach would be better.
> 
> Some flag (maybe just a bool) should be set dynamically when the
> ->attach_dev() callback is invoked and it should be a per device flag,
> not a per genpd flag. In this way, the genpd provider driver can make
> runtime decisions, perhaps even based on some DT compatible string for
> the device being attached to it, whether it should manage PM resources
> or not.
> 
> Additionally, we need a new genpd helper function that allows the
> consumer driver to check if the PM resources are managed from the PM
> domain level (genpd) or not.
> 
> If it sounds complicated, just let me know I can try to help put the
> pieces together.

Thanks, this sounds doable

> 
>>
>> 2) The GPU node doesn't want to own the gpu-clkgen reset. In fact nobody
>>    seems to want to own it, even though theoretically it should be owned by
>>    the clk_vo as this would describe the hardware best (it's resetting the
>>    GPU clocks). But then it would be trickier to get it from the PM driver,
>>    making the code more complex and harder to understand. Nonetheless I
>>    think it would work.
> 
> I guess it doesn't really matter to me. Perhaps model it as a reset
> and make the GPU be the consumer of it?

GPU driver maintainers already stated that they only want to consume a
single reset line, that would be 'gpu' [1]. The 'gpu-clkgen' is an orphan in
this situation, or a part of a SoC specific-glue code, so theoretically
since the PM driver in our case is also a SoC glue driver we could leave
the 'gpu-clkgen' in PM DT node.

[1] - https://lore.kernel.org/all/816db99d-7088-4c1a-af03-b9a825ac09dc@imgtec.com/
> 
>>
>> If this sounds good to you I will work on the code.
> 
> Sure, let's give this a try - I am here to help review and guide the best I can.

Thank you very much for your support, it’s invaluable!

> 
> [...]
> 
> Kind regards
> Uffe
> 

Re: [PATCH v1 1/2] dt-bindings: firmware: thead,th1520: Add clocks and resets

[Michal Wilczynski]

On 4/12/25 09:53, Michal Wilczynski wrote:
> 
> 
> On 4/10/25 14:34, Ulf Hansson wrote:
>> On Thu, 10 Apr 2025 at 12:42, Michal Wilczynski
>> <m.wilczynski@samsung.com> wrote:
>>>
>>>
>>>
>>> On 4/9/25 12:41, Ulf Hansson wrote:
>>>> On Wed, 9 Apr 2025 at 11:30, Michal Wilczynski <m.wilczynski@samsung.com> wrote:
>>>>>
>>>>> Prepare for handling GPU clock and reset sequencing through a generic
>>>>> power domain by adding clock and reset properties to the TH1520 AON
>>>>> firmware bindings.
>>>>>
>>>>> The T-HEAD TH1520 GPU requires coordinated management of two clocks
>>>>> (core and sys) and two resets (GPU and GPU CLKGEN). Due to SoC-specific
>>>>> requirements, the CLKGEN reset must be carefully managed alongside clock
>>>>> enables to ensure proper GPU operation, as discussed on the mailing list
>>>>> [1].
>>>>>
>>>>> Since the coordination is now handled through a power domain, only the
>>>>> programmable clocks (core and sys) are exposed. The GPU MEM clock is
>>>>> ignored, as it is not controllable on the TH1520 SoC.
>>>>>
>>>>> This approach follows upstream maintainers' recommendations [1] to
>>>>> avoid SoC-specific details leaking into the GPU driver or clock/reset
>>>>> frameworks directly.
>>>>>
>>>>> [1] - https://lore.kernel.org/all/38d9650fc11a674c8b689d6bab937acf@kernel.org/
>>>>>
>>>>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>>>>> ---
>>>>>  .../bindings/firmware/thead,th1520-aon.yaml   | 28 +++++++++++++++++++
>>>>>  1 file changed, 28 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
>>>>> index bbc183200400..8075874bcd6b 100644
>>>>> --- a/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
>>>>> +++ b/Documentation/devicetree/bindings/firmware/thead,th1520-aon.yaml
>>>>> @@ -25,6 +25,16 @@ properties:
>>>>>    compatible:
>>>>>      const: thead,th1520-aon
>>>>>
>>>>> +  clocks:
>>>>> +    items:
>>>>> +      - description: GPU core clock
>>>>> +      - description: GPU sys clock
>>>>> +
>>>>> +  clock-names:
>>>>> +    items:
>>>>> +      - const: gpu-core
>>>>> +      - const: gpu-sys
>>>>
>>>> These clocks don't look like they belong to the power-domain node, but
>>>> rather the GPU's node.
>>>>
>>>> Or is this in fact the correct description of the HW?
>>>
>>> Hi,
>>> Thank you for your input. Based on my understanding of Stephen
>>> presentation the power-domain layer could act as a middleware layer
>>> (like ACPI) that could own resources. That being said it was also stated
>>> that the proposed approach should work with already existing device
>>> trees, which implies that the DT should remain as is.
>>>
>>> So I could get the resources using attach_dev and detach_dev, but there
>>> are two problems with that:
>>>
>>> 1) The GPU driver will try to manage clocks/reset on it's own using those functions
>>>    if I provide non-stub working clocks and reset:
>>> 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 obviously I should invent a way to tell the drm/imagination driver to
>>> NOT manage. One obvious way to do this is to introduce new flag to genpd.flags
>>> called let's say GENPD_FLAG_EXCLUSIVE_CONTROL, which would tell the consumer
>>> driver that the power management is being done only done from the PM
>>> middleware driver.
>>
>> Something along those lines. Although, I think the below twist to the
>> approach would be better.
>>
>> Some flag (maybe just a bool) should be set dynamically when the
>> ->attach_dev() callback is invoked and it should be a per device flag,
>> not a per genpd flag. In this way, the genpd provider driver can make
>> runtime decisions, perhaps even based on some DT compatible string for
>> the device being attached to it, whether it should manage PM resources
>> or not.
>>
>> Additionally, we need a new genpd helper function that allows the
>> consumer driver to check if the PM resources are managed from the PM
>> domain level (genpd) or not.
>>
>> If it sounds complicated, just let me know I can try to help put the
>> pieces together.
> 
> Thanks, this sounds doable
> 
>>
>>>
>>> 2) The GPU node doesn't want to own the gpu-clkgen reset. In fact nobody
>>>    seems to want to own it, even though theoretically it should be owned by
>>>    the clk_vo as this would describe the hardware best (it's resetting the
>>>    GPU clocks). But then it would be trickier to get it from the PM driver,
>>>    making the code more complex and harder to understand. Nonetheless I
>>>    think it would work.
>>
>> I guess it doesn't really matter to me. Perhaps model it as a reset
>> and make the GPU be the consumer of it?
> 
> GPU driver maintainers already stated that they only want to consume a
> single reset line, that would be 'gpu' [1]. The 'gpu-clkgen' is an orphan in
> this situation, or a part of a SoC specific-glue code, so theoretically
> since the PM driver in our case is also a SoC glue driver we could leave
> the 'gpu-clkgen' in PM DT node.

Frank, Matt
Just to make sure, I would like to ask what you think about this ? Would
you be open to have an extra reset that would be managed from the
generic PM driver in your GPU DT node ?

Regards,
Michał

> 
> [1] - https://lore.kernel.org/all/816db99d-7088-4c1a-af03-b9a825ac09dc@imgtec.com/
>>
>>>
>>> If this sounds good to you I will work on the code.
>>
>> Sure, let's give this a try - I am here to help review and guide the best I can.
> 
> Thank you very much for your support, it’s invaluable!
> 
>>
>> [...]
>>
>> Kind regards
>> Uffe
>>

Re: [PATCH v1 1/2] dt-bindings: firmware: thead,th1520: Add clocks and resets

[Krzysztof Kozlowski]

On 09/04/2025 11:30, Michal Wilczynski wrote:
> Prepare for handling GPU clock and reset sequencing through a generic
> power domain by adding clock and reset properties to the TH1520 AON
> firmware bindings.
> 
> The T-HEAD TH1520 GPU requires coordinated management of two clocks
> (core and sys) and two resets (GPU and GPU CLKGEN). Due to SoC-specific
> requirements, the CLKGEN reset must be carefully managed alongside clock
> enables to ensure proper GPU operation, as discussed on the mailing list
> [1].
> 
<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter><form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>
Best regards,
Krzysztof

[PATCH v1 2/2] 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 needs to be abstracted away from the
Imagination GPU driver, which expects a standard single reset line.
Following discussions with kernel maintainers, this logic is placed
inside a PM domain instead of polluting the clock or reset frameworks,
or the GPU driver itself [1].

Managing this inside a generic power domain allows better coordination
of clocks, resets, and power state, and aligns with the direction of
treating PM domains as SoC-specific "power management drivers".

[1] - https://lore.kernel.org/all/CAPDyKFqsJaTrF0tBSY-TjpqdVt5=6aPQHYfnDebtphfRZSU=-Q@mail.gmail.com/

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

diff --git a/drivers/pmdomain/thead/th1520-pm-domains.c b/drivers/pmdomain/thead/th1520-pm-domains.c
index f702e20306f4..aa85c3954c39 100644
--- a/drivers/pmdomain/thead/th1520-pm-domains.c
+++ b/drivers/pmdomain/thead/th1520-pm-domains.c
@@ -5,17 +5,29 @@
  * 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>
 
+#define TH1520_GPU_RESET_IDX 0
+#define TH1520_GPU_CLKGEN_RESET_IDX 1
+
 struct th1520_power_domain {
 	struct th1520_aon_chan *aon_chan;
 	struct generic_pm_domain genpd;
 	u32 rsrc;
+
+	struct clk_bulk_data *clks;
+	int num_clks;
+	struct reset_control_bulk_data *resets;
+	int num_resets;
+
 };
 
 struct th1520_power_info {
@@ -61,6 +73,99 @@ 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_clocks(struct device *dev,
+				  struct th1520_power_domain *pd)
+{
+	static const char *const clk_names[] = { "gpu-core", "gpu-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_resets(struct device *dev,
+				  struct th1520_power_domain *pd)
+{
+	static const char *const reset_names[] = { "gpu", "gpu-clkgen" };
+	int i, ret;
+
+	pd->num_resets = ARRAY_SIZE(reset_names);
+	pd->resets = devm_kcalloc(dev, pd->num_resets, sizeof(*pd->resets),
+				  GFP_KERNEL);
+	if (!pd->resets)
+		return -ENOMEM;
+
+	for (i = 0; i < pd->num_resets; i++)
+		pd->resets[i].id = reset_names[i];
+
+	ret = devm_reset_control_bulk_get_exclusive(dev, pd->num_resets,
+						    pd->resets);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get GPU resets\n");
+
+	return 0;
+}
+
+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;
+
+	ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
+	if (ret)
+		return ret;
+
+	ret = reset_control_deassert(pd->resets[TH1520_GPU_CLKGEN_RESET_IDX].rstc);
+	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->resets[TH1520_GPU_RESET_IDX].rstc);
+	if (ret)
+		goto err_assert_clkgen;
+
+	return 0;
+
+err_assert_clkgen:
+	reset_control_assert(pd->resets[TH1520_GPU_CLKGEN_RESET_IDX].rstc);
+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);
+
+	reset_control_assert(pd->resets[TH1520_GPU_RESET_IDX].rstc);
+	reset_control_assert(pd->resets[TH1520_GPU_CLKGEN_RESET_IDX].rstc);
+	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 +204,20 @@ 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]) {
+		ret = th1520_gpu_init_clocks(dev, pd);
+		if (ret)
+			return ERR_PTR(ret);
+
+		ret = th1520_gpu_init_resets(dev, pd);
+		if (ret)
+			return ERR_PTR(ret);
+
+		pd->genpd.dev_ops.start = th1520_gpu_domain_start;
+		pd->genpd.dev_ops.stop = th1520_gpu_domain_stop;
+	}
+
 	ret = pm_genpd_init(&pd->genpd, NULL, true);
 	if (ret)
 		return ERR_PTR(ret);
-- 
2.34.1

Re: [PATCH v1 2/2] pmdomain: thead: Add GPU-specific clock and reset handling for TH1520

[Ulf Hansson]

On Wed, 9 Apr 2025 at 11:30, 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 needs to be abstracted away from the
> Imagination GPU driver, which expects a standard single reset line.
> Following discussions with kernel maintainers, this logic is placed
> inside a PM domain instead of polluting the clock or reset frameworks,
> or the GPU driver itself [1].
>
> Managing this inside a generic power domain allows better coordination
> of clocks, resets, and power state, and aligns with the direction of
> treating PM domains as SoC-specific "power management drivers".
>
> [1] - https://lore.kernel.org/all/CAPDyKFqsJaTrF0tBSY-TjpqdVt5=6aPQHYfnDebtphfRZSU=-Q@mail.gmail.com/
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  drivers/pmdomain/thead/th1520-pm-domains.c | 119 +++++++++++++++++++++
>  1 file changed, 119 insertions(+)
>
> diff --git a/drivers/pmdomain/thead/th1520-pm-domains.c b/drivers/pmdomain/thead/th1520-pm-domains.c
> index f702e20306f4..aa85c3954c39 100644
> --- a/drivers/pmdomain/thead/th1520-pm-domains.c
> +++ b/drivers/pmdomain/thead/th1520-pm-domains.c
> @@ -5,17 +5,29 @@
>   * 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>
>
> +#define TH1520_GPU_RESET_IDX 0
> +#define TH1520_GPU_CLKGEN_RESET_IDX 1
> +
>  struct th1520_power_domain {
>         struct th1520_aon_chan *aon_chan;
>         struct generic_pm_domain genpd;
>         u32 rsrc;
> +
> +       struct clk_bulk_data *clks;
> +       int num_clks;
> +       struct reset_control_bulk_data *resets;
> +       int num_resets;
> +
>  };
>
>  struct th1520_power_info {
> @@ -61,6 +73,99 @@ 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_clocks(struct device *dev,
> +                                 struct th1520_power_domain *pd)
> +{
> +       static const char *const clk_names[] = { "gpu-core", "gpu-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_resets(struct device *dev,
> +                                 struct th1520_power_domain *pd)
> +{
> +       static const char *const reset_names[] = { "gpu", "gpu-clkgen" };
> +       int i, ret;
> +
> +       pd->num_resets = ARRAY_SIZE(reset_names);
> +       pd->resets = devm_kcalloc(dev, pd->num_resets, sizeof(*pd->resets),
> +                                 GFP_KERNEL);
> +       if (!pd->resets)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < pd->num_resets; i++)
> +               pd->resets[i].id = reset_names[i];
> +
> +       ret = devm_reset_control_bulk_get_exclusive(dev, pd->num_resets,
> +                                                   pd->resets);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Failed to get GPU resets\n");
> +
> +       return 0;
> +}
> +
> +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;
> +
> +       ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
> +       if (ret)
> +               return ret;
> +
> +       ret = reset_control_deassert(pd->resets[TH1520_GPU_CLKGEN_RESET_IDX].rstc);
> +       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->resets[TH1520_GPU_RESET_IDX].rstc);
> +       if (ret)
> +               goto err_assert_clkgen;
> +
> +       return 0;
> +
> +err_assert_clkgen:
> +       reset_control_assert(pd->resets[TH1520_GPU_CLKGEN_RESET_IDX].rstc);
> +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);
> +
> +       reset_control_assert(pd->resets[TH1520_GPU_RESET_IDX].rstc);
> +       reset_control_assert(pd->resets[TH1520_GPU_CLKGEN_RESET_IDX].rstc);
> +       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 +204,20 @@ 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]) {
> +               ret = th1520_gpu_init_clocks(dev, pd);

Assuming both the clocks and the reset are really resources that
belong to the GPU, I would suggest to use two callbacks for the genpd
in question to implement the get/put of the clocks and reset from
there.

pd->genpd->attach_dev
pd->genpd->detach_dev

> +               if (ret)
> +                       return ERR_PTR(ret);
> +
> +               ret = th1520_gpu_init_resets(dev, pd);
> +               if (ret)
> +                       return ERR_PTR(ret);
> +
> +               pd->genpd.dev_ops.start = th1520_gpu_domain_start;
> +               pd->genpd.dev_ops.stop = th1520_gpu_domain_stop;
> +       }
> +
>         ret = pm_genpd_init(&pd->genpd, NULL, true);
>         if (ret)
>                 return ERR_PTR(ret);
> --
> 2.34.1
>

Kind regards
Uffe