[PATCH v2 00/10] MT8196 GPU Frequency/Power Control Support

[PATCH v2 00/10] MT8196 GPU Frequency/Power Control Support

[Nicolas Frattaroli]

This series introduces two new drivers to accomplish controlling the
frequency and power of the Mali GPU on MediaTek MT8196 SoCs.

The reason why it's not as straightforward as with other SoCs is that
the MT8196 has quite complex glue logic in order to squeeze the maximum
amount of performance possible out of the silicon. There's an additional
MCU running a specialised firmware, which communicates with the
application processor through a mailbox and some SRAM, and is in charge
of controlling the regulators, the PLL clocks, and the power gating of
the GPU, all while also being in charge of any DVFS control.

This set of drivers is enough to communicate desired OPP index limits to
the aforementioned MCU, referred to as "GPUEB" from here on out. The
GPUEB is still free to lower the effective frequency if the GPU has no
jobs going on at all, even when a higher OPP is set. There's also
several more powerful OPPs it seemingly refuses to apply. The downstream
chromeos kernel also doesn't reach the frequencies of those OPPs, so we
assume this is expected.

The frequency control driver lives in panthor's subdirectory, as it
registers a devfreq device for the panthor GPU device, and needs to
mingle with it somewhat closely. I've kept the tie-in parts generic
enough however to not make this a complete hack; mediatek_mfg (the
frequency control driver) registers itself as a "devfreq provider" with
panthor, and panthor picks it up during its probe function (or defers if
mediatek_mfg is not ready yet, after adding a device link first).

The mailbox driver is a fairly bog-standard common mailbox framework
driver, just specific to the firmware that runs on the GPUEB.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
Changes in v2:
- mali-valhall-csf binding: move from performance-controller to
  performance-domains property
- mali-valhall-csf binding: fix vendor name oopsie in compatible of if
  condition
- mt8196-gpufreq binding: move from performance-controller to
  performance-domains by adding the cells property
- mt8196-gpufreq binding: rename e2_id to hw_revision
- mt8196-gpufreq binding: add description that mentions "MediaTek
  Flexible Graphics"
- mt8196-gpufreq binding: get rid of mailbox channels we're unlikely to
  use any time soon, if ever
- mt8196-gpufreq binding: change name of mailbox channels to use -
  instead of _
- mailbox binding: change reg-names to "data" and "ctl"
- drm/panthor: mediatek_mfg: rename e2_id to hw_revision
- drm/panthor: devfreq: switch from performance-controller to
  performance-domains
- drm/panthor: devfreq: get rid of the accidental get_cur_freq function
  move
- mailbox: rename mtk_gpueb_mbox_ch to mtk_gpueb_mbox_chan_desc
- mailbox: use smaller types in mtk_gpueb_mbox_chan_desc where possible
- mailbox: add per-channel runtime data struct
- mailbox: request one threaded IRQ per channel, pass channel struct as
  data
- mailbox: make num_channels in variant struct u8
- mailbox: get rid of no_response, as it was redundant
- mailbox: enable and disable clock in mailbox startup/shutdown
- mailbox: point con_priv of mailbox framework channel struct to this
  driver's channel struct
- mailbox: request and free the threaded IRQ in startup/shutdown
- mailbox: only clear IRQ bit flag once RX data has been read from MMIO
- mailbox: reduce needlessly large receive buffer size
- mailbox: handle allocation errors wherever they could pop up
- mailbox: style cleanups in mtk_gpueb_mbox_read_rx
- mailbox: call platform_get_irq earlier on in probe
- mailbox: set drvdata later on in probe
- mailbox: ioremap resources by index, not name
- mailbox: handle devm_mbox_controller_register errors
- mailbox: rename channels to correspond to bindings
- mailbox: document a few of the private driver structs to be kind to
  the next person who will look at this code
- Link to v1: https://lore.kernel.org/r/20250905-mt8196-gpufreq-v1-0-7b6c2d6be221@collabora.com

---
Nicolas Frattaroli (10):
      dt-bindings: gpu: mali-valhall-csf: add mediatek,mt8196-mali variant
      dt-bindings: devfreq: add mt8196-gpufreq binding
      dt-bindings: sram: Add compatible for mediatek,mt8196-gpufreq-sram
      dt-bindings: mailbox: Add MT8196 GPUEB Mailbox
      mailbox: add MediaTek GPUEB IPI mailbox
      drm/panthor: call into devfreq for current frequency
      drm/panthor: move panthor_devfreq struct to header
      drm/panthor: devfreq: expose get_dev_status and make it more generic
      drm/panthor: devfreq: add pluggable devfreq providers
      drm/panthor: add support for MediaTek MFlexGraphics

 .../bindings/devfreq/mediatek,mt8196-gpufreq.yaml  |  113 +++
 .../bindings/gpu/arm,mali-valhall-csf.yaml         |   32 +-
 .../mailbox/mediatek,mt8196-gpueb-mbox.yaml        |   64 ++
 Documentation/devicetree/bindings/sram/sram.yaml   |    1 +
 drivers/gpu/drm/panthor/Kconfig                    |   13 +
 drivers/gpu/drm/panthor/Makefile                   |    2 +
 drivers/gpu/drm/panthor/mediatek_mfg.c             | 1053 ++++++++++++++++++++
 drivers/gpu/drm/panthor/panthor_devfreq.c          |  136 ++-
 drivers/gpu/drm/panthor/panthor_devfreq.h          |   57 +-
 drivers/gpu/drm/panthor/panthor_device.h           |    3 -
 drivers/gpu/drm/panthor/panthor_drv.c              |    4 +-
 drivers/mailbox/Kconfig                            |   10 +
 drivers/mailbox/Makefile                           |    2 +
 drivers/mailbox/mtk-gpueb-mailbox.c                |  337 +++++++
 14 files changed, 1788 insertions(+), 39 deletions(-)
---
base-commit: 51095600e8c19d53729a7fbd273abc4435a25e9b
change-id: 20250829-mt8196-gpufreq-a7645670d182
prerequisite-message-id: <20250829091913.131528-1-laura.nao@collabora.com>
prerequisite-patch-id: 441c4c2e3d22f83a41241a1ab5c9be1a442f742e
prerequisite-patch-id: 852bfc3d13e2bccc4d6f4813a71c42f329dadb0c
prerequisite-patch-id: 0bc5b7bf268e88a6ef22e46c91db7645e2ce6189
prerequisite-patch-id: 442533e316e46ecd47cd0b7fb410b58fad2b3bf9
prerequisite-patch-id: 6d6d70ccb7d718b3bcca6662cdaf1e8b64b6ddc2
prerequisite-patch-id: d61046e2cd2f33024092e96e8a987b9c34c24e73
prerequisite-patch-id: c27ca28bb3df435c98fe02438264188d6fa52b7c
prerequisite-patch-id: 27fadb12ce15099a684c08d4f8c785bedc87cef2
prerequisite-patch-id: 7796ec9a0162ae96b708ea513742016657c69e14
prerequisite-patch-id: f7549078f3702acdf1e6dcd36ddebab0e543b5db
prerequisite-patch-id: b123fb15cb8c97cf0896b826820f4ce33085170c
prerequisite-patch-id: fa96e18eae90efc14e4b9f13534c013b448a3f84
prerequisite-patch-id: 1e53ad7341ddb67c9788252456068cc14ab2f610
prerequisite-patch-id: ffff2977d8f2a3a44c3606f77d38f8745cb3c60a
prerequisite-patch-id: 71d23f4f096e424ae3aa59a23695f5b1e488fab0
prerequisite-patch-id: 3c12631f22a39d6def9340ed840e9e55e1a76304
prerequisite-patch-id: caec6572d5d9a37183601b8e38f50af797df635e
prerequisite-patch-id: d5b3d5719675a1e3be26e028b5596d39e839bc09
prerequisite-patch-id: da7b826d56ac70b3b72be58d64e7c2107445478f
prerequisite-patch-id: f3f789e0d919dd92b7d811a4e11c57bb05f71617
prerequisite-patch-id: 79cca92633ca3d9cc2f1f38b6fc977a8d8543d60
prerequisite-patch-id: f663f8f3bddf198d0cab083bac7ebb88689ffc82
prerequisite-patch-id: a0ffc5b88c5eb88c491f6187672012c621bd520c
prerequisite-patch-id: e6c6d67b034d06b6158a0e0f8299ad28f0e59134
prerequisite-patch-id: 25f658fbd1238bd57e05ff299d0436f942bdcc4d
prerequisite-patch-id: 7be8439e241a320b0eb0a264a8a59a9beef383d6
prerequisite-patch-id: b903714dbe7d6a44fbe18faa02d59862ceadf217

Best regards,
-- 
Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

[PATCH v2 01/10] dt-bindings: gpu: mali-valhall-csf: add mediatek,mt8196-mali variant

[Nicolas Frattaroli]

The Mali-based GPU on the MediaTek MT8196 SoC uses a separate MCU to
control the power and frequency of the GPU.

It lets us omit the OPP tables from the device tree, as those can now be
enumerated at runtime from the MCU.

Add the mediatek,mt8196-mali compatible, and a performance-domains
property which points to the MCU's device tree node in this case. It's
required on mt8196 devices.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 .../bindings/gpu/arm,mali-valhall-csf.yaml         | 32 +++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
index a5b4e00217587c5d1f889094e2fff7b76e6148eb..163b4457f7f25dcdd509c558558a73694521c96d 100644
--- a/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
+++ b/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
@@ -19,6 +19,7 @@ properties:
       - items:
           - enum:
               - rockchip,rk3588-mali
+              - mediatek,mt8196-mali
           - const: arm,mali-valhall-csf   # Mali Valhall GPU model/revision is fully discoverable
 
   reg:
@@ -53,6 +54,9 @@ properties:
   opp-table:
     type: object
 
+  performance-domains:
+    maxItems: 1
+
   power-domains:
     minItems: 1
     maxItems: 5
@@ -91,7 +95,6 @@ required:
   - interrupts
   - interrupt-names
   - clocks
-  - mali-supply
 
 additionalProperties: false
 
@@ -105,9 +108,24 @@ allOf:
       properties:
         clocks:
           minItems: 3
+        performance-domains: false
         power-domains:
           maxItems: 1
         power-domain-names: false
+      required:
+        - mali-supply
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: mediatek,mt8196-mali
+    then:
+      properties:
+        mali-supply: false
+        sram-supply: false
+        operating-points-v2: false
+      required:
+        - performance-domains
 
 examples:
   - |
@@ -143,5 +161,17 @@ examples:
             };
         };
     };
+  - |
+    gpu@48000000 {
+        compatible = "mediatek,mt8196-mali", "arm,mali-valhall-csf";
+        reg = <0x48000000 0x480000>;
+        clocks = <&mfgpll 0>;
+        clock-names = "core";
+        interrupts = <GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH 0>,
+                     <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH 0>,
+                     <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH 0>;
+        interrupt-names = "job", "mmu", "gpu";
+        performance-domains = <&gpufreq>;
+    };
 
 ...

-- 
2.51.0

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

[Nicolas Frattaroli]

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.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 .../bindings/devfreq/mediatek,mt8196-gpufreq.yaml  | 113 +++++++++++++++++++++
 1 file changed, 113 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..9d9efd4e70f1ef7ae446c833c15144beb9641b16
--- /dev/null
+++ b/Documentation/devicetree/bindings/devfreq/mediatek,mt8196-gpufreq.yaml
@@ -0,0 +1,113 @@
+# 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>
+
+description: |
+  A special-purpose embedded MCU to control power and frequency of GPU devices
+  using MediaTek Flexible Graphics integration hardware.
+
+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: hw_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: FastDVFS control
+
+  mbox-names:
+    items:
+      - const: fast-dvfs-event
+      - const: gpufreq
+      - const: sleep
+      - const: timer
+      - const: fhctl
+      - const: ccf
+      - const: fast-dvfs
+
+  shmem:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle to the shared memory region of the GPUEB MCU
+
+  "#performance-domain-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - mboxes
+  - mbox-names
+  - shmem
+  - "#performance-domain-cells"
+
+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", "hw_revision";
+        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 7>;
+        mbox-names = "fast-dvfs-event", "gpufreq", "sleep", "timer", "fhctl",
+                     "ccf", "fast-dvfs";
+        shmem = <&gpufreq_shmem>;
+        #performance-domain-cells = <0>;
+    };

-- 
2.51.0

[PATCH v2 03/10] dt-bindings: sram: Add compatible for mediatek,mt8196-gpufreq-sram

[Nicolas Frattaroli]

This compatible is used for an SRAM section that's shared between the
MT8196's application processor cores and the embedded GPUEB MCU that
controls the GPU frequency.

Through this SRAM section, things about the GPU frequency controller
like the OPP table can be read.

Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 Documentation/devicetree/bindings/sram/sram.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
index 7c1337e159f2371401ae99313375656fff014ed4..6ba0dd6a66def11f56a1d5276d7397b655bff11e 100644
--- a/Documentation/devicetree/bindings/sram/sram.yaml
+++ b/Documentation/devicetree/bindings/sram/sram.yaml
@@ -89,6 +89,7 @@ patternProperties:
             - arm,juno-scp-shmem
             - arm,scmi-shmem
             - arm,scp-shmem
+            - mediatek,mt8196-gpufreq-sram
             - renesas,smp-sram
             - rockchip,rk3066-smp-sram
             - samsung,exynos4210-sysram

-- 
2.51.0

[PATCH v2 04/10] dt-bindings: mailbox: Add MT8196 GPUEB Mailbox

[Nicolas Frattaroli]

The MediaTek MT8196 SoC includes an embedded MCU referred to as "GPUEB",
acting as glue logic to control power and frequency of the Mali GPU.
This MCU runs special-purpose firmware for this use, and the main
application processor communicates with it through a mailbox.

Add a binding that describes this mailbox.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 .../mailbox/mediatek,mt8196-gpueb-mbox.yaml        | 64 ++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,mt8196-gpueb-mbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,mt8196-gpueb-mbox.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..56508f406fce88c7c1699aa67b57394fc7b1c357
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/mediatek,mt8196-gpueb-mbox.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/mediatek,mt8196-gpueb-mbox.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek MFlexGraphics GPUEB Mailbox Controller
+
+maintainers:
+  - Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
+
+properties:
+  compatible:
+    enum:
+      - mediatek,mt8196-gpueb-mbox
+
+  reg:
+    items:
+      - description: mailbox data registers
+      - description: mailbox control registers
+
+  reg-names:
+    items:
+      - const: data
+      - const: ctl
+
+  clocks:
+    items:
+      - description: main clock of the GPUEB MCU
+
+  interrupts:
+    items:
+      - description: fires when a new message is received
+
+  "#mbox-cells":
+    const: 1
+    description:
+      The number of the mailbox channel.
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - interrupts
+  - "#mbox-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/mediatek,mt8196-clock.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    gpueb_mbox: mailbox@4b09fd80 {
+        compatible = "mediatek,mt8196-gpueb-mbox";
+        reg = <0x4b09fd80 0x280>,
+              <0x4b170000 0x7c>;
+        reg-names = "data", "ctl";
+        clocks = <&topckgen CLK_TOP_MFG_EB>;
+        interrupts = <GIC_SPI 608 IRQ_TYPE_LEVEL_HIGH 0>;
+        #mbox-cells = <1>;
+    };

-- 
2.51.0

[PATCH v2 05/10] mailbox: add MediaTek GPUEB IPI mailbox

[Nicolas Frattaroli]

The MT8196 SoC uses an embedded MCU to control frequencies and power of
the GPU. This controller is referred to as "GPUEB".

It communicates to the application processor, among other ways, through
a mailbox.

The mailbox exposes one interrupt, which appears to only be fired when a
response is received, rather than a transaction is completed. For us,
this means we unfortunately need to poll for txdone.

The mailbox also requires the EB clock to be on when touching any of the
mailbox registers.

Add a simple driver for it based on the common mailbox framework.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/mailbox/Kconfig             |  10 ++
 drivers/mailbox/Makefile            |   2 +
 drivers/mailbox/mtk-gpueb-mailbox.c | 337 ++++++++++++++++++++++++++++++++++++
 3 files changed, 349 insertions(+)

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index 02432d4a5ccd46a16156a09c7f277fb03e4013f5..2016defda1fabb5c0fcc8078f84a52d4e4e00167 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -294,6 +294,16 @@ config MTK_CMDQ_MBOX
 	  critical time limitation, such as updating display configuration
 	  during the vblank.
 
+config MTK_GPUEB_MBOX
+	tristate "MediaTek GPUEB Mailbox Support"
+	depends on ARCH_MEDIATEK || COMPILE_TEST
+	help
+	  The MediaTek GPUEB mailbox is used to communicate with the embedded
+	  controller in charge of GPU frequency and power management on some
+	  MediaTek SoCs, such as the MT8196.
+	  Say Y or m here if you want to support the MT8196 SoC in your kernel
+	  build.
+
 config ZYNQMP_IPI_MBOX
 	tristate "Xilinx ZynqMP IPI Mailbox"
 	depends on ARCH_ZYNQMP && OF
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 98a68f838486eed117d24296138bf59fda3f92e4..564d06e71313e6d1972e4a6036e1e78c8c7ec450 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -63,6 +63,8 @@ obj-$(CONFIG_MTK_ADSP_MBOX)	+= mtk-adsp-mailbox.o
 
 obj-$(CONFIG_MTK_CMDQ_MBOX)	+= mtk-cmdq-mailbox.o
 
+obj-$(CONFIG_MTK_GPUEB_MBOX)	+= mtk-gpueb-mailbox.o
+
 obj-$(CONFIG_ZYNQMP_IPI_MBOX)	+= zynqmp-ipi-mailbox.o
 
 obj-$(CONFIG_SUN6I_MSGBOX)	+= sun6i-msgbox.o
