Re: [PATCH 06/14] clk: renesas: Add support for RZ/T2H family clock

[Thierry Bultel <thierry.bultel.yh@bp.renesas.com>]

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@bp.renesas.com>
> > ---
> >  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