-
Notifications
You must be signed in to change notification settings - Fork 8.3k
drivers: memc: stm32_xspi_psram: Add conditional HAL API support #99432
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
base: main
Are you sure you want to change the base?
drivers: memc: stm32_xspi_psram: Add conditional HAL API support #99432
Conversation
|
Hello @Ayushkothari96, and thank you very much for your first pull request to the Zephyr project! |
Add conditional compilation to support different STM32 XSPI HAL capabilities across product lines: - Use XSPIM (XSPI Manager) configuration when available (STM32H7RS) - Use 16-line data mode on STM32H7RS, 8-line on STM32H5 - Include MaxTran and MemorySelect Init fields only when XSPIM exists This allows the driver to automatically use full hardware capabilities on STM32H7RS (16-line XSPI with dual-port manager) while maintaining compatibility with STM32H5 series that have simpler single-port XSPI/OCTOSPI implementation with 8-line max. Related to zephyrproject-rtos/hal_stm32#328 Signed-off-by: Ayush Kothari <[email protected]>
9affaa1 to
cee9123
Compare
|
|
Already addressed by #99194 |
|
Thanks but a similar PR was already opended last week: #99194 |
| #else | ||
| /* STM32H5 max is 8 lines */ | ||
| cmd.DataMode = HAL_XSPI_DATA_8_LINES; | ||
| #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.
Use more generic wording:
| #else | |
| /* STM32H5 max is 8 lines */ | |
| cmd.DataMode = HAL_XSPI_DATA_8_LINES; | |
| #endif | |
| #else /* 16 lines mode not supported */ | |
| cmd.DataMode = HAL_XSPI_DATA_8_LINES; | |
| #endif |
| cmd.AddressWidth = HAL_XSPI_ADDRESS_32_BITS; | ||
| cmd.AddressDTRMode = HAL_XSPI_ADDRESS_DTR_ENABLE; | ||
| #if defined(HAL_XSPI_DATA_16_LINES) | ||
| /* Use 16-line if DT requests and hardware supports it */ |
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.
| /* Use 16-line if DT requests and hardware supports it */ | |
| /* Use 16 lines mode if DT requests and hardware supports it */ |
| #if DT_CLOCKS_HAS_NAME(STM32_XSPI_NODE, xspi_mgr) | ||
| 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.
Why move this?
| 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.
| #endif | |
| #endif /* DT_CLOCKS_HAS_NAME(STM32_XSPI_NODE, xspi_mgr) */ |
| #if DT_CLOCKS_HAS_NAME(STM32_XSPI_NODE, xspi_mgr) | ||
| .MaxTran = 0U, /* Communication regulation (STM32H7RS only) */ | ||
| #endif | ||
| .Refresh = 0x81U, | ||
| .MemorySelect = HAL_XSPI_CSSEL_NCS1, | ||
| #if DT_CLOCKS_HAS_NAME(STM32_XSPI_NODE, xspi_mgr) | ||
| .MemorySelect = HAL_XSPI_CSSEL_NCS1, /* Chip select (STM32H7RS only) */ | ||
| #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.
Regroup both under same check:
.Refresh = 0x81U,
#if DT_CLOCKS_HAS_NAME(STM32_XSPI_NODE, xspi_mgr)
.MaxTran = 0U, /* Communication regulation (STM32H7RS only) */
.MemorySelect = HAL_XSPI_CSSEL_NCS1, /* Chip select (STM32H7RS only) */
#endif+ if series-specific, it should be under #if SOC_SERIES - either change condition or comment
Hello @erwango PR #99194: Adds #ifdef guards in the driver using HAL macro detection My PR: Fixes the root cause by adding XSPI1→OCTOSPI1 compatibility macros in hal_stm32 (PR #328) + uses DT-based detection (DT_CLOCKS_HAS_NAME) |
While I agree it seems cleaner, I prefer the other one. Indeed we avoid patching the HAL to stay closer to upstream and avoid maintaining downstream patches. Unless ... the patch you propose could be accepted in https://github.com/STMicroelectronics/STM32CubeH5 |
Fair point about staying clos to upstream. However, stm32_hal_legacy.h exists specifically for these compatibility cases, it has already contains similar macros for other STM32 series. |
Agreed, but it remains upstream code nonetheless.
Yes. But note that it can take some time before getting a response, so I propose we don't block #99194 and get back to it if your proposal is accepted. |
Understood, I'll propose the compatibility macros to STMicroelectronics/STM32CubeH5 and can revisit this approach if they accept it. In the meantime, I'm fine with proceeding with #99194 to unblock users. Thanks for the feedback. |



Fixes #99191
Add conditional compilation to support different STM32 XSPI HAL capabilities across product lines.
Related PRs:
Problem:
After adding XSPI1 compatibility macros (hal_stm32#328), the XSPI PSRAM driver still had STM32H5-incompatible code that assumed all STM32 XSPI peripherals have the same features (XSPI Manager, 16-line data mode, MaxTran/MemorySelect fields).
Solution:
xspi_mgrclock exists (STM32H7RS)HAL_XSPI_DATA_16_LINESis defined (STM32H7RS), otherwise 8-line (STM32H5)Benefits:
Testing:
memc_stm32_xspi_psram.c.obj