"Kristo Tero (Nokia-D/Tampere)" <Tero.Kristo@xxxxxxxxx> writes:
> Hi,
>
> Quick counter comments here. :P
>
>>This will leave cx->core_state to its previous value in case
>>of ON state. So just pwrdm_set_next_pwrst(core_pd,
>>cx->core_state) without if is better.
>
> I did not really consider the logic of cpu-idle code too much, just moved context / save functionality to omap_sram_idle. I just made suspend + dynamic idle work.
>
>>> + save_core = (pwrdm_read_next_pwrst(core_pwrdm) ==
>>PWRDM_POWER_OFF);
>>> + save_per = (pwrdm_read_next_pwrst(per_pwrdm) ==
>>> PWRDM_POWER_OFF);
>>
>>Just read next states here.
>
> I am comparing the values to PWRDM_POWER_OFF, because you will only
> need to save context if you enter OFF state.
Yes, but then you might need to read them again:
core_not_on = (pwrdm_read_next_pwrst(core_pwrdm) == PWRDM_POWER_OFF);
per_not_on = (pwrdm_read_next_pwrst(core_pwrdm) == PWRDM_POWER_OFF);
>
>>
>>> +
>>> + if (save_core) {
>>> + omap3_save_core_ctx();
>>> + omap3_save_prcm_ctx();
>>> + }
>>
>>And do this if core next_st < PWRDM_POWER_ON
>
> Save is not needed if you enter PWRDM_POWER_RET.
Yes I meant whole block:
if (core_not_on) {
/* Some more code here */
if (save_core) {
omap3_save_core_ctx();
omap3_save_prcm_ctx();
}
}
>
>>
>>> +
>>> + if (save_per)
>>> + omap3_save_per_ctx();
>>
>>And same here. Additionally, do this only if core next_st <
>>PWRDM_POWER_ON.
>
> Per domain can in theory go to OFF even if core stays on (yes, there
> is some discussion about tying Core + Per because of the gpio
> dependencies, but we might find something else for this.)
No per can't enter sleep if core is not entering sleep. Please read
discussion in cpuidle patch thread.
>
>>
>>>
>>> - omap3_save_per_ctx();
>>> omap2_gpio_prepare_for_retention();
>>>
>>> /* XXX This is for gpio fclk hack. Will be removed as
>>gpio driver
>>> * handqles fcks correctly */
>>> per_gpio_clk_disable();
>>>
>>> - omap_save_uart_ctx();
>>> + if (save_per)
>>> + omap_save_uart_ctx();
>>
>>Again, do this only if core next_st < PWRDM_POWER_ON and per
>>next_st == PWRDM_POWER_OFF.
>
> Same here.
>
>>
>>> +
>>> omap_serial_enable_clocks(0);
>>
>>Consider Rajendras idea to do this only if needed.
>
> I think the idea was to access uart clocks only if we can assume
> that per_pwrdm will enter ret / off? Yes good idea.
Yes, simpler option is to do this only if core or per is going to
sleep.
>
>>> /* XXX This is for gpio fclk hack. Will be removed as
>>gpio driver
>>> * handles fcks correctly */
>>>
>>> per_gpio_clk_enable();
>>> - omap3_restore_per_ctx();
>>> +
>>> + if (save_per)
>>> + omap3_restore_per_ctx();
>>>
>>> omap2_gpio_resume_after_retention();
>>
>>Same comments to restore part as before wfi. I think you
>>should look at what Rajendra has done (logic in
>>omap3_enter_idle). You might also want to look at previous
>>discussion related to this. Something like this:
>
> I have looked at this code, and I somehow consider it a bit too
> complicated for the purpose. The simpler setup I have implemented in
> omap_sram_idle is working, also all powerdomains have their next
> states set-up correctly when you reach omap_sram_idle. Anyway, it is
> easy to expand the code now that it actually works.
ok.
>
>>neon_pwrdm\n");
>>> + if (mpu_pwrdm == NULL || neon_pwrdm == NULL ||
>>per_pwrdm == NULL ||
>>> + core_pwrdm == NULL) {
>>> + printk(KERN_ERR "Failed to get pwrdm pointers\n");
>>
>>Neon handling is missing.
>
> Neon pwrdm is accessed only in the init code to make wakeup
> dependency from mpu -> neon. I have not tried out neon save /
> restore so far (just some vfp hack is needed.)
ok
>
> -Tero
>
>
--
Jouni Högander
--
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
