drivers: mtd_spi_nor MTD interface driver for SPI NOR flash chips by vincent-d · Pull Request #6762 · RIOT-OS/RIOT

vincent-d · 2017-03-20T08:45:05Z

This is based on #5668 with some minor improvements with erase and this has been ported to new spi interface.

jnohlgard · 2017-03-27T09:28:45Z

There's an accidental autosave file included in this PR

vincent-d · 2017-03-27T10:41:27Z

Fixed

jnohlgard

Found some minor comments. I will test the code on mulle later tonight or tomorrow

jnohlgard · 2017-03-27T09:23:25Z

    .private_data = &mulle_nvram_dev,
 };

+mtd_spi_nor_t mulle_nor_dev = {


Should be static

jnohlgard · 2017-03-27T09:25:13Z

 USEMODULE += devfs
+USEMODULE += mtd_spi_nor
+
+#FEATURES_REQUIRED += periph_spi


Remove, already handled by drivers/Makefile.dep

jnohlgard · 2017-03-27T09:26:18Z

    mulle_nvram->write(mulle_nvram, &rec.u8[0], MULLE_NVRAM_BOOT_COUNT, sizeof(rec.u32));
 }
+
+int mulle_nor_init(void)


Forgot to add a call to this function from board_init

jnohlgard · 2017-03-27T09:29:14Z

+#define MTD_SPI_NOR_H_
+
+#include <stdint.h>
+#include <stdbool.h>


Not used include

jnohlgard · 2017-03-27T09:30:22Z

@@ -0,0 +1,3 @@
+MODULE = mtd_spi_nor


Not needed when the directory name is the same as the module name

jnohlgard · 2017-03-27T09:32:48Z

+            break;
+        }
+#if MODULE_XTIMER
+        xtimer_usleep(50 * US_PER_MS); /* TODO magic number */


Let's move this to a macro define at the top of this file.

jnohlgard · 2017-03-28T12:39:09Z

Needs rebase

vincent-d · 2017-03-28T13:06:20Z

rebased

jnohlgard · 2017-03-28T15:37:49Z

+        /* 4 KiO sectors can be erased with sector erase command */
+        mtd_spi_cmd_addr_write(dev, dev->opcode->block_erase_32k, addr_be, NULL, 0);
+    }
+    else {


size is not checked here. This causes the unit test for erase of invalid size to fail.

jnohlgard · 2017-03-28T15:43:06Z

+         * software bugs (the erase-all-your-files kind) */
+        DEBUG("addr = %" PRIx32 " ~dev->erase_addr_mask = %" PRIx32 "", addr, ~dev->sec_addr_mask);
+        DEBUG("mtd_spi_nor_erase: ERR: erase addr not aligned on %" PRIu32 " byte boundary.\n",
+            sector_size);


riot/drivers/mtd_spi_nor/mtd_spi_nor.c:447:13: error: ‘sector_size’ undeclared (first use in this function) sector_size); ^

jnohlgard

Found a small bug in mtd_spi_cmd_addr_write

jnohlgard · 2017-03-29T10:39:54Z