diff --git a/drivers/mailbox/mtk-gpueb-mailbox.c b/drivers/mailbox/mtk-gpueb-mailbox.c
new file mode 100644
index 0000000000000000000000000000000000000000..ac7530c6e71b3a3b39f956d6976c02eabdd4bb71
--- /dev/null
+++ b/drivers/mailbox/mtk-gpueb-mailbox.c
@@ -0,0 +1,337 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MediaTek GPUEB mailbox driver for SoCs such as the MT8196
+ *
+ * Copyright (C) 2025, Collabora Ltd.
+ *
+ * Developers harmed in the making of this driver:
+ *  - Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
+ */
+
+#include <linux/atomic.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#define MBOX_CTL_TX_STS 0x0000
+#define MBOX_CTL_IRQ_SET 0x0004
+#define MBOX_CTL_IRQ_CLR 0x0074
+#define MBOX_CTL_RX_STS 0x0078
+
+#define MBOX_FULL BIT(0) /* i.e. we've received data */
+#define MBOX_CLOGGED BIT(1) /* i.e. the channel is shutdown */
+
+struct mtk_gpueb_mbox {
+	struct device *dev;
+	struct clk *clk;
+	void __iomem *mbox_mmio;
+	void __iomem *mbox_ctl;
+	struct mbox_controller mbox;
+	struct mtk_gpueb_mbox_chan *ch;
+	int irq;
+	const struct mtk_gpueb_mbox_variant *v;
+};
+
+/**
+ * struct mtk_gpueb_mbox_chan - per-channel runtime data
+ * @ebm: pointer to the parent &struct mtk_gpueb_mbox mailbox
+ * @full_name: descriptive name of channel for IRQ subsystem
+ * @num: channel number, starting at 0
+ * @rx_buf: pointer to memory where received data is copied from MMIO
+ * @rx_status: signifies whether channel reception is turned off, or full
+ * @c: pointer to the constant &struct mtk_gpueb_mbox_chan_desc channel data
+ */
+struct mtk_gpueb_mbox_chan {
+	struct mtk_gpueb_mbox *ebm;
+	char *full_name;
+	u8 num;
+	void *rx_buf;
+	atomic_t rx_status;
+	const struct mtk_gpueb_mbox_chan_desc *c;
+};
+
+/**
+ * struct mtk_gpueb_mbox_chan_desc - per-channel constant data
+ * @name: name of this channel
+ * @num: index of this channel, starting at 0
+ * @tx_offset: byte offset measured from mmio base for outgoing data
+ * @tx_len: size, in bytes, of the outgoing data on this channel
+ * @rx_offset: bytes offset measured from mmio base for incoming data
+ * @rx_len: size, in bytes, of the incoming data on this channel
+ */
+struct mtk_gpueb_mbox_chan_desc {
+	const char *name;
+	const u8 num;
+	const u16 tx_offset;
+	const u8 tx_len;
+	const u16 rx_offset;
+	const u8 rx_len;
+};
+
+struct mtk_gpueb_mbox_variant {
+	const u8 num_channels;
+	const struct mtk_gpueb_mbox_chan_desc channels[] __counted_by(num_channels);
+};
+
+/**
+ * mtk_gpueb_mbox_read_rx - read RX buffer from MMIO into channel's RX buffer
+ * @chan: pointer to the channel to read
+ */
+static void mtk_gpueb_mbox_read_rx(struct mtk_gpueb_mbox_chan *chan)
+{
+	memcpy_fromio(chan->rx_buf, chan->ebm->mbox_mmio + chan->c->rx_offset,
+		      chan->c->rx_len);
+}
+
+static irqreturn_t mtk_gpueb_mbox_isr(int irq, void *data)
+{
+	struct mtk_gpueb_mbox_chan *ch = data;
+	u32 rx_sts;
+
+	rx_sts = readl(ch->ebm->mbox_ctl + MBOX_CTL_RX_STS);
+
+	if (rx_sts & BIT(ch->num)) {
+		if (!atomic_cmpxchg(&ch->rx_status, 0, MBOX_FULL | MBOX_CLOGGED))
+			return IRQ_WAKE_THREAD;
+	}
+
+	return IRQ_NONE;
+}
+
+static irqreturn_t mtk_gpueb_mbox_thread(int irq, void *data)
+{
+	struct mtk_gpueb_mbox_chan *ch = data;
+	int status;
+
+	status = atomic_cmpxchg(&ch->rx_status,
+				MBOX_FULL | MBOX_CLOGGED, MBOX_FULL);
+	if (status == (MBOX_FULL | MBOX_CLOGGED)) {
+		mtk_gpueb_mbox_read_rx(ch);
+		writel(BIT(ch->num), ch->ebm->mbox_ctl + MBOX_CTL_IRQ_CLR);
+		mbox_chan_received_data(&ch->ebm->mbox.chans[ch->num],
+					ch->rx_buf);
+		atomic_set(&ch->rx_status, 0);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static int mtk_gpueb_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+	struct mtk_gpueb_mbox_chan *ch = chan->con_priv;
+	int i;
+	u32 *values = data;
+
+	if (atomic_read(&ch->rx_status))
+		return -EBUSY;
+
+	/*
+	 * We don't want any fancy nonsense, just write the 32-bit values in
+	 * order. memcpy_toio/__iowrite32_copy don't work here, because fancy.
+	 */
+	for (i = 0; i < ch->c->tx_len; i += 4)
+		writel(values[i / 4], ch->ebm->mbox_mmio + ch->c->tx_offset + i);
+
+	writel(BIT(ch->num), ch->ebm->mbox_ctl + MBOX_CTL_IRQ_SET);
+
+	return 0;
+}
+
+static int mtk_gpueb_mbox_startup(struct mbox_chan *chan)
+{
+	struct mtk_gpueb_mbox_chan *ch = chan->con_priv;
+	int ret;
+
+	atomic_set(&ch->rx_status, 0);
+
+	ret = clk_enable(ch->ebm->clk);
+	if (ret) {
+		dev_err(ch->ebm->dev, "Failed to enable EB clock: %pe\n",
+			ERR_PTR(ret));
+		goto err_clog;
+	}
+
+	writel(BIT(ch->num), ch->ebm->mbox_ctl + MBOX_CTL_IRQ_CLR);
+
+	ret = devm_request_threaded_irq(ch->ebm->dev, ch->ebm->irq, mtk_gpueb_mbox_isr,
+					mtk_gpueb_mbox_thread, IRQF_SHARED | IRQF_ONESHOT,
+					ch->full_name, ch);
+	if (ret) {
+		dev_err(ch->ebm->dev, "Failed to request IRQ: %pe\n",
+			ERR_PTR(ret));
+		goto err_unclk;
+	}
+
+	return 0;
+
+err_unclk:
+	clk_disable(ch->ebm->clk);
+err_clog:
+	atomic_set(&ch->rx_status, MBOX_CLOGGED);
+
+	return ret;
+}
+
+static void mtk_gpueb_mbox_shutdown(struct mbox_chan *chan)
+{
+	struct mtk_gpueb_mbox_chan *ch = chan->con_priv;
+
+	atomic_set(&ch->rx_status, MBOX_CLOGGED);
+
+	devm_free_irq(ch->ebm->dev, ch->ebm->irq, ch);
+
+	clk_disable(ch->ebm->clk);
+}
+
+static bool mtk_gpueb_mbox_last_tx_done(struct mbox_chan *chan)
+{
+	struct mtk_gpueb_mbox_chan *ch = chan->con_priv;
+
+	return !(readl(ch->ebm->mbox_ctl + MBOX_CTL_TX_STS) & BIT(ch->num));
+}
+
+const struct mbox_chan_ops mtk_gpueb_mbox_ops = {
+	.send_data = mtk_gpueb_mbox_send_data,
+	.startup = mtk_gpueb_mbox_startup,
+	.shutdown = mtk_gpueb_mbox_shutdown,
+	.last_tx_done = mtk_gpueb_mbox_last_tx_done,
+};
+
+static struct mbox_chan *
+mtk_gpueb_mbox_of_xlate(struct mbox_controller *mbox,
+			const struct of_phandle_args *sp)
+{
+	struct mtk_gpueb_mbox *ebm = dev_get_drvdata(mbox->dev);
+
+	if (!sp->args_count)
+		return ERR_PTR(-EINVAL);
+
+	if (sp->args[0] >= ebm->v->num_channels)
+		return ERR_PTR(-ECHRNG);
+
+	return &mbox->chans[sp->args[0]];
+}
+
+static int mtk_gpueb_mbox_probe(struct platform_device *pdev)
+{
+	struct mtk_gpueb_mbox *ebm;
+	unsigned int rx_buf_sz;
+	void *buf;
+	unsigned int i;
+
+	ebm = devm_kzalloc(&pdev->dev, sizeof(*ebm), GFP_KERNEL);
+	if (!ebm)
+		return -ENOMEM;
+
+	ebm->dev = &pdev->dev;
+	ebm->v = of_device_get_match_data(ebm->dev);
+
+	ebm->irq = platform_get_irq(pdev, 0);
+	if (ebm->irq < 0)
+		return ebm->irq;
+
+	ebm->clk = devm_clk_get_prepared(ebm->dev, NULL);
+	if (IS_ERR(ebm->clk))
+		return dev_err_probe(ebm->dev, PTR_ERR(ebm->clk),
+				     "Failed to get 'eb' clock\n");
+
+	ebm->mbox_mmio = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ebm->mbox_mmio))
+		return dev_err_probe(ebm->dev, PTR_ERR(ebm->mbox_mmio),
+				     "Couldn't map mailbox data registers\n");
+
+	ebm->mbox_ctl = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(ebm->mbox_ctl))
+		return dev_err_probe(
+			ebm->dev, PTR_ERR(ebm->mbox_ctl),
+			"Couldn't map mailbox control registers\n");
+
+	rx_buf_sz = (ebm->v->channels[ebm->v->num_channels - 1].rx_offset +
+		     ebm->v->channels[ebm->v->num_channels - 1].rx_len -
+		     ebm->v->channels[0].rx_offset);
+
+	buf = devm_kzalloc(ebm->dev, rx_buf_sz, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	ebm->ch = devm_kmalloc_array(ebm->dev, ebm->v->num_channels,
+				     sizeof(*ebm->ch), GFP_KERNEL);
+	if (!ebm->ch)
+		return -ENOMEM;
+
+	ebm->mbox.chans = devm_kcalloc(ebm->dev, ebm->v->num_channels,
+				       sizeof(struct mbox_chan), GFP_KERNEL);
+	if (!ebm->mbox.chans)
+		return -ENOMEM;
+
+	for (i = 0; i < ebm->v->num_channels; i++) {
+		ebm->ch[i].c = &ebm->v->channels[i];
+		ebm->ch[i].full_name = devm_kasprintf(ebm->dev, GFP_KERNEL, "%s:%s",
+						      dev_name(ebm->dev), ebm->ch[i].c->name);
+		if (!ebm->ch[i].full_name)
+			return -ENOMEM;
+
+		ebm->ch[i].ebm = ebm;
+		ebm->ch[i].num = i;
+		ebm->ch[i].rx_buf = buf + ebm->ch[i].c->rx_offset -
+				    ebm->v->channels[0].rx_offset;
+		spin_lock_init(&ebm->mbox.chans[i].lock);
+		ebm->mbox.chans[i].con_priv = &ebm->ch[i];
+		atomic_set(&ebm->ch[i].rx_status, MBOX_CLOGGED);
+	}
+
+	ebm->mbox.dev = ebm->dev;
+	ebm->mbox.num_chans = ebm->v->num_channels;
+	ebm->mbox.txdone_poll = true;
+	ebm->mbox.txpoll_period = 0; /* minimum hrtimer interval */
+	ebm->mbox.of_xlate = mtk_gpueb_mbox_of_xlate;
+	ebm->mbox.ops = &mtk_gpueb_mbox_ops;
+
+	dev_set_drvdata(ebm->dev, ebm);
+
+	return devm_mbox_controller_register(ebm->dev, &ebm->mbox);
+}
+
+static const struct mtk_gpueb_mbox_variant mtk_gpueb_mbox_mt8196 = {
+	.num_channels = 12,
+	.channels = {
+		{ "fast-dvfs-event", 0, 0x0000, 16, 0x00e0, 16 },
+		{ "gpufreq",         1, 0x0010, 32, 0x00f0, 32 },
+		{ "sleep",           2, 0x0030, 12, 0x0110,  4 },
+		{ "timer",           3, 0x003c, 24, 0x0114,  4 },
+		{ "fhctl",           4, 0x0054, 36, 0x0118,  4 },
+		{ "ccf",             5, 0x0078, 16, 0x011c, 16 },
+		{ "gpumpu",          6, 0x0088, 24, 0x012c,  4 },
+		{ "fast-dvfs",       7, 0x00a0, 24, 0x0130, 24 },
+		{ "ipir-c-met",      8, 0x00b8,  4, 0x0148, 16 },
+		{ "ipis-c-met",      9, 0x00bc, 16, 0x0158,  4 },
+		{ "brisket",        10, 0x00cc, 16, 0x015c, 16 },
+		{ "ppb",            11, 0x00dc,  4, 0x016c,  4 },
+	},
+};
+
+static const struct of_device_id mtk_gpueb_mbox_of_ids[] = {
+	{ .compatible = "mediatek,mt8196-gpueb-mbox", .data = &mtk_gpueb_mbox_mt8196 },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mtk_gpueb_mbox_of_ids);
+
+static struct platform_driver mtk_gpueb_mbox_drv = {
+	.probe = mtk_gpueb_mbox_probe,
+	.driver = {
+		.name = "mtk-gpueb-mbox",
+		.of_match_table = mtk_gpueb_mbox_of_ids,
+	}
+};
+module_platform_driver(mtk_gpueb_mbox_drv);
+
+MODULE_AUTHOR("Nicolas Frattaroli <nicolas.frattaroli@collabora.com>");
+MODULE_DESCRIPTION("MediaTek GPUEB mailbox driver");
+MODULE_LICENSE("GPL");

-- 
2.51.0

[PATCH v2 06/10] drm/panthor: call into devfreq for current frequency

[Nicolas Frattaroli]

As it stands, panthor keeps a cached current frequency value for when it
wants to retrieve it. This doesn't work well for when things might
switch frequency without panthor's knowledge.

Instead, implement the get_cur_freq operation, and expose it through a
helper function to the rest of panthor.

Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_devfreq.c | 33 +++++++++++++++++++++++++++----
 drivers/gpu/drm/panthor/panthor_devfreq.h |  2 ++
 drivers/gpu/drm/panthor/panthor_device.h  |  3 ---
 drivers/gpu/drm/panthor/panthor_drv.c     |  4 +++-
 4 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
