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