[PATCH] drm/bridge: ti-sn65dsi83: Add and use hs_rate and lp_rate

[Dave Stevenson]

Hi Marek

On Mon, 1 Aug 2022 at 14:12, Marek Vasut <marex at denx.de> wrote:
>
> Fill in hs_rate and lp_rate to struct mipi_dsi_device for this bridge and
> adjust DSI input frequency calculations such that they expect the DSI host
> to configure HS clock according to hs_rate.

I think this falls into another of the areas that is lacking in the DSI support.
hs_rate is defined as the *maximum* lane frequency in high speed
mode[1]. As documented there is no obligation on the DSI host to
choose this specific rate, just some frequency below it and above or
equal to that required by the pixel clock. At a system level, the link
frequency is often prescribed for EMC purposes.

The issue is then that the SN65DSI83 is configured to use the DSI
clock lane as the source for the PLL generating the LVDS clock,
therefore with no route for the DSI peripheral to query the link
frequency chosen by the host, you're stuck.

SN65DSI83 also supports non-burst mode (as currently), so how would
this be configured now?
Does MIPI_DSI_MODE_VIDEO_BURST [2] oblige the DSI host to run in burst
mode, or say that burst mode is supported by the peripheral? What if
your DSI host doesn't support burst mode and it is optional on your
peripheral?
I raised these questions and others at [3], but largely got no real answers.


The patch does exactly what you describe and has value, but without
definition in the frameworks of exactly how burst mode must be
implemented by the DSI host, I would say that it's going to cause
issues.
I am aware of users of your driver with the Raspberry Pi, but your
expectation isn't currently valid on the Pi as there is no definition
of the correct thing for the host to do.

Cheers.
  Dave

[1] https://elixir.bootlin.com/linux/latest/source/include/drm/drm_mipi_dsi.h#L176
[2] https://elixir.bootlin.com/linux/latest/source/include/drm/drm_mipi_dsi.h#L119
[3] start of thread at
https://lists.freedesktop.org/archives/dri-devel/2021-July/313576.html
and specifically hs_rate/burst at
https://lists.freedesktop.org/archives/dri-devel/2021-October/326732.html

> This is an optimization for the DSI burst mode case. In case the DSI device
> supports DSI burst mode, it is recommended to operate the DSI interface at
> the highest possible HS clock frequency which the DSI device supports. This
> permits the DSI host to send as short as possible bursts of data on the DSI
> link and keep the DSI data lanes in LP mode otherwise, which reduces power
> consumption.
>
> Signed-off-by: Marek Vasut <marex at denx.de>
> Cc: Jagan Teki <jagan at amarulasolutions.com>
> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Cc: Linus Walleij <linus.walleij at linaro.org>
> Cc: Robert Foss <robert.foss at linaro.org>
> Cc: Sam Ravnborg <sam at ravnborg.org>
> Cc: dri-devel at lists.freedesktop.org
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi83.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index 14e7aa77e7584..b161f25c3a2f5 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -286,8 +286,7 @@ static u8 sn65dsi83_get_lvds_range(struct sn65dsi83 *ctx,
>         return (mode_clock - 12500) / 25000;
>  }
>
> -static u8 sn65dsi83_get_dsi_range(struct sn65dsi83 *ctx,
> -                                 const struct drm_display_mode *mode)
> +static u8 sn65dsi83_get_dsi_range(struct sn65dsi83 *ctx)
>  {
>         /*
>          * The encoding of the CHA_DSI_CLK_RANGE is as follows:
> @@ -303,20 +302,20 @@ static u8 sn65dsi83_get_dsi_range(struct sn65dsi83 *ctx,
>          *  DSI_CLK = mode clock * bpp / dsi_data_lanes / 2
>          * the 2 is there because the bus is DDR.
>          */
> -       return DIV_ROUND_UP(clamp((unsigned int)mode->clock *
> -                           mipi_dsi_pixel_format_to_bpp(ctx->dsi->format) /
> -                           ctx->dsi->lanes / 2, 40000U, 500000U), 5000U);
> +       return DIV_ROUND_UP(ctx->dsi->hs_rate, 5000000U);
>  }
>
> -static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx)
> +static u8 sn65dsi83_get_dsi_div(struct sn65dsi83 *ctx,
> +                               const struct drm_display_mode *mode)
>  {
>         /* The divider is (DSI_CLK / LVDS_CLK) - 1, which really is: */
> -       unsigned int dsi_div = mipi_dsi_pixel_format_to_bpp(ctx->dsi->format);
> +       unsigned int dsi_div;
> +       int mode_clock = mode->clock;
>
> -       dsi_div /= ctx->dsi->lanes;
> +       if (ctx->lvds_dual_link)
> +               mode_clock /= 2;
>
> -       if (!ctx->lvds_dual_link)
> -               dsi_div /= 2;
> +       dsi_div = (ctx->dsi->hs_rate / mode_clock) / 1000;
>
>         return dsi_div - 1;
>  }
> @@ -397,9 +396,9 @@ static void sn65dsi83_atomic_enable(struct drm_bridge *bridge,
>                      REG_RC_LVDS_PLL_LVDS_CLK_RANGE(sn65dsi83_get_lvds_range(ctx, mode)) |
>                      REG_RC_LVDS_PLL_HS_CLK_SRC_DPHY);
>         regmap_write(ctx->regmap, REG_DSI_CLK,
> -                    REG_DSI_CLK_CHA_DSI_CLK_RANGE(sn65dsi83_get_dsi_range(ctx, mode)));
> +                    REG_DSI_CLK_CHA_DSI_CLK_RANGE(sn65dsi83_get_dsi_range(ctx)));
>         regmap_write(ctx->regmap, REG_RC_DSI_CLK,
> -                    REG_RC_DSI_CLK_DSI_CLK_DIVIDER(sn65dsi83_get_dsi_div(ctx)));
> +                    REG_RC_DSI_CLK_DSI_CLK_DIVIDER(sn65dsi83_get_dsi_div(ctx, mode)));
>
>         /* Set number of DSI lanes and LVDS link config. */
>         regmap_write(ctx->regmap, REG_DSI_LANE,
> @@ -643,6 +642,8 @@ static int sn65dsi83_host_attach(struct sn65dsi83 *ctx)
>         dsi->lanes = dsi_lanes;
>         dsi->format = MIPI_DSI_FMT_RGB888;
>         dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST;
> +       dsi->hs_rate = 500000000;
> +       dsi->lp_rate = 16000000;
>
>         ret = devm_mipi_dsi_attach(dev, dsi);
>         if (ret < 0) {
> --
> 2.35.1
>