Re: [PATCH v7 06/13] clk: renesas: Add support for R9A09G077 SoC

[Geert Uytterhoeven]

Hi Thierry,

Thanks for your patch!

On Thu, 3 Apr 2025 at 23:30, Thierry Bultel
<thierry.bultel.yh@xxxxxxxxxxxxxx> wrote:
> RZ/T2H has 2 registers blocks at different addresses.

register

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

> --- a/drivers/clk/renesas/Makefile
> +++ b/drivers/clk/renesas/Makefile
> @@ -31,6 +31,7 @@ obj-$(CONFIG_CLK_R8A779A0)            += r8a779a0-cpg-mssr.o
>  obj-$(CONFIG_CLK_R8A779F0)             += r8a779f0-cpg-mssr.o
>  obj-$(CONFIG_CLK_R8A779G0)             += r8a779g0-cpg-mssr.o
>  obj-$(CONFIG_CLK_R8A779H0)             += r8a779h0-cpg-mssr.o
> +obj-$(CONFIG_CLK_R9A09G077)            += r9a09g077-cpg-mssr.o

Please keep the list sorted.

>  obj-$(CONFIG_CLK_R9A06G032)            += r9a06g032-clocks.o
>  obj-$(CONFIG_CLK_R9A07G043)            += r9a07g043-cpg.o
>  obj-$(CONFIG_CLK_R9A07G044)            += r9a07g044-cpg.o
> diff --git a/drivers/clk/renesas/r9a09g077-cpg-mssr.c b/drivers/clk/renesas/r9a09g077-cpg-mssr.c
> new file mode 100644
> index 000000000000..b67dbf5d59d8
> --- /dev/null
> +++ b/drivers/clk/renesas/r9a09g077-cpg-mssr.c
> @@ -0,0 +1,238 @@
> +// 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 SCKCR          0x00
> +#define SCKCR2         0x04

Please add comments to indicate whether an offset is against base0
(e.g. SCKCR) or base1 (e.g. SCKCR2).

Perhaps encode it in a high-bit, like for the MSTPCRx registers, instead
of storing it separately in .sel_base, which is more error-prone?

> +#define SCKCR3         0x08
> +#define SCKCR4         0x0C
> +#define PMSEL          0x10
> +#define PMSEL_PLL0     BIT(0)
> +#define PMSEL_PLL2     BIT(2)
> +#define PMSEL_PLL3     BIT(3)
> +#define PLL0EN         BIT(0)
> +#define PLL2EN         BIT(0)
> +#define PLL3EN         BIT(0)

These 6 bit definitions don't match the documentation (and are unused?)

> +#define PLL0MON                0x20
> +#define PLL0EN_REG     0x30

Why the "_REG"-suffix? Unused, anyway.

> +#define PLL0_SSC_CTR   0x34
> +#define PLL1MON                0x40
> +#define LOCOCR         0x70
> +#define HIZCTRLEN      0x80
> +#define PLL2MON                0x90
> +#define PLL2EN_REG     0xA0
> +#define PLL2_SSC_CTR   0xAC
> +#define PLL3MON                0xB0
> +#define PLL3EN_REG     0xC0
> +#define PLL3_VCO_CTR0  0xC4
> +#define PLL3_VCO_CTR1  0xC8
> +#define PLL4MON                0xD0
> +#define PHYSEL         BIT(21)

SCKCR_PHYSEL? So it would belong under SCKCR.
But is unused, as you have the DIVETHPHY macro.

> +
> +#define MRCTLA         0x240
> +#define MRCTLE         0x250
> +#define MRCTLI         0x260
> +#define MRCTLM         0x270

Unused, these should end up in srcr_for_rzt2h[] in renesas-cpg-mssr.c
when adding reset controller support for RZ/T2H.

Until then, the call to cpg_mssr_reset_controller_register()
in cpg_mssr_probe() should be skipped when priv->reg_layout ==
CLK_REG_LAYOUT_RZ_T2H.

