Hi Geert,
Thank you for the review.
On Tue, Apr 15, 2025 at 4:57 PM Geert Uytterhoeven <ge...@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> 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.
> >
> > 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>
>
> 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
