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

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(-)
  

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 ?
  

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
  

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 ?
  

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.

Jagan.
  

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 ?
  

,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.
  

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@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.


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. 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).


Best regards
  

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@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.
> 
> 
> 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.
  

Hi Marek

On Fri, 2 Dec 2022 at 12:21, Marek Vasut <marex@denx.de> wrote:
>
> 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@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.
> >
> >
> > 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

Yes. I've just sent out a v3 of that patch set.

Hopefully setting the pre_enable_prev_first flag on your peripheral's
drm_bridge, or prepare_prev_first if a drm_panel, will result in a
more sensible initialisation order for your panel.

Note that host_transfer should ensure that the host is initialised, as
it is valid to call it with the host in any state. If it has to
initialise, then it should deinitialise once the transfer has
completed.

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.
  

On 02.12.22 15:55, Dave Stevenson wrote:
> Hi Marek
> 
> On Fri, 2 Dec 2022 at 12:21, Marek Vasut <marex@denx.de> wrote:
>>
>> 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@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.
>>>
>>>
>>> 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
> 
> Yes. I've just sent out a v3 of that patch set.
> 
> Hopefully setting the pre_enable_prev_first flag on your peripheral's
> drm_bridge, or prepare_prev_first if a drm_panel, will result in a
> more sensible initialisation order for your panel.
> 
> Note that host_transfer should ensure that the host is initialised, as
> it is valid to call it with the host in any state. If it has to
> initialise, then it should deinitialise once the transfer has
> completed.
> 
> 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://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2Fe96197f9-948a-997e-5453-9f9d179b5f5a%40samsung.com%2F&amp;data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Cc770caab3f274d9b50d108dad4753e1f%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C638055897263056680%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=1YV551YUhXQAjE4Cg0nAtMdMksWzMtscH49O83iKHRo%3D&amp;reserved=0
>>>
>>> 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.

Could we agree on adding the HW type based hack Marek S. proposed as a
quick fix?

This patchset was tested on Exynos so it's likely to not break any
existing setups. And for i.MX8, this is a new driver so there's not
really a requirement to have all setups working/supported from the
beginning.

Also having one or two hacks (marked with FIXME) in the code doesn't
hurt. As we can see there are drafts to fix them in conjunction with
changes in the DRM framework.

This has been pending for months and in my opinion if there's a chance
to get this into v6.2-rc1 we should take it.

If everyone agrees, Jagan, can you post a v9 which does the host
initialization in .prepare() for i.MX and on first transfer for Exynos?
  

Hi Frieder

On Mon, 5 Dec 2022 at 07:30, Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> On 02.12.22 15:55, Dave Stevenson wrote:
> > Hi Marek
> >
> > On Fri, 2 Dec 2022 at 12:21, Marek Vasut <marex@denx.de> wrote:
> >>
> >> 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@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.
> >>>
> >>>
> >>> 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
> >
> > Yes. I've just sent out a v3 of that patch set.
> >
> > Hopefully setting the pre_enable_prev_first flag on your peripheral's
> > drm_bridge, or prepare_prev_first if a drm_panel, will result in a
> > more sensible initialisation order for your panel.
> >
> > Note that host_transfer should ensure that the host is initialised, as
> > it is valid to call it with the host in any state. If it has to
> > initialise, then it should deinitialise once the transfer has
> > completed.
> >
> > 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://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2Fe96197f9-948a-997e-5453-9f9d179b5f5a%40samsung.com%2F&amp;data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Cc770caab3f274d9b50d108dad4753e1f%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C638055897263056680%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=1YV551YUhXQAjE4Cg0nAtMdMksWzMtscH49O83iKHRo%3D&amp;reserved=0
> >>>
> >>> 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.
>
> Could we agree on adding the HW type based hack Marek S. proposed as a
> quick fix?
>
> This patchset was tested on Exynos so it's likely to not break any
> existing setups. And for i.MX8, this is a new driver so there's not
> really a requirement to have all setups working/supported from the
> beginning.
>
> Also having one or two hacks (marked with FIXME) in the code doesn't
> hurt. As we can see there are drafts to fix them in conjunction with
> changes in the DRM framework.
>
> This has been pending for months and in my opinion if there's a chance
> to get this into v6.2-rc1 we should take it.

