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

[PATCH/RFC v2] 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.

This feature is only used with the vape supply of the db8500 PRCMU
driver which supplies the UARTs and MMC controllers, none of which have
support for managing vcore at runtime in mainline (only pl022 SPI
controller does).  Update that supply to have an always_on constraint
until the power domain support for the system is updated so that it is
enabled for these users, this is likely to have no impact on practical
systems as probably at least one of these devices will be active and
cause AMBA to hold the supply on anyway.

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

Updated to add the always_on constraint for db8500-prcmu as discussed in
the changelog.

 drivers/amba/bus.c         |   42 +-----------------------------------------
 drivers/mfd/db8500-prcmu.c |    1 +
 drivers/spi/spi-pl022.c    |    2 --
 include/linux/amba/bus.h   |    8 --------
 4 files changed, 2 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/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
index ebc1e86..5be3248 100644
--- a/drivers/mfd/db8500-prcmu.c
+++ b/drivers/mfd/db8500-prcmu.c
@@ -2788,6 +2788,7 @@ static struct regulator_init_data db8500_regulators[DB8500_NUM_REGULATORS] = {
 		.constraints = {
 			.name = "db8500-vape",
 			.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+			.always_on = true,
 		},
 		.consumer_supplies = db8500_vape_consumers,
 		.num_consumer_supplies = ARRAY_SIZE(db8500_vape_consumers),
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

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

[Linus Walleij]

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

> diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
> index ebc1e86..5be3248 100644
> --- a/drivers/mfd/db8500-prcmu.c
> +++ b/drivers/mfd/db8500-prcmu.c
> @@ -2788,6 +2788,7 @@ static struct regulator_init_data db8500_regulators[DB8500_NUM_REGULATORS] = {
>                .constraints = {
>                        .name = "db8500-vape",
>                        .valid_ops_mask = REGULATOR_CHANGE_STATUS,
> +                       .always_on = true,
>                },
>                .consumer_supplies = db8500_vape_consumers,
>                .num_consumer_supplies = ARRAY_SIZE(db8500_vape_consumers),

Combined with the PL022 patch this causes a power regression since
the PL022 is hereafter always on.

But I guess if I fix a power domain patch to accomplish much the
same things then nothing is really lost...

And I do like the change, if for nothing else so for the fact that it
eventually pushes to power domains what belongs there, so:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

But to the defence: power domain code was not in the kernel
when the AMBA "vcore" regulator was introduced so how else
could we do it... except for inventing power domains...

Yours,
Linus Walleij

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

[Rabin Vincent]

On Mon, Apr 2, 2012 at 00:52, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sun, Apr 1, 2012 at 8:58 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
>
>> diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
>> index ebc1e86..5be3248 100644
>> --- a/drivers/mfd/db8500-prcmu.c
>> +++ b/drivers/mfd/db8500-prcmu.c
>> @@ -2788,6 +2788,7 @@ static struct regulator_init_data db8500_regulators[DB8500_NUM_REGULATORS] = {
>>                .constraints = {
>>                        .name = "db8500-vape",
>>                        .valid_ops_mask = REGULATOR_CHANGE_STATUS,
>> +                       .always_on = true,
>>                },
>>                .consumer_supplies = db8500_vape_consumers,
>>                .num_consumer_supplies = ARRAY_SIZE(db8500_vape_consumers),
>
> Combined with the PL022 patch this causes a power regression since
> the PL022 is hereafter always on.

How can there be a "power regression" here?  This is the only user of
the vcore regulator, and, apart from the fact that its disable routine
only decrements an essentialy write-only variable, it is shared which
several devices which already never disable it.

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

[Mark Brown]

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

On Sun, Apr 01, 2012 at 09:22:50PM +0200, Linus Walleij wrote:

> Combined with the PL022 patch this causes a power regression since
> the PL022 is hereafter always on.

I guess this code isn't in mainline, though?  In that case you can
always add a revert of this commit to your out of tree patches if you
need to.

> But I guess if I fix a power domain patch to accomplish much the
> same things then nothing is really lost...

Let me know if you run into any trouble with that - I don't have any
systems which could usefully use such support so I'm unlikely to work on
it directly and from what you were saying you'll need to integrate with
existing power domain code anyway.

> And I do like the change, if for nothing else so for the fact that it
> eventually pushes to power domains what belongs there, so:

> Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks.

> But to the defence: power domain code was not in the kernel
> when the AMBA "vcore" regulator was introduced so how else
> could we do it... except for inventing power domains...

Which might've happened sooner if we'd noticed :)  There were some other
platforms doing similar things but they mostly used the clock API since
it was always entirely platform code until 3.4 so they're less intrusive
into the generic code.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Linus Walleij]

On Sun, Apr 1, 2012 at 11:27 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Sun, Apr 01, 2012 at 09:22:50PM +0200, Linus Walleij wrote:
>
>> Combined with the PL022 patch this causes a power regression since
>> the PL022 is hereafter always on.
>
> I guess this code isn't in mainline, though?  In that case you can
> always add a revert of this commit to your out of tree patches if you
> need to.

No, we can sure live with it... Out-of-mainline we do use power domains
so that's what we should do instead. It currently looks like this:
http://www.igloocommunity.org/gitweb/?p=kernel/igloo-kernel.git;a=blob;f=arch/arm/mach-ux500/pm/runtime.c;hb=HEAD

It's a really nice piece of code but uses some out-of-tree features,
the most obvious one is "atomic regulators" (which are exactly
that).

>> But to the defence: power domain code was not in the kernel
>> when the AMBA "vcore" regulator was introduced so how else
>> could we do it... except for inventing power domains...
>
> Which might've happened sooner if we'd noticed :)  There were some other
> platforms doing similar things but they mostly used the clock API since
> it was always entirely platform code until 3.4 so they're less intrusive
> into the generic code.

Yeah ... but this sounds familiar, (searching searching) Yes! We did ask on
the lists if regulators were proper for modeling power domains in 2008:
http://marc.info/?l=linux-arm-kernel&m=121580531500758&w=2

But I should've pushed for a proper answer ...

Yours,
Linus Walleij

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

[Linus Walleij]

On Sun, Apr 1, 2012 at 9:39 PM, Rabin Vincent <rabin@rab.in> wrote:
> On Mon, Apr 2, 2012 at 00:52, Linus Walleij <linus.walleij@linaro.org> wrote:
>> Combined with the PL022 patch this causes a power regression since
>> the PL022 is hereafter always on.
>
> How can there be a "power regression" here?  This is the only user of
> the vcore regulator, and, apart from the fact that its disable routine
> only decrements an essentialy write-only variable, it is shared which
> several devices which already never disable it.

Yes true... Hm I hope something else in mainline will increase refcount
to vape so it's not disabled at regulator_has_full_constraints()? Better
test it, then I'll see.

Mark: as Rabin says the v1 patch is probably fine, are you pushing this
to ARM SoC or into Russell's patch tracker?

Yours,
Linus Walleij

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

[Mark Brown]

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

On Mon, Apr 02, 2012 at 12:39:11AM +0200, Linus Walleij wrote:

> Mark: as Rabin says the v1 patch is probably fine, are you pushing this
> to ARM SoC or into Russell's patch tracker?

I was waiting for some pronouncement from Russell - looking at the git
logs it seems the patch tracker is the normal way to merge things.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Mark Brown]

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

On Mon, Apr 02, 2012 at 12:33:20AM +0200, Linus Walleij wrote:

> Yeah ... but this sounds familiar, (searching searching) Yes! We did ask on
> the lists if regulators were proper for modeling power domains in 2008:
> http://marc.info/?l=linux-arm-kernel&m=121580531500758&w=2

> But I should've pushed for a proper answer ...

It's not unreasonable to use regulators to model the domains (or as part
of modelling the domains, I'd imagine some systems will be able to do
things to regulators as a result of power domain actions).  So long as
the hookup which might affect other systems is done from SoC specific
code and doesn't impact other systems) there shouldn't be an issue from
a framework point of view.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Russell King - ARM Linux]

On Mon, Apr 02, 2012 at 11:14:28AM +0100, Mark Brown wrote:
> On Mon, Apr 02, 2012 at 12:39:11AM +0200, Linus Walleij wrote:
> 
> > Mark: as Rabin says the v1 patch is probably fine, are you pushing this
> > to ARM SoC or into Russell's patch tracker?
> 
> I was waiting for some pronouncement from Russell - looking at the git
> logs it seems the patch tracker is the normal way to merge things.

Well, this stuff was invented by Linus, so its Linus' decision about
how to deal with the power control.

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

[Mark Brown]

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

On Mon, Apr 02, 2012 at 11:43:57AM +0100, Russell King - ARM Linux wrote:
> On Mon, Apr 02, 2012 at 11:14:28AM +0100, Mark Brown wrote:

> > I was waiting for some pronouncement from Russell - looking at the git
> > logs it seems the patch tracker is the normal way to merge things.

> Well, this stuff was invented by Linus, so its Linus' decision about
> how to deal with the power control.

Linus already acked the patch, the question was more about how to get it
merged.  Since most of the AMBA changes seem to go via your tree I've
just submitted it to the patch system as 7366/1.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Linus Walleij]

On Mon, Apr 2, 2012 at 12:43 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Apr 02, 2012 at 11:14:28AM +0100, Mark Brown wrote:
>> On Mon, Apr 02, 2012 at 12:39:11AM +0200, Linus Walleij wrote:
>>
>> > Mark: as Rabin says the v1 patch is probably fine, are you pushing this
>> > to ARM SoC or into Russell's patch tracker?
>>
>> I was waiting for some pronouncement from Russell - looking at the git
>> logs it seems the patch tracker is the normal way to merge things.
>
> Well, this stuff was invented by Linus, so its Linus' decision about
> how to deal with the power control.

This falls into the "seemed like a good idea at the time" category,
I was wrong, we implemented power domains instead, so let's just
kill it before someone else hurts him/herself on it...

Yours,
Linus Walleij

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

[Russell King - ARM Linux]

On Mon, Apr 02, 2012 at 12:34:13PM +0100, Mark Brown wrote:
> On Mon, Apr 02, 2012 at 11:43:57AM +0100, Russell King - ARM Linux wrote:
> > On Mon, Apr 02, 2012 at 11:14:28AM +0100, Mark Brown wrote:
> 
> > > I was waiting for some pronouncement from Russell - looking at the git
> > > logs it seems the patch tracker is the normal way to merge things.
> 
> > Well, this stuff was invented by Linus, so its Linus' decision about
> > how to deal with the power control.
> 
> Linus already acked the patch, the question was more about how to get it
> merged.  Since most of the AMBA changes seem to go via your tree I've
> just submitted it to the patch system as 7366/1.

Does:

ARM PRIMECELL BUS SUPPORT
M:      Russell King <linux@arm.linux.org.uk>
S:      Maintained
F:      drivers/amba/
F:      include/linux/amba/bus.h

not provide enough clues?  Should I delete all my entries in that file
because the only people who look in there are spammers when the file is
published on the web?

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

[Mark Brown]

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

On Mon, Apr 02, 2012 at 05:57:42PM +0100, Russell King - ARM Linux wrote:

> ARM PRIMECELL BUS SUPPORT
> M:      Russell King <linux@arm.linux.org.uk>
> S:      Maintained
> F:      drivers/amba/
> F:      include/linux/amba/bus.h

> not provide enough clues?  Should I delete all my entries in that file
> because the only people who look in there are spammers when the file is
> published on the web?

I did look in there and also in the git history which shows the same
information, hence my CCing you.  Linus introduced some doubt into my
mind that this might be something that went through arm-soc now rather
than via your tree even if you are still maintaining it as no specific
git tree or anything is mentioned there.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Shawn Guo]

On Sun, Apr 01, 2012 at 07:58:40PM +0100, Mark Brown 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.
> 
> This feature is only used with the vape supply of the db8500 PRCMU
> driver which supplies the UARTs and MMC controllers, none of which have
> support for managing vcore at runtime in mainline (only pl022 SPI
> controller does).  Update that supply to have an always_on constraint
> until the power domain support for the system is updated so that it is
> enabled for these users, this is likely to have no impact on practical
> systems as probably at least one of these devices will be active and
> cause AMBA to hold the supply on anyway.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Tested-by: Shawn Guo <shawn.guo@linaro.org>

Russell,

I believe this patch fixes the amba probe failure that impacts any
AMBA platform build with CONFIG_REGULATOR enabled.  Could you consider
to send the fix for -rc soon?

The patch "ARM: amba: adapt to regulator probe deferral change" from
me could be ignored now.

-- 
Regards,
Shawn

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

[Rob Herring]

