[PATCH v3 2/3] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability
Inki Dae
daeinki at gmail.com
Sat Aug 1 23:56:43 PDT 2026
HI,
2026년 7월 23일 (목) 오전 4:04, Kaustabh Chakraborty <kauschluss at disroot.org>님이 작성:
>
> 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 at samsung.com>
> Link: https://lore.kernel.org/r/CAAQKjZMLMbwDVZRb5+Xb_5yz3AEP4uuzFJMuuZy9NFDu13VU5w@mail.gmail.com
> Tested-by: Marek Szyprowski <m.szyprowski at samsung.com>
> Signed-off-by: Kaustabh Chakraborty <kauschluss at disroot.org>
> ---
> 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
> @@ -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>
> @@ -788,7 +789,7 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
> {
> const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
> unsigned long fin, fout;
> - int timeout;
> + unsigned int timeout;
The value assigned to `timeout` below, `reg_values[PLL_TIMER] / fin`,
is an `unsigned long`. It compiles thanks to `__careful_cmp()` in
`max()`, but the result is silently narrowed to `unsigned int`. Please
make this `unsigned long timeout` instead.
> u8 p, s;
> u16 m;
> u32 reg;
> @@ -849,19 +850,26 @@ 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. Calcutate the timeout delay on-the-fly here.
Typo: "Calcutate" -> "Calculate".
> + * It is assumed that the bus clock is the first clock in the provided
> + * bulk clock data.
> + */
> + timeout = 100;
> + fin = clk_get_rate(dsi->driver_data->clk_data[0].clk) / HZ_PER_MHZ;
`fin` is used earlier in this function as the PLL reference clock in
Hz, and is printed by dev_dbg(). Overwriting it here with a completely
unrelated value - the APB bus clock in MHz - is confusing when
debugging. Please use a separate variable:
unsigned long bus_clk_mhz;
...
bus_clk_mhz = clk_get_rate(driver_data->clk_data[0].clk) / HZ_PER_MHZ;
> + if (fin)
> + timeout = max(dsi->driver_data->reg_values[PLL_TIMER] / fin,
> + timeout);
Also, there is already a `driver_data` local at the top of the
function which the rest of the function uses. Only the newly added
code dereferences `dsi->driver_data->` directly, which is
inconsistent.
> +
> + 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 (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.
ait_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.
One more thing - this removes the only user of
driver_data->pll_stable_bit. The field declaration in the header and
all seven driver_data initialisers are now dead. Please drop them in
the same series. (has_legacy_status_reg is still used by
samsung_dsim_init_link(), so that one should stay.)
>
> dsi->hs_clock = fout;
>
> @@ -1596,8 +1604,12 @@ 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);
> + return IRQ_HANDLED;
> + }
> +
> + if (!(status & (DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY)))
> return IRQ_HANDLED;
Two problems here.
First, DSIM_INT_PLL_STABLE is masked in INTMSK, so this interrupt can
never be delivered. The only place that writes INTMSK is the
SW_RST_RELEASE path just above:
unsigned long mask = ~(DSIM_INT_RX_DONE |
DSIM_INT_SFR_FIFO_EMPTY |
DSIM_INT_SFR_HDR_FIFO_EMPTY |
DSIM_INT_RX_ECC_ERR |
DSIM_INT_SW_RST_RELEASE);
samsung_dsim_write(dsi, DSIM_INTMSK_REG, mask);
DSIM_INT_PLL_STABLE is not in that list, so it stays masked. And
Exynos 7870 has wait_for_reset = 1, so samsung_dsim_init() runs:
samsung_dsim_wait_for_reset() <- runs this handler, programs INTMSK
samsung_dsim_enable_clock() <- calls samsung_dsim_set_pll()
meaning PLL_STABLE is guaranteed to be masked by the time set_pll()
runs. As a result this patch performs no PLL stabilization wait at
all, and instead sleeps for the full timeout on every enable. Because
of usecs_to_jiffies() resolution, 100us rounds up to at least one
jiffy - 4ms at HZ=250, 10ms at HZ=100. That is a new latency
regression compared to the old polling loop.
Please add DSIM_INT_PLL_STABLE to the INTMSK list. It would also be
safer to program INTMSK explicitly right after reset, so that SoCs
with wait_for_reset = 0 (which never reach this handler path) are
covered too.
Second, the early return swallows concurrent events. `status` can have
several bits set at once, and everything has already been cleared
write-1-to-clear in INTSRC above. If PLL_STABLE and SFR_FIFO_EMPTY are
raised together, the samsung_dsim_transfer_finish() handling is lost
permanently and that transfer stalls until its own timeout. The
pre-patch code handled all three bits in one condition and fell
through, so it did not have this problem.
Please make the handling non-exclusive:
if (status & DSIM_INT_PLL_STABLE)
complete(&dsi->pll_stabilized);
if (!(status & (DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY)))
return IRQ_HANDLED;
Thanks,
Inki Dae
>
> if (samsung_dsim_transfer_finish(dsi))
> @@ -2148,6 +2160,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
>
>
More information about the linux-arm-kernel
mailing list