diff mbox series

[v2,2/2] drm/bridge: ti-sn65dsi83: Fix enable/disable flow to meet spec

Message ID 20230503163313.2640898-3-frieder@fris.de (mailing list archive)
State New, archived
Headers show
Series Init flow fixes for Samsung DSIM and TI SN65DSI84 | expand

Commit Message

Frieder Schrempf May 3, 2023, 4:33 p.m. UTC 
From: Frieder Schrempf <frieder.schrempf@kontron.de>

The datasheet describes the following initialization flow including
minimum delay times between each step:

1. DSI data lanes need to be in LP-11 and the clock lane in HS mode
2. toggle EN signal
3. initialize registers
4. enable PLL
5. soft reset
6. enable DSI stream
7. check error status register

To meet this requirement we need to make sure the host bridge's
pre_enable() is called first by using the pre_enable_prev_first
flag.

Furthermore we need to split enable() into pre_enable() which covers
steps 2-5 from above and enable() which covers step 7 and is called
after the host bridge's enable().

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
Changes for v2:
* Drop RFC
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Comments

Alexander Stein May 4, 2023, 9:11 a.m. UTC | #1 
Am Mittwoch, 3. Mai 2023, 18:33:07 CEST schrieb Frieder Schrempf:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> The datasheet describes the following initialization flow including
> minimum delay times between each step:
> 
> 1. DSI data lanes need to be in LP-11 and the clock lane in HS mode
> 2. toggle EN signal
> 3. initialize registers
> 4. enable PLL
> 5. soft reset
> 6. enable DSI stream
> 7. check error status register
> 
> To meet this requirement we need to make sure the host bridge's
> pre_enable() is called first by using the pre_enable_prev_first
> flag.
> 
> Furthermore we need to split enable() into pre_enable() which covers
> steps 2-5 from above and enable() which covers step 7 and is called
> after the host bridge's enable().
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com> #TQMa8MxML/MBa8Mx

