Re: [PATCH RFC 02/10] dt-bindings: devfreq: add mt8196-gpufreq binding

Nicolas Frattaroli
2025-09-08T11:40:04Z

On Monday, 8 September 2025 13:15:03 Central European Summer Time AngeloGioacchino Del Regno wrote:
> Il 05/09/25 12:22, Nicolas Frattaroli ha scritto:
> > On the MediaTek MT8196 SoC, the GPU has its power and frequency
> > dynamically controlled by an embedded special-purpose MCU. This MCU is
> > in charge of powering up the GPU silicon. It also provides us with a
> > list of available OPPs at runtime, and is fully in control of all the
> > regulator and clock fiddling it takes to reach a certain level of
> > performance. It's also in charge of enforcing limits on power draw or
> > temperature.
> >
> > Add a binding for this device in the devfreq subdirectory, where it
> > seems to fit in best considering its tasks.
> >
> > The functions of many of the mailbox channels are unknown. This is not
> > the fault of this binding's author; we've never received adequate
> > documentation for this hardware, and the downstream code does not make
> > use of them in a way that'd reveal their purpose. They are kept in the
> > binding as the binding should be complete.
> >
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> > .../bindings/devfreq/mediatek,mt8196-gpufreq.yaml | 116 +++++++++++++++++++++
> > 1 file changed, 116 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/devfreq/mediatek,mt8196-gpufreq.yaml b/Documentation/devicetree/bindings/devfreq/mediatek,mt8196-gpufreq.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..1fe43c9fc94bb603b1fb77e9a97a27e92fea1ae8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/devfreq/mediatek,mt8196-gpufreq.yaml
> > @@ -0,0 +1,116 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/devfreq/mediatek,mt8196-gpufreq.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MediaTek MFlexGraphics Performance Controller
>
> Doesn't MFG stand for MediaTek Flexible Graphics? (or did they update the name?)
>
> Perhaps it's a good idea to also add that reference... I think it's a little more
> readable and understandable compared to "MFlexGraphics" :-)

"MFlexGraphics" is what the abbreviation section in the datasheet calls "MFG".
I don't see "Flexible Graphics" at all in the datasheet, but it's an obvious
inference of what the name means.

I think keeping "MFlexGraphics" is better for people grepping for what
the datasheet calls it.

>
> > +
> > +maintainers:
> > + - Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > +
> > +properties:
> > + $nodename:
> > + pattern: '^performance-controller@[a-f0-9]+$'
> > +
> > + compatible:
> > + enum:
> > + - mediatek,mt8196-gpufreq
> > +
> > + reg:
> > + items:
> > + - description: GPR memory area
> > + - description: RPC memory area
> > + - description: SoC variant ID register
> > +
> > + reg-names:
> > + items:
> > + - const: gpr
> > + - const: rpc
> > + - const: e2_id
>
> We should find a better name for that "e2_id".

Agreed, but we don't have a register map that includes this address
and would give us a different name. I think it's some sort of silicon
revision.

>
> > +
> > + clocks:
> > + items:
> > + - description: main clock of the embedded controller (EB)
> > + - description: core PLL
> > + - description: stack 0 PLL
> > + - description: stack 1 PLL
> > +
> > + clock-names:
> > + items:
> > + - const: eb
> > + - const: mfgpll
> > + - const: mfgpll_sc0
> > + - const: mfgpll_sc1
> > +
> > + mboxes:
> > + items:
> > + - description: FastDVFS events
> > + - description: frequency control
> > + - description: sleep control
> > + - description: timer control
> > + - description: frequency hopping control
> > + - description: hardware voter control
> > + - description: gpumpu (some type of memory control, unknown)
> > + - description: FastDVFS control
> > + - description: Unknown
> > + - description: Unknown
> > + - description: Unknown, but likely controls some boosting behaviour
> > + - description: Unknown
> > +
> > + mbox-names:
> > + items:
> > + - const: fast_dvfs_event
>
> Any problem if we avoid underscores in names?
>

No but I'm not sure what the canonical naming style is for mailbox
channels. "fastdvfsevent" is hard to read.

> > + - const: gpufreq
> > + - const: sleep
> > + - const: timer
> > + - const: fhctl
> > + - const: ccf
> > + - const: gpumpu
>
> "some type of memory control" .. it's really a MPU. For memory protection. :-)
> Besides, I don't think we have to touch anything in the gpumpu for freq control
> via gpueb.
>

Gotcha, so should I leave it out of the GPUFreq binding's used channels?

Would leave a gap, but that's probably fine.

> > + - const: fast_dvfs
> > + - const: ipir_c_met
> > + - const: ipis_c_met
>
> MET is a hardware event tracer / profiler... and I'm fairly sure that we have no
> real reason to support it (at least, not like that, and not in a first submission).
>
> Ah btw: ipir ipis .. ipi-receive ipi-send
>

Gotcha, will remove those as well.

> > + - const: brisket
>
> Brisket is... something. There's one for the GPU, one for CPU, and one for APU.
> Not sure what it exactly does, but seems to be or control a FLL (freq locked loop).
>
> > + - const: ppb
>
> PPB = Peak Power Budget
>
> The PPB needs its own "big" driver (the PBM - Power Budget Manager) in order to do
> anything - as in - this manages a SoC-global peak power setting based on the
> available maximum deliverable instantaneous (and/or sustainable) power from the
> board's power source and it is mainly used for smartphone usecase (battery!).
>
> In order to work, the PPB HW (yet another mcu) needs to be initialized with tables
> for CPU and GPU (and APU? and something else too?), and with other data explaining
> the maximum instantaneous power that can delivered at a certain battery percentage.
>
> Important point is... I doubt that PPB is being initialized by the bootloader, on
> all of Genio, Kompanio and Dimensity chips, so this should be disabled by default.
>
> You can keep it, especially now that you have a description for it - and because it
> does indeed exist, but I doubt that we're using this anytime soon.

If it's going to be used by a separate driver, wouldn't it be better if we don't make
this channel part of the channels the GPUFreq driver uses?

>
> Cheers,
> Angelo
>

Kind regards,
Nicolas Frattaroli

> > +
> > + shmem:
> > + $ref: /schemas/types.yaml#/definitions/phandle
> > + description: phandle to the shared memory region of the GPUEB MCU
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - reg-names
> > + - clocks
> > + - clock-names
> > + - mboxes
> > + - mbox-names
> > + - shmem
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/clock/mediatek,mt8196-clock.h>
> > +
> > + gpufreq: performance-controller@4b09fd00 {
> > + compatible = "mediatek,mt8196-gpufreq";
> > + reg = <0x4b09fd00 0x80>,
> > + <0x4b800000 0x1000>,
> > + <0x4b860128 0x4>;
> > + reg-names = "gpr", "rpc", "e2_id";
> > + clocks = <&topckgen CLK_TOP_MFG_EB>,
> > + <&mfgpll CLK_MFG_AO_MFGPLL>,
> > + <&mfgpll_sc0 CLK_MFGSC0_AO_MFGPLL_SC0>,
> > + <&mfgpll_sc1 CLK_MFGSC1_AO_MFGPLL_SC1>;
> > + clock-names = "eb", "mfgpll", "mfgpll_sc0",
> > + "mfgpll_sc1";
> > + mboxes = <&gpueb_mbox 0>, <&gpueb_mbox 1>, <&gpueb_mbox 2>,
> > + <&gpueb_mbox 3>, <&gpueb_mbox 4>, <&gpueb_mbox 5>,
> > + <&gpueb_mbox 6>, <&gpueb_mbox 7>, <&gpueb_mbox 8>,
> > + <&gpueb_mbox 9>, <&gpueb_mbox 10>, <&gpueb_mbox 11>;
> > + mbox-names = "fast_dvfs_event", "gpufreq", "sleep", "timer", "fhctl",
> > + "ccf", "gpumpu", "fast_dvfs", "ipir_c_met", "ipis_c_met",
> > + "brisket", "ppb";
> > + shmem = <&gpufreq_shmem>;
> > + };
> >
>
>