Re: [PATCH 03/14] clk: renesas: rzg2l-cpg: Add support for MSTOP

From: claudiu beznea
Date: Fri Nov 24 2023 - 04:24:50 EST

Hi, Geert,

On 23.11.2023 18:35, Geert Uytterhoeven wrote:
> Hi Claudiu,
>
> On Mon, Nov 20, 2023 at 8:01 AM Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>>
>> 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?

Yes.

>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>
>> --- 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.

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

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

>
>> + }
>> +
>> + 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);
>
>> @@ -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...
>
>> + } else {
>
> ... so this can just become "if (!mstop) {".

Ok, I'll review it.

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

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

>
>> + */
>> + 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
>
>> @@ -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

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