Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions drivers/Makefile.dep
Original file line number Diff line number Diff line change
Expand Up @@ -342,14 +342,21 @@ ifneq (,$(filter mrf24j40,$(USEMODULE)))
FEATURES_REQUIRED += periph_spi
endif

ifneq (,$(filter mtd_sdcard,$(USEMODULE)))
ifneq (,$(filter mtd_%,$(USEMODULE)))
USEMODULE += mtd
USEMODULE += sdcard_spi
endif

ifneq (,$(filter mtd_spi_nor,$(USEMODULE)))
USEMODULE += mtd
FEATURES_REQUIRED += periph_spi
ifneq (,$(filter mtd_sdcard,$(USEMODULE)))
USEMODULE += sdcard_spi
endif

ifneq (,$(filter mtd_spi_nor,$(USEMODULE)))
FEATURES_REQUIRED += periph_spi
endif

ifneq (,$(filter mtd_flashpage,$(USEMODULE)))
FEATURES_REQUIRED += periph_flashpage
FEATURES_REQUIRED += periph_flashpage_raw
endif
endif

ifneq (,$(filter my9221,$(USEMODULE)))
Expand Down
12 changes: 7 additions & 5 deletions drivers/include/mtd.h
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ struct mtd_desc {
int mtd_init(mtd_dev_t *mtd);

/**
* @brief mtd_read Read data from a MTD device
* @brief Read data from a MTD device
*
* No alignment is required on @p addr and @p count.
*
Expand All @@ -177,10 +177,11 @@ int mtd_init(mtd_dev_t *mtd);
int mtd_read(mtd_dev_t *mtd, void *dest, uint32_t addr, uint32_t count);

/**
* @brief mtd_read write data to a MTD device
* @brief Write data to a MTD device
*
* @p addr + @p count must be inside a page boundary. @p addr can be anywhere
* but the buffer cannot overlap two pages.
* but the buffer cannot overlap two pages. Though some devices might enforce alignement
* on both @p addr and @p buf.
*
* @param mtd the device to write to
* @param[in] src the buffer to write
Expand All @@ -194,11 +195,12 @@ int mtd_read(mtd_dev_t *mtd, void *dest, uint32_t addr, uint32_t count);
* @return -EOVERFLOW if @p addr or @p count are not valid, i.e. outside memory,
* or overlapping two pages
* @return -EIO if I/O error occured
* @return -EINVAL if parameters are invalid (invalid alignment for instance)
*/
int mtd_write(mtd_dev_t *mtd, const void *src, uint32_t addr, uint32_t count);

/**
* @brief mtd_erase Erase sectors of a MTD device
* @brief Erase sectors of a MTD device
*
* @p addr must be aligned on a sector boundary. @p count must be a multiple of a sector size.
*
Expand All @@ -216,7 +218,7 @@ int mtd_write(mtd_dev_t *mtd, const void *src, uint32_t addr, uint32_t count);
int mtd_erase(mtd_dev_t *mtd, uint32_t addr, uint32_t count);

/**
* @brief mtd_power Set power mode on a MTD device
* @brief Set power mode on a MTD device
*
* @param mtd the device to access
* @param[in] power the power mode to set
Expand Down
53 changes: 53 additions & 0 deletions drivers/include/mtd_flashpage.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Copyright (C) 2018 OTA keys S.A.
*
* This file is subject to the terms and conditions of the GNU Lesser
* General Public License v2.1. See the file LICENSE in the top level
* directory for more details.
*/

/**
* @defgroup drivers_mtd_flashpage Flashpage MTD
* @ingroup drivers_storage
* @brief Driver for internal flash devices implementing flashpage interface
*
* @{
*
* @file
* @brief Interface definition for the flashpage memory driver
*
* @author Vincent Dupont <[email protected]>
*/

#ifndef MTD_FLASHPAGE_H
#define MTD_FLASHPAGE_H

#include "mtd.h"
#include "periph/flashpage.h"

#ifdef __cplusplus
extern "C"
{
#endif

/**
* @brief Macro helper to initialize a mtd_t with flash-age driver
*/
#define MTD_FLASHPAGE_INIT_VAL(_pages_per_sector) { \
.driver = &mtd_flashpage_driver, \
.sector_count = FLASHPAGE_NUMOF, \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So by default, IIUC the MTD can take the whole flash. Can we make this configurable at build time with a more reasonable default value (half of the flash) ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vincent-d, do you think it would make sense to limit the default size used to half of the flash size ?

Copy link
Member Author

@vincent-d vincent-d Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a new look to this and there are 2 options:

  • we leave it as-is and rely on the configuration of the filesystem for the offset in the mtd (i.e. the mtd allows access to the whole internal flash and the filesystem is configured correctly to not write outside its area)
  • we change the mtd_flashpage to let the filesystem access only a part of the internal flash, but we need to have a specific mtd_flashpage_t descriptor to store the offset

If we only change sector_count the filesystem will be able to access the number of sector passed through this field, but it will always start from address 0 (the first sector)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure to understand everything clearly.

If the first option is kept, we'll have to trust the filesystem implementation and most of them come from external packages. My question in this case is how do you configure this external filesystem ? (I checked littlefs and I'm not sure to understand how to do that)

If the second option is kept, could the offset be automatically determined from the number of sectors defined (using offset = flashsize - number_of_sector) ?

he filesystem will be able to access the number of sector passed through this field, but it will always start from address 0

Is this an issue ?

.pages_per_sector = _pages_per_sector,\
.page_size = FLASHPAGE_SIZE / _pages_per_sector,\
Comment on lines +39 to +40
Copy link
Contributor

@benpicco benpicco Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason to create 'virtual' sectors here?
Usually page is the max transfer unit and sector the min erase unit. For internal flash they tend to be the same. So why artificially make the erase granularity worse (and complicate the code)?

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure if this is a good idea... What's the purpose of having this wrapper? The biggest problem I see is that you'd hardly know where you can write without precise knowledge about the non-filled flash space left when flashing a firmware... The main purpose of the flashpage driver is to write new program data, or extend the current one (new elf files, scripts of any kind, etc.), and, if I understand correctly, this driver aims to write user data...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal is to be able to use a file system on internal flash.

It's then completely up to the developer to be sure of what's he's doing and it might be dangerous, of course.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the point, but I have the issue of erasing. How do you cope with the small erases case? You need to always backup the whole block in RAM before adding something, then erase the whole block and write it from the buffer...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand your point here. This is the case with any flash, either internal or not.
A file system designed to run on a flash will not try to erase a few bytes, but always the entire sector. What is different here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what you're saying is that whenever you do a write using the MTD, you rely on the file system to do it correctly? In some devices the flash page size is huge (2KB for stm32f103re), that's why I'm concerned about using it in that fine-grained way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most SPI flashes have a sector size of 4KB, or even 64KB. And a typical "page" size (i.e. maximum number of bytes you can write with a single instruction) is 256B on these flashes.

Then yes, we rely on the file system to manage erases and write cycles.

Copy link
Contributor

@kYc0o kYc0o Apr 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, in overall (a bit out of scope now) using ANY file system over a flash-based MTD will take at least as much RAM as the smallest erasable block, is it? I just checked and for the N25Q128 series (the one attached to the iotlab-m3) has 4KB as eraseable sub-sector.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you take the assumption that a sector will be modified, this will be true. But a good filesystem won't need to modify a byte in the middle of a sector. See littles or spiffs, they don't need a buffer of the size of a sector.


/**
* @brief Flashpage MTD device operations table
*/
extern const mtd_desc_t mtd_flashpage_driver;

#ifdef __cplusplus
}
#endif

#endif /* MTD_FLASHPAGE_H */
/** @} */
1 change: 1 addition & 0 deletions drivers/mtd_flashpage/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
include $(RIOTBASE)/Makefile.base
99 changes: 99 additions & 0 deletions drivers/mtd_flashpage/mtd_flashpage.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* Copyright (C) 2018 OTA keys S.A.
*
* This file is subject to the terms and conditions of the GNU Lesser
* General Public License v2.1. See the file LICENSE in the top level
* directory for more details.
*/

/**
* @ingroup drivers_mtd_flashpage
* @brief Driver for internal flash devices implementing flashpage interface
*
* @{
*
* @file
* @brief Implementation for the flashpage memory driver
*
* @author Vincent Dupont <[email protected]>
* @}
*/

#include <string.h>
#include <errno.h>
#include <assert.h>

#include "cpu_conf.h"
#include "mtd_flashpage.h"
#include "periph/flashpage.h"

#define MTD_FLASHPAGE_END_ADDR CPU_FLASH_BASE + (FLASHPAGE_NUMOF * FLASHPAGE_SIZE)

static int _init(mtd_dev_t *dev)
{
(void)dev;
assert(dev->pages_per_sector * dev->page_size == FLASHPAGE_SIZE);
return 0;
}

static int _read(mtd_dev_t *dev, void *buf, uint32_t addr, uint32_t size)
{
assert(addr < MTD_FLASHPAGE_END_ADDR);
(void)dev;

if (addr % FLASHPAGE_RAW_ALIGNMENT) {
return -EINVAL;
}

memcpy(buf, (void*)addr, size);

return size;
}

