[PATCH/RFC] ARM: amba: Remove AMBA level regulator support

[PATCH/RFC] ARM: amba: Remove AMBA level regulator support

[Mark Brown]

The AMBA bus regulator support is being used to model on/off switches
for power domains which isn't terribly idiomatic for modern kernels with
the generic power domain code and creates integration problems on platforms
which don't use regulators for their power domains as it's hard to tell
the difference between a regulator that is needed but failed to be provided
and one that isn't supposed to be there (though DT does make that easier).

Platforms that wish to use the regulator API to manage their power domains
can indirect via the power domain interface.

The impact should be minimal since currently there are no mainline
systems which actually provide a vcore regulator so none need updating.

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

RFC because there's some disagreement about this.

 drivers/amba/bus.c       |   42 +-----------------------------------------
 drivers/spi/spi-pl022.c  |    2 --
 include/linux/amba/bus.h |    8 --------
 3 files changed, 1 insertions(+), 51 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 01c2cf4..cc27322 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -247,8 +247,7 @@ static int amba_pm_restore(struct device *dev)
 /*
  * Hooks to provide runtime PM of the pclk (bus clock).  It is safe to
  * enable/disable the bus clock at runtime PM suspend/resume as this
- * does not result in loss of context.  However, disabling vcore power
- * would do, so we leave that to the driver.
+ * does not result in loss of context.
  */
 static int amba_pm_runtime_suspend(struct device *dev)
 {
@@ -354,39 +353,6 @@ static void amba_put_disable_pclk(struct amba_device *pcdev)
 	clk_put(pclk);
 }
 
-static int amba_get_enable_vcore(struct amba_device *pcdev)
-{
-	struct regulator *vcore = regulator_get(&pcdev->dev, "vcore");
-	int ret;
-
-	pcdev->vcore = vcore;
-
-	if (IS_ERR(vcore)) {
-		/* It is OK not to supply a vcore regulator */
-		if (PTR_ERR(vcore) == -ENODEV)
-			return 0;
-		return PTR_ERR(vcore);
-	}
-
-	ret = regulator_enable(vcore);
-	if (ret) {
-		regulator_put(vcore);
-		pcdev->vcore = ERR_PTR(-ENODEV);
-	}
-
-	return ret;
-}
-
-static void amba_put_disable_vcore(struct amba_device *pcdev)
-{
-	struct regulator *vcore = pcdev->vcore;
-
-	if (!IS_ERR(vcore)) {
-		regulator_disable(vcore);
-		regulator_put(vcore);
-	}
-}
-
 /*
  * These are the device model conversion veneers; they convert the
  * device model structures to our more specific structures.
@@ -399,10 +365,6 @@ static int amba_probe(struct device *dev)
 	int ret;
 
 	do {
-		ret = amba_get_enable_vcore(pcdev);
-		if (ret)
-			break;
-
 		ret = amba_get_enable_pclk(pcdev);
 		if (ret)
 			break;
@@ -420,7 +382,6 @@ static int amba_probe(struct device *dev)
 		pm_runtime_put_noidle(dev);
 
 		amba_put_disable_pclk(pcdev);
-		amba_put_disable_vcore(pcdev);
 	} while (0);
 
 	return ret;
@@ -442,7 +403,6 @@ static int amba_remove(struct device *dev)
 	pm_runtime_put_noidle(dev);
 
 	amba_put_disable_pclk(pcdev);
-	amba_put_disable_vcore(pcdev);
 
 	return ret;
 }
diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 96f0da6..09c925a 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -2195,7 +2195,6 @@ static int pl022_runtime_suspend(struct device *dev)
 	struct pl022 *pl022 = dev_get_drvdata(dev);
 
 	clk_disable(pl022->clk);
-	amba_vcore_disable(pl022->adev);
 
 	return 0;
 }
@@ -2204,7 +2203,6 @@ static int pl022_runtime_resume(struct device *dev)
 {
 	struct pl022 *pl022 = dev_get_drvdata(dev);
 
-	amba_vcore_enable(pl022->adev);
 	clk_enable(pl022->clk);
 
 	return 0;
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index 7847e19..63a59ac 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -19,7 +19,6 @@
 #include <linux/mod_devicetable.h>
 #include <linux/err.h>
 #include <linux/resource.h>
-#include <linux/regulator/consumer.h>
 
 #define AMBA_NR_IRQS	2
 #define AMBA_CID	0xb105f00d
@@ -30,7 +29,6 @@ struct amba_device {
 	struct device		dev;
 	struct resource		res;
 	struct clk		*pclk;
-	struct regulator	*vcore;
 	u64			dma_mask;
 	unsigned int		periphid;
 	unsigned int		irq[AMBA_NR_IRQS];
@@ -75,12 +73,6 @@ void amba_release_regions(struct amba_device *);
 #define amba_pclk_disable(d)	\
 	do { if (!IS_ERR((d)->pclk)) clk_disable((d)->pclk); } while (0)
 
-#define amba_vcore_enable(d)	\
-	(IS_ERR((d)->vcore) ? 0 : regulator_enable((d)->vcore))
-
-#define amba_vcore_disable(d)	\
-	do { if (!IS_ERR((d)->vcore)) regulator_disable((d)->vcore); } while (0)
-
 /* Some drivers don't use the struct amba_device */
 #define AMBA_CONFIG_BITS(a) (((a) >> 24) & 0xff)
 #define AMBA_REV_BITS(a) (((a) >> 20) & 0x0f)
