Hi Claudiu,
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);
?
> + }
> +
> + 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
> + 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.
> + }
> +
> + return 0;
> +}
The rest LGTM.
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
