linux-kernel - Re: [PATCH V3 1/1] soc: mediatek: mtk-regulator-coupler: Add support for MT8189

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <542ceb46-0b57-4cd1-a679-ec55457043ae@collabora.com>
Date: Tue, 4 Nov 2025 15:24:14 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: "niklaus.liu" <Niklaus.Liu@...iatek.com>,
 Matthias Brugger <matthias.bgg@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org,
 Project_Global_Chrome_Upstream_Group@...iatek.com, sirius.wang@...iatek.com,
 vince-wl.liu@...iatek.com, jh.hsu@...iatek.com, zhigang.qin@...iatek.com,
 sen.chu@...iatek.com
Subject: Re: [PATCH V3 1/1] soc: mediatek: mtk-regulator-coupler: Add support
 for MT8189

Il 04/11/25 08:12, niklaus.liu ha scritto:
> From: Niklaus Liu <niklaus.liu@...iatek.com>
> 
> Enhance the regulator coupler driver to support GPU power control on the
> MediaTek MT8189 platform. This update ensures proper coordination of
> multiple regulators required for GPU operation,improving power management
> and system stability.
> 
> Signed-off-by: Niklaus Liu <niklaus.liu@...iatek.com>
> ---
>   drivers/soc/mediatek/mtk-regulator-coupler.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/soc/mediatek/mtk-regulator-coupler.c b/drivers/soc/mediatek/mtk-regulator-coupler.c
> index 0b6a2884145e..e2a1fb459e42 100644
> --- a/drivers/soc/mediatek/mtk-regulator-coupler.c
> +++ b/drivers/soc/mediatek/mtk-regulator-coupler.c
> @@ -42,6 +42,18 @@ static int mediatek_regulator_balance_voltage(struct regulator_coupler *coupler,
>   	int max_uV = INT_MAX;
>   	int ret;
>   
> +	/*
> +	 * When vsram_gpu is enabled or disabled and the use_count of the
> +	 * vsram_gpu regulator is zero, the regulator coupler driver will
> +	 * execute regulator_do_balance_voltage, which adjusts the vsram_gpu
> +	 * voltage to the minimum value. This may result in vsram_gpu being
> +	 * lower than vgpu. Therefore, when enabling or disabling vsram_gpu,
> +	 * the 8189 temporarily skips the regulator coupler driver's modification
> +	 * of the vsram_gpu voltage.
> +	 */

This will break the rules of your own hardware. Seriously.

The VSRAM_GPU has to be set in a specific range, where the minimum voltage has to
guarantee that the GPU can run in the first quarter of the OPP entries (which does
always correspond to lowest/lower/low or lowest/low) for the GPU.

If you return zero here, and VSRAM_GPU was set to Vmax (scenario: GPU was at Turbo
frequency) before shutting down completely (ex.: system suspend), at device resume
time, the VSRAM_GPU rail will be overvolting the GPU SRAM.

While that won't cause any "magic smoke" because the VSRAM is still in the maximum
operating ranges constraint, this will leak power, hence increase the chip's heat
output, and might as well create possible instabilities.

Remember that the GPU SRAM voltage has to *always be constrained* as:
  - VSRAM <= VGPU when VGPU must be over the VSRAM constraint
  - VSRAM == VGPU if the voltages are in range between each other
  - VSRAM > VGPU *only* for SLEEP (or LOWEST) freq setting

As I already said in my previous review for this patch, your devicetree may be
misconfiguring the regulators, or their hierarchy (as those are MFG0/1+Panfrost).

Besides, are you testing this on Panfrost, even, or on the Mali DDK driver?
That really doesn't look right.

Also - MT8189 is not special. There's nothing different between MT8189 and the
others in regard to how the GPU regulators should be coupled, and in regard to
their constraints relationship.

Regards,
Angelo

> +	if (of_machine_is_compatible("mediatek,mt8189") && rdev == mrc->vsram_rdev)
> +		return 0;
> +
>   	/*
>   	 * If the target device is on, setting the SRAM voltage directly
>   	 * is not supported as it scales through its coupled supply voltage.
> @@ -148,6 +160,7 @@ static int mediatek_regulator_coupler_init(void)
>   	if (!of_machine_is_compatible("mediatek,mt8183") &&
>   	    !of_machine_is_compatible("mediatek,mt8186") &&
>   	    !of_machine_is_compatible("mediatek,mt8188") &&
> +	    !of_machine_is_compatible("mediatek,mt8189") &&

(but this is ok here)

>   	    !of_machine_is_compatible("mediatek,mt8192"))
>   		return 0;
>