My patchset was sent in March with no one seeming to care enough to review it.

If the situation is that your devices fall into the same camp as those
for vc4 (the host needs to be initialised before the peripheral), at
least verifying that would be useful before rushing into a hack.

Your other comment references using a TI SN65DSI84. I know for certain
that falls into the category of needing the DSI bus initialised before
it is brought out of reset.

  Dave

> If everyone agrees, Jagan, can you post a v9 which does the host
> initialization in .prepare() for i.MX and on first transfer for Exynos?
>
  

Hi Dave,

On 05.12.22 16:20, Dave Stevenson wrote:
> Hi Frieder
> 
> On Mon, 5 Dec 2022 at 07:30, Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
>>
>> On 02.12.22 15:55, Dave Stevenson wrote:
>>> Hi Marek
>>>
>>> On Fri, 2 Dec 2022 at 12:21, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> 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@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.
>>>>>
>>>>>
>>>>> 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
>>>
>>> Yes. I've just sent out a v3 of that patch set.
>>>
>>> Hopefully setting the pre_enable_prev_first flag on your peripheral's
>>> drm_bridge, or prepare_prev_first if a drm_panel, will result in a
>>> more sensible initialisation order for your panel.
>>>
>>> Note that host_transfer should ensure that the host is initialised, as
>>> it is valid to call it with the host in any state. If it has to
>>> initialise, then it should deinitialise once the transfer has
>>> completed.
>>>
>>> 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://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2Fe96197f9-948a-997e-5453-9f9d179b5f5a%40samsung.com%2F&amp;data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Cee7b57ee420e45a73b1d08dad6d45306%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C638058504671330145%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=TQIIKNa4OVGP1dZo3tM%2FOMO3dlXrjLr04U%2FJFhd2rAs%3D&amp;reserved=0
>>>>>
>>>>> 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.
>>
>> Could we agree on adding the HW type based hack Marek S. proposed as a
>> quick fix?
>>
>> This patchset was tested on Exynos so it's likely to not break any
>> existing setups. And for i.MX8, this is a new driver so there's not
>> really a requirement to have all setups working/supported from the
>> beginning.
>>
>> Also having one or two hacks (marked with FIXME) in the code doesn't
>> hurt. As we can see there are drafts to fix them in conjunction with
>> changes in the DRM framework.
>>
>> This has been pending for months and in my opinion if there's a chance
>> to get this into v6.2-rc1 we should take it.
> 
> My patchset was sent in March with no one seeming to care enough to review it.

I wasn't referring to your patchset but rather to the Samsung DSIM
bridge transformation patchset.

My point was simply to not try getting everything done in one big step
because this will fail. The patchset this refers to needs testing on two
separate platforms which is painful enough (thanks to Marek for covering
the Exynos side!). I think we should focus on getting the DSIM bridge
transformation merged and accept a few small hacks that will be taken
care of in the next step.

> 
> If the situation is that your devices fall into the same camp as those
> for vc4 (the host needs to be initialised before the peripheral), at
> least verifying that would be useful before rushing into a hack.
> 
> Your other comment references using a TI SN65DSI84. I know for certain
> that falls into the category of needing the DSI bus initialised before
> it is brought out of reset.

I'm actually working on this right now and when I received your message
I was about to start typing a reply to your patchset.

The SN65DSI84 works with the i.MX8MM DSIM even using the default order
of host init after peripheral init in our setup, but this configuration
doesn't seem to be stable and occasionally the bridge doesn't come up
properly.

We are still in the process of verifying if the reversed order fixes
this reliably. But regardless of the results, without the reversal the
initialization sequence is way out of spec and we need to fix this in
any case.

See here for my testing branch including some follow-up patches that
improve the initialization flow for my setup:
https://git.kontron-electronics.de/sw/misc/linux/-/commits/v6.1-dsim-mx8mm.

Thanks
Frieder
  

