[PATCH] reset: thead: Scope TH1520 reset driver to VO subsystem

[PATCH] reset: thead: Scope TH1520 reset driver to VO subsystem

[Michal Wilczynski]

The reset controller driver for the TH1520 was using the generic
compatible string "thead,th1520-reset". However, the current
implementation only manages the resets for the Video Output (VO)
subsystem.

Using a generic compatible is incorrect as it implies control over all
reset units on the SoC. This could lead to conflicts if support for
other reset controllers on the TH1520 is added in the future like AP.

To ensure correctness and prevent future issues, this patch renames the
compatible string to "thead,th1520-reset-vo". The device tree bindings,
the th1520.dtsi file, and the driver itself are updated to use this new,
more specific compatible. The device tree node label is also renamed
from 'rst' to 'rst_vo' for clarity.

Fixes: 30e7573babdc ("dt-bindings: reset: Add T-HEAD TH1520 SoC Reset Controller")
Reported-by: Icenowy Zheng <uwu@icenowy.me>
Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml | 6 +++---
 arch/riscv/boot/dts/thead/th1520.dtsi                           | 6 +++---
 drivers/reset/reset-th1520.c                                    | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml b/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
index f2e91d0add7a60e12973c216bb5a989857c3c47c..f84c5ae8bc3569cb1d4e8f07999888ea26e175d0 100644
--- a/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
+++ b/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
@@ -16,7 +16,7 @@ maintainers:
 properties:
   compatible:
     enum:
-      - thead,th1520-reset
+      - thead,th1520-reset-vo
 
   reg:
     maxItems: 1
