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

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

On 7/9/26 11:27, Chen-Yu Tsai wrote:
> On Thu, Jul 9, 2026 at 4:30 PM <sashiko-bot@kernel.org> wrote:
>>
>> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>> - [High] The MT6319 PMIC fallback compatible triggers incorrect MT6315_PP mode ganging in the regulator driver, causing silent cross-talk between the CPU and EMI VDDQ regulators.
>> - [Low] Grammatical error in the comment explaining the firmware workaround.
>> --
>>
>> --- Patch [1]: [PATCH v2] arm64: dts: mediatek: mt8188-geralt: Add MT6319 PMIC ---
>> commit 0ff94c6622559a57199ed1f2d1f4063c6c911df1
>> Author: Chen-Yu Tsai <wenst@chromium.org>
>>
>> arm64: dts: mediatek: mt8188-geralt: Add MT6319 PMIC
>>
>> The Geralt design uses a MT6319 PMIC to power the big cores and LPDDR4X
>> DRAM.
>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi b/arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi
>>> index 8e423504ec052..af05315092f53 100644
>>> --- a/arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8188-geralt.dtsi
>> [ ... ]
>>> @@ -1225,6 +1235,70 @@ &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>;
>>
>> [Severity: High]
>> Does this fallback compatible trigger incorrect mode ganging?
>>
>> The mt6315-regulator driver maps USID 6 to the MT6315_PP variant, which
>> hardcodes mode ganging for vbuck1, vbuck2, and vbuck4. On the MT8188
>> Geralt's MT6319, vbuck4 is wired as an independent regulator for
>> pp0600_emi_vddq.
>>
>> If a mode change happens on vbuck1 (which is allowed by the
>> regulator-allowed-modes property), the driver will apply the ganged
>> mask and silently overwrite vbuck4's mode bits in the PMIC, potentially
>> forcing the RAM supply into an unexpected mode.
>>
>> Could this silent cross-talk between the CPU and EMI VDDQ regulators
>> lead to memory voltage instability?
> 
> This discrepency is actually bigger than Sashiko mentions. The ganging
> supported by the driver only applies to the PMICs in the Asuarada design:
> 
>    - SPMI ID 6 has 1+2 (GPU); 3 and 4 are unused
>    - SPMI ID 7 has 1+2+4 (big cores) and 3 (little cores)
> 
> On the Cherry design we have:
> 
>    - SPMI ID 6 has 1+2 (GPU); 3 and 4 are unused
>    - SPMI ID 7 has 1+2+3+4 (big cores)
> 
> On the Corsola designs with beefier chips:
> 
>    - SPMI ID 6 has 1+2 (big cores); 3 and 4 are unused
> 
> On the Geralt design (this one):
> 
>    - SPMI ID 7 has 1+2 (big cores); 3 (DRAM); 4 (DRAM)
> 
> Any ideas on what to do about this mess?

wenst, I think that the only way to fix that properly is to add generic support
for buck phases ganging in the regulator API... because AFAIK there's nothing
right now.

 From what I remember from last time I looked, MediaTek PMICs wouldn't be the
only ones, there's something else that also has ganged bucks, and again if I
remember correctly, I remember seeing one platform data struct for each of the
ganging combinations in that driver.

So.. especially because MTK wouldn't be the first, I think the addition of some
kind of generic properties (and callbacks) for this kind of configuration in
the API would be both profitable and welcome.

Mark, any considerations?

Cheers,
Angelo

> 
> Another thing is that I kind of wonder why the mode configuration register
> setting isn't ganged in hardware like the enable and voltage settings.
> 
> 
> Thanks
> ChenYu
> 
>>> +             pvdd1-supply = <&pp4200_s5>;
>>> +             pvdd2-supply = <&pp4200_s5>;
>>> +             pvdd3-supply = <&pp4200_s5>;
>>> +             pvdd4-supply = <&pp4200_s5>;
>>> +
>>> +             regulators {
>>> +                     /*
>>> +                      * vbuck1 is the cpu-supply for CPU6 and CPU7.
>>> +                      *
>>> +                      * However, this device originally shipped with a broken
>>> +                      * firmware that causes register reads over SPMI fail.
>>> +                      * This is fixed in firmware version 15842.175.0, which
>>> +                      * is included in ChromeOS releases R150-16700.22.0 or
>>> +                      * R151-16721.0.0 and later.
>>> +                      *
>>> +                      * Assigning the cpu-supply properties for CPU6 and CPU7
>>> +                      * without the fix will likely cause CPU DVFS to stop
>>> +                      * working. Hence the assignment is left out to avoid a
>>> +                      * regression of the function. If the user is confident
>>> +                      * that their system has the fix, they can added the
>>
>> [Severity: Low]
>> This isn't a bug, but there is a typo in this comment. It should say
>> "they can add the property themselves".
>>
>>> +                      * property themselves.
>>> +                      */
>>
>> --
>> Sashiko AI review · https://sashiko.dev/#/patchset/20260709081928.612294-1-wenst@chromium.org?part=1