Message-ID: <2866261.BEx9A2HvPv@workhorse>
Date: Mon, 14 Jul 2025 13:58:44 +0200
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Matthias Brugger <matthias.bgg@...il.com>,
 "Rafael J. Wysocki" <rafael@...nel.org>,
 Viresh Kumar <viresh.kumar@...aro.org>,
 Hector Yuan <hector.yuan@...iatek.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: kernel@...labora.com, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH 1/5] dt-bindings: mfd: syscon: Add mt8196 fdvfs syscons

On Monday, 14 July 2025 10:47:32 Central European Summer Time AngeloGioacchino Del Regno wrote:
> Il 11/07/25 16:57, Nicolas Frattaroli ha scritto:
> > The MT8196 SoC uses two syscon ranges for CPU DVFS that are separate
> > from each other. One, mt8196-fdvfs-config, is used to check for a magic
> > number at that memory address to verify that fdvfs should be used. The
> > other, mt8196-fdvfs, is used to configure the desired frequency for the
> > DVFS controller for each CPU core.
> > 
> 
> What is the reason why you're using syscons here?
> 
> Can't we simply assign the FDVFS MMIO to the cpufreq-hw node?

That would require refactoring the driver way more since it currently
gets the number of performance domains from the number of regs. If
you want me to do that, I'll need to know how we should disambiguate
performance domains from misc things like fdvfs. Stuff like string
comparisons on reg-names seems very ugly for the driver to do, but
adding a property to explicitly specify the number of performance
domains would then put into question what the existing binding did
by just assuming this information is something that implementations
can get without any ambiguity.

Even if we forget that Linux is the only kernel that cares about this
device tree, I'm not totally on board with having the smattering of
dozens of different tiny register ranges in every DT node on mediatek
like the vendor kernel does it. And not to forget, it'd change the
binding even more, to the point where I'd probably have to create a
new binding for mt8196.

> Or is there any reason why we can't declare it as mmio-sram? ...because I'm not
> entirely sure but the FDVFS space should effectively be a [c]SRAM memory range...

mmio-sram is fairly useless for the purposes of having something as
a fixed set of registers, hence why nobody else does it. From my
research, it appears to mainly be used if you want to actually treat
it like a pool of memory from which to then dynamically allocate
things.

To use it like a syscon, which is what we're doing, you'll have to
specify your mmio-sram node, then add a child node as a reserved range
for the "syscon-like" area, and then specify in ranges that you want
that child node's address translated into the global address space as
expected. Then in the driver, you can't just do a single function call
to get some regmap to write into, you have to follow a phandle in your
vendor property pointing to said sram range, then get its address,
translate said address to the global SoC address space, and then iomap
it. And the cleanup for error paths/driver remove isn't fun either.

Besides, we don't actually know whether this is an sram range, or how
large it is. The only confirmed sram range was the csram_sys thing at
like 0x11bc00 which is not used because it turned out to be useless,
and dealing with the kernel's sram interface to use it as a reserved
range just to read 4 bytes from it was a wasted afternoon. And that's
not even the real starting address of that sram area, that's just a
part we know is used because downstream uses 5 KiB there for a codepath
that's dead when fdvfs is present (except the one thing where it installs
a panic handler to shove something into a register to make it stop dvfs
logging if the kernel is in the process of crashing).

I can be convinced to go through the pain of making this mmio-sram if
I have documentation of the whole memory map of the SoC that shows me
where and how large each area of sram is, so that I don't need to come
up with my own starting addresses and offsets based on whatever random
stuff I can infer from downstream DT.

Really, every writable syscon is probably implemented as "sram" in
hardware. But an sram cell that is not general use memory but is
treated by the hardware as having some meaning is not mmio-sram. We
can't ever use it in any way other than as a syscon, and telling
implementations that they can except then slapping a huge reserved
range into it just makes them have to implement syscons twice, once
as syscons and once as syscons-except-it's-a-reserved-sram-mmio-range.

> 
> Cheers,
> Angelo
> 

Kind regards,
Nicolas Frattaroli

> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
> > ---
> >   Documentation/devicetree/bindings/mfd/syscon.yaml | 4 ++++
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
> > index 27672adeb1fedb7c81b8ae86c35f4f3b26d5516f..5ee49d2ba0cdb72dd697a0fd71c8416ad4fd2c1e 100644
> > --- a/Documentation/devicetree/bindings/mfd/syscon.yaml
> > +++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
> > @@ -88,6 +88,8 @@ select:
> >             - mediatek,mt8135-pctl-a-syscfg
> >             - mediatek,mt8135-pctl-b-syscfg
> >             - mediatek,mt8173-pctl-a-syscfg
> > +          - mediatek,mt8196-fdvfs
> > +          - mediatek,mt8196-fdvfs-config
> >             - mediatek,mt8365-syscfg
> >             - microchip,lan966x-cpu-syscon
> >             - microchip,mpfs-sysreg-scb
> > @@ -194,6 +196,8 @@ properties:
> >             - mediatek,mt8135-pctl-a-syscfg
> >             - mediatek,mt8135-pctl-b-syscfg
> >             - mediatek,mt8173-pctl-a-syscfg
> > +          - mediatek,mt8196-fdvfs
> > +          - mediatek,mt8196-fdvfs-config
> >             - mediatek,mt8365-infracfg-nao
> >             - mediatek,mt8365-syscfg
> >             - microchip,lan966x-cpu-syscon
> > 
> 
> 
> 

Powered by blists - more mailing lists