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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMuHMdVOvdU4ajB_f9OtQ8ao0Aodg+Rb9eGTmbNGC8o+aW-hzg@mail.gmail.com>
Date: Wed, 26 Jun 2024 12:06:51 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Prabhakar <prabhakar.csengg@...il.com>
Cc: Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Magnus Damm <magnus.damm@...il.com>, linux-renesas-soc@...r.kernel.org, 
	linux-clk@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [RFC PATCH v2 3/4] clk: renesas: Add family-specific clock driver
 for RZ/V2H(P)

Hi Prabhakar,

On Tue, Jun 11, 2024 at 1:32 AM Prabhakar <prabhakar.csengg@...il.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
>
> Add family-specific clock driver for RZ/V2H(P) SoCs.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> ---
> v1->v2
> - Introduced family specific config option
> - Now using register indexes for CLKON/CLKMON/RST/RSTMON
> - Introduced PLL_CONF macro
> - Dropped function pointer to get PLL_CLK1/2 offsets
> - Added range check for core clks
> - Dropped NULLified clocks check
> - Updated commit description

Thanks for the update!

> --- /dev/null
> +++ b/drivers/clk/renesas/rzv2h-cpg.c

> +/**
> + * struct rzv2h_cpg_priv - Clock Pulse Generator Private Data
> + *
> + * @rcdev: Reset controller entity
> + * @dev: CPG device
> + * @base: CPG register block base address
> + * @clks: Array containing all Core and Module Clocks
> + * @num_core_clks: Number of Core Clocks in clks[]
> + * @num_mod_clks: Number of Module Clocks in clks[]
> + * @num_resets: Number of Module Resets in info->resets[]
> + * @num_hw_resets: Number of resets supported by HW
> + * @last_dt_core_clk: ID of the last Core Clock exported to DT
> + * @info: Pointer to platform data
> + */
> +struct rzv2h_cpg_priv {
> +       struct reset_controller_dev rcdev;
> +       struct device *dev;
> +       void __iomem *base;
> +
> +       struct clk **clks;
> +       unsigned int num_core_clks;
> +       unsigned int num_mod_clks;
> +       unsigned int num_resets;
> +       unsigned int num_hw_resets;

This is not really used, so please drop it.

> +       unsigned int last_dt_core_clk;
> +
> +       const struct rzv2h_cpg_info *info;
> +};

