Message-ID: <b0d15a62-e164-4a8a-b4c7-77d9c3b2e7b2@tuxon.dev>
Date: Fri, 23 May 2025 10:41:30 +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 v2 4/8] clk: renesas: rzg2l-cpg: Add support for MSTOP in
 clock enable/disable API

Hi, Geert,

On 22.05.2025 17:46, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Wed, 14 May 2025 at 11:04, 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 bit
>> 2/ Set module CLKON bit
>>
>> B/ Switching to Standby State (e.g., during remove):
>> 1/ Clear CLKON bit
>> 2/ Set MSTOP bit
>>
>> 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 bit
>> 2/ Clear CLKON bit
>>
>> 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 bit
>> 2/ Clear MSTOP bit
>>
>> B/ Switching to Standby State (e.g., during remove):
>> 1/ Set MSTOP bit
>> 2/ Clear CLKON bit
>>
>> 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, Rev1.10.
>>
>> 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>
>> ---
>>
>> Changes in v2:
>> - udpated patch description to avoid plural in the configuration
>>   sequence description b/w MSTOP and CLK_ON
>> - use atomic type to store the usage counter; s/refcnt/usecnt/g
>> - moved MSTOP_OFF(), MSTOP_MASK() macros to rzg2l-cpg.c
>> - dropped struct mstp_clock::critical and use clk_hw_get_flags()
>>   instead to get the clock flags
>> - used unsigned int iterators in for loops
>> - keep memory allocated for a single list for clocks sharing the
>>   same MSTOP by updating the rzg2l_mod_clock_add_shared_mstop_clk();
>> - s/rzg2l_cpg_mstop_show/rzg2l_mod_clock_mstop_show/g,
>>   s/rzg2l_cpg_mstop/rzg2l_mod_clock_mstop/g,
>>   s/rzg2l_cpg_update_shared_mstop_clocks/rzg2l_mod_clock_update_shared_mstop_clks/g
>>    to keep the same naming conventions for functions handling mod clock MSTOP
>> - use the newly added for_each_mstp_clk() macro all over the code
> 
> Thanks for the update!
> 
>> --- a/drivers/clk/renesas/rzg2l-cpg.c
>> +++ b/drivers/clk/renesas/rzg2l-cpg.c
> 
>> @@ -1209,6 +1232,94 @@ struct mstp_clock {
>>                 else if (((hw) = __clk_get_hw((priv)->clks[(priv)->num_core_clks + i])) && \
>>                          ((mstp_clk) = to_mod_clock(hw)))
>>
>> +/* Need to be called with a lock held to avoid concurrent access to mstop->usecnt. */
>> +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 (unsigned int i = 0; i < clock->num_shared_mstop_clks; i++) {
>> +                       struct mstp_clock *clk = clock->shared_mstop_clks[i];
>> +
>> +                       if (clk_hw_get_flags(&clk->hw) & CLK_IS_CRITICAL)
>> +                               criticals++;
>> +               }
>> +
>> +               /*
>> +                * 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 == atomic_read(&mstop->usecnt))
>> +                       return;
>> +
>> +               value |= MSTOP_MASK(mstop->conf);
>> +
>> +               /* Allow updates on probe when usecnt = 0. */
>> +               if (!atomic_read(&mstop->usecnt))
>> +                       update = true;
>> +               else
>> +                       update = atomic_dec_and_test(&mstop->usecnt);
>> +       } else {
>> +               atomic_inc(&mstop->usecnt);
>> +               update = true;
> 
> Shouldn't the update be conditional, i.e.:
> 
>     if (!atomic_read(&mstop->usecnt))
>             update = true;
>     atomic_inc(&mstop->usecnt);
> 
> ?

Indeed, it should be conditional as you suggested.

> 
>> +       }
>> +
>> +       if (update)
>> +               writel(value, priv->base + MSTOP_OFF(mstop->conf));
>> +}
> 
>> +static int rzg2l_mod_clock_update_shared_mstop_clks(struct rzg2l_cpg_priv *priv,
>> +                                                   struct mstp_clock *clock)
>> +{
>> +       struct mstp_clock *clk;
>> +       struct clk_hw *hw;
>> +
>> +       if (!clock->mstop)
>> +               return 0;
>> +
>> +       for_each_mstp_clk(clk, hw, priv) {
>> +               struct mstp_clock **new_clks;
>> +               int num_shared_mstop_clks;
>> +               bool found = false;
>> +
>> +               if (clk->mstop != clock->mstop)
>> +                       continue;
>> +
>> +               num_shared_mstop_clks = clk->num_shared_mstop_clks;
>> +               for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
>> +                       if (clk->shared_mstop_clks[i] == clock) {
>> +                               found = true;
>> +                               break;
>> +                       }
>> +               }
>> +               if (found)
>> +                       continue;
> 
> Can this happen? With your current code, the answer is yes.
> But I think this loop and check can be removed...
> 
>> +
>> +               if (!num_shared_mstop_clks)
>> +                       new_clks = devm_kmalloc_array(priv->dev, 2, sizeof(*new_clks), GFP_KERNEL);
>> +               else
>> +                       new_clks = devm_krealloc(priv->dev, clk->shared_mstop_clks,
>> +                                                (num_shared_mstop_clks + 1) * sizeof(*new_clks),
>> +                                                GFP_KERNEL);
>> +
>> +               if (!new_clks)
>> +                       return -ENOMEM;
>> +
>> +               if (!num_shared_mstop_clks)
>> +                       new_clks[num_shared_mstop_clks++] = clk;
>> +               if (clk != clock)
> 
> This check is always true

If I'm not wrong now, when adding the clock to it's own list, and the list
is empty (!num_shared_mstop_clks check above is true), if this condition is
missing the clock it will be added twice in its own list.

> 
>> +                       new_clks[num_shared_mstop_clks++] = clock;
>> +
>> +               for (unsigned int i = 0; i < num_shared_mstop_clks; i++) {
>> +                       new_clks[i]->shared_mstop_clks = new_clks;
>> +                       new_clks[i]->num_shared_mstop_clks = num_shared_mstop_clks;
>> +               }
> 
> ... by adding a "break" here.  The loop above has already updated the
> shared_mstop_clks[] arrays for all clocks sharing the same mstop value.

It may happen that the entries in the module clock array provided by the
SoC specific drivers to not be sorted by module clock ID. That's the case
with RZ/G2L IA55 clocks (from r9a07g044-cpg.c):

static const struct {
	struct rzg2l_mod_clk common[79];
#ifdef CONFIG_CLK_R9A07G054
	struct rzg2l_mod_clk drp[5];
#endif
} mod_clks = {
  .common = {
    // ...

    DEF_MOD("ia55_pclk", R9A07G044_IA55_PCLK, R9A07G044_CLK_P2,
			 0x518, 0, MSTOP(BUS_PERI_CPU, BIT(13))),
    DEF_MOD("ia55_clk",	R9A07G044_IA55_CLK, R9A07G044_CLK_P1,
                         0x518, 1, MSTOP(BUS_PERI_CPU, BIT(13))),

    // ...
};

Where IDs are defined as:

#define R9A07G044_IA55_CLK     8
#define R9A07G044_IA55_PCLK    9

These clocks share the same MSTOP bit.

Because the ia55_pclk is the 1st clock registered (index 9) it will be
added to priv->clks[base + 9].

Next registered clock will be for ia55_clk, with index 8, it will be added
to priv->clks[base + 8].

for_each_mstp_clk() loops on clocks from priv->clks[] array. If a break
will be done at the end of the for_each_mstp_clk() loop, at the end of the
registration each of these clocks will have on it's shared_mstop_clks[]
only references to itself.

Thank you for your review,
Claudiu

> 
>> +       }
>> +
>> +       return 0;
>> +}
> 
> The rest LGTM.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
> 
> 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

Powered by blists - more mailing lists