drivers/mtd_flashpage: implement pagewise API, don't use raw addresses

drivers/include/mtd_flashpage.h

@@ -43,9 +43,7 @@ extern "C"
         .sector_count = FLASHPAGE_NUMOF,            \
         .pages_per_sector = _pages_per_sector,      \
         .page_size = FLASHPAGE_SIZE / _pages_per_sector, \
-        .write_size = FLASHPAGE_WRITE_BLOCK_SIZE >= FLASHPAGE_WRITE_BLOCK_ALIGNMENT \
-            ? FLASHPAGE_WRITE_BLOCK_SIZE            \
-            : FLASHPAGE_WRITE_BLOCK_ALIGNMENT,      \
+        .write_size = 1                             \
     },                                              \
 }
 
@@ -59,6 +57,9 @@ extern const mtd_desc_t mtd_flashpage_driver;
  */
 typedef struct {
     mtd_dev_t base;     /**< MTD generic device */
+    uint32_t offset;    /**< Offset in terms of MTD pages, which must comprise
+                             a whole number of sectors from the start of the
+                             flash */
 } mtd_flashpage_t;
 
 #ifdef __cplusplus

drivers/mtd/mtd.c

@@ -21,13 +21,31 @@
 #include <assert.h>
 #include <errno.h>
 #include <limits.h>
+#include <stdbool.h>
 #include <stddef.h>
 #include <stdlib.h>
 #include <string.h>
 
 #include "bitarithm.h"
 #include "mtd.h"
 
