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

AngeloGioacchino Del Regno
2025-09-08T12:49:24Z

Il 08/09/25 13:39, Nicolas Frattaroli ha scritto:
> 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.
>

Okay in MT8196 that was updated then.

On any other SoC previous to MT8196, the datasheet name is
"MediaTek Flexible Graphics (MFG)".

If you want to keep "MFlexGraphics" in MT8196-style, it's still a good idea to also
reference somewhere the old "MediaTek Flexible Graphics" name, as that was used for
more than 10 years. Really.

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

Yeah but still, it feels like this naming is MT8196-specific, and this driver is
not entirely specific to this SoC (this version is, but with minor modifications
this can work on other chips as well).

> I think it's some sort of silicon revision.

It is. And there's a broad range of names that you can use in place of "e2_id"...

If there's no precise name, always go with something that is generic enough but
that resembles what can be found inside of the mmio that you're specifying.

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

"fast-dvfs-event" would be good, wouldn't it? :-)

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

I really doubt that this is ever getting used at all for GPUFreq, so yes, leave it
out.

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

P.S.: of course I was implying that if we ever need to support those, we can always
add them later (but I still really doubt that we're ever going to use MET at all,
even though it would be *really* nice to).

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

Not sure if gpufreq needs to poke that channel to tell to the ppb "I'm setting this
frequency here", or if the MCUs can communicate that stuff on their own.
I didn't do any research about that.

Since adding stuff is kinda easier than removing, I guess avoiding to add this for
now is a sensible option. Let's do just that then.

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