drivers: flash: rpi_pico: Modifications to support rp2350 #86292

Manu3l0us · 2025-02-25T14:05:37Z

The exisiting flash driver had some quite complicated implementations for edge cases where partial pages where written. This commit modifies the driver to always use the flash_range_program function from boot ROM. This is a bit less efficient on partial page writes, but simplifies the code a lot and adds compatibility to the new rp2350.

@soburi This is the flash driver I prepared during the RP2350 pull request by @ajf58. I just see now that it has been merged, so I thought we could proceed with that one.

hanan619 · 2025-04-11T08:31:29Z

@Manu3l0us
I think you missed to set the flash conroller in dtsi file
zephyr,flash-controller = &qmi;
line should be added in file "boards/raspberrypi/rpi_pico2/rpi_pico2.dtsi"

hanan619 · 2025-04-11T08:34:12Z

@de-nordic
This pull request is 2 months old.
When will this gets merged in the main branch?

hanan619 · 2025-04-15T09:53:00Z

@Manu3l0us
Have your tried MCUBoot?
The IMAGE_DEFINITION_BLOCK should be included in bootloader and not in app.

de-nordic · 2025-04-15T10:59:32Z

@de-nordic This pull request is 2 months old. When will this gets merged in the main branch?

I do not know. Now it is a WIP, but you can actually help with reviewing, if you have time.

hanan619 · 2025-04-15T11:08:00Z

Yes I can help.
Already reviewed the code and tested with rpi_pico and rpi_pic2.
the flash controller support is working for both controllers.

Manu3l0us · 2025-04-15T11:29:11Z

I began this merge request but got distracted with other things. I am still interested to get this upstream, but at the moment lack the time for much activity.

@Manu3l0us
Have your tried MCUBoot?
The IMAGE_DEFINITION_BLOCK should be included in bootloader and not in app.

Yes, I have. I add the IMAGE_DEFINITION_BLOCK in mcuboot but not in the app. I also added the PR #86294 concerning this, but I found that there is related activity concerning the IMAGE_DEFINITION_BLOCK

Manu3l0us · 2025-04-15T11:36:37Z

I removed the WIP "label" as I don't plan to add something currently. The driver is working fine in my private branch

hanan619 · 2025-04-15T11:42:27Z

In the PR #86294 you missed to update Kconfig.defconfig

I have added my comment in PR.
After these changes, the MCUBoot works perfectly with pico2

Manu3l0us · 2025-04-15T13:01:00Z

@Manu3l0us I think you missed to set the flash conroller in dtsi file zephyr,flash-controller = &qmi; line should be added in file "boards/raspberrypi/rpi_pico2/rpi_pico2.dtsi"

I fixed that in the latest push. I forgot it as I'm using the RP2350 on a custom board already...

The exisiting flash driver had some quite complicated implementations for edge cases where partial pages where written. This commit modifies the driver to always use the flash_range_program function from boot ROM. This is a bit less efficient on partial page writes, but simplifies the code a lot and adds compatibility to the new rp2350. Signed-off-by: Manuel Aebischer <manuel.aebischer@belden.com>

de-nordic · 2025-04-15T13:38:47Z

drivers/flash/flash_rpi_pico.c

