Hi Claudiu,
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...
> > 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;
... but here you still create a single-entry array?
>
> return 0;
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
