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?
More review later, I fell too deep in the wrong rabbit hole ("mhz !=
megaHertz"), again...
> --- /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?
> + } 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.
> + } \
> +
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
