Skip to content

Conversation

@maholli
Copy link

@maholli maholli commented Apr 8, 2020

These additions/changes allow single-SPI (as opposed to dual or quad SPI) storage devices to act as external flash memory on the SAMD51 QSPI bus. Also adds a pycubed_mram board.

  • My specific use-case was for NVMRAM (Everspin MR25H40), but this should also enable things like an SD card to serve as external flash storage (and natively mount as a mass storage device).
  • These changes preserve original NOR flash functionality, but enable devices that don't have JEDEC responses or specific page/sector/erase commands can now function.

Guidance needed

Build size was preserved, but I could use help making the #ifdef more succinct and readable for users. Currently, it assumes anything with a #define EXTERNAL_FLASH_QSPI_SINGLE will operate like my NVMRAM, but that isn't realistic (SD cards will still need the sector erase, for example). The changes to ports/atmel-samd/supervisor/qspi_flash.c are a good example of where guidance is needed:

    #ifdef EXTERNAL_FLASH_QSPI_SINGLE
    QSPI->INSTRFRAME.reg = mode |
                           QSPI_INSTRFRAME_ADDRLEN_24BITS |
                           QSPI_INSTRFRAME_TFRTYPE_READMEMORY |
                           QSPI_INSTRFRAME_INSTREN |
                           QSPI_INSTRFRAME_ADDREN |
                           QSPI_INSTRFRAME_DATAEN |
                           QSPI_INSTRFRAME_DUMMYLEN(0);
    #else
    QSPI->INSTRFRAME.reg = mode |
                           QSPI_INSTRFRAME_ADDRLEN_24BITS |
                           QSPI_INSTRFRAME_TFRTYPE_READMEMORY |
                           QSPI_INSTRFRAME_INSTREN |
                           QSPI_INSTRFRAME_ADDREN |
                           QSPI_INSTRFRAME_DATAEN |
                           QSPI_INSTRFRAME_DUMMYLEN(8);
    #endif

Another good example is in supervisor/shared/external_flash/external_flash.c:

static bool wait_for_flash_ready(void) {
    bool ok = true;
    // Both the write enable and write in progress bits should be low.
    #ifdef EXTERNAL_FLASH_QSPI_SINGLE
        // For NVM without a ready bit in status register
        return ok;
    #else

@tannewt tannewt requested a review from dhalbert April 9, 2020 16:49
@dhalbert
Copy link
Collaborator

@maholli We did have single-channel SPI_FLASH_FILESYSTEM available, which uses supervisor/shared/external_flash/spi_flash.c. I'm guessing you were not able to use that, but you could say why?

@maholli
Copy link
Author

maholli commented Apr 16, 2020

@dhalbert I started with SPI_FLASH_FILESYSTEM, but the SAMD51 was crashing before CP could start (The SPI flash device is on the QSPI pins such that I have successfully used the board with a native QSPI device occupying the footprint).

My initial tests were using #define SPI_FLASH_MOSI_PIN etc... in the mpconfigboard.h file as you described. I got GDB running to debug and found it was failing when attempting to configure the QPSI pins for SPI. A quick sanity check showed any existing boards using SPI_FLASH_FILESYSTEM were not trying to do so on the QSPI pins. I was unsuccessful in chasing down where the initial config of the QSPI pins was occurring, so I left them as is and made my minor tweaks to the relevant external_flash.c and qspi_flash.c

@dhalbert
Copy link
Collaborator

I looked at why standard SPI would not work, and it's because the QSPI MOSI/MISO, and SCK pins cannot share a common SERCOM, per the datasheet pin mapping table. So you have to use the QSPI peripheral, which is fine, though more work. Thanks, proceeding on to review.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thanks for adding this feature!

My comments are pretty much about whether you are special-casing your particular chip in the SINGLE case. The SINGLE flag should be able to work with any flash chip. If you need special handling for the MRAM chip, then there should be flag bits set in the chip description struct that say what to do.

Have you tested these changes against regular dual and quad QSPI boards? It would be good to make sure there isn't a regression.

@maholli
Copy link
Author

maholli commented Apr 21, 2020

Thank you @dhalbert for the review!

  • I'll migrate the chip-specific stuff to the features struct and touch base again to verify variable names are appropriate.
  • I'll build and test on a couple different boards with QSPI chips

@maholli
Copy link
Author

maholli commented Apr 28, 2020

@dhalbert All requested changes have been implemented. Thank you for the guidance.

Comments

  • With how supervisor_flash_init() is structured in external_flash.c, it was necessary to add an extra define of EXTERNAL_FLASH_NO_JEDEC in my mpconfigboard.h file to establish the proper flash device. This is because members of the flash_device struct are not yet accessible at that point, but the JEDEC response is used in establishing it.

  • Three new members were added to external_flash_device struct:

    1. no_ready_bit
    2. no_erase_cmd
    3. no_reset_cmd
  • I also built CP for two boards with typical DUAL QSPI external flash chips (my pycubed board and a Feather M4 express), both of which behaved properly during my brief REPL test after updating the firmware via the bootloader.

Let me know if there's any syntax or semantic changes that should be made.

@maholli maholli requested a review from dhalbert April 28, 2020 19:25
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

A few small concerns. In general this is super interesting! 🚀

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Looks good! I also tested this on a Metro M4 with regular QSPI configuration, and also EXTERNAL_FLASH_QSPI_SINGLE and _DUAL, and it worked fine.

@tannewt Any further comments?

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks!

@tannewt tannewt merged commit f7303e6 into adafruit:master May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants