[PATCH] ARM: amba: adapt to regulator probe deferral change

[PATCH] ARM: amba: adapt to regulator probe deferral change

[Shawn Guo]

The commit 04bf301 (regulator: Support driver probe deferral) changes
regulator_get() and regulator_register() to return -EPROBE_DEFER
instead of -ENODEV.  Adapt amba bus driver to the change, otherwise
amba_probe() will fail on the platforms that do not have "vcore"
regulator device.

It fixes the boot failure on i.mx28 which uses amba-pl011 as serial
console.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/amba/bus.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 01c2cf4..aafee73 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -362,8 +362,8 @@ static int amba_get_enable_vcore(struct amba_device *pcdev)
 	pcdev->vcore = vcore;
 
 	if (IS_ERR(vcore)) {
-		/* It is OK not to supply a vcore regulator */
-		if (PTR_ERR(vcore) == -ENODEV)
+		/* It should not fail in case of -EPROBE_DEFER */
+		if (PTR_ERR(vcore) == -EPROBE_DEFER)
 			return 0;
 		return PTR_ERR(vcore);
 	}
-- 
1.7.5.4

[PATCH] ARM: amba: adapt to regulator probe deferral change

[Fabio Estevam]

On Fri, Mar 30, 2012 at 10:55 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> The commit 04bf301 (regulator: Support driver probe deferral) changes
> regulator_get() and regulator_register() to return -EPROBE_DEFER
> instead of -ENODEV. ?Adapt amba bus driver to the change, otherwise
> amba_probe() will fail on the platforms that do not have "vcore"
> regulator device.
>
> It fixes the boot failure on i.mx28 which uses amba-pl011 as serial
> console.

Tested this patch in linux-next-20120330 and my mx28evk still does not boot.

Any other patch is needed?

[PATCH] ARM: amba: adapt to regulator probe deferral change

[Russell King - ARM Linux]

On Fri, Mar 30, 2012 at 09:55:37PM +0800, Shawn Guo wrote:
> The commit 04bf301 (regulator: Support driver probe deferral) changes
> regulator_get() and regulator_register() to return -EPROBE_DEFER
> instead of -ENODEV.  Adapt amba bus driver to the change, otherwise
> amba_probe() will fail on the platforms that do not have "vcore"
> regulator device.

Are you sure this is correct?  Did you read and understand the comment
you removed?

What do platforms do which have AMBA devices but don't have any vcore
regulators?

We're not going through the same farce that the smsc network driver has
gone through: OMAP3430LDP remains fucked through that idiotic ill-thought
out change (c7e963f68888, net/smsc911x: Add regulator support).  See
http://www.arm.linux.org.uk/developer/build/result.php?type=boot&idx=99

[PATCH] ARM: amba: adapt to regulator probe deferral change

[Shawn Guo]

On Fri, Mar 30, 2012 at 11:23:13AM -0300, Fabio Estevam wrote:
> On Fri, Mar 30, 2012 at 10:55 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> > The commit 04bf301 (regulator: Support driver probe deferral) changes
> > regulator_get() and regulator_register() to return -EPROBE_DEFER
> > instead of -ENODEV. ?Adapt amba bus driver to the change, otherwise
> > amba_probe() will fail on the platforms that do not have "vcore"
> > regulator device.
> >
> > It fixes the boot failure on i.mx28 which uses amba-pl011 as serial
> > console.
> 
> Tested this patch in linux-next-20120330 and my mx28evk still does not boot.
> 
> Any other patch is needed?

Yes.  One more fix is needed.

serial: PL011: move interrupt clearing
http://permalink.gmane.org/gmane.linux.ports.arm.kernel/158694

-- 
Regards,
Shawn

[PATCH] ARM: amba: adapt to regulator probe deferral change

[Shawn Guo]

On Fri, Mar 30, 2012 at 07:01:34PM +0100, Russell King - ARM Linux wrote:
> On Fri, Mar 30, 2012 at 09:55:37PM +0800, Shawn Guo wrote:
> > The commit 04bf301 (regulator: Support driver probe deferral) changes
> > regulator_get() and regulator_register() to return -EPROBE_DEFER
> > instead of -ENODEV.  Adapt amba bus driver to the change, otherwise
> > amba_probe() will fail on the platforms that do not have "vcore"
> > regulator device.
> 
> Are you sure this is correct?  Did you read and understand the comment
> you removed?
> 
I did read the comment and try to understand it.  Correct me if the
understanding below is wrong.

> What do platforms do which have AMBA devices but don't have any vcore
> regulators?
> 
We will have two cases for platforms that do not have any vcore
regulators.

1) CONFIG_REGULATOR is not enabled

   The regulator_get() returns NULL, and regulator_enable() returns 0.
   amba_get_enable_vcore() will succeed with 'ret' as 0.  My patch
   changes nothing for this case.

