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

[PATCH v2 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 v1:
- Add AXI and AHB clock resets
- Set maxItems for !rk3588 in vop2 bindings

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

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

-- 
2.44.1

[PATCH v2 1/3] 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.

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 fdd768bbd487c..e81a67161d29a 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;
@@ -1915,6 +1917,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)
 {
@@ -2055,6 +2077,8 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
 
+	vop2_clk_reset(vp);
+
 	drm_crtc_vblank_on(crtc);
 
 	vop2_unlock(vop2);
@@ -2706,6 +2730,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.44.1

[PATCH v2 2/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/rk3588s.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
index 6ac5ac8b48abb..490a525700498 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -1193,6 +1193,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.44.1

[PATCH v2 3/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.

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.44.1

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 235 bytes --]

On Wed, May 22, 2024 at 02:57:50PM -0400, Detlev Casanova wrote:
> 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>

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re:[PATCH v2 1/3] vop2: Add clock resets support

[Andy Yan]

Hi Detlev，


At 2024-05-23 02:57:48, "Detlev Casanova" <detlev.casanova@collabora.com> wrote:
>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.

Would you please provide a detailed description of your test case?


>
>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 fdd768bbd487c..e81a67161d29a 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;
>@@ -1915,6 +1917,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;


In fact, this check is not necessary here.  The following reset control api will check for NULL pointer。

>+
>+	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)
> {
>@@ -2055,6 +2077,8 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
> 
> 	vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
> 
>+	vop2_clk_reset(vp);
>+
> 	drm_crtc_vblank_on(crtc);
> 
> 	vop2_unlock(vop2);
>@@ -2706,6 +2730,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.44.1
>
>
>_______________________________________________
>linux-arm-kernel mailing list
>linux-arm-kernel@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] vop2: Add clock resets support

[Detlev Casanova]

On Thursday, 23 May 2024 23:09:26 EST Andy Yan wrote:
> Hi Detlev，
> 
> At 2024-05-23 02:57:48, "Detlev Casanova" <detlev.casanova@collabora.com> 
wrote:
> >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.
> 
> Would you please provide a detailed description of your test case?

The test case was to switch modes (using modetest) until the HDMI signal was 
lost on the TV side. It was also possible to detect the issue by tracking the 
HDMI TX Controller_VIDEO_MONITOR_STATUS[1-6] registers, especially at address 
0x890, where the register would take the value `0x0000018c`.

After adding these resets, the issue cannot be reproduced. I can share a 
script that reproduced this in the past (but this is an old patchset now, so 
things could have changed)

> >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
> >fdd768bbd487c..e81a67161d29a 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;
> >
> >@@ -1915,6 +1917,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;
> 
> In fact, this check is not necessary here.  The following reset control api
> will check for NULL pointer

Agreed, I'll do a rebased v3 and remove the check.

> >+
> >+	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)
> > 
> > {
> >
> >@@ -2055,6 +2077,8 @@ static void vop2_crtc_atomic_enable(struct drm_crtc
> >*crtc,>
> > 	vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
> >
> >+	vop2_clk_reset(vp);
> >+
> >
> > 	drm_crtc_vblank_on(crtc);
> > 	
> > 	vop2_unlock(vop2);
> >
> >@@ -2706,6 +2730,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);

Re: [PATCH v2 1/3] vop2: Add clock resets support

[Detlev Casanova]

On Friday, 8 November 2024 11:39:57 EST Detlev Casanova wrote:
> On Thursday, 23 May 2024 23:09:26 EST Andy Yan wrote:
> > Hi Detlev，
> > 
> > At 2024-05-23 02:57:48, "Detlev Casanova" <detlev.casanova@collabora.com>
> 
> wrote:
> > >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.
> > 
> > Would you please provide a detailed description of your test case?
> 
> The test case was to switch modes (using modetest) until the HDMI signal was
> lost on the TV side. It was also possible to detect the issue by tracking
> the HDMI TX Controller_VIDEO_MONITOR_STATUS[1-6] registers, especially at
> address 0x890, where the register would take the value `0x0000018c`.
> 
> After adding these resets, the issue cannot be reproduced. I can share a
> script that reproduced this in the past (but this is an old patchset now, so
> things could have changed)
> 
> > >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
> > >fdd768bbd487c..e81a67161d29a 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;
> > >
> > >@@ -1915,6 +1917,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;
> > 
> > In fact, this check is not necessary here.  The following reset control
> > api
> > will check for NULL pointer
> 
> Agreed, I'll do a rebased v3 and remove the check.

Actually, re-thinking about it, the check is done to avoid the udelay(10); if 
there is no resets configured, so I'd rather keep it that way.

> > >+
> > >+	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)
> 
> > > {
> > >
> > >@@ -2055,6 +2077,8 @@ static void vop2_crtc_atomic_enable(struct drm_crtc
> > >*crtc,>
> > >
> > > 	vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
> > >
> > >+	vop2_clk_reset(vp);
> > >+
> > >
> > > 	drm_crtc_vblank_on(crtc);
> > > 	
> > > 	vop2_unlock(vop2);
> > >
> > >@@ -2706,6 +2730,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);