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

From: "Andy Yan" <andyshrk@163.com>
To: "Detlev Casanova" <detlev.casanova@collabora.com>
Cc: linux-kernel@vger.kernel.org, "Sandy Huang" <hjc@rock-chips.com>,
	"Heiko Stübner" <heiko@sntech.de>,
	"Andy Yan" <andy.yan@rock-chips.com>,
	"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Thomas Zimmermann" <tzimmermann@suse.de>,
	"David Airlie" <airlied@gmail.com>,
	"Daniel Vetter" <daniel@ffwll.ch>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Kozlowski" <krzk+dt@kernel.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Heiko Stuebner" <heiko.stuebner@cherry.de>,
	"Sebastian Reichel" <sebastian.reichel@collabora.com>,
	"Dragan Simic" <dsimic@manjaro.org>,
	"Chris Morgan" <macromorgan@hotmail.com>,
	"Diederik de Haas" <didi.debian@cknow.org>,
	"Boris Brezillon" <boris.brezillon@collabora.com>,
	dri-devel@lists.freedesktop.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, devicetree@vger.kernel.org
Subject: Re:[PATCH v2 1/3] vop2: Add clock resets support
Date: Fri, 24 May 2024 11:09:26 +0800 (CST)	[thread overview]
Message-ID: <6a3d3fb1.3755.18fa893239e.Coremail.andyshrk@163.com> (raw)
In-Reply-To: <20240522185924.461742-2-detlev.casanova@collabora.com>

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