| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+V-a8tCtVQA3DnzKMy+4gZDwZqhg29E0H+7cS0YojFkQ2Os_g@mail.gmail.com>
Date: Fri, 18 Apr 2025 10:51:29 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>,
Robert Foss <rfoss@...nel.org>, Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Biju Das <biju.das.jz@...renesas.com>,
Tomi Valkeinen <tomi.valkeinen+renesas@...asonboard.com>,
Geert Uytterhoeven <geert+renesas@...der.be>, Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>,
Magnus Damm <magnus.damm@...il.com>, dri-devel@...ts.freedesktop.org,
devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
linux-renesas-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
Tommaso Merciai <tommaso.merciai.xr@...renesas.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v2 01/15] clk: renesas: rzv2h-cpg: Add support for DSI clocks
Hi Geert,
Thank you for the review.
On Tue, Apr 15, 2025 at 4:57 PM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, 8 Apr 2025 at 22:09, Prabhakar <prabhakar.csengg@...il.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...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@...esas.com>
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@...esas.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...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
Powered by blists - more mailing lists