-	flush_cache();
-	flash_enable_xip_via_boot2();
-}
-
 static bool is_valid_range(off_t offset, uint32_t size)
 {
 	return (offset >= 0) && ((offset + size) <= FLASH_SIZE);


This should have issue added to Zephyr as it is not overflow safe.

Could you elaborate, please? An issue concerning this function in the current code?

de-nordic · 2025-04-15T13:39:55Z

drivers/flash/flash_rpi_pico.c

+	ARG_UNUSED(dev);
+
+	const uint8_t *data_pointer = (const uint8_t *)data;


Local definitions should go to the top of function if possible. Switch these around.

Before the ARG_UNUSED(), correct?

Yes. Just do not touch lines that do not really need touching. Leave the style as it was.

Understood!

Local definitions should go to the top of function if possible. Switch these around.

I disagree. C99 has been a thing for a long time. I feel like variable scope should be reduced where possible.
That said, I'd also prefer not to see these things done in the same commit as it makes it difficult for reviewers to split out what's required vs what's tidying up/refactoring work.

this is really not a hill I wanna die on.

That's perfectly reasonable. The key point is "reduce variable scope" is a distinct logical change, so should be in a separate commit (if you want to do it).

This is scope reduction:

int i = 0; if (something) { // only place where i is needed }

to

if (something) { int i; // only place where i is needed }

Moved definition is on the same scope it is just not visible above definition. Where declarations end up is a matter of style, nevertheless it avoids mess where you have to move definition up, if you want to use the variable earlier when doing code changes, at which point you may end up accidentally shadowing other variable, which has been defined in internal scope.
In short, when you keep variables in the top of the scope you avoid:

moving definitions during development

accidental shadowing.

more changes then needed because of 1) and 2).

Anyway, not my unit, I just need to keep flash working according to API. @soburi your call, btw, can re-assign the PR to you?

OK, I'll bite:

moving definitions during development

Yup

accidental shadowing.

I'm pretty sure -Wshadow is setup for gcc, clang and friends. so we avoid this with warning: declaration of 'foo' shadows a previous local [-Wshadow].

more changes then needed because of 1) and 2).

A good list needs three things ;-).

@de-nordic

In terms of practical benefits, I don't think there's any problem as this is checked with -Wshadow.
I take the position that "it is preferable to follow existing custom unless there is a particularly significant benefit",
but this is also a matter of stylistic preference, so I think the current code is acceptable.

The 2012 revision of MISRA-C removed the reference to declarations at the top, so I think this in itself should be treated as a matter of preference.

@soburi your call, btw, can re-assign the PR to you?

I assigned myself as the assignee.

accidental shadowing.

I'm pretty sure -Wshadow is setup for gcc, clang and friends. so we avoid this with warning: declaration of 'foo' shadows a previous local [-Wshadow].

They have many other switches too.

And you will, probably, have to rename the variable, after you get the warning, in entire internal scope, because there is a fair chance that the internal scope variable is a temporary thing that has nothing in common or does not affect the external scope variable nor is affected by it. So basically the cost of change is left on the next victim to touch the code. Anyway, not my unit, not my problem.

more changes then needed because of 1) and 2).

A good list needs three things ;-).

I think five, but I run out of ideas after two, as you can tell by the number three.

de-nordic · 2025-04-15T13:40:58Z

drivers/flash/flash_rpi_pico.c

 		return -EINVAL;
 	}

-	key = irq_lock();
+	uint32_t key = irq_lock();


Why moving the definition from the top? Seems that this is unrelated change.

I'll roll this back.

de-nordic · 2025-04-15T13:42:25Z

drivers/flash/flash_rpi_pico.c

 		return -EINVAL;
 	}

-	key = irq_lock();
+	uint32_t key = irq_lock();


Unneeded change.

I'll roll this back.

de-nordic · 2025-04-15T13:51:37Z

drivers/flash/flash_rpi_pico.c

-		flash_write_partial(offset, flash_ram_buffer, size);
+		memcpy(flash_ram_buffer + size,
+		       (uint8_t *)(CONFIG_FLASH_BASE_ADDRESS + offset + size), PAGE_SIZE - size);
+		flash_range_program(offset, flash_ram_buffer, PAGE_SIZE);


I do not think this is correct; the driver sets write_block_size = 1, which means that user can write anything from 1-256. If user now will attempt to write single bytes from, whatever the leftover size was, to the end of page, in their code, then the user will end up re-writing the page N times, where N can be in range [1,PAGE_SIZE - 1].

I agree that this would happen but from what I read in the datasheets it should be safe as you can "overwrite" a flash location with the same value without it having a side effect on the flash cell. I agree that in case of writing single bytes, this will be inefficient, but this is the trade-off for using the flash_range_program() ROM function.

No it is not generally ok to overwrite nor memory with, even with the same value, without erase.
There are memories that have limits for how many times you can do such writes.
Datasheets often does not mention such limit, and NOR is not, usually, affected by hammering, like DRAM, nor over-program, like NAND, but if this driver is supposed to work with memories that do not work well with different memories, then it can not do over-write.

While this should be a no-op on cell level and the electrical stress is caused by a) erasing and b) writing 1 -> 0, I understand your concerns and I cannot demonstrate it for every flash available.

