[PATCH] drm/rockchip: dsi: Reset ISP1 DPHY before powering it on

[PATCH] drm/rockchip: dsi: Reset ISP1 DPHY before powering it on

[Dragan Simic]

From: Ondrej Jirman <megi@xff.cz>

After a suspend and resume cycle, ISP1 stops receiving data, as observed
on the Pine64 PinePhone Pro, which is based on the Rockchip RK3399 SoC.
Re-initializing DPHY during the PHY power-on, if the SoC variant supports
initialization, fixes this issue.

[ dsimic: Added more details to the commit summary and description ]

Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
index 4cc8ed8f4fbd..9ad48c6dfac3 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
@@ -1240,6 +1240,14 @@ static int dw_mipi_dsi_dphy_power_on(struct phy *phy)
 		goto err_phy_cfg_clk;
 	}
 
+	if (dsi->cdata->dphy_rx_init) {
+		ret = dsi->cdata->dphy_rx_init(phy);
+		if (ret < 0) {
+			DRM_DEV_ERROR(dsi->dev, "hardware-specific phy init failed: %d\n", ret);
+			goto err_pwr_on;
+		}
+	}
+
 	/* do soc-variant specific init */
 	if (dsi->cdata->dphy_rx_power_on) {
 		ret = dsi->cdata->dphy_rx_power_on(phy);

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: dsi: Reset ISP1 DPHY before powering it on

[Dragan Simic]

Hello all,

On 2024-07-17 08:29, Dragan Simic wrote:
> From: Ondrej Jirman <megi@xff.cz>
> 
> After a suspend and resume cycle, ISP1 stops receiving data, as 
> observed
> on the Pine64 PinePhone Pro, which is based on the Rockchip RK3399 SoC.
> Re-initializing DPHY during the PHY power-on, if the SoC variant 
> supports
> initialization, fixes this issue.
> 
> [ dsimic: Added more details to the commit summary and description ]
> 
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>  drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> index 4cc8ed8f4fbd..9ad48c6dfac3 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> @@ -1240,6 +1240,14 @@ static int dw_mipi_dsi_dphy_power_on(struct phy 
> *phy)
>  		goto err_phy_cfg_clk;
>  	}
> 
> +	if (dsi->cdata->dphy_rx_init) {
> +		ret = dsi->cdata->dphy_rx_init(phy);
> +		if (ret < 0) {
> +			DRM_DEV_ERROR(dsi->dev, "hardware-specific phy init failed: %d\n", 
> ret);
> +			goto err_pwr_on;
> +		}
> +	}
> +
>  	/* do soc-variant specific init */
>  	if (dsi->cdata->dphy_rx_power_on) {
>  		ret = dsi->cdata->dphy_rx_power_on(phy);

After thinking a bit more about this patch in its original form [1]
that's preserved above, I think it would be better to move the
additional DPHY initialization to dw_mipi_dsi_rockchip_resume(),
because that function seems to be the right place for such fixes.

Please, let me know your thoughts.

[1] 
https://megous.com/git/linux/commit/?h=orange-pi-6.9&id=ed7992f668a1e529719ee6847ca114f9b67efacb

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: dsi: Reset ISP1 DPHY before powering it on

[Ondřej Jirman]

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

On Wed, Jul 17, 2024 at 08:48:29AM GMT, Dragan Simic wrote:
> Hello all,
> 
> On 2024-07-17 08:29, Dragan Simic wrote:
> > From: Ondrej Jirman <megi@xff.cz>
> > 
> > After a suspend and resume cycle, ISP1 stops receiving data, as observed
> > on the Pine64 PinePhone Pro, which is based on the Rockchip RK3399 SoC.
> > Re-initializing DPHY during the PHY power-on, if the SoC variant
> > supports
> > initialization, fixes this issue.
> > 
> > [ dsimic: Added more details to the commit summary and description ]
> > 
> > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> > Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> > ---
> >  drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> > b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> > index 4cc8ed8f4fbd..9ad48c6dfac3 100644
> > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> > @@ -1240,6 +1240,14 @@ static int dw_mipi_dsi_dphy_power_on(struct phy
> > *phy)
> >  		goto err_phy_cfg_clk;
> >  	}
> > 
> > +	if (dsi->cdata->dphy_rx_init) {
> > +		ret = dsi->cdata->dphy_rx_init(phy);
> > +		if (ret < 0) {
> > +			DRM_DEV_ERROR(dsi->dev, "hardware-specific phy init failed: %d\n",
> > ret);
> > +			goto err_pwr_on;
> > +		}
> > +	}
> > +
> >  	/* do soc-variant specific init */
> >  	if (dsi->cdata->dphy_rx_power_on) {
> >  		ret = dsi->cdata->dphy_rx_power_on(phy);
> 
> After thinking a bit more about this patch in its original form [1]
> that's preserved above, I think it would be better to move the
> additional DPHY initialization to dw_mipi_dsi_rockchip_resume(),
> because that function seems to be the right place for such fixes.
> 
> Please, let me know your thoughts.

That also works (see attachment) to fix the original issue in the commit
message, but if you keep the stream on across suspend/resume it does halt so
it's not a complete solution either.

Kind regards,
	o.

> [1] https://megous.com/git/linux/commit/?h=orange-pi-6.9&id=ed7992f668a1e529719ee6847ca114f9b67efacb

[-- Attachment #2: 0001-drm-rockchip-dw-mipi-dsi-rockchip-Restore-DPHY-confi.patch --]
[-- Type: text/plain, Size: 2657 bytes --]

From 4db7a9a913bb4b78094d7845295d2c3306513c56 Mon Sep 17 00:00:00 2001
From: Ondrej Jirman <megi@xff.cz>
Date: Wed, 17 Jul 2024 09:30:39 +0200
Subject: [PATCH] drm: rockchip: dw-mipi-dsi-rockchip: Restore DPHY config on
 resume

After a suspend and resume cycle, ISP1 stops receiving data, as observed on the
Pine64 PinePhone Pro, which is based on the Rockchip RK3399 SoC. Re-initializing
DPHY during resume, if the SoC variant supports initialization, fixes this
issue.

Signed-off-by: Ondrej Jirman <megi@xff.cz>
---
 .../gpu/drm/rockchip/dw-mipi-dsi-rockchip.c   | 51 ++++++++++++-------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
index 364b738b6935..c2b58e545080 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
@@ -1118,6 +1118,31 @@ static const struct component_ops dw_mipi_dsi_rockchip_dphy_ops = {
 	.unbind	= dw_mipi_dsi_rockchip_dphy_unbind,
 };
 
+static int dw_mipi_dsi_dphy_rx_init(struct dw_mipi_dsi_rockchip *dsi)
+{
+	int ret;
+
+	if (dsi->cdata->dphy_rx_init) {
+		ret = clk_prepare_enable(dsi->pclk);
+		if (ret < 0)
+			return ret;
+
+		ret = clk_prepare_enable(dsi->grf_clk);
+		if (ret) {
+			clk_disable_unprepare(dsi->pclk);
+			return ret;
+		}
+
+		ret = dsi->cdata->dphy_rx_init(dsi->dphy);
+		clk_disable_unprepare(dsi->grf_clk);
+		clk_disable_unprepare(dsi->pclk);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int dw_mipi_dsi_dphy_init(struct phy *phy)
 {
 	struct dw_mipi_dsi_rockchip *dsi = phy_get_drvdata(phy);
@@ -1138,23 +1163,9 @@ static int dw_mipi_dsi_dphy_init(struct phy *phy)
 	if (ret < 0)
 		goto err_graph;
 
-	if (dsi->cdata->dphy_rx_init) {
-		ret = clk_prepare_enable(dsi->pclk);
-		if (ret < 0)
-			goto err_init;
-
-		ret = clk_prepare_enable(dsi->grf_clk);
-		if (ret) {
-			clk_disable_unprepare(dsi->pclk);
-			goto err_init;
-		}
-
-		ret = dsi->cdata->dphy_rx_init(phy);
-		clk_disable_unprepare(dsi->grf_clk);
-		clk_disable_unprepare(dsi->pclk);
-		if (ret < 0)
-			goto err_init;
-	}
+	ret = dw_mipi_dsi_dphy_rx_init(dsi);
+	if (ret < 0)
+		goto err_init;
 
 	return 0;
 
@@ -1337,6 +1348,12 @@ static int __maybe_unused dw_mipi_dsi_rockchip_resume(struct device *dev)
 			dw_mipi_dsi_rockchip_config(dsi->slave);
 
 		clk_disable_unprepare(dsi->grf_clk);
+	} else if (dsi->usage_mode == DW_DSI_USAGE_PHY) {
+		ret = dw_mipi_dsi_dphy_rx_init(dsi);
+		if (ret < 0) {
+			DRM_DEV_ERROR(dsi->dev, "hardware-specific phy init failed: %d\n", ret);
+			return ret;
+		}
 	}
 
 	return 0;
-- 
2.45.2


[-- Attachment #3: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: dsi: Reset ISP1 DPHY before powering it on

[Dragan Simic]

Hello Ondrej,

On 2024-07-17 09:32, Ondřej Jirman wrote:
> On Wed, Jul 17, 2024 at 08:48:29AM GMT, Dragan Simic wrote:
>> Hello all,
>> 
>> On 2024-07-17 08:29, Dragan Simic wrote:
>> > From: Ondrej Jirman <megi@xff.cz>
>> >
>> > After a suspend and resume cycle, ISP1 stops receiving data, as observed
>> > on the Pine64 PinePhone Pro, which is based on the Rockchip RK3399 SoC.
>> > Re-initializing DPHY during the PHY power-on, if the SoC variant
>> > supports
>> > initialization, fixes this issue.
>> >
>> > [ dsimic: Added more details to the commit summary and description ]
>> >
>> > Signed-off-by: Ondrej Jirman <megi@xff.cz>
>> > Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> > ---
>> >  drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 8 ++++++++
>> >  1 file changed, 8 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
>> > b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
>> > index 4cc8ed8f4fbd..9ad48c6dfac3 100644
>> > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
>> > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
>> > @@ -1240,6 +1240,14 @@ static int dw_mipi_dsi_dphy_power_on(struct phy
>> > *phy)
>> >  		goto err_phy_cfg_clk;
>> >  	}
>> >
>> > +	if (dsi->cdata->dphy_rx_init) {
>> > +		ret = dsi->cdata->dphy_rx_init(phy);
>> > +		if (ret < 0) {
>> > +			DRM_DEV_ERROR(dsi->dev, "hardware-specific phy init failed: %d\n",
>> > ret);
>> > +			goto err_pwr_on;
>> > +		}
>> > +	}
>> > +
>> >  	/* do soc-variant specific init */
>> >  	if (dsi->cdata->dphy_rx_power_on) {
>> >  		ret = dsi->cdata->dphy_rx_power_on(phy);
>> 
>> After thinking a bit more about this patch in its original form [1]
>> that's preserved above, I think it would be better to move the
>> additional DPHY initialization to dw_mipi_dsi_rockchip_resume(),
>> because that function seems to be the right place for such fixes.
>> 
>> Please, let me know your thoughts.
> 
> That also works (see attachment) to fix the original issue in the 
> commit
> message, but if you keep the stream on across suspend/resume it does 
> halt so
> it's not a complete solution either.

Great, thanks for the attached patch.  I assume that you already have
a patch that performs the other required operations on suspend and 
resume,
i.e. stops the stream and restarts it?

How about dropping my "handled" variant of your patch and having you
submit the patch you sent as attachment, and the additional patch you
described as also needed?

>> [1] 
>> https://megous.com/git/linux/commit/?h=orange-pi-6.9&id=ed7992f668a1e529719ee6847ca114f9b67efacb

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip