Hi Geert, Thank you for your review !!
I will update and submit a new version. Cheers Chris -----Original Message----- From: Geert Uytterhoeven <[email protected]> Sent: Tuesday, October 21, 2025 10:02 AM To: Chris Brandt <[email protected]> Cc: Michael Turquette <[email protected]>; Stephen Boyd <[email protected]>; Biju Das <[email protected]>; Maarten Lankhorst <[email protected]>; Maxime Ripard <[email protected]>; Thomas Zimmermann <[email protected]>; David Airlie <[email protected]>; Simona Vetter <[email protected]>; Hien Huynh <[email protected]>; Nghia Vo <[email protected]>; Hugo Villeneuve <[email protected]>; [email protected]; [email protected]; [email protected] Subject: Re: [PATCH v2 1/2] clk: renesas: rzg2l: Remove DSI clock rate restrictions Hi Chris, On Fri, 12 Sept 2025 at 16:22, Chris Brandt <[email protected]> wrote: > Convert the limited MIPI clock calculations to a full range of > settings based on math including H/W limitation validation. > Since the required DSI division setting must be specified from > external sources before calculations, expose a new API to set it. > > Signed-off-by: Chris Brandt <[email protected]> > Signed-off-by: hienhuynh <[email protected]> > Signed-off-by: Nghia Vo <[email protected]> Thanks for your patch! > --- a/drivers/clk/renesas/rzg2l-cpg.c > +++ b/drivers/clk/renesas/rzg2l-cpg.c > @@ -74,6 +74,22 @@ > #define MSTOP_OFF(conf) FIELD_GET(GENMASK(31, 16), (conf)) > #define MSTOP_MASK(conf) FIELD_GET(GENMASK(15, 0), (conf)) > > +#define PLL5_FOUTVCO_MIN 800000000 > +#define PLL5_FOUTVCO_MAX 3000000000 > +#define PLL5_POSTDIV_MIN 1 > +#define PLL5_POSTDIV_MAX 7 > +#define PLL5_POSTDIV_DEF 1 > +#define PLL5_REFDIV_MIN 1 > +#define PLL5_REFDIV_MAX 2 Documentation says 1..63? > +#define PLL5_REFDIV_DEF 1 > +#define PLL5_INTIN_MIN 20 > +#define PLL5_INTIN_MAX 320 > +#define PLL5_INTIN_DEF 125 > +#define PLL5_FRACIN_DEF 0 > + > +#define PLL5_TARGET_DPI 0 > +#define PLL5_TARGET_DSI 1 These two should become an enum in include/linux/clk/renesas.h, as their values are passed (as magic numbers) from outside. > + > /** > * struct clk_hw_data - clock hardware data > * @hw: clock hw > @@ -129,6 +145,12 @@ struct rzg2l_pll5_param { > u8 pl5_spread; > }; > > +/* PLL5 output will be used for DPI or MIPI-DSI */ static int > +dsi_div_target = PLL5_TARGET_DPI; > + > +/* Required division ratio for MIPI D-PHY clock changes depending on > +resolution and lanes. */ static int dsi_div_ab; unsigned int > + > struct rzg2l_pll5_mux_dsi_div_param { > u8 clksrc; > u8 dsi_div_a; > @@ -557,24 +579,102 @@ rzg2l_cpg_sd_mux_clk_register(const struct > cpg_core_clk *core, } > > static unsigned long > -rzg2l_cpg_get_foutpostdiv_rate(struct rzg2l_pll5_param *params, > +rzg2l_cpg_get_foutpostdiv_rate(struct rzg2l_cpg_priv *priv, > + struct rzg2l_pll5_param *params, > unsigned long rate) { > unsigned long foutpostdiv_rate, foutvco_rate; > + u8 div = 1; > + bool found = 0; > + int a, b; unsigned int > + > + if (priv->mux_dsi_div_params.clksrc) > + div = 2; > + > + /* Calculate the DIV_DSI_A and DIV_DSI_B based on the final DIV DSI */ > + for (a = 0; a < 4; a++) { > + > + if (dsi_div_target == PLL5_TARGET_DPI && a == 0) > + continue; /* 1/1 div not supported for > DIV_DSI_A for DPI */ > + > + for (b = 0; b < 16; b++) { > + > + if (dsi_div_target == PLL5_TARGET_DPI && b != 0) > + continue; /* Only 1/1 div supported for > DIV_DSI_B in DPI */ > + > + if (((1 << a) * (b + 1)) == dsi_div_ab) { "(b + 1) << a"? > + priv->mux_dsi_div_params.dsi_div_a = a; > + priv->mux_dsi_div_params.dsi_div_b = > + b; > + > + goto found_dsi_div; > + } > + } > + } > + > +found_dsi_div: > + /* > + * Below conditions must be set for PLL5 parameters: > + * - REFDIV must be between 1 and 2. > + * - POSTDIV1/2 must be between 1 and 7. > + * - INTIN must be between 20 and 320. > + * - FOUTVCO must be between 800MHz and 3000MHz. > + */ > + for (params->pl5_postdiv1 = PLL5_POSTDIV_MIN; > + params->pl5_postdiv1 < PLL5_POSTDIV_MAX + 1; > + params->pl5_postdiv1++) { > + for (params->pl5_postdiv2 = PLL5_POSTDIV_MIN; > + params->pl5_postdiv2 < PLL5_POSTDIV_MAX + 1; > + params->pl5_postdiv2++) { > + foutvco_rate = rate * ((1 << > +priv->mux_dsi_div_params.dsi_div_a) * rate * ... << priv->mux_dsi_div_params.dsi_div_a; > + > (priv->mux_dsi_div_params.dsi_div_b + 1)) * > + div * params->pl5_postdiv1 * > params->pl5_postdiv2; > + if (foutvco_rate < PLL5_FOUTVCO_MIN + 1 || > + foutvco_rate > PLL5_FOUTVCO_MAX - 1) > + continue; > + > + for (params->pl5_refdiv = PLL5_REFDIV_MIN; > + params->pl5_refdiv < PLL5_REFDIV_MAX + 1; > + params->pl5_refdiv++) { > + params->pl5_intin = (foutvco_rate * > params->pl5_refdiv) / > + (EXTAL_FREQ_IN_MEGA_HZ * > MEGA); > + if (params->pl5_intin < PLL5_INTIN_MIN + 1 || > + params->pl5_intin > PLL5_INTIN_MAX - 1) > + continue; > + params->pl5_fracin = div_u64(((u64) > + (foutvco_rate * > params->pl5_refdiv) % > + > + (EXTAL_FREQ_IN_MEGA_HZ * MEGA)) << 24, Please move the cast to u64 outside the modulo operation, else the latter becomes a 64-by-32 modulo, which requires using a helper from <linux/math64.h> when compile-testing for a 32-bit platform. > + EXTAL_FREQ_IN_MEGA_HZ * > MEGA); > + found = 1; > + goto found_clk; > + } > + } > + } > + > +found_clk: > + if (!found) { > + params->pl5_intin = PLL5_INTIN_DEF; > + params->pl5_fracin = PLL5_FRACIN_DEF; > + params->pl5_refdiv = PLL5_REFDIV_DEF; > + params->pl5_postdiv1 = PLL5_POSTDIV_DEF; > + params->pl5_postdiv2 = PLL5_POSTDIV_DEF; > + } > > - params->pl5_intin = rate / MEGA; > - params->pl5_fracin = div_u64(((u64)rate % MEGA) << 24, MEGA); (u64)(rate % MEGA) > - params->pl5_refdiv = 2; > - params->pl5_postdiv1 = 1; > - params->pl5_postdiv2 = 1; > params->pl5_spread = 0x16; > > foutvco_rate = div_u64(mul_u32_u32(EXTAL_FREQ_IN_MEGA_HZ * MEGA, > - (params->pl5_intin << 24) + > params->pl5_fracin), > - params->pl5_refdiv) >> 24; > + (params->pl5_intin << 24) + params->pl5_fracin), > + params->pl5_refdiv) >> 24; > foutpostdiv_rate = DIV_ROUND_CLOSEST_ULL(foutvco_rate, foutvco_rate is unsigned long, but the division uses the _ULL() macro variant? > params->pl5_postdiv1 > * params->pl5_postdiv2); > > + /* If foutvco is above 1.5GHz, change parent and recalculate */ > + if (priv->mux_dsi_div_params.clksrc && foutvco_rate > 1500000000) { > + priv->mux_dsi_div_params.clksrc = 0; > + dsi_div_ab *= 2; > + dsi_div_target = PLL5_TARGET_DSI; /* Assume MIPI-DSI */ > + return rzg2l_cpg_get_foutpostdiv_rate(priv, params, rate); > + } > + > return foutpostdiv_rate; > } > > @@ -607,7 +707,7 @@ static unsigned long > rzg2l_cpg_get_vclk_parent_rate(struct clk_hw *hw, > struct rzg2l_pll5_param params; > unsigned long parent_rate; > > - parent_rate = rzg2l_cpg_get_foutpostdiv_rate(¶ms, rate); > + parent_rate = rzg2l_cpg_get_foutpostdiv_rate(priv, ¶ms, > + rate); > > if (priv->mux_dsi_div_params.clksrc) > parent_rate /= 2; > @@ -626,6 +726,13 @@ static int rzg2l_cpg_dsi_div_determine_rate(struct > clk_hw *hw, > return 0; > } > > +void rzg2l_cpg_dsi_div_set_divider(int divider, int target) unsigned int divider > +{ > + dsi_div_ab = divider; > + dsi_div_target = target; > +} > +EXPORT_SYMBOL_GPL(rzg2l_cpg_dsi_div_set_divider); > + > static int rzg2l_cpg_dsi_div_set_rate(struct clk_hw *hw, > unsigned long rate, > unsigned long parent_rate) @@ > -859,7 +966,7 @@ static int rzg2l_cpg_sipll5_set_rate(struct clk_hw > *hw, > > vclk_rate = rzg2l_cpg_get_vclk_rate(hw, rate); > sipll5->foutpostdiv_rate = > - rzg2l_cpg_get_foutpostdiv_rate(¶ms, vclk_rate); > + rzg2l_cpg_get_foutpostdiv_rate(priv, ¶ms, > + vclk_rate); > > /* Put PLL5 into standby mode */ > writel(CPG_SIPLL5_STBY_RESETB_WEN, priv->base + > CPG_SIPLL5_STBY); @@ -949,6 +1056,8 @@ rzg2l_cpg_sipll5_register(const struct > cpg_core_clk *core, > priv->mux_dsi_div_params.clksrc = 1; /* Use clk src 1 for DSI */ > priv->mux_dsi_div_params.dsi_div_a = 1; /* Divided by 2 */ > priv->mux_dsi_div_params.dsi_div_b = 2; /* Divided by 3 */ > + dsi_div_ab = (1 << priv->mux_dsi_div_params.dsi_div_a) * > + (priv->mux_dsi_div_params.dsi_div_b + 1); (priv->mux_dsi_div_params.dsi_div_b + 1) << priv->mux_dsi_div_params.dsi_div_a > > return clk_hw->clk; > } Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected] In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
