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/
