diff mbox series

[6.1.y-cip,05/20] drm: rcar-du: rzg2l_mipi_dsi: Enhance device lanes check

Message ID 20230905160737.167877-6-biju.das.jz@bp.renesas.com (mailing list archive)
State Accepted
Headers show
Series Add Renesas RZ/G2L DSI,VSP,FCP support | expand

Commit Message

Biju Das Sept. 5, 2023, 4:07 p.m. UTC 
commit 418bb3a69e1355c8977c528f8f3896a0aaaecb80 upstream.

Enhance device lanes check by reading TXSETR register at probe(),
and enforced in rzg2l_mipi_dsi_host_attach().

As per HW manual, we can read TXSETR register only after
DPHY initialization.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/gpu/drm/rcar-du/rzg2l_mipi_dsi.c | 122 ++++++++++++++++-------
 1 file changed, 88 insertions(+), 34 deletions(-)

Comments

Pavel Machek Sept. 6, 2023, 8:46 a.m. UTC | #1 
Hi!

> commit 418bb3a69e1355c8977c528f8f3896a0aaaecb80 upstream.
> 
> Enhance device lanes check by reading TXSETR register at probe(),
> and enforced in rzg2l_mipi_dsi_host_attach().
> 
> As per HW manual, we can read TXSETR register only after
> DPHY initialization.

So this is kind of strange:


> +++ b/drivers/gpu/drm/rcar-du/rzg2l_mipi_dsi.c
> @@ -220,12 +204,6 @@ static int rzg2l_mipi_dsi_startup(struct rzg2l_mipi_dsi *dsi,
>  			break;
>  	}
>  
> -	ret = pm_runtime_resume_and_get(dsi->dev);
> -	if (ret < 0)
> -		return ret;
> -
> -	clk_set_rate(dsi->vclk, mode->clock * 1000);
> -
>  	/* Initializing DPHY before accessing LINK */
>  	dphyctrl0 = DSIDPHYCTRL0_CAL_EN_HSRX_OFS | DSIDPHYCTRL0_CMN_MASTER_EN |
>  		    DSIDPHYCTRL0_RE_VDD_DETVCCQLV18 | DSIDPHYCTRL0_EN_BGR;
> @@ -259,10 +237,62 @@ static int rzg2l_mipi_dsi_startup(struct rzg2l_mipi_dsi *dsi,
>  
>  	ret = reset_control_deassert(dsi->rstc);
>  	if (ret < 0)
> -		goto err_pm_put;
> +		return ret;
>  
>  	udelay(1);
>  
> +	return 0;
> +}

If dsi_startup fails, it does not undo the hw register writes.

> +static void rzg2l_mipi_dsi_dphy_exit(struct rzg2l_mipi_dsi *dsi)
> +{
> +	u32 dphyctrl0;
> +
> +	dphyctrl0 = rzg2l_mipi_dsi_phy_read(dsi, DSIDPHYCTRL0);
> +
> +	dphyctrl0 &= ~(DSIDPHYCTRL0_EN_LDO1200 | DSIDPHYCTRL0_EN_BGR);
> +	rzg2l_mipi_dsi_phy_write(dsi, DSIDPHYCTRL0, dphyctrl0);
> +
> +	reset_control_assert(dsi->rstc);
> +}

If dsi_startup fails, this undoes the register writes, but
reset_control_assert is now redundant.

