Since commit 1fc12b05895e ("regulator: core: Avoid propagating to
supplies when possible") regulators marked with boot-on can't be
disabled anymore because the commit handles always-on and boot-on
regulators the same way.
Now commit 05f224ca6693 ("regulator: core: Clean enabling always-on
regulators + their supplies") changed the regulator_resolve_supply()
logic a bit by using 'use_count'. So we can't just skip the
'use_count++' during set_machine_constraints(). The easiest way I found
is to correct the 'use_count' just before returning the rdev device
during regulator_register().
Signed-off-by: Marco Felsch <[email protected]>
---
drivers/regulator/core.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e0c0cf462004..f9444f509440 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5170,6 +5170,11 @@ regulator_register(const struct regulator_desc *regulator_desc,
/* try to resolve regulators supply since a new one was registered */
class_for_each_device(®ulator_class, NULL, NULL,
regulator_register_resolve_supply);
+
+ /* cleanup use_count -> boot-on marked regulators can be disabled */
+ if (rdev->constraints->boot_on && !rdev->constraints->always_on)
+ rdev->use_count--;
+
kfree(config);
return rdev;
--
2.20.1
Hi,
On Tue, Sep 17, 2019 at 8:40 AM Marco Felsch <[email protected]> wrote:
>
> Since commit 1fc12b05895e ("regulator: core: Avoid propagating to
> supplies when possible") regulators marked with boot-on can't be
> disabled anymore because the commit handles always-on and boot-on
> regulators the same way.
>
> Now commit 05f224ca6693 ("regulator: core: Clean enabling always-on
> regulators + their supplies") changed the regulator_resolve_supply()
> logic a bit by using 'use_count'. So we can't just skip the
> 'use_count++' during set_machine_constraints(). The easiest way I found
> is to correct the 'use_count' just before returning the rdev device
> during regulator_register().
>
> Signed-off-by: Marco Felsch <[email protected]>
> ---
> drivers/regulator/core.c | 5 +++++
> 1 file changed, 5 insertions(+)
I will freely admit my ignorance here, but I've always been slightly
confused by the "always-on" vs. "boot-on" distinction...
The bindings say:
regulator-always-on:
description: boolean, regulator should never be disabled
regulator-boot-on:
description: bootloader/firmware enabled regulator
For 'boot-on' that's a bit ambiguous about what it means. The
constraints have a bit more details:
* @always_on: Set if the regulator should never be disabled.
* @boot_on: Set if the regulator is enabled when the system is initially
* started. If the regulator is not enabled by the hardware or
* bootloader then it will be enabled when the constraints are
* applied.
What I would take from that is that "boot_on" means that we expect
that the firmware probably turned this regulator on but if it didn't
then the kernel should turn it on (and presumably leave it on?). That
would mean that your patch is incorrect, I think?
...but then that begs the question of why we have two attributes?
Maybe this has already been discussed before and someone can point me
to a previous discussion? We should probably make it more clear in
the bindings and/or the constraints.
===
NOTE: I'm curious why you even have the "regulator-boot-on" attribute
in your case to begin with. Why not leave it off?
-Doug
On Mon, Sep 23, 2019 at 11:36:11AM -0700, Doug Anderson wrote:
> On Mon, Sep 23, 2019 at 11:14 AM Mark Brown <[email protected]> wrote:
> > Boot on means that it's powered on when the kernel starts, it's
> > for regulators that we can't read back the status of.
> 1. Would it be valid to say that it's always incorrect to set this
> property if there is a way to read the status back from the regulator?
As originally intended, yes. I'm now not 100% sure that it won't
break any existing systems though :/
> 2. Would this be a valid description of how the property is expected to behave
> a) At early boot this regulator will be turned on if it wasn't already on.
> b) If no clients are found for this regulator after everything has
> loaded, this regulator will be automatically disabled.
> If so then I don't _think_ #2b is happening, but I haven't confirmed.
> > boot-on just refers to the status at boot, we can still turn
> > those regulators off later on if we want to.
> How, exactly? As of my commit 5451781dadf8 ("regulator: core: Only
> count load for enabled consumers") if you do:
> r = regulator_get(...)
> regulator_disable(r)
> ...then you'll get "Underflow of regulator enable count". In other
> words, if a given regulator client disables more times than it enables
> then you will get an error. Since there is no client that did the
> initial "boot" enable then there's no way to do the disable unless it
> happens automatically (as per 2b above).
It should be possible to do a regulator_disable() though I'm not
sure anyone actually uses that. The pattern for a regular
consumer should be the normal enable/disable pair to handle
shared usage, only an exclusive consumer should be able to use
just a straight disable.
> ...or do you mean that people could call regulator_force_disable()?
> Couldn't they also do that with an always-on regulator?
No, nothing should use that in a non-emergency situation.
Hi,
On Mon, Sep 23, 2019 at 11:14 AM Mark Brown <[email protected]> wrote:
>
> On Mon, Sep 23, 2019 at 11:02:26AM -0700, Doug Anderson wrote:
>
> > I will freely admit my ignorance here, but I've always been slightly
> > confused by the "always-on" vs. "boot-on" distinction...
>
> > The bindings say:
>
> > regulator-always-on:
> > description: boolean, regulator should never be disabled
>
> > regulator-boot-on:
> > description: bootloader/firmware enabled regulator
>
> > For 'boot-on' that's a bit ambiguous about what it means. The
> > constraints have a bit more details:
>
> Boot on means that it's powered on when the kernel starts, it's
> for regulators that we can't read back the status of.
1. Would it be valid to say that it's always incorrect to set this
property if there is a way to read the status back from the regulator?
2. Would this be a valid description of how the property is expected to behave
a) At early boot this regulator will be turned on if it wasn't already on.
b) If no clients are found for this regulator after everything has
loaded, this regulator will be automatically disabled.
If so then I don't _think_ #2b is happening, but I haven't confirmed.
> > ...but then that begs the question of why we have two attributes?
> > Maybe this has already been discussed before and someone can point me
> > to a previous discussion? We should probably make it more clear in
> > the bindings and/or the constraints.
>
> boot-on just refers to the status at boot, we can still turn
> those regulators off later on if we want to.
How, exactly? As of my commit 5451781dadf8 ("regulator: core: Only
count load for enabled consumers") if you do:
r = regulator_get(...)
regulator_disable(r)
...then you'll get "Underflow of regulator enable count". In other
words, if a given regulator client disables more times than it enables
then you will get an error. Since there is no client that did the
initial "boot" enable then there's no way to do the disable unless it
happens automatically (as per 2b above).
...or do you mean that people could call regulator_force_disable()?
Couldn't they also do that with an always-on regulator?
-Doug
On Mon, Sep 23, 2019 at 11:02:26AM -0700, Doug Anderson wrote:
> I will freely admit my ignorance here, but I've always been slightly
> confused by the "always-on" vs. "boot-on" distinction...
> The bindings say:
> regulator-always-on:
> description: boolean, regulator should never be disabled
> regulator-boot-on:
> description: bootloader/firmware enabled regulator
> For 'boot-on' that's a bit ambiguous about what it means. The
> constraints have a bit more details:
Boot on means that it's powered on when the kernel starts, it's
for regulators that we can't read back the status of.
> ...but then that begs the question of why we have two attributes?
> Maybe this has already been discussed before and someone can point me
> to a previous discussion? We should probably make it more clear in
> the bindings and/or the constraints.
boot-on just refers to the status at boot, we can still turn
those regulators off later on if we want to.
Hi,
On Mon, Sep 23, 2019 at 11:49 AM Mark Brown <[email protected]> wrote:
>
> On Mon, Sep 23, 2019 at 11:36:11AM -0700, Doug Anderson wrote:
> > On Mon, Sep 23, 2019 at 11:14 AM Mark Brown <[email protected]> wrote:
>
> > > Boot on means that it's powered on when the kernel starts, it's
> > > for regulators that we can't read back the status of.
>
> > 1. Would it be valid to say that it's always incorrect to set this
> > property if there is a way to read the status back from the regulator?
>
> As originally intended, yes. I'm now not 100% sure that it won't
> break any existing systems though :/
Should I change the bindings doc to say that?
> > 2. Would this be a valid description of how the property is expected to behave
> > a) At early boot this regulator will be turned on if it wasn't already on.
> > b) If no clients are found for this regulator after everything has
> > loaded, this regulator will be automatically disabled.
>
> > If so then I don't _think_ #2b is happening, but I haven't confirmed.
>
> > > boot-on just refers to the status at boot, we can still turn
> > > those regulators off later on if we want to.
>
> > How, exactly? As of my commit 5451781dadf8 ("regulator: core: Only
> > count load for enabled consumers") if you do:
>
> > r = regulator_get(...)
> > regulator_disable(r)
>
> > ...then you'll get "Underflow of regulator enable count". In other
> > words, if a given regulator client disables more times than it enables
> > then you will get an error. Since there is no client that did the
> > initial "boot" enable then there's no way to do the disable unless it
> > happens automatically (as per 2b above).
>
> It should be possible to do a regulator_disable() though I'm not
> sure anyone actually uses that. The pattern for a regular
> consumer should be the normal enable/disable pair to handle
> shared usage, only an exclusive consumer should be able to use
> just a straight disable.
Ah, I see, I wasn't aware of the "exclusive" special case! Marco: is
this working for you? I wonder if we need to match
"regulator->enable_count" to "rdev->use_count" at the end of
_regulator_get() in the exclusive case...
-Doug
On Mon, Sep 23, 2019 at 03:40:09PM -0700, Doug Anderson wrote:
> On Mon, Sep 23, 2019 at 11:49 AM Mark Brown <[email protected]> wrote:
> > On Mon, Sep 23, 2019 at 11:36:11AM -0700, Doug Anderson wrote:
> > > 1. Would it be valid to say that it's always incorrect to set this
> > > property if there is a way to read the status back from the regulator?
> > As originally intended, yes. I'm now not 100% sure that it won't
> > break any existing systems though :/
> Should I change the bindings doc to say that?
Sure.
> > It should be possible to do a regulator_disable() though I'm not
> > sure anyone actually uses that. The pattern for a regular
> > consumer should be the normal enable/disable pair to handle
> > shared usage, only an exclusive consumer should be able to use
> > just a straight disable.
> Ah, I see, I wasn't aware of the "exclusive" special case! Marco: is
> this working for you? I wonder if we need to match
> "regulator->enable_count" to "rdev->use_count" at the end of
> _regulator_get() in the exclusive case...
Yes, I think that case has been missed when adding the enable
counts - I've never actually had a system myself that made any
use of this stuff. It probably needs an audit of the users to
make sure nobody's relying on the current behaviour though I
can't think how they would.
Hi,
On Tue, Sep 24, 2019 at 11:28 AM Mark Brown <[email protected]> wrote:
> On Mon, Sep 23, 2019 at 03:40:09PM -0700, Doug Anderson wrote:
> > On Mon, Sep 23, 2019 at 11:49 AM Mark Brown <[email protected]> wrote:
> > > On Mon, Sep 23, 2019 at 11:36:11AM -0700, Doug Anderson wrote:
>
> > > > 1. Would it be valid to say that it's always incorrect to set this
> > > > property if there is a way to read the status back from the regulator?
>
> > > As originally intended, yes. I'm now not 100% sure that it won't
> > > break any existing systems though :/
>
> > Should I change the bindings doc to say that?
>
> Sure.
Sent ("regulator: Document "regulator-boot-on" binding more thoroughly").
https://lore.kernel.org/r/20190926124115.1.Ice34ad5970a375c3c03cb15c3859b3ee501561bf@changeid
> > > It should be possible to do a regulator_disable() though I'm not
> > > sure anyone actually uses that. The pattern for a regular
> > > consumer should be the normal enable/disable pair to handle
> > > shared usage, only an exclusive consumer should be able to use
> > > just a straight disable.
>
> > Ah, I see, I wasn't aware of the "exclusive" special case! Marco: is
> > this working for you? I wonder if we need to match
> > "regulator->enable_count" to "rdev->use_count" at the end of
> > _regulator_get() in the exclusive case...
>
> Yes, I think that case has been missed when adding the enable
> counts - I've never actually had a system myself that made any
> use of this stuff. It probably needs an audit of the users to
> make sure nobody's relying on the current behaviour though I
> can't think how they would.
Marco: I'm going to assume you'll tackle this since I don't actually
have any use cases that need this.
-Doug
Hi Doug, Mark,
sorry for the delay..
On 19-09-26 12:44, Doug Anderson wrote:
> Hi,
>
> On Tue, Sep 24, 2019 at 11:28 AM Mark Brown <[email protected]> wrote:
> > On Mon, Sep 23, 2019 at 03:40:09PM -0700, Doug Anderson wrote:
> > > On Mon, Sep 23, 2019 at 11:49 AM Mark Brown <[email protected]> wrote:
> > > > On Mon, Sep 23, 2019 at 11:36:11AM -0700, Doug Anderson wrote:
> >
> > > > > 1. Would it be valid to say that it's always incorrect to set this
> > > > > property if there is a way to read the status back from the regulator?
> >
> > > > As originally intended, yes. I'm now not 100% sure that it won't
> > > > break any existing systems though :/
> >
> > > Should I change the bindings doc to say that?
> >
> > Sure.
>
> Sent ("regulator: Document "regulator-boot-on" binding more thoroughly").
>
> https://lore.kernel.org/r/20190926124115.1.Ice34ad5970a375c3c03cb15c3859b3ee501561bf@changeid
Yes, I saw it and thanks for it =)
> > > > It should be possible to do a regulator_disable() though I'm not
> > > > sure anyone actually uses that. The pattern for a regular
> > > > consumer should be the normal enable/disable pair to handle
> > > > shared usage, only an exclusive consumer should be able to use
> > > > just a straight disable.
In my case it is a regulator-fixed which uses the enable/disable pair.
But as my descriptions says this will not work currently because boot-on
marked regulators can't be disabled right now (using the same logic as
always-on regulators).
> > > Ah, I see, I wasn't aware of the "exclusive" special case! Marco: is
> > > this working for you? I wonder if we need to match
> > > "regulator->enable_count" to "rdev->use_count" at the end of
> > > _regulator_get() in the exclusive case...
So my fix isn't correct to fix this in general?
> > Yes, I think that case has been missed when adding the enable
> > counts - I've never actually had a system myself that made any
> > use of this stuff. It probably needs an audit of the users to
> > make sure nobody's relying on the current behaviour though I
> > can't think how they would.
>
> Marco: I'm going to assume you'll tackle this since I don't actually
> have any use cases that need this.
My use case is a simple regulator-fixed which is turned on by the
bootloader or to be more precise by the pmic-rom. To map that correctly
I marked this regulator as boot-on. Unfortunately as I pointed out above
this is handeld the same way as always-on.
Regards,
Marco
> -Doug
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi,
On Fri, Sep 27, 2019 at 1:47 AM Marco Felsch <[email protected]> wrote:
> > > > > It should be possible to do a regulator_disable() though I'm not
> > > > > sure anyone actually uses that. The pattern for a regular
> > > > > consumer should be the normal enable/disable pair to handle
> > > > > shared usage, only an exclusive consumer should be able to use
> > > > > just a straight disable.
>
> In my case it is a regulator-fixed which uses the enable/disable pair.
> But as my descriptions says this will not work currently because boot-on
> marked regulators can't be disabled right now (using the same logic as
> always-on regulators).
>
> > > > Ah, I see, I wasn't aware of the "exclusive" special case! Marco: is
> > > > this working for you? I wonder if we need to match
> > > > "regulator->enable_count" to "rdev->use_count" at the end of
> > > > _regulator_get() in the exclusive case...
>
> So my fix isn't correct to fix this in general?
I don't think your fix is correct. It sounds as if the intention of
"regulator-boot-on" is to have the OS turn the regulator on at bootup
and it keep an implicit reference until someone explicitly tells the
OS to drop the reference.
> > > Yes, I think that case has been missed when adding the enable
> > > counts - I've never actually had a system myself that made any
> > > use of this stuff. It probably needs an audit of the users to
> > > make sure nobody's relying on the current behaviour though I
> > > can't think how they would.
> >
> > Marco: I'm going to assume you'll tackle this since I don't actually
> > have any use cases that need this.
>
> My use case is a simple regulator-fixed which is turned on by the
> bootloader or to be more precise by the pmic-rom. To map that correctly
> I marked this regulator as boot-on. Unfortunately as I pointed out above
> this is handeld the same way as always-on.
It's a fixed regulator controlled by a GPIO? Presumably the GPIO can
be read. That would mean it ideally shouldn't be using
"regulator-boot-on" since this is _not_ a regulator whose software
state can't be read. Just remove the property.
-Doug
Hi dee Ho Peeps,
Long time no hear =)
On Tue, Oct 01, 2019 at 12:57:31PM -0700, Doug Anderson wrote:
> Hi,
>
> On Fri, Sep 27, 2019 at 1:47 AM Marco Felsch <[email protected]> wrote:
> > > > > > It should be possible to do a regulator_disable() though I'm not
> > > > > > sure anyone actually uses that. The pattern for a regular
> > > > > > consumer should be the normal enable/disable pair to handle
> > > > > > shared usage, only an exclusive consumer should be able to use
> > > > > > just a straight disable.
> >
> > In my case it is a regulator-fixed which uses the enable/disable pair.
> > But as my descriptions says this will not work currently because boot-on
> > marked regulators can't be disabled right now (using the same logic as
> > always-on regulators).
> >
I was developing driver for yet-another ROHM PMIC when I hit the
phenomena you have been discussing here (I think) :) I used regulator-boot-on
flag from DT in my test setup and then did a test consumer who does
regulator_get()
regulator_enable()
regulator_disable() pair.
As this 'test consumer' was only user for the regulator I expected the
regulator to be disabled after call to regulator_disable. But it was
not.
It seems to me that the use_count is incremented for boot-on regulators
before first call to regulator_enable(). So when the consumer does first
regulator_enable() the use_count will actually go to 2. Hence the
corresponding regulator_disable() won't actually disable the regulator
even though the consumer is actually only known user.
I did unbalanced regulator_disable() - which does disable the regulator
but it also spills the warning.
I did instrument the regmap helpers and regulator_enable/disable to
dump out the actual i2c accesses and use_counts. Regulator enable prints
use_count _before_ incrementing it.
Check enable state after regulator_get (calls regulator_is_enabled)
root@arm:/sys/kernel/mva_test/regulators# cat buck3_en
[ 123.251499] dbg_regulator_is_enabled_regmap: called for 'buck3'
[ 123.257524] regulator_is_enabled_regmap_dbg: Reading reg 0x1c
[ 123.267386] regulator_is_enabled_regmap_dbg: read succeeded, val 0xe
Enable regulator by test consumer (no i2c access as regulator is on)
1root@arm:/sys/kernel/mva_test/regulators# echo 1 > buck3_en
[ 171.438524] Calling regulator_enable
[ 171.446324] Enable requested, use-count 1
/* disable regulator by consumer */
root@arm:/sys/kernel/mva_test/regulators# echo 0 > buck3_en
[ 187.799956] Calling regulator_disable
[ 187.805935] regulator disable requested, use-count 2, always-on 0
/* Unbalanced disble */
root@arm:/sys/kernel/mva_test/regulators# echo 0 > buck3_en
[ 207.832682] Calling regulator_disable
[ 207.842949] regulator disable requested, use-count 1, always-on 0
[ 207.849237] regulator do disable
[ 207.852502] dbg_regulator_disable_regmap: called for 'buck3'
[ 207.858272] regulator_disable_regmap_dbg: reg 0x1c mask 0x8 val 0x0, masked_val 0x0
[ 207.909942] buck3: Underflow of regulator enable count
[ 207.915189] Failed to toggle regulator state. error(-22)
bash: echo: write error: Invalid argument
root@arm:/sys/kernel/mva_test/regulators#
> > > > > Ah, I see, I wasn't aware of the "exclusive" special case! Marco: is
> > > > > this working for you? I wonder if we need to match
> > > > > "regulator->enable_count" to "rdev->use_count" at the end of
> > > > > _regulator_get() in the exclusive case...
> >
> > So my fix isn't correct to fix this in general?
>
> I don't think your fix is correct. It sounds as if the intention of
> "regulator-boot-on" is to have the OS turn the regulator on at bootup
> and it keep an implicit reference until someone explicitly tells the
> OS to drop the reference.
Hmm.. What is the intended way to explicitly tell the OS to drop the
reference? I would assume we should still use same logic as with other
regulators - if last user calls regulator_disable() we should disable
the regulator? (I may not understand all this well enough though)
> > > > Yes, I think that case has been missed when adding the enable
> > > > counts - I've never actually had a system myself that made any
> > > > use of this stuff. It probably needs an audit of the users to
> > > > make sure nobody's relying on the current behaviour though I
> > > > can't think how they would.
> > >
> > > Marco: I'm going to assume you'll tackle this since I don't actually
> > > have any use cases that need this.
> >
> > My use case is a simple regulator-fixed which is turned on by the
> > bootloader or to be more precise by the pmic-rom. To map that correctly
> > I marked this regulator as boot-on. Unfortunately as I pointed out above
> > this is handeld the same way as always-on.
Here I am again just a man in the middle as I am "only a component vendor"
and lack of complete system information. But I _think_ some of the users
of BD71827 and BD71847 PMICs do use setup where regulator-boot-on is
used to enable certain BUCKs to power some graphics chip at start-up. At
later stage it should be possible to cut the power in order to do power
saving or decrease heating when graphichs are not needed. So I think it
would be nice to fix this somehow.
> It's a fixed regulator controlled by a GPIO? Presumably the GPIO can
> be read. That would mean it ideally shouldn't be using
> "regulator-boot-on" since this is _not_ a regulator whose software
> state can't be read. Just remove the property.
How should we handle cases where we want OS to enable regulator at
boot-up - possibly before consumer drivers can be load?
Br,
Matti Vaittinen
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
On Fri, Oct 04, 2019 at 09:34:43AM +0300, Matti Vaittinen wrote:
> On Tue, Oct 01, 2019 at 12:57:31PM -0700, Doug Anderson wrote:
> > I don't think your fix is correct. It sounds as if the intention of
> > "regulator-boot-on" is to have the OS turn the regulator on at bootup
> > and it keep an implicit reference until someone explicitly tells the
> > OS to drop the reference.
> Hmm.. What is the intended way to explicitly tell the OS to drop the
> reference? I would assume we should still use same logic as with other
> regulators - if last user calls regulator_disable() we should disable
> the regulator? (I may not understand all this well enough though)
Yes.
> > It's a fixed regulator controlled by a GPIO? Presumably the GPIO can
> > be read. That would mean it ideally shouldn't be using
> > "regulator-boot-on" since this is _not_ a regulator whose software
> > state can't be read. Just remove the property.
> How should we handle cases where we want OS to enable regulator at
> boot-up - possibly before consumer drivers can be load?
If you want the regulator to be on without any driver present then mark
it always-on. If you want the regulator to be enabled prior to the
driver being loaded then the expectation is that the bootloader will do
that, it's difficult to see what the benefit there is from having the
kernel enable things when it starts prior to having a driver unless the
intent is to keep the regulator always on.
On Fri, 2019-10-04 at 12:32 +0100, Mark Brown wrote:
> On Fri, Oct 04, 2019 at 09:34:43AM +0300, Matti Vaittinen wrote:
> > On Tue, Oct 01, 2019 at 12:57:31PM -0700, Doug Anderson wrote:
> > > I don't think your fix is correct. It sounds as if the intention
> > > of
> > > "regulator-boot-on" is to have the OS turn the regulator on at
> > > bootup
> > > and it keep an implicit reference until someone explicitly tells
> > > the
> > > OS to drop the reference.
> > Hmm.. What is the intended way to explicitly tell the OS to drop
> > the
> > reference? I would assume we should still use same logic as with
> > other
> > regulators - if last user calls regulator_disable() we should
> > disable
> > the regulator? (I may not understand all this well enough though)
>
> Yes.
>
> > > It's a fixed regulator controlled by a GPIO? Presumably the GPIO
> > > can
> > > be read. That would mean it ideally shouldn't be using
> > > "regulator-boot-on" since this is _not_ a regulator whose
> > > software
> > > state can't be read. Just remove the property.
> > How should we handle cases where we want OS to enable regulator at
> > boot-up - possibly before consumer drivers can be load?
>
> If you want the regulator to be on without any driver present then
> mark
> it always-on. If you want the regulator to be enabled prior to the
> driver being loaded then the expectation is that the bootloader will
> do
> that, it's difficult to see what the benefit there is from having the
> kernel enable things when it starts prior to having a driver unless
> the
> intent is to keep the regulator always on.
I thought the regulator-boot-on could have been used for that. But as I
said - I don't really know all this so well =) And no, I am not opposed
to offloading this from kernel to boot, I was just trying to learn what
is the correct thing to do (tm). Thanks for educating me on this :) I
will suggest adding the enabling to boot code if (when) I get questions
concerning this. (always-on won't do for regulators which need to be
controlled for power saving or heating issues).
Br,
Matti Vaittinen
On Fri, Oct 04, 2019 at 12:03:12PM +0000, Vaittinen, Matti wrote:
> On Fri, 2019-10-04 at 12:32 +0100, Mark Brown wrote:
> > If you want the regulator to be on without any driver present then
> > mark
> > it always-on. If you want the regulator to be enabled prior to the
> > driver being loaded then the expectation is that the bootloader will
> > do
> > that, it's difficult to see what the benefit there is from having the
> > kernel enable things when it starts prior to having a driver unless
> > the
> > intent is to keep the regulator always on.
> I thought the regulator-boot-on could have been used for that. But as I
> said - I don't really know all this so well =) And no, I am not opposed
> to offloading this from kernel to boot, I was just trying to learn what
> is the correct thing to do (tm). Thanks for educating me on this :) I
> will suggest adding the enabling to boot code if (when) I get questions
> concerning this. (always-on won't do for regulators which need to be
> controlled for power saving or heating issues).
If you want the kernel to do it early on without the bootloader then I
think we really need to understand the use case. My guess would be that
the underlying request would be to get the driver up earlier which is
something we should be better at but often easier said than done.
Hi Doug, Mark,
On 19-10-01 12:57, Doug Anderson wrote:
> Hi,
>
> On Fri, Sep 27, 2019 at 1:47 AM Marco Felsch <[email protected]> wrote:
> > > > > > It should be possible to do a regulator_disable() though I'm not
> > > > > > sure anyone actually uses that. The pattern for a regular
> > > > > > consumer should be the normal enable/disable pair to handle
> > > > > > shared usage, only an exclusive consumer should be able to use
> > > > > > just a straight disable.
> >
> > In my case it is a regulator-fixed which uses the enable/disable pair.
> > But as my descriptions says this will not work currently because boot-on
> > marked regulators can't be disabled right now (using the same logic as
> > always-on regulators).
> >
> > > > > Ah, I see, I wasn't aware of the "exclusive" special case! Marco: is
> > > > > this working for you? I wonder if we need to match
> > > > > "regulator->enable_count" to "rdev->use_count" at the end of
> > > > > _regulator_get() in the exclusive case...
> >
> > So my fix isn't correct to fix this in general?
>
> I don't think your fix is correct. It sounds as if the intention of
> "regulator-boot-on" is to have the OS turn the regulator on at bootup
> and it keep an implicit reference until someone explicitly tells the
> OS to drop the reference.
>
>
> > > > Yes, I think that case has been missed when adding the enable
> > > > counts - I've never actually had a system myself that made any
> > > > use of this stuff. It probably needs an audit of the users to
> > > > make sure nobody's relying on the current behaviour though I
> > > > can't think how they would.
> > >
> > > Marco: I'm going to assume you'll tackle this since I don't actually
> > > have any use cases that need this.
> >
> > My use case is a simple regulator-fixed which is turned on by the
> > bootloader or to be more precise by the pmic-rom. To map that correctly
> > I marked this regulator as boot-on. Unfortunately as I pointed out above
> > this is handeld the same way as always-on.
>
> It's a fixed regulator controlled by a GPIO? Presumably the GPIO can
> be read. That would mean it ideally shouldn't be using
> "regulator-boot-on" since this is _not_ a regulator whose software
> state can't be read. Just remove the property.
Sorry that won't fix my problem. If I drop the regulator-boot-on state
the fixed-regulator will disable this regulator but disable/enable this
regulator is only valid during suspend/resume. I don't say that my fix
is correct but we should fix this.
Regards,
Marco
> -Doug
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Mon, Oct 07, 2019 at 11:34:29AM +0200, Marco Felsch wrote:
> Sorry that won't fix my problem. If I drop the regulator-boot-on state
> the fixed-regulator will disable this regulator but disable/enable this
> regulator is only valid during suspend/resume. I don't say that my fix
> is correct but we should fix this.
I'm having a bit of trouble parsing this but it sounds like you want the
regulator to be always on in which case you should use the property
specifically for that.
On 19-10-07 19:29, Mark Brown wrote:
> On Mon, Oct 07, 2019 at 11:34:29AM +0200, Marco Felsch wrote:
>
> > Sorry that won't fix my problem. If I drop the regulator-boot-on state
> > the fixed-regulator will disable this regulator but disable/enable this
> > regulator is only valid during suspend/resume. I don't say that my fix
> > is correct but we should fix this.
>
> I'm having a bit of trouble parsing this but it sounds like you want the
> regulator to be always on in which case you should use the property
> specifically for that.
Sorry my english wasn't the best.. Imagine this case: The bootloader
turned the display on to show an early bootlogo. Now if I miss the
regulator-boot-on property the display is turned off and on. The turn
off comes from the regulator probe, the turn on comes from the cosumer.
Is that assumption correct?
Regards,
Marco
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Tue, Oct 08, 2019 at 08:03:11AM +0200, Marco Felsch wrote:
> On 19-10-07 19:29, Mark Brown wrote:
> > On Mon, Oct 07, 2019 at 11:34:29AM +0200, Marco Felsch wrote:
> > > Sorry that won't fix my problem. If I drop the regulator-boot-on state
> > > the fixed-regulator will disable this regulator but disable/enable this
> > > regulator is only valid during suspend/resume. I don't say that my fix
> > > is correct but we should fix this.
> > I'm having a bit of trouble parsing this but it sounds like you want the
> > regulator to be always on in which case you should use the property
> > specifically for that.
> Sorry my english wasn't the best.. Imagine this case: The bootloader
> turned the display on to show an early bootlogo. Now if I miss the
> regulator-boot-on property the display is turned off and on. The turn
> off comes from the regulator probe, the turn on comes from the cosumer.
> Is that assumption correct?
No, we shouldn't do anything when the regulator probes - we'll only
disable unused regulators when we get to the end of boot (currently we
delay this by 30s to give userspace a chance to run, that's a hack but
we're fresh out of better ideas). During boot the regulator state will
only be changed if some consumer appears and changes the state.
On 19-10-08 13:51, Mark Brown wrote:
> On Tue, Oct 08, 2019 at 08:03:11AM +0200, Marco Felsch wrote:
> > On 19-10-07 19:29, Mark Brown wrote:
> > > On Mon, Oct 07, 2019 at 11:34:29AM +0200, Marco Felsch wrote:
>
> > > > Sorry that won't fix my problem. If I drop the regulator-boot-on state
> > > > the fixed-regulator will disable this regulator but disable/enable this
> > > > regulator is only valid during suspend/resume. I don't say that my fix
> > > > is correct but we should fix this.
>
> > > I'm having a bit of trouble parsing this but it sounds like you want the
> > > regulator to be always on in which case you should use the property
> > > specifically for that.
>
> > Sorry my english wasn't the best.. Imagine this case: The bootloader
> > turned the display on to show an early bootlogo. Now if I miss the
> > regulator-boot-on property the display is turned off and on. The turn
> > off comes from the regulator probe, the turn on comes from the cosumer.
> > Is that assumption correct?
>
> No, we shouldn't do anything when the regulator probes - we'll only
> disable unused regulators when we get to the end of boot (currently we
> delay this by 30s to give userspace a chance to run, that's a hack but
> we're fresh out of better ideas). During boot the regulator state will
> only be changed if some consumer appears and changes the state.
Okay, so this won't disable the regualtor?
8<----------------------------------------------------------------
static int reg_fixed_voltage_probe(struct platform_device *pdev)
{
...
if (config->enabled_at_boot)
gflags = GPIOD_OUT_HIGH;
else
gflags = GPIOD_OUT_LOW;
...
}
8<----------------------------------------------------------------
Regards,
Marco
On Tue, Oct 08, 2019 at 04:56:05PM +0200, Marco Felsch wrote:
> On 19-10-08 13:51, Mark Brown wrote:
> > No, we shouldn't do anything when the regulator probes - we'll only
> > disable unused regulators when we get to the end of boot (currently we
> > delay this by 30s to give userspace a chance to run, that's a hack but
> > we're fresh out of better ideas). During boot the regulator state will
> > only be changed if some consumer appears and changes the state.
> Okay, so this won't disable the regualtor?
> 8<----------------------------------------------------------------
> static int reg_fixed_voltage_probe(struct platform_device *pdev)
> {
> ...
>
> if (config->enabled_at_boot)
> gflags = GPIOD_OUT_HIGH;
> else
> gflags = GPIOD_OUT_LOW;
>
> ...
> }
> 8<----------------------------------------------------------------
If this is a GPIO regulator then the Linux APIs mean you can't read the
status back so it's one of the regulators for which this property was
invented. This is a real limitation of the Linux APIs, with most
hardware you can actually read the status back so we shouldn't need
this.
On 19-10-08 16:42, Mark Brown wrote:
> On Tue, Oct 08, 2019 at 04:56:05PM +0200, Marco Felsch wrote:
> > On 19-10-08 13:51, Mark Brown wrote:
>
> > > No, we shouldn't do anything when the regulator probes - we'll only
> > > disable unused regulators when we get to the end of boot (currently we
> > > delay this by 30s to give userspace a chance to run, that's a hack but
> > > we're fresh out of better ideas). During boot the regulator state will
> > > only be changed if some consumer appears and changes the state.
>
> > Okay, so this won't disable the regualtor?
>
> > 8<----------------------------------------------------------------
> > static int reg_fixed_voltage_probe(struct platform_device *pdev)
> > {
> > ...
> >
> > if (config->enabled_at_boot)
> > gflags = GPIOD_OUT_HIGH;
> > else
> > gflags = GPIOD_OUT_LOW;
> >
> > ...
> > }
> > 8<----------------------------------------------------------------
>
> If this is a GPIO regulator then the Linux APIs mean you can't read the
> status back so it's one of the regulators for which this property was
> invented. This is a real limitation of the Linux APIs, with most
> hardware you can actually read the status back so we shouldn't need
> this.
I know and I followed the discussion between you and Doug. But it
is a valid use-case to have a external gpio-enabled regualtor connected
to a panel. If I don't mark the regulator as 'regualtor-boot-on' and use
the fixed.c driver (IMHO this is correct), the regulator gets disabled
during probe. So I will have a panel off/ panel on sequence during boot.
To avoid this I set the 'regualtor-boot-on' property but then I can't
disable the panel during suspend..
Can you give me an advice how I can handle that otherwise?
Regards,
Marco
On Tue, Oct 08, 2019 at 06:16:40PM +0200, Marco Felsch wrote:
> On 19-10-08 16:42, Mark Brown wrote:
> > If this is a GPIO regulator then the Linux APIs mean you can't read the
> > status back so it's one of the regulators for which this property was
> > invented. This is a real limitation of the Linux APIs, with most
> > hardware you can actually read the status back so we shouldn't need
> > this.
> I know and I followed the discussion between you and Doug. But it
> is a valid use-case to have a external gpio-enabled regualtor connected
> to a panel. If I don't mark the regulator as 'regualtor-boot-on' and use
> the fixed.c driver (IMHO this is correct), the regulator gets disabled
> during probe. So I will have a panel off/ panel on sequence during boot.
Right, this is why I am saying that this is one of the regulators for
which this property was defined and where you should be using it.
> To avoid this I set the 'regualtor-boot-on' property but then I can't
> disable the panel during suspend..
As you'll have seen from the discussion that's a bug, nothing should be
taking a reference to the regulator outside of explicit enable calls.
On 19-10-08 17:23, Mark Brown wrote:
> On Tue, Oct 08, 2019 at 06:16:40PM +0200, Marco Felsch wrote:
> > On 19-10-08 16:42, Mark Brown wrote:
>
> > > If this is a GPIO regulator then the Linux APIs mean you can't read the
> > > status back so it's one of the regulators for which this property was
> > > invented. This is a real limitation of the Linux APIs, with most
> > > hardware you can actually read the status back so we shouldn't need
> > > this.
>
> > I know and I followed the discussion between you and Doug. But it
> > is a valid use-case to have a external gpio-enabled regualtor connected
> > to a panel. If I don't mark the regulator as 'regualtor-boot-on' and use
> > the fixed.c driver (IMHO this is correct), the regulator gets disabled
> > during probe. So I will have a panel off/ panel on sequence during boot.
>
> Right, this is why I am saying that this is one of the regulators for
> which this property was defined and where you should be using it.
>
> > To avoid this I set the 'regualtor-boot-on' property but then I can't
> > disable the panel during suspend..
>
> As you'll have seen from the discussion that's a bug, nothing should be
> taking a reference to the regulator outside of explicit enable calls.
Okay now we are on the right way :) Is the solution proposed by Doug:
".. we need to match "regulator->enable_count" to "rdev->use_count" at
the end of _regulator_get() in the exclusive case..." the correct fix?
Another question. Please can you have a look on the "DA9062 PMIC fixes
and features" series as well?
Regards,
Marco
On Tue, Oct 08, 2019 at 10:16:22PM +0200, Marco Felsch wrote:
> On 19-10-08 17:23, Mark Brown wrote:
> > As you'll have seen from the discussion that's a bug, nothing should be
> > taking a reference to the regulator outside of explicit enable calls.
> Okay now we are on the right way :) Is the solution proposed by Doug:
> ".. we need to match "regulator->enable_count" to "rdev->use_count" at
> the end of _regulator_get() in the exclusive case..." the correct fix?
Yes, I think so.
> Another question. Please can you have a look on the "DA9062 PMIC fixes
> and features" series as well?
I don't know what that is, sorry.