Re: [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add support for Video Output subsystem

[Krzysztof Kozlowski]

On 03/12/2024 14:41, Michal Wilczynski wrote:
> The device tree bindings for the T-Head TH1520 SoC clocks currently
> support only the Application Processor (AP) subsystem. This commit
> extends the bindings to include the Video Output (VO) subsystem clocks.
> 
> Update the YAML schema to define the VO subsystem clocks, allowing the
> clock driver to configure and manage these clocks appropriately. This
> addition is necessary to enable the proper operation of the video output
> features on the TH1520 SoC.
> 
> Signed-off-by: Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>
> ---
>  .../bindings/clock/thead,th1520-clk-ap.yaml   | 31 +++++++++++++++----
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
> index 4a0806af2bf9..5a8f1041f766 100644
> --- a/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
> +++ b/Documentation/devicetree/bindings/clock/thead,th1520-clk-ap.yaml
> @@ -4,11 +4,13 @@
>  $id: http://devicetree.org/schemas/clock/thead,th1520-clk-ap.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: T-HEAD TH1520 AP sub-system clock controller
> +title: T-HEAD TH1520 sub-systems clock controller
>  
>  description: |
> -  The T-HEAD TH1520 AP sub-system clock controller configures the
> -  CPU, DPU, GMAC and TEE PLLs.
> +  The T-HEAD TH1520 sub-systems clock controller configures the
> +  CPU, DPU, GMAC and TEE PLLs for the AP subsystem. For the VO
> +  subsystem clock gates can be configured for the HDMI, MIPI and
> +  the GPU.
>  
>    SoC reference manual
>    https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf
> @@ -20,7 +22,9 @@ maintainers:
>  
>  properties:
>    compatible:
> -    const: thead,th1520-clk-ap
> +    enum:
> +      - thead,th1520-clk-ap
> +      - thead,th1520-clk-vo
>  
>    reg:
>      maxItems: 1
> @@ -29,6 +33,17 @@ properties:
>      items:
>        - description: main oscillator (24MHz)
>  
> +  thead,vosys-regmap:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: |
> +      Phandle to a syscon node representing the shared register
> +      space of the VO (Video Output) subsystem. This register space
> +      includes both clock control registers and other control
> +      registers used for operations like resetting the GPU. Since


It seems you wanted to implement reset controller...

> +      these registers reside in the same address space, access to
> +      them is coordinated through a shared syscon regmap provided by
> +      the specified syscon node.

Drop last sentence. syscon regmap is a Linux term, not hardware one.

Anyway, this needs to be constrained per variant.

> +
>    "#clock-cells":
>      const: 1
>      description:
> @@ -36,8 +51,6 @@ properties:
>  
>  required:
>    - compatible
> -  - reg

No, that's a clear NAK. You claim you have no address space but in the
same time you have address space via regmap.

> -  - clocks

Nope, not explained, unless you wanted to make it different per variants.

>    - "#clock-cells"
>  
>  additionalProperties: false
> @@ -51,3 +64,9 @@ examples:
>          clocks = <&osc>;
>          #clock-cells = <1>;
>      };
> +
> +    clock-controller-vo {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +        compatible = "thead,th1520-clk-vo";
> +        thead,vosys-regmap = <&vosys_regmap>;

That's a "reg" property. Do not encode address space as something else.


> +        #clock-cells = <1>;
> +    };


Best regards,
Krzysztof