Skip to content

Add workaround for STM32F4 hardfault in sleep mode #12662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 30, 2020
Merged

Add workaround for STM32F4 hardfault in sleep mode #12662

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40672c5
Disable sleep on STM32F4 as an workaround for stability issues.
teetak01 Jan 23, 2020
5aa66b5
Add workaround for F429 hardfault
Mar 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions targets/TARGET_STM/TARGET_STM32F4/device/stm32f4xx_hal_pwr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,9 @@ void HAL_PWR_EnterSLEEPMode(uint32_t Regulator, uint8_t SLEEPEntry)
{
/* Request Wait For Interrupt */
__WFI();
__NOP(); // Workaround for STM32F4 errata
__NOP(); // see chapter 2.1.3 - Debugging Sleep/Stop mode with WFE/WFI entry
__NOP(); // https://www.st.com/resource/en/errata_sheet/dm00037591-stm32f405-407xx-and-stm32f415-417xx-device-limitations-stmicroelectronics.pdf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the WFE() in below lines ? do we need them to be patched as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mbed OS doesn't use that mode, so not required for us - would just be an unnecessary image size increase. But if the change is being upstreamed to the HAL, seems like you'd want them there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the upstream comes to our repo, this will be updated once driver is updated.

@jeromecoutant We fine as it is here now, correct?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure that ST driver team will accept this change :-(
But as it solves some mbed issues, we are ok to merge this wrkaround.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better alternative, given the errata description? That is the most universally-feasible workaround in your errata sheet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As soon as confirmed, I'll merge this. It would be good to have this in today.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LMESTM if you don't mind, we'll proceed with this change for both debug and release for now, as there are other random failures in release mode that we're seeing in CI and need to investigate and rule out for the limited time we have.
Then we can introduce optimizations once we have more information.

Ok. If RELEASE build is used in CI as well, then the same loading mechanism will apply (thru debug port) and the same issue may arise. So we can't put the proposed compilation switches - please forget my comment and feel free to proceed ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a separate argument that something should be done to get that debug/sleep control register back into its default state anyway, otherwise the just-flashed release build in test is not representative of the deployed image in terms of sleep, whether that comes down to power measurements, timing, hitting bugs like this. Arguably release builds should be manually putting that back to its zero state. Flasher should also have taken more care...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We think it's the flasher role to do so. If you power cycle the product, the release build will work just fine ... I'm not sure we should add an explicit reset of those bits, because even the release build should be debugged and in this case the flasher or debugger (or flasher of the debugger) should set the bits.

Copy link
Contributor

@kjbracey kjbracey Mar 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know bigger systems like Linux are intensely paranoid about chip state post-bootloader, so tend not to rely on any default state. But we can't really afford the ROM space for that level of paranoia in an embedded image.

And yes, in this specific case, I see your point that a debugger may obviously have deliberately set those bits before running us, for a good reason, so an image should not reset any bits there, regardless of the general "trust default state or not" philosphy.

}
else
{
Expand Down