diff mbox series

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

Message ID 20241203134137.2114847-6-m.wilczynski@samsung.com (mailing list archive)
State Changes Requested, archived
Headers show
Series [RFC,v1,01/14] clk: thead: Refactor TH1520 clock driver to share common code | expand

Commit Message

Michal Wilczynski Dec. 3, 2024, 1:41 p.m. UTC
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@samsung.com>
---
 .../bindings/clock/thead,th1520-clk-ap.yaml   | 31 +++++++++++++++----
 1 file changed, 25 insertions(+), 6 deletions(-)

Comments

Rob Herring (Arm) Dec. 3, 2024, 2:24 p.m. UTC | #1
On Tue, 03 Dec 2024 14:41:28 +0100, 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@samsung.com>
> ---
>  .../bindings/clock/thead,th1520-clk-ap.yaml   | 31 +++++++++++++++----
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:


doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241203134137.2114847-6-m.wilczynski@samsung.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Krzysztof Kozlowski Dec. 3, 2024, 3:45 p.m. UTC | #2
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@samsung.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
Michal Wilczynski Dec. 4, 2024, 10:11 a.m. UTC | #3
On 12/3/24 16:45, Krzysztof Kozlowski wrote:
> 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@samsung.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: https://protect2.fireeye.com/v1/url?k=f595e769-941ef222-f5946c26-74fe485fb305-6d0b73471bbfc1a2&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fclock%2Fthead%2Cth1520-clk-ap.yaml%23
>>  $schema: https://protect2.fireeye.com/v1/url?k=5b94114b-3a1f0400-5b959a04-74fe485fb305-0e2c50f5c24cf3e9&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
>>  
>> -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://protect2.fireeye.com/v1/url?k=cceb6120-ad60746b-cceaea6f-74fe485fb305-b294b70f1b52a5ab&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=https%3A%2F%2Fopenbeagle.org%2Fbeaglev-ahead%2Fbeaglev-ahead%2F-%2Fblob%2Fmain%2Fdocs%2FTH1520%2520System%2520User%2520Manual.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...

Thank you for your feedback.

I understand your concern about the reset controller. In the TH1520 SoC,
the GPU doesn't have its own reset controller. Instead, its reset is
managed through the power domain's registers as part of the power-up
sequence.

According to the Video Image Processing Manual 1.4.1 [1], the GPU
requires the following steps to power up:

1) Enable the GPU clock gate.
2) After 32 core clock cycles, release the GPU soft reset

Since these steps are closely tied to power management, I implemented
the reset functionality within the power-domain driver.

Because the GPU doesn't support the resets property, introducing a reset
controller wouldn't align with the existing device tree well.

[1] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20Video%20Image%20Processing%20User%20Manual.pdf
> 
>> +      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.

I see your concern. The VOSYS subsystem's address space includes
registers for various components, such as clock gates and reset
controls, which are scattered throughout the address space as specified
in the manual 4.4.1 [2]. Initially, I attempted to use a shared syscon
regmap for access, but I realize this might not be the best approach.

To address this, I'll specify the 'reg' property in each node to define
the address ranges explicitly fragmenting the address space for the VOSYS
manually.

vosys_clk: clock-controller@ffef528050 {
	compatible = "thead,th1520-clk-vo";
	reg = <0xff 0xef528050 0x0 0x8>;
	#clock-cells = <1>;
};

pd: power-domain@ffef528000 {
	compatible = "thead,th1520-pd";
	reg = <0xff 0xef528000 0x0 0x8>;
	#power-domain-cells = <1>;
};

I would be happy to proceed like this if that's okay.

[2] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf

> 
>> -  - 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://protect2.fireeye.com/v1/url?k=1e9cac96-7f17b9dd-1e9d27d9-74fe485fb305-f0538482114df941&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=https%3A%2F%2Fdevicetree-specification.readthedocs.io%2Fen%2Flatest%2Fchapter2-devicetree-basics.html%23generic-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
>
Stephen Boyd Dec. 4, 2024, 8:21 p.m. UTC | #4
Quoting Michal Wilczynski (2024-12-04 02:11:26)
> On 12/3/24 16:45, Krzysztof Kozlowski wrote:
> > On 03/12/2024 14:41, Michal Wilczynski wrote:
> 
> [1] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20Video%20Image%20Processing%20User%20Manual.pdf
> > 
> >> +      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.
> 
> I see your concern. The VOSYS subsystem's address space includes
> registers for various components, such as clock gates and reset
> controls, which are scattered throughout the address space as specified
> in the manual 4.4.1 [2]. Initially, I attempted to use a shared syscon
> regmap for access, but I realize this might not be the best approach.
> 
> To address this, I'll specify the 'reg' property in each node to define
> the address ranges explicitly fragmenting the address space for the VOSYS
> manually.
> 
> vosys_clk: clock-controller@ffef528050 {
>         compatible = "thead,th1520-clk-vo";
>         reg = <0xff 0xef528050 0x0 0x8>;
>         #clock-cells = <1>;
> };
> 
> pd: power-domain@ffef528000 {
>         compatible = "thead,th1520-pd";
>         reg = <0xff 0xef528000 0x0 0x8>;
>         #power-domain-cells = <1>;
> };

You should have one node:

    clock-controller@ffef528000 {
      compatible = "thead,th1520-vo";
      reg = <0xff 0xef528050 0x0 0x1a04>;
      #clock-cells = <1>;
      #power-domain-cells = <1>;
    };
Stephen Boyd Dec. 4, 2024, 8:22 p.m. UTC | #5
Quoting Stephen Boyd (2024-12-04 12:21:11)
> Quoting Michal Wilczynski (2024-12-04 02:11:26)
> > 
> > To address this, I'll specify the 'reg' property in each node to define
> > the address ranges explicitly fragmenting the address space for the VOSYS
> > manually.
> > 
> > vosys_clk: clock-controller@ffef528050 {
> >         compatible = "thead,th1520-clk-vo";
> >         reg = <0xff 0xef528050 0x0 0x8>;
> >         #clock-cells = <1>;
> > };
> > 
> > pd: power-domain@ffef528000 {
> >         compatible = "thead,th1520-pd";
> >         reg = <0xff 0xef528000 0x0 0x8>;
> >         #power-domain-cells = <1>;
> > };
> 
> You should have one node:
> 
>     clock-controller@ffef528000 {
>       compatible = "thead,th1520-vo";
>       reg = <0xff 0xef528050 0x0 0x1a04>;

Apologies for the typo. Here's the correct line:

	reg = <0xff 0xef528000 0x0 0x1a04>;

>       #clock-cells = <1>;
>       #power-domain-cells = <1>;
>     };
>
Krzysztof Kozlowski Dec. 5, 2024, 7:27 a.m. UTC | #6
On 04/12/2024 11:11, Michal Wilczynski wrote:
> 
> 
> On 12/3/24 16:45, Krzysztof Kozlowski wrote:
>> 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@samsung.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: https://protect2.fireeye.com/v1/url?k=f595e769-941ef222-f5946c26-74fe485fb305-6d0b73471bbfc1a2&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fclock%2Fthead%2Cth1520-clk-ap.yaml%23
>>>  $schema: https://protect2.fireeye.com/v1/url?k=5b94114b-3a1f0400-5b959a04-74fe485fb305-0e2c50f5c24cf3e9&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
>>>  
>>> -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://protect2.fireeye.com/v1/url?k=cceb6120-ad60746b-cceaea6f-74fe485fb305-b294b70f1b52a5ab&q=1&e=6b918e4d-d81f-4b44-8516-776d7b4f9dae&u=https%3A%2F%2Fopenbeagle.org%2Fbeaglev-ahead%2Fbeaglev-ahead%2F-%2Fblob%2Fmain%2Fdocs%2FTH1520%2520System%2520User%2520Manual.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...
> 
> Thank you for your feedback.
> 
> I understand your concern about the reset controller. In the TH1520 SoC,
> the GPU doesn't have its own reset controller. Instead, its reset is
> managed through the power domain's registers as part of the power-up
> sequence.
> 
> According to the Video Image Processing Manual 1.4.1 [1], the GPU
> requires the following steps to power up:
> 
> 1) Enable the GPU clock gate.
> 2) After 32 core clock cycles, release the GPU soft reset
> 
> Since these steps are closely tied to power management, I implemented
> the reset functionality within the power-domain driver.
> 
> Because the GPU doesn't support the resets property, introducing a reset
> controller wouldn't align with the existing device tree well.

