Message-ID: <CAMuHMdXufBWPawkSMsvEdq66EPKi2SrB2REJjaLqu1RkSO8CWg@mail.gmail.com>
Date: Tue, 27 May 2025 11:10:36 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Claudiu Beznea <claudiu.beznea@...on.dev>
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 Claudiu,

On Tue, 27 May 2025 at 10:58, Claudiu Beznea <claudiu.beznea@...on.dev> wrote:
> On 26.05.2025 20:09, Geert Uytterhoeven wrote:
> > On Mon, 26 May 2025 at 17:55, Claudiu Beznea <claudiu.beznea@...on.dev> wrote:
> >> On 26.05.2025 16:33, Geert Uytterhoeven wrote:
> >>> On Fri, 23 May 2025 at 09:41, Claudiu Beznea <claudiu.beznea@...on.dev> wrote:
> >>>> On 22.05.2025 17:46, Geert Uytterhoeven wrote:
> >>>>> 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.
> >>>
> >>> 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?

Thanks!

> [1]
> https://lore.kernel.org/all/CAMuHMdXFtBmjDu=1RS2MLNYzhZ0fmpT7+1QbA9p4LvoLHitOuw@mail.gmail.com/

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