From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 39B10CAC581 for ; Mon, 8 Sep 2025 17:08:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=9HAwj1OxHTCCQy9bYmOg3NKIPsb30ZbA5RjhYkaT/CQ=; b=REZTvv9F4TLFnPRCcHga5FtfHj clqw06SbqNIEJG/9hK/lKyxGk8rCqgqbh/OXTtVPhzEJ2kRHvcKttE69pRU419bB+PVIdUan+twUZ v4XU7PGxxeW3WufrkGjEl8O/Hnx7k49Cx0vS+7CPJaumtnopFahFOCK24eVj3Jn1WAIYp58yG5aRg Yj4gpi5Pp8Ih/c+QXAaICDmPFrttEqlJktwQv1PdOZsIRgRWABpxQZMywm4IIR4KyuY1mHazqp60S yFL380pEo4O1aef7uddGTz1zoSTdyAhMFsDYbVVAWtMrs4VPGDUaMln6SH0DL36NVdfd1e6JvwQtR REcIRlfw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1uvfMH-000000011j8-3FKm; Mon, 08 Sep 2025 17:08:49 +0000 Received: from bali.collaboradmins.com ([2a01:4f8:201:9162::2]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1uvbJE-0000000H8Uc-3oNh; Mon, 08 Sep 2025 12:49:26 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1757335763; bh=BW9QqUTnxdImH2dw+lKf1PJLRVjcmqu6EdnalgtPi08=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=VhFZMXKm3QcD+UiicvaqWjZjZTESYw6AGmLRQTY/VDOfpcPn/rvE76yqFgBa+zMRv qmUZnj5gWjzOLg7eLR1xb5jED0/vnN9o7XK05FUbAybkB9E6fZjscI7yeqctu5A6Fu KyVo7eMIn/giIRYJXe2dbvNOlI0dwhUdxLxEswOShmW4p6hoRXCwHGCxdiusmsgSSX 26WV1OMpW2pBoQ9Ya4hHjXvpUBqarKPF7t1OaPo5jo9A5VD+RW4vkAmYP6U0UbLzJ1 Y0rIUPColxIRQY/ESvaVjAdnRJi1KdkzPr9Qq8lnmif0F2XloPTvmgQhfmucwnjK1k Kc2jLmNKkYgQw== Received: from [192.168.1.100] (2-237-20-237.ip236.fastwebnet.it [2.237.20.237]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) (Authenticated sender: kholk11) by bali.collaboradmins.com (Postfix) with ESMTPSA id 3462417E0C96; Mon, 8 Sep 2025 14:49:22 +0200 (CEST) Message-ID: <0b31377d-70b4-4a51-84d9-1a568c8be5df@collabora.com> Date: Mon, 8 Sep 2025 14:49:21 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH RFC 02/10] dt-bindings: devfreq: add mt8196-gpufreq binding To: Nicolas Frattaroli , Boris Brezillon , Steven Price , Liviu Dudau , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Matthias Brugger , MyungJoo Ham , Kyungmin Park , Chanwoo Choi , Jassi Brar , Kees Cook , "Gustavo A. R. Silva" Cc: Chia-I Wu , Chen-Yu Tsai , kernel@collabora.com, dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-pm@vger.kernel.org, linux-hardening@vger.kernel.org References: <20250905-mt8196-gpufreq-v1-0-7b6c2d6be221@collabora.com> <20250905-mt8196-gpufreq-v1-2-7b6c2d6be221@collabora.com> <751d3abc-cf40-40a2-a580-7c0ba425ac25@collabora.com> <13857717.uLZWGnKmhe@workhorse> From: AngeloGioacchino Del Regno Content-Language: en-US In-Reply-To: <13857717.uLZWGnKmhe@workhorse> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250908_054925_253184_F86AA93E X-CRM114-Status: GOOD ( 50.32 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Il 08/09/25 13:39, Nicolas Frattaroli ha scritto: > On Monday, 8 September 2025 13:15:03 Central European Summer Time AngeloGioacchino Del Regno wrote: >> Il 05/09/25 12:22, Nicolas Frattaroli ha scritto: >>> On the MediaTek MT8196 SoC, the GPU has its power and frequency >>> dynamically controlled by an embedded special-purpose MCU. This MCU is >>> in charge of powering up the GPU silicon. It also provides us with a >>> list of available OPPs at runtime, and is fully in control of all the >>> regulator and clock fiddling it takes to reach a certain level of >>> performance. It's also in charge of enforcing limits on power draw or >>> temperature. >>> >>> Add a binding for this device in the devfreq subdirectory, where it >>> seems to fit in best considering its tasks. >>> >>> The functions of many of the mailbox channels are unknown. This is not >>> the fault of this binding's author; we've never received adequate >>> documentation for this hardware, and the downstream code does not make >>> use of them in a way that'd reveal their purpose. They are kept in the >>> binding as the binding should be complete. >>> >>> Signed-off-by: Nicolas Frattaroli >>> --- >>> .../bindings/devfreq/mediatek,mt8196-gpufreq.yaml | 116 +++++++++++++++++++++ >>> 1 file changed, 116 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/devfreq/mediatek,mt8196-gpufreq.yaml b/Documentation/devicetree/bindings/devfreq/mediatek,mt8196-gpufreq.yaml >>> new file mode 100644 >>> index 0000000000000000000000000000000000000000..1fe43c9fc94bb603b1fb77e9a97a27e92fea1ae8 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/devfreq/mediatek,mt8196-gpufreq.yaml >>> @@ -0,0 +1,116 @@ >>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/devfreq/mediatek,mt8196-gpufreq.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: MediaTek MFlexGraphics Performance Controller >> >> Doesn't MFG stand for MediaTek Flexible Graphics? (or did they update the name?) >> >> Perhaps it's a good idea to also add that reference... I think it's a little more >> readable and understandable compared to "MFlexGraphics" :-) > > "MFlexGraphics" is what the abbreviation section in the datasheet calls "MFG". > I don't see "Flexible Graphics" at all in the datasheet, but it's an obvious > inference of what the name means. > > I think keeping "MFlexGraphics" is better for people grepping for what > the datasheet calls it. > Okay in MT8196 that was updated then. On any other SoC previous to MT8196, the datasheet name is "MediaTek Flexible Graphics (MFG)". If you want to keep "MFlexGraphics" in MT8196-style, it's still a good idea to also reference somewhere the old "MediaTek Flexible Graphics" name, as that was used for more than 10 years. Really. >> >>> + >>> +maintainers: >>> + - Nicolas Frattaroli >>> + >>> +properties: >>> + $nodename: >>> + pattern: '^performance-controller@[a-f0-9]+$' >>> + >>> + compatible: >>> + enum: >>> + - mediatek,mt8196-gpufreq >>> + >>> + reg: >>> + items: >>> + - description: GPR memory area >>> + - description: RPC memory area >>> + - description: SoC variant ID register >>> + >>> + reg-names: >>> + items: >>> + - const: gpr >>> + - const: rpc >>> + - const: e2_id >> >> We should find a better name for that "e2_id". > > Agreed, but we don't have a register map that includes this address > and would give us a different name. Yeah but still, it feels like this naming is MT8196-specific, and this driver is not entirely specific to this SoC (this version is, but with minor modifications this can work on other chips as well). > I think it's some sort of silicon revision. It is. And there's a broad range of names that you can use in place of "e2_id"... If there's no precise name, always go with something that is generic enough but that resembles what can be found inside of the mmio that you're specifying. >> >>> + >>> + clocks: >>> + items: >>> + - description: main clock of the embedded controller (EB) >>> + - description: core PLL >>> + - description: stack 0 PLL >>> + - description: stack 1 PLL >>> + >>> + clock-names: >>> + items: >>> + - const: eb >>> + - const: mfgpll >>> + - const: mfgpll_sc0 >>> + - const: mfgpll_sc1 >>> + >>> + mboxes: >>> + items: >>> + - description: FastDVFS events >>> + - description: frequency control >>> + - description: sleep control >>> + - description: timer control >>> + - description: frequency hopping control >>> + - description: hardware voter control >>> + - description: gpumpu (some type of memory control, unknown) >>> + - description: FastDVFS control >>> + - description: Unknown >>> + - description: Unknown >>> + - description: Unknown, but likely controls some boosting behaviour >>> + - description: Unknown >>> + >>> + mbox-names: >>> + items: >>> + - const: fast_dvfs_event >> >> Any problem if we avoid underscores in names? >> > > No but I'm not sure what the canonical naming style is for mailbox > channels. "fastdvfsevent" is hard to read. > "fast-dvfs-event" would be good, wouldn't it? :-) >>> + - const: gpufreq >>> + - const: sleep >>> + - const: timer >>> + - const: fhctl >>> + - const: ccf >>> + - const: gpumpu >> >> "some type of memory control" .. it's really a MPU. For memory protection. :-) >> Besides, I don't think we have to touch anything in the gpumpu for freq control >> via gpueb. >> > > Gotcha, so should I leave it out of the GPUFreq binding's used channels? > > Would leave a gap, but that's probably fine. > I really doubt that this is ever getting used at all for GPUFreq, so yes, leave it out. >>> + - const: fast_dvfs >>> + - const: ipir_c_met >>> + - const: ipis_c_met >> >> MET is a hardware event tracer / profiler... and I'm fairly sure that we have no >> real reason to support it (at least, not like that, and not in a first submission). >> >> Ah btw: ipir ipis .. ipi-receive ipi-send >> > > Gotcha, will remove those as well. > P.S.: of course I was implying that if we ever need to support those, we can always add them later (but I still really doubt that we're ever going to use MET at all, even though it would be *really* nice to). >>> + - const: brisket >> >> Brisket is... something. There's one for the GPU, one for CPU, and one for APU. >> Not sure what it exactly does, but seems to be or control a FLL (freq locked loop). >> >>> + - const: ppb >> >> PPB = Peak Power Budget >> >> The PPB needs its own "big" driver (the PBM - Power Budget Manager) in order to do >> anything - as in - this manages a SoC-global peak power setting based on the >> available maximum deliverable instantaneous (and/or sustainable) power from the >> board's power source and it is mainly used for smartphone usecase (battery!). >> >> In order to work, the PPB HW (yet another mcu) needs to be initialized with tables >> for CPU and GPU (and APU? and something else too?), and with other data explaining >> the maximum instantaneous power that can delivered at a certain battery percentage. >> >> Important point is... I doubt that PPB is being initialized by the bootloader, on >> all of Genio, Kompanio and Dimensity chips, so this should be disabled by default. >> >> You can keep it, especially now that you have a description for it - and because it >> does indeed exist, but I doubt that we're using this anytime soon. > > If it's going to be used by a separate driver, wouldn't it be better if we don't make > this channel part of the channels the GPUFreq driver uses? > Not sure if gpufreq needs to poke that channel to tell to the ppb "I'm setting this frequency here", or if the MCUs can communicate that stuff on their own. I didn't do any research about that. Since adding stuff is kinda easier than removing, I guess avoiding to add this for now is a sensible option. Let's do just that then. >> >> Cheers, >> Angelo >> > > Kind regards, > Nicolas Frattaroli > >>> + >>> + shmem: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: phandle to the shared memory region of the GPUEB MCU >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - reg-names >>> + - clocks >>> + - clock-names >>> + - mboxes >>> + - mbox-names >>> + - shmem >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include >>> + >>> + gpufreq: performance-controller@4b09fd00 { >>> + compatible = "mediatek,mt8196-gpufreq"; >>> + reg = <0x4b09fd00 0x80>, >>> + <0x4b800000 0x1000>, >>> + <0x4b860128 0x4>; >>> + reg-names = "gpr", "rpc", "e2_id"; >>> + clocks = <&topckgen CLK_TOP_MFG_EB>, >>> + <&mfgpll CLK_MFG_AO_MFGPLL>, >>> + <&mfgpll_sc0 CLK_MFGSC0_AO_MFGPLL_SC0>, >>> + <&mfgpll_sc1 CLK_MFGSC1_AO_MFGPLL_SC1>; >>> + clock-names = "eb", "mfgpll", "mfgpll_sc0", >>> + "mfgpll_sc1"; >>> + mboxes = <&gpueb_mbox 0>, <&gpueb_mbox 1>, <&gpueb_mbox 2>, >>> + <&gpueb_mbox 3>, <&gpueb_mbox 4>, <&gpueb_mbox 5>, >>> + <&gpueb_mbox 6>, <&gpueb_mbox 7>, <&gpueb_mbox 8>, >>> + <&gpueb_mbox 9>, <&gpueb_mbox 10>, <&gpueb_mbox 11>; >>> + mbox-names = "fast_dvfs_event", "gpufreq", "sleep", "timer", "fhctl", >>> + "ccf", "gpumpu", "fast_dvfs", "ipir_c_met", "ipis_c_met", >>> + "brisket", "ppb"; >>> + shmem = <&gpufreq_shmem>; >>> + }; >>> >> >> > > > >