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

Nicolas Frattaroli posted 10 patches 4 days, 11 hours ago
[PATCH RFC 02/10] dt-bindings: devfreq: add mt8196-gpufreq binding
Posted by Nicolas Frattaroli 4 days, 11 hours ago
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
+
+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
+
+  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
+      - const: gpufreq
+      - const: sleep
+      - const: timer
+      - const: fhctl
+      - const: ccf
+      - const: gpumpu
+      - const: fast_dvfs
+      - const: ipir_c_met
+      - const: ipis_c_met
+      - const: brisket
+      - const: ppb
+
+  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>;
+    };

-- 
2.51.0
Re: [PATCH RFC 02/10] dt-bindings: devfreq: add mt8196-gpufreq binding
Posted by AngeloGioacchino Del Regno 1 day, 10 hours ago
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" :-)

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

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

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

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

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

Cheers,
Angelo

> +
> +  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>;
> +    };
>
Re: [PATCH RFC 02/10] dt-bindings: devfreq: add mt8196-gpufreq binding
Posted by Nicolas Frattaroli 1 day, 9 hours ago
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>;
> > +    };
> > 
> 
>
Re: [PATCH RFC 02/10] dt-bindings: devfreq: add mt8196-gpufreq binding
Posted by AngeloGioacchino Del Regno 1 day, 8 hours ago
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>;
>>> +    };
>>>
>>
>>
> 
> 
> 
>