Re: [PATCH v4 12/19] arm64: dts: mediatek: mt8192-asurada: Couple VGPU and VSRAM_OTHER regulators

[Chen-Yu Tsai]

On Tue, Mar 7, 2023 at 5:30 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
>
> Il 07/03/23 10:24, Chen-Yu Tsai ha scritto:
> > On Fri, Mar 3, 2023 at 12:09 PM Chen-Yu Tsai <wenst@xxxxxxxxxxxx> wrote:
> >>
> >> On Thu, Mar 2, 2023 at 6:17 PM AngeloGioacchino Del Regno
> >> <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
> >>>
> >>> Il 02/03/23 11:03, Chen-Yu Tsai ha scritto:
> >>>> On Wed, Mar 1, 2023 at 5:55 PM AngeloGioacchino Del Regno
> >>>> <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
> >>>>>
> >>>>> Add coupling for these regulators, as VSRAM_OTHER is used to power the
> >>>>> GPU SRAM, and they have a strict voltage output relation to satisfy in
> >>>>> order to ensure GPU stable operation.
> >>>>> While at it, also add voltage constraint overrides for the GPU SRAM
> >>>>> regulator "mt6359_vsram_others" so that we stay in a safe range of
> >>>>> 0.75-0.80V.
> >>>>>
> >>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> >>>>> ---
> >>>>>    arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi | 9 +++++++++
> >>>>>    1 file changed, 9 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
> >>>>> index 8570b78c04a4..f858eca219d7 100644
> >>>>> --- a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
> >>>>> +++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
> >>>>> @@ -447,6 +447,13 @@ &mt6359_vrf12_ldo_reg {
> >>>>>           regulator-always-on;
> >>>>>    };
> >>>>>
> >>>>> +&mt6359_vsram_others_ldo_reg {
> >>>>> +       regulator-min-microvolt = <750000>;
> >>>>> +       regulator-max-microvolt = <800000>;
> >>>>> +       regulator-coupled-with = <&mt6315_7_vbuck1>;
> >>>>> +       regulator-coupled-max-spread = <10000>;
> >>>>
> >>>> Looking again at the downstream OPP table, it seems there's no voltage
> >>>> difference requirement. It only needs V_SRAM >= V_GPU. Same applies to
> >>>> MT8195. Looks like only MT8183 and MT8186 need V_SRAM - V_GPU >= 10000.
> >>>
> >>> On MT8195 we don't need any regulator coupling. There, the GPU-SRAM voltage
> >>> is fixed at .. I don't remember, 0.7V? - anyway - MT8195 doesn't need to
> >>> scale the vsram.
> >>
> >> Looks like it's fixed at 0.75V. I guess we're Ok on MT8195.
> >>
> >>>>
> >>>> Would setting max-spread to 0 work? I ask because with both regulator's
> >>>> maximum voltage set to 0.8V, there's no way we can reach the highest
> >>>> OPP.
> >>>>
> >>>
> >>> No that doesn't work. I can raise the Vgpu max voltage to 0.88V to solve the
> >>> issue right here and right now, or we can leave it like that and revisit it
> >>> later.
> >>>
> >>> I would at this point go for setting mt6315_7_vbuck1's max-microvolt to
> >>> 880000, as this is the maximum recommended voltage for the GPU as per the
> >>> MT8192 datasheet, it would also make sense as we would be still describing
> >>> the hardware in a correct manner.
> >>>
> >>> What do you think?
> >>
> >> If it's just to accommodate the coupler stuff, I say just set the maximum
> >> at the lowest possible setting that satisfies the coupler constraint and
> >> granularity of the regulator. The regulator does 6250 uV steps, so I guess
> >> we could set the maximum at 812500 uV, with a comment stating the nominal
> >> voltage of 800000 uV and that the extra 12500 uV is to workaround coupler
> >> limitations.
> >>
> >> Does that sound OK?
> >
> > Even without changing anything, the coupler seems to work OK:
> >
> >   vsram_others                     1    1      0  normal   800mV
> > 0mA   750mV   800mV
> >      10006000.syscon:power-controller-domain   1
> >           0mA     0mV     0mV
> >   Vgpu                             2    2      0  normal   800mV
> > 0mA   606mV   800mV
> >      13000000.gpu-mali             1
> > 0mA   800mV   800mV
> >      10006000.syscon:power-controller-domain   1
> >           0mA     0mV     0mV
> >
> > Am I missing something?
> >
>
> I don't think you are... I may be getting confused by all of the changesets
> that I'm pushing at once.
>
> Hence, is this commit fine as it is?

It works for some reason. Maybe it's a bug in the coupler. Either way I
think it works, even though the numbers might be a bit off. We can revisit
it later.

Reviewed-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>