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

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