I guess in that form it is therefore not viable. RP2350 mainline flash support has to wait then until someone successfully implements a qmi driver, which would be the proper thing to do anyway.

While this should be a no-op on cell level and the electrical stress is caused by a) erasing and b) writing 1 -> 0, I understand your concerns and I cannot demonstrate it for every flash available.

I know that SoC NOR often have these limitations specified, I also remember long time ago reading datasheet, for NOR, where there was per write word value and per erase page (so you could rewrite bits in erase page something like ~80 times). I can not find such info in specs anymore, so maybe the tech got the issue fixed and it does not matter anymore?

The lack of overwrite, though, allows to support devices without erase but with the same set of commands, like SPI MRAM, RRAM or FRAM devices - these devices will take endurance reduced by overwrites.

I guess in that form it is therefore not viable. RP2350 mainline flash support has to wait then until someone successfully implements a qmi driver, which would be the proper thing to do anyway.

I do not get this, as my knowledge of rp2350 is very limited, there was that flash_write_partial used, wasn't it supposed to do non page write?

How can we weasel out of this before we can get the correct driver running?

Maybe we should take the write-block-size from DTS definition and issue compile time warning that this driver will do overwrites and may not be optimal for devices that can not withstand that?

This way user may change the write-block-size (from the default PAGE_SIZE), in DTS, knowing what device has been connected to that controller and that there will be no issue, but the decision will be on user.

And to be completely on page with you here: soc-nv-flash represents here external NOR device the RP peripheral takes care of so that it looks like internal flash?

And to be completely on page with you here: soc-nv-flash represents here external NOR device the RP peripheral takes care of so that it looks like internal flash?

Yes, correct. RP2040 and RP2350 don't have internal flash, they use an external SPI NOR. The upcoming RP2354 has internal flash, but the implementation detail is, that it is a multi-die package with an W25Q16 integrated into it.

@soburi @yonsch Any thoughts on this?

The data sheet for the W25Q16JV actually used in Pico states:

https://docs.rs-online.com/068a/0900766b81622f8d.pdf

In some cases, less than 256 bytes (a
partial page) can be programmed without having any effect on other bytes within the same page. One
condition to perform a partial page program is that the number of clocks cannot exceed the remaining page
length. If more than 256 bytes are sent to the device the addressing will wrap to the beginning of the page
and overwrite previously sent data.

There is no doubt that it is desirable to implement partial rewrites appropriately.

Ideally, I think it would be ideal to move forward with adding this support to the pico-sdk side. (raspberrypi/pico-sdk#1169)

At the very least, I think it would be unacceptable to remove the implementation of partial writing in RP2040, as it would be a degradation.

Do not look at specific part, because it can be replaced, but the driver is generic thing for rp2040 flash support rather then device beyond it.

ajf58

A couple of nits, but I think the key thing is checking and correcting the flash partition for the Pico.

ajf58 · 2025-04-15T21:12:47Z

drivers/flash/flash_rpi_pico.c

+	ARG_UNUSED(dev);
+


This is a good change, but I would prefer if it was in a separate commit as it is not required for adding support in the new driver.

ajf58 · 2025-04-15T21:28:32Z

tests/drivers/flash/common/boards/rpi_pico.overlay

+			read-only;
+		};
+
+		storage_partition: partition@20000 {


The Pico only has 2MB of storage. Is this partitioning correct?

ajf58 · 2025-04-16T19:20:01Z

drivers/flash/flash_rpi_pico.c

+	ARG_UNUSED(dev);
+
+	const uint8_t *data_pointer = (const uint8_t *)data;


this is really not a hill I wanna die on.

That's perfectly reasonable. The key point is "reduce variable scope" is a distinct logical change, so should be in a separate commit (if you want to do it).

ajf58 · 2025-04-16T19:24:58Z

drivers/flash/flash_rpi_pico.c

+#define PAGE_SIZE   256
 #define SECTOR_SIZE DT_PROP(DT_CHOSEN(zephyr_flash), erase_block_size)
 #define ERASE_VALUE 0xff
-#define FLASH_SIZE KB(CONFIG_FLASH_SIZE)
-#define FLASH_BASE CONFIG_FLASH_BASE_ADDRESS
-#define SSI_BASE_ADDRESS DT_REG_ADDR(DT_CHOSEN(zephyr_flash_controller))
+#define FLASH_SIZE  KB(CONFIG_FLASH_SIZE)
+#define FLASH_BASE  CONFIG_FLASH_BASE_ADDRESS


Same comment as elsewhere, let's keep linting/whitespace change in a separate commit from changes that affect the logic of the code.

ajf58 · 2025-04-16T19:26:01Z

dts/arm/raspberrypi/rpi_pico/rp2350.dtsi

@@ -202,7 +202,7 @@
 				write-block-size = <1>;
 				erase-block-size = <DT_SIZE_K(4)>;
 			};
