[PATCH v2 0/2] Init flow fixes for Samsung DSIM and TI SN65DSI84

[PATCH v2 0/2] Init flow fixes for Samsung DSIM and TI SN65DSI84

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

This patchset contains a proposal to fix the initialization flow for
the display pipeline used on our i.MX8MM Kontron boards:

  i.MX8MM LCDIF -> i.MX8MM DSIM -> TI SN65DSI84 -> 7" LVDS Panel

Without these changes the display works most of the time, but fails
to come up occassionally when booting or doing on/off cycling tests
with:

  echo 0 > /sys/devices/platform/soc@0/32c00000.bus/32e00000.lcdif/graphics/fb0/blank
  echo 1 > /sys/devices/platform/soc@0/32c00000.bus/32e00000.lcdif/graphics/fb0/blank

All the changes intend to follow the documentation provided here:
https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

Changes for v2:
* Drop RFC
* Drop non-working Exynos cleanup patch 3/3

Frieder Schrempf (2):
  drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec
  drm/bridge: ti-sn65dsi83: Fix enable/disable flow to meet spec

 drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 19 ++++++++++++++++---
 2 files changed, 39 insertions(+), 5 deletions(-)

-- 
2.40.0

[PATCH v2 0/2] Init flow fixes for Samsung DSIM and TI SN65DSI84

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

This patchset contains a proposal to fix the initialization flow for
the display pipeline used on our i.MX8MM Kontron boards:

  i.MX8MM LCDIF -> i.MX8MM DSIM -> TI SN65DSI84 -> 7" LVDS Panel

Without these changes the display works most of the time, but fails
to come up occassionally when booting or doing on/off cycling tests
with:

  echo 0 > /sys/devices/platform/soc@0/32c00000.bus/32e00000.lcdif/graphics/fb0/blank
  echo 1 > /sys/devices/platform/soc@0/32c00000.bus/32e00000.lcdif/graphics/fb0/blank

All the changes intend to follow the documentation provided here:
https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

Changes for v2:
* Drop RFC
* Drop non-working Exynos cleanup patch 3/3

Frieder Schrempf (2):
  drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec
  drm/bridge: ti-sn65dsi83: Fix enable/disable flow to meet spec

 drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 19 ++++++++++++++++---
 2 files changed, 39 insertions(+), 5 deletions(-)

-- 
2.40.0

[PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

According to the documentation [1] the proper enable flow is:

1. Enable DSI link and keep data lanes in LP-11 (stop state)
2. Disable stop state to bring data lanes into HS mode

Currently we do this all at once within enable(), which doesn't
allow to meet the requirements of some downstream bridges.

To fix this we now enable the DSI in pre_enable() and force it
into stop state using the FORCE_STOP_STATE bit in the ESCMODE
register until enable() is called where we reset the bit.

We currently do this only for i.MX8M as Exynos uses a different
init flow where samsung_dsim_init() is called from
samsung_dsim_host_transfer().

[1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
Changes for v2:
* Drop RFC
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index e0a402a85787..9775779721d9 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
 	reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
 	reg &= ~DSIM_STOP_STATE_CNT_MASK;
 	reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
+
+	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
+		reg |= DSIM_FORCE_STOP_STATE;
+
 	samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
 
 	reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
@@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
 		ret = samsung_dsim_init(dsi);
 		if (ret)
 			return;
+
+		samsung_dsim_set_display_mode(dsi);
+		samsung_dsim_set_display_enable(dsi, true);
 	}
 }
 
@@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
 				       struct drm_bridge_state *old_bridge_state)
 {
 	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
+	u32 reg;
 
-	samsung_dsim_set_display_mode(dsi);
-	samsung_dsim_set_display_enable(dsi, true);
+	if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
+		samsung_dsim_set_display_mode(dsi);
+		samsung_dsim_set_display_enable(dsi, true);
+	} else {
+		reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
+		reg &= ~DSIM_FORCE_STOP_STATE;
+		samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
+	}
 
 	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
 }
@@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
 					struct drm_bridge_state *old_bridge_state)
 {
 	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
+	u32 reg;
 
 	if (!(dsi->state & DSIM_STATE_ENABLED))
 		return;
 
+	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
+		reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
+		reg |= DSIM_FORCE_STOP_STATE;
+		samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
+	}
+
 	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
 }
 
-- 
2.40.0

[PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

According to the documentation [1] the proper enable flow is:

1. Enable DSI link and keep data lanes in LP-11 (stop state)
2. Disable stop state to bring data lanes into HS mode

Currently we do this all at once within enable(), which doesn't
allow to meet the requirements of some downstream bridges.

To fix this we now enable the DSI in pre_enable() and force it
into stop state using the FORCE_STOP_STATE bit in the ESCMODE
register until enable() is called where we reset the bit.

We currently do this only for i.MX8M as Exynos uses a different
init flow where samsung_dsim_init() is called from
samsung_dsim_host_transfer().

[1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
Changes for v2:
* Drop RFC
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index e0a402a85787..9775779721d9 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
 	reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
 	reg &= ~DSIM_STOP_STATE_CNT_MASK;
 	reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
+
+	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
+		reg |= DSIM_FORCE_STOP_STATE;
+
 	samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
 
 	reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
@@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
 		ret = samsung_dsim_init(dsi);
 		if (ret)
 			return;
+
+		samsung_dsim_set_display_mode(dsi);
+		samsung_dsim_set_display_enable(dsi, true);
 	}
 }
 
@@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
 				       struct drm_bridge_state *old_bridge_state)
 {
 	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
+	u32 reg;
 
-	samsung_dsim_set_display_mode(dsi);
-	samsung_dsim_set_display_enable(dsi, true);
+	if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
+		samsung_dsim_set_display_mode(dsi);
+		samsung_dsim_set_display_enable(dsi, true);
+	} else {
+		reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
+		reg &= ~DSIM_FORCE_STOP_STATE;
+		samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
+	}
 
 	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
 }
@@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
 					struct drm_bridge_state *old_bridge_state)
 {
 	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
+	u32 reg;
 
 	if (!(dsi->state & DSIM_STATE_ENABLED))
 		return;
 
+	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
+		reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
+		reg |= DSIM_FORCE_STOP_STATE;
+		samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
+	}
+
 	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
 }
 
-- 
2.40.0

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

[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>
---
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);
-- 
2.40.0

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

[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>
---
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);
-- 
2.40.0

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Alexander Stein]

Hello Frieder,

Am Mittwoch, 3. Mai 2023, 18:33:06 CEST schrieb Frieder Schrempf:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> According to the documentation [1] the proper enable flow is:
> 
> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
> 2. Disable stop state to bring data lanes into HS mode
> 
> Currently we do this all at once within enable(), which doesn't
> allow to meet the requirements of some downstream bridges.
> 
> To fix this we now enable the DSI in pre_enable() and force it
> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
> register until enable() is called where we reset the bit.
> 
> We currently do this only for i.MX8M as Exynos uses a different
> init flow where samsung_dsim_init() is called from
> samsung_dsim_host_transfer().
> 
> [1]
> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
> Changes for v2:
> * Drop RFC
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c index e0a402a85787..9775779721d9
> 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim
> *dsi) reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>  	reg &= ~DSIM_STOP_STATE_CNT_MASK;
>  	reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> +
> +	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> +		reg |= DSIM_FORCE_STOP_STATE;
> +
>  	samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> 
>  	reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct
> drm_bridge *bridge, ret = samsung_dsim_init(dsi);
>  		if (ret)
>  			return;
> +
> +		samsung_dsim_set_display_mode(dsi);
> +		samsung_dsim_set_display_enable(dsi, true);
>  	}
>  }
> 
> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct
> drm_bridge *bridge, struct drm_bridge_state *old_bridge_state)
>  {
>  	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +	u32 reg;
> 
> -	samsung_dsim_set_display_mode(dsi);
> -	samsung_dsim_set_display_enable(dsi, true);
> +	if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> +		samsung_dsim_set_display_mode(dsi);
> +		samsung_dsim_set_display_enable(dsi, true);
> +	} else {
> +		reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> +		reg &= ~DSIM_FORCE_STOP_STATE;
> +		samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +	}
> 
>  	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>  }
> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct
> drm_bridge *bridge, struct drm_bridge_state *old_bridge_state)
>  {
>  	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +	u32 reg;
> 
>  	if (!(dsi->state & DSIM_STATE_ENABLED))
>  		return;
> 
> +	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> +		reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> +		reg |= DSIM_FORCE_STOP_STATE;
> +		samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +	}
> +
>  	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>  }

I know that this is necessary right now, but I don't like that 
'samsung_dsim_hw_is_exynos()' checks all over the place.

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

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Alexander Stein]

Hello Frieder,

Am Mittwoch, 3. Mai 2023, 18:33:06 CEST schrieb Frieder Schrempf:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> According to the documentation [1] the proper enable flow is:
> 
> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
> 2. Disable stop state to bring data lanes into HS mode
> 
> Currently we do this all at once within enable(), which doesn't
> allow to meet the requirements of some downstream bridges.
> 
> To fix this we now enable the DSI in pre_enable() and force it
> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
> register until enable() is called where we reset the bit.
> 
> We currently do this only for i.MX8M as Exynos uses a different
> init flow where samsung_dsim_init() is called from
> samsung_dsim_host_transfer().
> 
> [1]
> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
> Changes for v2:
> * Drop RFC
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c index e0a402a85787..9775779721d9
> 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim
> *dsi) reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>  	reg &= ~DSIM_STOP_STATE_CNT_MASK;
>  	reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> +
> +	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> +		reg |= DSIM_FORCE_STOP_STATE;
> +
>  	samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> 
>  	reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct
> drm_bridge *bridge, ret = samsung_dsim_init(dsi);
>  		if (ret)
>  			return;
> +
> +		samsung_dsim_set_display_mode(dsi);
> +		samsung_dsim_set_display_enable(dsi, true);
>  	}
>  }
> 
> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct
> drm_bridge *bridge, struct drm_bridge_state *old_bridge_state)
>  {
>  	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +	u32 reg;
> 
> -	samsung_dsim_set_display_mode(dsi);
> -	samsung_dsim_set_display_enable(dsi, true);
> +	if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> +		samsung_dsim_set_display_mode(dsi);
> +		samsung_dsim_set_display_enable(dsi, true);
> +	} else {
> +		reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> +		reg &= ~DSIM_FORCE_STOP_STATE;
> +		samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +	}
> 
>  	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>  }
> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct
> drm_bridge *bridge, struct drm_bridge_state *old_bridge_state)
>  {
>  	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +	u32 reg;
> 
>  	if (!(dsi->state & DSIM_STATE_ENABLED))
>  		return;
> 
> +	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> +		reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> +		reg |= DSIM_FORCE_STOP_STATE;
> +		samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +	}
> +
>  	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>  }

I know that this is necessary right now, but I don't like that 
'samsung_dsim_hw_is_exynos()' checks all over the place.

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

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

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

[Alexander Stein]

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);


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

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

[Alexander Stein]

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);


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Neil Armstrong]

On 03/05/2023 18:33, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> According to the documentation [1] the proper enable flow is:
> 
> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
> 2. Disable stop state to bring data lanes into HS mode
> 
> Currently we do this all at once within enable(), which doesn't
> allow to meet the requirements of some downstream bridges.
> 
> To fix this we now enable the DSI in pre_enable() and force it
> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
> register until enable() is called where we reset the bit.
> 
> We currently do this only for i.MX8M as Exynos uses a different
> init flow where samsung_dsim_init() is called from
> samsung_dsim_host_transfer().
> 
> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
> Changes for v2:
> * Drop RFC
> ---
>   drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
>   1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index e0a402a85787..9775779721d9 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
>   	reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>   	reg &= ~DSIM_STOP_STATE_CNT_MASK;
>   	reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> +
> +	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> +		reg |= DSIM_FORCE_STOP_STATE;
> +
>   	samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>   
>   	reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>   		ret = samsung_dsim_init(dsi);
>   		if (ret)
>   			return;
> +
> +		samsung_dsim_set_display_mode(dsi);
> +		samsung_dsim_set_display_enable(dsi, true);
>   	}
>   }
>   
> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>   				       struct drm_bridge_state *old_bridge_state)
>   {
>   	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +	u32 reg;
>   
> -	samsung_dsim_set_display_mode(dsi);
> -	samsung_dsim_set_display_enable(dsi, true);
> +	if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> +		samsung_dsim_set_display_mode(dsi);
> +		samsung_dsim_set_display_enable(dsi, true);
> +	} else {
> +		reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> +		reg &= ~DSIM_FORCE_STOP_STATE;
> +		samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +	}
>   
>   	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>   }
> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
>   					struct drm_bridge_state *old_bridge_state)
>   {
>   	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +	u32 reg;
>   
>   	if (!(dsi->state & DSIM_STATE_ENABLED))
>   		return;
>   
> +	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> +		reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> +		reg |= DSIM_FORCE_STOP_STATE;
> +		samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +	}
> +
>   	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>   }
>   

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Neil Armstrong]

On 03/05/2023 18:33, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> According to the documentation [1] the proper enable flow is:
> 
> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
> 2. Disable stop state to bring data lanes into HS mode
> 
> Currently we do this all at once within enable(), which doesn't
> allow to meet the requirements of some downstream bridges.
> 
> To fix this we now enable the DSI in pre_enable() and force it
> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
> register until enable() is called where we reset the bit.
> 
> We currently do this only for i.MX8M as Exynos uses a different
> init flow where samsung_dsim_init() is called from
> samsung_dsim_host_transfer().
> 
> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
> Changes for v2:
> * Drop RFC
> ---
>   drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
>   1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index e0a402a85787..9775779721d9 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
>   	reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>   	reg &= ~DSIM_STOP_STATE_CNT_MASK;
>   	reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> +
> +	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> +		reg |= DSIM_FORCE_STOP_STATE;
> +
>   	samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>   
>   	reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>   		ret = samsung_dsim_init(dsi);
>   		if (ret)
>   			return;
> +
> +		samsung_dsim_set_display_mode(dsi);
> +		samsung_dsim_set_display_enable(dsi, true);
>   	}
>   }
>   
> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>   				       struct drm_bridge_state *old_bridge_state)
>   {
>   	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +	u32 reg;
>   
> -	samsung_dsim_set_display_mode(dsi);
> -	samsung_dsim_set_display_enable(dsi, true);
> +	if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> +		samsung_dsim_set_display_mode(dsi);
> +		samsung_dsim_set_display_enable(dsi, true);
> +	} else {
> +		reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> +		reg &= ~DSIM_FORCE_STOP_STATE;
> +		samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +	}
>   
>   	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>   }
> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
>   					struct drm_bridge_state *old_bridge_state)
>   {
>   	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +	u32 reg;
>   
>   	if (!(dsi->state & DSIM_STATE_ENABLED))
>   		return;
>   
> +	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> +		reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> +		reg |= DSIM_FORCE_STOP_STATE;
> +		samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +	}
> +
>   	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>   }
>   

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

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

[Neil Armstrong]

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>

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

[Neil Armstrong]

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>

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

[Fabio Estevam]

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?

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

[Fabio Estevam]

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?

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

[Frieder Schrempf]

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")

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

[Frieder Schrempf]

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")

Re: [PATCH v2 0/2] Init flow fixes for Samsung DSIM and TI SN65DSI84

[Neil Armstrong]

Hi,

On Wed, 03 May 2023 18:33:05 +0200, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> This patchset contains a proposal to fix the initialization flow for
> the display pipeline used on our i.MX8MM Kontron boards:
> 
>   i.MX8MM LCDIF -> i.MX8MM DSIM -> TI SN65DSI84 -> 7" LVDS Panel
> 
> [...]

Thanks, Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next)

[1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=0c14d3130654fe459fca3067d2d4317fc607bc71
[2/2] drm/bridge: ti-sn65dsi83: Fix enable/disable flow to meet spec
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=dd9e329af7236e34c566d3705ea32a63069b9b13

-- 
Neil

Re: [PATCH v2 0/2] Init flow fixes for Samsung DSIM and TI SN65DSI84

[Neil Armstrong]

Hi,

On Wed, 03 May 2023 18:33:05 +0200, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> This patchset contains a proposal to fix the initialization flow for
> the display pipeline used on our i.MX8MM Kontron boards:
> 
>   i.MX8MM LCDIF -> i.MX8MM DSIM -> TI SN65DSI84 -> 7" LVDS Panel
> 
> [...]

Thanks, Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next)

