Hi Claudiu,

On Mon, Nov 20, 2023 at 8:01 AM Claudiu [off-list ref] wrote:

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

RZ/{G2L, V2L, G3S} based CPG versions have support for saving extra
power when clocks are disabled by activating module standby. This is done
though MSTOP specific registers that are part of CPG. Each individual
module have one or more bits associated in one MSTOP register (see table
"Registers for Module Standby Mode" from HW manuals). Hardware manual
associates modules' clocks to one or more MSTOP bits. There are 3 mappings
available (identified by researching RZ/G2L, RZ/G3S, RZ/V2L HW manuals):

case 1: N clocks mapped to N MSTOP bits (with N={0, ..., X})
case 2: N clocks mapped to 1 MSTOP bit  (with N={0, ..., X})
case 3: N clocks mapped to M MSTOP bits (with N={0, ..., X}, M={0, ..., Y})

Case 3 has been currently identified on RZ/V2L for VCPL4 module.

To cover all 3 cases the individual platform drivers will provide to
clock driver MSTOP register offset and associated bits in this register
as a bitmask and the clock driver will apply this bitmask to proper
MSTOP register.

As most of the modules have more than one clock and these clocks are
mapped to 1 MSTOP bitmap that need to be applied to MSTOP registers,
to avoid switching the module to/out of standby when the module has
enabled/disabled clocks a counter has been associated to each module
(though struct mstop::count) which is incremented/decremented every
time a module's clock is enabled/disabled and the settings to MSTOP
register are applied only when the counter reaches zero (counter zero
means either 1st clock of the module is going to be enabled or all clocks
of the module are going to be disabled).

Thanks for your patch!

The MSTOP functionality has been instantiated at the moment for RZ/G3S.

Do you plan to add support for the other SoCs, too?

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

According to Rev. 1.00 of the Hardware User's Manual, bit 3 of the
CPG_BUS_ACPU_MSTOP register is reserved?

Also, gic_gicclk is a critical module clock, so I guess this module
must never be put into standby?

concurrent

+static void rzg2l_mod_clock_module_set_standby(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) {
+               value |= MSTOP_MASK(mstop->conf);
+               /* Avoid overflow. */
+               if (mstop->count > 0)
+                       mstop->count--;

Should we add a WARN() here, or is it sufficient to rely on the WARN()
in drivers/clk/clk.c:clk_core_disable()?

+
+               if (!mstop->count)
+                       update = true;
+       } else {
+               if (!mstop->count)
+                       update = true;
+
+               /* Avoid overflow. */
+               if (mstop->count + 1 != 0)
+                       mstop->count++;

Trying to avoid an overflow won't help much here.  The counter
will be wrong afterwards anyway, and when decrementing again later, the
module will be put in standby too soon...

+       }
+
+       if (update)
+               writel(value, priv->base + MSTOP_OFF(mstop->conf));
+}
+
 static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
 {
        struct mstp_clock *clock = to_mod_clock(hw);

Please move the common assignment after the if/else block...

+               } else {

... so this can just become "if (!mstop) {".

+                       mstop = devm_kzalloc(dev, sizeof(*mstop), GFP_KERNEL);
+                       if (!mstop) {
+                               clk_unregister(clk);
+                               goto fail;

Please use "goto unregister", and call clk_unregister() after the new
unregister label.

+                       }
+
+                       mstop->conf = mod->mstop_conf;
+                       clock->mstop = mstop;
+               }
+
+               if (rzg2l_mod_clock_is_enabled(&clock->hw)) {
+                       if (clock->sibling)
+                               clock->mstop->count = 1;
+                       else
+                               clock->mstop->count++;
+               }
+
+               /*
+                * Out of reset all modules are enabled. Set module to standby
+                * in case associated clocks are disabled at probe.

Is that always true?
What about kexec and crashdump kernels?

I believe the bitmask is always a single bit.
So perhaps let MSTOP() take the bit number instead of the bitmaskl?
You can still store BIT(bit) inside the macro.

+#define MSTOP_OFF(conf)                ((conf) >> 16)
+#define MSTOP_MASK(conf)       ((conf) & GENMASK(15, 0))
+
 #define EXTAL_FREQ_IN_MEGA_HZ  (24)

 /**

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-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