Re: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap
On Tue, May 19, 2009 at 11:55:41AM -0700, Kevin Hilman wrote: > Russell King - ARM Linux writes: > > > On Mon, May 18, 2009 at 10:00:44AM -0700, Kevin Hilman wrote: > >> Russell King - ARM Linux writes: > >> > >> > On Fri, May 15, 2009 at 11:40:41AM -0700, Kevin Hilman wrote: > >> >> This patch is to sync the core linux-omap PM code with mainline. This > >> >> code has evolved and been used for a while the linux-omap tree, but > >> >> the attempt here is to finally get this into mainline. > >> >> > >> >> Following this will be a series of patches from the 'PM branch' of the > >> >> linux-omap tree to add full PM hardware support from the linux-omap > >> >> tree. > >> >> > >> >> Much of this PM core code was written by Jouni Hogander with > >> >> significant contributions from Paul Walmsley as well as many others > >> >> from Nokia, Texas Instruments and linux-omap community. > >> > > >> > Overall comment, I think we need to rework the idle support code so > >> > that enable_hlt/disable_hlt can be used even when pm_idle has been > >> > overridden, rather than OMAP going off and inventing its own mechanisms. > >> > >> Would adding: > >> > >>if (hlt_counter) > >>cpu_relax(); > >> > >> to the beginning of omap*_pm_idle functions be sufficient? That will > >> at least allow the hlt stuff to behave as expected. > > > > Yes, but the comment was also directed at the other functions which > > increment/decrement that atomic_t variable to enable/disable sleep mode. > > > > Russell, > > Do you have a preference in how to export the hlt_counter to > platform-specific code? Sorry, I haven't had time to look at this, and the work I did to change the idle loop got lost when I last re-built my git tree. But... your patch isn't what I was meaning. I was meaning to avoid calling the idle function (_any_ idle function) if hlt_counter was non-zero. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap
Russell King - ARM Linux writes:
> On Mon, May 18, 2009 at 10:00:44AM -0700, Kevin Hilman wrote:
>> Russell King - ARM Linux writes:
>>
>> > On Fri, May 15, 2009 at 11:40:41AM -0700, Kevin Hilman wrote:
>> >> This patch is to sync the core linux-omap PM code with mainline. This
>> >> code has evolved and been used for a while the linux-omap tree, but
>> >> the attempt here is to finally get this into mainline.
>> >>
>> >> Following this will be a series of patches from the 'PM branch' of the
>> >> linux-omap tree to add full PM hardware support from the linux-omap
>> >> tree.
>> >>
>> >> Much of this PM core code was written by Jouni Hogander with
>> >> significant contributions from Paul Walmsley as well as many others
>> >> from Nokia, Texas Instruments and linux-omap community.
>> >
>> > Overall comment, I think we need to rework the idle support code so
>> > that enable_hlt/disable_hlt can be used even when pm_idle has been
>> > overridden, rather than OMAP going off and inventing its own mechanisms.
>>
>> Would adding:
>>
>> if (hlt_counter)
>> cpu_relax();
>>
>> to the beginning of omap*_pm_idle functions be sufficient? That will
>> at least allow the hlt stuff to behave as expected.
>
> Yes, but the comment was also directed at the other functions which
> increment/decrement that atomic_t variable to enable/disable sleep mode.
>
Russell,
Do you have a preference in how to export the hlt_counter to
platform-specific code?
The patch below creates a can_hlt() function that can be called from
platform-specific idle code.
Kevin
>From 23c802a0a1e9bb24ca51af9cf18b972590f8d1dc Mon Sep 17 00:00:00 2001
From: Kevin Hilman
Date: Tue, 19 May 2009 11:34:12 -0700
Subject: [PATCH] ARM: add can_hlt() for platform PM code to check before idling
When platform-specific code overrides pm_idle, the hlt_counter
is no longer used. Create a can_hlt() check which returns
the hlt_counter so platform code can check it and thus
honor the requests of enable_hlt()/disable_hlt() users.
Signed-off-by: Kevin Hilman
---
arch/arm/include/asm/system.h |1 +
arch/arm/kernel/process.c |7 +++
2 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index bd4dc8e..ff9e2dd 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -313,6 +313,7 @@ static inline unsigned long __xchg(unsigned long x,
volatile void *ptr, int size
extern void disable_hlt(void);
extern void enable_hlt(void);
+extern int can_hlt(void);
#include
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index c3265a2..1b414b8 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -68,6 +68,13 @@ void enable_hlt(void)
EXPORT_SYMBOL(enable_hlt);
+int can_hlt(void)
+{
+ return !hlt_counter;
+}
+
+EXPORT_SYMBOL(can_hlt);
+
static int __init nohlt_setup(char *__unused)
{
hlt_counter = 1;
--
1.6.2.2
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap
Artem Bityutskiy writes: > Kevin Hilman wrote: >>> The problem here is that such an interface is extremely fragile. Consider >>> what happens if a program disables HLT, and then gets killed off for some >>> reason. How does this reference get balanced again? >>> >>> I think a better solution would be a char device driver which has to be >>> kept open as long as a reference is held. When userspace closes it (be >>> that because the program has exited, been killed, etc) you can drop any >>> pending references. >> >> OK, this interface is not intended for users/applications. It is >> intended only for OMAP PM developers who are developing the PM code >> and want to prevent idle for various reasons during development. It >> is not intended for productions systems. >> >> What about leaving /sys/power/sleep_while_idle but only if >> CONFIG_PM_DEBUG=y? > > Sounds like debugfs is the good place for this then. > Sound OK to me. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap
Kevin Hilman wrote: The problem here is that such an interface is extremely fragile. Consider what happens if a program disables HLT, and then gets killed off for some reason. How does this reference get balanced again? I think a better solution would be a char device driver which has to be kept open as long as a reference is held. When userspace closes it (be that because the program has exited, been killed, etc) you can drop any pending references. OK, this interface is not intended for users/applications. It is intended only for OMAP PM developers who are developing the PM code and want to prevent idle for various reasons during development. It is not intended for productions systems. What about leaving /sys/power/sleep_while_idle but only if CONFIG_PM_DEBUG=y? Sounds like debugfs is the good place for this then. -- Best Regards, Artem Bityutskiy (Артём Битюцкий) -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap
Russell King - ARM Linux writes:
> On Mon, May 18, 2009 at 02:04:19PM -0500, Woodruff, Richard wrote:
>> cpu_init() is not yet accessible at the point this code is running.
>
> Not at that _exact_ point, but there's no need what so ever for it
> to be at that exact point. There's nothing stopping a call to
> cpu_init() happening later. Much later.
>
>> You could reduce the context perhaps trying to optimize to what the
>> Linux-ARM implementation is today or do a more generic save like is
>> there now.
>>
>> This code is copied into place from the .S code into SRAM. Its
>> position independent and is not linked for this address. If you want
>> to call functions outside of it you need to manually setup linkage.
>
> I wouldn't want to.
>
>> Calling functions on the way _UP_ from wake is NOT easy (like the one
>> you propose) as the MMU is not yet enabled. The MMU is enabled only
>> below this. Calling cpu_init() is not do able here. Even if you
>> dress up the calling pointer to the physical address, it won't work
>> as cpu_init() makes a global variable dereferences (smp_processor_id
>> & stacks[]).
>
> As long as IRQs and FIQs are disabled, you are in a 100% predictable
> environment.
>
> 1. No IRQ or FIQ will be entered; if they are the CPU is buggy.
> 2. No data or prefetch abort will be entered _unless_ you purposely
>access some non-present memory (and you're not exactly going to
>start paging memory in with IRQs disabled.)
>
> So restoring the abort and IRQ mode register state is just plain not
> necessary in your SRAM code.
>
> Let's look at the code. Here are the pertinent bits from Kevin's patch:
>
> static void omap3_pm_idle(void)
> {
> local_irq_disable();
> local_fiq_disable();
>
> omap_sram_idle();
>
> local_fiq_enable();
> local_irq_enable();
> }
>
> static void omap_sram_idle(void)
> {
> _omap_sram_idle(NULL, save_state);
> }
>
> _omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
> omap34xx_cpu_suspend_sz);
> pm_idle = omap3_pm_idle;
>
> _omap_sram_idle() is the assembly code we're discussing, and here we're
> referring to its version in SRAM. From the above code, we can clearly
> see that this is entered with FIQs and IRQs disabled. So everything
> inside omap_sram_idle() is running in a well defined environment provided
> prefetch and data aborts don't happen.
>
> There is nothing stopping this from becoming:
>
> static void omap3_pm_idle(void)
> {
> local_irq_disable();
> local_fiq_disable();
>
> omap_sram_idle();
> + cpu_init();
>
> local_fiq_enable();
> local_irq_enable();
> }
>
> and having the saving of IRQ, FIQ and abort mode registers removed
> from the SRAM code.
I did some quick experiments on a kernel that includes full OFF-mode
support and this and this is working fine.
For it to work for both idle and suspend, I put the cpu_init()
immediately after the return from SRAM (IOW, right after the call to
_omap_sram_idle() inside omap_sram_idle()).
Now I can totally drop all the sp/lr/spsr save/restore code from the
asm code. Nice!
> Other SoCs do precisely this - let's look at PXA:
>
> pxa_cpu_pm_fns->enter(state);
> cpu_init();
>
> The call to the enter method essentially calls the core specific suspend
> function (eg, pxa25x_cpu_suspend()), which is an assembly function which
> ultimately ends up powering down the core resulting in full loss of state
> in the CPU. We seem to be able to avoid having to save the exception mode
> registers there.
>
> The same thing is done with sa11x0 and Samsung SoCs.
>
> I don't see any reason for OMAP to be "special" in this regard.
Neither do I. Thanks for the review and the suggestion.
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap
> From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] > Sent: Monday, May 18, 2009 4:11 PM > On Mon, May 18, 2009 at 03:47:31PM -0500, Woodruff, Richard wrote: > > The code flow is: > > - Wakeup event > > - ARM reboots and uses SOC mask ROM context restore helper > > - Mask ROM code jump to restore pointers with MMU OFF. > > - Restore code resets ARM CortexA8 state > > -*- Trustzone SMI calls are made to restore some secure state > > - Jump back from SRAM to C code > > > > The dangling question to me is if any of the cpu state is needed by > > the trustzone monitor code as a precondition. The doc's I have led > > me to believe its ok, but I've not verified this. > > There shouldn't be - it would be _really_ silly if the trustzone monitor > had pre-requisits for the exception mode registers. There's no way that > Linux's use of the exception mode registers is anywhere near the same as > other OSes, and Linux's use of the exception mode registers has changed > over time anyway. We make no promise that we'll keep the way we're using > these registers today either. > > So there are no external guarantees about what useful state the kernel > puts into these registers. Having external code rely on some state > there would itself be broken. > > There's also the problem that if there was a dependency on the insecure > exception mode registers from within the trusted environment, that'd > surely be a rather big security problem in itself. Yes those are reasonable comments. In OMAP2 time security required nasty hacks to Linux vectors code (and other OSes). For OMAP3 the security API does take parameters to say if local OS interrupts are allowed or not. In the case they are there the vector base in monitor is used. Some stacking does occur to allow return. So yes as long as local IRQs are off it should be ok. Changes still need validation. Security does something to SOC resources like DMA which have caused confusion. For instance you call the API and specify a DMA channel. This is ok. But you will find some status left in channel which needs clean up before a successful sleep can happen. Regards, Richard W. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap
On Mon, May 18, 2009 at 03:47:31PM -0500, Woodruff, Richard wrote: > The code flow is: > - Wakeup event > - ARM reboots and uses SOC mask ROM context restore helper > - Mask ROM code jump to restore pointers with MMU OFF. > - Restore code resets ARM CortexA8 state > -*- Trustzone SMI calls are made to restore some secure state > - Jump back from SRAM to C code > > The dangling question to me is if any of the cpu state is needed by > the trustzone monitor code as a precondition. The doc's I have led > me to believe its ok, but I've not verified this. There shouldn't be - it would be _really_ silly if the trustzone monitor had pre-requisits for the exception mode registers. There's no way that Linux's use of the exception mode registers is anywhere near the same as other OSes, and Linux's use of the exception mode registers has changed over time anyway. We make no promise that we'll keep the way we're using these registers today either. So there are no external guarantees about what useful state the kernel puts into these registers. Having external code rely on some state there would itself be broken. There's also the problem that if there was a dependency on the insecure exception mode registers from within the trusted environment, that'd surely be a rather big security problem in itself. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap
> From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] > Sent: Monday, May 18, 2009 3:25 PM > The call to the enter method essentially calls the core specific suspend > function (eg, pxa25x_cpu_suspend()), which is an assembly function which > ultimately ends up powering down the core resulting in full loss of state > in the CPU. We seem to be able to avoid having to save the exception mode > registers there. > > The same thing is done with sa11x0 and Samsung SoCs. > > I don't see any reason for OMAP to be "special" in this regard. The code flow is: - Wakeup event - ARM reboots and uses SOC mask ROM context restore helper - Mask ROM code jump to restore pointers with MMU OFF. - Restore code resets ARM CortexA8 state -*- Trustzone SMI calls are made to restore some secure state - Jump back from SRAM to C code The dangling question to me is if any of the cpu state is needed by the trustzone monitor code as a precondition. The doc's I have led me to believe its ok, but I've not verified this. Regards, Richard W. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap
On Mon, May 18, 2009 at 02:04:19PM -0500, Woodruff, Richard wrote:
> cpu_init() is not yet accessible at the point this code is running.
Not at that _exact_ point, but there's no need what so ever for it
to be at that exact point. There's nothing stopping a call to
cpu_init() happening later. Much later.
> You could reduce the context perhaps trying to optimize to what the
> Linux-ARM implementation is today or do a more generic save like is
> there now.
>
> This code is copied into place from the .S code into SRAM. Its
> position independent and is not linked for this address. If you want
> to call functions outside of it you need to manually setup linkage.
I wouldn't want to.
> Calling functions on the way _UP_ from wake is NOT easy (like the one
> you propose) as the MMU is not yet enabled. The MMU is enabled only
> below this. Calling cpu_init() is not do able here. Even if you
> dress up the calling pointer to the physical address, it won't work
> as cpu_init() makes a global variable dereferences (smp_processor_id
> & stacks[]).
As long as IRQs and FIQs are disabled, you are in a 100% predictable
environment.
1. No IRQ or FIQ will be entered; if they are the CPU is buggy.
2. No data or prefetch abort will be entered _unless_ you purposely
access some non-present memory (and you're not exactly going to
start paging memory in with IRQs disabled.)
So restoring the abort and IRQ mode register state is just plain not
necessary in your SRAM code.
Let's look at the code. Here are the pertinent bits from Kevin's patch:
static void omap3_pm_idle(void)
{
local_irq_disable();
local_fiq_disable();
omap_sram_idle();
local_fiq_enable();
local_irq_enable();
}
static void omap_sram_idle(void)
{
_omap_sram_idle(NULL, save_state);
}
_omap_sram_idle = omap_sram_push(omap34xx_cpu_suspend,
omap34xx_cpu_suspend_sz);
pm_idle = omap3_pm_idle;
_omap_sram_idle() is the assembly code we're discussing, and here we're
referring to its version in SRAM. From the above code, we can clearly
see that this is entered with FIQs and IRQs disabled. So everything
inside omap_sram_idle() is running in a well defined environment provided
prefetch and data aborts don't happen.
There is nothing stopping this from becoming:
static void omap3_pm_idle(void)
{
local_irq_disable();
local_fiq_disable();
omap_sram_idle();
+ cpu_init();
local_fiq_enable();
local_irq_enable();
}
and having the saving of IRQ, FIQ and abort mode registers removed
from the SRAM code.
Other SoCs do precisely this - let's look at PXA:
pxa_cpu_pm_fns->enter(state);
cpu_init();
The call to the enter method essentially calls the core specific suspend
function (eg, pxa25x_cpu_suspend()), which is an assembly function which
ultimately ends up powering down the core resulting in full loss of state
in the CPU. We seem to be able to avoid having to save the exception mode
registers there.
The same thing is done with sa11x0 and Samsung SoCs.
I don't see any reason for OMAP to be "special" in this regard.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap
> From: Kevin Hilman [mailto:khil...@deeprootsystems.com] > Sent: Monday, May 18, 2009 12:09 PM > To: Russell King - ARM Linux; Woodruff, Richard > > There is a function which re-initializes the abort mode registers already - > > cpu_init(). Please use that if possible instead. > > Upon a quick glance, using cpu_init() would not cover all the things > that this code does. cpu_init() only handles the init of sp for the > various modes, where this code saves/resores all the banked registers: > sp, lr and spsr as well as r8-r12 of FIQ mode. cpu_init() is not yet accessible at the point this code is running. You could reduce the context perhaps trying to optimize to what the Linux-ARM implementation is today or do a more generic save like is there now. This code is copied into place from the .S code into SRAM. Its position independent and is not linked for this address. If you want to call functions outside of it you need to manually setup linkage. Calling outside functions on the way _DOWN_ is ok. For instance in current TI reference code all cache flush is calling to kernel functions. This saves in space, duplication maintenance and is actually _much_ quicker as it executes from a cached address range. The cache flush code takes 770uS from kernel area and takes 2.2mS from sram (non-cached map). Calling functions on the way _UP_ from wake is NOT easy (like the one you propose) as the MMU is not yet enabled. The MMU is enabled only below this. Calling cpu_init() is not do able here. Even if you dress up the calling pointer to the physical address, it won't work as cpu_init() makes a global variable dereferences (smp_processor_id & stacks[]). Regards, Richard W. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap
On Mon, May 18, 2009 at 10:08:36AM -0700, Kevin Hilman wrote: > [ adding Richard W. to To: since he can probably shed some light here ] > > Russell King - ARM Linux writes: > > > On Fri, May 15, 2009 at 11:40:41AM -0700, Kevin Hilman wrote: > >> This patch is to sync the core linux-omap PM code with mainline. This > >> code has evolved and been used for a while the linux-omap tree, but > >> the attempt here is to finally get this into mainline. > > [...] > > Upon a quick glance, using cpu_init() would not cover all the things > that this code does. cpu_init() only handles the init of sp for the > various modes, where this code saves/resores all the banked registers: > sp, lr and spsr as well as r8-r12 of FIQ mode. It doesn't touch sp, lr and spsr for the other modes because there's no requirement to do so. Think about it for a moment. Is there any point to saving LR and SPSR for the abort, FIQ and IRQ modes? No - LR and SPSR are overwritten by the hardware immediately upon entry to the relevent mode via an exception. What about SP? We use SP in the vector entry code, and we need to ensure that this is re-initialized. We never change their value, so we know that these are constant, and therefore if we initialize them once we can repeat the same initialization to restore their values when those values are lost. This is precisely what cpu_init() does. As for FIQ r8-r12, yes this is missing, and it's something that needs to be handled by _every_ platform which puts the CPU into low power state, be that PXA, S3C2410, and so forth. We have code to allow the FIQ vector to be managed, and as yet those architectures which do support sleep modes don't use FIQ mode, hence why there's nothing for it yet. Feel free to add the necessary suspend/resume code to (eg) arch/arm/kernel/fiq.c to fill this hole though. Lastly is system mode. The kernel doesn't use system mode, and this mode doesn't exist in all CPUs (its marked as unpredictable on many older CPUs.) We need a _generic_ extension to support that mode _if_ it is used. Presently it is not used, and so we simply don't bother saving those registers. > The question in my mind however is whether the lr and spsr need to be > saved/restored? Do we really need to preserve the context of these > handlers across idle? Idle is called essentially in process context, with the exception that it's SVC mode rather than user mode. That means no exception handlers will be running. So I think you'll find cpu_init() does everything you require. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap
Russell King - ARM Linux writes: > On Mon, May 18, 2009 at 10:00:44AM -0700, Kevin Hilman wrote: >> Russell King - ARM Linux writes: >> >> > On Fri, May 15, 2009 at 11:40:41AM -0700, Kevin Hilman wrote: >> >> This patch is to sync the core linux-omap PM code with mainline. This >> >> code has evolved and been used for a while the linux-omap tree, but >> >> the attempt here is to finally get this into mainline. >> >> >> >> Following this will be a series of patches from the 'PM branch' of the >> >> linux-omap tree to add full PM hardware support from the linux-omap >> >> tree. >> >> >> >> Much of this PM core code was written by Jouni Hogander with >> >> significant contributions from Paul Walmsley as well as many others >> >> from Nokia, Texas Instruments and linux-omap community. >> > >> > Overall comment, I think we need to rework the idle support code so >> > that enable_hlt/disable_hlt can be used even when pm_idle has been >> > overridden, rather than OMAP going off and inventing its own mechanisms. >> >> Would adding: >> >> if (hlt_counter) >> cpu_relax(); >> >> to the beginning of omap*_pm_idle functions be sufficient? That will >> at least allow the hlt stuff to behave as expected. > > Yes, but the comment was also directed at the other functions which > increment/decrement that atomic_t variable to enable/disable sleep mode. Ah, right. The sleep_block stuff is only used in OMAP2, we don't use that anymore in OMAP3. In fact, even OMAP2 users of this should be updated to use the OMAP PM layer, so I'll just drop the sleep_block hooks. >> The only thing the OMAP /sys/power/sleep_while_idle hook adds to this >> functionality is the ability to control this from sysfs. >> >> Any objections to /sys/power/enable_hlt? > > That seems to be rather dangerous, even with your atomic based code which > bugs if the count goes below zero. Atomic sleep_block code to be removed. > The problem here is that such an interface is extremely fragile. Consider > what happens if a program disables HLT, and then gets killed off for some > reason. How does this reference get balanced again? > > I think a better solution would be a char device driver which has to be > kept open as long as a reference is held. When userspace closes it (be > that because the program has exited, been killed, etc) you can drop any > pending references. OK, this interface is not intended for users/applications. It is intended only for OMAP PM developers who are developing the PM code and want to prevent idle for various reasons during development. It is not intended for productions systems. What about leaving /sys/power/sleep_while_idle but only if CONFIG_PM_DEBUG=y? Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap
[ adding Richard W. to To: since he can probably shed some light here ]
Russell King - ARM Linux writes:
> On Fri, May 15, 2009 at 11:40:41AM -0700, Kevin Hilman wrote:
>> This patch is to sync the core linux-omap PM code with mainline. This
>> code has evolved and been used for a while the linux-omap tree, but
>> the attempt here is to finally get this into mainline.
[...]
[excerpt of sleep34xx.S]
>> +/* IRQ mode */
>> +bicr0, r7, #0x1F
>> +orrr0, r0, #0x12
>> +msrcpsr, r0 /*go into IRQ mode*/
>> +ldmia r3!,{r4-r6} /*load the SP and LR from SDRAM*/
>> +movsp, r4 /*update the SP */
>> +movlr, r5 /*update the LR */
>> +msrspsr, r6 /*update the SPSR */
>> +
>> +/* ABORT mode */
>> +bicr0, r7, #0x1F
>> +orrr0, r0, #0x17
>> +msrcpsr, r0 /* go into ABORT mode */
>> +ldmia r3!,{r4-r6} /*load the SP and LR from SDRAM */
>> +movsp, r4 /*update the SP */
>> +movlr, r5 /*update the LR */
>> +msrspsr, r6 /*update the SPSR */
>> +
>> +/* UNDEEF mode */
>> +bicr0, r7, #0x1F
>> +orrr0, r0, #0x1B
>> +msrcpsr, r0 /*go into UNDEF mode */
>> +ldmia r3!,{r4-r6} /*load the SP and LR from SDRAM */
>> +movsp, r4 /*update the SP*/
>> +movlr, r5 /*update the LR*/
>> +msrspsr, r6 /*update the SPSR*/
>> +
>> +/* SYSTEM (USER) mode */
>> +bicr0, r7, #0x1F
>> +orrr0, r0, #0x1F
>> +msrcpsr, r0 /*go into USR mode */
>> +ldmia r3!,{r4-r6} /*load the SP and LR from SDRAM*/
>> +movsp, r4 /*update the SP */
>> +movlr, r5 /*update the LR */
>> +msrspsr, r6 /*update the SPSR */
>> +msrcpsr, r7 /*back to original mode*/
>
> There is a function which re-initializes the abort mode registers already -
> cpu_init(). Please use that if possible instead.
Upon a quick glance, using cpu_init() would not cover all the things
that this code does. cpu_init() only handles the init of sp for the
various modes, where this code saves/resores all the banked registers:
sp, lr and spsr as well as r8-r12 of FIQ mode.
The question in my mind however is whether the lr and spsr need to be
saved/restored? Do we really need to preserve the context of these
handlers across idle? Presumably we should not hit idle/suspend in
the middle of one of these handlers, so do we need to save anything
other than the stp? Maybe Richard can shed some light here as to why
that was added.
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap
On Mon, May 18, 2009 at 10:00:44AM -0700, Kevin Hilman wrote: > Russell King - ARM Linux writes: > > > On Fri, May 15, 2009 at 11:40:41AM -0700, Kevin Hilman wrote: > >> This patch is to sync the core linux-omap PM code with mainline. This > >> code has evolved and been used for a while the linux-omap tree, but > >> the attempt here is to finally get this into mainline. > >> > >> Following this will be a series of patches from the 'PM branch' of the > >> linux-omap tree to add full PM hardware support from the linux-omap > >> tree. > >> > >> Much of this PM core code was written by Jouni Hogander with > >> significant contributions from Paul Walmsley as well as many others > >> from Nokia, Texas Instruments and linux-omap community. > > > > Overall comment, I think we need to rework the idle support code so > > that enable_hlt/disable_hlt can be used even when pm_idle has been > > overridden, rather than OMAP going off and inventing its own mechanisms. > > Would adding: > > if (hlt_counter) > cpu_relax(); > > to the beginning of omap*_pm_idle functions be sufficient? That will > at least allow the hlt stuff to behave as expected. Yes, but the comment was also directed at the other functions which increment/decrement that atomic_t variable to enable/disable sleep mode. > The only thing the OMAP /sys/power/sleep_while_idle hook adds to this > functionality is the ability to control this from sysfs. > > Any objections to /sys/power/enable_hlt? That seems to be rather dangerous, even with your atomic based code which bugs if the count goes below zero. The problem here is that such an interface is extremely fragile. Consider what happens if a program disables HLT, and then gets killed off for some reason. How does this reference get balanced again? I think a better solution would be a char device driver which has to be kept open as long as a reference is held. When userspace closes it (be that because the program has exited, been killed, etc) you can drop any pending references. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap
Russell King - ARM Linux writes:
> On Fri, May 15, 2009 at 11:40:41AM -0700, Kevin Hilman wrote:
>> This patch is to sync the core linux-omap PM code with mainline. This
>> code has evolved and been used for a while the linux-omap tree, but
>> the attempt here is to finally get this into mainline.
>>
>> Following this will be a series of patches from the 'PM branch' of the
>> linux-omap tree to add full PM hardware support from the linux-omap
>> tree.
>>
>> Much of this PM core code was written by Jouni Hogander with
>> significant contributions from Paul Walmsley as well as many others
>> from Nokia, Texas Instruments and linux-omap community.
>
> Overall comment, I think we need to rework the idle support code so
> that enable_hlt/disable_hlt can be used even when pm_idle has been
> overridden, rather than OMAP going off and inventing its own mechanisms.
Would adding:
if (hlt_counter)
cpu_relax();
to the beginning of omap*_pm_idle functions be sufficient? That will
at least allow the hlt stuff to behave as expected.
The only thing the OMAP /sys/power/sleep_while_idle hook adds to this
functionality is the ability to control this from sysfs.
Any objections to /sys/power/enable_hlt?
>> diff --git a/arch/arm/mach-omap2/pm24xx.c b/arch/arm/mach-omap2/pm24xx.c
>> new file mode 100644
>> index 000..2a2d1a3
>> --- /dev/null
>> +++ b/arch/arm/mach-omap2/pm24xx.c
>> @@ -0,0 +1,557 @@
>> +/*
>> + * OMAP2 Power Management Routines
>> + *
>> + * Copyright (C) 2005 Texas Instruments, Inc.
>> + * Copyright (C) 2006-2008 Nokia Corporation
>> + *
>> + * Written by:
>> + * Richard Woodruff
>> + * Tony Lindgren
>> + * Juha Yrjola
>> + * Amit Kucheria
>> + * Igor Stoppa
>> + *
>> + * Based on pm.c for omap1
>> + *
>> + * 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
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include
>> +#include
>> +#include
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>
> Should be linux/gpio.h
Ack
>> +/*
>> + * Note that you can use clock_event_device->min_delta_ns if you want to
>> + * avoid reprogramming timer too often when using CONFIG_NO_HZ.
>> + */
>> +static void omap2_pm_idle(void)
>> +{
>> +local_irq_disable();
>> +local_fiq_disable();
>> +
>> +if (!omap2_can_sleep()) {
>> +if (!atomic_read(&sleep_block) && omap2_irq_pending())
>> +goto out;
>> +omap2_enter_mpu_retention();
>> +goto out;
>> +}
>> +
>> +if (omap2_irq_pending())
>> +goto out;
>> +
>> +omap2_enter_full_retention();
>> +
>> +out:
>> +local_fiq_enable();
>> +local_irq_enable();
>> +}
>
> It's totally unclear what the comment above the function has to do with
> the function itself.
Indeed. Will be removed.
>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
>> new file mode 100644
>> index 000..0fb6bec
>> --- /dev/null
>> +++ b/arch/arm/mach-omap2/pm34xx.c
>> @@ -0,0 +1,607 @@
>> +/*
>> + * OMAP3 Power Management Routines
>> + *
>> + * Copyright (C) 2006-2008 Nokia Corporation
>> + * Tony Lindgren
>> + * Jouni Hogander
>> + *
>> + * Copyright (C) 2005 Texas Instruments, Inc.
>> + * Richard Woodruff
>> + *
>> + * Based on pm.c for omap1
>> + *
>> + * 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
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include
>
> Should be linux/gpio.h
Ack
>> +static void omap3_pm_idle(void)
>> +{
>> +local_irq_disable();
>> +local_fiq_disable();
>> +
>> +if (!omap3_can_sleep())
>> +goto out;
>> +
>> +if (omap_irq_pending())
>> +goto out;
>
> So what happens if an IRQ becomes pending at this precise point?
Then it gets missed this time, and will be triggered upon wakeup. If
it's a wakeup-capable interrupt, then it will wake the system
immediately.
In subsequent series of the PM branch, this will be going away in
favor of using the [enable|disable]_irq_wake() and the lazy IRQ
disabling features.
>> +
>> +omap_sram_idle();
>> +
>> +out:
>> +local_fiq_enable();
>> +local_irq_enable();
>> +}
>> +/* IRQ mode */
>> +bicr0, r7, #0x1F
>> +orrr0, r0, #0x12
>> +msrcpsr, r0 /*go into IRQ mode*/
>> +ldmia r3!,{r4-r6} /*load the SP and LR from SDRAM*/
>> +movsp, r4 /*update the SP */
>> +movlr, r5 /*update the LR */
>> +msrspsr, r6 /*update the SPSR */
>> +
>> +/* ABORT mode */
>> +bicr0, r7, #0x1F
>> +
Re: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap
On Fri, May 15, 2009 at 11:40:41AM -0700, Kevin Hilman wrote:
> This patch is to sync the core linux-omap PM code with mainline. This
> code has evolved and been used for a while the linux-omap tree, but
> the attempt here is to finally get this into mainline.
>
> Following this will be a series of patches from the 'PM branch' of the
> linux-omap tree to add full PM hardware support from the linux-omap
> tree.
>
> Much of this PM core code was written by Jouni Hogander with
> significant contributions from Paul Walmsley as well as many others
> from Nokia, Texas Instruments and linux-omap community.
Overall comment, I think we need to rework the idle support code so
that enable_hlt/disable_hlt can be used even when pm_idle has been
overridden, rather than OMAP going off and inventing its own mechanisms.
> diff --git a/arch/arm/mach-omap2/pm24xx.c b/arch/arm/mach-omap2/pm24xx.c
> new file mode 100644
> index 000..2a2d1a3
> --- /dev/null
> +++ b/arch/arm/mach-omap2/pm24xx.c
> @@ -0,0 +1,557 @@
> +/*
> + * OMAP2 Power Management Routines
> + *
> + * Copyright (C) 2005 Texas Instruments, Inc.
> + * Copyright (C) 2006-2008 Nokia Corporation
> + *
> + * Written by:
> + * Richard Woodruff
> + * Tony Lindgren
> + * Juha Yrjola
> + * Amit Kucheria
> + * Igor Stoppa
> + *
> + * Based on pm.c for omap1
> + *
> + * 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
> + * published by the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
Should be linux/gpio.h
> +/*
> + * Note that you can use clock_event_device->min_delta_ns if you want to
> + * avoid reprogramming timer too often when using CONFIG_NO_HZ.
> + */
> +static void omap2_pm_idle(void)
> +{
> + local_irq_disable();
> + local_fiq_disable();
> +
> + if (!omap2_can_sleep()) {
> + if (!atomic_read(&sleep_block) && omap2_irq_pending())
> + goto out;
> + omap2_enter_mpu_retention();
> + goto out;
> + }
> +
> + if (omap2_irq_pending())
> + goto out;
> +
> + omap2_enter_full_retention();
> +
> +out:
> + local_fiq_enable();
> + local_irq_enable();
> +}
It's totally unclear what the comment above the function has to do with
the function itself.
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> new file mode 100644
> index 000..0fb6bec
> --- /dev/null
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -0,0 +1,607 @@
> +/*
> + * OMAP3 Power Management Routines
> + *
> + * Copyright (C) 2006-2008 Nokia Corporation
> + * Tony Lindgren
> + * Jouni Hogander
> + *
> + * Copyright (C) 2005 Texas Instruments, Inc.
> + * Richard Woodruff
> + *
> + * Based on pm.c for omap1
> + *
> + * 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
> + * published by the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
Should be linux/gpio.h
> +static void omap3_pm_idle(void)
> +{
> + local_irq_disable();
> + local_fiq_disable();
> +
> + if (!omap3_can_sleep())
> + goto out;
> +
> + if (omap_irq_pending())
> + goto out;
So what happens if an IRQ becomes pending at this precise point?
> +
> + omap_sram_idle();
> +
> +out:
> + local_fiq_enable();
> + local_irq_enable();
> +}
> + /* IRQ mode */
> + bicr0, r7, #0x1F
> + orrr0, r0, #0x12
> + msrcpsr, r0 /*go into IRQ mode*/
> + ldmia r3!,{r4-r6} /*load the SP and LR from SDRAM*/
> + movsp, r4 /*update the SP */
> + movlr, r5 /*update the LR */
> + msrspsr, r6 /*update the SPSR */
> +
> + /* ABORT mode */
> + bicr0, r7, #0x1F
> + orrr0, r0, #0x17
> + msrcpsr, r0 /* go into ABORT mode */
> + ldmia r3!,{r4-r6} /*load the SP and LR from SDRAM */
> + movsp, r4 /*update the SP */
> + movlr, r5 /*update the LR */
> + msrspsr, r6 /*update the SPSR */
> +
> + /* UNDEEF mode */
> + bicr0, r7, #0x1F
> + orrr0, r0, #0x1B
> + msrcpsr, r0 /*go into UNDEF mode */
> + ldmia r3!,{r4-r6} /*load the SP and LR from SDRAM */
> + movsp, r4 /*update the SP*/
> + movlr, r5 /*update the LR*/
> + msrspsr, r6 /*update the SPSR*/
> +
> + /* SYSTEM (USER) mode */
> + bicr0, r7, #0x1F
> + orrr0, r0, #0x1F
> + msrcpsr, r0 /*go into USR mode */
> + ldmia r3!,{r4-r6} /*load the SP and LR from SDR