On 04/01/2012 01:58 PM, Mark Brown 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.
> 
> This feature is only used with the vape supply of the db8500 PRCMU
> driver which supplies the UARTs and MMC controllers, none of which have
> support for managing vcore at runtime in mainline (only pl022 SPI
> controller does).  Update that supply to have an always_on constraint
> until the power domain support for the system is updated so that it is
> enabled for these users, this is likely to have no impact on practical
> systems as probably at least one of these devices will be active and
> cause AMBA to hold the supply on anyway.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---

This and pl011 fixes are needed to get highbank working with 3.4, so:

Tested-by: Rob Herring <rob.herring@calxeda.com>

Rob

> 
> Updated to add the always_on constraint for db8500-prcmu as discussed in
> the changelog.
> 
>  drivers/amba/bus.c         |   42 +-----------------------------------------
>  drivers/mfd/db8500-prcmu.c |    1 +
>  drivers/spi/spi-pl022.c    |    2 --
>  include/linux/amba/bus.h   |    8 --------
>  4 files changed, 2 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/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
> index ebc1e86..5be3248 100644
> --- a/drivers/mfd/db8500-prcmu.c
> +++ b/drivers/mfd/db8500-prcmu.c
> @@ -2788,6 +2788,7 @@ static struct regulator_init_data db8500_regulators[DB8500_NUM_REGULATORS] = {
>  		.constraints = {
>  			.name = "db8500-vape",
>  			.valid_ops_mask = REGULATOR_CHANGE_STATUS,
> +			.always_on = true,
>  		},
>  		.consumer_supplies = db8500_vape_consumers,
>  		.num_consumer_supplies = ARRAY_SIZE(db8500_vape_consumers),
> 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)

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

[Russell King - ARM Linux]

On Fri, Apr 06, 2012 at 03:49:45PM +0800, Shawn Guo wrote:
> I believe this patch fixes the amba probe failure that impacts any
> AMBA platform build with CONFIG_REGULATOR enabled.  Could you consider
> to send the fix for -rc soon?

No idea.  I don't have Mark's patch in the patch system.

> The patch "ARM: amba: adapt to regulator probe deferral change" from
> me could be ignored now.

But I do have yours in there.  And what do I do with your other patch
in there touching this same area as well?

Please, sort it out between you and once the patch system is right, let
me know.

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

[Mark Brown]

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

On Tue, Apr 10, 2012 at 09:31:30AM +0100, Russell King - ARM Linux wrote:
> On Fri, Apr 06, 2012 at 03:49:45PM +0800, Shawn Guo wrote:
> > I believe this patch fixes the amba probe failure that impacts any
> > AMBA platform build with CONFIG_REGULATOR enabled.  Could you consider
> > to send the fix for -rc soon?

> No idea.  I don't have Mark's patch in the patch system.

It's there as far as I can tell - it's 7366/1, which is visible in the
web interface at:

http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=7366/1

Is there something else I need to do?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Russell King - ARM Linux]

On Tue, Apr 10, 2012 at 09:35:43AM +0100, Mark Brown wrote:
> On Tue, Apr 10, 2012 at 09:31:30AM +0100, Russell King - ARM Linux wrote:
> > On Fri, Apr 06, 2012 at 03:49:45PM +0800, Shawn Guo wrote:
> > > I believe this patch fixes the amba probe failure that impacts any
> > > AMBA platform build with CONFIG_REGULATOR enabled.  Could you consider
> > > to send the fix for -rc soon?
> 
> > No idea.  I don't have Mark's patch in the patch system.
> 
> It's there as far as I can tell - it's 7366/1, which is visible in the
> web interface at:
> 
> http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=7366/1
> 
> Is there something else I need to do?

Ok, I thought 7366/1 and 7367/1 were part of the same looking at just the
index.

In any case, what would really help is if people would discard patches
they no longer want applying.

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

[Shawn Guo]

On Tue, Apr 10, 2012 at 09:47:02AM +0100, Russell King - ARM Linux wrote:
... 
> In any case, what would really help is if people would discard patches
> they no longer want applying.

Ok, I did not know it before.  Just marked my patch "discarded".

-- 
Regards,
Shawn

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

[Ulf Hansson]

Hi Mark,

I realize what the intention is with this patch; but I am not sure we 
have thought about all consequence it might have.

OK, so the power domain which implement runtime PM support, shall be 
responsible for handling the vcore regualtor instead of the amba bus. 
That should be fine.

