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

HI,

2026=EB=85=84 7=EC=9B=94 23=EC=9D=BC (=EB=AA=A9) =EC=98=A4=EC=A0=84 4:04, K=
austabh Chakraborty <[email protected]>=EB=8B=98=EC=9D=B4 =EC=9E=91=EC=
=84=B1:
>
> 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 <[email protected]>
> Link: https://lore.kernel.org/r/CAAQKjZMLMbwDVZRb5+Xb_5yz3AEP4uuzFJMuuZy9=
[email protected]
> Tested-by: Marek Szyprowski <[email protected]>
> Signed-off-by: Kaustabh Chakraborty <[email protected]>
> ---
>  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/brid=
ge/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 sams=
ung_dsim *dsi,
>  {
>         const struct samsung_dsim_driver_data *driver_data =3D dsi->drive=
r_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 sa=
msung_dsim *dsi,
>         if (dsi->swap_dn_dp_data)
>                 reg |=3D DSIM_PLL_DPDNSWAP_DAT;
>
> +       /*
> +        * The PLL_TIMER value is the product of the timeout delay and th=
e 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 pro=
vided
> +        * bulk clock data.
> +        */
> +       timeout =3D 100;
> +       fin =3D 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 =3D clk_get_rate(driver_data->clk_data[0].clk) / HZ_PER_MHZ;


> +       if (fin)
> +               timeout =3D 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 =3D 3000;
> -       do {
> -               if (timeout-- =3D=3D 0) {
> -                       dev_err(dsi->dev, "PLL failed to stabilize\n");
> -                       return 0;
> -               }
> -               if (driver_data->has_legacy_status_reg)
> -                       reg =3D samsung_dsim_read(dsi, DSIM_STATUS_REG);
> -               else
> -                       reg =3D samsung_dsim_read(dsi, DSIM_LINK_STATUS_R=
EG);
> -       } while ((reg & BIT(driver_data->pll_stable_bit)) =3D=3D 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 =3D 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 =3D ~(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 =3D 1, so samsung_dsim_init() runs:

        samsung_dsim_wait_for_reset()   <- runs this handler, programs INTM=
SK
        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=3D250, 10ms at HZ=3D100. 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 =3D 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/samsu=
ng-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
>
>