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