[PATCH 1/2] MMC/pxamci: workaround regulator framework bugs

[PATCH 1/2] MMC/pxamci: workaround regulator framework bugs

[Daniel Ribeiro]

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

The voltage regulator framework can't sanely deal with voltage
regulators which are left enabled by the bootloader. We workaround this
by forcing an enable()/disable() pair so it has a sane use_count.

Signed-off-by: Daniel Ribeiro <drwyrm@gmail.com>

---
 drivers/mmc/host/pxamci.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index d7d7109..20fb3da 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -80,6 +80,18 @@ static inline void pxamci_init_ocr(struct pxamci_host *host)
 	if (IS_ERR(host->vcc))
 		host->vcc = NULL;
 	else {
+		/*
+		 * When the bootloader leaves a supply active, it's
+		 * initialized with zero usecount ... and we can't
+		 * disable it without first enabling it.  Until the
+		 * framework is fixed, we need a workaround like this
+		 * (which is safe for MMC, but not in general).
+		 */
+		if (regulator_is_enabled(host->vcc) > 0) {
+			regulator_enable(host->vcc);
+			regulator_disable(host->vcc);
+		}
+
 		host->mmc->ocr_avail = mmc_regulator_get_ocrmask(host->vcc);
 		if (host->pdata && host->pdata->ocr_mask)
 			dev_warn(mmc_dev(host->mmc),
-- 
tg: (87da0bf..) mmc/workaround-regulator-bugs (depends on: mmc/pxamci-regulator-support)

-- 
Daniel Ribeiro

[-- Attachment #2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/2] MMC/pxamci: workaround regulator framework bugs

[Mark Brown]

On Fri, Jun 26, 2009 at 08:07:48PM -0300, Daniel Ribeiro wrote:
> The voltage regulator framework can't sanely deal with voltage
> regulators which are left enabled by the bootloader. We workaround this
> by forcing an enable()/disable() pair so it has a sane use_count.

Please write a commit message explaining what the actual situation is
here.  Ideally this would include some documentation for the MMC core 
explaining the requirements it has.  The fact that you're trying to
force the regulator off is a big warning sign for the more common
non-exclusive consumers so you need to be clear about why this is
required.

> +		/*
> +		 * When the bootloader leaves a supply active, it's
> +		 * initialized with zero usecount ... and we can't
> +		 * disable it without first enabling it.  Until the
> +		 * framework is fixed, we need a workaround like this
> +		 * (which is safe for MMC, but not in general).

This also needs to explain the actual situation, especially the
exclusivity reqirement of the MMC core.  It might be worth providing a
helper in the MMC core, everything is going to need to be updated to use
exclusive get whenever someone implements it.

As discussed at some length the regulator API is not going to be changed
in the way you demand.  This is not something that it should be easy for
standard consumers to do since for standard consumers it normally points
out a programming error.

Re: [PATCH 1/2] MMC/pxamci: workaround regulator framework bugs

[Daniel Ribeiro]

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

Em Sáb, 2009-06-27 às 01:48 +0100, Mark Brown escreveu:
> > +		/*
> > +		 * When the bootloader leaves a supply active, it's
> > +		 * initialized with zero usecount ... and we can't
> > +		 * disable it without first enabling it.  Until the
> > +		 * framework is fixed, we need a workaround like this
> > +		 * (which is safe for MMC, but not in general).
> 
> This also needs to explain the actual situation, especially the
> exclusivity reqirement of the MMC core.  It might be worth providing a
> helper in the MMC core, everything is going to need to be updated to use
> exclusive get whenever someone implements it.

Heh, this patch was copy-n-paste from twl code. I really can't put much
time on this. :(

> As discussed at some length the regulator API is not going to be changed
> in the way you demand.  This is not something that it should be easy for
> standard consumers to do since for standard consumers it normally points
> out a programming error.

Ok, so I will "fix it" hacking the bootloader to power off the voltage
regulator before giving control to kernel. Lets hope that the next
developer that tries to use the regulator framework with a mmc card and
stumbles on the "regulator already enabled" case has more time to fix it
properly. :)

-- 
Daniel Ribeiro

[-- Attachment #2: Esta é uma parte de mensagem assinada digitalmente --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/2] MMC/pxamci: workaround regulator framework bugs

[Eric Miao]

Daniel Ribeiro wrote:
> Em Sáb, 2009-06-27 às 01:48 +0100, Mark Brown escreveu:
>>> +		/*
>>> +		 * When the bootloader leaves a supply active, it's
>>> +		 * initialized with zero usecount ... and we can't
>>> +		 * disable it without first enabling it.  Until the
>>> +		 * framework is fixed, we need a workaround like this
>>> +		 * (which is safe for MMC, but not in general).
>> This also needs to explain the actual situation, especially the
>> exclusivity reqirement of the MMC core.  It might be worth providing a
>> helper in the MMC core, everything is going to need to be updated to use
>> exclusive get whenever someone implements it.
> 
> Heh, this patch was copy-n-paste from twl code. I really can't put much
> time on this. :(
> 
>> As discussed at some length the regulator API is not going to be changed
>> in the way you demand.  This is not something that it should be easy for
>> standard consumers to do since for standard consumers it normally points
>> out a programming error.
> 
> Ok, so I will "fix it" hacking the bootloader to power off the voltage
> regulator before giving control to kernel. Lets hope that the next
> developer that tries to use the regulator framework with a mmc card and
> stumbles on the "regulator already enabled" case has more time to fix it
> properly. :)
> 

This looks to be running into the same issue as clocks - where for power
savings the clocks are assumed to be off as many as possible, leaving only
those essential ones enabled, yet the assumption of the boot loader does
this correctly is always a big problem, putting this into the kernel,
however, is ugly.

Re: [PATCH 1/2] MMC/pxamci: workaround regulator framework bugs

[Mark Brown]

On Mon, Jun 29, 2009 at 11:00:38AM +0800, Eric Miao wrote:

> This looks to be running into the same issue as clocks - where for power
> savings the clocks are assumed to be off as many as possible, leaving only
> those essential ones enabled, yet the assumption of the boot loader does
> this correctly is always a big problem, putting this into the kernel,
> however, is ugly.

At the minute the regulator API actually copes pretty well with this -
the only problem I'm aware of is with drivers like the MMC driver which
require exclusive control of the regulator.  With other drivers the core
API can clean up after startup and the drivers never need to worry about
fixing things up.

Re: [PATCH 1/2] MMC/pxamci: workaround regulator framework bugs

[David Brownell]

On Monday 29 June 2009, Mark Brown wrote:
> At the minute the regulator API actually copes pretty well with this -
> the only problem I'm aware of is with drivers like the MMC driver which
> require exclusive control of the regulator.

Which is a fairly typical situation for power-aware drivers.

Which belies your claim that the regulator API "copes pretty well".
It'd be more accurate to say "broken-as-designed", since you have
rejected numerous attempts to fix this, yet not fixed it yourself.


> With other drivers the core 
> API can clean up after startup and the drivers never need to worry about
> fixing things up.

MMC isn't particularly unusual.  It's just the first place this
type problem tends to come up.  Expect more such problelms as
folkl actually try to *use* the regulator framework.

Re: [PATCH 1/2] MMC/pxamci: workaround regulator framework bugs

[Mark Brown]

On Tue, Jun 30, 2009 at 07:36:20PM -0700, David Brownell wrote:
> On Monday 29 June 2009, Mark Brown wrote:

> > At the minute the regulator API actually copes pretty well with this -
> > the only problem I'm aware of is with drivers like the MMC driver which
> > require exclusive control of the regulator.

> Which is a fairly typical situation for power-aware drivers.

As has been mentioned a number of times in previous discussions of this
there is a very large class of devices which do not *require* any power
control at all but which can usefully switch their supplies on and off.

> Which belies your claim that the regulator API "copes pretty well".
> It'd be more accurate to say "broken-as-designed", since you have
> rejected numerous attempts to fix this, yet not fixed it yourself.

You've suggested variations of essentially one approach, forcing the
regulator to be off while the use count is zero.  In response to this
you've not only been given explanations of the problems this causes but
also several suggestions for alternative approaches such as adding a way
to request exclusive use of the regulator or adding something in
constraints to turn off the regulator at startup.

I have previously had to ask you to try to approach discussions on the
regulator API in a more constructive fashion, please let me renew that
request.  Doing so would be much less time consuming and for that reason
if nothing else would be very helpful in progressing things.

Re: [PATCH 1/2] MMC/pxamci: workaround regulator framework bugs

[David Brownell]

On Wednesday 01 July 2009, Mark Brown wrote:
> On Tue, Jun 30, 2009 at 07:36:20PM -0700, David Brownell wrote:
> > On Monday 29 June 2009, Mark Brown wrote:
> 
> > > At the minute the regulator API actually copes pretty well with this -
> > > the only problem I'm aware of is with drivers like the MMC driver which
> > > require exclusive control of the regulator.
> 
> > Which is a fairly typical situation for power-aware drivers.
> 
> As has been mentioned a number of times in previous discussions of this
> there is a very large class of devices which do not *require* any power
> control at all but which can usefully switch their supplies on and off.

Which I never argued with.  My comment was about their drivers.

Power-aware drivers may *require* that they can actually reduce
system power consumption ... else what's the point?  Likewise,
switching voltages needs care, and sometimes ability to switch
power on and off.


> > Which belies your claim that the regulator API "copes pretty well".
> > It'd be more accurate to say "broken-as-designed", since you have
> > rejected numerous attempts to fix this, yet not fixed it yourself.
> 
> You've suggested variations of essentially one approach, forcing the
> regulator to be off while the use count is zero.

For the record, that's simply not true.  The patches I sent tried
a variety of approaches, and I don't recall that ever being one of
them ... since that is in fact a fair description of what needed to
be FIXED.  The property my patches shared was:

	/* that this simple idiom would finally work */
	if (regulator_is_enabled(r))
		regulator_disable(r);

It's *your* proposals which preserved the property that the above
lines of code could fail (often rudely at boot time).  Until just
yesterday... when you posted

  http://marc.info/?l=linux-kernel&m=124818844611060&w=2

which is intended to provide a new mechanism, the only way to
ensure the above idiom can always work.  It looks like that will
work for $SUBJECT (MMC/SD drivers) and some similar cases.


> I have previously had to ask you to try to approach discussions on the
> regulator API in a more constructive fashion, please let me renew that
> request.  Doing so would be much less time consuming and for that reason
> if nothing else would be very helpful in progressing things.

I've previously had to ask you to respond to **what I said** not
to something you merely imagined I had said.  Don't pretend that
you were blameless.  Your approach was highly confrontational,
and rejected many constructive suggestions.  At one point you
flamed me for disagreeing ... with the point *I* was making, as
eventually became clear.

If you felt my responses were sufficiently non-constructive as to
deserve a lecture on courtesy ... you ought to have considered your
own participation.  What you saw was a rising tide of frustration
caused by (a) your refusal to address what I was actually saying,
(b) your falsely attributing statements and viewpoints to me, and
(c) rejecting around half a dozen patches to solve a problem, all
of which were within (d) an ever-increasing number of constraints
you grew with each new iteration.  I had to try so many different
approaches since nothing seemed to be getting through.  The lack
of constructive behavior was mostly on *your* part ... but when I
finally called you on that, I got a lecture back!!  Feh.  I don't
need to let such crap stand; but decided to wait until the anger
went away.

So it's no surprise I would conclude the only solution was to wait
for you to write a patch, which you would then accept.  And you
have now done that; thanks.

- Dave

Re: [PATCH 1/2] MMC/pxamci: workaround regulator framework bugs

[Mark Brown]

On Wed, Jul 22, 2009 at 02:03:07PM -0700, David Brownell wrote:

> 	/* that this simple idiom would finally work */
> 	if (regulator_is_enabled(r))
> 		regulator_disable(r);

For the avoidance of future disappointment please note that an exclusive
access request does not remove the reference counting on the regulator,
it only changes the way in which it is intialised.  This means that the
above code is only guaranteed to cause the regulator to be disabled so
long as the consumer never calls enable() twice without a matching
disable().  It will, however, not return an error due to unbalanced
enables and disables which I believe is your primary requirement here.

> of which were within (d) an ever-increasing number of constraints
> you grew with each new iteration.  I had to try so many different
> approaches since nothing seemed to be getting through.  The lack

For clarity and in the hope that this helps to avoid future issues
let me list the three major constraints in this area:

 - The API supports shared regulators.
 - Regulator enabling by software is reference counted.
 - Changing the state of regulators before drivers have had a chance to
   take control of the devices is very disruptive to the system and
   needs to be avoided as a result.

Everything else flows from those, in conjunction with the standard
considerations about making sure that any transitions are managed in as
graceful a fashion as possible.

I'm leaving the rest of your mail because I don't think discussing that
further here is going to get us anywhere.