Hi Claudiu,
On Thu, 10 Apr 2025 at 16:06, Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>
> The RZ/{G2L, V2L, G3S} CPG versions support a feature called MSTOP. Each
> module has one or more MSTOP bits associated with it, and these bits need
> to be configured along with the module clocks. Setting the MSTOP bits
> switches the module between normal and standby states.
>
> Previously, MSTOP support was abstracted through power domains
> (struct generic_pm_domain::{power_on, power_off} APIs). With this
> abstraction, the order of setting the MSTOP and CLKON bits was as follows:
>
> Previous Order:
> A/ Switching to Normal State (e.g., during probe):
> 1/ Clear module MSTOP bits
> 2/ Set module CLKON bits
>
> B/ Switching to Standby State (e.g., during remove):
> 1/ Clear CLKON bits
> 2/ Set MSTOP bits
>
> However, in some cases (when the clock is disabled through devres), the
> order may have been (due to the issue described in link section):
>
> 1/ Set MSTOP bits
> 2/ Clear CLKON bits
>
> Recently, the hardware team has suggested that the correct order to set
> the MSTOP and CLKON bits is:
>
> Updated Order:
> A/ Switching to Normal State (e.g., during probe):
> 1/ Set CLKON bits
> 2/ Clear MSTOP bits
>
> B/ Switching to Standby State (e.g., during remove):
> 1/ Set MSTOP bits
> 2/ Clear CLKON bits
>
> To prevent future issues due to incorrect ordering, the MSTOP setup has
> now been implemented in rzg2l_mod_clock_endisable(), ensuring compliance
> with the sequence suggested in Figure 41.5: Module Standby Mode Procedure
> from the RZ/G3S HW manual.
>
> Additionally, since multiple clocks of a single module may be mapped to a
> single MSTOP bit, MSTOP setup is reference-counted.
>
> Furthermore, as all modules start in the normal state after reset, if the
> module clocks are disabled, the module state is switched to standby. This
> prevents keeping the module in an invalid state, as recommended by the
> hardware team.
>
> Link: https://lore.kernel.org/all/20250215130849.227812-1-claudiu.beznea.uj@xxxxxxxxxxxxxx/
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
Thanks for your patch!
> --- a/drivers/clk/renesas/rzg2l-cpg.c
> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> @@ -1180,27 +1183,147 @@ rzg2l_cpg_register_core_clk(const struct cpg_core_clk *core,
> core->name, PTR_ERR(clk));
> }
>
> +/**
> + * struct mstop - MSTOP specific data structure
> + * @refcnt: reference counter for MSTOP settings (when zero the settings
> + * are applied to register)
> + * @conf: MSTOP configuration (register offset, setup bits)
> + */
> +struct mstop {
> + refcount_t refcnt;
> + u32 conf;
> +};
> +
> /**
> * struct mstp_clock - MSTP gating clock
> *
> * @priv: CPG/MSTP private data
> * @sibling: pointer to the other coupled clock
> + * @mstop: MSTOP configuration
> + * @shared_mstop_clks: clocks sharing the MSTOP with this clock
> * @hw: handle between common and hardware-specific interfaces
> * @off: register offset
> * @bit: ON/MON bit
> + * @num_shared_mstop_clks: number of the clocks sharing MSTOP with this clock
> * @enabled: soft state of the clock, if it is coupled with another clock
> + * @critical: specifies if this clock is critical
> */
> struct mstp_clock {
> struct rzg2l_cpg_priv *priv;
> struct mstp_clock *sibling;
> + struct mstop *mstop;
> + struct mstp_clock **shared_mstop_clks;
> struct clk_hw hw;
> u16 off;
> u8 bit;
> + u8 num_shared_mstop_clks;
> bool enabled;
> + bool critical;
I think you can do without this flag, and use
"clk_hw_get_flags(&mstp_clock.hw) & CLK_IS_CRITICAL" instead.
> };
>
> #define to_mod_clock(_hw) container_of(_hw, struct mstp_clock, hw)
>
> +/* Need to be called with a lock held to avoid concurrent access to mstop->refcnt. */
> +static void rzg2l_mod_clock_module_set_state(struct mstp_clock *clock,
> + bool standby)
> +{
> + struct rzg2l_cpg_priv *priv = clock->priv;
> + struct mstop *mstop = clock->mstop;
> + bool update = false;
> + u32 value;
> +
> + if (!mstop)
> + return;
> +
> + value = MSTOP_MASK(mstop->conf) << 16;
> +
> + if (standby) {
> + unsigned int criticals = 0;
> +
> + for (u8 i = 0; i < clock->num_shared_mstop_clks; i++) {
unsigned int
> + struct mstp_clock *clk = clock->shared_mstop_clks[i];
> +
> + if (clk->critical)
> + criticals++;
> + }
> +
> + /* Increment if clock is critical, too. */
> + if (clock->critical)
> + criticals++;
If clock->shared_mstop_clks[] would include the current clock, then
(a) this test would not be needed, and
(b) all clocks sharing the same mstop could share a single
clock->shared_mstop_clks[] array.
> +
> + /*
> + * If this is a shared MSTOP and it is shared with critical clocks,
> + * and the system boots up with this clock enabled but no driver
> + * uses it the CCF will disable it (as it is unused). As we don't
> + * increment reference counter for it at registration (to avoid
> + * messing with clocks enabled at probe but later used by drivers)
> + * do not set the MSTOP here too if it is shared with critical
> + * clocks and ref counted only by those critical clocks.
> + */
> + if (criticals && criticals == refcount_read(&mstop->refcnt))
> + return;
> +
> + value |= MSTOP_MASK(mstop->conf);
> +
> + /* Allow updates on probe when refcnt = 0. */
> + if (!refcount_read(&mstop->refcnt))
> + update = true;
> + else
> + update = refcount_dec_and_test(&mstop->refcnt);
> + } else {
> + if (!refcount_read(&mstop->refcnt)) {
> + refcount_set(&mstop->refcnt, 1);
> + update = true;
> + } else {
> + refcount_inc(&mstop->refcnt);
> + }
I think if you would replace the refcount_t by an atomic_t, you could
use atomic_inc() unconditionally, cfr. rzv2h-cpg.c.
> + }
> +
> + if (update)
> + writel(value, priv->base + MSTOP_OFF(mstop->conf));
> +}
> +
> +static int rzg2l_cpg_mstop_show(struct seq_file *s, void *what)
> +{
> + struct rzg2l_cpg_priv *priv = s->private;
> +
> + seq_printf(s, "%-20s %-5s %-10s\n", "", "", "MSTOP");
> + seq_printf(s, "%-20s %-5s %-10s\n", "", "clk", "-------------------------");
> + seq_printf(s, "%-20s %-5s %-5s %-5s %-6s %-6s\n",
> + "clk_name", "cnt", "cnt", "off", "val", "shared");
> + seq_printf(s, "%-20s %-5s %-5s %-5s %-6s %-6s\n",
> + "--------", "-----", "-----", "-----", "------", "------");
> +
> + for (unsigned int i = 0; i < priv->num_mod_clks; i++) {
> + struct mstp_clock *clk;
> + struct clk_hw *hw;
> + u32 val;
> +
> + if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-ENOENT))
> + continue;
> +
> + hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
> + clk = to_mod_clock(hw);
As this patch adds four more loops iterating over all module clocks
and skipping empty entries, I think it is worthwhile to introduce a
custom for_each_mstp_clock()-iterator.
> + if (!clk || !clk->mstop)
Can !clk happen? None of the other loops check for that.
> + continue;
> +
> + val = readl(priv->base + MSTOP_OFF(clk->mstop->conf)) &
> + MSTOP_MASK(clk->mstop->conf);
> +
> + seq_printf(s, "%-20s %-5d %-5d 0x%-3lx 0x%-4x ", clk_hw_get_name(hw),
Please drop the trailing space in the format...
> + __clk_get_enable_count(hw->clk), refcount_read(&clk->mstop->refcnt),
> + MSTOP_OFF(clk->mstop->conf), val);
> +
> + for (unsigned int i = 0; i < clk->num_shared_mstop_clks; i++)
> + seq_printf(s, "%pC ", clk->shared_mstop_clks[i]->hw.clk);
... add add it here, by changing this format to " %pC".
> +
> + seq_puts(s, "\n");
> + }
> +
> + return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(rzg2l_cpg_mstop);
> +
> static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
> {
> struct mstp_clock *clock = to_mod_clock(hw);
> +
> +static int rzg2l_cpg_add_shared_mstop_clock(struct device *dev,
> + struct mstp_clock *target,
> + struct mstp_clock *added)
> +{
> + for (u8 i = 0; i < target->num_shared_mstop_clks; i++) {
unsigned int
> + if (target->shared_mstop_clks[i] == added)
> + return 0;
> + }
> +
> + target->shared_mstop_clks = devm_krealloc(dev, target->shared_mstop_clks,
> + sizeof(*target->shared_mstop_clks) *
> + (target->num_shared_mstop_clks + 1),
> + GFP_KERNEL);
> + if (!target->shared_mstop_clks)
> + return -ENOMEM;
> +
> + target->shared_mstop_clks[target->num_shared_mstop_clks++] = added;
> +
> + return 0;
> +}
> +
> +static int rzg2l_cpg_update_shared_mstop_clocks(struct rzg2l_cpg_priv *priv,
> + struct mstp_clock *clock)
> +{
> + if (!clock->mstop)
> + return 0;
> +
> + for (unsigned int i = 0; i < priv->num_mod_clks; i++) {
> + struct mstp_clock *clk;
> + struct clk_hw *hw;
> + int ret;
> +
> + if (priv->clks[priv->num_core_clks + i] == ERR_PTR(-ENOENT))
> + continue;
> +
> + hw = __clk_get_hw(priv->clks[priv->num_core_clks + i]);
> + clk = to_mod_clock(hw);
> + if (clk == clock)
> + continue;
> +
> + if (!clk->mstop || clk->mstop != clock->mstop)
The first test is not needed, as clock->mstop is always non-zero here.
> + continue;
> +
> + ret = rzg2l_cpg_add_shared_mstop_clock(priv->dev, clock, clk);
> + if (ret)
> + return ret;
> +
> + ret = rzg2l_cpg_add_shared_mstop_clock(priv->dev, clk, clock);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static void __init
> rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
> const struct rzg2l_cpg_info *info,
> @@ -1406,6 +1655,12 @@ rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
> }
> }
>
> + ret = rzg2l_cpg_update_shared_mstop_clocks(priv, clock);
> + if (ret) {
> + clk = ERR_PTR(ret);
> + goto fail;
> + }
> +
> return;
>
> fail:
> @@ -1877,6 +2132,13 @@ static int __init rzg2l_cpg_probe(struct platform_device *pdev)
> for (i = 0; i < info->num_mod_clks; i++)
> rzg2l_cpg_register_mod_clk(&info->mod_clks[i], info, priv);
>
> + /*
> + * Initialize MSTOP after all the clocks were registered to avoid
> + * invalid reference counting when multiple clocks (critical,
> + * non-critical) shares the same MSTOP.
share
> + */
> + rzg2l_mod_clock_init_mstop(priv);
> +
> error = of_clk_add_provider(np, rzg2l_cpg_clk_src_twocell_get, priv);
> if (error)
> return error;
> --- a/drivers/clk/renesas/rzg2l-cpg.h
> +++ b/drivers/clk/renesas/rzg2l-cpg.h
> @@ -82,6 +82,10 @@
> #define SEL_PLL6_2 SEL_PLL_PACK(CPG_PL6_ETH_SSEL, 0, 1)
> #define SEL_GPU2 SEL_PLL_PACK(CPG_PL6_SSEL, 12, 1)
>
> +#define MSTOP(name, bitmask) ((CPG_##name##_MSTOP) << 16 | (bitmask))
> +#define MSTOP_OFF(conf) FIELD_GET(GENMASK(31, 16), (conf))
> +#define MSTOP_MASK(conf) FIELD_GET(GENMASK(15, 0), (conf))
The last two definitions are only used in rzg2l-cpg.c, so they can be
moved there.
> +
> #define EXTAL_FREQ_IN_MEGA_HZ (24)
>
> /**
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
