Re: [PATCH 01/11] OMAP2/3: PM: push core PM code from linux-omap

[Kevin Hilman <khilman@deeprootsystems.com>]

Russell King - ARM Linux <linux@arm.linux.org.uk> 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