Re: [PATCH v2 4/8] clk: renesas: rzg2l-cpg: Add support for MSTOP in clock enable/disable API

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

Hi, Geert,

On 26.05.2025 16:33, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Fri, 23 May 2025 at 09:41, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>> On 22.05.2025 17:46, Geert Uytterhoeven wrote:
>>> On Wed, 14 May 2025 at 11:04, Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.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@bp.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.
> 
> Sorry, I missed that this function is called _after_ the clock is
> added to priv->clks[].  So one question and comment here:
>   1. Do you need a one-entry array (actual allocation is two entries)
>      for module clocks with an mstop entry that is not shared?

That extra entry should not be needed. It should not happen to have an
mstop clock in the priv->clks[] array w/o at least a clock in its shared
list. I was wrong in both the initial code and the reply I sent to your
initial comment. Appologies for that.

>      Perhaps for critical clocks? That could be handled in
>      rzg2l_mod_clock_module_set_state(), by explicitly checking
>      the clock's own critical flag if num_shared_mstop_clks is zero.
> 
>   2. If rzg2l_mod_clock_update_shared_mstop_clks() would be called
>      _before_ the clock is added to priv->clks[], the clk != clock
>      check would not be needed.

Yes, you're right. Running rzg2l_mod_clock_update_shard_mstop_clks() before
the priv->clks[] is updated simplifies the logic (see below).

> 
>>>> +                       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.
> 
> If rzg2l_mod_clock_update_shared_mstop_clks() would be called _before_
> the clock is added to priv->clks[], this issue could not happen, right?

That's true. With the above update this is not happen:

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

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

    if (clock->num_shared_mstop_clks)
        return 0;

    clock->shared_mstop_clks = devm_kzalloc(priv->dev,
                                       sizeof(*clock->shared_mstop_clks),
                                       GFP_KERNEL);
    if (!clock->shared_mstop_clks)
        return -ENOMEM;

    clock->shared_mstop_clks[0] = clock;
    clock->num_shared_mstop_clks = 1;

    return 0;
}


Thank you for your review,
Claudiu