Hi Geert,
Thank you for the review.
On Wed, Apr 16, 2025 at 10:27 AM Geert Uytterhoeven
<ge...@linux-m68k.org> wrote:
>
> Hi Prabhakar, Fabrizio,
>
> Thanks for your patch!
>
> On Tue, 8 Apr 2025 at 22:09, Prabhakar <prabhakar.cse...@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad...@bp.renesas.com>
> >
> > Add support for PLLDSI and PLLDSI divider clocks.
> >
> > The `renesas-rzv2h-dsi.h` header file is added to share the PLL divider
> > algorithm between the CPG and DSI drivers.
>
> Please explain here why the DSI driver needs access to this algorithm.
>
> > Co-developed-by: Fabrizio Castro <fabrizio.castro...@renesas.com>
> > Signed-off-by: Fabrizio Castro <fabrizio.castro...@renesas.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad...@bp.renesas.com>
>
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
>
> > @@ -196,6 +225,253 @@ static int rzv2h_cpg_pll_clk_enable(struct clk_hw *hw)
> > return ret;
> > }
> >
> > +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 &= ((2 << ddiv.width) - 1);
>
> Shouldn't that "2" be "1"?
> GENMASK(ddiv.width - 1, 0), or even better: clk_div_mask(ddiv.width).
>
Agreed, I'll switch to 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 rzv2h_cpg_priv *priv = dsi_div->priv;
> > + struct rzv2h_plldsi_parameters *dsi_dividers =
> > &priv->plldsi_div_parameters;
> > + unsigned long long rate_mhz;
>
> u64?
OK.
> Please use "millihz" instead of "mhz" everywhere, so it becomes very
> clear this is really "mHz" and not "MHz".
>
OK.
> > +
> > + /*
> > + * Adjust the requested clock rate (`req->rate`) to ensure it falls
> > within
> > + * the supported range of 5.44 MHz to 187.5 MHz.
> > + */
> > + req->rate = clamp(req->rate, 5440000UL, 187500000UL);
> > +
> > + rate_mhz = req->rate * MILLI * 1ULL;
> > + if (rate_mhz == dsi_dividers->error_mhz + dsi_dividers->freq_mhz)
> > + goto exit_determine_rate;
> > +
> > + if (!rzv2h_dsi_get_pll_parameters_values(priv->dsi_limits,
> > + dsi_dividers, rate_mhz)) {
> > + dev_err(priv->dev,
> > + "failed to determine rate for req->rate: %lu\n",
> > + req->rate);
> > + return -EINVAL;
> > + }
> > +
> > +exit_determine_rate:
> > + req->best_parent_rate = req->rate * dsi_dividers->csdiv;
> > +
> > + return 0;
> > +};
> > +
> > +static int rzv2h_cpg_plldsi_div_set_rate(struct clk_hw *hw,
> > + unsigned long rate,
> > + 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 rzv2h_plldsi_parameters *dsi_dividers =
> > &priv->plldsi_div_parameters;
> > + struct ddiv ddiv = dsi_div->ddiv;
> > + const struct clk_div_table *clkt;
> > + u32 reg, shift, div;
> > +
> > + div = dsi_dividers->csdiv;
> > + for (clkt = dsi_div->dtable; clkt->div; clkt++) {
> > + if (clkt->div == div)
> > + break;
> > + }
> > +
> > + if (!clkt->div && !clkt->val)
> > + return -EINVAL;
>
> No need to check clkt->dev.
>
I'll drop this check and use a bool flag to determine if a div is found.
> > +
> > + shift = ddiv.shift;
> > + reg = readl(priv->base + ddiv.offset);
> > + reg &= ~(GENMASK(shift + ddiv.width, shift));
> > +
> > + writel(reg | (clkt->val << shift) |
> > + DDIV_DIVCTL_WEN(shift), priv->base + ddiv.offset);
> > +
> > + return 0;
>
> This function is very similar to the existing rzv2h_ddiv_set_rate().
> If you can't re-use it as-is, please consider factoring out the common
> part, or at least follow the same style of RMW-operation.
>
Ok, I'll follow the same RMW operation.
> > +};
>
>
> > +static long rzv2h_cpg_plldsi_round_rate(struct clk_hw *hw,
> > + unsigned long rate,
> > + unsigned long *parent_rate)
> > +{
> > + return clamp(rate, 25000000UL, 375000000UL);
>
> This only rounds rates outside the range from 25 to 375 MHz.
> What about rates between 25 and 375 MHz?
>
> > +}
> > +
> > +static unsigned long rzv2h_cpg_plldsi_clk_recalc_rate(struct clk_hw *hw,
> > + unsigned long
> > parent_rate)
> > +{
> > + struct pll_clk *pll_clk = to_pll(hw);
> > + struct rzv2h_cpg_priv *priv = pll_clk->priv;
> > + unsigned int val1, val2;
> > + u64 rate;
> > +
> > + val1 = readl(priv->base + CPG_PLL_CLK1(pll_clk->pll.offset));
> > + val2 = readl(priv->base + CPG_PLL_CLK2(pll_clk->pll.offset));
> > +
> > + rate = mul_u64_u32_shr(parent_rate, (CPG_PLL_CLK1_MDIV(val1) << 16)
> > +
> > + CPG_PLL_CLK1_KDIV(val1), 16 +
> > CPG_PLL_CLK2_SDIV(val2));
> > +
> > + return DIV_ROUND_CLOSEST_ULL(rate, CPG_PLL_CLK1_PDIV(val1));
>
> Can't you just reuse the existing rzv2h_cpg_pll_clk_recalc_rate()?
>
Agreed.
> > +}
> > +
> > +static int rzv2h_cpg_plldsi_set_rate(struct clk_hw *hw,
> > + unsigned long rate,
> > + unsigned long parent_rate)
> > +{
> > + struct pll_clk *pll_clk = to_pll(hw);
> > + struct rzv2h_cpg_priv *priv = pll_clk->priv;
> > + struct rzv2h_plldsi_parameters *dsi_dividers;
> > + struct pll pll = pll_clk->pll;
> > + u16 offset = pll.offset;
> > + u32 val;
> > + int ret;
> > +
> > + /* Put PLL into standby mode */
> > + writel(CPG_PLL_STBY_RESETB_WEN, priv->base + CPG_PLL_STBY(offset));
> > + ret = readl_poll_timeout_atomic(priv->base + CPG_PLL_MON(offset),
> > + val, !(val & CPG_PLL_MON_LOCK),
> > + 100, 2000);
> > + if (ret) {
> > + dev_err(priv->dev, "Failed to put PLLDSI into standby
> > mode");
> > + return ret;
> > + }
> > +
> > + dsi_dividers = &priv->plldsi_div_parameters;
> > + /* Output clock setting 1 */
> > + writel((dsi_dividers->k << 16) | (dsi_dividers->m << 6) |
> > (dsi_dividers->p),
> > + priv->base + CPG_PLL_CLK1(offset));
> > +
> > + /* Output clock setting 2 */
> > + val = readl(priv->base + CPG_PLL_CLK2(offset));
> > + writel((val & ~GENMASK(2, 0)) | dsi_dividers->s,
> > + priv->base + CPG_PLL_CLK2(offset));
> > +
> > + /* Put PLL to normal mode */
> > + writel(CPG_PLL_STBY_RESETB_WEN | CPG_PLL_STBY_RESETB,
> > + priv->base + CPG_PLL_STBY(offset));
> > +
> > + /* PLL normal mode transition, output clock stability check */
> > + ret = readl_poll_timeout_atomic(priv->base + CPG_PLL_MON(offset),
> > + val, (val & CPG_PLL_MON_LOCK),
> > + 100, 2000);
> > + if (ret) {
> > + dev_err(priv->dev, "Failed to put PLLDSI into normal mode");
> > + return ret;
> > + }
> > +
> > + return 0;
>
> This function could be reused for non-DSI PLLs?
>
Yes it could be reused, I'll rename this to rzv2h_cpg_pll_set_rate().
> > +};
> > +
> > +static const struct clk_ops rzv2h_cpg_plldsi_ops = {
> > + .recalc_rate = rzv2h_cpg_plldsi_clk_recalc_rate,
> > + .round_rate = rzv2h_cpg_plldsi_round_rate,
> > + .set_rate = rzv2h_cpg_plldsi_set_rate,
> > +};
> > +
> > +static struct clk * __init
> > +rzv2h_cpg_plldsi_clk_register(const struct cpg_core_clk *core,
> > + struct rzv2h_cpg_priv *priv)
> > +{
> > + void __iomem *base = priv->base;
> > + struct device *dev = priv->dev;
> > + struct clk_init_data init;
> > + const struct clk *parent;
> > + const char *parent_name;
> > + struct pll_clk *pll_clk;
> > + int ret;
> > +
> > + parent = priv->clks[core->parent];
> > + if (IS_ERR(parent))
> > + return ERR_CAST(parent);
> > +
> > + pll_clk = devm_kzalloc(dev, sizeof(*pll_clk), GFP_KERNEL);
> > + if (!pll_clk)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + parent_name = __clk_get_name(parent);
> > + init.name = core->name;
> > + init.ops = &rzv2h_cpg_plldsi_ops;
> > + init.flags = 0;
> > + init.parent_names = &parent_name;
> > + init.num_parents = 1;
> > +
> > + pll_clk->hw.init = &init;
> > + pll_clk->pll = core->cfg.pll;
> > + pll_clk->base = base;
> > + pll_clk->priv = priv;
> > +
> > + /* Disable SSC and turn on PLL clock when init */
> > + writel(CPG_PLL_STBY_RESETB_WEN | CPG_PLL_STBY_RESETB |
> > + CPG_PLL_STBY_SSCGEN_WEN, base +
> > CPG_PLL_STBY(pll_clk->pll.offset));
>
> Apart from the three lines above, this function does the same as the
> existing rzv2h_cpg_pll_clk_register(). Merge/reuse?
>
Agreed, I'll reuse this function and introduce a bool flag to turn on the PLL.
> > +
> > + ret = devm_clk_hw_register(dev, &pll_clk->hw);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + return pll_clk->hw.clk;
> > +}
> > +
> > static unsigned long rzv2h_cpg_pll_clk_recalc_rate(struct clk_hw *hw,
> > unsigned long
> > parent_rate)
> > {
>
> > --- /dev/null
> > +++ b/include/linux/clk/renesas-rzv2h-dsi.h
> > @@ -0,0 +1,207 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Renesas RZ/V2H(P) DSI CPG helper
> > + *
> > + * Copyright (C) 2025 Renesas Electronics Corp.
> > + */
>
> Missing include guard.
>
Ouch, Ill add one.
> > +
> > +#include <linux/limits.h>
> > +#include <linux/math.h>
> > +#include <linux/math64.h>
> > +#include <linux/units.h>
> > +
> > +#define OSC_CLK_IN_MEGA (24 * MEGA)
> > +
> > +struct rzv2h_plldsi_div_limits {
>
> This structure looks applicable to all RZ/V2H PLLs, so perhaps drop the
> "dsi" part from the name?
>
Agreed.
Cheers,
Prabhakar
