Start RP.Flash implementation

[Fabien-Chouteau]

I think the API could be improved:

• Maybe not use the C types here but HAL types
• Have dedicated types for number of pages and number of sectors rather than a precondition.

[JeremyGrosser]

I've started working on integrating this with the HAL.Block_Drivers interface, tests not yet passing. https://github.com/JeremyGrosser/rp2040_hal/tree/flash

[Fabien-Chouteau]

I would really prefer to have the basic Erase and Program procedures available to the users. The HAL.Block_Drivers interface can be useful but I want to be able to have my own access to the flash.

So I would move the block stuff to RP.Flash.Block.

[JeremyGrosser]

Yep, I'm planning to put those back in, just haven't had a chance yet.

[JeremyGrosser]

Ok, I've removed the HAL.Block_Drivers implementation for now, just to focus on getting the flashing right. I've defined a Flash_Offset type and added some conversion functions and preconditions to ensure you can't use flash erase/program on memory areas that aren't actually in flash.

The FLASH_BLOCK_SIZE that pico-sdk defines as 64k is probably a bug... It means all erases must be a multiple of 64k, so erasing anything smaller than that will have unexpected consequences. Therefore, I've made Block_Size an argument to Erase with a precondition that ensures that it's a multiple of 4k (Sector_Size). The flash needs to be erased before programming, so even though you can program pages as small as 256 bytes, an entire 4k sector needs to be erased first.

I wrote unit tests for Erase and Program, but it's commented out of the test suite right now, because GNATcov_RTS Witness functions try to execute from flash during coverage runs. I tried adding the Linker_Section and No_Inline aspects to those functions, but that didn't fix it. I think there might be some weird interaction with pragma Pure that's used by those packages, but I can't remove that pragma without patching the code gnatcov generates.

https://github.com/JeremyGrosser/rp2040_hal/blob/flash/src/drivers/rp-flash.ads

[Fabien-Chouteau]

The FLASH_BLOCK_SIZE that pico-sdk defines as 64k is probably a bug... It means all erases must be a multiple of 64k, so erasing anything smaller than that will have unexpected consequences. Therefore, I've made Block_Size an argument to Erase with a precondition that ensures that it's a multiple of 4k (Sector_Size).

I don't understand the Block_Size argument. In the SDK it's a fixed constant that's passed to the rom function.

   Flash_Size  : constant := 16#0100_0000#;
   --  XIP maps up to 16 MB of flash. Actual flash chip may be smaller.
   Page_Size   : constant := 16#100#;
   Sector_Size : constant := 16#1000#;

I am not a huge fan of sizes in hexadecimal as my brain is not able to convert them to decimal :sweat_smile:

[JeremyGrosser]

The SDK disagrees with the datasheet, which says

Erase a count bytes, starting at addr (offset from start of flash). Optionally, pass a block erase command e.g. D8h block erase, and the size of the block erased by this command — this function will use the larger block erase where possible, for much higher erase speed. addr must be aligned to a 4096-byte sector, and count must be a multiple of 4096 bytes.

I believe they set it to 65536 in the SDK as an optimization. Upon re-reading this, I guess smaller values might work, but I need to test it.

I'll change the constants back to decimal, it was just easier to keep everything in hex when I was looking at the debugger.

[JeremyGrosser]

Ok, I definitely was confused about the meaning of Count. I've changed Block_Size to have a default value of 65536 and renamed Count to Length, to match common Ada naming.

[JeremyGrosser]

Just for some more context, the bootrom has this comment near the erase function

// block_size must be a power of 2.
// Generally block_size > 4k, and block_cmd is some command which erases a block
// of this size. This accelerates erase speed.
// To use sector-erase only, set block_size to some value larger than flash,
// e.g. 1ul << 31.
// To override the default 20h erase cmd, set block_size == 4k.

[JeremyGrosser]

I'm just going back to having Block_Size be a constant... I can see now that they did that to avoid this confusion. I'm not a fan of the way they've overloaded the meaning of block_size in flash_range_erase, but I'm sure it had something to do with reducing code size.