Hi, Geert,

On 23.11.2023 18:35, Geert Uytterhoeven wrote:

Hi Claudiu,

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

quoted

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!

quoted

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

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

Yes.

quoted

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

quoted

--- a/drivers/clk/renesas/r9a08g045-cpg.c
+++ b/drivers/clk/renesas/r9a08g045-cpg.c

@@ -187,23 +187,39 @@ static const struct cpg_core_clk r9a08g045_core_clks[] __initconst = {
 };

 static const struct rzg2l_mod_clk r9a08g045_mod_clks[] = {
-       DEF_MOD("gic_gicclk",           R9A08G045_GIC600_GICCLK, R9A08G045_CLK_P1, 0x514, 0),
-       DEF_MOD("ia55_clk",             R9A08G045_IA55_CLK, R9A08G045_CLK_P1, 0x518, 1),
-       DEF_MOD("dmac_aclk",            R9A08G045_DMAC_ACLK, R9A08G045_CLK_P3, 0x52c, 0),
-       DEF_MOD("sdhi0_imclk",          R9A08G045_SDHI0_IMCLK, CLK_SD0_DIV4, 0x554, 0),
-       DEF_MOD("sdhi0_imclk2",         R9A08G045_SDHI0_IMCLK2, CLK_SD0_DIV4, 0x554, 1),
-       DEF_MOD("sdhi0_clk_hs",         R9A08G045_SDHI0_CLK_HS, R9A08G045_CLK_SD0, 0x554, 2),
-       DEF_MOD("sdhi0_aclk",           R9A08G045_SDHI0_ACLK, R9A08G045_CLK_P1, 0x554, 3),
-       DEF_MOD("sdhi1_imclk",          R9A08G045_SDHI1_IMCLK, CLK_SD1_DIV4, 0x554, 4),
-       DEF_MOD("sdhi1_imclk2",         R9A08G045_SDHI1_IMCLK2, CLK_SD1_DIV4, 0x554, 5),
-       DEF_MOD("sdhi1_clk_hs",         R9A08G045_SDHI1_CLK_HS, R9A08G045_CLK_SD1, 0x554, 6),
-       DEF_MOD("sdhi1_aclk",           R9A08G045_SDHI1_ACLK, R9A08G045_CLK_P1, 0x554, 7),
-       DEF_MOD("sdhi2_imclk",          R9A08G045_SDHI2_IMCLK, CLK_SD2_DIV4, 0x554, 8),
-       DEF_MOD("sdhi2_imclk2",         R9A08G045_SDHI2_IMCLK2, CLK_SD2_DIV4, 0x554, 9),
-       DEF_MOD("sdhi2_clk_hs",         R9A08G045_SDHI2_CLK_HS, R9A08G045_CLK_SD2, 0x554, 10),
-       DEF_MOD("sdhi2_aclk",           R9A08G045_SDHI2_ACLK, R9A08G045_CLK_P1, 0x554, 11),
-       DEF_MOD("scif0_clk_pck",        R9A08G045_SCIF0_CLK_PCK, R9A08G045_CLK_P0, 0x584, 0),
-       DEF_MOD("gpio_hclk",            R9A08G045_GPIO_HCLK, R9A08G045_OSCCLK, 0x598, 0),
+       DEF_MOD("gic_gicclk",           R9A08G045_GIC600_GICCLK, R9A08G045_CLK_P1, 0x514, 0,
+                                       MSTOP(ACPU, BIT(3))),

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

Hm... you're right. I've followed table 44.4 Registers for Module Standby
Mode to populate MSTOPs in r9a08g045_mod_clks[]. That table indicates bit 3
for GIC.

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

Good point. I'll remove the MSTOPs for critical clocks.

quoted

--- a/drivers/clk/renesas/rzg2l-cpg.c
+++ b/drivers/clk/renesas/rzg2l-cpg.c

@@ -1177,6 +1177,17 @@ rzg2l_cpg_register_core_clk(const struct cpg_core_clk *core,
                core->name, PTR_ERR(clk));
 }

+/**
+ * struct mstop - MSTOP specific data structure
+ * @count: reference counter for MSTOP settings (when zero the settings
+ *        are applied to register)
+ * @conf: MSTOP configuration (register offset, setup bits)
+ */
+struct mstop {
+       u32 count;
+       u32 conf;
+};
+
 /**
  * struct mstp_clock - MSTP gating clock
  *

@@ -1186,6 +1197,7 @@ rzg2l_cpg_register_core_clk(const struct cpg_core_clk *core,
  * @enabled: soft state of the clock, if it is coupled with another clock
  * @priv: CPG/MSTP private data
  * @sibling: pointer to the other coupled clock
+ * @mstop: MSTOP configuration
  */
 struct mstp_clock {
        struct clk_hw hw;

@@ -1194,10 +1206,46 @@ struct mstp_clock {
        bool enabled;
        struct rzg2l_cpg_priv *priv;
        struct mstp_clock *sibling;
+       struct mstop *mstop;
 };

 #define to_mod_clock(_hw) container_of(_hw, struct mstp_clock, hw)

+/* Need to be called with a lock held to avoid concurent access to mstop->count. */

concurrent

quoted

+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()?

I think it would be good to have it as mstop->count could be
incremented/decremented by more than one clock and could overflow faster
than struct clk_core::enable_count

quoted

+
+               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...

That's true. Would you prefer to have a WARN() for this too?

quoted

+       }
+
+       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);

quoted

@@ -1401,6 +1474,37 @@ rzg2l_cpg_register_mod_clk(const struct rzg2l_mod_clk *mod,
                }
        }

+       if (mod->mstop_conf) {
+               struct mstop *mstop = rzg2l_mod_clock_get_mstop(priv, mod->mstop_conf);
+
+               if (mstop) {
+                       clock->mstop = mstop;

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

quoted

+               } else {

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

Ok, I'll review it.

quoted

+                       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.

I kept it like this as I considered otherwise the error path might become
unnecessary complicated.

quoted

+                       }
+
+                       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 was referring to the hardware reset. In case we reach this point with
clocks already enabled by a previous kernel the state of the clocks in
hardware should be enabled and the mstop->count should be updated
accordingly by the above if block. Let me know if I'm missing something.

quoted

+                */
+               if (!clock->mstop->count)
+                       rzg2l_mod_clock_module_set_standby(clock, true);
+       }
+
        return;

 fail:

diff --git a/drivers/clk/renesas/rzg2l-cpg.h b/drivers/clk/renesas/rzg2l-cpg.h
index 6e38c8fc888c..10ee8aa4a5da 100644
--- a/drivers/clk/renesas/rzg2l-cpg.h
+++ b/drivers/clk/renesas/rzg2l-cpg.h

quoted

@@ -68,6 +73,10 @@
 #define SEL_PLL6_2     SEL_PLL_PACK(CPG_PL6_ETH_SSEL, 0, 1)
 #define SEL_GPU2       SEL_PLL_PACK(CPG_PL6_SSEL, 12, 1)

+#define MSTOP(name, bitmask)   ((CPG_##name##_MSTOP) << 16 | (bitmask))

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.

It is not always the case. That is why I've added the bitmask. The
identified scenarios are highlighted in commit description:

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})

Thank you for your review,
Claudiu Beznea

quoted

+#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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel