Re: [PATCH 3/4] clk: renesas: Add RZ/V2H CPG core wrapper driver

[Geert Uytterhoeven]

Hi Prabhakar,

Thanks for your patch!

On Fri, May 24, 2024 at 10:29 AM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
>
> Add CPG core helper wrapper driver for RZ/V2H SoC.

What is a "core helper wrapper"? ;-)

Looking at the structure, this looks like a family-specific clock driver?
Will there be more RZ/V2H-alike SoCs?

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> ---
>  drivers/clk/renesas/Kconfig     |   5 +
>  drivers/clk/renesas/Makefile    |   1 +
>  drivers/clk/renesas/rzv2h-cpg.c | 673 ++++++++++++++++++++++++++++++++
>  drivers/clk/renesas/rzv2h-cpg.h | 149 +++++++
>  4 files changed, 828 insertions(+)
>  create mode 100644 drivers/clk/renesas/rzv2h-cpg.c
>  create mode 100644 drivers/clk/renesas/rzv2h-cpg.h
>
> diff --git a/drivers/clk/renesas/Kconfig b/drivers/clk/renesas/Kconfig
> index d252150402e8..254203c2cb2e 100644
> --- a/drivers/clk/renesas/Kconfig
> +++ b/drivers/clk/renesas/Kconfig
> @@ -40,6 +40,7 @@ config CLK_RENESAS
>         select CLK_R9A07G054 if ARCH_R9A07G054
>         select CLK_R9A08G045 if ARCH_R9A08G045
>         select CLK_R9A09G011 if ARCH_R9A09G011
> +       select CLK_R9A09G057 if ARCH_R9A09G057
>         select CLK_SH73A0 if ARCH_SH73A0
>
>  if CLK_RENESAS
> @@ -193,6 +194,10 @@ config CLK_R9A09G011
>         bool "RZ/V2M clock support" if COMPILE_TEST
>         select CLK_RZG2L
>
> +config CLK_R9A09G057
> +       bool "Renesas RZ/V2H(P) clock support" if COMPILE_TEST

Please drop "Renesas "
(few other symbols have it, I'll fix the remaining).

> +       select RESET_CONTROLLER
> +
>  config CLK_SH73A0
>         bool "SH-Mobile AG5 clock support" if COMPILE_TEST
>         select CLK_RENESAS_CPG_MSTP
> diff --git a/drivers/clk/renesas/Makefile b/drivers/clk/renesas/Makefile
> index f7e18679c3b8..79cc7c4d77c6 100644
> --- a/drivers/clk/renesas/Makefile
> +++ b/drivers/clk/renesas/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_CLK_R9A07G044)           += r9a07g044-cpg.o
>  obj-$(CONFIG_CLK_R9A07G054)            += r9a07g044-cpg.o
>  obj-$(CONFIG_CLK_R9A08G045)            += r9a08g045-cpg.o
>  obj-$(CONFIG_CLK_R9A09G011)            += r9a09g011-cpg.o
> +obj-$(CONFIG_CLK_R9A09G057)            += rzv2h-cpg.o

If this is a family-specific clock driver, please use a separate Kconfig
symbol, like other families do, and move it ...

>  obj-$(CONFIG_CLK_SH73A0)               += clk-sh73a0.o
>
>  # Family

... here.

