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@xxxxxxxxxxx> wrote:
>>
>>
>>
>> On 4/9/25 12:41, Ulf Hansson wrote:
>>> On Wed, 9 Apr 2025 at 11:30, Michal Wilczynski <m.wilczynski@xxxxxxxxxxx> 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@xxxxxxxxxx/
>>>>
>>>> Signed-off-by: Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>
>>>> ---
>>>>  .../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@xxxxxxxxxx/
> 
>>
>> 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
>