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