Re: [RFC v2 PATCH 02/14] drm/exynos: dsi: delay setting clocks after reset

[Andrzej Hajda <a.hajda@samsung.com>]

On 04/23/2014 05:45 AM, YoungJun Cho wrote:
> Hi again Andrzej,
> 
> On 04/23/2014 10:01 AM, YoungJun Cho wrote:
>> Hi Andrzej
>>
>> Thank you for comments.
>>
>> On 04/22/2014 09:15 PM, Andrzej Hajda wrote:
>>> Hi YoungJun,
>>>
>>> On 04/21/2014 02:28 PM, YoungJun Cho wrote:
>>>> Some phy control registers are not kept after software reset.
>>>> So this patch makes the clocks containing phy control to be set
>>>> after software reset.
>>>>
>>>> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
>>>> Acked-by: Inki Dae <inki.dae@samsung.com>
>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>> ---
>>>>   drivers/gpu/drm/exynos/exynos_drm_dsi.c |    2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> index 956e5f3..2cf1f0b 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> @@ -946,10 +946,10 @@ static irqreturn_t exynos_dsi_irq(int irq, void
>>>> *dev_id)
>>>>
>>>>   static int exynos_dsi_init(struct exynos_dsi *dsi)
>>>>   {
>>>> -    exynos_dsi_enable_clock(dsi);
>>>>       exynos_dsi_reset(dsi);
>>>>       enable_irq(dsi->irq);
>>>>       exynos_dsi_wait_for_reset(dsi);
>>>> +    exynos_dsi_enable_clock(dsi);
>>>>       exynos_dsi_init_link(dsi);
>>>>
>>>>       return 0;
>>>
>>> I have commented it in the previous version of the patchset. I repeat it
>>> again.
>>> This is a regression, on exynos4210-trats I have 'timeout waiting for
>>> reset' message after dpms off, on.
>>
>> I'm really sorry for that. I misunderstood last time.
>>
>> I think the original codes were correct, because the reset timeout would
>> be occurred without clock activation.
> 
> This is not true.
> 
>>
>> I'll check and fix again.
>> (By the way, why am I ok?)
> 
> I have not verified with exynos4210-trats board yet(I have to get it),
> but reset timeout is occured in exynos_dsi_wait_for_reset()
> with &dsi->completed and that is completed by exynos_dsi_irq().
> 
> And the regulators and clocks are enabled by exynos_dsi_poweron(),
> NOT by exynos_dsi_enable_clock().
> 
> So I think the reset timeout is not related with this patch.


As far as I remember there were at least two issues with init sequence:
- spurious irq storm after power on and before reset,
- irq reset timeouts after reset and without enabled clock.

The current sequence is a result of tests on live hw (documentation were
not helpful in this case). I think it could be improved little bit more
by moving exynos_dsi_enable_clock just after enable_irq this will
eliminate possible timeout when RST_RELEASE irq is signaled but irq is
still disabled. The sequence should look like below:

	exynos_dsi_reset(dsi);
	enable_irq(dsi->irq);
	exynos_dsi_enable_clock(dsi);
	exynos_dsi_wait_for_reset(dsi);
	exynos_dsi_init_link(dsi);

And PHY related configuration could be put somewhere after
exynos_dsi_wait_for_reset.

I have tested this sequence on trats, it seems to be OK.

Regards
Andrzej


> 
> Anyway I need more investigation.
> 
> Thank you.
> Best regards YJ
> 
>>
>>>
>>> I will comment your previous answer here to make the discussion easier:
>>>> As I mentioned in description, it came from phy control registers.
>>>> Fortunately Exynos4 SoCs are safe, but the DSIM_PHYCTRL_REG,
>>>> DSIM_PHYTIMING_REG, DSIM_PHYTIMING1_REG and DSIM_PHYTIMING2_REG are
>>>> affected which are used in exynos_dsi_set_pll() for Exynos5 SoCs.
>>>>
>>>> So this patch is required for Exynos5 SoCs.
>>>
>>> In the moment this patch is applied exynos_dsi_set_pll do not touch phy
>>> registers you have mentioned.
>>> Your change would be more clear if it will be merged together with the
>>> patch adding PHYCTRL settings.
>>>
>>> Anyway, solution is simple - please set PHY registers after reset and
>>> configure clocks before reset to avoid
>>> reset timeouts, is there any reason to not do it this way?
>>
>> The only reason is that the PHY control is related with PLL control and
>> that was in exynos_dsi_enable_clock() call path.
>> So I just wanted to keep current sequence.
>>
>> If there is no way to use previous one, I'll consider your approach.
>>
>> Thank you.
>> Best regards YJ
>>
>>>
>>> Regards
>>> Andrzej
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>