static int _write(mtd_dev_t *dev, const void *buf, uint32_t addr, uint32_t size)
{
(void)dev;
if (addr % FLASHPAGE_RAW_ALIGNMENT) {
return -EINVAL;
}
if ((uintptr_t)buf % FLASHPAGE_RAW_ALIGNMENT) {
return -EINVAL;
}
if (size % FLASHPAGE_RAW_BLOCKSIZE) {
return -EOVERFLOW;
}
if (addr + size > MTD_FLASHPAGE_END_ADDR) {
return -EOVERFLOW;
}
flashpage_write_raw((void *)addr, buf, size);

return size;
}

int _erase(mtd_dev_t *dev, uint32_t addr, uint32_t size)
{
size_t sector_size = dev->page_size * dev->pages_per_sector;

if (size % sector_size) {
return -EOVERFLOW;
}
if (addr + size > MTD_FLASHPAGE_END_ADDR) {
return - EOVERFLOW;
}
if (addr % sector_size) {
return - EOVERFLOW;
}
for (size_t i = 0; i < size; i += sector_size) {
flashpage_write(flashpage_page((void *)addr), NULL);
}

return 0;
}


const mtd_desc_t mtd_flashpage_driver = {
.init = _init,
.read = _read,
.write = _write,
.erase = _erase,
};
6 changes: 6 additions & 0 deletions tests/mtd_flashpage/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
include ../Makefile.tests_common

USEMODULE += mtd_flashpage
USEMODULE += embunit

include $(RIOTBASE)/Makefile.include
160 changes: 160 additions & 0 deletions tests/mtd_flashpage/main.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
/*
* Copyright (C) 2016 OTA keys S.A.
*
* This file is subject to the terms and conditions of the GNU Lesser
* General Public License v2.1. See the file LICENSE in the top level
* directory for more details.
*/

/**
* @{
*
* @file
*/
#include <string.h>
#include <errno.h>

#include "embUnit.h"

#include "mtd.h"
#include "mtd_flashpage.h"

#define TEST_ADDRESS1 (uint32_t)flashpage_addr(FLASHPAGE_NUMOF - 1)
#define TEST_ADDRESS2 (uint32_t)flashpage_addr(FLASHPAGE_NUMOF - 2)

static mtd_dev_t _dev = MTD_FLASHPAGE_INIT_VAL(8);
static mtd_dev_t *dev = &_dev;

static void setup(void)
{
int ret = mtd_init(dev);
TEST_ASSERT_EQUAL_INT(0, ret);
mtd_erase(dev, TEST_ADDRESS1, dev->pages_per_sector * dev->page_size);
mtd_erase(dev, TEST_ADDRESS2, dev->pages_per_sector * dev->page_size);
}

static void teardown(void)
{
mtd_erase(dev, TEST_ADDRESS1, dev->pages_per_sector * dev->page_size);
mtd_erase(dev, TEST_ADDRESS2, dev->pages_per_sector * dev->page_size);
}

static void test_mtd_init(void)
{
int ret = mtd_init(dev);
TEST_ASSERT_EQUAL_INT(0, ret);
}

static void test_mtd_erase(void)
{
/* Erase last sector */
int ret = mtd_erase(dev, TEST_ADDRESS1, FLASHPAGE_SIZE);
TEST_ASSERT_EQUAL_INT(0, ret);

/* Erase with wrong size (less than sector size) */
ret = mtd_erase(dev, TEST_ADDRESS1, dev->page_size);
TEST_ASSERT_EQUAL_INT(-EOVERFLOW, ret);

/* Unaligned erase */
ret = mtd_erase(dev, TEST_ADDRESS1 + dev->page_size, dev->page_size);
TEST_ASSERT_EQUAL_INT(-EOVERFLOW, ret);

/* Erase 2 last sectors */
ret = mtd_erase(dev, TEST_ADDRESS2,
FLASHPAGE_SIZE * 2);
TEST_ASSERT_EQUAL_INT(0, ret);

/* Erase out of memory area */
ret = mtd_erase(dev, TEST_ADDRESS1,
FLASHPAGE_SIZE * 2);
TEST_ASSERT_EQUAL_INT(-EOVERFLOW, ret);
}

