rpi_pico: Added flash support

[yonsch]

Add support for flash read/write/erase on rpi_pico. Tested with the flash_shell sample.
Note: Writes are currently inefficient, as they are performed on a page basis. This is because pico-sdk does not currently provide an API for writing smaller ranges.

[Laczen]

@yonsch, why don't you use a write block size of 256 byte. This will limit the possibilities of using the flash but it will be correct and avoid unnecessary erases when trying to emulate 1 byte writing support. If you do wish to support 1 byte writing it should be required to read whatever is on the flash (instead of setting it to the erase value), apply the changes, and write to flash.

[Laczen]

@yonsch, is it safe to write to flash when executing from the same flash?

[galak]

remove label, its no longer needed.

[ycsin]

maybe KB(CONFIG_FLASH_SIZE)?
https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/sys/util.h#L447

[ycsin]

make the precedence explicit:

(offset + size) > FLASH_SIZE;

[ycsin]

MISRA C:2004, 13.2 - Tests of a value against zero should be made explicit, unless the operand is effectively Boolean.

[ycsin]

remove

[ycsin]

can be made into one line, the col limit is 100

[ycsin]

might be better to test if size > 0 as well:
MISRA C:2004, 13.2 - Tests of a value against zero should be made explicit, unless the operand is effectively Boolean.

[yonsch]

@Laczen

why don't you use a write block size of 256 byte

I wanted this to be future compatible for when this feature will be available, but it turns out that the flash works with page writes, so there is no way around a write size of 256.

is it safe to write to flash when executing from the same flash?

Yes, because the relevant functions are executed from RAM. Disabling interrupts was also added to prevent hard faults when ISRs are called during flash operations.

[galak]

depends on DT_HAS_RASPBERRYPI_PICO_FLASH_CONTROLLER_ENABLED should replace the 'depends on FLASH && SOC_SERIES_RP2XXXX`

[yonsch]

Added "real" byte-aligned writes. The low level functions were taken from the SDK and bootrom, and adapted to Zephyr (functionally and stylistically). I suppose this is fine in terms of licensing (the sdk is BSD-3 Clause), but would like someone to verify this.

[Laczen]

Added "real" byte-aligned writes. The low level functions were taken from the SDK and bootrom, and adapted to Zephyr (functionally and stylistically). I suppose this is fine in terms of licensing (the sdk is BSD-3 Clause), but would like someone to verify this.

@yonsch, regarding the "real" byte-aligned writes: it is strange that the write-block-size is set to the page size while the routines allow smaller writes, is this intended ? I didn't find the _write_partial routines, but do they work by reading, modifying and writing back ? If they do it might be a good idea to set the write-block-size to 16, this will enable support for most flash usage, but still keep the amount of rewrites reduced.

[lgehreke]

Write block size is given as 1 in the devicetree but as 256 here?

[yonsch]

@soburi Thank you for your review. I replaced XIP_BASE with FLASH_BASE, which is based on devicetree. However, I did not touch XIP_SSI_BASE, since I'm not sure it should be exposed to the devicetree. The user shouldn't really play with the SSI, and it's only used through this driver (this particular snippet is from the HAL, so it should be okay). I don't think we should create a compatible for the SSI just for this instance.

[carlescufi]

@soburi please approve if happy

[de-nordic]

Look OK. Thank you for the contribution.

Note on the PAGE_WRITE_SIZE and SECTOR_ERASE_SIZE: although I prefer the verb WRITE/ERASE within the name of the identifier, to clarify what the identifier is supposed to do rather than magic interpretation of sector as erase unit and page as write unit, I understand that this is this is how the device specification names them. If it will be desired by you or other reviewers to revert the names to original, I will approve the code anyway.

[Laczen]

@yonsch, the usage of PAGE_WRITE_SIZE and SECTOR_ERASE_SIZE introduce yet another terminology that only adds to the confusion. If possible I would like you to revert this to PAGE_SIZE and SECTOR_SIZE that can be easily found in the device specifications.

[Laczen]

Thanks