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

From: Geert Uytterhoeven
Date: Thu Nov 23 2023 - 11:42:58 EST

Next message: Marcelo Schmitt: "[PATCH v2 6/7] iio: adc: Add support for AD7091R-8"
Previous message: Marcelo Schmitt: "[PATCH v2 5/7] dt-bindings: iio: Add binding documentation for AD7091R-8"
In reply to: Claudiu: "[PATCH 03/14] clk: renesas: rzg2l-cpg: Add support for MSTOP"
Next in thread: Claudiu: "[PATCH 05/14] pinctrl: renesas: rzg2l: Move arg in the main function block"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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?

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

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

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

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

> @@ -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) {".

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

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

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

Next message: Marcelo Schmitt: "[PATCH v2 6/7] iio: adc: Add support for AD7091R-8"
Previous message: Marcelo Schmitt: "[PATCH v2 5/7] dt-bindings: iio: Add binding documentation for AD7091R-8"
In reply to: Claudiu: "[PATCH 03/14] clk: renesas: rzg2l-cpg: Add support for MSTOP"
Next in thread: Claudiu: "[PATCH 05/14] pinctrl: renesas: rzg2l: Move arg in the main function block"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]