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

[Jagan Teki <jagan@amarulasolutions.com>]

,On Sat, Nov 26, 2022 at 3:44 AM Marek Vasut <marex@denx.de> wrote:
>
> On 11/23/22 21:09, Jagan Teki wrote:
> > On Sat, Nov 19, 2022 at 7:45 PM Marek Vasut <marex@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@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?
> >>
> >> 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.

Jagan.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel