Re: [PATCH v5 4/4] drm: renesas: rz-du: mipi_dsi: Add support for RZ/V2H(P) SoC

["Lad, Prabhakar" <prabhakar.csengg@gmail.com>]

Hi Geert,

Thank you for the review.

On Fri, May 23, 2025 at 4:19 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar, Fabrizio,
>
> On Mon, 12 May 2025 at 20:43, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Add DSI support for Renesas RZ/V2H(P) SoC.
> >
> > Co-developed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Thanks for your patch!
>
> > --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> > @@ -5,6 +5,7 @@
> >   * Copyright (C) 2022 Renesas Electronics Corporation
> >   */
> >  #include <linux/clk.h>
> > +#include <linux/clk/renesas-rzv2h-dsi.h>
> >  #include <linux/delay.h>
> >  #include <linux/io.h>
> >  #include <linux/iopoll.h>
> > @@ -30,6 +31,9 @@
> >
> >  #define RZ_MIPI_DSI_FEATURE_16BPP      BIT(0)
> >
> > +#define RZV2H_MIPI_DPHY_FOUT_MIN_IN_MEGA       (80 * MEGA)
> > +#define RZV2H_MIPI_DPHY_FOUT_MAX_IN_MEGA       (1500 * MEGA)
>
> RZV2H_MIPI_DPHY_FOUT_M{IN,AX}_IN_MHZ?
>
Ok, I'll rename them as above.

