Message-ID: <e77c85de-4542-44e1-af2e-f63f72602ff8@tuxon.dev>
Date: Fri, 9 May 2025 13:54:24 +0300
From: Claudiu Beznea <claudiu.beznea@...on.dev>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: mturquette@...libre.com, sboyd@...nel.org, robh@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org, magnus.damm@...il.com,
 linux-renesas-soc@...r.kernel.org, linux-clk@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH 3/7] clk: renesas: rzg2l-cpg: Add support for MSTOP in
 clock enable/disable API

Hi, Geert,

On 07.05.2025 18:42, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Thu, 10 Apr 2025 at 16:06, Claudiu <claudiu.beznea@...on.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>>
>> 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@bp.renesas.com/
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> 
> 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.

You're right, it should be achievable.

> 
>>  };
>>
>>  #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

Agree!

> (b) all clocks sharing the same mstop could share a single
>     clock->shared_mstop_clks[] array.

I'll look into this but I'm not sure how should I do it w/o extra
processing at the end of registering all the clocks. FWICT, that would
involve freeing some shared_mstop_clks arrays and using a single reference
as the shared_mstop_clks[] is updated after every clock is registered. Can
you please let me know if this what you are thinking about?

Updating the shared_mstop_clks[] after each clock is registered is
necessary in this implementation to avoid wrong setup of MSTOP in case 2
clocks shared it, one is critical, one isn't and taking into account that
they can be registered in any order:

case 1:
- at moment T1 the critical clock is registered
- at moment T2 the non-critical clock is registered

case 2:
- at moment T1 the non-critical clock is registered
- at moment T2 the critical clock is registered

where T1 < T2

> 
>> +
>> +               /*
>> +                * 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.

OK

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

I was thinking about it and I tried do it with a macro, keeping this code
in it:

    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;

but it was a complicated macro and abandoned it in the end.

> 
>> +               if (!clk || !clk->mstop)
> 
> Can !clk happen? None of the other loops check for that.

You're right, it can't happen.

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

OK.

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

I agree.

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

I'll move it there.

Thank you for your review,
Claudiu

> 
>> +
>>  #define EXTAL_FREQ_IN_MEGA_HZ  (24)
>>
>>  /**
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Powered by blists - more mailing lists