GPIO PL061: Adding Clk framework support

From: Viresh KUMAR <[email protected]>

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 <[email protected]>

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(-)

drivers/amba/bus.c +11 -1

index f60b2b6..1b9f5f8 100644
@@ -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;
+ ? ? ? ? ? ? ? }
+

drivers/amba/bus.c +4 -1

- ? ? ? return pcdrv->probe(pcdev, id);
+ ? ? ? ? ? ? ? clk_disable(clk);
+ ? ? ? } while (0);
+
+ ? ? ? return ret;
 ?}

drivers/amba/bus.c +3 -0

+ ? ? ? ? ? ? ? clk_put(pcdev->busck);
+ ? ? ? }
+ ? ? ? return ret;
 ?}
 
 ?static void amba_shutdown(struct device *dev)

include/linux/amba/bus.h +0 -0

index 8b10386..f880c58 100644
@@ -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

include/linux/amba/bus.h +2 -0

+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];

include/linux/amba/bus.h +0 -0

@@ -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 *);

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.

Quoted 4 lines

- ? ? ? 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.

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.

Quoted 3 lines · Linus Walleij

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

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.

2010/7/10 Russell King - ARM Linux <[email protected]>:

Quoted 4 lines

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...

Quoted 2 lines

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?

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.

Quoted 4 lines · Linus Walleij

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.

Quoted 5 lines

Quoted 5 lines

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.

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).

Quoted 6 lines

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.

Quoted 4 lines · Viresh KUMAR

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.

Quoted 4 lines

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?

Quoted 7 lines

Quoted 6 lines

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.

Quoted 6 lines

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.

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.

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.

drivers/amba/bus.c +17 -0

+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;
+}
+

drivers/amba/bus.c +7 -2

@@ -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;
+

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?

/*
* These are the device model conversion veneers; they convert the
* device model structures to our more specific structures.

drivers/amba/bus.c +3 -2

@@ -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 {

drivers/amba/bus.c +3 -0

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

Quoted 21 lines

/*
* These are the device model conversion veneers; they convert the
* device model structures to our more specific structures.

drivers/amba/bus.c +10 -2

@@ -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;
 

Review static int amba_probe(struct device *dev) · line 170

Quoted 2 lines

(quoted diff) +1 -1

-	return pcdrv->probe(pcdev, id);
+		amba_put_disable_pclk(pcdev);

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

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

Quoted 21 lines · Kevin Wells

Quoted 21 lines

/*
* These are the device model conversion veneers; they convert the
* device model structures to our more specific structures.

drivers/amba/bus.c +10 -2

@@ -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;
 

Quoted 4 lines

Review static int amba_probe(struct device *dev) · line 170

Quoted 2 lines

(quoted diff) +1 -1

-	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.

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

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

2010/8/3 Kevin Wells <[email protected]>:

Quoted 2 lines

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]

Quoted 8 lines

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.

Quoted 5 lines

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.

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?

Quoted 10 lines

- ? ? 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;

Quoted 2 lines

- ? ? return pcdrv->probe(pcdev, id);
+ ? ? ? ? ? ? amba_put_disable_pclk(pcdev);

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

Review

Quoted 9 lines · Viresh KUMAR

(quoted diff) +6 -3

-	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.

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.

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.

Quoted 3 lines · Viresh KUMAR

Quoted 3 lines

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.

Quoted 6 lines · Russell King - ARM Linux

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.

Quoted 6 lines

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.