> > +
> >  struct rzg2l_mipi_dsi;
> >
> >  struct rzg2l_mipi_dsi_hw_info {
> > @@ -40,6 +44,7 @@ struct rzg2l_mipi_dsi_hw_info {
> >                               u64 *hsfreq_millihz);
> >         unsigned int (*dphy_mode_clk_check)(struct rzg2l_mipi_dsi *dsi,
> >                                             unsigned long mode_freq);
> > +       const struct rzv2h_pll_div_limits *cpg_dsi_limits;
> >         u32 phy_reg_offset;
> >         u32 link_reg_offset;
> >         unsigned long max_dclk;
> > @@ -47,6 +52,11 @@ struct rzg2l_mipi_dsi_hw_info {
> >         u8 features;
> >  };
> >
> > +struct rzv2h_dsi_mode_calc {
> > +       unsigned long mode_freq;
> > +       u64 mode_freq_hz;
>
> Interesting... I guess mode_freq is not in Hz?
>
Actually it is int Hz, I will make it unsigned long.

> > +};
> > +
> >  struct rzg2l_mipi_dsi {
> >         struct device *dev;
> >         void __iomem *mmio;
>
> > +static u16 rzv2h_dphy_find_ulpsexit(unsigned long freq)
> > +{
> > +       static const unsigned long hsfreq[] = {
> > +               1953125UL,
> > +               3906250UL,
> > +               7812500UL,
> > +               15625000UL,
> > +       };
> > +       static const u16 ulpsexit[] = {49, 98, 195, 391};
> > +       unsigned int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(hsfreq); i++) {
> > +               if (freq <= hsfreq[i])
> > +                       break;
> > +       }
> > +
> > +       if (i == ARRAY_SIZE(hsfreq))
> > +               i -= 1;
>
> i--
>
OK.

> > +
> > +       return ulpsexit[i];
> > +}
> > +
> > +static u16 rzv2h_dphy_find_timings_val(unsigned long freq, u8 index)
> > +{
> > +       const struct rzv2h_mipi_dsi_timings *timings;
> > +       u16 i;
> > +
> > +       timings = &rzv2h_dsi_timings_tables[index];
> > +       for (i = 0; i < timings->len; i++) {
> > +               unsigned long hsfreq = timings->hsfreq[i] * 10000000UL;
>
> (I wanted to say "MEGA", but then I noticed the 7th zero ;-)
>
> 10 * MEGA?
>
Agreed, I will update it as above.

> > +
> > +               if (freq <= hsfreq)
> > +                       break;
> > +       }
> > +
> > +       if (i == timings->len)
> > +               i -= 1;
>
> i--
>
> > +
> > +       return timings->start_index + i;
> > +};
> > +
> >  static void rzg2l_mipi_dsi_phy_write(struct rzg2l_mipi_dsi *dsi, u32 reg, u32 data)
> >  {
> >         iowrite32(data, dsi->mmio + dsi->info->phy_reg_offset + reg);
> > @@ -308,6 +479,158 @@ static int rzg2l_dphy_conf_clks(struct rzg2l_mipi_dsi *dsi, unsigned long mode_f
> >         return 0;
> >  }
> >
> > +static unsigned int rzv2h_dphy_mode_clk_check(struct rzg2l_mipi_dsi *dsi,
> > +                                             unsigned long mode_freq)
> > +{
> > +       struct rzv2h_plldsi_parameters *dsi_parameters = &dsi->dsi_parameters;
> > +       u64 hsfreq_millihz, mode_freq_hz, mode_freq_millihz;
> > +       struct rzv2h_plldsi_parameters cpg_dsi_parameters;
> > +       unsigned int bpp, i;
> > +
> > +       bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> > +
> > +       for (i = 0; i < 10; i += 1) {
> > +               unsigned long hsfreq;
> > +               bool parameters_found;
> > +
> > +               mode_freq_hz = mode_freq * MILLI + i;
>
> KILO?
>
OK, as mode_freq_hz is in Hz I'll make it unsigned long.

> And I guess you want to use mul_u32_u32(), as mode_freq_hz is u64?
>
and use mul_u32_u32() below...
> > +               mode_freq_millihz = mode_freq_hz * MILLI * 1ULL;
>
> Why * 1ULL?
>
Agreed, not needed, I will use mul_u32_u32() here.

> > +               parameters_found = rzv2h_dsi_get_pll_parameters_values(dsi->info->cpg_dsi_limits,
> > +                                                                      &cpg_dsi_parameters,
> > +                                                                      mode_freq_millihz);
> > +               if (!parameters_found)
> > +                       continue;
> > +
> > +               hsfreq_millihz = DIV_ROUND_CLOSEST_ULL(cpg_dsi_parameters.freq_millihz * bpp,
> > +                                                      dsi->lanes);
> > +               parameters_found = rzv2h_dsi_get_pll_parameters_values(&rzv2h_plldsi_div_limits,
> > +                                                                      dsi_parameters,
> > +                                                                      hsfreq_millihz);
> > +               if (!parameters_found)
> > +                       continue;
> > +
> > +               if (abs(dsi_parameters->error_millihz) >= 500)
> > +                       continue;
> > +
> > +               hsfreq = DIV_ROUND_CLOSEST_ULL(hsfreq_millihz, MILLI);
> > +               if (hsfreq >= RZV2H_MIPI_DPHY_FOUT_MIN_IN_MEGA &&
> > +                   hsfreq <= RZV2H_MIPI_DPHY_FOUT_MAX_IN_MEGA) {
> > +                       dsi->mode_calc.mode_freq_hz = mode_freq_hz;
> > +                       dsi->mode_calc.mode_freq = mode_freq;
> > +                       return MODE_OK;
> > +               }
> > +       }
> > +
> > +       return MODE_CLOCK_RANGE;
> > +}
>
> > --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi_regs.h
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi_regs.h
> > @@ -40,6 +40,39 @@
> >  #define DSIDPHYTIM3_THS_TRAIL(x)       ((x) << 8)
> >  #define DSIDPHYTIM3_THS_ZERO(x)                ((x) << 0)
> >
> > +/* RZ/V2H DPHY Registers */
> > +#define PLLENR                         0x000
> > +#define PLLENR_PLLEN                   BIT(0)
> > +
> > +#define PHYRSTR                                0x004
> > +#define PHYRSTR_PHYMRSTN               BIT(0)
> > +
> > +#define PLLCLKSET0R                    0x010
> > +#define PLLCLKSET0R_PLL_S(x)           ((x) << 0)
>
>  #define PLLCLKSET0R_PLL_S GENMASK(2, 0)
>
> and after that you can use FIELD_PREP(PLLCLKSET0R_PLL_S, x) in the code.
> More opportunities for masks below...
>
Thanks, I will make use of GENMASK/FIELD_PREP macros.

Cheers,
Prabhakar