Message-ID: <CAMuHMdUb79rt2YVQO66_+8YcmAyS-PwFVcx0XfZVN-to3EB2SQ@mail.gmail.com>
Date: Fri, 1 Dec 2023 16:36:38 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: claudiu beznea <claudiu.beznea@...on.dev>
Cc: s.shtylyov@....ru, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, robh+dt@...nel.org, 
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, linux@...linux.org.uk, 
	magnus.damm@...il.com, mturquette@...libre.com, sboyd@...nel.org, 
	linus.walleij@...aro.org, p.zabel@...gutronix.de, arnd@...db.de, 
	m.szyprowski@...sung.com, alexandre.torgue@...s.st.com, afd@...com, 
	broonie@...nel.org, alexander.stein@...tq-group.com, 
	eugen.hristev@...labora.com, sergei.shtylyov@...il.com, 
	prabhakar.mahadev-lad.rj@...renesas.com, biju.das.jz@...renesas.com, 
	linux-renesas-soc@...r.kernel.org, netdev@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org, 
	linux-gpio@...r.kernel.org, Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH 03/14] clk: renesas: rzg2l-cpg: Add support for MSTOP

Hi Claudiu,

On Mon, Nov 27, 2023 at 8:37 AM claudiu beznea <claudiu.beznea@...on.dev> wrote:
> On 24.11.2023 11:08, Geert Uytterhoeven wrote:
> > On Thu, Nov 23, 2023 at 5:35 PM Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> >> On Mon, Nov 20, 2023 at 8:01 AM Claudiu <claudiu.beznea@...on.dev> wrote:
> >>> From: Claudiu Beznea <claudiu.beznea.uj@...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).

> > 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.
>
> I prefer this alternative even though it cannot be applied for old DTBs, it
> looks to me that is more modular. What do you think?

I prefer the second alternative, too.

> The only thing is that MSTOP is not really a power off/on switch (if it
> would be implemented with generic_pm_domain.power_{on, off}) but is more

That's fine: Linux' PM Domains are fairly generic and abstract, and
not limited to pure power domains/areas.

> like a clock disable/enable functionality (it should not be an issue
> though, just saying)... According to manual (I'm referring to Figure 41.4
> Block Connection Overview for Module Standby Mode of HW manula of RZ/G3S),
> it disables/enables the module's bus clock.

Thanks for the pointer! That picture nicely shows the internal behavior.
For comparison, on SH/R-Mobile and R-Car SoCs there is a similar
internal structure, but it is less visible to the programmer:
there are no individual controls for each clock or reset that is
fed into a module.  These are all hidden behind a single Module
Stop resp. Reset control bit.  In Linux, we modeled the module stop
bit as a gate clock, controlled by Runtime PM through the Clock Domain's
.start()/.stop() callbacks.

Note that you also have to take into account Figure 41.2 ("Modules in
Power Domain").  When adding support for power transitions later, you
can register a PM Domain representing PD_ISOVCC, and use that as the
parent PM Domain for the individual PM Domains for modules belonging
to PD_ISOVCC.  All of that can be handled in the driver, and would
not need any changes to DT.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-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

Powered by blists - more mailing lists