lkml.org 
[lkml]   [2023]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 03/14] clk: renesas: rzg2l-cpg: Add support for MSTOP
    From
    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@tuxon.dev> 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?

    Yes.

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

    \
     
     \ /
      Last update: 2023-11-24 10:24    [W:2.939 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site