-
Notifications
You must be signed in to change notification settings - Fork 8.2k
stm32: fix memc initialization to avoid XSPIM configuration is APP_IN_EXT_FLASH #98683
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
stm32: fix memc initialization to avoid XSPIM configuration is APP_IN_EXT_FLASH #98683
Conversation
Avoid copying the whole XSPI_HandleTypeDef structure into the init function and use a pointer since the structure is already part of the _data structure. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
The PSRAM could well be plugged to the XSPI2 instead of XSPI1 hence allow configuration of the IOPort and avoid forcing the ChipSelect in order to allow working on both XSPI1 or XSPI2. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
mathieuchopstm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM
drivers/memc/memc_stm32_xspi_psram.c
Outdated
| #ifndef CONFIG_STM32_APP_IN_EXT_FLASH | ||
| XSPIM_CfgTypeDef cfg = {0}; | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: prefer
| #ifndef CONFIG_STM32_APP_IN_EXT_FLASH | |
| XSPIM_CfgTypeDef cfg = {0}; | |
| #endif | |
| XSPIM_CfgTypeDef __maybe_unused cfg = {0}; |
(or see second comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mathieuchopstm. I did using the IS_ENABLED, hence allowing to always compile most of the code. Thanks.
drivers/memc/memc_stm32_xspi_psram.c
Outdated
| #ifndef CONFIG_STM32_APP_IN_EXT_FLASH | ||
| /* | ||
| * Do not configure the XSPIManager if running on the ext flash | ||
| * since this includes stopping each XSPI instance during configuration | ||
| */ | ||
| if (hxspi->Instance == XSPI1) { | ||
| cfg.IOPort = HAL_XSPIM_IOPORT_1; | ||
| } else if (hxspi->Instance == XSPI2) { | ||
| cfg.IOPort = HAL_XSPIM_IOPORT_2; | ||
| } | ||
| cfg.nCSOverride = HAL_XSPI_CSSEL_OVR_DISABLED; | ||
|
|
||
| if (HAL_XSPIM_Config(&hxspi, &cfg, HAL_XSPI_TIMEOUT_DEFAULT_VALUE) != HAL_OK) { | ||
| if (HAL_XSPIM_Config(hxspi, &cfg, HAL_XSPI_TIMEOUT_DEFAULT_VALUE) != HAL_OK) { | ||
| LOG_ERR("XSPIMgr Init failed"); | ||
| return -EIO; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: prefer
| #ifndef CONFIG_STM32_APP_IN_EXT_FLASH | |
| /* | |
| * Do not configure the XSPIManager if running on the ext flash | |
| * since this includes stopping each XSPI instance during configuration | |
| */ | |
| if (hxspi->Instance == XSPI1) { | |
| cfg.IOPort = HAL_XSPIM_IOPORT_1; | |
| } else if (hxspi->Instance == XSPI2) { | |
| cfg.IOPort = HAL_XSPIM_IOPORT_2; | |
| } | |
| cfg.nCSOverride = HAL_XSPI_CSSEL_OVR_DISABLED; | |
| if (HAL_XSPIM_Config(&hxspi, &cfg, HAL_XSPI_TIMEOUT_DEFAULT_VALUE) != HAL_OK) { | |
| if (HAL_XSPIM_Config(hxspi, &cfg, HAL_XSPI_TIMEOUT_DEFAULT_VALUE) != HAL_OK) { | |
| LOG_ERR("XSPIMgr Init failed"); | |
| return -EIO; | |
| } | |
| #endif | |
| if (!IS_ENABLED(CONFIG_STM32_APP_IN_EXT_FLASH)) { | |
| /* | |
| * Do not configure the XSPIManager if running on the ext flash | |
| * since this includes stopping each XSPI instance during configuration | |
| */ | |
| XSPIM_CfgTypeDef cfg = {0}; //[1] | |
| if (hxspi->Instance == XSPI1) { | |
| cfg.IOPort = HAL_XSPIM_IOPORT_1; | |
| } else if (hxspi->Instance == XSPI2) { | |
| cfg.IOPort = HAL_XSPIM_IOPORT_2; | |
| } | |
| cfg.nCSOverride = HAL_XSPI_CSSEL_OVR_DISABLED; | |
| if (HAL_XSPIM_Config(hxspi, &cfg, HAL_XSPI_TIMEOUT_DEFAULT_VALUE) != HAL_OK) { | |
| LOG_ERR("XSPIMgr Init failed"); | |
| return -EIO; | |
| } | |
| } |
Note proposal [1] to move variable declaration in this block (I'm not sure if it is necessary though, i.e. if GCC will flag as unused because the if() block is dead code)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
I ran into the same issue as @avolmat-st with the same board and can confirm that this PR fixes the issue. Thanks |
Avoid calling the HAL_XSPIM_Config function if the app is running from flash in order to avoid locking since HAL_XSPIM_Config is once turning off each XSPI instance when performing the configuration. Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
f11da3d to
6ced3a5
Compare
|
| cfg.nCSOverride = HAL_XSPI_CSSEL_OVR_DISABLED; | ||
| if (!IS_ENABLED(CONFIG_STM32_APP_IN_EXT_FLASH)) { | ||
| /* | ||
| * Do not configure the XSPIManager if running on the ext flash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, i would rephrase this inline comment since it's inside the conditioned source lines.
| * Do not configure the XSPIManager if running on the ext flash | |
| * Configure XSPIManager only if not running on the external flash |
Alternatively:
/* Do not configure the XSPIManager if running on the external flash
* since this includes stopping each XSPI instance during configuration.
*/
if (!IS_ENABLED(CONFIG_STM32_APP_IN_EXT_FLASH)) {
drivers/memc/memc_stm32_xspi_psram.c
Outdated
| } else if (hxspi->Instance == XSPI2) { | ||
| cfg.IOPort = HAL_XSPIM_IOPORT_2; | ||
| } | ||
| cfg.nCSOverride = HAL_XSPI_CSSEL_OVR_DISABLED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the previous implementation. No issue on already supported platforms? stm32h7s78_dk and stm32n6570_dk do use NCS1 only according to their pinctrl states.
Should their be an added DT property to define the chip select pin override config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should their be an added DT property to define the chip select pin override config?
makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the previous implementation. No issue on already supported platforms? stm32h7s78_dk and stm32n6570_dk do use NCS1 only according to their pinctrl states. Should their be an added DT property to define the chip select pin override config?
Yeah, this changes a bit indeed but I checked that it is still working fine on both H7RS / N6 boards. I did that within the same PR but might be actually better to have this split and done post v4.3.0 in a new PR ? As @erwango mentioned as well, XSPI3 can also be added and similar handling has to be propagated as well to the xspi_flash driver which has roughly the same thing to do as well.
Discussion / proposal about new DT property can also be added in that new PR. What do you think ?
If ok for all, I could well just keep the basic fix in this PR (about avoiding XSPIM_ configuration overall) and open a new PR for other enhancement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, let's keep it minimal for current PR and provide full configurability + flash impact for next DV.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, update to come shortly (and new PR as well).
drivers/memc/memc_stm32_xspi_psram.c
Outdated
| cfg.IOPort = HAL_XSPIM_IOPORT_1; | ||
| if (hxspi->Instance == XSPI1) { | ||
| cfg.IOPort = HAL_XSPIM_IOPORT_1; | ||
| } else if (hxspi->Instance == XSPI2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else { may be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually there should be a final else here with error return on invalid instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually N6 has a XSPI3 (which is not described yet though).
JarmouniA
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! Was wondering what was the issue following #97992 (comment)



While trying to use the display sample app on the STM32H7S78_DK with app being executed from FLASH, I ran into issues in which the application wouldn't start after MCUBoot.
Other app such as Hello World are running just fine from the flash.
Turns out that the issue is linked to MEMC xspi_psram driver which during its initialization is reconfiguring the XSPI Manager block and, in order to do that needs to first disable both XSPI instances (which I believe would lead to issues considering that the app is running from the XSPI2).
I fixed that by preventing the reconfiguration of the XSPIManager in case of the app is running in flash, relying on the CONFIG_STM32_APP_IN_EXT_FLASH=y config for that.
While modifying the code, I also avoid hardcodying the PSRAM on the XSPI1 as it is currently, doing similar thing as already done in the flash driver.
With those modification in place, it is now possible to have the display sample app running fine on the STM32H7S78_DK.
Fixes: #98844