> +
> +#define DDIV_PACK(offset, bitpos, size) \
> +               (((offset) << 20) | ((bitpos) << 12) | ((size) << 8))

Indented by 2 TABs...

> +
> +#define DIVCA55                DDIV_PACK(SCKCR2, 8, 4)

This is not correct: these 4 bits are not a single bit mask, but 4
individual bits, one for each CPU core.
Oh, due to dtable_1_2[] just having values "0" and "15", all 4 CPU
cores are always clocked at the same rate. Hmm.....

> +#define DIVCA55S       DDIV_PACK(SCKCR2, 12, 1)
> +#define DIVCR520       DDIV_PACK(SCKCR2, 2, 2)
> +#define DIVCR521       DDIV_PACK(SCKCR2, 0, 2)

Surprisingly, you do handle the 2 CR52 cores individually ;-)
Unused, anyway...

> +#define DIVLCDC                DDIV_PACK(SCKCR3, 20, 3)

Should be "4" instead of "3".

> +#define DIVCKIO                DDIV_PACK(SCKCR, 16, 3)
> +#define DIVETHPHY      DDIV_PACK(SCKCR, 21, 1)
> +#define DIVCANFD       DDIV_PACK(SCKCR, 20, 1)
> +#define DIVSPI0                DDIV_PACK(SCKCR3, 0, 2)
> +#define DIVSPI1                DDIV_PACK(SCKCR3, 2, 2)
> +#define DIVSPI2                DDIV_PACK(SCKCR3, 4, 2)
> +#define DIVSPI3                DDIV_PACK(SCKCR2, 16, 2)

Perhaps sort these definitions by SCKCR* register?

> +
> +#define SEL_PLL_PACK(offset, bitpos, size) \
> +       (((offset) << 20) | ((bitpos) << 12) | ((size) << 8))

... indented by 1 TAB.  Please match them.

> +
> +#define SEL_PLL                SEL_PLL_PACK(SCKCR, 22, 1)
> +
> +#define GET_SHIFT(val)         FIELD_GET(GENMASK(19, 12), val)
> +#define GET_WIDTH(val)         FIELD_GET(GENMASK(11, 8), val)
> +#define GET_REG_OFFSET(val)    FIELD_GET(GENMASK(31, 20), val)

Please use consistent naming:
    SHIFT ~ bitpos
    WIDTH ~ size
    REG_OFFSET ~ offset

If you would use FIELD_*() helpers for both, you could write the above as e.g.

    #define OFFSET_MASK GENMASK(31, 20)
    #define SHIFT_MASK  GENMASK(19, 12)
    #define WIDTH_MASK  GENMASK(11, 8)

   #define SEL_PLL_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)

> +
> +enum clk_ids {
> +       /* Core Clock Outputs exported to DT */
> +       LAST_DT_CORE_CLK = R9A09G077_LCDC_CLKD,
> +
> +       /* External Input Clocks */
> +       CLK_EXTAL,
> +       CLK_LOCO,

This is an internally-generated clock.

> +
> +       /* Internal Core Clocks */
> +       CLK_PLL0,
> +       CLK_PLL1,
> +       CLK_PLL2,
> +       CLK_PLL3,
> +       CLK_PLL4,
> +       CLK_SEL_PLL0,
> +       CLK_SEL_CLK_PLL0,
> +       CLK_SEL_PLL1,
> +       CLK_SEL_CLK_PLL1,
> +       CLK_SEL_PLL2,
> +       CLK_SEL_CLK_PLL2,
> +       CLK_SEL_PLL4,
> +       CLK_SEL_CLK_PLL4,
> +       CLK_SEL_CLK_SRC,
> +       CLK_SEL_EXTAL,
> +       CLK_SEL_LOCO,
> +       CLK_PLL3_INPUT,
> +
> +       /* Module Clocks */
> +       MOD_CLK_BASE,
> +};
> +
> +static const struct clk_div_table dtable_1_2[] = {
> +       {0, 2},
> +       {15, 1},
> +       {0, 0},
> +};
> +
> +/* Mux clock tables */
> +static const char * const sel_clk_pll0[] = { ".sel_loco", ".sel_pll0" };
> +static const char * const sel_clk_pll1[] = { ".sel_loco", ".sel_pll1" };
> +static const char * const sel_clk_pll4[] = { ".sel_loco", ".sel_pll4" };
> +
> +static const struct cpg_core_clk r9a09g077_core_clks[] __initconst = {
> +       /* External Clock Inputs */
> +       DEF_INPUT("extal", CLK_EXTAL),
> +       DEF_INPUT("loco", CLK_LOCO),

DEF_RATE(), as this is an internally-generated clock.

> +
> +       /* Internal Core Clocks */
> +       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),
> +       DEF_FIXED(".sel_pll0", CLK_SEL_PLL0, CLK_PLL0, 1, 1),