On 05.12.22 16:37, Frieder Schrempf wrote:
> Hi Dave,
> 
> On 05.12.22 16:20, Dave Stevenson wrote:
>> Hi Frieder
>>
>> On Mon, 5 Dec 2022 at 07:30, Frieder Schrempf
>> <frieder.schrempf@kontron.de> wrote:
>>>
>>> On 02.12.22 15:55, Dave Stevenson wrote:
>>>> Hi Marek
>>>>
>>>> On Fri, 2 Dec 2022 at 12:21, Marek Vasut <marex@denx.de> wrote:
>>>>>
>>>>> 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@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.
>>>>>>
>>>>>>
>>>>>> 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
>>>>
>>>> Yes. I've just sent out a v3 of that patch set.
>>>>
>>>> Hopefully setting the pre_enable_prev_first flag on your peripheral's
>>>> drm_bridge, or prepare_prev_first if a drm_panel, will result in a
>>>> more sensible initialisation order for your panel.
>>>>
>>>> Note that host_transfer should ensure that the host is initialised, as
>>>> it is valid to call it with the host in any state. If it has to
>>>> initialise, then it should deinitialise once the transfer has
>>>> completed.
>>>>
>>>> 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://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2Fe96197f9-948a-997e-5453-9f9d179b5f5a%40samsung.com%2F&amp;data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Cee7b57ee420e45a73b1d08dad6d45306%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C638058504671330145%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=TQIIKNa4OVGP1dZo3tM%2FOMO3dlXrjLr04U%2FJFhd2rAs%3D&amp;reserved=0
>>>>>>
>>>>>> 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.
>>>
>>> Could we agree on adding the HW type based hack Marek S. proposed as a
>>> quick fix?
>>>
>>> This patchset was tested on Exynos so it's likely to not break any
>>> existing setups. And for i.MX8, this is a new driver so there's not
>>> really a requirement to have all setups working/supported from the
>>> beginning.
>>>
>>> Also having one or two hacks (marked with FIXME) in the code doesn't
>>> hurt. As we can see there are drafts to fix them in conjunction with
>>> changes in the DRM framework.
>>>
>>> This has been pending for months and in my opinion if there's a chance
>>> to get this into v6.2-rc1 we should take it.
>>
>> My patchset was sent in March with no one seeming to care enough to review it.
> 
> I wasn't referring to your patchset but rather to the Samsung DSIM
> bridge transformation patchset.
> 
> My point was simply to not try getting everything done in one big step
> because this will fail. The patchset this refers to needs testing on two
> separate platforms which is painful enough (thanks to Marek for covering
> the Exynos side!). I think we should focus on getting the DSIM bridge
> transformation merged and accept a few small hacks that will be taken
> care of in the next step.
> 
>>
>> If the situation is that your devices fall into the same camp as those
>> for vc4 (the host needs to be initialised before the peripheral), at
>> least verifying that would be useful before rushing into a hack.
>>
>> Your other comment references using a TI SN65DSI84. I know for certain
>> that falls into the category of needing the DSI bus initialised before
>> it is brought out of reset.
> 
> I'm actually working on this right now and when I received your message
> I was about to start typing a reply to your patchset.
> 
> The SN65DSI84 works with the i.MX8MM DSIM even using the default order
> of host init after peripheral init in our setup, but this configuration
> doesn't seem to be stable and occasionally the bridge doesn't come up
> properly.
> 
> We are still in the process of verifying if the reversed order fixes
> this reliably. But regardless of the results, without the reversal the
> initialization sequence is way out of spec and we need to fix this in
> any case.
> 
> See here for my testing branch including some follow-up patches that
> improve the initialization flow for my setup:
> https://git.kontron-electronics.de/sw/misc/linux/-/commits/v6.1-dsim-mx8mm.

To recap my thoughts on the two hacks for the DSIM bridge driver
discussed before:

(1) Passing null to previous bridge in samsung_dsim_attach()
(2) Always initialize host on first transfer (see this patch, 06/14)

My wild guess would be that both could be fixed up properly in the long
run by the following changes:

* Apply Dave's patchset [1]
* Set pre_enable_prev_first flag in the downstream bridge drivers and
  fix init flow if required ([2] for ti-sn65dsi83)
* Fix DSIM init to keep data lanes in LP11 until enable() is called [3]
* Only call init on transfer when not already initialized and deinit
  after transfer (tbd)

As that route needs proper testing on the affected hardware setups and
includes changes to other drivers and the framework, I would suggest the
following for the v9 of this patchset:

