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

[Marek Vasut]

On 12/2/22 11:52, Marek Szyprowski wrote:
> Hi,
> 
> Sorry for delay, I was on a sick leave last 2 weeks.
> 
> On 28.11.2022 15:43, Jagan Teki wrote:
>> ,On Sat, Nov 26, 2022 at 3:44 AM Marek Vasut <marex at denx.de> wrote:
>>> 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 ?
>> If I'm correct this sequence is required in order to work the existing
>> panel/bridges on exynos. Adjusting these panel/bridge codes can
>> possibly fix the sequence further.
>>
>> Marek Szyprowski, please add if you have anything.
> 
> 
> Well, frankly speaking the double initialization is not a correct
> sequence, but this is the only one that actually works on Exynos based
> boards with DSI panels after moving the initialization to bridge's
> .prepare() callback.

Somehow, I suspect this is related to the LP11 mode handling, which 
differs for different panels, right ? I think the RPi people worked on 
fixing that.

+CC Dave

> I've already explained this and shared the results
> of my investigation in my replies to the previous versions of this
> patchset. The original Exynos DSI driver does the initialization on the
> first DSI command. This however doesn't work for Jagan with I2C
> controlled panels/bridges, so he moved the initialization to the
> .prepare() callback, what broke the Exynos case (in-short - all tested
> panels works fine only if the DSI host initialization is done AFTER
> turning the panel's power on). For more information, see this thread:
> https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5f5a@samsung.com/
> 
> Now, the more I think of it, the more I'm convinced that we simply
> should add a hack based on the HW type: do the initialization in
> .prepare() for non-Exynos case and before the first transfer for the
> Exynos case, as there is no way to detect the panel/next bridge type
> (I2C or DSI controlled).

Let's see what Dave has to say about this, maybe there is some further help.