-- 
1.7.9.1

[PATCH/RFC] ARM: amba: Remove AMBA level regulator support

[Linus Walleij]

On Sun, Apr 1, 2012 at 5:29 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:

> The AMBA bus regulator support is being used to model on/off switches
> for power domains which isn't terribly idiomatic for modern kernels with
> the generic power domain code and creates integration problems on platforms
> which don't use regulators for their power domains as it's hard to tell
> the difference between a regulator that is needed but failed to be provided
> and one that isn't supposed to be there (though DT does make that easier).
>
> Platforms that wish to use the regulator API to manage their power domains
> can indirect via the power domain interface.

I don't see how this solves the problem of AMBA PrimeCell probing.

If you look at the code in bus.c, you can see that it acquires and
enables the regulator - as it does with the clock.

The reason is that is does this *before* the device can probe,
since it needs to map up the memory to read out some magic
values at the end to figure out what device it is in the first place.

We need the current code replaced with something that
enables a power domain before probe instead, then implement
these power domains for the in-kernel AMBA devices that need it.

Is the default behaviour of power domains such that they will
be enabled as soon as devices are registered but before
any buses probe()? Because that is what is needed in this case.

(AMBA devices are special in this way: no other ARM things
support auto-detection of devices using magic numbers,
basically the DT stuff came about because noone was using
a thing like this.)

> The impact should be minimal since currently there are no mainline
> systems which actually provide a vcore regulator so none need updating.

Oh yes there are:
drivers/mfd/db8500-prcmu.c

This driver registers a number of voltage domain regulators,
among those:

static struct regulator_consumer_supply db8500_vape_consumers[] = {
       (...)
        REGULATOR_SUPPLY("vcore", "sdi0"),
        REGULATOR_SUPPLY("vcore", "sdi1"),
        REGULATOR_SUPPLY("vcore", "sdi2"),
        REGULATOR_SUPPLY("vcore", "sdi3"),
        REGULATOR_SUPPLY("vcore", "sdi4"),
        (...)
        REGULATOR_SUPPLY("vcore", "uart0"),
        REGULATOR_SUPPLY("vcore", "uart1"),
        REGULATOR_SUPPLY("vcore", "uart2"),

(I know the supplies should probably be moved up to the
platform but there they are.) The first array is MMC controllers
and the second is a number of UARTs.

IIRC the machine will not boot (i.e. these drivers cannot even
probe) without these regulators in place, so they get enabled by
the AMBA bus code.

So we need something that not just removes stuff from the
AMBA bus, but also adds a better power domain mechanism
and fixes up taking the above regulators.

That said I'm all for replacing it - but I'd need to figure out the
details on how to do that.

We do have code for ux500 power domains. If it will will be
enough to handle this I can try to hack it up and submit it.

Yours,
Linus Walleij

[PATCH/RFC] ARM: amba: Remove AMBA level regulator support

[Mark Brown]

On Sun, Apr 01, 2012 at 07:22:46PM +0200, Linus Walleij wrote:
> On Sun, Apr 1, 2012 at 5:29 PM, Mark Brown

> > The AMBA bus regulator support is being used to model on/off switches
> > for power domains which isn't terribly idiomatic for modern kernels with

> I don't see how this solves the problem of AMBA PrimeCell probing.

I think you're misunderstanding what this fixes.  The main problem it
addresses is that as things stand platforms which have no intention of
using regulators to model power domains really ought to be providing
dummy vcores for their AMBA devices (or the AMBA bus should be doing
that).  Removing regulator usage from the AMBA core code obviously
accomplishes this, avoiding disruption to these platforms.

It does also remove the dodgy ignore the error idiom, but that's pretty
much secondary.

> We need the current code replaced with something that
> enables a power domain before probe instead, then implement
> these power domains for the in-kernel AMBA devices that need it.

> Is the default behaviour of power domains such that they will
> be enabled as soon as devices are registered but before
> any buses probe()? Because that is what is needed in this case.

Yes, this should be the case (TBH I can't actually remember if you have
to do the default in your power domain code or you just get to pick the
default state and the power domain figures it out).  If you think about
it power domains would be pretty useless if they didn't do something
sensible here - it's not like the need to power things on before
interacting with them is specific to AMBA.  As I keep saying they're
*really* simple to use and very idiomatic, drivers don't ever need to
interact with them directly at all.  They just do system and runtime
power management.

> (AMBA devices are special in this way: no other ARM things
> support auto-detection of devices using magic numbers,
> basically the DT stuff came about because noone was using
> a thing like this.)

There's at least PCI and USB as well, with more to come very soon, and
on-SoC platform devices behave in very much the same way really.

> > The impact should be minimal since currently there are no mainline
> > systems which actually provide a vcore regulator so none need updating.

> Oh yes there are:
> drivers/mfd/db8500-prcmu.c

Oh, ick.  As you say this stuff would obviously be expected to be in
arch/arm along with the rest of the platform code and quite frankly I'm
surprised that AMBA is producing textual dev_names - it's rather unusual
for a bus not to use the numeric IDs which it enumerates with which is
why I missed the code when I grepped.  The names aren't a problem, just
surprising.

> IIRC the machine will not boot (i.e. these drivers cannot even
> probe) without these regulators in place, so they get enabled by
> the AMBA bus code.

> So we need something that not just removes stuff from the
> AMBA bus, but also adds a better power domain mechanism
> and fixes up taking the above regulators.

> That said I'm all for replacing it - but I'd need to figure out the
> details on how to do that.

> We do have code for ux500 power domains. If it will will be
> enough to handle this I can try to hack it up and submit it.

This should be totally trivial, just shift your existing regulator stuff
into the power domain and out of AMBA.

Though actually given the fact that only the pl022 driver supports
turning the domain on and off at runtime and that's not one of the
drivers bound to the switchable supply in this system you'll probably
see zero impact on actual systems if you just constrain vape to be
always_on in the first instance, then work to optimise later.  With the
current mainline code I'd expect there to be at least one AMBA device
which forces the power on anyway.
-------------- 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/f72e642f/attachment.sig>

[PATCH/RFC] ARM: amba: Remove AMBA level regulator support

[Rabin Vincent]

On Sun, Apr 1, 2012 at 22:52, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sun, Apr 1, 2012 at 5:29 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
>> The impact should be minimal since currently there are no mainline
>> systems which actually provide a vcore regulator so none need updating.
>
> Oh yes there are:
> drivers/mfd/db8500-prcmu.c
>
> This driver registers a number of voltage domain regulators,
> among those:
>
> static struct regulator_consumer_supply db8500_vape_consumers[] = {
> ? ? ? (...)
> ? ? ? ?REGULATOR_SUPPLY("vcore", "sdi0"),
> ? ? ? ?REGULATOR_SUPPLY("vcore", "sdi1"),
> ? ? ? ?REGULATOR_SUPPLY("vcore", "sdi2"),
> ? ? ? ?REGULATOR_SUPPLY("vcore", "sdi3"),
> ? ? ? ?REGULATOR_SUPPLY("vcore", "sdi4"),
> ? ? ? ?(...)
> ? ? ? ?REGULATOR_SUPPLY("vcore", "uart0"),
> ? ? ? ?REGULATOR_SUPPLY("vcore", "uart1"),
> ? ? ? ?REGULATOR_SUPPLY("vcore", "uart2"),
>
> IIRC the machine will not boot (i.e. these drivers cannot even
> probe) without these regulators in place, so they get enabled by
> the AMBA bus code.

These vcore "regulators" do nothing but increment a variable which is
write-only in mainline, so the machine will boot and drivers will probe
fine with Mark's patch.