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

[Marek Vasut]

On 11/17/22 14:04, Marek Szyprowski wrote:
> 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 at samsung.com>
>>> Signed-off-by: Jagan Teki <jagan at 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?

That's odd , it does actually break panel support for me, without this 
double reset the panel works again. But I have to wonder, why would such 
a full reset be necessary at all , even on the exynos ?