Hi Geert, Thank you for the review.
On Mon, Oct 13, 2025 at 3:26 PM Geert Uytterhoeven <[email protected]> wrote: > > On Thu, 9 Oct 2025 at 18:07, Prabhakar <[email protected]> wrote: > > From: Lad Prabhakar <[email protected]> > > > > Add support for PLLDSI and its post-dividers to the RZ/V2H CPG driver and > > export a set of helper APIs to allow other consumers (notably the DSI > > driver) to compute and select PLL parameter combinations. > > > > Introduce per-PLL-DSI state in the CPG private structure and implement > > clk ops and registration for PLLDSI and PLLDSI divider clocks. Implement > > rzv2h_cpg_plldsi_determine_rate and rzv2h_cpg_plldsi_set_rate to drive > > PLL programming via the new per-PLL state and add a plldsi divider clk > > with determine/set/recalc operations that cooperate with the PLL > > algorithm. > > > > Centralize PLL parameter types and limits by moving definitions into a > > shared header (include/linux/clk/renesas.h). Add struct rzv2h_pll_limits, > > struct rzv2h_pll_pars and struct rzv2h_pll_div_pars, plus the > > RZV2H_CPG_PLL_DSI_LIMITS() macro to declare DSI PLL limits. > > > > Provide two exported helper functions, rzv2h_get_pll_pars() and > > rzv2h_get_pll_divs_pars(), that perform iterative searches over PLL > > parameters (M, K, P, S) and optional post-dividers to find the best (or > > exact) match for a requested frequency. Export these helpers in the > > "RZV2H_CPG" namespace for use by external drivers. > > > > This change centralizes DSI PLL rate selection logic, prevents duplicate > > implementations in multiple drivers, and enables the DSI driver to > > request accurate PLL rates and program the hardware consistently. > > > > Co-developed-by: Fabrizio Castro <[email protected]> > > Signed-off-by: Fabrizio Castro <[email protected]> > > Signed-off-by: Lad Prabhakar <[email protected]> > > --- > > v9->v10: > > - Dropped rzv2h_get_pll_div_pars() helper and opencoded instead. > > - Dropped rzv2h_get_pll_dtable_pars() helper and opencoded instead. > > - Added dummy helpers rzv2h_get_pll_pars() and rzv2h_get_pll_divs_pars() > > in renesas.h for !CONFIG_CLK_RZV2H case. > > - Updated commit message. > > Thanks for the update! > > > --- a/drivers/clk/renesas/rzv2h-cpg.c > > +++ b/drivers/clk/renesas/rzv2h-cpg.c > > > +static int rzv2h_cpg_plldsi_div_determine_rate(struct clk_hw *hw, > > + struct clk_rate_request *req) > > +{ > > + struct rzv2h_plldsi_div_clk *dsi_div = to_plldsi_div_clk(hw); > > + struct pll_clk *pll_clk = to_pll(clk_hw_get_parent(hw)); > > + struct rzv2h_cpg_priv *priv = dsi_div->priv; > > + u8 table[RZV2H_MAX_DIV_TABLES] = { 0 }; > > + struct rzv2h_pll_div_pars *dsi_params; > > + struct rzv2h_pll_dsi_info *dsi_info; > > + const struct clk_div_table *div; > > + u64 rate_millihz; > > + unsigned int i; > > + > > + dsi_info = &priv->pll_dsi_info[pll_clk->pll.instance]; > > + dsi_params = &dsi_info->pll_dsi_parameters; > > + > > + rate_millihz = mul_u32_u32(req->rate, MILLI); > > + if (rate_millihz == dsi_params->div.error_millihz + > > dsi_params->div.freq_millihz) > > + goto exit_determine_rate; > > + > > + div = dsi_div->dtable; > > This belongs inside the for-initializer below. > Agreed. > > + i = 0; > > Ditto; or better: in the variable declaration at the top of the function. > Ok, I will move to the top. > > + for (; div->div; div++) { > > + if (i >= RZV2H_MAX_DIV_TABLES) > > + return -EINVAL; > > + table[i++] = div->div; > > + } > > + > > + if (!rzv2h_get_pll_divs_pars(dsi_info->pll_dsi_limits, dsi_params, > > table, i, > > + rate_millihz)) { > > + dev_err(priv->dev, "failed to determine rate for req->rate: > > %lu\n", > > + req->rate); > > + return -EINVAL; > > + } > > + > > +exit_determine_rate: > > + req->rate = DIV_ROUND_CLOSEST_ULL(dsi_params->div.freq_millihz, > > MILLI); > > + req->best_parent_rate = req->rate * dsi_params->div.divider_value; > > + dsi_info->req_pll_dsi_rate = req->best_parent_rate; > > + > > + return 0; > > +} > > The rest LGTM, so with the above fixed, and the field changes factored > out into a separate patch: Ok, I will move the field changes into a separate patch. > Reviewed-by: Geert Uytterhoeven <[email protected]> Cheers, Prabhakar
