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?
> +
> + 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 BarkerAttachment:
OpenPGP_0x27F4B3459F002257.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
