This discussion would profit from having people around who use a platform with non-zero CPU_FLASH_BASE -- from a quick grep that'd be QN908X, LM4F120, RPx0xx and CC26xx. (could be @maribu, @fabian18 or @jeandudey from their commit history).

I can sympathize with both behaviors, because

• all MTDs start their addressing at 0, and
• mtd_flashpage is not really usable without mtd_mapper anyway, and when setting that up it's easier to deal in address space right away.

One point of concern might also be the use of FLASH_WRITABLE_INIT, which currently has no shortcut for setting up a corresponding mapped flashpage device.

I'm just using mtd_mapper and won't stop using it since I'm splitting the flash into two regions. To me this PR feels like duplicate implementation. But maybe other application could benefit from this feature.

I'm just using mtd_mapper and won't stop using it since I'm splitting the flash into two regions. To me this PR feels like duplicate implementation. But maybe other application could benefit from this feature.

I agree that mtd_flashpage without mtd_mapper has very limited usefulness, and that knowing that, makes mapping addresses in both layers seem silly. However, it was very confusing to me to have mtd_flashpage's address space mapped into my registers and RAM.

Perhaps a compromise could be to have mtd_flashpage return an error when accessing addresses less than CPU_FLASH_BASE? Perhaps enabled by DEVHELP?

This discussion would profit from having people around who use a platform with non-zero CPU_FLASH_BASE -- from a quick grep that'd be QN908X, LM4F120, RPx0xx and CC26xx.

The STM32's also have CPU_FLASH_BASE != 0. Actually, I was not aware that any CPU existed with address 0x0 falling into flash. My experience is always that such low addresses are registers or RAM.

One point of concern might also be the use of FLASH_WRITABLE_INIT, which currently has no shortcut for setting up a corresponding mapped flashpage device.

I'm not understanding what is meant here. Can you elaborate?

Maybe I am misunderstanding something. IMO you cannot use the sector in mtd_mapper as an offset. This offset is set as a start from 0 and it is checked that that it is not larger than the last mtd sector. When flash had an offset this offset might not be related to any sector size.

@Enoch247, @chrysn, @jue89, while going through the code it seems there is some inconsistentie in the flashpage driver. While working with pages the flashpage driver uses a address space of 0-N flash pages so the offset is added during operation. While working with addresses the flashpage driver is expecting cpu adresses. This is confusing.

To remove this inconsistentie it would be better that the flashpage driver uses 0-flash_size address and the offset is added in the flashpage driver.

There seems to be another inconsistentie in flashpage where flashpage_write takes a address while flashpage_read takes a page as input. This could be avoided by defining a flashpage_read_page taking a page no and flashpage_read taking a address.

The mtd_flashpage driver could then use a flashpage_read().

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

I am once again playing with flash, MTD, and filesystems. I am trying to allocate a filesystem within my MCU's flash. I have attempted to use mtd_mapper without this patch, thinking that maybe I was just doing things wrong before. I believe after this second attempt, I can say that when CPU_FLASH_BASE != 0 it is impossible to use mtd_flashpage together with mtd_mapper.

Here is why. Lets take a hypothetical MCU with and address space of 0 to 100. Let's say flash is mapped to addresses 50 to 100, and that pages are 10 bytes each (therefore FLASHPAGE_SIZE is defined as 10). So there are 5 pages of flash (therefore FLASHPAGE_NUMOF is defined as 5). Addresses 0 to 50 is RAM and registers. Let's take the following code:

mtd_flashpage_t mtd_flashpage = MTD_FLASHPAGE_INIT_VAL(4);
mtd_mapper_parent_t mtd_mapper_parent = MTD_PARENT_INIT(&mtd_flashpage.base);

mtd_mapper_region_t region1 = {
    .mtd = {
        .driver = &mtd_mapper_driver,
        .sector_count = FLASHPAGE_NUMOF,
        .pages_per_sector = 4,
        .page_size = FLASHPAGE_SIZE / 4,
    },
    .parent = &mtd_mapper_parent,
    .sector = FLASHPAGE_NUMOF,
};

This code is attempting to use mtd_mapper to create a partition in CPU's address space from address 50 to 100. The problem is that the line assert(region->sector + region->mtd.sector_count <= backing_mtd->sector_count); in _init() of mtd_mapper.c evaluates to assert(5 + 5 <= 5); and it is impossible to access any flash in the partition. This is because MTD_FLASHPAGE_INIT_VAL, rightly so, sets the sector count of the un-mapped mtd_flashpage equal to FLASHPAGE_NUMOF (ie the size of the flash, not the size of the entire CPU's address space).

I think a fix will need to either change mtd_flashpage to not map in the entire address space, or set the sector count to be much larger than the actual flash space to allow mapping past all the addresses that are not flash when flash is not located at address 0.

I see the problem you're running into. Thanks for clarifying this. Let's ask CI what it thinks about your patch ;-)

@Enoch247 ping?

@Enoch247 ping?

Sorry. I was wrapping up a large project. I will take a look at your suggestions.

@Enoch247 ping?

Sorry. I was wrapping up a large project. I will take a look at your suggestions.

No worries. Just wanted to be sure, you've seen the review. Take the time you need!

@jue89 : I committed your suggestions as a fixup. I will squash when you are ready.

Thank you for your effort and great work!

Murdock complains about the MSP340. The reason: the flash page address range reaches up to the very last address within the 16-bit address space. Thus, the compiler tells us that this check if the address is larger than this number will never be true.

I tried to fix this issue - and came up with a different approach by accident :-D

Here is my approach. What do you think?