> +	ret = rzg2l_mipi_dsi_dphy_init(dsi, hsfreq);
> +	if (ret < 0)
> +		goto err_phy;
> +
>  	/* Enable Data lanes and Clock lanes */
>  	txsetr = TXSETR_DLEN | TXSETR_NUMLANEUSE(dsi->lanes - 1) | TXSETR_CLEN;
>  	rzg2l_mipi_dsi_link_write(dsi, TXSETR, txsetr);
> @@ -301,7 +331,8 @@ static int rzg2l_mipi_dsi_startup(struct rzg2l_mipi_dsi *dsi,
>  
>  	return 0;
>  
> -err_pm_put:
> +err_phy:
> +	rzg2l_mipi_dsi_dphy_exit(dsi);
>  	pm_runtime_put(dsi->dev);
>

So this looks strange.

My suggestion would be "rzg2l_mipi_dsi_dphy_init() cleans for itself
in case of errors" (as we usually do), so there's no need to do
rzg2l_mipi_dsi_dphy_exit() in casa of failure.

Best regards,
								Pavel
Biju Das Sept. 6, 2023, 9:07 a.m. UTC | #2 
Hi Pavel,

> Subject: Re: [cip-dev] [PATCH 6.1.y-cip 05/20] drm: rcar-du:
> rzg2l_mipi_dsi: Enhance device lanes check
> 
> Hi!
> 
> > commit 418bb3a69e1355c8977c528f8f3896a0aaaecb80 upstream.
> >
> > Enhance device lanes check by reading TXSETR register at probe(), and
> > enforced in rzg2l_mipi_dsi_host_attach().
> >
> > As per HW manual, we can read TXSETR register only after DPHY
> > initialization.
> 
> So this is kind of strange:

OK.

> 
> 
> > +++ b/drivers/gpu/drm/rcar-du/rzg2l_mipi_dsi.c
> > @@ -220,12 +204,6 @@ static int rzg2l_mipi_dsi_startup(struct
> rzg2l_mipi_dsi *dsi,
> >  			break;
> >  	}
> >
> > -	ret = pm_runtime_resume_and_get(dsi->dev);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	clk_set_rate(dsi->vclk, mode->clock * 1000);
> > -
> >  	/* Initializing DPHY before accessing LINK */
> >  	dphyctrl0 = DSIDPHYCTRL0_CAL_EN_HSRX_OFS | DSIDPHYCTRL0_CMN_MASTER_EN
> |
> >  		    DSIDPHYCTRL0_RE_VDD_DETVCCQLV18 | DSIDPHYCTRL0_EN_BGR; @@
> > -259,10 +237,62 @@ static int rzg2l_mipi_dsi_startup(struct
> > rzg2l_mipi_dsi *dsi,
> >
> >  	ret = reset_control_deassert(dsi->rstc);
> >  	if (ret < 0)
> > -		goto err_pm_put;
> > +		return ret;
> >
> >  	udelay(1);
> >
> > +	return 0;
> > +}
> 
> If dsi_startup fails, it does not undo the hw register writes.

We don't need to undo all the hw register writes. The
one on the rzg2l_mipi_dsi_dphy_exit() is sufficient.

> 
> > +static void rzg2l_mipi_dsi_dphy_exit(struct rzg2l_mipi_dsi *dsi) {
> > +	u32 dphyctrl0;
> > +
> > +	dphyctrl0 = rzg2l_mipi_dsi_phy_read(dsi, DSIDPHYCTRL0);
> > +
> > +	dphyctrl0 &= ~(DSIDPHYCTRL0_EN_LDO1200 | DSIDPHYCTRL0_EN_BGR);
> > +	rzg2l_mipi_dsi_phy_write(dsi, DSIDPHYCTRL0, dphyctrl0);
> > +
> > +	reset_control_assert(dsi->rstc);
> > +}
> 
> If dsi_startup fails, this undoes the register writes, but
> reset_control_assert is now redundant.

Since we are using exclusive reset controller guarantees that the reset will be asserted[1].

https://elixir.bootlin.com/linux/latest/source/drivers/reset/core.c#L430

> 
> > +	ret = rzg2l_mipi_dsi_dphy_init(dsi, hsfreq);
> > +	if (ret < 0)
> > +		goto err_phy;
> > +
> >  	/* Enable Data lanes and Clock lanes */
> >  	txsetr = TXSETR_DLEN | TXSETR_NUMLANEUSE(dsi->lanes - 1) |
> TXSETR_CLEN;
> >  	rzg2l_mipi_dsi_link_write(dsi, TXSETR, txsetr); @@ -301,7 +331,8 @@
> > static int rzg2l_mipi_dsi_startup(struct rzg2l_mipi_dsi *dsi,
> >
> >  	return 0;
> >
> > -err_pm_put:
> > +err_phy:
> > +	rzg2l_mipi_dsi_dphy_exit(dsi);
> >  	pm_runtime_put(dsi->dev);
> >
> 
> So this looks strange.
> 
> My suggestion would be "rzg2l_mipi_dsi_dphy_init() cleans for itself in
> case of errors" (as we usually do), so there's no need to do
> rzg2l_mipi_dsi_dphy_exit() in casa of failure.

If I correctly remember, the R-Car DU maintainer wants
balanced API's.

Cheers,
Biju
diff mbox series

Patch

diff --git a/drivers/gpu/drm/rcar-du/rzg2l_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rzg2l_mipi_dsi.c
index 8579208db218..aa95b85a2964 100644
--- a/drivers/gpu/drm/rcar-du/rzg2l_mipi_dsi.c
+++ b/drivers/gpu/drm/rcar-du/rzg2l_mipi_dsi.c
@@ -171,6 +171,11 @@  static void rzg2l_mipi_dsi_link_write(struct rzg2l_mipi_dsi *dsi, u32 reg, u32 d
 	iowrite32(data, dsi->mmio + LINK_REG_OFFSET + reg);
 }
 
+static u32 rzg2l_mipi_dsi_phy_read(struct rzg2l_mipi_dsi *dsi, u32 reg)
+{
+	return ioread32(dsi->mmio + reg);
+}
+
 static u32 rzg2l_mipi_dsi_link_read(struct rzg2l_mipi_dsi *dsi, u32 reg)
 {
 	return ioread32(dsi->mmio + LINK_REG_OFFSET + reg);
@@ -180,19 +185,11 @@  static u32 rzg2l_mipi_dsi_link_read(struct rzg2l_mipi_dsi *dsi, u32 reg)
  * Hardware Setup
  */
 
-static int rzg2l_mipi_dsi_startup(struct rzg2l_mipi_dsi *dsi,
-				  const struct drm_display_mode *mode)
+static int rzg2l_mipi_dsi_dphy_init(struct rzg2l_mipi_dsi *dsi,
+				    unsigned long hsfreq)
 {
 	const struct rzg2l_mipi_dsi_timings *dphy_timings;
-	unsigned long hsfreq;
-	unsigned int i, bpp;
-	u32 txsetr;
-	u32 clstptsetr;
-	u32 lptrnstsetr;
-	u32 clkkpt;
-	u32 clkbfht;
-	u32 clkstpt;
-	u32 golpbkt;
+	unsigned int i;
 	u32 dphyctrl0;
 	u32 dphytim0;
 	u32 dphytim1;
@@ -200,19 +197,6 @@  static int rzg2l_mipi_dsi_startup(struct rzg2l_mipi_dsi *dsi,
 	u32 dphytim3;
 	int ret;
 
-	/*
-	 * Relationship between hsclk and vclk must follow
-	 * vclk * bpp = hsclk * 8 * lanes
-	 * where vclk: video clock (Hz)
-	 *       bpp: video pixel bit depth
-	 *       hsclk: DSI HS Byte clock frequency (Hz)
-	 *       lanes: number of data lanes
-	 *
-	 * hsclk(bit) = hsclk(byte) * 8
-	 */
-	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
-	hsfreq = (mode->clock * bpp * 8) / (8 * dsi->lanes);
-
 	/* All DSI global operation timings are set with recommended setting */
 	for (i = 0; i < ARRAY_SIZE(rzg2l_mipi_dsi_global_timings); ++i) {
 		dphy_timings = &rzg2l_mipi_dsi_global_timings[i];
@@ -220,12 +204,6 @@  static int rzg2l_mipi_dsi_startup(struct rzg2l_mipi_dsi *dsi,
 			break;
 	}
 
-	ret = pm_runtime_resume_and_get(dsi->dev);
-	if (ret < 0)
-		return ret;
-
-	clk_set_rate(dsi->vclk, mode->clock * 1000);
-
 	/* Initializing DPHY before accessing LINK */
 	dphyctrl0 = DSIDPHYCTRL0_CAL_EN_HSRX_OFS | DSIDPHYCTRL0_CMN_MASTER_EN |
 		    DSIDPHYCTRL0_RE_VDD_DETVCCQLV18 | DSIDPHYCTRL0_EN_BGR;
@@ -259,10 +237,62 @@  static int rzg2l_mipi_dsi_startup(struct rzg2l_mipi_dsi *dsi,
 
 	ret = reset_control_deassert(dsi->rstc);
 	if (ret < 0)
-		goto err_pm_put;
+		return ret;
 
 	udelay(1);
 
+	return 0;
+}
+
+static void rzg2l_mipi_dsi_dphy_exit(struct rzg2l_mipi_dsi *dsi)
+{
+	u32 dphyctrl0;
+
+	dphyctrl0 = rzg2l_mipi_dsi_phy_read(dsi, DSIDPHYCTRL0);
+
+	dphyctrl0 &= ~(DSIDPHYCTRL0_EN_LDO1200 | DSIDPHYCTRL0_EN_BGR);
+	rzg2l_mipi_dsi_phy_write(dsi, DSIDPHYCTRL0, dphyctrl0);
+
+	reset_control_assert(dsi->rstc);
+}
+
+static int rzg2l_mipi_dsi_startup(struct rzg2l_mipi_dsi *dsi,
+				  const struct drm_display_mode *mode)
+{
+	unsigned long hsfreq;
+	unsigned int bpp;
+	u32 txsetr;
+	u32 clstptsetr;
+	u32 lptrnstsetr;
+	u32 clkkpt;
+	u32 clkbfht;
+	u32 clkstpt;
+	u32 golpbkt;
+	int ret;
+
+	/*
+	 * Relationship between hsclk and vclk must follow
+	 * vclk * bpp = hsclk * 8 * lanes
+	 * where vclk: video clock (Hz)
+	 *       bpp: video pixel bit depth
+	 *       hsclk: DSI HS Byte clock frequency (Hz)
+	 *       lanes: number of data lanes
+	 *
+	 * hsclk(bit) = hsclk(byte) * 8
+	 */
+	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
+	hsfreq = (mode->clock * bpp * 8) / (8 * dsi->lanes);
+
+	ret = pm_runtime_resume_and_get(dsi->dev);
+	if (ret < 0)
+		return ret;
+
+	clk_set_rate(dsi->vclk, mode->clock * 1000);
+
+	ret = rzg2l_mipi_dsi_dphy_init(dsi, hsfreq);
+	if (ret < 0)
+		goto err_phy;
+
 	/* Enable Data lanes and Clock lanes */
 	txsetr = TXSETR_DLEN | TXSETR_NUMLANEUSE(dsi->lanes - 1) | TXSETR_CLEN;
 	rzg2l_mipi_dsi_link_write(dsi, TXSETR, txsetr);
@@ -301,7 +331,8 @@  static int rzg2l_mipi_dsi_startup(struct rzg2l_mipi_dsi *dsi,
 
 	return 0;
 
-err_pm_put:
+err_phy:
+	rzg2l_mipi_dsi_dphy_exit(dsi);
 	pm_runtime_put(dsi->dev);
 
 	return ret;
@@ -309,7 +340,7 @@  static int rzg2l_mipi_dsi_startup(struct rzg2l_mipi_dsi *dsi,
 
 static void rzg2l_mipi_dsi_stop(struct rzg2l_mipi_dsi *dsi)
 {
-	reset_control_assert(dsi->rstc);
+	rzg2l_mipi_dsi_dphy_exit(dsi);
 	pm_runtime_put(dsi->dev);
 }
 
@@ -666,7 +697,9 @@  static const struct dev_pm_ops rzg2l_mipi_pm_ops = {
 
 static int rzg2l_mipi_dsi_probe(struct platform_device *pdev)
 {
+	unsigned int num_data_lanes;
 	struct rzg2l_mipi_dsi *dsi;
+	u32 txsetr;
 	int ret;
 
 	dsi = devm_kzalloc(&pdev->dev, sizeof(*dsi), GFP_KERNEL);
@@ -681,7 +714,7 @@  static int rzg2l_mipi_dsi_probe(struct platform_device *pdev)
 		return dev_err_probe(dsi->dev, ret,
 				     "missing or invalid data-lanes property\n");
 
-	dsi->num_data_lanes = ret;
+	num_data_lanes = ret;
 
 	dsi->mmio = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(dsi->mmio))
@@ -710,6 +743,24 @@  static int rzg2l_mipi_dsi_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(dsi->dev);
 
+	ret = pm_runtime_resume_and_get(dsi->dev);
+	if (ret < 0)
+		goto err_pm_disable;
+
+	/*
+	 * TXSETR register can be read only after DPHY init. But during probe
+	 * mode->clock and format are not available. So initialize DPHY with
+	 * timing parameters for 80Mbps.
+	 */
+	ret = rzg2l_mipi_dsi_dphy_init(dsi, 80000);
+	if (ret < 0)
+		goto err_phy;
+
+	txsetr = rzg2l_mipi_dsi_link_read(dsi, TXSETR);
+	dsi->num_data_lanes = min(((txsetr >> 16) & 3) + 1, num_data_lanes);
+	rzg2l_mipi_dsi_dphy_exit(dsi);
+	pm_runtime_put(dsi->dev);
+
 	/* Initialize the DRM bridge. */
 	dsi->bridge.funcs = &rzg2l_mipi_dsi_bridge_ops;
 	dsi->bridge.of_node = dsi->dev->of_node;
@@ -723,6 +774,9 @@  static int rzg2l_mipi_dsi_probe(struct platform_device *pdev)
 
 	return 0;
 
+err_phy:
+	rzg2l_mipi_dsi_dphy_exit(dsi);
+	pm_runtime_put(dsi->dev);
 err_pm_disable:
 	pm_runtime_disable(dsi->dev);
 	return ret;