Re: [PATCH v8 06/14] drm: bridge: samsung-dsim: Handle proper DSI host initialization

[Marek Szyprowski <m.szyprowski@samsung.com>]

On 17.11.2022 05:58, Marek Vasut wrote:
> On 11/10/22 19:38, Jagan Teki wrote:
>> DSI host initialization handling in previous exynos dsi driver has
>> some pitfalls. It initializes the host during host transfer() hook
>> that is indeed not the desired call flow for I2C and any other DSI
>> configured downstream bridges.
>>
>> Host transfer() is usually triggered for downstream DSI panels or
>> bridges and I2C-configured-DSI bridges miss these host initialization
>> as these downstream bridges use bridge operations hooks like pre_enable,
>> and enable in order to initialize or set up the host.
>>
>> This patch is trying to handle the host init handler to satisfy all
>> downstream panels and bridges. Added the DSIM_STATE_REINITIALIZED state
>> flag to ensure that host init is also done on first cmd transfer, this
>> helps existing DSI panels work on exynos platform (form Marek
>> Szyprowski).
>>
>> v8, v7, v6, v5:
>> * none
>>
>> v4:
>> * update init handling to ensure host init done on first cmd transfer
>>
>> v3:
>> * none
>>
>> v2:
>> * check initialized state in samsung_dsim_init
>>
>> v1:
>> * keep DSI init in host transfer
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>> ---
>>   drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++--------
>>   include/drm/bridge/samsung-dsim.h     |  5 +++--
>>   2 files changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>> index bb1f45fd5a88..ec7e01ae02ea 100644
>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>> @@ -1234,12 +1234,17 @@ 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, unsigned int 
>> flag)
>>   {
>>       const struct samsung_dsim_driver_data *driver_data = 
>> dsi->driver_data;
>>   +    if (dsi->state & flag)
>> +        return 0;
>> +
>>       samsung_dsim_reset(dsi);
>> -    samsung_dsim_enable_irq(dsi);
>> +
>> +    if (!(dsi->state & DSIM_STATE_INITIALIZED))
>> +        samsung_dsim_enable_irq(dsi);
>>         if (driver_data->reg_values[RESET_TYPE] == DSIM_FUNCRST)
>>           samsung_dsim_enable_lane(dsi, BIT(dsi->lanes) - 1);
>> @@ -1250,6 +1255,8 @@ static int samsung_dsim_init(struct 
>> samsung_dsim *dsi)
>>       samsung_dsim_set_phy_ctrl(dsi);
>>       samsung_dsim_init_link(dsi);
>>   +    dsi->state |= flag;
>> +
>>       return 0;
>>   }
>>   @@ -1269,6 +1276,10 @@ static void 
>> samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>>       }
>>         dsi->state |= DSIM_STATE_ENABLED;
>> +
>> +    ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED);
>> +    if (ret)
>> +        return;
>>   }
>>     static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>> @@ -1458,12 +1469,9 @@ static ssize_t 
>> samsung_dsim_host_transfer(struct mipi_dsi_host *host,
>>       if (!(dsi->state & DSIM_STATE_ENABLED))
>>           return -EINVAL;
>>   -    if (!(dsi->state & DSIM_STATE_INITIALIZED)) {
>> -        ret = samsung_dsim_init(dsi);
>> -        if (ret)
>> -            return ret;
>> -        dsi->state |= DSIM_STATE_INITIALIZED;
>> -    }
>> +    ret = samsung_dsim_init(dsi, DSIM_STATE_REINITIALIZED);
>
> This triggers full controller reset and reprogramming upon first 
> command transfer, is such heavy handed reload really necessary ?

Yes it is, otherwise the proper DSI panels doesn't work with Exynos DRM 
DSI. If this is a real issue for you, then maybe the driver could do the 
initialization conditionally, in prepare() callback in case of IMX and 
on the first transfer in case of Exynos?


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland