[PATCH v4 0/3] drm: rockchip: vop2: Add VP clock resets support

[PATCH v4 0/3] drm: rockchip: vop2: Add VP clock resets support

[Detlev Casanova]

The clock reset must be used when the VOP is configured. Skipping it can
put the VOP in an unknown state where the HDMI signal is either lost or
not matching the selected mode.

This adds support for rk3588(s) based SoCs.

Changes since v3:
- Rebased on drm-misc-next
- Reword first patch subject
- Reorder commits for different trees

Changes since v2:
- Rebase on latest master
- Add details on how to reproduce the issue

Changes since v1:
- Add AXI and AHB clock resets
- Set maxItems for !rk3588 in vop2 bindings

Detlev Casanova (3):
  dt-bindings: display: vop2: Add VP clock resets
  drm/rockchip: vop2: Add clock resets support
  arm64: dts: rockchip: Add VOP clock resets for rk3588s

 .../display/rockchip/rockchip-vop2.yaml       | 40 +++++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 12 ++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c  | 30 ++++++++++++++
 3 files changed, 82 insertions(+)

-- 
2.47.0

[PATCH v4 1/3] dt-bindings: display: vop2: Add VP clock resets

[Detlev Casanova]

Add the documentation for VOP2 video ports reset clocks.
One reset can be set per video port.

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 .../display/rockchip/rockchip-vop2.yaml       | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
index 2531726af306b..5b59d91de47bd 100644
--- a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
+++ b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
@@ -65,6 +65,26 @@ properties:
       - const: dclk_vp3
       - const: pclk_vop
 
+  resets:
+    minItems: 5
+    items:
+      - description: AXI clock reset.
+      - description: AHB clock reset.
+      - description: Pixel clock reset for video port 0.
+      - description: Pixel clock reset for video port 1.
+      - description: Pixel clock reset for video port 2.
+      - description: Pixel clock reset for video port 3.
+
+  reset-names:
+    minItems: 5
+    items:
+      - const: aclk
+      - const: hclk
+      - const: dclk_vp0
+      - const: dclk_vp1
+      - const: dclk_vp2
+      - const: dclk_vp3
+
   rockchip,grf:
     $ref: /schemas/types.yaml#/definitions/phandle
     description:
@@ -128,6 +148,11 @@ allOf:
         clock-names:
           minItems: 7
 
+        resets:
+          minItems: 6
+        reset-names:
+          minItems: 6
+
         ports:
           required:
             - port@0
@@ -152,6 +177,11 @@ allOf:
         clock-names:
           maxItems: 5
 
+        resets:
+          maxItems: 5
+        reset-names:
+          maxItems: 5
+
         ports:
           required:
             - port@0
@@ -183,6 +213,16 @@ examples:
                               "dclk_vp0",
                               "dclk_vp1",
                               "dclk_vp2";