> +static struct clk
> +*rzv2h_cpg_clk_src_twocell_get(struct of_phandle_args *clkspec,
> +                              void *data)
> +{
> +       unsigned int clkidx = clkspec->args[1];
> +       struct rzv2h_cpg_priv *priv = data;
> +       struct device *dev = priv->dev;
> +       const char *type;
> +       int range_check;
> +       struct clk *clk;
> +
> +       switch (clkspec->args[0]) {
> +       case CPG_CORE:
> +               type = "core";
> +               if (clkidx > priv->last_dt_core_clk) {
> +                       dev_err(dev, "Invalid %s clock index %u\n", type, clkidx);
> +                       return ERR_PTR(-EINVAL);
> +               }
> +               clk = priv->clks[clkidx];
> +               break;
> +
> +       case CPG_MOD:
> +               type = "module";
> +               range_check = 15 - (clkidx % 16);
> +               if (range_check < 0 || clkidx >= priv->num_mod_clks) {

range_check is never negative
(leftover from sparse number space?)

> +                       dev_err(dev, "Invalid %s clock index %u\n", type,
> +                               clkidx);
> +                       return ERR_PTR(-EINVAL);
> +               }
> +               clk = priv->clks[priv->num_core_clks + clkidx];
> +               break;
> +
> +       default:
> +               dev_err(dev, "Invalid CPG clock type %u\n", clkspec->args[0]);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       if (IS_ERR(clk))
> +               dev_err(dev, "Cannot get %s clock %u: %ld", type, clkidx,
> +                       PTR_ERR(clk));
> +       else
> +               dev_dbg(dev, "clock (%u, %u) is %pC at %lu Hz\n",
> +                       clkspec->args[0], clkspec->args[1], clk,
> +                       clk_get_rate(clk));
> +       return clk;
> +}

> +/**
> + * struct mod_clock - Module clock
> + *
> + * @hw: handle between common and hardware-specific interfaces
> + * @off: register offset
> + * @bit: ON/MON bit
> + * @monoff: monitor register offset
> + * @monbit: montor bit
> + * @priv: CPG private data
> + */
> +struct mod_clock {
> +       struct clk_hw hw;
> +       u8 on_index;
> +       u8 on_bit;
> +       u16 mon_index;
> +       u8 mon_bit;

I noticed clock on and clock mon bits are related.
Clock on bits use only the lower 16 bits in a register, while clock
monitor bits use all 32 bits, hence:

    mon_index = on_index / 2
    mon_bit = (on_index % 2) * 16 + on_bit

Except for clocks without monitor bits, and for CGC_SPI_clk_spi and
CGC_SPI_clk_spix2, which share an on-bit, but have separate mon-bits.
So you cannot use these formulas.

Reset bits do not have such a relationship, as resets marked reserved
are skipped in the reset monitoring bit range.


> +       struct rzv2h_cpg_priv *priv;
> +};
> +
> +#define to_mod_clock(_hw) container_of(_hw, struct mod_clock, hw)
> +
> +static int rzv2h_mod_clock_endisable(struct clk_hw *hw, bool enable)
> +{
> +       struct mod_clock *clock = to_mod_clock(hw);
> +       unsigned int reg = GET_CLK_ON_OFFSET(clock->on_index);
> +       struct rzv2h_cpg_priv *priv = clock->priv;
> +       u32 bitmask = BIT(clock->on_bit);
> +       struct device *dev = priv->dev;
> +       u32 value;
> +       int error;
> +
> +       dev_dbg(dev, "CLK_ON 0x%x/%pC %s\n", reg, hw->clk,
> +               enable ? "ON" : "OFF");
> +
> +       value = bitmask << 16;
> +       if (enable)
> +               value |= bitmask;
> +
> +       writel(value, priv->base + reg);
> +
> +       if (!enable)
> +               return 0;
> +
> +       reg = GET_CLK_MON_OFFSET(clock->mon_index);

What if a clock does not have a clock monitor bit?
Clock bits in registers CPG_CLKON_22 and later do not have corresponding
clock monitor bits.

> +       bitmask = BIT(clock->mon_bit);
> +       error = readl_poll_timeout_atomic(priv->base + reg, value,
> +                                         value & bitmask, 0, 10);
> +       if (error)
> +               dev_err(dev, "Failed to enable CLK_ON %p\n",
> +                       priv->base + reg);
> +
> +       return error;
> +}

> --- /dev/null
> +++ b/drivers/clk/renesas/rzv2h-cpg.h

> +/**
> + * struct rzv2h_cpg_info - SoC-specific CPG Description
> + *
> + * @core_clks: Array of Core Clock definitions
> + * @num_core_clks: Number of entries in core_clks[]
> + * @last_dt_core_clk: ID of the last Core Clock exported to DT
> + * @num_total_core_clks: Total number of Core Clocks (exported + internal)
> + *
> + * @mod_clks: Array of Module Clock definitions
> + * @num_mod_clks: Number of entries in mod_clks[]
> + * @num_hw_mod_clks: Number of Module Clocks supported by the hardware
> + *
> + * @resets: Array of Module Reset definitions
> + * @num_resets: Number of entries in resets[]
> + * @num_hw_resets: Number of resets supported by the hardware
> + *
> + * @crit_mod_clks: Array with Module Clock IDs of critical clocks that
> + *                 should not be disabled without a knowledgeable driver
> + * @num_crit_mod_clks: Number of entries in crit_mod_clks[]
> + */
> +struct rzv2h_cpg_info {
> +       /* Core Clocks */
> +       const struct cpg_core_clk *core_clks;
> +       unsigned int num_core_clks;
> +       unsigned int last_dt_core_clk;
> +       unsigned int num_total_core_clks;
> +
> +       /* Module Clocks */
> +       const struct rzv2h_mod_clk *mod_clks;
> +       unsigned int num_mod_clks;
> +       unsigned int num_hw_mod_clks;
> +
> +       /* Resets */
> +       const struct rzv2h_reset *resets;
> +       unsigned int num_resets;
> +       unsigned int num_hw_resets;

This is not really used, so please drop it.

> +
> +       /* Critical Module Clocks that should not be disabled */
> +       const unsigned int *crit_mod_clks;
> +       unsigned int num_crit_mod_clks;
> +};
> +
> +#endif /* __RENESAS_RZV2H_CPG_H__ */

The rest LGTM.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