Re: [PATCH v3 2/3] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability

[Kaustabh Chakraborty]

On 2026-08-02 15:56 +09:00, Inki Dae wrote:
> HI,
>
> 2026년 7월 23일 (목) 오전 4:04, Kaustabh Chakraborty <kauschluss@xxxxxxxxxxx>님이 작성:
>>
>> Stabilizing PLL needs to be waited for. This is done using a loop,
>> checking the PLL_STABLE bit in the status register.
>>
>> DSIM fires an interrupt when the PLL is stabilized. Rely on this
>> functionality for stabilization wait, getting rid of the implicit loop.
>>
>> This has been tested on a Galaxy J6 (Exynos 7870). Unfortunately, since
>> testing on all supported devices is less feasible, introduce a stop-gap
>> measure where the timeout has a gracious lower bound of 100
>> microseconds. This will (hopefully) prevent regressions due to timeout
>> on other devices.
>>
>> Suggested-by: Inki Dae <inki.dae@xxxxxxxxxxx>
>> Link: https://lore.kernel.org/r/CAAQKjZMLMbwDVZRb5+Xb_5yz3AEP4uuzFJMuuZy9NFDu13VU5w@xxxxxxxxxxxxxx
>> Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>> Signed-off-by: Kaustabh Chakraborty <kauschluss@xxxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/bridge/samsung-dsim.c | 41 +++++++++++++++++++++++------------
>>  include/drm/bridge/samsung-dsim.h     |  1 +
>>  2 files changed, 28 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>> index da753ff6eed4..866cff205e71 100644
>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c

...

>>
>> -       timeout = 3000;
>> -       do {
>> -               if (timeout-- == 0) {
>> -                       dev_err(dsi->dev, "PLL failed to stabilize\n");
>> -                       return 0;
>> -               }
>> -               if (driver_data->has_legacy_status_reg)
>> -                       reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
>> -               else
>> -                       reg = samsung_dsim_read(dsi, DSIM_LINK_STATUS_REG);
>> -       } while ((reg & BIT(driver_data->pll_stable_bit)) == 0);
>> +       if (wait_for_completion_timeout(&dsi->pll_stabilized,
>> +                                       usecs_to_jiffies(timeout))) {
>> +               dev_err(dsi->dev, "PLL failed to stabilize\n");
>> +               return 0;
>> +       }
>
> This is the main problem: the condition is inverted.
>
> wait_for_completion_timeout() returns 0 on timeout, and the number of
> remaining jiffies (> 0) on completion. As written, this reports an
> error and returns 0 exactly when the PLL *did* stabilize, and silently
> succeeds when it timed out.
>
> Since samsung_dsim_set_pll() returning 0 makes its caller
> samsung_dsim_enable_clock() bail out with -EFAULT, the display would
> fail to come up entirely the moment a PLL_STABLE interrupt actually
> arrives.
>
>     if (!wait_for_completion_timeout(&dsi->pll_stabilized,
>                                    usecs_to_jiffies(timeout))) {
>
> The reason this still passed testing is the second issue below: the
> PLL_STABLE interrupt never fires in the first place. The two bugs are
> masking each other.

Unfortunately, fixing these my hardware does not issue the PLL_STABLE
interrupt for the first panel reset during boot. For subsequent ones it
does issue the same.

One lazy solution is to try to peek at the PLL stable bit after the
timeout and continue execution if it indicates that PLL is stable. In
that case we should be able to validate it with a fallback.

I should also remember to drop the Reviewed-by: tag from this patch.