This is the unimplemented selector to bypass the PLL, right?

> +       DEF_MUX(".sel_clk_pll0", CLK_SEL_CLK_PLL0, SEL_PLL,
> +               sel_clk_pll0, ARRAY_SIZE(sel_clk_pll0), 0, CLK_MUX_READ_ONLY),
> +       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),
> +       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),
> +
> +       /* Core output clk */
> +       DEF_DIV("CA55", R9A09G077_CA55, CLK_SEL_CLK_PLL0, DIVCA55,
> +               dtable_1_2, CLK_DIVIDER_HIWORD_MASK, 1),
> +       DEF_FIXED("PCLKM", R9A09G077_PCLKM, CLK_SEL_CLK_PLL1, 8, 1),
> +       DEF_FIXED("PCLKGPTL", R9A09G077_PCLKGPTL, CLK_SEL_CLK_PLL1, 2, 1),

Please sort these two alphabetically.

> +};
> +
> +static const struct mssr_mod_clk r9a09g077_mod_clks[] __initconst = {
> +       DEF_MOD("sci0", 108, R9A09G077_PCLKM),

Shouldn't that be 8 instead of 108?
Using R9A09G077_PCLKM as the parent is a temporary simplification,
right?

> +};
> +
> +static struct clk * __init
> +r9a09g077_cpg_div_clk_register(struct device *dev,
> +                              const struct cpg_core_clk *core,
> +                              void __iomem *base,
> +                              struct cpg_mssr_pub *pub)

Fits on one line less when wrapped.

> +{
> +       const struct clk *parent;
> +       const char *parent_name;
> +       struct clk_hw *clk_hw;
> +
> +       parent = pub->clks[core->parent];
> +
> +       if (IS_ERR(parent))
> +               return ERR_CAST(parent);
> +
> +       parent_name = __clk_get_name(parent);
> +
> +       if (core->dtable)
> +               clk_hw = clk_hw_register_divider_table(dev, core->name,
> +                                                      parent_name, 0,
> +                                                      base + GET_REG_OFFSET(core->conf),
> +                                                      GET_SHIFT(core->conf),
> +                                                      GET_WIDTH(core->conf),
> +                                                      core->flag,
> +                                                      core->dtable,
> +                                                      &pub->rmw_lock);
> +       else
> +               clk_hw = clk_hw_register_divider(dev, core->name,
> +                                                parent_name, 0,
> +                                                base + GET_REG_OFFSET(core->conf),
> +                                                GET_SHIFT(core->conf),
> +                                                GET_WIDTH(core->conf),
> +                                                core->flag, &pub->rmw_lock);
> +
> +       if (IS_ERR(clk_hw))
> +               return ERR_CAST(clk_hw);
> +
> +       return clk_hw->clk;
> +
> +}
> +
> +static struct clk * __init
> +r9a09g077_cpg_mux_clk_register(struct device *dev,
> +                              const struct cpg_core_clk *core,
> +                              void __iomem *base,
> +                              struct cpg_mssr_pub *pub)

Fits on one line less when wrapped.