> ---
> Changes for v2:
> * Drop RFC
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 75286c9afbb9..a82f10b8109f
> 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -321,8 +321,8 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
>  	return dsi_div - 1;
>  }
> 
> -static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
> -				    struct drm_bridge_state 
*old_bridge_state)
> +static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
> +					struct drm_bridge_state 
*old_bridge_state)
>  {
>  	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>  	struct drm_atomic_state *state = old_bridge_state->base.state;
> @@ -484,11 +484,22 @@ static void sn65dsi83_atomic_enable(struct drm_bridge
> *bridge, /* Trigger reset after CSR register update. */
>  	regmap_write(ctx->regmap, REG_RC_RESET, REG_RC_RESET_SOFT_RESET);
> 
> +	/* Wait for 10ms after soft reset as specified in datasheet */
> +	usleep_range(10000, 12000);
> +}
> +
> +static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
> +				    struct drm_bridge_state 
*old_bridge_state)
> +{
> +	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> +	unsigned int pval;
> +
>  	/* Clear all errors that got asserted during initialization. */
>  	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>  	regmap_write(ctx->regmap, REG_IRQ_STAT, pval);
> 
> -	usleep_range(10000, 12000);
> +	/* Wait for 1ms and check for errors in status register */
> +	usleep_range(1000, 1100);
>  	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>  	if (pval)
>  		dev_err(ctx->dev, "Unexpected link status 0x%02x\n", 
pval);
> @@ -555,6 +566,7 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = {
> .attach			= sn65dsi83_attach,
>  	.detach			= sn65dsi83_detach,
>  	.atomic_enable		= sn65dsi83_atomic_enable,
> +	.atomic_pre_enable	= sn65dsi83_atomic_pre_enable,
>  	.atomic_disable		= sn65dsi83_atomic_disable,
>  	.mode_valid		= sn65dsi83_mode_valid,
> 
> @@ -697,6 +709,7 @@ static int sn65dsi83_probe(struct i2c_client *client)
> 
>  	ctx->bridge.funcs = &sn65dsi83_funcs;
>  	ctx->bridge.of_node = dev->of_node;
> +	ctx->bridge.pre_enable_prev_first = true;
>  	drm_bridge_add(&ctx->bridge);
> 
>  	ret = sn65dsi83_host_attach(ctx);
Neil Armstrong May 16, 2023, 7:33 a.m. UTC | #2 
On 03/05/2023 18:33, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> The datasheet describes the following initialization flow including
> minimum delay times between each step:
> 
> 1. DSI data lanes need to be in LP-11 and the clock lane in HS mode
> 2. toggle EN signal
> 3. initialize registers
> 4. enable PLL
> 5. soft reset
> 6. enable DSI stream
> 7. check error status register
> 
> To meet this requirement we need to make sure the host bridge's
> pre_enable() is called first by using the pre_enable_prev_first
> flag.
> 
> Furthermore we need to split enable() into pre_enable() which covers
> steps 2-5 from above and enable() which covers step 7 and is called
> after the host bridge's enable().
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
> Changes for v2:
> * Drop RFC
> ---
>   drivers/gpu/drm/bridge/ti-sn65dsi83.c | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index 75286c9afbb9..a82f10b8109f 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -321,8 +321,8 @@ static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
>   	return dsi_div - 1;
>   }
>   
> -static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
> -				    struct drm_bridge_state *old_bridge_state)
> +static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
> +					struct drm_bridge_state *old_bridge_state)
>   {
>   	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>   	struct drm_atomic_state *state = old_bridge_state->base.state;
> @@ -484,11 +484,22 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
>   	/* Trigger reset after CSR register update. */
>   	regmap_write(ctx->regmap, REG_RC_RESET, REG_RC_RESET_SOFT_RESET);
>   
> +	/* Wait for 10ms after soft reset as specified in datasheet */
> +	usleep_range(10000, 12000);
> +}
> +
> +static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
> +				    struct drm_bridge_state *old_bridge_state)
> +{
> +	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> +	unsigned int pval;
> +
>   	/* Clear all errors that got asserted during initialization. */
>   	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>   	regmap_write(ctx->regmap, REG_IRQ_STAT, pval);
>   
> -	usleep_range(10000, 12000);
> +	/* Wait for 1ms and check for errors in status register */
> +	usleep_range(1000, 1100);
>   	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
>   	if (pval)
>   		dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);
> @@ -555,6 +566,7 @@ static const struct drm_bridge_funcs sn65dsi83_funcs = {
>   	.attach			= sn65dsi83_attach,
>   	.detach			= sn65dsi83_detach,
>   	.atomic_enable		= sn65dsi83_atomic_enable,
> +	.atomic_pre_enable	= sn65dsi83_atomic_pre_enable,
>   	.atomic_disable		= sn65dsi83_atomic_disable,
>   	.mode_valid		= sn65dsi83_mode_valid,
>   
> @@ -697,6 +709,7 @@ static int sn65dsi83_probe(struct i2c_client *client)
>   
>   	ctx->bridge.funcs = &sn65dsi83_funcs;
>   	ctx->bridge.of_node = dev->of_node;
> +	ctx->bridge.pre_enable_prev_first = true;
>   	drm_bridge_add(&ctx->bridge);
>   
>   	ret = sn65dsi83_host_attach(ctx);

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Fabio Estevam May 16, 2023, 10:22 p.m. UTC | #3 
On Thu, May 4, 2023 at 6:12 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Am Mittwoch, 3. Mai 2023, 18:33:07 CEST schrieb Frieder Schrempf:
> > From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >
> > The datasheet describes the following initialization flow including
> > minimum delay times between each step:
> >
> > 1. DSI data lanes need to be in LP-11 and the clock lane in HS mode
> > 2. toggle EN signal
> > 3. initialize registers
> > 4. enable PLL
> > 5. soft reset
> > 6. enable DSI stream
> > 7. check error status register
> >
> > To meet this requirement we need to make sure the host bridge's
> > pre_enable() is called first by using the pre_enable_prev_first
> > flag.
> >
> > Furthermore we need to split enable() into pre_enable() which covers
> > steps 2-5 from above and enable() which covers step 7 and is called
> > after the host bridge's enable().
> >
> > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>
> Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com> #TQMa8MxML/MBa8Mx

Should this have a Fixes tag so that it could be backported to stable kernels?
Frieder Schrempf May 22, 2023, 1:29 p.m. UTC | #4 
On 17.05.23 00:22, Fabio Estevam wrote:
> On Thu, May 4, 2023 at 6:12 AM Alexander Stein
> <alexander.stein@ew.tq-group.com> wrote:
>>
>> Am Mittwoch, 3. Mai 2023, 18:33:07 CEST schrieb Frieder Schrempf:
>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>
>>> The datasheet describes the following initialization flow including
>>> minimum delay times between each step:
>>>
>>> 1. DSI data lanes need to be in LP-11 and the clock lane in HS mode
>>> 2. toggle EN signal
>>> 3. initialize registers
>>> 4. enable PLL
>>> 5. soft reset
>>> 6. enable DSI stream
>>> 7. check error status register
>>>
>>> To meet this requirement we need to make sure the host bridge's
>>> pre_enable() is called first by using the pre_enable_prev_first
>>> flag.
>>>
>>> Furthermore we need to split enable() into pre_enable() which covers
>>> steps 2-5 from above and enable() which covers step 7 and is called
>>> after the host bridge's enable().
>>>
>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com> #TQMa8MxML/MBa8Mx
> 
> Should this have a Fixes tag so that it could be backported to stable kernels?

As this depends on the support for the pre_enable_prev_first flag,
currently the only candidates for backporting would be 6.3 and 6.4.

I can't tell if there are DSI host drivers which already implement the
proper init flow and would benefit from a backport.

Anyway, it shouldn't be a problem either so I guess the proper tags
would look like:

Cc: <stable@vger.kernel.org> # 6.3.x, 6.4.x
Fixes: ceb515ba29ba ("drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and
SN65DSI84 driver")
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 75286c9afbb9..a82f10b8109f 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -321,8 +321,8 @@  static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
 	return dsi_div - 1;
 }
 
-static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
-				    struct drm_bridge_state *old_bridge_state)
+static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
+					struct drm_bridge_state *old_bridge_state)
 {
 	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
 	struct drm_atomic_state *state = old_bridge_state->base.state;
@@ -484,11 +484,22 @@  static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
 	/* Trigger reset after CSR register update. */
 	regmap_write(ctx->regmap, REG_RC_RESET, REG_RC_RESET_SOFT_RESET);
 
+	/* Wait for 10ms after soft reset as specified in datasheet */
+	usleep_range(10000, 12000);
+}
+
+static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
+				    struct drm_bridge_state *old_bridge_state)
+{
+	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
+	unsigned int pval;
+
 	/* Clear all errors that got asserted during initialization. */
 	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
 	regmap_write(ctx->regmap, REG_IRQ_STAT, pval);
 
-	usleep_range(10000, 12000);
+	/* Wait for 1ms and check for errors in status register */
+	usleep_range(1000, 1100);
 	regmap_read(ctx->regmap, REG_IRQ_STAT, &pval);
 	if (pval)
 		dev_err(ctx->dev, "Unexpected link status 0x%02x\n", pval);
@@ -555,6 +566,7 @@  static const struct drm_bridge_funcs sn65dsi83_funcs = {
 	.attach			= sn65dsi83_attach,
 	.detach			= sn65dsi83_detach,
 	.atomic_enable		= sn65dsi83_atomic_enable,
+	.atomic_pre_enable	= sn65dsi83_atomic_pre_enable,
 	.atomic_disable		= sn65dsi83_atomic_disable,
 	.mode_valid		= sn65dsi83_mode_valid,
 
@@ -697,6 +709,7 @@  static int sn65dsi83_probe(struct i2c_client *client)
 
 	ctx->bridge.funcs = &sn65dsi83_funcs;
 	ctx->bridge.of_node = dev->of_node;
+	ctx->bridge.pre_enable_prev_first = true;
 	drm_bridge_add(&ctx->bridge);
 
 	ret = sn65dsi83_host_attach(ctx);