* Re: [PATCH] reset: thead: Scope TH1520 reset driver to VO subsystem
2025-08-10 21:14 ` [PATCH] reset: thead: Scope TH1520 reset driver to VO subsystem Michal Wilczynski
@ 2025-08-11 2:23 ` Yao Zi
2025-08-11 6:43 ` Krzysztof Kozlowski
2025-08-12 5:39 ` Drew Fustini
2 siblings, 0 replies; 6+ messages in thread
From: Yao Zi @ 2025-08-11 2:23 UTC (permalink / raw)
To: Michal Wilczynski, Drew Fustini, Guo Ren, Fu Wei, Philipp Zabel,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Paul Walmsley,
Palmer Dabbelt, Albert Ou, Alexandre Ghiti
Cc: Krzysztof Kozlowski, linux-riscv, devicetree, linux-kernel,
Icenowy Zheng
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>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] reset: thead: Scope TH1520 reset driver to VO subsystem
2025-08-10 21:14 ` [PATCH] reset: thead: Scope TH1520 reset driver to VO subsystem Michal Wilczynski
2025-08-11 2:23 ` Yao Zi
@ 2025-08-11 6:43 ` Krzysztof Kozlowski
2025-08-12 5:39 ` Drew Fustini
2 siblings, 0 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2025-08-11 6:43 UTC (permalink / raw)
To: Michal Wilczynski, Drew Fustini, Guo Ren, Fu Wei, Philipp Zabel,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Paul Walmsley,
Palmer Dabbelt, Albert Ou, Alexandre Ghiti
Cc: linux-riscv, devicetree, linux-kernel, Icenowy Zheng
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] reset: thead: Scope TH1520 reset driver to VO subsystem
2025-08-10 21:14 ` [PATCH] reset: thead: Scope TH1520 reset driver to VO subsystem Michal Wilczynski
2025-08-11 2:23 ` Yao Zi
2025-08-11 6:43 ` Krzysztof Kozlowski
@ 2025-08-12 5:39 ` Drew Fustini
2025-08-13 8:04 ` Yao Zi
2 siblings, 1 reply; 6+ messages in thread
From: Drew Fustini @ 2025-08-12 5:39 UTC (permalink / raw)
To: Michal Wilczynski
Cc: Guo Ren, Fu Wei, Philipp Zabel, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexandre Ghiti, Krzysztof Kozlowski, linux-riscv, devicetree,
linux-kernel, Icenowy Zheng
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] reset: thead: Scope TH1520 reset driver to VO subsystem
2025-08-12 5:39 ` Drew Fustini
@ 2025-08-13 8:04 ` Yao Zi
2025-08-13 18:48 ` Drew Fustini
0 siblings, 1 reply; 6+ messages in thread
From: Yao Zi @ 2025-08-13 8:04 UTC (permalink / raw)
To: Drew Fustini, Michal Wilczynski
Cc: Guo Ren, Fu Wei, Philipp Zabel, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexandre Ghiti, Krzysztof Kozlowski, linux-riscv, devicetree,
linux-kernel, Icenowy Zheng
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] reset: thead: Scope TH1520 reset driver to VO subsystem
2025-08-13 8:04 ` Yao Zi
@ 2025-08-13 18:48 ` Drew Fustini
0 siblings, 0 replies; 6+ messages in thread
From: Drew Fustini @ 2025-08-13 18:48 UTC (permalink / raw)
To: Yao Zi
Cc: Michal Wilczynski, Guo Ren, Fu Wei, Philipp Zabel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Alexandre Ghiti, Krzysztof Kozlowski, linux-riscv,
devicetree, linux-kernel, Icenowy Zheng
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
^ permalink raw reply [flat|nested] 6+ messages in thread