static void test_mtd_write_erase(void)
{
const char buf[] = "ABCDEFGHIJKLMNO";

/* stm32l0x and stm32l1x erase its flash with 0's */
#if defined(CPU_FAM_STM32L0) || defined(CPU_FAM_STM32L1)
uint8_t buf_empty[] = {0, 0, 0};
#else
uint8_t buf_empty[] = {0xff, 0xff, 0xff};
#endif
char buf_read[sizeof(buf) + sizeof(buf_empty)];
memset(buf_read, 0, sizeof(buf_read));

int ret = mtd_write(dev, buf, TEST_ADDRESS1, sizeof(buf));
TEST_ASSERT_EQUAL_INT(sizeof(buf), ret);

ret = mtd_erase(dev, TEST_ADDRESS1, dev->pages_per_sector * dev->page_size);
TEST_ASSERT_EQUAL_INT(0, ret);

uint8_t expected[sizeof(buf_read)];
#if defined(CPU_FAM_STM32L0) || defined(CPU_FAM_STM32L1)
memset(expected, 0, sizeof(expected));
#else
memset(expected, 0xff, sizeof(expected));
#endif
ret = mtd_read(dev, buf_read, TEST_ADDRESS1, sizeof(buf_read));
TEST_ASSERT_EQUAL_INT(sizeof(buf_read), ret);
TEST_ASSERT_EQUAL_INT(0, memcmp(expected, buf_read, sizeof(buf_read)));
}

static void test_mtd_write_read(void)
{
const char buf[] __attribute__ ((aligned (FLASHPAGE_RAW_ALIGNMENT))) = "ABCDEFGHIJKLMNO";

/* stm32l0x and stm32l1x erase its flash with 0's */
#if defined(CPU_FAM_STM32L0) || defined(CPU_FAM_STM32L1)
uint8_t buf_empty[] = {0, 0, 0};
#else
uint8_t buf_empty[] = {0xff, 0xff, 0xff};
#endif
char buf_read[sizeof(buf) + sizeof(buf_empty)];
memset(buf_read, 0, sizeof(buf_read));

/* Basic write / read */
int ret = mtd_write(dev, buf, TEST_ADDRESS1, sizeof(buf));
TEST_ASSERT_EQUAL_INT(sizeof(buf), ret);

ret = mtd_read(dev, buf_read, TEST_ADDRESS1, sizeof(buf_read));
TEST_ASSERT_EQUAL_INT(sizeof(buf_read), ret);
TEST_ASSERT_EQUAL_INT(0, memcmp(buf, buf_read, sizeof(buf)));
TEST_ASSERT_EQUAL_INT(0, memcmp(buf_empty, buf_read + sizeof(buf), sizeof(buf_empty)));

ret = mtd_erase(dev, TEST_ADDRESS1, dev->pages_per_sector * dev->page_size);
TEST_ASSERT_EQUAL_INT(0, ret);

/* Unaligned write / read */
ret = mtd_write(dev, buf, TEST_ADDRESS1 + sizeof(buf_empty), sizeof(buf));
TEST_ASSERT_EQUAL_INT(-EINVAL, ret);

/* Only Cortex-M0 doesn't allow unaligned reads */
#if defined(CPU_ARCH_CORTEX_M0)
ret = mtd_read(dev, buf_read, TEST_ADDRESS1 + sizeof(buf_empty), sizeof(buf_read));
TEST_ASSERT_EQUAL_INT(-EINVAL, ret);
#endif
}

Test *tests_mtd_flashpage_tests(void)
{
EMB_UNIT_TESTFIXTURES(fixtures) {
new_TestFixture(test_mtd_init),
new_TestFixture(test_mtd_erase),
new_TestFixture(test_mtd_write_erase),
new_TestFixture(test_mtd_write_read),
};

EMB_UNIT_TESTCALLER(mtd_flashpage_tests, setup, teardown, fixtures);

return (Test *)&mtd_flashpage_tests;
}

int main(void)
{
TESTS_START();
TESTS_RUN(tests_mtd_flashpage_tests());
TESTS_END();
return 0;
}
/** @} */
Loading