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.
>
> 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.)
> - 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.
>
> > + 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.)
>
> > 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.
>
> > +
> > +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
>
> > + 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.)
>
> > static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
> > {
> > struct power_state *pwrst;
> > @@ -115,6 +136,30 @@ static int __init pwrdms_setup(struct powerdomain *pwrdm, void *unused)
> > return omap_set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
> > }
> >
> > +void omap4_pm_off_mode_enable(int enable)
> > +{
> > + u32 next_state = PWRDM_POWER_RET;
> > + u32 next_logic_state = PWRDM_POWER_ON;
> > + struct power_state *pwrst;
> > +
> > + if (enable) {
> > + next_state = PWRDM_POWER_OFF;
> > + next_logic_state = PWRDM_POWER_OFF;
> > + }
> > +
> > + omap4_device_set_state_off(enable);
> > +
> > + list_for_each_entry(pwrst, &pwrst_list, node) {
> > + pwrst->next_state =
> > + get_achievable_state(pwrst->pwrdm->pwrsts, next_state);
> > + pwrst->next_logic_state =
> > + get_achievable_state(pwrst->pwrdm->pwrsts_logic_ret,
> > + next_logic_state);
> > + omap_set_pwrdm_state(pwrst->pwrdm, pwrst->next_state);
> > + pwrdm_set_logic_retst(pwrst->pwrdm, pwrst->next_logic_state);
> > + }
> > +}
> > +
> > void omap4_pm_oswr_mode_enable(int enable)
> > {
> > u32 next_logic_state;
> > diff --git a/arch/arm/mach-omap2/prm44xx.c b/arch/arm/mach-omap2/prm44xx.c
> > index 09d84de..86c6c6b 100644
> > --- a/arch/arm/mach-omap2/prm44xx.c
> > +++ b/arch/arm/mach-omap2/prm44xx.c
> > @@ -289,6 +289,72 @@ static void __init omap44xx_prm_enable_io_wakeup(void)
> > OMAP4_PRM_IO_PMCTRL_OFFSET);
> > }
> >
> > +/**
> > + * omap4_device_set_state_off() - setup device off state
> > + * @enable: set to off or not.
> > + *
> > + * When Device OFF is enabled, Device is allowed to perform
> > + * transition to off mode as soon as all power domains in MPU, IVA
> > + * and CORE voltage are in OFF or OSWR state (open switch retention)
> > + */
> > +void omap4_device_set_state_off(u8 enable)
> > +{
> > + if (enable)
> > + omap4_prminst_write_inst_reg(0x1 <<
> > + OMAP4430_DEVICE_OFF_ENABLE_SHIFT,
> > + OMAP4430_PRM_PARTITION,
> > + OMAP4430_PRM_DEVICE_INST,
> > + OMAP4_PRM_DEVICE_OFF_CTRL_OFFSET);
> > + else
> > + omap4_prminst_write_inst_reg(0x0 <<
> > + OMAP4430_DEVICE_OFF_ENABLE_SHIFT,
> > + OMAP4430_PRM_PARTITION,
> > + OMAP4430_PRM_DEVICE_INST,
> > + OMAP4_PRM_DEVICE_OFF_CTRL_OFFSET);
> > +}
>
> minor: a more compact, and more readable (so it's obvious at first
> glance that the same register is being written) would be:
>
> void omap4_device_set_state_off(u8 enable)
> {
> u8 val = enable ? 0x1 : 0x0;
>
> omap4_prminst_write_inst_reg(val <<
> OMAP4430_DEVICE_OFF_ENABLE_SHIFT,
> OMAP4430_PRM_PARTITION,
> OMAP4430_PRM_DEVICE_INST,
> OMAP4_PRM_DEVICE_OFF_CTRL_OFFSET);
> }
True.
>
> > +/**
> > + * omap4_device_prev_state_off:
> > + * returns true if the device hit OFF mode
> > + * This is API to check whether OMAP is waking up from device OFF mode.
> > + * There is no other status bit available for SW to read whether last state
> > + * entered was device OFF. To work around this, CORE PD, RFF context state
> > + * is used which is lost only when we hit device OFF state
> > + */
> > +bool omap4_device_prev_state_off(void)
> > +{
> > + u32 reg;
>
> minor: reg makes me think the result of the read is a register address,
> not a value. How about s/reg/val/.
Yea, good point.
>
> > + reg = omap4_prminst_read_inst_reg(OMAP4430_PRM_PARTITION,
> > + OMAP4430_PRM_CORE_INST,
> > + OMAP4_RM_L3_1_L3_1_CONTEXT_OFFSET)
> > + & OMAP4430_LOSTCONTEXT_RFF_MASK;
> > +
> > + return reg ? true : false;
> > +}
> > +
> > +void omap4_device_clear_prev_off_state(void)
> > +{
> > + omap4_prminst_write_inst_reg(OMAP4430_LOSTCONTEXT_RFF_MASK |
> > + OMAP4430_LOSTCONTEXT_DFF_MASK,
> > + OMAP4430_PRM_PARTITION,
> > + OMAP4430_PRM_CORE_INST,
> > + OMAP4_RM_L3_1_L3_1_CONTEXT_OFFSET);
> > +}
> > +
> > +/**
> > + * omap4_device_next_state_off:
> > + * returns true if the device next state is OFF
> > + * This is API to check whether OMAP is programmed for device OFF
> > + */
> > +bool omap4_device_next_state_off(void)
> > +{
> > + return omap4_prminst_read_inst_reg(OMAP4430_PRM_PARTITION,
> > + OMAP4430_PRM_DEVICE_INST,
> > + OMAP4_PRM_DEVICE_OFF_CTRL_OFFSET)
> > + & OMAP4430_DEVICE_OFF_ENABLE_MASK ? true : false;
>
> minor: when using these long register read helpers, IMO readability
> improves if this is of the form:
>
> val = <read register>;
> val &= <mask>;
> return val ? true : false;
True.
>
> > +}
> > +
> > static int __init omap4xxx_prcm_init(void)
> > {
> > if (cpu_is_omap44xx()) {
>
> Kevin
>
--
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
