On Tue, Feb 04, 2025 at 04:14:29PM +0000, Paul Barker wrote:
> Hi Thierry,
>
> On 29/01/2025 16:37, Thierry Bultel wrote:
> > Add the CPG driver for T2H family.
> >
> > Signed-off-by: Thierry Bultel <thierry.bultel.yh@xxxxxxxxxxxxxx>
> > ---
> > drivers/clk/renesas/Kconfig | 4 +
> > drivers/clk/renesas/Makefile | 1 +
> > drivers/clk/renesas/rzt2h-cpg.c | 549 ++++++++++++++++++++++++++++++++
> > drivers/clk/renesas/rzt2h-cpg.h | 201 ++++++++++++
> > 4 files changed, 755 insertions(+)
> > create mode 100644 drivers/clk/renesas/rzt2h-cpg.c
> > create mode 100644 drivers/clk/renesas/rzt2h-cpg.h
>
> [snip]
>
> > diff --git a/drivers/clk/renesas/rzt2h-cpg.c b/drivers/clk/renesas/rzt2h-cpg.c
> > new file mode 100644
> > index 000000000000..79dacbd2b186
> > --- /dev/null
> > +++ b/drivers/clk/renesas/rzt2h-cpg.c
>
> [snip]
>
> > +struct pll_clk {
> > + void __iomem *base;
> > + struct rzt2h_cpg_priv *priv;
> > + struct clk_hw hw;
> > + unsigned int conf;
> > + unsigned int type;
> > +};
> > +#define to_pll(_hw) container_of(_hw, struct pll_clk, hw)
>
> Please add a blank line between these definitions.
>
> > +
> > +static struct clk
> > +*rzt2h_cpg_clk_src_twocell_get(struct of_phandle_args *clkspec,
>
> The '*' is part of the type so should be on the previous line. i.e.
>
> static struct clk *
> rzt2h_cpg_clk_src_twocell_get(...)
>
> > + void *data)
> > +{
> > + unsigned int clkidx = clkspec->args[1];
> > + struct rzt2h_cpg_priv *priv = data;
> > + struct device *dev = priv->dev;
> > + const char *type;
> > + 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";
> > + 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
> > +rzt2h_cpg_register_core_clk(const struct cpg_core_clk *core,
> > + const struct rzt2h_cpg_info *info,
> > + struct rzt2h_cpg_priv *priv)
> > +{
> > + struct clk *clk = ERR_PTR(-EOPNOTSUPP), *parent;
> > + unsigned int id = core->id, div = core->div;
> > + struct device *dev = priv->dev;
> > + const char *parent_name;
> > +
> > + WARN_DEBUG(id >= priv->num_core_clks);
> > + WARN_DEBUG(PTR_ERR(priv->clks[id]) != -ENOENT);
> > +
> > + /* Skip NULLified clock */
> > + if (!core->name)
> > + return;
> > +
> > + switch (core->type) {
> > + case CLK_TYPE_IN:
> > + clk = of_clk_get_by_name(priv->dev->of_node, core->name);
> > + break;
> > + case CLK_TYPE_FF:
> > + WARN_DEBUG(core->parent >= priv->num_core_clks);
> > + parent = priv->clks[core->parent];
> > + if (IS_ERR(parent)) {
> > + clk = parent;
> > + goto fail;
> > + }
> > +
> > + parent_name = __clk_get_name(parent);
> > + clk = clk_register_fixed_factor(NULL, core->name,
> > + parent_name, CLK_SET_RATE_PARENT,
> > + core->mult, div);
> > + break;
> > + case CLK_TYPE_DIV:
> > + if (core->sel_base > 0)
> > + clk = rzt2h_cpg_div_clk_register(core,
> > + priv->cpg_base1, priv);
> > + else
> > + clk = rzt2h_cpg_div_clk_register(core,
> > + priv->cpg_base0, priv);
> > + break;
> > + case CLK_TYPE_MUX:
> > + clk = rzt2h_cpg_mux_clk_register(core, priv->cpg_base0, priv);
> > + break;
> > + default:
> > + goto fail;
>
> I would prefer `clk = ERR_PTR(-EOPNOTSUPP);` here instead of at the top
> of the function so that it's easier to see at a glance what is
> happening.
>
> > + }
> > +
> > + if (IS_ERR_OR_NULL(clk))
> > + goto fail;
> > +
> > + priv->clks[id] = clk;
> > + return;
> > +
> > +fail:
> > + dev_err(dev, "Failed to register %s clock %s: %ld\n", "core",
> > + core->name, PTR_ERR(clk));
> > +}
> > +
> > +/**
> > + * struct mstp_clock - MSTP gating clock
> > + *
> > + * @hw: handle between common and hardware-specific interfaces
> > + * @priv: CPG/MSTP private data
> > + * @sibling: pointer to the other coupled clock
> > + * @baseaddr: register base address
> > + * @enabled: soft state of the clock, if it is coupled with another clock
> > + * @off: register offset
> > + * @bit: ON/MON bit
> > + */
> > +struct mstp_clock {
> > + struct rzt2h_cpg_priv *priv;
> > + struct mstp_clock *sibling;
> > + void __iomem *baseaddr;
> > + struct clk_hw hw;
> > + bool enabled;
> > + u32 off;
> > + u8 bit;
> > +};
> > +#define to_mod_clock(_hw) container_of(_hw, struct mstp_clock, hw)
> > +static int rzt2h_mod_clock_is_enabled(struct clk_hw *hw)
>
> Please add blank lines between these three definitions.
>
> > +{
> > + struct mstp_clock *clock = to_mod_clock(hw);
> > + struct rzt2h_cpg_priv *priv = clock->priv;
> > + u32 bitmask = BIT(clock->bit);
> > + u32 value;
> > +
> > + if (!clock->off) {
> > + dev_dbg(priv->dev, "%pC does not support ON/OFF\n", hw->clk);
> > + return 1;
> > + }
> > + value = readl(clock->baseaddr + clock->off);
> > +
> > + /* For all Module Stop registers, read bit meaning is as such:
> > + * 0: Release from the module-stop state
> > + * 1: Transition to the module-stop state is made
> > + */
> > +
> > + return !(value & bitmask);
> > +}
> > +
> > +static const struct clk_ops rzt2h_mod_clock_ops = {
> > + .is_enabled = rzt2h_mod_clock_is_enabled,
> > +};
> > +
> > +static void __init
> > +rzt2h_cpg_register_mod_clk(const struct rzt2h_mod_clk *mod,
> > + const struct rzt2h_cpg_info *info,
> > + struct rzt2h_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);
> > +
> > + /* Skip NULLified clock */
> > + if (!mod->name)
> > + return;
> > +
> > + parent = priv->clks[mod->parent];
> > + if (IS_ERR(parent)) {
> > + clk = parent;
> > + goto fail;
> > + }
> > +
> > + clock = devm_kzalloc(dev, sizeof(*clock), GFP_KERNEL);
> > + if (!clock) {
> > + clk = ERR_PTR(-ENOMEM);
> > + goto fail;
> > + }
> > +
> > + init.name = mod->name;
> > + init.ops = &rzt2h_mod_clock_ops;
> > + init.flags = CLK_SET_RATE_PARENT;
> > + for (i = 0; i < info->num_crit_mod_clks; i++)
> > + if (id == info->crit_mod_clks[i]) {
> > + dev_dbg(dev, "CPG %s setting CLK_IS_CRITICAL\n",
> > + mod->name);
> > + init.flags |= CLK_IS_CRITICAL;
> > + break;
> > + }
> > +
> > + parent_name = __clk_get_name(parent);
> > + init.parent_names = &parent_name;
> > + init.num_parents = 1;
> > +
> > + clock->off = mod->addr;
> > + clock->bit = mod->bit;
> > + clock->baseaddr = mod->sel_base ? priv->cpg_base1 : priv->cpg_base0;
> > + clock->priv = priv;
> > + clock->hw.init = &init;
>
> Both init and parent_name are local variables and can't be used after we
> return from this function. So we mustn't store pointers to them into
> objects that have a longer lifetime.
>
> Could we add init and parent_name members to struct mstp_clock, then use
> clock->init and clock->parent_name instead of the local variables?
Yes indeed, but notice that similar renesas drivers (rzg2l-cpg, rzv2h-cpg), and
quite a lot of other ones, do exactly the same way.
This is because devm_clk_hw_register() ends in calling __clk_register(),
that dups the provided name and copies the other fields from clk_init_data
I think it is fine to let it as it is.
>
> > +
> > + clk = devm_clk_register(dev, &clock->hw);
> > + if (IS_ERR(clk))
> > + goto fail;
> > +
> > + priv->clks[id] = clk;
> > +
> > + return;
> > +
> > +fail:
> > + dev_err(dev, "Failed to register %s clock %s: %ld\n", "module",
> > + mod->name, PTR_ERR(clk));
> > +}
> > +
> > +static bool rzt2h_cpg_is_pm_clk(const struct of_phandle_args *clkspec)
> > +{
> > + if (clkspec->args_count != 2)
> > + return false;
> > +
> > + switch (clkspec->args[0]) {
> > + case CPG_MOD:
> > + return true;
> > +
> > + default:
> > + return false;
> > + }
>
> Can we replace this switch statement with:
>
> return (clkspec->args[0] == CPG_MOD);
>
> > +}
> > +
> > +static int rzt2h_cpg_attach_dev(struct generic_pm_domain *unused, struct device *dev)
> > +{
> > + struct device_node *np = dev->of_node;
> > + struct of_phandle_args clkspec;
> > + unsigned int i = 0;
> > + bool once = true;
> > + struct clk *clk;
> > + int error;
> > +
> > + while (!of_parse_phandle_with_args(np, "clocks", "#clock-cells", i,
> > + &clkspec)) {
> > + if (!rzt2h_cpg_is_pm_clk(&clkspec)) {
> > + of_node_put(clkspec.np);
> > + continue;
> > + }
> > +
> > + if (once) {
>
> Can we just use `if (!i)` here and drop the `once` variable?
>
> > + once = false;
> > + error = pm_clk_create(dev);
> > + if (error) {
> > + of_node_put(clkspec.np);
> > + goto err;
> > + }
> > + }
> > + clk = of_clk_get_from_provider(&clkspec);
> > + of_node_put(clkspec.np);
> > + if (IS_ERR(clk)) {
> > + error = PTR_ERR(clk);
> > + goto fail_destroy;
> > + }
> > + error = pm_clk_add_clk(dev, clk);
> > + if (error) {
> > + dev_err(dev, "pm_clk_add_clk failed %d\n", error);
> > + goto fail_put;
> > + }
> > + i++;
> > + }
> > +
> > + return 0;
> > +
> > +fail_put:
> > + clk_put(clk);
> > +
> > +fail_destroy:
> > + pm_clk_destroy(dev);
> > +err:
> > + return error;
> > +}
>
> [snip]
>
> > +static const struct of_device_id rzt2h_cpg_match[] = {
> > +#ifdef CONFIG_CLK_R9A09G077
> > + {
> > + .compatible = "renesas,r9a09g077-cpg",
> > + .data = &r9a09g077_cpg_info,
> > + },
> > +#endif
>
> CONFIG_CLK_R9A09G077 and r9a09g077_cpg_info are not defined until the
> subsequent patch. We should move this entry to the next patch, and leave
> this array empty here.
>
> For comparison see how the RZ/V2H CPG driver was added in the following
> commits:
>
> dd22e5621749 ("clk: renesas: Add family-specific clock driver for RZ/V2H(P)")
> 36932cbc3e6c ("clk: renesas: Add RZ/V2H(P) CPG driver")
>
> > + { /* sentinel */ }
> > +};
>
> [snip]
>
> > diff --git a/drivers/clk/renesas/rzt2h-cpg.h b/drivers/clk/renesas/rzt2h-cpg.h
> > new file mode 100644
> > index 000000000000..d9d28608e4c3
> > --- /dev/null
> > +++ b/drivers/clk/renesas/rzt2h-cpg.h
>
> [snip]
>
> > +/**
> > + * struct rzt2_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[]
> > + *
> > + * @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 rzt2h_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 rzt2h_mod_clk *mod_clks;
> > + unsigned int num_mod_clks;
> > + unsigned int num_hw_mod_clks;
> > +
> > + /* Resets */
> > + const struct rzt2h_reset *resets;
> > + unsigned int num_resets;
> > +
> > + /* Critical Module Clocks that should not be disabled */
> > + const unsigned int *crit_mod_clks;
> > + unsigned int num_crit_mod_clks;
>
> It looks like resets and crit_mod_clks are not populated in this initial
> patch series. We can drop support for both of these from this patch
> series.
>
> > +};
> > +
> > +extern const struct rzt2h_cpg_info r9a09g077_cpg_info;
> > +
> > +#endif
>
> --
> Paul Barker
