linux-kernel - Re: [PATCH] soc: mediatek: mediatek-regulator-coupler: Fix comment

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAC=S1niSXkCTLXeCv-0sQh4AQmv6eKm8OH_3yH=TeDoMVz72OA@mail.gmail.com>
Date: Wed, 23 Oct 2024 20:03:01 +0800
From: Fei Shao <fshao@...omium.org>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: Matthias Brugger <matthias.bgg@...il.com>, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH] soc: mediatek: mediatek-regulator-coupler: Fix comment

On Wed, Oct 23, 2024 at 6:40 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com> wrote:
>
> Il 23/10/24 12:19, Fei Shao ha scritto:
> > Fix two minor issues in the comments.
> >
> > 1. We balance VSRAM voltage based on the target VGPU voltage, so the
> >     comment likely refers to VGPU.
>
> Function `mediatek_regulator_balance_voltage()` refers, as stated in the comment
> located at the top of its signature, to "GPU<->SRAM" voltages relationships.
>
> So, we're taking into consideration only two regulators:
>                    VGPU and VSRAM
>
> The first comment says:
> "If we're asked to set a voltage (implicit: to VGPU) less than VSRAM min_uV[...]"
>
> ...so, I think that you've misunderstood what the comment says :-)

Let me make sure we're on the same page - VGPU never goes higher than
VSRAM (VGPU <= VSRAM), is that correct?

[ min VGPU, max VGPU ] ... spread ... [    min VSRAM  ,  max VSRAM    ]
 (min_uV)  (max_uV)                         (vsram_min_uV) (vsram_max_uV)

The longer comment is
"If we're asked to set a voltage less than VSRAM min_uV, set the
minimum allowed voltage on VSRAM, ..."
So VSRAM is the subject here I think? Because we "set voltage *on*
VSRAM" based on its own minimum allowed voltage? We never set either
VGPU min/max voltage to vsram_min_uV?
And it's attached to the line that decides vsram_target_min_uV, with
the maximum of (1) vsram_min_uV (i.e. minimum allowed VSRAM voltage)
or (2) min_uV + max_spread (min VGPU + spread)

That makes me believe VGPU is the correct candidate.
We manually configure VSRAM first (push the ceiling to higher), and
let the regulator core update VGPU afterwards.

In fact, IIUC, there should be other concerns here... it should be
"vsram_target_min_uV = max(vsram_min_uV, **max_uV** +
**min_spread**)", due to the same fact that VGPU <= VSRAM.

For max_uV, it's not causing any issues because min_uV == max_uV since
there's only one consumer... I can update the code in v2 if you agree
with this.

For min_spread part, the reason is that VSRAM should be somewhat ahead
of VGPU, but not way too far, i.e. VGPU + min_spread <= VSRAM <= VGPU
+ max_spread.
This is required on MT8183 and MT8186 - the spread should be at
between 0.1V to 0.25V (VGPU + 0.1V <= VSRAM <= VGPU + 0.25V).
The problem is that we don't have a "regulator-coupled-min-spread"
property, so we use the -max-spread property for minimum spread... and
without considering the real maximum spread.
But this is probably not that terrible also, currently
vsram_target_max_uV never goes too far from VGPU.

>
> > 2. .attach_regulator() returns 0 on success and 1 if the regulator is
> >     not suitable. The context suggests a successful return value (0).
>
> The comment is on top of a "refuse" or "error" case - one that wants to return 1
> and not zero.
>
> Besides, it clearly states:
> "The regulator core will keep walking through the list of couplers when any
>   .attach_regulator() callback returns 1"
>
> ...which is definitely true.

... I'm sorry, I guess I really need a dinner break - my brain
translated "when" as "until" so I read it the opposite way.
You and the original comment are both correct.

And I hope my first point still makes sense and it's not my brain
being lacking energy again...

Regards,
Fei

>
> drivers/regulator/core.c
> function `regulator_find_coupler()`:
>
>         list_for_each_entry_reverse(coupler, &regulator_coupler_list, list) {
>                 err = coupler->attach_regulator(coupler, rdev);
>                 [.....]
>                 if (err < 0)
>                         return ERR_PTR(err);
>
>                 if (err == 1)
>                         continue;
>
>                 break;
>         }
>
> Is that clear now?
>
> Cheers,
> Angelo