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@xxxxxxx> wrote:
>>>
>>> From: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> ---
>>> 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-operationFrom 770e4763255ba5bf086ae7b014330651e007bcb7 Mon Sep 17 00:00:00 2001
From: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
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@xxxxxxxxxxxxx>
Signed-off-by: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
---
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
