Skip to content

Conversation

gmarull
Copy link
Member

@gmarull gmarull commented Sep 16, 2025

Add flash (QSPI NOR) driver for SF32LB

@gmarull gmarull requested a review from cameled September 16, 2025 13:21
@github-actions github-actions bot added manifest manifest-hal_sifli DNM (manifest) This PR should not be merged (controlled by action-manifest) labels Sep 16, 2025
@gmarull gmarull force-pushed the sf32lb52x-flash branch 3 times, most recently from ce5be5e to 7335cff Compare September 17, 2025 19:05
@zephyrproject-rtos zephyrproject-rtos deleted a comment from github-actions bot Oct 1, 2025
@gmarull gmarull removed the DNM (manifest) This PR should not be merged (controlled by action-manifest) label Oct 1, 2025
@gmarull gmarull force-pushed the sf32lb52x-flash branch 3 times, most recently from 5218a60 to 003a034 Compare October 1, 2025 12:10
@gmarull gmarull marked this pull request as ready for review October 1, 2025 12:10
@gmarull
Copy link
Member Author

gmarull commented Oct 1, 2025

Questions for @rabbitsaviola

  • Please review the driver in general
  • DMA code is not placed in RAM, but it's called from flash_sf32lb_mpi_qspi_nor_write, is it okay?
  • Clarify in general what is strictly needed to be placed in RAM
  • Clarify why using write chunk lengths > FIFO size does not work (L324); use, e.g., teslabs@43a7ae6 to check

@gmarull gmarull added the DNM This PR should not be merged (Do Not Merge) label Oct 1, 2025
@rabbitsaviola
Copy link

  • DMA code is not placed in RAM, but it's called from flash_sf32lb_mpi_qspi_nor_write, is it okay?

it's ok that DMA code is not placed in RAM as sequence command mode used.

  • Clarify why using write chunk lengths > FIFO size does not work (L324); use, e.g., teslabs@43a7ae6 to check

write doesn't work when chunk_len > FIFO size is because RBSIZE is set to 3 by L535. If RBSIZE is 0, chunk_len=128byte works well.

@gmarull
Copy link
Member Author

gmarull commented Oct 14, 2025

last push: dropped row boundary config as it's not needed

@gmarull gmarull requested a review from fabiobaltieri October 14, 2025 12:43
fabiobaltieri
fabiobaltieri previously approved these changes Oct 14, 2025

chosen {
zephyr,flash = &py25q128ha;
zephyr,flash = &nor;
Copy link
Contributor

Choose a reason for hiding this comment

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

zephyr,flash chosen is usually a node with nv-soc-flash compatible, and it is what's expected by several DT macros and MCUboot.

COND_CODE_1(DT_NODE_HAS_COMPAT(DT_GPARENT(node_id), soc_nv_flash), (DT_GPARENT(node_id)), \

dt_prop(write_block_size PATH "${flash_node}" PROPERTY "write-block-size")

Copy link
Member Author

Choose a reason for hiding this comment

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

This soc has no "nv-soc-flash", it can only XIP from an external QSPI NOR. We may have to adjust these macros/cmake utils when needed I guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

The difficulty here is keeping the QSPI NOR description aligned with what one would find in Linux (I think it is, except for the extra binding I had to create due to QSPI controllers not owning a proper device class) and making Zephyr happy at the same time if we use that same QSPI NOR for XIP. I'm open to better suggestions, but I think it's yet another case of Zephyr going on its own, making things just difficult.

Copy link
Contributor

@JarmouniA JarmouniA Oct 14, 2025

Choose a reason for hiding this comment

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

You can have a look at

ext_flash_ctrl: qspi-flash-controller@0 {
for an example of how this should be done in my opinion to work properly with what Zephyr MCUboot and DT Macros expect.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can have a look at

ext_flash_ctrl: qspi-flash-controller@0 {

for an example of how this should be done in my opinion to work properly with what Zephyr MCUboot and DT Macros except.

IMHO this is not a good devicetree description. You have 3 levels just to make Zephyr happy (quadspi, ext_flash_ctrl and then ext_flash). I doubt you would find this in parts with such capability in Linux (like ST MP1/2)

Copy link
Contributor

@JarmouniA JarmouniA Oct 14, 2025

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I doubt you would find this in parts with such capability in Linux (like ST MP1/2)

Yes, Linux is simpler indeed https://github.com/STMicroelectronics/linux/blob/f01241fbba4d879fa770685629b49d42e904e43c/arch/arm/boot/dts/st/stm32h750i-art-pi.dts#L188

https://github.com/STMicroelectronics/linux/blob/f01241fbba4d879fa770685629b49d42e904e43c/arch/arm/boot/dts/st/stm32mp157d-ev1.dts#L587

The problem in Zephyr is that QSPI controllers have no proper driver class, so we currently have 1) a mess in drivers/flash and 2) devicetree zephyrisms to make things "work". To keep the controller/flash aligned with what one usually finds in Linux, I had to create the extra "memory" type node where you specify the memory-mapped absolute address.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had to create the extra "memory" type node where you specify the memory-mapped absolute address.

You can probably use ranges for that as suggested here
#97413 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

I had to create the extra "memory" type node where you specify the memory-mapped absolute address.

You can probably use ranges for that as suggested here #97413 (comment)

will give it a shot, thanks for the suggestion

Copy link
Member Author

Choose a reason for hiding this comment

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

ranges does not work as expected because

config FLASH_BASE_ADDRESS
	hex "Flash Base Address"
	default $(dt_chosen_reg_addr_hex,$(DT_CHOSEN_Z_FLASH)) if (XIP && (ARM || ARM64)) || !ARM

And since the flash@0 parent node has #size-cells set to 0, range translation will not happen, keeping FLASH_BASE_ADDRESS set to 0. Instead, I've now changed the flash base address in the SoC defconfig, so it points to the memory-mapped address. This way we keep aligned with Linux (where the '0' means SPI slave 0, as it's a child of an SPI bus) and we can skip devicetree zephyrisms.

static const struct flash_sf32lb_mpi_qspi_nor_config config##n = { \
.params = \
{ \
.write_block_size = 4U, \
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be set in flash device node via nv-soc-flash compatible's write-block-size property.

Related discussion #95152 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

As mentioned above, this is already known because we always execute from a QSPI NOR (not an internal NV flash where these values may differ depending on SoC)

Copy link
Contributor

Choose a reason for hiding this comment

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

But the driver can be used with different NOR chips, isn't write-block-size a NOR chip property?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it should actually be set to 1 (as in spi_nor.c driver, which deals with jedec compliant devices), are there any SPI NORs requiring something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should actually be set to 1

Ok, but let's set it via DT instead of hardcoding it in the driver.

are there any SPI NORs requiring something else?

I can't say

Copy link
Member Author

@gmarull gmarull Oct 14, 2025

Choose a reason for hiding this comment

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

I think it should actually be set to 1

Ok, but let's set it via DT instead of hardcoding it in the driver.

I don't think this is a variable attribute in JEDEC compliant SPI NORs (as you can see as well in spi_nor.c), it could actually be made a driver-level static variable.

.params = \
{ \
.write_block_size = 4U, \
.erase_value = 0xFFU, \
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto, with erase-block-size

Related discussion #96443 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

On SPI NOR devices, erase size is not strictly fixed, you can erase per sector, block or entire chip.

@gmarull
Copy link
Member Author

gmarull commented Oct 14, 2025

last push: moved flash nor parameters to module-level variable, same as in spi_nor.c (and fixed write block size).

@gmarull gmarull requested a review from fabiobaltieri October 14, 2025 15:28
@gmarull gmarull force-pushed the sf32lb52x-flash branch 2 times, most recently from 178403c to 8274aca Compare October 14, 2025 18:59
Add compatible for SiFli SF32LB MPI operating as QSPI NOR controller.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Compatible should be set at board level, depending on how MPI IP is used
(e.g. NOR, NAND, PSRAM).

Signed-off-by: Gerard Marull-Paretas <[email protected]>
No longer used.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Ideally we should be able to just use jedec,spi-nor (to keep Linux
compatibility), but, QSPI controllers live in a limbo in Zephyr. Adding
this binding won't make things worse than they are.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Command was missing.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
So we can obtain a register address (as hex) given its name.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Take it from the chosen flash node parent (MPI controller) 'nor'
register, which contains the memory mapped address for the NOR flash.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Initial driver for the SF32LB MPI accessing QSPI NOR flash devices.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
So that flash driver can be used (MPI2 works in QSPI mode).

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Allow obtaining SFDP parameters using SF32LB driver.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants