-
Notifications
You must be signed in to change notification settings - Fork 8.4k
[WIP] stm32 mspi drivers for the MSPI flash and controller #78186
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
Conversation
6dd0042 to
c574a3d
Compare
|
reading external NOR at offset 0 in spi/str mode : |
8ff65f9 to
4617600
Compare
a63ea6e to
2e07eb4
Compare
6c91135 to
8f865e7
Compare
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.
I will start by thanking your support for the MSPI API. @FRASTM
After taking a brief look and I think there are quite a lot of work to be done for both the controller and device driver.
Additionally, I would like to stress one more time that it is important to shroud hardware differences in the device driver. In this case, it is the flash_mspi_nor_mx.c. So that we may have shared drivers that can be used across platform.
drivers/flash/flash_mspi_nor_mx.c
Outdated
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.
So the user context way did not work out?
The methods(auto polling) introduced in this function is not a common one thus should not appear like this in a shared driver as others(controllers) will not be able to use this driver.
Also I'm confused with the use of async PIO but does not register callback?
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, I did not find a proper way to handle that with a user context
There is a struct mspi_xfer trans; or a struct mspi_xfer_packet packet; that are available in the flash_mspi_nor_mx_data but no user context structure appears clearly
I understand that the flash driver could first, give the AutoPolling bit (mask/match) to the MSPI controller and then call a flash_mspi_nor_mx_busy_wait(). But what should be the way to pass that AutoPolling bit (mask/match) from the flash to the mspi controller ?
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.
I have a example raw implementation of autopoll using Ambiq controller. We don't support autopoll natively by hardware but the upper layer should suffice to demonstrate my suggestion.
Please checkout https://github.com/AmbiqMicro/ambiqzephyr/commits/apollo3p-mspi-autopoll-trial/ and this commit 39e35c83590bdef7096e4e13e53a35346cbfaa23
You need to define your own user context for autopolling in the device driver as this may not be generic.
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.
ur own user context for autopolling in the device driver as this may
Ok, I can see the data->autopoll to pass to the mspi driver
What is not yet really clear is
- the goal of the magic
- the role of flash_mspi_atxp032_autopoll.match_ap/mask_ap compared to mspi_ambiq_autopoll_cfg->match
/ flash_mspi_atxp032_autopoll-> mask - in the mspi_stm32_transceive I still have a check on the command to send (RDSR) but I do not see where the autopoll_cb is used
- https://github.com/FRASTM/zephyr/tree/stm32_mspi_autopoll
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.
the goal of the magic
It is for type safety to make sure the upper layer did pass the correct structure one defined as the ctx defined in struct mspi_callback_context is of type void *.
the role of flash_mspi_atxp032_autopoll.match_ap/mask_ap compared to mspi_ambiq_autopoll_cfg->match
/ flash_mspi_atxp032_autopoll-> mask
I try to design the autopoll feature to not just about pulling one byte and verify one bit in the flash status register. So in mspi_ambiq_autopoll_cfg the mask and match are array of values and masks that one can use to match with what will be read from the MSPI interface. You don't have to follow through on this.
in the mspi_stm32_transceive I still have a check on the command to send (RDSR) but I do not see where the autopoll_cb is used
Here everything is configured and waiting for a interrupt, correct? What should happen is that your HAL_XSPI_IRQHandler should call the autopull_cb when the interrupt fires. I see you have copied mspi_autopoll_callback to your file, this is equivalent to your HAL_XSPI_xxxxCallback. Without looking into your hal, you may need to find a way to register mspi_autopoll_callback, similarly to how mspi_dma_callback is being registered.
Below is irrelevent to autopoll
MSPI_DMA : async/sync is meaningless with DMA (no DMA IT function)
I'm curious that since you have mspi_dma_callback this means dma operation can be async, why are you saying in your comment that it is meaningless?
One side note after looking at https://github.com/FRASTM/zephyr/tree/stm32_mspi_autopoll :
I think you are skewing the controller driver a bit too much to one specific use i.e. flash with all that mspi_stm32_mem_ready and mspi_stm32_write_enable. How do you already know that your device is already a flash and just do mspi_stm32_config_mem in mspi_stm32_dev_config?
My suggestion is to think about the scope of the controller driver as just managing transfers not its device's state.
drivers/flash/flash_mspi_nor_mx.c
Outdated
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.
You will need to consider controllers that does not support XIP read.
The config CONFIG_MSPI_XIP is used to indicate a controller's support on that.
It was my bad that I did not include those config in my PR for the atxp032 flash driver and will be fixed in the near future. You should consider adding them like this #79513
drivers/flash/flash_mspi_nor_mx.c
Outdated
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.
You can't assume that only setting IO mode and data rate is enough.
Different controller have different defaults when it comes to the member of struct mspi_dev_cfg. Even the initialization process may needed different values. So they should specified and configure clearly.
Thus this should be a CONFIG_ALL.
drivers/flash/flash_mspi_nor_mx.c
Outdated
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.
That is the point actually. The initial mspi_dev_config is only for device initialization and the settings should be power on default/OTP default. You can have several mspi_dev_config if needed be but the final mspi_dev_config should bring it to the target operational state.
drivers/flash/flash_mspi_nor_mx.c
Outdated
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.
Not sure I understand the reason why DMA is not used. Now that I think of it, maybe we should make it configurable for different controllers or users.
drivers/mspi/mspi_stm32.c
Outdated
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.
Does mspi_transceive return before DMA completes the transfer? If so, it is async. The complete status is updated through callback.
|
The MSPI adds an abstraction layer and allows for the addition of device-specific MSPI drivers (e.g., for MX, Winbond, etc.). However, it looks like a step backward from existing O/Q/X/SPI drivers. The SFDP autoconfiguration capabilities are dropped, and dependencies between lines and opcodes are not considered. The command configuration happens on the lowest level outside the device MSPI driver. That will lead to a bloated low-level driver, increasing footprint, and potentially degrading performance. Perhaps I am missing something. But what advantages does MSPI offer over the existing Q/O/X/SPI drivers, particularly for STM32 NOR-Flash support? To address some of these issues, device-specific drivers must have greater control over low-level aspects (e.g., data lines, opcodes, etc.). However, this could lead to code duplication across drivers for MX, Winbond, and other NOR-Flash devices. A potential solution might be a generic MSPI-NOR-Flash driver that works in conjunction with a specific low-level Q/O/X/SPI MSPI driver. For example, the Q/O/X/SPI MSPI driver must use the |
dts/bindings/mtd/mspi-nor-mx25.yaml
Outdated
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.
compatible should not be named after subsystems, why not something like jedec,mx25
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.
changing to jedec,mx25 is fine for me. Then using the DT_HAS_JEDEC_MX25_ENABLED
Should then, the CONFIG_FLASH_MSPI_NOR_MX also be named CONFIG_FLASH_MSPI_JEDEC_MX25 or so ?
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.
the kconfig name doesn't matter to me
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.
My suggestion is
| compatible: "mspi-nor-mx25" | |
| compatible: "jedec,mspi-nor-mx25" |
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.
let's go for "jedec,mspi-nor-mx25"
@GeorgeCGV, if you asking the question to me, I am not in a position to answer |
The SFDP autoconfiguration can definitely be handled within the device (flash) driver layer. Since opcodes and line dependencies are specific to the device or JEDEC standard, they should be managed by the device (flash) driver. While there are methods to optimize overhead and reduce footprint, I don't have sufficient knowledge of STM Q/O/X SPI hardware to offer specific recommendations. Ultimately, the implementation of the controller driver is entirely your decision.
I attempted to develop an MSPI NOR flash driver, but I found the JEDEC specification to be rather chaotic and cumbersome when trying to consolidate everything into a single driver. I don't recently have time to continue this endeavor and I'd be happy to see a better implementation. The opcode and IO mode specified in the device tree are part of the target device's configuration after initialization, reflecting the most common operational state of the device. I think surely the device lines and write and read opcode would not change frequently if the user are aiming for performance. To make it more friendly to the end user, we can save a set of default for the target operating condition in each device bindings. An important optimization for all controllers would be to skip reconfiguring settings in |
Add the mspi-device and mspi-flash controller bindings for the stm32 devices. Signed-off-by: Francois Ramu <[email protected]>
Replace the xpsi by the mspi node in the DTS of the stm32h5 serie Signed-off-by: Francois Ramu <[email protected]>
Enable the stm32 MSPI controller based on the xspi peripheral Signed-off-by: Francois Ramu <[email protected]>
Enable the MSPI NOR multi-SPI flash device accessed through a mspi controller. Signed-off-by: Francois Ramu <[email protected]>
Declare the mspi node of the stm32h573i_dk in place of the xspi. New properties are declared according to the mspi-controller.yaml. Only SPi/STR supported yet. XIP not supported yet. Signed-off-by: Francois Ramu <[email protected]>
Add the configuration and overlay files to run the testcases tests/drivers/mspi/flash on the stm32h573i_dk target Now exclude from the samples/drivers:spi_flash Signed-off-by: Francois Ramu <[email protected]>
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
this PR is introducing the MSPI driver device a controller for the stm32 mcus : stm32H5 serie
---> compat is
mspi_nor_mx25---> compat is
st,stm32-mspi-controllerBuild an run the samples/drivers/mspi/mspi_flash/ on stm32h573i_dk disco kit