Skip to content

stm32: add support to PYBD_SF6 for boards with larger flash #17067

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 8 commits into from
Apr 9, 2025

Conversation

dpgeorge
Copy link
Member

@dpgeorge dpgeorge commented Apr 2, 2025

Summary

There are some newer PYBD_SF6 being produced (finally!) which have a larger flash, namely two of 8MiB (instead of the older ones with two of 2MiB).

This PR adds support for these boards. The idea is to have the same PYBD_SF6 firmware run on both old and new boards. That m(eans autodetecting the flash at start-up and configuring all the relevant SPI/QSPI parameters, including for ROMFS and mboot.

I tried many ways of doing this, some more general and others more board-specific. The more general approaches touched a lot more files than this PR which is less general and has a lot of the auto-detection logic in the PYBD_SF6 board directory.

Eventually the logic here could be generalised so it can work with other stm32 boards and even ports, but for now it's mostly contained to PYBD_SF6.

Testing

Tested both mboot and the main firmware on original PYBD_SF6 hardware, and newer hardware with the larger flash.

I added some info to machine.info() to print out the detected flash size, which helps with debugging.

Trade-offs and Alternatives

As discussed above, there are many ways to achieve this goal, some more/less general than others. I think this PR strikes a good balance there and doesn't impact existing boards that don't need this auto-detection feature (such as the stm32 Arduino boards which do use the QSPI interface; a more general solution would need to modify those boards quite a bit).

@dpgeorge
Copy link
Member Author

dpgeorge commented Apr 2, 2025

The changes here could do with a few more comments 😄

@dpgeorge dpgeorge force-pushed the stm32-pybd-rev3b-v5 branch from 2121a16 to 3b501f1 Compare April 2, 2025 02:14
Copy link

github-actions bot commented Apr 2, 2025

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

@dpgeorge dpgeorge added this to the release-1.25.0 milestone Apr 3, 2025
@iabdalkader
Copy link
Contributor

I don't know if you've considered this in alternatives, but one more option is SFDP. I considered it before the OSPI settings tables, but it looked like it will take some effort to implement. If it is ever implemented though, as some generic layer in drivers/memory, it could possibly replace all hard-coded settings and allow supporting any complaint part.

@robert-hh
Copy link
Contributor

The SAMD port uses SFDP to some extend. Unfortunately, the capacity of the flash is not available in the SFDP mandatory section. The size is taken from the JEDEC ID, where there seems to be a frequently used encoding for the size.

@iabdalkader
Copy link
Contributor

the capacity of the flash is not available in the SFDP mandatory section.

I guess this is not a valid alternative then.

Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

I have a couple of comments/suggestions but in general this LGTM. It's fiddly but as far as I can tell it's necessarily fiddly. :/

#define MICROPY_BOARD_EARLY_INIT board_early_init_sf6

#define MICROPY_HW_SPIFLASH_DETECT_DEVICE (1)
#define MICROPY_HW_SPIFLASH_QREAD_NUM_DUMMY(spiflash) (chip_params[spiflash->config == &spiflash_config ? 0 : 1]->qread_num_dummy)
Copy link
Contributor

Choose a reason for hiding this comment

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

Having to look up entries in the 2-element chip_params array seems a little awkward.

Did you look at allowing a board to optionally enable a const mp_spiflash_chip_params_t *params member in mp_spiflash_config_t? That would allow all these ternary expressions to become spiflash->config->params, I think?)

Copy link
Member Author

Choose a reason for hiding this comment

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

