LKML Archive on lore.kernel.org
 help / Atom feed
* [PATCH] regulator: core: Clean enabling always-on regulators + their supplies
@ 2018-12-06 22:23 Douglas Anderson
  2018-12-07  1:31 ` Brian Masney
  2018-12-10 15:43 ` Mark Brown
  0 siblings, 2 replies; 9+ messages in thread
From: Douglas Anderson @ 2018-12-06 22:23 UTC (permalink / raw)
  To: Mark Brown
  Cc: linux-arm-msm, Dmitry Osipenko, evgreen, Brian Masney,
	Douglas Anderson, Liam Girdwood, linux-kernel

At the end of regulator_resolve_supply() we have historically turned
on our supply in some cases.  This could be for one of two reasons:

1. If resolving supplies was happening before the call to
   set_machine_constraints() we needed to predict if
   set_machine_constraints() was going to turn the regulator on and we
   needed to preemptively turn the supply on.
2. Maybe set_machine_constraints() happened before we could resolve
   supplies (because we failed the first time to resolve) and thus we
   might need to propagate an enable that already happened up to our
   supply.

Historically regulator_resolve_supply() used _regulator_is_enabled()
to decide whether to turn on the supply.

Let's change things a little bit.  Specifically:

1. Let's try to enable the supply and the regulator in the same place,
   both in set_machine_constraints().  This means that we have exactly
   the same logic for enabling the supply and the regulator.
2. Let's properly set use_count when we enable always-on or boot-on
   regulators even for those that don't have supplies.  The previous
   commit 1fc12b05895e ("regulator: core: Avoid propagating to
   supplies when possible") only did this right for regulators with
   supplies.
3. Let's make it clear that the only time we need to enable the supply
   in regulator_resolve_supply() is if the main regulator is currently
   in use.  By using use_count (like the rest of the code) to decide
   if we're going to enable our supply we keep everything consistent.

Overall the new scheme should be cleaner and easier to reason about.
In addition to fixing regulator_summary to be more correct (because of
the more correct use_count), this change also has the effect of no
longer using _regulator_is_enabled() in this code path.
_regulator_is_enabled() could return an error code for some regulators
at bootup (like RPMh) that can't read their initial state.  While one
can argue that the design of those regulators is sub-optimal, the new
logic sidesteps this brokenness.  This fix in particular fixes
observed problems on Qualcomm sdm845 boards which use the
above-mentioned RPMh regulator.  Those problems were made worse by
commit 1fc12b05895e ("regulator: core: Avoid propagating to supplies
when possible") because now we'd think at bootup that the SD
regulators were already enabled and we'd never try them again.

Fixes: 1fc12b05895e ("regulator: core: Avoid propagating to supplies when possible")
Reported-by: Evan Green <evgreen@chromium.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/regulator/core.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 6f8208170816..e9e66dd5cc3f 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1401,11 +1401,21 @@ 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 (rdev->supply) {
+			ret = regulator_enable(rdev->supply);
+			if (ret < 0) {
+				_regulator_put(rdev->supply);
+				rdev->supply = NULL;
+				return ret;
+			}
+		}
+
 		ret = _regulator_do_enable(rdev);
 		if (ret < 0 && ret != -EINVAL) {
 			rdev_err(rdev, "failed to enable\n");
 			return ret;
 		}
+		rdev->use_count++;
 	}
 
 	print_constraints(rdev);
@@ -1822,15 +1832,18 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 		return ret;
 	}
 
-	/* Cascade always-on state to supply */
-	if (_regulator_is_enabled(rdev)) {
+	/*
+	 * In set_machine_constraints() we may have turned this regulator on
+	 * but we couldn't propagate to the supply if it hadn't been resolved
+	 * yet.  Do it now.
+	 */
+	if (rdev->use_count) {
 		ret = regulator_enable(rdev->supply);
 		if (ret < 0) {
 			_regulator_put(rdev->supply);
 			rdev->supply = NULL;
 			return ret;
 		}
-		rdev->use_count = 1;
 	}
 
 	return 0;
-- 
2.20.0.rc2.403.gdbc3b29805-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] regulator: core: Clean enabling always-on regulators + their supplies
  2018-12-06 22:23 [PATCH] regulator: core: Clean enabling always-on regulators + their supplies Douglas Anderson
@ 2018-12-07  1:31 ` Brian Masney
  2018-12-10 15:43 ` Mark Brown
  1 sibling, 0 replies; 9+ messages in thread
From: Brian Masney @ 2018-12-07  1:31 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Mark Brown, linux-arm-msm, Dmitry Osipenko, evgreen,
	Liam Girdwood, linux-kernel

On Thu, Dec 06, 2018 at 02:23:18PM -0800, Douglas Anderson wrote:
> At the end of regulator_resolve_supply() we have historically turned
> on our supply in some cases.  This could be for one of two reasons:
> 
> 1. If resolving supplies was happening before the call to
>    set_machine_constraints() we needed to predict if
>    set_machine_constraints() was going to turn the regulator on and we
>    needed to preemptively turn the supply on.
> 2. Maybe set_machine_constraints() happened before we could resolve
>    supplies (because we failed the first time to resolve) and thus we
>    might need to propagate an enable that already happened up to our
>    supply.
> 
> Historically regulator_resolve_supply() used _regulator_is_enabled()
> to decide whether to turn on the supply.
> 
> Let's change things a little bit.  Specifically:
> 
> 1. Let's try to enable the supply and the regulator in the same place,
>    both in set_machine_constraints().  This means that we have exactly
>    the same logic for enabling the supply and the regulator.
> 2. Let's properly set use_count when we enable always-on or boot-on
>    regulators even for those that don't have supplies.  The previous
>    commit 1fc12b05895e ("regulator: core: Avoid propagating to
>    supplies when possible") only did this right for regulators with
>    supplies.
> 3. Let's make it clear that the only time we need to enable the supply
>    in regulator_resolve_supply() is if the main regulator is currently
>    in use.  By using use_count (like the rest of the code) to decide
>    if we're going to enable our supply we keep everything consistent.
> 
> Overall the new scheme should be cleaner and easier to reason about.
> In addition to fixing regulator_summary to be more correct (because of
> the more correct use_count), this change also has the effect of no
> longer using _regulator_is_enabled() in this code path.
> _regulator_is_enabled() could return an error code for some regulators
> at bootup (like RPMh) that can't read their initial state.  While one
> can argue that the design of those regulators is sub-optimal, the new
> logic sidesteps this brokenness.  This fix in particular fixes
> observed problems on Qualcomm sdm845 boards which use the
> above-mentioned RPMh regulator.  Those problems were made worse by
> commit 1fc12b05895e ("regulator: core: Avoid propagating to supplies
> when possible") because now we'd think at bootup that the SD
> regulators were already enabled and we'd never try them again.
> 
> Fixes: 1fc12b05895e ("regulator: core: Avoid propagating to supplies when possible")
> Reported-by: Evan Green <evgreen@chromium.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Looks good on the Nexus 5 (qcom msm8974).

Tested-by: Brian Masney <masneyb@onstation.org>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] regulator: core: Clean enabling always-on regulators + their supplies
  2018-12-06 22:23 [PATCH] regulator: core: Clean enabling always-on regulators + their supplies Douglas Anderson
  2018-12-07  1:31 ` Brian Masney
@ 2018-12-10 15:43 ` Mark Brown
  2018-12-10 16:32   ` Doug Anderson
  1 sibling, 1 reply; 9+ messages in thread
From: Mark Brown @ 2018-12-10 15:43 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: linux-arm-msm, Dmitry Osipenko, evgreen, Brian Masney,
	Liam Girdwood, linux-kernel

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

On Thu, Dec 06, 2018 at 02:23:18PM -0800, Douglas Anderson wrote:
> At the end of regulator_resolve_supply() we have historically turned
> on our supply in some cases.  This could be for one of two reasons:

This doesn't apply against current code, please check and resend.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] regulator: core: Clean enabling always-on regulators + their supplies
  2018-12-10 15:43 ` Mark Brown
@ 2018-12-10 16:32   ` Doug Anderson
  2018-12-11  0:52     ` Mark Brown
  0 siblings, 1 reply; 9+ messages in thread
From: Doug Anderson @ 2018-12-10 16:32 UTC (permalink / raw)
  To: Mark Brown
  Cc: linux-arm-msm, Dmitry Osipenko, Evan Green, masneyb, Liam Girdwood, LKML

Mark,

On Mon, Dec 10, 2018 at 7:43 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Dec 06, 2018 at 02:23:18PM -0800, Douglas Anderson wrote:
> > At the end of regulator_resolve_supply() we have historically turned
> > on our supply in some cases.  This could be for one of two reasons:
>
> This doesn't apply against current code, please check and resend.

Can you clarify?  See below where it's applying cleanly to "for-next" for me:

$ git remote -v | grep linuxregulator
linuxregulator
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
(fetch)
linuxregulator
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git
(push)

$ git fetch linuxregulator

$ git log --oneline -1 linuxregulator/for-next
f00e93a1224f (linuxregulator/for-next) Merge remote-tracking branch
'regulator/topic/coupled' into regulator-next

$ git checkout linuxregulator/for-next
[ ... cut ... ]
HEAD is now at f00e93a1224f Merge remote-tracking branch
'regulator/topic/coupled' into regulator-next

$ pwclient git-am -p lkml 1022439
Applying patch #1022439 using u'git am'
Description: regulator: core: Clean enabling always-on regulators +
their supplies
Applying: regulator: core: Clean enabling always-on regulators + their supplies

Thanks!

-Doug

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] regulator: core: Clean enabling always-on regulators + their supplies
  2018-12-10 16:32   ` Doug Anderson
@ 2018-12-11  0:52     ` Mark Brown
  2018-12-11  1:00       ` Mark Brown
  0 siblings, 1 reply; 9+ messages in thread
From: Mark Brown @ 2018-12-11  0:52 UTC (permalink / raw)
  To: Doug Anderson
  Cc: linux-arm-msm, Dmitry Osipenko, Evan Green, masneyb, Liam Girdwood, LKML

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

On Mon, Dec 10, 2018 at 08:32:32AM -0800, Doug Anderson wrote:
> On Mon, Dec 10, 2018 at 7:43 AM Mark Brown <broonie@kernel.org> wrote:

> > On Thu, Dec 06, 2018 at 02:23:18PM -0800, Douglas Anderson wrote:
> > > At the end of regulator_resolve_supply() we have historically turned
> > > on our supply in some cases.  This could be for one of two reasons:

> > This doesn't apply against current code, please check and resend.

> Can you clarify?  See below where it's applying cleanly to "for-next" for me:

I tried a git am on my for-4.21 and for-next branches and it didn't work
on either, I didn't investigate beyond that.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] regulator: core: Clean enabling always-on regulators + their supplies
  2018-12-11  0:52     ` Mark Brown
@ 2018-12-11  1:00       ` Mark Brown
  2018-12-11  2:40         ` Doug Anderson
  0 siblings, 1 reply; 9+ messages in thread
From: Mark Brown @ 2018-12-11  1:00 UTC (permalink / raw)
  To: Doug Anderson
  Cc: linux-arm-msm, Dmitry Osipenko, Evan Green, masneyb, Liam Girdwood, LKML

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

On Tue, Dec 11, 2018 at 12:52:21AM +0000, Mark Brown wrote:
> On Mon, Dec 10, 2018 at 08:32:32AM -0800, Doug Anderson wrote:

> > Can you clarify?  See below where it's applying cleanly to "for-next" for me:

> I tried a git am on my for-4.21 and for-next branches and it didn't work
> on either, I didn't investigate beyond that.

Applying: regulator: core: Clean enabling always-on regulators + their supplies
Using index info to reconstruct a base tree...
M	drivers/regulator/core.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/regulator/core.c
CONFLICT (content): Merge conflict in drivers/regulator/core.c
Recorded preimage for 'drivers/regulator/core.c'
error: Failed to merge in the changes.
Patch failed at 0001 regulator: core: Clean enabling always-on regulators + their supplies
hint: Use 'git am --show-current-patch' to see the failed patch

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] regulator: core: Clean enabling always-on regulators + their supplies
  2018-12-11  1:00       ` Mark Brown
@ 2018-12-11  2:40         ` Doug Anderson
  2018-12-11 14:06           ` Mark Brown
  0 siblings, 1 reply; 9+ messages in thread
From: Doug Anderson @ 2018-12-11  2:40 UTC (permalink / raw)
  To: Mark Brown
  Cc: linux-arm-msm, Dmitry Osipenko, Evan Green, masneyb, Liam Girdwood, LKML

Mark,

On Mon, Dec 10, 2018 at 5:00 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Dec 11, 2018 at 12:52:21AM +0000, Mark Brown wrote:
> > On Mon, Dec 10, 2018 at 08:32:32AM -0800, Doug Anderson wrote:
>
> > > Can you clarify?  See below where it's applying cleanly to "for-next" for me:
>
> > I tried a git am on my for-4.21 and for-next branches and it didn't work
> > on either, I didn't investigate beyond that.
>
> Applying: regulator: core: Clean enabling always-on regulators + their supplies
> Using index info to reconstruct a base tree...
> M       drivers/regulator/core.c
> Falling back to patching base and 3-way merge...
> Auto-merging drivers/regulator/core.c
> CONFLICT (content): Merge conflict in drivers/regulator/core.c
> Recorded preimage for 'drivers/regulator/core.c'
> error: Failed to merge in the changes.
> Patch failed at 0001 regulator: core: Clean enabling always-on regulators + their supplies
> hint: Use 'git am --show-current-patch' to see the failed patch

Can you confirm my patch applies cleanly to your for-next branch?  I
can see that it doesn't apply to your 4.21 branch but I can't see it
fail on your for-next branch.

The problem is that commit 2bb166636933 ("regulator: core: enable
power when setting up constraints") touches the same code but isn't in
the 4.21 branch.  If you pick that to your 4.21 then my patch applies
cleanly.

If you'd prefer that I post my patch atop 4.21 I can do that tomorrow.


-Doug

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] regulator: core: Clean enabling always-on regulators + their supplies
  2018-12-11  2:40         ` Doug Anderson
@ 2018-12-11 14:06           ` Mark Brown
  2018-12-11 16:21             ` Doug Anderson
  0 siblings, 1 reply; 9+ messages in thread
From: Mark Brown @ 2018-12-11 14:06 UTC (permalink / raw)
  To: Doug Anderson
  Cc: linux-arm-msm, Dmitry Osipenko, Evan Green, masneyb, Liam Girdwood, LKML

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

On Mon, Dec 10, 2018 at 06:40:53PM -0800, Doug Anderson wrote:

> Can you confirm my patch applies cleanly to your for-next branch?  I
> can see that it doesn't apply to your 4.21 branch but I can't see it
> fail on your for-next branch.

No, I tried both and it didn't work on either.

> The problem is that commit 2bb166636933 ("regulator: core: enable
> power when setting up constraints") touches the same code but isn't in
> the 4.21 branch.  If you pick that to your 4.21 then my patch applies
> cleanly.

> If you'd prefer that I post my patch atop 4.21 I can do that tomorrow.

I'd prefer that you resend like I said :(

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] regulator: core: Clean enabling always-on regulators + their supplies
  2018-12-11 14:06           ` Mark Brown
@ 2018-12-11 16:21             ` Doug Anderson
  0 siblings, 0 replies; 9+ messages in thread
From: Doug Anderson @ 2018-12-11 16:21 UTC (permalink / raw)
  To: Mark Brown
  Cc: linux-arm-msm, Dmitry Osipenko, Evan Green, masneyb, Liam Girdwood, LKML

Hi,

On Tue, Dec 11, 2018 at 6:06 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Dec 10, 2018 at 06:40:53PM -0800, Doug Anderson wrote:
>
> > Can you confirm my patch applies cleanly to your for-next branch?  I
> > can see that it doesn't apply to your 4.21 branch but I can't see it
> > fail on your for-next branch.
>
> No, I tried both and it didn't work on either.

I'm totally confused because I can't reproduce, but OK.


> > The problem is that commit 2bb166636933 ("regulator: core: enable
> > power when setting up constraints") touches the same code but isn't in
> > the 4.21 branch.  If you pick that to your 4.21 then my patch applies
> > cleanly.
>
> > If you'd prefer that I post my patch atop 4.21 I can do that tomorrow.
>
> I'd prefer that you resend like I said :(

If I can't reproduce the merge conflict that you're seeing I'm worried
that I'll just be sending you the exact same patch I sent before and
it'll have the same problem.  That's why I've been so focused on
trying to understand.

In any case it sounds as if sending it up against 4.21 should be fine
and maybe is preferred (since the patch it Fixes is in 4.21), so here
comes v2 against for-4.21.  OK, posted at
<https://lkml.kernel.org/r/20181211161720.102888-1-dianders@chromium.org>


-Doug

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-06 22:23 [PATCH] regulator: core: Clean enabling always-on regulators + their supplies Douglas Anderson
2018-12-07  1:31 ` Brian Masney
2018-12-10 15:43 ` Mark Brown
2018-12-10 16:32   ` Doug Anderson
2018-12-11  0:52     ` Mark Brown
2018-12-11  1:00       ` Mark Brown
2018-12-11  2:40         ` Doug Anderson
2018-12-11 14:06           ` Mark Brown
2018-12-11 16:21             ` Doug Anderson

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox