ARM: amba: adapt to regulator probe deferral change

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.

---
 drivers/amba/bus.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index 01c2cf4..aafee73 100644
@@ -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

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

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

I did read the comment and try to understand it. Correct me if the understanding below is wrong.

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.

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

Regards,
Shawn

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:

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.

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>

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>

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

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.

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.

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.

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.

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.

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>