Hi Tomi,
Thank you for the review.
On Thu, Aug 21, 2025 at 10:28 AM Tomi Valkeinen
<tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On 28/07/2025 23:14, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > Add DSI support for Renesas RZ/V2H(P) SoC.
>
> I think a bit longer desc would be in order, as this is not just a "add
> a new compatible string" patch, but we have new registers and functions.
>
Sure, I'll elaborate on the commit message.
> > Co-developed-by: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > v6->v7:
> > - Used the new apis for calculating the PLLDSI
> > parameters in the DSI driver.
> >
> > v5->v6:
> > - Made use of GENMASK() macro for PLLCLKSET0R_PLL_*,
> > PHYTCLKSETR_* and PHYTHSSETR_* macros.
> > - Replaced 10000000UL with 10 * MEGA
> > - Renamed mode_freq_hz to mode_freq_khz in rzv2h_dsi_mode_calc
> > - Replaced `i -= 1;` with `i--;`
> > - Renamed RZV2H_MIPI_DPHY_FOUT_MIN_IN_MEGA to
> > RZV2H_MIPI_DPHY_FOUT_MIN_IN_MHZ and
> > RZV2H_MIPI_DPHY_FOUT_MAX_IN_MEGA to
> > RZV2H_MIPI_DPHY_FOUT_MAX_IN_MHZ.
> >
> > v4->v5:
> > - No changes
> >
> > v3->v4
> > - In rzv2h_dphy_find_ulpsexit() made the array static const.
> >
> > v2->v3:
> > - Simplifed V2H DSI timings array to save space
> > - Switched to use fsleep() instead of udelay()
> >
> > v1->v2:
> > - Dropped unused macros
> > - Added missing LPCLK flag to rzv2h info
> > ---
> > .../gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c | 345 ++++++++++++++++++
> > .../drm/renesas/rz-du/rzg2l_mipi_dsi_regs.h | 34 ++
> > 2 files changed, 379 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> > index 893a90c7a886..3b2f77665309 100644
> > --- a/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> > +++ b/drivers/gpu/drm/renesas/rz-du/rzg2l_mipi_dsi.c
> > @@ -7,6 +7,7 @@
> >
> > #include <linux/bitfield.h>
> > #include <linux/clk.h>
> > +#include <linux/clk/renesas-rzv2h-cpg-pll.h>
> > #include <linux/delay.h>
> > #include <linux/dma-mapping.h>
> > #include <linux/io.h>
> > @@ -46,6 +47,11 @@ struct rzg2l_mipi_dsi_hw_info {
> > u64 *hsfreq_millihz);
> > unsigned int (*dphy_mode_clk_check)(struct rzg2l_mipi_dsi *dsi,
> > unsigned long mode_freq);
> > + struct {
> > + const struct rzv2h_pll_limits **limits;
> > + const u8 *table;
> > + const u8 table_size;
> > + } cpg_plldsi;
> > u32 phy_reg_offset;
> > u32 link_reg_offset;
> > unsigned long min_dclk;
> > @@ -53,6 +59,11 @@ struct rzg2l_mipi_dsi_hw_info {
> > u8 features;
> > };
> >
> > +struct rzv2h_dsi_mode_calc {
> > + unsigned long mode_freq_khz;
> > + struct rzv2h_pll_pars dsi_parameters;
> > +};
> > +
> > struct rzg2l_mipi_dsi {
> > struct device *dev;
> > void __iomem *mmio;
> > @@ -75,11 +86,22 @@ struct rzg2l_mipi_dsi {
> > unsigned int lanes;
> > unsigned long mode_flags;
> >
> > + struct rzv2h_dsi_mode_calc mode_calc;
> > +
> > /* DCS buffer pointers when using external memory. */
> > dma_addr_t dcs_buf_phys;
> > u8 *dcs_buf_virt;
> > };
> >
> > +static const struct rzv2h_pll_limits rzv2h_plldsi_div_limits = {
> > + .fout = { .min = 80 * MEGA, .max = 1500 * MEGA },
> > + .fvco = { .min = 1050 * MEGA, .max = 2100 * MEGA },
> > + .m = { .min = 64, .max = 1023 },
> > + .p = { .min = 1, .max = 4 },
> > + .s = { .min = 0, .max = 5 },
> > + .k = { .min = -32768, .max = 32767 },
> > +};
> > +
> > static inline struct rzg2l_mipi_dsi *
> > bridge_to_rzg2l_mipi_dsi(struct drm_bridge *bridge)
> > {
> > @@ -194,6 +216,155 @@ static const struct rzg2l_mipi_dsi_timings rzg2l_mipi_dsi_global_timings[] = {
> > },
> > };
> >
> > +struct rzv2h_mipi_dsi_timings {
> > + const u8 *hsfreq;
> > + u8 len;
> > + u8 start_index;
> > +};
> > +
> > +enum {
> > + TCLKPRPRCTL,
> > + TCLKZEROCTL,
> > + TCLKPOSTCTL,
> > + TCLKTRAILCTL,
> > + THSPRPRCTL,
> > + THSZEROCTL,
> > + THSTRAILCTL,
> > + TLPXCTL,
> > + THSEXITCTL,
> > +};
> > +
> > +static const u8 tclkprprctl[] = {
> > + 15, 26, 37, 47, 58, 69, 79, 90, 101, 111, 122, 133, 143, 150,
> > +};
> > +
> > +static const u8 tclkzeroctl[] = {
> > + 9, 11, 13, 15, 18, 21, 23, 24, 25, 27, 29, 31, 34, 36, 38,
> > + 41, 43, 45, 47, 50, 52, 54, 57, 59, 61, 63, 66, 68, 70, 73,
> > + 75, 77, 79, 82, 84, 86, 89, 91, 93, 95, 98, 100, 102, 105,
> > + 107, 109, 111, 114, 116, 118, 121, 123, 125, 127, 130, 132,
> > + 134, 137, 139, 141, 143, 146, 148, 150,
> > +};
> > +
> > +static const u8 tclkpostctl[] = {
> > + 8, 21, 34, 48, 61, 74, 88, 101, 114, 128, 141, 150,
> > +};
> > +
> > +static const u8 tclktrailctl[] = {
> > + 14, 25, 37, 48, 59, 71, 82, 94, 105, 117, 128, 139, 150,
> > +};
> > +
> > +static const u8 thsprprctl[] = {
> > + 11, 19, 29, 40, 50, 61, 72, 82, 93, 103, 114, 125, 135, 146, 150,
> > +};
> > +
> > +static const u8 thszeroctl[] = {
> > + 18, 24, 29, 35, 40, 46, 51, 57, 62, 68, 73, 79, 84, 90,
> > + 95, 101, 106, 112, 117, 123, 128, 134, 139, 145, 150,
> > +};
> > +
> > +static const u8 thstrailctl[] = {
> > + 10, 21, 32, 42, 53, 64, 75, 85, 96, 107, 118, 128, 139, 150,
> > +};
> > +
> > +static const u8 tlpxctl[] = {
> > + 13, 26, 39, 53, 66, 79, 93, 106, 119, 133, 146, 150,
> > +};
> > +
> > +static const u8 thsexitctl[] = {
> > + 15, 23, 31, 39, 47, 55, 63, 71, 79, 87,
> > + 95, 103, 111, 119, 127, 135, 143, 150,
> > +};
> > +
> > +static const struct rzv2h_mipi_dsi_timings rzv2h_dsi_timings_tables[] = {
> > + [TCLKPRPRCTL] = {
> > + .hsfreq = tclkprprctl,
> > + .len = ARRAY_SIZE(tclkprprctl),
> > + .start_index = 0,
> > + },
> > + [TCLKZEROCTL] = {
> > + .hsfreq = tclkzeroctl,
> > + .len = ARRAY_SIZE(tclkzeroctl),
> > + .start_index = 2,
> > + },
> > + [TCLKPOSTCTL] = {
> > + .hsfreq = tclkpostctl,
> > + .len = ARRAY_SIZE(tclkpostctl),
> > + .start_index = 6,
> > + },
> > + [TCLKTRAILCTL] = {
> > + .hsfreq = tclktrailctl,
> > + .len = ARRAY_SIZE(tclktrailctl),
> > + .start_index = 1,
> > + },
> > + [THSPRPRCTL] = {
> > + .hsfreq = thsprprctl,
> > + .len = ARRAY_SIZE(thsprprctl),
> > + .start_index = 0,
> > + },
> > + [THSZEROCTL] = {
> > + .hsfreq = thszeroctl,
> > + .len = ARRAY_SIZE(thszeroctl),
> > + .start_index = 0,
> > + },
> > + [THSTRAILCTL] = {
> > + .hsfreq = thstrailctl,
> > + .len = ARRAY_SIZE(thstrailctl),
> > + .start_index = 3,
> > + },
> > + [TLPXCTL] = {
> > + .hsfreq = tlpxctl,
> > + .len = ARRAY_SIZE(tlpxctl),
> > + .start_index = 0,
> > + },
> > + [THSEXITCTL] = {
> > + .hsfreq = thsexitctl,
> > + .len = ARRAY_SIZE(thsexitctl),
> > + .start_index = 1,
> > + },
> > +};
> > +
> > +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--;
> > +
> > + 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] * 10 * MEGA;
> > +
> > + if (freq <= hsfreq)
> > + break;
> > + }
> > +
> > + if (i == timings->len)
> > + i--;
> > +
> > + return timings->start_index + i;
> > +};
>
> I have to say I really don't like this... In the minimum, the method how
> this works has to be explained in a comment. These values can't really
> be calculated? If we really have to deal with hardcoded values and with
> that table from the docs, I would say that just replicate the table in
> the driver (i.e. a struct that represents one row of the table), instead
> of the method in this driver.
>
> Or was this method added based on earlier feedback, for v3? I see
> "Simplifed V2H DSI timings array to save space" in the change log. If
> so, at least document it clearly.
>
As mentioned in the previous thread, we will have to live with the
lookup tables. I'll add some comments for it.
> > +
> > 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);
> > @@ -318,6 +489,150 @@ 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)
> > +{
> > + u64 hsfreq_millihz, mode_freq_hz, mode_freq_millihz;
> > + struct rzv2h_pll_div_pars cpg_dsi_parameters;
> > + struct rzv2h_pll_pars dsi_parameters;
> > + bool parameters_found;
> > + unsigned int bpp;
> > +
> > + bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> > + mode_freq_hz = mul_u32_u32(mode_freq, KILO);
> > + mode_freq_millihz = mode_freq_hz * MILLI;
> > + parameters_found =
> > + rzv2h_get_pll_divs_pars(dsi->info->cpg_plldsi.limits[0],
> > + &cpg_dsi_parameters,
> > + dsi->info->cpg_plldsi.table,
> > + dsi->info->cpg_plldsi.table_size,
> > + mode_freq_millihz);
> > + if (!parameters_found)
> > + return MODE_CLOCK_RANGE;
> > +
> > + hsfreq_millihz = DIV_ROUND_CLOSEST_ULL(cpg_dsi_parameters.div.freq_millihz * bpp,
> > + dsi->lanes);
> > + parameters_found = rzv2h_get_pll_pars(&rzv2h_plldsi_div_limits,
> > + &dsi_parameters, hsfreq_millihz);
> > + if (!parameters_found)
> > + return MODE_CLOCK_RANGE;
> > +
> > + if (abs(dsi_parameters.error_millihz) >= 500)
> > + return MODE_CLOCK_RANGE;
> > +
> > + memcpy(&dsi->mode_calc.dsi_parameters, &dsi_parameters, sizeof(dsi_parameters));
> > + dsi->mode_calc.mode_freq_khz = mode_freq;
> > +
> > + return MODE_OK;
> > +}
> > +
> > +static int rzv2h_dphy_conf_clks(struct rzg2l_mipi_dsi *dsi, unsigned long mode_freq,
> > + u64 *hsfreq_millihz)
> > +{
> > + struct rzv2h_pll_pars *dsi_parameters = &dsi->mode_calc.dsi_parameters;
> > + unsigned long status;
> > +
> > + if (dsi->mode_calc.mode_freq_khz != mode_freq) {
> > + status = rzv2h_dphy_mode_clk_check(dsi, mode_freq);
> > + if (status != MODE_OK) {
> > + dev_err(dsi->dev, "No PLL parameters found for mode clk %lu\n",
> > + mode_freq);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + *hsfreq_millihz = dsi_parameters->freq_millihz;
> > +
> > + return 0;
> > +}
> > +
> > +static int rzv2h_mipi_dsi_dphy_init(struct rzg2l_mipi_dsi *dsi,
> > + u64 hsfreq_millihz)
> > +{
> > + struct rzv2h_pll_pars *dsi_parameters = &dsi->mode_calc.dsi_parameters;
> > + unsigned long lpclk_rate = clk_get_rate(dsi->lpclk);
> > + u32 phytclksetr, phythssetr, phytlpxsetr, phycr;
> > + struct rzg2l_mipi_dsi_timings dphy_timings;
> > + u16 ulpsexit;
> > + u64 hsfreq;
> > +
> > + hsfreq = DIV_ROUND_CLOSEST_ULL(hsfreq_millihz, MILLI);
> > +
> > + if (dsi_parameters->freq_millihz == hsfreq_millihz)
> > + goto parameters_found;
> > +
> > + if (rzv2h_get_pll_pars(&rzv2h_plldsi_div_limits,
> > + dsi_parameters, hsfreq_millihz))
> > + goto parameters_found;
> > +
> > + dev_err(dsi->dev, "No PLL parameters found for HSFREQ %lluHz\n", hsfreq);
> > + return -EINVAL;
> > +
> > +parameters_found:
>
> Maybe:
>
> if (dsi_parameters->freq_millihz != hsfreq_millihz &&
> !rzv2h_get_pll_pars(&rzv2h_plldsi_div_limits, dsi_parameters,
> hsfreq_millihz)) {
> dev_err(dsi->dev, "No PLL parameters found for HSFREQ %lluHz\n",
> hsfreq);
> return -EINVAL;
> }
>
> keeps the flow a bit cleaner.
>
Agreed, I will update as above.
> > + dphy_timings.tclk_trail =
> > + rzv2h_dphy_find_timings_val(hsfreq, TCLKTRAILCTL);
> > + dphy_timings.tclk_post =
> > + rzv2h_dphy_find_timings_val(hsfreq, TCLKPOSTCTL);
> > + dphy_timings.tclk_zero =
> > + rzv2h_dphy_find_timings_val(hsfreq, TCLKZEROCTL);
> > + dphy_timings.tclk_prepare =
> > + rzv2h_dphy_find_timings_val(hsfreq, TCLKPRPRCTL);
> > + dphy_timings.ths_exit =
> > + rzv2h_dphy_find_timings_val(hsfreq, THSEXITCTL);
> > + dphy_timings.ths_trail =
> > + rzv2h_dphy_find_timings_val(hsfreq, THSTRAILCTL);
> > + dphy_timings.ths_zero =
> > + rzv2h_dphy_find_timings_val(hsfreq, THSZEROCTL);
> > + dphy_timings.ths_prepare =
> > + rzv2h_dphy_find_timings_val(hsfreq, THSPRPRCTL);
> > + dphy_timings.tlpx =
> > + rzv2h_dphy_find_timings_val(hsfreq, TLPXCTL);
> > + ulpsexit = rzv2h_dphy_find_ulpsexit(lpclk_rate);
> > +
> > + phytclksetr = FIELD_PREP(PHYTCLKSETR_TCLKTRAILCTL, dphy_timings.tclk_trail) |
> > + FIELD_PREP(PHYTCLKSETR_TCLKPOSTCTL, dphy_timings.tclk_post) |
> > + FIELD_PREP(PHYTCLKSETR_TCLKZEROCTL, dphy_timings.tclk_zero) |
> > + FIELD_PREP(PHYTCLKSETR_TCLKPRPRCTL, dphy_timings.tclk_prepare);
> > + phythssetr = FIELD_PREP(PHYTHSSETR_THSEXITCTL, dphy_timings.ths_exit) |
> > + FIELD_PREP(PHYTHSSETR_THSTRAILCTL, dphy_timings.ths_trail) |
> > + FIELD_PREP(PHYTHSSETR_THSZEROCTL, dphy_timings.ths_zero) |
> > + FIELD_PREP(PHYTHSSETR_THSPRPRCTL, dphy_timings.ths_prepare);
> > + phytlpxsetr = rzg2l_mipi_dsi_phy_read(dsi, PHYTLPXSETR) & ~PHYTLPXSETR_TLPXCTL;
> > + phytlpxsetr |= FIELD_PREP(PHYTLPXSETR_TLPXCTL, dphy_timings.tlpx);
> > + phycr = rzg2l_mipi_dsi_phy_read(dsi, PHYCR) & ~GENMASK(9, 0);
> > + phycr |= FIELD_PREP(PHYCR_ULPSEXIT, ulpsexit);
> > +
> > + /* Setting all D-PHY Timings Registers */
> > + rzg2l_mipi_dsi_phy_write(dsi, PHYTCLKSETR, phytclksetr);
> > + rzg2l_mipi_dsi_phy_write(dsi, PHYTHSSETR, phythssetr);
> > + rzg2l_mipi_dsi_phy_write(dsi, PHYTLPXSETR, phytlpxsetr);
> > + rzg2l_mipi_dsi_phy_write(dsi, PHYCR, phycr);
> > +
> > + rzg2l_mipi_dsi_phy_write(dsi, PLLCLKSET0R,
> > + FIELD_PREP(PLLCLKSET0R_PLL_S, dsi_parameters->s) |
> > + FIELD_PREP(PLLCLKSET0R_PLL_P, dsi_parameters->p) |
> > + FIELD_PREP(PLLCLKSET0R_PLL_M, dsi_parameters->m));
> > + rzg2l_mipi_dsi_phy_write(dsi, PLLCLKSET1R,
> > + FIELD_PREP(PLLCLKSET1R_PLL_K, dsi_parameters->k));
> > + fsleep(20);
>
> Why sleep? Sleeps should (almost) always have a comment, explaining what
> it is waiting for.
>
Sure, I will add comments here (and below).
Cheers,
Prabhakar
> > +
> > + rzg2l_mipi_dsi_phy_write(dsi, PLLENR, PLLENR_PLLEN);
> > + fsleep(500);
> > +
> > + return 0;
> > +}
> > +
> > +static void rzv2h_mipi_dsi_dphy_startup_late_init(struct rzg2l_mipi_dsi *dsi)
> > +{
> > + fsleep(220);
>
> Especially sleeps like this, where the upper side is "open ended".
>
> > + rzg2l_mipi_dsi_phy_write(dsi, PHYRSTR, PHYRSTR_PHYMRSTN);
> > +}
> > +
>
> Tomi
>