index 3686515d368db5bb329f4858d4a7247a4957cc24..8903f60c0a3f06313ac2008791c210ff32b6bd52 100644
--- a/drivers/gpu/drm/panthor/panthor_devfreq.c
+++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
@@ -62,7 +62,6 @@ static void panthor_devfreq_update_utilization(struct panthor_devfreq *pdevfreq)
 static int panthor_devfreq_target(struct device *dev, unsigned long *freq,
 				  u32 flags)
 {
-	struct panthor_device *ptdev = dev_get_drvdata(dev);
 	struct dev_pm_opp *opp;
 	int err;
 
@@ -72,8 +71,6 @@ static int panthor_devfreq_target(struct device *dev, unsigned long *freq,
 	dev_pm_opp_put(opp);
 
 	err = dev_pm_opp_set_rate(dev, *freq);
-	if (!err)
-		ptdev->current_frequency = *freq;
 
 	return err;
 }
@@ -115,11 +112,21 @@ static int panthor_devfreq_get_dev_status(struct device *dev,
 	return 0;
 }
 
+static int panthor_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
+{
+	struct panthor_device *ptdev = dev_get_drvdata(dev);
+
+	*freq = clk_get_rate(ptdev->clks.core);
+
+	return 0;
+}
+
 static struct devfreq_dev_profile panthor_devfreq_profile = {
 	.timer = DEVFREQ_TIMER_DELAYED,
 	.polling_ms = 50, /* ~3 frames */
 	.target = panthor_devfreq_target,
 	.get_dev_status = panthor_devfreq_get_dev_status,
+	.get_cur_freq = panthor_devfreq_get_cur_freq,
 };
 
 int panthor_devfreq_init(struct panthor_device *ptdev)
@@ -198,7 +205,6 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
 		return PTR_ERR(opp);
 
 	panthor_devfreq_profile.initial_freq = cur_freq;
-	ptdev->current_frequency = cur_freq;
 
 	/*
 	 * Set the recommend OPP this will enable and configure the regulator
@@ -296,3 +302,22 @@ void panthor_devfreq_record_idle(struct panthor_device *ptdev)
 
 	spin_unlock_irqrestore(&pdevfreq->lock, irqflags);
 }
+
+unsigned long panthor_devfreq_get_freq(struct panthor_device *ptdev)
+{
+	struct panthor_devfreq *pdevfreq = ptdev->devfreq;
+	unsigned long freq = 0;
+	int ret;
+
+	if (!pdevfreq || !pdevfreq->devfreq)
+		return 0;
+
+	if (pdevfreq->devfreq->profile->get_cur_freq) {
+		ret = pdevfreq->devfreq->profile->get_cur_freq(ptdev->base.dev,
+							       &freq);
+		if (ret)
+			return 0;
+	}
+
+	return freq;
+}
diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.h b/drivers/gpu/drm/panthor/panthor_devfreq.h
index b7631de695f7d79456478c87e8af5dc47673cd1d..f8e29e02f66cb3281ed4bb4c75cda9bd4df82b92 100644
--- a/drivers/gpu/drm/panthor/panthor_devfreq.h
+++ b/drivers/gpu/drm/panthor/panthor_devfreq.h
@@ -18,4 +18,6 @@ void panthor_devfreq_suspend(struct panthor_device *ptdev);
 void panthor_devfreq_record_busy(struct panthor_device *ptdev);
 void panthor_devfreq_record_idle(struct panthor_device *ptdev);
 
+unsigned long panthor_devfreq_get_freq(struct panthor_device *ptdev);
+
 #endif /* __PANTHOR_DEVFREQ_H__ */
diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index 4fc7cf2aeed577f623aac73ed287d6327645ecaa..a14239c8f9ca9229d8d6d36d327e6fd6d05f8f2f 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -200,9 +200,6 @@ struct panthor_device {
 	/** @profile_mask: User-set profiling flags for job accounting. */
 	u32 profile_mask;
 
-	/** @current_frequency: Device clock frequency at present. Set by DVFS*/
-	unsigned long current_frequency;
-
 	/** @fast_rate: Maximum device clock frequency. Set by DVFS */
 	unsigned long fast_rate;
 
diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
index fdbe89ef7f43c54d705b90275917dfdee16a0152..ba871466d7e05a95b51b3e5bb6723c587410f3d8 100644
--- a/drivers/gpu/drm/panthor/panthor_drv.c
+++ b/drivers/gpu/drm/panthor/panthor_drv.c
@@ -25,6 +25,7 @@
 #include <drm/gpu_scheduler.h>
 #include <drm/panthor_drm.h>
 
+#include "panthor_devfreq.h"
 #include "panthor_device.h"
 #include "panthor_fw.h"
 #include "panthor_gem.h"
@@ -1519,7 +1520,8 @@ static void panthor_gpu_show_fdinfo(struct panthor_device *ptdev,
 		drm_printf(p, "drm-cycles-panthor:\t%llu\n", pfile->stats.cycles);
 
 	drm_printf(p, "drm-maxfreq-panthor:\t%lu Hz\n", ptdev->fast_rate);
-	drm_printf(p, "drm-curfreq-panthor:\t%lu Hz\n", ptdev->current_frequency);
+	drm_printf(p, "drm-curfreq-panthor:\t%lu Hz\n",
+		   panthor_devfreq_get_freq(ptdev));
 }
 
 static void panthor_show_internal_memory_stats(struct drm_printer *p, struct drm_file *file)

-- 
2.51.0

[PATCH v2 07/10] drm/panthor: move panthor_devfreq struct to header

[Nicolas Frattaroli]

In order to make files other than panthor_devfreq.c be able to touch the
members of a panthor_devfreq instance, it needs to live somewhere other
than the .c file.

Move it into the panthor_devfreq.h header, so that the upcoming MediaTek
MFG devfreq can use it as well.

Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_devfreq.c | 32 ---------------------------
 drivers/gpu/drm/panthor/panthor_devfreq.h | 36 ++++++++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
index 8903f60c0a3f06313ac2008791c210ff32b6bd52..02eb3ca15d1874e1cbafc6b614b196c5cc75b6a1 100644
--- a/drivers/gpu/drm/panthor/panthor_devfreq.c
+++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
@@ -12,38 +12,6 @@
 #include "panthor_devfreq.h"
 #include "panthor_device.h"
 
-/**
- * struct panthor_devfreq - Device frequency management
- */
-struct panthor_devfreq {
-	/** @devfreq: devfreq device. */
-	struct devfreq *devfreq;
-
-	/** @gov_data: Governor data. */
-	struct devfreq_simple_ondemand_data gov_data;
-
-	/** @busy_time: Busy time. */
-	ktime_t busy_time;
-
-	/** @idle_time: Idle time. */
-	ktime_t idle_time;
-
-	/** @time_last_update: Last update time. */
-	ktime_t time_last_update;
-
-	/** @last_busy_state: True if the GPU was busy last time we updated the state. */
-	bool last_busy_state;
-
-	/**
-	 * @lock: Lock used to protect busy_time, idle_time, time_last_update and
-	 * last_busy_state.
-	 *
-	 * These fields can be accessed concurrently by panthor_devfreq_get_dev_status()
-	 * and panthor_devfreq_record_{busy,idle}().
-	 */
-	spinlock_t lock;
-};
-
 static void panthor_devfreq_update_utilization(struct panthor_devfreq *pdevfreq)
 {
 	ktime_t now, last;
diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.h b/drivers/gpu/drm/panthor/panthor_devfreq.h
index f8e29e02f66cb3281ed4bb4c75cda9bd4df82b92..e8b5ccddd45c52ee3215e9c84c6ebd9109640282 100644
--- a/drivers/gpu/drm/panthor/panthor_devfreq.h
+++ b/drivers/gpu/drm/panthor/panthor_devfreq.h
@@ -4,11 +4,45 @@
 #ifndef __PANTHOR_DEVFREQ_H__
 #define __PANTHOR_DEVFREQ_H__
 
+#include <linux/devfreq.h>
+
 struct devfreq;
 struct thermal_cooling_device;
 
 struct panthor_device;
-struct panthor_devfreq;
+
+/**
+ * struct panthor_devfreq - Device frequency management
+ */
+struct panthor_devfreq {
+	/** @devfreq: devfreq device. */
+	struct devfreq *devfreq;
+
+	/** @gov_data: Governor data. */
+	struct devfreq_simple_ondemand_data gov_data;
+
+	/** @busy_time: Busy time. */
+	ktime_t busy_time;
+
+	/** @idle_time: Idle time. */
+	ktime_t idle_time;
+
+	/** @time_last_update: Last update time. */
+	ktime_t time_last_update;
+
+	/** @last_busy_state: True if the GPU was busy last time we updated the state. */
+	bool last_busy_state;
+
+	/**
+	 * @lock: Lock used to protect busy_time, idle_time, time_last_update and
+	 * last_busy_state.
+	 *
+	 * These fields can be accessed concurrently by panthor_devfreq_get_dev_status()
+	 * and panthor_devfreq_record_{busy,idle}().
+	 */
+	spinlock_t lock;
+};
+
 
 int panthor_devfreq_init(struct panthor_device *ptdev);
 

-- 
2.51.0

[PATCH v2 08/10] drm/panthor: devfreq: expose get_dev_status and make it more generic

[Nicolas Frattaroli]

The only device-specific part of panthor's get_dev_status devfreq
callback is getting the clock frequency. All the other logic surrounding
what it does may be useful for other devfreq implementations however.

Expose it in the panthor_devfreq.h header file, and make it call back
into get_cur_freq instead of poking the common clock framework directly.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_devfreq.c | 11 ++++++++---
 drivers/gpu/drm/panthor/panthor_devfreq.h |  3 +++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
index 02eb3ca15d1874e1cbafc6b614b196c5cc75b6a1..34b621b155f1324ba4f0a07c981da669d945a545 100644
--- a/drivers/gpu/drm/panthor/panthor_devfreq.c
+++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
@@ -50,14 +50,18 @@ static void panthor_devfreq_reset(struct panthor_devfreq *pdevfreq)
 	pdevfreq->time_last_update = ktime_get();
 }
 
-static int panthor_devfreq_get_dev_status(struct device *dev,
-					  struct devfreq_dev_status *status)
+int panthor_devfreq_get_dev_status(struct device *dev,
+				   struct devfreq_dev_status *status)
 {
 	struct panthor_device *ptdev = dev_get_drvdata(dev);
 	struct panthor_devfreq *pdevfreq = ptdev->devfreq;
+	struct devfreq_dev_profile *p = pdevfreq->devfreq->profile;
 	unsigned long irqflags;
+	int ret;
 
-	status->current_frequency = clk_get_rate(ptdev->clks.core);
+	ret = p->get_cur_freq(dev, &status->current_frequency);
+	if (ret)
+		return ret;
 
 	spin_lock_irqsave(&pdevfreq->lock, irqflags);
 
@@ -79,6 +83,7 @@ static int panthor_devfreq_get_dev_status(struct device *dev,
 
 	return 0;
 }
+EXPORT_SYMBOL(panthor_devfreq_get_dev_status);
 
 static int panthor_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
 {
diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.h b/drivers/gpu/drm/panthor/panthor_devfreq.h
index e8b5ccddd45c52ee3215e9c84c6ebd9109640282..a891cb5fdc34636444f141e10f5d45828fc35b51 100644
--- a/drivers/gpu/drm/panthor/panthor_devfreq.h
+++ b/drivers/gpu/drm/panthor/panthor_devfreq.h
@@ -52,6 +52,9 @@ void panthor_devfreq_suspend(struct panthor_device *ptdev);
 void panthor_devfreq_record_busy(struct panthor_device *ptdev);
 void panthor_devfreq_record_idle(struct panthor_device *ptdev);
 
+int panthor_devfreq_get_dev_status(struct device *dev,
+				   struct devfreq_dev_status *status);
+
 unsigned long panthor_devfreq_get_freq(struct panthor_device *ptdev);
 
 #endif /* __PANTHOR_DEVFREQ_H__ */

-- 
2.51.0

[PATCH v2 09/10] drm/panthor: devfreq: add pluggable devfreq providers

[Nicolas Frattaroli]

On some devices, devfreq is not controlled directly by DT OPPs and the
common clock framework, but through an external devfreq driver. To
permit this type of usage, add the concept of devfreq providers.

Devfreq providers for panthor register themselves with panthor as a
provider. panthor then gets whatever device is pointed at on its
performance-domains property, finds the registered devfreq provider init
function for it, and calls it.

Should the probe order work out such that panthor probes before the
devfreq provider is finished probing and registering itself, then we
just defer the probe after adding a device link.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_devfreq.c | 74 ++++++++++++++++++++++++++++++-
 drivers/gpu/drm/panthor/panthor_devfreq.h | 16 +++++++
 2 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
index 34b621b155f1324ba4f0a07c981da669d945a545..84157916350cf506d315603a47bfc99436a787f8 100644
--- a/drivers/gpu/drm/panthor/panthor_devfreq.c
+++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
@@ -4,6 +4,7 @@
 #include <linux/clk.h>
 #include <linux/devfreq.h>
 #include <linux/devfreq_cooling.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pm_opp.h>
 
@@ -12,6 +13,34 @@
 #include "panthor_devfreq.h"
 #include "panthor_device.h"
 
+static LIST_HEAD(panthor_devfreq_providers);
+static DEFINE_MUTEX(panthor_devfreq_providers_lock);
+
+int panthor_devfreq_register_provider(struct panthor_devfreq_provider *prov)
+{
+	guard(mutex)(&panthor_devfreq_providers_lock);
+
+	list_add(&prov->node, &panthor_devfreq_providers);
+
+	return 0;
+}
+EXPORT_SYMBOL(panthor_devfreq_register_provider);
+
+static int panthor_devfreq_init_provider(struct panthor_device *ptdev,
+					 struct device *provider_dev)
+{
+	struct panthor_devfreq_provider *prov;
+
+	guard(mutex)(&panthor_devfreq_providers_lock);
+
+	list_for_each_entry(prov, &panthor_devfreq_providers, node) {
+		if (prov->dev == provider_dev)
+			return prov->init(ptdev, provider_dev);
+	}
+
+	return -EPROBE_DEFER;
+}
+
 static void panthor_devfreq_update_utilization(struct panthor_devfreq *pdevfreq)
 {
 	ktime_t now, last;
@@ -102,7 +131,7 @@ static struct devfreq_dev_profile panthor_devfreq_profile = {
 	.get_cur_freq = panthor_devfreq_get_cur_freq,
 };
 
-int panthor_devfreq_init(struct panthor_device *ptdev)
+static int panthor_devfreq_init_of(struct panthor_device *ptdev)
 {
 	/* There's actually 2 regulators (mali and sram), but the OPP core only
 	 * supports one.
@@ -222,6 +251,49 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
 	return 0;
 }
 
+static int panthor_devfreq_init_platform(struct panthor_device *ptdev)
+{
+	struct device_node *pcnode;
+	struct platform_device *pdev;
+	struct device_link *link;
+	int ret;
+
+	pcnode = of_parse_phandle(ptdev->base.dev->of_node,
+				  "performance-domains", 0);
+	if (!pcnode)
+		return -EINVAL;
+
+	pdev = of_find_device_by_node(pcnode);
+	of_node_put(pcnode);
+	if (!pdev)
+		return -ENODEV;
+
+	link = device_link_add(ptdev->base.dev, &pdev->dev,
+			       DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
+	if (!link) {
+		dev_err(ptdev->base.dev, "failed to add device link\n");
+		return -ENODEV;
+	}
+
+	ret = panthor_devfreq_init_provider(ptdev, &pdev->dev);
+	if (ret)
+		return dev_err_probe(ptdev->base.dev, ret,
+				     "failed to initialize devfreq provider\n");
+
+	DRM_DEV_INFO(ptdev->base.dev, "initialized devfreq provider %s\n",
+		     dev_name(&pdev->dev));
+
+	return 0;
+}
+
+int panthor_devfreq_init(struct panthor_device *ptdev)
+{
+	if (!of_property_present(ptdev->base.dev->of_node, "performance-domains"))
+		return panthor_devfreq_init_of(ptdev);
+
+	return panthor_devfreq_init_platform(ptdev);
+}
+
 void panthor_devfreq_resume(struct panthor_device *ptdev)
 {
 	struct panthor_devfreq *pdevfreq = ptdev->devfreq;
diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.h b/drivers/gpu/drm/panthor/panthor_devfreq.h
index a891cb5fdc34636444f141e10f5d45828fc35b51..94c9768d5d038c4ba8516929edb565a1f13443fb 100644
--- a/drivers/gpu/drm/panthor/panthor_devfreq.h
+++ b/drivers/gpu/drm/panthor/panthor_devfreq.h
@@ -8,6 +8,7 @@
 
 struct devfreq;
 struct thermal_cooling_device;
+struct platform_device;
 
 struct panthor_device;
 
@@ -43,6 +44,19 @@ struct panthor_devfreq {
 	spinlock_t lock;
 };
 
+struct panthor_devfreq_provider {
+	/** @dev: device pointer to the provider device */
+	struct device *dev;
+	/**
+	 * @init: the provider's init callback that allocates a
+	 * &struct panthor_devfreq, adds it to panthor, and adds a devfreq
+	 * device to panthor. Will be called during panthor's probe.
+	 */
+	int (*init)(struct panthor_device *ptdev, struct device *dev);
+
+	struct list_head node;
+};
+
 
 int panthor_devfreq_init(struct panthor_device *ptdev);
 
@@ -57,4 +71,6 @@ int panthor_devfreq_get_dev_status(struct device *dev,
 
 unsigned long panthor_devfreq_get_freq(struct panthor_device *ptdev);
 
+int panthor_devfreq_register_provider(struct panthor_devfreq_provider *prov);
+
 #endif /* __PANTHOR_DEVFREQ_H__ */

-- 
2.51.0

[PATCH v2 10/10] drm/panthor: add support for MediaTek MFlexGraphics

[Nicolas Frattaroli]

MediaTek uses some glue logic to control frequency and power on some of
their GPUs. This is best exposed as a devfreq driver, as it saves us
from having to hardcode OPPs into the device tree, and can be extended
with additional devfreq-y logic like more clever governors that use the
hardware's GPUEB MCU to set frame time targets and power limits.

Add this driver to the panthor subdirectory. It needs to live here as it
needs to call into panthor's devfreq layer, and panthor for its part
also needs to call into this driver during probe to get a devfreq device
registered. Solving the cyclical dependency by having mediatek_mfg live
without knowledge of what a panthor is would require moving the devfreq
provider stuff into a generic devfreq subsystem solution, which I didn't
want to do.

Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
 drivers/gpu/drm/panthor/Kconfig        |   13 +
 drivers/gpu/drm/panthor/Makefile       |    2 +
 drivers/gpu/drm/panthor/mediatek_mfg.c | 1053 ++++++++++++++++++++++++++++++++
 3 files changed, 1068 insertions(+)

diff --git a/drivers/gpu/drm/panthor/Kconfig b/drivers/gpu/drm/panthor/Kconfig
index 55b40ad07f3b0779e0c434469ddc874ff74fde27..c4d2599c05df9e0e009b8e99b3d29c220269ca0d 100644
--- a/drivers/gpu/drm/panthor/Kconfig
+++ b/drivers/gpu/drm/panthor/Kconfig
@@ -21,3 +21,16 @@ config DRM_PANTHOR
 
 	  Note that the Mali-G68 and Mali-G78, while Valhall architecture, will
 	  be supported with the panfrost driver as they are not CSF GPUs.
+
+config DRM_PANTHOR_MEDIATEK_MFG
+	tristate "MediaTek MFlexGraphics Extensions for Panthor"
+	depends on (DRM_PANTHOR && ARCH_MEDIATEK) || COMPILE_TEST
+	select DEVFREQ_GOV_SIMPLE_ONDEMAND
+	select PM_DEVFREQ
+	select MTK_GPUEB_MBOX
+	help
+	  Support for power and clock control in Panthor for MediaTek
+	  MFlexGraphics devices, such as the GPU on the MT8196 or MT6991 SoCs.
+
+	  These extensions are required for the GPU to work on these platforms,
+	  as they control the glue logic that powers on the GPU.
diff --git a/drivers/gpu/drm/panthor/Makefile b/drivers/gpu/drm/panthor/Makefile
index 02db21748c125688d2ef20ed254b5ebd7ff642e4..e0ebfdfb20bd78e0003c860c86c040746248fb89 100644
--- a/drivers/gpu/drm/panthor/Makefile
+++ b/drivers/gpu/drm/panthor/Makefile
@@ -12,4 +12,6 @@ panthor-y := \
 	panthor_mmu.o \
 	panthor_sched.o
 
+obj-$(CONFIG_DRM_PANTHOR_MEDIATEK_MFG) += mediatek_mfg.o
+
 obj-$(CONFIG_DRM_PANTHOR) += panthor.o
diff --git a/drivers/gpu/drm/panthor/mediatek_mfg.c b/drivers/gpu/drm/panthor/mediatek_mfg.c
new file mode 100644
index 0000000000000000000000000000000000000000..e06a1d5d8cbf81ac276b521df4b04c81b4c75f0b
--- /dev/null
+++ b/drivers/gpu/drm/panthor/mediatek_mfg.c
@@ -0,0 +1,1053 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for MediaTek MFlexGraphics Devices
+ *
+ * Copyright (C) 2025, Collabora Ltd.
+ */
+
+#include <linux/atomic.h>
+#include <linux/completion.h>
+#include <linux/clk.h>
+#include <linux/container_of.h>
+#include <linux/devfreq.h>
+#include <linux/iopoll.h>
+#include <linux/mailbox_client.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
+#include <linux/regulator/consumer.h>
+
+#include <drm/drm_managed.h>
+
+#include "panthor_devfreq.h"
+#include "panthor_device.h"
+
+#define GPR_LP_STATE		0x0028
+#define   EB_ON_SUSPEND		0x0
+#define   EB_ON_RESUME		0x1
+#define GPR_IPI_MAGIC		0x34
+
+#define RPC_PWR_CON		0x0504
+#define   PWR_ACK_M		GENMASK(31, 30)
+#define RPC_DUMMY_REG_2		0x0658
+#define RPC_GHPM_CFG0_CON	0x0800
+#define   GHPM_ENABLE_M		BIT(0)
+#define   GHPM_ON_SEQ_M		BIT(2)
+#define RPC_GHPM_RO0_CON	0x09A4
+#define   GHPM_STATE_M		GENMASK(7, 0)
+#define   GHPM_PWR_STATE_M	BIT(16)
+
+#define GF_REG_MAGIC			0x0000
+#define GF_REG_GPU_OPP_IDX		0x0004
+#define GF_REG_STK_OPP_IDX		0x0008
+#define GF_REG_GPU_OPP_NUM		0x000c
+#define GF_REG_STK_OPP_NUM		0x0010
+#define GF_REG_GPU_OPP_SNUM		0x0014
+#define GF_REG_STK_OPP_SNUM		0x0018
+#define GF_REG_POWER_COUNT		0x001c
+#define GF_REG_BUCK_COUNT		0x0020
+#define GF_REG_MTCMOS_COUNT		0x0024
+#define GF_REG_CG_COUNT			0x0028 /* CG = Clock Gate? */
+#define GF_REG_ACTIVE_COUNT		0x002C
+#define GF_REG_TEMP_RAW			0x0030
+#define GF_REG_TEMP_NORM_GPU		0x0034
+#define GF_REG_TEMP_HIGH_GPU		0x0038
+#define GF_REG_TEMP_NORM_STK		0x003C
+#define GF_REG_TEMP_HIGH_STK		0x0040
+#define GF_REG_FREQ_CUR_GPU		0x0044
+#define GF_REG_FREQ_CUR_STK		0x0048
+#define GF_REG_FREQ_OUT_GPU		0x004C /* Guess: actual achieved freq */
+#define GF_REG_FREQ_OUT_STK		0x0050 /* Guess: actual achieved freq */
+#define GF_REG_FREQ_METER_GPU		0x0054 /* Seems unused, always 0 */
+#define GF_REG_FREQ_METER_STK		0x0058 /* Seems unused, always 0 */
+#define GF_REG_VOLT_CUR_GPU		0x005C /* in tens of microvolts */
+#define GF_REG_VOLT_CUR_STK		0x0060 /* in tens of microvolts */
+#define GF_REG_VOLT_CUR_GPU_SRAM	0x0064
+#define GF_REG_VOLT_CUR_STK_SRAM	0x0068
+#define GF_REG_VOLT_CUR_GPU_REG		0x006C /* Seems unused, always 0 */
+#define GF_REG_VOLT_CUR_STK_REG		0x0070 /* Seems unused, always 0 */
+#define GF_REG_VOLT_CUR_GPU_REG_SRAM	0x0074
+#define GF_REG_VOLT_CUR_STK_REG_SRAM	0x0078
+#define GF_REG_PWR_CUR_GPU		0x007C /* in milliwatts */
+#define GF_REG_PWR_CUR_STK		0x0080 /* in milliwatts */
+#define GF_REG_PWR_MAX_GPU		0x0084 /* in milliwatts */
+#define GF_REG_PWR_MAX_STK		0x0088 /* in milliwatts */
+#define GF_REG_PWR_MIN_GPU		0x008C /* in milliwatts */
+#define GF_REG_PWR_MIN_STK		0x0090 /* in milliwatts */
+#define GF_REG_LEAKAGE_RT_GPU		0x0094 /* Unknown */
+#define GF_REG_LEAKAGE_RT_STK		0x0098 /* Unknown */
+#define GF_REG_LEAKAGE_RT_SRAM		0x009C /* Unknown */
+#define GF_REG_LEAKAGE_HT_GPU		0x00A0 /* Unknown */
+#define GF_REG_LEAKAGE_HT_STK		0x00A4 /* Unknown */
+#define GF_REG_LEAKAGE_HT_SRAM		0x00A8 /* Unknown */
+#define GF_REG_VOLT_DAC_LOW_GPU		0x00AC /* Seems unused, always 0 */
+#define GF_REG_VOLT_DAC_LOW_STK		0x00B0 /* Seems unused, always 0 */
+#define GF_REG_OPP_CUR_CEIL		0x00B4
+#define GF_REG_OPP_CUR_FLOOR		0x00B8
+#define GF_REG_OPP_CUR_LIMITER_CEIL	0x00BC
+#define GF_REG_OPP_CUR_LIMITER_FLOOR	0x00C0
+#define GF_REG_OPP_PRIORITY_CEIL	0x00C4
+#define GF_REG_OPP_PRIORITY_FLOOR	0x00C8
+#define GF_REG_PWR_CTL			0x00CC
+#define GF_REG_ACTIVE_SLEEP_CTL		0x00D0
+#define GF_REG_DVFS_STATE		0x00D4
+#define GF_REG_SHADER_PRESENT		0x00D8
+#define GF_REG_ASENSOR_ENABLE		0x00DC
+#define GF_REG_AGING_LOAD		0x00E0
+#define GF_REG_AGING_MARGIN		0x00E4
+#define GF_REG_AVS_ENABLE		0x00E8
+#define GF_REG_AVS_MARGIN		0x00EC
+#define GF_REG_CHIP_TYPE		0x00F0
+#define GF_REG_SB_VERSION		0x00F4
+#define GF_REG_PTP_VERSION		0x00F8
+#define GF_REG_DBG_VERSION		0x00FC
+#define GF_REG_KDBG_VERSION		0x0100
+#define GF_REG_GPM1_MODE		0x0104
+#define GF_REG_GPM3_MODE		0x0108
+#define GF_REG_DFD_MODE			0x010C
+#define GF_REG_DUAL_BUCK		0x0110
+#define GF_REG_SEGMENT_ID		0x0114
+#define GF_REG_POWER_TIME_H		0x0118
+#define GF_REG_POWER_TIME_L		0x011C
+#define GF_REG_PWR_STATUS		0x0120
+#define GF_REG_STRESS_TEST		0x0124
+#define GF_REG_TEST_MODE		0x0128
+#define GF_REG_IPS_MODE			0x012C
+#define GF_REG_TEMP_COMP_MODE		0x0130
+#define GF_REG_HT_TEMP_COMP_MODE	0x0134
+#define GF_REG_PWR_TRACKER_MODE		0x0138
+#define GF_REG_OPP_TABLE_GPU		0x0314
+#define GF_REG_OPP_TABLE_STK		0x09A4
+#define GF_REG_OPP_TABLE_GPU_S		0x1034
+#define GF_REG_OPP_TABLE_STK_S		0x16c4
+#define GF_REG_LIMIT_TABLE		0x1d54
+#define GF_REG_GPM3_TABLE		0x223C
+
+#define MFG_MT8196_E2_ID	0x101
+#define GPUEB_SLEEP_MAGIC	0x55667788UL
+
+#define GPUEB_TIMEOUT_US	10000UL
+#define GPUEB_POLL_US		50
+
+#define MAX_OPP_NUM             70
+
+/*
+ * This enum is part of the ABI of the GPUEB firmware. Don't change the
+ * numbering, as you would wreak havoc.
+ */
+enum mtk_mfg_ipi_cmd {
+	CMD_INIT_SHARED_MEM		= 0,
+	CMD_GET_FREQ_BY_IDX		= 1,
+	CMD_GET_POWER_BY_IDX		= 2,
+	CMD_GET_OPPIDX_BY_FREQ		= 3,
+	CMD_GET_LEAKAGE_POWER		= 4,
+	CMD_SET_LIMIT			= 5,
+	CMD_POWER_CONTROL		= 6,
+	CMD_ACTIVE_SLEEP_CONTROL	= 7,
+	CMD_COMMIT			= 8,
+	CMD_DUAL_COMMIT			= 9,
+	CMD_PDCA_CONFIG			= 10,
+	CMD_UPDATE_DEBUG_OPP_INFO	= 11,
+	CMD_SWITCH_LIMIT		= 12,
+	CMD_FIX_TARGET_OPPIDX		= 13,
+	CMD_FIX_DUAL_TARGET_OPPIDX	= 14,
+	CMD_FIX_CUSTOM_FREQ_VOLT	= 15,
+	CMD_FIX_DUAL_CUSTOM_FREQ_VOLT	= 16,
+	CMD_SET_MFGSYS_CONFIG		= 17,
+	CMD_MSSV_COMMIT			= 18,
+	CMD_NUM				= 19,
+};
+
+/*
+ * This struct is part of the ABI of the GPUEB firmware. Changing it, or
+ * reordering fields in it, will break things, so don't do it. Thank you.
+ */
+struct __packed mtk_mfg_ipi_msg {
+	__le32 magic;
+	__le32 cmd;
+	__le32 target;
+	/*
+	 * Downstream relies on the compiler to implicitly add the following
+	 * padding, as it declares the struct as non-packed.
+	 */
+	__le32 padding_lol;
+	union {
+		s32 __bitwise oppidx;
+		s32 __bitwise return_value;
+		__le32 freq;
+		__le32 volt;
+		__le32 power;
+		__le32 power_state;
+		__le32 mode;
+		__le32 value;
+		struct {
+			__le64 base;
+			__le32 size;
+		} shared_mem;
+		struct {
+			__le32 freq;
+			__le32 volt;
+		} custom;
+		struct {
+			__le32 limiter;
+			s32 __bitwise ceiling_info;
+			s32 __bitwise floor_info;
+		} set_limit;
+		struct {
+			__le32 target;
+			__le32 val;
+		} mfg_cfg;
+		struct {
+			__le32 target;
+			__le32 val;
+		} mssv;
+		struct {
+			s32 __bitwise gpu_oppidx;
+			s32 __bitwise stack_oppidx;
+		} dual_commit;
+		struct {
+			__le32 fgpu;
+			__le32 vgpu;
+			__le32 fstack;
+			__le32 vstack;
+		} dual_custom;
+	} u;
+};
+
+struct __packed mtk_mfg_ipi_sleep_msg {
+	__le32 event;
+	__le32 state;
+	__le32 magic;
+};
+
+/**
+ * struct mtk_mfg_opp_entry - OPP table entry from firmware
+ * @freq_khz: The operating point's frequency in kilohertz
+ * @voltage_core: The operating point's core voltage in tens of microvolts
+ * @voltage_sram: The operating point's SRAM voltage in tens of microvolts
+ * @posdiv: exponent of base 2 for PLL frequency divisor used for this OPP
+ * @voltage_margin: Number of tens of microvolts the voltage can be undershot
+ * @power_mw: estimate of power usage at this operating point, in milliwatts
+ *
+ * This struct is part of the ABI with the EB firmware. Do not change it.
+ */
+struct __packed mtk_mfg_opp_entry {
+	__le32 freq_khz;
+	__le32 voltage_core;
+	__le32 voltage_sram;
+	__le32 posdiv;
+	__le32 voltage_margin;
+	__le32 power_mw;
+};
+
+struct mtk_mfg_mbox {
+	struct mbox_client cl;
+	struct completion rx_done;
+	struct mtk_mfg *mfg;
+	struct mbox_chan *ch;
+	void *rx_data;
+};
+
+struct mtk_mfg {
+	struct platform_device *pdev;
+	struct panthor_device *ptdev;
+	struct clk *clk_eb;
+	struct clk_bulk_data *gpu_clks;
+	struct regulator_bulk_data *gpu_regs;
+	void __iomem *rpc;
+	void __iomem *gpr;
+	void __iomem *sram;
+	phys_addr_t sram_phys;
+	unsigned int sram_size;
+	unsigned int ghpm_en_reg;
+	u32 ipi_magic;
+	unsigned int num_opps;
+	unsigned int num_unique_gpu_opps;
+	struct dev_pm_opp_data *gpu_opps;
+	struct dev_pm_opp_data *stack_opps;
+	struct mtk_mfg_mbox *gf_mbox;
+	struct mtk_mfg_mbox *slp_mbox;
+	int last_opp;
+	const struct mtk_mfg_variant *variant;
+};
+
+struct mtk_mfg_variant {
+	const char *const *clk_names;
+	unsigned int num_clks;
+	const char *const *regulator_names;
+	unsigned int num_regulators;
+	int (*init)(struct mtk_mfg *mfg);
+};
+
+static inline void mtk_mfg_update_reg_bits(void __iomem *addr, u32 mask, u32 val)
+{
+	writel((readl(addr) & ~mask) | (val & mask), addr);
+}
+
+static inline unsigned long mtk_mfg_read_gpu_freq(struct mtk_mfg *mfg)
+{
+	return readl(mfg->sram + GF_REG_FREQ_CUR_GPU) * 1000UL;
+}
+
+static int mtk_mfg_eb_on(struct mtk_mfg *mfg)
+{
+	struct device *dev = &mfg->pdev->dev;
+	u32 val;
+	int ret;
+
+	/*
+	 * If MFG is already on from e.g. the bootloader, we should skip doing
+	 * the power-on sequence, as it wouldn't work without powering it off
+	 * first.
+	 */
+	if ((readl(mfg->rpc + RPC_PWR_CON) & PWR_ACK_M) == PWR_ACK_M)
+		return 0;
+
+	ret = readl_poll_timeout(mfg->rpc + RPC_GHPM_RO0_CON, val,
+				 !(val & (GHPM_PWR_STATE_M | GHPM_STATE_M)),
+				 GPUEB_POLL_US, GPUEB_TIMEOUT_US);
+	if (ret) {
+		dev_err(dev, "timed out waiting for EB to power on\n");
+		return ret;
+	}
+
+	mtk_mfg_update_reg_bits(mfg->rpc + mfg->ghpm_en_reg, GHPM_ENABLE_M,
+				GHPM_ENABLE_M);
+
+	mtk_mfg_update_reg_bits(mfg->rpc + RPC_GHPM_CFG0_CON, GHPM_ON_SEQ_M, 0);
+	mtk_mfg_update_reg_bits(mfg->rpc + RPC_GHPM_CFG0_CON, GHPM_ON_SEQ_M,
+				GHPM_ON_SEQ_M);
+
+	mtk_mfg_update_reg_bits(mfg->rpc + mfg->ghpm_en_reg, GHPM_ENABLE_M, 0);
+
+
+	ret = readl_poll_timeout(mfg->rpc + RPC_PWR_CON, val,
+				 (val & PWR_ACK_M) == PWR_ACK_M,
+				 GPUEB_POLL_US, GPUEB_TIMEOUT_US);
+	if (ret) {
+		dev_err(dev, "timed out waiting for EB power ack, val = 0x%X\n",
+			val);
+		return ret;
+	}
+
+	ret = readl_poll_timeout(mfg->gpr + GPR_LP_STATE, val,
+				 (val == EB_ON_RESUME),
+				 GPUEB_POLL_US, GPUEB_TIMEOUT_US);
+	if (ret) {
+		dev_err(dev, "timed out waiting for EB to resume, status = 0x%X\n", val);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int mtk_mfg_eb_off(struct mtk_mfg *mfg)
+{
+	struct mtk_mfg_ipi_sleep_msg msg = {
+		.event = 0,
+		.state = 0,
+		.magic = GPUEB_SLEEP_MAGIC
+	};
+	u32 val;
+	int ret;
+
+	ret = mbox_send_message(mfg->slp_mbox->ch, &msg);
+	if (ret < 0) {
+		dev_err(&mfg->pdev->dev, "failure in mbox comms: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	ret = readl_poll_timeout(mfg->rpc + RPC_PWR_CON, val,
+				 !(val & PWR_ACK_M), GPUEB_POLL_US,
+				 GPUEB_TIMEOUT_US);
+
+	if (ret) {
+		dev_err(&mfg->pdev->dev,
+			"timed out waiting for EB to power off, val=0x%08X\n",
+			val);
+	}
+
+	return ret;
+}
+
+static int mtk_mfg_send_ipi(struct mtk_mfg *mfg, struct mtk_mfg_ipi_msg *msg)
+{
+	int ret;
+
+	msg->magic = mfg->ipi_magic;
+
+	ret = mbox_send_message(mfg->gf_mbox->ch, msg);
+	if (ret < 0) {
+		dev_err(&mfg->pdev->dev, "error from mailbox subsystem: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	wait_for_completion(&mfg->gf_mbox->rx_done);
+
+	memcpy(msg, mfg->gf_mbox->rx_data, sizeof(*msg));
+
+	if (msg->u.return_value < 0) {
+		dev_err(&mfg->pdev->dev, "IPI return: %d\n",
+			msg->u.return_value);
+		return -EPROTO;
+	}
+
+	return 0;
+}
+
+static int mtk_mfg_init_shared_mem(struct mtk_mfg *mfg)
+{
+	struct device *dev = &mfg->pdev->dev;
+	struct mtk_mfg_ipi_msg msg = {};
+	int ret;
+
+	dev_info(dev, "clearing GPUEB sram, 0x%X bytes\n", mfg->sram_size);
+	memset_io(mfg->sram, 0, mfg->sram_size);
+
+	msg.cmd = CMD_INIT_SHARED_MEM;
+	msg.u.shared_mem.base = mfg->sram_phys;
+	msg.u.shared_mem.size = mfg->sram_size;
+
+	ret = mtk_mfg_send_ipi(mfg, &msg);
+	if (ret)
+		return ret;
+
+	if (readl(mfg->sram) != 0xBABADADA) {
+		dev_err(dev, "EB did not initialise SRAM correctly\n");
+		return -EIO;
+	}
+
+	dev_info(dev, "initialised mem at phys 0x%016llX\n", mfg->sram_phys);
+
+	return 0;
+}
+
+static int mtk_mfg_power_control(struct mtk_mfg *mfg, bool enabled)
+{
+	struct mtk_mfg_ipi_msg msg = {};
+
+	msg.cmd = CMD_POWER_CONTROL;
+	msg.u.power_state = enabled ? 1 : 0;
+
+	return mtk_mfg_send_ipi(mfg, &msg);
+}
+
+static int mtk_mfg_set_oppidx(struct mtk_mfg *mfg, int opp_idx)
+{
+	struct mtk_mfg_ipi_msg msg = {};
+	int ret;
+
+	if (!mfg->gpu_opps || !mfg->stack_opps)
+		return 0;
+
+	if (opp_idx >= mfg->num_opps)
+		return -EINVAL;
+
+	if (mfg->last_opp == opp_idx)
+		return 0;
+
+	msg.cmd = CMD_FIX_TARGET_OPPIDX;
+	msg.u.oppidx = opp_idx;
+
+	ret = mtk_mfg_send_ipi(mfg, &msg);
+	if (ret)
+		return ret;
+
+	mfg->last_opp = opp_idx;
+
+	return 0;
+}
+
+static int mtk_mfg_read_opp_tables(struct mtk_mfg *mfg)
+{
+	struct device *dev = &mfg->pdev->dev;
+	struct mtk_mfg_opp_entry e = {};
+	unsigned int i;
+	unsigned long long last_freq;
+
+	mfg->num_opps = readl(mfg->sram + GF_REG_GPU_OPP_NUM);
+	if (mfg->num_opps != readl(mfg->sram + GF_REG_STK_OPP_NUM)) {
+		dev_err(dev, "OPP count of GPU and Stack differ, %u vs. %u\n",
+			mfg->num_opps, readl(mfg->sram + GF_REG_STK_OPP_NUM));
+		return -EINVAL;
+	}
+
+	if (mfg->num_opps > MAX_OPP_NUM || mfg->num_opps == 0) {
+		dev_err(dev, "OPP count (%u) out of range %u >= count > 0\n",
+			mfg->num_opps, MAX_OPP_NUM);
+		return -EINVAL;
+	}
+
+	mfg->gpu_opps = devm_kcalloc(dev, mfg->num_opps,
+				     sizeof(struct dev_pm_opp_data), GFP_KERNEL);
+	if (!mfg->gpu_opps)
+		return -ENOMEM;
+
+	mfg->stack_opps = devm_kcalloc(dev, mfg->num_opps,
+				       sizeof(struct dev_pm_opp_data), GFP_KERNEL);
+	if (!mfg->stack_opps)
+		return -ENOMEM;
+
+	for (i = 0; i < mfg->num_opps; i++) {
+		memcpy_fromio(&e, mfg->sram + GF_REG_OPP_TABLE_GPU + i * sizeof(e),
+			      sizeof(e));
+		if (mem_is_zero(&e, sizeof(e))) {
+			dev_err(dev, "ran into an empty GPU OPP at index %u\n",
+				i);
+			return -EINVAL;
+		}
+		mfg->gpu_opps[i].freq = e.freq_khz * 1000ULL;
+		mfg->gpu_opps[i].u_volt = e.voltage_core * 10;
+
+		if (!last_freq || mfg->gpu_opps[i].freq != last_freq)
+			mfg->num_unique_gpu_opps++;
+
+		last_freq = mfg->gpu_opps[i].freq;
+	}
+
+	/* Why do I even bother? */
+	for (i = 0; i < mfg->num_opps; i++) {
+		memcpy_fromio(&e, mfg->sram + GF_REG_OPP_TABLE_STK + i * sizeof(e),
+			      sizeof(e));
+		if (mem_is_zero(&e, sizeof(e))) {
+			dev_err(dev, "ran into an empty GPU OPP at index %u\n",
+				i);
+			return -EINVAL;
+		}
+		mfg->stack_opps[i].freq = e.freq_khz * 1000ULL;
+		mfg->stack_opps[i].u_volt = e.voltage_core * 10;
+	}
+
+	return 0;
+}
+
+static inline bool mtk_mfg_opp_idx_match(struct mtk_mfg *mfg, int idx,
+					 unsigned long rate)
+{
+	if ((idx == mfg->num_opps - 1) && mfg->gpu_opps[idx].freq >= rate)
+		return true;
+
+	if (mfg->gpu_opps[idx].freq >= rate && mfg->gpu_opps[idx + 1].freq < rate)
+		return true;
+
+	return false;
+}
+
+/**
+ * mtk_mfg_get_closest_opp_idx - find OPP index for desired GPU frequency
+ * @mfg: pointer to a &struct mtk_mfg driver instance
+ * @gpu_rate: desired rate of the GPU core in Hz
+ *
+ * Given a desired target frequency for the GPU core, find the index of a
+ * matching OPP, or the next higher OPP if no exact match is found, or the
+ * maximum OPP for frequencies exceeding the maximum OPP's frequency.
+ *
+ * For duplicate OPP entries, chooses the highest OPP index, as in, the one
+ * closest to the next lower frequency OPP.
+ *
+ * Returns -EINVAL on error, or the OPP index on success.
+ */
+static int mtk_mfg_get_closest_opp_idx(struct mtk_mfg *mfg, unsigned long gpu_rate)
+{
+	int l = 0;
+	int r = mfg->num_opps - 1;
+	int m;
+
+	if (!mfg->gpu_opps)
+		return -EINVAL;
+
+	if (mfg->gpu_opps[0].freq <= gpu_rate)
+		return 0;
+
+	while (l <= r) {
+		m = l + (r - l) / 2;
+		if (mtk_mfg_opp_idx_match(mfg, m, gpu_rate)) {
+			return m;
+		} else if (mfg->gpu_opps[m].freq >= gpu_rate) /* >= for dupes */
+			l = m + 1;
+		else
+			r = m - 1;
+	}
+
+	return -EINVAL;
+}
+
+static int mtk_mfg_devfreq_target(struct device *dev, unsigned long *freq,
+				  u32 flags)
+{
+	struct panthor_device *ptdev = dev_get_drvdata(dev);
+	struct panthor_devfreq *pdevfreq = ptdev->devfreq;
+	struct mtk_mfg *mfg = dev_get_drvdata(&pdevfreq->devfreq->dev);
+	int ret, opp;
+
+	opp = mtk_mfg_get_closest_opp_idx(mfg, *freq);
+	if (opp < 0) {
+		dev_err(dev, "Couldn't get closest OPP: %pe\n", ERR_PTR(opp));
+		return opp;
+	}
+
+	ret = pm_runtime_resume_and_get(&mfg->pdev->dev);
+	if (ret)
+		return ret;
+
+	ret =  mtk_mfg_set_oppidx(mfg, opp);
+	if (!ret)
+		*freq = mtk_mfg_read_gpu_freq(mfg);
+	else
+		dev_err(dev, "Couldn't set OPP: %pe\n", ERR_PTR(ret));
+
+	pm_runtime_put(&mfg->pdev->dev);
+
+	return ret;
+}
+
+static int mtk_mfg_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
+{
+	struct panthor_device *ptdev = dev_get_drvdata(dev);
+	struct panthor_devfreq *pdevfreq = ptdev->devfreq;
+	struct mtk_mfg *mfg;
+	int ret;
+
+	if (IS_ERR_OR_NULL(pdevfreq->devfreq))
+		return -ENODEV;
+
+	mfg = dev_get_drvdata(&pdevfreq->devfreq->dev);
+	if (IS_ERR_OR_NULL(mfg)) {
+		*freq = pdevfreq->devfreq->profile->initial_freq;
+		return 0;
+	}
+
+	ret = pm_runtime_resume_and_get(&mfg->pdev->dev);
+	if (ret)
+		return ret;
+
+	*freq = mtk_mfg_read_gpu_freq(mfg);
+
+	pm_runtime_put(&mfg->pdev->dev);
+
+	return 0;
+}
+
+static const struct devfreq_dev_profile mtk_mfg_devfreq_profile = {
+	.timer = DEVFREQ_TIMER_DELAYED,
+	.polling_ms = 50, /* ~3 frames */
+	.target = mtk_mfg_devfreq_target,
+	.get_dev_status = panthor_devfreq_get_dev_status,
+	.get_cur_freq = mtk_mfg_devfreq_get_cur_freq,
+};
+
+static const char *const mtk_mfg_mt8196_clk_names[] = {
+	"mfgpll",
+	"mfgpll_sc0",
+	"mfgpll_sc1",
+};
+
+static const char *const mtk_mfg_mt8196_regulators[] = {
+	"core",
+	"stack",
+	"sram",
+};
+
+static int mtk_mfg_mt8196_init(struct mtk_mfg *mfg)
+{
+	void __iomem *e2_base;
+
+	e2_base = devm_platform_ioremap_resource_byname(mfg->pdev, "hw_revision");
+	if (IS_ERR(e2_base))
+		return dev_err_probe(&mfg->pdev->dev, PTR_ERR(e2_base),
+				     "Couldn't get hw_revision register\n");
+
+	if (readl(e2_base) == MFG_MT8196_E2_ID)
+		mfg->ghpm_en_reg = RPC_DUMMY_REG_2;
+	else
+		mfg->ghpm_en_reg = RPC_GHPM_CFG0_CON;
+
+	return 0;
+};
+
+static const struct mtk_mfg_variant mtk_mfg_mt8196_variant = {
+	.clk_names = mtk_mfg_mt8196_clk_names,
+	.num_clks = ARRAY_SIZE(mtk_mfg_mt8196_clk_names),
+	.regulator_names = mtk_mfg_mt8196_regulators,
+	.num_regulators = ARRAY_SIZE(mtk_mfg_mt8196_regulators),
+	.init = mtk_mfg_mt8196_init,
+};
+
+static int mtk_mfg_init_devfreq(struct panthor_device *ptdev,
+				struct device *gpufreq_dev)
+{
+	struct device *gpu = ptdev->base.dev;
+	struct panthor_devfreq *pdevfreq;
+	struct mtk_mfg *mfg;
+	struct devfreq_dev_profile *profile;
+	struct dev_pm_opp *opp;
+	int j = 0;
+	int i, ret;
+
+	mfg = dev_get_drvdata(gpufreq_dev);
+
+	if (!mfg)
+		return -EPROBE_DEFER;
+
+	if (!mfg->gpu_opps)
+		return -EPROBE_DEFER;
+
+	pdevfreq = drmm_kzalloc(&ptdev->base, sizeof(*ptdev->devfreq), GFP_KERNEL);
+	if (!pdevfreq)
+		return -ENOMEM;
+
+	ptdev->devfreq = pdevfreq;
+	mfg->ptdev = ptdev;
+
+	spin_lock_init(&pdevfreq->lock);
+
+	profile = devm_kmemdup(gpu, &mtk_mfg_devfreq_profile, sizeof(*profile),
+			       GFP_KERNEL);
+
+	if (!profile)
+		return -ENOMEM;
+
+	ret = pm_runtime_resume_and_get(&mfg->pdev->dev);
+	if (ret) {
+		dev_err(gpufreq_dev, "could not resume runtime pm: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+	profile->initial_freq = mtk_mfg_read_gpu_freq(mfg);
+	pm_runtime_put(&mfg->pdev->dev);
+
+	profile->freq_table = devm_kcalloc(gpu, mfg->num_unique_gpu_opps,
+					   sizeof(*profile->freq_table),
+					   GFP_KERNEL);
+	if (!profile->freq_table)
+		return -ENOMEM;
+
+	profile->max_state = mfg->num_unique_gpu_opps;
+
+	for (i = mfg->num_opps - 1; i >= 0 && j < profile->max_state; i--) {
+		if ((j == 0) || (mfg->gpu_opps[i].freq != profile->freq_table[j - 1])) {
+			profile->freq_table[j] = mfg->gpu_opps[i].freq;
+			ret = dev_pm_opp_add_dynamic(gpu, &mfg->gpu_opps[i]);
+			if (ret) {
+				dev_err(gpu, "couldn't add OPP %d: %pe\n",
+					i, ERR_PTR(ret));
+				return ret;
+			}
+			j++;
+		}
+	}
+
+	opp = devfreq_recommended_opp(gpu, &profile->initial_freq, 0);
+	if (IS_ERR(opp)) {
+		dev_err(gpufreq_dev, "failed getting OPP recommendation: %pe\n",
+			opp);
+		return PTR_ERR(opp);
+	}
+
+	ret = dev_pm_opp_set_opp(gpu, opp);
+	dev_pm_opp_put(opp);
+	if (ret) {
+		dev_err(gpu, "Couldn't set recommended OPP\n");
+		return ret;
+	}
+
+	ptdev->fast_rate = profile->freq_table[profile->max_state - 1];
+
+	pdevfreq->time_last_update = ktime_get();
+	pdevfreq->gov_data.upthreshold = 45;
+	pdevfreq->gov_data.downdifferential = 5;
+
+	pdevfreq->devfreq = devm_devfreq_add_device(ptdev->base.dev, profile,
+						    DEVFREQ_GOV_SIMPLE_ONDEMAND,
+						    &pdevfreq->gov_data);
+	if (IS_ERR(pdevfreq->devfreq)) {
+		dev_err(gpu, "Failed to register devfreq device: %pe\n",
+			pdevfreq->devfreq);
+		return PTR_ERR(pdevfreq->devfreq);
+	}
+
+	/*
+	 * There are some ways devfreq_add_device can fail without setting an
+	 * error pointer, but setting a NULL pointer instead. We really don't
+	 * want to return 0 in the above check, so return -EINVAL here.
+	 */
+	if (!pdevfreq->devfreq) {
+		dev_err(gpu, "Failed to register devfreq device\n");
+		return -EINVAL;
+	}
+
+	dev_set_drvdata(&pdevfreq->devfreq->dev, mfg);
+
+	return 0;
+}
+
+static void mtk_mfg_mbox_rx_callback(struct mbox_client *cl, void *mssg)
+{
+	struct mtk_mfg_mbox *mb = container_of(cl, struct mtk_mfg_mbox, cl);
+
+	mb->rx_data = mssg;
+	complete(&mb->rx_done);
+}
+
+static int mtk_mfg_init_mbox(struct mtk_mfg *mfg)
+{
+	struct device *dev = &mfg->pdev->dev;
+	struct mtk_mfg_mbox *gf;
+	struct mtk_mfg_mbox *slp;
+
+	gf = devm_kzalloc(dev, sizeof(*gf), GFP_KERNEL);
+	if (!gf)
+		return -ENOMEM;
+
+	slp = devm_kzalloc(dev, sizeof(*slp), GFP_KERNEL);
+	if (!slp)
+		return -ENOMEM;
+
+	gf->mfg = mfg;
+	init_completion(&gf->rx_done);
+	gf->cl.dev = dev;
+	gf->cl.rx_callback = mtk_mfg_mbox_rx_callback;
+	gf->cl.tx_tout = GPUEB_TIMEOUT_US / USEC_PER_MSEC;
+	gf->ch = mbox_request_channel_byname(&gf->cl, "gpufreq");
+	if (IS_ERR(gf->ch))
+		return PTR_ERR(gf->ch);
+
+	mfg->gf_mbox = gf;
+
+	slp->mfg = mfg;
+	init_completion(&slp->rx_done);
+	slp->cl.dev = dev;
+	slp->cl.tx_tout = GPUEB_TIMEOUT_US / USEC_PER_MSEC;
+	slp->cl.tx_block = true;
+	slp->ch = mbox_request_channel_byname(&slp->cl, "sleep");
+	if (IS_ERR(slp->ch))
+		return PTR_ERR(slp->ch);
+
+	mfg->slp_mbox = slp;
+
+	return 0;
+}
+
+static const struct of_device_id mtk_mfg_of_match[] = {
+	{ .compatible = "mediatek,mt8196-gpufreq", .data = &mtk_mfg_mt8196_variant },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mtk_mfg_of_match);
+
+static int mtk_mfg_probe(struct platform_device *pdev)
+{
+	struct device_node *shmem __free(device_node);
+	struct panthor_devfreq_provider *prov;
+	struct mtk_mfg *mfg;
+	struct device *dev = &pdev->dev;
+	const struct mtk_mfg_variant *data = of_device_get_match_data(dev);
+	struct resource res;
+	int ret;
+	int i;
+
+	mfg = devm_kzalloc(dev, sizeof(*mfg), GFP_KERNEL);
+	if (!mfg)
+		return -ENOMEM;
+
+	prov = devm_kzalloc(dev, sizeof(*prov), GFP_KERNEL);
+	if (!prov)
+		return -ENOMEM;
+
+	prov->dev = dev;
+	prov->init = mtk_mfg_init_devfreq;
+
+	mfg->pdev = pdev;
+	mfg->variant = data;
+
+	dev_set_drvdata(dev, mfg);
+
+	mfg->gpr = devm_platform_ioremap_resource_byname(pdev, "gpr");
+	if (IS_ERR(mfg->gpr))
+		return dev_err_probe(dev, PTR_ERR(mfg->gpr),
+				     "Could not retrieve GPR MMIO registers\n");
+
+	mfg->rpc = devm_platform_ioremap_resource_byname(pdev, "rpc");
+	if (IS_ERR(mfg->rpc))
+		return dev_err_probe(dev, PTR_ERR(mfg->rpc),
+				     "Could not retrieve RPC MMIO registers\n");
+
+	mfg->clk_eb = devm_clk_get(dev, "eb");
+	if (IS_ERR(mfg->clk_eb))
+		return dev_err_probe(dev, PTR_ERR(mfg->clk_eb),
+				     "Could not get 'eb' clock\n");
+
+	mfg->gpu_clks = devm_kcalloc(dev, data->num_clks, sizeof(*mfg->gpu_clks),
+				     GFP_KERNEL);
+	if (!mfg->gpu_clks)
+		return -ENOMEM;
+
+	for (i = 0; i < data->num_clks; i++)
+		mfg->gpu_clks[i].id = data->clk_names[i];
+
+	ret = devm_clk_bulk_get(dev, data->num_clks, mfg->gpu_clks);
+	if (ret)
+		return dev_err_probe(dev, ret, "couldn't get GPU clocks\n");
+
+	mfg->gpu_regs = devm_kcalloc(dev, data->num_regulators,
+				     sizeof(*mfg->gpu_regs), GFP_KERNEL);
+	if (!mfg->gpu_regs)
+		return -ENOMEM;
+
+	for (i = 0; i < data->num_regulators; i++)
+		mfg->gpu_regs[i].supply = data->regulator_names[i];
+
+	ret = devm_regulator_bulk_get(dev, data->num_regulators, mfg->gpu_regs);
+	if (ret)
+		return dev_err_probe(dev, ret, "couldn't get GPU regulators\n");
+
+	shmem = of_parse_phandle(dev->of_node, "shmem", 0);
+	if (!shmem)
+		return dev_err_probe(dev, -ENODEV, "Could not get 'shmem'\n");
+
+	ret = of_address_to_resource(shmem, 0, &res);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to get GPUEB shared memory\n");
+
+	mfg->sram = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!mfg->sram)
+		return dev_err_probe(dev, -EADDRNOTAVAIL,
+				     "failed to ioremap GPUEB sram\n");
+	mfg->sram_size = resource_size(&res);
+	mfg->sram_phys = res.start;
+
+	if (data->init) {
+		ret = data->init(mfg);
+		if (ret)
+			return dev_err_probe(dev, ret, "Variant init failed\n");
+	}
+
+	ret = clk_prepare_enable(mfg->clk_eb);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to turn on EB clock\n");
+	mfg->ipi_magic = readl(mfg->gpr + GPR_IPI_MAGIC);
+	writel(0x0, mfg->gpr + GPR_IPI_MAGIC); /* why. */
+
+	ret = mtk_mfg_init_mbox(mfg);
+	if (ret) {
+		ret = dev_err_probe(dev, ret, "Couldn't initialise mailbox\n");
+		goto out;
+	}
+
+	clk_disable_unprepare(mfg->clk_eb);
+
+	mfg->last_opp = -1;
+
+	devm_pm_runtime_enable(dev);
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to power on MFG\n");
+
+	ret = mtk_mfg_init_shared_mem(mfg);
+	if (ret) {
+		dev_err(dev, "Couldn't initialize EB SRAM: %pe\n", ERR_PTR(ret));
+		goto out;
+	}
+
+	ret = mtk_mfg_read_opp_tables(mfg);
+	if (ret) {
+		dev_err(dev, "Error reading OPP tables from EB: %pe\n",
+			ERR_PTR(ret));
+		goto out;
+	}
+
+	panthor_devfreq_register_provider(prov);
+
+out:
+	pm_runtime_put(dev);
+	return ret;
+}
+
+static int mtk_mfg_suspend(struct device *dev)
+{
+	struct mtk_mfg *mfg = dev_get_drvdata(dev);
+	int ret;
+
+	ret = mtk_mfg_power_control(mfg, false);
+	if (ret) {
+		dev_err(dev, "power_control failed: %pe\n", ERR_PTR(ret));
+		return ret;
+	}
+	ret = mtk_mfg_eb_off(mfg);
+	if (ret) {
+		dev_err(dev, "eb_off failed: %pe\n", ERR_PTR(ret));
+		return ret;
+	}
+	clk_bulk_disable_unprepare(mfg->variant->num_clks, mfg->gpu_clks);
+	clk_disable_unprepare(mfg->clk_eb);
+	ret = regulator_bulk_disable(mfg->variant->num_regulators, mfg->gpu_regs);
+
+	return ret;
+}
+
+static int mtk_mfg_resume(struct device *dev)
+{
+	struct mtk_mfg *mfg = dev_get_drvdata(dev);
+	int ret;
+
+	ret = regulator_bulk_enable(mfg->variant->num_regulators,
+				    mfg->gpu_regs);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(mfg->clk_eb);
+	if (ret)
+		goto err_disable_regulators;
+
+	ret = clk_bulk_prepare_enable(mfg->variant->num_clks, mfg->gpu_clks);
+	if (ret)
+		goto err_disable_eb_clk;
+
+	ret = mtk_mfg_eb_on(mfg);
+	if (ret)
+		goto err_disable_clks;
+
+	ret = mtk_mfg_power_control(mfg, true);
+	if (ret)
+		goto err_eb_off;
+
+	return 0;
+
+err_eb_off:
+	mtk_mfg_eb_off(mfg);
+err_disable_clks:
+	clk_bulk_disable_unprepare(mfg->variant->num_clks, mfg->gpu_clks);
+err_disable_eb_clk:
+	clk_disable_unprepare(mfg->clk_eb);
+err_disable_regulators:
+	regulator_bulk_disable(mfg->variant->num_regulators, mfg->gpu_regs);
+
+	return ret;
+}
+
+
+static DEFINE_RUNTIME_DEV_PM_OPS(mtk_mfg_pm_ops,
+				 mtk_mfg_suspend,
+				 mtk_mfg_resume,
+				 NULL);
+
+static struct platform_driver mtk_mfg_driver = {
+	.driver = {
+		.name = "panthor-mtk-mfg",
+		.of_match_table = mtk_mfg_of_match,
+		.pm = pm_ptr(&mtk_mfg_pm_ops),
+	},
+	.probe = mtk_mfg_probe,
+};
+module_platform_driver(mtk_mfg_driver);
+
+MODULE_SOFTDEP("pre: panthor");
+MODULE_AUTHOR("Nicolas Frattaroli <nicolas.frattaroli@collabora.com>");
+MODULE_DESCRIPTION("MediaTek MFlexGraphics Extension for Panthor");
+MODULE_LICENSE("GPL");

-- 
2.51.0

Re: [PATCH v2 01/10] dt-bindings: gpu: mali-valhall-csf: add mediatek,mt8196-mali variant

[Chia-I Wu]

On Fri, Sep 12, 2025 at 11:38 AM Nicolas Frattaroli
<nicolas.frattaroli@collabora.com> wrote:
>
> The Mali-based GPU on the MediaTek MT8196 SoC uses a separate MCU to
> control the power and frequency of the GPU.
>
> It lets us omit the OPP tables from the device tree, as those can now be
> enumerated at runtime from the MCU.
>
> Add the mediatek,mt8196-mali compatible, and a performance-domains
> property which points to the MCU's device tree node in this case. It's
> required on mt8196 devices.
>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  .../bindings/gpu/arm,mali-valhall-csf.yaml         | 32 +++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
> index a5b4e00217587c5d1f889094e2fff7b76e6148eb..163b4457f7f25dcdd509c558558a73694521c96d 100644
> --- a/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
> +++ b/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
> @@ -19,6 +19,7 @@ properties:
>        - items:
>            - enum:
>                - rockchip,rk3588-mali
> +              - mediatek,mt8196-mali
>            - const: arm,mali-valhall-csf   # Mali Valhall GPU model/revision is fully discoverable
>
>    reg:
> @@ -53,6 +54,9 @@ properties:
>    opp-table:
>      type: object
>
> +  performance-domains:
> +    maxItems: 1
> +
>    power-domains:
>      minItems: 1
>      maxItems: 5
> @@ -91,7 +95,6 @@ required:
>    - interrupts
>    - interrupt-names
>    - clocks
> -  - mali-supply
>
>  additionalProperties: false
>
> @@ -105,9 +108,24 @@ allOf:
>        properties:
>          clocks:
>            minItems: 3
> +        performance-domains: false
>          power-domains:
>            maxItems: 1
>          power-domain-names: false
> +      required:
> +        - mali-supply
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: mediatek,mt8196-mali
> +    then:
> +      properties:
> +        mali-supply: false
> +        sram-supply: false
> +        operating-points-v2: false
> +      required:
> +        - performance-domains
>
>  examples:
>    - |
> @@ -143,5 +161,17 @@ examples:
>              };
>          };
>      };
> +  - |
> +    gpu@48000000 {
> +        compatible = "mediatek,mt8196-mali", "arm,mali-valhall-csf";
> +        reg = <0x48000000 0x480000>;
> +        clocks = <&mfgpll 0>;
This seems to be an input to the performance domain, not to the gpu.
The rule says

  clocks:
    minItems: 1
  power-domains:
    minItems: 1

but neither is needed on mt8196. Should we set both to 0 (and update
panthor to treat core clock as optional)?

> +        clock-names = "core";
> +        interrupts = <GIC_SPI 606 IRQ_TYPE_LEVEL_HIGH 0>,
> +                     <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH 0>,
> +                     <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH 0>;
> +        interrupt-names = "job", "mmu", "gpu";
> +        performance-domains = <&gpufreq>;
> +    };
>
>  ...
>
> --
> 2.51.0
>

Re: [PATCH v2 05/10] mailbox: add MediaTek GPUEB IPI mailbox

[Chia-I Wu]

On Fri, Sep 12, 2025 at 11:38 AM Nicolas Frattaroli
<nicolas.frattaroli@collabora.com> wrote:
<snipped>
> +static irqreturn_t mtk_gpueb_mbox_thread(int irq, void *data)
> +{
> +       struct mtk_gpueb_mbox_chan *ch = data;
> +       int status;
> +
> +       status = atomic_cmpxchg(&ch->rx_status,
> +                               MBOX_FULL | MBOX_CLOGGED, MBOX_FULL);
> +       if (status == (MBOX_FULL | MBOX_CLOGGED)) {
> +               mtk_gpueb_mbox_read_rx(ch);
> +               writel(BIT(ch->num), ch->ebm->mbox_ctl + MBOX_CTL_IRQ_CLR);
> +               mbox_chan_received_data(&ch->ebm->mbox.chans[ch->num],
> +                                       ch->rx_buf);
Given what other drivers do, and how mtk_mfg consumes the data, we should

  char buf[MAX_OF_RX_LEN]; //  MAX_OF_RX_LEN is 32; we can also
allocate it during probe
  mtk_gpueb_mbox_read_rx(ch);
  mbox_chan_received_data(..., buf);

mtx_mfg makes a copy eventually anyway. We don't need to maintain any
extra copy.

Then we might not need rx_status.

> +               atomic_set(&ch->rx_status, 0);
> +               return IRQ_HANDLED;
> +       }
> +
> +       return IRQ_NONE;
> +}
> +
> +static int mtk_gpueb_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
> +       struct mtk_gpueb_mbox_chan *ch = chan->con_priv;
> +       int i;
> +       u32 *values = data;
> +
> +       if (atomic_read(&ch->rx_status))
> +               return -EBUSY;
> +
> +       /*
> +        * We don't want any fancy nonsense, just write the 32-bit values in
> +        * order. memcpy_toio/__iowrite32_copy don't work here, because fancy.
> +        */
> +       for (i = 0; i < ch->c->tx_len; i += 4)
> +               writel(values[i / 4], ch->ebm->mbox_mmio + ch->c->tx_offset + i);
> +
> +       writel(BIT(ch->num), ch->ebm->mbox_ctl + MBOX_CTL_IRQ_SET);
> +
> +       return 0;
> +}
> +
> +static int mtk_gpueb_mbox_startup(struct mbox_chan *chan)
> +{
> +       struct mtk_gpueb_mbox_chan *ch = chan->con_priv;
> +       int ret;
> +
> +       atomic_set(&ch->rx_status, 0);
> +
> +       ret = clk_enable(ch->ebm->clk);
> +       if (ret) {
> +               dev_err(ch->ebm->dev, "Failed to enable EB clock: %pe\n",
> +                       ERR_PTR(ret));
> +               goto err_clog;
> +       }
> +
> +       writel(BIT(ch->num), ch->ebm->mbox_ctl + MBOX_CTL_IRQ_CLR);
> +
> +       ret = devm_request_threaded_irq(ch->ebm->dev, ch->ebm->irq, mtk_gpueb_mbox_isr,
> +                                       mtk_gpueb_mbox_thread, IRQF_SHARED | IRQF_ONESHOT,
> +                                       ch->full_name, ch);
I don't think this warrants a per-channel irq thread.

mbox_chan_received_data is atomic. I think wecan start simple with
just a devm_request_irq for all channels. mtk_gpueb_mbox_isr can

  read bits from MBOX_CTL_RX_STS
  for each bit set:
    read data from rx
    mbox_chan_received_data
  write bits to MBOX_CTL_IRQ_CLR

Re: [PATCH v2 09/10] drm/panthor: devfreq: add pluggable devfreq providers

[Chia-I Wu]

On Fri, Sep 12, 2025 at 11:38 AM Nicolas Frattaroli
<nicolas.frattaroli@collabora.com> wrote:
<snipped>
> diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.h b/drivers/gpu/drm/panthor/panthor_devfreq.h
> index a891cb5fdc34636444f141e10f5d45828fc35b51..94c9768d5d038c4ba8516929edb565a1f13443fb 100644
> --- a/drivers/gpu/drm/panthor/panthor_devfreq.h
> +++ b/drivers/gpu/drm/panthor/panthor_devfreq.h
> @@ -8,6 +8,7 @@
>
>  struct devfreq;
>  struct thermal_cooling_device;
> +struct platform_device;
>
>  struct panthor_device;
>
> @@ -43,6 +44,19 @@ struct panthor_devfreq {
>         spinlock_t lock;
>  };
>
> +struct panthor_devfreq_provider {
> +       /** @dev: device pointer to the provider device */
> +       struct device *dev;
> +       /**
> +        * @init: the provider's init callback that allocates a
> +        * &struct panthor_devfreq, adds it to panthor, and adds a devfreq
> +        * device to panthor. Will be called during panthor's probe.
> +        */
> +       int (*init)(struct panthor_device *ptdev, struct device *dev);
> +
> +       struct list_head node;
> +};
On mt8196, we have performance-domains to replace several other
properties: clocks, *-supply, power-domains, operating-points-v2.
There are also quirks, such as GPU_SHADER_PRESENT should be masked by
GF_REG_SHADER_PRESENT. It feels like that the scope of
panthor_devfreq_provider is more broader, and at least the naming is
not right.

Another issue is I am not sure if we need to expose panthor_device
internals to the provider. mtk_mfg accesses very few fields of
panthor_device. It seems we can make the two less coupled.

I might change my view as mtk_mfg evolves and requires tigher
integration with panthor. But as is, I might prefer for mtk_mfg to
live under drivers/soc/mediatek and provide a header for panthor to
use in soc-specific path.


> +
>
>  int panthor_devfreq_init(struct panthor_device *ptdev);
>
> @@ -57,4 +71,6 @@ int panthor_devfreq_get_dev_status(struct device *dev,
>
>  unsigned long panthor_devfreq_get_freq(struct panthor_device *ptdev);
>
> +int panthor_devfreq_register_provider(struct panthor_devfreq_provider *prov);
> +
>  #endif /* __PANTHOR_DEVFREQ_H__ */
>
> --
> 2.51.0
>

Re: [PATCH v2 10/10] drm/panthor: add support for MediaTek MFlexGraphics

[AngeloGioacchino Del Regno]

Il 12/09/25 20:37, Nicolas Frattaroli ha scritto:
> MediaTek uses some glue logic to control frequency and power on some of
> their GPUs. This is best exposed as a devfreq driver, as it saves us
> from having to hardcode OPPs into the device tree, and can be extended
> with additional devfreq-y logic like more clever governors that use the
> hardware's GPUEB MCU to set frame time targets and power limits.
> 
> Add this driver to the panthor subdirectory. It needs to live here as it
> needs to call into panthor's devfreq layer, and panthor for its part
> also needs to call into this driver during probe to get a devfreq device
> registered. Solving the cyclical dependency by having mediatek_mfg live
> without knowledge of what a panthor is would require moving the devfreq
> provider stuff into a generic devfreq subsystem solution, which I didn't
> want to do.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>   drivers/gpu/drm/panthor/Kconfig        |   13 +
>   drivers/gpu/drm/panthor/Makefile       |    2 +
>   drivers/gpu/drm/panthor/mediatek_mfg.c | 1053 ++++++++++++++++++++++++++++++++
>   3 files changed, 1068 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panthor/Kconfig b/drivers/gpu/drm/panthor/Kconfig
> index 55b40ad07f3b0779e0c434469ddc874ff74fde27..c4d2599c05df9e0e009b8e99b3d29c220269ca0d 100644
> --- a/drivers/gpu/drm/panthor/Kconfig
> +++ b/drivers/gpu/drm/panthor/Kconfig
> @@ -21,3 +21,16 @@ config DRM_PANTHOR
>   
>   	  Note that the Mali-G68 and Mali-G78, while Valhall architecture, will
>   	  be supported with the panfrost driver as they are not CSF GPUs.
> +
> +config DRM_PANTHOR_MEDIATEK_MFG
> +	tristate "MediaTek MFlexGraphics Extensions for Panthor"
> +	depends on (DRM_PANTHOR && ARCH_MEDIATEK) || COMPILE_TEST
> +	select DEVFREQ_GOV_SIMPLE_ONDEMAND
> +	select PM_DEVFREQ
> +	select MTK_GPUEB_MBOX
> +	help
> +	  Support for power and clock control in Panthor for MediaTek
> +	  MFlexGraphics devices, such as the GPU on the MT8196 or MT6991 SoCs.
> +
> +	  These extensions are required for the GPU to work on these platforms,
> +	  as they control the glue logic that powers on the GPU.
> diff --git a/drivers/gpu/drm/panthor/Makefile b/drivers/gpu/drm/panthor/Makefile
> index 02db21748c125688d2ef20ed254b5ebd7ff642e4..e0ebfdfb20bd78e0003c860c86c040746248fb89 100644
> --- a/drivers/gpu/drm/panthor/Makefile
> +++ b/drivers/gpu/drm/panthor/Makefile
> @@ -12,4 +12,6 @@ panthor-y := \
>   	panthor_mmu.o \
>   	panthor_sched.o
>   
> +obj-$(CONFIG_DRM_PANTHOR_MEDIATEK_MFG) += mediatek_mfg.o
> +
>   obj-$(CONFIG_DRM_PANTHOR) += panthor.o
> diff --git a/drivers/gpu/drm/panthor/mediatek_mfg.c b/drivers/gpu/drm/panthor/mediatek_mfg.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..e06a1d5d8cbf81ac276b521df4b04c81b4c75f0b
> --- /dev/null
> +++ b/drivers/gpu/drm/panthor/mediatek_mfg.c
> @@ -0,0 +1,1053 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for MediaTek MFlexGraphics Devices
> + *
> + * Copyright (C) 2025, Collabora Ltd.
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/completion.h>
> +#include <linux/clk.h>
> +#include <linux/container_of.h>
> +#include <linux/devfreq.h>
> +#include <linux/iopoll.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drm_managed.h>
> +
> +#include "panthor_devfreq.h"
> +#include "panthor_device.h"

..snip..

> +/*
> + * This enum is part of the ABI of the GPUEB firmware. Don't change the
> + * numbering, as you would wreak havoc.
> + */
> +enum mtk_mfg_ipi_cmd {
> +	CMD_INIT_SHARED_MEM		= 0,
> +	CMD_GET_FREQ_BY_IDX		= 1,
> +	CMD_GET_POWER_BY_IDX		= 2,
> +	CMD_GET_OPPIDX_BY_FREQ		= 3,
> +	CMD_GET_LEAKAGE_POWER		= 4,
> +	CMD_SET_LIMIT			= 5,
> +	CMD_POWER_CONTROL		= 6,
> +	CMD_ACTIVE_SLEEP_CONTROL	= 7,
> +	CMD_COMMIT			= 8,
> +	CMD_DUAL_COMMIT			= 9,
> +	CMD_PDCA_CONFIG			= 10,
> +	CMD_UPDATE_DEBUG_OPP_INFO	= 11,
> +	CMD_SWITCH_LIMIT		= 12,
> +	CMD_FIX_TARGET_OPPIDX		= 13,
> +	CMD_FIX_DUAL_TARGET_OPPIDX	= 14,
> +	CMD_FIX_CUSTOM_FREQ_VOLT	= 15,
> +	CMD_FIX_DUAL_CUSTOM_FREQ_VOLT	= 16,
> +	CMD_SET_MFGSYS_CONFIG		= 17,
> +	CMD_MSSV_COMMIT			= 18,
> +	CMD_NUM				= 19,
> +};
> +
> +/*
> + * This struct is part of the ABI of the GPUEB firmware. Changing it, or
> + * reordering fields in it, will break things, so don't do it. Thank you.
> + */
> +struct __packed mtk_mfg_ipi_msg {
> +	__le32 magic;
> +	__le32 cmd;
> +	__le32 target;
> +	/*
> +	 * Downstream relies on the compiler to implicitly add the following
> +	 * padding, as it declares the struct as non-packed.
> +	 */
> +	__le32 padding_lol;

__le32 reserved; <- looks way better.

> +	union {
> +		s32 __bitwise oppidx;
> +		s32 __bitwise return_value;
> +		__le32 freq;
> +		__le32 volt;
> +		__le32 power;
> +		__le32 power_state;
> +		__le32 mode;
> +		__le32 value;
> +		struct {
> +			__le64 base;
> +			__le32 size;
> +		} shared_mem;
> +		struct {
> +			__le32 freq;
> +			__le32 volt;
> +		} custom;
> +		struct {
> +			__le32 limiter;
> +			s32 __bitwise ceiling_info;
> +			s32 __bitwise floor_info;
> +		} set_limit;
> +		struct {
> +			__le32 target;
> +			__le32 val;
> +		} mfg_cfg;
> +		struct {
> +			__le32 target;
> +			__le32 val;
> +		} mssv;
> +		struct {
> +			s32 __bitwise gpu_oppidx;
> +			s32 __bitwise stack_oppidx;
> +		} dual_commit;
> +		struct {
> +			__le32 fgpu;
> +			__le32 vgpu;
> +			__le32 fstack;
> +			__le32 vstack;
> +		} dual_custom;
> +	} u;
> +};
> +
> +struct __packed mtk_mfg_ipi_sleep_msg {
> +	__le32 event;
> +	__le32 state;
> +	__le32 magic;
> +};
> +
> +/**
> + * struct mtk_mfg_opp_entry - OPP table entry from firmware
> + * @freq_khz: The operating point's frequency in kilohertz
> + * @voltage_core: The operating point's core voltage in tens of microvolts
> + * @voltage_sram: The operating point's SRAM voltage in tens of microvolts
> + * @posdiv: exponent of base 2 for PLL frequency divisor used for this OPP
> + * @voltage_margin: Number of tens of microvolts the voltage can be undershot
> + * @power_mw: estimate of power usage at this operating point, in milliwatts
> + *
> + * This struct is part of the ABI with the EB firmware. Do not change it.
> + */
> +struct __packed mtk_mfg_opp_entry {
> +	__le32 freq_khz;
> +	__le32 voltage_core;
> +	__le32 voltage_sram;
> +	__le32 posdiv;
> +	__le32 voltage_margin;
> +	__le32 power_mw;
> +};
> +
> +struct mtk_mfg_mbox {
> +	struct mbox_client cl;
> +	struct completion rx_done;
> +	struct mtk_mfg *mfg;
> +	struct mbox_chan *ch;
> +	void *rx_data;
> +};
> +
> +struct mtk_mfg {
> +	struct platform_device *pdev;
> +	struct panthor_device *ptdev;
> +	struct clk *clk_eb;
> +	struct clk_bulk_data *gpu_clks;
> +	struct regulator_bulk_data *gpu_regs;
> +	void __iomem *rpc;
> +	void __iomem *gpr;
> +	void __iomem *sram;
> +	phys_addr_t sram_phys;
> +	unsigned int sram_size;
> +	unsigned int ghpm_en_reg;
> +	u32 ipi_magic;
> +	unsigned int num_opps;
> +	unsigned int num_unique_gpu_opps;
> +	struct dev_pm_opp_data *gpu_opps;
> +	struct dev_pm_opp_data *stack_opps;
> +	struct mtk_mfg_mbox *gf_mbox;
> +	struct mtk_mfg_mbox *slp_mbox;
> +	int last_opp;
> +	const struct mtk_mfg_variant *variant;
> +};
> +
> +struct mtk_mfg_variant {
> +	const char *const *clk_names;
> +	unsigned int num_clks;
> +	const char *const *regulator_names;
> +	unsigned int num_regulators;
> +	int (*init)(struct mtk_mfg *mfg);
> +};
> +
> +static inline void mtk_mfg_update_reg_bits(void __iomem *addr, u32 mask, u32 val)
> +{
> +	writel((readl(addr) & ~mask) | (val & mask), addr);
> +}
> +
> +static inline unsigned long mtk_mfg_read_gpu_freq(struct mtk_mfg *mfg)
> +{
> +	return readl(mfg->sram + GF_REG_FREQ_CUR_GPU) * 1000UL;
> +}
> +
> +static int mtk_mfg_eb_on(struct mtk_mfg *mfg)
> +{
> +	struct device *dev = &mfg->pdev->dev;
> +	u32 val;
> +	int ret;
> +
> +	/*
> +	 * If MFG is already on from e.g. the bootloader, we should skip doing
> +	 * the power-on sequence, as it wouldn't work without powering it off
> +	 * first.
> +	 */
> +	if ((readl(mfg->rpc + RPC_PWR_CON) & PWR_ACK_M) == PWR_ACK_M)
> +		return 0;
> +
> +	ret = readl_poll_timeout(mfg->rpc + RPC_GHPM_RO0_CON, val,
> +				 !(val & (GHPM_PWR_STATE_M | GHPM_STATE_M)),
> +				 GPUEB_POLL_US, GPUEB_TIMEOUT_US);
> +	if (ret) {
> +		dev_err(dev, "timed out waiting for EB to power on\n");
> +		return ret;
> +	}
> +
> +	mtk_mfg_update_reg_bits(mfg->rpc + mfg->ghpm_en_reg, GHPM_ENABLE_M,
> +				GHPM_ENABLE_M);
> +
> +	mtk_mfg_update_reg_bits(mfg->rpc + RPC_GHPM_CFG0_CON, GHPM_ON_SEQ_M, 0);
> +	mtk_mfg_update_reg_bits(mfg->rpc + RPC_GHPM_CFG0_CON, GHPM_ON_SEQ_M,
> +				GHPM_ON_SEQ_M);
> +
> +	mtk_mfg_update_reg_bits(mfg->rpc + mfg->ghpm_en_reg, GHPM_ENABLE_M, 0);
> +
> +
> +	ret = readl_poll_timeout(mfg->rpc + RPC_PWR_CON, val,
> +				 (val & PWR_ACK_M) == PWR_ACK_M,
> +				 GPUEB_POLL_US, GPUEB_TIMEOUT_US);

I wonder if you can check how much time does the GPUEB really take to poweron,
just so that we might be able to reduce delay_us here.

> +	if (ret) {
> +		dev_err(dev, "timed out waiting for EB power ack, val = 0x%X\n",
> +			val);
> +		return ret;
> +	}
> +
> +	ret = readl_poll_timeout(mfg->gpr + GPR_LP_STATE, val,
> +				 (val == EB_ON_RESUME),
> +				 GPUEB_POLL_US, GPUEB_TIMEOUT_US);

Same here - and I think this one is more critical, as I can see this suspend/resume
control being used more extensively in the future.

Specifically, I'm wondering if we could add runtime PM ops that will request EB
suspend/resume - and also if doing so would make any sense.

I am guessing that the "suspend" LP_STATE stops the internal state machine, making
the EB MCU to either go in a low-power state or to anyway lower its power usage by
at least suspending the iterations.

Of course - here I mean that we could have
1. System suspend ops that powers off the EB completely like you're doing here and
2. Runtime PM op that may be called (very) aggressively

...this would obviously not be feasible if the EB suspend/resume (without complete
poweron/off) takes too much time to actually happen.

> +	if (ret) {
> +		dev_err(dev, "timed out waiting for EB to resume, status = 0x%X\n", val);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_mfg_eb_off(struct mtk_mfg *mfg)
> +{

struct device *dev = &mfg->pdev->dev;

> +	struct mtk_mfg_ipi_sleep_msg msg = {
> +		.event = 0,
> +		.state = 0,
> +		.magic = GPUEB_SLEEP_MAGIC
> +	};
> +	u32 val;
> +	int ret;
> +
> +	ret = mbox_send_message(mfg->slp_mbox->ch, &msg);
> +	if (ret < 0) {
> +		dev_err(&mfg->pdev->dev, "failure in mbox comms: %pe\n",
> +			ERR_PTR(ret));

Not sure why you're not simply doing

dev_err(dev, "Cannot send sleep command: %d\n", ret);
return ret;

> +		return ret;
> +	}
> +
> +	ret = readl_poll_timeout(mfg->rpc + RPC_PWR_CON, val,
> +				 !(val & PWR_ACK_M), GPUEB_POLL_US,
> +				 GPUEB_TIMEOUT_US);
> +
> +	if (ret) {
> +		dev_err(&mfg->pdev->dev,
> +			"timed out waiting for EB to power off, val=0x%08X\n",
> +			val);
> +	}
> +
> +	return ret;
> +}
> +
> +static int mtk_mfg_send_ipi(struct mtk_mfg *mfg, struct mtk_mfg_ipi_msg *msg)
> +{
> +	int ret;
> +
> +	msg->magic = mfg->ipi_magic;
> +
> +	ret = mbox_send_message(mfg->gf_mbox->ch, msg);
> +	if (ret < 0) {
> +		dev_err(&mfg->pdev->dev, "error from mailbox subsystem: %pe\n",
> +			ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	wait_for_completion(&mfg->gf_mbox->rx_done);
> +
> +	memcpy(msg, mfg->gf_mbox->rx_data, sizeof(*msg));
> +
> +	if (msg->u.return_value < 0) {
> +		dev_err(&mfg->pdev->dev, "IPI return: %d\n",
> +			msg->u.return_value);
> +		return -EPROTO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_mfg_init_shared_mem(struct mtk_mfg *mfg)
> +{
> +	struct device *dev = &mfg->pdev->dev;
> +	struct mtk_mfg_ipi_msg msg = {};
> +	int ret;
> +
> +	dev_info(dev, "clearing GPUEB sram, 0x%X bytes\n", mfg->sram_size);

Is this message really giving all that great information?
I'd go for dev_dbg() for both of the info msgs in this function.

> +	memset_io(mfg->sram, 0, mfg->sram_size);
> +
> +	msg.cmd = CMD_INIT_SHARED_MEM;
> +	msg.u.shared_mem.base = mfg->sram_phys;
> +	msg.u.shared_mem.size = mfg->sram_size;
> +
> +	ret = mtk_mfg_send_ipi(mfg, &msg);
> +	if (ret)
> +		return ret;
> +
> +	if (readl(mfg->sram) != 0xBABADADA) {

#define SOMETHING 0xBABADADA (call it however you want)

if (readl(mfg->sram) != SOMETHING) .....

> +		dev_err(dev, "EB did not initialise SRAM correctly\n");
> +		return -EIO;
> +	}
> +
> +	dev_info(dev, "initialised mem at phys 0x%016llX\n", mfg->sram_phys);

I don't like exposing addresses in kmsg. Please just don't.

> +
> +	return 0;
> +}
> +
> +static int mtk_mfg_power_control(struct mtk_mfg *mfg, bool enabled)
> +{
> +	struct mtk_mfg_ipi_msg msg = {};
> +
> +	msg.cmd = CMD_POWER_CONTROL;
> +	msg.u.power_state = enabled ? 1 : 0;
> +
> +	return mtk_mfg_send_ipi(mfg, &msg);
> +}
> +
> +static int mtk_mfg_set_oppidx(struct mtk_mfg *mfg, int opp_idx)
> +{
> +	struct mtk_mfg_ipi_msg msg = {};
> +	int ret;
> +
> +	if (!mfg->gpu_opps || !mfg->stack_opps)
> +		return 0;
> +
> +	if (opp_idx >= mfg->num_opps)
> +		return -EINVAL;
> +
> +	if (mfg->last_opp == opp_idx)
> +		return 0;
> +
> +	msg.cmd = CMD_FIX_TARGET_OPPIDX;
> +	msg.u.oppidx = opp_idx;
> +
> +	ret = mtk_mfg_send_ipi(mfg, &msg);
> +	if (ret)
> +		return ret;
> +
> +	mfg->last_opp = opp_idx;
> +
> +	return 0;
> +}
> +
> +static int mtk_mfg_read_opp_tables(struct mtk_mfg *mfg)
> +{
> +	struct device *dev = &mfg->pdev->dev;
> +	struct mtk_mfg_opp_entry e = {};
> +	unsigned int i;
> +	unsigned long long last_freq;
> +
> +	mfg->num_opps = readl(mfg->sram + GF_REG_GPU_OPP_NUM);
> +	if (mfg->num_opps != readl(mfg->sram + GF_REG_STK_OPP_NUM)) {
> +		dev_err(dev, "OPP count of GPU and Stack differ, %u vs. %u\n",
> +			mfg->num_opps, readl(mfg->sram + GF_REG_STK_OPP_NUM));
> +		return -EINVAL;
> +	}
> +
> +	if (mfg->num_opps > MAX_OPP_NUM || mfg->num_opps == 0) {
> +		dev_err(dev, "OPP count (%u) out of range %u >= count > 0\n",
> +			mfg->num_opps, MAX_OPP_NUM);
> +		return -EINVAL;
> +	}
> +
> +	mfg->gpu_opps = devm_kcalloc(dev, mfg->num_opps,
> +				     sizeof(struct dev_pm_opp_data), GFP_KERNEL);
> +	if (!mfg->gpu_opps)
> +		return -ENOMEM;
> +
> +	mfg->stack_opps = devm_kcalloc(dev, mfg->num_opps,
> +				       sizeof(struct dev_pm_opp_data), GFP_KERNEL);
> +	if (!mfg->stack_opps)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < mfg->num_opps; i++) {
> +		memcpy_fromio(&e, mfg->sram + GF_REG_OPP_TABLE_GPU + i * sizeof(e),
> +			      sizeof(e));
> +		if (mem_is_zero(&e, sizeof(e))) {
> +			dev_err(dev, "ran into an empty GPU OPP at index %u\n",
> +				i);
> +			return -EINVAL;
> +		}
> +		mfg->gpu_opps[i].freq = e.freq_khz * 1000ULL;
> +		mfg->gpu_opps[i].u_volt = e.voltage_core * 10;
> +
> +		if (!last_freq || mfg->gpu_opps[i].freq != last_freq)
> +			mfg->num_unique_gpu_opps++;
> +
> +		last_freq = mfg->gpu_opps[i].freq;
> +	}
> +
> +	/* Why do I even bother? */

Cleanup this comment, or make it so that people can actually understand what you
mean without reading the whole file.

> +	for (i = 0; i < mfg->num_opps; i++) {
> +		memcpy_fromio(&e, mfg->sram + GF_REG_OPP_TABLE_STK + i * sizeof(e),
> +			      sizeof(e));
> +		if (mem_is_zero(&e, sizeof(e))) {
> +			dev_err(dev, "ran into an empty GPU OPP at index %u\n",
> +				i);
> +			return -EINVAL;
> +		}
> +		mfg->stack_opps[i].freq = e.freq_khz * 1000ULL;
> +		mfg->stack_opps[i].u_volt = e.voltage_core * 10;
> +	}
> +
> +	return 0;
> +}
> +
> +static inline bool mtk_mfg_opp_idx_match(struct mtk_mfg *mfg, int idx,
> +					 unsigned long rate)
> +{
> +	if ((idx == mfg->num_opps - 1) && mfg->gpu_opps[idx].freq >= rate)
> +		return true;
> +
> +	if (mfg->gpu_opps[idx].freq >= rate && mfg->gpu_opps[idx + 1].freq < rate)
> +		return true;
> +
> +	return false;
> +}
> +
> +/**
> + * mtk_mfg_get_closest_opp_idx - find OPP index for desired GPU frequency
> + * @mfg: pointer to a &struct mtk_mfg driver instance
> + * @gpu_rate: desired rate of the GPU core in Hz
> + *
> + * Given a desired target frequency for the GPU core, find the index of a
> + * matching OPP, or the next higher OPP if no exact match is found, or the
> + * maximum OPP for frequencies exceeding the maximum OPP's frequency.
> + *
> + * For duplicate OPP entries, chooses the highest OPP index, as in, the one
> + * closest to the next lower frequency OPP.
> + *
> + * Returns -EINVAL on error, or the OPP index on success.
> + */
> +static int mtk_mfg_get_closest_opp_idx(struct mtk_mfg *mfg, unsigned long gpu_rate)
> +{

int r = mfg->num_opps - 1;
int l = 0;
int m;

> +	int l = 0;
> +	int r = mfg->num_opps - 1;
> +	int m;
> +
> +	if (!mfg->gpu_opps)
> +		return -EINVAL;
> +
> +	if (mfg->gpu_opps[0].freq <= gpu_rate)
> +		return 0;
> +
> +	while (l <= r) {
> +		m = l + (r - l) / 2;
> +		if (mtk_mfg_opp_idx_match(mfg, m, gpu_rate)) {
> +			return m;

If the above is returning, you don't need an else, do you.

if (mtk_mfg_opp_idx_match(..))
	return m;

if (mfg->gpu_opps[m].freq >= gpu_rate)
	l = ++m;
else
	r = --m;

(or m + 1 and m - 1 are also fine, I don't really care how that's written)


> +		} else if (mfg->gpu_opps[m].freq >= gpu_rate) /* >= for dupes */
> +			l = m + 1;
> +		else
> +			r = m - 1;
> +	}
> +
> +	return -EINVAL;
> +}
> +

Cheers,
Angelo

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

[AngeloGioacchino Del Regno]

Il 12/09/25 20:37, 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.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>   .../bindings/devfreq/mediatek,mt8196-gpufreq.yaml  | 113 +++++++++++++++++++++
>   1 file changed, 113 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..9d9efd4e70f1ef7ae446c833c15144beb9641b16
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/mediatek,mt8196-gpufreq.yaml
> @@ -0,0 +1,113 @@
> +# 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>
> +
> +description: |
> +  A special-purpose embedded MCU to control power and frequency of GPU devices
> +  using MediaTek Flexible Graphics integration hardware.
> +
> +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: hw_revision

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

What about using a bit more generic clock names?

main, gpu-core, gpu-stack0, gpu-stack1

...or something along that line :-)

> +      - 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: FastDVFS control
> +
> +  mbox-names:
> +    items:
> +      - const: fast-dvfs-event
> +      - const: gpufreq
> +      - const: sleep
> +      - const: timer
> +      - const: fhctl
> +      - const: ccf
> +      - const: fast-dvfs
> +
> +  shmem:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle to the shared memory region of the GPUEB MCU
> +
> +  "#performance-domain-cells":
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - mboxes
> +  - mbox-names
> +  - shmem
> +  - "#performance-domain-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/mediatek,mt8196-clock.h>
> +
> +    gpufreq: performance-controller@4b09fd00 {

You're not using any phandle to gpufreq in this example, are you?

Drop that `gpufreq: ` :-)

> +        compatible = "mediatek,mt8196-gpufreq";
> +        reg = <0x4b09fd00 0x80>,
> +              <0x4b800000 0x1000>,
> +              <0x4b860128 0x4>;
> +        reg-names = "gpr", "rpc", "hw_revision";
> +        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 7>;
> +        mbox-names = "fast-dvfs-event", "gpufreq", "sleep", "timer", "fhctl",
> +                     "ccf", "fast-dvfs";
> +        shmem = <&gpufreq_shmem>;
> +        #performance-domain-cells = <0>;
> +    };
> 

Re: [PATCH v2 06/10] drm/panthor: call into devfreq for current frequency

[AngeloGioacchino Del Regno]

Il 12/09/25 20:37, Nicolas Frattaroli ha scritto:
> As it stands, panthor keeps a cached current frequency value for when it
> wants to retrieve it. This doesn't work well for when things might
> switch frequency without panthor's knowledge.
> 
> Instead, implement the get_cur_freq operation, and expose it through a
> helper function to the rest of panthor.
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v2 07/10] drm/panthor: move panthor_devfreq struct to header

[AngeloGioacchino Del Regno]

Il 12/09/25 20:37, Nicolas Frattaroli ha scritto:
> In order to make files other than panthor_devfreq.c be able to touch the
> members of a panthor_devfreq instance, it needs to live somewhere other
> than the .c file.
> 
> Move it into the panthor_devfreq.h header, so that the upcoming MediaTek
> MFG devfreq can use it as well.
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v2 05/10] mailbox: add MediaTek GPUEB IPI mailbox

[Nicolas Frattaroli]

On Saturday, 13 September 2025 00:11:10 Central European Summer Time Chia-I Wu wrote:
> On Fri, Sep 12, 2025 at 11:38 AM Nicolas Frattaroli
> <nicolas.frattaroli@collabora.com> wrote:
> <snipped>
> > +static irqreturn_t mtk_gpueb_mbox_thread(int irq, void *data)
> > +{
> > +       struct mtk_gpueb_mbox_chan *ch = data;
> > +       int status;
> > +
> > +       status = atomic_cmpxchg(&ch->rx_status,
> > +                               MBOX_FULL | MBOX_CLOGGED, MBOX_FULL);
> > +       if (status == (MBOX_FULL | MBOX_CLOGGED)) {
> > +               mtk_gpueb_mbox_read_rx(ch);
> > +               writel(BIT(ch->num), ch->ebm->mbox_ctl + MBOX_CTL_IRQ_CLR);
> > +               mbox_chan_received_data(&ch->ebm->mbox.chans[ch->num],
> > +                                       ch->rx_buf);
> Given what other drivers do, and how mtk_mfg consumes the data, we should
> 
>   char buf[MAX_OF_RX_LEN]; //  MAX_OF_RX_LEN is 32; we can also
> allocate it during probe
>   mtk_gpueb_mbox_read_rx(ch);
>   mbox_chan_received_data(..., buf);
> 
> mtx_mfg makes a copy eventually anyway.

We don't right now, at least not until after the callback returns.
So we need to have the copy in the mtk_mfg callback, not after the
completion. That's fine and I do want to do this as this is what
the mailbox framework seems to expect clients to do.

> We don't need to maintain any
> extra copy.
> 
> Then we might not need rx_status.

We can probably get rid of it if we keep the per-channel
interrupt handler. Otherwise, we may still need clogged,
as we don't want to process interrupts on channels we have
no user for.

> 
> > +               atomic_set(&ch->rx_status, 0);
> > +               return IRQ_HANDLED;
> > +       }
> > +
> > +       return IRQ_NONE;
> > +}
> > +
> > +static int mtk_gpueb_mbox_send_data(struct mbox_chan *chan, void *data)
> > +{
> > +       struct mtk_gpueb_mbox_chan *ch = chan->con_priv;
> > +       int i;
> > +       u32 *values = data;
> > +
> > +       if (atomic_read(&ch->rx_status))
> > +               return -EBUSY;
> > +
> > +       /*
> > +        * We don't want any fancy nonsense, just write the 32-bit values in
> > +        * order. memcpy_toio/__iowrite32_copy don't work here, because fancy.
> > +        */
> > +       for (i = 0; i < ch->c->tx_len; i += 4)
> > +               writel(values[i / 4], ch->ebm->mbox_mmio + ch->c->tx_offset + i);
> > +
> > +       writel(BIT(ch->num), ch->ebm->mbox_ctl + MBOX_CTL_IRQ_SET);
> > +
> > +       return 0;
> > +}
> > +
> > +static int mtk_gpueb_mbox_startup(struct mbox_chan *chan)
> > +{
> > +       struct mtk_gpueb_mbox_chan *ch = chan->con_priv;
> > +       int ret;
> > +
> > +       atomic_set(&ch->rx_status, 0);
> > +
> > +       ret = clk_enable(ch->ebm->clk);
> > +       if (ret) {
> > +               dev_err(ch->ebm->dev, "Failed to enable EB clock: %pe\n",
> > +                       ERR_PTR(ret));
> > +               goto err_clog;
> > +       }
> > +
> > +       writel(BIT(ch->num), ch->ebm->mbox_ctl + MBOX_CTL_IRQ_CLR);
> > +
> > +       ret = devm_request_threaded_irq(ch->ebm->dev, ch->ebm->irq, mtk_gpueb_mbox_isr,
> > +                                       mtk_gpueb_mbox_thread, IRQF_SHARED | IRQF_ONESHOT,
> > +                                       ch->full_name, ch);
> I don't think this warrants a per-channel irq thread.
> 
> mbox_chan_received_data is atomic. I think wecan start simple with
> just a devm_request_irq for all channels. mtk_gpueb_mbox_isr can
> 
>   read bits from MBOX_CTL_RX_STS
>   for each bit set:
>     read data from rx
>     mbox_chan_received_data
>   write bits to MBOX_CTL_IRQ_CLR
> 

I don't like this approach. It brings us back to having to process
multiple channels per ISR, keep track of when the interrupt should
be enabled and disabled based on how many channels are in use, and
also is not in line with what e.g. omap-mailbox.c does.

Remember that `mbox_chan_received_data` synchronously calls the
mailbox client's rx_callback. In mediatek_mfg's case, this is
fairly small, though with the request to not make the rx buffer
persist beyond the rx_callback it will gain an additional memory
copy. But we can't guarantee that someone isn't going to put a
slow operation in the path. Sure, it's going to be atomic, but
waiting for a spinlock is atomic and not something an ISR would
enjoy. I don't think mailbox clients would expect that if they
take their time they'll stall the interrupt handler for every
other channel.

So we'd keep the interrupt disabled for all channels until the
client that received a message has processed it.

I can see myself getting rid of the handler and just having the
thread function as the bottom half, but I'd really like to keep
the one-IRQ-request-per-channel thing I've got going now as it
made the code a lot easier to reason about. However, doing this
would mean the interrupt is re-enabled after the generic upper
half, when all the business logic that needs to not run
concurrently for an individual channel is in the bottom half.

As far as I can tell, this would then mean we'd have to add
some concurrency exclusion mechanism to the bottom half.

Moving all the logic into the upper half handler function
would make that handler somewhat longer, and I don't know
if IRQF_ONESHOT masks the interrupt for all users of that
IRQ number or just for those with that dev_id. If it's per
dev_id, then I'm fine with moving stuff up there. But from
my reading of the core IRQ handling code, that does not
appear to be the case; one channel getting a reply would
mask *all* channels of the mailbox until the upper half is
completed, and if the upper half calls into a driver
callback synchronously, that may take a hot minute.

Put differently: Is there a problem with one thread per used
channel, or are we going off vibes here? The way it currently
works uses the shared interrupt to mark just that one channel
as busy with rx_status before letting the IRQ for all channels
be unmasked again, which seems ideal to me.

Re: [PATCH v2 09/10] drm/panthor: devfreq: add pluggable devfreq providers

[Nicolas Frattaroli]

On Saturday, 13 September 2025 00:53:50 Central European Summer Time Chia-I Wu wrote:
> On Fri, Sep 12, 2025 at 11:38 AM Nicolas Frattaroli
> <nicolas.frattaroli@collabora.com> wrote:
> <snipped>
> > diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.h b/drivers/gpu/drm/panthor/panthor_devfreq.h
> > index a891cb5fdc34636444f141e10f5d45828fc35b51..94c9768d5d038c4ba8516929edb565a1f13443fb 100644
> > --- a/drivers/gpu/drm/panthor/panthor_devfreq.h
> > +++ b/drivers/gpu/drm/panthor/panthor_devfreq.h
> > @@ -8,6 +8,7 @@
> >
> >  struct devfreq;
> >  struct thermal_cooling_device;
> > +struct platform_device;
> >
> >  struct panthor_device;
> >
> > @@ -43,6 +44,19 @@ struct panthor_devfreq {
> >         spinlock_t lock;
> >  };
> >
> > +struct panthor_devfreq_provider {
> > +       /** @dev: device pointer to the provider device */
> > +       struct device *dev;
> > +       /**
> > +        * @init: the provider's init callback that allocates a
> > +        * &struct panthor_devfreq, adds it to panthor, and adds a devfreq
> > +        * device to panthor. Will be called during panthor's probe.
> > +        */
> > +       int (*init)(struct panthor_device *ptdev, struct device *dev);
> > +
> > +       struct list_head node;
> > +};
> On mt8196, we have performance-domains to replace several other
> properties: clocks, *-supply, power-domains, operating-points-v2.
> There are also quirks, such as GPU_SHADER_PRESENT should be masked by
> GF_REG_SHADER_PRESENT. It feels like that the scope of
> panthor_devfreq_provider is more broader, and at least the naming is
> not right.

True, though I'm still not entirely sure whether mtk_mfg needs to do
the GF_REG_SHADER_PRESENT thing. It's entirely possible this is just
an efuse value the GPUEB reads and then puts in SRAM for us, and we
could simply read this efuse cell ourselves. Among a list of questions
about the hardware we're sending to MediaTek, whether this is an efuse
cell and where it is placed is one of them.

If it turns out to be the case that we can simply read an efuse in
panthor in the other mt8196 integration code, then we can keep
mtk_mfg basically entirely focused on the devfreq-y part. I'd really
prefer this solution.

However, assuming we can't go down this path either because this is
not how the hardware works, or because MediaTek never replies, or
because someone doesn't like reading efuses in panthor, I think
generalising "devfreq_provider" to "performance_controller" or
something like that would be a good move.

In a way, the fused off core mask is part of the vague domain of
"performance", and it'll also allow us to extend it with other
things relevant to performance control in different vendor integration
logic designs. I'm thinking memory bandwidth control and job scheduling
preferences. E.g. if the interconnect tells us one core is spending a
lot of time waiting on the interconnect, maybe because a different
piece of the SoC that's active shares the same path on the
interconnect, we could then communicate a scheduling preference for
the other cores that have bandwidth headroom even if they are busier
in compute. Maybe this doesn't make sense though because interconnect
designs are fully switched these days or panthor's scheduler will
already figure this out from job completion times.

If any other SoC vendor or people working on hardware of those vendors
want to chime in and say whether they've got any other uses for
communicating more than just devfreq from glue logic to panthor, then
this would be a great time to do it, so that we can get this interface
right from the beginning.

> Another issue is I am not sure if we need to expose panthor_device
> internals to the provider. mtk_mfg accesses very few fields of
> panthor_device. It seems we can make the two less coupled.
> 
> I might change my view as mtk_mfg evolves and requires tigher
> integration with panthor. But as is, I might prefer for mtk_mfg to
> live under drivers/soc/mediatek and provide a header for panthor to
> use in soc-specific path.

I'm not very confident it's possible to cleanly decouple them without
inventing a bunch of very panthor-and-mfg specific interfaces that
masquerade as general solutions in the process. It'd also mean I'd
have to duplicate all of `panthor_devfreq_get_dev_status` instead of
just being able to reuse it, unless that is also exposed in said
header file, which would need a better justification than "well there
is one other user of it and we're trying to couple it more loosely".

I know that it's almost independent, but unfortunately, even a tiny
dependency between the two will mean that mediatek_mfg will need to
know about panthor.

Other things needed from panthor are the pdevfreq->gov_data, and
the panthor struct device* itself, as well as stuff like "fast_rate"
in the panthor_device struct.

In the future, we may want to expand this driver with governors
beyond SIMPLE_ONDEMAND, based on the job completion duration targets
we can communicate to the GPUEB. That may either make the driver
more tightly coupled or more loosely coupled, I don't really know
yet.

One advantage of looking to completely decouple them (though again,
I doubt that's possible at the moment without questionable refactors)
could be that we could also support panfrost devices that need this.

> 
> 
> > +
> >
> >  int panthor_devfreq_init(struct panthor_device *ptdev);
> >
> > @@ -57,4 +71,6 @@ int panthor_devfreq_get_dev_status(struct device *dev,
> >
> >  unsigned long panthor_devfreq_get_freq(struct panthor_device *ptdev);
> >
> > +int panthor_devfreq_register_provider(struct panthor_devfreq_provider *prov);
> > +
> >  #endif /* __PANTHOR_DEVFREQ_H__ */
> >
> > --
> > 2.51.0
> >
> 

Re: [PATCH v2 10/10] drm/panthor: add support for MediaTek MFlexGraphics

[Nicolas Frattaroli]

On Monday, 15 September 2025 12:28:09 Central European Summer Time AngeloGioacchino Del Regno wrote:
> Il 12/09/25 20:37, Nicolas Frattaroli ha scritto:
> > MediaTek uses some glue logic to control frequency and power on some of
> > their GPUs. This is best exposed as a devfreq driver, as it saves us
> > from having to hardcode OPPs into the device tree, and can be extended
> > with additional devfreq-y logic like more clever governors that use the
> > hardware's GPUEB MCU to set frame time targets and power limits.
> > 
> > Add this driver to the panthor subdirectory. It needs to live here as it
> > needs to call into panthor's devfreq layer, and panthor for its part
> > also needs to call into this driver during probe to get a devfreq device
> > registered. Solving the cyclical dependency by having mediatek_mfg live
> > without knowledge of what a panthor is would require moving the devfreq
> > provider stuff into a generic devfreq subsystem solution, which I didn't
> > want to do.
> > 
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >   drivers/gpu/drm/panthor/Kconfig        |   13 +
> >   drivers/gpu/drm/panthor/Makefile       |    2 +
> >   drivers/gpu/drm/panthor/mediatek_mfg.c | 1053 ++++++++++++++++++++++++++++++++
> >   3 files changed, 1068 insertions(+)
> > 
> [ ... snip ...]
> > +static int mtk_mfg_eb_on(struct mtk_mfg *mfg)
> > +{
> > +	struct device *dev = &mfg->pdev->dev;
> > +	u32 val;
> > +	int ret;
> > +
> > +	/*
> > +	 * If MFG is already on from e.g. the bootloader, we should skip doing
> > +	 * the power-on sequence, as it wouldn't work without powering it off
> > +	 * first.
> > +	 */
> > +	if ((readl(mfg->rpc + RPC_PWR_CON) & PWR_ACK_M) == PWR_ACK_M)
> > +		return 0;
> > +
> > +	ret = readl_poll_timeout(mfg->rpc + RPC_GHPM_RO0_CON, val,
> > +				 !(val & (GHPM_PWR_STATE_M | GHPM_STATE_M)),
> > +				 GPUEB_POLL_US, GPUEB_TIMEOUT_US);
> > +	if (ret) {
> > +		dev_err(dev, "timed out waiting for EB to power on\n");
> > +		return ret;
> > +	}
> > +
> > +	mtk_mfg_update_reg_bits(mfg->rpc + mfg->ghpm_en_reg, GHPM_ENABLE_M,
> > +				GHPM_ENABLE_M);
> > +
> > +	mtk_mfg_update_reg_bits(mfg->rpc + RPC_GHPM_CFG0_CON, GHPM_ON_SEQ_M, 0);
> > +	mtk_mfg_update_reg_bits(mfg->rpc + RPC_GHPM_CFG0_CON, GHPM_ON_SEQ_M,
> > +				GHPM_ON_SEQ_M);
> > +
> > +	mtk_mfg_update_reg_bits(mfg->rpc + mfg->ghpm_en_reg, GHPM_ENABLE_M, 0);
> > +
> > +
> > +	ret = readl_poll_timeout(mfg->rpc + RPC_PWR_CON, val,
> > +				 (val & PWR_ACK_M) == PWR_ACK_M,
> > +				 GPUEB_POLL_US, GPUEB_TIMEOUT_US);
> 
> I wonder if you can check how much time does the GPUEB really take to poweron,
> just so that we might be able to reduce delay_us here.

I already did, that's where the 50us value is from as far as I remember.

> 
> > +	if (ret) {
> > +		dev_err(dev, "timed out waiting for EB power ack, val = 0x%X\n",
> > +			val);
> > +		return ret;
> > +	}
> > +
> > +	ret = readl_poll_timeout(mfg->gpr + GPR_LP_STATE, val,
> > +				 (val == EB_ON_RESUME),
> > +				 GPUEB_POLL_US, GPUEB_TIMEOUT_US);
> 
> Same here - and I think this one is more critical, as I can see this suspend/resume
> control being used more extensively in the future.
> 
> Specifically, I'm wondering if we could add runtime PM ops that will request EB
> suspend/resume - and also if doing so would make any sense.
> 
> I am guessing that the "suspend" LP_STATE stops the internal state machine, making
> the EB MCU to either go in a low-power state or to anyway lower its power usage by
> at least suspending the iterations.

I think I briefly fiddled with this but then it did nothing other than
break everything. Is the current time it takes to resume a problem?

> 
> Of course - here I mean that we could have
> 1. System suspend ops that powers off the EB completely like you're doing here and
> 2. Runtime PM op that may be called (very) aggressively
> 
> ...this would obviously not be feasible if the EB suspend/resume (without complete
> poweron/off) takes too much time to actually happen.

We probably don't want to aggressively suspend the thing doing DVFS
while a workload is running, and if no workload is running, it
already suspends. I can't really say how normal desktop usage will
play out yet, but generally speaking I think it's a bit early to
find a comfortable place on the transition latency vs power draw
curve at this point.

> [... snip ...]
> > +static int mtk_mfg_init_shared_mem(struct mtk_mfg *mfg)
> > +{
> > [... snip ...]
> > +
> > +	dev_info(dev, "initialised mem at phys 0x%016llX\n", mfg->sram_phys);
> 
> I don't like exposing addresses in kmsg. Please just don't.

It's a physical address. This is not a kernel pointer, but something
that can be read from the DTS. But sure, I'll remove it I guess?

> [... snip ...]
> 
> Cheers,
> Angelo
> 

You can assume me not responding to a part of the feedback in this
e-mail means I'll address it in the next revision of the patch
series.

Kind regards,
Nicolas Frattaroli

Re: [PATCH v2 10/10] drm/panthor: add support for MediaTek MFlexGraphics

[AngeloGioacchino Del Regno]

Il 15/09/25 15:32, Nicolas Frattaroli ha scritto:
> On Monday, 15 September 2025 12:28:09 Central European Summer Time AngeloGioacchino Del Regno wrote:
>> Il 12/09/25 20:37, Nicolas Frattaroli ha scritto:
>>> MediaTek uses some glue logic to control frequency and power on some of
>>> their GPUs. This is best exposed as a devfreq driver, as it saves us
>>> from having to hardcode OPPs into the device tree, and can be extended
>>> with additional devfreq-y logic like more clever governors that use the
>>> hardware's GPUEB MCU to set frame time targets and power limits.
>>>
>>> Add this driver to the panthor subdirectory. It needs to live here as it
>>> needs to call into panthor's devfreq layer, and panthor for its part
>>> also needs to call into this driver during probe to get a devfreq device
>>> registered. Solving the cyclical dependency by having mediatek_mfg live
>>> without knowledge of what a panthor is would require moving the devfreq
>>> provider stuff into a generic devfreq subsystem solution, which I didn't
>>> want to do.
>>>
>>> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
>>> ---
>>>    drivers/gpu/drm/panthor/Kconfig        |   13 +
>>>    drivers/gpu/drm/panthor/Makefile       |    2 +
>>>    drivers/gpu/drm/panthor/mediatek_mfg.c | 1053 ++++++++++++++++++++++++++++++++
>>>    3 files changed, 1068 insertions(+)
>>>
>> [ ... snip ...]
>>> +static int mtk_mfg_eb_on(struct mtk_mfg *mfg)
>>> +{
>>> +	struct device *dev = &mfg->pdev->dev;
>>> +	u32 val;
>>> +	int ret;
>>> +
>>> +	/*
>>> +	 * If MFG is already on from e.g. the bootloader, we should skip doing
>>> +	 * the power-on sequence, as it wouldn't work without powering it off
>>> +	 * first.
>>> +	 */
>>> +	if ((readl(mfg->rpc + RPC_PWR_CON) & PWR_ACK_M) == PWR_ACK_M)
>>> +		return 0;
>>> +
>>> +	ret = readl_poll_timeout(mfg->rpc + RPC_GHPM_RO0_CON, val,
>>> +				 !(val & (GHPM_PWR_STATE_M | GHPM_STATE_M)),
>>> +				 GPUEB_POLL_US, GPUEB_TIMEOUT_US);
>>> +	if (ret) {
>>> +		dev_err(dev, "timed out waiting for EB to power on\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	mtk_mfg_update_reg_bits(mfg->rpc + mfg->ghpm_en_reg, GHPM_ENABLE_M,
>>> +				GHPM_ENABLE_M);
>>> +
>>> +	mtk_mfg_update_reg_bits(mfg->rpc + RPC_GHPM_CFG0_CON, GHPM_ON_SEQ_M, 0);
>>> +	mtk_mfg_update_reg_bits(mfg->rpc + RPC_GHPM_CFG0_CON, GHPM_ON_SEQ_M,
>>> +				GHPM_ON_SEQ_M);
>>> +
>>> +	mtk_mfg_update_reg_bits(mfg->rpc + mfg->ghpm_en_reg, GHPM_ENABLE_M, 0);
>>> +
>>> +
>>> +	ret = readl_poll_timeout(mfg->rpc + RPC_PWR_CON, val,
>>> +				 (val & PWR_ACK_M) == PWR_ACK_M,
>>> +				 GPUEB_POLL_US, GPUEB_TIMEOUT_US);
>>
>> I wonder if you can check how much time does the GPUEB really take to poweron,
>> just so that we might be able to reduce delay_us here.
> 
> I already did, that's where the 50us value is from as far as I remember.
> 

Ah, perfect.

>>
>>> +	if (ret) {
>>> +		dev_err(dev, "timed out waiting for EB power ack, val = 0x%X\n",
>>> +			val);
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = readl_poll_timeout(mfg->gpr + GPR_LP_STATE, val,
>>> +				 (val == EB_ON_RESUME),
>>> +				 GPUEB_POLL_US, GPUEB_TIMEOUT_US);
>>
>> Same here - and I think this one is more critical, as I can see this suspend/resume
>> control being used more extensively in the future.
>>
>> Specifically, I'm wondering if we could add runtime PM ops that will request EB
>> suspend/resume - and also if doing so would make any sense.
>>
>> I am guessing that the "suspend" LP_STATE stops the internal state machine, making
>> the EB MCU to either go in a low-power state or to anyway lower its power usage by
>> at least suspending the iterations.
> 
> I think I briefly fiddled with this but then it did nothing other than
> break everything. Is the current time it takes to resume a problem?
> 
>>
>> Of course - here I mean that we could have
>> 1. System suspend ops that powers off the EB completely like you're doing here and
>> 2. Runtime PM op that may be called (very) aggressively
>>
>> ...this would obviously not be feasible if the EB suspend/resume (without complete
>> poweron/off) takes too much time to actually happen.
> 
> We probably don't want to aggressively suspend the thing doing DVFS
> while a workload is running, and if no workload is running, it
> already suspends. I can't really say how normal desktop usage will
> play out yet, but generally speaking I think it's a bit early to
> find a comfortable place on the transition latency vs power draw
> curve at this point.
> 

Okay let's leave it for now and revisit that after everything is upstreamed.
It's only an improvement anyway, not critical for functionality, and maybe
not even feasible.

>> [... snip ...]
>>> +static int mtk_mfg_init_shared_mem(struct mtk_mfg *mfg)
>>> +{
>>> [... snip ...]
>>> +
>>> +	dev_info(dev, "initialised mem at phys 0x%016llX\n", mfg->sram_phys);
>>
>> I don't like exposing addresses in kmsg. Please just don't.
> 
> It's a physical address. This is not a kernel pointer, but something
> that can be read from the DTS. But sure, I'll remove it I guess?
> 

Yeah, please.

>> [... snip ...]
>>
>> Cheers,
>> Angelo
>>
> 
> You can assume me not responding to a part of the feedback in this
> e-mail means I'll address it in the next revision of the patch
> series.
> 

Alright!

Cheers,
Angelo

> Kind regards,
> Nicolas Frattaroli
> 
> 

Re: [PATCH v2 04/10] dt-bindings: mailbox: Add MT8196 GPUEB Mailbox

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2862 bytes --]

On Fri, Sep 12, 2025 at 08:37:03PM +0200, Nicolas Frattaroli wrote:
> The MediaTek MT8196 SoC includes an embedded MCU referred to as "GPUEB",
> acting as glue logic to control power and frequency of the Mali GPU.
> This MCU runs special-purpose firmware for this use, and the main
> application processor communicates with it through a mailbox.
> 
> Add a binding that describes this mailbox.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
>  .../mailbox/mediatek,mt8196-gpueb-mbox.yaml        | 64 ++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,mt8196-gpueb-mbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,mt8196-gpueb-mbox.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..56508f406fce88c7c1699aa67b57394fc7b1c357
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/mediatek,mt8196-gpueb-mbox.yaml
> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/mediatek,mt8196-gpueb-mbox.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek MFlexGraphics GPUEB Mailbox Controller
> +
> +maintainers:
> +  - Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt8196-gpueb-mbox
> +
> +  reg:
> +    items:
> +      - description: mailbox data registers
> +      - description: mailbox control registers
> +
> +  reg-names:
> +    items:
> +      - const: data
> +      - const: ctl
> +
> +  clocks:
> +    items:
> +      - description: main clock of the GPUEB MCU
> +
> +  interrupts:
> +    items:
> +      - description: fires when a new message is received
> +
> +  "#mbox-cells":
> +    const: 1
> +    description:
> +      The number of the mailbox channel.
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - interrupts
> +  - "#mbox-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/mediatek,mt8196-clock.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    gpueb_mbox: mailbox@4b09fd80 {

drop the label, but otherwise this binding looks okay to me.
Acked-by: Conor Dooley <conor.dooley@microchip.com>

> +        compatible = "mediatek,mt8196-gpueb-mbox";
> +        reg = <0x4b09fd80 0x280>,
> +              <0x4b170000 0x7c>;
> +        reg-names = "data", "ctl";
> +        clocks = <&topckgen CLK_TOP_MFG_EB>;
> +        interrupts = <GIC_SPI 608 IRQ_TYPE_LEVEL_HIGH 0>;
> +        #mbox-cells = <1>;
> +    };
> 
> -- 
> 2.51.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 05/10] mailbox: add MediaTek GPUEB IPI mailbox

[Chia-I Wu]

On Mon, Sep 15, 2025 at 6:34 AM Nicolas Frattaroli
<nicolas.frattaroli@collabora.com> wrote:
>
> On Saturday, 13 September 2025 00:11:10 Central European Summer Time Chia-I Wu wrote:
> > On Fri, Sep 12, 2025 at 11:38 AM Nicolas Frattaroli
> > <nicolas.frattaroli@collabora.com> wrote:
> > <snipped>
> > > +static irqreturn_t mtk_gpueb_mbox_thread(int irq, void *data)
> > > +{
> > > +       struct mtk_gpueb_mbox_chan *ch = data;
> > > +       int status;
> > > +
> > > +       status = atomic_cmpxchg(&ch->rx_status,
> > > +                               MBOX_FULL | MBOX_CLOGGED, MBOX_FULL);
> > > +       if (status == (MBOX_FULL | MBOX_CLOGGED)) {
> > > +               mtk_gpueb_mbox_read_rx(ch);
> > > +               writel(BIT(ch->num), ch->ebm->mbox_ctl + MBOX_CTL_IRQ_CLR);
> > > +               mbox_chan_received_data(&ch->ebm->mbox.chans[ch->num],
> > > +                                       ch->rx_buf);
> > Given what other drivers do, and how mtk_mfg consumes the data, we should
> >
> >   char buf[MAX_OF_RX_LEN]; //  MAX_OF_RX_LEN is 32; we can also
> > allocate it during probe
> >   mtk_gpueb_mbox_read_rx(ch);
> >   mbox_chan_received_data(..., buf);
> >
> > mtx_mfg makes a copy eventually anyway.
>
> We don't right now, at least not until after the callback returns.
> So we need to have the copy in the mtk_mfg callback, not after the
> completion. That's fine and I do want to do this as this is what
> the mailbox framework seems to expect clients to do.
>
> > We don't need to maintain any
> > extra copy.
> >
> > Then we might not need rx_status.
>
> We can probably get rid of it if we keep the per-channel
> interrupt handler. Otherwise, we may still need clogged,
> as we don't want to process interrupts on channels we have
> no user for.
>
> >
> > > +               atomic_set(&ch->rx_status, 0);
> > > +               return IRQ_HANDLED;
> > > +       }
> > > +
> > > +       return IRQ_NONE;
> > > +}
> > > +
> > > +static int mtk_gpueb_mbox_send_data(struct mbox_chan *chan, void *data)
> > > +{
> > > +       struct mtk_gpueb_mbox_chan *ch = chan->con_priv;
> > > +       int i;
> > > +       u32 *values = data;
> > > +
> > > +       if (atomic_read(&ch->rx_status))
> > > +               return -EBUSY;
> > > +
> > > +       /*
> > > +        * We don't want any fancy nonsense, just write the 32-bit values in
> > > +        * order. memcpy_toio/__iowrite32_copy don't work here, because fancy.
> > > +        */
> > > +       for (i = 0; i < ch->c->tx_len; i += 4)
> > > +               writel(values[i / 4], ch->ebm->mbox_mmio + ch->c->tx_offset + i);
> > > +
> > > +       writel(BIT(ch->num), ch->ebm->mbox_ctl + MBOX_CTL_IRQ_SET);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int mtk_gpueb_mbox_startup(struct mbox_chan *chan)
> > > +{
> > > +       struct mtk_gpueb_mbox_chan *ch = chan->con_priv;
> > > +       int ret;
> > > +
> > > +       atomic_set(&ch->rx_status, 0);
> > > +
> > > +       ret = clk_enable(ch->ebm->clk);
> > > +       if (ret) {
> > > +               dev_err(ch->ebm->dev, "Failed to enable EB clock: %pe\n",
> > > +                       ERR_PTR(ret));
> > > +               goto err_clog;
> > > +       }
> > > +
> > > +       writel(BIT(ch->num), ch->ebm->mbox_ctl + MBOX_CTL_IRQ_CLR);
> > > +
> > > +       ret = devm_request_threaded_irq(ch->ebm->dev, ch->ebm->irq, mtk_gpueb_mbox_isr,
> > > +                                       mtk_gpueb_mbox_thread, IRQF_SHARED | IRQF_ONESHOT,
> > > +                                       ch->full_name, ch);
> > I don't think this warrants a per-channel irq thread.
> >
> > mbox_chan_received_data is atomic. I think wecan start simple with
> > just a devm_request_irq for all channels. mtk_gpueb_mbox_isr can
> >
> >   read bits from MBOX_CTL_RX_STS
> >   for each bit set:
> >     read data from rx
> >     mbox_chan_received_data
> >   write bits to MBOX_CTL_IRQ_CLR
> >
>
> I don't like this approach. It brings us back to having to process
> multiple channels per ISR, keep track of when the interrupt should
> be enabled and disabled based on how many channels are in use, and
> also is not in line with what e.g. omap-mailbox.c does.
>
> Remember that `mbox_chan_received_data` synchronously calls the
> mailbox client's rx_callback. In mediatek_mfg's case, this is
> fairly small, though with the request to not make the rx buffer
> persist beyond the rx_callback it will gain an additional memory
> copy. But we can't guarantee that someone isn't going to put a
> slow operation in the path. Sure, it's going to be atomic, but
> waiting for a spinlock is atomic and not something an ISR would
> enjoy. I don't think mailbox clients would expect that if they
> take their time they'll stall the interrupt handler for every
> other channel.
>
> So we'd keep the interrupt disabled for all channels until the
> client that received a message has processed it.
>
> I can see myself getting rid of the handler and just having the
> thread function as the bottom half, but I'd really like to keep
> the one-IRQ-request-per-channel thing I've got going now as it
> made the code a lot easier to reason about. However, doing this
> would mean the interrupt is re-enabled after the generic upper
> half, when all the business logic that needs to not run
> concurrently for an individual channel is in the bottom half.
>
> As far as I can tell, this would then mean we'd have to add
> some concurrency exclusion mechanism to the bottom half.
>
> Moving all the logic into the upper half handler function
> would make that handler somewhat longer, and I don't know
> if IRQF_ONESHOT masks the interrupt for all users of that
> IRQ number or just for those with that dev_id. If it's per
> dev_id, then I'm fine with moving stuff up there. But from
> my reading of the core IRQ handling code, that does not
> appear to be the case; one channel getting a reply would
> mask *all* channels of the mailbox until the upper half is
> completed, and if the upper half calls into a driver
> callback synchronously, that may take a hot minute.
>
> Put differently: Is there a problem with one thread per used
> channel, or are we going off vibes here? The way it currently
> works uses the shared interrupt to mark just that one channel
> as busy with rx_status before letting the IRQ for all channels
> be unmasked again, which seems ideal to me.
No, one thread per used channel can work. I can't say I like it, but I
also don't know the hw as well as you do.

Re: [PATCH v2 09/10] drm/panthor: devfreq: add pluggable devfreq providers

[Chia-I Wu]

On Mon, Sep 15, 2025 at 6:34 AM Nicolas Frattaroli
<nicolas.frattaroli@collabora.com> wrote:
>
> On Saturday, 13 September 2025 00:53:50 Central European Summer Time Chia-I Wu wrote:
> > On Fri, Sep 12, 2025 at 11:38 AM Nicolas Frattaroli
> > <nicolas.frattaroli@collabora.com> wrote:
> > <snipped>
> > > diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.h b/drivers/gpu/drm/panthor/panthor_devfreq.h
> > > index a891cb5fdc34636444f141e10f5d45828fc35b51..94c9768d5d038c4ba8516929edb565a1f13443fb 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_devfreq.h
> > > +++ b/drivers/gpu/drm/panthor/panthor_devfreq.h
> > > @@ -8,6 +8,7 @@
> > >
> > >  struct devfreq;
> > >  struct thermal_cooling_device;
> > > +struct platform_device;
> > >
> > >  struct panthor_device;
> > >
> > > @@ -43,6 +44,19 @@ struct panthor_devfreq {
> > >         spinlock_t lock;
> > >  };
> > >
> > > +struct panthor_devfreq_provider {
> > > +       /** @dev: device pointer to the provider device */
> > > +       struct device *dev;
> > > +       /**
> > > +        * @init: the provider's init callback that allocates a
> > > +        * &struct panthor_devfreq, adds it to panthor, and adds a devfreq
> > > +        * device to panthor. Will be called during panthor's probe.
> > > +        */
> > > +       int (*init)(struct panthor_device *ptdev, struct device *dev);
> > > +
> > > +       struct list_head node;
> > > +};
> > On mt8196, we have performance-domains to replace several other
> > properties: clocks, *-supply, power-domains, operating-points-v2.
> > There are also quirks, such as GPU_SHADER_PRESENT should be masked by
> > GF_REG_SHADER_PRESENT. It feels like that the scope of
> > panthor_devfreq_provider is more broader, and at least the naming is
> > not right.
>
> True, though I'm still not entirely sure whether mtk_mfg needs to do
> the GF_REG_SHADER_PRESENT thing. It's entirely possible this is just
> an efuse value the GPUEB reads and then puts in SRAM for us, and we
> could simply read this efuse cell ourselves. Among a list of questions
> about the hardware we're sending to MediaTek, whether this is an efuse
> cell and where it is placed is one of them.
>
> If it turns out to be the case that we can simply read an efuse in
> panthor in the other mt8196 integration code, then we can keep
> mtk_mfg basically entirely focused on the devfreq-y part. I'd really
> prefer this solution.
>
> However, assuming we can't go down this path either because this is
> not how the hardware works, or because MediaTek never replies, or
> because someone doesn't like reading efuses in panthor, I think
> generalising "devfreq_provider" to "performance_controller" or
> something like that would be a good move.
Yeah, let's see what MTK has to say on shader core mask.

Another thing is that panthor still requires a "core" clk. Is it also
required on mt8196?

>
> In a way, the fused off core mask is part of the vague domain of
> "performance", and it'll also allow us to extend it with other
> things relevant to performance control in different vendor integration
> logic designs. I'm thinking memory bandwidth control and job scheduling
> preferences. E.g. if the interconnect tells us one core is spending a
> lot of time waiting on the interconnect, maybe because a different
> piece of the SoC that's active shares the same path on the
> interconnect, we could then communicate a scheduling preference for
> the other cores that have bandwidth headroom even if they are busier
> in compute. Maybe this doesn't make sense though because interconnect
> designs are fully switched these days or panthor's scheduler will
> already figure this out from job completion times.
>
> If any other SoC vendor or people working on hardware of those vendors
> want to chime in and say whether they've got any other uses for
> communicating more than just devfreq from glue logic to panthor, then
> this would be a great time to do it, so that we can get this interface
> right from the beginning.
>
> > Another issue is I am not sure if we need to expose panthor_device
> > internals to the provider. mtk_mfg accesses very few fields of
> > panthor_device. It seems we can make the two less coupled.
> >
> > I might change my view as mtk_mfg evolves and requires tigher
> > integration with panthor. But as is, I might prefer for mtk_mfg to
> > live under drivers/soc/mediatek and provide a header for panthor to
> > use in soc-specific path.
>
> I'm not very confident it's possible to cleanly decouple them without
> inventing a bunch of very panthor-and-mfg specific interfaces that
> masquerade as general solutions in the process. It'd also mean I'd
> have to duplicate all of `panthor_devfreq_get_dev_status` instead of
> just being able to reuse it, unless that is also exposed in said
> header file, which would need a better justification than "well there
> is one other user of it and we're trying to couple it more loosely".
>
> I know that it's almost independent, but unfortunately, even a tiny
> dependency between the two will mean that mediatek_mfg will need to
> know about panthor.
>
> Other things needed from panthor are the pdevfreq->gov_data, and
> the panthor struct device* itself, as well as stuff like "fast_rate"
> in the panthor_device struct.
>
> In the future, we may want to expand this driver with governors
> beyond SIMPLE_ONDEMAND, based on the job completion duration targets
> we can communicate to the GPUEB. That may either make the driver
> more tightly coupled or more loosely coupled, I don't really know
> yet.
>
> One advantage of looking to completely decouple them (though again,
> I doubt that's possible at the moment without questionable refactors)
> could be that we could also support panfrost devices that need this.
There is also tyr, although I don't follow its status.

I can see the concern over "very panthor-and-mfg specific interfaces
that masquerade as general solutions" or "questionable refactors". But
I also don't like, for example, how mtk_mfg_init_devfreq inits
panthor_devfreq manually. Beyond initialization, the remaining
coupling comes from that we need panthor to provide get_dev_status
callback for devfreq_dev_profile, and we need mtk_mfg to provide
target and get_cur_freq callbacks. That seems like something solvable
too.

I really appreciate the work and I don't want to block it by vague
concerns. If others have no preference, we should start with what we
have.
>
> >
> >
> > > +
> > >
> > >  int panthor_devfreq_init(struct panthor_device *ptdev);
> > >
> > > @@ -57,4 +71,6 @@ int panthor_devfreq_get_dev_status(struct device *dev,
> > >
> > >  unsigned long panthor_devfreq_get_freq(struct panthor_device *ptdev);
> > >
> > > +int panthor_devfreq_register_provider(struct panthor_devfreq_provider *prov);
> > > +
> > >  #endif /* __PANTHOR_DEVFREQ_H__ */
> > >
> > > --
> > > 2.51.0
> > >
> >
>
>
>
>

Re: [PATCH v2 05/10] mailbox: add MediaTek GPUEB IPI mailbox

[Nicolas Frattaroli]

On Tuesday, 16 September 2025 06:55:30 Central European Summer Time Chia-I Wu wrote:
> On Mon, Sep 15, 2025 at 6:34 AM Nicolas Frattaroli
> <nicolas.frattaroli@collabora.com> wrote:
> >
> > On Saturday, 13 September 2025 00:11:10 Central European Summer Time Chia-I Wu wrote:
> > > On Fri, Sep 12, 2025 at 11:38 AM Nicolas Frattaroli
> > > <nicolas.frattaroli@collabora.com> wrote:
> > > <snipped>
> > > > +static irqreturn_t mtk_gpueb_mbox_thread(int irq, void *data)
> > > > +{
> > > > +       struct mtk_gpueb_mbox_chan *ch = data;
> > > > +       int status;
> > > > +
> > > > +       status = atomic_cmpxchg(&ch->rx_status,
> > > > +                               MBOX_FULL | MBOX_CLOGGED, MBOX_FULL);
> > > > +       if (status == (MBOX_FULL | MBOX_CLOGGED)) {
> > > > +               mtk_gpueb_mbox_read_rx(ch);
> > > > +               writel(BIT(ch->num), ch->ebm->mbox_ctl + MBOX_CTL_IRQ_CLR);
> > > > +               mbox_chan_received_data(&ch->ebm->mbox.chans[ch->num],
> > > > +                                       ch->rx_buf);
> > > Given what other drivers do, and how mtk_mfg consumes the data, we should
> > >
> > >   char buf[MAX_OF_RX_LEN]; //  MAX_OF_RX_LEN is 32; we can also
> > > allocate it during probe
> > >   mtk_gpueb_mbox_read_rx(ch);
> > >   mbox_chan_received_data(..., buf);
> > >
> > > mtx_mfg makes a copy eventually anyway.
> >
> > We don't right now, at least not until after the callback returns.
> > So we need to have the copy in the mtk_mfg callback, not after the
> > completion. That's fine and I do want to do this as this is what
> > the mailbox framework seems to expect clients to do.
> >
> > > We don't need to maintain any
> > > extra copy.
> > >
> > > Then we might not need rx_status.
> >
> > We can probably get rid of it if we keep the per-channel
> > interrupt handler. Otherwise, we may still need clogged,
> > as we don't want to process interrupts on channels we have
> > no user for.
> >
> > >
> > > > +               atomic_set(&ch->rx_status, 0);
> > > > +               return IRQ_HANDLED;
> > > > +       }
> > > > +
> > > > +       return IRQ_NONE;
> > > > +}
> > > > +
> > > > +static int mtk_gpueb_mbox_send_data(struct mbox_chan *chan, void *data)
> > > > +{
> > > > +       struct mtk_gpueb_mbox_chan *ch = chan->con_priv;
> > > > +       int i;
> > > > +       u32 *values = data;
> > > > +
> > > > +       if (atomic_read(&ch->rx_status))
> > > > +               return -EBUSY;
> > > > +
> > > > +       /*
> > > > +        * We don't want any fancy nonsense, just write the 32-bit values in
> > > > +        * order. memcpy_toio/__iowrite32_copy don't work here, because fancy.
> > > > +        */
> > > > +       for (i = 0; i < ch->c->tx_len; i += 4)
> > > > +               writel(values[i / 4], ch->ebm->mbox_mmio + ch->c->tx_offset + i);
> > > > +
> > > > +       writel(BIT(ch->num), ch->ebm->mbox_ctl + MBOX_CTL_IRQ_SET);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static int mtk_gpueb_mbox_startup(struct mbox_chan *chan)
> > > > +{
> > > > +       struct mtk_gpueb_mbox_chan *ch = chan->con_priv;
> > > > +       int ret;
> > > > +
> > > > +       atomic_set(&ch->rx_status, 0);
> > > > +
> > > > +       ret = clk_enable(ch->ebm->clk);
> > > > +       if (ret) {
> > > > +               dev_err(ch->ebm->dev, "Failed to enable EB clock: %pe\n",
> > > > +                       ERR_PTR(ret));
> > > > +               goto err_clog;
> > > > +       }
> > > > +
> > > > +       writel(BIT(ch->num), ch->ebm->mbox_ctl + MBOX_CTL_IRQ_CLR);
> > > > +
> > > > +       ret = devm_request_threaded_irq(ch->ebm->dev, ch->ebm->irq, mtk_gpueb_mbox_isr,
> > > > +                                       mtk_gpueb_mbox_thread, IRQF_SHARED | IRQF_ONESHOT,
> > > > +                                       ch->full_name, ch);
> > > I don't think this warrants a per-channel irq thread.
> > >
> > > mbox_chan_received_data is atomic. I think wecan start simple with
> > > just a devm_request_irq for all channels. mtk_gpueb_mbox_isr can
> > >
> > >   read bits from MBOX_CTL_RX_STS
> > >   for each bit set:
> > >     read data from rx
> > >     mbox_chan_received_data
> > >   write bits to MBOX_CTL_IRQ_CLR
> > >
> >
> > I don't like this approach. It brings us back to having to process
> > multiple channels per ISR, keep track of when the interrupt should
> > be enabled and disabled based on how many channels are in use, and
> > also is not in line with what e.g. omap-mailbox.c does.
> >
> > Remember that `mbox_chan_received_data` synchronously calls the
> > mailbox client's rx_callback. In mediatek_mfg's case, this is
> > fairly small, though with the request to not make the rx buffer
> > persist beyond the rx_callback it will gain an additional memory
> > copy. But we can't guarantee that someone isn't going to put a
> > slow operation in the path. Sure, it's going to be atomic, but
> > waiting for a spinlock is atomic and not something an ISR would
> > enjoy. I don't think mailbox clients would expect that if they
> > take their time they'll stall the interrupt handler for every
> > other channel.
> >
> > So we'd keep the interrupt disabled for all channels until the
> > client that received a message has processed it.
> >
> > I can see myself getting rid of the handler and just having the
> > thread function as the bottom half, but I'd really like to keep
> > the one-IRQ-request-per-channel thing I've got going now as it
> > made the code a lot easier to reason about. However, doing this
> > would mean the interrupt is re-enabled after the generic upper
> > half, when all the business logic that needs to not run
> > concurrently for an individual channel is in the bottom half.
> >
> > As far as I can tell, this would then mean we'd have to add
> > some concurrency exclusion mechanism to the bottom half.
> >
> > Moving all the logic into the upper half handler function
> > would make that handler somewhat longer, and I don't know
> > if IRQF_ONESHOT masks the interrupt for all users of that
> > IRQ number or just for those with that dev_id. If it's per
> > dev_id, then I'm fine with moving stuff up there. But from
> > my reading of the core IRQ handling code, that does not
> > appear to be the case; one channel getting a reply would
> > mask *all* channels of the mailbox until the upper half is
> > completed, and if the upper half calls into a driver
> > callback synchronously, that may take a hot minute.
> >
> > Put differently: Is there a problem with one thread per used
> > channel, or are we going off vibes here? The way it currently
> > works uses the shared interrupt to mark just that one channel
> > as busy with rx_status before letting the IRQ for all channels
> > be unmasked again, which seems ideal to me.
> No, one thread per used channel can work. I can't say I like it, but I
> also don't know the hw as well as you do.
> 

Your knowledge is probably not far behind mine on this hardware :(

I'll keep the per-channel thread for v3 for now, so that it's
clearer as to how this will look. It'll also give us both an
opportunity to run the code and add some measurements to see if
this causes any problems, and to experiment with your proposed
solution.

What I mainly am worried about is that if we go back to one IRQ
for all channels, then we have to do our own "how many channels
are enabled?" accounting to disable the IRQ later, because the
enable_irq/disable_irq accounting works the opposite way where
you can disable as many times as you want but your enables can't
exceed disables + 1.

Kind regards,
Nicolas Frattaroli

Re: [PATCH v2 09/10] drm/panthor: devfreq: add pluggable devfreq providers

[Nicolas Frattaroli]

On Tuesday, 16 September 2025 08:17:18 Central European Summer Time Chia-I Wu wrote:
> On Mon, Sep 15, 2025 at 6:34 AM Nicolas Frattaroli
> <nicolas.frattaroli@collabora.com> wrote:
> >
> > On Saturday, 13 September 2025 00:53:50 Central European Summer Time Chia-I Wu wrote:
> > > On Fri, Sep 12, 2025 at 11:38 AM Nicolas Frattaroli
> > > <nicolas.frattaroli@collabora.com> wrote:
> > > <snipped>
> > > > diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.h b/drivers/gpu/drm/panthor/panthor_devfreq.h
> > > > index a891cb5fdc34636444f141e10f5d45828fc35b51..94c9768d5d038c4ba8516929edb565a1f13443fb 100644
> > > > --- a/drivers/gpu/drm/panthor/panthor_devfreq.h
> > > > +++ b/drivers/gpu/drm/panthor/panthor_devfreq.h
> > > > @@ -8,6 +8,7 @@
> > > >
> > > >  struct devfreq;
> > > >  struct thermal_cooling_device;
> > > > +struct platform_device;
> > > >
> > > >  struct panthor_device;
> > > >
> > > > @@ -43,6 +44,19 @@ struct panthor_devfreq {
> > > >         spinlock_t lock;
> > > >  };
> > > >
> > > > +struct panthor_devfreq_provider {
> > > > +       /** @dev: device pointer to the provider device */
> > > > +       struct device *dev;
> > > > +       /**
> > > > +        * @init: the provider's init callback that allocates a
> > > > +        * &struct panthor_devfreq, adds it to panthor, and adds a devfreq
> > > > +        * device to panthor. Will be called during panthor's probe.
> > > > +        */
> > > > +       int (*init)(struct panthor_device *ptdev, struct device *dev);
> > > > +
> > > > +       struct list_head node;
> > > > +};
> > > On mt8196, we have performance-domains to replace several other
> > > properties: clocks, *-supply, power-domains, operating-points-v2.
> > > There are also quirks, such as GPU_SHADER_PRESENT should be masked by
> > > GF_REG_SHADER_PRESENT. It feels like that the scope of
> > > panthor_devfreq_provider is more broader, and at least the naming is
> > > not right.
> >
> > True, though I'm still not entirely sure whether mtk_mfg needs to do
> > the GF_REG_SHADER_PRESENT thing. It's entirely possible this is just
> > an efuse value the GPUEB reads and then puts in SRAM for us, and we
> > could simply read this efuse cell ourselves. Among a list of questions
> > about the hardware we're sending to MediaTek, whether this is an efuse
> > cell and where it is placed is one of them.
> >
> > If it turns out to be the case that we can simply read an efuse in
> > panthor in the other mt8196 integration code, then we can keep
> > mtk_mfg basically entirely focused on the devfreq-y part. I'd really
> > prefer this solution.
> >
> > However, assuming we can't go down this path either because this is
> > not how the hardware works, or because MediaTek never replies, or
> > because someone doesn't like reading efuses in panthor, I think
> > generalising "devfreq_provider" to "performance_controller" or
> > something like that would be a good move.
> Yeah, let's see what MTK has to say on shader core mask.
> 
> Another thing is that panthor still requires a "core" clk. Is it also
> required on mt8196?

Nope, I'm planning on getting rid of it in v3.

> >
> > In a way, the fused off core mask is part of the vague domain of
> > "performance", and it'll also allow us to extend it with other
> > things relevant to performance control in different vendor integration
> > logic designs. I'm thinking memory bandwidth control and job scheduling
> > preferences. E.g. if the interconnect tells us one core is spending a
> > lot of time waiting on the interconnect, maybe because a different
> > piece of the SoC that's active shares the same path on the
> > interconnect, we could then communicate a scheduling preference for
> > the other cores that have bandwidth headroom even if they are busier
> > in compute. Maybe this doesn't make sense though because interconnect
> > designs are fully switched these days or panthor's scheduler will
> > already figure this out from job completion times.
> >
> > If any other SoC vendor or people working on hardware of those vendors
> > want to chime in and say whether they've got any other uses for
> > communicating more than just devfreq from glue logic to panthor, then
> > this would be a great time to do it, so that we can get this interface
> > right from the beginning.
> >
> > > Another issue is I am not sure if we need to expose panthor_device
> > > internals to the provider. mtk_mfg accesses very few fields of
> > > panthor_device. It seems we can make the two less coupled.
> > >
> > > I might change my view as mtk_mfg evolves and requires tigher
> > > integration with panthor. But as is, I might prefer for mtk_mfg to
> > > live under drivers/soc/mediatek and provide a header for panthor to
> > > use in soc-specific path.
> >
> > I'm not very confident it's possible to cleanly decouple them without
> > inventing a bunch of very panthor-and-mfg specific interfaces that
> > masquerade as general solutions in the process. It'd also mean I'd
> > have to duplicate all of `panthor_devfreq_get_dev_status` instead of
> > just being able to reuse it, unless that is also exposed in said
> > header file, which would need a better justification than "well there
> > is one other user of it and we're trying to couple it more loosely".
> >
> > I know that it's almost independent, but unfortunately, even a tiny
> > dependency between the two will mean that mediatek_mfg will need to
> > know about panthor.
> >
> > Other things needed from panthor are the pdevfreq->gov_data, and
> > the panthor struct device* itself, as well as stuff like "fast_rate"
> > in the panthor_device struct.
> >
> > In the future, we may want to expand this driver with governors
> > beyond SIMPLE_ONDEMAND, based on the job completion duration targets
> > we can communicate to the GPUEB. That may either make the driver
> > more tightly coupled or more loosely coupled, I don't really know
> > yet.
> >
> > One advantage of looking to completely decouple them (though again,
> > I doubt that's possible at the moment without questionable refactors)
> > could be that we could also support panfrost devices that need this.
> There is also tyr, although I don't follow its status.
> 
> I can see the concern over "very panthor-and-mfg specific interfaces
> that masquerade as general solutions" or "questionable refactors". But
> I also don't like, for example, how mtk_mfg_init_devfreq inits
> panthor_devfreq manually. Beyond initialization, the remaining
> coupling comes from that we need panthor to provide get_dev_status
> callback for devfreq_dev_profile, and we need mtk_mfg to provide
> target and get_cur_freq callbacks. That seems like something solvable
> too.

Yeah I agree, I think the panthor_devfreq initialisation should happen
within panthor_devfreq. It's independent of registering the actual
devfreq device.

I'll note that down as something I will refactor. Once that's done,
I'll have a clearer picture of whether moving the driver out of
panthor is feasible.

> 
> I really appreciate the work and I don't want to block it by vague
> concerns. If others have no preference, we should start with what we
> have.

Thanks! Don't worry, this is in no rush to be merged, since mainline
still doesn't have everything to boot on this platform, so we're not
in a hurry to get GPU enablement done I don't think. The important
part is getting this right in a way where panthor doesn't carry my
technical debt for years to come, so I'm grateful for the reviews.

Kind regards,
Nicolas Frattaroli

> >
> > >
> > >
> > > > +
> > > >
> > > >  int panthor_devfreq_init(struct panthor_device *ptdev);
> > > >
> > > > @@ -57,4 +71,6 @@ int panthor_devfreq_get_dev_status(struct device *dev,
> > > >
> > > >  unsigned long panthor_devfreq_get_freq(struct panthor_device *ptdev);
> > > >
> > > > +int panthor_devfreq_register_provider(struct panthor_devfreq_provider *prov);
> > > > +
> > > >  #endif /* __PANTHOR_DEVFREQ_H__ */
> > > >
> > > > --
> > > > 2.51.0
> > > >
> > >
> >
> >
> >
> >
>