diff --git a/drivers/mtd_flashpage/mtd_flashpage.c b/drivers/mtd_flashpage/mtd_flashpage.c
index 731d0e915d..31f7779d37 100644
--- a/drivers/mtd_flashpage/mtd_flashpage.c
+++ b/drivers/mtd_flashpage/mtd_flashpage.c
@@ -19,6 +19,7 @@
  * @}
  */
 
+#include <stdint.h>
 #include <string.h>
 #include <errno.h>
 #include <assert.h>
@@ -29,15 +30,20 @@
 #include "mtd_flashpage.h"
 #include "periph/flashpage.h"
 
-#define MTD_FLASHPAGE_END_ADDR     ((uint32_t) CPU_FLASH_BASE + (FLASHPAGE_NUMOF * FLASHPAGE_SIZE))
+#define MTD_FLASHPAGE_SIZE         (FLASHPAGE_NUMOF * FLASHPAGE_SIZE)
+
+static inline bool _range_within_bounds(uint32_t addr, uint32_t size)
+{
+    return addr + size <= MTD_FLASHPAGE_SIZE;
+}
 
 /* The MTD driver operates on an address space of 0 to N. However, the */
 /* flashpage driver operates on an address space of CPU_FLASH_BASE to */
 /* MTD_FLASHPAGE_END_ADDR. This function takes a MTD address and returns a */
 /* flashpage address. */
-static uword_t _mtd_to_flashpage_addr(uint32_t addr)
+static void* _mtd_to_flashpage_addr(uint32_t addr)
 {
-    return addr + CPU_FLASH_BASE;
+    return (void*)((uword_t) addr + CPU_FLASH_BASE);
 }
 
 static int _init(mtd_dev_t *dev)
@@ -49,27 +55,26 @@ static int _init(mtd_dev_t *dev)
 
 static int _read(mtd_dev_t *dev, void *buf, uint32_t addr, uint32_t size)
 {
-    const uword_t flash_addr = _mtd_to_flashpage_addr(addr);
-
-    assert(flash_addr < MTD_FLASHPAGE_END_ADDR);
-
     (void)dev;
 
 #ifndef CPU_HAS_UNALIGNED_ACCESS
-    if (flash_addr % sizeof(uword_t)) {
+    assert(CPU_FLASH_BASE % sizeof(uword_t));
+    if (addr % sizeof(uword_t)) {
         return -EINVAL;
     }
 #endif
 
-    memcpy(buf, (void *)flash_addr, size);
+    if (!_range_within_bounds(addr, size)) {
+        return -EOVERFLOW;
+    }
+
+    memcpy(buf, _mtd_to_flashpage_addr(addr), size);
 
     return 0;
 }
 
 static int _write(mtd_dev_t *dev, const void *buf, uint32_t addr, uint32_t size)
 {
-    const uword_t flash_addr = _mtd_to_flashpage_addr(addr);
-
     (void)dev;
 
 #ifndef CPU_HAS_UNALIGNED_ACCESS
@@ -77,30 +82,30 @@ static int _write(mtd_dev_t *dev, const void *buf, uint32_t addr, uint32_t size)
         return -EINVAL;
     }
 #endif
-    if (flash_addr % FLASHPAGE_WRITE_BLOCK_ALIGNMENT) {
+    assert(CPU_FLASH_BASE % FLASHPAGE_WRITE_BLOCK_ALIGNMENT);
+    if (addr % FLASHPAGE_WRITE_BLOCK_ALIGNMENT) {
         return -EINVAL;
     }
     if (size % FLASHPAGE_WRITE_BLOCK_SIZE) {
         return -EOVERFLOW;
     }
-    if (flash_addr + size > MTD_FLASHPAGE_END_ADDR) {
+    if (!_range_within_bounds(addr, size)) {
         return -EOVERFLOW;
     }
 
-    flashpage_write((void *)flash_addr, buf, size);
+    flashpage_write(_mtd_to_flashpage_addr(addr), buf, size);
 
     return 0;
 }
 
 int _erase(mtd_dev_t *dev, uint32_t addr, uint32_t size)
 {
-    const uword_t flash_addr = _mtd_to_flashpage_addr(addr);
     size_t sector_size = dev->page_size * dev->pages_per_sector;
 
     if (size % sector_size) {
         return -EOVERFLOW;
     }
-    if (flash_addr + size > MTD_FLASHPAGE_END_ADDR) {
+    if (!_range_within_bounds(addr, size)) {
         return -EOVERFLOW;
     }
     if (addr % sector_size) {
@@ -108,7 +113,7 @@ int _erase(mtd_dev_t *dev, uint32_t addr, uint32_t size)
     }
 
     for (size_t i = 0; i < size; i += sector_size) {
-        flashpage_erase(flashpage_page((void *)(flash_addr + i)));
+        flashpage_erase(flashpage_page(_mtd_to_flashpage_addr(addr + i)));
     }
 
     return 0;

Your patch looks good to me. I had two comments on it. I pushed it as a fixup commit so that I could add those comments.

Alternatively I could just split out the changes to mtd_flashpage from #18608 to a separate PR.
I think it's an error that mtd_flashpage just expects you to pass through raw flash addresses, this is totally against the idea of the MTD abstraction layer where a MTD device should always start at 0 and the application should not have to know what MTD backend is used.

I think it's an error that mtd_flashpage just expects you to pass through raw flash addresses, ...

I agree, that's what this MR aims to fix. That is, it remaps the addresses used by mtd_flashpage to be 0 based. This is because, mtd_mapper has assumptions that all mtd devices' address space start at 0. See comment above.

Does the (now merged) solution form #19258 work for you?

Does the (now merged) solution form #19258 work for you?

I believe it will. Closing this PR.