linux-kernel - Re: [PATCH] regulator: palmas: set supply

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: <54FC87BA-D45A-4ABF-B233-2A70B4A4A632@goldelico.com>
Date:   Wed, 30 Jun 2021 19:17:28 +0200
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     Tony Lindgren <tony@...mide.com>,
        Graeme Gregory <gg@...mlogic.co.uk>,
        Liam Girdwood <lgirdwood@...il.com>,
        Nishanth Menon <nm@...com>,
        Linux-OMAP <linux-omap@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Discussions about the Letux Kernel 
        <letux-kernel@...nphoenux.org>, kernel@...a-handheld.com,
        Peter Ujfalusi <peter.ujfalusi@...il.com>
Subject: Re: [PATCH] regulator: palmas: set supply_name after registering the
 regulator

Hi Mark,

> Am 30.06.2021 um 18:45 schrieb Mark Brown <broonie@...nel.org>:
> 
> On Wed, Jun 30, 2021 at 04:43:14PM +0200, H. Nikolaus Schaller wrote:
>>> Am 30.06.2021 um 15:04 schrieb Mark Brown <broonie@...nel.org>:
>>> On Wed, Jun 30, 2021 at 02:29:02PM +0200, H. Nikolaus Schaller wrote:
>>>>> Am 30.06.2021 um 14:13 schrieb Mark Brown <broonie@...nel.org>:
> 
>>> It's a super weird hardware design if the DT is accurate,
> 
>> I get the impression that the vdds_1v8_main is in the DT (omap5-board-common.dtsi)
>> only as an alias for smps7. Maybe to get more flexibility in overwriting
>> in board files? I.e. replace the power controller without having a fixed
>> definition of smps7 elsewhere.
> 
> It doesn't seem to have any effect in software and the input is
> specified at the same voltage as the output which would be very unusual.
> No idea why you'd do any aliasing, you can already name the regulators
> with DT handles and with user visible strings.
> 
>> Looking into the schematics of the OMAP5432EVM or the Pyra handheld does
>> not reveal a physical regulator. It is just that the output signal of
>> smps7 is called "VDDS_1v8_MAIN".
> 
> It could be something incorrectly factored out of some early prototypes
> or something.

Yes, most likely. Or as I assumed: separating regulator names from signal names.

According to git blame it was introduced 5 years ago by Nisanth.
Maybe he is still reading here and wants to comment.

> 
>> Therefore, a completely different approach could be to remove fixedregulator-vdds_1v8_main
>> and replace by smps7_reg.
> 
> If there's no physical regulator on the board then that is indeed a DT
> bug, the fixed regulator just shouldn't be there.

Agreed.

> 
>> But is changing the DT the right solution if the Palmas and Fixed regulator
>> drivers can't handle the untouched DT which is logically correct (not physically)?
> 
> Well, it's a good thing to do anyway since the DT is supposed to
> accurately reflect the hardware.  Like I say splitting the LDOs and
> SMPSs can also be done independently and should separately resolve the
> issue.

According to the code they are done separately by calling smps_register
first and then ldo_register inside palmas_regulators_probe().

But separate regulators or regulator blocks could probe independently.

Other similar drivers seem to split registration into many
individual regulators, e.g. twl6030-regulator.c while others
seem to do it more like the Palmas, e.g. tps65090-regulator.c

Splitting into many regulators also needs to touch the device trees
to have individual compatible entries which currently do not exist.

On the other hand, a theoretical system could have a real fixed regulator
in between (maybe a power switch?) and should still work. Why should 
driver core care about that case and not the core system it is using?

> 
>>> it's hard to see how it's not going to be hurting efficiency.
> 
>> Well, I think the regulators are enabled only once during boot so nobody
>> notices an issue.
> 
> When I say having an extra regulator in there hurts efficiency I'm
> saying that the power losses from regulation will be increased as
> there's more of it happening.

Ah, you did think about hardware efficiency, I did about software efficiency :)

Well, a real "LDO" from 1.8V to 1.8V makes no sense but a power
interrupt switch (with e.g. 12 mΩ RDSon like some ADP197) could in some
circuits, e.g. to reduce reverse leakage or whatever.

We don't have it of course but it would be modeled as a fixed regulator
with GPIO and 1.8V output although input is also 1.8V.

But that doesn't help for the Palmas issue.

Generally, I'd now prefer the "DTS" fix approach and leaving to fix the
intermediate fixed-regulator solution by rewriting the Palmas driver
to future discussion.

BR and thanks,
Nikolaus