@@ -36,8 +36,8 @@ examples:
     soc {
       #address-cells = <2>;
       #size-cells = <2>;
-      rst: reset-controller@ffef528000 {
-        compatible = "thead,th1520-reset";
+      rst_vo: reset-controller@ffef528000 {
+        compatible = "thead,th1520-reset-vo";
         reg = <0xff 0xef528000 0x0 0x1000>;
         #reset-cells = <1>;
       };
diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 42724bf7e90e08fac326c464d0f080e3bd2cd59b..9cc2f1adf489ac432b2f3fbb06b655490d9e14b3 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -235,7 +235,7 @@ aon: aon {
 		compatible = "thead,th1520-aon";
 		mboxes = <&mbox_910t 1>;
 		mbox-names = "aon";
-		resets = <&rst TH1520_RESET_ID_GPU_CLKGEN>;
+		resets = <&rst_vo TH1520_RESET_ID_GPU_CLKGEN>;
 		reset-names = "gpu-clkgen";
 		#power-domain-cells = <1>;
 	};
@@ -500,8 +500,8 @@ clk: clock-controller@ffef010000 {
 			#clock-cells = <1>;
 		};
 
-		rst: reset-controller@ffef528000 {
-			compatible = "thead,th1520-reset";
+		rst_vo: reset-controller@ffef528000 {
+			compatible = "thead,th1520-reset-vo";
 			reg = <0xff 0xef528000 0x0 0x4f>;
 			#reset-cells = <1>;
 		};
diff --git a/drivers/reset/reset-th1520.c b/drivers/reset/reset-th1520.c
index 7874f0693e1b427a094a68f2b6d783985e789bf8..05ed11972774618df4512b7c9f9f12e71455e48b 100644
--- a/drivers/reset/reset-th1520.c
+++ b/drivers/reset/reset-th1520.c
@@ -116,7 +116,7 @@ static int th1520_reset_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id th1520_reset_match[] = {
-	{ .compatible = "thead,th1520-reset" },
+	{ .compatible = "thead,th1520-reset-vo" },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, th1520_reset_match);

---
base-commit: 561c80369df0733ba0574882a1635287b20f9de2
change-id: 20250810-fix_reset_2-a618d7426534

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>

Re: [PATCH] reset: thead: Scope TH1520 reset driver to VO subsystem

[Yao Zi]

On Sun, Aug 10, 2025 at 11:14:19PM +0200, Michal Wilczynski wrote:
> The reset controller driver for the TH1520 was using the generic
> compatible string "thead,th1520-reset". However, the current
> implementation only manages the resets for the Video Output (VO)
> subsystem.
> 
> Using a generic compatible is incorrect as it implies control over all
> reset units on the SoC. This could lead to conflicts if support for
> other reset controllers on the TH1520 is added in the future like AP.
> 
> To ensure correctness and prevent future issues, this patch renames the
> compatible string to "thead,th1520-reset-vo". The device tree bindings,
> the th1520.dtsi file, and the driver itself are updated to use this new,
> more specific compatible. The device tree node label is also renamed
> from 'rst' to 'rst_vo' for clarity.
> 
> Fixes: 30e7573babdc ("dt-bindings: reset: Add T-HEAD TH1520 SoC Reset Controller")
> Reported-by: Icenowy Zheng <uwu@icenowy.me>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml | 6 +++---
>  arch/riscv/boot/dts/thead/th1520.dtsi                           | 6 +++---
>  drivers/reset/reset-th1520.c                                    | 2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml b/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
> index f2e91d0add7a60e12973c216bb5a989857c3c47c..f84c5ae8bc3569cb1d4e8f07999888ea26e175d0 100644
> --- a/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
> +++ b/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
> @@ -16,7 +16,7 @@ maintainers:
>  properties:
>    compatible:
>      enum:
> -      - thead,th1520-reset
> +      - thead,th1520-reset-vo

I think we should mark thead,th1520-reset as deprecated instead of
removing it completely, to demonstrate the ABI problem and make the
situation clear.

>    reg:
>      maxItems: 1
> @@ -36,8 +36,8 @@ examples:
>      soc {
>        #address-cells = <2>;
>        #size-cells = <2>;
> -      rst: reset-controller@ffef528000 {
> -        compatible = "thead,th1520-reset";
> +      rst_vo: reset-controller@ffef528000 {
> +        compatible = "thead,th1520-reset-vo";
>          reg = <0xff 0xef528000 0x0 0x1000>;
>          #reset-cells = <1>;
>        };
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index 42724bf7e90e08fac326c464d0f080e3bd2cd59b..9cc2f1adf489ac432b2f3fbb06b655490d9e14b3 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -235,7 +235,7 @@ aon: aon {
>  		compatible = "thead,th1520-aon";
>  		mboxes = <&mbox_910t 1>;
>  		mbox-names = "aon";
> -		resets = <&rst TH1520_RESET_ID_GPU_CLKGEN>;
> +		resets = <&rst_vo TH1520_RESET_ID_GPU_CLKGEN>;
>  		reset-names = "gpu-clkgen";
>  		#power-domain-cells = <1>;
>  	};
> @@ -500,8 +500,8 @@ clk: clock-controller@ffef010000 {
>  			#clock-cells = <1>;
>  		};
>  
> -		rst: reset-controller@ffef528000 {
> -			compatible = "thead,th1520-reset";
> +		rst_vo: reset-controller@ffef528000 {
> +			compatible = "thead,th1520-reset-vo";
>  			reg = <0xff 0xef528000 0x0 0x4f>;
>  			#reset-cells = <1>;
>  		};
> diff --git a/drivers/reset/reset-th1520.c b/drivers/reset/reset-th1520.c
> index 7874f0693e1b427a094a68f2b6d783985e789bf8..05ed11972774618df4512b7c9f9f12e71455e48b 100644
> --- a/drivers/reset/reset-th1520.c
> +++ b/drivers/reset/reset-th1520.c
> @@ -116,7 +116,7 @@ static int th1520_reset_probe(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id th1520_reset_match[] = {
> -	{ .compatible = "thead,th1520-reset" },
> +	{ .compatible = "thead,th1520-reset-vo" },

And this change actually breaks compatibility with older devicetrees.
thead,th1520-reset has been part of the ABI, and we should keep the
compatible string to maintain the compatibility.

With these two changes, I think the changes could be seperated into
different patches, one for the dt-binding, one for the driver, and one
for the devicetree, which could make their scope more clear.

Thanks,
Yao Zi

>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, th1520_reset_match);
> 
> ---
> base-commit: 561c80369df0733ba0574882a1635287b20f9de2
> change-id: 20250810-fix_reset_2-a618d7426534
> 
> Best regards,
> -- 
> Michal Wilczynski <m.wilczynski@samsung.com>
> 

Re: [PATCH] reset: thead: Scope TH1520 reset driver to VO subsystem

[Krzysztof Kozlowski]

On 10/08/2025 23:14, Michal Wilczynski wrote:
> The reset controller driver for the TH1520 was using the generic
> compatible string "thead,th1520-reset". However, the current
> implementation only manages the resets for the Video Output (VO)
> subsystem.
> 
> Using a generic compatible is incorrect as it implies control over all

Depends, it does not need to imply that.

But in general this is why we ask - and writing bindings have it
documented - to post COMPLETE bindings.

> reset units on the SoC. This could lead to conflicts if support for
> other reset controllers on the TH1520 is added in the future like AP.
> 
> To ensure correctness and prevent future issues, this patch renames the
> compatible string to "thead,th1520-reset-vo". The device tree bindings,
> the th1520.dtsi file, and the driver itself are updated to use this new,
> more specific compatible. The device tree node label is also renamed
> from 'rst' to 'rst_vo' for clarity.
> 
> Fixes: 30e7573babdc ("dt-bindings: reset: Add T-HEAD TH1520 SoC Reset Controller")

That's not a commit from the current RC. Where is cc stable?

> Reported-by: Icenowy Zheng <uwu@icenowy.me>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml | 6 +++---
>  arch/riscv/boot/dts/thead/th1520.dtsi                           | 6 +++---
>  drivers/reset/reset-th1520.c                                    | 2 +-

Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.

>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml b/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
> index f2e91d0add7a60e12973c216bb5a989857c3c47c..f84c5ae8bc3569cb1d4e8f07999888ea26e175d0 100644
> --- a/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
> +++ b/Documentation/devicetree/bindings/reset/thead,th1520-reset.yaml
> @@ -16,7 +16,7 @@ maintainers:
>  properties:
>    compatible:
>      enum:
> -      - thead,th1520-reset
> +      - thead,th1520-reset-vo

No, don't rename the compatible. That's ABI break and this was ALREADY
released.

That's a NAK because you break ABI for not important reason - style.

>  
>    reg:
>      maxItems: 1
> @@ -36,8 +36,8 @@ examples:
>      soc {
>        #address-cells = <2>;
>        #size-cells = <2>;
> -      rst: reset-controller@ffef528000 {
> -        compatible = "thead,th1520-reset";
> +      rst_vo: reset-controller@ffef528000 {

Drop the label. Why is it here in the first place?

> +        compatible = "thead,th1520-reset-vo";
>          reg = <0xff 0xef528000 0x0 0x1000>;
>          #reset-cells = <1>;
>        };
> diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
> index 42724bf7e90e08fac326c464d0f080e3bd2cd59b..9cc2f1adf489ac432b2f3fbb06b655490d9e14b3 100644
> --- a/arch/riscv/boot/dts/thead/th1520.dtsi
> +++ b/arch/riscv/boot/dts/thead/th1520.dtsi
> @@ -235,7 +235,7 @@ aon: aon {
>  		compatible = "thead,th1520-aon";
>  		mboxes = <&mbox_910t 1>;
>  		mbox-names = "aon";
> -		resets = <&rst TH1520_RESET_ID_GPU_CLKGEN>;
> +		resets = <&rst_vo TH1520_RESET_ID_GPU_CLKGEN>;
>  		reset-names = "gpu-clkgen";
>  		#power-domain-cells = <1>;
>  	};
> @@ -500,8 +500,8 @@ clk: clock-controller@ffef010000 {
>  			#clock-cells = <1>;
>  		};
>  
> -		rst: reset-controller@ffef528000 {
> -			compatible = "thead,th1520-reset";
> +		rst_vo: reset-controller@ffef528000 {
> +			compatible = "thead,th1520-reset-vo";
>  			reg = <0xff 0xef528000 0x0 0x4f>;
>  			#reset-cells = <1>;
>  		};
> diff --git a/drivers/reset/reset-th1520.c b/drivers/reset/reset-th1520.c
> index 7874f0693e1b427a094a68f2b6d783985e789bf8..05ed11972774618df4512b7c9f9f12e71455e48b 100644
> --- a/drivers/reset/reset-th1520.c
> +++ b/drivers/reset/reset-th1520.c
> @@ -116,7 +116,7 @@ static int th1520_reset_probe(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id th1520_reset_match[] = {
> -	{ .compatible = "thead,th1520-reset" },
> +	{ .compatible = "thead,th1520-reset-vo" },

NAK, actual ABI break.


Best regards,
Krzysztof

Re: [PATCH] reset: thead: Scope TH1520 reset driver to VO subsystem

[Drew Fustini]

On Sun, Aug 10, 2025 at 11:14:19PM +0200, Michal Wilczynski wrote:
> The reset controller driver for the TH1520 was using the generic
> compatible string "thead,th1520-reset". However, the current
> implementation only manages the resets for the Video Output (VO)
> subsystem.

Looking at Section 5.4 on Page 451 of the TH1520 System User Manual [1],
it does seem like we would ultimately need 6 separate nodes for reset
controllers:

 0xFF_EF01_4000: AP_SUBSYS
 0xFF_EC02_C000: MISC_SUBSYS
 0xFF_E404_0000: VI_SUBSYS
 0xFF_EF52_8000: VO_SUBSYS
 0xFF_ECC3_0000: VP_SUBSYS
 0xFF_EF04_0000: DSP_SUBSYS

Maybe we should take this opportunity to document the bindings for all
the resets that the REE (e.g. Linux) can control?

It seemed like that was overkill for the 2 resets needed for the GPU,
but, as Krzysztof noted in this thread, problems arise when bindings are
introduced that are not complete.

Thanks,
Drew

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

Re: [PATCH] reset: thead: Scope TH1520 reset driver to VO subsystem

[Yao Zi]

On Mon, Aug 11, 2025 at 10:39:55PM -0700, Drew Fustini wrote:
> On Sun, Aug 10, 2025 at 11:14:19PM +0200, Michal Wilczynski wrote:
> > The reset controller driver for the TH1520 was using the generic
> > compatible string "thead,th1520-reset". However, the current
> > implementation only manages the resets for the Video Output (VO)
> > subsystem.
> 
> Looking at Section 5.4 on Page 451 of the TH1520 System User Manual [1],
> it does seem like we would ultimately need 6 separate nodes for reset
> controllers:

Yes, this is true. And another six nodes for clock controllers (there's
already one).

>  0xFF_EF01_4000: AP_SUBSYS
>  0xFF_EC02_C000: MISC_SUBSYS
>  0xFF_E404_0000: VI_SUBSYS
>  0xFF_EF52_8000: VO_SUBSYS
>  0xFF_ECC3_0000: VP_SUBSYS
>  0xFF_EF04_0000: DSP_SUBSYS
> 
> Maybe we should take this opportunity to document the bindings for all
> the resets that the REE (e.g. Linux) can control?

It's worth noting that with either mainline U-Boot or vendor U-Boot, no
core is configured to run as REE. IOW, AON_SUBSYS could be accessed by
AP cores as well.

I think introducing read-only clock support to Linux could help us to
correctly describe pvt clocks which are now replaced with a aonsys
placeholder and resolve issues like what is described in 0370395d45ca
("clk: thead: Mark essential bus clocks as CLK_IGNORE_UNUSED").

Futhermore, there may be downstream projects, e.g. U-Boot, make use of
these TEE-only devices, which could benefit if we have these devices
documented and described in devicetree, too. Thus I think the AON clock
and reset controllers should be documented as well if we're going to
document every reset/clock controller in a batch.

> It seemed like that was overkill for the 2 resets needed for the GPU,
> but, as Krzysztof noted in this thread, problems arise when bindings are
> introduced that are not complete.
> 
> Thanks,
> Drew
> 
> [1] https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf

Best regards,
Yao Zi

Re: [PATCH] reset: thead: Scope TH1520 reset driver to VO subsystem

[Drew Fustini]

On Wed, Aug 13, 2025 at 08:04:40AM +0000, Yao Zi wrote:
> On Mon, Aug 11, 2025 at 10:39:55PM -0700, Drew Fustini wrote:
> > On Sun, Aug 10, 2025 at 11:14:19PM +0200, Michal Wilczynski wrote:
> > > The reset controller driver for the TH1520 was using the generic
> > > compatible string "thead,th1520-reset". However, the current
> > > implementation only manages the resets for the Video Output (VO)
> > > subsystem.
> > 
> > Looking at Section 5.4 on Page 451 of the TH1520 System User Manual [1],
> > it does seem like we would ultimately need 6 separate nodes for reset
> > controllers:
> 
> Yes, this is true. And another six nodes for clock controllers (there's
> already one).
> 
> >  0xFF_EF01_4000: AP_SUBSYS
> >  0xFF_EC02_C000: MISC_SUBSYS
> >  0xFF_E404_0000: VI_SUBSYS
> >  0xFF_EF52_8000: VO_SUBSYS
> >  0xFF_ECC3_0000: VP_SUBSYS
> >  0xFF_EF04_0000: DSP_SUBSYS
> > 
> > Maybe we should take this opportunity to document the bindings for all
> > the resets that the REE (e.g. Linux) can control?
> 
> It's worth noting that with either mainline U-Boot or vendor U-Boot, no
> core is configured to run as REE. IOW, AON_SUBSYS could be accessed by
> AP cores as well.

Interesting, I didn't know that the AP cores could access TEE resources.
> 
> I think introducing read-only clock support to Linux could help us to
> correctly describe pvt clocks which are now replaced with a aonsys
> placeholder and resolve issues like what is described in 0370395d45ca
> ("clk: thead: Mark essential bus clocks as CLK_IGNORE_UNUSED").
> 
> Futhermore, there may be downstream projects, e.g. U-Boot, make use of
> these TEE-only devices, which could benefit if we have these devices
> documented and described in devicetree, too. Thus I think the AON clock
> and reset controllers should be documented as well if we're going to
> document every reset/clock controller in a batch.

I think that does make sense to document the AONSYS clock and reset
controllers in the DT bindings as they are part of the hardware and
described in the t-head documentation. It would be great to be able to
make use of more functionality in the t-head sdk like the ability to
reboot the board.

Thanks,
Drew