Hi Geert, Thank you for the review.
On Mon, Oct 6, 2025 at 1:45 PM Geert Uytterhoeven <[email protected]> wrote: > > Hi Prabhakar, > > On Thu, 2 Oct 2025 at 18:17, Prabhakar <[email protected]> wrote: > > From: Lad Prabhakar <[email protected]> > > > > Add support for PLLDSI and its post-dividers in the RZ/V2H CPG driver and > > export helper APIs for use by the DSI driver. > > > > Introduce per-PLL-DSI state in the CPG private structure and provide a > > set of helper functions that find valid PLL parameter combinations for > > a requested frequency. The new helpers are rzv2h_get_pll_pars(), > > rzv2h_get_pll_div_pars(), rzv2h_get_pll_divs_pars() and > > rzv2h_get_pll_dtable_pars() and they are exported in the "RZV2H_CPG" > > namespace for use by other consumers (notably the DSI driver). These > > helpers perform iterative searches over PLL parameters (M, K, P, S) > > and optional post-dividers and return the best match (or an exact > > match when possible). > > > > Move PLL/CLK related limits and parameter types into the shared > > include (include/linux/clk/renesas.h) by adding struct rzv2h_pll_limits, > > struct rzv2h_pll_pars and struct rzv2h_pll_div_pars plus the > > RZV2H_CPG_PLL_DSI_LIMITS() helper macro to define DSI PLL limits. > > > > This change centralises the PLLDSI algorithms so the CPG and DSI > > drivers compute PLL parameters consistently and allows the DSI driver > > to accurately request rates and program its PLL. > > > > Co-developed-by: Fabrizio Castro <[email protected]> > > Signed-off-by: Fabrizio Castro <[email protected]> > > Signed-off-by: Lad Prabhakar <[email protected]> > > --- > > v8->v9: > > - Dropped `renesas-rzv2h-cpg-pll.h` header and merged into `renesas.h` > > - Exported the symbols for PLL calculation apis > > - Updated commit message > > - Dropped reviewed-by tags due to above changes > > Thanks for the update! > > > --- a/drivers/clk/renesas/rzv2h-cpg.c > > +++ b/drivers/clk/renesas/rzv2h-cpg.c > > > +/* > > + * rzv2h_get_pll_div_pars - Finds the best combination of PLL parameters > > + * and divider value for a given frequency. > > + * > > + * @limits: Pointer to the structure containing the limits for the PLL > > parameters > > + * @pars: Pointer to the structure where the best calculated PLL > > parameters and > > + * divider values will be stored > > + * @divider: Divider value to be applied to the PLL output > > + * @freq_millihz: Target output frequency in millihertz > > + * > > + * This function calculates the best set of PLL parameters (M, K, P, S) > > where > > + * the divider value is already known. See rzv2h_get_pll_pars() for more > > details > > + * on how the PLL parameters are calculated. > > + */ > > +bool rzv2h_get_pll_div_pars(const struct rzv2h_pll_limits *limits, > > + struct rzv2h_pll_div_pars *pars, u8 divider, > > + u64 freq_millihz) > > +{ > > + if (!rzv2h_get_pll_pars(limits, &pars->pll, freq_millihz * divider)) > > + return false; > > + > > + pars->div.divider_value = divider; > > + pars->div.freq_millihz = > > DIV_U64_ROUND_CLOSEST(pars->pll.freq_millihz, divider); > > + pars->div.error_millihz = freq_millihz - pars->div.freq_millihz; > > + > > + return true; > > +} > > +EXPORT_SYMBOL_NS_GPL(rzv2h_get_pll_div_pars, "RZV2H_CPG"); > > This function does not seem to be used outside this module yet, > so why is it exported? > Agreed, I will drop it. > If you do ever need it, you could define a simple wrapper in the > header file: > > static inline bool rzv2h_get_pll_div_pars(const struct > rzv2h_pll_limits *limits, > struct rzv2h_pll_div_pars *pars, > u8 divider, u64 freq_millihz) > { > return rzv2h_get_pll_divs_pars(limits, pars, ÷r, 1, > freq_millihz); > } > Agreed. > > + > > +/* > > + * rzv2h_get_pll_divs_pars - Finds the best combination of PLL parameters > > + * and divider value for a given frequency. > > + * > > + * @limits: Pointer to the structure containing the limits for the PLL > > parameters > > + * @pars: Pointer to the structure where the best calculated PLL > > parameters and > > + * divider values will be stored > > + * @table: Pointer to the array of valid divider values > > + * @table_size: Size of the divider values array > > + * @freq_millihz: Target output frequency in millihertz > > + * > > + * This function calculates the best set of PLL parameters (M, K, P, S) > > and divider > > + * value to achieve the desired frequency. See rzv2h_get_pll_pars() for > > more details > > + * on how the PLL parameters are calculated. > > + * > > + * freq_millihz is the desired frequency generated by the PLL followed by a > > + * a gear. > > + */ > > +bool rzv2h_get_pll_divs_pars(const struct rzv2h_pll_limits *limits, > > + struct rzv2h_pll_div_pars *pars, > > + const u8 *table, u8 table_size, u64 > > freq_millihz) > > +{ > > + struct rzv2h_pll_div_pars p, best; > > + > > + best.div.error_millihz = S64_MAX; > > + p.div.error_millihz = S64_MAX; > > + for (unsigned int i = 0; i < table_size; i++) { > > + if (!rzv2h_get_pll_div_pars(limits, &p, table[i], > > freq_millihz)) > > If you don't need rzv2h_get_pll_div_pars() elsewhere, you could just > expand it here. > Agreed, I will expand it here and drop rzv2h_get_pll_div_pars(). > > + continue; > > + > > + if (p.div.error_millihz == 0) { > > + *pars = p; > > + return true; > > + } > > + > > + if (abs(best.div.error_millihz) > abs(p.div.error_millihz)) > > + best = p; > > + } > > + > > + if (best.div.error_millihz == S64_MAX) > > + return false; > > + > > + *pars = best; > > + return true; > > +} > > +EXPORT_SYMBOL_NS_GPL(rzv2h_get_pll_divs_pars, "RZV2H_CPG"); > > + > > +/* > > + * rzv2h_get_pll_dtable_pars - Finds the best combination of PLL parameters > > + * and divider value for a given frequency using a divider table. > > + * > > + * @limits: Pointer to the structure containing the limits for the PLL > > parameters > > + * @pars: Pointer to the structure where the best calculated PLL > > parameters and > > + * divider values will be stored > > + * @dtable: Pointer to the array of valid divider values > > + * @freq_millihz: Target output frequency in millihertz > > + * > > + * See rzv2h_get_pll_divs_pars() for more details on how the PLL > > + * parameters and divider values are calculated. > > + */ > > +bool rzv2h_get_pll_dtable_pars(const struct rzv2h_pll_limits *limits, > > + struct rzv2h_pll_div_pars *pars, > > + const struct clk_div_table *dtable, u64 > > freq_millihz) > > +{ > > + const struct clk_div_table *div = dtable; > > + u8 table[RZV2H_MAX_DIV_TABLES] = { 0 }; > > + unsigned int i = 0; > > + > > + for (; div->div; div++) { > > + if (i >= RZV2H_MAX_DIV_TABLES) > > + return false; > > + table[i++] = div->div; > > + } > > + > > + return rzv2h_get_pll_divs_pars(limits, pars, table, i, > > freq_millihz); > > +} > > +EXPORT_SYMBOL_NS_GPL(rzv2h_get_pll_dtable_pars, "RZV2H_CPG"); > > This function does not seem to be used outside this module yet, > so why is it exported? > Agreed, I will drop it. > > + > > +static unsigned long rzv2h_cpg_plldsi_div_recalc_rate(struct clk_hw *hw, > > + unsigned long > > parent_rate) > > +{ > > + struct rzv2h_plldsi_div_clk *dsi_div = to_plldsi_div_clk(hw); > > + struct rzv2h_cpg_priv *priv = dsi_div->priv; > > + struct ddiv ddiv = dsi_div->ddiv; > > + u32 div; > > + > > + div = readl(priv->base + ddiv.offset); > > + div >>= ddiv.shift; > > + div &= clk_div_mask(ddiv.width); > > + div = dsi_div->dtable[div].div; > > + > > + return DIV_ROUND_CLOSEST_ULL(parent_rate, div); > > +} > > + > > +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; > > + struct rzv2h_pll_div_pars *dsi_params; > > + struct rzv2h_pll_dsi_info *dsi_info; > > + u64 rate_millihz; > > + > > + 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; > > + > > + if (!rzv2h_get_pll_dtable_pars(dsi_info->pll_dsi_limits, > > dsi_params, dsi_div->dtable, > > + rate_millihz)) { > > If you don't need rzv2h_get_pll_dtable_pars() elsewhere, you could just > expand it here. > Ok, I will expand it here. > > + 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; > > +} > > > --- a/include/linux/clk/renesas.h > > +++ b/include/linux/clk/renesas.h > > @@ -10,7 +10,9 @@ > > #ifndef __LINUX_CLK_RENESAS_H_ > > #define __LINUX_CLK_RENESAS_H_ > > > > +#include <linux/clk-provider.h> > > #include <linux/types.h> > > +#include <linux/units.h> > > > > struct device; > > struct device_node; > > @@ -32,4 +34,138 @@ void cpg_mssr_detach_dev(struct generic_pm_domain > > *unused, struct device *dev); > > #define cpg_mssr_attach_dev NULL > > #define cpg_mssr_detach_dev NULL > > #endif > > + > > +/** > > + * struct rzv2h_pll_limits - PLL parameter constraints > > + * > > + * This structure defines the minimum and maximum allowed values for > > + * various parameters used to configure a PLL. These limits ensure > > + * the PLL operates within valid and stable ranges. > > + * > > + * @fout: Output frequency range (in MHz) > > + * @fout.min: Minimum allowed output frequency > > + * @fout.max: Maximum allowed output frequency > > + * > > + * @fvco: PLL oscillation frequency range (in MHz) > > + * @fvco.min: Minimum allowed VCO frequency > > + * @fvco.max: Maximum allowed VCO frequency > > + * > > + * @m: Main-divider range > > + * @m.min: Minimum main-divider value > > + * @m.max: Maximum main-divider value > > + * > > + * @p: Pre-divider range > > + * @p.min: Minimum pre-divider value > > + * @p.max: Maximum pre-divider value > > + * > > + * @s: Divider range > > + * @s.min: Minimum divider value > > + * @s.max: Maximum divider value > > + * > > + * @k: Delta-sigma modulator range (signed) > > + * @k.min: Minimum delta-sigma value > > + * @k.max: Maximum delta-sigma value > > + */ > > +struct rzv2h_pll_limits { > > + struct { > > + u32 min; > > + u32 max; > > + } fout; > > + > > + struct { > > + u32 min; > > + u32 max; > > + } fvco; > > + > > + struct { > > + u16 min; > > + u16 max; > > + } m; > > + > > + struct { > > + u8 min; > > + u8 max; > > + } p; > > + > > + struct { > > + u8 min; > > + u8 max; > > + } s; > > + > > + struct { > > + s16 min; > > + s16 max; > > + } k; > > +}; > > + > > +/** > > + * struct rzv2h_pll_pars - PLL configuration parameters > > + * > > + * This structure contains the configuration parameters for the > > + * Phase-Locked Loop (PLL), used to achieve a specific output frequency. > > + * > > + * @m: Main divider value > > + * @p: Pre-divider value > > + * @s: Output divider value > > + * @k: Delta-sigma modulation value > > + * @freq_millihz: Calculated PLL output frequency in millihertz > > + * @error_millihz: Frequency error from target in millihertz (signed) > > + */ > > +struct rzv2h_pll_pars { > > + u16 m; > > + u8 p; > > + u8 s; > > + s16 k; > > + u64 freq_millihz; > > + s64 error_millihz; > > +}; > > + > > +/** > > + * struct rzv2h_pll_div_pars - PLL parameters with post-divider > > + * > > + * This structure is used for PLLs that include an additional post-divider > > + * stage after the main PLL block. It contains both the PLL configuration > > + * parameters and the resulting frequency/error values after the divider. > > + * > > + * @pll: Main PLL configuration parameters (see struct rzv2h_pll_pars) > > + * > > + * @div: Post-divider configuration and result > > + * @div.divider_value: Divider applied to the PLL output > > + * @div.freq_millihz: Output frequency after divider in millihertz > > + * @div.error_millihz: Frequency error from target in millihertz (signed) > > + */ > > +struct rzv2h_pll_div_pars { > > + struct rzv2h_pll_pars pll; > > + struct { > > + u8 divider_value; > > + u64 freq_millihz; > > + s64 error_millihz; > > + } div; > > +}; > > + > > +#define RZV2H_CPG_PLL_DSI_LIMITS(name) \ > > + static const struct rzv2h_pll_limits (name) = { \ > > + .fout = { .min = 25 * MEGA, .max = 375 * MEGA }, \ > > + .fvco = { .min = 1600 * MEGA, .max = 3200 * MEGA }, \ > > + .m = { .min = 64, .max = 533 }, \ > > + .p = { .min = 1, .max = 4 }, \ > > + .s = { .min = 0, .max = 6 }, \ > > + .k = { .min = -32768, .max = 32767 }, \ > > + } \ > > + > > +bool rzv2h_get_pll_pars(const struct rzv2h_pll_limits *limits, > > + struct rzv2h_pll_pars *pars, u64 freq_millihz); > > Please add a dummy returning false for the !CONFIG_CLK_RZV2H case. > Agreed, I will add a static inline helper returning false. > > + > > +bool rzv2h_get_pll_div_pars(const struct rzv2h_pll_limits *limits, > > + struct rzv2h_pll_div_pars *pars, u8 divider, > > + u64 freq_millihz); > > Unused, please drop. > OK. > > + > > +bool rzv2h_get_pll_divs_pars(const struct rzv2h_pll_limits *limits, > > + struct rzv2h_pll_div_pars *pars, > > + const u8 *table, u8 table_size, u64 > > freq_millihz); > > Please add a dummy returning false for the !CONFIG_CLK_RZV2H case. > Agreed, I will add a static inline helper returning false. > > + > > +bool rzv2h_get_pll_dtable_pars(const struct rzv2h_pll_limits *limits, > > + struct rzv2h_pll_div_pars *pars, > > + const struct clk_div_table *dtable, u64 > > freq_millihz); > > Unused, please drop. > Ok. Cheers, Prabhakar
