| Message ID | 1315144466-9395-1-git-send-email-santosh.shilimkar@ti.com |
|---|---|
| State | New |
| Headers | show |
Hello. On 04-09-2011 17:54, Santosh Shilimkar wrote: > Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> > Cc: Kevin Hilman<khilman@ti.com> > --- > arch/arm/mach-omap2/omap-mpuss-lowpower.c | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c > index 4dd9d0f..0bc7610 100644 > --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c > +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c [...] > @@ -270,6 +272,7 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state) > scu_pwrst_prepare(cpu, power_state); > l2x0_pwrst_prepare(cpu, save_state); > > + Stray newline? > /* > * Call low level function with targeted CPU id > * and its low power state. WBR, Sergei
Hello. On 04-09-2011 17:54, Santosh Shilimkar wrote: > OMAP4 L2X0 initialisation code uses BUG_ON() for the ioremap() > failure scenarios. > Use WARN_ON() instead and allow graceful function exits. > This was suggsted by Kevin Hilman<khilman@ti.com> during > OMAP4 PM code review. > Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> > Cc: Kevin Hilman<khilman@ti.com> > --- > arch/arm/mach-omap2/omap4-common.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > diff --git a/arch/arm/mach-omap2/omap4-common.c b/arch/arm/mach-omap2/omap4-common.c > index 4791370..4904025 100644 > --- a/arch/arm/mach-omap2/omap4-common.c > +++ b/arch/arm/mach-omap2/omap4-common.c > @@ -121,7 +121,8 @@ static int __init omap_l2_cache_init(void) > > /* Static mapping, never released */ > l2cache_base = ioremap(OMAP44XX_L2CACHE_BASE, SZ_4K); > - BUG_ON(!l2cache_base); > + if (WARN_ON(!l2cache_base)) > + return -ENODEV; Rather ENOMEM... WBR, Sergei
On Monday 05 September 2011 03:41 PM, Sergei Shtylyov wrote: > Hello. > > On 04-09-2011 17:54, Santosh Shilimkar wrote: > >> OMAP4 L2X0 initialisation code uses BUG_ON() for the ioremap() >> failure scenarios. > >> Use WARN_ON() instead and allow graceful function exits. > >> This was suggsted by Kevin Hilman<khilman@ti.com> during >> OMAP4 PM code review. > >> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> Cc: Kevin Hilman<khilman@ti.com> >> --- >> arch/arm/mach-omap2/omap4-common.c | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) > >> diff --git a/arch/arm/mach-omap2/omap4-common.c >> b/arch/arm/mach-omap2/omap4-common.c >> index 4791370..4904025 100644 >> --- a/arch/arm/mach-omap2/omap4-common.c >> +++ b/arch/arm/mach-omap2/omap4-common.c >> @@ -121,7 +121,8 @@ static int __init omap_l2_cache_init(void) >> >> /* Static mapping, never released */ >> l2cache_base = ioremap(OMAP44XX_L2CACHE_BASE, SZ_4K); >> - BUG_ON(!l2cache_base); >> + if (WARN_ON(!l2cache_base)) >> + return -ENODEV; > > Rather ENOMEM... > Nope. Even though it's related to memory, it's a memory controller so DEV is right. Regards Santosh
On Monday 05 September 2011 03:38 PM, Sergei Shtylyov wrote: > Hello. > > On 04-09-2011 17:54, Santosh Shilimkar wrote: > >> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> Cc: Kevin Hilman<khilman@ti.com> >> --- >> arch/arm/mach-omap2/omap-mpuss-lowpower.c | 5 +++++ >> 1 files changed, 5 insertions(+), 0 deletions(-) > >> diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c >> b/arch/arm/mach-omap2/omap-mpuss-lowpower.c >> index 4dd9d0f..0bc7610 100644 >> --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c >> +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c > [...] >> @@ -270,6 +272,7 @@ int omap4_enter_lowpower(unsigned int cpu, >> unsigned int power_state) >> scu_pwrst_prepare(cpu, power_state); >> l2x0_pwrst_prepare(cpu, save_state); >> >> + > > Stray newline? > I noticed this later after scanning it on the list. Will remove it. Regards Sntosh
On Mon, Sep 05, 2011 at 04:12:02PM +0530, Santosh wrote: > On Monday 05 September 2011 03:41 PM, Sergei Shtylyov wrote: >> Hello. >> >> On 04-09-2011 17:54, Santosh Shilimkar wrote: >> >>> OMAP4 L2X0 initialisation code uses BUG_ON() for the ioremap() >>> failure scenarios. >> >>> Use WARN_ON() instead and allow graceful function exits. >> >>> This was suggsted by Kevin Hilman<khilman@ti.com> during >>> OMAP4 PM code review. >> >>> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >>> Cc: Kevin Hilman<khilman@ti.com> >>> --- >>> arch/arm/mach-omap2/omap4-common.c | 3 ++- >>> 1 files changed, 2 insertions(+), 1 deletions(-) >> >>> diff --git a/arch/arm/mach-omap2/omap4-common.c >>> b/arch/arm/mach-omap2/omap4-common.c >>> index 4791370..4904025 100644 >>> --- a/arch/arm/mach-omap2/omap4-common.c >>> +++ b/arch/arm/mach-omap2/omap4-common.c >>> @@ -121,7 +121,8 @@ static int __init omap_l2_cache_init(void) >>> >>> /* Static mapping, never released */ >>> l2cache_base = ioremap(OMAP44XX_L2CACHE_BASE, SZ_4K); >>> - BUG_ON(!l2cache_base); >>> + if (WARN_ON(!l2cache_base)) >>> + return -ENODEV; >> >> Rather ENOMEM... >> > Nope. Even though it's related to memory, it's > a memory controller so DEV is right. No it's not. The most likely reason ioremap() fails is because: 1. it's run out of memory for the metadata or page tables. 2. you passed it an invalid memory type (unlikely, you're using ioremap which uses a fixed known good type). 3. the address was >4GB but was not section aligned (also unlikely). Therefore, the error code should be "out of memory" not "no such device".
On Monday 05 September 2011 04:17 PM, Russell King - ARM Linux wrote: > On Mon, Sep 05, 2011 at 04:12:02PM +0530, Santosh wrote: >> On Monday 05 September 2011 03:41 PM, Sergei Shtylyov wrote: [..] >>>> /* Static mapping, never released */ >>>> l2cache_base = ioremap(OMAP44XX_L2CACHE_BASE, SZ_4K); >>>> - BUG_ON(!l2cache_base); >>>> + if (WARN_ON(!l2cache_base)) >>>> + return -ENODEV; >>> >>> Rather ENOMEM... >>> >> Nope. Even though it's related to memory, it's >> a memory controller so DEV is right. > > No it's not. The most likely reason ioremap() fails is because: > > 1. it's run out of memory for the metadata or page tables. > 2. you passed it an invalid memory type (unlikely, you're using ioremap > which uses a fixed known good type). > 3. the address was>4GB but was not section aligned (also unlikely). > > Therefore, the error code should be "out of memory" not "no such device". Got it. Will fix that.
Hi Santosh, just a question below. On Sun, Sep 04, 2011 at 02:54:21PM +0100, Santosh Shilimkar wrote: > When MPUSS hits off-mode e, L2 cache is lost. This patch adds L2X0 > necessary maintenance operations and context restoration in the > low power code. > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Kevin Hilman <khilman@ti.com> > --- > arch/arm/mach-omap2/include/mach/omap-secure.h | 5 + > arch/arm/mach-omap2/omap-mpuss-lowpower.c | 38 +++++++++- > arch/arm/mach-omap2/omap4-sar-layout.h | 4 + > arch/arm/mach-omap2/sleep44xx.S | 96 ++++++++++++++++++++++++ > 4 files changed, 142 insertions(+), 1 deletions(-) > [...] > diff --git a/arch/arm/mach-omap2/sleep44xx.S b/arch/arm/mach-omap2/sleep44xx.S > index 230ab8c..a7cce0b 100644 > --- a/arch/arm/mach-omap2/sleep44xx.S > +++ b/arch/arm/mach-omap2/sleep44xx.S > @@ -32,6 +32,9 @@ > ppa_zero_params: > .word 0x0 > > +ppa_por_params: > + .word 1, 0 > + > /* > * ============================= > * == CPU suspend finisher == > @@ -130,6 +133,55 @@ skip_scu_gp_set: > mcrne p15, 0, r0, c1, c0, 1 > isb > dsb > +#ifdef CONFIG_CACHE_L2X0 > + /* > + * Clean and invalidate the L2 cache. > + * Common cache-l2x0.c functions can't be used here since it > + * uses spinlocks. We are out of coherency here with data cache > + * disabled. The spinlock implementation uses exclusive load/store > + * instruction which can fail without data cache being enabled. > + * OMAP4 hardware doesn't support exclusive monitor which can > + * overcome exclusive access issue. Because of this, CPU can > + * lead to deadlock. > + */ > +l2x_clean_inv: > + bl omap4_get_sar_ram_base > + mov r8, r0 > + mrc p15, 0, r5, c0, c0, 5 @ Read MPIDR > + ands r5, r5, #0x0f > + ldreq r0, [r8, #L2X0_SAVE_OFFSET0] > + ldrne r0, [r8, #L2X0_SAVE_OFFSET1] > + cmp r0, #3 > + bne do_WFI > +#ifdef CONFIG_PL310_ERRATA_727915 > + mov r0, #0x03 > + mov r12, #OMAP4_MON_L2X0_DBG_CTRL_INDEX > + DO_SMC > +#endif > + bl omap4_get_l2cache_base > + mov r2, r0 > + ldr r0, =0xffff > + str r0, [r2, #L2X0_CLEAN_INV_WAY] > +wait: > + ldr r0, [r2, #L2X0_CLEAN_INV_WAY] > + ldr r1, =0xffff > + ands r0, r0, r1 > + bne wait > +#ifdef CONFIG_PL310_ERRATA_727915 > + mov r0, #0x00 > + mov r12, #OMAP4_MON_L2X0_DBG_CTRL_INDEX > + DO_SMC > +#endif > +l2x_sync: > + bl omap4_get_l2cache_base > + mov r2, r0 > + mov r0, #0x0 > + str r0, [r2, #L2X0_CACHE_SYNC] > +sync: > + ldr r0, [r2, #L2X0_CACHE_SYNC] > + ands r0, r0, #0x1 > + bne sync > +#endif > If I am not mistaken, here the PL310 is still on. Is it safe to go to wfi (PL310 logic state is lost then ?) with the controller enabled ? You still use the stack and I think prefetch is enabled, do not know if you can end up having AXI bus transactions ongoing whilst HW yanks the power. To disable it I think you need a secure call, again it is just a question for my information. Thanks, Lorenzo > do_WFI: > bl omap_do_wfi > @@ -222,6 +274,50 @@ enable_smp_bit: > mcreq p15, 0, r0, c1, c0, 1 > isb > skip_ns_smp_enable: > +#ifdef CONFIG_CACHE_L2X0 > + /* > + * Restore the L2 AUXCTRL and enable the L2 cache. > + * OMAP4_MON_L2X0_AUXCTRL_INDEX = Program the L2X0 AUXCTRL > + * OMAP4_MON_L2X0_CTRL_INDEX = Enable the L2 using L2X0 CTRL > + * register r0 contains value to be programmed. > + * L2 cache is already invalidate by ROM code as part > + * of MPUSS OFF wakeup path. > + */ > + ldr r2, =OMAP44XX_L2CACHE_BASE > + ldr r0, [r2, #L2X0_CTRL] > + and r0, #0x0f > + cmp r0, #1 > + beq skip_l2en @ Skip if already enabled > + ldr r3, =OMAP44XX_SAR_RAM_BASE > + ldr r1, [r3, #OMAP_TYPE_OFFSET] > + cmp r1, #0x1 @ Check for HS device > + bne set_gp_por > + ldr r0, =OMAP4_PPA_L2_POR_INDEX > + ldr r1, =OMAP44XX_SAR_RAM_BASE > + ldr r4, [r1, #L2X0_PREFETCH_CTRL_OFFSET] > + adr r3, ppa_por_params > + str r4, [r3, #0x04] > + mov r1, #0x0 @ Process ID > + mov r2, #0x4 @ Flag > + mov r6, #0xff > + mov r12, #0x00 @ Secure Service ID > + DO_SMC > + b set_aux_ctrl > +set_gp_por: > + ldr r1, =OMAP44XX_SAR_RAM_BASE > + ldr r0, [r1, #L2X0_PREFETCH_CTRL_OFFSET] > + ldr r12, =OMAP4_MON_L2X0_PREFETCH_INDEX @ Setup L2 PREFETCH > + DO_SMC > +set_aux_ctrl: > + ldr r1, =OMAP44XX_SAR_RAM_BASE > + ldr r0, [r1, #L2X0_AUXCTRL_OFFSET] > + ldr r12, =OMAP4_MON_L2X0_AUXCTRL_INDEX @ Setup L2 AUXCTRL > + DO_SMC > + mov r0, #0x1 > + ldr r12, =OMAP4_MON_L2X0_CTRL_INDEX @ Enable L2 cache > + DO_SMC > +skip_l2en: > +#endif > > b cpu_resume @ Jump to generic resume > ENDPROC(omap4_cpu_resume) > -- > 1.7.4.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Monday 05 September 2011 07:31 PM, Lorenzo Pieralisi wrote: > Hi Santosh, > > just a question below. > > On Sun, Sep 04, 2011 at 02:54:21PM +0100, Santosh Shilimkar wrote: >> When MPUSS hits off-mode e, L2 cache is lost. This patch adds L2X0 >> necessary maintenance operations and context restoration in the >> low power code. >> >> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> Cc: Kevin Hilman<khilman@ti.com> >> --- >> arch/arm/mach-omap2/include/mach/omap-secure.h | 5 + >> arch/arm/mach-omap2/omap-mpuss-lowpower.c | 38 +++++++++- >> arch/arm/mach-omap2/omap4-sar-layout.h | 4 + >> arch/arm/mach-omap2/sleep44xx.S | 96 ++++++++++++++++++++++++ >> 4 files changed, 142 insertions(+), 1 deletions(-) >> > > [...] > >> diff --git a/arch/arm/mach-omap2/sleep44xx.S b/arch/arm/mach-omap2/sleep44xx.S >> index 230ab8c..a7cce0b 100644 >> --- a/arch/arm/mach-omap2/sleep44xx.S >> +++ b/arch/arm/mach-omap2/sleep44xx.S >> @@ -32,6 +32,9 @@ >> ppa_zero_params: >> .word 0x0 >> >> +ppa_por_params: >> + .word 1, 0 >> + >> /* >> * ============================= >> * == CPU suspend finisher == >> @@ -130,6 +133,55 @@ skip_scu_gp_set: >> mcrne p15, 0, r0, c1, c0, 1 >> isb >> dsb >> +#ifdef CONFIG_CACHE_L2X0 >> + /* >> + * Clean and invalidate the L2 cache. >> + * Common cache-l2x0.c functions can't be used here since it >> + * uses spinlocks. We are out of coherency here with data cache >> + * disabled. The spinlock implementation uses exclusive load/store >> + * instruction which can fail without data cache being enabled. >> + * OMAP4 hardware doesn't support exclusive monitor which can >> + * overcome exclusive access issue. Because of this, CPU can >> + * lead to deadlock. >> + */ >> +l2x_clean_inv: >> + bl omap4_get_sar_ram_base >> + mov r8, r0 >> + mrc p15, 0, r5, c0, c0, 5 @ Read MPIDR >> + ands r5, r5, #0x0f >> + ldreq r0, [r8, #L2X0_SAVE_OFFSET0] >> + ldrne r0, [r8, #L2X0_SAVE_OFFSET1] >> + cmp r0, #3 >> + bne do_WFI >> +#ifdef CONFIG_PL310_ERRATA_727915 >> + mov r0, #0x03 >> + mov r12, #OMAP4_MON_L2X0_DBG_CTRL_INDEX >> + DO_SMC >> +#endif >> + bl omap4_get_l2cache_base >> + mov r2, r0 >> + ldr r0, =0xffff >> + str r0, [r2, #L2X0_CLEAN_INV_WAY] >> +wait: >> + ldr r0, [r2, #L2X0_CLEAN_INV_WAY] >> + ldr r1, =0xffff >> + ands r0, r0, r1 >> + bne wait >> +#ifdef CONFIG_PL310_ERRATA_727915 >> + mov r0, #0x00 >> + mov r12, #OMAP4_MON_L2X0_DBG_CTRL_INDEX >> + DO_SMC >> +#endif >> +l2x_sync: >> + bl omap4_get_l2cache_base >> + mov r2, r0 >> + mov r0, #0x0 >> + str r0, [r2, #L2X0_CACHE_SYNC] >> +sync: >> + ldr r0, [r2, #L2X0_CACHE_SYNC] >> + ands r0, r0, #0x1 >> + bne sync >> +#endif >> > > If I am not mistaken, here the PL310 is still on. Is it > safe to go to wfi (PL310 logic state is lost then ?) with the controller > enabled ? You still use the stack and I think prefetch is enabled, > do not know if you can end up having AXI bus transactions ongoing > whilst HW yanks the power. To disable it I think you need a secure call, > again it is just a question for my information. > PL310 is ON but C-bit is disabled. More over the Last sync will ensure that all out-standing transactions are completed. So it should be safe to go down. On OMAP, hardware waits for AXI transation complete before CPU transitioning to low power, so that's not an issue. For disabling, we need secure call. I didn't disable it because we haven't seen any issue so far and AXI ack is already in place. Regards Santosh
Santosh Shilimkar <santosh.shilimkar@ti.com> writes: > As per OMAP4430 TRM, the dynamic dependency between MPUSS -> EMIF > and MPUSS -> L3* and DUCATI -> L3 *clockdomains is enable by default. > Refer register CM_MPU_DYNAMICDEP description for details. > > But these dynamic dependencies doesn't work as expected. The hardware > recommendation is to keep above dependencies. Minor nit on changelog wording... The use of "above dependencies" here is a bit confusing, since the only thing refered to above are dynamic dependencies. I think this should be worked to be clear that what is being kept are the *static* dependencies (assuming I understand it correctly.) > Without this system locks up or randomly crashesh. Is 'crashesh' a particular kind of crash. maybe a silent one? ;) > Signed-off-by: Rajendra Nayak <rnayak@ti.com> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Acked-by: Paul Walmsley <paul@pwsan.com> > Cc: Kevin Hilman <khilman@ti.com> > --- > arch/arm/mach-omap2/pm44xx.c | 28 ++++++++++++++++++++++++++++ > 1 files changed, 28 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/mach-omap2/pm44xx.c b/arch/arm/mach-omap2/pm44xx.c > index 4bbc6fe..35ba028 100644 > --- a/arch/arm/mach-omap2/pm44xx.c > +++ b/arch/arm/mach-omap2/pm44xx.c > @@ -116,6 +116,8 @@ static void omap_default_idle(void) > static int __init omap4_pm_init(void) > { > int ret; > + struct clockdomain *emif_clkdm, *mpuss_clkdm, *l3_1_clkdm; > + struct clockdomain *ducati_clkdm, *l3_2_clkdm; > > if (!cpu_is_omap44xx()) > return -ENODEV; > @@ -128,6 +130,32 @@ static int __init omap4_pm_init(void) > goto err2; > } > > + /* > + * The dynamic dependency between MPUSS -> MEMIF and > + * MPUSS -> L3_* and DUCATI -> doesn't work as expected. > + * The hardware recommendation is to keep above dependencies. same here... "The hardware recommendation is to use static dependencies." > + * Without this system locks up or randomly crashesh. > + */ > + mpuss_clkdm = clkdm_lookup("mpuss_clkdm"); > + emif_clkdm = clkdm_lookup("l3_emif_clkdm"); > + l3_1_clkdm = clkdm_lookup("l3_1_clkdm"); > + l3_2_clkdm = clkdm_lookup("l3_2_clkdm"); > + ducati_clkdm = clkdm_lookup("ducati_clkdm"); > + if ((!mpuss_clkdm) || (!emif_clkdm) || (!l3_1_clkdm) || > + (!l3_2_clkdm) || (!ducati_clkdm)) > + goto err2; > + > + ret = clkdm_add_wkdep(mpuss_clkdm, emif_clkdm); > + ret |= clkdm_add_wkdep(mpuss_clkdm, l3_1_clkdm); > + ret |= clkdm_add_wkdep(mpuss_clkdm, l3_2_clkdm); > + ret |= clkdm_add_wkdep(ducati_clkdm, l3_1_clkdm); > + ret |= clkdm_add_wkdep(ducati_clkdm, l3_2_clkdm); > + if (ret) { > + pr_err("Failed to add MPUSS -> L3/EMIF, DUCATI -> L3 " > + "wakeup dependency\n"); > + goto err2; > + } > + > #ifdef CONFIG_SUSPEND > suspend_set_ops(&omap_pm_ops); > #endif /* CONFIG_SUSPEND */ Kevin
Santosh Shilimkar <santosh.shilimkar@ti.com> writes: > OMAP WakeupGen is the interrupt controller extension used along > with ARM GIC to wake the CPU out from low power states on > external interrupts. > > The WakeupGen unit is responsible for generating wakeup event > from the incoming interrupts and enable bits. It is implemented > in MPU always ON power domain. During normal operation, > WakeupGen delivers external interrupts directly to the GIC. > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Kevin Hilman <khilman@ti.com> [...] > +#ifdef CONFIG_PM > +/* > + * Masking wakeup irqs is handled by the IRQCHIP_MASK_ON_SUSPEND flag, > + * so no action is necessary in set_wake, but implement an empty handler > + * here to prevent enable_irq_wake() returning an error. > + * FIXME: Remove the dummy handler once gen irq code fix above. > + */ Just curious... is there a fix for this pending for v3.2? Kevin
Hi Santosh, On Mon, Sep 5, 2011 at 12:51 PM, Santosh <santosh.shilimkar@ti.com> wrote: > On Monday 05 September 2011 04:17 PM, Russell King - ARM Linux wrote: >> >> On Mon, Sep 05, 2011 at 04:12:02PM +0530, Santosh wrote: >>> >>> On Monday 05 September 2011 03:41 PM, Sergei Shtylyov wrote: > > [..] > >>>>> /* Static mapping, never released */ >>>>> l2cache_base = ioremap(OMAP44XX_L2CACHE_BASE, SZ_4K); >>>>> - BUG_ON(!l2cache_base); >>>>> + if (WARN_ON(!l2cache_base)) >>>>> + return -ENODEV; >>>> >>>> Rather ENOMEM... >>>> >>> Nope. Even though it's related to memory, it's >>> a memory controller so DEV is right. >> >> No it's not. The most likely reason ioremap() fails is because: >> >> 1. it's run out of memory for the metadata or page tables. >> 2. you passed it an invalid memory type (unlikely, you're using ioremap >> which uses a fixed known good type). >> 3. the address was>4GB but was not section aligned (also unlikely). >> >> Therefore, the error code should be "out of memory" not "no such device". > > Got it. Will fix that. Patch [07/25] has the same code and so will need fixing too. Regards, Jean > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Santosh, On Sun, Sep 4, 2011 at 3:54 PM, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > On OMAP secure/emulation devices, certain APIs are exported by secure > code. Add an infrastructure so that relevant operations on secure > devices can be implemented using it. > > While at this, omap44xx-smc.S to omap-smc.S since the common APIs > can be used on other OMAP's too. 'move' or 'rename' is missing in the sentence. Regards, Jean
On Sun, Sep 4, 2011 at 3:54 PM, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > OMAP WakeupGen is the interrupt controller extension used along > with ARM GIC to wake the CPU out from low power states on > external interrupts. > > The WakeupGen unit is responsible for generating wakeup event ... for generating the wakeup events > from the incoming interrupts and enable bits. It is implemented > in MPU always ON power domain. During normal operation, in the MPU... > WakeupGen delivers external interrupts directly to the GIC. ... delivers the external interrupts > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Kevin Hilman <khilman@ti.com> > --- > arch/arm/mach-omap2/Makefile | 2 +- > arch/arm/mach-omap2/include/mach/omap-wakeupgen.h | 39 ++++ > arch/arm/mach-omap2/omap-wakeupgen.c | 241 +++++++++++++++++++++ > arch/arm/mach-omap2/omap4-common.c | 3 + > 4 files changed, 284 insertions(+), 1 deletions(-) > create mode 100644 arch/arm/mach-omap2/include/mach/omap-wakeupgen.h > create mode 100644 arch/arm/mach-omap2/omap-wakeupgen.c > ... > + */ > +int __init omap_wakeupgen_init(void) > +{ > + int i; > + unsigned int boot_cpu = smp_processor_id(); > + > + /* Not supported on OMAP4 ES1.0 silicon */ > + if (omap_rev() == OMAP4430_REV_ES1_0) { > + WARN(1, "WakeupGen: Not supported on OMAP4430 ES1.0\n"); > + return -EPERM; > + } > + > + /* Static mapping, never released */ > + wakeupgen_base = ioremap(OMAP44XX_WKUPGEN_BASE, SZ_4K); > + if (WARN_ON(!wakeupgen_base)) > + return -ENODEV; -ENOMEM os required as discussed in the other patches of the series. Regards, Jean
On Sun, Sep 4, 2011 at 3:54 PM, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > Allocate the memory to save secure ram context which needs > to be done when MPU is hitting off mode. > > The ROM code expects a physical address to this memory > and hence use memblock APIs to reserve this memory as part > of .reserve() callback. > > Maximum possible size is allocated and can cater to OMAP3XXX / OMAP4XXX > secure RAM size requirements. > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > --- > arch/arm/mach-omap2/include/mach/omap-secure.h | 4 +++ > arch/arm/mach-omap2/omap-secure.c | 29 ++++++++++++++++++++++++ > arch/arm/plat-omap/common.c | 3 ++ > 3 files changed, 36 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/mach-omap2/include/mach/omap-secure.h b/arch/arm/mach-omap2/include/mach/omap-secure.h > index 26e7bcc..e2f95a0 100644 > --- a/arch/arm/mach-omap2/include/mach/omap-secure.h > +++ b/arch/arm/mach-omap2/include/mach/omap-secure.h > @@ -26,6 +26,8 @@ > #define FLAG_FIQ_ENABLE 0x1 > #define NO_FLAG 0x0 > > +/* Maximum Secure memory storage size */ > +#define OMAP_SECURE_RAM_STORAGE (88 * SZ_1K) Is this valid for all supported devices? How to differentiate variations in the size for new chips variants in the future? Regards, Jean
On Sun, Sep 4, 2011 at 3:54 PM, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > This patch adds the CPU0 and CPU1 off mode support. CPUX close switch > retention (CSWR) is not supported by hardware design. > > The CPUx OFF mode isn't supported on OMAP4430 ES1.0 > > CPUx sleep code is common for hotplug, suspend and CPUilde. > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Kevin Hilman <khilman@ti.com> > --- > arch/arm/mach-omap2/Makefile | 3 +- > arch/arm/mach-omap2/include/mach/omap-secure.h | 8 + > arch/arm/mach-omap2/include/mach/omap4-common.h | 25 +++ > arch/arm/mach-omap2/omap-mpuss-lowpower.c | 249 +++++++++++++++++++++++ > arch/arm/mach-omap2/omap-smp.c | 6 + > arch/arm/mach-omap2/omap4-sar-layout.h | 9 + > arch/arm/mach-omap2/pm44xx.c | 6 + > arch/arm/mach-omap2/sleep44xx.S | 213 +++++++++++++++++++ > 8 files changed, 518 insertions(+), 1 deletions(-) > create mode 100644 arch/arm/mach-omap2/omap-mpuss-lowpower.c > > diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile > index b032c21..b4f2eeb 100644 > --- a/arch/arm/mach-omap2/Makefile > +++ b/arch/arm/mach-omap2/Makefile > @@ -63,7 +63,8 @@ obj-$(CONFIG_ARCH_OMAP2) += pm24xx.o > obj-$(CONFIG_ARCH_OMAP2) += sleep24xx.o > obj-$(CONFIG_ARCH_OMAP3) += pm34xx.o sleep34xx.o \ > cpuidle34xx.o > -obj-$(CONFIG_ARCH_OMAP4) += pm44xx.o sleep44xx.o > +obj-$(CONFIG_ARCH_OMAP4) += pm44xx.o sleep44xx.o \ > + omap-mpuss-lowpower.o > obj-$(CONFIG_PM_DEBUG) += pm-debug.o > obj-$(CONFIG_OMAP_SMARTREFLEX) += sr_device.o smartreflex.o > obj-$(CONFIG_OMAP_SMARTREFLEX_CLASS3) += smartreflex-class3.o > diff --git a/arch/arm/mach-omap2/include/mach/omap-secure.h b/arch/arm/mach-omap2/include/mach/omap-secure.h > index e2f95a0..0062d49 100644 > --- a/arch/arm/mach-omap2/include/mach/omap-secure.h > +++ b/arch/arm/mach-omap2/include/mach/omap-secure.h > @@ -35,10 +35,18 @@ > #define OMAP4_HAL_SAVEALL_INDEX 0x1c > #define OMAP4_HAL_SAVEGIC_INDEX 0x1d > > +/* Secure Monitor mode APIs */ > +#define OMAP4_MON_SCU_PWR_INDEX 0x108 > + > +/* Secure PPA(Primary Protected Application) APIs */ > +#define OMAP4_PPA_CPU_ACTRL_SMP_INDEX 0x25 > + > +#ifndef __ASSEMBLER__ > extern u32 omap_secure_dispatcher(u32 idx, u32 flag, u32 nargs, > u32 arg1, u32 arg2, u32 arg3, u32 arg4); > extern u32 omap_smc2(u32 id, u32 falg, u32 pargs); > extern phys_addr_t omap_secure_ram_mempool_base(void); > extern int omap_secure_ram_reserve_memblock(void); > > +#endif /* __ASSEMBLER__ */ > #endif /* OMAP_ARCH_OMAP_SECURE_H */ > diff --git a/arch/arm/mach-omap2/include/mach/omap4-common.h b/arch/arm/mach-omap2/include/mach/omap4-common.h > index 040dcf6..662f37c 100644 > --- a/arch/arm/mach-omap2/include/mach/omap4-common.h > +++ b/arch/arm/mach-omap2/include/mach/omap4-common.h > @@ -45,5 +45,30 @@ extern u32 omap_modify_auxcoreboot0(u32 set_mask, u32 clear_mask); > extern void omap_auxcoreboot_addr(u32 cpu_addr); > extern u32 omap_read_auxcoreboot0(void); > #endif > + > +#if defined(CONFIG_SMP) && defined(CONFIG_PM) > +extern int omap4_mpuss_init(void); > +extern int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state); > +extern int omap4_finish_suspend(unsigned long cpu_state); > +extern void omap4_cpu_resume(void); > +#else > +static inline int omap4_enter_lowpower(unsigned int cpu, > + unsigned int power_state) > +{ > + omap_do_wfi(); > + return 0; > +} > + > +static inline int omap4_mpuss_init(void) > +{ > + return 0; > +} > + > +static inline int omap4_finish_suspend(unsigned long cpu_state) > +{} The int function should return something (0?) Has the code been test compiled without SMP and/or PM? > +static inline void omap4_cpu_resume(void) > +{} > +#endif > + > #endif /* __ASSEMBLER__ */ > #endif /* OMAP_ARCH_OMAP4_COMMON_H */ > diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c ... > +/** > + * omap4_enter_lowpower: OMAP4 MPUSS Low Power Entry Function > + * The purpose of this function is to manage low power programming > + * of OMAP4 MPUSS subsystem > + * @cpu : CPU ID > + * @power_state: Low power state. > + */ > +int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state) > +{ > + unsigned int save_state = 0; > + unsigned int wakeup_cpu; > + > + if (omap_rev() == OMAP4430_REV_ES1_0) > + return -ENXIO; > + > + switch (power_state) { > + case PWRDM_POWER_ON: > + case PWRDM_POWER_INACTIVE: > + save_state = 0; > + break; > + case PWRDM_POWER_OFF: > + save_state = 1; > + break; > + case PWRDM_POWER_RET: > + default: > + /* > + * CPUx CSWR is invalid hardware state. Also CPUx OSWR > + * doesn't make much scense, since logic is lost and $L1 > + * needs to be cleaned because of coherency. This makes > + * CPUx OSWR equivalent to CPUX OFF and hence not supported > + */ > + WARN_ON(1); > + return -ENXIO; > + } > + > + clear_cpu_prev_pwrst(cpu); > + set_cpu_next_pwrst(cpu, power_state); > + set_cpu_wakeup_addr(cpu, virt_to_phys(omap4_cpu_resume)); > + scu_pwrst_prepare(cpu, power_state); > + > + /* > + * Call low level function with targeted CPU id > + * and its low power state. > + */ > + cpu_suspend(save_state, omap4_finish_suspend); There is no CPU id parameter to the call although the above comment says so. ... > diff --git a/arch/arm/mach-omap2/sleep44xx.S b/arch/arm/mach-omap2/sleep44xx.S ... > +/* > + * ============================= > + * == CPU suspend finisher == > + * ============================= > + * > + * void omap4_finish_suspend(unsigned long cpu_state) > + * > + * This function code saves the CPU context and performs the CPU > + * power down sequence. Calling WFI effectively changes the CPU > + * power domains states to the desired target power state. > + * > + * @cpu_state : contains context save state (r0) > + * 0 - No context lost > + * 1 - CPUx L1 and logic lost: MPUSS CSWR > + * 2 - CPUx L1 and logic lost + GIC lost: MPUSS OSWR > + * 3 - CPUx L1 and logic lost + GIC + L2 lost: MPUSS OFF > + * @return: This function never returns for CPU OFF and DORMANT power states. > + * Post WFI, CPU transitions to DORMANT or OFF power state and on wake-up > + * from this follows a full CPU reset path via ROM code to CPU restore code. Is the ROM code jumping to the physical address of omap4_cpu_resume? If so please add it in the comment. ... > +/* > + * ============================ > + * == CPU resume entry point == > + * ============================ > + * > + * void omap4_cpu_resume(void) > + * > + * ROM code jumps to this function while waking up from CPU > + * OFF or DORMANT state. Physical address of the function is > + * stored in the SAR RAM while entering to OFF or DORMANT mode. Which code stores the address in the SAR RAM? Please describe it here. > + */ > +ENTRY(omap4_cpu_resume) > + /* ... Regards, Jean
Hi Santosh, On Sun, Sep 4, 2011 at 3:54 PM, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > This series adds OMAP4 MPUSS (MPU SubSystem) power management support for > suspend (S2R), CPU hotplug and CPUidle. > > Most of these patches have been posted and reviewed earlier [1] on the list > and have missed last couple of merge windows because of dependencies. > New set of patches have diverged more and hence the series version > continuity isn't maintained. > > Below are the main updates from previous versions. > - Use of generic ARM suspend hooks instead of OMAP custom code. > - Making use of common GIC code instead of OMAP custom code. > - Use of generic CPU PM notifiers for CPUIDLE and suspend. > - Use of CPU PM notifiers and hotplug notifiers for GIC extension. > - PM support of OMAP4 HS devices. > - Introduction of interconnect barriers as per the OMAP4 requirements. > > Special thanks to, > - Kevin Hilman for the detailed reviews. > - Russell for adding the L2 cache handling support to generic suspend. > - Colin Cross for the generic CPU PM notifier patches. > - Rajendra Nayak and Paul Walmsley for clock-domain sequencing series. > > Below series has dependency on Russell's L2 generic suspend support [2] > and earlier posted CPU PM notifiers series [3]. > An integrated branch with these dependencies can be found here [4]. > > The series is tested on OMAP4430 SDP for suspend, hotplug and CPUidle > with OMAP4 GP and HS (secure) devices. > > The following changes since commit c6a389f123b9f68d605bb7e0f9b32ec1e3e14132: > > Linux 3.1-rc4 (2011-08-28 21:16:01 -0700) > > are available in the git repository at: > git://gitorious.org/omap-sw-develoment/linux-omap-dev.git v3.1-rc4-omap4-mpuss-pm Some comments have been sent on this patch set, otherwise OK after review. FWIW: Acked-by: Jean Pihet <j-pihet@ti.com> Regards, Jean > > Santosh Shilimkar (25): > ARM: mm: Add strongly ordered descriptor support. > OMAP4: Redefine mandatory barriers for OMAP to include interconnect barriers. > OMAP4: PM: Use custom omap_do_wfi() for suspend and default idle. > OMAP4: Remove un-used do_wfi() macro. > OMAP4: Use WARN_ON() instead of BUG_ON() with graceful exit > OMAP4: Export omap4_get_base*() rather than global address pointers > OMAP4: PM: Add SAR RAM support > OMAP4: PM: Keep static dep between MPUSS-EMIF and MPUSS-L3 and DUCATI-L3 > OMAP4: PM: Avoid omap4_pm_init() on OMAP4430 ES1.0 > OMAP4: PM: Initialise all the clockdomains to supported states > OMAP: Add Secure HAL and monitor mode API infrastructure. > OMAP: Add support to allocate the memory for secure RAM > OMAP4: PM: Add WakeupGen module as OMAP gic_arch_extn > OMAP4: PM: Add CPUX OFF mode support > OMAP4: Remove __INIT from omap_secondary_startup() to re-use it for hotplug. > OMAP4: PM: Program CPU1 to hit OFF when off-lined > OMAP4: PM: CPU1 wakeup workaround from Low power modes > OMAP4: suspend: Add MPUSS power domain RETENTION support > OMAP4: PM: Add WakeupGen and secure GIC low power support > OMAP4: PM: Add L2X0 cache lowpower support > OMAP4: PM: Add MPUSS power domain OSWR support > OMAP4: PM: Add power domain statistics support > OMAP4: PM: Add CPUidle support > OMAP4: cpuidle: Switch to gptimer from twd in deeper C-states. > OMAP3: CPUidle: Make use of CPU PM notifiers > > arch/arm/include/asm/mach/map.h | 1 + > arch/arm/include/asm/pgtable.h | 3 + > arch/arm/mach-omap2/Kconfig | 1 + > arch/arm/mach-omap2/Makefile | 15 +- > arch/arm/mach-omap2/cpuidle34xx.c | 7 + > arch/arm/mach-omap2/cpuidle44xx.c | 206 ++++++++++ > arch/arm/mach-omap2/include/mach/barriers.h | 48 +++ > arch/arm/mach-omap2/include/mach/omap-secure.h | 57 +++ > arch/arm/mach-omap2/include/mach/omap-wakeupgen.h | 39 ++ > arch/arm/mach-omap2/include/mach/omap4-common.h | 70 +++- > arch/arm/mach-omap2/omap-headsmp.S | 5 - > arch/arm/mach-omap2/omap-hotplug.c | 14 +- > arch/arm/mach-omap2/omap-mpuss-lowpower.c | 398 +++++++++++++++++++ > arch/arm/mach-omap2/omap-secure.c | 81 ++++ > arch/arm/mach-omap2/{omap44xx-smc.S => omap-smc.S} | 23 ++ > arch/arm/mach-omap2/omap-smp.c | 38 ++ > arch/arm/mach-omap2/omap-wakeupgen.c | 403 ++++++++++++++++++++ > arch/arm/mach-omap2/omap4-common.c | 93 +++++- > arch/arm/mach-omap2/omap4-sar-layout.h | 50 +++ > arch/arm/mach-omap2/pm.h | 1 + > arch/arm/mach-omap2/pm44xx.c | 155 ++++++++- > arch/arm/mach-omap2/sleep44xx.S | 385 +++++++++++++++++++ > arch/arm/mm/mmu.c | 8 + > arch/arm/plat-omap/common.c | 3 + > arch/arm/plat-omap/include/plat/omap44xx.h | 1 + > arch/arm/plat-omap/include/plat/sram.h | 1 + > arch/arm/plat-omap/sram.c | 47 ++- > 27 files changed, 2104 insertions(+), 49 deletions(-) > create mode 100644 arch/arm/mach-omap2/cpuidle44xx.c > create mode 100644 arch/arm/mach-omap2/include/mach/barriers.h > create mode 100644 arch/arm/mach-omap2/include/mach/omap-secure.h > create mode 100644 arch/arm/mach-omap2/include/mach/omap-wakeupgen.h > create mode 100644 arch/arm/mach-omap2/omap-mpuss-lowpower.c > create mode 100644 arch/arm/mach-omap2/omap-secure.c > rename arch/arm/mach-omap2/{omap44xx-smc.S => omap-smc.S} (70%) > create mode 100644 arch/arm/mach-omap2/omap-wakeupgen.c > create mode 100644 arch/arm/mach-omap2/omap4-sar-layout.h > create mode 100644 arch/arm/mach-omap2/sleep44xx.S > > Regards > Santosh > > [1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg47511.html > > [2] http://www.spinics.net/lists/arm-kernel/msg138803.html > > [3] https://lkml.org/lkml/2011/9/3/49 > > [4] https://gitorious.org/omap-sw-develoment/linux-omap-dev/commits/v3.1-rc4-omap4-pm-integrated > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Thursday 08 September 2011 11:27 PM, Kevin Hilman wrote: > Hi Santosh, > > Santosh Shilimkar<santosh.shilimkar@ti.com> writes: > >> These notifiers can be used to isolate non-cpu specific PM >> code from CPU idle code. >> >> To start with, we can already leverage VFP handlers using notifier >> chain for low power idle code. So use them. >> >> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> Cc: Kevin Hilman<khilman@ti.com> > > A comment below related to our other discussion in the CPU PM notifiers > thread... > After the discussion, this patch needs to be updated. >> --- >> arch/arm/mach-omap2/cpuidle34xx.c | 7 +++++++ >> 1 files changed, 7 insertions(+), 0 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/cpuidle34xx.c b/arch/arm/mach-omap2/cpuidle34xx.c >> index 4bf6e6e..1112519 100644 >> --- a/arch/arm/mach-omap2/cpuidle34xx.c >> +++ b/arch/arm/mach-omap2/cpuidle34xx.c >> @@ -24,6 +24,7 @@ >> >> #include<linux/sched.h> >> #include<linux/cpuidle.h> >> +#include<linux/cpu_pm.h> >> >> #include<plat/prcm.h> >> #include<plat/irqs.h> >> @@ -118,9 +119,15 @@ static int omap3_enter_idle(struct cpuidle_device *dev, >> pwrdm_for_each_clkdm(core_pd, _cpuidle_deny_idle); >> } >> >> + if (mpu_state == PWRDM_POWER_OFF) >> + cpu_pm_enter(); > > Making all CPU PM notifiers conditional on the MPU power state is not > right since upon an MPU transition, other domains might also transition. > > What if there are notifiers for other drivers in other power domains > whose states might be transitioning? Notifiers for those devices should > be called no matter what the MPU's target state is. > > Also triggering the notifiers here has the same problem that I mentioned > with the syscore PM ops. Namely, that next power states are not yet > determined, so any device notifier decisions based on that will be wrong. > > The way I envision this working is (at least on OMAP) is CPU PM > notifiers are triggered only *after* the next power states are > determined and programmed. This allows the device-specific notifiers to > check their target power state and take appropriate action. > > I've been testing the CPU PM notifiers with the patch below (after also > dropping [PATCH 2/5] from your the CPU PM notifiers series.) This patch works > for both suspend and idle by using the code common to them, and > triggering the event only after the target states are programmed. > > I'm testing this patch now along with the 2nd patch below which removes > all the UART calls from the idle/suspend path and just adds a notifier > in the UART driver. > > Will post these officially after a bit more testing, but so far it's > working on OMAP2420/n810 and OMAP3430/n900. > Agree. I will drop my patch in favour of below patch. Regards Santosh
On Thursday 08 September 2011 11:36 PM, Kevin Hilman wrote: > Santosh Shilimkar<santosh.shilimkar@ti.com> writes: > >> As per OMAP4430 TRM, the dynamic dependency between MPUSS -> EMIF >> and MPUSS -> L3* and DUCATI -> L3 *clockdomains is enable by default. >> Refer register CM_MPU_DYNAMICDEP description for details. >> >> But these dynamic dependencies doesn't work as expected. The hardware >> recommendation is to keep above dependencies. > > Minor nit on changelog wording... > > The use of "above dependencies" here is a bit confusing, since the only > thing refered to above are dynamic dependencies. > > I think this should be worked to be clear that what is being kept are > the *static* dependencies (assuming I understand it correctly.) > Will mention about static deps. > >> Without this system locks up or randomly crashesh. > > Is 'crashesh' a particular kind of crash. maybe a silent one? ;) > My typo made it special :) Will fix it. Regards Santosh
On Friday 09 September 2011 12:28 AM, Jean Pihet wrote: > Santosh, > > On Sun, Sep 4, 2011 at 3:54 PM, Santosh Shilimkar > <santosh.shilimkar@ti.com> wrote: >> On OMAP secure/emulation devices, certain APIs are exported by secure >> code. Add an infrastructure so that relevant operations on secure >> devices can be implemented using it. >> >> While at this, omap44xx-smc.S to omap-smc.S since the common APIs >> can be used on other OMAP's too. > 'move' or 'rename' is missing in the sentence. > Yep. Will fix it. Regards Santosh
On Friday 09 September 2011 12:46 AM, Jean Pihet wrote: > On Sun, Sep 4, 2011 at 3:54 PM, Santosh Shilimkar > <santosh.shilimkar@ti.com> wrote: >> OMAP WakeupGen is the interrupt controller extension used along >> with ARM GIC to wake the CPU out from low power states on >> external interrupts. >> >> The WakeupGen unit is responsible for generating wakeup event > ... for generating the wakeup events >> from the incoming interrupts and enable bits. It is implemented >> in MPU always ON power domain. During normal operation, > in the MPU... >> WakeupGen delivers external interrupts directly to the GIC. > ... delivers the external interrupts > OK. >> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> Cc: Kevin Hilman<khilman@ti.com> >> --- >> arch/arm/mach-omap2/Makefile | 2 +- >> arch/arm/mach-omap2/include/mach/omap-wakeupgen.h | 39 ++++ >> arch/arm/mach-omap2/omap-wakeupgen.c | 241 +++++++++++++++++++++ >> arch/arm/mach-omap2/omap4-common.c | 3 + >> 4 files changed, 284 insertions(+), 1 deletions(-) >> create mode 100644 arch/arm/mach-omap2/include/mach/omap-wakeupgen.h >> create mode 100644 arch/arm/mach-omap2/omap-wakeupgen.c >> > ... > >> + */ >> +int __init omap_wakeupgen_init(void) >> +{ >> + int i; >> + unsigned int boot_cpu = smp_processor_id(); >> + >> + /* Not supported on OMAP4 ES1.0 silicon */ >> + if (omap_rev() == OMAP4430_REV_ES1_0) { >> + WARN(1, "WakeupGen: Not supported on OMAP4430 ES1.0\n"); >> + return -EPERM; >> + } >> + >> + /* Static mapping, never released */ >> + wakeupgen_base = ioremap(OMAP44XX_WKUPGEN_BASE, SZ_4K); >> + if (WARN_ON(!wakeupgen_base)) >> + return -ENODEV; > -ENOMEM os required as discussed in the other patches of the series. > Yep.
On Friday 09 September 2011 01:45 AM, Jean Pihet wrote: > Hi Santosh, > > On Sun, Sep 4, 2011 at 3:54 PM, Santosh Shilimkar > <santosh.shilimkar@ti.com> wrote: >> This series adds OMAP4 MPUSS (MPU SubSystem) power management support for >> suspend (S2R), CPU hotplug and CPUidle. >> >> Most of these patches have been posted and reviewed earlier [1] on the list >> and have missed last couple of merge windows because of dependencies. >> New set of patches have diverged more and hence the series version >> continuity isn't maintained. >> >> Below are the main updates from previous versions. >> - Use of generic ARM suspend hooks instead of OMAP custom code. >> - Making use of common GIC code instead of OMAP custom code. >> - Use of generic CPU PM notifiers for CPUIDLE and suspend. >> - Use of CPU PM notifiers and hotplug notifiers for GIC extension. >> - PM support of OMAP4 HS devices. >> - Introduction of interconnect barriers as per the OMAP4 requirements. >> >> Special thanks to, >> - Kevin Hilman for the detailed reviews. >> - Russell for adding the L2 cache handling support to generic suspend. >> - Colin Cross for the generic CPU PM notifier patches. >> - Rajendra Nayak and Paul Walmsley for clock-domain sequencing series. >> >> Below series has dependency on Russell's L2 generic suspend support [2] >> and earlier posted CPU PM notifiers series [3]. >> An integrated branch with these dependencies can be found here [4]. >> >> The series is tested on OMAP4430 SDP for suspend, hotplug and CPUidle >> with OMAP4 GP and HS (secure) devices. >> >> The following changes since commit c6a389f123b9f68d605bb7e0f9b32ec1e3e14132: >> >> Linux 3.1-rc4 (2011-08-28 21:16:01 -0700) >> >> are available in the git repository at: >> git://gitorious.org/omap-sw-develoment/linux-omap-dev.git v3.1-rc4-omap4-mpuss-pm > > Some comments have been sent on this patch set, otherwise OK after review. > > FWIW: > Acked-by: Jean Pihet<j-pihet@ti.com> > Thanks Regards Santosh
On Thursday 08 September 2011 11:57 PM, Kevin Hilman wrote: > Santosh Shilimkar<santosh.shilimkar@ti.com> writes: > >> OMAP WakeupGen is the interrupt controller extension used along >> with ARM GIC to wake the CPU out from low power states on >> external interrupts. >> >> The WakeupGen unit is responsible for generating wakeup event >> from the incoming interrupts and enable bits. It is implemented >> in MPU always ON power domain. During normal operation, >> WakeupGen delivers external interrupts directly to the GIC. >> >> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> Cc: Kevin Hilman<khilman@ti.com> > > [...] > >> +#ifdef CONFIG_PM >> +/* >> + * Masking wakeup irqs is handled by the IRQCHIP_MASK_ON_SUSPEND flag, >> + * so no action is necessary in set_wake, but implement an empty handler >> + * here to prevent enable_irq_wake() returning an error. >> + * FIXME: Remove the dummy handler once gen irq code fix above. >> + */ > > Just curious... is there a fix for this pending for v3.2? > I have sent a proposed change to Thomas but have not seen any response yet from him. http://www.spinics.net/lists/arm-kernel/msg134297.html Regards Santosh
On Thursday 08 September 2011 11:27 PM, Kevin Hilman wrote: > From 8ad40f8c7f950105b25cc8d2cc35caa50871f86f Mon Sep 17 00:00:00 2001 > From: Kevin Hilman<khilman@ti.com> > Date: Wed, 7 Sep 2011 12:04:44 -0700 > Subject: [PATCH 1/2] OMAP2/3: PM: trigger CPU PM notifiers in idle and > suspend path > > Add calls to CPU PM notifier core in the idle and suspend paths > so any CPU PM notifiers are properly called. > > [add more description about the need to trigger notifiers only after > next power states are programmed.] > > Signed-off-by: Kevin Hilman<khilman@ti.com> > --- > arch/arm/mach-omap2/pm24xx.c | 5 +++++ > arch/arm/mach-omap2/pm34xx.c | 5 +++++ > 2 files changed, 10 insertions(+), 0 deletions(-) > [...] > diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c > index 7255d9b..b167c7f 100644 > --- a/arch/arm/mach-omap2/pm34xx.c > +++ b/arch/arm/mach-omap2/pm34xx.c > @@ -29,6 +29,7 @@ > #include<linux/delay.h> > #include<linux/slab.h> > #include<linux/console.h> > +#include<linux/cpu_pm.h> > #include<trace/events/power.h> > > #include<asm/suspend.h> > @@ -386,6 +387,8 @@ void omap_sram_idle(void) > if (!console_trylock()) > goto console_still_active; > > + cpu_pm_enter(); > + I don't think you should blindly call CPU_PM notifiers. This should called if the MPU targeted state is OFF. With blind notifiers, CPU context like VFP or GIC or L2 will be saved and restored even if CPU/CPU cluster is not entering into low power. This is highly un-optimal for CPUIDLE and defeats the purpose of adding different CPU PM events. Looking at your UART driver notfier usage, you might need add new events instead of using the CPU and CPU cluster events. Even though on OMAP UART is idles as part of CPU, it's still an independent PD. E.g just to make a point, MPU is programmed to INACTIVE and let say PER PD is programmed to OFF. From MPU point of view the CPU_PM notifier change need not be called because, it's doesn't loose any context, but UART notifier needs to be executed since UART context must be saved. Regards Santosh
Hi Santosh, On Sun, Sep 04, 2011 at 07:24:15PM +0530, Santosh Shilimkar wrote: > This patch adds the CPU0 and CPU1 off mode support. CPUX close switch > retention (CSWR) is not supported by hardware design. > > The CPUx OFF mode isn't supported on OMAP4430 ES1.0 > > CPUx sleep code is common for hotplug, suspend and CPUilde. > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Kevin Hilman <khilman@ti.com> > --- > arch/arm/mach-omap2/Makefile | 3 +- > arch/arm/mach-omap2/include/mach/omap-secure.h | 8 + > arch/arm/mach-omap2/include/mach/omap4-common.h | 25 +++ > arch/arm/mach-omap2/omap-mpuss-lowpower.c | 249 +++++++++++++++++++++++ > arch/arm/mach-omap2/omap-smp.c | 6 + > arch/arm/mach-omap2/omap4-sar-layout.h | 9 + > arch/arm/mach-omap2/pm44xx.c | 6 + > arch/arm/mach-omap2/sleep44xx.S | 213 +++++++++++++++++++ > 8 files changed, 518 insertions(+), 1 deletions(-) > create mode 100644 arch/arm/mach-omap2/omap-mpuss-lowpower.c > [...] > diff --git a/arch/arm/mach-omap2/sleep44xx.S b/arch/arm/mach-omap2/sleep44xx.S > index 049f426..230ab8c 100644 > --- a/arch/arm/mach-omap2/sleep44xx.S > +++ b/arch/arm/mach-omap2/sleep44xx.S > @@ -11,8 +11,221 @@ > > #include <linux/linkage.h> > #include <asm/system.h> > +#include <asm/smp_scu.h> > +#include <asm/memory.h> > +#include <asm/hardware/cache-l2x0.h> > > +#include <plat/omap44xx.h> > #include <mach/omap4-common.h> > +#include <mach/omap-secure.h> > + > +#include "omap4-sar-layout.h" > + > +#ifdef CONFIG_SMP > + > +.macro DO_SMC > + dsb > + smc #0 > + dsb > +.endm > + > +ppa_zero_params: > + .word 0x0 > + > +/* > + * ============================= > + * == CPU suspend finisher == > + * ============================= > + * > + * void omap4_finish_suspend(unsigned long cpu_state) > + * > + * This function code saves the CPU context and performs the CPU > + * power down sequence. Calling WFI effectively changes the CPU > + * power domains states to the desired target power state. > + * > + * @cpu_state : contains context save state (r0) > + * 0 - No context lost > + * 1 - CPUx L1 and logic lost: MPUSS CSWR > + * 2 - CPUx L1 and logic lost + GIC lost: MPUSS OSWR > + * 3 - CPUx L1 and logic lost + GIC + L2 lost: MPUSS OFF I was told by rmk that same as imx6q, omap44xx will retain L2 content across suspen/resume cycle. Then what does "L2 lost" mean here? Or what rmk meant is the case cpu_state == 2?
On Friday 09 September 2011 12:49 PM, Thomas Gleixner wrote: > On Fri, 9 Sep 2011, Santosh wrote: > >> On Thursday 08 September 2011 11:57 PM, Kevin Hilman wrote: >>> Santosh Shilimkar<santosh.shilimkar@ti.com> writes: >>> >>>> OMAP WakeupGen is the interrupt controller extension used along >>>> with ARM GIC to wake the CPU out from low power states on >>>> external interrupts. >>>> >>>> The WakeupGen unit is responsible for generating wakeup event >>>> from the incoming interrupts and enable bits. It is implemented >>>> in MPU always ON power domain. During normal operation, >>>> WakeupGen delivers external interrupts directly to the GIC. >>>> >>>> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >>>> Cc: Kevin Hilman<khilman@ti.com> >>> >>> [...] >>> >>>> +#ifdef CONFIG_PM >>>> +/* >>>> + * Masking wakeup irqs is handled by the IRQCHIP_MASK_ON_SUSPEND flag, >>>> + * so no action is necessary in set_wake, but implement an empty handler >>>> + * here to prevent enable_irq_wake() returning an error. >>>> + * FIXME: Remove the dummy handler once gen irq code fix above. >>>> + */ >>> >>> Just curious... is there a fix for this pending for v3.2? >>> >> I have sent a proposed change to Thomas but have not seen >> any response yet from him. >> >> http://www.spinics.net/lists/arm-kernel/msg134297.html > > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index 0a7840a..cd4bc01 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > > @@ -467,6 +467,9 @@ static int set_irq_wake_real(unsigned int irq, unsigned int on) > > struct irq_desc *desc = irq_to_desc(irq); > int ret = -ENXIO; > > + if (irq_desc_get_chip(desc)->flags& IRQCHIP_MASK_ON_SUSPEND) > + return 0; > + > if (desc->irq_data.chip->irq_set_wake) > ret = desc->irq_data.chip->irq_set_wake(&desc->irq_data, on); > > The flag says: MASK ON SUSPEND and it does not imply that you don't > need a wake function. There might be cases where you want to setup > stuff in that function in order to have the wakeup happen on that > interrupt line despite of the mask on suspend. > I see your point. > We either need a separate flag or a global dummy set_wake function in > the core to avoid empty copies all over the place. > A flag is probably better since you mentioned that on some arch, there might be need to have actual set_wake() handler. Or if the global dummy can be over-ridden by platform, that's fine too. Regards Santosh
On Friday 09 September 2011 01:34 PM, Shawn Guo wrote: > Hi Santosh, > > On Sun, Sep 04, 2011 at 07:24:15PM +0530, Santosh Shilimkar wrote: >> This patch adds the CPU0 and CPU1 off mode support. CPUX close switch >> retention (CSWR) is not supported by hardware design. >> >> The CPUx OFF mode isn't supported on OMAP4430 ES1.0 >> >> CPUx sleep code is common for hotplug, suspend and CPUilde. >> >> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> Cc: Kevin Hilman<khilman@ti.com> >> --- >> arch/arm/mach-omap2/Makefile | 3 +- >> arch/arm/mach-omap2/include/mach/omap-secure.h | 8 + >> arch/arm/mach-omap2/include/mach/omap4-common.h | 25 +++ >> arch/arm/mach-omap2/omap-mpuss-lowpower.c | 249 +++++++++++++++++++++++ >> arch/arm/mach-omap2/omap-smp.c | 6 + >> arch/arm/mach-omap2/omap4-sar-layout.h | 9 + >> arch/arm/mach-omap2/pm44xx.c | 6 + >> arch/arm/mach-omap2/sleep44xx.S | 213 +++++++++++++++++++ >> 8 files changed, 518 insertions(+), 1 deletions(-) >> create mode 100644 arch/arm/mach-omap2/omap-mpuss-lowpower.c >> > > [...] > >> diff --git a/arch/arm/mach-omap2/sleep44xx.S b/arch/arm/mach-omap2/sleep44xx.S >> index 049f426..230ab8c 100644 >> --- a/arch/arm/mach-omap2/sleep44xx.S >> +++ b/arch/arm/mach-omap2/sleep44xx.S >> @@ -11,8 +11,221 @@ >> >> #include<linux/linkage.h> >> #include<asm/system.h> >> +#include<asm/smp_scu.h> >> +#include<asm/memory.h> >> +#include<asm/hardware/cache-l2x0.h> >> >> +#include<plat/omap44xx.h> >> #include<mach/omap4-common.h> >> +#include<mach/omap-secure.h> >> + >> +#include "omap4-sar-layout.h" >> + >> +#ifdef CONFIG_SMP >> + >> +.macro DO_SMC >> + dsb >> + smc #0 >> + dsb >> +.endm >> + >> +ppa_zero_params: >> + .word 0x0 >> + >> +/* >> + * ============================= >> + * == CPU suspend finisher == >> + * ============================= >> + * >> + * void omap4_finish_suspend(unsigned long cpu_state) >> + * >> + * This function code saves the CPU context and performs the CPU >> + * power down sequence. Calling WFI effectively changes the CPU >> + * power domains states to the desired target power state. >> + * >> + * @cpu_state : contains context save state (r0) >> + * 0 - No context lost >> + * 1 - CPUx L1 and logic lost: MPUSS CSWR >> + * 2 - CPUx L1 and logic lost + GIC lost: MPUSS OSWR >> + * 3 - CPUx L1 and logic lost + GIC + L2 lost: MPUSS OFF > > I was told by rmk that same as imx6q, omap44xx will retain L2 content > across suspen/resume cycle. Then what does "L2 lost" mean here? Or > what rmk meant is the case cpu_state == 2? > Yes. The last case is entire SOC OFF. We call Device OFF in OMAP. All voltages will scale to 0 V. This isn't supported by this series. Regards Santosh
On Fri, 9 Sep 2011, Santosh wrote: > On Friday 09 September 2011 12:49 PM, Thomas Gleixner wrote: > > > > The flag says: MASK ON SUSPEND and it does not imply that you don't > > need a wake function. There might be cases where you want to setup > > stuff in that function in order to have the wakeup happen on that > > interrupt line despite of the mask on suspend. > > > I see your point. > > > We either need a separate flag or a global dummy set_wake function in > > the core to avoid empty copies all over the place. > > > A flag is probably better since you mentioned that on some arch, there > might be need to have actual set_wake() handler. Or if the global > dummy can be over-ridden by platform, that's fine too. Global dummy would mean: int irq_set_wake_dummy(...) { return 0; } And you just add it to your chip, but either way I don't care whether it's a dummy function or a flag. Thanks, tglx
On Friday 09 September 2011 12:49 AM, Jean Pihet wrote: > On Sun, Sep 4, 2011 at 3:54 PM, Santosh Shilimkar > <santosh.shilimkar@ti.com> wrote: >> Allocate the memory to save secure ram context which needs >> to be done when MPU is hitting off mode. >> >> The ROM code expects a physical address to this memory >> and hence use memblock APIs to reserve this memory as part >> of .reserve() callback. >> >> Maximum possible size is allocated and can cater to OMAP3XXX / OMAP4XXX >> secure RAM size requirements. >> >> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> --- >> arch/arm/mach-omap2/include/mach/omap-secure.h | 4 +++ >> arch/arm/mach-omap2/omap-secure.c | 29 ++++++++++++++++++++++++ >> arch/arm/plat-omap/common.c | 3 ++ >> 3 files changed, 36 insertions(+), 0 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/include/mach/omap-secure.h b/arch/arm/mach-omap2/include/mach/omap-secure.h >> index 26e7bcc..e2f95a0 100644 >> --- a/arch/arm/mach-omap2/include/mach/omap-secure.h >> +++ b/arch/arm/mach-omap2/include/mach/omap-secure.h >> @@ -26,6 +26,8 @@ >> #define FLAG_FIQ_ENABLE 0x1 >> #define NO_FLAG 0x0 >> >> +/* Maximum Secure memory storage size */ >> +#define OMAP_SECURE_RAM_STORAGE (88 * SZ_1K) > Is this valid for all supported devices? How to differentiate > variations in the size for new chips variants in the future? > You don't have to. ROM code does that job. We should just ensure that maximum needed memory is made available to secure code. Regards Santosh
On Friday 09 September 2011 01:09 AM, Jean Pihet wrote: > On Sun, Sep 4, 2011 at 3:54 PM, Santosh Shilimkar > <santosh.shilimkar@ti.com> wrote: >> This patch adds the CPU0 and CPU1 off mode support. CPUX close switch >> retention (CSWR) is not supported by hardware design. >> >> The CPUx OFF mode isn't supported on OMAP4430 ES1.0 >> >> CPUx sleep code is common for hotplug, suspend and CPUilde. >> >> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> Cc: Kevin Hilman<khilman@ti.com> >> --- >> arch/arm/mach-omap2/Makefile | 3 +- >> arch/arm/mach-omap2/include/mach/omap-secure.h | 8 + >> arch/arm/mach-omap2/include/mach/omap4-common.h | 25 +++ >> arch/arm/mach-omap2/omap-mpuss-lowpower.c | 249 +++++++++++++++++++++++ >> arch/arm/mach-omap2/omap-smp.c | 6 + >> arch/arm/mach-omap2/omap4-sar-layout.h | 9 + >> arch/arm/mach-omap2/pm44xx.c | 6 + >> arch/arm/mach-omap2/sleep44xx.S | 213 +++++++++++++++++++ >> 8 files changed, 518 insertions(+), 1 deletions(-) >> create mode 100644 arch/arm/mach-omap2/omap-mpuss-lowpower.c >> >> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile >> index b032c21..b4f2eeb 100644 >> --- a/arch/arm/mach-omap2/Makefile >> +++ b/arch/arm/mach-omap2/Makefile >> @@ -63,7 +63,8 @@ obj-$(CONFIG_ARCH_OMAP2) += pm24xx.o >> obj-$(CONFIG_ARCH_OMAP2) += sleep24xx.o >> obj-$(CONFIG_ARCH_OMAP3) += pm34xx.o sleep34xx.o \ >> cpuidle34xx.o >> -obj-$(CONFIG_ARCH_OMAP4) += pm44xx.o sleep44xx.o >> +obj-$(CONFIG_ARCH_OMAP4) += pm44xx.o sleep44xx.o \ >> + omap-mpuss-lowpower.o >> obj-$(CONFIG_PM_DEBUG) += pm-debug.o >> obj-$(CONFIG_OMAP_SMARTREFLEX) += sr_device.o smartreflex.o >> obj-$(CONFIG_OMAP_SMARTREFLEX_CLASS3) += smartreflex-class3.o >> diff --git a/arch/arm/mach-omap2/include/mach/omap-secure.h b/arch/arm/mach-omap2/include/mach/omap-secure.h >> index e2f95a0..0062d49 100644 >> --- a/arch/arm/mach-omap2/include/mach/omap-secure.h >> +++ b/arch/arm/mach-omap2/include/mach/omap-secure.h >> @@ -35,10 +35,18 @@ >> #define OMAP4_HAL_SAVEALL_INDEX 0x1c >> #define OMAP4_HAL_SAVEGIC_INDEX 0x1d >> >> +/* Secure Monitor mode APIs */ >> +#define OMAP4_MON_SCU_PWR_INDEX 0x108 >> + >> +/* Secure PPA(Primary Protected Application) APIs */ >> +#define OMAP4_PPA_CPU_ACTRL_SMP_INDEX 0x25 >> + >> +#ifndef __ASSEMBLER__ >> extern u32 omap_secure_dispatcher(u32 idx, u32 flag, u32 nargs, >> u32 arg1, u32 arg2, u32 arg3, u32 arg4); >> extern u32 omap_smc2(u32 id, u32 falg, u32 pargs); >> extern phys_addr_t omap_secure_ram_mempool_base(void); >> extern int omap_secure_ram_reserve_memblock(void); >> >> +#endif /* __ASSEMBLER__ */ >> #endif /* OMAP_ARCH_OMAP_SECURE_H */ >> diff --git a/arch/arm/mach-omap2/include/mach/omap4-common.h b/arch/arm/mach-omap2/include/mach/omap4-common.h >> index 040dcf6..662f37c 100644 >> --- a/arch/arm/mach-omap2/include/mach/omap4-common.h >> +++ b/arch/arm/mach-omap2/include/mach/omap4-common.h >> @@ -45,5 +45,30 @@ extern u32 omap_modify_auxcoreboot0(u32 set_mask, u32 clear_mask); >> extern void omap_auxcoreboot_addr(u32 cpu_addr); >> extern u32 omap_read_auxcoreboot0(void); >> #endif >> + >> +#if defined(CONFIG_SMP)&& defined(CONFIG_PM) >> +extern int omap4_mpuss_init(void); >> +extern int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state); >> +extern int omap4_finish_suspend(unsigned long cpu_state); >> +extern void omap4_cpu_resume(void); >> +#else >> +static inline int omap4_enter_lowpower(unsigned int cpu, >> + unsigned int power_state) >> +{ >> + omap_do_wfi(); >> + return 0; >> +} >> + >> +static inline int omap4_mpuss_init(void) >> +{ >> + return 0; >> +} >> + >> +static inline int omap4_finish_suspend(unsigned long cpu_state) >> +{} > The int function should return something (0?) > Has the code been test compiled without SMP and/or PM? > >> +static inline void omap4_cpu_resume(void) >> +{} >> +#endif >> + >> #endif /* __ASSEMBLER__ */ >> #endif /* OMAP_ARCH_OMAP4_COMMON_H */ >> diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c > ... > >> +/** >> + * omap4_enter_lowpower: OMAP4 MPUSS Low Power Entry Function >> + * The purpose of this function is to manage low power programming >> + * of OMAP4 MPUSS subsystem >> + * @cpu : CPU ID >> + * @power_state: Low power state. >> + */ >> +int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state) >> +{ >> + unsigned int save_state = 0; >> + unsigned int wakeup_cpu; >> + >> + if (omap_rev() == OMAP4430_REV_ES1_0) >> + return -ENXIO; >> + >> + switch (power_state) { >> + case PWRDM_POWER_ON: >> + case PWRDM_POWER_INACTIVE: >> + save_state = 0; >> + break; >> + case PWRDM_POWER_OFF: >> + save_state = 1; >> + break; >> + case PWRDM_POWER_RET: >> + default: >> + /* >> + * CPUx CSWR is invalid hardware state. Also CPUx OSWR >> + * doesn't make much scense, since logic is lost and $L1 >> + * needs to be cleaned because of coherency. This makes >> + * CPUx OSWR equivalent to CPUX OFF and hence not supported >> + */ >> + WARN_ON(1); >> + return -ENXIO; >> + } >> + >> + clear_cpu_prev_pwrst(cpu); >> + set_cpu_next_pwrst(cpu, power_state); >> + set_cpu_wakeup_addr(cpu, virt_to_phys(omap4_cpu_resume)); >> + scu_pwrst_prepare(cpu, power_state); >> + >> + /* >> + * Call low level function with targeted CPU id >> + * and its low power state. >> + */ >> + cpu_suspend(save_state, omap4_finish_suspend); > There is no CPU id parameter to the call although the above comment says so. > > ... > >> diff --git a/arch/arm/mach-omap2/sleep44xx.S b/arch/arm/mach-omap2/sleep44xx.S > ... > >> +/* >> + * ============================= >> + * == CPU suspend finisher == >> + * ============================= >> + * >> + * void omap4_finish_suspend(unsigned long cpu_state) >> + * >> + * This function code saves the CPU context and performs the CPU >> + * power down sequence. Calling WFI effectively changes the CPU >> + * power domains states to the desired target power state. >> + * >> + * @cpu_state : contains context save state (r0) >> + * 0 - No context lost >> + * 1 - CPUx L1 and logic lost: MPUSS CSWR >> + * 2 - CPUx L1 and logic lost + GIC lost: MPUSS OSWR >> + * 3 - CPUx L1 and logic lost + GIC + L2 lost: MPUSS OFF >> + * @return: This function never returns for CPU OFF and DORMANT power states. >> + * Post WFI, CPU transitions to DORMANT or OFF power state and on wake-up >> + * from this follows a full CPU reset path via ROM code to CPU restore code. > Is the ROM code jumping to the physical address of omap4_cpu_resume? > If so please add it in the comment. > > ... > >> +/* >> + * ============================ >> + * == CPU resume entry point == >> + * ============================ >> + * >> + * void omap4_cpu_resume(void) >> + * >> + * ROM code jumps to this function while waking up from CPU >> + * OFF or DORMANT state. Physical address of the function is >> + * stored in the SAR RAM while entering to OFF or DORMANT mode. > Which code stores the address in the SAR RAM? Please describe it here. > Will add that comment. Regards Santosh
Santosh, On Fri, Sep 9, 2011 at 11:43 AM, Santosh <santosh.shilimkar@ti.com> wrote: > On Friday 09 September 2011 12:49 AM, Jean Pihet wrote: >> >> On Sun, Sep 4, 2011 at 3:54 PM, Santosh Shilimkar >> <santosh.shilimkar@ti.com> wrote: >>> >>> Allocate the memory to save secure ram context which needs >>> to be done when MPU is hitting off mode. >>> >>> The ROM code expects a physical address to this memory >>> and hence use memblock APIs to reserve this memory as part >>> of .reserve() callback. >>> >>> Maximum possible size is allocated and can cater to OMAP3XXX / OMAP4XXX >>> secure RAM size requirements. >>> >>> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >>> --- >>> arch/arm/mach-omap2/include/mach/omap-secure.h | 4 +++ >>> arch/arm/mach-omap2/omap-secure.c | 29 >>> ++++++++++++++++++++++++ >>> arch/arm/plat-omap/common.c | 3 ++ >>> 3 files changed, 36 insertions(+), 0 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/include/mach/omap-secure.h >>> b/arch/arm/mach-omap2/include/mach/omap-secure.h >>> index 26e7bcc..e2f95a0 100644 >>> --- a/arch/arm/mach-omap2/include/mach/omap-secure.h >>> +++ b/arch/arm/mach-omap2/include/mach/omap-secure.h >>> @@ -26,6 +26,8 @@ >>> #define FLAG_FIQ_ENABLE 0x1 >>> #define NO_FLAG 0x0 >>> >>> +/* Maximum Secure memory storage size */ >>> +#define OMAP_SECURE_RAM_STORAGE (88 * SZ_1K) >> >> Is this valid for all supported devices? How to differentiate >> variations in the size for new chips variants in the future? >> > You don't have to. ROM code does that job. We should just ensure > that maximum needed memory is made available to secure code. The question was: what would be the code if tomorrow -for example- we have OMAP4460 with 88K of secure RAM, OMAP5432 with 128K and OMAP6667 with 132K? How to code the differences? Regards, Jean > > Regards > Santosh >
On Fri, Sep 9, 2011 at 6:24 PM, Jean Pihet <jean.pihet@newoldbits.com> wrote: > Santosh, > > On Fri, Sep 9, 2011 at 11:43 AM, Santosh <santosh.shilimkar@ti.com> wrote: >> On Friday 09 September 2011 12:49 AM, Jean Pihet wrote: >>> >>> On Sun, Sep 4, 2011 at 3:54 PM, Santosh Shilimkar >>> <santosh.shilimkar@ti.com> wrote: >>>> >>>> Allocate the memory to save secure ram context which needs >>>> to be done when MPU is hitting off mode. >>>> >>>> The ROM code expects a physical address to this memory >>>> and hence use memblock APIs to reserve this memory as part >>>> of .reserve() callback. >>>> >>>> Maximum possible size is allocated and can cater to OMAP3XXX / OMAP4XXX >>>> secure RAM size requirements. >>>> >>>> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >>>> --- >>>> arch/arm/mach-omap2/include/mach/omap-secure.h | 4 +++ >>>> arch/arm/mach-omap2/omap-secure.c | 29 >>>> ++++++++++++++++++++++++ >>>> arch/arm/plat-omap/common.c | 3 ++ >>>> 3 files changed, 36 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-omap2/include/mach/omap-secure.h >>>> b/arch/arm/mach-omap2/include/mach/omap-secure.h >>>> index 26e7bcc..e2f95a0 100644 >>>> --- a/arch/arm/mach-omap2/include/mach/omap-secure.h >>>> +++ b/arch/arm/mach-omap2/include/mach/omap-secure.h >>>> @@ -26,6 +26,8 @@ >>>> #define FLAG_FIQ_ENABLE 0x1 >>>> #define NO_FLAG 0x0 >>>> >>>> +/* Maximum Secure memory storage size */ >>>> +#define OMAP_SECURE_RAM_STORAGE (88 * SZ_1K) >>> >>> Is this valid for all supported devices? How to differentiate >>> variations in the size for new chips variants in the future? >>> >> You don't have to. ROM code does that job. We should just ensure >> that maximum needed memory is made available to secure code. > The question was: what would be the code if tomorrow -for example- we > have OMAP4460 with 88K of secure RAM, OMAP5432 with 128K and OMAP6667 > with 132K? How to code the differences? > The size doesn't depend on silicon but depends on Secure software stack size. As I said, we will allocate maximum needed as per security team recommendation. If on newer security stack, if we need more, we will just increase it to maximum needed.
On Fri, Sep 9, 2011 at 7:43 PM, Shawn Guo <shawn.guo@freescale.com> wrote: > On Fri, Sep 09, 2011 at 01:39:51PM +0530, Santosh wrote: >> On Friday 09 September 2011 01:34 PM, Shawn Guo wrote: >> >Hi Santosh, >> > >> >On Sun, Sep 04, 2011 at 07:24:15PM +0530, Santosh Shilimkar wrote: >> >>This patch adds the CPU0 and CPU1 off mode support. CPUX close switch >> >>retention (CSWR) is not supported by hardware design. >> >> >> >>The CPUx OFF mode isn't supported on OMAP4430 ES1.0 >> >> >> >>CPUx sleep code is common for hotplug, suspend and CPUilde. >> >> >> >>Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> >>Cc: Kevin Hilman<khilman@ti.com> >> >>--- >> >> arch/arm/mach-omap2/Makefile | 3 +- >> >> arch/arm/mach-omap2/include/mach/omap-secure.h | 8 + >> >> arch/arm/mach-omap2/include/mach/omap4-common.h | 25 +++ >> >> arch/arm/mach-omap2/omap-mpuss-lowpower.c | 249 +++++++++++++++++++++++ >> >> arch/arm/mach-omap2/omap-smp.c | 6 + >> >> arch/arm/mach-omap2/omap4-sar-layout.h | 9 + >> >> arch/arm/mach-omap2/pm44xx.c | 6 + >> >> arch/arm/mach-omap2/sleep44xx.S | 213 +++++++++++++++++++ >> >> 8 files changed, 518 insertions(+), 1 deletions(-) >> >> create mode 100644 arch/arm/mach-omap2/omap-mpuss-lowpower.c >> >> >> > >> >[...] >> > >> >>diff --git a/arch/arm/mach-omap2/sleep44xx.S b/arch/arm/mach-omap2/sleep44xx.S >> >>index 049f426..230ab8c 100644 >> >>--- a/arch/arm/mach-omap2/sleep44xx.S >> >>+++ b/arch/arm/mach-omap2/sleep44xx.S >> >>@@ -11,8 +11,221 @@ >> >> >> >> #include<linux/linkage.h> >> >> #include<asm/system.h> >> >>+#include<asm/smp_scu.h> >> >>+#include<asm/memory.h> >> >>+#include<asm/hardware/cache-l2x0.h> >> >> >> >>+#include<plat/omap44xx.h> >> >> #include<mach/omap4-common.h> >> >>+#include<mach/omap-secure.h> >> >>+ >> >>+#include "omap4-sar-layout.h" >> >>+ >> >>+#ifdef CONFIG_SMP >> >>+ >> >>+.macro DO_SMC >> >>+ dsb >> >>+ smc #0 >> >>+ dsb >> >>+.endm >> >>+ >> >>+ppa_zero_params: >> >>+ .word 0x0 >> >>+ >> >>+/* >> >>+ * ============================= >> >>+ * == CPU suspend finisher == >> >>+ * ============================= >> >>+ * >> >>+ * void omap4_finish_suspend(unsigned long cpu_state) >> >>+ * >> >>+ * This function code saves the CPU context and performs the CPU >> >>+ * power down sequence. Calling WFI effectively changes the CPU >> >>+ * power domains states to the desired target power state. >> >>+ * >> >>+ * @cpu_state : contains context save state (r0) >> >>+ * 0 - No context lost >> >>+ * 1 - CPUx L1 and logic lost: MPUSS CSWR >> >>+ * 2 - CPUx L1 and logic lost + GIC lost: MPUSS OSWR >> >>+ * 3 - CPUx L1 and logic lost + GIC + L2 lost: MPUSS OFF >> > >> >I was told by rmk that same as imx6q, omap44xx will retain L2 content >> >across suspen/resume cycle. Then what does "L2 lost" mean here? Or >> >what rmk meant is the case cpu_state == 2? >> > >> Yes. >> >> The last case is entire SOC OFF. We call Device OFF in OMAP. >> All voltages will scale to 0 V. This isn't supported by this >> series. >> > Then the second question would be what the following patch in this > series is for. > > [PATCH 20/25] OMAP4: PM: Add L2X0 cache lowpower support > > I could have read the patch incorrectly, but it seems l2x_clean_inv > will also be called for "MPUSS OSWR" in which case L2 is retained? > Shouldn't L2 for this case have been handled by rmk's patch (ARM: pm: > add L2 cache cleaning for suspend)? > Yes you did read it wrongly :) It won't be called for OSWR
On Fri, Sep 09, 2011 at 01:39:51PM +0530, Santosh wrote: > On Friday 09 September 2011 01:34 PM, Shawn Guo wrote: > >Hi Santosh, > > > >On Sun, Sep 04, 2011 at 07:24:15PM +0530, Santosh Shilimkar wrote: > >>This patch adds the CPU0 and CPU1 off mode support. CPUX close switch > >>retention (CSWR) is not supported by hardware design. > >> > >>The CPUx OFF mode isn't supported on OMAP4430 ES1.0 > >> > >>CPUx sleep code is common for hotplug, suspend and CPUilde. > >> > >>Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> > >>Cc: Kevin Hilman<khilman@ti.com> > >>--- > >> arch/arm/mach-omap2/Makefile | 3 +- > >> arch/arm/mach-omap2/include/mach/omap-secure.h | 8 + > >> arch/arm/mach-omap2/include/mach/omap4-common.h | 25 +++ > >> arch/arm/mach-omap2/omap-mpuss-lowpower.c | 249 +++++++++++++++++++++++ > >> arch/arm/mach-omap2/omap-smp.c | 6 + > >> arch/arm/mach-omap2/omap4-sar-layout.h | 9 + > >> arch/arm/mach-omap2/pm44xx.c | 6 + > >> arch/arm/mach-omap2/sleep44xx.S | 213 +++++++++++++++++++ > >> 8 files changed, 518 insertions(+), 1 deletions(-) > >> create mode 100644 arch/arm/mach-omap2/omap-mpuss-lowpower.c > >> > > > >[...] > > > >>diff --git a/arch/arm/mach-omap2/sleep44xx.S b/arch/arm/mach-omap2/sleep44xx.S > >>index 049f426..230ab8c 100644 > >>--- a/arch/arm/mach-omap2/sleep44xx.S > >>+++ b/arch/arm/mach-omap2/sleep44xx.S > >>@@ -11,8 +11,221 @@ > >> > >> #include<linux/linkage.h> > >> #include<asm/system.h> > >>+#include<asm/smp_scu.h> > >>+#include<asm/memory.h> > >>+#include<asm/hardware/cache-l2x0.h> > >> > >>+#include<plat/omap44xx.h> > >> #include<mach/omap4-common.h> > >>+#include<mach/omap-secure.h> > >>+ > >>+#include "omap4-sar-layout.h" > >>+ > >>+#ifdef CONFIG_SMP > >>+ > >>+.macro DO_SMC > >>+ dsb > >>+ smc #0 > >>+ dsb > >>+.endm > >>+ > >>+ppa_zero_params: > >>+ .word 0x0 > >>+ > >>+/* > >>+ * ============================= > >>+ * == CPU suspend finisher == > >>+ * ============================= > >>+ * > >>+ * void omap4_finish_suspend(unsigned long cpu_state) > >>+ * > >>+ * This function code saves the CPU context and performs the CPU > >>+ * power down sequence. Calling WFI effectively changes the CPU > >>+ * power domains states to the desired target power state. > >>+ * > >>+ * @cpu_state : contains context save state (r0) > >>+ * 0 - No context lost > >>+ * 1 - CPUx L1 and logic lost: MPUSS CSWR > >>+ * 2 - CPUx L1 and logic lost + GIC lost: MPUSS OSWR > >>+ * 3 - CPUx L1 and logic lost + GIC + L2 lost: MPUSS OFF > > > >I was told by rmk that same as imx6q, omap44xx will retain L2 content > >across suspen/resume cycle. Then what does "L2 lost" mean here? Or > >what rmk meant is the case cpu_state == 2? > > > Yes. > > The last case is entire SOC OFF. We call Device OFF in OMAP. > All voltages will scale to 0 V. This isn't supported by this > series. > Then the second question would be what the following patch in this series is for. [PATCH 20/25] OMAP4: PM: Add L2X0 cache lowpower support I could have read the patch incorrectly, but it seems l2x_clean_inv will also be called for "MPUSS OSWR" in which case L2 is retained? Shouldn't L2 for this case have been handled by rmk's patch (ARM: pm: add L2 cache cleaning for suspend)?
On Fri, Sep 09, 2011 at 07:41:08PM +0530, Shilimkar, Santosh wrote: > On Fri, Sep 9, 2011 at 7:43 PM, Shawn Guo <shawn.guo@freescale.com> wrote: > > On Fri, Sep 09, 2011 at 01:39:51PM +0530, Santosh wrote: > >> On Friday 09 September 2011 01:34 PM, Shawn Guo wrote: > >> >Hi Santosh, > >> > > >> >On Sun, Sep 04, 2011 at 07:24:15PM +0530, Santosh Shilimkar wrote: > >> >>This patch adds the CPU0 and CPU1 off mode support. CPUX close switch > >> >>retention (CSWR) is not supported by hardware design. > >> >> > >> >>The CPUx OFF mode isn't supported on OMAP4430 ES1.0 > >> >> > >> >>CPUx sleep code is common for hotplug, suspend and CPUilde. > >> >> > >> >>Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> > >> >>Cc: Kevin Hilman<khilman@ti.com> > >> >>--- > >> >> arch/arm/mach-omap2/Makefile | 3 +- > >> >> arch/arm/mach-omap2/include/mach/omap-secure.h | 8 + > >> >> arch/arm/mach-omap2/include/mach/omap4-common.h | 25 +++ > >> >> arch/arm/mach-omap2/omap-mpuss-lowpower.c | 249 +++++++++++++++++++++++ > >> >> arch/arm/mach-omap2/omap-smp.c | 6 + > >> >> arch/arm/mach-omap2/omap4-sar-layout.h | 9 + > >> >> arch/arm/mach-omap2/pm44xx.c | 6 + > >> >> arch/arm/mach-omap2/sleep44xx.S | 213 +++++++++++++++++++ > >> >> 8 files changed, 518 insertions(+), 1 deletions(-) > >> >> create mode 100644 arch/arm/mach-omap2/omap-mpuss-lowpower.c > >> >> > >> > > >> >[...] > >> > > >> >>diff --git a/arch/arm/mach-omap2/sleep44xx.S b/arch/arm/mach-omap2/sleep44xx.S > >> >>index 049f426..230ab8c 100644 > >> >>--- a/arch/arm/mach-omap2/sleep44xx.S > >> >>+++ b/arch/arm/mach-omap2/sleep44xx.S > >> >>@@ -11,8 +11,221 @@ > >> >> > >> >> #include<linux/linkage.h> > >> >> #include<asm/system.h> > >> >>+#include<asm/smp_scu.h> > >> >>+#include<asm/memory.h> > >> >>+#include<asm/hardware/cache-l2x0.h> > >> >> > >> >>+#include<plat/omap44xx.h> > >> >> #include<mach/omap4-common.h> > >> >>+#include<mach/omap-secure.h> > >> >>+ > >> >>+#include "omap4-sar-layout.h" > >> >>+ > >> >>+#ifdef CONFIG_SMP > >> >>+ > >> >>+.macro DO_SMC > >> >>+ dsb > >> >>+ smc #0 > >> >>+ dsb > >> >>+.endm > >> >>+ > >> >>+ppa_zero_params: > >> >>+ .word 0x0 > >> >>+ > >> >>+/* > >> >>+ * ============================= > >> >>+ * == CPU suspend finisher == > >> >>+ * ============================= > >> >>+ * > >> >>+ * void omap4_finish_suspend(unsigned long cpu_state) > >> >>+ * > >> >>+ * This function code saves the CPU context and performs the CPU > >> >>+ * power down sequence. Calling WFI effectively changes the CPU > >> >>+ * power domains states to the desired target power state. > >> >>+ * > >> >>+ * @cpu_state : contains context save state (r0) > >> >>+ * 0 - No context lost > >> >>+ * 1 - CPUx L1 and logic lost: MPUSS CSWR > >> >>+ * 2 - CPUx L1 and logic lost + GIC lost: MPUSS OSWR > >> >>+ * 3 - CPUx L1 and logic lost + GIC + L2 lost: MPUSS OFF > >> > > >> >I was told by rmk that same as imx6q, omap44xx will retain L2 content > >> >across suspen/resume cycle. Then what does "L2 lost" mean here? Or > >> >what rmk meant is the case cpu_state == 2? > >> > > >> Yes. > >> > >> The last case is entire SOC OFF. We call Device OFF in OMAP. > >> All voltages will scale to 0 V. This isn't supported by this > >> series. > >> > > Then the second question would be what the following patch in this > > series is for. > > > > [PATCH 20/25] OMAP4: PM: Add L2X0 cache lowpower support > > > > I could have read the patch incorrectly, but it seems l2x_clean_inv > > will also be called for "MPUSS OSWR" in which case L2 is retained? > > Shouldn't L2 for this case have been handled by rmk's patch (ARM: pm: > > add L2 cache cleaning for suspend)? > > > Yes you did read it wrongly :) > It won't be called for OSWR > Come on, man. It's really hard for me who is still at entry level of assembly to understand that. So please help me out. The following is a copy from arch/arm/mach-omap2/sleep44xx.S after I apply this patch series on v3.1-rc4. > /* > * ============================= > * == CPU suspend finisher == > * ============================= > * > * void omap4_finish_suspend(unsigned long cpu_state) > * > * This function code saves the CPU context and performs the CPU > * power down sequence. Calling WFI effectively changes the CPU > * power domains states to the desired target power state. > * > * @cpu_state : contains context save state (r0) So cpu_state is passed in r0. > * 0 - No context lost > * 1 - CPUx L1 and logic lost: MPUSS CSWR > * 2 - CPUx L1 and logic lost + GIC lost: MPUSS OSWR > * 3 - CPUx L1 and logic lost + GIC + L2 lost: MPUSS OFF > * @return: This function never returns for CPU OFF and DORMANT power states. > * Post WFI, CPU transitions to DORMANT or OFF power state and on wake-up > * from this follows a full CPU reset path via ROM code to CPU restore code. > * It returns to the caller for CPU INACTIVE and ON power states or in case > * CPU failed to transition to targeted OFF/DORMANT state. > */ > ENTRY(omap4_finish_suspend) > stmfd sp!, {lr} > cmp r0, #0x0 > beq do_WFI @ No lowpower state, jump to WFI The case "0" is handled here. > > /* > * Flush all data from the L1 data cache before disabling > * SCTLR.C bit. > */ > bl omap4_get_sar_ram_base Isn't r0 lost here? > ldr r9, [r0, #OMAP_TYPE_OFFSET] > cmp r9, #0x1 @ Check for HS device > bne skip_secure_l1_clean > mov r0, #SCU_PM_NORMAL > mov r1, #0xFF @ clean seucre L1 > stmfd r13!, {r4-r12, r14} > ldr r12, =OMAP4_MON_SCU_PWR_INDEX > DO_SMC > ldmfd r13!, {r4-r12, r14} > skip_secure_l1_clean: > > /* > * Clear the SCTLR.C bit to prevent further data cache > * allocation. Clearing SCTLR.C would make all the data accesses > * strongly ordered and would not hit the cache. > */ > mrc p15, 0, r0, c1, c0, 0 > bic r0, r0, #(1 << 2) @ Disable the C bit > mcr p15, 0, r0, c1, c0, 0 > isb > > /* > * Invalidate L1 data cache. Even though only invalidate is > * necessary exported flush API is used here. Doing clean > * on already clean cache would be almost NOP. > */ > bl v7_flush_dcache_all > > /* > * Switch the CPU from Symmetric Multiprocessing (SMP) mode > * to AsymmetricMultiprocessing (AMP) mode by programming > * the SCU power status to DORMANT or OFF mode. > * This enables the CPU to be taken out of coherency by > * preventing the CPU from receiving cache, TLB, or BTB > * maintenance operations broadcast by other CPUs in the cluster. > */ > bl omap4_get_sar_ram_base > mov r8, r0 > ldr r9, [r8, #OMAP_TYPE_OFFSET] > cmp r9, #0x1 @ Check for HS device > bne scu_gp_set > mrc p15, 0, r0, c0, c0, 5 @ Read MPIDR > ands r0, r0, #0x0f > ldreq r0, [r8, #SCU_OFFSET0] > ldrne r0, [r8, #SCU_OFFSET1] > mov r1, #0x00 > stmfd r13!, {r4-r12, r14} > ldr r12, =OMAP4_MON_SCU_PWR_INDEX > DO_SMC > ldmfd r13!, {r4-r12, r14} > b skip_scu_gp_set > scu_gp_set: > mrc p15, 0, r0, c0, c0, 5 @ Read MPIDR > ands r0, r0, #0x0f > ldreq r1, [r8, #SCU_OFFSET0] > ldrne r1, [r8, #SCU_OFFSET1] > bl omap4_get_scu_base > bl scu_power_mode > skip_scu_gp_set: > mrc p15, 0, r0, c1, c1, 2 @ Read NSACR data > tst r0, #(1 << 18) > mrcne p15, 0, r0, c1, c0, 1 > bicne r0, r0, #(1 << 6) @ Disable SMP bit > mcrne p15, 0, r0, c1, c0, 1 > isb > dsb > #ifdef CONFIG_CACHE_L2X0 > /* > * Clean and invalidate the L2 cache. > * Common cache-l2x0.c functions can't be used here since it > * uses spinlocks. We are out of coherency here with data cache > * disabled. The spinlock implementation uses exclusive load/store > * instruction which can fail without data cache being enabled. > * OMAP4 hardware doesn't support exclusive monitor which can > * overcome exclusive access issue. Because of this, CPU can > * lead to deadlock. > */ > l2x_clean_inv: > bl omap4_get_sar_ram_base > mov r8, r0 > mrc p15, 0, r5, c0, c0, 5 @ Read MPIDR > ands r5, r5, #0x0f > ldreq r0, [r8, #L2X0_SAVE_OFFSET0] > ldrne r0, [r8, #L2X0_SAVE_OFFSET1] > cmp r0, #3 > bne do_WFI It looks like you are bypassing L2 clean and invalidate for cases "1" and "2" here. But I really do not understand how you get r0 back here. Regards, Shawn > #ifdef CONFIG_PL310_ERRATA_727915 > mov r0, #0x03 > mov r12, #OMAP4_MON_L2X0_DBG_CTRL_INDEX > DO_SMC > #endif > bl omap4_get_l2cache_base > mov r2, r0 > ldr r0, =0xffff > str r0, [r2, #L2X0_CLEAN_INV_WAY] > wait: > ldr r0, [r2, #L2X0_CLEAN_INV_WAY] > ldr r1, =0xffff > ands r0, r0, r1 > bne wait > #ifdef CONFIG_PL310_ERRATA_727915 > mov r0, #0x00 > mov r12, #OMAP4_MON_L2X0_DBG_CTRL_INDEX > DO_SMC > #endif > l2x_sync: > bl omap4_get_l2cache_base > mov r2, r0 > mov r0, #0x0 > str r0, [r2, #L2X0_CACHE_SYNC] > sync: > ldr r0, [r2, #L2X0_CACHE_SYNC] > ands r0, r0, #0x1 > bne sync > #endif > > do_WFI: > bl omap_do_wfi > > /* > * CPU is here when it failed to enter OFF/DORMANT or > * no low power state was attempted. > */ > mrc p15, 0, r0, c1, c0, 0 > tst r0, #(1 << 2) @ Check C bit enabled? > orreq r0, r0, #(1 << 2) @ Enable the C bit > mcreq p15, 0, r0, c1, c0, 0 > isb > > /* > * Ensure the CPU power state is set to NORMAL in > * SCU power state so that CPU is back in coherency. > * In non-coherent mode CPU can lock-up and lead to > * system deadlock. > */ > mrc p15, 0, r0, c1, c0, 1 > tst r0, #(1 << 6) @ Check SMP bit enabled? > orreq r0, r0, #(1 << 6) > mcreq p15, 0, r0, c1, c0, 1 > isb > bl omap4_get_sar_ram_base > mov r8, r0 > ldr r9, [r8, #OMAP_TYPE_OFFSET] > cmp r9, #0x1 @ Check for HS device > bne scu_gp_clear > mov r0, #SCU_PM_NORMAL > mov r1, #0x00 > stmfd r13!, {r4-r12, r14} > ldr r12, =OMAP4_MON_SCU_PWR_INDEX > DO_SMC > ldmfd r13!, {r4-r12, r14} > b skip_scu_gp_clear > scu_gp_clear: > bl omap4_get_scu_base > mov r1, #SCU_PM_NORMAL > bl scu_power_mode > skip_scu_gp_clear: > isb > dsb > ldmfd sp!, {pc} > ENDPROC(omap4_finish_suspend)
On Friday 09 September 2011 08:57 PM, Shawn Guo wrote: > On Fri, Sep 09, 2011 at 07:41:08PM +0530, Shilimkar, Santosh wrote: >> On Fri, Sep 9, 2011 at 7:43 PM, Shawn Guo<shawn.guo@freescale.com> wrote: >>> On Fri, Sep 09, 2011 at 01:39:51PM +0530, Santosh wrote: >>>> On Friday 09 September 2011 01:34 PM, Shawn Guo wrote: >>>>> Hi Santosh, [...] >> #ifdef CONFIG_CACHE_L2X0 >> /* >> * Clean and invalidate the L2 cache. >> * Common cache-l2x0.c functions can't be used here since it >> * uses spinlocks. We are out of coherency here with data cache >> * disabled. The spinlock implementation uses exclusive load/store >> * instruction which can fail without data cache being enabled. >> * OMAP4 hardware doesn't support exclusive monitor which can >> * overcome exclusive access issue. Because of this, CPU can >> * lead to deadlock. >> */ >> l2x_clean_inv: >> bl omap4_get_sar_ram_base >> mov r8, r0 >> mrc p15, 0, r5, c0, c0, 5 @ Read MPIDR >> ands r5, r5, #0x0f >> ldreq r0, [r8, #L2X0_SAVE_OFFSET0] >> ldrne r0, [r8, #L2X0_SAVE_OFFSET1] >> cmp r0, #3 >> bne do_WFI > > It looks like you are bypassing L2 clean and invalidate for cases > "1" and "2" here. But I really do not understand how you get r0 > back here. > The value which is passed in R0 is also stored in scratch patch memory and retrieved using L2X0_SAVE_OFFSET0. Simple :) Regards Santosh
Santosh <santosh.shilimkar@ti.com> writes: > On Friday 09 September 2011 08:57 PM, Shawn Guo wrote: >> On Fri, Sep 09, 2011 at 07:41:08PM +0530, Shilimkar, Santosh wrote: >>> On Fri, Sep 9, 2011 at 7:43 PM, Shawn Guo<shawn.guo@freescale.com> wrote: >>>> On Fri, Sep 09, 2011 at 01:39:51PM +0530, Santosh wrote: >>>>> On Friday 09 September 2011 01:34 PM, Shawn Guo wrote: >>>>>> Hi Santosh, > > [...] > >>> #ifdef CONFIG_CACHE_L2X0 >>> /* >>> * Clean and invalidate the L2 cache. >>> * Common cache-l2x0.c functions can't be used here since it >>> * uses spinlocks. We are out of coherency here with data cache >>> * disabled. The spinlock implementation uses exclusive load/store >>> * instruction which can fail without data cache being enabled. >>> * OMAP4 hardware doesn't support exclusive monitor which can >>> * overcome exclusive access issue. Because of this, CPU can >>> * lead to deadlock. >>> */ >>> l2x_clean_inv: >>> bl omap4_get_sar_ram_base >>> mov r8, r0 >>> mrc p15, 0, r5, c0, c0, 5 @ Read MPIDR >>> ands r5, r5, #0x0f >>> ldreq r0, [r8, #L2X0_SAVE_OFFSET0] >>> ldrne r0, [r8, #L2X0_SAVE_OFFSET1] >>> cmp r0, #3 >>> bne do_WFI >> >> It looks like you are bypassing L2 clean and invalidate for cases >> "1" and "2" here. But I really do not understand how you get r0 >> back here. >> > The value which is passed in R0 is also stored in scratch patch memory > and retrieved using L2X0_SAVE_OFFSET0. > Simple :) Sounds like some in-code documentation needs to be added to make this a bit more understandable. Thanks, Kevin
On Fri, Sep 09, 2011 at 10:29:49PM +0530, Santosh wrote: > On Friday 09 September 2011 08:57 PM, Shawn Guo wrote: > >On Fri, Sep 09, 2011 at 07:41:08PM +0530, Shilimkar, Santosh wrote: > >>On Fri, Sep 9, 2011 at 7:43 PM, Shawn Guo<shawn.guo@freescale.com> wrote: > >>>On Fri, Sep 09, 2011 at 01:39:51PM +0530, Santosh wrote: > >>>>On Friday 09 September 2011 01:34 PM, Shawn Guo wrote: > >>>>>Hi Santosh, > > [...] > > >>#ifdef CONFIG_CACHE_L2X0 > >> /* > >> * Clean and invalidate the L2 cache. > >> * Common cache-l2x0.c functions can't be used here since it > >> * uses spinlocks. We are out of coherency here with data cache > >> * disabled. The spinlock implementation uses exclusive load/store > >> * instruction which can fail without data cache being enabled. > >> * OMAP4 hardware doesn't support exclusive monitor which can > >> * overcome exclusive access issue. Because of this, CPU can > >> * lead to deadlock. > >> */ > >>l2x_clean_inv: > >> bl omap4_get_sar_ram_base > >> mov r8, r0 > >> mrc p15, 0, r5, c0, c0, 5 @ Read MPIDR > >> ands r5, r5, #0x0f > >> ldreq r0, [r8, #L2X0_SAVE_OFFSET0] > >> ldrne r0, [r8, #L2X0_SAVE_OFFSET1] > >> cmp r0, #3 > >> bne do_WFI > > > >It looks like you are bypassing L2 clean and invalidate for cases > >"1" and "2" here. But I really do not understand how you get r0 > >back here. > > > The value which is passed in R0 is also stored in scratch patch memory > and retrieved using L2X0_SAVE_OFFSET0. > Simple :) > Aha, no wonder that I read it wrongly. The parameter passing in r0 is here simply for confusing people. Also, IMO, lable "l2x_clean_inv" should be put after the "bne do_WFI". Otherwise, my original statement (it seems l2x_clean_inv will be called for case "2") stands correct :)
IOn Sat, Sep 10, 2011 at 5:04 AM, Shawn Guo <shawn.guo@freescale.com> wrote: > On Fri, Sep 09, 2011 at 10:29:49PM +0530, Santosh wrote: >> On Friday 09 September 2011 08:57 PM, Shawn Guo wrote: >> >On Fri, Sep 09, 2011 at 07:41:08PM +0530, Shilimkar, Santosh wrote: >> >>On Fri, Sep 9, 2011 at 7:43 PM, Shawn Guo<shawn.guo@freescale.com> wrote: >> >>>On Fri, Sep 09, 2011 at 01:39:51PM +0530, Santosh wrote: >> >>>>On Friday 09 September 2011 01:34 PM, Shawn Guo wrote: >> >>>>>Hi Santosh, >> >> [...] >> >> >>#ifdef CONFIG_CACHE_L2X0 >> >> /* >> >> * Clean and invalidate the L2 cache. >> >> * Common cache-l2x0.c functions can't be used here since it >> >> * uses spinlocks. We are out of coherency here with data cache >> >> * disabled. The spinlock implementation uses exclusive load/store >> >> * instruction which can fail without data cache being enabled. >> >> * OMAP4 hardware doesn't support exclusive monitor which can >> >> * overcome exclusive access issue. Because of this, CPU can >> >> * lead to deadlock. >> >> */ >> >>l2x_clean_inv: >> >> bl omap4_get_sar_ram_base >> >> mov r8, r0 >> >> mrc p15, 0, r5, c0, c0, 5 @ Read MPIDR >> >> ands r5, r5, #0x0f >> >> ldreq r0, [r8, #L2X0_SAVE_OFFSET0] >> >> ldrne r0, [r8, #L2X0_SAVE_OFFSET1] >> >> cmp r0, #3 >> >> bne do_WFI >> > >> >It looks like you are bypassing L2 clean and invalidate for cases >> >"1" and "2" here. But I really do not understand how you get r0 >> >back here. >> > >> The value which is passed in R0 is also stored in scratch patch memory >> and retrieved using L2X0_SAVE_OFFSET0. >> Simple :) >> > Aha, no wonder that I read it wrongly. The parameter passing in r0 > is here simply for confusing people. > No It's not. Same cod is used for CPUIDLE and we do have different C-states than just OSWR. ON and INACTIVE makes use of it. > Also, IMO, lable "l2x_clean_inv" should be put after the "bne do_WFI". > Otherwise, my original statement (it seems l2x_clean_inv will be > called for case "2") stands correct :) > It's just a label. All L2 related code and checks for the valid state is kept under that by purpose. Regards Santosh
On Sat, Sep 10, 2011 at 12:04 AM, Kevin Hilman <khilman@ti.com> wrote: > Santosh <santosh.shilimkar@ti.com> writes: > >> On Friday 09 September 2011 08:57 PM, Shawn Guo wrote: >>> On Fri, Sep 09, 2011 at 07:41:08PM +0530, Shilimkar, Santosh wrote: >>>> On Fri, Sep 9, 2011 at 7:43 PM, Shawn Guo<shawn.guo@freescale.com> wrote: >>>>> On Fri, Sep 09, 2011 at 01:39:51PM +0530, Santosh wrote: >>>>>> On Friday 09 September 2011 01:34 PM, Shawn Guo wrote: >>>>>>> Hi Santosh, >> >> [...] >> >>>> #ifdef CONFIG_CACHE_L2X0 >>>> /* >>>> * Clean and invalidate the L2 cache. >>>> * Common cache-l2x0.c functions can't be used here since it >>>> * uses spinlocks. We are out of coherency here with data cache >>>> * disabled. The spinlock implementation uses exclusive load/store >>>> * instruction which can fail without data cache being enabled. >>>> * OMAP4 hardware doesn't support exclusive monitor which can >>>> * overcome exclusive access issue. Because of this, CPU can >>>> * lead to deadlock. >>>> */ >>>> l2x_clean_inv: >>>> bl omap4_get_sar_ram_base >>>> mov r8, r0 >>>> mrc p15, 0, r5, c0, c0, 5 @ Read MPIDR >>>> ands r5, r5, #0x0f >>>> ldreq r0, [r8, #L2X0_SAVE_OFFSET0] >>>> ldrne r0, [r8, #L2X0_SAVE_OFFSET1] >>>> cmp r0, #3 >>>> bne do_WFI >>> >>> It looks like you are bypassing L2 clean and invalidate for cases >>> "1" and "2" here. But I really do not understand how you get r0 >>> back here. >>> >> The value which is passed in R0 is also stored in scratch patch memory >> and retrieved using L2X0_SAVE_OFFSET0. >> Simple :) > > Sounds like some in-code documentation needs to be added to make this a > bit more understandable. > Will add a comment
On Sat, Sep 10, 2011 at 09:08:16AM +0530, Shilimkar, Santosh wrote: > IOn Sat, Sep 10, 2011 at 5:04 AM, Shawn Guo <shawn.guo@freescale.com> wrote: [...] > > Also, IMO, lable "l2x_clean_inv" should be put after the "bne do_WFI". > > Otherwise, my original statement (it seems l2x_clean_inv will be > > called for case "2") stands correct :) > > > It's just a label. All L2 related code and checks for the valid state is > kept under that by purpose. > So for some case, you enter l2x_clean_inv but do not actually clean and invalidate L2. I do not see this label being used anywhere, so you may want to remove it to save the confusion to stupid people like me. Just my opinion.
On Saturday 10 September 2011 10:24 AM, Shawn Guo wrote: > On Sat, Sep 10, 2011 at 09:08:16AM +0530, Shilimkar, Santosh wrote: >> IOn Sat, Sep 10, 2011 at 5:04 AM, Shawn Guo<shawn.guo@freescale.com> wrote: > [...] >>> Also, IMO, lable "l2x_clean_inv" should be put after the "bne do_WFI". >>> Otherwise, my original statement (it seems l2x_clean_inv will be >>> called for case "2") stands correct :) >>> >> It's just a label. All L2 related code and checks for the valid state is >> kept under that by purpose. >> > So for some case, you enter l2x_clean_inv but do not actually clean > and invalidate L2. I do not see this label being used anywhere, so > you may want to remove it to save the confusion to stupid people like > me. Just my opinion. > Will remove the label :) Regards Santosh
On Fri, 9 Sep 2011, Santosh wrote: > On Friday 09 September 2011 01:48 PM, Thomas Gleixner wrote: > > On Fri, 9 Sep 2011, Santosh wrote: > > > On Friday 09 September 2011 12:49 PM, Thomas Gleixner wrote: > > > > > > > > The flag says: MASK ON SUSPEND and it does not imply that you don't > > > > need a wake function. There might be cases where you want to setup > > > > stuff in that function in order to have the wakeup happen on that > > > > interrupt line despite of the mask on suspend. > > > > > > > I see your point. > > > > > > > We either need a separate flag or a global dummy set_wake function in > > > > the core to avoid empty copies all over the place. > > > > > > > A flag is probably better since you mentioned that on some arch, there > > > might be need to have actual set_wake() handler. Or if the global > > > dummy can be over-ridden by platform, that's fine too. > > > > Global dummy would mean: > > > > int irq_set_wake_dummy(...) > > { > > return 0; > > } > > > > And you just add it to your chip, but either way I don't care whether > > it's a dummy function or a flag. > > > Will below patch work for you then ? > Attaching the same in case mailer damages it. It does :) > Regards, > Santosh > > From d63d4347dc8fb144b19f4d4e7c0621397cccea94 Mon Sep 17 00:00:00 2001 > From: Santosh Shilimkar <santosh.shilimkar@ti.com> > Date: Fri, 9 Sep 2011 13:59:35 +0530 > Subject: [PATCH] irq: Add IRQCHIP_SKIP_SET_WAKE flag to avoid need of dummy > set_wake() handler. > > Certain IRQCHIP's may not need to install the irq_set_wake() handler if > the IRQCHIP_MASK_ON_SUSPEND flag is set. But but if it's not implemented, > enable_irq_wake() will return an error. That needs the IRQCHIP to install > an empty set_wake handler. > > Add an 'IRQCHIP_SKIP_SET_WAKE' flag so that IRQCHIP can inform the core > irq code that irq_set_wake() handler is not necessary. > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Thomas Gleixner <tglx@linutronix.de> Queued to irq/core for 3.2 Thanks, tglx
On Monday 12 September 2011 01:26 PM, Thomas Gleixner wrote: > On Fri, 9 Sep 2011, Santosh wrote: > [..] >> From d63d4347dc8fb144b19f4d4e7c0621397cccea94 Mon Sep 17 00:00:00 2001 >> From: Santosh Shilimkar<santosh.shilimkar@ti.com> >> Date: Fri, 9 Sep 2011 13:59:35 +0530 >> Subject: [PATCH] irq: Add IRQCHIP_SKIP_SET_WAKE flag to avoid need of dummy >> set_wake() handler. >> >> Certain IRQCHIP's may not need to install the irq_set_wake() handler if >> the IRQCHIP_MASK_ON_SUSPEND flag is set. But but if it's not implemented, >> enable_irq_wake() will return an error. That needs the IRQCHIP to install >> an empty set_wake handler. >> >> Add an 'IRQCHIP_SKIP_SET_WAKE' flag so that IRQCHIP can inform the core >> irq code that irq_set_wake() handler is not necessary. >> >> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> Cc: Thomas Gleixner<tglx@linutronix.de> > > Queued to irq/core for 3.2 > Thanks a lot Thomas. Will update OMAP PM code based on above change. Regards Santosh
Santosh Shilimkar <santosh.shilimkar@ti.com> writes: > This patch adds the MPUSS OSWR (Open Switch Retention) support. The MPUSS > OSWR configuration is as below. > - CPUx L1 and logic lost, MPUSS logic lost, L2 memory is retained > > OMAP4460 onwards, MPUSS power domain doesn't support OFF state any more > anymore just like CORE power domain. The deepest state supported is OSWR. > On OMAP4430 secure devices too, MPUSS off mode can't be used because of > a bug which alters Ducati and Tesla states. Hence MPUSS off mode as an > independent state isn't supported on OMAP44XX devices. > > Ofcourse when MPUSS power domain transitions to OSWR along > with device off mode, it eventually hits off state since memory > contents are lost. > > Hence the MPUSS off mode independent state is not attempted without > device off mode. All the necessary infrastructure code for MPUSS > off mode is in place as part of this series. > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Kevin Hilman <khilman@ti.com> Compile failure for the !CONFIG_PM case: [...] > @@ -70,11 +71,18 @@ static inline int omap4_mpuss_init(void) > { > return 0; > } > - > +static inline u32 omap4_mpuss_read_prev_context_state(void) > +{ > + return 0; > +} added here > static inline int omap4_finish_suspend(unsigned long cpu_state) > {} > static inline void omap4_cpu_resume(void) > {} > +static inline u32 omap4_mpuss_read_prev_context_state(void) > +{ > + return 0; > +} same thing added here Kevin
Santosh Shilimkar <santosh.shilimkar@ti.com> writes: > This patch adds the CPU0 and CPU1 off mode support. CPUX close switch > retention (CSWR) is not supported by hardware design. > > The CPUx OFF mode isn't supported on OMAP4430 ES1.0 > > CPUx sleep code is common for hotplug, suspend and CPUilde. > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Kevin Hilman <khilman@ti.com> [...] > @@ -38,6 +39,11 @@ void __iomem *omap4_get_scu_base(void) > > void __cpuinit platform_secondary_init(unsigned int cpu) > { > + /* Enable NS access to SMP bit for this CPU on EMU/HS devices */ > + if (cpu_is_omap443x() && (omap_type() != OMAP2_DEVICE_TYPE_GP)) A comment here about why this is 443x specific would be helpful. I see a comment in omap4_cpu_resume() that seems to indicate that SMP bit is accessible on 446x NS devices, but repeating that commen here would help future readability. Kevin
Santosh Shilimkar <santosh.shilimkar@ti.com> writes: > Program non-boot CPUs to hit lowest supported power state > when it is off-lined using cpu hotplug framework. > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Kevin Hilman <khilman@ti.com> [...] > @@ -39,15 +43,15 @@ void platform_cpu_die(unsigned int cpu) > * we're ready for shutdown now, so do it > */ > if (omap_modify_auxcoreboot0(0x0, 0x200) != 0x0) > - printk(KERN_CRIT "Secure clear status failed\n"); > + pr_err("Secure clear status failed\n"); nit: pr_crit() exists if you want to keep the same level. Or did you lower the level on purpose? Kevin
On Tuesday 13 September 2011 02:42 AM, Kevin Hilman wrote: > Santosh Shilimkar<santosh.shilimkar@ti.com> writes: > >> Program non-boot CPUs to hit lowest supported power state >> when it is off-lined using cpu hotplug framework. >> >> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> Cc: Kevin Hilman<khilman@ti.com> > > [...] > >> @@ -39,15 +43,15 @@ void platform_cpu_die(unsigned int cpu) >> * we're ready for shutdown now, so do it >> */ >> if (omap_modify_auxcoreboot0(0x0, 0x200) != 0x0) >> - printk(KERN_CRIT "Secure clear status failed\n"); >> + pr_err("Secure clear status failed\n"); > > nit: pr_crit() exists if you want to keep the same level. Or did you > lower the level on purpose? > I lowered it on purpose since I didn't find it crtical. Regards Santosh
On Tuesday 13 September 2011 12:22 AM, Kevin Hilman wrote: > Santosh Shilimkar<santosh.shilimkar@ti.com> writes: > >> This patch adds the MPUSS OSWR (Open Switch Retention) support. The MPUSS >> OSWR configuration is as below. >> - CPUx L1 and logic lost, MPUSS logic lost, L2 memory is retained >> >> OMAP4460 onwards, MPUSS power domain doesn't support OFF state any more >> anymore just like CORE power domain. The deepest state supported is OSWR. >> On OMAP4430 secure devices too, MPUSS off mode can't be used because of >> a bug which alters Ducati and Tesla states. Hence MPUSS off mode as an >> independent state isn't supported on OMAP44XX devices. >> >> Ofcourse when MPUSS power domain transitions to OSWR along >> with device off mode, it eventually hits off state since memory >> contents are lost. >> >> Hence the MPUSS off mode independent state is not attempted without >> device off mode. All the necessary infrastructure code for MPUSS >> off mode is in place as part of this series. >> >> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> Cc: Kevin Hilman<khilman@ti.com> > > Compile failure for the !CONFIG_PM case: > > [...] > >> @@ -70,11 +71,18 @@ static inline int omap4_mpuss_init(void) >> { >> return 0; >> } >> - >> +static inline u32 omap4_mpuss_read_prev_context_state(void) >> +{ >> + return 0; >> +} > > added here > >> static inline int omap4_finish_suspend(unsigned long cpu_state) >> {} >> static inline void omap4_cpu_resume(void) >> {} >> +static inline u32 omap4_mpuss_read_prev_context_state(void) >> +{ >> + return 0; >> +} > > same thing added here > I noticed this yesterday and was planning to send a note on this patch. :( Have removed this already in the updated patches. Thanks for pointing it out. Regards Santosh
On Tuesday 13 September 2011 02:36 AM, Kevin Hilman wrote: > Santosh Shilimkar<santosh.shilimkar@ti.com> writes: > >> This patch adds the CPU0 and CPU1 off mode support. CPUX close switch >> retention (CSWR) is not supported by hardware design. >> >> The CPUx OFF mode isn't supported on OMAP4430 ES1.0 >> >> CPUx sleep code is common for hotplug, suspend and CPUilde. >> >> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> Cc: Kevin Hilman<khilman@ti.com> > > [...] > >> @@ -38,6 +39,11 @@ void __iomem *omap4_get_scu_base(void) >> >> void __cpuinit platform_secondary_init(unsigned int cpu) >> { >> + /* Enable NS access to SMP bit for this CPU on EMU/HS devices */ >> + if (cpu_is_omap443x()&& (omap_type() != OMAP2_DEVICE_TYPE_GP)) > > A comment here about why this is 443x specific would be helpful. > > I see a comment in omap4_cpu_resume() that seems to indicate that SMP > bit is accessible on 446x NS devices, but repeating that commen here > would help future readability. > Ok. Will add comments here too. Was just trying to save some lines :) Regards Santosh
Hi Santosh, On Tue, Sep 13, 2011 at 7:37 AM, Santosh <santosh.shilimkar@ti.com> wrote: > On Tuesday 13 September 2011 12:22 AM, Kevin Hilman wrote: >> >> Santosh Shilimkar<santosh.shilimkar@ti.com> writes: >> >>> This patch adds the MPUSS OSWR (Open Switch Retention) support. The MPUSS >>> OSWR configuration is as below. >>> - CPUx L1 and logic lost, MPUSS logic lost, L2 memory is retained >>> >>> OMAP4460 onwards, MPUSS power domain doesn't support OFF state any more >>> anymore just like CORE power domain. The deepest state supported is OSWR. >>> On OMAP4430 secure devices too, MPUSS off mode can't be used because of >>> a bug which alters Ducati and Tesla states. Hence MPUSS off mode as an >>> independent state isn't supported on OMAP44XX devices. >>> >>> Ofcourse when MPUSS power domain transitions to OSWR along >>> with device off mode, it eventually hits off state since memory >>> contents are lost. >>> >>> Hence the MPUSS off mode independent state is not attempted without >>> device off mode. All the necessary infrastructure code for MPUSS >>> off mode is in place as part of this series. >>> >>> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >>> Cc: Kevin Hilman<khilman@ti.com> >> >> Compile failure for the !CONFIG_PM case: >> >> [...] >> >>> @@ -70,11 +71,18 @@ static inline int omap4_mpuss_init(void) >>> { >>> return 0; >>> } >>> - >>> +static inline u32 omap4_mpuss_read_prev_context_state(void) >>> +{ >>> + return 0; >>> +} >> >> added here >> >>> static inline int omap4_finish_suspend(unsigned long cpu_state) >>> {} This one should return 0, as I already pointed out in the comments for [14/25]. Regards, Jean >>> static inline void omap4_cpu_resume(void) >>> {} >>> +static inline u32 omap4_mpuss_read_prev_context_state(void) >>> +{ >>> + return 0; >>> +} >> >> same thing added here >> > I noticed this yesterday and was planning to send a note on this > patch. :( > Have removed this already in the updated patches. > Thanks for pointing it out. > > Regards > Santosh > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Tuesday 13 September 2011 01:09 PM, Jean Pihet wrote: > Hi Santosh, > > On Tue, Sep 13, 2011 at 7:37 AM, Santosh<santosh.shilimkar@ti.com> wrote: >> On Tuesday 13 September 2011 12:22 AM, Kevin Hilman wrote: >>> [..] >>> >>>> static inline int omap4_finish_suspend(unsigned long cpu_state) >>>> {} > This one should return 0, as I already pointed out in the comments for [14/25]. > Taken care already. Regards Santosh
Santosh <santosh.shilimkar@ti.com> writes: > On Tuesday 13 September 2011 02:36 AM, Kevin Hilman wrote: >> Santosh Shilimkar<santosh.shilimkar@ti.com> writes: >> >>> This patch adds the CPU0 and CPU1 off mode support. CPUX close switch >>> retention (CSWR) is not supported by hardware design. >>> >>> The CPUx OFF mode isn't supported on OMAP4430 ES1.0 >>> >>> CPUx sleep code is common for hotplug, suspend and CPUilde. >>> >>> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >>> Cc: Kevin Hilman<khilman@ti.com> >> >> [...] >> >>> @@ -38,6 +39,11 @@ void __iomem *omap4_get_scu_base(void) >>> >>> void __cpuinit platform_secondary_init(unsigned int cpu) >>> { >>> + /* Enable NS access to SMP bit for this CPU on EMU/HS devices */ >>> + if (cpu_is_omap443x()&& (omap_type() != OMAP2_DEVICE_TYPE_GP)) >> >> A comment here about why this is 443x specific would be helpful. >> >> I see a comment in omap4_cpu_resume() that seems to indicate that SMP >> bit is accessible on 446x NS devices, but repeating that commen here >> would help future readability. >> > Ok. Will add comments here too. Was just trying to save some lines :) heh, this is a negative side-effect of people caring primarily about diffstat. :( One other comment on this patch. You need spaces around the '&&' above. Kevin
* Santosh Shilimkar <santosh.shilimkar@ti.com> [110904 06:22]: > On certain architectures, there might be a need to mark certain > addresses with strongly ordered memory attributes to avoid ordering > issues at the interconnect level. This is something Russell needs to look. You might want to also read the mailing list archives regarding the strongly ordered access. Basically it still won't guarantee that the write gets to the device, only a read back from the device in question guarantees that at the bus level. Regards, Tony
* Santosh Shilimkar <santosh.shilimkar@ti.com> [110904 06:22]: > On OMAP4 SOC intecronnects has many write buffers in the async bridges > and they can be drained only with stongly ordered accesses. This is not correct, strongly ordered access does not guarantee anything here. If it fixes issues, it's because it makes the writes to reach the device faster. Strongly ordered does not affect anything outside ARM, so the bus access won't change. The only real fix here is to do a read back of the device in question to guarantee the write got to the device. > There are two ports as below from MPU and both needs to be drained. > - MPU --> L3 T2ASYNC FIFO > - MPU --> DDR T2ASYNC FIFO > > Without the interconnect barriers, many issues have been observed > leading to system freeze, CPU deadlocks, random crashes with > register accesses, synchronization loss on initiators operating > on both interconnect port simultaneously. We had these issues for omap3 too. Adding a few read backs solved those kinds of issues. Regards, Tony
* Santosh Shilimkar <santosh.shilimkar@ti.com> [110904 06:23]: > OMAP WakeupGen is the interrupt controller extension used along > with ARM GIC to wake the CPU out from low power states on > external interrupts. > > The WakeupGen unit is responsible for generating wakeup event > from the incoming interrupts and enable bits. It is implemented > in MPU always ON power domain. During normal operation, > WakeupGen delivers external interrupts directly to the GIC. ... > + /* > + * Override GIC architecture specific functions to add > + * OMAP WakeupGen interrupt controller along with GIC > + */ > + gic_arch_extn.irq_mask = wakeupgen_mask; > + gic_arch_extn.irq_unmask = wakeupgen_unmask; > + gic_arch_extn.irq_set_wake = wakeupgen_set_wake; > + gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND; As I've commented before, there should not be any need to tweak the wakeupgen registers for each interrupt during the runtime. AFAIK the wakeupgen registers only need to be armed every time before entering idle. Regards, Tony
On Tue, Sep 13, 2011 at 11:03 PM, Kevin Hilman <khilman@ti.com> wrote: > Santosh <santosh.shilimkar@ti.com> writes: > >> On Tuesday 13 September 2011 02:36 AM, Kevin Hilman wrote: >>> Santosh Shilimkar<santosh.shilimkar@ti.com> writes: >>> >>>> This patch adds the CPU0 and CPU1 off mode support. CPUX close switch >>>> retention (CSWR) is not supported by hardware design. >>>> >>>> The CPUx OFF mode isn't supported on OMAP4430 ES1.0 >>>> >>>> CPUx sleep code is common for hotplug, suspend and CPUilde. >>>> >>>> Signed-off-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >>>> Cc: Kevin Hilman<khilman@ti.com> >>> >>> [...] >>> >>>> @@ -38,6 +39,11 @@ void __iomem *omap4_get_scu_base(void) >>>> >>>> void __cpuinit platform_secondary_init(unsigned int cpu) >>>> { >>>> + /* Enable NS access to SMP bit for this CPU on EMU/HS devices */ >>>> + if (cpu_is_omap443x()&& (omap_type() != OMAP2_DEVICE_TYPE_GP)) >>> >>> A comment here about why this is 443x specific would be helpful. >>> >>> I see a comment in omap4_cpu_resume() that seems to indicate that SMP >>> bit is accessible on 446x NS devices, but repeating that commen here >>> would help future readability. >>> >> Ok. Will add comments here too. Was just trying to save some lines :) > > heh, this is a negative side-effect of people caring primarily about > diffstat. :( > ya. > One other comment on this patch. You need spaces around the '&&' above. > The original patch is fine for that. Some replies has eaten that space. My Thunderbird email client is doing the same. :( Regards Santosh
Tony, On Wed, Sep 14, 2011 at 2:06 AM, Tony Lindgren <tony@atomide.com> wrote: > * Santosh Shilimkar <santosh.shilimkar@ti.com> [110904 06:23]: >> OMAP WakeupGen is the interrupt controller extension used along >> with ARM GIC to wake the CPU out from low power states on >> external interrupts. >> >> The WakeupGen unit is responsible for generating wakeup event >> from the incoming interrupts and enable bits. It is implemented >> in MPU always ON power domain. During normal operation, >> WakeupGen delivers external interrupts directly to the GIC. > ... > >> + /* >> + * Override GIC architecture specific functions to add >> + * OMAP WakeupGen interrupt controller along with GIC >> + */ >> + gic_arch_extn.irq_mask = wakeupgen_mask; >> + gic_arch_extn.irq_unmask = wakeupgen_unmask; >> + gic_arch_extn.irq_set_wake = wakeupgen_set_wake; >> + gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND; > > As I've commented before, there should not be any need to tweak > the wakeupgen registers for each interrupt during the runtime. > And I gave you all the reasons why it needs to be done this way. > AFAIK the wakeupgen registers only need to be armed every time > before entering idle. > No that doesn't work and it completely hacky approach. This problem is for all SOC's using A9 SMP and GIC and every soc has an architecture specific interrupt controller extension. And that was the reason the GIC arch_extn was proposed. It's just another IRQCHIP and works seamlessly being part of the framework. And it will also initialized with primary IRQCHIP( GIC). Regards Santosh
On Wed, Sep 14, 2011 at 1:53 AM, Tony Lindgren <tony@atomide.com> wrote: > * Santosh Shilimkar <santosh.shilimkar@ti.com> [110904 06:22]: >> On certain architectures, there might be a need to mark certain >> addresses with strongly ordered memory attributes to avoid ordering >> issues at the interconnect level. > > This is something Russell needs to look. > > You might want to also read the mailing list archives regarding the > strongly ordered access. > > Basically it still won't guarantee that the write gets to the > device, only a read back from the device in question guarantees > that at the bus level. > Russell has already seen this. I have sent this patches to Russell before adding them in the queue. This is different. There is a BUG in asynchronous bridges on OMAP44XX devices and that's the reason it' s needed. Regards Santosh
On Wed, Sep 14, 2011 at 1:57 AM, Tony Lindgren <tony@atomide.com> wrote: > * Santosh Shilimkar <santosh.shilimkar@ti.com> [110904 06:22]: >> On OMAP4 SOC intecronnects has many write buffers in the async bridges >> and they can be drained only with stongly ordered accesses. > > This is not correct, strongly ordered access does not guarantee > anything here. If it fixes issues, it's because it makes the writes > to reach the device faster. Strongly ordered does not affect anything > outside ARM, so the bus access won't change. > > The only real fix here is to do a read back of the device in question > to guarantee the write got to the device. > >> There are two ports as below from MPU and both needs to be drained. >> - MPU --> L3 T2ASYNC FIFO >> - MPU --> DDR T2ASYNC FIFO >> >> Without the interconnect barriers, many issues have been observed >> leading to system freeze, CPU deadlocks, random crashes with >> register accesses, synchronization loss on initiators operating >> on both interconnect port simultaneously. > > We had these issues for omap3 too. Adding a few read backs solved > those kinds of issues. > No. Don't mix things. OMAP3 behaviour is a interconnect level and it was single interconnect channel. The issue here is a BUG in the asynchronous bridges and OMAp4 has two separate channels at interconnect level form MPU side as mentioned above. Both of these patches I have passed it through Russell and Caralin before including them here. Regards Santosh
On Wednesday 14 September 2011 01:57 AM, Tony Lindgren wrote: > * Santosh Shilimkar<santosh.shilimkar@ti.com> [110904 06:22]: >> On OMAP4 SOC intecronnects has many write buffers in the async bridges >> and they can be drained only with stongly ordered accesses. > > This is not correct, strongly ordered access does not guarantee > anything here. If it fixes issues, it's because it makes the writes > to reach the device faster. Strongly ordered does not affect anything > outside ARM, so the bus access won't change. > What I said is the aync bridges WB and what is said is correct from MPU accesses point of view. It's not about faster or slower. With device memory the, writes can get stuck into write buffers where as with SO, the write buffers will be bypassed. The behaviours is limited to the MPU side async bridge boundary which is the problem. The statement is not for l3 and l4 interconnect which probably you mean. There is always a hardware signal to communicate CPU at async bridges to ensure that data is not stuck in these bridges before CPU clock/voltage is cut. Unfortunately we have a BUG on OMAP44XX devices and the dual channel makes it even worst since both pipes have the same BUG. So what we are doing is issuing SO write/read accesses on these pipes so that there is nothing stuck there before MPU hits low power states and also avoids any race conditions when both channels are used together by some initiators. The behaviour is validated at RTL level and there is no ambiguity about it. May be you have mistaken the L3 and L4 as the interconnect levels in this case. Regards Santosh
* Shilimkar, Santosh <santosh.shilimkar@ti.com> [110913 22:01]: > Tony, > On Wed, Sep 14, 2011 at 2:06 AM, Tony Lindgren <tony@atomide.com> wrote: > > * Santosh Shilimkar <santosh.shilimkar@ti.com> [110904 06:23]: > >> OMAP WakeupGen is the interrupt controller extension used along > >> with ARM GIC to wake the CPU out from low power states on > >> external interrupts. > >> > >> The WakeupGen unit is responsible for generating wakeup event > >> from the incoming interrupts and enable bits. It is implemented > >> in MPU always ON power domain. During normal operation, > >> WakeupGen delivers external interrupts directly to the GIC. > > ... > > > >> + /* > >> + * Override GIC architecture specific functions to add > >> + * OMAP WakeupGen interrupt controller along with GIC > >> + */ > >> + gic_arch_extn.irq_mask = wakeupgen_mask; > >> + gic_arch_extn.irq_unmask = wakeupgen_unmask; > >> + gic_arch_extn.irq_set_wake = wakeupgen_set_wake; > >> + gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND; > > > > As I've commented before, there should not be any need to tweak > > the wakeupgen registers for each interrupt during the runtime. > > > And I gave you all the reasons why it needs to be done this way. Hmm, I don't think you ever answered the main question: Why would you need to write the wakeupgen registers for every interrupt during the runtime instead of arming them only when entering idle? > > AFAIK the wakeupgen registers only need to be armed every time > > before entering idle. > > > No that doesn't work and it completely hacky approach. And how is writing the registers over and over again unnecessarily a non-hacky approach? > This problem is for all SOC's using A9 SMP and GIC and every soc > has an architecture specific interrupt controller extension. And that > was the reason the GIC arch_extn was proposed. > It's just another IRQCHIP and works seamlessly being part of the > framework. And it will also initialized with primary IRQCHIP( GIC). Sure, but I don't know if there is really a need for for that? Regards, Tony
On Wednesday 14 September 2011 08:51 PM, Tony Lindgren wrote: > * Shilimkar, Santosh<santosh.shilimkar@ti.com> [110913 22:01]: >> Tony, >> On Wed, Sep 14, 2011 at 2:06 AM, Tony Lindgren<tony@atomide.com> wrote: >>> * Santosh Shilimkar<santosh.shilimkar@ti.com> [110904 06:23]: >>>> OMAP WakeupGen is the interrupt controller extension used along >>>> with ARM GIC to wake the CPU out from low power states on >>>> external interrupts. >>>> >>>> The WakeupGen unit is responsible for generating wakeup event >>>> from the incoming interrupts and enable bits. It is implemented >>>> in MPU always ON power domain. During normal operation, >>>> WakeupGen delivers external interrupts directly to the GIC. >>> ... >>> >>>> + /* >>>> + * Override GIC architecture specific functions to add >>>> + * OMAP WakeupGen interrupt controller along with GIC >>>> + */ >>>> + gic_arch_extn.irq_mask = wakeupgen_mask; >>>> + gic_arch_extn.irq_unmask = wakeupgen_unmask; >>>> + gic_arch_extn.irq_set_wake = wakeupgen_set_wake; >>>> + gic_arch_extn.flags = IRQCHIP_MASK_ON_SUSPEND; >>> >>> As I've commented before, there should not be any need to tweak >>> the wakeupgen registers for each interrupt during the runtime. >>> >> And I gave you all the reasons why it needs to be done this way. > > Hmm, I don't think you ever answered the main question: > > Why would you need to write the wakeupgen registers for every > interrupt during the runtime instead of arming them only when > entering idle? > I thought I did that in last thread. Let me try again, First and foremost, I have to go with the approach here because MPUSS hardware team put a requirement that GIC and wakeupgen should always be kept in sync. If needed we can discuss this off-the list with Richard. Below is the extract from the veyron func specs. ------------------------------------- Version 1.6 of veyron spec has this. From page 95, paragraph 2 on version 1.6: "It is SW responsibility to program interrupt enabling/disabling coherently in the GIC and in the Wugen enable registers. That is, a given interrupt for a given CPU is either enable at both GIC and Wugen, or disable at both, but no mix." ------------------------------------- The way understand this IP is, even in normal scenario's every IRQ will come to wakeupgen and then it will pass that to GIC. CPU clock domains are kept under HW supervised always and they can enter inactive any time without WFI. Only wakeup gen can bring the CPU out of inactive state. That's requirement really lead to this design choice. Just to add all ARM SOC's using GIC has a gic extension interrupt controller and follow the same approach for the secondary IRQCHIPO. Below points as such don't matter after the strict hardware requirement. Still ..... Let's say, we ignore the hardware recommendation and try to do what you are suggesting. How will you know while entering in idle which IRQ's to be masked and which are to be unmasked ? The only way is to run though entire 1024 possible IRQ's from GIC and then check the state of each IRQ and set things accordingly. At GIC level, mask and unmask registers are different so you will end up reading those many registers. That also means you need to export some non-standard APIs from GIC driver. In system wide suspend, the core irq code, communicates the wakeup and non-wakeup functionality using standard mask/ unmask APIs when used with IRQCHIP_MASK_ON_SUSPEND. With what you are suggesting it won't work as desired. Because that information is only passed to the IRQ chips. So you will still need IRQCHIP and mask/unmask APIs. That can be done as part of set_wake() handler as well though. The wakeupgen is within CPU cluster and the accesses to it are not as expensive as like accessing 32 K timer or GP timer. By making the wakeupgen as an IRQCHIP, we meet the hardware requirement and also make use of this IP properly for the desired functionality using standard IRQCHIP interfaces No need of non-standard hacking. It also avoid platform code monkeing with common GIC code and irq subsystem to hack the stuff. Btw, not exactly related here, but because of common code consolidation, we need to actually use GIC common save/restore hooks, even though OMAP has very optimal software save and hardware restore mechanism for GIC. Hope this email summarise all previous multiple discussions in one place. Regards Santosh
* Santosh <santosh.shilimkar@ti.com> [110914 09:16]: > > First and foremost, I have to go with the approach here because MPUSS > hardware team put a requirement that GIC and wakeupgen should always be > kept in sync. If needed we can discuss this off-the list with Richard. > > Below is the extract from the veyron func specs. > ------------------------------------- > Version 1.6 of veyron spec has this. > > From page 95, paragraph 2 on version 1.6: > > "It is SW responsibility to program interrupt enabling/disabling > coherently in the GIC and in the Wugen enable registers. That is, a > given interrupt for a given CPU is either enable at both GIC and Wugen, > or disable at both, but no mix." > ------------------------------------- > > The way understand this IP is, even in normal scenario's every IRQ > will come to wakeupgen and then it will pass that to GIC. CPU clock > domains are kept under HW supervised always and they can enter inactive > any time without WFI. Only wakeup gen can bring the CPU out of inactive > state. > > That's requirement really lead to this design choice. Just to add > all ARM SOC's using GIC has a gic extension interrupt controller and > follow the same approach for the secondary IRQCHIPO. Thanks for the clarification. It seems to me the spec is most likely wrong as we've had the GIC working for over two years now without doing anything with the wakeup gen registers :) Of course the wakeup events probably don't work currently, but the GIC interrupts certainly do. So most likely there's no need to continuously syncronize the wakeup gen registers with the GIC registers. > Below points as such don't matter after the strict hardware > requirement. Still ..... I think the issue is that you're assuming the spec is correct, which does not seem to be the case here. > Let's say, we ignore the hardware recommendation and try > to do what you are suggesting. > > How will you know while entering in idle which IRQ's to be > masked and which are to be unmasked ? > The only way is to run though entire 1024 possible IRQ's from GIC > and then check the state of each IRQ and set things accordingly. > At GIC level, mask and unmask registers are different so you will > end up reading those many registers. That also means you need to > export some non-standard APIs from GIC driver. When entering idle, we have plenty of time to do that. Sounds like that could be easily implemented in a generic way. > In system wide suspend, the core irq code, communicates > the wakeup and non-wakeup functionality using standard mask/ > unmask APIs when used with IRQCHIP_MASK_ON_SUSPEND. > With what you are suggesting it won't work > as desired. Because that information is only passed > to the IRQ chips. So you will still need IRQCHIP and > mask/unmask APIs. That can be done as part of set_wake() > handler as well though. > > The wakeupgen is within CPU cluster and the accesses > to it are not as expensive as like accessing 32 K timer or > GP timer. Sure, but it still causes unnecesary writes for every interrupt. There's no technical reason to do that. > By making the wakeupgen as an IRQCHIP, we meet the hardware > requirement and also make use of this IP properly for the > desired functionality using standard IRQCHIP interfaces > No need of non-standard hacking. Well it seems the "hardware requirement" is based on a buggy spec considering things are currently working. > It also avoid platform code monkeing with common GIC code > and irq subsystem to hack the stuff. What I'm suggesting can be implemented in a generic way. > Btw, not exactly related here, but because of common code > consolidation, we need to actually use GIC common > save/restore hooks, even though OMAP has very > optimal software save and hardware restore mechanism > for GIC. > > Hope this email summarise all previous multiple discussions > in one place. Thanks, but unfortunately it does not. To me it still seems this is the wrong approach for the wakeup triggers. Regards, Tony
On Wednesday 14 September 2011 10:38 PM, Tony Lindgren wrote: > * Santosh<santosh.shilimkar@ti.com> [110914 09:16]: >> >> First and foremost, I have to go with the approach here because MPUSS >> hardware team put a requirement that GIC and wakeupgen should always be >> kept in sync. If needed we can discuss this off-the list with Richard. >> >> Below is the extract from the veyron func specs. >> ------------------------------------- >> Version 1.6 of veyron spec has this. >> >> From page 95, paragraph 2 on version 1.6: >> >> "It is SW responsibility to program interrupt enabling/disabling >> coherently in the GIC and in the Wugen enable registers. That is, a >> given interrupt for a given CPU is either enable at both GIC and Wugen, >> or disable at both, but no mix." >> ------------------------------------- >> >> The way understand this IP is, even in normal scenario's every IRQ >> will come to wakeupgen and then it will pass that to GIC. CPU clock >> domains are kept under HW supervised always and they can enter inactive >> any time without WFI. Only wakeup gen can bring the CPU out of inactive >> state. >> >> That's requirement really lead to this design choice. Just to add >> all ARM SOC's using GIC has a gic extension interrupt controller and >> follow the same approach for the secondary IRQCHIPO. > > Thanks for the clarification. It seems to me the spec is most likely > wrong as we've had the GIC working for over two years now without > doing anything with the wakeup gen registers :) > It's working because CPU clockdomain are never put under HW supervised mode and they are kept in force wakeup. Clock-domain never idles on mainline code. PM series will put the clock-domains under HW supervison as needed to achieve any low power states and then all sorts of corner cases will come out. Regards Santosh
* Santosh <santosh.shilimkar@ti.com> [110914 09:40]: > On Wednesday 14 September 2011 10:38 PM, Tony Lindgren wrote: > >* Santosh<santosh.shilimkar@ti.com> [110914 09:16]: > > > >Thanks for the clarification. It seems to me the spec is most likely > >wrong as we've had the GIC working for over two years now without > >doing anything with the wakeup gen registers :) > > > It's working because CPU clockdomain are never put under HW > supervised mode and they are kept in force wakeup. Clock-domain > never idles on mainline code. PM series will put the clock-domains > under HW supervison as needed to achieve any low power states and > then all sorts of corner cases will come out. But again the wakeup gen triggers only do something when hitting idle. There should be no use for them during runtime, right? Tony
On Wednesday 14 September 2011 10:48 PM, Tony Lindgren wrote: > * Santosh<santosh.shilimkar@ti.com> [110914 09:40]: >> On Wednesday 14 September 2011 10:38 PM, Tony Lindgren wrote: >>> * Santosh<santosh.shilimkar@ti.com> [110914 09:16]: >>> >>> Thanks for the clarification. It seems to me the spec is most likely >>> wrong as we've had the GIC working for over two years now without >>> doing anything with the wakeup gen registers :) >>> >> It's working because CPU clockdomain are never put under HW >> supervised mode and they are kept in force wakeup. Clock-domain >> never idles on mainline code. PM series will put the clock-domains >> under HW supervison as needed to achieve any low power states and >> then all sorts of corner cases will come out. > > But again the wakeup gen triggers only do something when hitting > idle. There should be no use for them during runtime, right? > You missed my point in the description. Clockdomain inactive doesn't depend on idle or WFI execution. Under HW supervison CPU clock domain can get into inactive when CPU is stalled and waiting for a read response from slow interconnect. Regards Santosh
On Wednesday 14 September 2011 10:48 PM, Tony Lindgren wrote: > * Santosh<santosh.shilimkar@ti.com> [110914 09:40]: >> On Wednesday 14 September 2011 10:38 PM, Tony Lindgren wrote: >>> * Santosh<santosh.shilimkar@ti.com> [110914 09:16]: >>> >>> Thanks for the clarification. It seems to me the spec is most likely >>> wrong as we've had the GIC working for over two years now without >>> doing anything with the wakeup gen registers :) >>> >> It's working because CPU clockdomain are never put under HW >> supervised mode and they are kept in force wakeup. Clock-domain >> never idles on mainline code. PM series will put the clock-domains >> under HW supervison as needed to achieve any low power states and >> then all sorts of corner cases will come out. > > But again the wakeup gen triggers only do something when hitting > idle. There should be no use for them during runtime, right? > You missed my point in the description. Clockdomain inactive doesn't depend on idle or WFI execution. Under HW supervison CPU clock domain can get into inactive when CPU is stalled and waiting for a read response from slow interconnect. One thing for sure. Designers has chosen a wrong name to this IP. Wakeugen apears like needed only for low power wakeup which not seems to be entirely correct as per specs Regards Santosh
* Santosh <santosh.shilimkar@ti.com> [110914 09:49]: > On Wednesday 14 September 2011 10:48 PM, Tony Lindgren wrote: > >* Santosh<santosh.shilimkar@ti.com> [110914 09:40]: > >>On Wednesday 14 September 2011 10:38 PM, Tony Lindgren wrote: > >>>* Santosh<santosh.shilimkar@ti.com> [110914 09:16]: > >>> > >>>Thanks for the clarification. It seems to me the spec is most likely > >>>wrong as we've had the GIC working for over two years now without > >>>doing anything with the wakeup gen registers :) > >>> > >>It's working because CPU clockdomain are never put under HW > >>supervised mode and they are kept in force wakeup. Clock-domain > >>never idles on mainline code. PM series will put the clock-domains > >>under HW supervison as needed to achieve any low power states and > >>then all sorts of corner cases will come out. > > > >But again the wakeup gen triggers only do something when hitting > >idle. There should be no use for them during runtime, right? > > > You missed my point in the description. Clockdomain inactive > doesn't depend on idle or WFI execution. Under HW supervison > CPU clock domain can get into inactive when CPU is stalled and > waiting for a read response from slow interconnect. Ah OK. If it's needed during runtime too then that explains why the registers need to be kept in sync. > One thing for sure. Designers has chosen a wrong name to this > IP. Wakeugen apears like needed only for low power wakeup which > not seems to be entirely correct as per specs Yes it's not obvious reading the TRM either. Maybe add some comment about that to the patch? Regards, Tony
Santosh Shilimkar <santosh.shilimkar@ti.com> writes: > This patch adds MPUSS(MPU Sub System) power domain > CSWR(Close Switch Retention) support to system wide suspend. > For both MPUSS RET support, CPUs are programmed to OFF state. is 'both' in the wrong place in this sentence? I think you meant: For MPUSS retention support, both CPUs are programmed to OFF state. > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Kevin Hilman <khilman@ti.com> > --- > arch/arm/mach-omap2/omap-mpuss-lowpower.c | 16 +++++++ > arch/arm/mach-omap2/pm44xx.c | 71 +++++++++++++++++++++++++++-- > 2 files changed, 82 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c > index 9d68abf..9f632fe 100644 > --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c > +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c > @@ -66,6 +66,7 @@ struct omap4_cpu_pm_info { > }; > > static DEFINE_PER_CPU(struct omap4_cpu_pm_info, omap4_pm_info); > +static struct powerdomain *mpuss_pd; > > /* > * Program the wakeup routine address for the CPU0 and CPU1 > @@ -140,6 +141,13 @@ static void scu_pwrst_prepare(unsigned int cpu_id, unsigned int cpu_state) > * of OMAP4 MPUSS subsystem > * @cpu : CPU ID > * @power_state: Low power state. > + * > + * MPUSS states for the context save: > + * save_state = > + * 0 - Nothing lost and no need to save: MPUSS INACTIVE > + * 1 - CPUx L1 and logic lost: MPUSS CSWR > + * 2 - CPUx L1 and logic lost + GIC lost: MPUSS OSWR > + * 3 - CPUx L1 and logic lost + GIC + L2 lost: DEVICE OFF > */ > int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state) > { > @@ -169,6 +177,7 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state) > return -ENXIO; > } > > + pwrdm_clear_all_prev_pwrst(mpuss_pd); > clear_cpu_prev_pwrst(cpu); > set_cpu_next_pwrst(cpu, power_state); > set_cpu_wakeup_addr(cpu, virt_to_phys(omap4_cpu_resume)); > @@ -270,6 +279,13 @@ int __init omap4_mpuss_init(void) > /* Initialise CPU1 power domain state to ON */ > pwrdm_set_next_pwrst(pm_info->pwrdm, PWRDM_POWER_ON); > > + mpuss_pd = pwrdm_lookup("mpu_pwrdm"); > + if (!mpuss_pd) { > + pr_err("Failed to lookup MPUSS power domain\n"); > + return -ENODEV; > + } > + pwrdm_clear_all_prev_pwrst(mpuss_pd); > + > /* Save device type on scratchpad for low level code to use */ > if (omap_type() != OMAP2_DEVICE_TYPE_GP) > __raw_writel(1, sar_base + OMAP_TYPE_OFFSET); > diff --git a/arch/arm/mach-omap2/pm44xx.c b/arch/arm/mach-omap2/pm44xx.c > index 4f39de5..63e8f9b 100644 > --- a/arch/arm/mach-omap2/pm44xx.c > +++ b/arch/arm/mach-omap2/pm44xx.c > @@ -1,8 +1,9 @@ > /* > * OMAP4 Power Management Routines > * > - * Copyright (C) 2010 Texas Instruments, Inc. > + * Copyright (C) 2010-2011 Texas Instruments, Inc. > * Rajendra Nayak <rnayak@ti.com> > + * Santosh Shilimkar <santosh.shilimkar@ti.com> > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 as > @@ -16,9 +17,11 @@ > #include <linux/err.h> > #include <linux/slab.h> > > +#include <mach/omap4-common.h> > + > #include "powerdomain.h" > #include "clockdomain.h" > -#include <mach/omap4-common.h> > +#include "pm.h" > > struct power_state { > struct powerdomain *pwrdm; > @@ -34,7 +37,48 @@ static LIST_HEAD(pwrst_list); > #ifdef CONFIG_SUSPEND > static int omap4_pm_suspend(void) > { > - omap_do_wfi(); > + struct power_state *pwrst; > + int state, ret = 0; > + u32 cpu_id = smp_processor_id(); > + > + /* Save current powerdomain state */ > + list_for_each_entry(pwrst, &pwrst_list, node) { > + pwrst->saved_state = pwrdm_read_next_pwrst(pwrst->pwrdm); > + } > + > + /* Set targeted power domain states by suspend */ > + list_for_each_entry(pwrst, &pwrst_list, node) { > + omap_set_pwrdm_state(pwrst->pwrdm, pwrst->next_state); > + } > + > + /* > + * For MPUSS to hit power domain retention(CSWR or OSWR), > + * CPU0 and CPU1 power domain needs to be in OFF or DORMANT s/domain needs/domains need/ > + * state. For MPUSS to reach off-mode. CPU0 and CPU1 power domain > + * should be in off state. nit: please be consistent with naming of power states (e.g. OFF vs. off) > + * Only master CPU followes suspend path. All other CPUs follow > + * cpu-hotplug path in system wide suspend. On OMAP4, CPU power > + * domain CSWR is not supported by hardware. I think this sentence belongs a little earlier. E.g. something like ...CPU0 and CPU1 power domains need to be in OFF or DORMANT state, since CPU power domain CSWR is not supported by hardware. > + * More details can be found in OMAP4430 TRM section 4.3.4.2. > + */ > + omap4_enter_lowpower(cpu_id, PWRDM_POWER_OFF); > + > + /* Restore next powerdomain state */ > + list_for_each_entry(pwrst, &pwrst_list, node) { > + state = pwrdm_read_prev_pwrst(pwrst->pwrdm); > + if (state > pwrst->next_state) { > + pr_info("Powerdomain (%s) didn't enter " > + "target state %d\n", > + pwrst->pwrdm->name, pwrst->next_state); > + ret = -1; > + } > + omap_set_pwrdm_state(pwrst->pwrdm, pwrst->saved_state); > + } > + if (ret) > + pr_err("Could not enter target state in pm_suspend\n"); Without more details, this isn't terribly useful. I'd suggest just making the per-state one above pr_err(). > + else > + pr_err("Successfully put all powerdomains to target state\n"); and this one pr_info. > return 0; > } > > @@ -97,14 +141,31 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused) > if (!pwrdm->pwrsts) > return 0; > > + /* > + * Skip CPU0 and CPU1 power domains. CPU1 is programmed > + * through hotplug path and CPU0 explicitly programmed > + * further down in the code path > + */ > + if ((!strcmp(pwrdm->name, "cpu0_pwrdm")) || > + (!strcmp(pwrdm->name, "cpu1_pwrdm"))) or just one compare using strncmp(pwrdm->name, "cpu", 3) > + return 0; > + > + /* > + * FIXME: Remove this check when core retention is supported > + * Only MPUSS power domain is added in the list. > + */ > + if (strcmp(pwrdm->name, "mpu_pwrdm")) > + return 0; > + > pwrst = kmalloc(sizeof(struct power_state), GFP_ATOMIC); > if (!pwrst) > return -ENOMEM; > + > pwrst->pwrdm = pwrdm; > - pwrst->next_state = PWRDM_POWER_ON; > + pwrst->next_state = PWRDM_POWER_RET; > list_add(&pwrst->node, &pwrst_list); > > - return pwrdm_set_next_pwrst(pwrst->pwrdm, pwrst->next_state); > + return omap_set_pwrdm_state(pwrst->pwrdm, pwrst->next_state); > } > > /** Kevin
On Thursday 15 September 2011 12:34 AM, Tony Lindgren wrote: > * Santosh <santosh.shilimkar@ti.com> [110914 09:49]: >> On Wednesday 14 September 2011 10:48 PM, Tony Lindgren wrote: >>> * Santosh<santosh.shilimkar@ti.com> [110914 09:40]: >>>> On Wednesday 14 September 2011 10:38 PM, Tony Lindgren wrote: >>>>> * Santosh<santosh.shilimkar@ti.com> [110914 09:16]: >>>>> >>>>> Thanks for the clarification. It seems to me the spec is most likely >>>>> wrong as we've had the GIC working for over two years now without >>>>> doing anything with the wakeup gen registers :) >>>>> >>>> It's working because CPU clockdomain are never put under HW >>>> supervised mode and they are kept in force wakeup. Clock-domain >>>> never idles on mainline code. PM series will put the clock-domains >>>> under HW supervison as needed to achieve any low power states and >>>> then all sorts of corner cases will come out. >>> >>> But again the wakeup gen triggers only do something when hitting >>> idle. There should be no use for them during runtime, right? >>> >> You missed my point in the description. Clockdomain inactive >> doesn't depend on idle or WFI execution. Under HW supervison >> CPU clock domain can get into inactive when CPU is stalled and >> waiting for a read response from slow interconnect. > > Ah OK. If it's needed during runtime too then that explains why > the registers need to be kept in sync. > >> One thing for sure. Designers has chosen a wrong name to this >> IP. Wakeugen apears like needed only for low power wakeup which >> not seems to be entirely correct as per specs > > Yes it's not obvious reading the TRM either. Maybe add some > comment about that to the patch? > You are right. Documentation isn't clear about this. Will add the above point in change log. btw, thanks for the good discussion on this topic. Regards Santosh
On Thursday 15 September 2011 05:57 AM, Kevin Hilman wrote: > Santosh Shilimkar <santosh.shilimkar@ti.com> writes: > >> This patch adds MPUSS(MPU Sub System) power domain >> CSWR(Close Switch Retention) support to system wide suspend. >> For both MPUSS RET support, CPUs are programmed to OFF state. > > is 'both' in the wrong place in this sentence? I think you meant: > You are right. > For MPUSS retention support, both CPUs are programmed to OFF state. > >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >> Cc: Kevin Hilman <khilman@ti.com> >> --- >> arch/arm/mach-omap2/omap-mpuss-lowpower.c | 16 +++++++ >> arch/arm/mach-omap2/pm44xx.c | 71 +++++++++++++++++++++++++++-- >> 2 files changed, 82 insertions(+), 5 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c >> index 9d68abf..9f632fe 100644 >> --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c >> +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c >> @@ -66,6 +66,7 @@ struct omap4_cpu_pm_info { >> }; >> >> static DEFINE_PER_CPU(struct omap4_cpu_pm_info, omap4_pm_info); >> +static struct powerdomain *mpuss_pd; >> >> /* >> * Program the wakeup routine address for the CPU0 and CPU1 >> @@ -140,6 +141,13 @@ static void scu_pwrst_prepare(unsigned int cpu_id, unsigned int cpu_state) >> * of OMAP4 MPUSS subsystem >> * @cpu : CPU ID >> * @power_state: Low power state. >> + * >> + * MPUSS states for the context save: >> + * save_state = >> + * 0 - Nothing lost and no need to save: MPUSS INACTIVE >> + * 1 - CPUx L1 and logic lost: MPUSS CSWR >> + * 2 - CPUx L1 and logic lost + GIC lost: MPUSS OSWR >> + * 3 - CPUx L1 and logic lost + GIC + L2 lost: DEVICE OFF >> */ >> int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state) >> { >> @@ -169,6 +177,7 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state) >> return -ENXIO; >> } >> >> + pwrdm_clear_all_prev_pwrst(mpuss_pd); >> clear_cpu_prev_pwrst(cpu); >> set_cpu_next_pwrst(cpu, power_state); >> set_cpu_wakeup_addr(cpu, virt_to_phys(omap4_cpu_resume)); >> @@ -270,6 +279,13 @@ int __init omap4_mpuss_init(void) >> /* Initialise CPU1 power domain state to ON */ >> pwrdm_set_next_pwrst(pm_info->pwrdm, PWRDM_POWER_ON); >> >> + mpuss_pd = pwrdm_lookup("mpu_pwrdm"); >> + if (!mpuss_pd) { >> + pr_err("Failed to lookup MPUSS power domain\n"); >> + return -ENODEV; >> + } >> + pwrdm_clear_all_prev_pwrst(mpuss_pd); >> + >> /* Save device type on scratchpad for low level code to use */ >> if (omap_type() != OMAP2_DEVICE_TYPE_GP) >> __raw_writel(1, sar_base + OMAP_TYPE_OFFSET); >> diff --git a/arch/arm/mach-omap2/pm44xx.c b/arch/arm/mach-omap2/pm44xx.c >> index 4f39de5..63e8f9b 100644 >> --- a/arch/arm/mach-omap2/pm44xx.c >> +++ b/arch/arm/mach-omap2/pm44xx.c >> @@ -1,8 +1,9 @@ >> /* >> * OMAP4 Power Management Routines >> * >> - * Copyright (C) 2010 Texas Instruments, Inc. >> + * Copyright (C) 2010-2011 Texas Instruments, Inc. >> * Rajendra Nayak <rnayak@ti.com> >> + * Santosh Shilimkar <santosh.shilimkar@ti.com> >> * >> * This program is free software; you can redistribute it and/or modify >> * it under the terms of the GNU General Public License version 2 as >> @@ -16,9 +17,11 @@ >> #include <linux/err.h> >> #include <linux/slab.h> >> >> +#include <mach/omap4-common.h> >> + >> #include "powerdomain.h" >> #include "clockdomain.h" >> -#include <mach/omap4-common.h> >> +#include "pm.h" >> >> struct power_state { >> struct powerdomain *pwrdm; >> @@ -34,7 +37,48 @@ static LIST_HEAD(pwrst_list); >> #ifdef CONFIG_SUSPEND >> static int omap4_pm_suspend(void) >> { >> - omap_do_wfi(); >> + struct power_state *pwrst; >> + int state, ret = 0; >> + u32 cpu_id = smp_processor_id(); >> + >> + /* Save current powerdomain state */ >> + list_for_each_entry(pwrst, &pwrst_list, node) { >> + pwrst->saved_state = pwrdm_read_next_pwrst(pwrst->pwrdm); >> + } >> + >> + /* Set targeted power domain states by suspend */ >> + list_for_each_entry(pwrst, &pwrst_list, node) { >> + omap_set_pwrdm_state(pwrst->pwrdm, pwrst->next_state); >> + } >> + >> + /* >> + * For MPUSS to hit power domain retention(CSWR or OSWR), >> + * CPU0 and CPU1 power domain needs to be in OFF or DORMANT > > s/domain needs/domains need/ > ok >> + * state. For MPUSS to reach off-mode. CPU0 and CPU1 power domain >> + * should be in off state. > > nit: please be consistent with naming of power states (e.g. OFF vs. off) > ok. >> + * Only master CPU followes suspend path. All other CPUs follow >> + * cpu-hotplug path in system wide suspend. On OMAP4, CPU power >> + * domain CSWR is not supported by hardware. > > I think this sentence belongs a little earlier. E.g. something like > ...CPU0 and CPU1 power domains need to be in OFF or DORMANT state, since > CPU power domain CSWR is not supported by hardware. > Looks better. >> + * More details can be found in OMAP4430 TRM section 4.3.4.2. >> + */ >> + omap4_enter_lowpower(cpu_id, PWRDM_POWER_OFF); >> + >> + /* Restore next powerdomain state */ >> + list_for_each_entry(pwrst, &pwrst_list, node) { >> + state = pwrdm_read_prev_pwrst(pwrst->pwrdm); >> + if (state > pwrst->next_state) { >> + pr_info("Powerdomain (%s) didn't enter " >> + "target state %d\n", >> + pwrst->pwrdm->name, pwrst->next_state); >> + ret = -1; >> + }0001 >> + omap_set_pwrdm_state(pwrst->pwrdm, pwrst->saved_state); >> + } >> + if (ret) >> + pr_err("Could not enter target state in pm_suspend\n"); > > Without more details, this isn't terribly useful. I'd suggest just > making the per-state one above pr_err(). > You mean pr_crit ? >> + else >> + pr_err("Successfully put all powerdomains to target state\n"); > > and this one pr_info. > >> return 0; >> } >> >> @@ -97,14 +141,31 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused) >> if (!pwrdm->pwrsts) >> return 0; >> >> + /* >> + * Skip CPU0 and CPU1 power domains. CPU1 is programmed >> + * through hotplug path and CPU0 explicitly programmed >> + * further down in the code path >> + */ >> + if ((!strcmp(pwrdm->name, "cpu0_pwrdm")) || >> + (!strcmp(pwrdm->name, "cpu1_pwrdm"))) > > or just one compare using strncmp(pwrdm->name, "cpu", 3) > Let me check this. Regards Santosh
Hi Santosh, On 9/14/2011 7:22 PM, Shilimkar, Santosh wrote: > On Wednesday 14 September 2011 10:48 PM, Tony Lindgren wrote: >> * Santosh<santosh.shilimkar@ti.com> [110914 09:40]: >>> On Wednesday 14 September 2011 10:38 PM, Tony Lindgren wrote: >>>> * Santosh<santosh.shilimkar@ti.com> [110914 09:16]: >>>> >>>> Thanks for the clarification. It seems to me the spec is most likely >>>> wrong as we've had the GIC working for over two years now without >>>> doing anything with the wakeup gen registers :) >>>> >>> It's working because CPU clockdomain are never put under HW >>> supervised mode and they are kept in force wakeup. Clock-domain >>> never idles on mainline code. PM series will put the clock-domains >>> under HW supervison as needed to achieve any low power states and >>> then all sorts of corner cases will come out. >> >> But again the wakeup gen triggers only do something when hitting >> idle. There should be no use for them during runtime, right? >> > You missed my point in the description. Clockdomain inactive > doesn't depend on idle or WFI execution. Under HW supervison > CPU clock domain can get into inactive when CPU is stalled and > waiting for a read response from slow interconnect. I'm a little bit confused by that statement... I'm wondering how the PRCM can gate the CPU/MPU clock and re-enable it if the MPU is gated? What kind of event can wakeup the CPU in case of CPU stalled? The spec seems to indicate that wakeupgen is needed only if CPU are in WFI. It is even written: "CPUx will change power state only when StandbyWFI is asserted.". Even a WFE is not supposed to trigger a standby. How can the CPU be inactive at clock domain level without a WFI? Regards, Benoit
On Thu, Sep 15, 2011 at 3:06 PM, Cousson, Benoit <b-cousson@ti.com> wrote: > Hi Santosh, > > On 9/14/2011 7:22 PM, Shilimkar, Santosh wrote: >> >> On Wednesday 14 September 2011 10:48 PM, Tony Lindgren wrote: >>> >>> * Santosh<santosh.shilimkar@ti.com> [110914 09:40]: >>>> >>>> On Wednesday 14 September 2011 10:38 PM, Tony Lindgren wrote: >>>>> >>>>> * Santosh<santosh.shilimkar@ti.com> [110914 09:16]: >>>>> >>>>> Thanks for the clarification. It seems to me the spec is most likely >>>>> wrong as we've had the GIC working for over two years now without >>>>> doing anything with the wakeup gen registers :) >>>>> >>>> It's working because CPU clockdomain are never put under HW >>>> supervised mode and they are kept in force wakeup. Clock-domain >>>> never idles on mainline code. PM series will put the clock-domains >>>> under HW supervison as needed to achieve any low power states and >>>> then all sorts of corner cases will come out. >>> >>> But again the wakeup gen triggers only do something when hitting >>> idle. There should be no use for them during runtime, right? >>> >> You missed my point in the description. Clockdomain inactive >> doesn't depend on idle or WFI execution. Under HW supervison >> CPU clock domain can get into inactive when CPU is stalled and >> waiting for a read response from slow interconnect. > > I'm a little bit confused by that statement... > > I'm wondering how the PRCM can gate the CPU/MPU clock and re-enable it if > the MPU is gated? What kind of event can wakeup the CPU in case of CPU > stalled? > > The spec seems to indicate that wakeupgen is needed only if CPU are in WFI. > It is even written: "CPUx will change power state only when StandbyWFI is > asserted.". Even a WFE is not supposed to trigger a standby. > How can the CPU be inactive at clock domain level without a WFI? > I mean only CPU clock domain and not the power domain inactive and local CPU clock can be gated without WFI when clock domain is kept under hardware supervision. But I agree with you that, for the stalled case, wakeupgen can't generate any event and it will LPRM state-machine which take care of un-gating the clock for that local CPU. I have been discussing today morning with design IP team on the restriction in the functional specs and they said they will check and comeback. For now, I would like to go with what specs says. By next merge window, am sure we will be clear inputs from IP team on this and if it happens that the requirement needs to be changed and we need not do mask/unmask in non-low power scenario, we can start looking at other aspects as Tony suggested. Regards Santosh
> From: Shilimkar, Santosh > Sent: Thursday, September 15, 2011 7:02 AM > >> You missed my point in the description. Clockdomain inactive > >> doesn't depend on idle or WFI execution. Under HW supervison > >> CPU clock domain can get into inactive when CPU is stalled and > >> waiting for a read response from slow interconnect. My understanding is MPSS global clock domain can not gate unless each cpu is in wfi and other conditions are met. Other conditions are a wugen ack that there are no irqs and ocp-bridge logic giving its ok. What kind of local clock gating is there per cpu or sub-clock domains is not as clear. The mpu's pivot on individual wfi while other blocks in mpuss have other considerations. The spec is very clear about coherent programmation of wugen and gic. Like being discussed. Its examples are about how wugen used I've heard about are around transition in and out of standbys at wfi. Wugen will process all irqs before giving ack to sleep, it also has ability to fail sleep if new irqs come in before final idle. * It may be possible depending on clarifications to handle near wfi, but you might have to do it for -every- cpu's wfi unless you can coordinate who the last one is. Doing it as you go with gic would avoid redoing work over and over at the many wfi calls. This extra work costs in latency and power. Doing housekeeping along with gic seems better from a run time perspective. Regards, Richard W.
Santosh <santosh.shilimkar@ti.com> writes: > On Wednesday 14 September 2011 01:57 AM, Tony Lindgren wrote: >> * Santosh Shilimkar<santosh.shilimkar@ti.com> [110904 06:22]: >>> On OMAP4 SOC intecronnects has many write buffers in the async bridges >>> and they can be drained only with stongly ordered accesses. >> >> This is not correct, strongly ordered access does not guarantee >> anything here. If it fixes issues, it's because it makes the writes >> to reach the device faster. Strongly ordered does not affect anything >> outside ARM, so the bus access won't change. >> > What I said is the aync bridges WB and what is said is correct > from MPU accesses point of view. > > It's not about faster or slower. With device memory the, writes > can get stuck into write buffers where as with SO, the write buffers > will be bypassed. > > The behaviours is limited to the MPU side async bridge boundary which > is the problem. The statement is not for l3 and l4 interconnect which > probably you mean. > > There is always a hardware signal to communicate CPU at async bridges > to ensure that data is not stuck in these bridges before CPU > clock/voltage is cut. Unfortunately we have a BUG on OMAP44XX devices > and the dual channel makes it even worst since both pipes have the > same BUG. So what we are doing is issuing SO write/read accesses > on these pipes so that there is nothing stuck there before MPU > hits low power states and also avoids any race conditions when > both channels are used together by some initiators. The behaviour > is validated at RTL level and there is no ambiguity about it. > > May be you have mistaken the L3 and L4 as the interconnect levels > in this case. Sounds to me like the changelog needs to be a bit more verbose. Remember, we're all probably going to forget the gory details of this in a few months and want to be able to go back to the code w/changelog to refresh our memories. Thanks, Kevin
On Thu, Sep 15, 2011 at 10:47 PM, Kevin Hilman <khilman@ti.com> wrote: > Santosh <santosh.shilimkar@ti.com> writes: > >> On Wednesday 14 September 2011 01:57 AM, Tony Lindgren wrote: >>> * Santosh Shilimkar<santosh.shilimkar@ti.com> [110904 06:22]: >>>> On OMAP4 SOC intecronnects has many write buffers in the async bridges >>>> and they can be drained only with stongly ordered accesses. >>> >>> This is not correct, strongly ordered access does not guarantee >>> anything here. If it fixes issues, it's because it makes the writes >>> to reach the device faster. Strongly ordered does not affect anything >>> outside ARM, so the bus access won't change. >>> >> What I said is the aync bridges WB and what is said is correct >> from MPU accesses point of view. >> >> It's not about faster or slower. With device memory the, writes >> can get stuck into write buffers where as with SO, the write buffers >> will be bypassed. >> >> The behaviours is limited to the MPU side async bridge boundary which >> is the problem. The statement is not for l3 and l4 interconnect which >> probably you mean. >> >> There is always a hardware signal to communicate CPU at async bridges >> to ensure that data is not stuck in these bridges before CPU >> clock/voltage is cut. Unfortunately we have a BUG on OMAP44XX devices >> and the dual channel makes it even worst since both pipes have the >> same BUG. So what we are doing is issuing SO write/read accesses >> on these pipes so that there is nothing stuck there before MPU >> hits low power states and also avoids any race conditions when >> both channels are used together by some initiators. The behaviour >> is validated at RTL level and there is no ambiguity about it. >> >> May be you have mistaken the L3 and L4 as the interconnect levels >> in this case. > > Sounds to me like the changelog needs to be a bit more verbose. > > Remember, we're all probably going to forget the gory details of this in > a few months and want to be able to go back to the code w/changelog to > refresh our memories. > Change log was accurate but I agree it needs more description to make it clear. I plan to update it.
* Shilimkar, Santosh <santosh.shilimkar@ti.com> [110915 09:51]: > On Thu, Sep 15, 2011 at 10:47 PM, Kevin Hilman <khilman@ti.com> wrote: > > Santosh <santosh.shilimkar@ti.com> writes: > > > >> On Wednesday 14 September 2011 01:57 AM, Tony Lindgren wrote: > >>> * Santosh Shilimkar<santosh.shilimkar@ti.com> [110904 06:22]: > >>>> On OMAP4 SOC intecronnects has many write buffers in the async bridges > >>>> and they can be drained only with stongly ordered accesses. > >>> > >>> This is not correct, strongly ordered access does not guarantee > >>> anything here. If it fixes issues, it's because it makes the writes > >>> to reach the device faster. Strongly ordered does not affect anything > >>> outside ARM, so the bus access won't change. > >>> > >> What I said is the aync bridges WB and what is said is correct > >> from MPU accesses point of view. > >> > >> It's not about faster or slower. With device memory the, writes > >> can get stuck into write buffers where as with SO, the write buffers > >> will be bypassed. > >> > >> The behaviours is limited to the MPU side async bridge boundary which > >> is the problem. The statement is not for l3 and l4 interconnect which > >> probably you mean. > >> > >> There is always a hardware signal to communicate CPU at async bridges > >> to ensure that data is not stuck in these bridges before CPU > >> clock/voltage is cut. Unfortunately we have a BUG on OMAP44XX devices > >> and the dual channel makes it even worst since both pipes have the > >> same BUG. So what we are doing is issuing SO write/read accesses > >> on these pipes so that there is nothing stuck there before MPU > >> hits low power states and also avoids any race conditions when > >> both channels are used together by some initiators. The behaviour > >> is validated at RTL level and there is no ambiguity about it. > >> > >> May be you have mistaken the L3 and L4 as the interconnect levels > >> in this case. > > > > Sounds to me like the changelog needs to be a bit more verbose. > > > > Remember, we're all probably going to forget the gory details of this in > > a few months and want to be able to go back to the code w/changelog to > > refresh our memories. > > > Change log was accurate but I agree it needs more description to make it > clear. I plan to update it. Please also include the errata in the description and set it up with a Kconfig entry with something like ARM_ERRATA_XXXXXX or TI_ERRATA_XXXXXX. Also it would be best to apply this fix at the end of the PM series so it is easier to verify the bug and potentially remove the hack if a better fix is ever available. Regards, Tony
On Thu, Sep 15, 2011 at 11:23 PM, Tony Lindgren <tony@atomide.com> wrote: > * Shilimkar, Santosh <santosh.shilimkar@ti.com> [110915 09:51]: >> On Thu, Sep 15, 2011 at 10:47 PM, Kevin Hilman <khilman@ti.com> wrote: >> > Santosh <santosh.shilimkar@ti.com> writes: >> > >> >> On Wednesday 14 September 2011 01:57 AM, Tony Lindgren wrote: >> >>> * Santosh Shilimkar<santosh.shilimkar@ti.com> [110904 06:22]: >> >>>> On OMAP4 SOC intecronnects has many write buffers in the async bridges >> >>>> and they can be drained only with stongly ordered accesses. >> >>> >> >>> This is not correct, strongly ordered access does not guarantee >> >>> anything here. If it fixes issues, it's because it makes the writes >> >>> to reach the device faster. Strongly ordered does not affect anything >> >>> outside ARM, so the bus access won't change. >> >>> >> >> What I said is the aync bridges WB and what is said is correct >> >> from MPU accesses point of view. >> >> >> >> It's not about faster or slower. With device memory the, writes >> >> can get stuck into write buffers where as with SO, the write buffers >> >> will be bypassed. >> >> >> >> The behaviours is limited to the MPU side async bridge boundary which >> >> is the problem. The statement is not for l3 and l4 interconnect which >> >> probably you mean. >> >> >> >> There is always a hardware signal to communicate CPU at async bridges >> >> to ensure that data is not stuck in these bridges before CPU >> >> clock/voltage is cut. Unfortunately we have a BUG on OMAP44XX devices >> >> and the dual channel makes it even worst since both pipes have the >> >> same BUG. So what we are doing is issuing SO write/read accesses >> >> on these pipes so that there is nothing stuck there before MPU >> >> hits low power states and also avoids any race conditions when >> >> both channels are used together by some initiators. The behaviour >> >> is validated at RTL level and there is no ambiguity about it. >> >> >> >> May be you have mistaken the L3 and L4 as the interconnect levels >> >> in this case. >> > >> > Sounds to me like the changelog needs to be a bit more verbose. >> > >> > Remember, we're all probably going to forget the gory details of this in >> > a few months and want to be able to go back to the code w/changelog to >> > refresh our memories. >> > >> Change log was accurate but I agree it needs more description to make it >> clear. I plan to update it. > > Please also include the errata in the description and set it up with > a Kconfig entry with something like ARM_ERRATA_XXXXXX or TI_ERRATA_XXXXXX. > Sure. It's a TI ERRATA. > Also it would be best to apply this fix at the end of the PM series so > it is easier to verify the bug and potentially remove the hack if > a better fix is ever available. > As such order doesn't matter much because it can be removed either way even if it is in the beginning of the series with KCONFIG entry. But I can change the order if you prefer that way. Regards Santosh
* Shilimkar, Santosh <santosh.shilimkar@ti.com> [110915 10:49]: > On Thu, Sep 15, 2011 at 11:23 PM, Tony Lindgren <tony@atomide.com> wrote: > > > > Please also include the errata in the description and set it up with > > a Kconfig entry with something like ARM_ERRATA_XXXXXX or TI_ERRATA_XXXXXX. > > > Sure. > It's a TI ERRATA. OK > > Also it would be best to apply this fix at the end of the PM series so > > it is easier to verify the bug and potentially remove the hack if > > a better fix is ever available. > > > As such order doesn't matter much because it can be removed either way > even if it is in the beginning of the series with KCONFIG entry. > > But I can change the order if you prefer that way. Well it seems that it's the omap4_finish_suspend in this series that can be used to trigger the bug, although the bug has been around for few years. So then instead of mentioning random hangups you can have a better description with something like "Patch xyz added PM idle support for omap4, however bug 123 causes so and so. Fix the issue with blah blah". Also, if you have some easy way to reproduce the bug maybe mention that too? That would make it easy to verify if issue has been fixed. Regards, Tony
On Fri, Sep 16, 2011 at 1:13 AM, Tony Lindgren <tony@atomide.com> wrote: > * Shilimkar, Santosh <santosh.shilimkar@ti.com> [110915 10:49]: >> On Thu, Sep 15, 2011 at 11:23 PM, Tony Lindgren <tony@atomide.com> wrote: >> > >> > Please also include the errata in the description and set it up with >> > a Kconfig entry with something like ARM_ERRATA_XXXXXX or TI_ERRATA_XXXXXX. >> > >> Sure. >> It's a TI ERRATA. > > OK > >> > Also it would be best to apply this fix at the end of the PM series so >> > it is easier to verify the bug and potentially remove the hack if >> > a better fix is ever available. >> > >> As such order doesn't matter much because it can be removed either way >> even if it is in the beginning of the series with KCONFIG entry. >> >> But I can change the order if you prefer that way. > > Well it seems that it's the omap4_finish_suspend in this series that > can be used to trigger the bug, although the bug has been around for > few years. So then instead of mentioning random hangups you can have > a better description with something like "Patch xyz added PM idle > support for omap4, however bug 123 causes so and so. Fix the issue > with blah blah". > Sounds good . > Also, if you have some easy way to reproduce the bug maybe mention > that too? That would make it easy to verify if issue has been fixed. > There are some multimedia usecases where the bug was discovered but on mainline obviously we don't have that support. I will check with IP folks if any other simple test-case is possible to reproduce the issue and If I find one, will mention that. Thanks for the review. Regards Santosh
On Fri, Sep 16, 2011 at 1:30 AM, Shilimkar, Santosh <santosh.shilimkar@ti.com> wrote: > On Fri, Sep 16, 2011 at 1:13 AM, Tony Lindgren <tony@atomide.com> wrote: >> * Shilimkar, Santosh <santosh.shilimkar@ti.com> [110915 10:49]: >>> On Thu, Sep 15, 2011 at 11:23 PM, Tony Lindgren <tony@atomide.com> wrote: >>> > >>> > Please also include the errata in the description and set it up with >>> > a Kconfig entry with something like ARM_ERRATA_XXXXXX or TI_ERRATA_XXXXXX. >>> > >>> Sure. >>> It's a TI ERRATA. >> >> OK >> >>> > Also it would be best to apply this fix at the end of the PM series so >>> > it is easier to verify the bug and potentially remove the hack if >>> > a better fix is ever available. >>> > >>> As such order doesn't matter much because it can be removed either way >>> even if it is in the beginning of the series with KCONFIG entry. >>> >>> But I can change the order if you prefer that way. >> >> Well it seems that it's the omap4_finish_suspend in this series that >> can be used to trigger the bug, although the bug has been around for >> few years. So then instead of mentioning random hangups you can have >> a better description with something like "Patch xyz added PM idle >> support for omap4, however bug 123 causes so and so. Fix the issue >> with blah blah". >> > Sounds good . > >> Also, if you have some easy way to reproduce the bug maybe mention >> that too? That would make it easy to verify if issue has been fixed. >> Just for record, Below are the errata details. ERRATA: i688: Async Bridge Corruption If a data is stalled inside asynchronous bridge because of backpressure, it may be accepted multiple times, creating pointer misalignment that will corrupt next transfers on that data path until next reset of the system (No recovery procedure once the issue is hit, the path remains consistently broken). Async bridge can be found on path between MPU to EMIF, MPU to L3 interconnect and Cortex M3 to Emif . This situation can happen only when the idle is initiated by a Master Request Disconnection (which is trigged by software whenexecuting WFI) WORKAROUND All the initiators connected through Async Bridge must ensure that data path is properly drained before issuing WFI. This condition will be met if one Strongly ordered access is performed to the target right before executing the WFI. > There are some multimedia usecases where the bug was discovered > but on mainline obviously we don't have that support. > > I will check with IP folks if any other simple test-case is possible > to reproduce the issue and If I find one, will mention that. > We have a GFX usecase which demonstrate this issue but can't be tested on mainline sicne no GFX support. But the same patch is already in the product line which has fixed the GFX usecase issue. Regards Santosh
Santosh Shilimkar <santosh.shilimkar@ti.com> writes: > When MPUSS hits off-mode e, L2 cache is lost. This patch adds L2X0 ^^^ extra 'e' ? > necessary maintenance operations and context restoration in the > low power code. > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Kevin Hilman <khilman@ti.com> [...] > @@ -135,6 +138,33 @@ static void scu_pwrst_prepare(unsigned int cpu_id, unsigned int cpu_state) > __raw_writel(scu_pwr_st, pm_info->scu_sar_addr); > } > > +/* > + * Store the CPU cluster state for L2X0 low power operations. > + */ > +static void l2x0_pwrst_prepare(unsigned int cpu_id, unsigned int save_state) > +{ > + struct omap4_cpu_pm_info *pm_info = &per_cpu(omap4_pm_info, cpu_id); > + > + __raw_writel(save_state, pm_info->l2x0_sar_addr); > +} > + > +/* > + * Save the L2X0 AUXCTRL and POR value to SAR memory. Its used to > + * in every restore MPUSS OFF path. > + */ > +static void save_l2x0_context(void) > +{ > +#ifdef CONFIG_CACHE_L2X0 > + u32 val; > + void __iomem *l2x0_base = omap4_get_l2cache_base(); > + > + val = __raw_readl(l2x0_base + L2X0_AUX_CTRL); > + __raw_writel(val, sar_base + L2X0_AUXCTRL_OFFSET); > + val = __raw_readl(l2x0_base + L2X0_PREFETCH_CTRL); > + __raw_writel(val, sar_base + L2X0_PREFETCH_CTRL_OFFSET); > +#endif nit: (c.f. '#ifdefs are ugly' in Documentatin/SubmittingPatches) This should probably be more like #ifdef CONFIG_CACHE_L2X0 static void save_l2x0_context(void) { /* real function */ } #else static void save_l2x0_context(void) {} #endif Kevin
Hi Santosh, Santosh Shilimkar <santosh.shilimkar@ti.com> writes: > Add OMAP4 CPUIDLE support. CPU1 is left with defualt idle and > the low power state for it is managed via cpu-hotplug. > > This patch adds MPUSS low power states in cpuidle. > > C1 - CPU0 ON + CPU1 ON + MPU ON > C2 - CPU0 OFF + CPU1 OFF + MPU CSWR > C3 - CPU0 OFF + CPU1 OFF + MPU OSWR > > OMAP4460 onwards, MPUSS power domain doesn't support OFF state any more > anymore just like CORE power domain. The deepest state supported is OSWr. > Ofcourse when MPUSS and CORE PD transitions to OSWR along with device > off mode, even the memory contemts are lost which is as good as > the PD off state. > > On OMAP4 because of hardware constraints, no low power states are > targeted when both CPUs are online and in SMP mode. The low power > states are attempted only when secondary CPU gets offline to OFF > through hotplug infrastructure. > > Thanks to Nicole Chalhoub <n-chalhoub@ti.com> for doing exhaustive > C-state latency profiling. > > Signed-off-by: Rajendra Nayak <rnayak@ti.com> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Kevin Hilman <khilman@ti.com> A handful of minor comments below... [...] > +/** > + * omap4_enter_idle - Programs OMAP4 to enter the specified state > + * @dev: cpuidle device > + * @state: The target state to be programmed > + * > + * Called from the CPUidle framework to program the device to the > + * specified low power state selected by the governor. > + * Returns the amount of time spent in the low power state. > + */ > +static int omap4_enter_idle(struct cpuidle_device *dev, > + struct cpuidle_state *state) > +{ > + struct omap4_idle_statedata *cx = cpuidle_get_statedata(state); > + struct timespec ts_preidle, ts_postidle, ts_idle; > + u32 cpu1_state; > + > + /* Used to keep track of the total time in idle */ > + getnstimeofday(&ts_preidle); > + > + local_irq_disable(); > + local_fiq_disable(); > + > + /* > + * Keep demoting CPU0 C-state till CPU1 hits OFF state. IMO, the working here isn't as clear as it could be. IOW, I don't like "keep demoting", which makes it sound iterative through a set of states, where what's happening here is a one-time decision. Rather, what I think you mean is "CPU0 has to stay on (e.g in C1) until CPU1 is off." > + * This is necessary to honour hardware recommondation > + * of triggeing all the possible low power modes once CPU1 is > + * out of coherency and in OFF mode. > + * Update dev->last_state so that governor stats reflects right > + * data. > + */ > + cpu1_state = pwrdm_read_pwrst(cpu1_pd); > + if (cpu1_state != PWRDM_POWER_OFF) { > + dev->last_state = dev->safe_state; > + cx = cpuidle_get_statedata(dev->safe_state); > + } > + > + /* Call idle CPU PM enter notifier chain */ This comment doesn't add any value over the code. If anything, a comment explaining why it's only there for off-mode transitions would be helpful. > + if (cx->cpu_state == PWRDM_POWER_OFF) > + cpu_pm_enter(); I think the CPU PM notifier usage should probably be a separate patch. > + pwrdm_set_logic_retst(mpu_pd, cx->mpu_logic_state); > + omap_set_pwrdm_state(mpu_pd, cx->mpu_state); > + > + /* Call idle CPU cluster PM enter notifier chain */ > + if ((cx->mpu_state == PWRDM_POWER_RET) && > + (cx->mpu_logic_state == PWRDM_POWER_OFF)) > + cpu_cluster_pm_enter(); > + > + omap4_enter_lowpower(dev->cpu, cx->cpu_state); > + > + /* Call idle CPU PM exit notifier chain */ As above, this comment doesn't add any value over the code. While I understand why it's only done for CPU0 here (CPU1 is managed by hotplug), we should anticipating the moment where we will have forgotten why it's only done for CPU0, and add a comment here. > + if (pwrdm_read_prev_pwrst(cpu0_pd) == PWRDM_POWER_OFF) > + cpu_pm_exit(); > + > + /* Call idle CPU cluster PM exit notifier chain */ again, comment not helpful > + if (omap4_mpuss_read_prev_context_state()) > + cpu_cluster_pm_exit(); > + > + getnstimeofday(&ts_postidle); > + ts_idle = timespec_sub(ts_postidle, ts_preidle); > + > + local_irq_enable(); > + local_fiq_enable(); > + > + return ts_idle.tv_nsec / NSEC_PER_USEC + ts_idle.tv_sec * USEC_PER_SEC; > +} > + > +DEFINE_PER_CPU(struct cpuidle_device, omap4_idle_dev); > + > +struct cpuidle_driver omap4_idle_driver = { > + .name = "omap4_idle", > + .owner = THIS_MODULE, > +}; > + > +/* Fill in the state data from the mach tables and register the driver_data */ if documented, it should have a kerneldoc header instead > +static inline struct omap4_idle_statedata *_fill_cstate( > + struct cpuidle_device *dev, > + int idx, const char *descr) > +{ > + struct omap4_idle_statedata *cx = &omap4_idle_data[idx]; > + struct cpuidle_state *state = &dev->states[idx]; > + > + state->exit_latency = cpuidle_params_table[idx].exit_latency; > + state->target_residency = cpuidle_params_table[idx].target_residency; > + state->flags = CPUIDLE_FLAG_TIME_VALID; > + state->enter = omap4_enter_idle; > + cx->valid = cpuidle_params_table[idx].valid; > + sprintf(state->name, "C%d", idx + 1); > + strncpy(state->desc, descr, CPUIDLE_DESC_LEN); > + cpuidle_set_statedata(state, cx); > + > + return cx; > +} Kevin
Santosh Shilimkar <santosh.shilimkar@ti.com> writes: > CPU local timer(TWD) stops when the CPU is transitioning into > deeper C-States. Since these timers are not wakeup capable, we > need the wakeup capable global timer to program the wakeup time > depending on the next timer expiry. > > It can be handled by registering a global wakeup capable timer along > with local timers marked with (mis)feature flag CLOCK_EVT_FEAT_C3STOP. nit: this comment makes is sound like this patch adds the registration of this timer, when all this patch does is add the notification. Changelog should be updated to make it clear that the global timer exists already, and is already marked with the C3STOP flag. > Then notify the clock events layer from idle code using > CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT). > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Kevin Hilman <khilman@ti.com> Other than that Acked-by: Kevin Hilman <khilman@ti.com>
On Friday 16 September 2011 10:53 PM, Kevin Hilman wrote: > Santosh Shilimkar <santosh.shilimkar@ti.com> writes: > >> When MPUSS hits off-mode e, L2 cache is lost. This patch adds L2X0 > ^^^ > extra 'e' ? > >> necessary maintenance operations and context restoration in the >> low power code. >> >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >> Cc: Kevin Hilman <khilman@ti.com> > > [...] > >> @@ -135,6 +138,33 @@ static void scu_pwrst_prepare(unsigned int cpu_id, unsigned int cpu_state) >> __raw_writel(scu_pwr_st, pm_info->scu_sar_addr); >> } >> >> +/* >> + * Store the CPU cluster state for L2X0 low power operations. >> + */ >> +static void l2x0_pwrst_prepare(unsigned int cpu_id, unsigned int save_state) >> +{ >> + struct omap4_cpu_pm_info *pm_info = &per_cpu(omap4_pm_info, cpu_id); >> + >> + __raw_writel(save_state, pm_info->l2x0_sar_addr); >> +} >> + >> +/* >> + * Save the L2X0 AUXCTRL and POR value to SAR memory. Its used to >> + * in every restore MPUSS OFF path. >> + */ >> +static void save_l2x0_context(void) >> +{ >> +#ifdef CONFIG_CACHE_L2X0 >> + u32 val; >> + void __iomem *l2x0_base = omap4_get_l2cache_base(); >> + >> + val = __raw_readl(l2x0_base + L2X0_AUX_CTRL); >> + __raw_writel(val, sar_base + L2X0_AUXCTRL_OFFSET); >> + val = __raw_readl(l2x0_base + L2X0_PREFETCH_CTRL); >> + __raw_writel(val, sar_base + L2X0_PREFETCH_CTRL_OFFSET); >> +#endif > > nit: (c.f. '#ifdefs are ugly' in Documentatin/SubmittingPatches) > > This should probably be more like > > #ifdef CONFIG_CACHE_L2X0 > static void save_l2x0_context(void) > { > /* real function */ > } > #else > static void save_l2x0_context(void) {} > #endif > Ok. Will fix this. Regards Santosh
On Friday 16 September 2011 11:15 PM, Kevin Hilman wrote: > Hi Santosh, > > Santosh Shilimkar <santosh.shilimkar@ti.com> writes: > >> Add OMAP4 CPUIDLE support. CPU1 is left with defualt idle and >> the low power state for it is managed via cpu-hotplug. >> >> This patch adds MPUSS low power states in cpuidle. >> >> C1 - CPU0 ON + CPU1 ON + MPU ON >> C2 - CPU0 OFF + CPU1 OFF + MPU CSWR >> C3 - CPU0 OFF + CPU1 OFF + MPU OSWR >> >> OMAP4460 onwards, MPUSS power domain doesn't support OFF state any more >> anymore just like CORE power domain. The deepest state supported is OSWr. >> Ofcourse when MPUSS and CORE PD transitions to OSWR along with device >> off mode, even the memory contemts are lost which is as good as >> the PD off state. >> >> On OMAP4 because of hardware constraints, no low power states are >> targeted when both CPUs are online and in SMP mode. The low power >> states are attempted only when secondary CPU gets offline to OFF >> through hotplug infrastructure. >> >> Thanks to Nicole Chalhoub <n-chalhoub@ti.com> for doing exhaustive >> C-state latency profiling. >> >> Signed-off-by: Rajendra Nayak <rnayak@ti.com> >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >> Cc: Kevin Hilman <khilman@ti.com> > > A handful of minor comments below... > Will take care of them. Regards Santosh
On Friday 16 September 2011 11:21 PM, Kevin Hilman wrote: > Santosh Shilimkar <santosh.shilimkar@ti.com> writes: > >> CPU local timer(TWD) stops when the CPU is transitioning into >> deeper C-States. Since these timers are not wakeup capable, we >> need the wakeup capable global timer to program the wakeup time >> depending on the next timer expiry. >> >> It can be handled by registering a global wakeup capable timer along >> with local timers marked with (mis)feature flag CLOCK_EVT_FEAT_C3STOP. > > nit: this comment makes is sound like this patch adds the registration > of this timer, when all this patch does is add the notification. > > Changelog should be updated to make it clear that the global timer exists > already, and is already marked with the C3STOP flag. > Will add couple of lines in the change log to make it clear. >> Then notify the clock events layer from idle code using >> CLOCK_EVT_NOTIFY_BROADCAST_ENTER/EXIT). >> >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >> Cc: Kevin Hilman <khilman@ti.com> > > Other than that > > Acked-by: Kevin Hilman <khilman@ti.com> > Thanks. Regards Santosh
> -----Original Message----- > From: linux-arm-kernel-bounces@lists.infradead.org [mailto:linux- > arm-kernel-bounces@lists.infradead.org] On Behalf Of Santosh > Shilimkar > Sent: Sunday, September 04, 2011 7:24 PM > To: linux-omap@vger.kernel.org > Cc: khilman@ti.com; Santosh Shilimkar; rnayak@ti.com; > linux@arm.linux.org.uk; linux-arm-kernel@lists.infradead.org > Subject: [PATCH 00/25] OMAP4: PM: suspend, CPU-hotplug and CPUilde > support > > This series adds OMAP4 MPUSS (MPU SubSystem) power management > support for > suspend (S2R), CPU hotplug and CPUidle. > > Most of these patches have been posted and reviewed earlier [1] on > the list > and have missed last couple of merge windows because of dependencies. > New set of patches have diverged more and hence the series version > continuity isn't maintained. > > Below are the main updates from previous versions. > - Use of generic ARM suspend hooks instead of OMAP custom code. > - Making use of common GIC code instead of OMAP custom code. > - Use of generic CPU PM notifiers for CPUIDLE and suspend. > - Use of CPU PM notifiers and hotplug notifiers for GIC extension. > - PM support of OMAP4 HS devices. > - Introduction of interconnect barriers as per the OMAP4 > requirements. > > Special thanks to, > - Kevin Hilman for the detailed reviews. > - Russell for adding the L2 cache handling support to generic > suspend. > - Colin Cross for the generic CPU PM notifier patches. > - Rajendra Nayak and Paul Walmsley for clock-domain sequencing > series. > > Below series has dependency on Russell's L2 generic suspend support > [2] > and earlier posted CPU PM notifiers series [3]. > An integrated branch with these dependencies can be found here [4]. > > The series is tested on OMAP4430 SDP for suspend, hotplug and > CPUidle > with OMAP4 GP and HS (secure) devices. > > The following changes since commit > c6a389f123b9f68d605bb7e0f9b32ec1e3e14132: > > Linux 3.1-rc4 (2011-08-28 21:16:01 -0700) > > are available in the git repository at: > git://gitorious.org/omap-sw-develoment/linux-omap-dev.git v3.1- > rc4-omap4-mpuss-pm I have tested these patch series after adding core retention support (In suspend/resume path)[1] and it worked Fine. I do see that Core and MPU are entering CSWR upon suspend and able to wake up via uart. You could add tested-by: Vishwanath BS < Vishwanath.bs@ti.com> if you want. [1]: git://gitorious.org/omap-pm/linux.git omap4_core_pm Regards Vishwa > > Santosh Shilimkar (25): > ARM: mm: Add strongly ordered descriptor support. > OMAP4: Redefine mandatory barriers for OMAP to include > interconnect barriers. > OMAP4: PM: Use custom omap_do_wfi() for suspend and default > idle. > OMAP4: Remove un-used do_wfi() macro. > OMAP4: Use WARN_ON() instead of BUG_ON() with graceful exit > OMAP4: Export omap4_get_base*() rather than global address > pointers > OMAP4: PM: Add SAR RAM support > OMAP4: PM: Keep static dep between MPUSS-EMIF and MPUSS-L3 and > DUCATI-L3 > OMAP4: PM: Avoid omap4_pm_init() on OMAP4430 ES1.0 > OMAP4: PM: Initialise all the clockdomains to supported states > OMAP: Add Secure HAL and monitor mode API infrastructure. > OMAP: Add support to allocate the memory for secure RAM > OMAP4: PM: Add WakeupGen module as OMAP gic_arch_extn > OMAP4: PM: Add CPUX OFF mode support > OMAP4: Remove __INIT from omap_secondary_startup() to re-use > it for hotplug. > OMAP4: PM: Program CPU1 to hit OFF when off-lined > OMAP4: PM: CPU1 wakeup workaround from Low power modes > OMAP4: suspend: Add MPUSS power domain RETENTION support > OMAP4: PM: Add WakeupGen and secure GIC low power support > OMAP4: PM: Add L2X0 cache lowpower support > OMAP4: PM: Add MPUSS power domain OSWR support > OMAP4: PM: Add power domain statistics support > OMAP4: PM: Add CPUidle support > OMAP4: cpuidle: Switch to gptimer from twd in deeper C-states. > OMAP3: CPUidle: Make use of CPU PM notifiers > > arch/arm/include/asm/mach/map.h | 1 + > arch/arm/include/asm/pgtable.h | 3 + > arch/arm/mach-omap2/Kconfig | 1 + > arch/arm/mach-omap2/Makefile | 15 +- > arch/arm/mach-omap2/cpuidle34xx.c | 7 + > arch/arm/mach-omap2/cpuidle44xx.c | 206 > ++++++++++ > arch/arm/mach-omap2/include/mach/barriers.h | 48 +++ > arch/arm/mach-omap2/include/mach/omap-secure.h | 57 +++ > arch/arm/mach-omap2/include/mach/omap-wakeupgen.h | 39 ++ > arch/arm/mach-omap2/include/mach/omap4-common.h | 70 +++- > arch/arm/mach-omap2/omap-headsmp.S | 5 - > arch/arm/mach-omap2/omap-hotplug.c | 14 +- > arch/arm/mach-omap2/omap-mpuss-lowpower.c | 398 > +++++++++++++++++++ > arch/arm/mach-omap2/omap-secure.c | 81 ++++ > arch/arm/mach-omap2/{omap44xx-smc.S => omap-smc.S} | 23 ++ > arch/arm/mach-omap2/omap-smp.c | 38 ++ > arch/arm/mach-omap2/omap-wakeupgen.c | 403 > ++++++++++++++++++++ > arch/arm/mach-omap2/omap4-common.c | 93 +++++- > arch/arm/mach-omap2/omap4-sar-layout.h | 50 +++ > arch/arm/mach-omap2/pm.h | 1 + > arch/arm/mach-omap2/pm44xx.c | 155 ++++++++- > arch/arm/mach-omap2/sleep44xx.S | 385 > +++++++++++++++++++ > arch/arm/mm/mmu.c | 8 + > arch/arm/plat-omap/common.c | 3 + > arch/arm/plat-omap/include/plat/omap44xx.h | 1 + > arch/arm/plat-omap/include/plat/sram.h | 1 + > arch/arm/plat-omap/sram.c | 47 ++- > 27 files changed, 2104 insertions(+), 49 deletions(-) > create mode 100644 arch/arm/mach-omap2/cpuidle44xx.c > create mode 100644 arch/arm/mach-omap2/include/mach/barriers.h > create mode 100644 arch/arm/mach-omap2/include/mach/omap-secure.h > create mode 100644 arch/arm/mach-omap2/include/mach/omap- > wakeupgen.h > create mode 100644 arch/arm/mach-omap2/omap-mpuss-lowpower.c > create mode 100644 arch/arm/mach-omap2/omap-secure.c > rename arch/arm/mach-omap2/{omap44xx-smc.S => omap-smc.S} (70%) > create mode 100644 arch/arm/mach-omap2/omap-wakeupgen.c > create mode 100644 arch/arm/mach-omap2/omap4-sar-layout.h > create mode 100644 arch/arm/mach-omap2/sleep44xx.S > > Regards > Santosh > > [1] http://www.mail-archive.com/linux- > omap@vger.kernel.org/msg47511.html > > [2] http://www.spinics.net/lists/arm-kernel/msg138803.html > > [3] https://lkml.org/lkml/2011/9/3/49 > > [4] https://gitorious.org/omap-sw-develoment/linux-omap- > dev/commits/v3.1-rc4-omap4-pm-integrated > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tuesday 20 September 2011 04:54 PM, Vishwanath Sripathy wrote: >> -----Original Message----- >> From: linux-arm-kernel-bounces@lists.infradead.org [mailto:linux- >> arm-kernel-bounces@lists.infradead.org] On Behalf Of Santosh >> Shilimkar >> Sent: Sunday, September 04, 2011 7:24 PM >> To: linux-omap@vger.kernel.org >> Cc: khilman@ti.com; Santosh Shilimkar; rnayak@ti.com; >> linux@arm.linux.org.uk; linux-arm-kernel@lists.infradead.org >> Subject: [PATCH 00/25] OMAP4: PM: suspend, CPU-hotplug and CPUilde >> support >> [...] >> The series is tested on OMAP4430 SDP for suspend, hotplug and >> CPUidle >> with OMAP4 GP and HS (secure) devices. >> >> The following changes since commit >> c6a389f123b9f68d605bb7e0f9b32ec1e3e14132: >> >> Linux 3.1-rc4 (2011-08-28 21:16:01 -0700) >> >> are available in the git repository at: >> git://gitorious.org/omap-sw-develoment/linux-omap-dev.git v3.1- >> rc4-omap4-mpuss-pm > I have tested these patch series after adding core retention support (In > suspend/resume path)[1] and it worked Fine. I do see that Core and MPU are > entering CSWR upon suspend and able to wake up via uart. > > You could add tested-by: Vishwanath BS < Vishwanath.bs@ti.com> if you > want. > Yes I want tested-by and will add your tested-by on the series. Thanks for testing Regards Santosh
On Sunday 04 September 2011 07:24 PM, Santosh Shilimkar wrote: > This series adds OMAP4 MPUSS (MPU SubSystem) power management support for > suspend (S2R), CPU hotplug and CPUidle. > Thanks all for your review's and testing of this series. Have addressed all the concerns/comments as part of the next post which I am holding for couple of days more. Reason is mainly to ensure that dependent patches(series) are merged or on the way for 3.2, and the patches are re-based against the right tree for the merge. As documented earlier, there are 3 dependencies for this series. 1) Russell's L2 cache update to generic suspend which seems to be be ready now since Lorenzo confirmed that pending issue is fixed. http://www.spinics.net/lists/linux-omap/msg57625.html 2) CPU PM notifier series for which I have pull request ready to be send. https://lkml.org/lkml/2011/9/3/49 3) IRQ subystem patch which is already in Thomas's irq/core for 3.2 http://www.mail-archive.com/linux-omap@vger.kernel.org/msg55483.html Regards Santosh
This series adds OMAP4 MPUSS (MPU SubSystem) power management support for suspend (S2R), CPU hotplug and CPUidle. Most of these patches have been posted and reviewed earlier [1] on the list and have missed last couple of merge windows because of dependencies. New set of patches have diverged more and hence the series version continuity isn't maintained. Below are the main updates from previous versions. - Use of generic ARM suspend hooks instead of OMAP custom code. - Making use of common GIC code instead of OMAP custom code. - Use of generic CPU PM notifiers for CPUIDLE and suspend. - Use of CPU PM notifiers and hotplug notifiers for GIC extension. - PM support of OMAP4 HS devices. - Introduction of interconnect barriers as per the OMAP4 requirements. Special thanks to, - Kevin Hilman for the detailed reviews. - Russell for adding the L2 cache handling support to generic suspend. - Colin Cross for the generic CPU PM notifier patches. - Rajendra Nayak and Paul Walmsley for clock-domain sequencing series. Below series has dependency on Russell's L2 generic suspend support [2] and earlier posted CPU PM notifiers series [3]. An integrated branch with these dependencies can be found here [4]. The series is tested on OMAP4430 SDP for suspend, hotplug and CPUidle with OMAP4 GP and HS (secure) devices. The following changes since commit c6a389f123b9f68d605bb7e0f9b32ec1e3e14132: Linux 3.1-rc4 (2011-08-28 21:16:01 -0700) are available in the git repository at: git://gitorious.org/omap-sw-develoment/linux-omap-dev.git v3.1-rc4-omap4-mpuss-pm Santosh Shilimkar (25): ARM: mm: Add strongly ordered descriptor support. OMAP4: Redefine mandatory barriers for OMAP to include interconnect barriers. OMAP4: PM: Use custom omap_do_wfi() for suspend and default idle. OMAP4: Remove un-used do_wfi() macro. OMAP4: Use WARN_ON() instead of BUG_ON() with graceful exit OMAP4: Export omap4_get_base*() rather than global address pointers OMAP4: PM: Add SAR RAM support OMAP4: PM: Keep static dep between MPUSS-EMIF and MPUSS-L3 and DUCATI-L3 OMAP4: PM: Avoid omap4_pm_init() on OMAP4430 ES1.0 OMAP4: PM: Initialise all the clockdomains to supported states OMAP: Add Secure HAL and monitor mode API infrastructure. OMAP: Add support to allocate the memory for secure RAM OMAP4: PM: Add WakeupGen module as OMAP gic_arch_extn OMAP4: PM: Add CPUX OFF mode support OMAP4: Remove __INIT from omap_secondary_startup() to re-use it for hotplug. OMAP4: PM: Program CPU1 to hit OFF when off-lined OMAP4: PM: CPU1 wakeup workaround from Low power modes OMAP4: suspend: Add MPUSS power domain RETENTION support OMAP4: PM: Add WakeupGen and secure GIC low power support OMAP4: PM: Add L2X0 cache lowpower support OMAP4: PM: Add MPUSS power domain OSWR support OMAP4: PM: Add power domain statistics support OMAP4: PM: Add CPUidle support OMAP4: cpuidle: Switch to gptimer from twd in deeper C-states. OMAP3: CPUidle: Make use of CPU PM notifiers arch/arm/include/asm/mach/map.h | 1 + arch/arm/include/asm/pgtable.h | 3 + arch/arm/mach-omap2/Kconfig | 1 + arch/arm/mach-omap2/Makefile | 15 +- arch/arm/mach-omap2/cpuidle34xx.c | 7 + arch/arm/mach-omap2/cpuidle44xx.c | 206 ++++++++++ arch/arm/mach-omap2/include/mach/barriers.h | 48 +++ arch/arm/mach-omap2/include/mach/omap-secure.h | 57 +++ arch/arm/mach-omap2/include/mach/omap-wakeupgen.h | 39 ++ arch/arm/mach-omap2/include/mach/omap4-common.h | 70 +++- arch/arm/mach-omap2/omap-headsmp.S | 5 - arch/arm/mach-omap2/omap-hotplug.c | 14 +- arch/arm/mach-omap2/omap-mpuss-lowpower.c | 398 +++++++++++++++++++ arch/arm/mach-omap2/omap-secure.c | 81 ++++ arch/arm/mach-omap2/{omap44xx-smc.S => omap-smc.S} | 23 ++ arch/arm/mach-omap2/omap-smp.c | 38 ++ arch/arm/mach-omap2/omap-wakeupgen.c | 403 ++++++++++++++++++++ arch/arm/mach-omap2/omap4-common.c | 93 +++++- arch/arm/mach-omap2/omap4-sar-layout.h | 50 +++ arch/arm/mach-omap2/pm.h | 1 + arch/arm/mach-omap2/pm44xx.c | 155 ++++++++- arch/arm/mach-omap2/sleep44xx.S | 385 +++++++++++++++++++ arch/arm/mm/mmu.c | 8 + arch/arm/plat-omap/common.c | 3 + arch/arm/plat-omap/include/plat/omap44xx.h | 1 + arch/arm/plat-omap/include/plat/sram.h | 1 + arch/arm/plat-omap/sram.c | 47 ++- 27 files changed, 2104 insertions(+), 49 deletions(-) create mode 100644 arch/arm/mach-omap2/cpuidle44xx.c create mode 100644 arch/arm/mach-omap2/include/mach/barriers.h create mode 100644 arch/arm/mach-omap2/include/mach/omap-secure.h create mode 100644 arch/arm/mach-omap2/include/mach/omap-wakeupgen.h create mode 100644 arch/arm/mach-omap2/omap-mpuss-lowpower.c create mode 100644 arch/arm/mach-omap2/omap-secure.c rename arch/arm/mach-omap2/{omap44xx-smc.S => omap-smc.S} (70%) create mode 100644 arch/arm/mach-omap2/omap-wakeupgen.c create mode 100644 arch/arm/mach-omap2/omap4-sar-layout.h create mode 100644 arch/arm/mach-omap2/sleep44xx.S Regards Santosh [1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg47511.html [2] http://www.spinics.net/lists/arm-kernel/msg138803.html [3] https://lkml.org/lkml/2011/9/3/49 [4] https://gitorious.org/omap-sw-develoment/linux-omap-dev/commits/v3.1-rc4-omap4-pm-integrated