> +{
> +       struct clk_hw *clk_hw;
> +
> +       clk_hw = devm_clk_hw_register_mux(dev, core->name,
> +                                         core->parent_names, core->num_parents,
> +                                         core->flag,
> +                                         base + GET_REG_OFFSET(core->conf),
> +                                         GET_SHIFT(core->conf),
> +                                         GET_WIDTH(core->conf),
> +                                         core->mux_flags, &pub->rmw_lock);

Missing error check for clk_hw.

> +       return clk_hw->clk;
> +}
> +
> +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)

Fits on one line less when wrapped.

> +{
> +       void __iomem *base = core->sel_base ? pub->base1 : pub->base0;
> +
> +       switch (core->type) {
> +       case CLK_TYPE_DIV:
> +               return r9a09g077_cpg_div_clk_register(dev, core, base, pub);
> +       case CLK_TYPE_MUX:
> +               return r9a09g077_cpg_mux_clk_register(dev, core, base, pub);
> +       default:
> +               return ERR_PTR(-EINVAL);
> +       }
> +}
> +
> +const struct cpg_mssr_info r9a09g077_cpg_mssr_info = {
> +       /* Core Clocks */
> +       .core_clks = r9a09g077_core_clks,
> +       .num_core_clks = ARRAY_SIZE(r9a09g077_core_clks),
> +       .last_dt_core_clk = LAST_DT_CORE_CLK,
> +       .num_total_core_clks = MOD_CLK_BASE,
> +
> +       /* Module Clocks */
> +       .mod_clks = r9a09g077_mod_clks,
> +       .num_mod_clks = ARRAY_SIZE(r9a09g077_mod_clks),
> +       .num_hw_mod_clks = 12 * 32,

"14 * 32", to account for the two gaps in the MSTPCRx registers?

> +
> +       .reg_layout = CLK_REG_LAYOUT_RZ_T2H,
> +       .cpg_clk_register = r9a09g077_cpg_clk_register,
> +};
> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> index 4bdfa4f65ab4..123bc1558e53 100644
> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> @@ -79,6 +79,27 @@ 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) & \

"|" instead of "&"

