[PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Jon Hunter]

From: Jon Hunter <jon-hunter@ti.com>

This patch is based upon Ming Lei's patch to add runtime PM support for OMAP4
[1]. In Ming's original patch the CTI interrupts were being enabled during
runtime when the PMU was used but they were only configured once during init.
Therefore move the configuration of the CTI interrupts to the runtime PM
functions.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074153.html

Cc: Ming Lei <ming.lei@canonical.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Kevin Hilman <khilman@ti.com>

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/mach-omap2/devices.c |   50 ++++++++++++++++++++++------------------
 1 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
index 636533d..b02aa81 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/platform_data/omap4-keypad.h>
+#include <linux/pm_runtime.h>
 
 #include <mach/hardware.h>
 #include <mach/irqs.h>
@@ -434,13 +435,22 @@ static struct omap_device_pm_latency omap_pmu_latency[] = {
 };
 
 static struct cti omap4_cti[2];
+static struct platform_device *pmu_dev;
 
 static void omap4_enable_cti(int irq)
 {
-	if (irq == OMAP44XX_IRQ_CTI0)
+	pm_runtime_get_sync(&pmu_dev->dev);
+	if (irq == OMAP44XX_IRQ_CTI0) {
+		/* configure CTI0 for pmu irq routing */
+		cti_unlock(&omap4_cti[0]);
+		cti_map_trigger(&omap4_cti[0], 1, 6, 2);
 		cti_enable(&omap4_cti[0]);
-	else if (irq == OMAP44XX_IRQ_CTI1)
+	} else if (irq == OMAP44XX_IRQ_CTI1) {
+		/* configure CTI1 for pmu irq routing */
+		cti_unlock(&omap4_cti[1]);
+		cti_map_trigger(&omap4_cti[1], 1, 6, 2);
 		cti_enable(&omap4_cti[1]);
+	}
 }
 
 static void omap4_disable_cti(int irq)
@@ -449,6 +459,7 @@ static void omap4_disable_cti(int irq)
 		cti_disable(&omap4_cti[0]);
 	else if (irq == OMAP44XX_IRQ_CTI1)
 		cti_disable(&omap4_cti[1]);
+	pm_runtime_put(&pmu_dev->dev);
 }
 
 static irqreturn_t omap4_pmu_handler(int irq, void *dev, irq_handler_t handler)
@@ -461,27 +472,20 @@ static irqreturn_t omap4_pmu_handler(int irq, void *dev, irq_handler_t handler)
 	return handler(irq, dev);
 }
 
-static void __init omap4_configure_pmu_irq(void)
+static int __init omap4_configure_pmu(void)
 {
-	void __iomem *base0;
-	void __iomem *base1;
+	omap4_cti[0].base = ioremap(OMAP44XX_CTI0_BASE, SZ_4K);
+	omap4_cti[1].base = ioremap(OMAP44XX_CTI1_BASE, SZ_4K);
 
-	base0 = ioremap(OMAP44XX_CTI0_BASE, SZ_4K);
-	base1 = ioremap(OMAP44XX_CTI1_BASE, SZ_4K);
-	if (!base0 && !base1) {
+	if (!omap4_cti[0].base || !omap4_cti[1].base) {
 		pr_err("ioremap for OMAP4 CTI failed\n");
-		return;
+		return -ENOMEM;
 	}
 
-	/*configure CTI0 for pmu irq routing*/
-	cti_init(&omap4_cti[0], base0, OMAP44XX_IRQ_CTI0, 6);
-	cti_unlock(&omap4_cti[0]);
-	cti_map_trigger(&omap4_cti[0], 1, 6, 2);
+	cti_init(&omap4_cti[0], omap4_cti[0].base, OMAP44XX_IRQ_CTI0, 6);
+	cti_init(&omap4_cti[1], omap4_cti[1].base, OMAP44XX_IRQ_CTI1, 6);
 
-	/*configure CTI1 for pmu irq routing*/
-	cti_init(&omap4_cti[1], base1, OMAP44XX_IRQ_CTI1, 6);
-	cti_unlock(&omap4_cti[1]);
-	cti_map_trigger(&omap4_cti[1], 1, 6, 2);
+	return 0;
 }
 
 static struct platform_device* __init omap4_init_pmu(void)
@@ -492,6 +496,9 @@ static struct platform_device* __init omap4_init_pmu(void)
 	struct omap_hwmod* oh[3];
 	char *dev_name = "arm-pmu";
 
+	if (omap4_configure_pmu())
+		return NULL;
+
 	hw = "l3_main_3";
 	oh[0] = omap_hwmod_lookup(hw);
 	if (!oh[0]) {
@@ -530,14 +537,11 @@ static void __init omap_init_pmu(void)
 	} else if (cpu_is_omap34xx()) {
 		omap_pmu_device.resource = &omap3_pmu_resource;
 	} else if (cpu_is_omap44xx()) {
-		struct platform_device *pd;
-
-		pd = omap4_init_pmu();
-		if (!pd)
+		pmu_dev = omap4_init_pmu();
+		if (!pmu_dev)
 			return;
 
-		omap_device_enable(&od->pdev);
-		omap4_configure_pmu_irq();
+		pm_runtime_enable(&pmu_dev->dev);
 		return;
 	} else {
 		return;
-- 
1.7.5.4

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Shilimkar, Santosh]

On Thu, May 10, 2012 at 3:05 AM, Jon Hunter <jon-hunter@ti.com> wrote:
> From: Jon Hunter <jon-hunter@ti.com>
>
> This patch is based upon Ming Lei's patch to add runtime PM support for OMAP4
> [1]. In Ming's original patch the CTI interrupts were being enabled during
> runtime when the PMU was used but they were only configured once during init.
> Therefore move the configuration of the CTI interrupts to the runtime PM
> functions.
>
You might want to give the reason why you need to move the re-confiuration
from init to to run-time PM functions so that the actual issue of
reset gets addressed.


> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074153.html
>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Kevin Hilman <khilman@ti.com>
>
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> ---
Patch as such looks fine to me.
Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Ming Lei]

On Thu, May 10, 2012 at 5:35 AM, Jon Hunter <jon-hunter@ti.com> wrote:
> From: Jon Hunter <jon-hunter@ti.com>
>
> This patch is based upon Ming Lei's patch to add runtime PM support for OMAP4
> [1]. In Ming's original patch the CTI interrupts were being enabled during
> runtime when the PMU was used but they were only configured once during init.
> Therefore move the configuration of the CTI interrupts to the runtime PM
> functions.

As Shilimkar pointed out, you need to give the reason why the change
is introduced
in the patch.

>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074153.html
>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Kevin Hilman <khilman@ti.com>
>
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> ---
>  arch/arm/mach-omap2/devices.c |   50 ++++++++++++++++++++++------------------
>  1 files changed, 27 insertions(+), 23 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
> index 636533d..b02aa81 100644
> --- a/arch/arm/mach-omap2/devices.c
> +++ b/arch/arm/mach-omap2/devices.c
> @@ -18,6 +18,7 @@
>  #include <linux/slab.h>
>  #include <linux/of.h>
>  #include <linux/platform_data/omap4-keypad.h>
> +#include <linux/pm_runtime.h>
>
>  #include <mach/hardware.h>
>  #include <mach/irqs.h>
> @@ -434,13 +435,22 @@ static struct omap_device_pm_latency omap_pmu_latency[] = {
>  };
>
>  static struct cti omap4_cti[2];
> +static struct platform_device *pmu_dev;
>
>  static void omap4_enable_cti(int irq)
>  {
> -       if (irq == OMAP44XX_IRQ_CTI0)
> +       pm_runtime_get_sync(&pmu_dev->dev);
> +       if (irq == OMAP44XX_IRQ_CTI0) {
> +               /* configure CTI0 for pmu irq routing */
> +               cti_unlock(&omap4_cti[0]);
> +               cti_map_trigger(&omap4_cti[0], 1, 6, 2);
>                cti_enable(&omap4_cti[0]);
> -       else if (irq == OMAP44XX_IRQ_CTI1)
> +       } else if (irq == OMAP44XX_IRQ_CTI1) {
> +               /* configure CTI1 for pmu irq routing */
> +               cti_unlock(&omap4_cti[1]);
> +               cti_map_trigger(&omap4_cti[1], 1, 6, 2);

The above line should be changed to below

                     cti_map_trigger(&omap4_cti[1], 1, 6, 3);

See below link for addressed irq flood issue.

http://permalink.gmane.org/gmane.linux.linaro.devel/10532

>                cti_enable(&omap4_cti[1]);
> +       }
>  }
>
>  static void omap4_disable_cti(int irq)
> @@ -449,6 +459,7 @@ static void omap4_disable_cti(int irq)
>                cti_disable(&omap4_cti[0]);
>        else if (irq == OMAP44XX_IRQ_CTI1)
>                cti_disable(&omap4_cti[1]);
> +       pm_runtime_put(&pmu_dev->dev);
>  }
>
>  static irqreturn_t omap4_pmu_handler(int irq, void *dev, irq_handler_t handler)
> @@ -461,27 +472,20 @@ static irqreturn_t omap4_pmu_handler(int irq, void *dev, irq_handler_t handler)
>        return handler(irq, dev);
>  }
>
> -static void __init omap4_configure_pmu_irq(void)
> +static int __init omap4_configure_pmu(void)
>  {
> -       void __iomem *base0;
> -       void __iomem *base1;
> +       omap4_cti[0].base = ioremap(OMAP44XX_CTI0_BASE, SZ_4K);
> +       omap4_cti[1].base = ioremap(OMAP44XX_CTI1_BASE, SZ_4K);
>
> -       base0 = ioremap(OMAP44XX_CTI0_BASE, SZ_4K);
> -       base1 = ioremap(OMAP44XX_CTI1_BASE, SZ_4K);
> -       if (!base0 && !base1) {
> +       if (!omap4_cti[0].base || !omap4_cti[1].base) {
>                pr_err("ioremap for OMAP4 CTI failed\n");
> -               return;
> +               return -ENOMEM;
>        }
>
> -       /*configure CTI0 for pmu irq routing*/
> -       cti_init(&omap4_cti[0], base0, OMAP44XX_IRQ_CTI0, 6);
> -       cti_unlock(&omap4_cti[0]);
> -       cti_map_trigger(&omap4_cti[0], 1, 6, 2);
> +       cti_init(&omap4_cti[0], omap4_cti[0].base, OMAP44XX_IRQ_CTI0, 6);
> +       cti_init(&omap4_cti[1], omap4_cti[1].base, OMAP44XX_IRQ_CTI1, 6);
>
> -       /*configure CTI1 for pmu irq routing*/
> -       cti_init(&omap4_cti[1], base1, OMAP44XX_IRQ_CTI1, 6);
> -       cti_unlock(&omap4_cti[1]);
> -       cti_map_trigger(&omap4_cti[1], 1, 6, 2);
> +       return 0;
>  }
>
>  static struct platform_device* __init omap4_init_pmu(void)
> @@ -492,6 +496,9 @@ static struct platform_device* __init omap4_init_pmu(void)
>        struct omap_hwmod* oh[3];
>        char *dev_name = "arm-pmu";
>
> +       if (omap4_configure_pmu())
> +               return NULL;
> +
>        hw = "l3_main_3";
>        oh[0] = omap_hwmod_lookup(hw);
>        if (!oh[0]) {
> @@ -530,14 +537,11 @@ static void __init omap_init_pmu(void)
>        } else if (cpu_is_omap34xx()) {
>                omap_pmu_device.resource = &omap3_pmu_resource;
>        } else if (cpu_is_omap44xx()) {
> -               struct platform_device *pd;
> -
> -               pd = omap4_init_pmu();
> -               if (!pd)
> +               pmu_dev = omap4_init_pmu();
> +               if (!pmu_dev)
>                        return;
>
> -               omap_device_enable(&od->pdev);
> -               omap4_configure_pmu_irq();
> +               pm_runtime_enable(&pmu_dev->dev);
>                return;
>        } else {
>                return;
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Jon Hunter]

Hi Ming,

On 05/14/2012 11:53 PM, Ming Lei wrote:
> On Thu, May 10, 2012 at 5:35 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>> From: Jon Hunter <jon-hunter@ti.com>
>>
>> This patch is based upon Ming Lei's patch to add runtime PM support for OMAP4
>> [1]. In Ming's original patch the CTI interrupts were being enabled during
>> runtime when the PMU was used but they were only configured once during init.
>> Therefore move the configuration of the CTI interrupts to the runtime PM
>> functions.
> 
> As Shilimkar pointed out, you need to give the reason why the change
> is introduced
> in the patch.

Yes will do. I have been waiting to get some feedback on HW_AUTO versus
SW_SLEEP from design before posting a V2. I will enhance the changelog.

Cheers
Jon

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Ming Lei]

