* [PATCH v4 1/8] dt-bindings: gpu: mali-valhall-csf: add mediatek,mt8196-mali variant
2025-09-23 11:39 [PATCH v4 0/8] MT8196 GPU Frequency/Power Control Support Nicolas Frattaroli
@ 2025-09-23 11:39 ` Nicolas Frattaroli
2025-09-23 11:39 ` [PATCH v4 2/8] dt-bindings: power: Add MT8196 GPU frequency control binding Nicolas Frattaroli
` (6 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Nicolas Frattaroli @ 2025-09-23 11:39 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Boris Brezillon, Jassi Brar,
Chia-I Wu, Chen-Yu Tsai, Steven Price, Liviu Dudau,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, Kees Cook, Gustavo A. R. Silva, Ulf Hansson
Cc: kernel, dri-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-hardening, linux-pm, Nicolas Frattaroli
The Mali-based GPU on the MediaTek MT8196 SoC uses a separate MCU to
control the power and frequency of the GPU. This is modelled as a power
domain and clock provider.
It lets us omit the OPP tables from the device tree, as those can now be
enumerated at runtime from the MCU.
Add the necessary schema logic to handle what this SoC expects in terms
of clocks and power-domains.
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
.../bindings/gpu/arm,mali-valhall-csf.yaml | 40 ++++++++++++++++++++--
1 file changed, 38 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
index 7ad5a3ffc5f5c753322eda9e74cc65de89d11c73..860691ce985e560536b6c515b82441ba6d367c46 100644
--- a/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
+++ b/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.yaml
@@ -45,7 +45,9 @@ properties:
minItems: 1
items:
- const: core
- - const: coregroup
+ - enum:
+ - coregroup
+ - stacks
- const: stacks
mali-supply: true
@@ -92,7 +94,6 @@ required:
- interrupts
- interrupt-names
- clocks
- - mali-supply
additionalProperties: false
@@ -109,6 +110,29 @@ allOf:
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
+ power-domains:
+ maxItems: 1
+ power-domain-names: false
+ clocks:
+ maxItems: 2
+ clock-names:
+ items:
+ - const: core
+ - const: stacks
+ required:
+ - power-domains
examples:
- |
@@ -144,5 +168,17 @@ examples:
};
};
};
+ - |
+ gpu@48000000 {
+ compatible = "mediatek,mt8196-mali", "arm,mali-valhall-csf";
+ reg = <0x48000000 0x480000>;
+ clocks = <&gpufreq 0>, <&gpufreq 1>;
+ clock-names = "core", "stacks";
+ 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";
+ power-domains = <&gpufreq>;
+ };
...
--
2.51.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v4 2/8] dt-bindings: power: Add MT8196 GPU frequency control binding
2025-09-23 11:39 [PATCH v4 0/8] MT8196 GPU Frequency/Power Control Support Nicolas Frattaroli
2025-09-23 11:39 ` [PATCH v4 1/8] dt-bindings: gpu: mali-valhall-csf: add mediatek,mt8196-mali variant Nicolas Frattaroli
@ 2025-09-23 11:39 ` Nicolas Frattaroli
2025-09-24 11:30 ` AngeloGioacchino Del Regno
2025-09-23 11:39 ` [PATCH v4 3/8] dt-bindings: sram: Add compatible for mediatek,mt8196-gpufreq-sram Nicolas Frattaroli
` (5 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Nicolas Frattaroli @ 2025-09-23 11:39 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Boris Brezillon, Jassi Brar,
Chia-I Wu, Chen-Yu Tsai, Steven Price, Liviu Dudau,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, Kees Cook, Gustavo A. R. Silva, Ulf Hansson
Cc: kernel, dri-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-hardening, linux-pm, Nicolas Frattaroli
On the MT8196 and MT6991 SoCs, the GPU power and frequency is controlled
by some integration logic, referred to as "MFlexGraphics" by MediaTek,
which comes in the form of an embedded controller running
special-purpose firmware.
This controller takes care of the regulators and PLL clock frequencies
to squeeze the maximum amount of power out of the silicon.
Add a binding which models it as a power domain.
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
.../bindings/power/mediatek,mt8196-gpufreq.yaml | 117 +++++++++++++++++++++
1 file changed, 117 insertions(+)
diff --git a/Documentation/devicetree/bindings/power/mediatek,mt8196-gpufreq.yaml b/Documentation/devicetree/bindings/power/mediatek,mt8196-gpufreq.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..03721244a737ce0914a89cc0aedd88fa3b6b2038
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/mediatek,mt8196-gpufreq.yaml
@@ -0,0 +1,117 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/mediatek,mt8196-gpufreq.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek MFlexGraphics Power and Frequency 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: '^power-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: core
+ - const: stack0
+ - const: stack1
+
+ 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
+
+ "#clock-cells":
+ const: 1
+
+ "#power-domain-cells":
+ const: 0
+
+required:
+ - compatible
+ - reg
+ - reg-names
+ - clocks
+ - clock-names
+ - mboxes
+ - mbox-names
+ - shmem
+ - "#clock-cells"
+ - "#power-domain-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/mediatek,mt8196-clock.h>
+
+ power-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", "core", "stack0", "stack1";
+ 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>;
+ #clock-cells = <1>;
+ #power-domain-cells = <0>;
+ };
--
2.51.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 2/8] dt-bindings: power: Add MT8196 GPU frequency control binding
2025-09-23 11:39 ` [PATCH v4 2/8] dt-bindings: power: Add MT8196 GPU frequency control binding Nicolas Frattaroli
@ 2025-09-24 11:30 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 18+ messages in thread
From: AngeloGioacchino Del Regno @ 2025-09-24 11:30 UTC (permalink / raw)
To: Nicolas Frattaroli, Boris Brezillon, Jassi Brar, Chia-I Wu,
Chen-Yu Tsai, Steven Price, Liviu Dudau, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
Kees Cook, Gustavo A. R. Silva, Ulf Hansson
Cc: kernel, dri-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-hardening, linux-pm
Il 23/09/25 13:39, Nicolas Frattaroli ha scritto:
> On the MT8196 and MT6991 SoCs, the GPU power and frequency is controlled
> by some integration logic, referred to as "MFlexGraphics" by MediaTek,
> which comes in the form of an embedded controller running
> special-purpose firmware.
>
> This controller takes care of the regulators and PLL clock frequencies
> to squeeze the maximum amount of power out of the silicon.
>
> Add a binding which models it as a power domain.
>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
> .../bindings/power/mediatek,mt8196-gpufreq.yaml | 117 +++++++++++++++++++++
> 1 file changed, 117 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/mediatek,mt8196-gpufreq.yaml b/Documentation/devicetree/bindings/power/mediatek,mt8196-gpufreq.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..03721244a737ce0914a89cc0aedd88fa3b6b2038
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/mediatek,mt8196-gpufreq.yaml
> @@ -0,0 +1,117 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/mediatek,mt8196-gpufreq.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek MFlexGraphics Power and Frequency Controller
> +
> +maintainers:
> + - Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> +
> +description: |
This vertical bar is not needed. Please drop it.
Cheers,
Angelo
> + A special-purpose embedded MCU to control power and frequency of GPU devices
> + using MediaTek Flexible Graphics integration hardware.
> +
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 3/8] dt-bindings: sram: Add compatible for mediatek,mt8196-gpufreq-sram
2025-09-23 11:39 [PATCH v4 0/8] MT8196 GPU Frequency/Power Control Support Nicolas Frattaroli
2025-09-23 11:39 ` [PATCH v4 1/8] dt-bindings: gpu: mali-valhall-csf: add mediatek,mt8196-mali variant Nicolas Frattaroli
2025-09-23 11:39 ` [PATCH v4 2/8] dt-bindings: power: Add MT8196 GPU frequency control binding Nicolas Frattaroli
@ 2025-09-23 11:39 ` Nicolas Frattaroli
2025-09-23 11:39 ` [PATCH v4 4/8] dt-bindings: mailbox: Add MT8196 GPUEB Mailbox Nicolas Frattaroli
` (4 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Nicolas Frattaroli @ 2025-09-23 11:39 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Boris Brezillon, Jassi Brar,
Chia-I Wu, Chen-Yu Tsai, Steven Price, Liviu Dudau,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, Kees Cook, Gustavo A. R. Silva, Ulf Hansson
Cc: kernel, dri-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-hardening, linux-pm, 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.
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v4 4/8] dt-bindings: mailbox: Add MT8196 GPUEB Mailbox
2025-09-23 11:39 [PATCH v4 0/8] MT8196 GPU Frequency/Power Control Support Nicolas Frattaroli
` (2 preceding siblings ...)
2025-09-23 11:39 ` [PATCH v4 3/8] dt-bindings: sram: Add compatible for mediatek,mt8196-gpufreq-sram Nicolas Frattaroli
@ 2025-09-23 11:39 ` Nicolas Frattaroli
2025-09-23 14:39 ` Rob Herring (Arm)
2025-09-23 11:39 ` [PATCH v4 5/8] mailbox: add MediaTek GPUEB IPI mailbox Nicolas Frattaroli
` (3 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Nicolas Frattaroli @ 2025-09-23 11:39 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Boris Brezillon, Jassi Brar,
Chia-I Wu, Chen-Yu Tsai, Steven Price, Liviu Dudau,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, Kees Cook, Gustavo A. R. Silva, Ulf Hansson
Cc: kernel, dri-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-hardening, linux-pm, 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.
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
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..ab5b780cb83a708a3897ca1a440131d97b56c3a6
--- /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>
+
+ 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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 4/8] dt-bindings: mailbox: Add MT8196 GPUEB Mailbox
2025-09-23 11:39 ` [PATCH v4 4/8] dt-bindings: mailbox: Add MT8196 GPUEB Mailbox Nicolas Frattaroli
@ 2025-09-23 14:39 ` Rob Herring (Arm)
0 siblings, 0 replies; 18+ messages in thread
From: Rob Herring (Arm) @ 2025-09-23 14:39 UTC (permalink / raw)
To: Nicolas Frattaroli
Cc: linux-arm-kernel, dri-devel, Simona Vetter, Jassi Brar, Chia-I Wu,
Steven Price, linux-mediatek, AngeloGioacchino Del Regno, kernel,
Kees Cook, Thomas Zimmermann, Chen-Yu Tsai, Matthias Brugger,
David Airlie, Gustavo A. R. Silva, Liviu Dudau, linux-kernel,
Maarten Lankhorst, Krzysztof Kozlowski, Boris Brezillon,
devicetree, linux-hardening, linux-pm, Ulf Hansson, Conor Dooley,
Maxime Ripard
On Tue, 23 Sep 2025 13:39:57 +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.
>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
> .../mailbox/mediatek,mt8196-gpueb-mbox.yaml | 64 ++++++++++++++++++++++
> 1 file changed, 64 insertions(+)
>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 5/8] mailbox: add MediaTek GPUEB IPI mailbox
2025-09-23 11:39 [PATCH v4 0/8] MT8196 GPU Frequency/Power Control Support Nicolas Frattaroli
` (3 preceding siblings ...)
2025-09-23 11:39 ` [PATCH v4 4/8] dt-bindings: mailbox: Add MT8196 GPUEB Mailbox Nicolas Frattaroli
@ 2025-09-23 11:39 ` Nicolas Frattaroli
2025-09-24 11:35 ` AngeloGioacchino Del Regno
2025-09-23 11:39 ` [PATCH v4 6/8] drm/panthor: call into devfreq for current frequency Nicolas Frattaroli
` (2 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Nicolas Frattaroli @ 2025-09-23 11:39 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Boris Brezillon, Jassi Brar,
Chia-I Wu, Chen-Yu Tsai, Steven Price, Liviu Dudau,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, Kees Cook, Gustavo A. R. Silva, Ulf Hansson
Cc: kernel, dri-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-hardening, linux-pm, 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 | 318 ++++++++++++++++++++++++++++++++++++
3 files changed, 330 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..d388418349dfb70f6aff4756fc9a368c2325135f
--- /dev/null
+++ b/drivers/mailbox/mtk-gpueb-mailbox.c
@@ -0,0 +1,318 @@
+// 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 GPUEB_MBOX_CTL_TX_STS 0x00
+#define GPUEB_MBOX_CTL_IRQ_SET 0x04
+#define GPUEB_MBOX_CTL_IRQ_CLR 0x74
+#define GPUEB_MBOX_CTL_RX_STS 0x78
+
+#define GPUEB_MBOX_FULL BIT(0) /* i.e. we've received data */
+#define GPUEB_MBOX_BLOCKED BIT(1) /* i.e. the channel is shutdown */
+
+#define GPUEB_MBOX_MAX_RX_SIZE 32 /* in bytes */
+
+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_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;
+ 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
+ * @buf: buffer to read into
+ * @chan: pointer to the channel to read
+ */
+static void mtk_gpueb_mbox_read_rx(void *buf, struct mtk_gpueb_mbox_chan *chan)
+{
+ memcpy_fromio(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 + GPUEB_MBOX_CTL_RX_STS);
+
+ if (rx_sts & BIT(ch->num)) {
+ if (!atomic_cmpxchg(&ch->rx_status, 0, GPUEB_MBOX_FULL | GPUEB_MBOX_BLOCKED))
+ 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;
+ u8 buf[GPUEB_MBOX_MAX_RX_SIZE] = {};
+ int status;
+
+ status = atomic_cmpxchg(&ch->rx_status, GPUEB_MBOX_FULL | GPUEB_MBOX_BLOCKED,
+ GPUEB_MBOX_FULL);
+ if (status == (GPUEB_MBOX_FULL | GPUEB_MBOX_BLOCKED)) {
+ mtk_gpueb_mbox_read_rx(buf, ch);
+ writel(BIT(ch->num), ch->ebm->mbox_ctl + GPUEB_MBOX_CTL_IRQ_CLR);
+ mbox_chan_received_data(&ch->ebm->mbox.chans[ch->num], 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;
+ u32 *values = data;
+ int i;
+
+ 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, as they may use
+ * writes of different sizes or memory ordering characteristics depending
+ * on the architecture, alignment and the current phase of the moon.
+ */
+ 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 + GPUEB_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_block;
+ }
+
+ writel(BIT(ch->num), ch->ebm->mbox_ctl + GPUEB_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_block:
+ atomic_set(&ch->rx_status, GPUEB_MBOX_BLOCKED);
+
+ 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, GPUEB_MBOX_BLOCKED);
+
+ 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 + GPUEB_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 int mtk_gpueb_mbox_probe(struct platform_device *pdev)
+{
+ struct mtk_gpueb_mbox_chan *ch;
+ struct mtk_gpueb_mbox *ebm;
+ 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");
+
+ 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++) {
+ ch = &ebm->ch[i];
+ ch->c = &ebm->v->channels[i];
+ if (ch->c->rx_len > GPUEB_MBOX_MAX_RX_SIZE) {
+ dev_err(ebm->dev, "Channel %s RX size (%d) too large\n",
+ ch->c->name, ch->c->rx_len);
+ return -EINVAL;
+ }
+ ch->full_name = devm_kasprintf(ebm->dev, GFP_KERNEL, "%s:%s",
+ dev_name(ebm->dev), ch->c->name);
+ if (!ch->full_name)
+ return -ENOMEM;
+
+ ch->ebm = ebm;
+ ch->num = i;
+ spin_lock_init(&ebm->mbox.chans[i].lock);
+ ebm->mbox.chans[i].con_priv = ch;
+ atomic_set(&ch->rx_status, GPUEB_MBOX_BLOCKED);
+ }
+
+ 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.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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 5/8] mailbox: add MediaTek GPUEB IPI mailbox
2025-09-23 11:39 ` [PATCH v4 5/8] mailbox: add MediaTek GPUEB IPI mailbox Nicolas Frattaroli
@ 2025-09-24 11:35 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 18+ messages in thread
From: AngeloGioacchino Del Regno @ 2025-09-24 11:35 UTC (permalink / raw)
To: Nicolas Frattaroli, Boris Brezillon, Jassi Brar, Chia-I Wu,
Chen-Yu Tsai, Steven Price, Liviu Dudau, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
Kees Cook, Gustavo A. R. Silva, Ulf Hansson
Cc: kernel, dri-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-hardening, linux-pm
Il 23/09/25 13:39, Nicolas Frattaroli ha scritto:
> 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>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 6/8] drm/panthor: call into devfreq for current frequency
2025-09-23 11:39 [PATCH v4 0/8] MT8196 GPU Frequency/Power Control Support Nicolas Frattaroli
` (4 preceding siblings ...)
2025-09-23 11:39 ` [PATCH v4 5/8] mailbox: add MediaTek GPUEB IPI mailbox Nicolas Frattaroli
@ 2025-09-23 11:39 ` Nicolas Frattaroli
2025-09-23 11:40 ` [PATCH v4 7/8] drm/panthor: Use existing OPP table if present Nicolas Frattaroli
2025-09-23 11:40 ` [PATCH v4 8/8] pmdomain: mediatek: Add support for MFlexGraphics Nicolas Frattaroli
7 siblings, 0 replies; 18+ messages in thread
From: Nicolas Frattaroli @ 2025-09-23 11:39 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Boris Brezillon, Jassi Brar,
Chia-I Wu, Chen-Yu Tsai, Steven Price, Liviu Dudau,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, Kees Cook, Gustavo A. R. Silva, Ulf Hansson
Cc: kernel, dri-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-hardening, linux-pm, 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>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.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 9f0649ecfc4fc697a21a8b2fc4dd89c8ecf298df..f32c1868bf6d782d99df9dbd0babcea049c917e0 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -214,9 +214,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 ea4a37b566a8b215f2b7a09c333a696f1dcdb58f..4d59d94c353c3ca76f4b98a411c8f8284efafd08 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
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v4 7/8] drm/panthor: Use existing OPP table if present
2025-09-23 11:39 [PATCH v4 0/8] MT8196 GPU Frequency/Power Control Support Nicolas Frattaroli
` (5 preceding siblings ...)
2025-09-23 11:39 ` [PATCH v4 6/8] drm/panthor: call into devfreq for current frequency Nicolas Frattaroli
@ 2025-09-23 11:40 ` Nicolas Frattaroli
2025-09-23 11:40 ` [PATCH v4 8/8] pmdomain: mediatek: Add support for MFlexGraphics Nicolas Frattaroli
7 siblings, 0 replies; 18+ messages in thread
From: Nicolas Frattaroli @ 2025-09-23 11:40 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Boris Brezillon, Jassi Brar,
Chia-I Wu, Chen-Yu Tsai, Steven Price, Liviu Dudau,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, Kees Cook, Gustavo A. R. Silva, Ulf Hansson
Cc: kernel, dri-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-hardening, linux-pm, Nicolas Frattaroli
On SoCs where the GPU's power-domain is in charge of setting performance
levels, the OPP table of the GPU node will have already been populated
during said power-domain's attach_dev operation.
To avoid initialising an OPP table twice, only set the OPP regulator and
the OPPs from DT if there's no OPP table present.
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
drivers/gpu/drm/panthor/panthor_devfreq.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
index 8903f60c0a3f06313ac2008791c210ff32b6bd52..4ec46a67db7d4331ac31a249e41ee19378cd411e 100644
--- a/drivers/gpu/drm/panthor/panthor_devfreq.c
+++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
@@ -143,6 +143,7 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
struct panthor_devfreq *pdevfreq;
struct dev_pm_opp *opp;
unsigned long cur_freq;
+ struct opp_table *t;
unsigned long freq = ULONG_MAX;
int ret;
@@ -152,17 +153,22 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
ptdev->devfreq = pdevfreq;
- ret = devm_pm_opp_set_regulators(dev, reg_names);
- if (ret) {
- if (ret != -EPROBE_DEFER)
- DRM_DEV_ERROR(dev, "Couldn't set OPP regulators\n");
+ t = dev_pm_opp_get_opp_table(dev);
+ if (IS_ERR_OR_NULL(t)) {
+ ret = devm_pm_opp_set_regulators(dev, reg_names);
+ if (ret) {
+ if (ret != -EPROBE_DEFER)
+ DRM_DEV_ERROR(dev, "Couldn't set OPP regulators\n");
- return ret;
- }
+ return ret;
+ }
- ret = devm_pm_opp_of_add_table(dev);
- if (ret)
- return ret;
+ ret = devm_pm_opp_of_add_table(dev);
+ if (ret)
+ return ret;
+ } else {
+ dev_pm_opp_put_opp_table(t);
+ }
spin_lock_init(&pdevfreq->lock);
--
2.51.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v4 8/8] pmdomain: mediatek: Add support for MFlexGraphics
2025-09-23 11:39 [PATCH v4 0/8] MT8196 GPU Frequency/Power Control Support Nicolas Frattaroli
` (6 preceding siblings ...)
2025-09-23 11:40 ` [PATCH v4 7/8] drm/panthor: Use existing OPP table if present Nicolas Frattaroli
@ 2025-09-23 11:40 ` Nicolas Frattaroli
2025-09-23 14:24 ` Ulf Hansson
2025-09-23 16:25 ` AngeloGioacchino Del Regno
7 siblings, 2 replies; 18+ messages in thread
From: Nicolas Frattaroli @ 2025-09-23 11:40 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Boris Brezillon, Jassi Brar,
Chia-I Wu, Chen-Yu Tsai, Steven Price, Liviu Dudau,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, Kees Cook, Gustavo A. R. Silva, Ulf Hansson
Cc: kernel, dri-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-hardening, linux-pm, Nicolas Frattaroli
Various MediaTek SoCs use GPU integration silicon named "MFlexGraphics"
by MediaTek. On the MT8196 and MT6991 SoCs, interacting with this
integration silicon is required to power on the GPU.
This glue silicon is in the form of an embedded microcontroller running
special-purpose firmware, which autonomously adjusts clocks and
regulators.
Implement a driver, modelled as a pmdomain driver with a
set_performance_state operation, to support these SoCs.
The driver also exposes the actual achieved clock rate, as read back
from the MCU, as common clock framework clocks, by acting as a clock
provider as well.
Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
---
drivers/pmdomain/mediatek/Kconfig | 16 +
drivers/pmdomain/mediatek/Makefile | 1 +
drivers/pmdomain/mediatek/mtk-mfg-pmdomain.c | 928 +++++++++++++++++++++++++++
3 files changed, 945 insertions(+)
diff --git a/drivers/pmdomain/mediatek/Kconfig b/drivers/pmdomain/mediatek/Kconfig
index 0e34a517ab7d5a867bebaab11c0d866282a15e45..2abf78c85d017b1e3526b41c81f274f78d581fd0 100644
--- a/drivers/pmdomain/mediatek/Kconfig
+++ b/drivers/pmdomain/mediatek/Kconfig
@@ -26,6 +26,22 @@ config MTK_SCPSYS_PM_DOMAINS
Control Processor System (SCPSYS) has several power management related
tasks in the system.
+config MTK_MFG_PM_DOMAIN
+ tristate "MediaTek MFlexGraphics power domain"
+ default ARCH_MEDIATEK
+ depends on PM
+ depends on OF
+ depends on COMMON_CLK
+ select PM_GENERIC_DOMAINS
+ imply MTK_GPUEB_MBOX
+ help
+ Say y or m here to enable the power domains driver for MediaTek
+ MFlexGraphics. This driver allows for power and frequency control of
+ GPUs on MediaTek SoCs such as the MT8196 or MT6991.
+
+ This driver is required for the Mali GPU to work at all on MT8196 and
+ MT6991.
+
config AIROHA_CPU_PM_DOMAIN
tristate "Airoha CPU power domain"
default ARCH_AIROHA
diff --git a/drivers/pmdomain/mediatek/Makefile b/drivers/pmdomain/mediatek/Makefile
index 18ba92e3c418154e1d428dbc6b59b97b26056d98..b424f1ed867604393b3ff96364855363aedaa40c 100644
--- a/drivers/pmdomain/mediatek/Makefile
+++ b/drivers/pmdomain/mediatek/Makefile
@@ -1,4 +1,5 @@
# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_MTK_MFG_PM_DOMAIN) += mtk-mfg-pmdomain.o
obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
obj-$(CONFIG_MTK_SCPSYS_PM_DOMAINS) += mtk-pm-domains.o
obj-$(CONFIG_AIROHA_CPU_PM_DOMAIN) += airoha-cpu-pmdomain.o
diff --git a/drivers/pmdomain/mediatek/mtk-mfg-pmdomain.c b/drivers/pmdomain/mediatek/mtk-mfg-pmdomain.c
new file mode 100644
index 0000000000000000000000000000000000000000..3148796a6b8aea9958c424f695efb7d8af23b7ce
--- /dev/null
+++ b/drivers/pmdomain/mediatek/mtk-mfg-pmdomain.c
@@ -0,0 +1,928 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for MediaTek MFlexGraphics Devices
+ *
+ * Copyright (C) 2025, Collabora Ltd.
+ */
+
+#include <linux/completion.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/container_of.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/overflow.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_opp.h>
+#include <linux/regulator/consumer.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_SRAM_MAGIC 0xBABADADAUL
+
+#define GPUEB_TIMEOUT_US 10000UL
+#define GPUEB_POLL_US 50
+
+#define MAX_OPP_NUM 70
+
+#define GPUEB_MBOX_MAX_RX_SIZE 32 /* in bytes */
+
+/*
+ * 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 reserved;
+ 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 generic_pm_domain pd;
+ struct platform_device *pdev;
+ struct clk *clk_eb;
+ struct clk_bulk_data *gpu_clks;
+ struct clk_hw clk_core_hw;
+ struct clk_hw clk_stack_hw;
+ 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 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;
+ /** @turbo_below: opp indices below this value are considered turbo */
+ unsigned int turbo_below;
+ int (*init)(struct mtk_mfg *mfg);
+};
+
+static inline struct mtk_mfg *mtk_mfg_from_genpd(struct generic_pm_domain *pd)
+{
+ return container_of(pd, struct mtk_mfg, pd);
+}
+
+static inline void mtk_mfg_update_reg_bits(void __iomem *addr, u32 mask, u32 val)
+{
+ writel((readl(addr) & ~mask) | (val & mask), addr);
+}
+
+static unsigned long mtk_mfg_recalc_rate_gpu(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct mtk_mfg *mfg = container_of(hw, struct mtk_mfg, clk_core_hw);
+
+ return readl(mfg->sram + GF_REG_FREQ_OUT_GPU) * 1000UL;
+}
+
+static unsigned long mtk_mfg_recalc_rate_stack(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct mtk_mfg *mfg = container_of(hw, struct mtk_mfg, clk_stack_hw);
+
+ return readl(mfg->sram + GF_REG_FREQ_OUT_STK) * 1000UL;
+}
+
+static const struct clk_ops mtk_mfg_clk_gpu_ops = {
+ .recalc_rate = mtk_mfg_recalc_rate_gpu,
+};
+
+static const struct clk_ops mtk_mfg_clk_stack_ops = {
+ .recalc_rate = mtk_mfg_recalc_rate_stack,
+};
+
+static const struct clk_init_data mtk_mfg_clk_gpu_init = {
+ .name = "gpu-core",
+ .ops = &mtk_mfg_clk_gpu_ops,
+ .flags = CLK_GET_RATE_NOCACHE,
+};
+
+static const struct clk_init_data mtk_mfg_clk_stack_init = {
+ .name = "gpu-stack",
+ .ops = &mtk_mfg_clk_stack_ops,
+ .flags = CLK_GET_RATE_NOCACHE,
+};
+
+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 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(dev, "Cannot send sleep command: %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(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)
+{
+ struct device *dev = &mfg->pdev->dev;
+ unsigned long wait;
+ int ret;
+
+ msg->magic = mfg->ipi_magic;
+
+ ret = mbox_send_message(mfg->gf_mbox->ch, msg);
+ if (ret < 0) {
+ dev_err(dev, "Cannot send GPUFreq IPI command: %pe\n", ERR_PTR(ret));
+ return ret;
+ }
+
+ wait = wait_for_completion_timeout(&mfg->gf_mbox->rx_done, msecs_to_jiffies(500));
+ if (!wait)
+ return -ETIMEDOUT;
+
+ msg = mfg->gf_mbox->rx_data;
+
+ if (msg->u.return_value < 0) {
+ dev_err(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_dbg(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) != GPUEB_SRAM_MAGIC) {
+ dev_err(dev, "EB did not initialise SRAM correctly\n");
+ return -EIO;
+ }
+
+ 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, unsigned int opp_idx)
+{
+ struct mtk_mfg_ipi_msg msg = {};
+ int ret;
+
+ 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) {
+ dev_err(&mfg->pdev->dev, "Failed to set OPP %u: %pe\n",
+ opp_idx, ERR_PTR(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 > 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;
+
+ 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;
+ mfg->gpu_opps[i].level = i;
+ if (i < mfg->variant->turbo_below)
+ mfg->gpu_opps[i].turbo = true;
+
+ if (!last_freq || mfg->gpu_opps[i].freq != last_freq)
+ mfg->num_unique_gpu_opps++;
+
+ last_freq = mfg->gpu_opps[i].freq;
+ }
+
+ return 0;
+}
+
+static const char *const mtk_mfg_mt8196_clk_names[] = {
+ "core",
+ "stack0",
+ "stack1",
+};
+
+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),
+ .turbo_below = 7,
+ .init = mtk_mfg_mt8196_init,
+};
+
+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);
+
+ if (mb->rx_data)
+ mb->rx_data = memcpy(mb->rx_data, mssg, GPUEB_MBOX_MAX_RX_SIZE);
+ complete(&mb->rx_done);
+}
+
+static int mtk_mfg_attach_dev(struct generic_pm_domain *pd, struct device *dev)
+{
+ struct mtk_mfg *mfg = mtk_mfg_from_genpd(pd);
+ struct dev_pm_opp_data *opps = mfg->gpu_opps;
+ int i, ret;
+
+ for (i = mfg->num_opps - 1; i >= 0; i--) {
+ if ((i == mfg->num_opps - 1) || (opps[i].freq != opps[i + 1].freq)) {
+ ret = dev_pm_opp_add_dynamic(dev, &opps[i]);
+ if (ret) {
+ dev_err(dev, "Failed to add OPP level %u from PD %s\n",
+ opps[i].level, pd->name);
+ dev_pm_opp_remove_all_dynamic(dev);
+ return ret;
+ }
+ }
+ }
+
+ return 0;
+}
+
+static void mtk_mfg_detach_dev(struct generic_pm_domain *pd, struct device *dev)
+{
+ dev_pm_opp_remove_all_dynamic(dev);
+}
+
+static int mtk_mfg_set_performance(struct generic_pm_domain *pd,
+ unsigned int state)
+{
+ struct mtk_mfg *mfg = mtk_mfg_from_genpd(pd);
+
+ /*
+ * Occasionally, we're asked to set OPPs when we're off. This will fail,
+ * so don't do it at all. We do foo != GENPD_STATE_ON instead of !foo
+ * as to not depend on the actual value of the enum.
+ */
+ if (mfg->pd.status != GENPD_STATE_ON)
+ return 0;
+
+ return mtk_mfg_set_oppidx(mfg, state);
+}
+
+static int mtk_mfg_power_on(struct generic_pm_domain *pd)
+{
+ struct mtk_mfg *mfg = mtk_mfg_from_genpd(pd);
+ 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 int mtk_mfg_power_off(struct generic_pm_domain *pd)
+{
+ struct mtk_mfg *mfg = mtk_mfg_from_genpd(pd);
+ struct device *dev = &mfg->pdev->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;
+ }
+ mfg->last_opp = -1;
+ 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_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->rx_data = devm_kzalloc(dev, GPUEB_MBOX_MAX_RX_SIZE, GFP_KERNEL);
+ if (!gf->rx_data)
+ return -ENOMEM;
+ 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 int mtk_mfg_init_clk_provider(struct mtk_mfg *mfg)
+{
+ struct device *dev = &mfg->pdev->dev;
+ struct clk_hw_onecell_data *clk_data;
+ int ret;
+
+ clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, 2), GFP_KERNEL);
+ if (!clk_data)
+ return -ENOMEM;
+
+ clk_data->num = 2;
+
+ mfg->clk_core_hw.init = &mtk_mfg_clk_gpu_init;
+ mfg->clk_stack_hw.init = &mtk_mfg_clk_stack_init;
+
+ ret = devm_clk_hw_register(dev, &mfg->clk_core_hw);
+ if (ret)
+ return dev_err_probe(dev, ret, "Couldn't register GPU core clock\n");
+
+ ret = devm_clk_hw_register(dev, &mfg->clk_stack_hw);
+ if (ret)
+ return dev_err_probe(dev, ret, "Couldn't register GPU stack clock\n");
+
+ clk_data->hws[0] = &mfg->clk_core_hw;
+ clk_data->hws[1] = &mfg->clk_stack_hw;
+
+ ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
+ if (ret)
+ return dev_err_probe(dev, ret, "Couldn't register clock provider\n");
+
+ 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 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, i;
+
+ mfg = devm_kzalloc(dev, sizeof(*mfg), GFP_KERNEL);
+ if (!mfg)
+ return -ENOMEM;
+
+ 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");
+ }
+
+ mfg->pd.name = dev_name(dev);
+ mfg->pd.attach_dev = mtk_mfg_attach_dev;
+ mfg->pd.detach_dev = mtk_mfg_detach_dev;
+ mfg->pd.power_off = mtk_mfg_power_off;
+ mfg->pd.power_on = mtk_mfg_power_on;
+ mfg->pd.set_performance_state = mtk_mfg_set_performance;
+ mfg->pd.flags = GENPD_FLAG_OPP_TABLE_FW;
+ pm_genpd_init(&mfg->pd, NULL, false);
+
+ 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);
+ /* Downstream does this, don't know why. */
+ writel(0x0, mfg->gpr + GPR_IPI_MAGIC);
+
+ ret = mtk_mfg_init_mbox(mfg);
+ if (ret) {
+ ret = dev_err_probe(dev, ret, "Couldn't initialise mailbox\n");
+ goto out;
+ }
+
+ mfg->last_opp = -1;
+
+ ret = mtk_mfg_power_on(&mfg->pd);
+ clk_disable_unprepare(mfg->clk_eb);
+ 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;
+ }
+
+ ret = mtk_mfg_init_clk_provider(mfg);
+ if (ret)
+ goto out;
+
+ ret = of_genpd_add_provider_simple(pdev->dev.of_node, &mfg->pd);
+ if (ret) {
+ ret = dev_err_probe(dev, ret, "Failed to add pmdomain provider\n");
+ goto out;
+ }
+
+ return 0;
+
+out:
+ mtk_mfg_power_off(&mfg->pd);
+ return ret;
+}
+
+static struct platform_driver mtk_mfg_driver = {
+ .driver = {
+ .name = "mtk-mfg-pmdomain",
+ .of_match_table = mtk_mfg_of_match,
+ },
+ .probe = mtk_mfg_probe,
+};
+module_platform_driver(mtk_mfg_driver);
+
+MODULE_AUTHOR("Nicolas Frattaroli <nicolas.frattaroli@collabora.com>");
+MODULE_DESCRIPTION("MediaTek MFlexGraphics Power Domain Driver");
+MODULE_LICENSE("GPL");
--
2.51.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 8/8] pmdomain: mediatek: Add support for MFlexGraphics
2025-09-23 11:40 ` [PATCH v4 8/8] pmdomain: mediatek: Add support for MFlexGraphics Nicolas Frattaroli
@ 2025-09-23 14:24 ` Ulf Hansson
2025-09-25 14:04 ` Nicolas Frattaroli
2025-09-23 16:25 ` AngeloGioacchino Del Regno
1 sibling, 1 reply; 18+ messages in thread
From: Ulf Hansson @ 2025-09-23 14:24 UTC (permalink / raw)
To: Nicolas Frattaroli
Cc: AngeloGioacchino Del Regno, Boris Brezillon, Jassi Brar,
Chia-I Wu, Chen-Yu Tsai, Steven Price, Liviu Dudau,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, Kees Cook, Gustavo A. R. Silva, kernel,
dri-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-hardening, linux-pm
On Tue, 23 Sept 2025 at 13:41, Nicolas Frattaroli
<nicolas.frattaroli@collabora.com> wrote:
>
> Various MediaTek SoCs use GPU integration silicon named "MFlexGraphics"
> by MediaTek. On the MT8196 and MT6991 SoCs, interacting with this
> integration silicon is required to power on the GPU.
>
> This glue silicon is in the form of an embedded microcontroller running
> special-purpose firmware, which autonomously adjusts clocks and
> regulators.
>
> Implement a driver, modelled as a pmdomain driver with a
> set_performance_state operation, to support these SoCs.
>
> The driver also exposes the actual achieved clock rate, as read back
> from the MCU, as common clock framework clocks, by acting as a clock
> provider as well.
>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
> drivers/pmdomain/mediatek/Kconfig | 16 +
> drivers/pmdomain/mediatek/Makefile | 1 +
> drivers/pmdomain/mediatek/mtk-mfg-pmdomain.c | 928 +++++++++++++++++++++++++++
> 3 files changed, 945 insertions(+)
[...]
> +
> +static int mtk_mfg_set_performance(struct generic_pm_domain *pd,
> + unsigned int state)
> +{
> + struct mtk_mfg *mfg = mtk_mfg_from_genpd(pd);
> +
> + /*
> + * Occasionally, we're asked to set OPPs when we're off. This will fail,
> + * so don't do it at all. We do foo != GENPD_STATE_ON instead of !foo
> + * as to not depend on the actual value of the enum.
> + */
> + if (mfg->pd.status != GENPD_STATE_ON)
> + return 0;
Returning 0 here, means that we may end up never restoring the
performance state for a device and its genpd, when the device is
getting runtime resumed. In genpd_runtime_resume() we are calling
genpd_restore_performance_state() before calling genpd_power_on().
This is deliberate, see commit ae8ac19655e0.
That said, I think we need to manage the restore in the ->power_on()
callback. In principle, it means we should call
mtk_mfg_set_oppidx(mfg, genpd->performance_state) from there.
> +
> + return mtk_mfg_set_oppidx(mfg, state);
> +}
> +
> +static int mtk_mfg_power_on(struct generic_pm_domain *pd)
> +{
> + struct mtk_mfg *mfg = mtk_mfg_from_genpd(pd);
> + 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;
> +}
[...]
Note, I intend to have a bit closer look at this soon, but I just
observed the issue I pointed out above from my first quick look.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 8/8] pmdomain: mediatek: Add support for MFlexGraphics
2025-09-23 14:24 ` Ulf Hansson
@ 2025-09-25 14:04 ` Nicolas Frattaroli
2025-09-29 9:58 ` Ulf Hansson
0 siblings, 1 reply; 18+ messages in thread
From: Nicolas Frattaroli @ 2025-09-25 14:04 UTC (permalink / raw)
To: Ulf Hansson
Cc: AngeloGioacchino Del Regno, Boris Brezillon, Jassi Brar,
Chia-I Wu, Chen-Yu Tsai, Steven Price, Liviu Dudau,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, Kees Cook, Gustavo A. R. Silva, kernel,
dri-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-hardening, linux-pm
On Tuesday, 23 September 2025 16:24:13 Central European Summer Time Ulf Hansson wrote:
> On Tue, 23 Sept 2025 at 13:41, Nicolas Frattaroli
> <nicolas.frattaroli@collabora.com> wrote:
> >
> > Various MediaTek SoCs use GPU integration silicon named "MFlexGraphics"
> > by MediaTek. On the MT8196 and MT6991 SoCs, interacting with this
> > integration silicon is required to power on the GPU.
> >
> > This glue silicon is in the form of an embedded microcontroller running
> > special-purpose firmware, which autonomously adjusts clocks and
> > regulators.
> >
> > Implement a driver, modelled as a pmdomain driver with a
> > set_performance_state operation, to support these SoCs.
> >
> > The driver also exposes the actual achieved clock rate, as read back
> > from the MCU, as common clock framework clocks, by acting as a clock
> > provider as well.
> >
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> > drivers/pmdomain/mediatek/Kconfig | 16 +
> > drivers/pmdomain/mediatek/Makefile | 1 +
> > drivers/pmdomain/mediatek/mtk-mfg-pmdomain.c | 928 +++++++++++++++++++++++++++
> > 3 files changed, 945 insertions(+)
>
> [...]
>
> > +
> > +static int mtk_mfg_set_performance(struct generic_pm_domain *pd,
> > + unsigned int state)
> > +{
> > + struct mtk_mfg *mfg = mtk_mfg_from_genpd(pd);
> > +
> > + /*
> > + * Occasionally, we're asked to set OPPs when we're off. This will fail,
> > + * so don't do it at all. We do foo != GENPD_STATE_ON instead of !foo
> > + * as to not depend on the actual value of the enum.
> > + */
> > + if (mfg->pd.status != GENPD_STATE_ON)
> > + return 0;
>
> Returning 0 here, means that we may end up never restoring the
> performance state for a device and its genpd, when the device is
> getting runtime resumed. In genpd_runtime_resume() we are calling
> genpd_restore_performance_state() before calling genpd_power_on().
> This is deliberate, see commit ae8ac19655e0.
>
> That said, I think we need to manage the restore in the ->power_on()
> callback. In principle, it means we should call
> mtk_mfg_set_oppidx(mfg, genpd->performance_state) from there.
>
Thanks for pointing this out, yeah setting the oppidx in power_on
does look like the right choice.
> > +
> > + return mtk_mfg_set_oppidx(mfg, state);
> > +}
> > +
> > +static int mtk_mfg_power_on(struct generic_pm_domain *pd)
> > +{
> > + struct mtk_mfg *mfg = mtk_mfg_from_genpd(pd);
> > + 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;
> > +}
>
> [...]
>
> Note, I intend to have a bit closer look at this soon, but I just
> observed the issue I pointed out above from my first quick look.
I actually have a question about a mystery that has been puzzling
me as I work on v5: when unloading the driver that uses the PD
(panthor) and my driver using `modprobe -r panthor mtk_mfg_pmdomain`,
I see that sometimes detach_dev is called with the pd status
reportedly being off, but according to my own power-on/power-off
counting I hacked in, it actually being on. It then proceeds to
call my remove, which results in three splats from the regulator
subsystem as the regulators weren't balanced with disables before
they were freed, which isn't the main problem but more a symptom
of the bigger problem that power_off and power_on calls don't
appear to be balanced by the pmdomain subsystem when a driver is
removed under certain circumstances.
Did I run into a core pmdomain bug here, or do I have to balance
the power_on/off myself somehow in detach_dev? If the latter, I'm
struggling to figure out how I can determine that the PD is still
on without doing my own bookkeeping, as pmdomain core seems to clear
these fields before my detach_dev gets to them :(
>
> Kind regards
> Uffe
>
Kind regards,
Nicolas Frattaroli
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 8/8] pmdomain: mediatek: Add support for MFlexGraphics
2025-09-25 14:04 ` Nicolas Frattaroli
@ 2025-09-29 9:58 ` Ulf Hansson
0 siblings, 0 replies; 18+ messages in thread
From: Ulf Hansson @ 2025-09-29 9:58 UTC (permalink / raw)
To: Nicolas Frattaroli
Cc: AngeloGioacchino Del Regno, Boris Brezillon, Jassi Brar,
Chia-I Wu, Chen-Yu Tsai, Steven Price, Liviu Dudau,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, Kees Cook, Gustavo A. R. Silva, kernel,
dri-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-hardening, linux-pm
[...]
>
> I actually have a question about a mystery that has been puzzling
> me as I work on v5: when unloading the driver that uses the PD
> (panthor) and my driver using `modprobe -r panthor mtk_mfg_pmdomain`,
> I see that sometimes detach_dev is called with the pd status
> reportedly being off, but according to my own power-on/power-off
> counting I hacked in, it actually being on. It then proceeds to
Yes, ->detach_dev() may be called for a device, when its corresponding
PM domain is both on or off.
> call my remove, which results in three splats from the regulator
> subsystem as the regulators weren't balanced with disables before
> they were freed, which isn't the main problem but more a symptom
> of the bigger problem that power_off and power_on calls don't
> appear to be balanced by the pmdomain subsystem when a driver is
> removed under certain circumstances.
If the callbacks aren't called in a properly balanced manner that
would be a bug in genpd. Certainly.
>
> Did I run into a core pmdomain bug here, or do I have to balance
> the power_on/off myself somehow in detach_dev? If the latter, I'm
> struggling to figure out how I can determine that the PD is still
> on without doing my own bookkeeping, as pmdomain core seems to clear
> these fields before my detach_dev gets to them :(
When genpd_dev_pm_detach() is called, the device is being detached
from its genpd and part of that procedure means calling the
->detach_dev() callback.
When the device has been detached, we may have the PM domain being
powered-on for no good reason. That's why we also punt a work to check
if we can turn it off (see the call to genpd_queue_power_off_work()).
Kind regards
Uffe
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 8/8] pmdomain: mediatek: Add support for MFlexGraphics
2025-09-23 11:40 ` [PATCH v4 8/8] pmdomain: mediatek: Add support for MFlexGraphics Nicolas Frattaroli
2025-09-23 14:24 ` Ulf Hansson
@ 2025-09-23 16:25 ` AngeloGioacchino Del Regno
2025-09-24 19:11 ` Nicolas Frattaroli
1 sibling, 1 reply; 18+ messages in thread
From: AngeloGioacchino Del Regno @ 2025-09-23 16:25 UTC (permalink / raw)
To: Nicolas Frattaroli, Boris Brezillon, Jassi Brar, Chia-I Wu,
Chen-Yu Tsai, Steven Price, Liviu Dudau, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
Kees Cook, Gustavo A. R. Silva, Ulf Hansson
Cc: kernel, dri-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-hardening, linux-pm
Il 23/09/25 13:40, Nicolas Frattaroli ha scritto:
> Various MediaTek SoCs use GPU integration silicon named "MFlexGraphics"
> by MediaTek. On the MT8196 and MT6991 SoCs, interacting with this
> integration silicon is required to power on the GPU.
>
> This glue silicon is in the form of an embedded microcontroller running
> special-purpose firmware, which autonomously adjusts clocks and
> regulators.
>
> Implement a driver, modelled as a pmdomain driver with a
> set_performance_state operation, to support these SoCs.
>
> The driver also exposes the actual achieved clock rate, as read back
> from the MCU, as common clock framework clocks, by acting as a clock
> provider as well.
>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
> drivers/pmdomain/mediatek/Kconfig | 16 +
> drivers/pmdomain/mediatek/Makefile | 1 +
> drivers/pmdomain/mediatek/mtk-mfg-pmdomain.c | 928 +++++++++++++++++++++++++++
> 3 files changed, 945 insertions(+)
>
> diff --git a/drivers/pmdomain/mediatek/Kconfig b/drivers/pmdomain/mediatek/Kconfig
> index 0e34a517ab7d5a867bebaab11c0d866282a15e45..2abf78c85d017b1e3526b41c81f274f78d581fd0 100644
> --- a/drivers/pmdomain/mediatek/Kconfig
> +++ b/drivers/pmdomain/mediatek/Kconfig
> @@ -26,6 +26,22 @@ config MTK_SCPSYS_PM_DOMAINS
> Control Processor System (SCPSYS) has several power management related
> tasks in the system.
>
> +config MTK_MFG_PM_DOMAIN
> + tristate "MediaTek MFlexGraphics power domain"
> + default ARCH_MEDIATEK
> + depends on PM
> + depends on OF
> + depends on COMMON_CLK
> + select PM_GENERIC_DOMAINS
> + imply MTK_GPUEB_MBOX
> + help
> + Say y or m here to enable the power domains driver for MediaTek
> + MFlexGraphics. This driver allows for power and frequency control of
> + GPUs on MediaTek SoCs such as the MT8196 or MT6991.
> +
> + This driver is required for the Mali GPU to work at all on MT8196 and
> + MT6991.
> +
> config AIROHA_CPU_PM_DOMAIN
> tristate "Airoha CPU power domain"
> default ARCH_AIROHA
> diff --git a/drivers/pmdomain/mediatek/Makefile b/drivers/pmdomain/mediatek/Makefile
> index 18ba92e3c418154e1d428dbc6b59b97b26056d98..b424f1ed867604393b3ff96364855363aedaa40c 100644
> --- a/drivers/pmdomain/mediatek/Makefile
> +++ b/drivers/pmdomain/mediatek/Makefile
> @@ -1,4 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_MTK_MFG_PM_DOMAIN) += mtk-mfg-pmdomain.o
> obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> obj-$(CONFIG_MTK_SCPSYS_PM_DOMAINS) += mtk-pm-domains.o
> obj-$(CONFIG_AIROHA_CPU_PM_DOMAIN) += airoha-cpu-pmdomain.o
> diff --git a/drivers/pmdomain/mediatek/mtk-mfg-pmdomain.c b/drivers/pmdomain/mediatek/mtk-mfg-pmdomain.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..3148796a6b8aea9958c424f695efb7d8af23b7ce
> --- /dev/null
> +++ b/drivers/pmdomain/mediatek/mtk-mfg-pmdomain.c
> @@ -0,0 +1,928 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for MediaTek MFlexGraphics Devices
> + *
> + * Copyright (C) 2025, Collabora Ltd.
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/container_of.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/overflow.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_opp.h>
> +#include <linux/regulator/consumer.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_SRAM_MAGIC 0xBABADADAUL
> +
> +#define GPUEB_TIMEOUT_US 10000UL
> +#define GPUEB_POLL_US 50
> +
> +#define MAX_OPP_NUM 70
> +
> +#define GPUEB_MBOX_MAX_RX_SIZE 32 /* in bytes */
> +
> +/*
> + * 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,
I don't really like seeing index assignments to enumeration, especially when there
are no holes... and you have also clearly written that this is ABI-do-not-touch so
I'm not sure that having those numbers here is improving anything.
I also haven't got strong opinions about that, anyway.
> +};
> +
> +/*
> + * 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 reserved;
> + 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 generic_pm_domain pd;
> + struct platform_device *pdev;
> + struct clk *clk_eb;
> + struct clk_bulk_data *gpu_clks;
> + struct clk_hw clk_core_hw;
> + struct clk_hw clk_stack_hw;
> + 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;
u16 ghpm_en_reg ?
> + u32 ipi_magic;
> + unsigned int num_opps;
> + unsigned int num_unique_gpu_opps;
(num_opps, num_unique_gpu_opps) -> unsigned short?
> + struct dev_pm_opp_data *gpu_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;
> + /** @turbo_below: opp indices below this value are considered turbo */
> + unsigned int turbo_below;
> + int (*init)(struct mtk_mfg *mfg);
> +};
> +
> +static inline struct mtk_mfg *mtk_mfg_from_genpd(struct generic_pm_domain *pd)
> +{
> + return container_of(pd, struct mtk_mfg, pd);
> +}
> +
> +static inline void mtk_mfg_update_reg_bits(void __iomem *addr, u32 mask, u32 val)
> +{
> + writel((readl(addr) & ~mask) | (val & mask), addr);
> +}
> +
> +static unsigned long mtk_mfg_recalc_rate_gpu(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct mtk_mfg *mfg = container_of(hw, struct mtk_mfg, clk_core_hw);
> +
> + return readl(mfg->sram + GF_REG_FREQ_OUT_GPU) * 1000UL;
#include <linux/units.h>
return readl(mfg->sram + GF_REG_FREQ_OUT_GPU) * HZ_PER_KHZ;
> +}
> +
> +static unsigned long mtk_mfg_recalc_rate_stack(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct mtk_mfg *mfg = container_of(hw, struct mtk_mfg, clk_stack_hw);
> +
> + return readl(mfg->sram + GF_REG_FREQ_OUT_STK) * 1000UL;
same
> +}
> +
> +static const struct clk_ops mtk_mfg_clk_gpu_ops = {
> + .recalc_rate = mtk_mfg_recalc_rate_gpu,
> +};
> +
> +static const struct clk_ops mtk_mfg_clk_stack_ops = {
> + .recalc_rate = mtk_mfg_recalc_rate_stack,
> +};
> +
> +static const struct clk_init_data mtk_mfg_clk_gpu_init = {
> + .name = "gpu-core",
> + .ops = &mtk_mfg_clk_gpu_ops,
> + .flags = CLK_GET_RATE_NOCACHE,
> +};
> +
> +static const struct clk_init_data mtk_mfg_clk_stack_init = {
> + .name = "gpu-stack",
> + .ops = &mtk_mfg_clk_stack_ops,
> + .flags = CLK_GET_RATE_NOCACHE,
> +};
> +
> +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 device *dev = &mfg->pdev->dev;
> + struct mtk_mfg_ipi_sleep_msg msg = {
Can this be constified?
> + .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(dev, "Cannot send sleep command: %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(dev, "timed out waiting for EB to power off, val=0x%08X\n",
> + val);
90 columns is fine, one line please.
> +
> + return ret;
> +}
> +
> +static int mtk_mfg_send_ipi(struct mtk_mfg *mfg, struct mtk_mfg_ipi_msg *msg)
> +{
> + struct device *dev = &mfg->pdev->dev;
> + unsigned long wait;
> + int ret;
> +
> + msg->magic = mfg->ipi_magic;
> +
> + ret = mbox_send_message(mfg->gf_mbox->ch, msg);
> + if (ret < 0) {
> + dev_err(dev, "Cannot send GPUFreq IPI command: %pe\n", ERR_PTR(ret));
> + return ret;
> + }
> +
> + wait = wait_for_completion_timeout(&mfg->gf_mbox->rx_done, msecs_to_jiffies(500));
> + if (!wait)
> + return -ETIMEDOUT;
> +
> + msg = mfg->gf_mbox->rx_data;
> +
> + if (msg->u.return_value < 0) {
> + dev_err(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_dbg(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) != GPUEB_SRAM_MAGIC) {
> + dev_err(dev, "EB did not initialise SRAM correctly\n");
> + return -EIO;
> + }
> +
> + 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, unsigned int opp_idx)
> +{
> + struct mtk_mfg_ipi_msg msg = {};
> + int ret;
> +
> + 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) {
> + dev_err(&mfg->pdev->dev, "Failed to set OPP %u: %pe\n",
> + opp_idx, ERR_PTR(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 > 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;
> +
> + 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].freq = e.freq_khz * HZ_PER_KHZ;
> + mfg->gpu_opps[i].u_volt = e.voltage_core * 10;
> + mfg->gpu_opps[i].level = i;
> + if (i < mfg->variant->turbo_below)
> + mfg->gpu_opps[i].turbo = true;
> +
> + if (!last_freq || mfg->gpu_opps[i].freq != last_freq)
> + mfg->num_unique_gpu_opps++;
> +
> + last_freq = mfg->gpu_opps[i].freq;
> + }
> +
> + return 0;
> +}
> +
> +static const char *const mtk_mfg_mt8196_clk_names[] = {
> + "core",
> + "stack0",
> + "stack1",
> +};
> +
> +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),
> + .turbo_below = 7,
> + .init = mtk_mfg_mt8196_init,
> +};
> +
> +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);
> +
> + if (mb->rx_data)
> + mb->rx_data = memcpy(mb->rx_data, mssg, GPUEB_MBOX_MAX_RX_SIZE);
> + complete(&mb->rx_done);
> +}
> +
> +static int mtk_mfg_attach_dev(struct generic_pm_domain *pd, struct device *dev)
> +{
> + struct mtk_mfg *mfg = mtk_mfg_from_genpd(pd);
> + struct dev_pm_opp_data *opps = mfg->gpu_opps;
> + int i, ret;
> +
> + for (i = mfg->num_opps - 1; i >= 0; i--) {
> + if ((i == mfg->num_opps - 1) || (opps[i].freq != opps[i + 1].freq)) {
/* Add a comment here, because you're using a trick, and it's not
* very fast to read, as in, if you skim through that, you're most
* probably losing the fact that the first OPP is always added
* regardless of anything.
*/
if ((i != mfg->num_opps - 1) || (opps[i].freq == opps[i + 1].freq))
continue;
/* Reduced indentation :-) */
ret = dev_pm_opp_add_dynamic(.....) etc
> + ret = dev_pm_opp_add_dynamic(dev, &opps[i]);
> + if (ret) {
> + dev_err(dev, "Failed to add OPP level %u from PD %s\n",
> + opps[i].level, pd->name);
> + dev_pm_opp_remove_all_dynamic(dev);
> + return ret;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void mtk_mfg_detach_dev(struct generic_pm_domain *pd, struct device *dev)
> +{
> + dev_pm_opp_remove_all_dynamic(dev);
> +}
> +
> +static int mtk_mfg_set_performance(struct generic_pm_domain *pd,
> + unsigned int state)
> +{
> + struct mtk_mfg *mfg = mtk_mfg_from_genpd(pd);
> +
> + /*
> + * Occasionally, we're asked to set OPPs when we're off. This will fail,
Occasionally, a request to set OPPs may come while the domain is off, and
that is expected to fail. When we fail, we return success, because ... well..
> + * so don't do it at all. We do foo != GENPD_STATE_ON instead of !foo
> + * as to not depend on the actual value of the enum.
> + */
> + if (mfg->pd.status != GENPD_STATE_ON)
> + return 0;
...because.. why?
Feels like there needs to be one more restriction - is there a specific pattern
in this? When the domain is OFF, is the OPPIDX always lower (low freq) compared
to the currently set one?
Think about the case in which the power domain is never enabled because of a bug
somewhere (api or driver) and the set_performance() call is supposed to not fail.
> +
> + return mtk_mfg_set_oppidx(mfg, state);
> +}
> +
> +static int mtk_mfg_power_on(struct generic_pm_domain *pd)
> +{
> + struct mtk_mfg *mfg = mtk_mfg_from_genpd(pd);
> + 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 int mtk_mfg_power_off(struct generic_pm_domain *pd)
> +{
> + struct mtk_mfg *mfg = mtk_mfg_from_genpd(pd);
> + struct device *dev = &mfg->pdev->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;
> + }
> + mfg->last_opp = -1;
> + 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);
if (ret)
return ret;
return 0;
> +
> + return ret;
> +}
> +
> +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->rx_data = devm_kzalloc(dev, GPUEB_MBOX_MAX_RX_SIZE, GFP_KERNEL);
> + if (!gf->rx_data)
> + return -ENOMEM;
> + 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 int mtk_mfg_init_clk_provider(struct mtk_mfg *mfg)
> +{
> + struct device *dev = &mfg->pdev->dev;
> + struct clk_hw_onecell_data *clk_data;
> + int ret;
> +
> + clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, 2), GFP_KERNEL);
> + if (!clk_data)
> + return -ENOMEM;
> +
> + clk_data->num = 2;
> +
> + mfg->clk_core_hw.init = &mtk_mfg_clk_gpu_init;
> + mfg->clk_stack_hw.init = &mtk_mfg_clk_stack_init;
> +
> + ret = devm_clk_hw_register(dev, &mfg->clk_core_hw);
> + if (ret)
> + return dev_err_probe(dev, ret, "Couldn't register GPU core clock\n");
> +
> + ret = devm_clk_hw_register(dev, &mfg->clk_stack_hw);
> + if (ret)
> + return dev_err_probe(dev, ret, "Couldn't register GPU stack clock\n");
> +
> + clk_data->hws[0] = &mfg->clk_core_hw;
> + clk_data->hws[1] = &mfg->clk_stack_hw;
> +
> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
> + if (ret)
> + return dev_err_probe(dev, ret, "Couldn't register clock provider\n");
> +
> + 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);
Please move the of_device_id array after the probe function.
> +
> +static int mtk_mfg_probe(struct platform_device *pdev)
> +{
> + struct device_node *shmem __free(device_node);
> + 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, i;
> +
> + mfg = devm_kzalloc(dev, sizeof(*mfg), GFP_KERNEL);
> + if (!mfg)
> + return -ENOMEM;
> +
> + mfg->pdev = pdev;
> + mfg->variant = data;
> +
> + dev_set_drvdata(dev, mfg);
> +
> + mfg->gpr = devm_platform_ioremap_resource_byname(pdev, "gpr");
Just do it by index please. You can enforce the mmio index with the bindings.
> + 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");
I'm not sure, but a doubt just came to my mind.
See the following coreboot table:
0. 0000000000100000-0000000000106fff: RAMSTAGE
1. 0000000000108000-000000000010bfff: RAMSTAGE
2. 0000000080000000-00000000800fffff: RAM
3. 0000000080100000-0000000080443fff: RAMSTAGE
4. 0000000080444000-00000000804fffff: RAM
5. 0000000080500000-00000000854fffff: RESERVED
6. 0000000085500000-00000000945fffff: RAM
7. 0000000094600000-00000000947fffff: BL31
8. 0000000094800000-000000009fffffff: RAM
9. 00000000a0000000-00000000a01fffff: RESERVED
10. 00000000a0200000-00000000ffec2fff: RAM
11. 00000000ffec3000-00000000ffffffff: CONFIGURATION TABLES
12. 0000000100000000-000000047fffffff: RAM
...and see the following information:
sync rank num:2, rank0_size:0x200000000, rank1_size:0x200000000
[INFO ] Mapping address range [0x80000000:0x480000000) as cacheable |
read-write | non-secure | normal
[INFO ] Mapping address range [0x80000000:0x80100000) as non-cacheable |
read-write | non-secure | normal
[DEBUG] Backing address range [0x80000000:0xc0000000) with new page table @0x00104000
[DEBUG] Backing address range [0x80000000:0x80200000) with new page table @0x00105000
This means that the GPUEB memory, which is at 0xa0000000 and is 0x200000 long
is indeed placed in RAM.
At this point, I think it'd be best if you use reserved-memory instead, as that
is not SRAM, but really a (system) DRAM carveout that is reserved to the EB
firmware. That would give a correct representation of this memory in devicetree.
Cool thing is, that would also make the code shorter in this case as the calls
to of_parse_phandle() -> of_address_to_resource() would become simply
ret = of_reserved_mem_region_to_resource(dev->of_node, 0, &res);
and instead of having a "shmem" property, you'd have the standard "memory-region"
one, which then fits perfectly.
> +
> + 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");
> + }
> +
> + mfg->pd.name = dev_name(dev);
> + mfg->pd.attach_dev = mtk_mfg_attach_dev;
> + mfg->pd.detach_dev = mtk_mfg_detach_dev;
> + mfg->pd.power_off = mtk_mfg_power_off;
> + mfg->pd.power_on = mtk_mfg_power_on;
> + mfg->pd.set_performance_state = mtk_mfg_set_performance;
> + mfg->pd.flags = GENPD_FLAG_OPP_TABLE_FW;
> + pm_genpd_init(&mfg->pd, NULL, false);
The pm_genpd_init() function may fail. Error check please.
> +
> + ret = clk_prepare_enable(mfg->clk_eb);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to turn on EB clock\n");
What happens if the `gpu_regs` regulator(s) is/are not enabled at boot?
I am guessing that the EB doesn't depend at all on these being enabled, as it
should be powered by the internal vscp or sspm - but still asking to make sure
that this wasn't an overlook.
> + mfg->ipi_magic = readl(mfg->gpr + GPR_IPI_MAGIC);
> + /* Downstream does this, don't know why. */
Preventing reinitialization?
Did you try to avoid that write? What happens in that case?
Also, if you unload this module and reload it, are you able to reinitialize the EB,
or are you reading zero in GPR_IPI_MAGIC (preventing you from correctly reinit this
driver again)?
> + writel(0x0, mfg->gpr + GPR_IPI_MAGIC);
> +
> + ret = mtk_mfg_init_mbox(mfg);
> + if (ret) {
> + ret = dev_err_probe(dev, ret, "Couldn't initialise mailbox\n");
> + goto out;
> + }
> +
> + mfg->last_opp = -1;
> +
> + ret = mtk_mfg_power_on(&mfg->pd);
> + clk_disable_unprepare(mfg->clk_eb);
> + 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;
> + }
> +
> + ret = mtk_mfg_init_clk_provider(mfg);
> + if (ret)
> + goto out;
> +
> + ret = of_genpd_add_provider_simple(pdev->dev.of_node, &mfg->pd);
> + if (ret) {
> + ret = dev_err_probe(dev, ret, "Failed to add pmdomain provider\n");
> + goto out;
> + }
> +
> + return 0;
> +
> +out:
> + mtk_mfg_power_off(&mfg->pd);
> + return ret;
> +}
static void mtk_mfg_remove(struct platform_device *pdev)
{
struct mtk_mfg *mfg = dev_get_drvdata(&pdev->dev);
of_genpd_del_provider(....)
pm_genpd_remove(....)
mtk_mfg_power_off(...)
mbox_free_channel(mfg->gf_mbox->ch);
mfg->gf_mbox->ch = NULL;
mbox_free_channel(mfg->slp_mbox->ch);
mfg->slp_mbox->ch = NULL;
}
> +
> +static struct platform_driver mtk_mfg_driver = {
> + .driver = {
> + .name = "mtk-mfg-pmdomain",
> + .of_match_table = mtk_mfg_of_match,
> + },
> + .probe = mtk_mfg_probe,
.remove = mtk_mfg_remove,
> +};
> +module_platform_driver(mtk_mfg_driver);
> +
> +MODULE_AUTHOR("Nicolas Frattaroli <nicolas.frattaroli@collabora.com>");
> +MODULE_DESCRIPTION("MediaTek MFlexGraphics Power Domain Driver");
> +MODULE_LICENSE("GPL");
>
There might be more, but for now, I'm done with this review round :-)
Cheers,
Angelo
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 8/8] pmdomain: mediatek: Add support for MFlexGraphics
2025-09-23 16:25 ` AngeloGioacchino Del Regno
@ 2025-09-24 19:11 ` Nicolas Frattaroli
2025-09-25 13:56 ` AngeloGioacchino Del Regno
0 siblings, 1 reply; 18+ messages in thread
From: Nicolas Frattaroli @ 2025-09-24 19:11 UTC (permalink / raw)
To: Boris Brezillon, Jassi Brar, Chia-I Wu, Chen-Yu Tsai,
Steven Price, Liviu Dudau, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Matthias Brugger, Kees Cook,
Gustavo A. R. Silva, Ulf Hansson, AngeloGioacchino Del Regno
Cc: kernel, dri-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-hardening, linux-pm
On Tuesday, 23 September 2025 18:25:53 Central European Summer Time AngeloGioacchino Del Regno wrote:
> Il 23/09/25 13:40, Nicolas Frattaroli ha scritto:
> > Various MediaTek SoCs use GPU integration silicon named "MFlexGraphics"
> > by MediaTek. On the MT8196 and MT6991 SoCs, interacting with this
> > integration silicon is required to power on the GPU.
> >
> > This glue silicon is in the form of an embedded microcontroller running
> > special-purpose firmware, which autonomously adjusts clocks and
> > regulators.
> >
> > Implement a driver, modelled as a pmdomain driver with a
> > set_performance_state operation, to support these SoCs.
> >
> > The driver also exposes the actual achieved clock rate, as read back
> > from the MCU, as common clock framework clocks, by acting as a clock
> > provider as well.
> >
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> > drivers/pmdomain/mediatek/Kconfig | 16 +
> > drivers/pmdomain/mediatek/Makefile | 1 +
> > drivers/pmdomain/mediatek/mtk-mfg-pmdomain.c | 928 +++++++++++++++++++++++++++
> > 3 files changed, 945 insertions(+)
> >
> > diff --git a/drivers/pmdomain/mediatek/Kconfig b/drivers/pmdomain/mediatek/Kconfig
> > index 0e34a517ab7d5a867bebaab11c0d866282a15e45..2abf78c85d017b1e3526b41c81f274f78d581fd0 100644
> > [ ... 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,
>
> I don't really like seeing index assignments to enumeration, especially when there
> are no holes... and you have also clearly written that this is ABI-do-not-touch so
> I'm not sure that having those numbers here is improving anything.
>
> I also haven't got strong opinions about that, anyway.
My main worry is that someone comes by and alphabetically sorts them
with either some style linter script and does not think to read the
comment, and it's either an overworked maintainer or get acked by
an overworked maintainer.
> [... snip ...]
> > +
> > +static int mtk_mfg_eb_off(struct mtk_mfg *mfg)
> > +{
> > + struct device *dev = &mfg->pdev->dev;
> > + struct mtk_mfg_ipi_sleep_msg msg = {
>
> Can this be constified?
No :( mbox_send_message's msg parameter is not const, so it'd discard
the qualifier, and instead of explicitly discarding the qualifier
(which would be a very stinky code smell) we would have to look into
whether the mailbox subsystem is cool with const void* for messages,
and whether all the mailbox drivers are fine with that too.
> > + .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(dev, "Cannot send sleep command: %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(dev, "timed out waiting for EB to power off, val=0x%08X\n",
> > + val);
>
> 90 columns is fine, one line please.
>
> > +
> > + return ret;
> > +}
> > +
> [... snip ...]
> > +
> > +static int mtk_mfg_attach_dev(struct generic_pm_domain *pd, struct device *dev)
> > +{
> > + struct mtk_mfg *mfg = mtk_mfg_from_genpd(pd);
> > + struct dev_pm_opp_data *opps = mfg->gpu_opps;
> > + int i, ret;
> > +
> > + for (i = mfg->num_opps - 1; i >= 0; i--) {
> > + if ((i == mfg->num_opps - 1) || (opps[i].freq != opps[i + 1].freq)) {
>
> /* Add a comment here, because you're using a trick, and it's not
> * very fast to read, as in, if you skim through that, you're most
> * probably losing the fact that the first OPP is always added
> * regardless of anything.
> */
> if ((i != mfg->num_opps - 1) || (opps[i].freq == opps[i + 1].freq))
> continue;
>
> /* Reduced indentation :-) */
> ret = dev_pm_opp_add_dynamic(.....) etc
>
Sure, but not before properly applying De Morgan's law here ;) It
should be the following as far as I can tell:
if ((i != mfg->num_opps - 1) && (opps[i].freq == opps[i + 1].freq))
continue;
> > + ret = dev_pm_opp_add_dynamic(dev, &opps[i]);
> > + if (ret) {
> > + dev_err(dev, "Failed to add OPP level %u from PD %s\n",
> > + opps[i].level, pd->name);
> > + dev_pm_opp_remove_all_dynamic(dev);
> > + return ret;
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> [... snip ...]
> > +
> > +static int mtk_mfg_probe(struct platform_device *pdev)
> > +{
> [... snip ...]
> > +
> > + ret = clk_prepare_enable(mfg->clk_eb);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to turn on EB clock\n");
>
> What happens if the `gpu_regs` regulator(s) is/are not enabled at boot?
>
> I am guessing that the EB doesn't depend at all on these being enabled, as it
> should be powered by the internal vscp or sspm - but still asking to make sure
> that this wasn't an overlook.
Yeah, the EB doesn't need those regulators on. After somewhat fixing module
unload and reload on my side, I can now confirm that it doesn't appear to
need them during probe.
>
> > + mfg->ipi_magic = readl(mfg->gpr + GPR_IPI_MAGIC);
> > + /* Downstream does this, don't know why. */
>
> Preventing reinitialization?
> Did you try to avoid that write? What happens in that case?
>
> Also, if you unload this module and reload it, are you able to reinitialize the EB,
> or are you reading zero in GPR_IPI_MAGIC (preventing you from correctly reinit this
> driver again)?
Okay so this led me down a deep rabbit hole and I realised that so far, we
could only read the IPI magic because the bootloader helpfully left MFG on
for us. So on second probe, we'd get a magic number of 0, and all IPI comms
that use it would fail.
Fix is simple though, just read the magic in power_on. I also left out the
0 write but I might experimentally add it back in to see if it changes any
of the other behaviour I'm currently chasing.
>
> > + writel(0x0, mfg->gpr + GPR_IPI_MAGIC);
> > +
> > + ret = mtk_mfg_init_mbox(mfg);
> > + if (ret) {
> > + ret = dev_err_probe(dev, ret, "Couldn't initialise mailbox\n");
> > + goto out;
> > + }
> > +
> > + mfg->last_opp = -1;
> > +
> > + ret = mtk_mfg_power_on(&mfg->pd);
> > + clk_disable_unprepare(mfg->clk_eb);
> > + 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;
> > + }
> > +
> > + ret = mtk_mfg_init_clk_provider(mfg);
> > + if (ret)
> > + goto out;
> > +
> > + ret = of_genpd_add_provider_simple(pdev->dev.of_node, &mfg->pd);
> > + if (ret) {
> > + ret = dev_err_probe(dev, ret, "Failed to add pmdomain provider\n");
> > + goto out;
> > + }
> > +
> > + return 0;
> > +
> > +out:
> > + mtk_mfg_power_off(&mfg->pd);
> > + return ret;
> > +}
>
> static void mtk_mfg_remove(struct platform_device *pdev)
> {
> struct mtk_mfg *mfg = dev_get_drvdata(&pdev->dev);
>
> of_genpd_del_provider(....)
>
> pm_genpd_remove(....)
>
> mtk_mfg_power_off(...)
Unconditional power_off will go poorly if the thing isn't powered
on at removal time, so I need to figure out something more clever.
Unfortunately, that something more clever isn't "dev_pm_genpd_is_on"
because that has a case where it will return false and then devres
kicks in and says hey you left your regulators on that's not cool.
I'll have to spend another day at the debug print factory until
I can figure out what's wrong there, and if I can't, then I guess
we'll add our own pd_on counting.
>
> mbox_free_channel(mfg->gf_mbox->ch);
> mfg->gf_mbox->ch = NULL;
>
> mbox_free_channel(mfg->slp_mbox->ch);
> mfg->slp_mbox->ch = NULL;
>
>
> }
>
> > +
> > +static struct platform_driver mtk_mfg_driver = {
> > + .driver = {
> > + .name = "mtk-mfg-pmdomain",
> > + .of_match_table = mtk_mfg_of_match,
> > + },
> > + .probe = mtk_mfg_probe,
>
> .remove = mtk_mfg_remove,
>
> > +};
> > +module_platform_driver(mtk_mfg_driver);
> > +
> > +MODULE_AUTHOR("Nicolas Frattaroli <nicolas.frattaroli@collabora.com>");
> > +MODULE_DESCRIPTION("MediaTek MFlexGraphics Power Domain Driver");
> > +MODULE_LICENSE("GPL");
> >
>
> There might be more, but for now, I'm done with this review round :-)
>
> Cheers,
> Angelo
>
Thanks for the review. Assume all comments I didn't reply to (including
the big one) are acknowledged and will be addressed.
Kind regards,
Nicolas Frattaroli
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v4 8/8] pmdomain: mediatek: Add support for MFlexGraphics
2025-09-24 19:11 ` Nicolas Frattaroli
@ 2025-09-25 13:56 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 18+ messages in thread
From: AngeloGioacchino Del Regno @ 2025-09-25 13:56 UTC (permalink / raw)
To: Nicolas Frattaroli, Boris Brezillon, Jassi Brar, Chia-I Wu,
Chen-Yu Tsai, Steven Price, Liviu Dudau, Maarten Lankhorst,
Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
Kees Cook, Gustavo A. R. Silva, Ulf Hansson
Cc: kernel, dri-devel, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek, linux-hardening, linux-pm
Il 24/09/25 21:11, Nicolas Frattaroli ha scritto:
> On Tuesday, 23 September 2025 18:25:53 Central European Summer Time AngeloGioacchino Del Regno wrote:
>> Il 23/09/25 13:40, Nicolas Frattaroli ha scritto:
>>> Various MediaTek SoCs use GPU integration silicon named "MFlexGraphics"
>>> by MediaTek. On the MT8196 and MT6991 SoCs, interacting with this
>>> integration silicon is required to power on the GPU.
>>>
>>> This glue silicon is in the form of an embedded microcontroller running
>>> special-purpose firmware, which autonomously adjusts clocks and
>>> regulators.
>>>
>>> Implement a driver, modelled as a pmdomain driver with a
>>> set_performance_state operation, to support these SoCs.
>>>
>>> The driver also exposes the actual achieved clock rate, as read back
>>> from the MCU, as common clock framework clocks, by acting as a clock
>>> provider as well.
>>>
>>> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
>>> ---
>>> drivers/pmdomain/mediatek/Kconfig | 16 +
>>> drivers/pmdomain/mediatek/Makefile | 1 +
>>> drivers/pmdomain/mediatek/mtk-mfg-pmdomain.c | 928 +++++++++++++++++++++++++++
>>> 3 files changed, 945 insertions(+)
>>>
>>> diff --git a/drivers/pmdomain/mediatek/Kconfig b/drivers/pmdomain/mediatek/Kconfig
>>> index 0e34a517ab7d5a867bebaab11c0d866282a15e45..2abf78c85d017b1e3526b41c81f274f78d581fd0 100644
>>> [ ... 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,
>>
>> I don't really like seeing index assignments to enumeration, especially when there
>> are no holes... and you have also clearly written that this is ABI-do-not-touch so
>> I'm not sure that having those numbers here is improving anything.
>>
>> I also haven't got strong opinions about that, anyway.
>
> My main worry is that someone comes by and alphabetically sorts them
> with either some style linter script and does not think to read the
> comment, and it's either an overworked maintainer or get acked by
> an overworked maintainer.
>
>> [... snip ...]
>>> +
>>> +static int mtk_mfg_eb_off(struct mtk_mfg *mfg)
>>> +{
>>> + struct device *dev = &mfg->pdev->dev;
>>> + struct mtk_mfg_ipi_sleep_msg msg = {
>>
>> Can this be constified?
>
> No :( mbox_send_message's msg parameter is not const, so it'd discard
> the qualifier, and instead of explicitly discarding the qualifier
> (which would be a very stinky code smell) we would have to look into
> whether the mailbox subsystem is cool with const void* for messages,
> and whether all the mailbox drivers are fine with that too.
>
>>> + .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(dev, "Cannot send sleep command: %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(dev, "timed out waiting for EB to power off, val=0x%08X\n",
>>> + val);
>>
>> 90 columns is fine, one line please.
>>
>>> +
>>> + return ret;
>>> +}
>>> +
>> [... snip ...]
>>> +
>>> +static int mtk_mfg_attach_dev(struct generic_pm_domain *pd, struct device *dev)
>>> +{
>>> + struct mtk_mfg *mfg = mtk_mfg_from_genpd(pd);
>>> + struct dev_pm_opp_data *opps = mfg->gpu_opps;
>>> + int i, ret;
>>> +
>>> + for (i = mfg->num_opps - 1; i >= 0; i--) {
>>> + if ((i == mfg->num_opps - 1) || (opps[i].freq != opps[i + 1].freq)) {
>>
>> /* Add a comment here, because you're using a trick, and it's not
>> * very fast to read, as in, if you skim through that, you're most
>> * probably losing the fact that the first OPP is always added
>> * regardless of anything.
>> */
>> if ((i != mfg->num_opps - 1) || (opps[i].freq == opps[i + 1].freq))
>> continue;
>>
>> /* Reduced indentation :-) */
>> ret = dev_pm_opp_add_dynamic(.....) etc
>>
>
> Sure, but not before properly applying De Morgan's law here ;) It
> should be the following as far as I can tell:
WHOOOOOOPS! That was quite a bad typo :D
(yes, of course it's &&)
>
> if ((i != mfg->num_opps - 1) && (opps[i].freq == opps[i + 1].freq))
> continue;
>
>>> + ret = dev_pm_opp_add_dynamic(dev, &opps[i]);
>>> + if (ret) {
>>> + dev_err(dev, "Failed to add OPP level %u from PD %s\n",
>>> + opps[i].level, pd->name);
>>> + dev_pm_opp_remove_all_dynamic(dev);
>>> + return ret;
>>> + }
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>> [... snip ...]
>>> +
>>> +static int mtk_mfg_probe(struct platform_device *pdev)
>>> +{
>> [... snip ...]
>>> +
>>> + ret = clk_prepare_enable(mfg->clk_eb);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret, "failed to turn on EB clock\n");
>>
>> What happens if the `gpu_regs` regulator(s) is/are not enabled at boot?
>>
>> I am guessing that the EB doesn't depend at all on these being enabled, as it
>> should be powered by the internal vscp or sspm - but still asking to make sure
>> that this wasn't an overlook.
>
> Yeah, the EB doesn't need those regulators on. After somewhat fixing module
> unload and reload on my side, I can now confirm that it doesn't appear to
> need them during probe.
>
>>
>>> + mfg->ipi_magic = readl(mfg->gpr + GPR_IPI_MAGIC);
>>> + /* Downstream does this, don't know why. */
>>
>> Preventing reinitialization?
>> Did you try to avoid that write? What happens in that case?
>>
>> Also, if you unload this module and reload it, are you able to reinitialize the EB,
>> or are you reading zero in GPR_IPI_MAGIC (preventing you from correctly reinit this
>> driver again)?
>
> Okay so this led me down a deep rabbit hole and I realised that so far, we
> could only read the IPI magic because the bootloader helpfully left MFG on
> for us. So on second probe, we'd get a magic number of 0, and all IPI comms
> that use it would fail.
I had a hunch ;-)
>
> Fix is simple though, just read the magic in power_on. I also left out the
> 0 write but I might experimentally add it back in to see if it changes any
> of the other behaviour I'm currently chasing.
>
Yeah, we don't know what this write is all about and it logically doesn't make
much sense - so if it works without, let's just leave it out.
>>
>>> + writel(0x0, mfg->gpr + GPR_IPI_MAGIC);
>>> +
>>> + ret = mtk_mfg_init_mbox(mfg);
>>> + if (ret) {
>>> + ret = dev_err_probe(dev, ret, "Couldn't initialise mailbox\n");
>>> + goto out;
>>> + }
>>> +
>>> + mfg->last_opp = -1;
>>> +
>>> + ret = mtk_mfg_power_on(&mfg->pd);
>>> + clk_disable_unprepare(mfg->clk_eb);
>>> + 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;
>>> + }
>>> +
>>> + ret = mtk_mfg_init_clk_provider(mfg);
>>> + if (ret)
>>> + goto out;
>>> +
>>> + ret = of_genpd_add_provider_simple(pdev->dev.of_node, &mfg->pd);
>>> + if (ret) {
>>> + ret = dev_err_probe(dev, ret, "Failed to add pmdomain provider\n");
>>> + goto out;
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +out:
>>> + mtk_mfg_power_off(&mfg->pd);
>>> + return ret;
>>> +}
>>
>> static void mtk_mfg_remove(struct platform_device *pdev)
>> {
>> struct mtk_mfg *mfg = dev_get_drvdata(&pdev->dev);
>>
>> of_genpd_del_provider(....)
>>
>> pm_genpd_remove(....)
>>
>> mtk_mfg_power_off(...)
>
> Unconditional power_off will go poorly if the thing isn't powered
> on at removal time, so I need to figure out something more clever.
>
> Unfortunately, that something more clever isn't "dev_pm_genpd_is_on"
> because that has a case where it will return false and then devres
> kicks in and says hey you left your regulators on that's not cool.
>
> I'll have to spend another day at the debug print factory until
> I can figure out what's wrong there, and if I can't, then I guess
> we'll add our own pd_on counting.
I did suspect that unconditional poweroff could be possibly wreaking havoc, and
I also knew that you'd see that in case, so I didn't write anything about my own
suspects :-P
But then, good. Since we don't care about refcounting in this case, I guess that
if there's no other way, eventually just a bool to track off/on state is enough.
Cheers!
>
>>
>> mbox_free_channel(mfg->gf_mbox->ch);
>> mfg->gf_mbox->ch = NULL;
>>
>> mbox_free_channel(mfg->slp_mbox->ch);
>> mfg->slp_mbox->ch = NULL;
>>
>>
>> }
>>
>>> +
>>> +static struct platform_driver mtk_mfg_driver = {
>>> + .driver = {
>>> + .name = "mtk-mfg-pmdomain",
>>> + .of_match_table = mtk_mfg_of_match,
>>> + },
>>> + .probe = mtk_mfg_probe,
>>
>> .remove = mtk_mfg_remove,
>>
>>> +};
>>> +module_platform_driver(mtk_mfg_driver);
>>> +
>>> +MODULE_AUTHOR("Nicolas Frattaroli <nicolas.frattaroli@collabora.com>");
>>> +MODULE_DESCRIPTION("MediaTek MFlexGraphics Power Domain Driver");
>>> +MODULE_LICENSE("GPL");
>>>
>>
>> There might be more, but for now, I'm done with this review round :-)
>>
>> Cheers,
>> Angelo
>>
>
> Thanks for the review. Assume all comments I didn't reply to (including
> the big one) are acknowledged and will be addressed.
>
> Kind regards,
> Nicolas Frattaroli
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread