[PATCH v4] GPIO PL061: Adding Clk framework support

[PATCH v4] GPIO PL061: Adding Clk framework support

[Viresh KUMAR]

From: Viresh KUMAR <viresh.kumar@st.com>

GPIO Clk is never enabled on Platforms, like: SPEAr, which are based upon clock
framework and use PL061 driver. This patch adds support for Clk enabling and
disabling as and when gpios are requested and freed.

Modifications in V4:
 - Not registering gpio_request and gpio_free if clk is not found.

Modifications in V3:
 - if clk_get returns -ENOENT, then work without clk enable/disable.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/gpio/pl061.c |   49 +++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/pl061.c b/drivers/gpio/pl061.c
index 105701a..b826d2b 100644
--- a/drivers/gpio/pl061.c
+++ b/drivers/gpio/pl061.c
@@ -11,7 +11,10 @@
  *
  * Data sheet: ARM DDI 0190B, September 2000
  */
+
+#include <linux/clk.h>
 #include <linux/spinlock.h>
+#include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/module.h>
 #include <linux/list.h>
@@ -56,8 +59,26 @@ struct pl061_gpio {
 	void __iomem		*base;
 	unsigned		irq_base;
 	struct gpio_chip	gc;
+	struct clk		*clk;
 };
 
+static int pl061_request(struct gpio_chip *gc, unsigned offset)
+{
+	struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
+
+	if (offset >= gc->ngpio)
+		return -EINVAL;
+
+	return clk_enable(chip->clk);
+}
+
+static void pl061_free(struct gpio_chip *gc, unsigned offset)
+{
+	struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
+
+	clk_disable(chip->clk);
+}
+
 static int pl061_direction_input(struct gpio_chip *gc, unsigned offset)
 {
 	struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
@@ -260,9 +281,18 @@ static int __init pl061_probe(struct amba_device *dev, struct amba_id *id)
 		goto release_region;
 	}
 
-	spin_lock_init(&chip->lock);
-	spin_lock_init(&chip->irq_lock);
-	INIT_LIST_HEAD(&chip->list);
+	chip->clk = clk_get(&dev->dev, NULL);
+	if (IS_ERR(chip->clk)) {
+		ret = PTR_ERR(chip->clk);
+		/* clk Not present */
+		if (ret == -ENOENT)
+			chip->clk = NULL;
+		else
+			goto iounmap;
+	} else {
+		chip->gc.request = pl061_request;
+		chip->gc.free = pl061_free;
+	}
 
 	chip->gc.direction_input = pl061_direction_input;
 	chip->gc.direction_output = pl061_direction_output;
@@ -277,9 +307,13 @@ static int __init pl061_probe(struct amba_device *dev, struct amba_id *id)
 
 	chip->irq_base = pdata->irq_base;
 
+	spin_lock_init(&chip->lock);
+	spin_lock_init(&chip->irq_lock);
+	INIT_LIST_HEAD(&chip->list);
+
 	ret = gpiochip_add(&chip->gc);
 	if (ret)
-		goto iounmap;
+		goto free_clk;
 
 	/*
 	 * irq_chip support
@@ -292,7 +326,7 @@ static int __init pl061_probe(struct amba_device *dev, struct amba_id *id)
 	irq = dev->irq[0];
 	if (irq < 0) {
 		ret = -ENODEV;
-		goto iounmap;
+		goto free_clk;
 	}
 	set_irq_chained_handler(irq, pl061_irq_handler);
 	if (!test_and_set_bit(irq, init_irq)) { /* list initialized? */
@@ -300,7 +334,7 @@ static int __init pl061_probe(struct amba_device *dev, struct amba_id *id)
 		if (chip_list == NULL) {
 			clear_bit(irq, init_irq);
 			ret = -ENOMEM;
-			goto iounmap;
+			goto free_clk;
 		}
 		INIT_LIST_HEAD(chip_list);
 		set_irq_data(irq, chip_list);
@@ -323,6 +357,9 @@ static int __init pl061_probe(struct amba_device *dev, struct amba_id *id)
 
 	return 0;
 
+free_clk:
+	if (chip->clk)
+		clk_put(chip->clk);
 iounmap:
 	iounmap(chip->base);
 release_region:
-- 
1.6.0.2

[PATCH v4] GPIO PL061: Adding Clk framework support

[Baruch Siach]

Hi Viresh,

On Tue, Jun 22, 2010 at 10:37:27AM +0530, Viresh KUMAR wrote:
> From: Viresh KUMAR <viresh.kumar@st.com>
> 
> GPIO Clk is never enabled on Platforms, like: SPEAr, which are based upon clock
> framework and use PL061 driver. This patch adds support for Clk enabling and
> disabling as and when gpios are requested and freed.
> 
> Modifications in V4:
>  - Not registering gpio_request and gpio_free if clk is not found.
> 
> Modifications in V3:
>  - if clk_get returns -ENOENT, then work without clk enable/disable.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>

Acked-by: Baruch Siach <baruch@tkos.co.il>

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH v4] GPIO PL061: Adding Clk framework support

[Russell King - ARM Linux]

