ARM: PMU: fix runtime PM enable

PATCH New Jon Hunter 3 participants 10 messages Oct 23, 2012 – Oct 25, 2012

Subsystems (6)

Arm (32-bit) Arm sub-architectures perf events OMAP2+ support +2 more subsystems

lore

Share & tools

lore

Jon Hunter

author

2012-10-23 20:31 UTC #0 lore raw

PATCH 1/1 ARM: PMU: fix runtime PM enable

Commit 7be2958 (ARM: PMU: Add runtime PM Support) updated the ARM PMU code to use runtime PM which was prototyped and validated on the OMAP devices. In this commit, there is no call pm_runtime_enable() and for OMAP devices pm_runtime_enable() is currently being called from the OMAP PMU code when the PMU device is created. However, there are two problems with this:

1. For any other ARM device wishing to use runtime PM for PMU they will need to call pm_runtime_enable() for runtime PM to work.
2. When booting with device-tree and using device-tree to create the PMU device, pm_runtime_enable() needs to be called from within the ARM PERF driver as we are no longer calling any device specific code to create the device. Hence, PMU does not work on OMAP devices that use the runtime PM callbacks when using device-tree to create the PMU device.

Therefore, add a new platform data variable to indicate whether runtime PM is used and if so call pm_runtime_enable() when the PMU device is registered. Note that devices using runtime PM may not use the runtime_resume/suspend callbacks and so we cannot use the presence of these handlers in the platform data to determine whether we should call pm_runtime_enable().

Tested with PERF on OMAP2420, OMAP3430 and OMAP4460.

Signed-off-by Jon Hunter <[email protected]>

---
 arch/arm/include/asm/pmu.h   |    1 +
 arch/arm/kernel/perf_event.c |    6 ++++++
 arch/arm/mach-omap2/pmu.c    |    9 +++++----
 3 files changed, 12 insertions(+), 4 deletions(-)

arch/arm/include/asm/pmu.h +1 -0

index a26170d..50a6c3b 100644
@@ -36,6 +36,7 @@
 struct arm_pmu_platdata {
 	irqreturn_t (*handle_irq)(int irq, void *dev,
 				  irq_handler_t pmu_handler);
+	bool use_runtime_pm;
 	int (*runtime_resume)(struct device *dev);
 	int (*runtime_suspend)(struct device *dev);
 };

arch/arm/kernel/perf_event.c +6 -0

index 93971b1..a8c5ddf 100644
@@ -515,7 +515,13 @@ static void __init armpmu_init(struct arm_pmu *armpmu)
 
 int armpmu_register(struct arm_pmu *armpmu, char *name, int type)
 {
+	struct platform_device *plat_device = armpmu->plat_device;
+	struct arm_pmu_platdata *plat = dev_get_platdata(&plat_device->dev);
+
 	armpmu_init(armpmu);
+	if (plat && plat->use_runtime_pm)
+		pm_runtime_enable(&armpmu->plat_device->dev);
+
 	pr_info("enabled with %s PMU driver, %d counters available\n",
 			armpmu->name, armpmu->num_events);
 	return perf_pmu_register(&armpmu->pmu, name, type);

arch/arm/mach-omap2/pmu.c +5 -4

index 2a79176..1a3d4d7 100644
@@ -22,6 +22,7 @@ static char *omap2_pmu_oh_names[] = {"mpu"};
 static char *omap3_pmu_oh_names[] = {"mpu", "debugss"};
 static char *omap4430_pmu_oh_names[] = {"l3_main_3", "l3_instr", "debugss"};
 static struct platform_device *omap_pmu_dev;
+static struct arm_pmu_platdata omap_pmu_data;
 
 /**
  * omap2_init_pmu - creates and registers PMU platform device

@@ -49,16 +50,14 @@ static int __init omap2_init_pmu(unsigned oh_num, char *oh_names[])
 		}
 	}
 
-	omap_pmu_dev = omap_device_build_ss(dev_name, -1, oh, oh_num, NULL, 0,
-					    NULL, 0, 0);
+	omap_pmu_dev = omap_device_build_ss(dev_name, -1, oh, oh_num,
+			&omap_pmu_data, sizeof(omap_pmu_data), NULL, 0, 0);
 	WARN(IS_ERR(omap_pmu_dev), "Can't build omap_device for %s.\n",
 	     dev_name);
 
 	if (IS_ERR(omap_pmu_dev))
 		return PTR_ERR(omap_pmu_dev);
 
-	pm_runtime_enable(&omap_pmu_dev->dev);
-
 	return 0;
 }

@@ -85,9 +84,11 @@ static int __init omap_init_pmu(void)
 	} else if (cpu_is_omap34xx() || cpu_is_omap44xx()) {
 		oh_num = ARRAY_SIZE(omap3_pmu_oh_names);
 		oh_names = omap3_pmu_oh_names;
+		omap_pmu_data.use_runtime_pm = true;
 	} else {
 		oh_num = ARRAY_SIZE(omap2_pmu_oh_names);
 		oh_names = omap2_pmu_oh_names;
+		omap_pmu_data.use_runtime_pm = false;
 	}
 
 	return omap2_init_pmu(oh_num, oh_names);

Signature · 1 line

1.7.9.5

Will Deacon

2012-10-24 09:31 UTC #1 lore raw

PATCH 1/1 ARM: PMU: fix runtime PM enable

Hi Jon,

Quoted 19 lines · Jon Hunter

Commit 7be2958 (ARM: PMU: Add runtime PM Support) updated the ARM PMU code to use runtime PM which was prototyped and validated on the OMAP devices. In this commit, there is no call pm_runtime_enable() and for OMAP devices pm_runtime_enable() is currently being called from the OMAP PMU code when the PMU device is created. However, there are two problems with this:
1. For any other ARM device wishing to use runtime PM for PMU they will need to call pm_runtime_enable() for runtime PM to work.
2. When booting with device-tree and using device-tree to create the PMU device, pm_runtime_enable() needs to be called from within the ARM PERF driver as we are no longer calling any device specific code to create the device. Hence, PMU does not work on OMAP devices that use the runtime PM callbacks when using device-tree to create the PMU device.
Therefore, add a new platform data variable to indicate whether runtime PM is used and if so call pm_runtime_enable() when the PMU device is registered. Note that devices using runtime PM may not use the runtime_resume/suspend callbacks and so we cannot use the presence of these handlers in the platform data to determine whether we should call pm_runtime_enable().

[...]

arch/arm/include/asm/pmu.h line 41

Quoted 11 lines

arch/arm/include/asm/pmu.h +1 -0

index a26170d..50a6c3b 100644
@@ -36,6 +36,7 @@
 struct arm_pmu_platdata {
 	irqreturn_t (*handle_irq)(int irq, void *dev,
 				  irq_handler_t pmu_handler);
+	bool use_runtime_pm;
 	int (*runtime_resume)(struct device *dev);
 	int (*runtime_suspend)(struct device *dev);

Can we not just use the presence of the resume/suspend function pointers to indicate whether we should enable runtime pm or not? i.e. if they're not NULL, then enable the thing?

Cheers,

Will

Jon Hunter

author

2012-10-24 14:16 UTC #2 lore raw

PATCH 1/1 ARM: PMU: fix runtime PM enable

Hi Will,

Quoted 25 lines · Will Deacon

Hi Jon,
Quoted 19 lines · Jon Hunter
Commit 7be2958 (ARM: PMU: Add runtime PM Support) updated the ARM PMU code to use runtime PM which was prototyped and validated on the OMAP devices. In this commit, there is no call pm_runtime_enable() and for OMAP devices pm_runtime_enable() is currently being called from the OMAP PMU code when the PMU device is created. However, there are two problems with this:
1. For any other ARM device wishing to use runtime PM for PMU they will need to call pm_runtime_enable() for runtime PM to work.
2. When booting with device-tree and using device-tree to create the PMU device, pm_runtime_enable() needs to be called from within the ARM PERF driver as we are no longer calling any device specific code to create the device. Hence, PMU does not work on OMAP devices that use the runtime PM callbacks when using device-tree to create the PMU device.
Therefore, add a new platform data variable to indicate whether runtime PM is used and if so call pm_runtime_enable() when the PMU device is registered. Note that devices using runtime PM may not use the runtime_resume/suspend callbacks and so we cannot use the presence of these handlers in the platform data to determine whether we should call pm_runtime_enable().
[...]

Quoted 15 lines

arch/arm/include/asm/pmu.h line 41
Quoted 11 lines
arch/arm/include/asm/pmu.h +1 -0
index a26170d..50a6c3b 100644
@@ -36,6 +36,7 @@
 struct arm_pmu_platdata {
 	irqreturn_t (*handle_irq)(int irq, void *dev,
 				  irq_handler_t pmu_handler);
+	bool use_runtime_pm;
 	int (*runtime_resume)(struct device *dev);
 	int (*runtime_suspend)(struct device *dev);
Can we not just use the presence of the resume/suspend function pointers to indicate whether we should enable runtime pm or not? i.e. if they're not NULL, then enable the thing?

I wondered if you would ask that :-)

Unfortunately, we can't. For example, OMAP3430 and OMAP4460 both require runtime_pm to be enabled to turn on the debug sub-system in OMAP for PMU to work. However, neither of these devices are using the suspend/resume callbacks because the HWMOD framework is doing this for us behind the scenes.

So then you ask, why do we need the resume/suspend callbacks, well we will need them for OMAP4430 where in addition to turning on the debug sub-system we need to configure the CTI module. Therefore, I had to add another plat data variable.

By the way, I did add a comment in the above changelog about this. See the last paragraph, not sure if this is 100% clear.

One alternative I had thought about was always calling pm_runtime_enable() on registering the PMU device and avoid having a use_runtime_pm variable. For ARM platforms that don't use runtime PM the pm_runtime_enable() function does nothing. However, I was more concerned about adding unnecessary overhead for ARM platforms that are using runtime PM but don't need runtime PM for PMU. I don't see any side affects on our OMAP2 platform that does not need runtime PM for PMU. However, was not sure what is best here.

Cheers
Jon

Will Deacon

2012-10-24 14:32 UTC #3 lore raw

PATCH 1/1 ARM: PMU: fix runtime PM enable

Quoted 15 lines · Jon Hunter

Hi Will,
Quoted 3 lines · Will Deacon
Can we not just use the presence of the resume/suspend function pointers to indicate whether we should enable runtime pm or not? i.e. if they're not NULL, then enable the thing?
I wondered if you would ask that :-)
Unfortunately, we can't. For example, OMAP3430 and OMAP4460 both require runtime_pm to be enabled to turn on the debug sub-system in OMAP for PMU to work. However, neither of these devices are using the suspend/resume callbacks because the HWMOD framework is doing this for us behind the scenes.

Quoted 4 lines

So then you ask, why do we need the resume/suspend callbacks, well we will need them for OMAP4430 where in addition to turning on the debug sub-system we need to configure the CTI module. Therefore, I had to add another plat data variable.

Hmmm, now I start to wonder whether your original idea of having separate callbacks for enable/disable irq and resume/suspend doesn't make more sense. Then the CTI magic can go in the irq management code and be totally separate to the PM stuff.

What do you reckon?

Quoted 2 lines

By the way, I did add a comment in the above changelog about this. See the last paragraph, not sure if this is 100% clear.

Sorry, I missed that this morning.

Quoted 8 lines

One alternative I had thought about was always calling pm_runtime_enable() on registering the PMU device and avoid having a use_runtime_pm variable. For ARM platforms that don't use runtime PM the pm_runtime_enable() function does nothing. However, I was more concerned about adding unnecessary overhead for ARM platforms that are using runtime PM but don't need runtime PM for PMU. I don't see any side affects on our OMAP2 platform that does not need runtime PM for PMU. However, was not sure what is best here.

Nah, we should be able to fix this in the platdata, I'd just rather have function pointers instead of state variables in there.

Will

Jon Hunter

author

2012-10-24 15:06 UTC #4 lore raw

PATCH 1/1 ARM: PMU: fix runtime PM enable

Quoted 21 lines · Will Deacon

Quoted 15 lines · Jon Hunter
Hi Will,
Quoted 3 lines · Will Deacon
Can we not just use the presence of the resume/suspend function pointers to indicate whether we should enable runtime pm or not? i.e. if they're not NULL, then enable the thing?
I wondered if you would ask that :-)
Unfortunately, we can't. For example, OMAP3430 and OMAP4460 both require runtime_pm to be enabled to turn on the debug sub-system in OMAP for PMU to work. However, neither of these devices are using the suspend/resume callbacks because the HWMOD framework is doing this for us behind the scenes.
Quoted 4 lines
So then you ask, why do we need the resume/suspend callbacks, well we will need them for OMAP4430 where in addition to turning on the debug sub-system we need to configure the CTI module. Therefore, I had to add another plat data variable.

Quoted 6 lines

Hmmm, now I start to wonder whether your original idea of having separate callbacks for enable/disable irq and resume/suspend doesn't make more sense. Then the CTI magic can go in the irq management code and be totally separate to the PM stuff.
What do you reckon?

The resume/suspend calls really replaced the enable/disable irq callbacks. That still seems like a good approach given that we need runtime PM for OMAP and PMU.

The problem is going to be with device-tree. When booting with device-tree we will not call the current OMAP specific code that is creating the PMU device and calling pm_runtime_enable(). So calling pm_runtime_enable() needs to be done somewhere in the PERF driver and that's the real problem here.

Quoted 14 lines

Quoted 2 lines
By the way, I did add a comment in the above changelog about this. See the last paragraph, not sure if this is 100% clear.
Sorry, I missed that this morning.
Quoted 8 lines
One alternative I had thought about was always calling pm_runtime_enable() on registering the PMU device and avoid having a use_runtime_pm variable. For ARM platforms that don't use runtime PM the pm_runtime_enable() function does nothing. However, I was more concerned about adding unnecessary overhead for ARM platforms that are using runtime PM but don't need runtime PM for PMU. I don't see any side affects on our OMAP2 platform that does not need runtime PM for PMU. However, was not sure what is best here.

Quoted 2 lines

Nah, we should be able to fix this in the platdata, I'd just rather have function pointers instead of state variables in there.

Well, we could pass a pointer to pm_runtime_enable() function in the platdata.

Cheers
Jon

Will Deacon

2012-10-24 17:23 UTC #5 lore raw

PATCH 1/1 ARM: PMU: fix runtime PM enable

Quoted 11 lines · Jon Hunter

Quoted 6 lines · Will Deacon
Hmmm, now I start to wonder whether your original idea of having separate callbacks for enable/disable irq and resume/suspend doesn't make more sense. Then the CTI magic can go in the irq management code and be totally separate to the PM stuff.
What do you reckon?
The resume/suspend calls really replaced the enable/disable irq callbacks. That still seems like a good approach given that we need runtime PM for OMAP and PMU.

Ok, perhaps splitting it up isn't worth it then. I'm still not convinced either way.

Quoted 5 lines

Quoted 2 lines
Nah, we should be able to fix this in the platdata, I'd just rather have function pointers instead of state variables in there.
Well, we could pass a pointer to pm_runtime_enable() function in the platdata.

What do other drivers do? Grepping around, I see calls to pm_runtime_enable made in various drivers and, given that you pass the device in there, what's the problem with us just calling that unconditionally from perf? I know you said that will work for OMAP, but I'm trying to understand the effect that has on PM-aware platforms that don't require this for the PMU (since this seems to be per-device).

Will

Jon Hunter

author

2012-10-24 17:41 UTC #6 lore raw

PATCH 1/1 ARM: PMU: fix runtime PM enable

Quoted 7 lines · Will Deacon

Quoted 6 lines · Jon Hunter
Quoted 5 lines · Will Deacon
Hmmm, now I start to wonder whether your original idea of having separate callbacks for enable/disable irq and resume/suspend doesn't make more sense. Then the CTI magic can go in the irq management code and be totally separate to the PM stuff.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
What do you reckon?
The resume/suspend calls really replaced the enable/disable irq callbacks. That still seems like a good approach given that we need runtime PM for OMAP and PMU.
Ok, perhaps splitting it up isn't worth it then. I'm still not convinced either way.

Given that we needed to employ runtime PM for OMAP, adding the handlers is a natural progression and fits more with the PM framework model.

Quoted 6 lines

Quoted 5 lines
Quoted 2 lines
Nah, we should be able to fix this in the platdata, I'd just rather have function pointers instead of state variables in there.
Well, we could pass a pointer to pm_runtime_enable() function in the platdata.

Quoted 6 lines

What do other drivers do? Grepping around, I see calls to pm_runtime_enable made in various drivers and, given that you pass the device in there, what's the problem with us just calling that unconditionally from perf? I know you said that will work for OMAP, but I'm trying to understand the effect that has on PM-aware platforms that don't require this for the PMU (since this seems to be per-device).

I had done this initially when testing on OMAP platforms that do and don't require runtime PM for PMU. I don't see any side affect of this, however, may be Kevin could comment on if that is ok. It would be the best approach.

Cheers
Jon

Kevin Hilman

2012-10-25 16:41 UTC #7 lore raw

PATCH 1/1 ARM: PMU: fix runtime PM enable

Quoted 33 lines · Jon Hunter

Quoted 7 lines · Will Deacon
Quoted 6 lines · Jon Hunter
Quoted 5 lines · Will Deacon
Hmmm, now I start to wonder whether your original idea of having separate callbacks for enable/disable irq and resume/suspend doesn't make more sense. Then the CTI magic can go in the irq management code and be totally separate to the PM stuff.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
What do you reckon?
The resume/suspend calls really replaced the enable/disable irq callbacks. That still seems like a good approach given that we need runtime PM for OMAP and PMU.
Ok, perhaps splitting it up isn't worth it then. I'm still not convinced either way.
Given that we needed to employ runtime PM for OMAP, adding the handlers is a natural progression and fits more with the PM framework model.
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
Nah, we should be able to fix this in the platdata, I'd just rather have function pointers instead of state variables in there.
Well, we could pass a pointer to pm_runtime_enable() function in the platdata.
Quoted 6 lines
What do other drivers do? Grepping around, I see calls to pm_runtime_enable made in various drivers and, given that you pass the device in there, what's the problem with us just calling that unconditionally from perf? I know you said that will work for OMAP, but I'm trying to understand the effect that has on PM-aware platforms that don't require this for the PMU (since this seems to be per-device).

Quoted 4 lines

I had done this initially when testing on OMAP platforms that do and don't require runtime PM for PMU. I don't see any side affect of this, however, may be Kevin could comment on if that is ok. It would be the best approach.

Unconditonally enabling runtime PM should be fine. It may add a slight bit of overhead calling runtime PM functions that ultimately do nothing (because there are no callbacks), but it will be harmless.

Personally, I think that would be cleaner. The less pdata we need, the better, IMO.

Kevin

Will Deacon

2012-10-25 16:47 UTC #8 lore raw

PATCH 1/1 ARM: PMU: fix runtime PM enable

Quoted 14 lines · Kevin Hilman

Quoted 8 lines · Jon Hunter
Quoted 6 lines · Will Deacon
What do other drivers do? Grepping around, I see calls to pm_runtime_enable made in various drivers and, given that you pass the device in there, what's the problem with us just calling that unconditionally from perf? I know you said that will work for OMAP, but I'm trying to understand the effect that has on PM-aware platforms that don't require this for the PMU (since this seems to be per-device).
Quoted 4 lines
I had done this initially when testing on OMAP platforms that do and don't require runtime PM for PMU. I don't see any side affect of this, however, may be Kevin could comment on if that is ok. It would be the best approach.

Quoted 6 lines

Unconditonally enabling runtime PM should be fine. It may add a slight bit of overhead calling runtime PM functions that ultimately do nothing (because there are no callbacks), but it will be harmless.
Personally, I think that would be cleaner. The less pdata we need, the better, IMO.

Thanks Kevin, I'm fine with that. Jon: want me to write a patch or do you have something I can take into the ARM perf tree (if the latter, please base against perf/updates)?

Cheers,

Will

Jon Hunter

author

2012-10-25 16:50 UTC #9 lore raw

PATCH 1/1 ARM: PMU: fix runtime PM enable

Quoted 19 lines · Will Deacon

Quoted 18 lines · Kevin Hilman
Quoted 8 lines · Jon Hunter
Quoted 6 lines · Will Deacon
What do other drivers do? Grepping around, I see calls to pm_runtime_enable made in various drivers and, given that you pass the device in there, what's the problem with us just calling that unconditionally from perf? I know you said that will work for OMAP, but I'm trying to understand the effect that has on PM-aware platforms that don't require this for the PMU (since this seems to be per-device).
Quoted 4 lines
I had done this initially when testing on OMAP platforms that do and don't require runtime PM for PMU. I don't see any side affect of this, however, may be Kevin could comment on if that is ok. It would be the best approach.
Unconditonally enabling runtime PM should be fine. It may add a slight bit of overhead calling runtime PM functions that ultimately do nothing (because there are no callbacks), but it will be harmless.

Quoted 6 lines

Quoted 2 lines
Personally, I think that would be cleaner. The less pdata we need, the better, IMO.
Thanks Kevin, I'm fine with that. Jon: want me to write a patch or do you have something I can take into the ARM perf tree (if the latter, please base against perf/updates)?

I can easily spin this. Will base on top of your branch.

Cheers
Jon