+                resets = <&cru SRST_A_VOP>,
+                         <&cru SRST_H_VOP>,
+                         <&cru SRST_VOP0>,
+                         <&cru SRST_VOP1>,
+                         <&cru SRST_VOP2>;
+                reset-names = "aclk",
+                              "hclk",
+                              "dclk_vp0",
+                              "dclk_vp1",
+                              "dclk_vp2";
                 power-domains = <&power RK3568_PD_VO>;
                 iommus = <&vop_mmu>;
                 vop_out: ports {
-- 
2.47.0

Re: [PATCH v4 1/3] dt-bindings: display: vop2: Add VP clock resets

[Heiko Stübner]

Am Freitag, 15. November 2024, 17:20:40 Mitteleuropäische Sommerzeit schrieb Detlev Casanova:
> Add the documentation for VOP2 video ports reset clocks.
> One reset can be set per video port.
> 
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> ---
>  .../display/rockchip/rockchip-vop2.yaml       | 40 +++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
> index 2531726af306b..5b59d91de47bd 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop2.yaml
> @@ -65,6 +65,26 @@ properties:
>        - const: dclk_vp3
>        - const: pclk_vop
>  
> +  resets:
> +    minItems: 5
> +    items:
> +      - description: AXI clock reset.
> +      - description: AHB clock reset.
> +      - description: Pixel clock reset for video port 0.
> +      - description: Pixel clock reset for video port 1.
> +      - description: Pixel clock reset for video port 2.
> +      - description: Pixel clock reset for video port 3.
> +
> +  reset-names:
> +    minItems: 5
> +    items:
> +      - const: aclk
> +      - const: hclk

the vop1 uses "axi" and "ahb" (and "dclk") for these reset names.

The vendor vop2 code also uses that name in comments, like
/*
 * Reset AXI to get a clean state, which is conducive to recovering
 * from exceptions when enable at next time(such as iommu page fault)
 */

So for these two we're not resetting clocks, but the parts of the
vop2 ... so I'd strongly wish for matching names for the vop2 :-)

Thanks
Heiko



> +      - const: dclk_vp0
> +      - const: dclk_vp1
> +      - const: dclk_vp2
> +      - const: dclk_vp3
> +
>    rockchip,grf:
>      $ref: /schemas/types.yaml#/definitions/phandle
>      description:
> @@ -128,6 +148,11 @@ allOf:
>          clock-names:
>            minItems: 7
>  
> +        resets:
> +          minItems: 6
> +        reset-names:
> +          minItems: 6
> +
>          ports:
>            required:
>              - port@0
> @@ -152,6 +177,11 @@ allOf:
>          clock-names:
>            maxItems: 5
>  
> +        resets:
> +          maxItems: 5
> +        reset-names:
> +          maxItems: 5
> +
>          ports:
>            required:
>              - port@0
> @@ -183,6 +213,16 @@ examples:
>                                "dclk_vp0",
>                                "dclk_vp1",
>                                "dclk_vp2";
> +                resets = <&cru SRST_A_VOP>,
> +                         <&cru SRST_H_VOP>,
> +                         <&cru SRST_VOP0>,
> +                         <&cru SRST_VOP1>,
> +                         <&cru SRST_VOP2>;
> +                reset-names = "aclk",
> +                              "hclk",
> +                              "dclk_vp0",
> +                              "dclk_vp1",
> +                              "dclk_vp2";
>                  power-domains = <&power RK3568_PD_VO>;
>                  iommus = <&vop_mmu>;
>                  vop_out: ports {
> 

[PATCH v4 2/3] drm/rockchip: vop2: Add clock resets support

[Detlev Casanova]

At the end of initialization, each VP clock needs to be reset before
they can be used.

Failing to do so can put the VOP in an undefined state where the
generated HDMI signal is either lost or not matching the selected mode.

This issue can be reproduced by switching modes multiple times.
Depending on the setup, after about 10 mode switches, the signal will be
lost and the value in register 0x890 (VSYNCWIDTH + VFRONT) will take the value
`0x0000018c`.
That makes VSYNCWIDTH=0, which is wrong.

Adding the clock resets after the VOP configuration fixes the issue.

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 30 ++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 9ad025aa9ab05..42e165e42c833 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -17,6 +17,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
+#include <linux/reset.h>
 #include <linux/swab.h>
 
 #include <drm/drm.h>
@@ -157,6 +158,7 @@ struct vop2_win {
 struct vop2_video_port {
 	struct drm_crtc crtc;
 	struct vop2 *vop2;
+	struct reset_control *dclk_rst;
 	struct clk *dclk;
 	unsigned int id;
 	const struct vop2_video_port_data *data;
@@ -2060,6 +2062,26 @@ static int us_to_vertical_line(struct drm_display_mode *mode, int us)
 	return us * mode->clock / mode->htotal / 1000;
 }
 
+static int vop2_clk_reset(struct vop2_video_port *vp)
+{
+	struct reset_control *rstc = vp->dclk_rst;
+	struct vop2 *vop2 = vp->vop2;
+	int ret;
+
+	if (!rstc)
+		return 0;
+
+	ret = reset_control_assert(rstc);
+	if (ret < 0)
+		drm_warn(vop2->drm, "failed to assert reset\n");
+	udelay(10);
+	ret = reset_control_deassert(rstc);
+	if (ret < 0)
+		drm_warn(vop2->drm, "failed to deassert reset\n");
+
+	return ret;
+}
+
 static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
 				    struct drm_atomic_state *state)
 {
@@ -2202,6 +2224,8 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	vop2_crtc_atomic_try_set_gamma(vop2, vp, crtc, crtc_state);
 
+	vop2_clk_reset(vp);
+
 	drm_crtc_vblank_on(crtc);
 
 	vop2_unlock(vop2);
@@ -2891,6 +2915,12 @@ static int vop2_create_crtcs(struct vop2 *vop2)
 		vp->data = vp_data;
 
 		snprintf(dclk_name, sizeof(dclk_name), "dclk_vp%d", vp->id);
+		vp->dclk_rst = devm_reset_control_get_optional(vop2->dev, dclk_name);
+		if (IS_ERR(vp->dclk_rst)) {
+		        drm_err(vop2->drm, "failed to get %s reset\n", dclk_name);
+		        return PTR_ERR(vp->dclk_rst);
+		}
+
 		vp->dclk = devm_clk_get(vop2->dev, dclk_name);
 		if (IS_ERR(vp->dclk)) {
 			drm_err(vop2->drm, "failed to get %s\n", dclk_name);
-- 
2.47.0

[PATCH v4 3/3] arm64: dts: rockchip: Add VOP clock resets for rk3588s

[Detlev Casanova]

This adds the needed clock resets for all rk3588(s) based SOCs.

Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
index d97d84b888375..b5c19423de9b6 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
@@ -1268,6 +1268,18 @@ vop: vop@fdd90000 {
 			      "pclk_vop";
 		iommus = <&vop_mmu>;
 		power-domains = <&power RK3588_PD_VOP>;
+		resets = <&cru SRST_A_VOP>,
+			 <&cru SRST_H_VOP>,
+			 <&cru SRST_D_VOP0>,
+			 <&cru SRST_D_VOP1>,
+			 <&cru SRST_D_VOP2>,
+			 <&cru SRST_D_VOP3>;
+		reset-names = "aclk",
+			      "hclk",
+			      "dclk_vp0",
+			      "dclk_vp1",
+			      "dclk_vp2",
+			      "dclk_vp3";
 		rockchip,grf = <&sys_grf>;
 		rockchip,vop-grf = <&vop_grf>;
 		rockchip,vo1-grf = <&vo1_grf>;
-- 
2.47.0

Re:[PATCH v4 0/3] drm: rockchip: vop2: Add VP clock resets support

[Andy Yan]

Hello Detlev,

At 2024-11-16 00:20:39, "Detlev Casanova" <detlev.casanova@collabora.com> wrote:
>The clock reset must be used when the VOP is configured. Skipping it can
>put the VOP in an unknown state where the HDMI signal is either lost or
>not matching the selected mode.

Can you provide some detail about how to reproduce this issue ?
If we can reproduce this issue, we might be able to do a more deep analysis.

>
>This adds support for rk3588(s) based SoCs.
>
>Changes since v3:
>- Rebased on drm-misc-next
>- Reword first patch subject
>- Reorder commits for different trees
>
>Changes since v2:
>- Rebase on latest master
>- Add details on how to reproduce the issue
>
>Changes since v1:
>- Add AXI and AHB clock resets
>- Set maxItems for !rk3588 in vop2 bindings
>
>Detlev Casanova (3):
>  dt-bindings: display: vop2: Add VP clock resets
>  drm/rockchip: vop2: Add clock resets support
>  arm64: dts: rockchip: Add VOP clock resets for rk3588s
>
> .../display/rockchip/rockchip-vop2.yaml       | 40 +++++++++++++++++++
> arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 12 ++++++
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c  | 30 ++++++++++++++
> 3 files changed, 82 insertions(+)
>
>-- 
>2.47.0

Re: [PATCH v4 0/3] drm: rockchip: vop2: Add VP clock resets support

[Detlev Casanova]

Hi Andy,

Yes, the issue can be reproduced with the details in patch 2/3 [1].

[1]: https://lore.kernel.org/linux-arm-kernel/20241115162120.83990-3-detlev.casanova@collabora.com/T/#m82b38f4a83c4793bb82919bf736b2f6bd804a283

Detlev.

On Monday, 25 November 2024 02:55:41 EST Andy Yan wrote:
> Hello Detlev,
> 
> At 2024-11-16 00:20:39, "Detlev Casanova" <detlev.casanova@collabora.com> 
wrote:
> >The clock reset must be used when the VOP is configured. Skipping it can
> >put the VOP in an unknown state where the HDMI signal is either lost or
> >not matching the selected mode.
> 
> Can you provide some detail about how to reproduce this issue ?
> If we can reproduce this issue, we might be able to do a more deep analysis.
> >This adds support for rk3588(s) based SoCs.
> >
> >Changes since v3:
> >- Rebased on drm-misc-next
> >- Reword first patch subject
> >- Reorder commits for different trees
> >
> >Changes since v2:
> >- Rebase on latest master
> >- Add details on how to reproduce the issue
> >
> >Changes since v1:
> >- Add AXI and AHB clock resets
> >- Set maxItems for !rk3588 in vop2 bindings
> >
> >Detlev Casanova (3):
> >  dt-bindings: display: vop2: Add VP clock resets
> >  drm/rockchip: vop2: Add clock resets support
> >  arm64: dts: rockchip: Add VOP clock resets for rk3588s
> > 
> > .../display/rockchip/rockchip-vop2.yaml       | 40 +++++++++++++++++++
> > arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 12 ++++++
> > drivers/gpu/drm/rockchip/rockchip_drm_vop2.c  | 30 ++++++++++++++
> > 3 files changed, 82 insertions(+)