Skip to content
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.

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

drivers: flash: stm32f4: resets OPTCR register #18296

Closed
wants to merge 1 commit into from
Closed

drivers: flash: stm32f4: resets OPTCR register #18296

wants to merge 1 commit into from

Conversation

StefJar
Copy link

@StefJar StefJar commented Aug 15, 2019

the current stm32f4 flash code assumes that it is run after reset.
If a programm, like a bootloader, runs before the zephyr build firmware
it can happen that the FLASH controller registers are changed. In my
case this lead to failed read/write/erase operations. This fix addresses
the stm32f4 flash driver. It clears the OPTCR register and take care,
that the driver resets the flash settings to the reset defaults.

Fixes #18263

Signed-off-by: Stefan Jaritz stefan@kokoontech.com

the current stm32f4 flash code assumes that it is run after reset.
If a programm, like a bootloader, runs before the zephyr build firmware
it can happen that the FLASH controller registers are changed. In my
case this lead to failed read/write/erase operations. This fix addresses
the stm32f4 flash driver. It clears the OPTCR register and take care,
that the driver resets the flash settings to the reset defaults.

Fixes #18263

Signed-off-by: Stefan Jaritz <stefan@kokoontech.com>
@StefJar StefJar requested a review from superna9999 as a code owner August 15, 2019 09:52
Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

Aim of the option bytes is to configure a behavior persistent across reset.
Blindly erasing this configuration at boot makes quite little sense.

@@ -329,6 +329,28 @@ static int stm32_flash_init(struct device *dev)
LL_AHB3_GRP1_EnableClock(LL_AHB3_GRP1_PERIPH_HSEM);
#endif /* CONFIG_SOC_SERIES_STM32WBX */

#ifdef CONFIG_SOC_SERIES_STM32F4X
Copy link
Member

Choose a reason for hiding this comment

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

Since we'll need dedicated code for each series, this will soon get unreadable.
For code genericity, please use a function that will be implemented in each series dedicated file.

@@ -329,6 +329,28 @@ static int stm32_flash_init(struct device *dev)
LL_AHB3_GRP1_EnableClock(LL_AHB3_GRP1_PERIPH_HSEM);
#endif /* CONFIG_SOC_SERIES_STM32WBX */

#ifdef CONFIG_SOC_SERIES_STM32F4X
/*
* make sure that user options are disabled
Copy link
Member

Choose a reason for hiding this comment

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

"make sure that user options are disabled".
Well, this is your particular case. Other users may have correctly configured options they want to rely on.
And since this is the purpose of the option bytes, this should rather be the default behavior.
So I see this as a particular optional feature that should be default n.

Copy link
Author

Choose a reason for hiding this comment

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

I leave it to the ST guys to maintain the stm32 drivers. I quickly adopted the coding style from the code above. Think maybe a reset function for the flash driver would be useful. This function can be implemented different for each stm32.

return -EIO;
}
/* write reset config register content*/
regs->optcr = 0x0FFFFFED;
Copy link
Member

Choose a reason for hiding this comment

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

Looking to 2 F4 variants ref man, I've found reset value to 0x0FFFAAED

Copy link
Author

Choose a reason for hiding this comment

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

check RM0402 from st, it's the reference manual for the stm32f412 series. On page 77 you will find the value.

Copy link
Member

Choose a reason for hiding this comment

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

This would work for F412 but not F401 not F446... at least, while this code will be used for all F4 series.

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

Blindly resetting flash setting at driver init doesn't sound like a behavior that would be requested

@StefJar
Copy link
Author

StefJar commented Aug 16, 2019

Blindly resetting flash setting at driver init doesn't sound like a behavior that would be requested

The reset of the flash controler is done once at the init of the flash driver. So it is not blindly. The current implementation of the flash driver running on a stm32f412 is not taking care of the OPTCR register settings.
Maybe the reset or unlock should move into the flash_write_protection_set function

@erwango
Copy link
Member

erwango commented Aug 16, 2019

The reset of the flash controler is done once at the init of the flash driver. So it is not blindly.

It is done irrespective of the other option bytes settings.
What if WDG_SW of BOR_LEV bits are set ?

@StefJar
Copy link
Author

StefJar commented Aug 16, 2019

after discussing it with @erwango we decided to close this merge. Key points:
1st) this code only works for STM32F412
2nd) it sets blindly the flash OPTCR register to reset mode. Maybe this is not wanted.
3rd) we leave it to the user to know from which context his zephyr image is run. So the user needs to take care of this register.

I put the code to a git repro. so everyone can use this as a blueprint for similar problems.

https://github.com/StefJar/zephyr_stm32_flashOPTCRreset

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flash sector erase fails on stm32f412
2 participants