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

[Inki Dae]

Thanks for update,

2026년 8월 9일 (일) 오후 5:59, 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.

I like the direction - replacing the busy loop with the interrupt the
hardware already provides is clearly the right thing to do. Below are
my comments.

>
> To utilize this, explicitly unmask PLL_STABLE and SW_RST_RELEASE right
> after reset, and wait for reset before setting up clock.
>
> 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
> Signed-off-by: Kaustabh Chakraborty <kauschluss@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 49 +++++++++++++++++++++++++----------
>  include/drm/bridge/samsung-dsim.h     |  1 +
>  2 files changed, 37 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index fc42acc6b9ef..9f58c7b1be53 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -17,6 +17,7 @@
>  #include <linux/export.h>
>  #include <linux/irq.h>
>  #include <linux/media-bus-format.h>
> +#include <linux/minmax.h>
>  #include <linux/of.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> @@ -722,9 +723,14 @@ static void samsung_dsim_wait_for_reset(struct samsung_dsim *dsi)
>  static void samsung_dsim_reset(struct samsung_dsim *dsi)
>  {
>         u32 reset_val = dsi->driver_data->reg_values[RESET_TYPE];
> +       u32 status;
>
>         reinit_completion(&dsi->completed);
>         samsung_dsim_write(dsi, DSIM_SWRST_REG, reset_val);
> +
> +       status = samsung_dsim_read(dsi, DSIM_INTMSK_REG);
> +       status &= ~(DSIM_INT_PLL_STABLE | DSIM_INT_SW_RST_RELEASE);
> +       samsung_dsim_write(dsi, DSIM_INTMSK_REG, status);

What does this hunk actually buy us? Two concerns:

First, it is racy. You trigger the software reset and then immediately
read-modify-write DSIM_INTMSK. If the reset is still in flight, the
hardware may restore the register defaults and drop this write.

Second, it looks like a no-op. The fact that SW_RST_RELEASE is
delivered today - without any driver code unmasking it - implies that
DSIM_INTMSK reads back as all-unmasked after reset. If that is true,
clearing those two bits changes nothing.

The change that actually matters is adding DSIM_INT_PLL_STABLE to the
mask written from the SW_RST_RELEASE handler, which you do further
down. Unless there is a case I am missing, please drop this hunk. If
it is needed, please add a comment explaining why.

>  }
>
>  static unsigned long samsung_dsim_pll_find_pms(struct samsung_dsim *dsi,
> @@ -787,8 +793,7 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>                                           unsigned long freq)
>  {
>         const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
> -       unsigned long fin, fout;
> -       int timeout;
> +       unsigned long fin, fout, timeout_us, bus_clk_mhz;
>         u8 p, s;
>         u16 m;
>         u32 reg;
> @@ -849,19 +854,33 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>         if (dsi->swap_dn_dp_data)
>                 reg |= DSIM_PLL_DPDNSWAP_DAT;
>
> +       /*
> +        * The PLL_TIMER value is the product of the timeout delay and the APB
> +        * bus clock rate. Calculate the timeout delay on-the-fly here.
> +        * It is assumed that the bus clock is the first clock in the provided
> +        * bulk clock data.
> +        */
> +       timeout_us = 100;
> +       bus_clk_mhz = clk_get_rate(dsi->driver_data->clk_data[0].clk) / HZ_PER_MHZ;
> +       if (bus_clk_mhz)
> +               timeout_us = max(driver_data->reg_values[PLL_TIMER] / bus_clk_mhz,
> +                                timeout_us);

PLL_TIMER is a hardware PLL lock timer. Deriving a *software* timeout
from it is a heuristic with no documentation behind it, and the values
we carry vary wildly (500, 22200, 80000).
Since the completion wakes us up as soon as the PLL is stable, a
generous fixed timeout costs nothing on working hardware and is much
easier to reason about. Please just use a fixed value in the
millisecond range.

That also removes the need for the linux/minmax.h include and, more
importantly, for reaching into clk_data[] by index.

On that last point: clk_data points at a static array
(exynos3_clk_bulk_data[] and friends) shared by every instance using
the same driver_data. Indexing it with a hardcoded 0 and documenting
the "first clock is the bus clock" assumption only in a comment is
fragile.
If you do need the bus clock rate, please add an explicit index or
pointer to struct samsung_dsim_driver_data rather than relying on
array ordering.

> +
> +       reinit_completion(&dsi->pll_stabilized);
>         samsung_dsim_write(dsi, DSIM_PLLCTRL_REG, reg);
>
> -       timeout = 3000;
> -       do {
> -               if (timeout-- == 0) {
> -                       dev_err(dsi->dev, "PLL failed to stabilize\n");
> -                       return 0;
> -               }
> +       if (!wait_for_completion_timeout(&dsi->pll_stabilized,
> +                                        usecs_to_jiffies(timeout_us))) {
>                 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 (!(reg & BIT(driver_data->pll_stable_bit))) {
> +                       dev_err(dsi->dev, "PLL failed to stabilize\n");
> +                       return 0;
> +               }
> +       }

usecs_to_jiffies(100) rounds up to a single jiffy on any HZ we
support, and wait_for_completion_timeout() arms a timer that expires
on the next tick boundary. The actual wait can therefore be
arbitrarily close to zero. The "gracious lower bound of 100
microseconds" from the changelog simply does not exist in practice.

Combined with the fallback being a *single* register read, this is a
regression waiting to happen: on any platform where PLL_STABLE is not
delivered - and per my note above, that is every platform except your
Exynos7870 as far as we know - we go from 3000 polls (milliseconds) to
one read taken possibly microseconds after the PLL was programmed.

Please raise the lower bound to something meaningful, e.g. 20 ms.

>
>         dsi->hs_clock = fout;
>
> @@ -1586,7 +1605,8 @@ static irqreturn_t samsung_dsim_irq(int irq, void *dev_id)
>         samsung_dsim_write(dsi, DSIM_INTSRC_REG, status);
>
>         if (status & DSIM_INT_SW_RST_RELEASE) {
> -               unsigned long mask = ~(DSIM_INT_RX_DONE |
> +               unsigned long mask = ~(DSIM_INT_PLL_STABLE |
> +                                      DSIM_INT_RX_DONE |
>                                        DSIM_INT_SFR_FIFO_EMPTY |
>                                        DSIM_INT_SFR_HDR_FIFO_EMPTY |
>                                        DSIM_INT_RX_ECC_ERR |
> @@ -1596,8 +1616,10 @@ static irqreturn_t samsung_dsim_irq(int irq, void *dev_id)
>                 return IRQ_HANDLED;
>         }
>
> -       if (!(status & (DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY |
> -                       DSIM_INT_PLL_STABLE)))
> +       if (status & DSIM_INT_PLL_STABLE)
> +               complete(&dsi->pll_stabilized);
> +
> +       if (!(status & (DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY)))
>                 return IRQ_HANDLED;

The unmask change is correct, but the placement of the complete() call
is not. The SW_RST_RELEASE branch returns before we ever look at
DSIM_INT_PLL_STABLE, and DSIM_INTSRC has already been write-cleared
for the full status word at the top of the handler.

So if both bits are set in a single read, the PLL_STABLE source is
acknowledged in hardware but complete() is never called, and the event
is lost for good. set_pll() then always hits the timeout and falls
back to the single read discussed above.

This is not theoretical for the wait_for_reset == 0
platforms(exynos5433, imx8mm, imx8mp): there we proceed to set_pll()
without waiting for the reset to complete, so the two events can
genuinely overlap.

Please move the PLL_STABLE handling above the SW_RST_RELEASE block.

Thanks,
Inki Dae

>
>         if (samsung_dsim_transfer_finish(dsi))
> @@ -1635,9 +1657,9 @@ static int samsung_dsim_init(struct samsung_dsim *dsi)
>         if (driver_data->reg_values[RESET_TYPE] == DSIM_FUNCRST)
>                 samsung_dsim_enable_lane(dsi, BIT(dsi->lanes) - 1);
>
> -       samsung_dsim_enable_clock(dsi);
>         if (driver_data->wait_for_reset)
>                 samsung_dsim_wait_for_reset(dsi);
> +       samsung_dsim_enable_clock(dsi);
>         samsung_dsim_set_phy_ctrl(dsi);
>         samsung_dsim_init_link(dsi);
>
> @@ -2148,6 +2170,7 @@ int samsung_dsim_probe(struct platform_device *pdev)
>                 return PTR_ERR(dsi);
>
>         init_completion(&dsi->completed);
> +       init_completion(&dsi->pll_stabilized);
>         spin_lock_init(&dsi->transfer_lock);
>         INIT_LIST_HEAD(&dsi->transfer_list);
>
> diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
> index 03005e474704..e3433da21ad0 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -123,6 +123,7 @@ struct samsung_dsim {
>         int state;
>         struct drm_property *brightness;
>         struct completion completed;
> +       struct completion pll_stabilized;
>
>         spinlock_t transfer_lock; /* protects transfer_list */
>         struct list_head transfer_list;
>
> --
> 2.54.0
>
>