[PATCH 09/20] drm/bridge: tc358775: remove complex vsdelay calculation

[Daniel Semkowicz]

Hello Michael,

On Mon, May 6, 2024 at 3:35 PM Michael Walle <mwalle at kernel.org> wrote:
>
> To cite the datasheet on VSDELAY:
>   During DSI link speed is slower than that of LVDS link’s, data needs
>   to be buffer within 775XBG before outputting to prevent data from
>   underflow. Register field VPCTRL[VSDELAY] is used to for this purpose
>
> This driver assumes that the DSI link speed is the pixel clock (as does
> every DSI bridge driver), after all the LVDS clock is derived from the
> DSI clock. Thus we know for a fact, that the DSI link is not slower than
> the LVDS side. Just use the (sane) default value of the bridge and drop
> the complicated calculation here.

I am not convinced this is a good idea to revert to a default
VSdelay value. I tested your patch series with RK3399 platform
and default value (5) was not enough there. There was small data
underflow visible, resulting in display offset. Removing this patch
and using the original calculation formula fixed the problem.
The calculated VSDELAY value seems to be a lot bigger than required,
but keeps us on the safe side.

It looks that hback-porch value for panel is used also on DSI link,
effectively delaying hactive data delivered to TC358775 bridge.
I suspect this causes the requirement for higher VSDELAY.

>
> While at it, replace the TC358775_VPCTRL_MSF() and
> TC358775_VPCTRL_OPXLFMT() inline functions by the usual macros for a bit
> flag.
>
> Signed-off-by: Michael Walle <mwalle at kernel.org>
> ---
>  drivers/gpu/drm/bridge/tc358775.c | 49 +++++++--------------------------------
>  1 file changed, 8 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c
> index 54aea58a3406..a9d731e87970 100644
> --- a/drivers/gpu/drm/bridge/tc358775.c
> +++ b/drivers/gpu/drm/bridge/tc358775.c
> @@ -109,7 +109,9 @@
>  #define RDPKTLN         0x0404  /* Command Read Packet Length */
>
>  #define VPCTRL          0x0450  /* Video Path Control */
> -#define EVTMODE                BIT(5)  /* Video event mode enable, tc35876x only */
> +#define VPCTRL_MSF     BIT(0)
> +#define VPCTRL_OPXLFMT BIT(8)
> +#define VPCTRL_EVTMODE BIT(5)  /* Video event mode enable, tc35876x only */
>  #define HTIM1           0x0454  /* Horizontal Timing Control 1 */
>  #define HTIM2           0x0458  /* Horizontal Timing Control 2 */
>  #define VTIM1           0x045C  /* Vertical Timing Control 1 */
> @@ -187,30 +189,6 @@ enum {
>
>  #define L0EN BIT(1)
>
> -#define TC358775_VPCTRL_VSDELAY__MASK  0x3FF00000
> -#define TC358775_VPCTRL_VSDELAY__SHIFT 20
> -static inline u32 TC358775_VPCTRL_VSDELAY(uint32_t val)
> -{
> -       return ((val) << TC358775_VPCTRL_VSDELAY__SHIFT) &
> -                       TC358775_VPCTRL_VSDELAY__MASK;
> -}
> -
> -#define TC358775_VPCTRL_OPXLFMT__MASK  0x00000100
> -#define TC358775_VPCTRL_OPXLFMT__SHIFT 8
> -static inline u32 TC358775_VPCTRL_OPXLFMT(uint32_t val)
> -{
> -       return ((val) << TC358775_VPCTRL_OPXLFMT__SHIFT) &
> -                       TC358775_VPCTRL_OPXLFMT__MASK;
> -}
> -
> -#define TC358775_VPCTRL_MSF__MASK      0x00000001
> -#define TC358775_VPCTRL_MSF__SHIFT     0
> -static inline u32 TC358775_VPCTRL_MSF(uint32_t val)
> -{
> -       return ((val) << TC358775_VPCTRL_MSF__SHIFT) &
> -                       TC358775_VPCTRL_MSF__MASK;
> -}
> -
>  #define TC358775_LVCFG_PCLKDIV__MASK   0x000000f0
>  #define TC358775_LVCFG_PCLKDIV__SHIFT  4
>  static inline u32 TC358775_LVCFG_PCLKDIV(uint32_t val)
> @@ -350,7 +328,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>         u32 hback_porch, hsync_len, hfront_porch, hactive, htime1, htime2;
>         u32 vback_porch, vsync_len, vfront_porch, vactive, vtime1, vtime2;
>         unsigned int val = 0;
> -       u16 dsiclk, clkdiv, byteclk, t1, t2, t3, vsdelay;
>         struct drm_display_mode *mode;
>         struct drm_connector *connector = get_connector(bridge->encoder);
>
> @@ -398,27 +375,17 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>
>         /* Video event mode vs pulse mode bit, does not exist for tc358775 */
>         if (tc->type == TC358765)
> -               val = EVTMODE;
> +               val = VPCTRL_EVTMODE;
>         else
>                 val = 0;
>
>         if (tc->bpc == 8)
> -               val |= TC358775_VPCTRL_OPXLFMT(1);
> +               val |= VPCTRL_OPXLFMT;
>         else /* bpc = 6; */
> -               val |= TC358775_VPCTRL_MSF(1);
> -
> -       dsiclk = mode->crtc_clock * 3 * tc->bpc / tc->num_dsi_lanes / 1000;
> -       clkdiv = dsiclk / (tc->lvds_link == DUAL_LINK ? DIVIDE_BY_6 : DIVIDE_BY_3);
> -       byteclk = dsiclk / 4;
> -       t1 = hactive * (tc->bpc * 3 / 8) / tc->num_dsi_lanes;
> -       t2 = ((100000 / clkdiv)) * (hactive + hback_porch + hsync_len + hfront_porch) / 1000;
> -       t3 = ((t2 * byteclk) / 100) - (hactive * (tc->bpc * 3 / 8) /
> -               tc->num_dsi_lanes);
> -
> -       vsdelay = (clkdiv * (t1 + t3) / byteclk) - hback_porch - hsync_len - hactive;
> +               val |= VPCTRL_MSF;
>
> -       val |= TC358775_VPCTRL_VSDELAY(vsdelay);
> -       regmap_write(tc->regmap, VPCTRL, val);
> +       regmap_update_bits(tc->regmap, VPCTRL, val,
> +                          VPCTRL_OPXLFMT | VPCTRL_MSF | VPCTRL_EVTMODE);
>
>         regmap_write(tc->regmap, HTIM1, htime1);
>         regmap_write(tc->regmap, VTIM1, vtime1);
>
> --
> 2.39.2
>

Kind regards
Daniel