But, how should those amba drivers that implements runtime PM support be 
able to switch of the vcore regulator during normal suspend? In normal 
suspend case we can not use pm_runtime_put/pm_runtime_put_sync to 
trigger the power domain runtime functions to switch of vcore. This is 
kind of more generic problem when dealing with power domains, but as 
said this patch will have consequences.

As far as I can see, the power domain must then implement a 
suspend_noirq function to make sure same things is done as for the 
runtime_suspend function. Do you agree with this as well or is there 
another option?

Kind regards
Ulf Hansson

On 04/01/2012 08:58 PM, Mark Brown 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.
>
> This feature is only used with the vape supply of the db8500 PRCMU
> driver which supplies the UARTs and MMC controllers, none of which have
> support for managing vcore at runtime in mainline (only pl022 SPI
> controller does).  Update that supply to have an always_on constraint
> until the power domain support for the system is updated so that it is
> enabled for these users, this is likely to have no impact on practical
> systems as probably at least one of these devices will be active and
> cause AMBA to hold the supply on anyway.
>
> Signed-off-by: Mark Brown<broonie@opensource.wolfsonmicro.com>
> ---
>
> Updated to add the always_on constraint for db8500-prcmu as discussed in
> the changelog.
>
>   drivers/amba/bus.c         |   42 +-----------------------------------------
>   drivers/mfd/db8500-prcmu.c |    1 +
>   drivers/spi/spi-pl022.c    |    2 --
>   include/linux/amba/bus.h   |    8 --------
>   4 files changed, 2 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/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
> index ebc1e86..5be3248 100644
> --- a/drivers/mfd/db8500-prcmu.c
> +++ b/drivers/mfd/db8500-prcmu.c
> @@ -2788,6 +2788,7 @@ static struct regulator_init_data db8500_regulators[DB8500_NUM_REGULATORS] = {
>   		.constraints = {
>   			.name = "db8500-vape",
>   			.valid_ops_mask = REGULATOR_CHANGE_STATUS,
> +			.always_on = true,
>   		},
>   		.consumer_supplies = db8500_vape_consumers,
>   		.num_consumer_supplies = ARRAY_SIZE(db8500_vape_consumers),
> 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)

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

[Mark Brown]

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

On Fri, Apr 13, 2012 at 11:03:56AM +0200, Ulf Hansson wrote:

> But, how should those amba drivers that implements runtime PM
> support be able to switch of the vcore regulator during normal
> suspend? In normal suspend case we can not use

A generic AMBA driver should have no idea about the implementation of
the particular SoC that it's integrated on to.  This applies even more
to system suspend (where drivers can generally just assume that they
will loose all power normally) than it does to runtime suspend.

> pm_runtime_put/pm_runtime_put_sync to trigger the power domain
> runtime functions to switch of vcore. This is kind of more generic
> problem when dealing with power domains, but as said this patch will
> have consequences.

The power domain gets callbacks on the system suspend path too and can
do whatever is sensible there.

> As far as I can see, the power domain must then implement a
> suspend_noirq function to make sure same things is done as for the
> runtime_suspend function. Do you agree with this as well or is there
> another option?

Yes, the power domain should just be handling this transparently.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Ulf Hansson]

On 04/13/2012 11:18 AM, Mark Brown wrote:
> On Fri, Apr 13, 2012 at 11:03:56AM +0200, Ulf Hansson wrote:
>
>> But, how should those amba drivers that implements runtime PM
>> support be able to switch of the vcore regulator during normal
>> suspend? In normal suspend case we can not use
>
> A generic AMBA driver should have no idea about the implementation of
> the particular SoC that it's integrated on to.  This applies even more
> to system suspend (where drivers can generally just assume that they
> will loose all power normally) than it does to runtime suspend.
>
>> pm_runtime_put/pm_runtime_put_sync to trigger the power domain
>> runtime functions to switch of vcore. This is kind of more generic
>> problem when dealing with power domains, but as said this patch will
>> have consequences.
>
> The power domain gets callbacks on the system suspend path too and can
> do whatever is sensible there.
>
>> As far as I can see, the power domain must then implement a
>> suspend_noirq function to make sure same things is done as for the
>> runtime_suspend function. Do you agree with this as well or is there
>> another option?
>
> Yes, the power domain should just be handling this transparently.

Alright, thanks for your confirmation. Let's see how this works out then.

Kind regards
Ulf Hansson