| Message ID | 1351024268-26734-1-git-send-email-jon-hunter@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jon, On Tue, Oct 23, 2012 at 09:31:08PM +0100, Jon Hunter wrote: > 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(). [...] > diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h > index a26170d..50a6c3b 100644 > --- a/arch/arm/include/asm/pmu.h > +++ b/arch/arm/include/asm/pmu.h > @@ -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
Hi Will, On 10/24/2012 04:31 AM, Will Deacon wrote: > Hi Jon, > > On Tue, Oct 23, 2012 at 09:31:08PM +0100, Jon Hunter wrote: >> 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(). > > [...] > >> diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h >> index a26170d..50a6c3b 100644 >> --- a/arch/arm/include/asm/pmu.h >> +++ b/arch/arm/include/asm/pmu.h >> @@ -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
On Wed, Oct 24, 2012 at 03:16:43PM +0100, Jon Hunter wrote: > Hi Will, > > On 10/24/2012 04:31 AM, Will Deacon wrote: > > 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. 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? > 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. > 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
On 10/24/2012 09:32 AM, Will Deacon wrote: > On Wed, Oct 24, 2012 at 03:16:43PM +0100, Jon Hunter wrote: >> Hi Will, >> >> On 10/24/2012 04:31 AM, Will Deacon wrote: >>> 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. > > 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. >> 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. > >> 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. Well, we could pass a pointer to pm_runtime_enable() function in the platdata. Cheers Jon
On Wed, Oct 24, 2012 at 04:06:07PM +0100, Jon Hunter wrote: > On 10/24/2012 09:32 AM, Will Deacon wrote: > > 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. > > 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
On 10/24/2012 12:23 PM, Will Deacon wrote: > On Wed, Oct 24, 2012 at 04:06:07PM +0100, Jon Hunter wrote: >> On 10/24/2012 09:32 AM, Will Deacon wrote: >>> 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. 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. >>> 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). 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
Jon Hunter <jon-hunter@ti.com> writes: > On 10/24/2012 12:23 PM, Will Deacon wrote: >> On Wed, Oct 24, 2012 at 04:06:07PM +0100, Jon Hunter wrote: >>> On 10/24/2012 09:32 AM, Will Deacon wrote: >>>> 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. > > 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. > >>>> 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). > > 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
On Thu, Oct 25, 2012 at 05:42:21PM +0100, Kevin Hilman wrote: > Jon Hunter <jon-hunter@ti.com> writes: > > On 10/24/2012 12:23 PM, Will Deacon wrote: > >> 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. > > 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
On 10/25/2012 11:47 AM, Will Deacon wrote: > On Thu, Oct 25, 2012 at 05:42:21PM +0100, Kevin Hilman wrote: >> Jon Hunter <jon-hunter@ti.com> writes: >>> On 10/24/2012 12:23 PM, Will Deacon wrote: >>>> 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. >> >> 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)? I can easily spin this. Will base on top of your branch. Cheers Jon
diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h index a26170d..50a6c3b 100644 --- a/arch/arm/include/asm/pmu.h +++ b/arch/arm/include/asm/pmu.h @@ -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); }; diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c index 93971b1..a8c5ddf 100644 --- a/arch/arm/kernel/perf_event.c +++ b/arch/arm/kernel/perf_event.c @@ -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); diff --git a/arch/arm/mach-omap2/pmu.c b/arch/arm/mach-omap2/pmu.c index 2a79176..1a3d4d7 100644 --- a/arch/arm/mach-omap2/pmu.c +++ b/arch/arm/mach-omap2/pmu.c @@ -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);
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 <jon-hunter@ti.com> --- 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(-)