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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f21ffd12-167b-4d10-9017-33041ec322b0@kernel.org>
Date: Tue, 3 Dec 2024 16:45:25 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Michal Wilczynski <m.wilczynski@...sung.com>, mturquette@...libre.com,
 sboyd@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 drew@...7.com, guoren@...nel.org, wefu@...hat.com, jassisinghbrar@...il.com,
 paul.walmsley@...ive.com, palmer@...belt.com, aou@...s.berkeley.edu,
 frank.binns@...tec.com, matt.coster@...tec.com,
 maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de,
 airlied@...il.com, simona@...ll.ch, ulf.hansson@...aro.org,
 jszhang@...nel.org, m.szyprowski@...sung.com
Cc: linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
 dri-devel@...ts.freedesktop.org, linux-pm@...r.kernel.org
Subject: Re: [RFC PATCH v1 05/14] dt-bindings: clock: thead,th1520: Add
 support for Video Output subsystem

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@...sung.com>
> ---
>  .../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