Skip to content

samd: Support using a board's external flash chip as boot file system. #10233

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

Merged
merged 5 commits into from
Jun 5, 2023

Conversation

robert-hh
Copy link
Contributor

@robert-hh robert-hh commented Dec 15, 2022

Quite a few boards provide an external on-board flash chip, which this PR supports for the default file system. That provides a larger file system and faster I/O operation, especially for the SAMD51 devices:

  • a simple Python flash block device driver, similar to the one already used for the ESP8266. This block device driver is then used with C-coded raw flash drivers.
  • a flash driver for the built-in MCU flash, which is the previous samd_flash.c omitting the block device code.
  • a QSPI flash driver specific for the SAMD51 series.
  • a SPI flash driver, which is port agnostic. It should work with any port providing a SPI and PIN class.
  • a changed _boot.py, which enables the appropriate block device/flash device driver combination.

The flash device to be used for the boot file system is defined in the board's mpconfigboard.h.

Note: On some SAMD51 devices, using the QSPI flash for the boot file system increases the code execution speed by about 60%.

@robert-hh robert-hh force-pushed the samd_flash branch 2 times, most recently from 7fa1233 to c2bcbf2 Compare December 21, 2022 20:49
@robert-hh robert-hh force-pushed the samd_flash branch 5 times, most recently from bee6acd to 31c20c0 Compare January 31, 2023 09:17
robert-hh added a commit to robert-hh/micropython that referenced this pull request Apr 8, 2023
mbedtls adds about 100k of code. Therefore it is enabled
at the moment for SAMD51x20 devices only. It can change if
PR micropython#10233 is merged.
robert-hh added a commit to robert-hh/micropython that referenced this pull request Apr 12, 2023
mbedtls adds about 100k of code. Therefore it is enabled
at the moment for SAMD51x20 devices only. It can change if
PR micropython#10233 is merged.
@dpgeorge
Copy link
Member

This is definitely a great feature to have.

But I'm curious why you didn't use the existing drivers/memory/spiflash.[ch] driver for this? That driver supports both SPI and QSPI operation and should be generic enough to plug in to the samd port. All that's required is a SPI and/or QSPI backend based on the API defined in driver/bus/.

# Try to mount an external flash drive.
try:
if hasattr(samd, "QSPIflash"):
bdev = FlashBdev(samd.QSPIflash())
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why the FlashBdev class is needed, it is a very thin wrapper. Why not just make QSPIflash, SPIflash and MCUflash implement the block protocol? Then they can be used directly (and the existing samd.Flash could just be renamed to samd.MCUFlash).

Copy link
Contributor Author

@robert-hh robert-hh May 22, 2023

Choose a reason for hiding this comment

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

Thank you for reviewing. FlashBdevI is intentionally as small as possible, and I wanted to be able to support more than one flash type at a device, like external flash and mcuflash. The block device driver was initially combined with the mcuflash driver. But yes, the block device driver can as well be moved back into the specific flash type drivers.

About drivers/memory/spiflash.[ch]. I had seen them sometime during the development. The seemed to me pretty specific for Pyboard needing more driver code from the sptm32 directory, and I could not tell how to use them, besides seeming quite large..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I added a commit which integrates the block device driver into the QSPI and SPI flash drivers. It rolls also back the change to samd_flash.c. If that's ok, the last two commits can be squashed into one.

@dpgeorge
Copy link
Member

About drivers/memory/spiflash.[ch]. I had seen them sometime during the development. The seemed to me pretty specific for Pyboard needing more driver code from the sptm32 directory, and I could not tell how to use them, besides seeming quite large..

That SPI flash driver is intended to be used by any port. It supports both normal SPI and QSPI. It also supports caching to be able to erase parts that are smaller than the native erase size of the flash, but that's optional via MICROPY_HW_SPIFLASH_ENABLE_CACHE.

In the code in this PR there is duplication between the SPI and QSPI driver. That duplication is gone if you use drivers/memory/spiflash.c. Also the latter code handles errors from timeouts.

IMO it doesn't make sense to have duplicated SPI flash driver code in the samd port and in drivers/memory. They are both doing the same thing. It would be great if you can try and use the drivers code instead in this PR. What's needed is:

  1. A backend implementation of the (Q)SPI protocol. There are a few places you can look at to see how it's done: ports/stm32/qspi.c, drivers/bus/softspi.c and drivers/bus/softqspi.c.
  2. A frontend for the Python object that implements the block protocol. In stm32 this is in ports/stm32/storage.c combined with ports/stm32/spibdev.c. But that's pretty complicated and it can be much simpler.

Eventually part (2) above could be made general enough to use by other ports.

@robert-hh
Copy link
Contributor Author

Thank you for your hints. Part of these I had seen when considering to use drivers/memory/spiflash.c.
Bur in the existing PR there is less duplication that it seems, since a board is going to use either SPI of QSPI drivers, and both are made to be as small as possible to fit the constrained flash of especially the SAMD21 boards. I had the impression, that using a generic approach would result in a substantially larger code size.
Reworking the existing PR according your suggestion seems not reasonable. Instead, it has to be started over again.
With your existing PR, I followed your suggestion include the block device driver part in each and every flash driver, which reduced the code size.

@dpgeorge
Copy link
Member

Reworking the existing PR according your suggestion seems not reasonable. Instead, it has to be started over again.

Yes I appreciate that.

OK, so let's stick with what we have here for the moment. Maybe we can refactor at some later stage.


One thing I would suggest to simplify further what you have here is:

  1. hard code the SPI flash parameters in the SPIflash constructor, ie always just use the compile time configuration of the SPI pins and baudrate and don't accept any arguments to the SPIflash constructor
  2. name all the flash types the same, just samd.Flash() (only one is enabled at any one time, I assume)
  3. this will allow to simplify _boot.py, and it could even go back to how it was before this PR

@robert-hh
Copy link
Contributor Author

One thing I would suggest to simplify further what you have here is

Thanks. That sounds good. I'll implement that later today.

@robert-hh
Copy link
Contributor Author

this will allow to simplify _boot.py, and it could even go back to how it was before this PR

One challenge is that the progsize values are different.

@dpgeorge
Copy link
Member

One challenge is that the progsize values are different.

I suggest using 256 in all cases.

