Jon Hunter <jon-hunter@xxxxxx> 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@xxxxxx> writes: >>> >>>> From: Jon Hunter <jon-hunter@xxxxxx> >>>> >>>> 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@xxxxxxxxxxxxx> >>>> Cc: Will Deacon <will.deacon@xxxxxxx> >>>> Cc: Benoit Cousson <b-cousson@xxxxxx> >>>> Cc: Paul Walmsley <paul@xxxxxxxxx> >>>> Cc: Kevin Hilman <khilman@xxxxxx> >>>> >>>> Signed-off-by: Jon Hunter <jon-hunter@xxxxxx> >>>> --- >>>> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
