| 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: <CAMuHMdVvHG+UBBwWbBgiFh10u-sAYj_sONj6YPh4c5De2gyJOQ@mail.gmail.com>
Date: Tue, 13 May 2025 14:09:46 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Thierry Bultel <thierry.bultel.yh@...renesas.com>
Cc: thierry.bultel@...atsea.fr, linux-renesas-soc@...r.kernel.org,
paul.barker.ct@...renesas.com, linux-kernel@...r.kernel.org,
linux-clk@...r.kernel.org
Subject: Re: [PATCH v8 06/11] clk: renesas: Add support for R9A09G077 SoC
Hi Thierry,
On Tue, 29 Apr 2025 at 10:20, Thierry Bultel
<thierry.bultel.yh@...renesas.com> wrote:
> RZ/T2H has 2 register blocks at different addresses.
>
> The clock tree has configurable dividers and mux selectors.
> Add these new clock types, new register layout type, and
> registration code for mux and div in registration callback.
>
> Signed-off-by: Thierry Bultel <thierry.bultel.yh@...renesas.com>
> ---
> Changes v7->v8:
> - Makefile: keep ordered list
> - r9a09g077-cpg-mssr.c: use high bit instead of sel_base,
> same macro for DIV and MUX
> - removed unused clocks
> - CLK_LOCO is internal with a DEF_RATE definition
> - added CLK_PLL4D1 & CLK_SCI0ASYNC
> - added per-CA55 clocks
> - added missing error check in r9a09g077_cpg_mux_clk_register
> - fixed num_hw_mod_clks to 14
> - added missing 2 holes in mstpcr_for_rzt2h
> - renamed cpg_read_rzt2h_mstp_from_offset to cpg_read_rzt2h_mstp,
> directly reads at calculated address
> - added cpg_write_rzt2h_mstp and call in cpg_mstp_clock_endisable
> - do not register reset controller in case of CLK_REG_LAYOUT_RZ_T2H
> - moved CLK_DIV & CLK_MUX definitions to RZT2H specifics
Thanks for the update!
> index 000000000000..029619a6cb19
> --- /dev/null
> +++ b/drivers/clk/renesas/r9a09g077-cpg-mssr.c
r9a09g077-cpg.c
> @@ -0,0 +1,252 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * r9a09g077 Clock Pulse Generator / Module Standby and Software Reset
> + *
> + * Copyright (C) 2025 Renesas Electronics Corp.
> + *
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +
> +#include <dt-bindings/clock/renesas,r9a09g077-cpg-mssr.h>
> +#include "renesas-cpg-mssr.h"
> +
> +#define RZT2H_REG_BLOCK_SHIFT 11
> +#define RZT2H_REG_OFFSET_MASK GENMASK(10, 0)
> +#define RZT2H_REG_CONF(block, offset) (((block) << RZT2H_REG_BLOCK_SHIFT) | \
> + ((offset) & RZT2H_REG_OFFSET_MASK))
> +
> +#define RZT2H_REG_BLOCK(x) ((x) >> RZT2H_REG_BLOCK_SHIFT)
> +#define RZT2H_REG_OFFSET(x) ((x) & RZT2H_REG_OFFSET_MASK)
> +
> +#define SCKCR RZT2H_REG_CONF(0, 0x00)
> +#define SCKCR2 RZT2H_REG_CONF(1, 0x04)
> +#define SCKCR3 RZT2H_REG_CONF(0, 0x08)
> +
> +#define OFFSET_MASK GENMASK(31, 20)
> +#define SHIFT_MASK GENMASK(19, 12)
> +#define WIDTH_MASK GENMASK(11, 8)
> +
> +#define CONF_PACK(offset, shift, width) \
> + (FIELD_PREP_CONST(OFFSET_MASK, (offset)) | \
> + FIELD_PREP_CONST(SHIFT_MASK, (shift)) | \
> + FIELD_PREP_CONST(WIDTH_MASK, (width)))
> +
> +#define GET_SHIFT(val) FIELD_GET(SHIFT_MASK, val)
> +#define GET_WIDTH(val) FIELD_GET(WIDTH_MASK, val)
> +#define GET_REG_OFFSET(val) FIELD_GET(OFFSET_MASK, val)
> +
> +#define DIVCA55C0 CONF_PACK(SCKCR2, 8, 1)
> +#define DIVCA55C1 CONF_PACK(SCKCR2, 9, 1)
> +#define DIVCA55C2 CONF_PACK(SCKCR2, 10, 1)
> +#define DIVCA55C3 CONF_PACK(SCKCR2, 11, 1)
> +#define DIVCA55S CONF_PACK(SCKCR2, 12, 1)
> +
> +#define DIVSCI0ASYNC CONF_PACK(SCKCR3, 6, 1)
This field holds two bits, not one.
> +
> +#define SEL_PLL CONF_PACK(SCKCR, 22, 1)
> +
> +
> +enum rzt2h_clk_types {
> + CLK_TYPE_RZT2H_DIV = CLK_TYPE_CUSTOM, /* Clock with divider */
> + CLK_TYPE_RZT2H_MUX, /* Clock with clock source selector */
> +};
> +
> +#define DEF_DIV(_name, _id, _parent, _conf, _dtable, _flag) \
> + DEF_TYPE(_name, _id, CLK_TYPE_RZT2H_DIV, .conf = _conf, \
> + .parent = _parent, .dtable = _dtable, .flag = _flag)
The _flag parameter is always zero?
> +#define DEF_MUX(_name, _id, _conf, _parent_names, _num_parents, _flag, \
> + _mux_flags) \
> + DEF_TYPE(_name, _id, CLK_TYPE_RZT2H_MUX, .conf = _conf, \
> + .parent_names = _parent_names, .num_parents = _num_parents, \
> + .flag = _flag, .mux_flags = _mux_flags)
The _flag parameter is always zero (see below)?
> +
> +enum clk_ids {
> + /* Core Clock Outputs exported to DT */
> + LAST_DT_CORE_CLK = R9A09G077_CLK_PCLKM,
> +
> + /* External Input Clocks */
> + CLK_EXTAL,
> +
> + /* Internal Core Clocks */
> + CLK_LOCO,
> + CLK_MAIN,
Unused
> + CLK_PLL0,
> + CLK_PLL1,
> + CLK_PLL4,
> + CLK_SEL_PLL0,
> + CLK_SEL_CLK_PLL0,
> + CLK_SEL_PLL1,
> + CLK_SEL_CLK_PLL1,
> + CLK_SEL_PLL4,
> + CLK_SEL_CLK_PLL4,
> + CLK_PLL4D1,
> + CLK_SCI0ASYNC,
> +
> + /* Module Clocks */
> + MOD_CLK_BASE,
> +};
> +
> +static const struct clk_div_table dtable_1_2[] = {
> + {0, 2},
> + {1, 1},
> + {0, 0},
> +};
> +
> +static const struct clk_div_table dtable_24_25_30_32[] = {
> + {0, 24},
> + {0, 25},
> + {0, 30},
> + {0, 32},
The first value of each tuple must contain the register bit field value,
and usually the tables are sorted by value (although the code doesn't
seem to rely on that):
{0, 32},
{1, 30},
{2, 25},
{3, 24},
> + {0, 0},
> +};
> +
> +/* Mux clock tables */
> +
> +static const char * const sel_clk_pll0[] = { "loco", ".sel_pll0" };
> +static const char * const sel_clk_pll1[] = { "loco", ".sel_pll1" };
> +static const char * const sel_clk_pll4[] = { "loco", ".sel_pll4" };
".loco", as this is an internal clock.
> +
> +static const struct cpg_core_clk r9a09g077_core_clks[] __initconst = {
> + /* External Clock Inputs */
> + DEF_INPUT("extal", CLK_EXTAL),
> +
> + /* Internal Core Clocks */
> + DEF_RATE("loco", CLK_LOCO, 1000 * 1000),
".loco", as this is an internal clock.
> + DEF_FIXED(".pll0", CLK_PLL0, CLK_EXTAL, 1, 48),
> + DEF_FIXED(".pll1", CLK_PLL1, CLK_EXTAL, 1, 40),
> + DEF_FIXED(".pll4", CLK_PLL4, CLK_EXTAL, 1, 96),
> + /* unimplemented CLMA0 selector */
> + DEF_FIXED(".sel_pll0", CLK_SEL_PLL0, CLK_PLL0, 1, 1),
Will there ever be a need to implement these CLMAx selectors?
If not, you can just drop all SEL_PLLx clocks with 1/1 mul/div.
IIUIC, the CPG will switch automatically to clocks derived from the
LOCO if an anomaly is detected, yielding the same clock frequencies
as during normal operation (albeit with less precision)?
> + DEF_MUX(".sel_clk_pll0", CLK_SEL_CLK_PLL0, SEL_PLL,
> + sel_clk_pll0, ARRAY_SIZE(sel_clk_pll0), 0, CLK_MUX_READ_ONLY),
> + /* unimplemented CLMA1 selector */
> + DEF_FIXED(".sel_pll1", CLK_SEL_PLL1, CLK_PLL1, 1, 1),
> + DEF_MUX(".sel_clk_pll1", CLK_SEL_CLK_PLL1, SEL_PLL,
> + sel_clk_pll1, ARRAY_SIZE(sel_clk_pll1), 0, CLK_MUX_READ_ONLY),
> + /* unimplemented CLMA4 selector */
> + DEF_FIXED(".sel_pll4", CLK_SEL_PLL4, CLK_PLL4, 1, 1),
> + DEF_MUX(".sel_clk_pll4", CLK_SEL_CLK_PLL4, SEL_PLL,
> + sel_clk_pll4, ARRAY_SIZE(sel_clk_pll4), 0, CLK_MUX_READ_ONLY),
> +
> + DEF_FIXED(".pll4d1", CLK_PLL4D1, CLK_SEL_CLK_PLL4, 1, 1),
> + DEF_DIV(".sci0async", CLK_SCI0ASYNC, CLK_PLL4D1, DIVSCI0ASYNC,
> + dtable_24_25_30_32, CLK_DIVIDER_HIWORD_MASK),
CLK_DIVIDER_HIWORD_MASK is wrong, as there is no mask in
the higher 16-bit of the SCKCR3 register...
> +
> + /* Core output clk */
> + DEF_DIV("CA55C0", R9A09G077_CLK_CA55C0, CLK_SEL_CLK_PLL0, DIVCA55C0,
> + dtable_1_2, CLK_DIVIDER_HIWORD_MASK),
... nor in the SCKCR2 register.
> + DEF_DIV("CA55C1", R9A09G077_CLK_CA55C1, CLK_SEL_CLK_PLL0, DIVCA55C1,
> + dtable_1_2, CLK_DIVIDER_HIWORD_MASK),
> + DEF_DIV("CA55C2", R9A09G077_CLK_CA55C2, CLK_SEL_CLK_PLL0, DIVCA55C2,
> + dtable_1_2, CLK_DIVIDER_HIWORD_MASK),
> + DEF_DIV("CA55C3", R9A09G077_CLK_CA55C3, CLK_SEL_CLK_PLL0, DIVCA55C3,
> + dtable_1_2, CLK_DIVIDER_HIWORD_MASK),
> + DEF_DIV("CA55S", R9A09G077_CLK_CA55S, CLK_SEL_CLK_PLL0, DIVCA55S,
> + dtable_1_2, CLK_DIVIDER_HIWORD_MASK),
> + DEF_FIXED("PCLKGPTL", R9A09G077_CLK_PCLKGPTL, CLK_SEL_CLK_PLL1, 2, 1),
> + DEF_FIXED("PCLKM", R9A09G077_CLK_PCLKM, CLK_SEL_CLK_PLL1, 8, 1),
> +};
> +
> +static const struct mssr_mod_clk r9a09g077_mod_clks[] __initconst = {
> + DEF_MOD("sci0fck", R9A09G077_PCLK_SCI0, CLK_SCI0ASYNC),
> +};
> +
> +static struct clk * __init
> +r9a09g077_cpg_div_clk_register(struct device *dev,
> + const struct cpg_core_clk *core,
> + void __iomem *addr, struct cpg_mssr_pub *pub)
> +{
> + const struct clk *parent;
> + const char *parent_name;
> + struct clk_hw *clk_hw;
> +
> + parent = pub->clks[core->parent];
> +
Please drop this blank line.
> + if (IS_ERR(parent))
> + return ERR_CAST(parent);
> +static struct clk * __init
> +r9a09g077_cpg_clk_register(struct device *dev, const struct cpg_core_clk *core,
> + const struct cpg_mssr_info *info,
> + struct cpg_mssr_pub *pub)
> +{
> + u32 offset = GET_REG_OFFSET(core->conf);
> + void __iomem *base = RZT2H_REG_BLOCK(offset) ? pub->base1 : pub->base0;
> + void __iomem *addr = base + offset;
... + RZT2H_REG_OFFSET(offset);
> +
> + switch (core->type) {
> + case CLK_TYPE_RZT2H_DIV:
> + return r9a09g077_cpg_div_clk_register(dev, core, addr, pub);
> + case CLK_TYPE_RZT2H_MUX:
> + return r9a09g077_cpg_mux_clk_register(dev, core, addr, pub);
> + default:
> + return ERR_PTR(-EINVAL);
> + }
> +}
> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> @@ -79,6 +79,37 @@ static const u16 mstpcr_for_gen4[] = {
> 0x2D60, 0x2D64, 0x2D68, 0x2D6C, 0x2D70, 0x2D74,
> };
>
> +/*
> + * Module Stop Control Register (RZ/T2H)
> + * RZ/T2H has 2 registers blocks,
> + * Bit 12 is used to differentiate them
> + */
> +
> +#define RZT2H_MSTPCR_BLOCK_SHIFT 12
> +#define RZT2H_MSTPCR_OFFSET_MASK GENMASK(11, 0)
> +#define RZT2H_MSTPCR(block, offset) (((block) << RZT2H_MSTPCR_BLOCK_SHIFT) | \
> + ((offset) & RZT2H_MSTPCR_OFFSET_MASK))
> +
> +#define RZT2H_MSTPCR_BLOCK(x) ((x) >> RZT2H_MSTPCR_BLOCK_SHIFT)
> +#define RZT2H_MSTPCR_OFFSET(x) ((x) & RZT2H_MSTPCR_OFFSET_MASK)
> @@ -187,6 +218,26 @@ struct mstp_clock {
>
> #define to_mstp_clock(_hw) container_of(_hw, struct mstp_clock, hw)
>
> +static u32 cpg_read_rzt2h_mstp(struct clk_hw *hw, u16 offset)
rzt2h_mstpcr_read(), to match the naming of the helper macros above?
> +{
> + struct mstp_clock *clock = to_mstp_clock(hw);
> + struct cpg_mssr_priv *priv = clock->priv;
> + void __iomem *base =
> + RZT2H_MSTPCR_BLOCK(offset) ? priv->pub.base1 : priv->pub.base0;
> +
> + return readl(base + RZT2H_MSTPCR_OFFSET(offset));
> +}
> +
> +static void cpg_write_rzt2h_mstp(struct clk_hw *hw, u16 offset, u32 value)
rzt2h_mstpcr_write()?
> +{
> + struct mstp_clock *clock = to_mstp_clock(hw);
> + struct cpg_mssr_priv *priv = clock->priv;
> + void __iomem *base =
> + RZT2H_MSTPCR_BLOCK(offset) ? priv->pub.base1 : priv->pub.base0;
> +
> + writel(value, base + RZT2H_MSTPCR_OFFSET(offset));
> +}
> +
> static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
> {
> struct mstp_clock *clock = to_mstp_clock(hw);
> @@ -215,6 +266,19 @@ static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
> readb(priv->pub.base0 + priv->control_regs[reg]);
> barrier_data(priv->pub.base0 + priv->control_regs[reg]);
>
> + } else if (priv->reg_layout == CLK_REG_LAYOUT_RZ_T2H) {
> + pr_info("RZTH2 case");
Please drop this pr_info() call.
> + value = cpg_read_rzt2h_mstp(hw,
> + priv->control_regs[reg]);
> +
> + if (enable)
> + value &= ~bitmask;
> + else
> + value |= bitmask;
> +
> + cpg_write_rzt2h_mstp(hw,
> + priv->control_regs[reg],
> + value);
> } else {
> value = readl(priv->pub.base0 + priv->control_regs[reg]);
> if (enable)
> @@ -1064,6 +1138,13 @@ static int __init cpg_mssr_common_init(struct device *dev,
> error = -ENOMEM;
> goto out_err;
> }
> + if (info->reg_layout == CLK_REG_LAYOUT_RZ_T2H) {
> + priv->pub.base1 = of_iomap(np, 1);
> + if (!priv->pub.base1) {
> + error = -ENOMEM;
> + goto out_err;
> + }
> + }
>
> priv->num_core_clks = info->num_total_core_clks;
> priv->num_mod_clks = info->num_hw_mod_clks;
> @@ -1077,6 +1158,8 @@ static int __init cpg_mssr_common_init(struct device *dev,
> priv->reset_clear_regs = srstclr;
> } else if (priv->reg_layout == CLK_REG_LAYOUT_RZ_A) {
> priv->control_regs = stbcr;
> + } else if (priv->reg_layout == CLK_REG_LAYOUT_RZ_T2H) {
> + priv->control_regs = mstpcr_for_rzt2h;
> } else if (priv->reg_layout == CLK_REG_LAYOUT_RCAR_GEN4) {
> priv->status_regs = mstpsr_for_gen4;
> priv->control_regs = mstpcr_for_gen4;
> @@ -1107,6 +1190,8 @@ static int __init cpg_mssr_common_init(struct device *dev,
> out_err:
> if (priv->pub.base0)
> iounmap(priv->pub.base0);
> + if (priv->pub.base1)
> + iounmap(priv->pub.base1);
> kfree(priv);
>
> return error;
> @@ -1171,7 +1256,8 @@ static int __init cpg_mssr_probe(struct platform_device *pdev)
> goto reserve_exit;
>
> /* Reset Controller not supported for Standby Control SoCs */
> - if (priv->reg_layout == CLK_REG_LAYOUT_RZ_A)
> + if (priv->reg_layout == CLK_REG_LAYOUT_RZ_A ||
> + priv->reg_layout == CLK_REG_LAYOUT_RZ_T2H)
> goto reserve_exit;
>
> error = cpg_mssr_reset_controller_register(priv);
> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.h b/drivers/clk/renesas/renesas-cpg-mssr.h
> index 7ce3cc9a64c1..2801d6bf2f6d 100644
> --- a/drivers/clk/renesas/renesas-cpg-mssr.h
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.h
> @@ -22,6 +22,10 @@
> struct cpg_core_clk {
> /* Common */
> const char *name;
> + union {
> + const char * const *parent_names;
> + const struct clk_div_table *dtable;
> + };
Please move them below, as they are type-specific.
> unsigned int id;
> unsigned int type;
> /* Depending on type */
> @@ -29,18 +33,24 @@ struct cpg_core_clk {
> unsigned int div;
> unsigned int mult;
> unsigned int offset;
> + u32 conf;
> + u16 flag;
> + u8 mux_flags;
> + u8 num_parents;
> };
>
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