(Or if it really can't spare the RAM for that, either use gc.mem_free() to work out the size, or add an ioctl to samd.Flash() which returns a suggested optimal value for progsize.)

@robert-hh
Copy link
Contributor Author

robert-hh commented Jun 3, 2023

So I moved the reference to Peter's code in the copyright note, removed the reference to the not used EEPROM parts of Christopher and renamed some variables to more expressive words. Force pushed in the samd_spiflash.c file.
Edit: This is the diff

diff --git a/ports/samd/samd_spiflash.c b/ports/samd/samd_spiflash.c
index 32563762f..724916f65 100644
--- a/ports/samd/samd_spiflash.c
+++ b/ports/samd/samd_spiflash.c
@@ -3,6 +3,7 @@
  *
  * The MIT License (MIT)
  *
+ * Copyright (c) 2019-2020 Peter Hinch
  * Copyright (c) 2023 Robert Hammelrath
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
@@ -23,10 +24,6 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  *
- * Based on spiflash.py by Christopher Arndt and the EEPROM library of Peter Hinch
- *
- * https://github.com/SpotlightKid/micropython-stm-lib/tree/master/spiflash
- * https://github.com/peterhinch/micropython_eeprom.git
  */
 
 #include <stdint.h>
@@ -44,14 +41,13 @@
 #define _PROGRAM_PAGE_INDEX (1)
 #define _SECTOR_ERASE_INDEX (2)
 
-const uint8_t _CMDS3BA[] = {0x03, 0x02, 0x20};  // CMD_READ CMD_PROGRAM_PAGE CMD_ERASE_4K
-const uint8_t _CMDS4BA[] = {0x13, 0x12, 0x21};  // CMD_READ CMD_PROGRAM_PAGE CMD_ERASE_4K
+const uint8_t _COMMANDS_24BIT[] = {0x03, 0x02, 0x20};  // READ, PROGRAM_PAGE, ERASE_4K
+const uint8_t _COMMANDS_32BIT[] = {0x13, 0x12, 0x21};  // READ, PROGRAM_PAGE, ERASE_4K
 
-#define CMD_JEDEC_ID (0x9F)
-#define CMD_READ_STATUS (0x05)
-#define CMD_WRITE_ENABLE (0x06)
-#define CMD_READ_UID (0x4B)
-#define CMD_READ_SFDP (0x5A)
+#define COMMAND_JEDEC_ID (0x9F)
+#define COMMAND_READ_STATUS (0x05)
+#define COMMAND_WRITE_ENABLE (0x06)
+#define COMMAND_READ_SFDP (0x5A)
 #define PAGE_SIZE (256)
 #define SECTOR_SIZE (4096)
 
@@ -59,7 +55,7 @@ typedef struct _spiflash_obj_t {
     mp_obj_base_t base;
     mp_obj_base_t *spi;
     mp_hal_pin_obj_t cs;
-    bool addr4b;
+    bool addr_is_32bit;
     uint16_t pagesize;
     uint16_t sectorsize;
     const uint8_t *commands;
@@ -81,7 +77,7 @@ static void wait(spiflash_obj_t *self) {
     // 100000 wait 500ms max. at 120Mhz. Sector erase lasts about
     // 100ms worst case, page write is < 1ms.
     do {
-        msg[0] = CMD_READ_STATUS;
+        msg[0] = COMMAND_READ_STATUS;
         mp_hal_pin_write(self->cs, 0);
         spi_transfer((mp_obj_base_t *)self->spi, 2, msg, msg);
         mp_hal_pin_write(self->cs, 1);
@@ -91,7 +87,7 @@ static void wait(spiflash_obj_t *self) {
 static void get_id(spiflash_obj_t *self, uint8_t id[3]) {
     uint8_t msg[1];
 
-    msg[0] = CMD_JEDEC_ID;
+    msg[0] = COMMAND_JEDEC_ID;
     mp_hal_pin_write(self->cs, 0);
     spi_transfer(self->spi, 1, msg, NULL);
     spi_transfer(self->spi, 3, id, id);
@@ -101,20 +97,20 @@ static void write_addr(spiflash_obj_t *self, uint8_t cmd, uint32_t addr) {
     uint8_t msg[5];
     uint8_t index = 1;
     msg[0] = cmd;
-    if (self->addr4b) {
+    if (self->addr_is_32bit) {
         msg[index++] = addr >> 24;
     }
     msg[index++] = (addr >> 16) & 0xff;
     msg[index++] = (addr >> 8) & 0xff;
     msg[index++] = addr & 0xff;
     mp_hal_pin_write(self->cs, 0);
-    spi_transfer(self->spi, self->addr4b ? 5 : 4, msg, msg);
+    spi_transfer(self->spi, self->addr_is_32bit ? 5 : 4, msg, msg);
 }
 
 static void write_enable(spiflash_obj_t *self) {
     uint8_t msg[1];
 
-    msg[0] = CMD_WRITE_ENABLE;
+    msg[0] = COMMAND_WRITE_ENABLE;
     mp_hal_pin_write(self->cs, 0);
     spi_transfer(self->spi, 1, msg, NULL);
     mp_hal_pin_write(self->cs, 1);
@@ -122,7 +118,7 @@ static void write_enable(spiflash_obj_t *self) {
 
 static void get_sfdp(spiflash_obj_t *self, uint32_t addr, uint8_t *buffer, int size) {
     uint8_t dummy[1];
-    write_addr(self, CMD_READ_SFDP, addr);
+    write_addr(self, COMMAND_READ_SFDP, addr);
     spi_transfer(self->spi, 1, dummy, NULL);
     spi_transfer(self->spi, size, buffer, buffer);
     mp_hal_pin_write(self->cs, 1);
@@ -150,7 +146,7 @@ STATIC mp_obj_t spiflash_make_new(const mp_obj_type_t *type, size_t n_args, size
     machine_pin_obj_t *cs = MP_OBJ_TYPE_GET_SLOT(&machine_pin_type, make_new)((mp_obj_t)&machine_pin_type, 2, 0, pin_args);
     self->cs = cs->pin_id;
 
-    self->addr4b = false; // initial setting.
+    self->addr_is_32bit = false; // initial setting.
     self->pagesize = PAGE_SIZE;
     self->sectorsize = SECTOR_SIZE;
     mp_hal_pin_write(self->cs, 1);
@@ -169,7 +165,7 @@ STATIC mp_obj_t spiflash_make_new(const mp_obj_type_t *type, size_t n_args, size
         self->size = 1 << id[2];
     }
 
-    // Get the addr4b flag and the sector size
+    // Get the addr_is_32bit flag and the sector size
     uint8_t buffer[128];
     get_sfdp(self, 0, buffer, 16);  // get the header
     int len = MIN(buffer[11] * 4, sizeof(buffer));
@@ -177,9 +173,9 @@ STATIC mp_obj_t spiflash_make_new(const mp_obj_type_t *type, size_t n_args, size
         int addr = buffer[12] + (buffer[13] << 8) + (buffer[14] << 16);
         get_sfdp(self, addr, buffer, len);  // Get the JEDEC mandatory table
         self->sectorsize = 1 << buffer[28];
-        self->addr4b = ((buffer[2] >> 1) & 0x03) != 0;
+        self->addr_is_32bit = ((buffer[2] >> 1) & 0x03) != 0;
     }
-    self->commands = self->addr4b ? _CMDS4BA : _CMDS3BA;
+    self->commands = self->addr_is_32bit ? _COMMANDS_32BIT : _COMMANDS_24BIT;
 
     return self;
 }

@robert-hh robert-hh force-pushed the samd_flash branch 2 times, most recently from a836f6c to b5d6ea0 Compare June 5, 2023 06:45
@robert-hh
Copy link
Contributor Author

This last change makes the spi_flash object a singleton. It was dynamic before without a need. Still using spi_flash decreases the code size by >368 bytes compared to the driver for internal flash. Using qspi_flash increases the code size by 12 bytes.

#error Only one type of flash must be used
#endif
{ MP_ROM_QSTR(MP_QSTR_Flash), MP_ROM_PTR(&spiflash_type) },
#endif
Copy link
Member

Choose a reason for hiding this comment

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

This (messy?) if logic could be simplified by either:

  1. having a single name for the flash type, spiflash_type, instead of samd_flash_type and samd_qspiflash_type. Then the linker will give an error if multiple are defined
  2. at the top of this file having a #define SPIFLASH_TYPE <relevant type> based on which one is configured. Then the preprocessor will give an error if multiple are defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went for Option 2. The code looks much more clean.

@robert-hh
Copy link
Contributor Author

Updated as suggested.

@param addr the address to read or write from. If the instruction doesn't require an address, this parameter is meaningless.
@param buffer pointer to the data to be written or stored depending on the type is Read or Write
@param size the number of bytes to read or write.
*/
Copy link
Member

Choose a reason for hiding this comment

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

It's OK to leave the comments here, just maybe in a different format that matches existing code, eg:

// Run a single QSPI instruction.
// Parameters are:
// - command: instruction code
// - iframe: ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

#endif
#if MICROPY_HW_SPIFLASH
extern const mp_obj_type_t spiflash_type;
#define SPIFLASH_TYPE spiflash_type
#endif
#if MICROPY_HW_MCUFLASH
Copy link
Member

Choose a reason for hiding this comment

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

Can this be moved up to line 35, and wrap the extern const mp_obj_type_t samd_flash_type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure & done

@robert-hh
Copy link
Contributor Author

Updated, and sorry for the confusion.

PIN_PA08,D4
PIN_PA08,FLASH_MOSI
PIN_PA14,FLASH_MISO
PIN_PA09,FLASH_SCK
Copy link
Member

Choose a reason for hiding this comment

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

Should these be renamed? There's no longer any need to have them accessible in _boot.py as these names.

It's a little inconsistent that these are called FLASH and the others below are QSPI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are used in samd_spiflash.c. And they are called FLASH_xxxx, to make clear that they are connected to the on-board flash chip, and user accessible SPI pins of a board.

Copy link
Member

Choose a reason for hiding this comment

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

You're right!

OK, this is fine then.

@@ -39,6 +39,9 @@ extern const mp_obj_type_t machine_dac_type;
extern const mp_obj_type_t machine_i2c_type;
#endif
extern const mp_obj_type_t machine_pin_type;
#ifdef MICROPY_HW_QSPIFLASH
extern const mp_obj_type_t machine_qspiflash_type;
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be removed, it's the wrong name anyway (doesn't match what's in the .c file).

Copy link
Contributor Author

@robert-hh robert-hh Jun 5, 2023

Choose a reason for hiding this comment

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

Oh yes. That should not be there.

{ MP_ROM_QSTR(MP_QSTR_pininfo), MP_ROM_PTR(&samd_pininfo_obj) },
{ MP_ROM_QSTR(MP_QSTR_Flash), MP_ROM_PTR(&SPIFLASH_TYPE) },
Copy link
Member

Choose a reason for hiding this comment

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

This can be put back above in its original order (before pininfo).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

uos.mount(vfs, "/")

del vfs, fs_type, bdev, uos, samd, progsize
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised this doesn't crash... progsize isn't defined!

Also, why not delete after the gc.collect(), so that gc can also be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it crashed, but silently. Removed and added del gc.

@@ -27,10 +27,13 @@
#include <stdio.h>

#include "py/runtime.h"
#include "py/mphal.h"
Copy link
Member

Choose a reason for hiding this comment

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

Is this extra include needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No.

}
STATIC MP_DEFINE_CONST_FUN_OBJ_0(samd_flash_init_obj, samd_flash_init);

// FLASH stuff
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment can go. It's not doing anything useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old, not gold.

uint32_t size;
} spiflash_obj_t;

extern const mp_obj_type_t spiflash_type;
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be named samd_spiflash_type, to match the other two (flash, qspi).

Copy link
Contributor Author

@robert-hh robert-hh Jun 5, 2023

Choose a reason for hiding this comment

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

Done. It had that name initially, and then I considered to make it independent from the SAMD port, and so I dropped the samd part of the name. That independent mode required to supply the SPI object to the driver, which was changed. And now it's again a SAMD only driver.

@robert-hh
Copy link
Contributor Author

Updated again into the last commit. Thank you for the detailed review.

robert-hh added 5 commits June 6, 2023 00:42
For SAMD21 devices, the board flash signals must be named in pins.csv as
FLASH_MOSI, FLASH_MISO, FLASH_SCK, FLASH_CS for creating the SPI object.

And rename the QSPI pins to QSPI_xxxx instead of FLASH_xxx.

Signed-off-by: robert-hh <robert@hammelrath.com>
The SPI flash driver includes the block device for being used as a
filesystem.  It provides the same methods as the driver for the internal
flash.

Signed-off-by: robert-hh <robert@hammelrath.com>
The QSPI driver provides the interface for using an on-board QSPI flash for
the filesystem.  It provides the same methods as the driver for the
internal flash and uses the same name.  Therefore, only one of the drivers
for internal flash, SPI flash and QSPI flash must be enabled at a time.

Signed-off-by: robert-hh <robert@hammelrath.com>
Checks are added to ensure, that only one of the flash drivers is selected.

Signed-off-by: robert-hh <robert@hammelrath.com>
Code size limits are charged to:
- SAMD21:    184K -> 248K
- SAMD51x19: 368K -> 496K
- SAMD51x20: 368K -> 1008K

Signed-off-by: robert-hh <robert@hammelrath.com>
@dpgeorge
Copy link
Member

dpgeorge commented Jun 5, 2023

Thanks for updating. I broke up and squashed the last commit as appropriate, and tweaked the wording of the commits.

@robert-hh
Copy link
Contributor Author

Thank you. Sorry for making wore work than needed. My next question would have been, whether I should resort the changes into the respective commits.

@dpgeorge dpgeorge merged commit db5444f into micropython:master Jun 5, 2023
@dpgeorge
Copy link
Member

dpgeorge commented Jun 5, 2023

It's all good. It's easy for me to fix minor things during merge, and saves going back and forth.

@robert-hh robert-hh deleted the samd_flash branch June 6, 2023 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants