| 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
| ||
|
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