Message-ID: <13857717.uLZWGnKmhe@workhorse>
Date: Mon, 08 Sep 2025 13:39:22 +0200
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: Boris Brezillon <boris.brezillon@...labora.com>,
 Steven Price <steven.price@....com>, Liviu Dudau <liviu.dudau@....com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Matthias Brugger <matthias.bgg@...il.com>,
 MyungJoo Ham <myungjoo.ham@...sung.com>,
 Kyungmin Park <kyungmin.park@...sung.com>,
 Chanwoo Choi <cw00.choi@...sung.com>, Jassi Brar <jassisinghbrar@...il.com>,
 Kees Cook <kees@...nel.org>, "Gustavo A. R. Silva" <gustavoars@...nel.org>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: Chia-I Wu <olvaffe@...il.com>, Chen-Yu Tsai <wenst@...omium.org>,
 kernel@...labora.com, dri-devel@...ts.freedesktop.org,
 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, linux-hardening@...r.kernel.org
Subject:
 Re: [PATCH RFC 02/10] dt-bindings: devfreq: add mt8196-gpufreq binding

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@...labora.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@...labora.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@...9fd00 {
> > +        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>;
> > +    };
> > 
> 
> 

Powered by blists - more mailing lists