> --- /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
> + * @rmw_lock: protects register accesses
> + * @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[]
> + * @info: Pointer to platform data
> + */
> +struct rzv2h_cpg_priv {
> +       struct reset_controller_dev rcdev;
> +       struct device *dev;
> +       void __iomem *base;
> +       spinlock_t rmw_lock;

Unused

> +
> +       struct clk **clks;
> +       unsigned int num_core_clks;
> +       unsigned int num_mod_clks;
> +       unsigned int num_resets;
> +
> +       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;
> +       struct clk *clk;
> +
> +       switch (clkspec->args[0]) {
> +       case CPG_CORE:
> +               type = "core";
> +               clk = priv->clks[clkidx];

No range checking?

> +               break;
> +
> +       case CPG_MOD:
> +               type = "module";
> +               if (clkidx >= priv->num_mod_clks) {
> +                       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;
> +}

> +static void __init
> +rzv2h_cpg_register_mod_clk(const struct rzv2h_mod_clk *mod,
> +                          const struct rzv2h_cpg_info *info,
> +                          struct rzv2h_cpg_priv *priv)
> +{
> +       struct mstp_clock *clock = NULL;
> +       struct device *dev = priv->dev;
> +       unsigned int id = mod->id;
> +       struct clk_init_data init;
> +       struct clk *parent, *clk;
> +       const char *parent_name;
> +       unsigned int i;
> +
> +       WARN_DEBUG(id < priv->num_core_clks);
> +       WARN_DEBUG(id >= priv->num_core_clks + priv->num_mod_clks);
> +       WARN_DEBUG(mod->parent >= priv->num_core_clks + priv->num_mod_clks);
> +       WARN_DEBUG(PTR_ERR(priv->clks[id]) != -ENOENT);
> +
> +       if (!mod->name) {
> +               /* Skip NULLified clock */
> +               return;
> +       }

Do you have NULLified clocks?


> new file mode 100644
> index 000000000000..689c123d01c5
> --- /dev/null
> +++ b/drivers/clk/renesas/rzv2h-cpg.h

> +/**
> + * struct rzv2h_mod_clk - Module Clocks definitions
> + *
> + * @name: handle between common and hardware-specific interfaces
> + * @id: clock index in array containing all Core and Module Clocks
> + * @parent: id of parent clock
> + * @off: register offset

control register offset

> + * @bit: ON/MON bit

> + * @monoff: monitor register offset
> + * @monbit: monitor bit
> + */
> +struct rzv2h_mod_clk {
> +       const char *name;
> +       unsigned int id;
> +       unsigned int parent;
> +       u16 off;
> +       u8 bit;

Perhaps name them ctrl{off,bit}?

However, as all CPG_CLKONn registers are contiguous, storing
the register index (u8) might be better than the register offset (u16)?

> +       u16 monoff;
> +       u8 monbit;

Likewise for the CPG_CLKMONx registers...

> +};
> +
> +#define DEF_MOD_BASE(_name, _id, _parent, _off, _bit, _monoff, _monbit)        \
> +       { \
> +               .name = _name, \
> +               .id = MOD_CLK_BASE + (_id), \
> +               .parent = (_parent), \
> +               .off = (_off), \
> +               .bit = (_bit), \
> +               .monoff = (_monoff), \
> +               .monbit = (_monbit), \
> +       }
> +
> +#define DEF_MOD(_name, _id, _parent, _off, _bit, _monoff, _monbit)     \
> +       DEF_MOD_BASE(_name, _id, _parent, _off, _bit, _monoff, _monbit)
> +
> +/**
> + * struct rzv2h_reset - Reset definitions
> + *
> + * @off: reset register offset
> + * @bit: reset bit
> + * @monoff: monitor register offset
> + * @monbit: monitor bit
> + */
> +struct rzv2h_reset {
> +       u16 resoff;
> +       u8 resbit;
> +       u16 monoff;
> +       u8 monbit;

... and the CPG_RSTx and CPG_RSTMONx registers.


> +};
> +
> +#define DEF_RST(_id, _resoff, _resbit, _monoff, _monbit)       \
> +       [_id] = { \
> +               .resoff = (_resoff), \
> +               .resbit = (_resbit), \
> +               .monoff = (_monoff), \
> +               .monbit = (_monbit) \
> +       }
> +
> +/**
> + * struct rzv2h_cpg_info - SoC-specific CPG Description
> + *
> + * @core_clks: Array of Core Clock definitions
> + * @num_core_clks: Number of entries in core_clks[]
> + * @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[]
> + *
> + * @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[]
> + * @pll_get_clk1_offset: Function pointer to get PLL CLK1 offset
> + * @pll_get_clk2_offset: Function pointer to get PLL CLK2 offset
> + */
> +struct rzv2h_cpg_info {

> +       /* function pointers for PLL information */
> +       int (*pll_get_clk1_offset)(int clk);
> +       int (*pll_get_clk2_offset)(int clk);

Why are these function pointers?

> +};
> +
> +#endif /* __RENESAS_RZV2H_CPG_H__ */

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