It can't be in mp_spiflash_config_t because that's a const struct that defines the bus and not the thing attached to the bus. That struct would need to become non-const, and its definition included in mpconfigboard.h.

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 did try putting such an element in the mp_spiflash_t struct, but it means that drivers/memory/spiflash.h needs to be included in mpconfigboard.h which gets messy (but it's doable).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, my mistake.

What about having a board-level flag like define MICROPY_HW_SPIFLASH_PARAMS 1 (or define MICROPY_HW_SPIFLASH_PARAMS mp_spiflash_chip_params_t if the structure definition is chosen by the board)?

That doesn't seem to create more inversion in the headers than this approach, does it?

Copy link
Member Author

Choose a reason for hiding this comment

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

What about having a board-level flag like define MICROPY_HW_SPIFLASH_PARAMS 1

Do you mean that that config option would add a mp_spiflash_chip_params_t * entry to mp_spiflash_t?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean that that config option would add a mp_spiflash_chip_params_t * entry to mp_spiflash_t?

Yes, like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, this turned out quite good, and allowed me to move mp_spiflash_chip_params_t to drivers/memory/spiflash.h.

The trick to getting it working was to #include "storage.h" in qspi.c, so the latter knows about all the needed struct's (from spi_bdev2). Because it's impossible to include storage.h in mpconfigboard.h, which is what tripped me up in my previous attempt.

Copy link
Contributor

Choose a reason for hiding this comment

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

This approach looks good to me!

@dpgeorge dpgeorge force-pushed the stm32-pybd-rev3b-v5 branch from 7c1c3fd to 4b32ac2 Compare April 8, 2025 04:48
#define MICROPY_BOARD_EARLY_INIT board_early_init_sf6

#define CHIP_PARAMS0 (spi_bdev.spiflash.chip_params)
#define CHIP_PARAMS1 (spi_bdev2.spiflash.chip_params)
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'll rename these macros to something better if we agree this is a good approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, now renamed.

(I still need to squash the fixing commits, but otherwise this should be in its final state now.)

@robert-hh
Copy link
Contributor

@iabdalkader Reading the SFDP spec again it seems I was not right. The size of the flash is coded there, and quite a few more important parameters, like how for write the status bits. Things that would have to be defined otherwise. So using that information looks promising for a more generic approach to flash. It should be possible to drop quite a few board variants/boards which just differ by flash parameters.

@iabdalkader
Copy link
Contributor

iabdalkader commented Apr 8, 2025

@iabdalkader Reading the SFDP spec again it seems I was not right. The size of the flash is coded there, and quite a few more important parameters, like how for write the status bits. Things that would have to be defined otherwise. So using that information looks promising for a more generic approach to flash. It should be possible to drop quite a few board variants/boards which just differ by flash parameters.

Yes, if I remember correctly, it also includes many useful things for OSPI chips (opcodes to switch to octal mode, commands for erase/read/write, dummy cycles? etc..). This would be a very good alternative to many things, but it's quite an undertaking to implement generically, so I also resorted to tables in another port.

@projectgus
Copy link
Contributor

projectgus commented Apr 9, 2025

This would be a very good alternative to many things, but it's quiet an undertaking to implement generically, so I also resorted to tables in another port.

FWIW I fully agree that it's useful but also a big undertaking. It'd be interesting to see if we could find a way to do it generically across ports.

My other recollection of SFDP, from Espressif, is that eventually there are chips which need manually inserted "quirks" because the SFDP parameters are wrong or don't apply in a particular host config, or something like that. That's not a reason to discount it, though.

#define CHIP_PARAMS0 (spi_bdev.spiflash.chip_params)
#define CHIP_PARAMS1 (spi_bdev2.spiflash.chip_params)
#define MICROPY_BOARD_SPIFLASH_CHIP_PARAMS0 (spi_bdev.spiflash.chip_params)
#define MICROPY_BOARD_SPIFLASH_CHIP_PARAMS1 (spi_bdev2.spiflash.chip_params)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but suggest a couple of inline comments here about // QSPI Flash 1, // QSPI Flash 2 or similar, to link these back to the hardware naming scheme

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I've added comments to all the bits in this section.

@dpgeorge dpgeorge force-pushed the stm32-pybd-rev3b-v5 branch from fb5b263 to bbbf67c Compare April 9, 2025 12:02
@robert-hh
Copy link
Contributor

FWIW I fully agree that it's useful but also a big undertaking. It'd be interesting to see if we could find a way to do it generically across ports.

I see a few challenges:

  • Verify, that all information a port needs is in the mandatory section of SFDP. That's the one which should always be there IF the flash device supports SFDP.
  • Get a sufficient test coverage and test confidence over the various chip types that exist.
  • How to deal with flash chips that do not support SFDP. I have seen a few, and those were chips with smaller capacity, like <= 1MByte. At least, all flash chips I have seen larger than 1Mbyte had SFDP support.
  • Generalize it across ports, where possible, as you said.

dpgeorge added 8 commits April 9, 2025 22:36
This commit allows the user of this driver to dynamically configure the SPI
flash chip parameters.  For this, enable `MICROPY_HW_SPIFLASH_CHIP_PARAMS`
and then set the `mp_spiflash_t::chip_params` element to point to a valid
`mp_spiflash_chip_params_t` struct.

Signed-off-by: Damien George <damien@micropython.org>
This commit allows the user of this driver to intercept the SPI flash
initialisation routine and possibly take some action based on the JEDEC id,
for example change the `mp_spiflash_t::chip_params` element.

To do this, enable `MICROPY_HW_SPIFLASH_DETECT_DEVICE` and define a
function called `mp_spiflash_detect()`.

Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
Options for a board to configure ROMFS are:

- Leave ROMFS disabled, do nothing.

- Enable by defining `MICROPY_HW_ROMFS_ENABLE_PARTx` to 1 and then in the
  linker script define `_micropy_hw_romfs_partX_start` and
  `_micropy_hw_romfs_partX_size`.

- Enable by defining `MICROPY_HW_ROMFS_ENABLE_PARTx` to 1 and also define
  `MICROPY_HW_ROMFS_PARTx_START` and `MICROPY_HW_ROMFS_PARTx_SIZE` which
  can be arbitrary expressions (not necessarily static)

Signed-off-by: Damien George <damien@micropython.org>
Allows `MICROPY_HW_QSPIFLASH_SIZE_BITS_LOG2` and
`MICROPY_HW_QSPI_MPU_REGION_SIZE` to be arbitrary expressions, eg function
calls.

The `storage.h` header needs to be included in case access to `spi_bdev_t`
is needed by the macros.

Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
There are some newer PYBD_SF6 being produced which have a larger flash,
namely two of 8MiB (instead of the older ones with two of 2MiB).

This commit adds support for these boards.  The idea is to have the same
PYBD_SF6 firmware run on both old and new boards.  That means autodetecting
the flash at start-up and configuring all the relevant SPI/QSPI parameters,
including for ROMFS and mboot.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge dpgeorge force-pushed the stm32-pybd-rev3b-v5 branch from bbbf67c to db85427 Compare April 9, 2025 12:42
@dpgeorge dpgeorge merged commit db85427 into micropython:master Apr 9, 2025
23 checks passed
@dpgeorge dpgeorge deleted the stm32-pybd-rev3b-v5 branch April 9, 2025 23:52
@robert-hh
Copy link
Contributor

Looking again into the various versions of the SFDP spec and into the SFDP data of various chips the situation is not overly promising. Besides the flash size and the size of the addresses a required information that varies between chips is the method of enabling the quad write mode == setting the QE flag. This piece of information is not covered by the initial SFDP spec. Newer version of SFDP have it in DWORD15 of the data. The problem: None of flash chips which I looked at have this data. They all comply only to the first version of SFDP. That means, that either this QE flag method has to be configured manually, or Quad Write cannot be supported. That sounds worse than it may be. Writing is relatively slow, and at high clock frequencies the data transfer time is way smaller than the page program time, like ~50µs transfer time @ 40Mhz vs 500 µs. page program time.

@dpgeorge
Copy link
Member Author

Besides the flash size and the size of the addresses a required information that varies between chips is the method of enabling the quad write mode == setting the QE flag. This piece of information is not covered by the initial SFDP spec

You could still maintain some simple logic to work out how to set the QE flag. There are probably only a handful of ways that need to be supported for that.

Regardless, I'm not sure it's worth the effort to do this right now. There are many other things to work on!

@robert-hh
Copy link
Contributor

here are probably only a handful of ways that need to be supported for that.

The flag has 6 possible settings, but three of them can be handled the same way, and I have never seen one of them. So it's 3 or 4 variants.

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.

4 participants