+    do {
+        /* Send opcode followed by address */
+        spi_transfer_byte(dev->spi, dev->cs, true, opcode);
+        spi_transfer_bytes(dev->spi, dev->cs, true, (char *)addr_buf, NULL, dev->addr_width);


This currently keeps the CS pin asserted on erase commands (count = 0), causes unit test failures on Mulle
Fixed:

bool cont = (count > 0); /* only keep CS asserted when there is data that follows */ spi_transfer_bytes(dev->spi, dev->cs, cont, (char *)addr_buf, NULL, dev->addr_width); /* Write data */ if (cont) {

Fixed, thanks

jnohlgard · 2017-03-29T11:02:16Z

mtd_tests.test_mtd_erase (tests/unittests/tests-mtd/tests-mtd.c 152) exp 0 was -139

    /* Erase 2nd - 3rd sector */
    ret = mtd_erase(dev, dev->pages_per_sector * dev->page_size,
                    dev->pages_per_sector * dev->page_size * 2);
    TEST_ASSERT_EQUAL_INT(0, ret);

erase only supports a single page in the current implementation

mtd_tests.test_mtd_write_read (tests/unittests/tests-mtd/tests-mtd.c 209) exp -139 was 9

    /* out of bounds write (addr) */
    ret = mtd_write(dev, buf, dev->pages_per_sector * dev->page_size * dev->sector_count,
                    sizeof(buf));
    TEST_ASSERT_EQUAL_INT(-EOVERFLOW, ret);

not sure what happens here.

jnohlgard · 2017-03-29T11:09:34Z

jnohlgard@451af3d has some configuration for the Mulle MTD flash

Depends on #6810

vincent-d · 2017-03-29T12:25:47Z

Fixed unittests. It passes on our hardware.
check bounds white write and erase

jnohlgard · 2017-03-29T14:27:08Z

OK to squash

This is a generic SPI NOR flash driver which can be used with many different flash chips.

vincent-d · 2017-03-29T14:54:30Z

Squashed

jnohlgard

Tested working on Mulle, but only when #6810 is added.

jnohlgard · 2017-03-30T09:22:05Z

@vincent-d do you mind approving #6810? It's a very small changeset

vincent-d · 2017-03-30T09:24:26Z

\o/ Thanks @gebart

benpicco

I think I just discovered an old bug that was never noticed in the last 4 years 😨

benpicco · 2020-10-25T22:40:00Z

+    .page_program    = 0x02,
+    .sector_erase    = 0x20,
+    .block_erase_32k = 0x52,
+    .block_erase     = 0xd8,


This is 64KB BLOCK ERASE

benpicco · 2020-10-25T22:41:05Z

+    else {
+        for (size_t i = 0; i < size / sector_size; i++) {
+            mtd_spi_cmd_addr_write(dev, dev->opcode->block_erase, addr_be, NULL, 0);
+            addr += sector_size;


and sector_size != 64k
So this will delete a lot more data than it should.

vincent-d assigned jnohlgard and kaspar030 Mar 20, 2017

jnohlgard reviewed Mar 27, 2017

View reviewed changes

This was referenced Mar 27, 2017

mx25v NOR SPI flash driver [wip] #5660

Closed

WIP mtd_spi_nor: MTD interface driver for SPI NOR flash chips #5668

Closed

vincent-d force-pushed the feat/mtd-spi-nor branch from 9281eaf to 66d8e09 Compare March 28, 2017 13:06

This was referenced Mar 28, 2017

pkg: add SPIFFS support #5625

Merged

mtd: Redefine MTD_0 as a mtd_dev_t * #6808

Merged

jnohlgard reviewed Mar 28, 2017

View reviewed changes

jnohlgard requested changes Mar 29, 2017

View reviewed changes

jnohlgard mentioned this pull request Mar 29, 2017

mulle: Configure hardware CS for all on-board SPI slaves #6810

Merged

vincent-d force-pushed the feat/mtd-spi-nor branch from c7c5ea3 to e8219d9 Compare March 29, 2017 12:22

vincent-d mentioned this pull request Mar 29, 2017

stm32_common/spi: fix receive after transmit-only error #6764

Merged

Joakim Nohlgård added 2 commits March 29, 2017 16:52

mtd_spi_nor: MTD interface driver for SPI NOR flash memory

9313d8e

This is a generic SPI NOR flash driver which can be used with many different flash chips.

mulle: Add a configuration for the onboard flash memory

2c6088d

vincent-d force-pushed the feat/mtd-spi-nor branch from 42a82c5 to 2c6088d Compare March 29, 2017 14:53

vincent-d added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 29, 2017

jnohlgard approved these changes Mar 30, 2017

View reviewed changes

jnohlgard merged commit e9db827 into RIOT-OS:master Mar 30, 2017

vincent-d deleted the feat/mtd-spi-nor branch March 30, 2017 09:54

benpicco reviewed Oct 25, 2020

View reviewed changes

Uh oh!

Conversation

vincent-d commented Mar 20, 2017

Uh oh!

jnohlgard commented Mar 27, 2017

Uh oh!

vincent-d commented Mar 27, 2017

Uh oh!

jnohlgard left a comment

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!

jnohlgard commented Mar 28, 2017

Uh oh!

vincent-d commented Mar 28, 2017

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!

jnohlgard left a comment

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!

jnohlgard commented Mar 29, 2017

Uh oh!

jnohlgard commented Mar 29, 2017

Uh oh!

vincent-d commented Mar 29, 2017

Uh oh!

jnohlgard commented Mar 29, 2017

Uh oh!

vincent-d commented Mar 29, 2017

Uh oh!

jnohlgard left a comment • 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!

jnohlgard commented Mar 30, 2017

Uh oh!

vincent-d commented Mar 30, 2017

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benpicco Oct 25, 2020 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jnohlgard left a comment •

edited

Loading

benpicco Oct 25, 2020 •

edited

Loading