On Tue, 2012-05-29 at 11:31 -0700, Kevin Hilman wrote:
> Tero Kristo <t-kristo@xxxxxx> writes:
>
> > On Wed, 2012-05-16 at 15:36 -0700, Kevin Hilman wrote:
> >> +Jean for functional power states
> >>
> >> Tero Kristo <t-kristo@xxxxxx> writes:
> >>
> >> > This patch adds device off support to OMAP4 device type.
> >>
> >> Description is rather thin for a patch that is doing so much.
> >>
> >> > OFF mode is disabled by default,
> >>
> >> why?
> >
> > Good question. For historical reference I guess. The device off works
> > pretty nicely with the current kernel already, so it should be possible
> > to enable it by default and blame the people who break it.
> >
> >>
> >> > however, there are two ways to enable OFF mode:
> >> > a) In the board file, call omap4_pm_off_mode_enable(1)
> >> > b) Enable OFF mode using the debugfs entry
> >> > echo "1">/sys/kernel/debug/pm_debug/enable_off_mode
> >> > (conversely echo '0' will disable it as well).
> >>
> >> This part needs to be a separate patch.
> >>
> >> But, as stated in the core retention series, I'd like to move away from
> >> these global flags all together.
> >>
> >> The way we manage the disabling of certain states (like off) is already
> >> clumsy for OMAP3, and it's getting worse with OMAP4. Basically, I think
> >> this feature needs to be supported by using constraints on functional
> >> power states. Having checks all over the place is getting unwieldy and
> >> not attractive to maintain.
> >>
> >> The combination of constraints and functional power states should make
> >> this much more manageable. Until we have that, I'd prefer to keep
> >> the debugfs enable/disable stuff as separate patches at the end of the
> >> series used only for testing.
> >
> > Okay, this sounds like a good plan.
> >
> >>
> >> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@xxxxxx>
> >> > [t-kristo@xxxxxx: largely re-structured the code]
> >>
> >> then the sign-off above from Santosh probably doesn't apply anymore.
> >> You should change that to a Cc and just mention tht this is based upon
> >> some original work from Santosh.
> >
> > Yeah... I am not quite sure where the line goes here as I am modifying
> > the patches quite heavily but try to keep credits to the original
> > authors... will change this like so.
>
> I guess it's up to you whether you keep Santosh as author. It all
> depends how much you've changed the original. But you can use the
> changlog to give credits to Santosh, or state it was a collaboration,
> whatever you like. You can say stuff like "based on an origianl patch
> by...", and/or briefly summarize the changes you made from the original
> verions, etc.
>
> >>
> >> First, some general comments:
> >>
> >> There is a lot going on in this patch, so it is hard to follow what all
> >> is related, and why. Just a quick glance suggests it needs to be broken
> >> up into at least a few parts:
> >
> > What is the merge plan for the func power state stuff? I don't want to
> > create new dependencies if unnecessary. Otherwise the split should be
> > okay.
> >>
> >> - low-level PRM support: new APIs for various off-mode features)
> >> (should probably be done on top of functional power states)
> >> - powerdomain core support or "achievable" states
> >> (should probably be done on top of functional power states)
> >> - IRQ/GIC context save/restore
> >> - secure RAM save/restore (this has been tightly coupled to the GIC
> >> but it's not obvious why)
> >
> > This is tightly coupled to GIC because the ROM code has following API
> > calls:
> >
> > - save gic
> > - save secure RAM
> > - save secure all (gic + RAM + some other mysterious stuff)
> >
> > It is difficult/impossible to separate these without adding redundant
> > code execution (e.g. doing a GIC save from the GIC code + then doing a
> > second GIC save with save secure all from core PM code.)
>
> Ok, thanks for clarifying.
>
> That should be explained in the changelog for this patch when it's
> broken up.
Ok will add some comment about this.
>
> >> - PM debug support to enable/disable off-mode
> >> (for testing only, not for merge)
> >>
> >> > Signed-off-by: Tero Kristo <t-kristo@xxxxxx>
> >> > ---
> >> > arch/arm/mach-omap2/omap-mpuss-lowpower.c | 10 ++++-
> >> > arch/arm/mach-omap2/omap-wakeupgen.c | 47 +++++++++++++++++++-
> >> > arch/arm/mach-omap2/pm-debug.c | 17 +++++--
> >> > arch/arm/mach-omap2/pm.h | 20 +++++++++
> >> > arch/arm/mach-omap2/pm44xx.c | 45 +++++++++++++++++++
> >> > arch/arm/mach-omap2/prm44xx.c | 66 +++++++++++++++++++++++++++++
> >> > 6 files changed, 197 insertions(+), 8 deletions(-)
> >> >
> >> > diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> >> > index e02c082..7418e7c 100644
> >> > --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> >> > +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> >> > @@ -60,6 +60,7 @@
> >> > #include "prcm44xx.h"
> >> > #include "prm44xx.h"
> >> > #include "prm-regbits-44xx.h"
> >> > +#include "cm44xx.h"
> >> >
> >> > #ifdef CONFIG_SMP
> >> >
> >> > @@ -263,9 +264,13 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
> >> > * In MPUSS OSWR or device OFF, interrupt controller contest is lost.
> >> > */
> >> > mpuss_clear_prev_logic_pwrst();
> >> > - if ((pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_RET) &&
> >> > + if (omap4_device_next_state_off())
> >> > + save_state = 3;
> >>
> >> Why?
> >>
> >> I don't see why this check is needed in addition to the mpuss_pd check
> >> added just below?
> >
> > mpuss_pd does not go to off, thats why. It goes to OSWR state during
> > standard device-off. It is possible for it to go to OFF mode, but it is
> > not recommended.
>
> What is confusing is that the check below specifically checks for
> mpuss_pd == off.
Yes, I guess I'll just drop that check out. Or add an error if someone
actually tries to use it.
>
> >>
> >> > + else if ((pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_RET) &&
> >> > (pwrdm_read_logic_retst(mpuss_pd) == PWRDM_POWER_OFF))
> >> > save_state = 2;
> >> > + else if (pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_OFF)
> >> > + save_state = 3;
> >> > cpu_clear_prev_logic_pwrst(cpu);
> >> > set_cpu_next_pwrst(cpu, power_state);
> >> > @@ -288,6 +293,9 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
> >> > wakeup_cpu = smp_processor_id();
> >> > set_cpu_next_pwrst(wakeup_cpu, PWRDM_POWER_ON);
> >> >
> >> > + if (omap4_device_prev_state_off())
> >> > + omap4_device_clear_prev_off_state();
> >> > +
> >>
> >> The 'clear prev off state' is subsequently removed in the last patch
> >> "ARM: OMAP4: powerdomain: update mpu / core off counters during device
> >> off." Why is it needed here?
> >
> > That patch is moving the clear part to the lost_context_rff func, it is
> > probably not that clear. Should be easier to follow the logic once I
> > re-structure the code a bit more (separate the prm support funcs out to
> > their own set etc.)
>
> Yes, thanks.
>
> >>
> >> > pwrdm_post_transition();
> >> >
> >> > return 0;
> >> > diff --git a/arch/arm/mach-omap2/omap-wakeupgen.c b/arch/arm/mach-omap2/omap-wakeupgen.c
> >> > index 42cd7fb..805d08d 100644
> >> > --- a/arch/arm/mach-omap2/omap-wakeupgen.c
> >> > +++ b/arch/arm/mach-omap2/omap-wakeupgen.c
> >> > @@ -32,6 +32,7 @@
> >> >
> >> > #include "omap4-sar-layout.h"
> >> > #include "common.h"
> >> > +#include "pm.h"
> >> >
> >> > #define NR_REG_BANKS 4
> >> > #define MAX_IRQS 128
> >> > @@ -46,6 +47,8 @@ static void __iomem *sar_base;
> >> > static DEFINE_SPINLOCK(wakeupgen_lock);
> >> > static unsigned int irq_target_cpu[NR_IRQS];
> >> >
> >> > +static struct powerdomain *mpuss_pd;
> >> > +
> >> > /*
> >> > * Static helper functions.
> >> > */
> >> > @@ -259,7 +262,7 @@ static void irq_save_context(void)
> >> > /*
> >> > * Clear WakeupGen SAR backup status.
> >> > */
> >> > -void irq_sar_clear(void)
> >> > +static void irq_sar_clear(void)
> >> > {
> >> > u32 val;
> >> > val = __raw_readl(sar_base + SAR_BACKUP_STATUS_OFFSET);
> >> > @@ -271,7 +274,7 @@ void irq_sar_clear(void)
> >> > * Save GIC and Wakeupgen interrupt context using secure API
> >> > * for HS/EMU devices.
> >> > */
> >> > -static void irq_save_secure_context(void)
> >> > +static void irq_save_secure_gic(void)
> >> > {
> >> > u32 ret;
> >> > ret = omap_secure_dispatcher(OMAP4_HAL_SAVEGIC_INDEX,
> >> > @@ -282,6 +285,40 @@ static void irq_save_secure_context(void)
> >> > }
> >> > #endif
> >> >
> >> > +static void save_secure_ram(void)
> >> > +{
> >> > + u32 ret;
> >>
> >> CodingStyle nit: blank line needed here (and in multiple other places)
> >
> > Okay.
> >
> >>
> >> > + ret = omap_secure_dispatcher(OMAP4_HAL_SAVESECURERAM_INDEX,
> >> > + FLAG_START_CRITICAL,
> >> > + 1, omap_secure_ram_mempool_base(),
> >> > + 0, 0, 0);
> >> > + if (ret != API_HAL_RET_VALUE_OK)
> >> > + pr_err("Secure ram context save failed\n");
> >> > +}
> >> > +
> >> > +static void save_secure_all(void)
> >> > +{
> >> > + u32 ret;
> >> > + ret = omap_secure_dispatcher(OMAP4_HAL_SAVEALL_INDEX,
> >> > + FLAG_START_CRITICAL,
> >> > + 1, omap_secure_ram_mempool_base(),
> >> > + 0, 0, 0);
> >> > + if (ret != API_HAL_RET_VALUE_OK)
> >> > + pr_err("Secure all context save failed\n");
> >> > +}
> >>
> >> This secure mode save/restore seems misplaced in the wakeupgen driver.
> >> Seems to me like it belongs in omap-secure.c.
> >
> > Can't really move it there, as described above. Unless we want to save
> > GIC context twice.
>
> OK
>
> >>
> >> > +
> >> > +static void irq_save_secure_context(void)
> >> > +{
> >> > + if (omap4_device_next_state_off()) {
> >> > + save_secure_all();
> >> > + } else if (pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_OFF) {
> >>
> >> Why is this check needed? This is called from the CPU_CLUSTER_PM_ENTER
> >> notifier, which AFAICT, is only called when the cluster is going off.
> >
> > This is checking against the different MPU PD states we can go into. If:
> > - MPU OSWR => GIC save needed only
> > - MPU OFF => GIC + secure RAM save needed (not recommended to be used
> > though)
> > - MPU OSWR / OFF + device-off => save all needed
>
> OK, more detailed changelog and descriptive kerneldoc for this function
> would certainly help.
Yep will add.
>
> >>
> >> > + irq_save_secure_gic();
> >> > + save_secure_ram();
> >> > + } else {
> >> > + irq_save_secure_gic();
> >> > + }
> >> > +}
> >> > +
> >> > #ifdef CONFIG_HOTPLUG_CPU
> >> > static int __cpuinit irq_cpu_hotplug_notify(struct notifier_block *self,
> >> > unsigned long action, void *hcpu)
> >> > @@ -388,5 +425,11 @@ int __init omap_wakeupgen_init(void)
> >> > irq_hotplug_init();
> >> > irq_pm_init();
> >> >
> >> > + mpuss_pd = pwrdm_lookup("mpu_pwrdm");
> >> > + if (!mpuss_pd) {
> >> > + pr_err("wakeupgen: unable to get mpu_pwrdm\n");
> >> > + return -EINVAL;
> >> > + }
> >> > +
> >> > return 0;
> >> > }
> >> > diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
> >> > index d9a8e42..d8cf5e5 100644
> >> > --- a/arch/arm/mach-omap2/pm-debug.c
> >> > +++ b/arch/arm/mach-omap2/pm-debug.c
> >> > @@ -40,6 +40,7 @@
> >> >
> >> > u32 enable_off_mode;
> >> > static u32 enable_oswr_mode;
> >> > +static void (*off_mode_enable_func) (int);
> >> >
> >> > #ifdef CONFIG_DEBUG_FS
> >> > #include <linux/debugfs.h>
> >> > @@ -249,7 +250,8 @@ static int option_set(void *data, u64 val)
> >> > else
> >> > omap_pm_disable_off_mode();
> >> >
> >> > - omap3_pm_off_mode_enable(val);
> >> > + if (off_mode_enable_func)
> >> > + off_mode_enable_func(val);
> >> > }
> >> >
> >> > if (option == &enable_oswr_mode)
> >> > @@ -278,16 +280,21 @@ static int __init pm_dbg_init(void)
> >> >
> >> > pwrdm_for_each(pwrdms_setup, (void *)d);
> >> >
> >> > - if (cpu_is_omap34xx())
> >> > - (void) debugfs_create_file("enable_off_mode",
> >> > - S_IRUGO | S_IWUSR, d, &enable_off_mode,
> >> > - &pm_dbg_option_fops);
> >> > + (void) debugfs_create_file("enable_off_mode",
> >> > + S_IRUGO | S_IWUSR, d, &enable_off_mode,
> >> > + &pm_dbg_option_fops);
> >> >
> >> > if (cpu_is_omap44xx())
> >> > (void) debugfs_create_file("enable_oswr_mode",
> >> > S_IRUGO | S_IWUSR, d, &enable_oswr_mode,
> >> > &pm_dbg_option_fops);
> >> >
> >> > + if (cpu_is_omap34xx())
> >> > + off_mode_enable_func = omap3_pm_off_mode_enable;
> >> > +
> >> > + if (cpu_is_omap44xx())
> >> > + off_mode_enable_func = omap4_pm_off_mode_enable;
> >> > +
> >> > pm_dbg_init_done = 1;
> >> >
> >> > return 0;
> >> > diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> >> > index c36ab63..d95f8c5 100644
> >> > --- a/arch/arm/mach-omap2/pm.h
> >> > +++ b/arch/arm/mach-omap2/pm.h
> >> > @@ -18,6 +18,7 @@
> >> > extern void *omap3_secure_ram_storage;
> >> > extern void omap3_pm_off_mode_enable(int);
> >> > extern void omap4_pm_oswr_mode_enable(int);
> >> > +extern void omap4_pm_off_mode_enable(int);
> >> > extern void omap_sram_idle(void);
> >> > extern int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
> >> > extern int omap3_idle_init(void);
> >> > @@ -25,6 +26,25 @@ extern int omap4_idle_init(void);
> >> > extern int omap_pm_clkdms_setup(struct clockdomain *clkdm, void *unused);
> >> > extern int (*omap_pm_suspend)(void);
> >> >
> >> > +#ifdef CONFIG_PM
> >> > +extern void omap4_device_set_state_off(u8 enable);
> >> > +extern bool omap4_device_prev_state_off(void);
> >> > +extern bool omap4_device_next_state_off(void);
> >> > +extern void omap4_device_clear_prev_off_state(void);
> >> > +#else
> >> > +static inline void omap4_device_set_state_off(u8 enable)
> >> > +{
> >> > +}
> >> > +static inline bool omap4_device_prev_state_off(void)
> >> > +{
> >> > + return false;
> >> > +}
> >> > +static inline bool omap4_device_next_state_off(void)
> >> > +{
> >> > + return false;
> >> > +}
> >> > +#endif
> >> > +
> >> > #if defined(CONFIG_PM_OPP)
> >> > extern int omap3_opp_init(void);
> >> > extern int omap4_opp_init(void);
> >> > diff --git a/arch/arm/mach-omap2/pm44xx.c b/arch/arm/mach-omap2/pm44xx.c
> >> > index 07ac0d3..8f0ec56 100644
> >> > --- a/arch/arm/mach-omap2/pm44xx.c
> >> > +++ b/arch/arm/mach-omap2/pm44xx.c
> >> > @@ -87,6 +87,27 @@ static int omap4_pm_suspend(void)
> >> > }
> >> > #endif /* CONFIG_SUSPEND */
> >> >
> >> > +/**
> >> > + * get_achievable_state() - Provide achievable state
> >> > + * @available_states: what states are available
> >> > + * @req_min_state: what state is the minimum we'd like to hit
> >> > + *
> >> > + * Power domains have varied capabilities. When attempting a low power
> >> > + * state such as OFF/RET, a specific min requested state may not be
> >> > + * supported on the power domain, in which case, the next higher power
> >> > + * state which is supported is returned. This is because a combination
> >> > + * of system power states where the parent PD's state is not in line
> >> > + * with expectation can result in system instabilities.
> >> > + */
> >> > +static inline u8 get_achievable_state(u8 available_states, u8 req_min_state)
> >> > +{
> >> > + u16 mask = 0xFF << req_min_state;
> >> > +
> >> > + if (available_states & mask)
> >> > + return __ffs(available_states & mask);
> >> > + return PWRDM_POWER_ON;
> >> > +}
> >>
> >> This feature needs to be generalized (preferably on top of functional
> >> power states) because we have the same need on various AM3xxx SoCs which
> >> don't support RET/OFF. Mark Greer has started on this, but I think it
> >> needs to be part of the functional power states work.
> >>
> >> It also needs to be part of the powerdomain layer, IMO.
> >
> > So, are you saying this whole set should be re-based on top of the
> > functional power states stuff? If yes, that can be done (Jean actually
> > already did some work on this and got it working I think.)
>
> Yes please.
Okay, next version will be based on top of func power states.
-Tero
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