> +                                       ((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)
> +
> +static const u16 mstpcr_for_rzt2h[] = {
> +       RZT2H_MSTPCR(0, 0x300), RZT2H_MSTPCR(0, 0x304), RZT2H_MSTPCR(0, 0x308),
> +       RZT2H_MSTPCR(0, 0x30c), RZT2H_MSTPCR(0, 0x310), RZT2H_MSTPCR(1, 0x318),
> +       RZT2H_MSTPCR(1, 0x320), RZT2H_MSTPCR(0, 0x324), RZT2H_MSTPCR(0, 0x328),
> +       RZT2H_MSTPCR(0, 0x32c), RZT2H_MSTPCR(0, 0x330), RZT2H_MSTPCR(1, 0x334),

Missing holes for non-existent MSTPCRF (0x314) and MSTPCRH (0x31c).

> +};
> +
>  /*
>   * Standby Control Register offsets (RZ/A)
>   * Base address is FRQCR register

> @@ -227,7 +257,8 @@ static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
>
>         spin_unlock_irqrestore(&priv->pub.rmw_lock, flags);
>
> -       if (!enable || priv->reg_layout == CLK_REG_LAYOUT_RZ_A)
> +       if (!enable || priv->reg_layout == CLK_REG_LAYOUT_RZ_A ||
> +               priv->reg_layout == CLK_REG_LAYOUT_RZ_T2H)

Please align the continuation of the if-condition with the start of
the condition above.

>                 return 0;
>
>         error = readl_poll_timeout_atomic(priv->pub.base0 + priv->status_regs[reg],
> @@ -258,6 +289,12 @@ static int cpg_mstp_clock_is_enabled(struct clk_hw *hw)
>
>         if (priv->reg_layout == CLK_REG_LAYOUT_RZ_A)
>                 value = readb(priv->pub.base0 + priv->control_regs[reg]);
> +       else if (priv->reg_layout == CLK_REG_LAYOUT_RZ_T2H) {

Please use curly braces in all branches if you need them in at least
one branch...

> +               void __iomem *addr =
> +                       cpg_rzt2h_addr_from_offset(hw,
> +                                                  priv->control_regs[reg]);
> +               value = readw(addr);

Aren't the MSTPCR* registers 32-bit wide?

... however, you can avoid the curly braces by moving the register
read into the helper function.

> +       }
>         else
>                 value = readl(priv->pub.base0 + priv->status_regs[reg]);
>

> --- a/drivers/clk/renesas/renesas-cpg-mssr.h
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.h
> @@ -22,6 +22,8 @@
>  struct cpg_core_clk {
>         /* Common */
>         const char *name;
> +       const char * const *parent_names;
> +       const struct clk_div_table *dtable;

Please move them below, as they are type-specific.
I think they are never used together, so you can reduce kernel size
by combining them into an anonymous union.

>         unsigned int id;
>         unsigned int type;
>         /* Depending on type */
> @@ -29,18 +31,26 @@ struct cpg_core_clk {
>         unsigned int div;
>         unsigned int mult;
>         unsigned int offset;
> +       unsigned int conf;

u32

> +       int flag;

u8? (or u16, cfr. clk_divider.flags? see below)

> +       int mux_flags;

u8 (cfr. clk_mux.flags)

> +       int num_parents;

u8?

> +       int sel_base;

bool?

> +};

FTR, your additions as-is would have increased the size of each core
clock on arm64 by 32 bytes.

Note to self: use unions for every core clock type.

>  /**
>   * struct cpg_mssr_pub - Private data shared with
>   * device-specific clk registration code
>   *
>   * @base0: CPG/MSSR register block base0 address
> + * @base1: CPG/MSSR register block base1 address
>   * @rmw_lock: protects RMW register accesses
>   * @notifiers: Notifier chain to save/restore clock state for system resume
>   * @clks: pointer to clocks
>   */
>  struct cpg_mssr_pub {
>         void __iomem *base0;
> +       void __iomem *base1;
>         struct raw_notifier_head notifiers;
>         spinlock_t rmw_lock;
>         struct clk **clks;
> @@ -53,6 +63,8 @@ enum clk_types {
>         CLK_TYPE_DIV6P1,        /* DIV6 Clock with 1 parent clock */
>         CLK_TYPE_DIV6_RO,       /* DIV6 Clock read only with extra divisor */
>         CLK_TYPE_FR,            /* Fixed Rate Clock */
> +       CLK_TYPE_DIV,           /* Clock with divider */
> +       CLK_TYPE_MUX,           /* Clock with clock source selector */

Please move these into a new enum in r9a09g077-cpg-mssr.c, starting
from CLK_TYPE_CUSTOM, as they are specific to RZ/T2H.

>
>         /* Custom definitions start here */
>         CLK_TYPE_CUSTOM,
> @@ -73,6 +85,15 @@ enum clk_types {
>         DEF_BASE(_name, _id, CLK_TYPE_DIV6_RO, _parent, .offset = _offset, .div = _div, .mult = 1)
>  #define DEF_RATE(_name, _id, _rate)    \
>         DEF_TYPE(_name, _id, CLK_TYPE_FR, .mult = _rate)
> +#define DEF_DIV(_name, _id, _parent, _conf, _dtable, _flag, _sel_base) \
> +       DEF_TYPE(_name, _id, CLK_TYPE_DIV, .conf = _conf, \
> +                .parent = _parent, .dtable = _dtable, .flag = _flag, .sel_base = _sel_base)
> +#define DEF_MUX(_name, _id, _conf, _parent_names, _num_parents, _flag, \
> +               _mux_flags) \
> +       DEF_TYPE(_name, _id, CLK_TYPE_MUX, .conf = _conf, \
> +                .parent_names = _parent_names, .num_parents = _num_parents, \
> +                .flag = _flag, .mux_flags = _mux_flags)

The passed _flag parameter is always 0?
So when non-zero, cpg_core_clk.flag is always clk_divider.flags.

Please move both definitions to r9a09g077-cpg-mssr.c, as they are
specific to RZ/T2H.

> +
>
>      /*
>       * Definitions of Module Clocks

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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