Re: [PATCH] arm64: dts: mediatek: mt8188-geralt: Add MT6319 PMIC

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 7/8/26 06:25, Chen-Yu Tsai wrote:
> On Tue, Jul 7, 2026 at 7:24 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> On 7/7/26 13:08, Chen-Yu Tsai wrote:
>>> On Tue, Jul 7, 2026 at 7:05 PM AngeloGioacchino Del Regno
>>> <angelogioacchino.delregno@collabora.com> wrote:
>>>>
>>>> On 7/7/26 12:44, Chen-Yu Tsai wrote:
>>>>> The Geralt design uses a MT6319 PMIC to power the big cores and LPDDR4X
>>>>> DRAM.
>>>>>
>>>>> Add a device node for it and hook up all the supplies.
>>>>>
>>>>> This change requires a firmware fix for the SPMI bus to read back
>>>>> correctly. The required firmware version is 15842.175.0. This is
>>>>> included in ChromeOS releases R150-16700.22.0 (available in Beta
>>>>> channel as of writing or stable channel in mid-July) or
>>>>> R151-16721.0.0 and later.
>>>>>
>>>>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
>>>>
>>>> This is a big problem then.
>>>>
>>>> I take it as if the firmware fix is not in place, probing the CPU power supplies
>>>> will fail, with all the consequences.
>>>
>>> That's right.
>>>
>>>> This means that with this, we're breaking all Geralt machines with older firmware,
>>>> which is not acceptable...
>>>>
>>>> ...so this needs a different solution, or strong reasons to make me understand that
>>>> I'm wrong, if I'm wrong.
>>>
>>> We can drop the CPU supplies (they don't matter since cpufreq is hardware
>>> driven) and just add the regulators. How does that sound? If the firmware
>>> isn't updated, the PMIC will fail to probe, but since nothing is using it,
>>> the system will continue to work (with some annoying error messages).
>>>
>>
>> That'd be wrong, but less wrong than not having anything described...
> 
> Yeah. As I said, it doesn't affect usability.
> 
>> ...I wonder if, at this point, you could set the SPMI node to status = "fail" and
>> have the *new* firmware override that to "ok".
>>
>> That's the only reasonable way to go forward, IMO.
> 
> I'm afraid it is unlikely to get a firmware release to fix a non-critical
> issue. We were fortunate that there was an actual critical issue being
> fixed that allowed me to merge the small fix for the SPMI controller.
> 

Yeah, I know how it is regarding firmware updates... but you know, I *must* try to
get OEMs/ODMs to do the right thing when hacky things get in front of me, and I am
fully aware that many of them largely underestimate such issues, and even close the
possibility to fix those, which is even worst.

On the other hand, I know you, and I know that you always try to do the same
whenever you can, so please, be aware that I'm not complaining about you.

> I think it would be great if everyone could update their OS and firmware,
> but I understand that some devices never get updates, such as those in
> board farms that never boot into ChromeOS.

Unfortunately, this is not just a ChromeOS problem but way larger...

Anyway, I'll stop here to prevent myself from going OT too much and being grumpier
than I already am :-P

I guess then just add the SPMI PMIC and add a big comment that clearly states
something like

/*
  * The mt6319_buck1 is the cpu-supply for CPU6 and CPU7.
  *
  * However, this device ships with a broken firmware which needs to be updated to
  * at version XXXXXXX or newer in order to workaround a bug that (describe bug).
  *
  * [blahblah text so that's why the cpu supply was not assigned to cpu6/7].
  */

...so that everyone reading the DT is fully aware of what's going on and can act
accordingly if they wish.

How does that sound?

Cheers,
Angelo

