Hi Tomi, > -----Original Message----- > From: dri-devel <[email protected]> On Behalf Of Tomi > Valkeinen > Sent: 21 August 2025 10:29 > Subject: Re: [PATCH v7 6/6] drm: renesas: rz-du: mipi_dsi: Add support for > RZ/V2H(P) SoC > > 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.
It was added based on v3 based on my comment on v2[1]. If you see each table entries it is now just 1 byte vs 10 bytes (u8 vs (u64 + u16)). So, it is saving considerable space by using this methos. Maybe we need to document this [1] https://lore.kernel.org/all/ty3pr01mb113462cc30072b670e23ec7f586...@ty3pr01mb11346.jpnprd01.prod.outlook.com/ Cheers, Biju