So add resets to GPU. You said here that VO has reset registers, so it
should be a reset controller.

> 
> [1] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20Video%20Image%20Processing%20User%20Manual.pdf
>>
>>> +      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.
> 
> I see your concern. The VOSYS subsystem's address space includes
> registers for various components, such as clock gates and reset
> controls, which are scattered throughout the address space as specified
> in the manual 4.4.1 [2]. Initially, I attempted to use a shared syscon
> regmap for access, but I realize this might not be the best approach.

No, you miss the point of how device nodes are supposed to look like.
Don't bring your driver architecture here.

You cannot have regmap/syscon if you do not have reg. Your VOSYS is a
clock, reset and whatever-provider. Your power domain - if it does not
have reg - just does not exist. There is no such device and we do not
care if you have such device DRIVER.

> 
> To address this, I'll specify the 'reg' property in each node to define
> the address ranges explicitly fragmenting the address space for the VOSYS
> manually.
> 
> vosys_clk: clock-controller@ffef528050 {
> 	compatible = "thead,th1520-clk-vo";
> 	reg = <0xff 0xef528050 0x0 0x8>;
> 	#clock-cells = <1>;
> };
> 
> pd: power-domain@ffef528000 {
> 	compatible = "thead,th1520-pd";
> 	reg = <0xff 0xef528000 0x0 0x8>;
> 	#power-domain-cells = <1>;
> };

I don't think you really get the point here. Clock controllers and power
domains per one clock or domain are also a no-go.

Best regards,
Krzysztof
Krzysztof Kozlowski Dec. 5, 2024, 7:28 a.m. UTC | #7
On 04/12/2024 21:21, Stephen Boyd wrote:
>>> No, that's a clear NAK. You claim you have no address space but in the
>>> same time you have address space via regmap.
>>
>> I see your concern. The VOSYS subsystem's address space includes
>> registers for various components, such as clock gates and reset
>> controls, which are scattered throughout the address space as specified
>> in the manual 4.4.1 [2]. Initially, I attempted to use a shared syscon
>> regmap for access, but I realize this might not be the best approach.
>>
>> To address this, I'll specify the 'reg' property in each node to define
>> the address ranges explicitly fragmenting the address space for the VOSYS
>> manually.
>>
>> vosys_clk: clock-controller@ffef528050 {
>>         compatible = "thead,th1520-clk-vo";
>>         reg = <0xff 0xef528050 0x0 0x8>;
>>         #clock-cells = <1>;
>> };
>>
>> pd: power-domain@ffef528000 {
>>         compatible = "thead,th1520-pd";
>>         reg = <0xff 0xef528000 0x0 0x8>;
>>         #power-domain-cells = <1>;
>> };
> 
> You should have one node:
> 
>     clock-controller@ffef528000 {
>       compatible = "thead,th1520-vo";
>       reg = <0xff 0xef528050 0x0 0x1a04>;

Yes, obviously, but probably entire block:

<0xff 0xef528000 0x0 0x1000>;

>       #clock-cells = <1>;
>       #power-domain-cells = <1>;
>     };


Best regards,
Krzysztof
diff mbox series

Patch

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
+      these registers reside in the same address space, access to
+      them is coordinated through a shared syscon regmap provided by
+      the specified syscon node.
+
   "#clock-cells":
     const: 1
     description:
@@ -36,8 +51,6 @@  properties:
 
 required:
   - compatible
-  - reg
-  - clocks
   - "#clock-cells"
 
 additionalProperties: false
@@ -51,3 +64,9 @@  examples:
         clocks = <&osc>;
         #clock-cells = <1>;
     };
+
+    clock-controller-vo {
+        compatible = "thead,th1520-clk-vo";
+        thead,vosys-regmap = <&vosys_regmap>;
+        #clock-cells = <1>;
+    };