+static bool out_of_bounds(mtd_dev_t *mtd, uint32_t page, uint32_t offset, uint32_t len)
+{
+    const uint32_t page_shift = bitarithm_msb(mtd->page_size);
+    const uint32_t pages_numof = mtd->sector_count * mtd->pages_per_sector;
+
+    /* 2 TiB SD cards might be a problem */
+    assert(pages_numof >= mtd->sector_count);
+
+    /* read n byte buffer -> last byte will be at n - 1 */
+    page += (offset + len - 1) >> page_shift;
+    if (page >= pages_numof) {
+        return true;
+    }
+
+    return false;
+}
+
 int mtd_init(mtd_dev_t *mtd)
 {
     if (!mtd || !mtd->driver) {
@@ -70,6 +88,10 @@ int mtd_read(mtd_dev_t *mtd, void *dest, uint32_t addr, uint32_t count)
         return -ENODEV;
     }
 
+    if (out_of_bounds(mtd, 0, addr, count)) {
+        return -EOVERFLOW;
+    }
+
     if (mtd->driver->read) {
         return mtd->driver->read(mtd, dest, addr, count);
     }
@@ -88,6 +110,10 @@ int mtd_read_page(mtd_dev_t *mtd, void *dest, uint32_t page, uint32_t offset,
         return -ENODEV;
     }
 
+    if (out_of_bounds(mtd, page, offset, count)) {
+        return -EOVERFLOW;
+    }
+
     if (mtd->driver->read_page == NULL) {
         /* TODO: remove when all backends implement read_page */
         if (mtd->driver->read) {
@@ -139,6 +165,10 @@ int mtd_write(mtd_dev_t *mtd, const void *src, uint32_t addr, uint32_t count)
         return -ENODEV;
     }
 
+    if (out_of_bounds(mtd, 0, addr, count)) {
+        return -EOVERFLOW;
+    }
+
     if (mtd->driver->write) {
         return mtd->driver->write(mtd, src, addr, count);
     }
@@ -214,6 +244,10 @@ int mtd_write_page(mtd_dev_t *mtd, const void *data, uint32_t page,
         return -ENODEV;
     }
 
+    if (out_of_bounds(mtd, page, offset, len)) {
+        return -EOVERFLOW;
+    }
+
     if (mtd->driver->flags & MTD_DRIVER_FLAG_DIRECT_WRITE) {
         return mtd_write_page_raw(mtd, data, page, offset, len);
     }
@@ -247,6 +281,10 @@ int mtd_write_page_raw(mtd_dev_t *mtd, const void *src, uint32_t page, uint32_t
         return -ENODEV;
     }
 
+    if (out_of_bounds(mtd, page, offset, count)) {
+        return -EOVERFLOW;
+    }
+
     if (mtd->driver->write_page == NULL) {
         /* TODO: remove when all backends implement write_page */
         if (mtd->driver->write) {
@@ -321,7 +359,11 @@ int mtd_erase_sector(mtd_dev_t *mtd, uint32_t sector, uint32_t count)
         return -ENODEV;
     }
 
-    if (sector >= mtd->sector_count) {
+    if (sector + count > mtd->sector_count) {
+        return -EOVERFLOW;
+    }
+
+    if (sector + count < sector) {
         return -EOVERFLOW;
     }
 

drivers/mtd_flashpage/mtd_flashpage.c

@@ -16,6 +16,8 @@
  * @brief       Implementation for the flashpage memory driver
  *
  * @author      Vincent Dupont <vincent@otakeys.com>
+ * @author      Benjamin Valentin <benjamin.valentin@ml-pa.com>
+ * @author      Fabian Hüßler <fabian.huessler@st.ovgu.de>
  * @}
  */
 
@@ -26,87 +28,131 @@
 #include "architecture.h"
 #include "cpu.h"
 #include "cpu_conf.h"
+#include "macros/utils.h"
 #include "mtd_flashpage.h"
 #include "periph/flashpage.h"
 
+#define ENABLE_DEBUG 0
+#include "debug.h"
+
 #define MTD_FLASHPAGE_END_ADDR     ((uint32_t) CPU_FLASH_BASE + (FLASHPAGE_NUMOF * FLASHPAGE_SIZE))
 
 static int _init(mtd_dev_t *dev)
 {
-    (void)dev;
+    mtd_flashpage_t *super = container_of(dev, mtd_flashpage_t, base);
+    (void)super;
     assert(dev->pages_per_sector * dev->page_size == FLASHPAGE_SIZE);
+    assert(!(super->offset % dev->pages_per_sector));
+
+    assert((int)flashpage_addr(super->offset / dev->pages_per_sector) >= (int)CPU_FLASH_BASE);
+    assert((uintptr_t)flashpage_addr(super->offset / dev->pages_per_sector)
+           + dev->pages_per_sector * dev->page_size * dev->sector_count <= MTD_FLASHPAGE_END_ADDR);
+    assert((uintptr_t)flashpage_addr(super->offset / dev->pages_per_sector)
+           + dev->pages_per_sector * dev->page_size * dev->sector_count > CPU_FLASH_BASE);
     return 0;
 }
 
-static int _read(mtd_dev_t *dev, void *buf, uint32_t addr, uint32_t size)
+static int _read_page(mtd_dev_t *dev, void *buf, uint32_t page,
+                      uint32_t offset, uint32_t size)
 {
-    assert(addr < MTD_FLASHPAGE_END_ADDR);
+    mtd_flashpage_t *super = container_of(dev, mtd_flashpage_t, base);
+
+    assert(page + super->offset >= page);
+    page += super->offset;
+
+    /* mtd flashpage maps multiple pages to one virtual sector for unknown reason */
+    uint32_t fpage = page / dev->pages_per_sector;
+    offset += (page % dev->pages_per_sector) * dev->page_size;
+    uintptr_t addr = (uintptr_t)flashpage_addr(fpage);
+
+    addr += offset;
 
-    (void)dev;
+    DEBUG("flashpage: read %"PRIu32" bytes from %p to %p\n", size, (void *)addr, buf);
 
 #ifndef CPU_HAS_UNALIGNED_ACCESS
     if (addr % sizeof(uword_t)) {
-        return -EINVAL;
+        uword_t tmp;
+
+        offset = addr % sizeof(uword_t);
+        size = MIN(size, sizeof(uword_t) - offset);
+
+        DEBUG("flashpage: read %"PRIu32" unaligned bytes\n", size);
+
+        memcpy(&tmp, (uint8_t *)addr - offset, sizeof(tmp));
+        memcpy(buf, (uint8_t *)&tmp + offset, size);
+        return size;
     }
 #endif
 
-    uword_t dst_addr = addr;
-    memcpy(buf, (void *)dst_addr, size);
-
-    return 0;
+    memcpy(buf, (void *)addr, size);
+    return size;
 }
 
-static int _write(mtd_dev_t *dev, const void *buf, uint32_t addr, uint32_t size)
+static int _write_page(mtd_dev_t *dev, const void *buf, uint32_t page, uint32_t offset,
+                       uint32_t size)
 {
-    (void)dev;
+    mtd_flashpage_t *super = container_of(dev, mtd_flashpage_t, base);
 
-#ifndef CPU_HAS_UNALIGNED_ACCESS
-    if ((uintptr_t)buf % sizeof(uword_t)) {
-        return -EINVAL;
-    }
-#endif
-    if (addr % FLASHPAGE_WRITE_BLOCK_ALIGNMENT) {
-        return -EINVAL;
-    }
-    if (size % FLASHPAGE_WRITE_BLOCK_SIZE) {
-        return -EOVERFLOW;
-    }
-    if (addr + size > MTD_FLASHPAGE_END_ADDR) {
-        return -EOVERFLOW;
+    assert(page + super->offset >= page);
+
+    page += super->offset;
+
+    /* mtd flashpage maps multiple pages to one virtual sector for unknown reason */
+    uint32_t fpage = page / dev->pages_per_sector;
+    offset += (page % dev->pages_per_sector) * dev->page_size;
+    uintptr_t addr = (uintptr_t)flashpage_addr(fpage);
+
+    addr += offset;
+
+    DEBUG("flashpage: write %"PRIu32" bytes from %p to %p\n", size, buf, (void *)addr);
+
+    size = MIN(flashpage_size(fpage) - offset, size);
+
+    if ((addr % FLASHPAGE_WRITE_BLOCK_ALIGNMENT) || (size < FLASHPAGE_WRITE_BLOCK_SIZE) ||
+        ((uintptr_t)buf % FLASHPAGE_WRITE_BLOCK_ALIGNMENT)) {
+        uint8_t tmp[FLASHPAGE_WRITE_BLOCK_SIZE]
+                __attribute__ ((aligned (FLASHPAGE_WRITE_BLOCK_ALIGNMENT)));
+
+        offset = addr % FLASHPAGE_WRITE_BLOCK_ALIGNMENT;
+        size = MIN(size, FLASHPAGE_WRITE_BLOCK_ALIGNMENT - offset);
+
+        DEBUG("flashpage: write %"PRIu32" unaligned bytes\n", size);
+
+        memcpy(&tmp[0], (uint8_t *)addr - offset, sizeof(tmp));
+        memcpy(&tmp[offset], buf, size);
+
+        flashpage_write((uint8_t *)addr - offset, tmp, sizeof(tmp));
+
+        return size;
     }
 
-    uword_t dst_addr = addr;
-    flashpage_write((void *)dst_addr, buf, size);
+    /* don't write less than the write block size */
+    size &= ~(FLASHPAGE_WRITE_BLOCK_SIZE - 1);
 
-    return 0;
+    flashpage_write((void *)addr, buf, size);
+    return size;
 }
 
-int _erase(mtd_dev_t *dev, uint32_t addr, uint32_t size)
+static int _erase_sector(mtd_dev_t *dev, uint32_t sector, uint32_t count)
 {
-    size_t sector_size = dev->page_size * dev->pages_per_sector;
+    mtd_flashpage_t *super = container_of(dev, mtd_flashpage_t, base);
 
-    if (size % sector_size) {
-        return -EOVERFLOW;
-    }
-    if (addr + size > MTD_FLASHPAGE_END_ADDR) {
-        return -EOVERFLOW;
-    }
-    if (addr % sector_size) {
+    if (sector + (super->offset / dev->pages_per_sector) < sector) {
         return -EOVERFLOW;
     }
+    sector += (super->offset / dev->pages_per_sector);
 
-    uword_t dst_addr = addr;
-
-    for (size_t i = 0; i < size; i += sector_size) {
-        flashpage_erase(flashpage_page((void *)(dst_addr + i)));
+    while (count--) {
+        DEBUG("flashpage: erase sector %"PRIu32"\n", sector);
+        flashpage_erase(sector++);
     }
 
     return 0;
 }
 
 const mtd_desc_t mtd_flashpage_driver = {
     .init = _init,
-    .read = _read,
-    .write = _write,
-    .erase = _erase,
+    .read_page = _read_page,
+    .write_page = _write_page,
+    .erase_sector = _erase_sector,
 };

tests/mtd_flashpage/Makefile

@@ -1,6 +1,7 @@
 include ../Makefile.tests_common
 
 USEMODULE += mtd_flashpage
+USEMODULE += mtd_write_page
 USEMODULE += embunit
 
 include $(RIOTBASE)/Makefile.include

tests/mtd_flashpage/app.config.test

@@ -2,4 +2,5 @@
 # application configuration. This is only needed during migration.
 CONFIG_MODULE_MTD=y
 CONFIG_MODULE_MTD_FLASHPAGE=y
+CONFIG_MODULE_MTD_WRITE_PAGE=y
 CONFIG_MODULE_EMBUNIT=y