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

[Paul Barker <paul.barker.ct@bp.renesas.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 11989 bytes --]

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?

> +
> +	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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]