-			status = "disabled";
+			status = "okay";


I think that "okay" is the implicit default, so this can be removed.

soburi · 2025-04-17T23:12:23Z

drivers/flash/flash_rpi_pico.c

+		off_t page_offset = offset & ~(PAGE_SIZE - 1);
+		size_t bytes_to_write = MIN(PAGE_SIZE - offset_within_page, size);


If placing declarations here, please add const if you can.

soburi · 2025-04-17T23:12:47Z

drivers/flash/flash_rpi_pico.c

 		data_pointer += bytes_to_write;
 		size -= bytes_to_write;
 		offset += bytes_to_write;
 	}

 	while (size >= PAGE_SIZE) {
-		bytes_to_write = PAGE_SIZE;
+		size_t bytes_to_write = PAGE_SIZE;


hanan619 · 2025-04-28T08:54:56Z

@soburi I have created a PR #89182 which contains the support for partial write.

github-actions · 2025-06-28T00:38:24Z

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.

zephyrbot added area: Flash platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) labels Feb 25, 2025

zephyrbot requested review from ajf58, de-nordic, rghaddab, soburi, ThreeEights and yonsch February 25, 2025 14:06

zephyrbot assigned de-nordic Feb 25, 2025

Manu3l0us force-pushed the rework_flash_driver branch 4 times, most recently from b5e3190 to cb4ba48 Compare February 25, 2025 15:29

soburi mentioned this pull request Mar 11, 2025

TaskList for Supporting RaspberryPi Pico peripherals #53810

Open

55 tasks

de-nordic mentioned this pull request Apr 11, 2025

Flash Controller support for Raspberry Pi Pico2 (RP2350) #88488

Open

Manu3l0us changed the title ~~WIP: drivers: flash: rpi_pico: Modifications to support rp2350~~ drivers: flash: rpi_pico: Modifications to support rp2350 Apr 15, 2025

Manu3l0us force-pushed the rework_flash_driver branch from cb4ba48 to e7b7f44 Compare April 15, 2025 12:59

Manu3l0us force-pushed the rework_flash_driver branch from e7b7f44 to 6ac5a6a Compare April 15, 2025 13:03

de-nordic reviewed Apr 15, 2025

View reviewed changes

ajf58 requested changes Apr 16, 2025

View reviewed changes

soburi self-assigned this Apr 17, 2025

soburi reviewed Apr 17, 2025

View reviewed changes

github-actions bot added the Stale label Jun 28, 2025

		ARG_UNUSED(dev);

		const uint8_t data_pointer = (const uint8_t )data;

		off_t page_offset = offset & ~(PAGE_SIZE - 1);
		size_t bytes_to_write = MIN(PAGE_SIZE - offset_within_page, size);

drivers: flash: rpi_pico: Modifications to support rp2350 #86292

Are you sure you want to change the base?

drivers: flash: rpi_pico: Modifications to support rp2350 #86292

Conversation

Manu3l0us commented Feb 25, 2025

Uh oh!

hanan619 commented Apr 11, 2025

Uh oh!

hanan619 commented Apr 11, 2025

Uh oh!

hanan619 commented Apr 15, 2025

Uh oh!

de-nordic commented Apr 15, 2025

Uh oh!

hanan619 commented Apr 15, 2025

Uh oh!

Manu3l0us commented Apr 15, 2025

Uh oh!

Manu3l0us commented Apr 15, 2025

Uh oh!

hanan619 commented Apr 15, 2025

Uh oh!

Manu3l0us commented Apr 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soburi Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajf58 left a comment

Choose a reason for hiding this comment

soburi Apr 17, 2025 •

edited

Loading

soburi Apr 17, 2025 •

edited

Loading