Hi, Geert,
On 27.05.2025 12:10, Geert Uytterhoeven wrote:
> Hi Claudiu,
>
> On Tue, 27 May 2025 at 10:58, Claudiu Beznea <claudiu.beznea@xxxxxxxxx> wrote:
>> On 26.05.2025 20:09, Geert Uytterhoeven wrote:
>>> On Mon, 26 May 2025 at 17:55, Claudiu Beznea <claudiu.beznea@xxxxxxxxx> wrote:
>>>> On 26.05.2025 16:33, Geert Uytterhoeven wrote:
>>>>> On Fri, 23 May 2025 at 09:41, Claudiu Beznea <claudiu.beznea@xxxxxxxxx> wrote:
>>>>>> On 22.05.2025 17:46, Geert Uytterhoeven wrote:
>>>>>>> On Wed, 14 May 2025 at 11:04, 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 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@xxxxxxxxxxxxxx/
>>>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> 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.
>>>
>>> So no single-entry arrays...
>>
>> Oh, I missread it yesterday everning. Sorry for confusion. Let me try again:
>>
>>>
>>>>> Perhaps for critical clocks? That could be handled in
>>
>> The clock is added to its own shared_mstop_clk[] array to avoid extra
>> conditions when all the clocks sharing the same mstop need to be checked
>> for an action. One example is code at [A] (for critical clocks) that was
>> available in v1.
>>
>>
>>>>> rzg2l_mod_clock_module_set_state(), by explicitly checking
>>>>> the clock's own critical flag if num_shared_mstop_clks is zero.
>>
>> The clock was added to its own shared_mstop_clk[] array as a result of this
>> comment I got from you on v1 (with regards to checking the clock's critical
>> flag):
>>
>> "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 I understood correctly, in [1] it has been proposed to have something
>> like what is proposed here to avoid extra conditional check (like [A]), in
>> rzg2l_mod_clock_module_set_state():
>>
>> 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) // <<< [A]
>> criticals++;
>>
>> Please let me know if I misunderstood your initial request?
>
> You did not misunderstand my initial request. Until recently, I just
> hadn't realized that you still need to check for a critical clock if
> the mstop is not shared, sorry for that.
>
> However, I think it is better to just add an extra check
>
> if (!clock->num_shared_mstop_clks &&
> (clk_hw_get_flags(&clock->hw) & CLK_IS_CRITICAL))
> criticals++;
>
> to rzg2l_mod_clock_module_set_state(), than to allocate single-entry
> arrays for each and every clock that does not share the mstop.
> Do you agree?
Yes!
Thank you for your review,
Claudiu
