Hi, On 28/07/2025 23:14, Prabhakar wrote: > From: Lad Prabhakar <[email protected]> > > 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. > Co-developed-by: Fabrizio Castro <[email protected]> > Signed-off-by: Fabrizio Castro <[email protected]> > Signed-off-by: Lad Prabhakar <[email protected]> > --- > 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. > + > 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. > + 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. > + > + 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