2) CONFIG_REGULATOR is enabled but platform never registers any vcore
   regulators

   Before commit 04bf301 (regulator: Support driver probe deferral),
   regulator_get() returns ERR_PTR(-ENODEV) when no "vore" regulator
   is found, and the check "if (PTR_ERR(vcore) == -ENODEV)" in
   amba_get_enable_vcore() forces a success return.   Now that commit
   changes regulator_get() to returns ERR_PTR(-EPROBE_DEFER) in case
   that no "vore" regulator is found.  My patch changes the check in
   amba_get_enable_vcore() from -ENODEV to -EPROBE_DEFER accordingly.
   And that's all what my patch does.

> We're not going through the same farce that the smsc network driver has
> gone through: OMAP3430LDP remains fucked through that idiotic ill-thought
> out change (c7e963f68888, net/smsc911x: Add regulator support).  See
> http://www.arm.linux.org.uk/developer/build/result.php?type=boot&idx=99

If the understanding above is correct, I do not see how my patch would
bring the same farce into amba driver.

-- 
Regards,
Shawn

[PATCH] ARM: amba: adapt to regulator probe deferral change

[Mark Brown]

On Fri, Mar 30, 2012 at 07:01:34PM +0100, Russell King - ARM Linux wrote:
> On Fri, Mar 30, 2012 at 09:55:37PM +0800, Shawn Guo wrote:
> > The commit 04bf301 (regulator: Support driver probe deferral) changes
> > regulator_get() and regulator_register() to return -EPROBE_DEFER
> > instead of -ENODEV.  Adapt amba bus driver to the change, otherwise
> > amba_probe() will fail on the platforms that do not have "vcore"
> > regulator device.

> Are you sure this is correct?  Did you read and understand the comment
> you removed?

Shawn's change is a good minimal fix for 3.4, probably the comment
should be maintained but the entire change in the regulator framework
was a change in return code so changing the return code we check for
should restore the previous behaviour for now.  Going forward we
probably want to do something else but for 3.4 this looks like the best
approach:

Reviwed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

> What do platforms do which have AMBA devices but don't have any vcore
> regulators?

We need to change the AMBA bus so it does something better than what it
does now and work out a good way to roll this out.  The more I think
about this the less convinced I am that AMBA should have regualtor
support in the bus itself at all.

I can see a few options here:

 - Add stub vcore regulators to the code for the relevant SoCs.  This is
   tedious, annoying and defeats the point of frameworks so to me it's a
   clear non-starter.

 - Require systems which do have vcore regulators to tell AMBA about it
   when registering the devices then either have the AMBA bus suppress
   the regulator calls or tell the regulator framework about it (which
   is more idiomatic and probably easier in the long run).  This is a
   bit inelegant but should minimise the amount of work.

 - Move the vcore management from AMBA into the SoC-specific power domain
   management code for relevant SoCs.  The SoCs should know if there is a
   vcore that's supplied via a visible regulator (as opposed to power
   gating within the SoC which often has other requirements or features
   and is generally implemented via the power domain support) and this
   should integrate nicely with similar handling for clocks.

I don't know if anyone else has any better ideas?  

To me the power domain approach feels right and looking at the changelog
for the commit introducing this support that seems to be roughly the use
case.  The one actual in tree AMBA device using this stuff seems to be
the pl022 (though there don't look to be any boards with the vcore
regulator supplied) and it certainly looks like it'd fit well with this
idiom.

The current approach leaves us tied to playing init ordering games to
ensure that the regulator is available prior to the AMBA device probing.
We want to get away from relying on those since they end up breaking
down and really only work when everything is built in.  It also give us
poor error handling since we end up silently ignoring failures, for
example if the driver for a vcore regulator is not enabled or the supply
not hooked up when required on a board.

In general any patch which will just ignore errors when requesting a
regulator should be treated with suspicion.

> We're not going through the same farce that the smsc network driver has
> gone through: OMAP3430LDP remains fucked through that idiotic ill-thought
> out change (c7e963f68888, net/smsc911x: Add regulator support).  See
> http://www.arm.linux.org.uk/developer/build/result.php?type=boot&idx=99

For that board it's really very straightforward to add the required
regulators and CONFIG_REGULATOR_DUMMY will normally get things going
immediately, hopefully this is now taken care of by the latest patch
series that was floating around for that stuff?

It's one of these things where it's annoying to have to do the board
update but the update is so simple to do and the alternatives introduce
sufficient fragility and general ick to make it seem like it's the
better alternative.

In general people do need to do an audit of existing boards in the tree
when they add regulator support to an existing driver and make some
effort to at least let people know that updates are required, I did ask
for that to be done with the smsc911x change but it's apparent that this
didn't happen.

