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

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

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