[1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=0c14d3130654fe459fca3067d2d4317fc607bc71
[2/2] drm/bridge: ti-sn65dsi83: Fix enable/disable flow to meet spec
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=dd9e329af7236e34c566d3705ea32a63069b9b13

-- 
Neil

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Tim Harvey]

On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
>
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>
> According to the documentation [1] the proper enable flow is:
>
> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
> 2. Disable stop state to bring data lanes into HS mode
>
> Currently we do this all at once within enable(), which doesn't
> allow to meet the requirements of some downstream bridges.
>
> To fix this we now enable the DSI in pre_enable() and force it
> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
> register until enable() is called where we reset the bit.
>
> We currently do this only for i.MX8M as Exynos uses a different
> init flow where samsung_dsim_init() is called from
> samsung_dsim_host_transfer().
>
> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
> Changes for v2:
> * Drop RFC
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index e0a402a85787..9775779721d9 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
>         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>         reg &= ~DSIM_STOP_STATE_CNT_MASK;
>         reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> +
> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> +               reg |= DSIM_FORCE_STOP_STATE;
> +
>         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>
>         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>                 ret = samsung_dsim_init(dsi);
>                 if (ret)
>                         return;
> +
> +               samsung_dsim_set_display_mode(dsi);
> +               samsung_dsim_set_display_enable(dsi, true);
>         }
>  }
>
> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>                                        struct drm_bridge_state *old_bridge_state)
>  {
>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +       u32 reg;
>
> -       samsung_dsim_set_display_mode(dsi);
> -       samsung_dsim_set_display_enable(dsi, true);
> +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> +               samsung_dsim_set_display_mode(dsi);
> +               samsung_dsim_set_display_enable(dsi, true);
> +       } else {
> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> +               reg &= ~DSIM_FORCE_STOP_STATE;
> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +       }
>
>         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>  }
> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
>                                         struct drm_bridge_state *old_bridge_state)
>  {
>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +       u32 reg;
>
>         if (!(dsi->state & DSIM_STATE_ENABLED))
>                 return;
>
> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> +               reg |= DSIM_FORCE_STOP_STATE;
> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +       }
> +
>         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>  }
>
> --
> 2.40.0
>

Hi Frieder,

I found this patch to break mipi-dsi display on my board which has:
 - FocalTech FT5406 10pt touch controller (with no interrupt)
 - Powertip PH800480T013-IDF02 compatible panel
 - Toshiba TC358762 compatible DSI to DBI bridge
 - ATTINY based regulator used for backlight controller and panel enable

I enable this via a dt overlay in a pending patch
imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
6.5-rc1 which has this patch.

The issue appears as:
[    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
64 01 05 00 00 00
[    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)

Instead of
[    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
using dummy regulator
[    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
using dummy regulator
[    5.649928] samsung-dsim 32e10000.dsi:
[drm:samsung_dsim_host_attach] Attached tc358762 device

I'm curious what board/panel were you needing this for and do you have
any ideas why it broke my setup?

I'm also curious what board/panel Alexander tested this with and if
Adam or Jagan (or others) have tested this with their hardware?

best regards,

Tim
[1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20230711221124.2127186-1-tharvey@gateworks.com/

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Tim Harvey]

On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
>
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>
> According to the documentation [1] the proper enable flow is:
>
> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
> 2. Disable stop state to bring data lanes into HS mode
>
> Currently we do this all at once within enable(), which doesn't
> allow to meet the requirements of some downstream bridges.
>
> To fix this we now enable the DSI in pre_enable() and force it
> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
> register until enable() is called where we reset the bit.
>
> We currently do this only for i.MX8M as Exynos uses a different
> init flow where samsung_dsim_init() is called from
> samsung_dsim_host_transfer().
>
> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
> Changes for v2:
> * Drop RFC
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index e0a402a85787..9775779721d9 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
>         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>         reg &= ~DSIM_STOP_STATE_CNT_MASK;
>         reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> +
> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> +               reg |= DSIM_FORCE_STOP_STATE;
> +
>         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>
>         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>                 ret = samsung_dsim_init(dsi);
>                 if (ret)
>                         return;
> +
> +               samsung_dsim_set_display_mode(dsi);
> +               samsung_dsim_set_display_enable(dsi, true);
>         }
>  }
>
> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>                                        struct drm_bridge_state *old_bridge_state)
>  {
>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +       u32 reg;
>
> -       samsung_dsim_set_display_mode(dsi);
> -       samsung_dsim_set_display_enable(dsi, true);
> +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> +               samsung_dsim_set_display_mode(dsi);
> +               samsung_dsim_set_display_enable(dsi, true);
> +       } else {
> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> +               reg &= ~DSIM_FORCE_STOP_STATE;
> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +       }
>
>         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>  }
> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
>                                         struct drm_bridge_state *old_bridge_state)
>  {
>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> +       u32 reg;
>
>         if (!(dsi->state & DSIM_STATE_ENABLED))
>                 return;
>
> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> +               reg |= DSIM_FORCE_STOP_STATE;
> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +       }
> +
>         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>  }
>
> --
> 2.40.0
>

Hi Frieder,

I found this patch to break mipi-dsi display on my board which has:
 - FocalTech FT5406 10pt touch controller (with no interrupt)
 - Powertip PH800480T013-IDF02 compatible panel
 - Toshiba TC358762 compatible DSI to DBI bridge
 - ATTINY based regulator used for backlight controller and panel enable

I enable this via a dt overlay in a pending patch
imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
6.5-rc1 which has this patch.