In order to reduce the amount of pain this stuff causes it's looking to
me that we should be pushing new off-SoC drivers that don't get power
from their bus to always include at least stub regulator support, that
will at least mean that if more detailed support is added in the future
we don't disrupt things too much.  One of the things I've been trying to
find time to do is adding something like what you've got for AMBA (but
with a driver specified supply list) to the driver core so that this is
as simple as possible.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120331/062e1ac8/attachment.sig>

[PATCH] ARM: amba: adapt to regulator probe deferral change

[Russell King - ARM Linux]

On Sat, Mar 31, 2012 at 02:42:15PM +0100, Mark Brown wrote:
> We need to change the AMBA bus so it does something better than what it
> does now and work out a good way to roll this out.  The more I think
> about this the less convinced I am that AMBA should have regualtor
> support in the bus itself at all.

How else does it power up the peripheral to read out the IDs?  You can't
bind a driver until you've read the ID, which implies that the bus layer
needs to have regulator support to turn the power on.

[PATCH] ARM: amba: adapt to regulator probe deferral change

[Mark Brown]

On Sat, Mar 31, 2012 at 03:35:39PM +0100, Russell King - ARM Linux wrote:
> On Sat, Mar 31, 2012 at 02:42:15PM +0100, Mark Brown wrote:

> > We need to change the AMBA bus so it does something better than what it
> > does now and work out a good way to roll this out.  The more I think
> > about this the less convinced I am that AMBA should have regualtor
> > support in the bus itself at all.

> How else does it power up the peripheral to read out the IDs?  You can't
> bind a driver until you've read the ID, which implies that the bus layer
> needs to have regulator support to turn the power on.

Like I said further down my mail it seems like any systems which do have
individual regulators for the AMBA devices probably ought to be managing
this via power domain code rather than by going direct to the regulator
API.  If actual regulators are required the power domain code for the
platform would be responsible for doing that rather than AMBA itself but
for most SoCs it's likely to not go through the regulator API if there's
any sort of gating support at all.

It's possible I'm misreading this as I'm not terribly familiar with AMBA
but based on what I do know plus the changelog for the patch introducing
it and the one driver that manages vcore at runtime it really does look
like what's going on.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120331/665d48b5/attachment-0001.sig>

[PATCH] ARM: amba: adapt to regulator probe deferral change

[Linus Walleij]

On Sat, Mar 31, 2012 at 6:53 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Sat, Mar 31, 2012 at 03:35:39PM +0100, Russell King - ARM Linux wrote:
>> On Sat, Mar 31, 2012 at 02:42:15PM +0100, Mark Brown wrote:
>
>> > We need to change the AMBA bus so it does something better than what it
>> > does now and work out a good way to roll this out. ?The more I think
>> > about this the less convinced I am that AMBA should have regualtor
>> > support in the bus itself at all.
>
>> How else does it power up the peripheral to read out the IDs? ?You can't
>> bind a driver until you've read the ID, which implies that the bus layer
>> needs to have regulator support to turn the power on.
>
> Like I said further down my mail it seems like any systems which do have
> individual regulators for the AMBA devices probably ought to be managing
> this via power domain code rather than by going direct to the regulator
> API. ?If actual regulators are required the power domain code for the
> platform would be responsible for doing that rather than AMBA itself but
> for most SoCs it's likely to not go through the regulator API if there's
> any sort of gating support at all.

Modeling the actual switches for the power domains as regulators
is a good idea IMO. If using the thing in drivers/base/power/domain.c
this still results in some code that has to flip the actual switch
somewhere, and that very switch may well be modeled as a
regulator. Example drivers/regulator/db8500-prcmu.c

So this would turn to the question of inserting a layer of power
domain abstraction inbetween the device/bus and the regulator.

So the AMBA bus would instead of using regulators directly have
to use the power domains directly to achieve the same effect.
Then the power domain implementation would need to do the
handling of deferred probing, if it happens to be using regulators
to turn the voltage domain switches.

That could (on some) platforms be handled in a centralized manner
such as done by drivers/base/power/clock_ops.c
say a regulator_ops.c thing instead.

What I think is the central question is whether it was a mistake
after all to model power domain switches as regulators, since the
subsystem may be developing in a way more suited for off-chip
regulator things with complex dependency chains and isn't
suitable for the type of "just flip a bit in a memory
mapped register" type of voltage domain regulator.

Maybe a power domain regulator shall be something simpler
than an ordinary regulator? I think the abstraction we get
through having it as a regulator is essentially good but implying
a relationship to off-chip I2C-based regulators with complex
deferred probing may not be desireable.