Jon,

On Wed, May 16, 2012 at 6:05 AM, Ming Lei <ming.lei@canonical.com> wrote:
>
>
> On Tuesday, May 15, 2012, Jon Hunter <jon-hunter@ti.com> wrote:
>> Hi Ming,
>>
>> On 05/14/2012 11:53 PM, Ming Lei wrote:
>>> On Thu, May 10, 2012 at 5:35 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>>>> From: Jon Hunter <jon-hunter@ti.com>
>>>>
>>>> This patch is based upon Ming Lei's patch to add runtime PM support for
>>>> OMAP4
>>>> [1]. In Ming's original patch the CTI interrupts were being enabled
>>>> during
>>>> runtime when the PMU was used but they were only configured once during
>>>> init.
>>>> Therefore move the configuration of the CTI interrupts to the runtime PM
>>>> functions.
>>>
>>> As Shilimkar pointed out, you need to give the reason why the change
>>> is introduced
>>> in the patch.
>>
>> Yes will do. I have been waiting to get some feedback on HW_AUTO versus
>> SW_SLEEP from design before posting a V2. I will enhance the changelog.
>
> Looks pandaboard will hang during kernel boot
> with the latest next tree, so perf can't be tested now.
> Once the problem is fixed, l will run perf test and provide my feedback.

After bisecting, looks it is the 1st commit which triggers kernel
boot hang:

commit 5f3aa31f77733605f04a5a92ddc475ffd439585f
Merge: 0f131ea ce4deaa
Author: Stephen Rothwell <sfr@canb.auug.org.au>
Date:   Mon May 14 18:55:39 2012 +1000

    Merge remote-tracking branch 'arm-soc/for-next'


These 6 patches can't be applied against previous commit, so
I can't verify these patches now.


Thanks,
--
Ming Lei

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Ming Lei]

On Wed, May 16, 2012 at 4:17 PM, Ming Lei <ming.lei@canonical.com> wrote:
> Jon,
>
> On Wed, May 16, 2012 at 6:05 AM, Ming Lei <ming.lei@canonical.com> wrote:
>>
>>
>> On Tuesday, May 15, 2012, Jon Hunter <jon-hunter@ti.com> wrote:
>>> Hi Ming,
>>>
>>> On 05/14/2012 11:53 PM, Ming Lei wrote:
>>>> On Thu, May 10, 2012 at 5:35 AM, Jon Hunter <jon-hunter@ti.com> wrote:
>>>>> From: Jon Hunter <jon-hunter@ti.com>
>>>>>
>>>>> This patch is based upon Ming Lei's patch to add runtime PM support for
>>>>> OMAP4
>>>>> [1]. In Ming's original patch the CTI interrupts were being enabled
>>>>> during
>>>>> runtime when the PMU was used but they were only configured once during
>>>>> init.
>>>>> Therefore move the configuration of the CTI interrupts to the runtime PM
>>>>> functions.
>>>>
>>>> As Shilimkar pointed out, you need to give the reason why the change
>>>> is introduced
>>>> in the patch.
>>>
>>> Yes will do. I have been waiting to get some feedback on HW_AUTO versus
>>> SW_SLEEP from design before posting a V2. I will enhance the changelog.
>>
>> Looks pandaboard will hang during kernel boot
>> with the latest next tree, so perf can't be tested now.
>> Once the problem is fixed, l will run perf test and provide my feedback.
>
> After bisecting, looks it is the 1st commit which triggers kernel
> boot hang:
>
> commit 5f3aa31f77733605f04a5a92ddc475ffd439585f
> Merge: 0f131ea ce4deaa
> Author: Stephen Rothwell <sfr@canb.auug.org.au>
> Date:   Mon May 14 18:55:39 2012 +1000
>
>    Merge remote-tracking branch 'arm-soc/for-next'

The above kernel hang is caused by mmc driver probe failure,
which can be fixed by the patch in below link:

      http://www.mail-archive.com/linux-omap@vger.kernel.org/msg68848.html

After applying the patch, I can test the 6 patches and looks they work well
about perf support on omap4, also perf can work well after S2R.

Tested-by: Ming Lei <ming.lei@canonical.com>


Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Kevin Hilman]

Jon Hunter <jon-hunter@ti.com> writes:

> From: Jon Hunter <jon-hunter@ti.com>
>
> This patch is based upon Ming Lei's patch to add runtime PM support for OMAP4
> [1]. In Ming's original patch the CTI interrupts were being enabled during
> runtime when the PMU was used but they were only configured once during init.
> Therefore move the configuration of the CTI interrupts to the runtime PM
> functions.

Lovely.  This is exactly the workaround I was hoping to see.   Thanks!

Some comments below...

> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074153.html
>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Kevin Hilman <khilman@ti.com>
>
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> ---
>  arch/arm/mach-omap2/devices.c |   50 ++++++++++++++++++++++------------------
>  1 files changed, 27 insertions(+), 23 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
> index 636533d..b02aa81 100644
> --- a/arch/arm/mach-omap2/devices.c
> +++ b/arch/arm/mach-omap2/devices.c
> @@ -18,6 +18,7 @@
>  #include <linux/slab.h>
>  #include <linux/of.h>
>  #include <linux/platform_data/omap4-keypad.h>
> +#include <linux/pm_runtime.h>
>  
>  #include <mach/hardware.h>
>  #include <mach/irqs.h>
> @@ -434,13 +435,22 @@ static struct omap_device_pm_latency omap_pmu_latency[] = {
>  };
>  
>  static struct cti omap4_cti[2];
> +static struct platform_device *pmu_dev;
>  
>  static void omap4_enable_cti(int irq)
>  {
> -	if (irq == OMAP44XX_IRQ_CTI0)
> +	pm_runtime_get_sync(&pmu_dev->dev);
> +	if (irq == OMAP44XX_IRQ_CTI0) {
> +		/* configure CTI0 for pmu irq routing */
> +		cti_unlock(&omap4_cti[0]);
> +		cti_map_trigger(&omap4_cti[0], 1, 6, 2);
>  		cti_enable(&omap4_cti[0]);
> -	else if (irq == OMAP44XX_IRQ_CTI1)
> +	} else if (irq == OMAP44XX_IRQ_CTI1) {
> +		/* configure CTI1 for pmu irq routing */
> +		cti_unlock(&omap4_cti[1]);
> +		cti_map_trigger(&omap4_cti[1], 1, 6, 2);
>  		cti_enable(&omap4_cti[1]);
> +	}
>  }

The cti_* functions really belong in the ->runtime_resume() callback.

In the case of multiple users, I don't think the current implementation
will do what is intended.  IOW, you only want to (re)init for the first
user (when runtime PM usecount goes from zero to one.)   That is when
the ->runtime_resume() is called.   For a 2nd user, the
->runtime_resume() callback will not be called, but the cti_* functions
will still be called.   I don't think that is what you want.

>  static void omap4_disable_cti(int irq)
> @@ -449,6 +459,7 @@ static void omap4_disable_cti(int irq)
>  		cti_disable(&omap4_cti[0]);
>  	else if (irq == OMAP44XX_IRQ_CTI1)
>  		cti_disable(&omap4_cti[1]);
> +	pm_runtime_put(&pmu_dev->dev);

Similarily here.  the cti_ functions should be in the
->runtime_suspend() callback.

>  }
>  
>  static irqreturn_t omap4_pmu_handler(int irq, void *dev, irq_handler_t handler)
> @@ -461,27 +472,20 @@ static irqreturn_t omap4_pmu_handler(int irq, void *dev, irq_handler_t handler)
>  	return handler(irq, dev);
>  }
>  
> -static void __init omap4_configure_pmu_irq(void)
> +static int __init omap4_configure_pmu(void)
>  {
> -	void __iomem *base0;
> -	void __iomem *base1;
> +	omap4_cti[0].base = ioremap(OMAP44XX_CTI0_BASE, SZ_4K);
> +	omap4_cti[1].base = ioremap(OMAP44XX_CTI1_BASE, SZ_4K);
>  
> -	base0 = ioremap(OMAP44XX_CTI0_BASE, SZ_4K);
> -	base1 = ioremap(OMAP44XX_CTI1_BASE, SZ_4K);
> -	if (!base0 && !base1) {
> +	if (!omap4_cti[0].base || !omap4_cti[1].base) {
>  		pr_err("ioremap for OMAP4 CTI failed\n");
> -		return;
> +		return -ENOMEM;
>  	}
>  
> -	/*configure CTI0 for pmu irq routing*/
> -	cti_init(&omap4_cti[0], base0, OMAP44XX_IRQ_CTI0, 6);
> -	cti_unlock(&omap4_cti[0]);
> -	cti_map_trigger(&omap4_cti[0], 1, 6, 2);
> +	cti_init(&omap4_cti[0], omap4_cti[0].base, OMAP44XX_IRQ_CTI0, 6);
> +	cti_init(&omap4_cti[1], omap4_cti[1].base, OMAP44XX_IRQ_CTI1, 6);
>  
> -	/*configure CTI1 for pmu irq routing*/
> -	cti_init(&omap4_cti[1], base1, OMAP44XX_IRQ_CTI1, 6);
> -	cti_unlock(&omap4_cti[1]);
> -	cti_map_trigger(&omap4_cti[1], 1, 6, 2);
> +	return 0;
>  }
>  
>  static struct platform_device* __init omap4_init_pmu(void)
> @@ -492,6 +496,9 @@ static struct platform_device* __init omap4_init_pmu(void)
>  	struct omap_hwmod* oh[3];
>  	char *dev_name = "arm-pmu";
>  
> +	if (omap4_configure_pmu())
> +		return NULL;
> +
>  	hw = "l3_main_3";
>  	oh[0] = omap_hwmod_lookup(hw);
>  	if (!oh[0]) {
> @@ -530,14 +537,11 @@ static void __init omap_init_pmu(void)
>  	} else if (cpu_is_omap34xx()) {
>  		omap_pmu_device.resource = &omap3_pmu_resource;
>  	} else if (cpu_is_omap44xx()) {
> -		struct platform_device *pd;
> -
> -		pd = omap4_init_pmu();
> -		if (!pd)
> +		pmu_dev = omap4_init_pmu();

Shouldn't the device be accessible for this call?

IOW, with runtime PM, there should be a get/put around this.

Kevin

> +		if (!pmu_dev)
>  			return;
>  
> -		omap_device_enable(&od->pdev);
> -		omap4_configure_pmu_irq();
> +		pm_runtime_enable(&pmu_dev->dev);
>  		return;
>  	} else {
>  		return;

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Jon Hunter]

Hi Kevin,

On 05/29/2012 04:17 PM, Kevin Hilman wrote:
> Jon Hunter <jon-hunter@ti.com> writes:
> 
>> From: Jon Hunter <jon-hunter@ti.com>
>>
>> This patch is based upon Ming Lei's patch to add runtime PM support for OMAP4
>> [1]. In Ming's original patch the CTI interrupts were being enabled during
>> runtime when the PMU was used but they were only configured once during init.
>> Therefore move the configuration of the CTI interrupts to the runtime PM
>> functions.
> 
> Lovely.  This is exactly the workaround I was hoping to see.   Thanks!
> 
> Some comments below...

Thanks! Great timing, I am getting ready to post V2 :-)

>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074153.html
>>
>> Cc: Ming Lei <ming.lei@canonical.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Benoit Cousson <b-cousson@ti.com>
>> Cc: Paul Walmsley <paul@pwsan.com>
>> Cc: Kevin Hilman <khilman@ti.com>
>>
>> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
>> ---
>>  arch/arm/mach-omap2/devices.c |   50 ++++++++++++++++++++++------------------
>>  1 files changed, 27 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
>> index 636533d..b02aa81 100644
>> --- a/arch/arm/mach-omap2/devices.c
>> +++ b/arch/arm/mach-omap2/devices.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/of.h>
>>  #incluhttp://marc.info/?l=linux-arm-kernel&m=132227620816504&w=2de <linux/platform_data/omap4-keypad.h>
>> +#include <linux/pm_runtime.h>
>>  
>>  #include <mach/hardware.h>
>>  #include <mach/irqs.h>
>> @@ -434,13 +435,22 @@ static struct omap_device_pm_latency omap_pmu_latency[] = {
>>  };
>>  
>>  static struct cti omap4_cti[2];
>> +static struct platform_device *pmu_dev;
>>  
>>  static void omap4_enable_cti(int irq)
>>  {
>> -	if (irq == OMAP44XX_IRQ_CTI0)
>> +	pm_runtime_get_sync(&pmu_dev->dev);
>> +	if (irq == OMAP44XX_IRQ_CTI0) {
>> +		/* configure CTI0 for pmu irq routing */
>> +		cti_unlock(&omap4_cti[0]);
>> +		cti_map_trigger(&omap4_cti[0], 1, 6, 2);
>>  		cti_enablook at thisle(&omap4_cti[0]);
>> -	else if (irq == OMAP44XX_IRQ_CTI1)
>> +	} else if (irq == OMAP44XX_IRQ_CTI1) {
>> +		/* configure CTI1 for pmu irq routing */
>> +		cti_unlolook at thisck(&omap4_cti[1]);
>> +		cti_map_trigger(&omap4_cti[1], 1, 6, 2);
>>  		cti_enable(&omap4_cti[1]);
>> +	}
>>  }
> 
> The cti_* functions really belong in the ->runtime_resume() callback.
> 
> In the case of multiple users, I don't think the current implementation
> will do what is intended.  IOW, you only want to (re)init for the first
> user (when runtime PM usecount goes from zero to one.)   That is when
> the ->runtime_resume() is called.   For a 2nd user, the
> ->runtime_resume() callback will not be called, but the cti_* functions
> will still be called.   I don't think that is what you want.

Ah, yes that would not work. Ok, let me make that change.

>>  static void omap4_disable_cti(int irq)
>> @@ -449,6 +459,7 @@ static void omap4_disable_cti(int irq)
>>  		cti_disable(&omap4_cti[0]);
>>  	else if (irq == OMAP44XX_IRQ_CTI1)
>>  		cti_disable(&omap4_cti[1]);
>> +	pm_runtime_put(&pmu_dev->dev);
> 
> Similarily here.  the cti_ functions should be in the
> ->runtime_suspend() callback.

Ok, will change that too.

>>  }
>>  
>>  static irqreturn_t omap4_pmu_handler(int irq, void *dev, irq_handler_t handler)
>> @@ -461,27 +472,20 @@ static irqreturn_t omap4_pmu_handler(int irq, void *dev, irq_handler_t handler)
>>  	return handler(irq, dev);
>>  }
>>  
>> -static void __init omap4_configure_pmu_irq(void)
>> +static int __init omap4_configure_pmu(void)
>>  {
>> -	void __iomem *base0;
>> -	void __iomem *base1;
>> +	omap4_cti[0].base = ioremap(OMAP44XX_CTI0_BASE, SZ_4K);
>> +	omap4_cti[1].base = ioremap(OMAP44XX_CTI1_BASE, SZ_4K);
>>  
>> -	base0 = ioremap(OMAP44XX_CTI0_BASE, SZ_4K);
>> -	base1 = ioremap(OMAP44XX_CTI1_BASE, SZ_4K);
>> -	if (!base0 && !base1) {
>> +	if (!omap4_cti[0].base || !omap4_cti[1].base) {
>>  		pr_err("ioremap for OMAP4 CTI failed\n");
>> -		return;
>> +		return -ENOMEM;
>>  	}
>>  
>> -	/*configure CTI0 for pmu irq routing*/
>> -	cti_init(&omap4_cti[0], base0, OMAP44XX_IRQ_CTI0, 6);
>> -	cti_unlock(&omap4_cti[0]);
>> -	cti_map_trigger(&omap4_cti[0], 1, 6, 2);
>> +	cti_init(&omap4_cti[0], omap4_cti[0].base, OMAP44XX_IRQ_CTI0, 6);
>> +	cti_init(&omap4_cti[1], omap4_cti[1].base, OMAP44XX_IRQ_CTI1, 6);
>>  
>> -	/*configure CTI1 for pmu irq routing*/
>> -	cti_init(&omap4_cti[1], base1, OMAP44XX_IRQ_CTI1, 6);
>> -	cti_unlock(&omap4_cti[1]);
>> -	cti_map_trigger(&omap4_cti[1], 1, 6, 2);
>> +	return 0;
>>  }
>>  
>>  static struct platform_device* __init omap4_init_pmu(void)
>> @@ -492,6 +496,9 @@ static struct platform_device* __init omap4_init_pmu(void)
>>  	struct omap_hwmod* oh[3];
>>  	char *dev_name = "arm-pmu";
>>  
>> +	if (omap4_configure_pmu())
>> +		return NULL;
>> +
>>  	hw = "l3_main_3";
>>  	oh[0] = omap_hwmod_lookup(hw);
>>  	if (!oh[0]) {
>> @@ -530,14 +537,11 @@ static void __init omap_init_pmu(void)
>>  	} else if (cpu_is_omap34xx()) {
>>  		omap_pmu_device.resource = &omap3_pmu_resource;
>>  	} else if (cpu_is_omap44xx()) {
>> -		struct platform_device *pd;
>> -
>> -		pd = omap4_init_pmu();
>> -		if (!pd)
>> +		pmu_dev = omap4_init_pmu();
> 
> Shouldn't the device be accessible for this call?
> 
> IOW, with runtime PM, there should be a get/put around this.

This function does not actually touch the hardware and so it should be ok.

Cheers
Jon

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Jon Hunter]

Hi Kevin,

On 05/29/2012 05:07 PM, Jon Hunter wrote:
> Hi Kevin,
> 
> On 05/29/2012 04:17 PM, Kevin Hilman wrote:
>> Jon Hunter <jon-hunter@ti.com> writes:
>>
>>> From: Jon Hunter <jon-hunter@ti.com>
>>>
>>> This patch is based upon Ming Lei's patch to add runtime PM support for OMAP4
>>> [1]. In Ming's original patch the CTI interrupts were being enabled during
>>> runtime when the PMU was used but they were only configured once during init.
>>> Therefore move the configuration of the CTI interrupts to the runtime PM
>>> functions.
>>
>> Lovely.  This is exactly the workaround I was hoping to see.   Thanks!
>>
>> Some comments below...
> 
> Thanks! Great timing, I am getting ready to post V2 :-)
> 
>>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074153.html
>>>
>>> Cc: Ming Lei <ming.lei@canonical.com>
>>> Cc: Will Deacon <will.deacon@arm.com>
>>> Cc: Benoit Cousson <b-cousson@ti.com>
>>> Cc: Paul Walmsley <paul@pwsan.com>
>>> Cc: Kevin Hilman <khilman@ti.com>
>>>
>>> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
>>> ---
>>>  arch/arm/mach-omap2/devices.c |   50 ++++++++++++++++++++++------------------
>>>  1 files changed, 27 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
>>> index 636533d..b02aa81 100644
>>> --- a/arch/arm/mach-omap2/devices.c
>>> +++ b/arch/arm/mach-omap2/devices.c
>>> @@ -18,6 +18,7 @@
>>>  #include <linux/slab.h>
>>>  #include <linux/of.h>
>>>  #incluhttp://marc.info/?l=linux-arm-kernel&m=132227620816504&w=2de <linux/platform_data/omap4-keypad.h>
>>> +#include <linux/pm_runtime.h>
>>>  
>>>  #include <mach/hardware.h>
>>>  #include <mach/irqs.h>
>>> @@ -434,13 +435,22 @@ static struct omap_device_pm_latency omap_pmu_latency[] = {
>>>  };
>>>  
>>>  static struct cti omap4_cti[2];
>>> +static struct platform_device *pmu_dev;
>>>  
>>>  static void omap4_enable_cti(int irq)
>>>  {
>>> -	if (irq == OMAP44XX_IRQ_CTI0)
>>> +	pm_runtime_get_sync(&pmu_dev->dev);
>>> +	if (irq == OMAP44XX_IRQ_CTI0) {
>>> +		/* configure CTI0 for pmu irq routing */
>>> +		cti_unlock(&omap4_cti[0]);
>>> +		cti_map_trigger(&omap4_cti[0], 1, 6, 2);
>>>  		cti_enablook at thisle(&omap4_cti[0]);
>>> -	else if (irq == OMAP44XX_IRQ_CTI1)
>>> +	} else if (irq == OMAP44XX_IRQ_CTI1) {
>>> +		/* configure CTI1 for pmu irq routing */
>>> +		cti_unlolook at thisck(&omap4_cti[1]);
>>> +		cti_map_trigger(&omap4_cti[1], 1, 6, 2);
>>>  		cti_enable(&omap4_cti[1]);
>>> +	}
>>>  }
>>
>> The cti_* functions really belong in the ->runtime_resume() callback.
>>
>> In the case of multiple users, I don't think the current implementation
>> will do what is intended.  IOW, you only want to (re)init for the first
>> user (when runtime PM usecount goes from zero to one.)   That is when
>> the ->runtime_resume() is called.   For a 2nd user, the
>> ->runtime_resume() callback will not be called, but the cti_* functions
>> will still be called.   I don't think that is what you want.
> 
> Ah, yes that would not work. Ok, let me make that change.

Actually, looking at this some more, the above omap4_enable_cti()
function is getting called from armpmu_reserve_hardware() function in
the pmu driver. In armpmu_reserve_hardware(), it calls reserve_pmu() to
see if the PMU is in-use and if not acquires it. So I believe that this
code should be atomic already. May be Will or Ming can confirm. However,
if this is the case, then do you agree we should be ok?

I can see that ideally, we should use ->runtime_resume/suspend, but the
arm-pmu does not currently have support for these functions and hence
there is no easy way to specify such platform functions. Obviously it
could be done but would be a bigger change.

Let me know your thoughts.

Cheers
Jon

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Kevin Hilman]

Jon Hunter <jon-hunter@ti.com> writes:

> Hi Kevin,
>
> On 05/29/2012 05:07 PM, Jon Hunter wrote:
>> Hi Kevin,
>> 
>> On 05/29/2012 04:17 PM, Kevin Hilman wrote:
>>> Jon Hunter <jon-hunter@ti.com> writes:
>>>
>>>> From: Jon Hunter <jon-hunter@ti.com>
>>>>
>>>> This patch is based upon Ming Lei's patch to add runtime PM support for OMAP4
>>>> [1]. In Ming's original patch the CTI interrupts were being enabled during
>>>> runtime when the PMU was used but they were only configured once during init.
>>>> Therefore move the configuration of the CTI interrupts to the runtime PM
>>>> functions.
>>>
>>> Lovely.  This is exactly the workaround I was hoping to see.   Thanks!
>>>
>>> Some comments below...
>> 
>> Thanks! Great timing, I am getting ready to post V2 :-)
>> 
>>>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/074153.html
>>>>
>>>> Cc: Ming Lei <ming.lei@canonical.com>
>>>> Cc: Will Deacon <will.deacon@arm.com>
>>>> Cc: Benoit Cousson <b-cousson@ti.com>
>>>> Cc: Paul Walmsley <paul@pwsan.com>
>>>> Cc: Kevin Hilman <khilman@ti.com>
>>>>
>>>> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
>>>> ---
>>>>  arch/arm/mach-omap2/devices.c |   50 ++++++++++++++++++++++------------------
>>>>  1 files changed, 27 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
>>>> index 636533d..b02aa81 100644
>>>> --- a/arch/arm/mach-omap2/devices.c
>>>> +++ b/arch/arm/mach-omap2/devices.c
>>>> @@ -18,6 +18,7 @@
>>>>  #include <linux/slab.h>
>>>>  #include <linux/of.h>
>>>>  #incluhttp://marc.info/?l=linux-arm-kernel&m=132227620816504&w=2de <linux/platform_data/omap4-keypad.h>
>>>> +#include <linux/pm_runtime.h>
>>>>  
>>>>  #include <mach/hardware.h>
>>>>  #include <mach/irqs.h>
>>>> @@ -434,13 +435,22 @@ static struct omap_device_pm_latency omap_pmu_latency[] = {
>>>>  };
>>>>  
>>>>  static struct cti omap4_cti[2];
>>>> +static struct platform_device *pmu_dev;
>>>>  
>>>>  static void omap4_enable_cti(int irq)
>>>>  {
>>>> -	if (irq == OMAP44XX_IRQ_CTI0)
>>>> +	pm_runtime_get_sync(&pmu_dev->dev);
>>>> +	if (irq == OMAP44XX_IRQ_CTI0) {
>>>> +		/* configure CTI0 for pmu irq routing */
>>>> +		cti_unlock(&omap4_cti[0]);
>>>> +		cti_map_trigger(&omap4_cti[0], 1, 6, 2);
>>>>  		cti_enablook at thisle(&omap4_cti[0]);
>>>> -	else if (irq == OMAP44XX_IRQ_CTI1)
>>>> +	} else if (irq == OMAP44XX_IRQ_CTI1) {
>>>> +		/* configure CTI1 for pmu irq routing */
>>>> +		cti_unlolook at thisck(&omap4_cti[1]);
>>>> +		cti_map_trigger(&omap4_cti[1], 1, 6, 2);
>>>>  		cti_enable(&omap4_cti[1]);
>>>> +	}
>>>>  }
>>>
>>> The cti_* functions really belong in the ->runtime_resume() callback.
>>>
>>> In the case of multiple users, I don't think the current implementation
>>> will do what is intended.  IOW, you only want to (re)init for the first
>>> user (when runtime PM usecount goes from zero to one.)   That is when
>>> the ->runtime_resume() is called.   For a 2nd user, the
>>> ->runtime_resume() callback will not be called, but the cti_* functions
>>> will still be called.   I don't think that is what you want.
>> 
>> Ah, yes that would not work. Ok, let me make that change.
>
> Actually, looking at this some more, the above omap4_enable_cti()
> function is getting called from armpmu_reserve_hardware() function in
> the pmu driver. In armpmu_reserve_hardware(), it calls reserve_pmu() to
> see if the PMU is in-use and if not acquires it. So I believe that this
> code should be atomic already. May be Will or Ming can confirm. However,
> if this is the case, then do you agree we should be ok?
>
> I can see that ideally, we should use ->runtime_resume/suspend, but the
> arm-pmu does not currently have support for these functions and hence
> there is no easy way to specify such platform functions. Obviously it
> could be done but would be a bigger change.
>
> Let me know your thoughts.

I'm guessing you probably know my thoughts since you've already thought
through how this should probably look.

Basically, I don't like the result when we have to hack around missing
runtime PM support for a driver, so IMO, the driver should be updated.

IOW, it looks to me like the armpmu driver should grow runtime PM
support.  The current armpmu_release|reserve should probably be replaced
with runtime PM get/put, and the functionality in those functions would
be the runtime PM callbacks instead.

Will, any objections to armpmu growing runtime PM support?

Kevin

P.S.  Jon, for readability sake, any objections to  moving the PMU device init
out of devices.c into pmu.c?  devices.c is awful crowded.

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Will Deacon]

Hi Kevin,

On Wed, May 30, 2012 at 10:50:01PM +0100, Kevin Hilman wrote:
> Basically, I don't like the result when we have to hack around missing
> runtime PM support for a driver, so IMO, the driver should be updated.
> 
> IOW, it looks to me like the armpmu driver should grow runtime PM
> support.  The current armpmu_release|reserve should probably be replaced
> with runtime PM get/put, and the functionality in those functions would
> be the runtime PM callbacks instead.
> 
> Will, any objections to armpmu growing runtime PM support?

My plan for the armpmu reservation is to kill the global reservation scheme
that we currently have and push those function pointers into the arm_pmu,
so that fits with what you'd like.

The only concern I have is that we need the mutual exclusion even when we
don't have support for runtime PM. If we can solve that then I'm fine with
the approach.

Cheers,

Will

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Jon Hunter]

Hi Kevin,

On 05/30/2012 04:50 PM, Kevin Hilman wrote:

[...]

> I'm guessing you probably know my thoughts since you've already thought
> through how this should probably look.
> 
> Basically, I don't like the result when we have to hack around missing
> runtime PM support for a driver, so IMO, the driver should be updated.
> 
> IOW, it looks to me like the armpmu driver should grow runtime PM
> support.  The current armpmu_release|reserve should probably be replaced
> with runtime PM get/put, and the functionality in those functions would
> be the runtime PM callbacks instead.
> 
> Will, any objections to armpmu growing runtime PM support?
> 
> Kevin
> 
> P.S.  Jon, for readability sake, any objections to  moving the PMU device init
> out of devices.c into pmu.c?  devices.c is awful crowded.

No objections. I am guessing that pmu was not supported back in the ARM9
days and so this is only really specific to omap2 devices. That being
said, should this still go into plat-omap dir or just mach-omap2?

Cheers
Jon

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Jon Hunter]

Hi Will,

On 05/30/2012 08:29 PM, Will Deacon wrote:
> Hi Kevin,
> 
> On Wed, May 30, 2012 at 10:50:01PM +0100, Kevin Hilman wrote:
>> Basically, I don't like the result when we have to hack around missing
>> runtime PM support for a driver, so IMO, the driver should be updated.
>>
>> IOW, it looks to me like the armpmu driver should grow runtime PM
>> support.  The current armpmu_release|reserve should probably be replaced
>> with runtime PM get/put, and the functionality in those functions would
>> be the runtime PM callbacks instead.
>>
>> Will, any objections to armpmu growing runtime PM support?
> 
> My plan for the armpmu reservation is to kill the global reservation scheme
> that we currently have and push those function pointers into the arm_pmu,
> so that fits with what you'd like.
> 
> The only concern I have is that we need the mutual exclusion even when we
> don't have support for runtime PM. If we can solve that then I'm fine with
> the approach.

I am not sure I follow your last point. Can you elaborate a little more?

Thanks
Jon

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Kevin Hilman]

Jon Hunter <jon-hunter@ti.com> writes:

> Hi Kevin,
>
> On 05/30/2012 04:50 PM, Kevin Hilman wrote:
>
> [...]
>
>> I'm guessing you probably know my thoughts since you've already thought
>> through how this should probably look.
>> 
>> Basically, I don't like the result when we have to hack around missing
>> runtime PM support for a driver, so IMO, the driver should be updated.
>> 
>> IOW, it looks to me like the armpmu driver should grow runtime PM
>> support.  The current armpmu_release|reserve should probably be replaced
>> with runtime PM get/put, and the functionality in those functions would
>> be the runtime PM callbacks instead.
>> 
>> Will, any objections to armpmu growing runtime PM support?
>> 
>> Kevin
>> 
>> P.S.  Jon, for readability sake, any objections to  moving the PMU device init
>> out of devices.c into pmu.c?  devices.c is awful crowded.
>
> No objections. I am guessing that pmu was not supported back in the ARM9
> days and so this is only really specific to omap2 devices. That being
> said, should this still go into plat-omap dir or just mach-omap2?

I was referring to mach-omap2/devices.c.  Probably just pulling out the
PMU stuff into mach-omap2/pmu.c will be enough.

Kevin

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Jon Hunter]

Hi Kevin, Will,

On 05/30/2012 08:29 PM, Will Deacon wrote:
> Hi Kevin,
> 
> On Wed, May 30, 2012 at 10:50:01PM +0100, Kevin Hilman wrote:
>> Basically, I don't like the result when we have to hack around missing
>> runtime PM support for a driver, so IMO, the driver should be updated.
>>
>> IOW, it looks to me like the armpmu driver should grow runtime PM
>> support.  The current armpmu_release|reserve should probably be replaced
>> with runtime PM get/put, and the functionality in those functions would
>> be the runtime PM callbacks instead.
>>
>> Will, any objections to armpmu growing runtime PM support?
> 
> My plan for the armpmu reservation is to kill the global reservation scheme
> that we currently have and push those function pointers into the arm_pmu,
> so that fits with what you'd like.
> 
> The only concern I have is that we need the mutual exclusion even when we
> don't have support for runtime PM. If we can solve that then I'm fine with
> the approach.

To add a bit more food for thought, I had implemented a quick patch to add runtime PM support for PMU. You will notice that I have been conservative on where I have placed the pm_runtime_get/put calls, because I am not too familiar with the PMU driver to know exactly where we need to maintain the PMU context. So right now these are just around the reserve_hardware/release_hardware calls. This works on OMAP for some quick testing. However, I would need to make sure this does not break compilation without runtime PM enabled.

Let me know your thoughts.

Cheers
Jon

>From b111bcb24737e070ee1ce7ea3d1deb60a4d6f266 Mon Sep 17 00:00:00 2001
From: Jon Hunter <jon-hunter@ti.com>
Date: Thu, 31 May 2012 13:05:20 -0500
Subject: [PATCH] ARM: PMU: Add runtime PM Support

---
 arch/arm/include/asm/pmu.h   |    2 ++
 arch/arm/kernel/perf_event.c |   30 ++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
index 90114fa..db9f20c 100644
--- a/arch/arm/include/asm/pmu.h
+++ b/arch/arm/include/asm/pmu.h
@@ -43,6 +43,8 @@ struct arm_pmu_platdata {
 				  irq_handler_t pmu_handler);
 	void (*enable_irq)(int irq);
 	void (*disable_irq)(int irq);
+	int (*runtime_resume)(struct device *dev);
+	int (*runtime_suspend)(struct device *dev);
 };
 
 #ifdef CONFIG_CPU_HAS_PMU
diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 186c8cb..3b2b016 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -20,6 +20,7 @@
 #include <linux/platform_device.h>
 #include <linux/spinlock.h>
 #include <linux/uaccess.h>
+#include <linux/pm_runtime.h>
 
 #include <asm/cputype.h>
 #include <asm/irq.h>
@@ -460,6 +461,8 @@ hw_perf_event_destroy(struct perf_event *event)
 		armpmu_release_hardware(armpmu);
 		mutex_unlock(pmu_reserve_mutex);
 	}
+
+	pm_runtime_put_sync(&armpmu->plat_device->dev);
 }
 
 static int
