cpu/stm32_common: fix stm32f1/2/4 pm #7385
Conversation
There was a problem hiding this comment.
Please add a Readme for the new test application. The expected behavior of the test and how to use it needs to be stated somewhere. Even better would be if there was an expect script for it like a few of the other tests have, though I won't reject this PR if you don't have it.
The kinetis changes are OK from a functionality view point, but do you think there is another way that avoids adding an identical one line file to each kinetis CPU?
I am thinking about the ease of maintenance in the future, for us maintainers.
OK, I will. The purpose was first to be sure to compile the pm_layered somewhere and have a simple app to unblock modes and measure power consumption.
I don't know. The feature needs to be 'exported' in a way to the global makefiles. The kinetis common one seemed to not be included in any other makefile. The simplest way was then to include from every kinetis.
Indeed, could be a better choice, but I had no board with an working RTT. I'll try to add the option with some |
|
Well, RTT support turns out to be a hell if I want to have an app which is compatible with both RTC and RTT. Some cpus implement the RTC and RTT with the same hardware periph, then are mutually exclusive. When using |
|
Readme added |
|
thanks a lot, I was hitting this issue on a customer board. I'll be AFK for the next two weeks, if this is still open, I'll test asap. |
| /* Set SLEEPDEEP bit of system control block */ | ||
| deep = 1; | ||
| break; | ||
| } | ||
| #endif | ||
|
|
||
| cortexm_sleep(deep); | ||
|
|
||
| #if defined(CPU_FAM_STM32F1) || defined(CPU_FAM_STM32F2) || defined(CPU_FAM_STM32F4) |
There was a problem hiding this comment.
if done here, doesn't this delay reinitialization until idle is scheduled after the isr that triggered wakeup and any possibly scheduled threads?
There was a problem hiding this comment.
I didn't think about that...but it looks like it could be more complicated that what I expected.
I guess the solution would be to re-initialize the PLL from the interrupt which triggered the wake-up...but this would imply more code impact.
There was a problem hiding this comment.
Interrupts are disabled in pm_set_lowest so I guess interrupts are only handled after executing the clock init code and re-enabling interrupts in pm_set_lowest
edit:
It seems that I was wrong. cortexm_sleep also disables and enables interrupts
There was a problem hiding this comment.
I ran another test.
With the current code, it works as @kaspar030 pointed out: another thread can be scheduled before PLL is reinitialized.
If we change cortexm_sleep() from
static inline void cortexm_sleep(int deep)
{
if (deep) {
SCB->SCR |= (SCB_SCR_SLEEPDEEP_Msk);
}
else {
SCB->SCR &= ~(SCB_SCR_SLEEPDEEP_Msk);
}
/* ensure that all memory accesses have completed and trigger sleeping */
__disable_irq();
__DSB();
__WFI();
__enable_irq();
}to:
static inline void cortexm_sleep(int deep)
{
if (deep) {
SCB->SCR |= (SCB_SCR_SLEEPDEEP_Msk);
}
else {
SCB->SCR &= ~(SCB_SCR_SLEEPDEEP_Msk);
}
/* ensure that all memory accesses have completed and trigger sleeping */
unsigned state = irq_disable();
__DSB();
__WFI();
irq_restore(state);
}i.e. replacing __disable_irq()/__enable_irq() by irq_disable()/irq_restore(state).
Then the issue is no longer present: the clock is initialized, then the irq are restored and the interrupt is served, then only after threads can be scheduled.
So there are 2 choices: either we change cortexm_sleep as proposed here, or we remove disabling/re-enabling interrupt from cortexm_sleep and let the layers above taking care of interrupts.
@kaspar030 @gebart @haukepetersen what's your opinion on that?
There was a problem hiding this comment.
I've discussed this offline with @haukepetersen, and the latter solution (replace __disable_irq/__enable_irq with irq_disable/irq_restore) seems the most elegant to us. It only changes these two lines, and it should work with both boards using pm_layered and with boards that don't.
@vincent-d would you mind updating the PR accordingly?
There was a problem hiding this comment.
Seems to me it's the best, indeed.
Done.
Should be OK for a final review |
|
Needs a rebase. IMO it makes sense to split up this PR into stm32 fix, test application and kinetis fixes. |
|
rebased I could split in 2 PR, but kinetis fixes are needed for the test app |
please do! I can test nucleos, but not kinetis... |
|
Kinetis in current master has disabled all PM modes, so there's not much to test. I have a WIP branch for implementing low power in the periph drivers on Kinetis, not yet PRed though. |
kinetis fix is really only a build issue when compiling the test app, I guess it could stay in one PR. The test app is more convenient to test pm modes IMO. |
|
I split the stm32 code and test app/kinetis in 2 PRs. Hopefully, it will ease the review. |
It does. :) please squash! |
|
Squashed |
@kaspar030 does it mean you ack'ed? ;) |
|
When debugging #7687 git bisect brought me back around to this PR: apparently dd49f22 breaks the When doing |
|
opened an issue for this -> #8024 |
The goal of this PR was to fix stm32
pm_setimplementation. The clock needs to be reinitialized after leaving stop mode.I also added a simple test app formoved to #7666pm_layered, then I had to fix kinetis build issues.