Should we think of something like a simpler, atomic ops
type "SoC regulator" or something like that, and then
require power domains to use such an abstraction instead
of the ordinary regulator?

Yours,
Linus Walleij

[PATCH] ARM: amba: adapt to regulator probe deferral change

[Mark Brown]

On Sun, Apr 01, 2012 at 02:19:32PM +0200, Linus Walleij wrote:

> Modeling the actual switches for the power domains as regulators
> is a good idea IMO. If using the thing in drivers/base/power/domain.c
> this still results in some code that has to flip the actual switch
> somewhere, and that very switch may well be modeled as a
> regulator. Example drivers/regulator/db8500-prcmu.c

I have to say that this is a really unusual implementation of power
domains, and it's not currently deployed in mainline in this way (in
that nothing I can find has a vcore hooked up) .  The only other on-SoC
regulators are the anatop regulators in newer i.MX processors but those
are clearly a use case for the regulator API since they do voltage
changes.

> So the AMBA bus would instead of using regulators directly have
> to use the power domains directly to achieve the same effect.
> Then the power domain implementation would need to do the
> handling of deferred probing, if it happens to be using regulators
> to turn the voltage domain switches.

Absolutely, yes.  But we'd also get the benefit that platforms which
don't implement things in this specific way wouldn't have to stub things
out or anything and platforms that have more detailed requirements for
power domain management that go over multiple APIs wouldn't have to
work out if they try to dance around the standard support or ignore it.

One nice thing here is that power domains don't require per-bus
handling, the driver model takes care of everything transparently to the
bus using runtime PM.  This is similar to the move to dev_pm_ops in that
it greatly reduces the amount of open coding that is required in buses
to support power management features.

> That could (on some) platforms be handled in a centralized manner
> such as done by drivers/base/power/clock_ops.c
> say a regulator_ops.c thing instead.

If you were to read my original reply to Russell you'll see I did mutter
about this idea...  it should have a few more bells and whistles than
what the clock ops do but it's about there.

> What I think is the central question is whether it was a mistake
> after all to model power domain switches as regulators, since the
> subsystem may be developing in a way more suited for off-chip
> regulator things with complex dependency chains and isn't
> suitable for the type of "just flip a bit in a memory
> mapped register" type of voltage domain regulator.

This isn't something that's changed - the code that's there in mainline
has never been good practice for regulator usage, it's never been a good
idea to silenty ignore a failure to acquire power for the device.  For
most hardware power is fairly important to correct operation.  I'd have
said exactly the same things if I'd noticed the code prior to the change
for deferred probe, it's just that I was unaware of it until Shawn sent
his patch.

For example, if we get an init ordering problem or something with the
current approach you might end up with the regulator API deciding to
power off power domains it thinks aren't in use (because the
regulator_get() failed) and no errors reported until the driver notices
that the hardware underneath it doesn't have power any more.

> Maybe a power domain regulator shall be something simpler
> than an ordinary regulator? I think the abstraction we get
> through having it as a regulator is essentially good but implying
> a relationship to off-chip I2C-based regulators with complex
> deferred probing may not be desireable.

You've still got the same problems on chip as off chip - all this stuff
can be triggered just as easily by init order changes, making things
modular or new inter-subsystem dependencies appearing.  Off chip gives
boards more flexibility to do fun stuff which triggers issues but
fundamentally the issues are always there.

> Should we think of something like a simpler, atomic ops
> type "SoC regulator" or something like that, and then
> require power domains to use such an abstraction instead
> of the ordinary regulator?

Honestly I'd just use power domains, it's a large part of what they're
there for and it plays more nicely.  If all you're looking for from the
framework is reference counting it delivers that very nicely with no
work on the part of the bus and no impact on systems that don't use the
feature.  

You also don't have to deal with the regulator machine interface to
match the power domains with their devices which is the problem Shawn's
run into and is going to be a general annoyance.

With gen_pd it's really simple to implement this stuff, and indeed it
should be fairly simple to make a generic gen_pd domain which takes a
regulator and turns it on and off as needed that people can pick up off
the shelf.  That should be a good way to have platforms that want to use
this model positively select for it while allowing platforms with
different needs the flexibilty to do other things.

Power domains have also got features like being able to defer the
suspend for latency reasons and make it much easier to integrate more
complex constraints on when and how to enter states since the system can
insert random code which talks to any API.  From the SoCs I've looked at
you're usually going to want to do something more than what the
regulator API supports directly.  Even if you're going directly to an
off chip regulator with the control if you want to do something like
voltage scaling you're going to need something SoC specific in the way
to make the decisions.

As a regulator API maintainer when I've been working on power domains
for S3C64xx I went straight to the generic power domain code, it's
really nice and to me it seems like it's a much better fit for the
problem.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120401/0df5c55c/attachment.sig>