[PATCH] regulator: palmas: set supply_name after registering the regulator

[PATCH] regulator: palmas: set supply_name after registering the regulator

[H. Nikolaus Schaller]

Commit 98e48cd9283d ("regulator: core: resolve supply for boot-on/always-on regulators")

introduced a new rule which makes Palmas regulator registration fail:

[    5.407712] ldo1: supplied by vsys_cobra
[    5.412748] ldo2: supplied by vsys_cobra
[    5.417603] palmas-pmic 48070000.i2c:palmas@48:palmas_pmic: failed to register 48070000.i2c:palmas@48:palmas_pmic regulator

This seems to block additions initializations and finally the
Pyra-Handheld hangs when trying to access MMC because there is
no mmc-supply available.

Fixes: 98e48cd9283d ("regulator: core: resolve supply for boot-on/always-on regulators")
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/regulator/palmas-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/palmas-regulator.c b/drivers/regulator/palmas-regulator.c
index 337dd614695e..b49cc05f847f 100644
--- a/drivers/regulator/palmas-regulator.c
+++ b/drivers/regulator/palmas-regulator.c
@@ -975,7 +975,6 @@ static int palmas_ldo_registration(struct palmas_pmic *pmic,
 		else
 			config.init_data = NULL;
 
-		desc->supply_name = rinfo->sname;
 		config.of_node = ddata->palmas_matches[id].of_node;
 
 		rdev = devm_regulator_register(pmic->dev, desc, &config);
@@ -986,6 +985,7 @@ static int palmas_ldo_registration(struct palmas_pmic *pmic,
 			return PTR_ERR(rdev);
 		}
 
+		desc->supply_name = rinfo->sname;
 		/* Initialise sleep/init values from platform data */
 		if (pdata) {
 			reg_init = pdata->reg_init[id];
-- 
2.31.1

Re: [PATCH] regulator: palmas: set supply_name after registering the regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 958 bytes --]

On Tue, Jun 29, 2021 at 05:24:03PM +0200, H. Nikolaus Schaller wrote:
> Commit 98e48cd9283d ("regulator: core: resolve supply for boot-on/always-on regulators")
> 
> introduced a new rule which makes Palmas regulator registration fail:
> 
> [    5.407712] ldo1: supplied by vsys_cobra
> [    5.412748] ldo2: supplied by vsys_cobra
> [    5.417603] palmas-pmic 48070000.i2c:palmas@48:palmas_pmic: failed to register 48070000.i2c:palmas@48:palmas_pmic regulator
> 
> This seems to block additions initializations and finally the
> Pyra-Handheld hangs when trying to access MMC because there is
> no mmc-supply available.

What is that rule and how is this patch intended to ensure that Palmas
meets it?  As covered in submitting-patches.rst your changelog should
explain this so that in review we can verify that this is a good fix.
The change itself looks worrying like it just shuts the error up and
could cause problems for other systems.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: palmas: set supply_name after registering the regulator

[H. Nikolaus Schaller]

Hi Mark,

> Am 29.06.2021 um 17:59 schrieb Mark Brown <broonie@kernel.org>:
> 
> On Tue, Jun 29, 2021 at 05:24:03PM +0200, H. Nikolaus Schaller wrote:
>> Commit 98e48cd9283d ("regulator: core: resolve supply for boot-on/always-on regulators")
>> 
>> introduced a new rule which makes Palmas regulator registration fail:
>> 
>> [    5.407712] ldo1: supplied by vsys_cobra
>> [    5.412748] ldo2: supplied by vsys_cobra
>> [    5.417603] palmas-pmic 48070000.i2c:palmas@48:palmas_pmic: failed to register 48070000.i2c:palmas@48:palmas_pmic regulator
>> 
>> This seems to block additions initializations and finally the
>> Pyra-Handheld hangs when trying to access MMC because there is
>> no mmc-supply available.
> 
> What is that rule and how is this patch intended to ensure that Palmas
> meets it?
>  As covered in submitting-patches.rst your changelog should
> explain this so that in review we can verify that this is a good fix.

I am very sorry, but I simply believed that it is not necessary to copy&paste or
describe this because it appears not to be difficult to retrieve.

Commit 98e48cd9283d ("regulator: core: resolve supply for boot-on/always-on regulators")

    regulator: core: resolve supply for boot-on/always-on regulators
    
    For the boot-on/always-on regulators the set_machine_constrainst() is
    called before resolving rdev->supply. Thus the code would try to enable
    rdev before enabling supplying regulator. Enforce resolving supply
    regulator before enabling rdev.

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index f192bf19492e..e20e77e4c159 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1425,6 +1425,12 @@ static int set_machine_constraints(struct regulator_dev *rdev)
         * and we have control then make sure it is enabled.
         */
        if (rdev->constraints->always_on || rdev->constraints->boot_on) {
+               /* If we want to enable this regulator, make sure that we know
+                * the supplying regulator.
+                */
+               if (rdev->supply_name && !rdev->supply)
+                       return -EPROBE_DEFER;
+
                if (rdev->supply) {
                        ret = regulator_enable(rdev->supply);
                        if (ret < 0) {

This rule (rdev->supply_name && !rdev->supply) did not exist before 98e48cd9283d
and it seems to return early with EPROBE_DEFER if there is a desc->supply_name defined,
but no supply resolved.

The Palmas driver is setting desc->supply_name to some string constant (i.e. not NULL)
and is then calling devm_regulator_register().

So it was working fine without having the supplying regulator resolved. AFAIK they
just serve as fixed regulators in the device tree and have no physical equivalent.

My proposal just moves setting the supply_name behind devm_regulator_register() and
by that restores the old behaviour.

Well, unless...

... devm_regulator_register() does something differently if desc->supply_name
is not set before and changed afterwards. It may miss that change.

I did not observe any indications, but something could happen in the guts of
devm_regulator_register(). If it is, then my approach is clearly faulty.

There may be a better solution where I hope more reviewers (especially
those more familiar with the Palmas driver than me) can comment on.

> The change itself looks worrying like it just shuts the error up and
> could cause problems for other systems.


Since this change is confined to the Palmas driver and results in the same
behaviour as it had before 98e48cd9283d did break it, it should not harm
other systems.

So I hope for guidance if my approach is good or needs a different solution.

BR and thanks,
Nikolaus Schaller

Re: [PATCH] regulator: palmas: set supply_name after registering the regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2668 bytes --]

On Tue, Jun 29, 2021 at 08:34:55PM +0200, H. Nikolaus Schaller wrote:
> > Am 29.06.2021 um 17:59 schrieb Mark Brown <broonie@kernel.org>:

> > What is that rule and how is this patch intended to ensure that Palmas
> > meets it?
> >  As covered in submitting-patches.rst your changelog should
> > explain this so that in review we can verify that this is a good fix.

> I am very sorry, but I simply believed that it is not necessary to copy&paste or
> describe this because it appears not to be difficult to retrieve.

So, I did actually look at the commit but I couldn't figure out what the
change was supposed to do about it.

> This rule (rdev->supply_name && !rdev->supply) did not exist before 98e48cd9283d
> and it seems to return early with EPROBE_DEFER if there is a desc->supply_name defined,
> but no supply resolved.

> The Palmas driver is setting desc->supply_name to some string constant (i.e. not NULL)
> and is then calling devm_regulator_register().

Right, this is how a regualtor driver should specify the name of its
supply.

> So it was working fine without having the supplying regulator resolved. AFAIK they
> just serve as fixed regulators in the device tree and have no physical equivalent.

No, not at all - it's representing whatever provides input power to the
regulator.  There may be no physical control of it at runtime on your
system but that may not be true on other systems.  It's quite common for
there to be a chain of regulators (eg, DCDCs supplying LDOs) and then
they all need to get get power managed appropriately and you don't end
up thinking a regulator is enabled when the input regulator is disabled.  

> My proposal just moves setting the supply_name behind devm_regulator_register() and
> by that restores the old behaviour.

This means that we won't actually map the supply and any system that
relies on software handling the supply regulator will be broken.

> Well, unless...

> ... devm_regulator_register() does something differently if desc->supply_name
> is not set before and changed afterwards. It may miss that change.

We resolve supplies during regulator registration, this would
effectively just skip mapping of the supply.

> So I hope for guidance if my approach is good or needs a different solution.

What I would expect to happen here would be that once vsys_cobra is
registered the regulators supplied by it can register and then all their
consumers would in turn be able to register.  You should look into why
that supply regulator isn't appearing and resolve that, or if a consumer
isn't handling deferral then that would need to be addressed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: palmas: set supply_name after registering the regulator

[H. Nikolaus Schaller]

Hi Mark,

> Am 29.06.2021 um 20:56 schrieb Mark Brown <broonie@kernel.org>:
> 
> On Tue, Jun 29, 2021 at 08:34:55PM +0200, H. Nikolaus Schaller wrote:
>>> Am 29.06.2021 um 17:59 schrieb Mark Brown <broonie@kernel.org>:
> 
> 
>> So it was working fine without having the supplying regulator resolved. AFAIK they
>> just serve as fixed regulators in the device tree and have no physical equivalent.
> 
> No, not at all - it's representing whatever provides input power to the
> regulator.  There may be no physical control of it at runtime on your
> system but that may not be true on other systems.  It's quite common for
> there to be a chain of regulators (eg, DCDCs supplying LDOs) and then
> they all need to get get power managed appropriately and you don't end
> up thinking a regulator is enabled when the input regulator is disabled.  

Yes, that is how it is chained in other cases.

> 
>> My proposal just moves setting the supply_name behind devm_regulator_register() and
>> by that restores the old behaviour.
> 
> This means that we won't actually map the supply and any system that
> relies on software handling the supply regulator will be broken.

Well, it seems as if the supply regulators are only vsys_cobra and vdds_1v8_main.
At least in omap5-board-common.dtsi which is what I can test.

Both are fixed regulators where calling enable or not doesn't become
physically visible. The hardware still supplies the 5V and 1.8V to the palmas
chip.

Maybe the new rule by commit 98e48cd9283dreveals a design issue inside of
the Palmas driver which assumes that there is no need to control its supply
regulators. And does not handle probe deferral.

Then of course my patch is just a work-around for a bug but not a solution.

> 
>> Well, unless...
> 
>> ... devm_regulator_register() does something differently if desc->supply_name
>> is not set before and changed afterwards. It may miss that change.
> 
> We resolve supplies during regulator registration, this would
> effectively just skip mapping of the supply.
> 
>> So I hope for guidance if my approach is good or needs a different solution.
> 
> What I would expect to happen here would be that once vsys_cobra is
> registered the regulators supplied by it can register and then all their
> consumers would in turn be able to register.  You should look into why
> that supply regulator isn't appearing and resolve that, or if a consumer
> isn't handling deferral then that would need to be addressed.

Ah, I think I get an idea what may be going wrong.

There seems to be a deadlock in probing:

	e.g. ldo3_reg depends on vdds_1v8_main supply
	vdds_1v8_main depends on smps7_reg supply
	smps7_reg depends on vsys_cobra supply
	vsys_cobra depends on nothing

This would normally lead to a simple chain as you described above. But
ldo3_reg and smps7_reg share the same driver and can probe successfully or
fail only in common.

Now if ldo3_reg defers probe through the new rule, smps7_reg is never
probed successfully because it is the same driver. Hence vdds_1v8_main can
not become available. And the Palmas continues to run in boot initialization
until some driver (MMC) wants to switch voltages or whatever.

Maybe Tony or Graeme has an idea how to do it right...

BR and thanks,
Nikolaus

Re: [PATCH] regulator: palmas: set supply_name after registering the regulator

[Graeme Gregory]

On Tue, 29 Jun 2021, at 9:21 PM, H. Nikolaus Schaller wrote:
> Hi Mark,
> 
> > Am 29.06.2021 um 20:56 schrieb Mark Brown <broonie@kernel.org>:
> > 
> > On Tue, Jun 29, 2021 at 08:34:55PM +0200, H. Nikolaus Schaller wrote:
> >>> Am 29.06.2021 um 17:59 schrieb Mark Brown <broonie@kernel.org>:
> > 
> > 
> >> So it was working fine without having the supplying regulator resolved. AFAIK they
> >> just serve as fixed regulators in the device tree and have no physical equivalent.
> > 
> > No, not at all - it's representing whatever provides input power to the
> > regulator.  There may be no physical control of it at runtime on your
> > system but that may not be true on other systems.  It's quite common for
> > there to be a chain of regulators (eg, DCDCs supplying LDOs) and then
> > they all need to get get power managed appropriately and you don't end
> > up thinking a regulator is enabled when the input regulator is disabled.  
> 
> Yes, that is how it is chained in other cases.
> 
> > 
> >> My proposal just moves setting the supply_name behind devm_regulator_register() and
> >> by that restores the old behaviour.
> > 
> > This means that we won't actually map the supply and any system that
> > relies on software handling the supply regulator will be broken.
> 
> Well, it seems as if the supply regulators are only vsys_cobra and 
> vdds_1v8_main.
> At least in omap5-board-common.dtsi which is what I can test.
> 
> Both are fixed regulators where calling enable or not doesn't become
> physically visible. The hardware still supplies the 5V and 1.8V to the palmas
> chip.
> 
> Maybe the new rule by commit 98e48cd9283dreveals a design issue inside of
> the Palmas driver which assumes that there is no need to control its supply
> regulators. And does not handle probe deferral.
> 
> Then of course my patch is just a work-around for a bug but not a solution.
> 
> > 
> >> Well, unless...
> > 
> >> ... devm_regulator_register() does something differently if desc->supply_name
> >> is not set before and changed afterwards. It may miss that change.
> > 
> > We resolve supplies during regulator registration, this would
> > effectively just skip mapping of the supply.
> > 
> >> So I hope for guidance if my approach is good or needs a different solution.
> > 
> > What I would expect to happen here would be that once vsys_cobra is
> > registered the regulators supplied by it can register and then all their
> > consumers would in turn be able to register.  You should look into why
> > that supply regulator isn't appearing and resolve that, or if a consumer
> > isn't handling deferral then that would need to be addressed.
> 
> Ah, I think I get an idea what may be going wrong.
> 
> There seems to be a deadlock in probing:
> 
> 	e.g. ldo3_reg depends on vdds_1v8_main supply
> 	vdds_1v8_main depends on smps7_reg supply
> 	smps7_reg depends on vsys_cobra supply
> 	vsys_cobra depends on nothing
> 
> This would normally lead to a simple chain as you described above. But
> ldo3_reg and smps7_reg share the same driver and can probe successfully or
> fail only in common.
> 
> Now if ldo3_reg defers probe through the new rule, smps7_reg is never
> probed successfully because it is the same driver. Hence vdds_1v8_main can
> not become available. And the Palmas continues to run in boot initialization
> until some driver (MMC) wants to switch voltages or whatever.
> 
> Maybe Tony or Graeme has an idea how to do it right...
> 
Sorry after almost 10 years I have forgotten all about this driver.

Graeme

Re: [PATCH] regulator: palmas: set supply_name after registering the regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

On Tue, Jun 29, 2021 at 10:21:45PM +0200, H. Nikolaus Schaller wrote:

> There seems to be a deadlock in probing:

> 	e.g. ldo3_reg depends on vdds_1v8_main supply
> 	vdds_1v8_main depends on smps7_reg supply
> 	smps7_reg depends on vsys_cobra supply
> 	vsys_cobra depends on nothing

> This would normally lead to a simple chain as you described above. But
> ldo3_reg and smps7_reg share the same driver and can probe successfully or
> fail only in common.

I don't see any deadlock there?  Just a straightforward set of
dependencies.  Anything circular would clearly be a driver bug.

> Now if ldo3_reg defers probe through the new rule, smps7_reg is never
> probed successfully because it is the same driver. Hence vdds_1v8_main can
> not become available. And the Palmas continues to run in boot initialization
> until some driver (MMC) wants to switch voltages or whatever.

The driver should just register all the DCDCs before the LDOs, then
everything will sort itself out.  It's *possible* you might see a system
trying to link things together regulators of the same type but that's
very unusual as it makes the system less efficient.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: palmas: set supply_name after registering the regulator

[H. Nikolaus Schaller]

> Am 30.06.2021 um 14:13 schrieb Mark Brown <broonie@kernel.org>:
> 
> On Tue, Jun 29, 2021 at 10:21:45PM +0200, H. Nikolaus Schaller wrote:
> 
>> There seems to be a deadlock in probing:
> 
>> 	e.g. ldo3_reg depends on vdds_1v8_main supply
>> 	vdds_1v8_main depends on smps7_reg supply
>> 	smps7_reg depends on vsys_cobra supply
>> 	vsys_cobra depends on nothing
> 
>> This would normally lead to a simple chain as you described above. But
>> ldo3_reg and smps7_reg share the same driver and can probe successfully or
>> fail only in common.
> 
> I don't see any deadlock there?  Just a straightforward set of
> dependencies.  Anything circular would clearly be a driver bug.

I think it could be indirectly circular since ldo3_reg does not find smps3
registered. But I have to run more tests with printk inserted.

So I am not sure if that is the issue but the best hypothesis I currently know of.

> 
>> Now if ldo3_reg defers probe through the new rule, smps7_reg is never
>> probed successfully because it is the same driver. Hence vdds_1v8_main can
>> not become available. And the Palmas continues to run in boot initialization
>> until some driver (MMC) wants to switch voltages or whatever.
> 
> The driver should just register all the DCDCs before the LDOs, then
> everything will sort itself out.

Basically the driver code does it that way. But fails. Probing seems to defer
until deferral limits (AFAIR there is a timer or counter in the probe deferral
queue) an does not succeed.

It only works if I disable the new condition - but apparently it suffices
to do so for the LDOs because I noticed that the smps initialization also
defines supply_name and there it is no problem to have it before
devm_regulator_register(). Every simple problem becomes complex if you look
deeper :)

Which I interpret (without additional tests) that the smps7_reg is registered
but does not pass the (new) test when asked by ldo3_reg.

>  It's *possible* you might see a system
> trying to link things together regulators of the same type but that's
> very unusual as it makes the system less efficient.

I can check that - and there may be other LDOs in the Palmas
chained (as described by device tree).

It will need some test time to get more factual information about this
issue besides that reverting the new rule or adding a trick to make
the Palmas driver initialize properly again.

BR and thanks,
Nikolaus

Re: [PATCH] regulator: palmas: set supply_name after registering the regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

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@kernel.org>:

> >> 	e.g. ldo3_reg depends on vdds_1v8_main supply
> >> 	vdds_1v8_main depends on smps7_reg supply
> >> 	smps7_reg depends on vsys_cobra supply
> >> 	vsys_cobra depends on nothing

> > I don't see any deadlock there?  Just a straightforward set of
> > dependencies.  Anything circular would clearly be a driver bug.

> I think it could be indirectly circular since ldo3_reg does not find smps3
> registered. But I have to run more tests with printk inserted.

Why would LDO3 have a dependency on SPMS3 given what's written above and
how would that be circular?

> > The driver should just register all the DCDCs before the LDOs, then
> > everything will sort itself out.

> Basically the driver code does it that way. But fails. Probing seems to defer
> until deferral limits (AFAIR there is a timer or counter in the probe deferral
> queue) an does not succeed.

Ah, I see - the issue is the intervening 1.8V regulator.  That's not a
circularity, that's the callout to a separate device in the middle of
the chain.  It's a super weird hardware design if the DT is accurate,
it's hard to see how it's not going to be hurting efficiency.  In any
case simplest thing would be to have separate MFD subdevices in Palmas
for the LDOs and DCDCs, that'll do the right thing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: palmas: set supply_name after registering the regulator

[H. Nikolaus Schaller]

Hi Mark,

> Am 30.06.2021 um 15:04 schrieb Mark Brown <broonie@kernel.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@kernel.org>:
> 
>> I think it could be indirectly circular since ldo3_reg does not find smps3
>> registered. But I have to run more tests with printk inserted.
> 
> Why would LDO3 have a dependency on SPMS3 given what's written above and
> how would that be circular?

because both can only probe successfully in common or not at all. If either
fails the other is rewound.

> 
>>> The driver should just register all the DCDCs before the LDOs, then
>>> everything will sort itself out.
> 
>> Basically the driver code does it that way. But fails. Probing seems to defer
>> until deferral limits (AFAIR there is a timer or counter in the probe deferral
>> queue) an does not succeed.
> 
> Ah, I see - the issue is the intervening 1.8V regulator.  That's not a
> circularity, that's the callout to a separate device in the middle of
> the chain.

Ok, "circular" is maybe the wrong word. It is an unexpected dependency...

>  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.

Or to separate DT node names defined by the power controller (smps1-5)
from their useage on the board (vmain, vsys, vdds_1v8, vmmcsd, ...).

And, vdds_1v8_main is the only fixed-regulator used as a wrapper.
Others have either no vin-supply or are real regulators with a control gpio.

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".

Therefore, a completely different approach could be to remove fixedregulator-vdds_1v8_main
and replace by smps7_reg.

I tried with

	#define vdds_1v8_main smps7_reg

and it compiles and boots successfully.

There are still messages from the new rule for supply_name && !supply, but this time
Palmas gets initialized (maybe the -EPROBE_DEFER is silently ignored somewhere).

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)?

> 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.

>  In any
> case simplest thing would be to have separate MFD subdevices in Palmas
> for the LDOs and DCDCs, that'll do the right thing.

Attached is some logging an additional rdev_info() if the new rule
in set_machine_constraints() if it triggers and returns -EPROBE_DEFER.

We can see that several regulators trigger on the condition but indeed
ldo3 fails (which I so far only deduced from DT as the potential disturbance).

So we know several ways to get the hardware running again:
* revert 98e48cd9283d
* my hack to set supply_name later (makes the new rule ignored but may have side-effects)
* modify DTS to make vdds_1v8_main == smps7_reg
* (untested) make SMPS and LDOs separate subdevices in Palmas drivers

@Tony, your comments are needed...

Maybe you didn't notice since you may have configured Palmas into the kernel
which changes probe sequence.

BR and thanks,
Nikolaus

[    2.017857] palmas-rtc 48070000.i2c:palmas@48:rtc: registered as rtc0
[    2.026122] palmas-rtc 48070000.i2c:palmas@48:rtc: setting system clock to 2000-01-01T00:00:00 UTC (946684800)
[    2.041192] smps123: supply_name: smps1-in supply: 00000000
[    2.047112] smps123: supplied by regulator-dummy
[    2.054376] smps45: supply_name: smps4-in supply: 00000000
[    2.060240] smps45: supplied by regulator-dummy
[    2.067193] smps6: supply_name: smps6-in supply: 00000000
[    2.072909] smps6: supplied by vsys_cobra
[    2.079644] smps7: supply_name: smps7-in supply: 00000000
[    2.085346] smps7: supplied by vsys_cobra
[    2.092034] smps8: supply_name: smps8-in supply: 00000000
[    2.097735] smps8: supplied by vsys_cobra
[    2.105096] smps9: Bringing 0uV into 2100000-2100000uV
[    2.113152] smps9: supplied by vsys_cobra
[    2.118035] smps10_out2: supply_name: smps10-in supply: 00000000
[    2.124429] smps10_out2: supplied by regulator-dummy
[    2.133172] smps10_out1: supplied by regulator-dummy
[    2.138787] ldo1: Bringing 0uV into 1800000-1800000uV
[    2.145928] ldo1: supplied by vsys_cobra
[    2.150994] ldo2: supplied by vsys_cobra
[    2.155633] ldo3: supply_name: ldo3-in supply: 00000000
[    2.161436] palmas-pmic 48070000.i2c:palmas@48:palmas_pmic: failed to register 48070000.i2c:palmas@48:palmas_pmic regulator
[    2.178796] omap_hsmmc 480ad000.mmc: allocated mmc-pwrseq
[    2.186431] mmc_pwrseq_simple_set_gpios_value: value=1
[    2.187507] smps123: supply_name: smps1-in supply: 00000000
[    2.197895] smps123: supplied by regulator-dummy
[    2.204583] smps45: supply_name: smps4-in supply: 00000000
[    2.210473] smps45: supplied by regulator-dummy
[    2.217019] smps6: supply_name: smps6-in supply: 00000000
[    2.223173] smps6: supplied by vsys_cobra
[    2.228906] smps7: supply_name: smps7-in supply: 00000000
[    2.234673] smps7: supplied by vsys_cobra
[    2.240930] smps8: supply_name: smps8-in supply: 00000000
[    2.246631] smps8: supplied by vsys_cobra
[    2.252745] smps9: Bringing 0uV into 2100000-2100000uV
[    2.259318] smps9: supplied by vsys_cobra
[    2.264469] smps10_out2: supply_name: smps10-in supply: 00000000
[    2.270932] smps10_out2: supplied by regulator-dummy
[    2.278222] smps10_out1: supplied by regulator-dummy
[    2.284324] ldo1: supplied by vsys_cobra
[    2.289422] ldo2: supplied by vsys_cobra
[    2.294270] ldo3: supply_name: ldo3-in supply: 00000000
[    2.299859] palmas-pmic 48070000.i2c:palmas@48:palmas_pmic: failed to register 48070000.i2c:palmas@48:palmas_pmic regulator
[    2.299905] mmc_pwrseq_simple_set_gpios_value: value=0
[    2.318572] input: user-buttons as /devices/platform/user-buttons/input/input0
[    2.329003] smps123: supply_name: smps1-in supply: 00000000
[    2.334916] input: pyra-game-buttons as /devices/platform/pyra-game-buttons/input/input1
[    2.336655] input: pyra-lid-wakeup as /devices/platform/pyra-lid-wakeup/input/input2
[    2.352845] smps123: supplied by regulator-dummy
[    2.359670] l3main2_cm:clk:0010:0: failed to disable
[    2.370659] l4sec_cm:clk:0038:0: failed to disable
[    2.376292] ALSA device list:
[    2.376739] smps45: supply_name: smps4-in supply: 00000000
[    2.379521]   No soundcards found.
[    2.389296] smps45: supplied by regulator-dummy
[    2.396203] smps6: supply_name: smps6-in supply: 00000000
[    2.401981] smps6: supplied by vsys_cobra
[    2.406732] omap_hsmmc 480ad000.mmc: card claims to support voltages below defined range
[    2.416790] smps7: supply_name: smps7-in supply: 00000000
[    2.422604] smps7: supplied by vsys_cobra
[    2.428674] smps8: supply_name: smps8-in supply: 00000000
[    2.434472] smps8: supplied by vsys_cobra
[    2.440728] smps9: Bringing 0uV into 2100000-2100000uV
[    2.447350] smps9: supplied by vsys_cobra
[    2.452508] smps10_out2: supply_name: smps10-in supply: 00000000
[    2.458917] smps10_out2: supplied by regulator-dummy
[    2.467645] mmc2: new high speed SDIO card at address 0001
[    2.474295] mmc_pwrseq_simple_set_gpios_value: value=1
[    2.480100] smps10_out1: supplied by regulator-dummy
[    2.486272] ldo1: supplied by vsys_cobra
[    2.492500] ldo2: supplied by vsys_cobra
[    2.497155] ldo3: supply_name: ldo3-in supply: 00000000
[    2.502709] palmas-pmic 48070000.i2c:palmas@48:palmas_pmic: failed to register 48070000.i2c:palmas@48:palmas_pmic regulator
[    2.522668] smps123: supply_name: smps1-in supply: 00000000
[    2.528588] smps123: supplied by regulator-dummy
[    2.535669] smps45: supply_name: smps4-in supply: 00000000
[    2.541711] smps45: supplied by regulator-dummy
[    2.548638] smps6: supply_name: smps6-in supply: 00000000
[    2.554498] smps6: supplied by vsys_cobra
[    2.560245] smps7: supply_name: smps7-in supply: 00000000
[    2.565968] smps7: supplied by vsys_cobra
[    2.572198] smps8: supply_name: smps8-in supply: 00000000
[    2.577897] smps8: supplied by vsys_cobra
[    2.584466] smps9: Bringing 0uV into 2100000-2100000uV
[    2.591631] smps9: supplied by vsys_cobra
[    2.596520] smps10_out2: supply_name: smps10-in supply: 00000000
[    2.603112] smps10_out2: supplied by regulator-dummy
[    2.610365] smps10_out1: supplied by regulator-dummy
[    2.616175] ldo1: supplied by vsys_cobra
[    2.621193] ldo2: supplied by vsys_cobra
[    2.625845] ldo3: supply_name: ldo3-in supply: 00000000
[    2.631402] palmas-pmic 48070000.i2c:palmas@48:palmas_pmic: failed to register 48070000.i2c:palmas@48:palmas_pmic regulator
[    2.646447] Waiting for root device PARTUUID=7c769003-02..

Re: [PATCH] regulator: palmas: set supply_name after registering the regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

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@kernel.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@kernel.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.

> 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.

> 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.

> > 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regulator: palmas: set supply_name after registering the regulator

[H. Nikolaus Schaller]

Hi Mark,

> Am 30.06.2021 um 18:45 schrieb Mark Brown <broonie@kernel.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@kernel.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@kernel.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

Re: [PATCH] regulator: palmas: set supply_name after registering the regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

On Wed, Jun 30, 2021 at 07:17:28PM +0200, H. Nikolaus Schaller wrote:

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

No, it doesn't.  There's absolutely no need for any specific mapping
between Linux devices and compatible strings or nodes in the DT, we can
create any number of Linux devices for any number of compatibles - just
look at MFDs (where we create multiple Linux devices for a single DT
compatible string) or system devices (where we create Linux devices with
potentially no node in the device tree).

> 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?

For deferred probe to be guaranteed to work we really should have one
regulator per Linux device but in practice that is overhead and effort
that almost never buys us anything in practical systems (I can't
emphasize strongly enough how unusual chains of more than two regulators
are) so we don't enforce doing that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]