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

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Tue, Apr 15, 2025 at 4:57 PM Geert Uytterhoeven <geert at linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> 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.
> >
> > 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>
>
> Thanks for your patch!
>
> > --- 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);
> > +
> > +       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;
> > +
> > +       /*
> > +        * 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;
>
> The first multiplication overflows on 32-bit systems.
> Probably you wanted to use mul_u32_u32() instead?
>
Ok, I'll switch to mul_u32_u32() .

> More review later, I fell too deep in the wrong rabbit hole ("mhz !=
> megaHertz"), again...
>
After fixing the review comments on this I'll send out a v3 with this
change, so that its easier to review.

> > --- /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.
> > + */
> > +
> > +#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 {
> > +       struct {
> > +               u64 min;
> > +               u64 max;
>
> u32 should be sufficient?
>
Agreed.

> > +       } 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 {
> > +               u8 min;
> > +               u8 max;
> > +       } csdiv;
> > +};
> > +
> > +struct rzv2h_plldsi_parameters {
> > +       u64 freq_mhz;
> > +       s64 error_mhz;
> > +       u16 m;
> > +       s16 k;
> > +       u8 csdiv;
> > +       u8 p;
> > +       u8 s;
> > +};
> > +
> > +#define RZV2H_CPG_PLL_DSI_LIMITS(name)                                 \
> > +       static const struct rzv2h_plldsi_div_limits (name) = {          \
> > +               .m = { .min = 64, .max = 533 },                         \
> > +               .p = { .min = 1, .max = 4 },                            \
> > +               .s = { .min = 0, .max = 6 },                            \
> > +               .k = { .min = -32768, .max = 32767 },                   \
> > +               .csdiv = { .min = 2, .max = 32 },                       \
> > +               .fvco = { .min = 1600 * MEGA, .max = 3200 * MEGA }      \
>
> Please initialize the members in declaration order.
>
Ok.

Cheers,
Prabhakar