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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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