Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
MIME-Version: 1.0
References: <20231120070024.4079344-1-claudiu.beznea.uj@bp.renesas.com>
 <20231120070024.4079344-4-claudiu.beznea.uj@bp.renesas.com> <CAMuHMdVMpKVY8WX7dbtHfgnwgePH+i9+2BAumb37sFmqccb44g@mail.gmail.com>
In-Reply-To: <CAMuHMdVMpKVY8WX7dbtHfgnwgePH+i9+2BAumb37sFmqccb44g@mail.gmail.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Fri, 24 Nov 2023 10:08:49 +0100
Message-ID: <CAMuHMdVWvVtFMUe+J9R2ZU8Hi5CGs0NQfwUxitganM85183KkA@mail.gmail.com>
Subject: Re: [PATCH 03/14] clk: renesas: rzg2l-cpg: Add support for MSTOP
To:     Claudiu <claudiu.beznea@tuxon.dev>
Cc:     s.shtylyov@omp.ru, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        linux@armlinux.org.uk, magnus.damm@gmail.com,
        mturquette@baylibre.com, sboyd@kernel.org,
        linus.walleij@linaro.org, p.zabel@pengutronix.de, arnd@arndb.de,
        m.szyprowski@samsung.com, alexandre.torgue@foss.st.com, afd@ti.com,
        broonie@kernel.org, alexander.stein@ew.tq-group.com,
        eugen.hristev@collabora.com, sergei.shtylyov@gmail.com,
        prabhakar.mahadev-lad.rj@bp.renesas.com,
        biju.das.jz@bp.renesas.com, linux-renesas-soc@vger.kernel.org,
        netdev@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-clk@vger.kernel.org, linux-gpio@vger.kernel.org,
        Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

Hi Claudiu,

On Thu, Nov 23, 2023 at 5:35 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 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.
> >
> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

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

After giving this some more thought, it feels odd to derive the standby
state of a module from the state of its module clocks, while the latter
are already controlled through Runtime PM and a Clock Domain.

A first alternative solution could be to drop the GENPD_FLAG_PM_CLK
flag from the RZ/G2L CPG clock domain, and provide your own
gpd_dev_ops.start() and .stop() callbacks that take care of both
module standby and clocks (through pm_clk_{resume,suspend}().
(See https://elixir.bootlin.com/linux/v6.7-rc2/source/drivers/base/power/domain.c#L2093
for the GENPD_FLAG_PM_CLK case).
That still leaves you with a need to associate an MSTOP register and
bitmask with a device through its module clocks.

A second alternative solution could be to increase #power-domain-cells
from zero to one, and register individual PM Domains for each module,
and control module standby from the generic_pm_domain.power_{on,off}()
callbacks.  Devices would specify the module using the power-domains =
<&cpg <id> > property in DT, with <id> one of the to-be-added list of
modules in include/dt-bindings/clock/r9a08g045-cpg.h.  The RZ/G2L CPG
driver can handle the mapping from <id> to MSTOP register and bitmask.
This solution requires updates to DT, but you can keep compatibility
with old DTBs by only registering the new PM Domains when
#power-domain-cells is one.
The extra power saving would only be applicable with new DTBs, though.

Thoughts?

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

I was wrong, the N->N or N->M cases need a bitmask.

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