On Tue, Jun 22, 2010 at 10:37:27AM +0530, Viresh KUMAR wrote:
> -	spin_lock_init(&chip->lock);
> -	spin_lock_init(&chip->irq_lock);
> -	INIT_LIST_HEAD(&chip->list);
> +	chip->clk = clk_get(&dev->dev, NULL);
> +	if (IS_ERR(chip->clk)) {
> +		ret = PTR_ERR(chip->clk);
> +		/* clk Not present */
> +		if (ret == -ENOENT)
> +			chip->clk = NULL;

Please don't assume that NULL is not a valid value for a 'struct clk'.
Eg,

struct clk *clk_get(struct device *dev, const char *id)
{
        return dev && strcmp(dev_name(dev), "mb:16") == 0 ? NULL : ERR_PTR(-ENOENT);
}

However, this patch begs the question about what you're trying to do,
as the primecell doesn't take a clock for its operation other than the
AMBA bus clock.

As I've already pointed out, the current primecell implementations do
not assume that the bus clock is switched - and so they aren't written
such that the bus clock will be fiddled with when they do clk_enable
and clk_disable on their functional clocks.

As there do seem to be platforms which can switch the bus clock to
individual primecells, we need to add this support - and using the
existing clk API calls won't do, especially as we have primecell code
which accesses registers prior to doing anything with the clk API.

How about we have the core primecell code request the bus clock on
driver probe, enable it, and then call the drivers probe method.  On
driver remove, the bus clock is disabled and put after the drivers
remove method has been called.  We provide two new function calls,
amba_bus_clk_enable() and amba_bus_clk_disable() which primecell
drivers can use to control the bus clock to the peripheral - these
can be just macros to clk_enable and clk_disable respectively.

So something like the patch below (untested):

 drivers/amba/bus.c       |   35 +++++++++++++++++++++++++++++++----
 include/linux/amba/bus.h |   10 ++++++++++
 2 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index f60b2b6..1b9f5f8 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -130,17 +130,44 @@ static int amba_probe(struct device *dev)
 {
 	struct amba_device *pcdev = to_amba_device(dev);
 	struct amba_driver *pcdrv = to_amba_driver(dev->driver);
-	struct amba_id *id;
+	struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
+	struct clk *clk;
+	int ret;
+
+	do {
+		pcdev->busck = clk = clk_get(dev, "busck");
+		if (IS_ERR(clk) && PTR_ERR(clk) != -ENOENT) {
+			ret = PTR_ERR(clk);
+			break;
+		}
+
+		if (!IS_ERR(clk)) {
+			ret = clk_enable(clk);
+			if (ret)
+				break;
+		}
 
-	id = amba_lookup(pcdrv->id_table, pcdev);
+		ret = pcdrv->probe(pcdev, id);
+		if (ret == 0)
+			break;
 
-	return pcdrv->probe(pcdev, id);
+		clk_disable(clk);
+	} while (0);
+
+	return ret;
 }
 
 static int amba_remove(struct device *dev)
 {
+	struct amba_device *pcdev = to_amba_device(dev);
 	struct amba_driver *drv = to_amba_driver(dev->driver);
-	return drv->remove(to_amba_device(dev));
+	int ret = drv->remove(pcdev);
+
+	if (!IS_ERR(pcdev->busck)) {
+		clk_disable(pcdev->busck);
+		clk_put(pcdev->busck);
+	}
+	return ret;
 }
 
 static void amba_shutdown(struct device *dev)
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index 8b10386..f880c58 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -15,13 +15,17 @@
 #define ASMARM_AMBA_H
 
 #include <linux/device.h>
+#include <linux/err.h>
 #include <linux/resource.h>
 
 #define AMBA_NR_IRQS	2
 
+struct clk;
+
 struct amba_device {
 	struct device		dev;
 	struct resource		res;
+	struct clk		*busck;
 	u64			dma_mask;
 	unsigned int		periphid;
 	unsigned int		irq[AMBA_NR_IRQS];
@@ -59,6 +63,12 @@ struct amba_device *amba_find_device(const char *, struct device *, unsigned int
 int amba_request_regions(struct amba_device *, const char *);
 void amba_release_regions(struct amba_device *);
 
+#define amba_bus_clk_enable(d)	\
+	(IS_ERR((d)->busck) ? 0 : clk_enable((d)->busck))
+
+#define amba_bus_clk_disable(d)	\
+	do { if (!IS_ERR((d)->busck)) clk_disable((d)->busck); } while (0)
+
 #define amba_config(d)	(((d)->periphid >> 24) & 0xff)
 #define amba_rev(d)	(((d)->periphid >> 20) & 0x0f)
 #define amba_manf(d)	(((d)->periphid >> 12) & 0xff)

[PATCH v4] GPIO PL061: Adding Clk framework support

[Linus Walleij]

2010/7/9 Russell King - ARM Linux <linux@arm.linux.org.uk>:

> How about we have the core primecell code request the bus clock on
> driver probe, enable it, and then call the drivers probe method. ?On
> driver remove, the bus clock is disabled and put after the drivers
> remove method has been called. ?We provide two new function calls,
> amba_bus_clk_enable() and amba_bus_clk_disable() which primecell
> drivers can use to control the bus clock to the peripheral - these
> can be just macros to clk_enable and clk_disable respectively.

This looks mostly good to me (comments below) but will have as
a side effect that all clock frameworks on all platforms using PrimeCells
will need to be initialized *before* the PrimeCell devices are added.

In U8500 we solved this by moving the clock initialization to the IRQ
initialization from the .init_irq field in the machine struct, this
change would need that (or something similar) to be done uniformly.

(I can fix it for U300 and other platforms I can test.)

> So something like the patch below (untested):
>
> ?drivers/amba/bus.c ? ? ? | ? 35 +++++++++++++++++++++++++++++++----
> ?include/linux/amba/bus.h | ? 10 ++++++++++
> ?2 files changed, 41 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index f60b2b6..1b9f5f8 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -130,17 +130,44 @@ static int amba_probe(struct device *dev)
> ?{
> ? ? ? ?struct amba_device *pcdev = to_amba_device(dev);
> ? ? ? ?struct amba_driver *pcdrv = to_amba_driver(dev->driver);
> - ? ? ? struct amba_id *id;
> + ? ? ? struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
> + ? ? ? struct clk *clk;
> + ? ? ? int ret;
> +
> + ? ? ? do {
> + ? ? ? ? ? ? ? pcdev->busck = clk = clk_get(dev, "busck");
> + ? ? ? ? ? ? ? if (IS_ERR(clk) && PTR_ERR(clk) != -ENOENT) {
> + ? ? ? ? ? ? ? ? ? ? ? ret = PTR_ERR(clk);
> + ? ? ? ? ? ? ? ? ? ? ? break;
> + ? ? ? ? ? ? ? }
> +
> + ? ? ? ? ? ? ? if (!IS_ERR(clk)) {
> + ? ? ? ? ? ? ? ? ? ? ? ret = clk_enable(clk);
> + ? ? ? ? ? ? ? ? ? ? ? if (ret)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
> + ? ? ? ? ? ? ? }
>
> - ? ? ? id = amba_lookup(pcdrv->id_table, pcdev);
> + ? ? ? ? ? ? ? ret = pcdrv->probe(pcdev, id);
> + ? ? ? ? ? ? ? if (ret == 0)
> + ? ? ? ? ? ? ? ? ? ? ? break;

So this means you always turn on the block clock at
probe, and then leave it on if the probe was successful.

>
> - ? ? ? return pcdrv->probe(pcdev, id);
> + ? ? ? ? ? ? ? clk_disable(clk);
> + ? ? ? } while (0);
> +
> + ? ? ? return ret;
> ?}
>
> ?static int amba_remove(struct device *dev)
> ?{
> + ? ? ? struct amba_device *pcdev = to_amba_device(dev);
> ? ? ? ?struct amba_driver *drv = to_amba_driver(dev->driver);
> - ? ? ? return drv->remove(to_amba_device(dev));
> + ? ? ? int ret = drv->remove(pcdev);
> +
> + ? ? ? if (!IS_ERR(pcdev->busck)) {
> + ? ? ? ? ? ? ? clk_disable(pcdev->busck);

And here it is disabled.

For drivers that need to conserve power, also the block
clock has to be gated when the device is not in use,
sometimes since that affects some hardware that keep
track of the usecount of the clock tree for a larger region of
silicon, so the power savings can be drastic.

It would be good if drivers could opt-out of leaving the
clock on after probe, some more property perhaps,
like "gateme" or so, then the driver will be responsible
to clk_enable() the clock when it's needed.

This would make it possible to convert the PrimeCell
drivers over to this behaviour later.

Another option would be to always gate them all off,
and then sprinkle clk_enable(adev->bus_clk) into every
PrimeCell driver probe() and clk_disable(adev->bus_clk) into
every PrimeCell driver remove(). This makes things more
explicit.

> + ? ? ? ? ? ? ? clk_put(pcdev->busck);
> + ? ? ? }
> + ? ? ? return ret;
> ?}

> ?static void amba_shutdown(struct device *dev)
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index 8b10386..f880c58 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -15,13 +15,17 @@
> ?#define ASMARM_AMBA_H
>
> ?#include <linux/device.h>
> +#include <linux/err.h>
> ?#include <linux/resource.h>
>
> ?#define AMBA_NR_IRQS ? 2
>
> +struct clk;
> +
> ?struct amba_device {
> ? ? ? ?struct device ? ? ? ? ? dev;
> ? ? ? ?struct resource ? ? ? ? res;
> + ? ? ? struct clk ? ? ? ? ? ? ?*busck;

Here you could add:
bool gateme

> ? ? ? ?u64 ? ? ? ? ? ? ? ? ? ? dma_mask;
> ? ? ? ?unsigned int ? ? ? ? ? ?periphid;
> ? ? ? ?unsigned int ? ? ? ? ? ?irq[AMBA_NR_IRQS];
> @@ -59,6 +63,12 @@ struct amba_device *amba_find_device(const char *, struct device *, unsigned int
> ?int amba_request_regions(struct amba_device *, const char *);
> ?void amba_release_regions(struct amba_device *);
>
> +#define amba_bus_clk_enable(d) \
> + ? ? ? (IS_ERR((d)->busck) ? 0 : clk_enable((d)->busck))
> +
> +#define amba_bus_clk_disable(d) ? ? ? ?\
> + ? ? ? do { if (!IS_ERR((d)->busck)) clk_disable((d)->busck); } while (0)
> +
> ?#define amba_config(d) (((d)->periphid >> 24) & 0xff)
> ?#define amba_rev(d) ? ?(((d)->periphid >> 20) & 0x0f)
> ?#define amba_manf(d) ? (((d)->periphid >> 12) & 0xff)
>

Yours,
Linus Walleij

[PATCH v4] GPIO PL061: Adding Clk framework support

[Russell King - ARM Linux]

On Sat, Jul 10, 2010 at 01:55:26AM +0200, Linus Walleij wrote:
> This looks mostly good to me (comments below) but will have as
> a side effect that all clock frameworks on all platforms using PrimeCells
> will need to be initialized *before* the PrimeCell devices are added.

No it doesn't - it needs the clock API to be initialized before the
first driver is probed.  As drivers are registered during the initcalls,
later than postcore level

> > - ? ? ? id = amba_lookup(pcdrv->id_table, pcdev);
> > + ? ? ? ? ? ? ? ret = pcdrv->probe(pcdev, id);
> > + ? ? ? ? ? ? ? if (ret == 0)
> > + ? ? ? ? ? ? ? ? ? ? ? break;
> 
> So this means you always turn on the block clock at
> probe, and then leave it on if the probe was successful.

Correct, which is what drivers currently expect.  This is also why
two additional amba_* callbacks are supplied - if it is appropriate
for a driver to disable their bus clock after being probed, they can
do that but they have to participate in that.

Basically, they have to call amba_bus_clk_disable() on successful
probe, and then amba_bus_clk_enable() each time that they want to
access their registers, followed by an amba_bus_clk_disable() when
they've finished.  Finally, they must call amba_bus_clk_enable()
in their remove method and avoid amba_bus_clk_disable() before
returning.

> For drivers that need to conserve power, also the block
> clock has to be gated when the device is not in use,
> sometimes since that affects some hardware that keep
> track of the usecount of the clock tree for a larger region of
> silicon, so the power savings can be drastic.

You can't have the core code doing that.  If you unconditionally turn
the bus clock off after probe, what happens when a driver receives an
interrupt and tries to access its registers?

Hint: the core code can't know that the driver has registered an IRQ
handler.

[PATCH v4] GPIO PL061: Adding Clk framework support

[Russell King - ARM Linux]

On Sat, Jul 10, 2010 at 08:19:13AM +0100, Russell King - ARM Linux wrote:
> On Sat, Jul 10, 2010 at 01:55:26AM +0200, Linus Walleij wrote:
> > This looks mostly good to me (comments below) but will have as
> > a side effect that all clock frameworks on all platforms using PrimeCells
> > will need to be initialized *before* the PrimeCell devices are added.
> 
> No it doesn't - it needs the clock API to be initialized before the
> first driver is probed.  As drivers are registered during the initcalls,
> later than postcore level

... (to finish this) ...

drivers/amba/bus.c:postcore_initcall(amba_init);

This is the earliest that any driver can be registered, as the AMBA bus
type will not be registered until that point.  So, provided the clk API
is initialized at or before postcore level, AMBA devices are fine.

What we do need to do is add a 'busck' to amba using platforms as the
NULL connection name for the function clock in the clkdev tables will
match.

[PATCH v4] GPIO PL061: Adding Clk framework support

[Linus Walleij]

2010/7/10 Russell King - ARM Linux <linux@arm.linux.org.uk>:

> Correct, which is what drivers currently expect. ?This is also why
> two additional amba_* callbacks are supplied - if it is appropriate
> for a driver to disable their bus clock after being probed, they can
> do that but they have to participate in that.

Aha I get it, then it looks good...

Yours,
Linus Walleij

[PATCH v4] GPIO PL061: Adding Clk framework support

[Viresh KUMAR]

On 7/9/2010 6:10 PM, Russell King - ARM Linux wrote:
> On Tue, Jun 22, 2010 at 10:37:27AM +0530, Viresh KUMAR wrote:
>> -	spin_lock_init(&chip->lock);
>> -	spin_lock_init(&chip->irq_lock);
>> -	INIT_LIST_HEAD(&chip->list);
>> +	chip->clk = clk_get(&dev->dev, NULL);
>> +	if (IS_ERR(chip->clk)) {
>> +		ret = PTR_ERR(chip->clk);
>> +		/* clk Not present */
>> +		if (ret == -ENOENT)
>> +			chip->clk = NULL;
> 
> Please don't assume that NULL is not a valid value for a 'struct clk'.
> Eg,
> 
> struct clk *clk_get(struct device *dev, const char *id)
> {
>         return dev && strcmp(dev_name(dev), "mb:16") == 0 ? NULL : ERR_PTR(-ENOENT);
> }

I knew this. This convention is just local to the driver.

> 
> However, this patch begs the question about what you're trying to do,
> as the primecell doesn't take a clock for its operation other than the
> AMBA bus clock.

This issue is different from the issue i raised which was common to
all amba devices. This is just for this GPIO driver, we need to enable clk
before using any gpio pin.


viresh.

[PATCH v4] GPIO PL061: Adding Clk framework support

[Linus Walleij]

2010/7/12 Viresh KUMAR <viresh.kumar@st.com>:

> This issue is different from the issue i raised which was common to
> all amba devices. This is just for this GPIO driver, we need to enable clk
> before using any gpio pin.

I think what Russell means is that all current clocks looked up by the
drivers are implicitly of types that control output signals to things like
UART RX, SPI out, etc, i.e. clocks that control signals that exit the system.

These are usually fixed, e.g. in ARM Ltd. reference designs there is a
fixed 24MHz clock for all of these.

The clock that controls the internal logic in the PrimeCell itself should
be modeled into the AMBA (PrimeCell) bus abstraction.

Yours,
Linus Walleij

[PATCH v4] GPIO PL061: Adding Clk framework support

[Russell King - ARM Linux]

On Mon, Jul 12, 2010 at 09:37:43AM +0530, Viresh KUMAR wrote:
> This issue is different from the issue i raised which was common to
> all amba devices. This is just for this GPIO driver, we need to enable clk
> before using any gpio pin.

PL061 does not have any other clock signal other than PCLK, the APB (AMBA
peripheral bus) clock.  This clock needs to be running to access any
register in an APB peripheral.  The primecell drivers and AMBA support
is currently implemented with the assumtion that this clock is
permanently running.

If you need to enable a clock before using any GPIO pin, and it's not the
APB clock, then that's a platform extension that is outside of the PL061
domain, and probably also affects alternate functions of the GPIO pins as
well.  That means the PL061 code shouldn't manage that clock signal as it
won't know what's going on with the alternate functions.

[PATCH v4] GPIO PL061: Adding Clk framework support

[Viresh KUMAR]

On 7/12/2010 1:37 PM, Russell King - ARM Linux wrote:
> On Mon, Jul 12, 2010 at 09:37:43AM +0530, Viresh KUMAR wrote:
>> > This issue is different from the issue i raised which was common to
>> > all amba devices. This is just for this GPIO driver, we need to enable clk
>> > before using any gpio pin.
> PL061 does not have any other clock signal other than PCLK, the APB (AMBA
> peripheral bus) clock.  This clock needs to be running to access any
> register in an APB peripheral.  The primecell drivers and AMBA support
> is currently implemented with the assumtion that this clock is
> permanently running.
> 
> If you need to enable a clock before using any GPIO pin, and it's not the
> APB clock, then that's a platform extension that is outside of the PL061
> domain, and probably also affects alternate functions of the GPIO pins as
> well.  That means the PL061 code shouldn't manage that clock signal as it
> won't know what's going on with the alternate functions.
> 

Russell,

I agree that this clock is outside of GPIO's domain, but this is how it
is done for all other amba and non amba devices too, like pl011, pl022,
etc. There also we are handling platform specific clocks in drivers only.

Am i missing something??

viresh.

[PATCH v4] GPIO PL061: Adding Clk framework support

[Russell King - ARM Linux]

On Mon, Jul 12, 2010 at 01:48:31PM +0530, Viresh KUMAR wrote:
> On 7/12/2010 1:37 PM, Russell King - ARM Linux wrote:
> > PL061 does not have any other clock signal other than PCLK, the APB (AMBA
> > peripheral bus) clock.  This clock needs to be running to access any
> > register in an APB peripheral.  The primecell drivers and AMBA support
> > is currently implemented with the assumtion that this clock is
> > permanently running.
> > 
> > If you need to enable a clock before using any GPIO pin, and it's not the
> > APB clock, then that's a platform extension that is outside of the PL061
> > domain, and probably also affects alternate functions of the GPIO pins as
> > well.  That means the PL061 code shouldn't manage that clock signal as it
> > won't know what's going on with the alternate functions.
> > 
> 
> Russell,
> 
> I agree that this clock is outside of GPIO's domain, but this is how it
> is done for all other amba and non amba devices too, like pl011, pl022,
> etc. There also we are handling platform specific clocks in drivers only.

No.

1. All primecells take a clock called PCLK, which is the bus clock which
   times accesses to the registers from the CPU.

2. PL011 takes a clock, called UARTCLK.  PL022 takes a clock, called SSPCLK.
   PL061 does not take such a clock.

If you look that up in the ARM documentation (DDI0183, DDI0194 and DDI0190
respectively), you'll find these signal listed in the relevant appendix -
A.1 for the bus signals and A.2 for the internal on-chip peripheral signals.

The existing clk API usage in primecell drivers covers (2).  It does not
cover (1).  As such, and as PL011 and PL022 do have non-APB clock signals,
they have clk API support.  As PL061 doesn't have non-APB clock signals,
it doesn't have clk API support and shouldn't need it.

If your device is setup such that there is a 'GPIO clock' control, it is
either relating to a control for the APB bus clock, or it is something on
the pad side of the PL061 block, or is just a dummy control bit to keep
programmers happy which does nothing.

[PATCH v4] GPIO PL061: Adding Clk framework support

[Russell King - ARM Linux]

On Sat, Jul 10, 2010 at 05:36:54PM +0200, Linus Walleij wrote:
> 2010/7/10 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> 
> > Correct, which is what drivers currently expect. ?This is also why
> > two additional amba_* callbacks are supplied - if it is appropriate
> > for a driver to disable their bus clock after being probed, they can
> > do that but they have to participate in that.
> 
> Aha I get it, then it looks good...

Is that an Ack for the patch?

I think there's one change I'd like, which is to change "busck" to
"apb_pclk" to make it clear what it's referring to.

Also, is there any chance you can convert your platform(s) over to use
it before I merge this patch?

[PATCH v4] GPIO PL061: Adding Clk framework support

[Linus Walleij]

2010/7/13 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Sat, Jul 10, 2010 at 05:36:54PM +0200, Linus Walleij wrote:

>> Aha I get it, then it looks good...
>
> Is that an Ack for the patch?

Yep! FWIW
Acked-by: Linus Walleij <linus.walleij@stericsson.com>

> I think there's one change I'd like, which is to change "busck" to
> "apb_pclk" to make it clear what it's referring to.

Looks good.

> Also, is there any chance you can convert your platform(s) over to use
> it before I merge this patch?

I can take a round over the U300 within a few days, Rabin can
maybe look at the U8500.

In the U300 (maybe the U8500 as well, I don't know OTOH) the
APB pclk and e.g. SSPCLK is simply just wired together and
connected to the same clock terminal. This means that currently
the PCLK is toggled on/off in unison with the SSPCLK all the time
already, and it's working.

Since that works, it means that we can likely insert
amba_bus_clk_[disable|enable] at the same sites that we have
this in the current code for the external clocks at the same time,
atleast for pl011, pl022 and pl180.

Yours,
Linus Walleij

[PATCH v4] GPIO PL061: Adding Clk framework support

[Russell King - ARM Linux]

On Tue, Jul 13, 2010 at 01:00:51PM +0200, Linus Walleij wrote:
> In the U300 (maybe the U8500 as well, I don't know OTOH) the
> APB pclk and e.g. SSPCLK is simply just wired together and
> connected to the same clock terminal. This means that currently
> the PCLK is toggled on/off in unison with the SSPCLK all the time
> already, and it's working.

That means you want to alias the device specific apb_pclk to the same
struct clk as (eg) the device's sspclk - which means that when the
driver wants either PCLK or SSPCLK to be on, the bit to enable them
will be turned on.

> Since that works, it means that we can likely insert
> amba_bus_clk_[disable|enable] at the same sites that we have
> this in the current code for the external clocks at the same time,
> atleast for pl011, pl022 and pl180.

Almost - but there's a few corner cases.  Basically, what I think you'll
need for pl011 is:
1. add an amba_bus_clk_disable() at the end of the successful probe
   function
2. add amba_bus_clk_enable() at the beginning of the remove function.
3. add amba_bus_clk_enable()/amba_bus_clk_disable() around each of the
   suspend(), resume(), pl011_console_write(), and
   pl011_console_get_options() functions.
4. amba_bus_clk_enable() at the start of the startup method.
5. amba_bus_clk_disable() at the end of the shutdown method.

Most of these do tie up with existing clk_enable()s, but there are some
additional places you'd need to enable the pclk because of register
accesses.

[PATCH v4] GPIO PL061: Adding Clk framework support

[Viresh KUMAR]

On 7/13/2010 11:56 PM, Russell King - ARM Linux wrote:
> On Tue, Jul 13, 2010 at 01:00:51PM +0200, Linus Walleij wrote:
>> Since that works, it means that we can likely insert
>> amba_bus_clk_[disable|enable] at the same sites that we have
>> this in the current code for the external clocks at the same time,
>> atleast for pl011, pl022 and pl180.
> 
> Almost - but there's a few corner cases.  Basically, what I think you'll
> need for pl011 is:
> 1. add an amba_bus_clk_disable() at the end of the successful probe
>    function
> 2. add amba_bus_clk_enable() at the beginning of the remove function.
> 3. add amba_bus_clk_enable()/amba_bus_clk_disable() around each of the
>    suspend(), resume(), pl011_console_write(), and
>    pl011_console_get_options() functions.
> 4. amba_bus_clk_enable() at the start of the startup method.
> 5. amba_bus_clk_disable() at the end of the shutdown method.
> 
> Most of these do tie up with existing clk_enable()s, but there are some
> additional places you'd need to enable the pclk because of register
> accesses.
> 

Russell,

These cases particularly in our architecture where functional and
interface clock are tied together (through same gating option) would
produce difficult scenarios (as already mentioned by you) and which
would be difficult and not so clean to handle.

For example just looking at driver src which disables bus clock (without
enabling it) in different scenarios would not be very readable.
Further that would vary from architecture to architecture (even for
standard drivers).

>> > For drivers that need to conserve power, also the block
>> > clock has to be gated when the device is not in use,
>> > sometimes since that affects some hardware that keep
>> > track of the usecount of the clock tree for a larger region of
>> > silicon, so the power savings can be drastic.
> You can't have the core code doing that.  If you unconditionally turn
> the bus clock off after probe, what happens when a driver receives an
> interrupt and tries to access its registers?
> 
> Hint: the core code can't know that the driver has registered an IRQ
> handler.

We haven't seen this kind of issues in our devices, SPEAr as well as
U300 (as we have both clocks controlled by same bit). Normally, when device
is not in use then interrupts are disabled. When device is used then interrupts
and clock are enabled and clocks are not disabled till the time work is
finished. So, this condition might not occur that you have landed in interrupt
handler with clocks off.

IMO, Clocks enabling is required at following places:
1. During device add where we need to read id registers.
- We can do this by simple enabling/disabling "apb_bus" clk before and
  after reading registers.
2. In driver, when device is used.
- This can be handled best by the driver itself. We don't require to
  enable interface clock from amba_probe for ever.


One way to do this cleanly is:
- Add clock structures for devices with connection id "apb_bus".
  (Platforms can have 2 different structures if they have different
controlling bits otherwise single structure will solve the purpose.)
- amba_device_register can simple get this clk and use it.
- Drivers need to enable/disable both interface and functional clock to
save maximum power.



regards,
viresh kumar.

[PATCH v4] GPIO PL061: Adding Clk framework support

[Russell King - ARM Linux]

On Thu, Jul 15, 2010 at 11:32:19AM +0530, Viresh KUMAR wrote:
> These cases particularly in our architecture where functional and
> interface clock are tied together (through same gating option) would
> produce difficult scenarios (as already mentioned by you) and which
> would be difficult and not so clean to handle.

I don't see the problem.

What I'm proposing means:

1. when the driver probe function is called, pclk is guaranteed to be
   enabled, and therefore device registers are guaranteed to be accessible.
   This clock will then stay enabled all the time the device is bound to
   the driver unless the driver wants to request that it is disabled.

2. if the driver wants to do something with its functional clock, it
   clk_get() that, and does the standard enable/disable on it at the
   appropriate time.

So, if pclk and the functional clock are bound together, then at probe
time the 'pclk' is obtained and enabled, which sets your enable bit and
increases the clk use count to 1.

When the driver gets its functional clock, which happens to be the same
as clk structure as 'pclk', calling clk_enable() increases the use count
to 2, but doesn't touch the register because the clock is already enabled.

If the driver subsequently calls clk_disable(), this decreases the use
count to 1, and because there's still one user, the clock isn't disabled.

Only if the driver wants both its pclk and functional clock disabled will
your enable bit be reset.

So, provided a driver participating in pclk control (by disabling it in
its probe function) always re-enables pclk before it accesses the device
then the pclk control is completely transparent - whether or not it's
tied to the functional clock.

If a driver isn't participating in pclk control, it continues to work as
is with no alterations - because we guarantee that pclk will be enabled
to the device whenever the driver is bound to the device.

This allows us to incrementally add pclk control to each primecell driver.

> For example just looking at driver src which disables bus clock (without
> enabling it) in different scenarios would not be very readable.
> Further that would vary from architecture to architecture (even for
> standard drivers).

How so?

> > You can't have the core code doing that.  If you unconditionally turn
> > the bus clock off after probe, what happens when a driver receives an
> > interrupt and tries to access its registers?
> > 
> > Hint: the core code can't know that the driver has registered an IRQ
> > handler.
> 
> We haven't seen this kind of issues in our devices, SPEAr as well as
> U300 (as we have both clocks controlled by same bit). Normally, when
> device is not in use then interrupts are disabled. When device is
> used then interrupts and clock are enabled and clocks are not disabled
> till the time work is finished. So, this condition might not occur that
> you have landed in interrupt handler with clocks off.

So what happens with the PL011 driver which accesses the device with
the primecell UARTCLK disabled?  Eg, when reading the procfs file in
/proc/tty/driver/ ?

What about the SPI primecell, which only enables its functional clock
when its really required?  It accesses device registers without the
functional clock enabled?

Basically, we do not guarantee that drivers will have their functional
clock enabled prior to accessing their registers.

[PATCH v4] GPIO PL061: Adding Clk framework support

[Viresh KUMAR]

On 7/15/2010 2:00 PM, Russell King - ARM Linux wrote:
> On Thu, Jul 15, 2010 at 11:32:19AM +0530, Viresh KUMAR wrote:
>> These cases particularly in our architecture where functional and
>> interface clock are tied together (through same gating option) would
>> produce difficult scenarios (as already mentioned by you) and which
>> would be difficult and not so clean to handle.
> 
> I don't see the problem.
> 
> What I'm proposing means:
> 
> 1. when the driver probe function is called, pclk is guaranteed to be
>    enabled, and therefore device registers are guaranteed to be accessible.
>    This clock will then stay enabled all the time the device is bound to
>    the driver unless the driver wants to request that it is disabled.
> 
> 2. if the driver wants to do something with its functional clock, it
>    clk_get() that, and does the standard enable/disable on it at the
>    appropriate time.
> 
> So, if pclk and the functional clock are bound together, then at probe
> time the 'pclk' is obtained and enabled, which sets your enable bit and
> increases the clk use count to 1.
> 
> When the driver gets its functional clock, which happens to be the same
> as clk structure as 'pclk', calling clk_enable() increases the use count
> to 2, but doesn't touch the register because the clock is already enabled.
> 
> If the driver subsequently calls clk_disable(), this decreases the use
> count to 1, and because there's still one user, the clock isn't disabled.
> 
> Only if the driver wants both its pclk and functional clock disabled will
> your enable bit be reset.
> 
> So, provided a driver participating in pclk control (by disabling it in
> its probe function) always re-enables pclk before it accesses the device
> then the pclk control is completely transparent - whether or not it's
> tied to the functional clock.
> 
> If a driver isn't participating in pclk control, it continues to work as
> is with no alterations - because we guarantee that pclk will be enabled
> to the device whenever the driver is bound to the device.
> 
> This allows us to incrementally add pclk control to each primecell driver.
> 
>> For example just looking at driver src which disables bus clock (without
>> enabling it) in different scenarios would not be very readable.
>> Further that would vary from architecture to architecture (even for
>> standard drivers).
> 
> How so?
> 
>>> You can't have the core code doing that.  If you unconditionally turn
>>> the bus clock off after probe, what happens when a driver receives an
>>> interrupt and tries to access its registers?
>>>
>>> Hint: the core code can't know that the driver has registered an IRQ
>>> handler.
>>
>> We haven't seen this kind of issues in our devices, SPEAr as well as
>> U300 (as we have both clocks controlled by same bit). Normally, when
>> device is not in use then interrupts are disabled. When device is
>> used then interrupts and clock are enabled and clocks are not disabled
>> till the time work is finished. So, this condition might not occur that
>> you have landed in interrupt handler with clocks off.
> 
> So what happens with the PL011 driver which accesses the device with
> the primecell UARTCLK disabled?  Eg, when reading the procfs file in
> /proc/tty/driver/ ?
> 
> What about the SPI primecell, which only enables its functional clock
> when its really required?  It accesses device registers without the
> functional clock enabled?
> 
> Basically, we do not guarantee that drivers will have their functional
> clock enabled prior to accessing their registers.
> 

I got it!!!

Just a little issue, in your patch you were enabling interface clock in
amba_probe which is called after reading peripheral id registers in
amba_device_register. We need interface clock enabled before reading these
registers.

viresh.

[PATCH v4] GPIO PL061: Adding Clk framework support

[Linus Walleij]

2010/7/15 Viresh KUMAR <viresh.kumar@st.com>:

> Just a little issue, in your patch you were enabling interface clock in
> amba_probe which is called after reading peripheral id registers in
> amba_device_register. We need interface clock enabled before reading these
> registers.

Rabin already noted this and Russell says he'll fix it before applying the
patch.

Yours,
Linus Walleij

[PATCH v4] GPIO PL061: Adding Clk framework support

[Russell King - ARM Linux]

On Thu, Jul 15, 2010 at 03:05:52PM +0530, Viresh KUMAR wrote:
> Just a little issue, in your patch you were enabling interface clock in
> amba_probe which is called after reading peripheral id registers in
> amba_device_register. We need interface clock enabled before reading these
> registers.

Yes, I've already updated my patch.  I've also renamed a the bus clock
functions so that we consistently call this clock 'pclk' after the
signal which it refers to.

I had noticed that Linus' patches were referring to 'ahb_pclk' - the
slave interface for primcells lives on APB not AHB.

 drivers/amba/bus.c       |   88 ++++++++++++++++++++++++++++++++++++----------
 include/linux/amba/bus.h |   11 ++++++
 2 files changed, 80 insertions(+), 19 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index f60b2b6..d31590e 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -122,6 +122,31 @@ static int __init amba_init(void)
 
 postcore_initcall(amba_init);
 
+static int amba_get_enable_pclk(struct amba_device *pcdev)
+{
+	struct clk *pclk = clk_get(&pcdev->dev, "apb_pclk");
+	int ret;
+
+	pcdev->pclk = pclk;
+
+	if (IS_ERR(pclk))
+		return PTR_ERR(pclk);
+
+	ret = clk_enable(pclk);
+	if (ret)
+		clk_put(pclk);
+
+	return ret;
+}
+
+static void amba_put_disable_pclk(struct amba_device *pcdev)
+{
+	struct clk *pclk = pcdev->pclk;
+
+	clk_disable(pclk);
+	clk_put(pclk);
+}
+
 /*
  * These are the device model conversion veneers; they convert the
  * device model structures to our more specific structures.
@@ -130,17 +155,33 @@ static int amba_probe(struct device *dev)
 {
 	struct amba_device *pcdev = to_amba_device(dev);
 	struct amba_driver *pcdrv = to_amba_driver(dev->driver);
-	struct amba_id *id;
+	struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
+	int ret;
 
-	id = amba_lookup(pcdrv->id_table, pcdev);
+	do {
+		ret = amba_get_enable_pclk(pcdev);
+		if (ret)
+			break;
+
+		ret = pcdrv->probe(pcdev, id);
+		if (ret == 0)
+			break;
 
-	return pcdrv->probe(pcdev, id);
+		amba_put_disable_pclk(pcdev);
+	} while (0);
+
+	return ret;
 }
 
 static int amba_remove(struct device *dev)
 {
+	struct amba_device *pcdev = to_amba_device(dev);
 	struct amba_driver *drv = to_amba_driver(dev->driver);
-	return drv->remove(to_amba_device(dev));
+	int ret = drv->remove(pcdev);
+
+	amba_put_disable_pclk(pcdev);
+
+	return ret;
 }
 
 static void amba_shutdown(struct device *dev)
@@ -203,7 +244,6 @@ static void amba_device_release(struct device *dev)
  */
 int amba_device_register(struct amba_device *dev, struct resource *parent)
 {
-	u32 pid, cid;
 	u32 size;
 	void __iomem *tmp;
 	int i, ret;
@@ -241,25 +281,35 @@ int amba_device_register(struct amba_device *dev, struct resource *parent)
 		goto err_release;
 	}
 
-	/*
-	 * Read pid and cid based on size of resource
-	 * they are located at end of region
-	 */
-	for (pid = 0, i = 0; i < 4; i++)
-		pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) << (i * 8);
-	for (cid = 0, i = 0; i < 4; i++)
-		cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) << (i * 8);
+	ret = amba_get_enable_pclk(dev);
+	if (ret == 0) {
+		u32 pid, cid;
 
-	iounmap(tmp);
+		/*
+		 * Read pid and cid based on size of resource
+		 * they are located@end of region
+		 */
+		for (pid = 0, i = 0; i < 4; i++)
+			pid |= (readl(tmp + size - 0x20 + 4 * i) & 255) <<
+				(i * 8);
+		for (cid = 0, i = 0; i < 4; i++)
+			cid |= (readl(tmp + size - 0x10 + 4 * i) & 255) <<
+				(i * 8);
 
-	if (cid == 0xb105f00d)
-		dev->periphid = pid;
+		amba_put_disable_pclk(dev);
 
-	if (!dev->periphid) {
-		ret = -ENODEV;
-		goto err_release;
+		if (cid == 0xb105f00d)
+			dev->periphid = pid;
+
+		if (!dev->periphid)
+			ret = -ENODEV;
 	}
 
+	iounmap(tmp);
+
+	if (ret)
+		goto err_release;
+
 	ret = device_add(&dev->dev);
 	if (ret)
 		goto err_release;
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index 8b10386..b0c1740 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -14,14 +14,19 @@
 #ifndef ASMARM_AMBA_H
 #define ASMARM_AMBA_H
 
+#include <linux/clk.h>
 #include <linux/device.h>
+#include <linux/err.h>
 #include <linux/resource.h>
 
 #define AMBA_NR_IRQS	2
 
+struct clk;
+
 struct amba_device {
 	struct device		dev;
 	struct resource		res;
+	struct clk		*pclk;
 	u64			dma_mask;
 	unsigned int		periphid;
 	unsigned int		irq[AMBA_NR_IRQS];
@@ -59,6 +64,12 @@ struct amba_device *amba_find_device(const char *, struct device *, unsigned int
 int amba_request_regions(struct amba_device *, const char *);
 void amba_release_regions(struct amba_device *);
 
+#define amba_pclk_enable(d)	\
+	(IS_ERR((d)->pclk) ? 0 : clk_enable((d)->pclk))
+
+#define amba_pclk_disable(d)	\
+	do { if (!IS_ERR((d)->pclk)) clk_disable((d)->pclk); } while (0)
+
 #define amba_config(d)	(((d)->periphid >> 24) & 0xff)
 #define amba_rev(d)	(((d)->periphid >> 20) & 0x0f)
 #define amba_manf(d)	(((d)->periphid >> 12) & 0xff)

[PATCH v4] GPIO PL061: Adding Clk framework support

[Rabin Vincent]

On Thu, Jul 15, 2010 at 10:56:37AM +0100, Russell King - ARM Linux wrote:
> +static int amba_get_enable_pclk(struct amba_device *pcdev)
> +{
> +	struct clk *pclk = clk_get(&pcdev->dev, "apb_pclk");
> +	int ret;
> +
> +	pcdev->pclk = pclk;
> +
> +	if (IS_ERR(pclk))
> +		return PTR_ERR(pclk);
> +
> +	ret = clk_enable(pclk);
> +	if (ret)
> +		clk_put(pclk);
> +
> +	return ret;
> +}
> +
...
> @@ -130,17 +155,33 @@ static int amba_probe(struct device *dev)
>  {
>  	struct amba_device *pcdev = to_amba_device(dev);
>  	struct amba_driver *pcdrv = to_amba_driver(dev->driver);
> -	struct amba_id *id;
> +	struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
> +	int ret;
>  
> -	id = amba_lookup(pcdrv->id_table, pcdev);
> +	do {
> +		ret = amba_get_enable_pclk(pcdev);
> +		if (ret)
> +			break;
> +
> +		ret = pcdrv->probe(pcdev, id);
> +		if (ret == 0)
> +			break;
>  
> -	return pcdrv->probe(pcdev, id);
> +		amba_put_disable_pclk(pcdev);
> +	} while (0);
> +
> +	return ret;
>  }

In your earlier patch, you proceeded with the probe if the error from
clk_get() was -ENOENT, but not in this version.  Isn't the -ENOENT
special handling preferable, since it avoids the need to change all
existing platforms?

[PATCH v4] GPIO PL061: Adding Clk framework support

[Russell King - ARM Linux]

On Thu, Jul 15, 2010 at 09:39:22PM +0530, Rabin Vincent wrote:
> In your earlier patch, you proceeded with the probe if the error from
> clk_get() was -ENOENT, but not in this version.  Isn't the -ENOENT
> special handling preferable, since it avoids the need to change all
> existing platforms?

Well, the problem is that most platforms now declare their clocks
such that asking for the pclk will match the functional clock.

I have a patch which adds dummy pclks to all ARM AMBA using platforms.
This patch comes before the pclk-using patch to so that things
shouldn't break.

[PATCH v4] GPIO PL061: Adding Clk framework support

[Kevin Wells]

>  /*
>   * These are the device model conversion veneers; they convert the
>   * device model structures to our more specific structures.
> @@ -130,17 +155,33 @@ static int amba_probe(struct device *dev)
>  {
>  	struct amba_device *pcdev = to_amba_device(dev);
>  	struct amba_driver *pcdrv = to_amba_driver(dev->driver);
> -	struct amba_id *id;
> +	struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
> +	int ret;
> 
> -	id = amba_lookup(pcdrv->id_table, pcdev);
> +	do {
> +		ret = amba_get_enable_pclk(pcdev);
> +		if (ret)
> +			break;
> +
> +		ret = pcdrv->probe(pcdev, id);
> +		if (ret == 0)
> +			break;
> 
> -	return pcdrv->probe(pcdev, id);
> +		amba_put_disable_pclk(pcdev);

Should the AMBA clock be disabled if the probe() fails?

> +	} while (0);
> +
> +	return ret;
>  }
> 

[PATCH v4] GPIO PL061: Adding Clk framework support

[Russell King - ARM Linux]

On Fri, Jul 30, 2010 at 01:22:58AM +0200, Kevin Wells wrote:
> >  /*
> >   * These are the device model conversion veneers; they convert the
> >   * device model structures to our more specific structures.
> > @@ -130,17 +155,33 @@ static int amba_probe(struct device *dev)
> >  {
> >  	struct amba_device *pcdev = to_amba_device(dev);
> >  	struct amba_driver *pcdrv = to_amba_driver(dev->driver);
> > -	struct amba_id *id;
> > +	struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
> > +	int ret;
> > 
> > -	id = amba_lookup(pcdrv->id_table, pcdev);
> > +	do {
> > +		ret = amba_get_enable_pclk(pcdev);
> > +		if (ret)
> > +			break;
> > +
> > +		ret = pcdrv->probe(pcdev, id);
> > +		if (ret == 0)
> > +			break;
> > 
> > -	return pcdrv->probe(pcdev, id);
> > +		amba_put_disable_pclk(pcdev);
> 
> Should the AMBA clock be disabled if the probe() fails?

This is exactly what happens here.

[PATCH v4] GPIO PL061: Adding Clk framework support

[Linus Walleij]

2010/7/30 Kevin Wells <kevin.wells@nxp.com>:

>> - ? ? id = amba_lookup(pcdrv->id_table, pcdev);
>> + ? ? do {
>> + ? ? ? ? ? ? ret = amba_get_enable_pclk(pcdev);
>> + ? ? ? ? ? ? if (ret)
>> + ? ? ? ? ? ? ? ? ? ? break;
>> +
>> + ? ? ? ? ? ? ret = pcdrv->probe(pcdev, id);
>> + ? ? ? ? ? ? if (ret == 0)
>> + ? ? ? ? ? ? ? ? ? ? break;
>>
>> - ? ? return pcdrv->probe(pcdev, id);
>> + ? ? ? ? ? ? amba_put_disable_pclk(pcdev);
>
> Should the AMBA clock be disabled if the probe() fails?

Yes. This is the pclk for this one PrimeCell. It is first turned on,
then the driver attempts to probe and in case that fails there
obviously is no driver for this piece of silicon so why should it
be clocked?

Yours,
Linus Walleij

[PATCH v4] GPIO PL061: Adding Clk framework support

[Kevin Wells]

> Cc: Viresh KUMAR; rabin at rab.in; Linus Walleij; baruch at tkos.co.il;
> linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH v4] GPIO PL061: Adding Clk framework support
> 
> On Fri, Jul 30, 2010 at 01:22:58AM +0200, Kevin Wells wrote:
> > >  /*
> > >   * These are the device model conversion veneers; they convert the
> > >   * device model structures to our more specific structures.
> > > @@ -130,17 +155,33 @@ static int amba_probe(struct device *dev)
> > >  {
> > >  	struct amba_device *pcdev = to_amba_device(dev);
> > >  	struct amba_driver *pcdrv = to_amba_driver(dev->driver);
> > > -	struct amba_id *id;
> > > +	struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
> > > +	int ret;
> > >
> > > -	id = amba_lookup(pcdrv->id_table, pcdev);
> > > +	do {
> > > +		ret = amba_get_enable_pclk(pcdev);
> > > +		if (ret)
> > > +			break;
> > > +
> > > +		ret = pcdrv->probe(pcdev, id);
> > > +		if (ret == 0)
> > > +			break;
> > >
> > > -	return pcdrv->probe(pcdev, id);
> > > +		amba_put_disable_pclk(pcdev);
> >
> > Should the AMBA clock be disabled if the probe() fails?
> 
> This is exactly what happens here.

I think I got my logic reversed..

One more item I noted. These changes want to keep the clock enabled
after the probe - only disabling the clock on amba_remove. Both the
AMBA peripheral driver (ie, the LCD or MMCI driver) and the amba_remove
function now disable the clocks.

Wouldn't drivers that use clk_disable in their xxx_suspend functions
have some problems with this as multiple clk_enables have been called?

ie,
AMBA enables clock in amba_probe() <<< clock use count = 1
pl022 driver enables clock in mmci_probe <<< clock use count = 2
...SPI stuff happens...
...Linux enters suspend mode...
pl022 driver enters suspend mode, disables clock <<< clock use count = 1

Because the AMBA driver enabled the clock, the clock driver would still
see a use count > 1 on suspend and keep the clock enabled. Should the
suspend and resume functions in bus.c also disable/enable clocks? Or
maybe the peripheral drivers should just handle clocking and the clock
enable in amba_probe should be removed?

[PATCH v4] GPIO PL061: Adding Clk framework support

[Linus Walleij]

2010/8/3 Kevin Wells <kevin.wells@nxp.com>:

> Wouldn't drivers that use clk_disable in their xxx_suspend functions
> have some problems with this as multiple clk_enables have been called?

Russell already answered that, all drivers have to be augmented
in the following way (this is for PL011, the same will be valid for
any other PrimeCell with aggressive clocking):

[Russell King]
> 1. add an amba_bus_clk_disable() at the end of the successful probe
>  function
> 2. add amba_bus_clk_enable() at the beginning of the remove function.
> 3. add amba_bus_clk_enable()/amba_bus_clk_disable() around each of the
> suspend(), resume(), pl011_console_write(), and
>  pl011_console_get_options() functions.
> 4. amba_bus_clk_enable() at the start of the startup method.
> 5. amba_bus_clk_disable() at the end of the shutdown method.

> AMBA enables clock in amba_probe() <<< clock use count = 1
> pl022 driver enables clock in mmci_probe <<< clock use count = 2
> ...SPI stuff happens...
> ...Linux enters suspend mode...
> pl022 driver enters suspend mode, disables clock <<< clock use count = 1

OK I feel some responsibility for that driver so I will try to fix it,
maybe I can take a round@PL011 and perhaps MMCI as well.

Yours,
Linus Walleij

[PATCH v4] GPIO PL061: Adding Clk framework support

[Kevin Wells]

> 2010/8/3 Kevin Wells <kevin.wells@nxp.com>:
>
>> Wouldn't drivers that use clk_disable in their xxx_suspend functions
>> have some problems with this as multiple clk_enables have been called?
>
> Russell already answered that, all drivers have to be augmented
> in the following way (this is for PL011, the same will be valid for
> any other PrimeCell with aggressive clocking):
>
> [Russell King]
>> 1. add an amba_bus_clk_disable() at the end of the successful probe
>> ?function
>> 2. add amba_bus_clk_enable() at the beginning of the remove function.
>> 3. add amba_bus_clk_enable()/amba_bus_clk_disable() around each of the
>> suspend(), resume(), pl011_console_write(), and
>> ?pl011_console_get_options() functions.
>> 4. amba_bus_clk_enable() at the start of the startup method.
>> 5. amba_bus_clk_disable() at the end of the shutdown method.
>
>> AMBA enables clock in amba_probe() <<< clock use count = 1
>> pl022 driver enables clock in mmci_probe <<< clock use count = 2
>> ...SPI stuff happens...
>> ...Linux enters suspend mode...
>> pl022 driver enters suspend mode, disables clock <<< clock use count = 1
>
> OK I feel some responsibility for that driver so I will try to fix it,
> maybe I can take a round at PL011 and perhaps MMCI as well.
>

Thanks for the info Linus. I'll take an initial try at the pl11x driver..

Kevin

[PATCH v4] GPIO PL061: Adding Clk framework support

[Russell King - ARM Linux]

On Tue, Aug 03, 2010 at 02:40:39AM +0200, Kevin Wells wrote:
> One more item I noted. These changes want to keep the clock enabled
> after the probe - only disabling the clock on amba_remove. Both the
> AMBA peripheral driver (ie, the LCD or MMCI driver) and the amba_remove
> function now disable the clocks.

Absolutely correct.  If we enable the clock for the probe call, and then
disable it afterwards, then what do AMBA bus drivers which have registered
interrupt routines in their probe function do when they receive an
interrupt and want to access their statuc register?

Do we just let them go ahead and fault?  That's just stupid.

No, what we do is create a setup where existing drivers continue to work
without modification - and that means ensuring that the bus clock is
enabled for the duration that the driver is active.

As I've already said, if drivers then want to achieve greater power savings,
they need to use the supplied functions to manage the bus clock - with the
assumption that they will be called with the bus clock already enabled.
If a driver doesn't participate in managing the APB clock, then they don't
need to be modified.

> Wouldn't drivers that use clk_disable in their xxx_suspend functions
> have some problems with this as multiple clk_enables have been called?

Probably, and I left this intentionally open for the time being because
it wasn't clear whether the right action was to disable the bus clock
across a suspend.

However, thinking about it some more, it's probably safe to disable the
bus clock _after_ the driver specific suspend call has successfully
completed, and re-enable it before calling the resume method.  So let's
do this...