@@ -546,6 +549,8 @@ static int armpmu_event_init(struct perf_event *event)
 	if (armpmu->map_event(event) == -ENOENT)
 		return -ENOENT;
 
+	pm_runtime_get_sync(&armpmu->plat_device->dev);
+
 	event->destroy = hw_perf_event_destroy;
 
 	if (!atomic_inc_not_zero(active_events)) {
@@ -584,6 +589,26 @@ static void armpmu_disable(struct pmu *pmu)
 	armpmu->stop();
 }
 
+static int armpmu_runtime_resume(struct device *dev)
+{
+	struct arm_pmu_platdata *plat = dev_get_platdata(dev);
+
+	if (plat->runtime_resume)
+		return plat->runtime_resume(dev);
+
+	return 0;
+}
+
+static int armpmu_runtime_suspend(struct device *dev)
+{
+	struct arm_pmu_platdata *plat = dev_get_platdata(dev);
+
+	if (plat->runtime_suspend)
+		return plat->runtime_suspend(dev);
+
+	return 0;
+}
+
 static void __init armpmu_init(struct arm_pmu *armpmu)
 {
 	atomic_set(&armpmu->active_events, 0);
@@ -650,9 +675,14 @@ static int __devinit armpmu_device_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct dev_pm_ops armpmu_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(armpmu_runtime_suspend, armpmu_runtime_resume, NULL)
+};
+
 static struct platform_driver armpmu_driver = {
 	.driver		= {
 		.name	= "arm-pmu",
+		.pm	= &armpmu_dev_pm_ops,
 		.of_match_table = armpmu_of_device_ids,
 	},
 	.probe		= armpmu_device_probe,
-- 
1.7.9.5

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Jon Hunter]

On 05/31/2012 10:05 AM, Jon Hunter wrote:
> Hi Will,
> 
> On 05/30/2012 08:29 PM, Will Deacon wrote:
>> Hi Kevin,
>>
>> On Wed, May 30, 2012 at 10:50:01PM +0100, Kevin Hilman wrote:
>>> Basically, I don't like the result when we have to hack around missing
>>> runtime PM support for a driver, so IMO, the driver should be updated.
>>>
>>> IOW, it looks to me like the armpmu driver should grow runtime PM
>>> support.  The current armpmu_release|reserve should probably be replaced
>>> with runtime PM get/put, and the functionality in those functions would
>>> be the runtime PM callbacks instead.
>>>
>>> Will, any objections to armpmu growing runtime PM support?
>>
>> My plan for the armpmu reservation is to kill the global reservation scheme
>> that we currently have and push those function pointers into the arm_pmu,
>> so that fits with what you'd like.
>>
>> The only concern I have is that we need the mutual exclusion even when we
>> don't have support for runtime PM. If we can solve that then I'm fine with
>> the approach.
> 
> I am not sure I follow your last point. Can you elaborate a little more?

By the way, I do see your point now. I have just posted a very simple PM
runtime adaption, which I am not sure if this is exactly what you and
Kevin have in mind. However, with this implementation we would not need
to worry about the mutual exclusion as we don't change the flow.
Alternatively, the reserve_pmu() call could be done outside of the
runtime pm callbacks.

Cheers
Jon

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Kevin Hilman]

Jon Hunter <jon-hunter@ti.com> writes:

> Hi Kevin, Will,
>
> On 05/30/2012 08:29 PM, Will Deacon wrote:
>> Hi Kevin,
>> 
>> On Wed, May 30, 2012 at 10:50:01PM +0100, Kevin Hilman wrote:
>>> Basically, I don't like the result when we have to hack around missing
>>> runtime PM support for a driver, so IMO, the driver should be updated.
>>>
>>> IOW, it looks to me like the armpmu driver should grow runtime PM
>>> support.  The current armpmu_release|reserve should probably be replaced
>>> with runtime PM get/put, and the functionality in those functions would
>>> be the runtime PM callbacks instead.
>>>
>>> Will, any objections to armpmu growing runtime PM support?
>> 
>> My plan for the armpmu reservation is to kill the global reservation scheme
>> that we currently have and push those function pointers into the arm_pmu,
>> so that fits with what you'd like.
>> 
>> The only concern I have is that we need the mutual exclusion even when we
>> don't have support for runtime PM. If we can solve that then I'm fine with
>> the approach.
>
> To add a bit more food for thought, I had implemented a quick patch to
> add runtime PM support for PMU. You will notice that I have been
> conservative on where I have placed the pm_runtime_get/put calls,
> because I am not too familiar with the PMU driver to know exactly
> where we need to maintain the PMU context. So right now these are just
> around the reserve_hardware/release_hardware calls. This works on OMAP
> for some quick testing. However, I would need to make sure this does
> not break compilation without runtime PM enabled.
>
> Let me know your thoughts.

That looks good, but I'm curious what would be done in the new
plat->runtime_* hooks.  Maybe the irq enable/disable stuff in the pmu
driver needs to be moved into the runtime PM hooks?

Kevin

> Cheers
> Jon
>
> From b111bcb24737e070ee1ce7ea3d1deb60a4d6f266 Mon Sep 17 00:00:00 2001
> From: Jon Hunter <jon-hunter@ti.com>
> Date: Thu, 31 May 2012 13:05:20 -0500
> Subject: [PATCH] ARM: PMU: Add runtime PM Support
>
> ---
>  arch/arm/include/asm/pmu.h   |    2 ++
>  arch/arm/kernel/perf_event.c |   30 ++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
> index 90114fa..db9f20c 100644
> --- a/arch/arm/include/asm/pmu.h
> +++ b/arch/arm/include/asm/pmu.h
> @@ -43,6 +43,8 @@ struct arm_pmu_platdata {
>  				  irq_handler_t pmu_handler);
>  	void (*enable_irq)(int irq);
>  	void (*disable_irq)(int irq);
> +	int (*runtime_resume)(struct device *dev);
> +	int (*runtime_suspend)(struct device *dev);
>  };
>  
>  #ifdef CONFIG_CPU_HAS_PMU
> diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
> index 186c8cb..3b2b016 100644
> --- a/arch/arm/kernel/perf_event.c
> +++ b/arch/arm/kernel/perf_event.c
> @@ -20,6 +20,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/spinlock.h>
>  #include <linux/uaccess.h>
> +#include <linux/pm_runtime.h>
>  
>  #include <asm/cputype.h>
>  #include <asm/irq.h>
> @@ -460,6 +461,8 @@ hw_perf_event_destroy(struct perf_event *event)
>  		armpmu_release_hardware(armpmu);
>  		mutex_unlock(pmu_reserve_mutex);
>  	}
> +
> +	pm_runtime_put_sync(&armpmu->plat_device->dev);
>  }
>  
>  static int
> @@ -546,6 +549,8 @@ static int armpmu_event_init(struct perf_event *event)
>  	if (armpmu->map_event(event) == -ENOENT)
>  		return -ENOENT;
>  
> +	pm_runtime_get_sync(&armpmu->plat_device->dev);
> +
>  	event->destroy = hw_perf_event_destroy;
>  
>  	if (!atomic_inc_not_zero(active_events)) {
> @@ -584,6 +589,26 @@ static void armpmu_disable(struct pmu *pmu)
>  	armpmu->stop();
>  }
>  
> +static int armpmu_runtime_resume(struct device *dev)
> +{
> +	struct arm_pmu_platdata *plat = dev_get_platdata(dev);
> +
> +	if (plat->runtime_resume)
> +		return plat->runtime_resume(dev);
> +
> +	return 0;
> +}
> +
> +static int armpmu_runtime_suspend(struct device *dev)
> +{
> +	struct arm_pmu_platdata *plat = dev_get_platdata(dev);
> +
> +	if (plat->runtime_suspend)
> +		return plat->runtime_suspend(dev);
> +
> +	return 0;
> +}
> +
>  static void __init armpmu_init(struct arm_pmu *armpmu)
>  {
>  	atomic_set(&armpmu->active_events, 0);
> @@ -650,9 +675,14 @@ static int __devinit armpmu_device_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct dev_pm_ops armpmu_dev_pm_ops = {
> +	SET_RUNTIME_PM_OPS(armpmu_runtime_suspend, armpmu_runtime_resume, NULL)
> +};
> +
>  static struct platform_driver armpmu_driver = {
>  	.driver		= {
>  		.name	= "arm-pmu",
> +		.pm	= &armpmu_dev_pm_ops,
>  		.of_match_table = armpmu_of_device_ids,
>  	},
>  	.probe		= armpmu_device_probe,

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Jon Hunter]

Hi Kevin,

On 05/31/2012 03:42 PM, Kevin Hilman wrote:
> Jon Hunter <jon-hunter@ti.com> writes:
> 
>> Hi Kevin, Will,
>>
>> On 05/30/2012 08:29 PM, Will Deacon wrote:
>>> Hi Kevin,
>>>
>>> On Wed, May 30, 2012 at 10:50:01PM +0100, Kevin Hilman wrote:
>>>> Basically, I don't like the result when we have to hack around missing
>>>> runtime PM support for a driver, so IMO, the driver should be updated.
>>>>
>>>> IOW, it looks to me like the armpmu driver should grow runtime PM
>>>> support.  The current armpmu_release|reserve should probably be replaced
>>>> with runtime PM get/put, and the functionality in those functions would
>>>> be the runtime PM callbacks instead.
>>>>
>>>> Will, any objections to armpmu growing runtime PM support?
>>>
>>> My plan for the armpmu reservation is to kill the global reservation scheme
>>> that we currently have and push those function pointers into the arm_pmu,
>>> so that fits with what you'd like.
>>>
>>> The only concern I have is that we need the mutual exclusion even when we
>>> don't have support for runtime PM. If we can solve that then I'm fine with
>>> the approach.
>>
>> To add a bit more food for thought, I had implemented a quick patch to
>> add runtime PM support for PMU. You will notice that I have been
>> conservative on where I have placed the pm_runtime_get/put calls,
>> because I am not too familiar with the PMU driver to know exactly
>> where we need to maintain the PMU context. So right now these are just
>> around the reserve_hardware/release_hardware calls. This works on OMAP
>> for some quick testing. However, I would need to make sure this does
>> not break compilation without runtime PM enabled.
>>
>> Let me know your thoughts.
> 
> That looks good, but I'm curious what would be done in the new
> plat->runtime_* hooks.  Maybe the irq enable/disable stuff in the pmu
> driver needs to be moved into the runtime PM hooks?

For omap4, the plat->runtime_* hooks look like ...

+static int omap4_pmu_runtime_resume(struct device *dev)
+{
+       /* configure CTI0 for PMU IRQ routing */
+       cti_unlock(&omap4_cti[0]);
+       cti_map_trigger(&omap4_cti[0], 1, 6, 2);
+       cti_enable(&omap4_cti[0]);
+
+       /* configure CTI1 for PMU IRQ routing */
+       cti_unlock(&omap4_cti[1]);
+       cti_map_trigger(&omap4_cti[1], 1, 6, 3);
+       cti_enable(&omap4_cti[1]);
+
+       return 0;
+}
+
+static int omap4_pmu_runtime_suspend(struct device *dev)
+{
+       cti_disable(&omap4_cti[0]);
+       cti_disable(&omap4_cti[1]);
+
+       return 0;
+}

This is what I have implemented so far and currently testing. So really
just using the hooks to configure the cross triggering interface.

Is this what you were thinking?

Cheers
Jon

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Kevin Hilman]

Jon Hunter <jon-hunter@ti.com> writes:

> Hi Kevin,
>
> On 05/31/2012 03:42 PM, Kevin Hilman wrote:
>> Jon Hunter <jon-hunter@ti.com> writes:
>> 
>>> Hi Kevin, Will,
>>>
>>> On 05/30/2012 08:29 PM, Will Deacon wrote:
>>>> Hi Kevin,
>>>>
>>>> On Wed, May 30, 2012 at 10:50:01PM +0100, Kevin Hilman wrote:
>>>>> Basically, I don't like the result when we have to hack around missing
>>>>> runtime PM support for a driver, so IMO, the driver should be updated.
>>>>>
>>>>> IOW, it looks to me like the armpmu driver should grow runtime PM
>>>>> support.  The current armpmu_release|reserve should probably be replaced
>>>>> with runtime PM get/put, and the functionality in those functions would
>>>>> be the runtime PM callbacks instead.
>>>>>
>>>>> Will, any objections to armpmu growing runtime PM support?
>>>>
>>>> My plan for the armpmu reservation is to kill the global reservation scheme
>>>> that we currently have and push those function pointers into the arm_pmu,
>>>> so that fits with what you'd like.
>>>>
>>>> The only concern I have is that we need the mutual exclusion even when we
>>>> don't have support for runtime PM. If we can solve that then I'm fine with
>>>> the approach.
>>>
>>> To add a bit more food for thought, I had implemented a quick patch to
>>> add runtime PM support for PMU. You will notice that I have been
>>> conservative on where I have placed the pm_runtime_get/put calls,
>>> because I am not too familiar with the PMU driver to know exactly
>>> where we need to maintain the PMU context. So right now these are just
>>> around the reserve_hardware/release_hardware calls. This works on OMAP
>>> for some quick testing. However, I would need to make sure this does
>>> not break compilation without runtime PM enabled.
>>>
>>> Let me know your thoughts.
>> 
>> That looks good, but I'm curious what would be done in the new
>> plat->runtime_* hooks.  Maybe the irq enable/disable stuff in the pmu
>> driver needs to be moved into the runtime PM hooks?
>
> For omap4, the plat->runtime_* hooks look like ...
>
> +static int omap4_pmu_runtime_resume(struct device *dev)
> +{
> +       /* configure CTI0 for PMU IRQ routing */
> +       cti_unlock(&omap4_cti[0]);
> +       cti_map_trigger(&omap4_cti[0], 1, 6, 2);
> +       cti_enable(&omap4_cti[0]);
> +
> +       /* configure CTI1 for PMU IRQ routing */
> +       cti_unlock(&omap4_cti[1]);
> +       cti_map_trigger(&omap4_cti[1], 1, 6, 3);
> +       cti_enable(&omap4_cti[1]);
> +
> +       return 0;
> +}
> +
> +static int omap4_pmu_runtime_suspend(struct device *dev)
> +{
> +       cti_disable(&omap4_cti[0]);
> +       cti_disable(&omap4_cti[1]);
> +
> +       return 0;
> +}
>
> This is what I have implemented so far and currently testing. So really
> just using the hooks to configure the cross triggering interface.
>
> Is this what you were thinking?
>

Basically, yes.  

But since I haven't studied the PMU driver closely, I have some dumb
questions.  My concern is that these look bsically like the
plat->irq_[enable|disable] hooks, so I guess the root of my question is
do we need both the irq enable/disable and runtime suspend/resume hooks
in plat?  or can we get by with one set.

Kevin

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Jon Hunter]

Hi Kevin,

On 05/31/2012 05:36 PM, Kevin Hilman wrote:
> Jon Hunter <jon-hunter@ti.com> writes:
> 
>> Hi Kevin,
>>
>> On 05/31/2012 03:42 PM, Kevin Hilman wrote:
>>> Jon Hunter <jon-hunter@ti.com> writes:
>>>
>>>> Hi Kevin, Will,
>>>>
>>>> On 05/30/2012 08:29 PM, Will Deacon wrote:
>>>>> Hi Kevin,
>>>>>
>>>>> On Wed, May 30, 2012 at 10:50:01PM +0100, Kevin Hilman wrote:
>>>>>> Basically, I don't like the result when we have to hack around missing
>>>>>> runtime PM support for a driver, so IMO, the driver should be updated.
>>>>>>
>>>>>> IOW, it looks to me like the armpmu driver should grow runtime PM
>>>>>> support.  The current armpmu_release|reserve should probably be replaced
>>>>>> with runtime PM get/put, and the functionality in those functions would
>>>>>> be the runtime PM callbacks instead.
>>>>>>
>>>>>> Will, any objections to armpmu growing runtime PM support?
>>>>>
>>>>> My plan for the armpmu reservation is to kill the global reservation scheme
>>>>> that we currently have and push those function pointers into the arm_pmu,
>>>>> so that fits with what you'd like.
>>>>>
>>>>> The only concern I have is that we need the mutual exclusion even when we
>>>>> don't have support for runtime PM. If we can solve that then I'm fine with
>>>>> the approach.
>>>>
>>>> To add a bit more food for thought, I had implemented a quick patch to
>>>> add runtime PM support for PMU. You will notice that I have been
>>>> conservative on where I have placed the pm_runtime_get/put calls,
>>>> because I am not too familiar with the PMU driver to know exactly
>>>> where we need to maintain the PMU context. So right now these are just
>>>> around the reserve_hardware/release_hardware calls. This works on OMAP
>>>> for some quick testing. However, I would need to make sure this does
>>>> not break compilation without runtime PM enabled.
>>>>
>>>> Let me know your thoughts.
>>>
>>> That looks good, but I'm curious what would be done in the new
>>> plat->runtime_* hooks.  Maybe the irq enable/disable stuff in the pmu
>>> driver needs to be moved into the runtime PM hooks?
>>
>> For omap4, the plat->runtime_* hooks look like ...
>>
>> +static int omap4_pmu_runtime_resume(struct device *dev)
>> +{
>> +       /* configure CTI0 for PMU IRQ routing */
>> +       cti_unlock(&omap4_cti[0]);
>> +       cti_map_trigger(&omap4_cti[0], 1, 6, 2);
>> +       cti_enable(&omap4_cti[0]);
>> +
>> +       /* configure CTI1 for PMU IRQ routing */
>> +       cti_unlock(&omap4_cti[1]);
>> +       cti_map_trigger(&omap4_cti[1], 1, 6, 3);
>> +       cti_enable(&omap4_cti[1]);
>> +
>> +       return 0;
>> +}
>> +
>> +static int omap4_pmu_runtime_suspend(struct device *dev)
>> +{
>> +       cti_disable(&omap4_cti[0]);
>> +       cti_disable(&omap4_cti[1]);
>> +
>> +       return 0;
>> +}
>>
>> This is what I have implemented so far and currently testing. So really
>> just using the hooks to configure the cross triggering interface.
>>
>> Is this what you were thinking?
>>
> 
> Basically, yes.  
> 
> But since I haven't studied the PMU driver closely, I have some dumb
> questions.  My concern is that these look bsically like the
> plat->irq_[enable|disable] hooks, so I guess the root of my question is
> do we need both the irq enable/disable and runtime suspend/resume hooks
> in plat?  or can we get by with one set.

No you are right. The way it is now we could get by with just the one of
hooks. However, the main reason I added the new hooks would be if there
are other places we can call the pm_runtime_* functions. I am not too
familiar with the flow in which the functions are called in the PMU
driver and so this was a simple attempt to push the PM runtime framework
in the PMU driver.

Hmmm ... however, now looking at the history behind the plat->irq_*
hooks, I see that Ming specifically added these for omap4 [1]. I was
under the impression other architectures may be using this. I guess not.
So if it is preferred we could do-away with the plat->irq_* and replace
with the plat->runtime_*.

Cheers
Jon

[1] http://marc.info/?l=linux-arm-kernel&m=131946766428315&w=2

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Kevin Hilman]

Jon Hunter <jon-hunter@ti.com> writes:

> Hi Kevin,
>
> On 05/31/2012 05:36 PM, Kevin Hilman wrote:
>> Jon Hunter <jon-hunter@ti.com> writes:
>> 
>>> Hi Kevin,
>>>
>>> On 05/31/2012 03:42 PM, Kevin Hilman wrote:
>>>> Jon Hunter <jon-hunter@ti.com> writes:
>>>>
>>>>> Hi Kevin, Will,
>>>>>
>>>>> On 05/30/2012 08:29 PM, Will Deacon wrote:
>>>>>> Hi Kevin,
>>>>>>
>>>>>> On Wed, May 30, 2012 at 10:50:01PM +0100, Kevin Hilman wrote:
>>>>>>> Basically, I don't like the result when we have to hack around missing
>>>>>>> runtime PM support for a driver, so IMO, the driver should be updated.
>>>>>>>
>>>>>>> IOW, it looks to me like the armpmu driver should grow runtime PM
>>>>>>> support.  The current armpmu_release|reserve should probably be replaced
>>>>>>> with runtime PM get/put, and the functionality in those functions would
>>>>>>> be the runtime PM callbacks instead.
>>>>>>>
>>>>>>> Will, any objections to armpmu growing runtime PM support?
>>>>>>
>>>>>> My plan for the armpmu reservation is to kill the global reservation scheme
>>>>>> that we currently have and push those function pointers into the arm_pmu,
>>>>>> so that fits with what you'd like.
>>>>>>
>>>>>> The only concern I have is that we need the mutual exclusion even when we
>>>>>> don't have support for runtime PM. If we can solve that then I'm fine with
>>>>>> the approach.
>>>>>
>>>>> To add a bit more food for thought, I had implemented a quick patch to
>>>>> add runtime PM support for PMU. You will notice that I have been
>>>>> conservative on where I have placed the pm_runtime_get/put calls,
>>>>> because I am not too familiar with the PMU driver to know exactly
>>>>> where we need to maintain the PMU context. So right now these are just
>>>>> around the reserve_hardware/release_hardware calls. This works on OMAP
>>>>> for some quick testing. However, I would need to make sure this does
>>>>> not break compilation without runtime PM enabled.
>>>>>
>>>>> Let me know your thoughts.
>>>>
>>>> That looks good, but I'm curious what would be done in the new
>>>> plat->runtime_* hooks.  Maybe the irq enable/disable stuff in the pmu
>>>> driver needs to be moved into the runtime PM hooks?
>>>
>>> For omap4, the plat->runtime_* hooks look like ...
>>>
>>> +static int omap4_pmu_runtime_resume(struct device *dev)
>>> +{
>>> +       /* configure CTI0 for PMU IRQ routing */
>>> +       cti_unlock(&omap4_cti[0]);
>>> +       cti_map_trigger(&omap4_cti[0], 1, 6, 2);
>>> +       cti_enable(&omap4_cti[0]);
>>> +
>>> +       /* configure CTI1 for PMU IRQ routing */
>>> +       cti_unlock(&omap4_cti[1]);
>>> +       cti_map_trigger(&omap4_cti[1], 1, 6, 3);
>>> +       cti_enable(&omap4_cti[1]);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int omap4_pmu_runtime_suspend(struct device *dev)
>>> +{
>>> +       cti_disable(&omap4_cti[0]);
>>> +       cti_disable(&omap4_cti[1]);
>>> +
>>> +       return 0;
>>> +}
>>>
>>> This is what I have implemented so far and currently testing. So really
>>> just using the hooks to configure the cross triggering interface.
>>>
>>> Is this what you were thinking?
>>>
>> 
>> Basically, yes.  
>> 
>> But since I haven't studied the PMU driver closely, I have some dumb
>> questions.  My concern is that these look bsically like the
>> plat->irq_[enable|disable] hooks, so I guess the root of my question is
>> do we need both the irq enable/disable and runtime suspend/resume hooks
>> in plat?  or can we get by with one set.
>
> No you are right. The way it is now we could get by with just the one of
> hooks. However, the main reason I added the new hooks would be if there
> are other places we can call the pm_runtime_* functions. I am not too
> familiar with the flow in which the functions are called in the PMU
> driver and so this was a simple attempt to push the PM runtime framework
> in the PMU driver.
>
> Hmmm ... however, now looking at the history behind the plat->irq_*
> hooks, I see that Ming specifically added these for omap4 [1]. I was
> under the impression other architectures may be using this. I guess not.
> So if it is preferred we could do-away with the plat->irq_* and replace
> with the plat->runtime_*.

Yes, that would certainly be my preference from a runtime PM readability
PoV.  I guess it's Will's call since it's his code.

Kevin

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Jon Hunter]

On 05/31/2012 07:27 PM, Kevin Hilman wrote:
> Jon Hunter <jon-hunter@ti.com> writes:
> 
>> Hi Kevin,
>>
>> On 05/31/2012 05:36 PM, Kevin Hilman wrote:
>>> Jon Hunter <jon-hunter@ti.com> writes:
>>>
>>>> Hi Kevin,
>>>>
>>>> On 05/31/2012 03:42 PM, Kevin Hilman wrote:
>>>>> Jon Hunter <jon-hunter@ti.com> writes:
>>>>>
>>>>>> Hi Kevin, Will,
>>>>>>
>>>>>> On 05/30/2012 08:29 PM, Will Deacon wrote:
>>>>>>> Hi Kevin,
>>>>>>>
>>>>>>> On Wed, May 30, 2012 at 10:50:01PM +0100, Kevin Hilman wrote:
>>>>>>>> Basically, I don't like the result when we have to hack around missing
>>>>>>>> runtime PM support for a driver, so IMO, the driver should be updated.
>>>>>>>>
>>>>>>>> IOW, it looks to me like the armpmu driver should grow runtime PM
>>>>>>>> support.  The current armpmu_release|reserve should probably be replaced
>>>>>>>> with runtime PM get/put, and the functionality in those functions would
>>>>>>>> be the runtime PM callbacks instead.
>>>>>>>>
>>>>>>>> Will, any objections to armpmu growing runtime PM support?
>>>>>>>
>>>>>>> My plan for the armpmu reservation is to kill the global reservation scheme
>>>>>>> that we currently have and push those function pointers into the arm_pmu,
>>>>>>> so that fits with what you'd like.
>>>>>>>
>>>>>>> The only concern I have is that we need the mutual exclusion even when we
>>>>>>> don't have support for runtime PM. If we can solve that then I'm fine with
>>>>>>> the approach.
>>>>>>
>>>>>> To add a bit more food for thought, I had implemented a quick patch to
>>>>>> add runtime PM support for PMU. You will notice that I have been
>>>>>> conservative on where I have placed the pm_runtime_get/put calls,
>>>>>> because I am not too familiar with the PMU driver to know exactly
>>>>>> where we need to maintain the PMU context. So right now these are just
>>>>>> around the reserve_hardware/release_hardware calls. This works on OMAP
>>>>>> for some quick testing. However, I would need to make sure this does
>>>>>> not break compilation without runtime PM enabled.
>>>>>>
>>>>>> Let me know your thoughts.
>>>>>
>>>>> That looks good, but I'm curious what would be done in the new
>>>>> plat->runtime_* hooks.  Maybe the irq enable/disable stuff in the pmu
>>>>> driver needs to be moved into the runtime PM hooks?
>>>>
>>>> For omap4, the plat->runtime_* hooks look like ...
>>>>
>>>> +static int omap4_pmu_runtime_resume(struct device *dev)
>>>> +{
>>>> +       /* configure CTI0 for PMU IRQ routing */
>>>> +       cti_unlock(&omap4_cti[0]);
>>>> +       cti_map_trigger(&omap4_cti[0], 1, 6, 2);
>>>> +       cti_enable(&omap4_cti[0]);
>>>> +
>>>> +       /* configure CTI1 for PMU IRQ routing */
>>>> +       cti_unlock(&omap4_cti[1]);
>>>> +       cti_map_trigger(&omap4_cti[1], 1, 6, 3);
>>>> +       cti_enable(&omap4_cti[1]);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static int omap4_pmu_runtime_suspend(struct device *dev)
>>>> +{
>>>> +       cti_disable(&omap4_cti[0]);
>>>> +       cti_disable(&omap4_cti[1]);
>>>> +
>>>> +       return 0;
>>>> +}
>>>>
>>>> This is what I have implemented so far and currently testing. So really
>>>> just using the hooks to configure the cross triggering interface.
>>>>
>>>> Is this what you were thinking?
>>>>
>>>
>>> Basically, yes.  
>>>
>>> But since I haven't studied the PMU driver closely, I have some dumb
>>> questions.  My concern is that these look bsically like the
>>> plat->irq_[enable|disable] hooks, so I guess the root of my question is
>>> do we need both the irq enable/disable and runtime suspend/resume hooks
>>> in plat?  or can we get by with one set.
>>
>> No you are right. The way it is now we could get by with just the one of
>> hooks. However, the main reason I added the new hooks would be if there
>> are other places we can call the pm_runtime_* functions. I am not too
>> familiar with the flow in which the functions are called in the PMU
>> driver and so this was a simple attempt to push the PM runtime framework
>> in the PMU driver.
>>
>> Hmmm ... however, now looking at the history behind the plat->irq_*
>> hooks, I see that Ming specifically added these for omap4 [1]. I was
>> under the impression other architectures may be using this. I guess not.
>> So if it is preferred we could do-away with the plat->irq_* and replace
>> with the plat->runtime_*.
> 
> Yes, that would certainly be my preference from a runtime PM readability
> PoV.  I guess it's Will's call since it's his code.

Ok great.

Will, let me know your thoughts. I have a V2 series ready to post, I
just need to know your thoughts on handling this runtime PM business. Or
if you would just like me to send it out for review anyway, I can do
that too.

Cheers
Jon

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Will Deacon]

Hi Jon, Kevin,

I've been between timezones, so sorry for the slow response.

On Fri, Jun 01, 2012 at 03:42:56PM +0100, Jon Hunter wrote:
> On 05/31/2012 07:27 PM, Kevin Hilman wrote:
> >> Hmmm ... however, now looking at the history behind the plat->irq_*
> >> hooks, I see that Ming specifically added these for omap4 [1]. I was
> >> under the impression other architectures may be using this. I guess not.
> >> So if it is preferred we could do-away with the plat->irq_* and replace
> >> with the plat->runtime_*.
> > 
> > Yes, that would certainly be my preference from a runtime PM readability
> > PoV.  I guess it's Will's call since it's his code.
> 
> Ok great.
> 
> Will, let me know your thoughts. I have a V2 series ready to post, I
> just need to know your thoughts on handling this runtime PM business. Or
> if you would just like me to send it out for review anyway, I can do
> that too.

>From my perspective, we have up to three pairs of functions on the PMU:

1. enable/disable irq
2. reserve/release pmu
3. suspend/resume pmu

As you pointed out, (1) was added purely for OMAP so I'd definitely like
to remove that if we can. What I wonder is whether (2) and (3) can also be
combined into a single pair of functions. The default behaviour could be the
simple bit lock that we have in pmu.c. If a platform wants to do more, then
it can supply its own functions for these and do PM there as well as the
mutual exclusion (which we may well get for free). This also fits with my
previous plans for making reserve/release PMU-specific and killing the
arm_pmu_type enumeration.

So, if we can condense this all down into one pair of functions that also
work correctly for the non-PM case (including mutual exclusion) then I'd
love to see that merged.

Cheers,

Will

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Jon Hunter]

Hi Will,

On 06/02/2012 11:42 AM, Will Deacon wrote:
> Hi Jon, Kevin,
> 
> I've been between timezones, so sorry for the slow response.

No problem. I was expecting you guys in the UK to be out of office for the
next couple days :-)
 
> On Fri, Jun 01, 2012 at 03:42:56PM +0100, Jon Hunter wrote:
>> On 05/31/2012 07:27 PM, Kevin Hilman wrote:
>>>> Hmmm ... however, now looking at the history behind the plat->irq_*
>>>> hooks, I see that Ming specifically added these for omap4 [1]. I was
>>>> under the impression other architectures may be using this. I guess not.
>>>> So if it is preferred we could do-away with the plat->irq_* and replace
>>>> with the plat->runtime_*.
>>>
>>> Yes, that would certainly be my preference from a runtime PM readability
>>> PoV.  I guess it's Will's call since it's his code.
>>
>> Ok great.
>>
>> Will, let me know your thoughts. I have a V2 series ready to post, I
>> just need to know your thoughts on handling this runtime PM business. Or
>> if you would just like me to send it out for review anyway, I can do
>> that too.
> 
> From my perspective, we have up to three pairs of functions on the PMU:
> 
> 1. enable/disable irq
> 2. reserve/release pmu
> 3. suspend/resume pmu
> 
> As you pointed out, (1) was added purely for OMAP so I'd definitely like
> to remove that if we can. What I wonder is whether (2) and (3) can also be
> combined into a single pair of functions. The default behaviour could be the
> simple bit lock that we have in pmu.c. If a platform wants to do more, then
> it can supply its own functions for these and do PM there as well as the
> mutual exclusion (which we may well get for free). This also fits with my
> previous plans for making reserve/release PMU-specific and killing the
> arm_pmu_type enumeration.
> 
> So, if we can condense this all down into one pair of functions that also
> work correctly for the non-PM case (including mutual exclusion) then I'd
> love to see that merged.

That is definitely a possibility, but I believe that Kevin wanted this to
use the runtime callbacks so this fits in nicely with the pm runtime model.
The only slight difference I see here with what Kevin was suggesting is we
have ...

1. enable/disable irq
2. reserve/release pmu
3. pm_runtime_get/put pmu (enable/disable pmu)
4. runtime_resume/suspend (callbacks invoked by pm_runtime_get/put)

The callbacks, #4, are the hooks we use to do the custom platform 
specific stuff and the pm_runtime_get/put calls are non-platform 
specific functions to enable/disable the pmu.

So what we can do is replace #1 with #4 and then group #2 and #3
together. In fact we could place the pm_runtime_get/put in the
release/release pmu functions.

So with that being said, we could do something like the following.
Please note that patch is a bit nosier that I wanted as I needed to 
move the definitions of the reserve/release pmu for it to compile
(I need to look at this some more to make sure it does not break
anything).

>From 15940e67c0f012278c0b65aed910fa5b0de20f06 Mon Sep 17 00:00:00 2001
From: Jon Hunter <jon-hunter@ti.com>
Date: Thu, 31 May 2012 13:05:20 -0500
Subject: [PATCH] ARM: PMU: Add runtime PM Support

Add runtime PM support to the ARM PMU driver so that devices such as OMAP
supporting dynamic PM can use the platform->runtime_* hooks to initialise
hardware at runtime. Without having these runtime PM hooks in place any
configuration of the PMU hardware would be lost when low power states are
entered and hence would prevent PMU from working.

This change also replaces the PMU platform functions enable_irq and disable_irq
added by Ming Lei with runtime_resume and runtime_suspend funtions. Ming had
added the enable_irq and disable_irq functions as a method to configure the
cross trigger interface on OMAP4 for routing the PMU interrupts. By adding
runtime PM support, we can move the code called by enable_irq and disable_irq
into the runtime PM callbacks runtime_resume and runtime_suspend.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/include/asm/pmu.h   |   88 ++++++++++++++++++++++--------------------
 arch/arm/kernel/perf_event.c |   40 ++++++++++++++-----
 arch/arm/kernel/pmu.c        |   12 ++++--
 3 files changed, 84 insertions(+), 56 deletions(-)

diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
index 90114fa..763faf5 100644
--- a/arch/arm/include/asm/pmu.h
+++ b/arch/arm/include/asm/pmu.h
@@ -31,54 +31,24 @@ enum arm_pmu_type {
  *	interrupt and passed the address of the low level handler,
  *	and can be used to implement any platform specific handling
  *	before or after calling it.
- * @enable_irq: an optional handler which will be called after
- *	request_irq and be used to handle some platform specific
- *	irq enablement
- * @disable_irq: an optional handler which will be called before
- *	free_irq and be used to handle some platform specific
- *	irq disablement
+ * @runtime_resume: an optional handler which will be called by the
+ *	runtime PM framework following a call to pm_runtime_get().
+ *	Note that if pm_runtime_get() is called more than once in
+ *	succession this handler will only be called once.
+ * @runtime_suspend: an optional handler which will be called by the
+ *	runtime PM framework following a call to pm_runtime_put().
+ *	Note that if pm_runtime_get() is called more than once in
+ *	succession this handler will only be called following the
+ *	final call to pm_runtime_put() that actually disables the
+ *	hardware.
  */
 struct arm_pmu_platdata {
 	irqreturn_t (*handle_irq)(int irq, void *dev,
 				  irq_handler_t pmu_handler);
-	void (*enable_irq)(int irq);
-	void (*disable_irq)(int irq);
+	int (*runtime_resume)(struct device *dev);
+	int (*runtime_suspend)(struct device *dev);
 };
 
-#ifdef CONFIG_CPU_HAS_PMU
-
-/**
- * reserve_pmu() - reserve the hardware performance counters
- *
- * Reserve the hardware performance counters in the system for exclusive use.
- * Returns 0 on success or -EBUSY if the lock is already held.
- */
-extern int
-reserve_pmu(enum arm_pmu_type type);
-
-/**
- * release_pmu() - Relinquish control of the performance counters
- *
- * Release the performance counters and allow someone else to use them.
- */
-extern void
-release_pmu(enum arm_pmu_type type);
-
-#else /* CONFIG_CPU_HAS_PMU */
-
-#include <linux/err.h>
-
-static inline int
-reserve_pmu(enum arm_pmu_type type)
-{
-	return -ENODEV;
-}
-
-static inline void
-release_pmu(enum arm_pmu_type type)	{ }
-
-#endif /* CONFIG_CPU_HAS_PMU */
-
 #ifdef CONFIG_HW_PERF_EVENTS
 
 /* The events for a given PMU register set. */
@@ -140,6 +110,40 @@ int armpmu_event_set_period(struct perf_event *event,
 			    struct hw_perf_event *hwc,
 			    int idx);
 
+#ifdef CONFIG_CPU_HAS_PMU
+
+/**
+ * reserve_pmu() - reserve the hardware performance counters
+ *
+ * Reserve the hardware performance counters in the system for exclusive use.
+ * Returns 0 on success or -EBUSY if the lock is already held.
+ */
+extern int
+reserve_pmu(struct arm_pmu *armpmu);
+
+/**
+ * release_pmu() - Relinquish control of the performance counters
+ *
+ * Release the performance counters and allow someone else to use them.
+ */
+extern void
+release_pmu(struct arm_pmu *armpmu);
+
+#else /* CONFIG_CPU_HAS_PMU */
+
+#include <linux/err.h>
+
+static inline int
+reserve_pmu(struct arm_pmu *armpmu)
+{
+	return -ENODEV;
+}
+
+static inline void
+release_pmu(struct arm_pmu *armpmu)	{ }
+
+#endif /* CONFIG_CPU_HAS_PMU */
+
 #endif /* CONFIG_HW_PERF_EVENTS */
 
 #endif /* __ARM_PMU_H__ */
diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 186c8cb..34076fb 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -20,6 +20,7 @@
 #include <linux/platform_device.h>
 #include <linux/spinlock.h>
 #include <linux/uaccess.h>
+#include <linux/pm_runtime.h>
 
 #include <asm/cputype.h>
 #include <asm/irq.h>
@@ -367,8 +368,6 @@ armpmu_release_hardware(struct arm_pmu *armpmu)
 {
 	int i, irq, irqs;
 	struct platform_device *pmu_device = armpmu->plat_device;
-	struct arm_pmu_platdata *plat =
-		dev_get_platdata(&pmu_device->dev);
 
 	irqs = min(pmu_device->num_resources, num_possible_cpus());
 
@@ -376,14 +375,11 @@ armpmu_release_hardware(struct arm_pmu *armpmu)
 		if (!cpumask_test_and_clear_cpu(i, &armpmu->active_irqs))
 			continue;
 		irq = platform_get_irq(pmu_device, i);
-		if (irq >= 0) {
-			if (plat && plat->disable_irq)
-				plat->disable_irq(irq);
+		if (irq >= 0)
 			free_irq(irq, armpmu);
-		}
 	}
 
-	release_pmu(armpmu->type);
+	release_pmu(armpmu);
 }
 
 static int
@@ -397,7 +393,7 @@ armpmu_reserve_hardware(struct arm_pmu *armpmu)
 	if (!pmu_device)
 		return -ENODEV;
 
-	err = reserve_pmu(armpmu->type);
+	err = reserve_pmu(armpmu);
 	if (err) {
 		pr_warning("unable to reserve pmu\n");
 		return err;
@@ -440,8 +436,7 @@ armpmu_reserve_hardware(struct arm_pmu *armpmu)
 				irq);
 			armpmu_release_hardware(armpmu);
 			return err;
-		} else if (plat && plat->enable_irq)
-			plat->enable_irq(irq);
+		}
 
 		cpumask_set_cpu(i, &armpmu->active_irqs);
 	}
@@ -584,6 +579,26 @@ static void armpmu_disable(struct pmu *pmu)
 	armpmu->stop();
 }
 
+static int armpmu_runtime_resume(struct device *dev)
+{
+	struct arm_pmu_platdata *plat = dev_get_platdata(dev);
+
+	if (plat->runtime_resume)
+		return plat->runtime_resume(dev);
+
+	return 0;
+}
+
+static int armpmu_runtime_suspend(struct device *dev)
+{
+	struct arm_pmu_platdata *plat = dev_get_platdata(dev);
+
+	if (plat->runtime_suspend)
+		return plat->runtime_suspend(dev);
+
+	return 0;
+}
+
 static void __init armpmu_init(struct arm_pmu *armpmu)
 {
 	atomic_set(&armpmu->active_events, 0);
@@ -650,9 +665,14 @@ static int __devinit armpmu_device_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct dev_pm_ops armpmu_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(armpmu_runtime_suspend, armpmu_runtime_resume, NULL)
+};
+
 static struct platform_driver armpmu_driver = {
 	.driver		= {
 		.name	= "arm-pmu",
+		.pm	= &armpmu_dev_pm_ops,
 		.of_match_table = armpmu_of_device_ids,
 	},
 	.probe		= armpmu_device_probe,
diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c
index 2334bf8..8ffbb09 100644
--- a/arch/arm/kernel/pmu.c
+++ b/arch/arm/kernel/pmu.c
@@ -13,6 +13,8 @@
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/platform_device.h>
 
 #include <asm/pmu.h>
 
@@ -22,15 +24,17 @@
 static unsigned long pmu_lock[BITS_TO_LONGS(ARM_NUM_PMU_DEVICES)];
 
 int
-reserve_pmu(enum arm_pmu_type type)
+reserve_pmu(struct arm_pmu *armpmu)
 {
-	return test_and_set_bit_lock(type, pmu_lock) ? -EBUSY : 0;
+	pm_runtime_get_sync(&armpmu->plat_device->dev);
+	return test_and_set_bit_lock(armpmu->type, pmu_lock) ? -EBUSY : 0;
 }
 EXPORT_SYMBOL_GPL(reserve_pmu);
 
 void
-release_pmu(enum arm_pmu_type type)
+release_pmu(struct arm_pmu *armpmu)
 {
-	clear_bit_unlock(type, pmu_lock);
+	clear_bit_unlock(armpmu->type, pmu_lock);
+	pm_runtime_put_sync(&armpmu->plat_device->dev);
 }
 EXPORT_SYMBOL_GPL(release_pmu);
-- 
1.7.9.5

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Jon Hunter]

Hi Will,

On 06/04/2012 04:44 PM, Jon Hunter wrote:

[...]

> diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c
> index 2334bf8..8ffbb09 100644
> --- a/arch/arm/kernel/pmu.c
> +++ b/arch/arm/kernel/pmu.c
> @@ -13,6 +13,8 @@
>  #include <linux/err.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/platform_device.h>
>  
>  #include <asm/pmu.h>
>  
> @@ -22,15 +24,17 @@
>  static unsigned long pmu_lock[BITS_TO_LONGS(ARM_NUM_PMU_DEVICES)];
>  
>  int
> -reserve_pmu(enum arm_pmu_type type)
> +reserve_pmu(struct arm_pmu *armpmu)
>  {
> -	return test_and_set_bit_lock(type, pmu_lock) ? -EBUSY : 0;
> +	pm_runtime_get_sync(&armpmu->plat_device->dev);
> +	return test_and_set_bit_lock(armpmu->type, pmu_lock) ? -EBUSY : 0;
>  }
>  EXPORT_SYMBOL_GPL(reserve_pmu);
>  
>  void
> -release_pmu(enum arm_pmu_type type)
> +release_pmu(struct arm_pmu *armpmu)
>  {
> -	clear_bit_unlock(type, pmu_lock);
> +	clear_bit_unlock(armpmu->type, pmu_lock);
> +	pm_runtime_put_sync(&armpmu->plat_device->dev);
>  }
>  EXPORT_SYMBOL_GPL(release_pmu);

I have realised that there is a slight bug in the above
pm_runtime_get/put. The calls to pm_runtime_get/put need to be
symmetrical otherwise the if we call _get more than _put the pmu will
stay on. So in the reserve_pmu, I should only call pm_runtime_get if we
acquire the lock.

Anyway, let me know what you think of this approach. An alternative is
to put the calls pm_runtime_get/put outside of the reserve/release_pmu,
which would be a simpler change, but I was thinking that the above maybe
more aligned with your thinking.

Cheers
Jon

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Will Deacon]

On Tue, Jun 05, 2012 at 02:19:02PM +0100, Jon Hunter wrote:
> Hi Will,

Hi Jon,

> On 06/04/2012 04:44 PM, Jon Hunter wrote:
> Anyway, let me know what you think of this approach. An alternative is
> to put the calls pm_runtime_get/put outside of the reserve/release_pmu,
> which would be a simpler change, but I was thinking that the above maybe
> more aligned with your thinking.

Ok, thanks for this. Whilst your code is definitely more like I'm
envisaging, you're right about the churn and until I've sorted out the
reservation code so that it's a callback via the PMU structure, it is
overly messy.

So for the time being let's do what you suggested and put the suspend/resume
calls into armpmu_{reserve,release}_hardware. We can still kill the irq
enable/disable calls and I can rework this slightly when I change the
reservation code.

Cheers,

Will

Re: [PATCH 4/6] ARM: OMAP4: PMU: Add runtime PM support

[Jon Hunter]

Hi Will,

On 06/06/2012 12:33 PM, Will Deacon wrote:
> On Tue, Jun 05, 2012 at 02:19:02PM +0100, Jon Hunter wrote:
>> Hi Will,
> 
> Hi Jon,
> 
>> On 06/04/2012 04:44 PM, Jon Hunter wrote:
>> Anyway, let me know what you think of this approach. An alternative is
>> to put the calls pm_runtime_get/put outside of the reserve/release_pmu,
>> which would be a simpler change, but I was thinking that the above maybe
>> more aligned with your thinking.
> 
> Ok, thanks for this. Whilst your code is definitely more like I'm
> envisaging, you're right about the churn and until I've sorted out the
> reservation code so that it's a callback via the PMU structure, it is
> overly messy.
> 
> So for the time being let's do what you suggested and put the suspend/resume
> calls into armpmu_{reserve,release}_hardware. We can still kill the irq
> enable/disable calls and I can rework this slightly when I change the
> reservation code.

Sounds good. I will send out my V2 series tomorrow that will include the
above change.

Cheers
Jon