[PATCH v2 01/15] clk: renesas: rzv2h-cpg: Add support for DSI clocks

[Lad, Prabhakar]

Hi Geert,

On Wed, Apr 16, 2025 at 10:27 AM Geert Uytterhoeven
<geert at linux-m68k.org> wrote:
>
> Hi Prabhakar, Fabrizio,
>
> Thanks for your patch!
>
> On Tue, 8 Apr 2025 at 22:09, Prabhakar <prabhakar.csengg at gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj at 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.jz at renesas.com>
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz at renesas.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj at 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).
>
> > +
> > +       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?
> Please use "millihz" instead of "mhz" everywhere, so it becomes very
> clear this is really "mHz" and not "MHz".
>
> > +
> > +       /*
> > +        * 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.
>
> > +
> > +       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.
>
> > +};
>
>
> > +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?
>
My intention was to clamp it and let the PLL DSI DIV determine_rate
handle rejecting the rates if they dont get best dividers. Let's take
this discussion to v3.

Cheers,
Prabhakar