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

[Marek Vasut]

On 11/23/22 21:09, Jagan Teki wrote:
> On Sat, Nov 19, 2022 at 7:45 PM Marek Vasut <marex at denx.de> wrote:
>>
>> 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 ?
> 
> Is it breaking samsung_dsim_reset from host_transfer? maybe checking
> whether a reset is required before calling it might fix the issue.  I
> agree with double initialization is odd but it seems it is required on
> some panels in Exynos, I think tweaking them and adjusting the panel
> code might resolve this discrepancy.

Can someone provide further details on the exynos problem ?