The issue appears as:
[    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
64 01 05 00 00 00
[    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)

Instead of
[    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
using dummy regulator
[    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
using dummy regulator
[    5.649928] samsung-dsim 32e10000.dsi:
[drm:samsung_dsim_host_attach] Attached tc358762 device

I'm curious what board/panel were you needing this for and do you have
any ideas why it broke my setup?

I'm also curious what board/panel Alexander tested this with and if
Adam or Jagan (or others) have tested this with their hardware?

best regards,

Tim
[1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20230711221124.2127186-1-tharvey@gateworks.com/

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Adam Ford]

On Wed, Jul 12, 2023 at 5:34 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
> >
> > From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >
> > According to the documentation [1] the proper enable flow is:
> >
> > 1. Enable DSI link and keep data lanes in LP-11 (stop state)
> > 2. Disable stop state to bring data lanes into HS mode
> >
> > Currently we do this all at once within enable(), which doesn't
> > allow to meet the requirements of some downstream bridges.
> >
> > To fix this we now enable the DSI in pre_enable() and force it
> > into stop state using the FORCE_STOP_STATE bit in the ESCMODE
> > register until enable() is called where we reset the bit.
> >
> > We currently do this only for i.MX8M as Exynos uses a different
> > init flow where samsung_dsim_init() is called from
> > samsung_dsim_host_transfer().
> >
> > [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> >
> > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > ---
> > Changes for v2:
> > * Drop RFC
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index e0a402a85787..9775779721d9 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
> >         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> >         reg &= ~DSIM_STOP_STATE_CNT_MASK;
> >         reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> > +
> > +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> > +               reg |= DSIM_FORCE_STOP_STATE;
> > +
> >         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >
> >         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
> > @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
> >                 ret = samsung_dsim_init(dsi);
> >                 if (ret)
> >                         return;
> > +
> > +               samsung_dsim_set_display_mode(dsi);
> > +               samsung_dsim_set_display_enable(dsi, true);
> >         }
> >  }
> >
> > @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
> >                                        struct drm_bridge_state *old_bridge_state)
> >  {
> >         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> > +       u32 reg;
> >
> > -       samsung_dsim_set_display_mode(dsi);
> > -       samsung_dsim_set_display_enable(dsi, true);
> > +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> > +               samsung_dsim_set_display_mode(dsi);
> > +               samsung_dsim_set_display_enable(dsi, true);
> > +       } else {
> > +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> > +               reg &= ~DSIM_FORCE_STOP_STATE;
> > +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> > +       }
> >
> >         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
> >  }
> > @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
> >                                         struct drm_bridge_state *old_bridge_state)
> >  {
> >         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> > +       u32 reg;
> >
> >         if (!(dsi->state & DSIM_STATE_ENABLED))
> >                 return;
> >
> > +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> > +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> > +               reg |= DSIM_FORCE_STOP_STATE;
> > +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> > +       }
> > +
> >         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
> >  }
> >
> > --
> > 2.40.0
> >
>
> Hi Frieder,
>
> I found this patch to break mipi-dsi display on my board which has:
>  - FocalTech FT5406 10pt touch controller (with no interrupt)
>  - Powertip PH800480T013-IDF02 compatible panel
>  - Toshiba TC358762 compatible DSI to DBI bridge
>  - ATTINY based regulator used for backlight controller and panel enable
>
> I enable this via a dt overlay in a pending patch
> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
> 6.5-rc1 which has this patch.
>
> The issue appears as:
> [    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
> 64 01 05 00 00 00
> [    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)
>
> Instead of
> [    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
> using dummy regulator
> [    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
> using dummy regulator
> [    5.649928] samsung-dsim 32e10000.dsi:
> [drm:samsung_dsim_host_attach] Attached tc358762 device
>
> I'm curious what board/panel were you needing this for and do you have
> any ideas why it broke my setup?
>
> I'm also curious what board/panel Alexander tested this with and if
> Adam or Jagan (or others) have tested this with their hardware?

I have used the imx8mm and imx8mn with both an Analog Devices ADV7535
HDMI bridge, and a ti,sn65dsi83, and the imx8mp with the just the
adv7535.  I haven't seen any issues with this patch, but I wonder if
the downstream part for Tim needs to do something with this patch
installed, or whether or not we should add some sort of dsim flag that
either does this or skips it.  I would be curious to know if the NXP
downstream code works with either Frieder's or Tim's.

adam
>
> best regards,
>
> Tim
> [1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20230711221124.2127186-1-tharvey@gateworks.com/

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Adam Ford]

On Wed, Jul 12, 2023 at 5:34 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
> >
> > From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >
> > According to the documentation [1] the proper enable flow is:
> >
> > 1. Enable DSI link and keep data lanes in LP-11 (stop state)
> > 2. Disable stop state to bring data lanes into HS mode
> >
> > Currently we do this all at once within enable(), which doesn't
> > allow to meet the requirements of some downstream bridges.
> >
> > To fix this we now enable the DSI in pre_enable() and force it
> > into stop state using the FORCE_STOP_STATE bit in the ESCMODE
> > register until enable() is called where we reset the bit.
> >
> > We currently do this only for i.MX8M as Exynos uses a different
> > init flow where samsung_dsim_init() is called from
> > samsung_dsim_host_transfer().
> >
> > [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> >
> > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > ---
> > Changes for v2:
> > * Drop RFC
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index e0a402a85787..9775779721d9 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
> >         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> >         reg &= ~DSIM_STOP_STATE_CNT_MASK;
> >         reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> > +
> > +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> > +               reg |= DSIM_FORCE_STOP_STATE;
> > +
> >         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >
> >         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
> > @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
> >                 ret = samsung_dsim_init(dsi);
> >                 if (ret)
> >                         return;
> > +
> > +               samsung_dsim_set_display_mode(dsi);
> > +               samsung_dsim_set_display_enable(dsi, true);
> >         }
> >  }
> >
> > @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
> >                                        struct drm_bridge_state *old_bridge_state)
> >  {
> >         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> > +       u32 reg;
> >
> > -       samsung_dsim_set_display_mode(dsi);
> > -       samsung_dsim_set_display_enable(dsi, true);
> > +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> > +               samsung_dsim_set_display_mode(dsi);
> > +               samsung_dsim_set_display_enable(dsi, true);
> > +       } else {
> > +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> > +               reg &= ~DSIM_FORCE_STOP_STATE;
> > +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> > +       }
> >
> >         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
> >  }
> > @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
> >                                         struct drm_bridge_state *old_bridge_state)
> >  {
> >         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> > +       u32 reg;
> >
> >         if (!(dsi->state & DSIM_STATE_ENABLED))
> >                 return;
> >
> > +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> > +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> > +               reg |= DSIM_FORCE_STOP_STATE;
> > +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> > +       }
> > +
> >         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
> >  }
> >
> > --
> > 2.40.0
> >
>
> Hi Frieder,
>
> I found this patch to break mipi-dsi display on my board which has:
>  - FocalTech FT5406 10pt touch controller (with no interrupt)
>  - Powertip PH800480T013-IDF02 compatible panel
>  - Toshiba TC358762 compatible DSI to DBI bridge
>  - ATTINY based regulator used for backlight controller and panel enable
>
> I enable this via a dt overlay in a pending patch
> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
> 6.5-rc1 which has this patch.
>
> The issue appears as:
> [    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
> 64 01 05 00 00 00
> [    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)
>
> Instead of
> [    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
> using dummy regulator
> [    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
> using dummy regulator
> [    5.649928] samsung-dsim 32e10000.dsi:
> [drm:samsung_dsim_host_attach] Attached tc358762 device
>
> I'm curious what board/panel were you needing this for and do you have
> any ideas why it broke my setup?
>
> I'm also curious what board/panel Alexander tested this with and if
> Adam or Jagan (or others) have tested this with their hardware?

I have used the imx8mm and imx8mn with both an Analog Devices ADV7535
HDMI bridge, and a ti,sn65dsi83, and the imx8mp with the just the
adv7535.  I haven't seen any issues with this patch, but I wonder if
the downstream part for Tim needs to do something with this patch
installed, or whether or not we should add some sort of dsim flag that
either does this or skips it.  I would be curious to know if the NXP
downstream code works with either Frieder's or Tim's.

adam
>
> best regards,
>
> Tim
> [1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20230711221124.2127186-1-tharvey@gateworks.com/

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Alexander Stein]

Hi,

Am Donnerstag, 13. Juli 2023, 00:34:47 CEST schrieb Tim Harvey:
> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
> > From: Frieder Schrempf <frieder.schrempf@kontron.de>
> > 
> > According to the documentation [1] the proper enable flow is:
> > 
> > 1. Enable DSI link and keep data lanes in LP-11 (stop state)
> > 2. Disable stop state to bring data lanes into HS mode
> > 
> > Currently we do this all at once within enable(), which doesn't
> > allow to meet the requirements of some downstream bridges.
> > 
> > To fix this we now enable the DSI in pre_enable() and force it
> > into stop state using the FORCE_STOP_STATE bit in the ESCMODE
> > register until enable() is called where we reset the bit.
> > 
> > We currently do this only for i.MX8M as Exynos uses a different
> > init flow where samsung_dsim_init() is called from
> > samsung_dsim_host_transfer().
> > 
> > [1]
> > https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operatio
> > n
> > 
> > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > ---
> > Changes for v2:
> > * Drop RFC
> > ---
> > 
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> > b/drivers/gpu/drm/bridge/samsung-dsim.c index e0a402a85787..9775779721d9
> > 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim
> > *dsi)> 
> >         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> >         reg &= ~DSIM_STOP_STATE_CNT_MASK;
> >         reg |=
> >         DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> > 
> > +
> > +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> > +               reg |= DSIM_FORCE_STOP_STATE;
> > +
> > 
> >         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >         
> >         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
> > 
> > @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct
> > drm_bridge *bridge,> 
> >                 ret = samsung_dsim_init(dsi);
> >                 if (ret)
> >                 
> >                         return;
> > 
> > +
> > +               samsung_dsim_set_display_mode(dsi);
> > +               samsung_dsim_set_display_enable(dsi, true);
> > 
> >         }
> >  
> >  }
> > 
> > @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct
> > drm_bridge *bridge,> 
> >                                        struct drm_bridge_state
> >                                        *old_bridge_state)
> >  
> >  {
> >  
> >         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> > 
> > +       u32 reg;
> > 
> > -       samsung_dsim_set_display_mode(dsi);
> > -       samsung_dsim_set_display_enable(dsi, true);
> > +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> > +               samsung_dsim_set_display_mode(dsi);
> > +               samsung_dsim_set_display_enable(dsi, true);
> > +       } else {
> > +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> > +               reg &= ~DSIM_FORCE_STOP_STATE;
> > +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> > +       }
> > 
> >         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
> >  
> >  }
> > 
> > @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct
> > drm_bridge *bridge,> 
> >                                         struct drm_bridge_state
> >                                         *old_bridge_state)
> >  
> >  {
> >  
> >         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> > 
> > +       u32 reg;
> > 
> >         if (!(dsi->state & DSIM_STATE_ENABLED))
> >         
> >                 return;
> > 
> > +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> > +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> > +               reg |= DSIM_FORCE_STOP_STATE;
> > +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> > +       }
> > +
> > 
> >         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
> >  
> >  }
> > 
> > --
> > 2.40.0
> 
> Hi Frieder,
> 
> I found this patch to break mipi-dsi display on my board which has:
>  - FocalTech FT5406 10pt touch controller (with no interrupt)
>  - Powertip PH800480T013-IDF02 compatible panel
>  - Toshiba TC358762 compatible DSI to DBI bridge
>  - ATTINY based regulator used for backlight controller and panel enable
> 
> I enable this via a dt overlay in a pending patch
> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
> 6.5-rc1 which has this patch.
> 
> The issue appears as:
> [    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
> 64 01 05 00 00 00
> [    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)
> 
> Instead of
> [    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
> using dummy regulator
> [    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
> using dummy regulator
> [    5.649928] samsung-dsim 32e10000.dsi:
> [drm:samsung_dsim_host_attach] Attached tc358762 device
> 
> I'm curious what board/panel were you needing this for and do you have
> any ideas why it broke my setup?
> 
> I'm also curious what board/panel Alexander tested this with and if
> Adam or Jagan (or others) have tested this with their hardware?

I tested this with imx8mm and ti,sn65dsi83 DSI-LVDS bridge [1]. I don't know 
anything about tc358762, but I was trying to get tc9595 (DSI-DP bridge) 
running on a imx8mp based board. One of my problems was that the bridge 
requires LP-11 before reset release, which is currently not given using 
samsung-dsim and tc358767 driver. Maybe this is the case for you as well.
AFAICS there is a lot more to do to get this running properly.

Best regards,
Alexander

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/
arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql-mba8mpxl-lvds.dtso

> best regards,
> 
> Tim
> [1]
> https://patchwork.kernel.org/project/linux-arm-kernel/patch/20230711221124.
> 2127186-1-tharvey@gateworks.com/


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Alexander Stein]

Hi,

Am Donnerstag, 13. Juli 2023, 00:34:47 CEST schrieb Tim Harvey:
> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
> > From: Frieder Schrempf <frieder.schrempf@kontron.de>
> > 
> > According to the documentation [1] the proper enable flow is:
> > 
> > 1. Enable DSI link and keep data lanes in LP-11 (stop state)
> > 2. Disable stop state to bring data lanes into HS mode
> > 
> > Currently we do this all at once within enable(), which doesn't
> > allow to meet the requirements of some downstream bridges.
> > 
> > To fix this we now enable the DSI in pre_enable() and force it
> > into stop state using the FORCE_STOP_STATE bit in the ESCMODE
> > register until enable() is called where we reset the bit.
> > 
> > We currently do this only for i.MX8M as Exynos uses a different
> > init flow where samsung_dsim_init() is called from
> > samsung_dsim_host_transfer().
> > 
> > [1]
> > https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operatio
> > n
> > 
> > Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> > ---
> > Changes for v2:
> > * Drop RFC
> > ---
> > 
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> > b/drivers/gpu/drm/bridge/samsung-dsim.c index e0a402a85787..9775779721d9
> > 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim
> > *dsi)> 
> >         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> >         reg &= ~DSIM_STOP_STATE_CNT_MASK;
> >         reg |=
> >         DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> > 
> > +
> > +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> > +               reg |= DSIM_FORCE_STOP_STATE;
> > +
> > 
> >         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >         
> >         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
> > 
> > @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct
> > drm_bridge *bridge,> 
> >                 ret = samsung_dsim_init(dsi);
> >                 if (ret)
> >                 
> >                         return;
> > 
> > +
> > +               samsung_dsim_set_display_mode(dsi);
> > +               samsung_dsim_set_display_enable(dsi, true);
> > 
> >         }
> >  
> >  }
> > 
> > @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct
> > drm_bridge *bridge,> 
> >                                        struct drm_bridge_state
> >                                        *old_bridge_state)
> >  
> >  {
> >  
> >         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> > 
> > +       u32 reg;
> > 
> > -       samsung_dsim_set_display_mode(dsi);
> > -       samsung_dsim_set_display_enable(dsi, true);
> > +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> > +               samsung_dsim_set_display_mode(dsi);
> > +               samsung_dsim_set_display_enable(dsi, true);
> > +       } else {
> > +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> > +               reg &= ~DSIM_FORCE_STOP_STATE;
> > +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> > +       }
> > 
> >         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
> >  
> >  }
> > 
> > @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct
> > drm_bridge *bridge,> 
> >                                         struct drm_bridge_state
> >                                         *old_bridge_state)
> >  
> >  {
> >  
> >         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> > 
> > +       u32 reg;
> > 
> >         if (!(dsi->state & DSIM_STATE_ENABLED))
> >         
> >                 return;
> > 
> > +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> > +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> > +               reg |= DSIM_FORCE_STOP_STATE;
> > +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> > +       }
> > +
> > 
> >         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
> >  
> >  }
> > 
> > --
> > 2.40.0
> 
> Hi Frieder,
> 
> I found this patch to break mipi-dsi display on my board which has:
>  - FocalTech FT5406 10pt touch controller (with no interrupt)
>  - Powertip PH800480T013-IDF02 compatible panel
>  - Toshiba TC358762 compatible DSI to DBI bridge
>  - ATTINY based regulator used for backlight controller and panel enable
> 
> I enable this via a dt overlay in a pending patch
> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
> 6.5-rc1 which has this patch.
> 
> The issue appears as:
> [    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
> 64 01 05 00 00 00
> [    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)
> 
> Instead of
> [    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
> using dummy regulator
> [    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
> using dummy regulator
> [    5.649928] samsung-dsim 32e10000.dsi:
> [drm:samsung_dsim_host_attach] Attached tc358762 device
> 
> I'm curious what board/panel were you needing this for and do you have
> any ideas why it broke my setup?
> 
> I'm also curious what board/panel Alexander tested this with and if
> Adam or Jagan (or others) have tested this with their hardware?

I tested this with imx8mm and ti,sn65dsi83 DSI-LVDS bridge [1]. I don't know 
anything about tc358762, but I was trying to get tc9595 (DSI-DP bridge) 
running on a imx8mp based board. One of my problems was that the bridge 
requires LP-11 before reset release, which is currently not given using 
samsung-dsim and tc358767 driver. Maybe this is the case for you as well.
AFAICS there is a lot more to do to get this running properly.

Best regards,
Alexander

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/
arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql-mba8mpxl-lvds.dtso

> best regards,
> 
> Tim
> [1]
> https://patchwork.kernel.org/project/linux-arm-kernel/patch/20230711221124.
> 2127186-1-tharvey@gateworks.com/


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Frieder Schrempf]

Hi Tim,

On 13.07.23 00:34, Tim Harvey wrote:
> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
>>
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> According to the documentation [1] the proper enable flow is:
>>
>> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
>> 2. Disable stop state to bring data lanes into HS mode
>>
>> Currently we do this all at once within enable(), which doesn't
>> allow to meet the requirements of some downstream bridges.
>>
>> To fix this we now enable the DSI in pre_enable() and force it
>> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
>> register until enable() is called where we reset the bit.
>>
>> We currently do this only for i.MX8M as Exynos uses a different
>> init flow where samsung_dsim_init() is called from
>> samsung_dsim_host_transfer().
>>
>> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> ---
>> Changes for v2:
>> * Drop RFC
>> ---
>>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
>>  1 file changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>> index e0a402a85787..9775779721d9 100644
>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
>>         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>         reg &= ~DSIM_STOP_STATE_CNT_MASK;
>>         reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
>> +
>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
>> +               reg |= DSIM_FORCE_STOP_STATE;
>> +
>>         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>
>>         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
>> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>>                 ret = samsung_dsim_init(dsi);
>>                 if (ret)
>>                         return;
>> +
>> +               samsung_dsim_set_display_mode(dsi);
>> +               samsung_dsim_set_display_enable(dsi, true);
>>         }
>>  }
>>
>> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>>                                        struct drm_bridge_state *old_bridge_state)
>>  {
>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>> +       u32 reg;
>>
>> -       samsung_dsim_set_display_mode(dsi);
>> -       samsung_dsim_set_display_enable(dsi, true);
>> +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>> +               samsung_dsim_set_display_mode(dsi);
>> +               samsung_dsim_set_display_enable(dsi, true);
>> +       } else {
>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>> +               reg &= ~DSIM_FORCE_STOP_STATE;
>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>> +       }
>>
>>         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>>  }
>> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
>>                                         struct drm_bridge_state *old_bridge_state)
>>  {
>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>> +       u32 reg;
>>
>>         if (!(dsi->state & DSIM_STATE_ENABLED))
>>                 return;
>>
>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>> +               reg |= DSIM_FORCE_STOP_STATE;
>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>> +       }
>> +
>>         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>>  }
>>
>> --
>> 2.40.0
>>
> 
> Hi Frieder,
> 
> I found this patch to break mipi-dsi display on my board which has:
>  - FocalTech FT5406 10pt touch controller (with no interrupt)
>  - Powertip PH800480T013-IDF02 compatible panel
>  - Toshiba TC358762 compatible DSI to DBI bridge
>  - ATTINY based regulator used for backlight controller and panel enable
> 
> I enable this via a dt overlay in a pending patch
> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
> 6.5-rc1 which has this patch.
> 
> The issue appears as:
> [    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
> 64 01 05 00 00 00
> [    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)
> 
> Instead of
> [    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
> using dummy regulator
> [    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
> using dummy regulator
> [    5.649928] samsung-dsim 32e10000.dsi:
> [drm:samsung_dsim_host_attach] Attached tc358762 device
> 
> I'm curious what board/panel were you needing this for and do you have
> any ideas why it broke my setup?
> 
> I'm also curious what board/panel Alexander tested this with and if
> Adam or Jagan (or others) have tested this with their hardware?

Sorry for breaking your setup. My test- and use-case is the same as
Alexander's. I have the SN65DSI84 downstream bridge and without this
patch it fails to come up in some cases.

The failure is probably related to your downstream bridge being
controlled by the DSI itself using command mode. The SN65DSI84 is
instead controlled via I2C.

Actually we should have tested this with a bridge that uses command mode
before merging, now that I think of it. But I wasn't really aware of
this until now.

I'll have a closer look at the issue and then will get back to you. In
the meantime if anyone can help analyze the problem or has proposals how
to fix it, please let us know.

Thanks!
Frieder

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Frieder Schrempf]

Hi Tim,

On 13.07.23 00:34, Tim Harvey wrote:
> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
>>
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> According to the documentation [1] the proper enable flow is:
>>
>> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
>> 2. Disable stop state to bring data lanes into HS mode
>>
>> Currently we do this all at once within enable(), which doesn't
>> allow to meet the requirements of some downstream bridges.
>>
>> To fix this we now enable the DSI in pre_enable() and force it
>> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
>> register until enable() is called where we reset the bit.
>>
>> We currently do this only for i.MX8M as Exynos uses a different
>> init flow where samsung_dsim_init() is called from
>> samsung_dsim_host_transfer().
>>
>> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> ---
>> Changes for v2:
>> * Drop RFC
>> ---
>>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
>>  1 file changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>> index e0a402a85787..9775779721d9 100644
>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
>>         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>         reg &= ~DSIM_STOP_STATE_CNT_MASK;
>>         reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
>> +
>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
>> +               reg |= DSIM_FORCE_STOP_STATE;
>> +
>>         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>
>>         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
>> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>>                 ret = samsung_dsim_init(dsi);
>>                 if (ret)
>>                         return;
>> +
>> +               samsung_dsim_set_display_mode(dsi);
>> +               samsung_dsim_set_display_enable(dsi, true);
>>         }
>>  }
>>
>> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>>                                        struct drm_bridge_state *old_bridge_state)
>>  {
>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>> +       u32 reg;
>>
>> -       samsung_dsim_set_display_mode(dsi);
>> -       samsung_dsim_set_display_enable(dsi, true);
>> +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>> +               samsung_dsim_set_display_mode(dsi);
>> +               samsung_dsim_set_display_enable(dsi, true);
>> +       } else {
>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>> +               reg &= ~DSIM_FORCE_STOP_STATE;
>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>> +       }
>>
>>         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>>  }
>> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
>>                                         struct drm_bridge_state *old_bridge_state)
>>  {
>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>> +       u32 reg;
>>
>>         if (!(dsi->state & DSIM_STATE_ENABLED))
>>                 return;
>>
>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>> +               reg |= DSIM_FORCE_STOP_STATE;
>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>> +       }
>> +
>>         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>>  }
>>
>> --
>> 2.40.0
>>
> 
> Hi Frieder,
> 
> I found this patch to break mipi-dsi display on my board which has:
>  - FocalTech FT5406 10pt touch controller (with no interrupt)
>  - Powertip PH800480T013-IDF02 compatible panel
>  - Toshiba TC358762 compatible DSI to DBI bridge
>  - ATTINY based regulator used for backlight controller and panel enable
> 
> I enable this via a dt overlay in a pending patch
> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
> 6.5-rc1 which has this patch.
> 
> The issue appears as:
> [    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
> 64 01 05 00 00 00
> [    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)
> 
> Instead of
> [    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
> using dummy regulator
> [    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
> using dummy regulator
> [    5.649928] samsung-dsim 32e10000.dsi:
> [drm:samsung_dsim_host_attach] Attached tc358762 device
> 
> I'm curious what board/panel were you needing this for and do you have
> any ideas why it broke my setup?
> 
> I'm also curious what board/panel Alexander tested this with and if
> Adam or Jagan (or others) have tested this with their hardware?

Sorry for breaking your setup. My test- and use-case is the same as
Alexander's. I have the SN65DSI84 downstream bridge and without this
patch it fails to come up in some cases.

The failure is probably related to your downstream bridge being
controlled by the DSI itself using command mode. The SN65DSI84 is
instead controlled via I2C.

Actually we should have tested this with a bridge that uses command mode
before merging, now that I think of it. But I wasn't really aware of
this until now.

I'll have a closer look at the issue and then will get back to you. In
the meantime if anyone can help analyze the problem or has proposals how
to fix it, please let us know.

Thanks!
Frieder

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Frieder Schrempf]

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

Hi Tim,

On 13.07.23 09:18, Frieder Schrempf wrote:
> Hi Tim,
> 
> On 13.07.23 00:34, Tim Harvey wrote:
>> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
>>>
>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>
>>> According to the documentation [1] the proper enable flow is:
>>>
>>> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
>>> 2. Disable stop state to bring data lanes into HS mode
>>>
>>> Currently we do this all at once within enable(), which doesn't
>>> allow to meet the requirements of some downstream bridges.
>>>
>>> To fix this we now enable the DSI in pre_enable() and force it
>>> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
>>> register until enable() is called where we reset the bit.
>>>
>>> We currently do this only for i.MX8M as Exynos uses a different
>>> init flow where samsung_dsim_init() is called from
>>> samsung_dsim_host_transfer().
>>>
>>> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>>>
>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>>> ---
>>> Changes for v2:
>>> * Drop RFC
>>> ---
>>>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
>>>  1 file changed, 23 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> index e0a402a85787..9775779721d9 100644
>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
>>>         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>         reg &= ~DSIM_STOP_STATE_CNT_MASK;
>>>         reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
>>> +
>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
>>> +               reg |= DSIM_FORCE_STOP_STATE;
>>> +
>>>         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>
>>>         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
>>> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>>>                 ret = samsung_dsim_init(dsi);
>>>                 if (ret)
>>>                         return;
>>> +
>>> +               samsung_dsim_set_display_mode(dsi);
>>> +               samsung_dsim_set_display_enable(dsi, true);
>>>         }
>>>  }
>>>
>>> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>>>                                        struct drm_bridge_state *old_bridge_state)
>>>  {
>>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>>> +       u32 reg;
>>>
>>> -       samsung_dsim_set_display_mode(dsi);
>>> -       samsung_dsim_set_display_enable(dsi, true);
>>> +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>>> +               samsung_dsim_set_display_mode(dsi);
>>> +               samsung_dsim_set_display_enable(dsi, true);
>>> +       } else {
>>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>> +               reg &= ~DSIM_FORCE_STOP_STATE;
>>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>> +       }
>>>
>>>         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>>>  }
>>> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
>>>                                         struct drm_bridge_state *old_bridge_state)
>>>  {
>>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>>> +       u32 reg;
>>>
>>>         if (!(dsi->state & DSIM_STATE_ENABLED))
>>>                 return;
>>>
>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>> +               reg |= DSIM_FORCE_STOP_STATE;
>>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>> +       }
>>> +
>>>         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>>>  }
>>>
>>> --
>>> 2.40.0
>>>
>>
>> Hi Frieder,
>>
>> I found this patch to break mipi-dsi display on my board which has:
>>  - FocalTech FT5406 10pt touch controller (with no interrupt)
>>  - Powertip PH800480T013-IDF02 compatible panel
>>  - Toshiba TC358762 compatible DSI to DBI bridge
>>  - ATTINY based regulator used for backlight controller and panel enable
>>
>> I enable this via a dt overlay in a pending patch
>> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
>> 6.5-rc1 which has this patch.
>>
>> The issue appears as:
>> [    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
>> 64 01 05 00 00 00
>> [    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)
>>
>> Instead of
>> [    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
>> using dummy regulator
>> [    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
>> using dummy regulator
>> [    5.649928] samsung-dsim 32e10000.dsi:
>> [drm:samsung_dsim_host_attach] Attached tc358762 device
>>
>> I'm curious what board/panel were you needing this for and do you have
>> any ideas why it broke my setup?
>>
>> I'm also curious what board/panel Alexander tested this with and if
>> Adam or Jagan (or others) have tested this with their hardware?
> 
> Sorry for breaking your setup. My test- and use-case is the same as
> Alexander's. I have the SN65DSI84 downstream bridge and without this
> patch it fails to come up in some cases.
> 
> The failure is probably related to your downstream bridge being
> controlled by the DSI itself using command mode. The SN65DSI84 is
> instead controlled via I2C.
> 
> Actually we should have tested this with a bridge that uses command mode
> before merging, now that I think of it. But I wasn't really aware of
> this until now.
> 
> I'll have a closer look at the issue and then will get back to you. In
> the meantime if anyone can help analyze the problem or has proposals how
> to fix it, please let us know.

With my patch samsung_dsim_init() now initializes the DSIM to stop
state. When being called from samsung_dsim_atomic_pre_enable() this
works as the stop state is cleared later in samsung_dsim_atomic_enable().

When being called from samsung_dsim_host_transfer() to handle transfers
before samsung_dsim_atomic_pre_enable() was called, the stop state is
never cleared and transfers will fail.

This is the case in your setup as tc358762_init() is called in
tc358762_pre_enable().

I think that requiring to initialize the DSI host during transfer could
be avoided in this case by moving tc358762_init() from
tc358762_pre_enable() to tc358762_enable().

But at the same time according to the docs at [1] this seems to be a
valid case that we need to support in the DSIM driver:

  Whilst it is valid to call host_transfer prior to pre_enable or
  after post_disable, the exact state of the lanes is undefined at
  this point. The DSI host should initialise the interface, transmit
  the data, and then disable the interface again.

Therefore I would propose to try a fix like in the attachement. If you
could test this, that would be great.

Thanks
Frieder

[1]
https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

[-- Attachment #2: 0001-drm-bridge-samsung-dsim-Fix-init-during-host-transfe.patch --]
[-- Type: text/x-patch, Size: 3217 bytes --]

From 770e4763255ba5bf086ae7b014330651e007bcb7 Mon Sep 17 00:00:00 2001
From: Frieder Schrempf <frieder.schrempf@kontron.de>
Date: Thu, 13 Jul 2023 11:47:47 +0200
Subject: [PATCH] drm: bridge: samsung-dsim: Fix init during host transfer

In case the downstream bridge or panel uses DSI transfers before the
DSI host was actually initialized through samsung_dsim_atomic_enable()
which clears the stop state (LP11) mode, all transfers will fail.

This happens with downstream bridges that are controlled by DSI
commands such as the tc358762.

To fix this do not enable stop state when the DSI host was initialized
through samsung_dsim_host_transfer() which restores the previous
behavior.

Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec")
Reported-by: Tim Harvey <tharvey@gateworks.com>
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 043b8109e64a..4d371eaa4fa2 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -833,7 +833,7 @@ static void samsung_dsim_enable_lane(struct samsung_dsim *dsi, u32 lane)
 	samsung_dsim_write(dsi, DSIM_CONFIG_REG, reg);
 }
 
-static int samsung_dsim_init_link(struct samsung_dsim *dsi)
+static int samsung_dsim_init_link(struct samsung_dsim *dsi, bool force_stop_state)
 {
 	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
 	int timeout;
@@ -939,7 +939,7 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
 	reg &= ~DSIM_STOP_STATE_CNT_MASK;
 	reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
 
-	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
+	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type) && force_stop_state)
 		reg |= DSIM_FORCE_STOP_STATE;
 
 	samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
@@ -1386,7 +1386,7 @@ static void samsung_dsim_disable_irq(struct samsung_dsim *dsi)
 	disable_irq(dsi->irq);
 }
 
-static int samsung_dsim_init(struct samsung_dsim *dsi)
+static int samsung_dsim_init(struct samsung_dsim *dsi, bool force_stop_state)
 {
 	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
 
@@ -1403,7 +1403,7 @@ static int samsung_dsim_init(struct samsung_dsim *dsi)
 	if (driver_data->wait_for_reset)
 		samsung_dsim_wait_for_reset(dsi);
 	samsung_dsim_set_phy_ctrl(dsi);
-	samsung_dsim_init_link(dsi);
+	samsung_dsim_init_link(dsi, force_stop_state);
 
 	dsi->state |= DSIM_STATE_INITIALIZED;
 
@@ -1432,7 +1432,7 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
 	 * the host initialization during DSI transfer.
 	 */
 	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
-		ret = samsung_dsim_init(dsi);
+		ret = samsung_dsim_init(dsi, true);
 		if (ret)
 			return;
 
@@ -1771,7 +1771,7 @@ static ssize_t samsung_dsim_host_transfer(struct mipi_dsi_host *host,
 	if (!(dsi->state & DSIM_STATE_ENABLED))
 		return -EINVAL;
 
-	ret = samsung_dsim_init(dsi);
+	ret = samsung_dsim_init(dsi, false);
 	if (ret)
 		return ret;
 
-- 
2.41.0

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Frieder Schrempf]

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

Hi Tim,

On 13.07.23 09:18, Frieder Schrempf wrote:
> Hi Tim,
> 
> On 13.07.23 00:34, Tim Harvey wrote:
>> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
>>>
>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>
>>> According to the documentation [1] the proper enable flow is:
>>>
>>> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
>>> 2. Disable stop state to bring data lanes into HS mode
>>>
>>> Currently we do this all at once within enable(), which doesn't
>>> allow to meet the requirements of some downstream bridges.
>>>
>>> To fix this we now enable the DSI in pre_enable() and force it
>>> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
>>> register until enable() is called where we reset the bit.
>>>
>>> We currently do this only for i.MX8M as Exynos uses a different
>>> init flow where samsung_dsim_init() is called from
>>> samsung_dsim_host_transfer().
>>>
>>> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>>>
>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>>> ---
>>> Changes for v2:
>>> * Drop RFC
>>> ---
>>>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
>>>  1 file changed, 23 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> index e0a402a85787..9775779721d9 100644
>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
>>>         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>         reg &= ~DSIM_STOP_STATE_CNT_MASK;
>>>         reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
>>> +
>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
>>> +               reg |= DSIM_FORCE_STOP_STATE;
>>> +
>>>         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>
>>>         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
>>> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>>>                 ret = samsung_dsim_init(dsi);
>>>                 if (ret)
>>>                         return;
>>> +
>>> +               samsung_dsim_set_display_mode(dsi);
>>> +               samsung_dsim_set_display_enable(dsi, true);
>>>         }
>>>  }
>>>
>>> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>>>                                        struct drm_bridge_state *old_bridge_state)
>>>  {
>>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>>> +       u32 reg;
>>>
>>> -       samsung_dsim_set_display_mode(dsi);
>>> -       samsung_dsim_set_display_enable(dsi, true);
>>> +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>>> +               samsung_dsim_set_display_mode(dsi);
>>> +               samsung_dsim_set_display_enable(dsi, true);
>>> +       } else {
>>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>> +               reg &= ~DSIM_FORCE_STOP_STATE;
>>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>> +       }
>>>
>>>         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>>>  }
>>> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
>>>                                         struct drm_bridge_state *old_bridge_state)
>>>  {
>>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>>> +       u32 reg;
>>>
>>>         if (!(dsi->state & DSIM_STATE_ENABLED))
>>>                 return;
>>>
>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>> +               reg |= DSIM_FORCE_STOP_STATE;
>>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>> +       }
>>> +
>>>         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>>>  }
>>>
>>> --
>>> 2.40.0
>>>
>>
>> Hi Frieder,
>>
>> I found this patch to break mipi-dsi display on my board which has:
>>  - FocalTech FT5406 10pt touch controller (with no interrupt)
>>  - Powertip PH800480T013-IDF02 compatible panel
>>  - Toshiba TC358762 compatible DSI to DBI bridge
>>  - ATTINY based regulator used for backlight controller and panel enable
>>
>> I enable this via a dt overlay in a pending patch
>> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
>> 6.5-rc1 which has this patch.
>>
>> The issue appears as:
>> [    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
>> 64 01 05 00 00 00
>> [    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)
>>
>> Instead of
>> [    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
>> using dummy regulator
>> [    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
>> using dummy regulator
>> [    5.649928] samsung-dsim 32e10000.dsi:
>> [drm:samsung_dsim_host_attach] Attached tc358762 device
>>
>> I'm curious what board/panel were you needing this for and do you have
>> any ideas why it broke my setup?
>>
>> I'm also curious what board/panel Alexander tested this with and if
>> Adam or Jagan (or others) have tested this with their hardware?
> 
> Sorry for breaking your setup. My test- and use-case is the same as
> Alexander's. I have the SN65DSI84 downstream bridge and without this
> patch it fails to come up in some cases.
> 
> The failure is probably related to your downstream bridge being
> controlled by the DSI itself using command mode. The SN65DSI84 is
> instead controlled via I2C.
> 
> Actually we should have tested this with a bridge that uses command mode
> before merging, now that I think of it. But I wasn't really aware of
> this until now.
> 
> I'll have a closer look at the issue and then will get back to you. In
> the meantime if anyone can help analyze the problem or has proposals how
> to fix it, please let us know.

With my patch samsung_dsim_init() now initializes the DSIM to stop
state. When being called from samsung_dsim_atomic_pre_enable() this
works as the stop state is cleared later in samsung_dsim_atomic_enable().

When being called from samsung_dsim_host_transfer() to handle transfers
before samsung_dsim_atomic_pre_enable() was called, the stop state is
never cleared and transfers will fail.

This is the case in your setup as tc358762_init() is called in
tc358762_pre_enable().

I think that requiring to initialize the DSI host during transfer could
be avoided in this case by moving tc358762_init() from
tc358762_pre_enable() to tc358762_enable().

But at the same time according to the docs at [1] this seems to be a
valid case that we need to support in the DSIM driver:

  Whilst it is valid to call host_transfer prior to pre_enable or
  after post_disable, the exact state of the lanes is undefined at
  this point. The DSI host should initialise the interface, transmit
  the data, and then disable the interface again.

Therefore I would propose to try a fix like in the attachement. If you
could test this, that would be great.

Thanks
Frieder

[1]
https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

[-- Attachment #2: 0001-drm-bridge-samsung-dsim-Fix-init-during-host-transfe.patch --]
[-- Type: text/x-patch, Size: 3217 bytes --]

From 770e4763255ba5bf086ae7b014330651e007bcb7 Mon Sep 17 00:00:00 2001
From: Frieder Schrempf <frieder.schrempf@kontron.de>
Date: Thu, 13 Jul 2023 11:47:47 +0200
Subject: [PATCH] drm: bridge: samsung-dsim: Fix init during host transfer

In case the downstream bridge or panel uses DSI transfers before the
DSI host was actually initialized through samsung_dsim_atomic_enable()
which clears the stop state (LP11) mode, all transfers will fail.

This happens with downstream bridges that are controlled by DSI
commands such as the tc358762.

To fix this do not enable stop state when the DSI host was initialized
through samsung_dsim_host_transfer() which restores the previous
behavior.

Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec")
Reported-by: Tim Harvey <tharvey@gateworks.com>
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 043b8109e64a..4d371eaa4fa2 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -833,7 +833,7 @@ static void samsung_dsim_enable_lane(struct samsung_dsim *dsi, u32 lane)
 	samsung_dsim_write(dsi, DSIM_CONFIG_REG, reg);
 }
 
-static int samsung_dsim_init_link(struct samsung_dsim *dsi)
+static int samsung_dsim_init_link(struct samsung_dsim *dsi, bool force_stop_state)
 {
 	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
 	int timeout;
@@ -939,7 +939,7 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
 	reg &= ~DSIM_STOP_STATE_CNT_MASK;
 	reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
 
-	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
+	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type) && force_stop_state)
 		reg |= DSIM_FORCE_STOP_STATE;
 
 	samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
@@ -1386,7 +1386,7 @@ static void samsung_dsim_disable_irq(struct samsung_dsim *dsi)
 	disable_irq(dsi->irq);
 }
 
-static int samsung_dsim_init(struct samsung_dsim *dsi)
+static int samsung_dsim_init(struct samsung_dsim *dsi, bool force_stop_state)
 {
 	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
 
@@ -1403,7 +1403,7 @@ static int samsung_dsim_init(struct samsung_dsim *dsi)
 	if (driver_data->wait_for_reset)
 		samsung_dsim_wait_for_reset(dsi);
 	samsung_dsim_set_phy_ctrl(dsi);
-	samsung_dsim_init_link(dsi);
+	samsung_dsim_init_link(dsi, force_stop_state);
 
 	dsi->state |= DSIM_STATE_INITIALIZED;
 
@@ -1432,7 +1432,7 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
 	 * the host initialization during DSI transfer.
 	 */
 	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
-		ret = samsung_dsim_init(dsi);
+		ret = samsung_dsim_init(dsi, true);
 		if (ret)
 			return;
 
@@ -1771,7 +1771,7 @@ static ssize_t samsung_dsim_host_transfer(struct mipi_dsi_host *host,
 	if (!(dsi->state & DSIM_STATE_ENABLED))
 		return -EINVAL;
 
-	ret = samsung_dsim_init(dsi);
+	ret = samsung_dsim_init(dsi, false);
 	if (ret)
 		return ret;
 
-- 
2.41.0

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Tim Harvey]

On Thu, Jul 13, 2023 at 3:01 AM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> Hi Tim,
>
> On 13.07.23 09:18, Frieder Schrempf wrote:
> > Hi Tim,
> >
> > On 13.07.23 00:34, Tim Harvey wrote:
> >> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
> >>>
> >>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>>
> >>> According to the documentation [1] the proper enable flow is:
> >>>
> >>> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
> >>> 2. Disable stop state to bring data lanes into HS mode
> >>>
> >>> Currently we do this all at once within enable(), which doesn't
> >>> allow to meet the requirements of some downstream bridges.
> >>>
> >>> To fix this we now enable the DSI in pre_enable() and force it
> >>> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
> >>> register until enable() is called where we reset the bit.
> >>>
> >>> We currently do this only for i.MX8M as Exynos uses a different
> >>> init flow where samsung_dsim_init() is called from
> >>> samsung_dsim_host_transfer().
> >>>
> >>> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> >>>
> >>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>> ---
> >>> Changes for v2:
> >>> * Drop RFC
> >>> ---
> >>>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
> >>>  1 file changed, 23 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>> index e0a402a85787..9775779721d9 100644
> >>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> >>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
> >>>         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> >>>         reg &= ~DSIM_STOP_STATE_CNT_MASK;
> >>>         reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> >>> +
> >>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> >>> +               reg |= DSIM_FORCE_STOP_STATE;
> >>> +
> >>>         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >>>
> >>>         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
> >>> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
> >>>                 ret = samsung_dsim_init(dsi);
> >>>                 if (ret)
> >>>                         return;
> >>> +
> >>> +               samsung_dsim_set_display_mode(dsi);
> >>> +               samsung_dsim_set_display_enable(dsi, true);
> >>>         }
> >>>  }
> >>>
> >>> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
> >>>                                        struct drm_bridge_state *old_bridge_state)
> >>>  {
> >>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> >>> +       u32 reg;
> >>>
> >>> -       samsung_dsim_set_display_mode(dsi);
> >>> -       samsung_dsim_set_display_enable(dsi, true);
> >>> +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> >>> +               samsung_dsim_set_display_mode(dsi);
> >>> +               samsung_dsim_set_display_enable(dsi, true);
> >>> +       } else {
> >>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> >>> +               reg &= ~DSIM_FORCE_STOP_STATE;
> >>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >>> +       }
> >>>
> >>>         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
> >>>  }
> >>> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
> >>>                                         struct drm_bridge_state *old_bridge_state)
> >>>  {
> >>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> >>> +       u32 reg;
> >>>
> >>>         if (!(dsi->state & DSIM_STATE_ENABLED))
> >>>                 return;
> >>>
> >>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> >>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> >>> +               reg |= DSIM_FORCE_STOP_STATE;
> >>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >>> +       }
> >>> +
> >>>         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
> >>>  }
> >>>
> >>> --
> >>> 2.40.0
> >>>
> >>
> >> Hi Frieder,
> >>
> >> I found this patch to break mipi-dsi display on my board which has:
> >>  - FocalTech FT5406 10pt touch controller (with no interrupt)
> >>  - Powertip PH800480T013-IDF02 compatible panel
> >>  - Toshiba TC358762 compatible DSI to DBI bridge
> >>  - ATTINY based regulator used for backlight controller and panel enable
> >>
> >> I enable this via a dt overlay in a pending patch
> >> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
> >> 6.5-rc1 which has this patch.
> >>
> >> The issue appears as:
> >> [    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
> >> 64 01 05 00 00 00
> >> [    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)
> >>
> >> Instead of
> >> [    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
> >> using dummy regulator
> >> [    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
> >> using dummy regulator
> >> [    5.649928] samsung-dsim 32e10000.dsi:
> >> [drm:samsung_dsim_host_attach] Attached tc358762 device
> >>
> >> I'm curious what board/panel were you needing this for and do you have
> >> any ideas why it broke my setup?
> >>
> >> I'm also curious what board/panel Alexander tested this with and if
> >> Adam or Jagan (or others) have tested this with their hardware?
> >
> > Sorry for breaking your setup. My test- and use-case is the same as
> > Alexander's. I have the SN65DSI84 downstream bridge and without this
> > patch it fails to come up in some cases.
> >
> > The failure is probably related to your downstream bridge being
> > controlled by the DSI itself using command mode. The SN65DSI84 is
> > instead controlled via I2C.
> >
> > Actually we should have tested this with a bridge that uses command mode
> > before merging, now that I think of it. But I wasn't really aware of
> > this until now.
> >
> > I'll have a closer look at the issue and then will get back to you. In
> > the meantime if anyone can help analyze the problem or has proposals how
> > to fix it, please let us know.
>
> With my patch samsung_dsim_init() now initializes the DSIM to stop
> state. When being called from samsung_dsim_atomic_pre_enable() this
> works as the stop state is cleared later in samsung_dsim_atomic_enable().
>
> When being called from samsung_dsim_host_transfer() to handle transfers
> before samsung_dsim_atomic_pre_enable() was called, the stop state is
> never cleared and transfers will fail.
>
> This is the case in your setup as tc358762_init() is called in
> tc358762_pre_enable().
>
> I think that requiring to initialize the DSI host during transfer could
> be avoided in this case by moving tc358762_init() from
> tc358762_pre_enable() to tc358762_enable().
>
> But at the same time according to the docs at [1] this seems to be a
> valid case that we need to support in the DSIM driver:
>
>   Whilst it is valid to call host_transfer prior to pre_enable or
>   after post_disable, the exact state of the lanes is undefined at
>   this point. The DSI host should initialise the interface, transmit
>   the data, and then disable the interface again.
>
> Therefore I would propose to try a fix like in the attachement. If you
> could test this, that would be great.
>
> Thanks
> Frieder
>
> [1]
> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

Hi Frieder,

The patch does not resolve the issue. I still get the 'xfer timed out'
from samsung-dsim but noticing today that this issue doesn't exist in
linux-next I've found that its resolved by Marek's patch:
commit 8a4b2fc9c91a ("drm/bridge: tc358762: Split register programming
from pre-enable to enable")

I'm not clear on how that patch is staged in linux-next. If we can get
that pulled into 6.5 then it will resolve the breakage.

best regards,

Tim

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Tim Harvey]

On Thu, Jul 13, 2023 at 3:01 AM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> Hi Tim,
>
> On 13.07.23 09:18, Frieder Schrempf wrote:
> > Hi Tim,
> >
> > On 13.07.23 00:34, Tim Harvey wrote:
> >> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
> >>>
> >>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>>
> >>> According to the documentation [1] the proper enable flow is:
> >>>
> >>> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
> >>> 2. Disable stop state to bring data lanes into HS mode
> >>>
> >>> Currently we do this all at once within enable(), which doesn't
> >>> allow to meet the requirements of some downstream bridges.
> >>>
> >>> To fix this we now enable the DSI in pre_enable() and force it
> >>> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
> >>> register until enable() is called where we reset the bit.
> >>>
> >>> We currently do this only for i.MX8M as Exynos uses a different
> >>> init flow where samsung_dsim_init() is called from
> >>> samsung_dsim_host_transfer().
> >>>
> >>> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> >>>
> >>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>> ---
> >>> Changes for v2:
> >>> * Drop RFC
> >>> ---
> >>>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
> >>>  1 file changed, 23 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>> index e0a402a85787..9775779721d9 100644
> >>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> >>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
> >>>         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> >>>         reg &= ~DSIM_STOP_STATE_CNT_MASK;
> >>>         reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> >>> +
> >>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> >>> +               reg |= DSIM_FORCE_STOP_STATE;
> >>> +
> >>>         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >>>
> >>>         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
> >>> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
> >>>                 ret = samsung_dsim_init(dsi);
> >>>                 if (ret)
> >>>                         return;
> >>> +
> >>> +               samsung_dsim_set_display_mode(dsi);
> >>> +               samsung_dsim_set_display_enable(dsi, true);
> >>>         }
> >>>  }
> >>>
> >>> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
> >>>                                        struct drm_bridge_state *old_bridge_state)
> >>>  {
> >>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> >>> +       u32 reg;
> >>>
> >>> -       samsung_dsim_set_display_mode(dsi);
> >>> -       samsung_dsim_set_display_enable(dsi, true);
> >>> +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> >>> +               samsung_dsim_set_display_mode(dsi);
> >>> +               samsung_dsim_set_display_enable(dsi, true);
> >>> +       } else {
> >>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> >>> +               reg &= ~DSIM_FORCE_STOP_STATE;
> >>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >>> +       }
> >>>
> >>>         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
> >>>  }
> >>> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
> >>>                                         struct drm_bridge_state *old_bridge_state)
> >>>  {
> >>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> >>> +       u32 reg;
> >>>
> >>>         if (!(dsi->state & DSIM_STATE_ENABLED))
> >>>                 return;
> >>>
> >>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> >>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> >>> +               reg |= DSIM_FORCE_STOP_STATE;
> >>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >>> +       }
> >>> +
> >>>         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
> >>>  }
> >>>
> >>> --
> >>> 2.40.0
> >>>
> >>
> >> Hi Frieder,
> >>
> >> I found this patch to break mipi-dsi display on my board which has:
> >>  - FocalTech FT5406 10pt touch controller (with no interrupt)
> >>  - Powertip PH800480T013-IDF02 compatible panel
> >>  - Toshiba TC358762 compatible DSI to DBI bridge
> >>  - ATTINY based regulator used for backlight controller and panel enable
> >>
> >> I enable this via a dt overlay in a pending patch
> >> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
> >> 6.5-rc1 which has this patch.
> >>
> >> The issue appears as:
> >> [    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
> >> 64 01 05 00 00 00
> >> [    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)
> >>
> >> Instead of
> >> [    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
> >> using dummy regulator
> >> [    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
> >> using dummy regulator
> >> [    5.649928] samsung-dsim 32e10000.dsi:
> >> [drm:samsung_dsim_host_attach] Attached tc358762 device
> >>
> >> I'm curious what board/panel were you needing this for and do you have
> >> any ideas why it broke my setup?
> >>
> >> I'm also curious what board/panel Alexander tested this with and if
> >> Adam or Jagan (or others) have tested this with their hardware?
> >
> > Sorry for breaking your setup. My test- and use-case is the same as
> > Alexander's. I have the SN65DSI84 downstream bridge and without this
> > patch it fails to come up in some cases.
> >
> > The failure is probably related to your downstream bridge being
> > controlled by the DSI itself using command mode. The SN65DSI84 is
> > instead controlled via I2C.
> >
> > Actually we should have tested this with a bridge that uses command mode
> > before merging, now that I think of it. But I wasn't really aware of
> > this until now.
> >
> > I'll have a closer look at the issue and then will get back to you. In
> > the meantime if anyone can help analyze the problem or has proposals how
> > to fix it, please let us know.
>
> With my patch samsung_dsim_init() now initializes the DSIM to stop
> state. When being called from samsung_dsim_atomic_pre_enable() this
> works as the stop state is cleared later in samsung_dsim_atomic_enable().
>
> When being called from samsung_dsim_host_transfer() to handle transfers
> before samsung_dsim_atomic_pre_enable() was called, the stop state is
> never cleared and transfers will fail.
>
> This is the case in your setup as tc358762_init() is called in
> tc358762_pre_enable().
>
> I think that requiring to initialize the DSI host during transfer could
> be avoided in this case by moving tc358762_init() from
> tc358762_pre_enable() to tc358762_enable().
>
> But at the same time according to the docs at [1] this seems to be a
> valid case that we need to support in the DSIM driver:
>
>   Whilst it is valid to call host_transfer prior to pre_enable or
>   after post_disable, the exact state of the lanes is undefined at
>   this point. The DSI host should initialise the interface, transmit
>   the data, and then disable the interface again.
>
> Therefore I would propose to try a fix like in the attachement. If you
> could test this, that would be great.
>
> Thanks
> Frieder
>
> [1]
> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

Hi Frieder,

The patch does not resolve the issue. I still get the 'xfer timed out'
from samsung-dsim but noticing today that this issue doesn't exist in
linux-next I've found that its resolved by Marek's patch:
commit 8a4b2fc9c91a ("drm/bridge: tc358762: Split register programming
from pre-enable to enable")

I'm not clear on how that patch is staged in linux-next. If we can get
that pulled into 6.5 then it will resolve the breakage.

best regards,

Tim

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Frieder Schrempf]

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

Hi Tim,

On 19.07.23 01:03, Tim Harvey wrote:
> On Thu, Jul 13, 2023 at 3:01 AM Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
>>
>> Hi Tim,
>>
>> On 13.07.23 09:18, Frieder Schrempf wrote:
>>> Hi Tim,
>>>
>>> On 13.07.23 00:34, Tim Harvey wrote:
>>>> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
>>>>>
>>>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>>>
>>>>> According to the documentation [1] the proper enable flow is:
>>>>>
>>>>> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
>>>>> 2. Disable stop state to bring data lanes into HS mode
>>>>>
>>>>> Currently we do this all at once within enable(), which doesn't
>>>>> allow to meet the requirements of some downstream bridges.
>>>>>
>>>>> To fix this we now enable the DSI in pre_enable() and force it
>>>>> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
>>>>> register until enable() is called where we reset the bit.
>>>>>
>>>>> We currently do this only for i.MX8M as Exynos uses a different
>>>>> init flow where samsung_dsim_init() is called from
>>>>> samsung_dsim_host_transfer().
>>>>>
>>>>> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>>>>>
>>>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>>> ---
>>>>> Changes for v2:
>>>>> * Drop RFC
>>>>> ---
>>>>>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
>>>>>  1 file changed, 23 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> index e0a402a85787..9775779721d9 100644
>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
>>>>>         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>>>         reg &= ~DSIM_STOP_STATE_CNT_MASK;
>>>>>         reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
>>>>> +
>>>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
>>>>> +               reg |= DSIM_FORCE_STOP_STATE;
>>>>> +
>>>>>         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>>>
>>>>>         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
>>>>> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>>>>>                 ret = samsung_dsim_init(dsi);
>>>>>                 if (ret)
>>>>>                         return;
>>>>> +
>>>>> +               samsung_dsim_set_display_mode(dsi);
>>>>> +               samsung_dsim_set_display_enable(dsi, true);
>>>>>         }
>>>>>  }
>>>>>
>>>>> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>>>>>                                        struct drm_bridge_state *old_bridge_state)
>>>>>  {
>>>>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>>>>> +       u32 reg;
>>>>>
>>>>> -       samsung_dsim_set_display_mode(dsi);
>>>>> -       samsung_dsim_set_display_enable(dsi, true);
>>>>> +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>>>>> +               samsung_dsim_set_display_mode(dsi);
>>>>> +               samsung_dsim_set_display_enable(dsi, true);
>>>>> +       } else {
>>>>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>>> +               reg &= ~DSIM_FORCE_STOP_STATE;
>>>>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>>> +       }
>>>>>
>>>>>         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>>>>>  }
>>>>> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
>>>>>                                         struct drm_bridge_state *old_bridge_state)
>>>>>  {
>>>>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>>>>> +       u32 reg;
>>>>>
>>>>>         if (!(dsi->state & DSIM_STATE_ENABLED))
>>>>>                 return;
>>>>>
>>>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>>>>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>>> +               reg |= DSIM_FORCE_STOP_STATE;
>>>>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>>> +       }
>>>>> +
>>>>>         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>>>>>  }
>>>>>
>>>>> --
>>>>> 2.40.0
>>>>>
>>>>
>>>> Hi Frieder,
>>>>
>>>> I found this patch to break mipi-dsi display on my board which has:
>>>>  - FocalTech FT5406 10pt touch controller (with no interrupt)
>>>>  - Powertip PH800480T013-IDF02 compatible panel
>>>>  - Toshiba TC358762 compatible DSI to DBI bridge
>>>>  - ATTINY based regulator used for backlight controller and panel enable
>>>>
>>>> I enable this via a dt overlay in a pending patch
>>>> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
>>>> 6.5-rc1 which has this patch.
>>>>
>>>> The issue appears as:
>>>> [    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
>>>> 64 01 05 00 00 00
>>>> [    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)
>>>>
>>>> Instead of
>>>> [    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
>>>> using dummy regulator
>>>> [    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
>>>> using dummy regulator
>>>> [    5.649928] samsung-dsim 32e10000.dsi:
>>>> [drm:samsung_dsim_host_attach] Attached tc358762 device
>>>>
>>>> I'm curious what board/panel were you needing this for and do you have
>>>> any ideas why it broke my setup?
>>>>
>>>> I'm also curious what board/panel Alexander tested this with and if
>>>> Adam or Jagan (or others) have tested this with their hardware?
>>>
>>> Sorry for breaking your setup. My test- and use-case is the same as
>>> Alexander's. I have the SN65DSI84 downstream bridge and without this
>>> patch it fails to come up in some cases.
>>>
>>> The failure is probably related to your downstream bridge being
>>> controlled by the DSI itself using command mode. The SN65DSI84 is
>>> instead controlled via I2C.
>>>
>>> Actually we should have tested this with a bridge that uses command mode
>>> before merging, now that I think of it. But I wasn't really aware of
>>> this until now.
>>>
>>> I'll have a closer look at the issue and then will get back to you. In
>>> the meantime if anyone can help analyze the problem or has proposals how
>>> to fix it, please let us know.
>>
>> With my patch samsung_dsim_init() now initializes the DSIM to stop
>> state. When being called from samsung_dsim_atomic_pre_enable() this
>> works as the stop state is cleared later in samsung_dsim_atomic_enable().
>>
>> When being called from samsung_dsim_host_transfer() to handle transfers
>> before samsung_dsim_atomic_pre_enable() was called, the stop state is
>> never cleared and transfers will fail.
>>
>> This is the case in your setup as tc358762_init() is called in
>> tc358762_pre_enable().
>>
>> I think that requiring to initialize the DSI host during transfer could
>> be avoided in this case by moving tc358762_init() from
>> tc358762_pre_enable() to tc358762_enable().
>>
>> But at the same time according to the docs at [1] this seems to be a
>> valid case that we need to support in the DSIM driver:
>>
>>   Whilst it is valid to call host_transfer prior to pre_enable or
>>   after post_disable, the exact state of the lanes is undefined at
>>   this point. The DSI host should initialise the interface, transmit
>>   the data, and then disable the interface again.
>>
>> Therefore I would propose to try a fix like in the attachement. If you
>> could test this, that would be great.
>>
>> Thanks
>> Frieder
>>
>> [1]
>> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> 
> Hi Frieder,
> 
> The patch does not resolve the issue. I still get the 'xfer timed out'
> from samsung-dsim but noticing today that this issue doesn't exist in
> linux-next I've found that its resolved by Marek's patch:
> commit 8a4b2fc9c91a ("drm/bridge: tc358762: Split register programming
> from pre-enable to enable")

Thanks for testing. I didn't notice there already is a patch from Marek
for the tc358762 driver. This is exactly the change that I was
considering above as a fix on the downstream bridge side.

> 
> I'm not clear on how that patch is staged in linux-next. If we can get
> that pulled into 6.5 then it will resolve the breakage.

Still the documentation says that the DSI host must be able to handle
this and there might be other drivers that are not yet fixed like this
or can't be changed to make DSI transfers only after the host's
pre_enable() was called.

Therefore I would prefer to fix the DSIM driver and apply this fix to
6.5-rc instead of backporting the tc358762 patch. I gave it another shot
and maybe you could do one more test with the attached patch and without
the fix in tc358762.

Thanks
Frieder

[-- Attachment #2: 0001-drm-bridge-samsung-dsim-Fix-init-during-host-transfe.patch --]
[-- Type: text/x-patch, Size: 3315 bytes --]

From 203984732d6af45d9a8bd71d0e1234965960ed19 Mon Sep 17 00:00:00 2001
From: Frieder Schrempf <frieder.schrempf@kontron.de>
Date: Thu, 13 Jul 2023 11:47:47 +0200
Subject: [PATCH] drm: bridge: samsung-dsim: Fix init during host transfer

In case the downstream bridge or panel uses DSI transfers before the
DSI host was actually initialized through samsung_dsim_atomic_enable()
which clears the stop state (LP11) mode, all transfers will fail.

This happens with downstream bridges that are controlled by DSI
commands such as the tc358762.

To fix this do not enable stop state when the DSI host was initialized
through samsung_dsim_host_transfer() which restores the previous
behavior.

Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec")
Reported-by: Tim Harvey <tharvey@gateworks.com>
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 043b8109e64a..1714fa725e24 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -833,7 +833,7 @@ static void samsung_dsim_enable_lane(struct samsung_dsim *dsi, u32 lane)
 	samsung_dsim_write(dsi, DSIM_CONFIG_REG, reg);
 }
 
-static int samsung_dsim_init_link(struct samsung_dsim *dsi)
+static int samsung_dsim_init_link(struct samsung_dsim *dsi, bool force_stop_state)
 {
 	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
 	int timeout;
@@ -939,8 +939,12 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
 	reg &= ~DSIM_STOP_STATE_CNT_MASK;
 	reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
 
-	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
-		reg |= DSIM_FORCE_STOP_STATE;
+	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
+		if (force_stop_state)
+			reg |= DSIM_FORCE_STOP_STATE;
+		else
+			reg &= ~DSIM_FORCE_STOP_STATE;
+	}
 
 	samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
 
@@ -1386,7 +1390,7 @@ static void samsung_dsim_disable_irq(struct samsung_dsim *dsi)
 	disable_irq(dsi->irq);
 }
 
-static int samsung_dsim_init(struct samsung_dsim *dsi)
+static int samsung_dsim_init(struct samsung_dsim *dsi, bool force_stop_state)
 {
 	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
 
@@ -1403,7 +1407,7 @@ static int samsung_dsim_init(struct samsung_dsim *dsi)
 	if (driver_data->wait_for_reset)
 		samsung_dsim_wait_for_reset(dsi);
 	samsung_dsim_set_phy_ctrl(dsi);
-	samsung_dsim_init_link(dsi);
+	samsung_dsim_init_link(dsi, force_stop_state);
 
 	dsi->state |= DSIM_STATE_INITIALIZED;
 
@@ -1432,7 +1436,7 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
 	 * the host initialization during DSI transfer.
 	 */
 	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
-		ret = samsung_dsim_init(dsi);
+		ret = samsung_dsim_init(dsi, true);
 		if (ret)
 			return;
 
@@ -1771,7 +1775,7 @@ static ssize_t samsung_dsim_host_transfer(struct mipi_dsi_host *host,
 	if (!(dsi->state & DSIM_STATE_ENABLED))
 		return -EINVAL;
 
-	ret = samsung_dsim_init(dsi);
+	ret = samsung_dsim_init(dsi, false);
 	if (ret)
 		return ret;
 
-- 
2.41.0

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Frieder Schrempf]

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

Hi Tim,

On 19.07.23 01:03, Tim Harvey wrote:
> On Thu, Jul 13, 2023 at 3:01 AM Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
>>
>> Hi Tim,
>>
>> On 13.07.23 09:18, Frieder Schrempf wrote:
>>> Hi Tim,
>>>
>>> On 13.07.23 00:34, Tim Harvey wrote:
>>>> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
>>>>>
>>>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>>>
>>>>> According to the documentation [1] the proper enable flow is:
>>>>>
>>>>> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
>>>>> 2. Disable stop state to bring data lanes into HS mode
>>>>>
>>>>> Currently we do this all at once within enable(), which doesn't
>>>>> allow to meet the requirements of some downstream bridges.
>>>>>
>>>>> To fix this we now enable the DSI in pre_enable() and force it
>>>>> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
>>>>> register until enable() is called where we reset the bit.
>>>>>
>>>>> We currently do this only for i.MX8M as Exynos uses a different
>>>>> init flow where samsung_dsim_init() is called from
>>>>> samsung_dsim_host_transfer().
>>>>>
>>>>> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>>>>>
>>>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>>> ---
>>>>> Changes for v2:
>>>>> * Drop RFC
>>>>> ---
>>>>>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
>>>>>  1 file changed, 23 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> index e0a402a85787..9775779721d9 100644
>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
>>>>>         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>>>         reg &= ~DSIM_STOP_STATE_CNT_MASK;
>>>>>         reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
>>>>> +
>>>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
>>>>> +               reg |= DSIM_FORCE_STOP_STATE;
>>>>> +
>>>>>         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>>>
>>>>>         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
>>>>> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>>>>>                 ret = samsung_dsim_init(dsi);
>>>>>                 if (ret)
>>>>>                         return;
>>>>> +
>>>>> +               samsung_dsim_set_display_mode(dsi);
>>>>> +               samsung_dsim_set_display_enable(dsi, true);
>>>>>         }
>>>>>  }
>>>>>
>>>>> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>>>>>                                        struct drm_bridge_state *old_bridge_state)
>>>>>  {
>>>>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>>>>> +       u32 reg;
>>>>>
>>>>> -       samsung_dsim_set_display_mode(dsi);
>>>>> -       samsung_dsim_set_display_enable(dsi, true);
>>>>> +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>>>>> +               samsung_dsim_set_display_mode(dsi);
>>>>> +               samsung_dsim_set_display_enable(dsi, true);
>>>>> +       } else {
>>>>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>>> +               reg &= ~DSIM_FORCE_STOP_STATE;
>>>>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>>> +       }
>>>>>
>>>>>         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>>>>>  }
>>>>> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
>>>>>                                         struct drm_bridge_state *old_bridge_state)
>>>>>  {
>>>>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>>>>> +       u32 reg;
>>>>>
>>>>>         if (!(dsi->state & DSIM_STATE_ENABLED))
>>>>>                 return;
>>>>>
>>>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>>>>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>>> +               reg |= DSIM_FORCE_STOP_STATE;
>>>>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>>> +       }
>>>>> +
>>>>>         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>>>>>  }
>>>>>
>>>>> --
>>>>> 2.40.0
>>>>>
>>>>
>>>> Hi Frieder,
>>>>
>>>> I found this patch to break mipi-dsi display on my board which has:
>>>>  - FocalTech FT5406 10pt touch controller (with no interrupt)
>>>>  - Powertip PH800480T013-IDF02 compatible panel
>>>>  - Toshiba TC358762 compatible DSI to DBI bridge
>>>>  - ATTINY based regulator used for backlight controller and panel enable
>>>>
>>>> I enable this via a dt overlay in a pending patch
>>>> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
>>>> 6.5-rc1 which has this patch.
>>>>
>>>> The issue appears as:
>>>> [    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
>>>> 64 01 05 00 00 00
>>>> [    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)
>>>>
>>>> Instead of
>>>> [    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
>>>> using dummy regulator
>>>> [    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
>>>> using dummy regulator
>>>> [    5.649928] samsung-dsim 32e10000.dsi:
>>>> [drm:samsung_dsim_host_attach] Attached tc358762 device
>>>>
>>>> I'm curious what board/panel were you needing this for and do you have
>>>> any ideas why it broke my setup?
>>>>
>>>> I'm also curious what board/panel Alexander tested this with and if
>>>> Adam or Jagan (or others) have tested this with their hardware?
>>>
>>> Sorry for breaking your setup. My test- and use-case is the same as
>>> Alexander's. I have the SN65DSI84 downstream bridge and without this
>>> patch it fails to come up in some cases.
>>>
>>> The failure is probably related to your downstream bridge being
>>> controlled by the DSI itself using command mode. The SN65DSI84 is
>>> instead controlled via I2C.
>>>
>>> Actually we should have tested this with a bridge that uses command mode
>>> before merging, now that I think of it. But I wasn't really aware of
>>> this until now.
>>>
>>> I'll have a closer look at the issue and then will get back to you. In
>>> the meantime if anyone can help analyze the problem or has proposals how
>>> to fix it, please let us know.
>>
>> With my patch samsung_dsim_init() now initializes the DSIM to stop
>> state. When being called from samsung_dsim_atomic_pre_enable() this
>> works as the stop state is cleared later in samsung_dsim_atomic_enable().
>>
>> When being called from samsung_dsim_host_transfer() to handle transfers
>> before samsung_dsim_atomic_pre_enable() was called, the stop state is
>> never cleared and transfers will fail.
>>
>> This is the case in your setup as tc358762_init() is called in
>> tc358762_pre_enable().
>>
>> I think that requiring to initialize the DSI host during transfer could
>> be avoided in this case by moving tc358762_init() from
>> tc358762_pre_enable() to tc358762_enable().
>>
>> But at the same time according to the docs at [1] this seems to be a
>> valid case that we need to support in the DSIM driver:
>>
>>   Whilst it is valid to call host_transfer prior to pre_enable or
>>   after post_disable, the exact state of the lanes is undefined at
>>   this point. The DSI host should initialise the interface, transmit
>>   the data, and then disable the interface again.
>>
>> Therefore I would propose to try a fix like in the attachement. If you
>> could test this, that would be great.
>>
>> Thanks
>> Frieder
>>
>> [1]
>> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> 
> Hi Frieder,
> 
> The patch does not resolve the issue. I still get the 'xfer timed out'
> from samsung-dsim but noticing today that this issue doesn't exist in
> linux-next I've found that its resolved by Marek's patch:
> commit 8a4b2fc9c91a ("drm/bridge: tc358762: Split register programming
> from pre-enable to enable")

Thanks for testing. I didn't notice there already is a patch from Marek
for the tc358762 driver. This is exactly the change that I was
considering above as a fix on the downstream bridge side.

> 
> I'm not clear on how that patch is staged in linux-next. If we can get
> that pulled into 6.5 then it will resolve the breakage.

Still the documentation says that the DSI host must be able to handle
this and there might be other drivers that are not yet fixed like this
or can't be changed to make DSI transfers only after the host's
pre_enable() was called.

Therefore I would prefer to fix the DSIM driver and apply this fix to
6.5-rc instead of backporting the tc358762 patch. I gave it another shot
and maybe you could do one more test with the attached patch and without
the fix in tc358762.

Thanks
Frieder

[-- Attachment #2: 0001-drm-bridge-samsung-dsim-Fix-init-during-host-transfe.patch --]
[-- Type: text/x-patch, Size: 3315 bytes --]

From 203984732d6af45d9a8bd71d0e1234965960ed19 Mon Sep 17 00:00:00 2001
From: Frieder Schrempf <frieder.schrempf@kontron.de>
Date: Thu, 13 Jul 2023 11:47:47 +0200
Subject: [PATCH] drm: bridge: samsung-dsim: Fix init during host transfer

In case the downstream bridge or panel uses DSI transfers before the
DSI host was actually initialized through samsung_dsim_atomic_enable()
which clears the stop state (LP11) mode, all transfers will fail.

This happens with downstream bridges that are controlled by DSI
commands such as the tc358762.

To fix this do not enable stop state when the DSI host was initialized
through samsung_dsim_host_transfer() which restores the previous
behavior.

Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec")
Reported-by: Tim Harvey <tharvey@gateworks.com>
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 043b8109e64a..1714fa725e24 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -833,7 +833,7 @@ static void samsung_dsim_enable_lane(struct samsung_dsim *dsi, u32 lane)
 	samsung_dsim_write(dsi, DSIM_CONFIG_REG, reg);
 }
 
-static int samsung_dsim_init_link(struct samsung_dsim *dsi)
+static int samsung_dsim_init_link(struct samsung_dsim *dsi, bool force_stop_state)
 {
 	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
 	int timeout;
@@ -939,8 +939,12 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
 	reg &= ~DSIM_STOP_STATE_CNT_MASK;
 	reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
 
-	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
-		reg |= DSIM_FORCE_STOP_STATE;
+	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
+		if (force_stop_state)
+			reg |= DSIM_FORCE_STOP_STATE;
+		else
+			reg &= ~DSIM_FORCE_STOP_STATE;
+	}
 
 	samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
 
@@ -1386,7 +1390,7 @@ static void samsung_dsim_disable_irq(struct samsung_dsim *dsi)
 	disable_irq(dsi->irq);
 }
 
-static int samsung_dsim_init(struct samsung_dsim *dsi)
+static int samsung_dsim_init(struct samsung_dsim *dsi, bool force_stop_state)
 {
 	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
 
@@ -1403,7 +1407,7 @@ static int samsung_dsim_init(struct samsung_dsim *dsi)
 	if (driver_data->wait_for_reset)
 		samsung_dsim_wait_for_reset(dsi);
 	samsung_dsim_set_phy_ctrl(dsi);
-	samsung_dsim_init_link(dsi);
+	samsung_dsim_init_link(dsi, force_stop_state);
 
 	dsi->state |= DSIM_STATE_INITIALIZED;
 
@@ -1432,7 +1436,7 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
 	 * the host initialization during DSI transfer.
 	 */
 	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
-		ret = samsung_dsim_init(dsi);
+		ret = samsung_dsim_init(dsi, true);
 		if (ret)
 			return;
 
@@ -1771,7 +1775,7 @@ static ssize_t samsung_dsim_host_transfer(struct mipi_dsi_host *host,
 	if (!(dsi->state & DSIM_STATE_ENABLED))
 		return -EINVAL;
 
-	ret = samsung_dsim_init(dsi);
+	ret = samsung_dsim_init(dsi, false);
 	if (ret)
 		return ret;
 
-- 
2.41.0

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Tim Harvey]

On Wed, Jul 19, 2023 at 12:05 AM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> Hi Tim,
>
> On 19.07.23 01:03, Tim Harvey wrote:
> > On Thu, Jul 13, 2023 at 3:01 AM Frieder Schrempf
> > <frieder.schrempf@kontron.de> wrote:
> >>
> >> Hi Tim,
> >>
> >> On 13.07.23 09:18, Frieder Schrempf wrote:
> >>> Hi Tim,
> >>>
> >>> On 13.07.23 00:34, Tim Harvey wrote:
> >>>> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
> >>>>>
> >>>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>>>>
> >>>>> According to the documentation [1] the proper enable flow is:
> >>>>>
> >>>>> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
> >>>>> 2. Disable stop state to bring data lanes into HS mode
> >>>>>
> >>>>> Currently we do this all at once within enable(), which doesn't
> >>>>> allow to meet the requirements of some downstream bridges.
> >>>>>
> >>>>> To fix this we now enable the DSI in pre_enable() and force it
> >>>>> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
> >>>>> register until enable() is called where we reset the bit.
> >>>>>
> >>>>> We currently do this only for i.MX8M as Exynos uses a different
> >>>>> init flow where samsung_dsim_init() is called from
> >>>>> samsung_dsim_host_transfer().
> >>>>>
> >>>>> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> >>>>>
> >>>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>>>> ---
> >>>>> Changes for v2:
> >>>>> * Drop RFC
> >>>>> ---
> >>>>>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
> >>>>>  1 file changed, 23 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>> index e0a402a85787..9775779721d9 100644
> >>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
> >>>>>         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> >>>>>         reg &= ~DSIM_STOP_STATE_CNT_MASK;
> >>>>>         reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> >>>>> +
> >>>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> >>>>> +               reg |= DSIM_FORCE_STOP_STATE;
> >>>>> +
> >>>>>         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >>>>>
> >>>>>         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
> >>>>> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
> >>>>>                 ret = samsung_dsim_init(dsi);
> >>>>>                 if (ret)
> >>>>>                         return;
> >>>>> +
> >>>>> +               samsung_dsim_set_display_mode(dsi);
> >>>>> +               samsung_dsim_set_display_enable(dsi, true);
> >>>>>         }
> >>>>>  }
> >>>>>
> >>>>> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
> >>>>>                                        struct drm_bridge_state *old_bridge_state)
> >>>>>  {
> >>>>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> >>>>> +       u32 reg;
> >>>>>
> >>>>> -       samsung_dsim_set_display_mode(dsi);
> >>>>> -       samsung_dsim_set_display_enable(dsi, true);
> >>>>> +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> >>>>> +               samsung_dsim_set_display_mode(dsi);
> >>>>> +               samsung_dsim_set_display_enable(dsi, true);
> >>>>> +       } else {
> >>>>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> >>>>> +               reg &= ~DSIM_FORCE_STOP_STATE;
> >>>>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >>>>> +       }
> >>>>>
> >>>>>         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
> >>>>>  }
> >>>>> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
> >>>>>                                         struct drm_bridge_state *old_bridge_state)
> >>>>>  {
> >>>>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> >>>>> +       u32 reg;
> >>>>>
> >>>>>         if (!(dsi->state & DSIM_STATE_ENABLED))
> >>>>>                 return;
> >>>>>
> >>>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> >>>>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> >>>>> +               reg |= DSIM_FORCE_STOP_STATE;
> >>>>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >>>>> +       }
> >>>>> +
> >>>>>         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
> >>>>>  }
> >>>>>
> >>>>> --
> >>>>> 2.40.0
> >>>>>
> >>>>
> >>>> Hi Frieder,
> >>>>
> >>>> I found this patch to break mipi-dsi display on my board which has:
> >>>>  - FocalTech FT5406 10pt touch controller (with no interrupt)
> >>>>  - Powertip PH800480T013-IDF02 compatible panel
> >>>>  - Toshiba TC358762 compatible DSI to DBI bridge
> >>>>  - ATTINY based regulator used for backlight controller and panel enable
> >>>>
> >>>> I enable this via a dt overlay in a pending patch
> >>>> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
> >>>> 6.5-rc1 which has this patch.
> >>>>
> >>>> The issue appears as:
> >>>> [    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
> >>>> 64 01 05 00 00 00
> >>>> [    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)
> >>>>
> >>>> Instead of
> >>>> [    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
> >>>> using dummy regulator
> >>>> [    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
> >>>> using dummy regulator
> >>>> [    5.649928] samsung-dsim 32e10000.dsi:
> >>>> [drm:samsung_dsim_host_attach] Attached tc358762 device
> >>>>
> >>>> I'm curious what board/panel were you needing this for and do you have
> >>>> any ideas why it broke my setup?
> >>>>
> >>>> I'm also curious what board/panel Alexander tested this with and if
> >>>> Adam or Jagan (or others) have tested this with their hardware?
> >>>
> >>> Sorry for breaking your setup. My test- and use-case is the same as
> >>> Alexander's. I have the SN65DSI84 downstream bridge and without this
> >>> patch it fails to come up in some cases.
> >>>
> >>> The failure is probably related to your downstream bridge being
> >>> controlled by the DSI itself using command mode. The SN65DSI84 is
> >>> instead controlled via I2C.
> >>>
> >>> Actually we should have tested this with a bridge that uses command mode
> >>> before merging, now that I think of it. But I wasn't really aware of
> >>> this until now.
> >>>
> >>> I'll have a closer look at the issue and then will get back to you. In
> >>> the meantime if anyone can help analyze the problem or has proposals how
> >>> to fix it, please let us know.
> >>
> >> With my patch samsung_dsim_init() now initializes the DSIM to stop
> >> state. When being called from samsung_dsim_atomic_pre_enable() this
> >> works as the stop state is cleared later in samsung_dsim_atomic_enable().
> >>
> >> When being called from samsung_dsim_host_transfer() to handle transfers
> >> before samsung_dsim_atomic_pre_enable() was called, the stop state is
> >> never cleared and transfers will fail.
> >>
> >> This is the case in your setup as tc358762_init() is called in
> >> tc358762_pre_enable().
> >>
> >> I think that requiring to initialize the DSI host during transfer could
> >> be avoided in this case by moving tc358762_init() from
> >> tc358762_pre_enable() to tc358762_enable().
> >>
> >> But at the same time according to the docs at [1] this seems to be a
> >> valid case that we need to support in the DSIM driver:
> >>
> >>   Whilst it is valid to call host_transfer prior to pre_enable or
> >>   after post_disable, the exact state of the lanes is undefined at
> >>   this point. The DSI host should initialise the interface, transmit
> >>   the data, and then disable the interface again.
> >>
> >> Therefore I would propose to try a fix like in the attachement. If you
> >> could test this, that would be great.
> >>
> >> Thanks
> >> Frieder
> >>
> >> [1]
> >> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> >
> > Hi Frieder,
> >
> > The patch does not resolve the issue. I still get the 'xfer timed out'
> > from samsung-dsim but noticing today that this issue doesn't exist in
> > linux-next I've found that its resolved by Marek's patch:
> > commit 8a4b2fc9c91a ("drm/bridge: tc358762: Split register programming
> > from pre-enable to enable")
>
> Thanks for testing. I didn't notice there already is a patch from Marek
> for the tc358762 driver. This is exactly the change that I was
> considering above as a fix on the downstream bridge side.
>
> >
> > I'm not clear on how that patch is staged in linux-next. If we can get
> > that pulled into 6.5 then it will resolve the breakage.
>
> Still the documentation says that the DSI host must be able to handle
> this and there might be other drivers that are not yet fixed like this
> or can't be changed to make DSI transfers only after the host's
> pre_enable() was called.
>
> Therefore I would prefer to fix the DSIM driver and apply this fix to
> 6.5-rc instead of backporting the tc358762 patch. I gave it another shot
> and maybe you could do one more test with the attached patch and without
> the fix in tc358762.
>

Frieder,

This patch doesn't resolve the issue either. Let me know if there is
some quick debugging you want to add somewhere. I don't have a lot of
time to troubleshoot this week as I'm trying to wrap up some work
before a 2-week vacation but it's quick for me to apply patches and do
a quick boot test.

best regards,

Tim

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Tim Harvey]

On Wed, Jul 19, 2023 at 12:05 AM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> Hi Tim,
>
> On 19.07.23 01:03, Tim Harvey wrote:
> > On Thu, Jul 13, 2023 at 3:01 AM Frieder Schrempf
> > <frieder.schrempf@kontron.de> wrote:
> >>
> >> Hi Tim,
> >>
> >> On 13.07.23 09:18, Frieder Schrempf wrote:
> >>> Hi Tim,
> >>>
> >>> On 13.07.23 00:34, Tim Harvey wrote:
> >>>> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
> >>>>>
> >>>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>>>>
> >>>>> According to the documentation [1] the proper enable flow is:
> >>>>>
> >>>>> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
> >>>>> 2. Disable stop state to bring data lanes into HS mode
> >>>>>
> >>>>> Currently we do this all at once within enable(), which doesn't
> >>>>> allow to meet the requirements of some downstream bridges.
> >>>>>
> >>>>> To fix this we now enable the DSI in pre_enable() and force it
> >>>>> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
> >>>>> register until enable() is called where we reset the bit.
> >>>>>
> >>>>> We currently do this only for i.MX8M as Exynos uses a different
> >>>>> init flow where samsung_dsim_init() is called from
> >>>>> samsung_dsim_host_transfer().
> >>>>>
> >>>>> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> >>>>>
> >>>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> >>>>> ---
> >>>>> Changes for v2:
> >>>>> * Drop RFC
> >>>>> ---
> >>>>>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
> >>>>>  1 file changed, 23 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>> index e0a402a85787..9775779721d9 100644
> >>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
> >>>>>         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> >>>>>         reg &= ~DSIM_STOP_STATE_CNT_MASK;
> >>>>>         reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> >>>>> +
> >>>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> >>>>> +               reg |= DSIM_FORCE_STOP_STATE;
> >>>>> +
> >>>>>         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >>>>>
> >>>>>         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
> >>>>> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
> >>>>>                 ret = samsung_dsim_init(dsi);
> >>>>>                 if (ret)
> >>>>>                         return;
> >>>>> +
> >>>>> +               samsung_dsim_set_display_mode(dsi);
> >>>>> +               samsung_dsim_set_display_enable(dsi, true);
> >>>>>         }
> >>>>>  }
> >>>>>
> >>>>> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
> >>>>>                                        struct drm_bridge_state *old_bridge_state)
> >>>>>  {
> >>>>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> >>>>> +       u32 reg;
> >>>>>
> >>>>> -       samsung_dsim_set_display_mode(dsi);
> >>>>> -       samsung_dsim_set_display_enable(dsi, true);
> >>>>> +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> >>>>> +               samsung_dsim_set_display_mode(dsi);
> >>>>> +               samsung_dsim_set_display_enable(dsi, true);
> >>>>> +       } else {
> >>>>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> >>>>> +               reg &= ~DSIM_FORCE_STOP_STATE;
> >>>>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >>>>> +       }
> >>>>>
> >>>>>         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
> >>>>>  }
> >>>>> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
> >>>>>                                         struct drm_bridge_state *old_bridge_state)
> >>>>>  {
> >>>>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> >>>>> +       u32 reg;
> >>>>>
> >>>>>         if (!(dsi->state & DSIM_STATE_ENABLED))
> >>>>>                 return;
> >>>>>
> >>>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> >>>>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> >>>>> +               reg |= DSIM_FORCE_STOP_STATE;
> >>>>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> >>>>> +       }
> >>>>> +
> >>>>>         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
> >>>>>  }
> >>>>>
> >>>>> --
> >>>>> 2.40.0
> >>>>>
> >>>>
> >>>> Hi Frieder,
> >>>>
> >>>> I found this patch to break mipi-dsi display on my board which has:
> >>>>  - FocalTech FT5406 10pt touch controller (with no interrupt)
> >>>>  - Powertip PH800480T013-IDF02 compatible panel
> >>>>  - Toshiba TC358762 compatible DSI to DBI bridge
> >>>>  - ATTINY based regulator used for backlight controller and panel enable
> >>>>
> >>>> I enable this via a dt overlay in a pending patch
> >>>> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
> >>>> 6.5-rc1 which has this patch.
> >>>>
> >>>> The issue appears as:
> >>>> [    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
> >>>> 64 01 05 00 00 00
> >>>> [    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)
> >>>>
> >>>> Instead of
> >>>> [    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
> >>>> using dummy regulator
> >>>> [    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
> >>>> using dummy regulator
> >>>> [    5.649928] samsung-dsim 32e10000.dsi:
> >>>> [drm:samsung_dsim_host_attach] Attached tc358762 device
> >>>>
> >>>> I'm curious what board/panel were you needing this for and do you have
> >>>> any ideas why it broke my setup?
> >>>>
> >>>> I'm also curious what board/panel Alexander tested this with and if
> >>>> Adam or Jagan (or others) have tested this with their hardware?
> >>>
> >>> Sorry for breaking your setup. My test- and use-case is the same as
> >>> Alexander's. I have the SN65DSI84 downstream bridge and without this
> >>> patch it fails to come up in some cases.
> >>>
> >>> The failure is probably related to your downstream bridge being
> >>> controlled by the DSI itself using command mode. The SN65DSI84 is
> >>> instead controlled via I2C.
> >>>
> >>> Actually we should have tested this with a bridge that uses command mode
> >>> before merging, now that I think of it. But I wasn't really aware of
> >>> this until now.
> >>>
> >>> I'll have a closer look at the issue and then will get back to you. In
> >>> the meantime if anyone can help analyze the problem or has proposals how
> >>> to fix it, please let us know.
> >>
> >> With my patch samsung_dsim_init() now initializes the DSIM to stop
> >> state. When being called from samsung_dsim_atomic_pre_enable() this
> >> works as the stop state is cleared later in samsung_dsim_atomic_enable().
> >>
> >> When being called from samsung_dsim_host_transfer() to handle transfers
> >> before samsung_dsim_atomic_pre_enable() was called, the stop state is
> >> never cleared and transfers will fail.
> >>
> >> This is the case in your setup as tc358762_init() is called in
> >> tc358762_pre_enable().
> >>
> >> I think that requiring to initialize the DSI host during transfer could
> >> be avoided in this case by moving tc358762_init() from
> >> tc358762_pre_enable() to tc358762_enable().
> >>
> >> But at the same time according to the docs at [1] this seems to be a
> >> valid case that we need to support in the DSIM driver:
> >>
> >>   Whilst it is valid to call host_transfer prior to pre_enable or
> >>   after post_disable, the exact state of the lanes is undefined at
> >>   this point. The DSI host should initialise the interface, transmit
> >>   the data, and then disable the interface again.
> >>
> >> Therefore I would propose to try a fix like in the attachement. If you
> >> could test this, that would be great.
> >>
> >> Thanks
> >> Frieder
> >>
> >> [1]
> >> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> >
> > Hi Frieder,
> >
> > The patch does not resolve the issue. I still get the 'xfer timed out'
> > from samsung-dsim but noticing today that this issue doesn't exist in
> > linux-next I've found that its resolved by Marek's patch:
> > commit 8a4b2fc9c91a ("drm/bridge: tc358762: Split register programming
> > from pre-enable to enable")
>
> Thanks for testing. I didn't notice there already is a patch from Marek
> for the tc358762 driver. This is exactly the change that I was
> considering above as a fix on the downstream bridge side.
>
> >
> > I'm not clear on how that patch is staged in linux-next. If we can get
> > that pulled into 6.5 then it will resolve the breakage.
>
> Still the documentation says that the DSI host must be able to handle
> this and there might be other drivers that are not yet fixed like this
> or can't be changed to make DSI transfers only after the host's
> pre_enable() was called.
>
> Therefore I would prefer to fix the DSIM driver and apply this fix to
> 6.5-rc instead of backporting the tc358762 patch. I gave it another shot
> and maybe you could do one more test with the attached patch and without
> the fix in tc358762.
>

Frieder,

This patch doesn't resolve the issue either. Let me know if there is
some quick debugging you want to add somewhere. I don't have a lot of
time to troubleshoot this week as I'm trying to wrap up some work
before a 2-week vacation but it's quick for me to apply patches and do
a quick boot test.

best regards,

Tim

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Frieder Schrempf]

On 19.07.23 18:34, Tim Harvey wrote:
> On Wed, Jul 19, 2023 at 12:05 AM Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
>>
>> Hi Tim,
>>
>> On 19.07.23 01:03, Tim Harvey wrote:
>>> On Thu, Jul 13, 2023 at 3:01 AM Frieder Schrempf
>>> <frieder.schrempf@kontron.de> wrote:
>>>>
>>>> Hi Tim,
>>>>
>>>> On 13.07.23 09:18, Frieder Schrempf wrote:
>>>>> Hi Tim,
>>>>>
>>>>> On 13.07.23 00:34, Tim Harvey wrote:
>>>>>> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
>>>>>>>
>>>>>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>>>>>
>>>>>>> According to the documentation [1] the proper enable flow is:
>>>>>>>
>>>>>>> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
>>>>>>> 2. Disable stop state to bring data lanes into HS mode
>>>>>>>
>>>>>>> Currently we do this all at once within enable(), which doesn't
>>>>>>> allow to meet the requirements of some downstream bridges.
>>>>>>>
>>>>>>> To fix this we now enable the DSI in pre_enable() and force it
>>>>>>> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
>>>>>>> register until enable() is called where we reset the bit.
>>>>>>>
>>>>>>> We currently do this only for i.MX8M as Exynos uses a different
>>>>>>> init flow where samsung_dsim_init() is called from
>>>>>>> samsung_dsim_host_transfer().
>>>>>>>
>>>>>>> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>>>>>>>
>>>>>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>>>>> ---
>>>>>>> Changes for v2:
>>>>>>> * Drop RFC
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
>>>>>>>  1 file changed, 23 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> index e0a402a85787..9775779721d9 100644
>>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
>>>>>>>         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>>>>>         reg &= ~DSIM_STOP_STATE_CNT_MASK;
>>>>>>>         reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
>>>>>>> +
>>>>>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
>>>>>>> +               reg |= DSIM_FORCE_STOP_STATE;
>>>>>>> +
>>>>>>>         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>>>>>
>>>>>>>         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
>>>>>>> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>>>>>>>                 ret = samsung_dsim_init(dsi);
>>>>>>>                 if (ret)
>>>>>>>                         return;
>>>>>>> +
>>>>>>> +               samsung_dsim_set_display_mode(dsi);
>>>>>>> +               samsung_dsim_set_display_enable(dsi, true);
>>>>>>>         }
>>>>>>>  }
>>>>>>>
>>>>>>> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>>>>>>>                                        struct drm_bridge_state *old_bridge_state)
>>>>>>>  {
>>>>>>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>>>>>>> +       u32 reg;
>>>>>>>
>>>>>>> -       samsung_dsim_set_display_mode(dsi);
>>>>>>> -       samsung_dsim_set_display_enable(dsi, true);
>>>>>>> +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>>>>>>> +               samsung_dsim_set_display_mode(dsi);
>>>>>>> +               samsung_dsim_set_display_enable(dsi, true);
>>>>>>> +       } else {
>>>>>>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>>>>> +               reg &= ~DSIM_FORCE_STOP_STATE;
>>>>>>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>>>>> +       }
>>>>>>>
>>>>>>>         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>>>>>>>  }
>>>>>>> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
>>>>>>>                                         struct drm_bridge_state *old_bridge_state)
>>>>>>>  {
>>>>>>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>>>>>>> +       u32 reg;
>>>>>>>
>>>>>>>         if (!(dsi->state & DSIM_STATE_ENABLED))
>>>>>>>                 return;
>>>>>>>
>>>>>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>>>>>>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>>>>> +               reg |= DSIM_FORCE_STOP_STATE;
>>>>>>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>>>>> +       }
>>>>>>> +
>>>>>>>         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>>>>>>>  }
>>>>>>>
>>>>>>> --
>>>>>>> 2.40.0
>>>>>>>
>>>>>>
>>>>>> Hi Frieder,
>>>>>>
>>>>>> I found this patch to break mipi-dsi display on my board which has:
>>>>>>  - FocalTech FT5406 10pt touch controller (with no interrupt)
>>>>>>  - Powertip PH800480T013-IDF02 compatible panel
>>>>>>  - Toshiba TC358762 compatible DSI to DBI bridge
>>>>>>  - ATTINY based regulator used for backlight controller and panel enable
>>>>>>
>>>>>> I enable this via a dt overlay in a pending patch
>>>>>> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
>>>>>> 6.5-rc1 which has this patch.
>>>>>>
>>>>>> The issue appears as:
>>>>>> [    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
>>>>>> 64 01 05 00 00 00
>>>>>> [    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)
>>>>>>
>>>>>> Instead of
>>>>>> [    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
>>>>>> using dummy regulator
>>>>>> [    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
>>>>>> using dummy regulator
>>>>>> [    5.649928] samsung-dsim 32e10000.dsi:
>>>>>> [drm:samsung_dsim_host_attach] Attached tc358762 device
>>>>>>
>>>>>> I'm curious what board/panel were you needing this for and do you have
>>>>>> any ideas why it broke my setup?
>>>>>>
>>>>>> I'm also curious what board/panel Alexander tested this with and if
>>>>>> Adam or Jagan (or others) have tested this with their hardware?
>>>>>
>>>>> Sorry for breaking your setup. My test- and use-case is the same as
>>>>> Alexander's. I have the SN65DSI84 downstream bridge and without this
>>>>> patch it fails to come up in some cases.
>>>>>
>>>>> The failure is probably related to your downstream bridge being
>>>>> controlled by the DSI itself using command mode. The SN65DSI84 is
>>>>> instead controlled via I2C.
>>>>>
>>>>> Actually we should have tested this with a bridge that uses command mode
>>>>> before merging, now that I think of it. But I wasn't really aware of
>>>>> this until now.
>>>>>
>>>>> I'll have a closer look at the issue and then will get back to you. In
>>>>> the meantime if anyone can help analyze the problem or has proposals how
>>>>> to fix it, please let us know.
>>>>
>>>> With my patch samsung_dsim_init() now initializes the DSIM to stop
>>>> state. When being called from samsung_dsim_atomic_pre_enable() this
>>>> works as the stop state is cleared later in samsung_dsim_atomic_enable().
>>>>
>>>> When being called from samsung_dsim_host_transfer() to handle transfers
>>>> before samsung_dsim_atomic_pre_enable() was called, the stop state is
>>>> never cleared and transfers will fail.
>>>>
>>>> This is the case in your setup as tc358762_init() is called in
>>>> tc358762_pre_enable().
>>>>
>>>> I think that requiring to initialize the DSI host during transfer could
>>>> be avoided in this case by moving tc358762_init() from
>>>> tc358762_pre_enable() to tc358762_enable().
>>>>
>>>> But at the same time according to the docs at [1] this seems to be a
>>>> valid case that we need to support in the DSIM driver:
>>>>
>>>>   Whilst it is valid to call host_transfer prior to pre_enable or
>>>>   after post_disable, the exact state of the lanes is undefined at
>>>>   this point. The DSI host should initialise the interface, transmit
>>>>   the data, and then disable the interface again.
>>>>
>>>> Therefore I would propose to try a fix like in the attachement. If you
>>>> could test this, that would be great.
>>>>
>>>> Thanks
>>>> Frieder
>>>>
>>>> [1]
>>>> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>>>
>>> Hi Frieder,
>>>
>>> The patch does not resolve the issue. I still get the 'xfer timed out'
>>> from samsung-dsim but noticing today that this issue doesn't exist in
>>> linux-next I've found that its resolved by Marek's patch:
>>> commit 8a4b2fc9c91a ("drm/bridge: tc358762: Split register programming
>>> from pre-enable to enable")
>>
>> Thanks for testing. I didn't notice there already is a patch from Marek
>> for the tc358762 driver. This is exactly the change that I was
>> considering above as a fix on the downstream bridge side.
>>
>>>
>>> I'm not clear on how that patch is staged in linux-next. If we can get
>>> that pulled into 6.5 then it will resolve the breakage.
>>
>> Still the documentation says that the DSI host must be able to handle
>> this and there might be other drivers that are not yet fixed like this
>> or can't be changed to make DSI transfers only after the host's
>> pre_enable() was called.
>>
>> Therefore I would prefer to fix the DSIM driver and apply this fix to
>> 6.5-rc instead of backporting the tc358762 patch. I gave it another shot
>> and maybe you could do one more test with the attached patch and without
>> the fix in tc358762.
>>
> 
> Frieder,
> 
> This patch doesn't resolve the issue either. Let me know if there is
> some quick debugging you want to add somewhere. I don't have a lot of
> time to troubleshoot this week as I'm trying to wrap up some work
> before a 2-week vacation but it's quick for me to apply patches and do
> a quick boot test.

Ok, thanks for testing anyway. I think I need to go back to the drawing
board with this as I'm obviously missing something. I won't bother you
more before I did some more research myself.

Re: [PATCH v2 1/2] drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec

[Frieder Schrempf]

On 19.07.23 18:34, Tim Harvey wrote:
> On Wed, Jul 19, 2023 at 12:05 AM Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
>>
>> Hi Tim,
>>
>> On 19.07.23 01:03, Tim Harvey wrote:
>>> On Thu, Jul 13, 2023 at 3:01 AM Frieder Schrempf
>>> <frieder.schrempf@kontron.de> wrote:
>>>>
>>>> Hi Tim,
>>>>
>>>> On 13.07.23 09:18, Frieder Schrempf wrote:
>>>>> Hi Tim,
>>>>>
>>>>> On 13.07.23 00:34, Tim Harvey wrote:
>>>>>> On Wed, May 3, 2023 at 9:33 AM Frieder Schrempf <frieder@fris.de> wrote:
>>>>>>>
>>>>>>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>>>>>
>>>>>>> According to the documentation [1] the proper enable flow is:
>>>>>>>
>>>>>>> 1. Enable DSI link and keep data lanes in LP-11 (stop state)
>>>>>>> 2. Disable stop state to bring data lanes into HS mode
>>>>>>>
>>>>>>> Currently we do this all at once within enable(), which doesn't
>>>>>>> allow to meet the requirements of some downstream bridges.
>>>>>>>
>>>>>>> To fix this we now enable the DSI in pre_enable() and force it
>>>>>>> into stop state using the FORCE_STOP_STATE bit in the ESCMODE
>>>>>>> register until enable() is called where we reset the bit.
>>>>>>>
>>>>>>> We currently do this only for i.MX8M as Exynos uses a different
>>>>>>> init flow where samsung_dsim_init() is called from
>>>>>>> samsung_dsim_host_transfer().
>>>>>>>
>>>>>>> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>>>>>>>
>>>>>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>>>>>>> ---
>>>>>>> Changes for v2:
>>>>>>> * Drop RFC
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++++++++--
>>>>>>>  1 file changed, 23 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> index e0a402a85787..9775779721d9 100644
>>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> @@ -859,6 +859,10 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
>>>>>>>         reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>>>>>         reg &= ~DSIM_STOP_STATE_CNT_MASK;
>>>>>>>         reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
>>>>>>> +
>>>>>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
>>>>>>> +               reg |= DSIM_FORCE_STOP_STATE;
>>>>>>> +
>>>>>>>         samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>>>>>
>>>>>>>         reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
>>>>>>> @@ -1340,6 +1344,9 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>>>>>>>                 ret = samsung_dsim_init(dsi);
>>>>>>>                 if (ret)
>>>>>>>                         return;
>>>>>>> +
>>>>>>> +               samsung_dsim_set_display_mode(dsi);
>>>>>>> +               samsung_dsim_set_display_enable(dsi, true);
>>>>>>>         }
>>>>>>>  }
>>>>>>>
>>>>>>> @@ -1347,9 +1354,16 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>>>>>>>                                        struct drm_bridge_state *old_bridge_state)
>>>>>>>  {
>>>>>>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>>>>>>> +       u32 reg;
>>>>>>>
>>>>>>> -       samsung_dsim_set_display_mode(dsi);
>>>>>>> -       samsung_dsim_set_display_enable(dsi, true);
>>>>>>> +       if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>>>>>>> +               samsung_dsim_set_display_mode(dsi);
>>>>>>> +               samsung_dsim_set_display_enable(dsi, true);
>>>>>>> +       } else {
>>>>>>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>>>>> +               reg &= ~DSIM_FORCE_STOP_STATE;
>>>>>>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>>>>> +       }
>>>>>>>
>>>>>>>         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>>>>>>>  }
>>>>>>> @@ -1358,10 +1372,17 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
>>>>>>>                                         struct drm_bridge_state *old_bridge_state)
>>>>>>>  {
>>>>>>>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>>>>>>> +       u32 reg;
>>>>>>>
>>>>>>>         if (!(dsi->state & DSIM_STATE_ENABLED))
>>>>>>>                 return;
>>>>>>>
>>>>>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>>>>>>> +               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>>>>>>> +               reg |= DSIM_FORCE_STOP_STATE;
>>>>>>> +               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
>>>>>>> +       }
>>>>>>> +
>>>>>>>         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>>>>>>>  }
>>>>>>>
>>>>>>> --
>>>>>>> 2.40.0
>>>>>>>
>>>>>>
>>>>>> Hi Frieder,
>>>>>>
>>>>>> I found this patch to break mipi-dsi display on my board which has:
>>>>>>  - FocalTech FT5406 10pt touch controller (with no interrupt)
>>>>>>  - Powertip PH800480T013-IDF02 compatible panel
>>>>>>  - Toshiba TC358762 compatible DSI to DBI bridge
>>>>>>  - ATTINY based regulator used for backlight controller and panel enable
>>>>>>
>>>>>> I enable this via a dt overlay in a pending patch
>>>>>> imx8mm-venice-gw72xx-0x-rpidsi.dtso [1] which works on 6.4 but not
>>>>>> 6.5-rc1 which has this patch.
>>>>>>
>>>>>> The issue appears as:
>>>>>> [    6.110585] samsung-dsim 32e60000.dsi: xfer timed out: 29 06 00 00
>>>>>> 64 01 05 00 00 00
>>>>>> [    6.326588] tc358762 32e60000.dsi.0: error initializing bridge (-110)
>>>>>>
>>>>>> Instead of
>>>>>> [    1.011729] samsung-dsim 32e10000.dsi: supply vddcore not found,
>>>>>> using dummy regulator
>>>>>> [    1.019829] samsung-dsim 32e10000.dsi: supply vddio not found,
>>>>>> using dummy regulator
>>>>>> [    5.649928] samsung-dsim 32e10000.dsi:
>>>>>> [drm:samsung_dsim_host_attach] Attached tc358762 device
>>>>>>
>>>>>> I'm curious what board/panel were you needing this for and do you have
>>>>>> any ideas why it broke my setup?
>>>>>>
>>>>>> I'm also curious what board/panel Alexander tested this with and if
>>>>>> Adam or Jagan (or others) have tested this with their hardware?
>>>>>
>>>>> Sorry for breaking your setup. My test- and use-case is the same as
>>>>> Alexander's. I have the SN65DSI84 downstream bridge and without this
>>>>> patch it fails to come up in some cases.
>>>>>
>>>>> The failure is probably related to your downstream bridge being
>>>>> controlled by the DSI itself using command mode. The SN65DSI84 is
>>>>> instead controlled via I2C.
>>>>>
>>>>> Actually we should have tested this with a bridge that uses command mode
>>>>> before merging, now that I think of it. But I wasn't really aware of
>>>>> this until now.
>>>>>
>>>>> I'll have a closer look at the issue and then will get back to you. In
>>>>> the meantime if anyone can help analyze the problem or has proposals how
>>>>> to fix it, please let us know.
>>>>
>>>> With my patch samsung_dsim_init() now initializes the DSIM to stop
>>>> state. When being called from samsung_dsim_atomic_pre_enable() this
>>>> works as the stop state is cleared later in samsung_dsim_atomic_enable().
>>>>
>>>> When being called from samsung_dsim_host_transfer() to handle transfers
>>>> before samsung_dsim_atomic_pre_enable() was called, the stop state is
>>>> never cleared and transfers will fail.
>>>>
>>>> This is the case in your setup as tc358762_init() is called in
>>>> tc358762_pre_enable().
>>>>
>>>> I think that requiring to initialize the DSI host during transfer could
>>>> be avoided in this case by moving tc358762_init() from
>>>> tc358762_pre_enable() to tc358762_enable().
>>>>
>>>> But at the same time according to the docs at [1] this seems to be a
>>>> valid case that we need to support in the DSIM driver:
>>>>
>>>>   Whilst it is valid to call host_transfer prior to pre_enable or
>>>>   after post_disable, the exact state of the lanes is undefined at
>>>>   this point. The DSI host should initialise the interface, transmit
>>>>   the data, and then disable the interface again.
>>>>
>>>> Therefore I would propose to try a fix like in the attachement. If you
>>>> could test this, that would be great.
>>>>
>>>> Thanks
>>>> Frieder
>>>>
>>>> [1]
>>>> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>>>
>>> Hi Frieder,
>>>
>>> The patch does not resolve the issue. I still get the 'xfer timed out'
>>> from samsung-dsim but noticing today that this issue doesn't exist in
>>> linux-next I've found that its resolved by Marek's patch:
>>> commit 8a4b2fc9c91a ("drm/bridge: tc358762: Split register programming
>>> from pre-enable to enable")
>>
>> Thanks for testing. I didn't notice there already is a patch from Marek
>> for the tc358762 driver. This is exactly the change that I was
>> considering above as a fix on the downstream bridge side.
>>
>>>
>>> I'm not clear on how that patch is staged in linux-next. If we can get
>>> that pulled into 6.5 then it will resolve the breakage.
>>
>> Still the documentation says that the DSI host must be able to handle
>> this and there might be other drivers that are not yet fixed like this
>> or can't be changed to make DSI transfers only after the host's
>> pre_enable() was called.
>>
>> Therefore I would prefer to fix the DSIM driver and apply this fix to
>> 6.5-rc instead of backporting the tc358762 patch. I gave it another shot
>> and maybe you could do one more test with the attached patch and without
>> the fix in tc358762.
>>
> 
> Frieder,
> 
> This patch doesn't resolve the issue either. Let me know if there is
> some quick debugging you want to add somewhere. I don't have a lot of
> time to troubleshoot this week as I'm trying to wrap up some work
> before a 2-week vacation but it's quick for me to apply patches and do
> a quick boot test.

Ok, thanks for testing anyway. I think I need to go back to the drawing
board with this as I'm obviously missing something. I won't bother you
more before I did some more research myself.