Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mtd: Introduce write granularity #17683

Merged
merged 10 commits into from
May 20, 2022
1 change: 1 addition & 0 deletions boards/native/board_init.c
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ mtd_native_dev_t mtd0_dev = {
.sector_count = MTD_SECTOR_NUM,
.pages_per_sector = MTD_SECTOR_SIZE / MTD_PAGE_SIZE,
.page_size = MTD_PAGE_SIZE,
.write_size = MTD_WRITE_SIZE,
},
.fname = MTD_NATIVE_FILENAME,
};
Expand Down
5 changes: 5 additions & 0 deletions boards/native/include/board.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,11 @@ void _native_LED_RED_TOGGLE(void);
#ifndef MTD_SECTOR_NUM
#define MTD_SECTOR_NUM (2048)
#endif
/** Advertised write size. While the file system backend supports single byte
* granularity, this can be increased to mimic other media. */
#ifndef MTD_WRITE_SIZE
#define MTD_WRITE_SIZE (1)
#endif
#ifndef MTD_NATIVE_FILENAME
#define MTD_NATIVE_FILENAME "MEMORY.bin"
#endif
Expand Down
4 changes: 4 additions & 0 deletions cpu/esp_common/periph/flash.c
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ void spi_flash_drive_init (void)
_flash_driver.write_page = &_flash_write_page;
_flash_driver.erase = &_flash_erase;
_flash_driver.power = &_flash_power;
_flash_driver.flags = MTD_DRIVER_FLAG_CLEARING_OVERWRITE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be correct, ESP32 SoCs use NOR flash chips.


/* first, set the beginning of flash to 0x0 to read partition table */
_flash_beg = 0x0;
Expand Down Expand Up @@ -200,6 +201,9 @@ void spi_flash_drive_init (void)

_flash_dev.pages_per_sector = _flashchip->sector_size / _flashchip->page_size;
_flash_dev.page_size = _flashchip->page_size;
/* Emulation for smaller / unaligned writes is present, but at reduced
* performance */
_flash_dev.write_size = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be correct since address and length "should" be 4-byte aligned.


DEBUG("%s flashchip chip_size=%d block_size=%d sector_size=%d page_size=%d\n", __func__,
_flashchip->chip_size, _flashchip->block_size,
Expand Down
1 change: 1 addition & 0 deletions drivers/at24cxxx/mtd/mtd.c
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ static int _mtd_at24cxxx_init(mtd_dev_t *mtd)
mtd->pages_per_sector = 1;
mtd->sector_count = DEV(mtd)->params.eeprom_size /
DEV(mtd)->params.page_size;
mtd->write_size = 1;
return 0;
}

Expand Down
1 change: 1 addition & 0 deletions drivers/at25xxx/mtd/mtd.c
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ static int mtd_at25xxx_init(mtd_dev_t *dev)
dev->pages_per_sector = 1;
dev->page_size = mtd_at25xxx->params->page_size;
dev->sector_count = mtd_at25xxx->params->size / mtd_at25xxx->params->page_size;
dev->write_size = 1;
return 0;
}
return -EIO;
Expand Down
45 changes: 36 additions & 9 deletions drivers/include/mtd.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
*
* MTD devices expose a block based erase and write interface. In that, they
* are the distinct from block devices (like hard disks) on which individual
* bytes can be overridden. The [Linux MTD FAQ](http://www.linux-mtd.infradead.org/faq/general.html)
* bytes can be overwritten. The [Linux MTD FAQ](http://www.linux-mtd.infradead.org/faq/general.html)
* has a convenient comparison (beware though of terminology differences
* outlined below). They can be erased (with some granularity, often wearing
* out the erased area a bit), and erased areas can be written to (sometimes
Expand All @@ -44,11 +44,25 @@
*
* Pages are a subdivision of sectors.
*
* * The **write size** is the minimum size of writes to the device, and also
* the required alignment of writes.
*
* The write size is a divider of the page. It is often between 1 to 4 bytes
* long, but may be up to the full page size.
*
* * The device's **flags** indicate features, eg. whether a memory location
* can be overwritten without erasing it first.
*
* Note that some properties of the backend are currently not advertised to the
* user (see the documentation of @ref mtd_write).
* Unless a flag (such as @ref MTD_DRIVER_FLAG_DIRECT_WRITE or @ref
* MTD_DRIVER_FLAG_CLEARING_OVERWRITE) allows it, this MTD API does not allow
* memory areas to be written to twice between erase operations. Drivers are
* not expected to count write accesses, and neither do this module's
* functions: The performance impact would be too great. It is up to the
* application to only write to erased memory once. Failure to do so may damage
* hardware.
jue89 marked this conversation as resolved.
Show resolved Hide resolved
*
* This MTD API currently does not specify which value will be read from an
* erased sector.
*
* @file
*
Expand Down Expand Up @@ -96,6 +110,7 @@ typedef struct {
uint32_t sector_count; /**< Number of sector in the MTD */
uint32_t pages_per_sector; /**< Number of pages by sector in the MTD */
uint32_t page_size; /**< Size of the pages in the MTD */
uint32_t write_size; /**< Minimum size and alignment of writes to the device */
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about it, doesn't this also apply for reads?

Copy link
Member Author

Choose a reason for hiding this comment

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

(Weird, did GitHub reject my mail? If delayed, sorry for the duplicate.)

It would (albeit as a separate parameter; eg. some STM32 flashes are
written in 64bit words but read down to byte granularity), but unlike
with writes, drivers can trivially* and safely (no RWM) emulate that.

So we could, but unlike the write case this is barely actionable. For
writes, the user (like FlashDB) might use the information to reconsider
its data layout. For reads I doubt it'd influence the layout, and then
when there is a large read size, it'd just shift the complexity and the
stack use from the driver to the consumer (which, unlike the driver,
can't employ clever tricks any more).

*: Might cost an arm and a leg on the stack, or not if the driver (say)
splits the 512 byte SPI read that the memory might require it to do into
some bytes that are read to /dev/null before reading into the target
memory.

#if defined(MODULE_MTD_WRITE_PAGE) || DOXYGEN
void *work_area; /**< sector-sized buffer (only present when @ref mtd_write_page is enabled) */
#endif
Expand All @@ -105,13 +120,22 @@ typedef struct {
* @brief MTD driver can write any data to the storage without erasing it first.
*
* If this is set, a write completely overrides the previous values.
*
* Its absence makes no statement on whether or not writes to memory areas that
* have been written to previously are allowed, and if so, whether previously
* written bits should be written again or not written.
*/
#define MTD_DRIVER_FLAG_DIRECT_WRITE (1 << 0)

/**
* @brief MTD driver supports arbitrary clearing overwrites
*
* If this is set, (arbitrarily) many writes are permitted per write size, and
* the result is the old value bitwise-AND the written value.
*
* This property is common for managed flash memories. (By comparison, the raw
* flash often used internally by MCUs may not allow overwrites, or may allow
* them with the same semantics, but only for a limited number of writes
* between erasures; there is currently no flag describing these any further).
*/
#define MTD_DRIVER_FLAG_CLEARING_OVERWRITE (1 << 1)
Comment on lines +126 to +137
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this information useful, yet? We don't know the erased state of the underlying storage (0xff vs 0x00).


/**
* @brief MTD driver interface
*
Expand Down Expand Up @@ -315,8 +339,9 @@ int mtd_read_page(mtd_dev_t *mtd, void *dest, uint32_t page, uint32_t offset, ui
* @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. Though some devices might enforce alignment
* on both @p addr and @p buf.
* but the buffer cannot overlap two pages.
*
* Both parameters must be multiples of the device's write size.
*
* @param mtd the device to write to
* @param[in] src the buffer to write
Expand All @@ -342,6 +367,8 @@ int mtd_write(mtd_dev_t *mtd, const void *src, uint32_t addr, uint32_t count);
*
* This performs a raw write, no automatic read-modify-write cycle is performed.
*
* Both @p offset and @p size must be multiples of the device's write size.
*
* @p offset must be smaller than the page size
*
* @param mtd the device to write to
Expand Down
3 changes: 3 additions & 0 deletions drivers/include/mtd_flashpage.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ 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, \
}, \
}

Expand Down
1 change: 1 addition & 0 deletions drivers/include/mtd_mapper.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
* .sector_count = SECTOR_COUNT / 2,
* .pages_per_sector = PAGE_PER_SECTOR,
* .page_size = PAGE_SIZE,
* .write_size = WRITE_SIZE,
* },
* .parent = &parent,
* .sector = SECTOR_COUNT / 2
Expand Down
9 changes: 9 additions & 0 deletions drivers/mtd/mtd.c
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,15 @@ int mtd_init(mtd_dev_t *mtd)

int res = -ENOTSUP;

/* Drivers preceding the introduction of write_size need to set it. While
* this assert breaks applications that previously worked, it is likely
* that these applications silently assumed a certain write size and would
* break when switching the MTD backend. When tripping over this assert,
* please update your driver to produce a correct value *and* place a check
* in your application for whether the backend allows sufficiently small
* writes. */
assert(mtd->write_size != 0);

if (mtd->driver->init) {
res = mtd->driver->init(mtd);
}
Expand Down
1 change: 1 addition & 0 deletions drivers/mtd_mapper/mtd_mapper.c
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ static int _init(mtd_dev_t *mtd)
assert(backing_mtd->page_size == region->mtd.page_size);
assert(backing_mtd->pages_per_sector == region->mtd.pages_per_sector);
assert(backing_mtd->sector_count >= region->mtd.sector_count);
assert(backing_mtd->write_size == region->mtd.write_size);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we instead just

Suggested change
assert(backing_mtd->write_size == region->mtd.write_size);
backing_mtd->write_size = region->mtd.write_size;

Seems pretty redundant to require the user to set this.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but I value consistency more highly here (page size and pages per sector are set manually as well).


/* offset + region size must not exceed the backing device */
assert(region->sector + region->mtd.sector_count <= backing_mtd->sector_count);
Expand Down
1 change: 1 addition & 0 deletions drivers/mtd_mci/mtd_mci.c
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ static int mtd_mci_init(mtd_dev_t *dev)

dev->pages_per_sector = 1;
dev->page_size = SD_HC_BLOCK_SIZE;
dev->write_size = SD_HC_BLOCK_SIZE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dev->write_size = SD_HC_BLOCK_SIZE;
dev->write_size = 1; /* TODO: move RMW code to MTD */

mci_ioctl(GET_SECTOR_COUNT, &dev->sector_count);

DEBUG("sector size: %lu\n", dev->page_size);
Expand Down
1 change: 1 addition & 0 deletions drivers/mtd_sdcard/mtd_sdcard.c
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ static int mtd_sdcard_init(mtd_dev_t *dev)

/* sdcard_spi always uses the fixed block size of SD-HC cards */
dev->page_size = SD_HC_BLOCK_SIZE;
dev->write_size = SD_HC_BLOCK_SIZE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dev->write_size = SD_HC_BLOCK_SIZE;
dev->write_size = IS_USED(MODULE_MTD_WRITE_PAGE) ? 1 : SD_HC_BLOCK_SIZE; /* TODO: move RMW code to MTD */

return 0;
}
return -EIO;
Expand Down
3 changes: 3 additions & 0 deletions drivers/mtd_spi_nor/mtd_spi_nor.c
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,9 @@ static int mtd_spi_nor_init(mtd_dev_t *mtd)
mtd->sector_count = mtd_spi_nor_get_size(&dev->jedec_id)
/ (mtd->pages_per_sector * mtd->page_size);
}
/* SPI NOR is byte addressable; instances don't need to configure that */
assert(mtd->write_size <= 1);
mtd->write_size = 1;
_set_addr_width(mtd);

DEBUG("mtd_spi_nor_init: %" PRIu32 " bytes "
Expand Down
6 changes: 6 additions & 0 deletions tests/mtd_mapper/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@
#ifndef PAGE_SIZE
#define PAGE_SIZE 64
#endif
#ifndef WRITE_SIZE
#define WRITE_SIZE 4
#endif

#define MEMORY_PAGE_COUNT PAGE_PER_SECTOR * SECTOR_COUNT

Expand Down Expand Up @@ -180,6 +183,7 @@ static mtd_dev_t dev = {
.sector_count = SECTOR_COUNT,
.pages_per_sector = PAGE_PER_SECTOR,
.page_size = PAGE_SIZE,
.write_size = WRITE_SIZE,
};

static mtd_mapper_parent_t _parent = MTD_PARENT_INIT(&dev);
Expand All @@ -190,6 +194,7 @@ static mtd_mapper_region_t _region_a = {
.sector_count = SECTOR_COUNT / 2,
.pages_per_sector = PAGE_PER_SECTOR,
.page_size = PAGE_SIZE,
.write_size = WRITE_SIZE,
},
.parent = &_parent,
.sector = 0,
Expand All @@ -201,6 +206,7 @@ static mtd_mapper_region_t _region_b = {
.sector_count = SECTOR_COUNT / 2,
.pages_per_sector = PAGE_PER_SECTOR,
.page_size = PAGE_SIZE,
.write_size = WRITE_SIZE,
},
.parent = &_parent,
.sector = SECTOR_COUNT / 2,
Expand Down
1 change: 1 addition & 0 deletions tests/pkg_littlefs/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ static mtd_dev_t dev = {
.sector_count = SECTOR_COUNT,
.pages_per_sector = PAGE_PER_SECTOR,
.page_size = PAGE_SIZE,
.write_size = 1,
};

static mtd_dev_t *_dev = (mtd_dev_t*) &dev;
Expand Down
1 change: 1 addition & 0 deletions tests/pkg_littlefs2/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ static mtd_dev_t dev = {
.sector_count = SECTOR_COUNT,
.pages_per_sector = PAGE_PER_SECTOR,
.page_size = PAGE_SIZE,
.write_size = 1,
};

static mtd_dev_t *_dev = (mtd_dev_t*) &dev;
Expand Down
1 change: 1 addition & 0 deletions tests/pkg_spiffs/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ static mtd_dev_t dev = {
.sector_count = SECTOR_COUNT,
.pages_per_sector = PAGE_PER_SECTOR,
.page_size = PAGE_SIZE,
.write_size = 1,
};

static mtd_dev_t *_dev = (mtd_dev_t*) &dev;
Expand Down
4 changes: 4 additions & 0 deletions tests/unittests/tests-mtd/tests-mtd.c
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@
#ifndef PAGE_SIZE
#define PAGE_SIZE 128
#endif
#ifndef WRITE_SIZE
#define WRITE_SIZE 1
#endif

static uint8_t dummy_memory[PAGE_PER_SECTOR * PAGE_SIZE * SECTOR_COUNT];

Expand Down Expand Up @@ -115,6 +118,7 @@ static mtd_dev_t _dev = {
.sector_count = SECTOR_COUNT,
.pages_per_sector = PAGE_PER_SECTOR,
.page_size = PAGE_SIZE,
.write_size = WRITE_SIZE,
};

static mtd_dev_t *dev = (mtd_dev_t*) &_dev;
Expand Down