* Keep hack (1)
* Make hack (2) dependent on the platform (Exynos)

As this is what Marek S. already suggested above, Jagan, can you send
the v9 with this change included?
Then Marek V. can test on his setup and if everything looks good we can
move on.

[1]
https://patchwork.kernel.org/project/dri-devel/cover/20221205173328.1395350-1-dave.stevenson@raspberrypi.com/
[2]
https://git.kontron-electronics.de/sw/misc/linux/-/commit/7769180e6bbdcb2e42c2b39cda5127efc21d3653
[3]
https://git.kontron-electronics.de/sw/misc/linux/-/commit/e6aba93de1189432dcaac17d969546a92541dc87
  

On Tue, Dec 6, 2022 at 2:32 PM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> On 05.12.22 16:37, Frieder Schrempf wrote:
> > Hi Dave,
> >
> > On 05.12.22 16:20, Dave Stevenson wrote:
> >> Hi Frieder
> >>
> >> On Mon, 5 Dec 2022 at 07:30, Frieder Schrempf
> >> <frieder.schrempf@kontron.de> wrote:
> >>>
> >>> On 02.12.22 15:55, Dave Stevenson wrote:
> >>>> Hi Marek
> >>>>
> >>>> On Fri, 2 Dec 2022 at 12:21, Marek Vasut <marex@denx.de> wrote:
> >>>>>
> >>>>> 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@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.
> >>>>>>
> >>>>>>
> >>>>>> 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
> >>>>
> >>>> Yes. I've just sent out a v3 of that patch set.
> >>>>
> >>>> Hopefully setting the pre_enable_prev_first flag on your peripheral's
> >>>> drm_bridge, or prepare_prev_first if a drm_panel, will result in a
> >>>> more sensible initialisation order for your panel.
> >>>>
> >>>> Note that host_transfer should ensure that the host is initialised, as
> >>>> it is valid to call it with the host in any state. If it has to
> >>>> initialise, then it should deinitialise once the transfer has
> >>>> completed.
> >>>>
> >>>> 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://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2Fe96197f9-948a-997e-5453-9f9d179b5f5a%40samsung.com%2F&amp;data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Cee7b57ee420e45a73b1d08dad6d45306%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C638058504671330145%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=TQIIKNa4OVGP1dZo3tM%2FOMO3dlXrjLr04U%2FJFhd2rAs%3D&amp;reserved=0
> >>>>>>
> >>>>>> 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.
> >>>
> >>> Could we agree on adding the HW type based hack Marek S. proposed as a
> >>> quick fix?
> >>>
> >>> This patchset was tested on Exynos so it's likely to not break any
> >>> existing setups. And for i.MX8, this is a new driver so there's not
> >>> really a requirement to have all setups working/supported from the
> >>> beginning.
> >>>
> >>> Also having one or two hacks (marked with FIXME) in the code doesn't
> >>> hurt. As we can see there are drafts to fix them in conjunction with
> >>> changes in the DRM framework.
> >>>
> >>> This has been pending for months and in my opinion if there's a chance
> >>> to get this into v6.2-rc1 we should take it.
> >>
> >> My patchset was sent in March with no one seeming to care enough to review it.
> >
> > I wasn't referring to your patchset but rather to the Samsung DSIM
> > bridge transformation patchset.
> >
> > My point was simply to not try getting everything done in one big step
> > because this will fail. The patchset this refers to needs testing on two
> > separate platforms which is painful enough (thanks to Marek for covering
> > the Exynos side!). I think we should focus on getting the DSIM bridge
> > transformation merged and accept a few small hacks that will be taken
> > care of in the next step.
> >
> >>
> >> If the situation is that your devices fall into the same camp as those
> >> for vc4 (the host needs to be initialised before the peripheral), at
> >> least verifying that would be useful before rushing into a hack.
> >>
> >> Your other comment references using a TI SN65DSI84. I know for certain
> >> that falls into the category of needing the DSI bus initialised before
> >> it is brought out of reset.
> >
> > I'm actually working on this right now and when I received your message
> > I was about to start typing a reply to your patchset.
> >
> > The SN65DSI84 works with the i.MX8MM DSIM even using the default order
> > of host init after peripheral init in our setup, but this configuration
> > doesn't seem to be stable and occasionally the bridge doesn't come up
> > properly.
> >
> > We are still in the process of verifying if the reversed order fixes
> > this reliably. But regardless of the results, without the reversal the
> > initialization sequence is way out of spec and we need to fix this in
> > any case.
> >
> > See here for my testing branch including some follow-up patches that
> > improve the initialization flow for my setup:
> > https://git.kontron-electronics.de/sw/misc/linux/-/commits/v6.1-dsim-mx8mm.
>
> To recap my thoughts on the two hacks for the DSIM bridge driver
> discussed before:
>
> (1) Passing null to previous bridge in samsung_dsim_attach()
> (2) Always initialize host on first transfer (see this patch, 06/14)
>
> My wild guess would be that both could be fixed up properly in the long
> run by the following changes:
>
> * Apply Dave's patchset [1]
> * Set pre_enable_prev_first flag in the downstream bridge drivers and
>   fix init flow if required ([2] for ti-sn65dsi83)
> * Fix DSIM init to keep data lanes in LP11 until enable() is called [3]
> * Only call init on transfer when not already initialized and deinit
>   after transfer (tbd)
>
> As that route needs proper testing on the affected hardware setups and
> includes changes to other drivers and the framework, I would suggest the
> following for the v9 of this patchset:

I did it on drm-misc-next [1].

>
> * Keep hack (1)

This has gone, not needed.

> * Make hack (2) dependent on the platform (Exynos)

I think with pre_enable_prev_first flag the Exynos pipeline will start
from bridge funcs like pre_enable so initializing the host will work
for exynos to work. [2]

Marek Sz. Can you confirm this?

[1] https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v9
[2] https://gitlab.com/openedev/kernel/-/commit/95ab71b797310952284fabfbc8476a9831902c5c

Jagan.
  

On 08.12.2022 12:32, Jagan Teki wrote:
> On Tue, Dec 6, 2022 at 2:32 PM Frieder Schrempf
> <frieder.schrempf@kontron.de> wrote:
>> On 05.12.22 16:37, Frieder Schrempf wrote:
>>> Hi Dave,
>>>
>>> On 05.12.22 16:20, Dave Stevenson wrote:
>>>> Hi Frieder
>>>>
>>>> On Mon, 5 Dec 2022 at 07:30, Frieder Schrempf
>>>> <frieder.schrempf@kontron.de> wrote:
>>>>> On 02.12.22 15:55, Dave Stevenson wrote:
>>>>>> Hi Marek
>>>>>>
>>>>>> On Fri, 2 Dec 2022 at 12:21, Marek Vasut <marex@denx.de> wrote:
>>>>>>> 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@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.
>>>>>>>>
>>>>>>>> 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
>>>>>> Yes. I've just sent out a v3 of that patch set.
>>>>>>
>>>>>> Hopefully setting the pre_enable_prev_first flag on your peripheral's
>>>>>> drm_bridge, or prepare_prev_first if a drm_panel, will result in a
>>>>>> more sensible initialisation order for your panel.
>>>>>>
>>>>>> Note that host_transfer should ensure that the host is initialised, as
>>>>>> it is valid to call it with the host in any state. If it has to
>>>>>> initialise, then it should deinitialise once the transfer has
>>>>>> completed.
>>>>>>
>>>>>> 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://protect2.fireeye.com/v1/url?k=b96d9a6f-d8e68f59-b96c1120-74fe485fffe0-6f8ca12c5049bf4e&q=1&e=b9def28e-897a-4edd-bfe3-86e02b64eb27&u=https%3A%2F%2Feur04.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Flore.kernel.org%252Fall%252Fe96197f9-948a-997e-5453-9f9d179b5f5a%2540samsung.com%252F%26amp%3Bdata%3D05%257C01%257Cfrieder.schrempf%2540kontron.de%257Cee7b57ee420e45a73b1d08dad6d45306%257C8c9d3c973fd941c8a2b1646f3942daf1%257C0%257C0%257C638058504671330145%257CUnknown%257CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%253D%257C3000%257C%257C%257C%26amp%3Bsdata%3DTQIIKNa4OVGP1dZo3tM%252FOMO3dlXrjLr04U%252FJFhd2rAs%253D%26amp%3Breserved%3D0
>>>>>>>>
>>>>>>>> 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.
>>>>> Could we agree on adding the HW type based hack Marek S. proposed as a
>>>>> quick fix?
>>>>>
>>>>> This patchset was tested on Exynos so it's likely to not break any
>>>>> existing setups. And for i.MX8, this is a new driver so there's not
>>>>> really a requirement to have all setups working/supported from the
>>>>> beginning.
>>>>>
>>>>> Also having one or two hacks (marked with FIXME) in the code doesn't
>>>>> hurt. As we can see there are drafts to fix them in conjunction with
>>>>> changes in the DRM framework.
>>>>>
>>>>> This has been pending for months and in my opinion if there's a chance
>>>>> to get this into v6.2-rc1 we should take it.
>>>> My patchset was sent in March with no one seeming to care enough to review it.
>>> I wasn't referring to your patchset but rather to the Samsung DSIM
>>> bridge transformation patchset.
>>>
>>> My point was simply to not try getting everything done in one big step
>>> because this will fail. The patchset this refers to needs testing on two
>>> separate platforms which is painful enough (thanks to Marek for covering
>>> the Exynos side!). I think we should focus on getting the DSIM bridge
>>> transformation merged and accept a few small hacks that will be taken
>>> care of in the next step.
>>>
>>>> If the situation is that your devices fall into the same camp as those
>>>> for vc4 (the host needs to be initialised before the peripheral), at
>>>> least verifying that would be useful before rushing into a hack.
>>>>
>>>> Your other comment references using a TI SN65DSI84. I know for certain
>>>> that falls into the category of needing the DSI bus initialised before
>>>> it is brought out of reset.
>>> I'm actually working on this right now and when I received your message
>>> I was about to start typing a reply to your patchset.
>>>
>>> The SN65DSI84 works with the i.MX8MM DSIM even using the default order
>>> of host init after peripheral init in our setup, but this configuration
>>> doesn't seem to be stable and occasionally the bridge doesn't come up
>>> properly.
>>>
>>> We are still in the process of verifying if the reversed order fixes
>>> this reliably. But regardless of the results, without the reversal the
>>> initialization sequence is way out of spec and we need to fix this in
>>> any case.
>>>
>>> See here for my testing branch including some follow-up patches that
>>> improve the initialization flow for my setup:
>>> https://protect2.fireeye.com/v1/url?k=0a67acf1-6becb9c7-0a6627be-74fe485fffe0-38ee812d7e32f480&q=1&e=b9def28e-897a-4edd-bfe3-86e02b64eb27&u=https%3A%2F%2Fgit.kontron-electronics.de%2Fsw%2Fmisc%2Flinux%2F-%2Fcommits%2Fv6.1-dsim-mx8mm.
>> To recap my thoughts on the two hacks for the DSIM bridge driver
>> discussed before:
>>
>> (1) Passing null to previous bridge in samsung_dsim_attach()
>> (2) Always initialize host on first transfer (see this patch, 06/14)
>>
>> My wild guess would be that both could be fixed up properly in the long
>> run by the following changes:
>>
>> * Apply Dave's patchset [1]
>> * Set pre_enable_prev_first flag in the downstream bridge drivers and
>>    fix init flow if required ([2] for ti-sn65dsi83)
>> * Fix DSIM init to keep data lanes in LP11 until enable() is called [3]
>> * Only call init on transfer when not already initialized and deinit
>>    after transfer (tbd)
>>
>> As that route needs proper testing on the affected hardware setups and
>> includes changes to other drivers and the framework, I would suggest the
>> following for the v9 of this patchset:
> I did it on drm-misc-next [1].
>
>> * Keep hack (1)
> This has gone, not needed.
>
>> * Make hack (2) dependent on the platform (Exynos)
> I think with pre_enable_prev_first flag the Exynos pipeline will start
> from bridge funcs like pre_enable so initializing the host will work
> for exynos to work. [2]
>
> Marek Sz. Can you confirm this?

Host initialization is orthogonal to the hack used on the bridge list. 
Dave's patchset removes the need for that hack, but the DSI host still 
has to be initialized on the first DSI command transfer for Exynos HW, 
otherwise the tested DSI panels display nothing.


Best regards