> 
> 
> ChenYu
> 
>> Of course, avoid having the firmware adding the CPU supplies, because that would
>> be rather sketchy then. Just "if spmi status fail found, change to ok".
>>
>> Cheers,
>> Angelo
>>
>>>
>>> ChenYu
>>>
>>>> Cheers,
>>>> Angelo
>>>>
>>>>> ---
>>>>>     .../boot/dts/mediatek/mt8188-geralt.dtsi      | 66 +++++++++++++++++++
>>>>>     1 file changed, 66 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi b/arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi
>>>>> index f382f90c48f5..fea52c377d88 100644
>>>>> --- a/arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi
>>>>> +++ b/arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi
>>>>> @@ -4,6 +4,8 @@
>>>>>      */
>>>>>     /dts-v1/;
>>>>>     #include <dt-bindings/gpio/gpio.h>
>>>>> +#include <dt-bindings/spmi/spmi.h>
>>>>> +
>>>>>     #include "mt8188.dtsi"
>>>>>     #include "mt6359.dtsi"
>>>>>
>>>>> @@ -241,6 +243,14 @@ &cpu5 {
>>>>>         cpu-supply = <&mt6359_vcore_buck_reg>;
>>>>>     };
>>>>>
>>>>> +&cpu6 {
>>>>> +     cpu-supply = <&mt6319_buck1>;
>>>>> +};
>>>>> +
>>>>> +&cpu7 {
>>>>> +     cpu-supply = <&mt6319_buck1>;
>>>>> +};
>>>>> +
>>>>>     /*
>>>>>      * Geralt is the reference design and doesn't have target TDP.
>>>>>      * Ciri is (currently) the only device following Geralt, and its
>>>>> @@ -1156,6 +1166,14 @@ pins-bus {
>>>>>                 };
>>>>>         };
>>>>>
>>>>> +     spmi_pins: spmi-pins {
>>>>> +             pins-bus {
>>>>> +                     pinmux = <PINMUX_GPIO175__FUNC_B0_SPMI_M_SCL>,
>>>>> +                              <PINMUX_GPIO176__FUNC_B0_SPMI_M_SDA>;
>>>>> +                     bias-disable;
>>>>> +             };
>>>>> +     };
>>>>> +
>>>>>         uart0_pins: uart0-pins {
>>>>>                 pins-bus {
>>>>>                         pinmux = <PINMUX_GPIO31__FUNC_O_UTXD0>,
>>>>> @@ -1267,6 +1285,54 @@ &spi2 {
>>>>>         status = "okay";
>>>>>     };
>>>>>
>>>>> +&spmi {
>>>>> +     pinctrl-names = "default";
>>>>> +     pinctrl-0 = <&spmi_pins>;
>>>>> +     #address-cells = <2>;
>>>>> +     #size-cells = <0>;
>>>>> +     status = "okay";
>>>>> +
>>>>> +     pmic@6 {
>>>>> +             compatible = "mediatek,mt6319-regulator", "mediatek,mt6315-regulator";
>>>>> +             reg = <0x6 SPMI_USID>;
>>>>> +             pvdd1-supply = <&pp4200_s5>;
>>>>> +             pvdd2-supply = <&pp4200_s5>;
>>>>> +             pvdd3-supply = <&pp4200_s5>;
>>>>> +             pvdd4-supply = <&pp4200_s5>;
>>>>> +
>>>>> +             regulators {
>>>>> +                     mt6319_buck1: vbuck1 {
>>>>> +                                     regulator-name = "ppvar_dvdd_proc_bc";
>>>>> +                                     regulator-min-microvolt = <520000>;
>>>>> +                                     regulator-max-microvolt = <1155000>;
>>>>> +                                     regulator-enable-ramp-delay = <256>;
>>>>> +                                     regulator-allowed-modes = <0 1 2>;
>>>>> +                                     regulator-always-on;
>>>>> +                     };
>>>>> +
>>>>> +                     /* vbuck2 is ganged with vbuck1 */
>>>>> +
>>>>> +                     mt6319_buck3: vbuck3 {
>>>>> +                                     regulator-name = "pp1125_emi_vdd2";
>>>>> +                                     regulator-min-microvolt = <1060000>;
>>>>> +                                     regulator-max-microvolt = <1170000>;
>>>>> +                                     regulator-enable-ramp-delay = <256>;
>>>>> +                                     regulator-allowed-modes = <0 1 2>;
>>>>> +                                     regulator-always-on;
>>>>> +                     };
>>>>> +
>>>>> +                     mt6319_buck4: vbuck4 {
>>>>> +                                     regulator-name = "pp0600_emi_vddq";
>>>>> +                                     regulator-min-microvolt = <570000>;
>>>>> +                                     regulator-max-microvolt = <650000>;
>>>>> +                                     regulator-enable-ramp-delay = <256>;
>>>>> +                                     regulator-allowed-modes = <0 1 2>;
>>>>> +                                     regulator-always-on;
>>>>> +                     };
>>>>> +             };
>>>>> +     };
>>>>> +};
>>>>> +
>>>>>     &uart0 {
>>>>>         pinctrl-names = "default";
>>>>>         pinctrl-0 = <&uart0_pins>;
>>>>