drivers: flash: Add one byte read write emulation to qspi nor nrf

[sigvartmh]

Adds the functionality of doing one byte read and write to QSPI nor
driver. This is achived through the use of stack in the function calls.

It also adds the functionality of using stack if the data passed to the
driver are stored in flash and not in RAM. So that the DMA will still
work if you refrence data in flash by copying the data onto the stack.

Signed-off-by: Sigvart Hovland sigvart.hovland@nordicsemi.no

[sigvartmh]

Couldn't do it the same way as the nrf soc flash driver as nrfx does not expose a nrfx_nvmc_bytes_write function for QSPI but I changed the implementation to use stack instead of heap.

So some more clarification for this PR. The goal of this PR is to make sure that we can use MCUBoot with external flash. What is needed for that to work is the possibility of doing

• 1 byte access read/write to memory
• Write const data which are resides in FLASH not RAM

[NordicBuilder]

All checks are passing now.

checkpatch (informational only, not a failure)

-:304: WARNING:LONG_LINE: line over 80 characters
#304: FILE: drivers/flash/nrf_qspi_nor.c:609:
+	} else if (IS_ENABLED(CONFIG_NORDIC_QSPI_NOR_FLASH_ALLOW_STACK_USAGE_FOR_DATA_IN_FLASH) &&

- total: 0 errors, 1 warnings, 295 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPDX_LICENSE_TAG SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

[kamlaz]

I have looked into your code and I have one question. Your solution provides possibility of writting less than four bytes, but in fact it writes four bytes, but vacants are filled with 0xFF pattern. It is OK when we assume that the data overwritten with mentioned pattern is not needed anymore. What about this scenario: You would like to write 1 byte to some address.

1. Read four bytes from desired address and copy them to the buffer.
2. Copy data to write to the buffer that contains data that is present in flash. Using this we overwrite only first byte, the rest (three bytes) remains the same.
3. Write this 4 byte buffer to the memory.
   I am not sure if this case is useful. What do you think about it?

[pabigot]

Apparently I never posted this.

[sigvartmh]

Sure I can add back the read, copy and write version.

[pabigot]

I have looked into your code and I have one question. Your solution provides possibility of writting less than four bytes, but in fact it writes four bytes, but vacants are filled with 0xFF pattern. It is OK when we assume that the data overwritten with mentioned pattern is not needed anymore.

Is that really what happens? If the underlying device is a standard SPI NOR, then doesn't writing 0xFF to a location leave it unchanged (because write only clears 1s to 0s, not sets 0s to 1s)? In that case you could write 0x01FFFFFF, 0xFF02FFFF, 0xFFFF03FF, 0xFFFFFF04 to the same address in four separate transactions and read back 0x01020304.

I guess if the erase value of the device could be 0x00 then it'd be wrong and you'd need a read, set, write.

[sigvartmh]

I guess if the erase value of the device could be 0x00 then it'd be wrong and you'd need a read, set, write.

Yeah, it could be that I made a wrong assumption there expecting no-op for 0xFF it could be that the SPI controller will interpret it differently.

The one on board the DK works the way you would expect. By having 0xFF as no-op and 0xFFFFFFFF as the erase word

[pabigot]

The question is whether the QSPI peripheral supports devices with an erase value of zero. I don't see any reason in principle why it wouldn't.

I've re-opened my upstream PR which would make mcuboot work in 2.3.0 if it got merged; it doesn't try to support sub-word writes, only reads, and only at word boundaries (which could be easily fixed).

[sigvartmh]

My suggestion is to rework it a bit. I think I've identified 1 issue I need to fix and have this as a [nrf temphack] since I require more than what is suggested upstream. Then rework the QSPI NOR so that it fulfills the requirements of zephyrproject-rtos/zephyr#23628

Then the [nrf temphack] can be removed.

[sigvartmh]

Since zephyrproject-rtos/zephyr#25292 has been merged I'll add that commit I assume that it will come in the upmerge and just focus on the write part.

[oyvindronningstad]

I'd prefer we remove the config, but I can live with this as it is. Thanks for all the changes.

I think it's best not to give users any surprise behavior they haven't opted in for.

I'm not familiar with the existing QSPI driver, what is the surprise?

The speed penalty of using data in flash. Due to the small size copy.

Is that less surprising than an error?

Yes, if you get the error you can chose either apply workaround or copy your data locally and pass it to the drive.

Ok, make sure the error is well documented, and mentions the config, so the user knows the choice they have.

[oyvindronningstad]

Thanks for adding. If you have the time, it would be better to have this documentation in the header file for qspi_nor_write, since I think that's where the user will look if they get the error, but it's not a must.

[sigvartmh]

Okei I'll try to move the information there.

[nvlsianpu]

@sigvartmh Will be this upstreamed?

[sigvartmh]

@nvlsianpu all of it? or just sub-word write @pabigot zephyrproject-rtos/zephyr#25971 made an upstream PR it seems.