Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Alexander Stein]

Hi Michael,

Am Montag, 13. November 2023, 17:43:44 CET schrieb Michael Walle:
> The FORCE_STOP_STATE bit is unsuitable to force the DSI link into LP-11
> mode. It seems the bridge internally queues DSI packets and when the
> FORCE_STOP_STATE bit is cleared, they are sent in close succession
> without any useful timing (this also means that the DSI lanes won't go
> into LP-11 mode). The length of this gibberish varies between 1ms and
> 5ms. This sometimes breaks an attached bridge (TI SN65DSI84 in this
> case). In our case, the bridge will fail in about 1 per 500 reboots.
> 
> The FORCE_STOP_STATE handling was introduced to have the DSI lanes in
> LP-11 state during the .pre_enable phase. But as it turns out, none of
> this is needed at all. Between samsung_dsim_init() and
> samsung_dsim_set_display_enable() the lanes are already in LP-11 mode.

Apparently LP-11 is actually entered with the call to 
samsung_dsim_enable_lane(), but I don't know about other requisites on that 
matter. Unfortunately documentation lacks a lot in that regard.

> The code as it was before commit 20c827683de0 ("drm: bridge:
> samsung-dsim: Fix init during host transfer") and 0c14d3130654 ("drm:
> bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec") was correct
> in this regard.
> 
> This patch basically reverts both commits. It was tested on an i.MX8M
> SoC with an SN65DSI84 bridge. The signals were probed and the DSI
> packets were decoded during initialization and link start-up. After this
> patch the first DSI packet on the link is a VSYNC packet and the timing
> is correct.

At which point does SN65DSI84 require LP-11?
You have access to a DSI/D-PHY analyzer?

> Command mode between .pre_enable and .enable was also briefly tested by
> a quick hack. There was no DSI link partner which would have responded,
> but it was made sure the DSI packet was send on the link. As a side
> note, the command mode seems to just work in HS mode. I couldn't find
> that the bridge will handle commands in LP mode.

AFAIK ti-sn65dsi83.c only uses I2C for communication. Did you send DSI read/
writes instead?

best regards,
Alexander

> Fixes: 20c827683de0 ("drm: bridge: samsung-dsim: Fix init during host
> transfer") Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M
> enable flow to meet spec") Signed-off-by: Michael Walle <mwalle@xxxxxxxxxx>
> ---
> Let me know wether this should be two commits each reverting one, but both
> commits appeared first in kernel 6.5.
> 
>  drivers/gpu/drm/bridge/samsung-dsim.c | 32 ++-------------------------
>  1 file changed, 2 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c index cf777bdb25d2..4233a50baac7
> 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -939,10 +939,6 @@ 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);
> @@ -1387,18 +1383,6 @@ static void samsung_dsim_disable_irq(struct
> samsung_dsim *dsi) disable_irq(dsi->irq);
>  }
> 
> -static void samsung_dsim_set_stop_state(struct samsung_dsim *dsi, bool
> enable) -{
> -	u32 reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> -
> -	if (enable)
> -		reg |= DSIM_FORCE_STOP_STATE;
> -	else
> -		reg &= ~DSIM_FORCE_STOP_STATE;
> -
> -	samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> -}
> -
>  static int samsung_dsim_init(struct samsung_dsim *dsi)
>  {
>  	const struct samsung_dsim_driver_data *driver_data = dsi-
>driver_data;
> @@ -1448,9 +1432,6 @@ 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);
>  	}
>  }
> 
> @@ -1459,12 +1440,8 @@ static void samsung_dsim_atomic_enable(struct
> drm_bridge *bridge, {
>  	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> 
> -	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 {
> -		samsung_dsim_set_stop_state(dsi, false);
> -	}
> +	samsung_dsim_set_display_mode(dsi);
> +	samsung_dsim_set_display_enable(dsi, true);
> 
>  	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>  }
> @@ -1477,9 +1454,6 @@ static void samsung_dsim_atomic_disable(struct
> drm_bridge *bridge, if (!(dsi->state & DSIM_STATE_ENABLED))
>  		return;
> 
> -	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> -		samsung_dsim_set_stop_state(dsi, true);
> -
>  	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>  }
> 
> @@ -1781,8 +1755,6 @@ static ssize_t samsung_dsim_host_transfer(struct
> mipi_dsi_host *host, if (ret)
>  		return ret;
> 
> -	samsung_dsim_set_stop_state(dsi, false);
> -
>  	ret = mipi_dsi_create_packet(&xfer.packet, msg);
>  	if (ret < 0)
>  		return ret;


-- 
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/