Support spi/qspi flash chips over 16MB. #5166
Conversation
abcecd6 to
eab1a10
Compare
|
@dpgeorge I just tested this newer change set on master and it works well for my 32MB spiflash chip, split into two partitions (fat and lfs). |
Yes. That was trying to fix the problems with the CPU doing speculative reads from QSPI flash, and in the end it was not needed (at least so far I everything works fine).
Great! I think I have an STM32F7 discovery board with >16MB flash which I can test this on. |
ports/stm32/qspi.c
Outdated
| uint8_t cmd = 0xeb; // quad read opcode | ||
| uint8_t adsize = 2; // 24-bit address size | ||
| // For chip addresses over 16MB, 32 bit addressing is required instead of 24 bit | ||
| if ((MICROPY_HW_QSPIFLASH_SIZE_BITS_LOG2 -3 -1) >= 24) { |
There was a problem hiding this comment.
Since this is a macro this test could probably become static/compile-time, and this whole set of changes made compile-time configurable.
Although there could be value in keeping it dynamically selectable I think for now it'd be good to try and make it static.
Eg:
#if (MICROPY_HW_QSPIFLASH_SIZE_BITS_LOG2 -3 -1) >= 24
#define QSPI_CMD 0xec
#define QSPI_ADSIZE 3
#else
#define QSPI_CMD 0xeb
#define QSPI_ADSIZE 2
#endif There was a problem hiding this comment.
While I like the idea of getting to the point of being able to configure spi / qspi flash devices at runtime, this does make sense to have static in the mean time! It can be reverted to a dynamic form if/when the rest of the module supports it!
eab1a10 to
a8e5b3d
Compare
a8e5b3d to
4bd0643
Compare
|
Memory map command settings now static / compile-time configured. |
|
Is this going to be merged any time soon ? |
drivers/bus/qspi.h
Outdated
| @@ -52,6 +52,22 @@ typedef struct _mp_soft_qspi_obj_t { | |||
| mp_hal_pin_obj_t io3; | |||
| } mp_soft_qspi_obj_t; | |||
|
|
|||
| #define MP_SPI_ADDR_32B(addr) (addr & 0xF000) | |||
There was a problem hiding this comment.
Shouldn't this be (addr & 0xff000000)? Since addr is measured in bytes everywhere (I think, that would need to be verified).
There was a problem hiding this comment.
Wow... I'm, not sure now where the 0xF000 came from... unless in an earlier iteration of this change that val was in blocks?
Either way, yes I agree with the change. It also likely explains the corruption issue I've posted about.
Thanks for picking this up!
drivers/bus/softqspi.c
Outdated
| @@ -168,9 +168,10 @@ STATIC void mp_soft_qspi_write_cmd_data(void *self_in, uint8_t cmd, size_t len, | |||
|
|
|||
| STATIC void mp_soft_qspi_write_cmd_addr_data(void *self_in, uint8_t cmd, uint32_t addr, size_t len, const uint8_t *src) { | |||
| mp_soft_qspi_obj_t *self = (mp_soft_qspi_obj_t*)self_in; | |||
| uint8_t cmd_buf[4] = {cmd, addr >> 16, addr >> 8, addr}; | |||
| uint8_t cmd_buf[4] = {cmd}; | |||
There was a problem hiding this comment.
This needs to be cmd_buf[5] in the case of 32-bit addressing (gcc 9 caught this as a compile error).
|
Thanks @andrewleech for this, it's nice and clean. I tested it (with the 2 changes suggested above) on an STM32F769DISC with the 64MB spiflash configured for storage, and it seems to work correctly. |
Over 16MB, 32-bit addressing is required rather than the standard 24-bit.
4bd0643 to
ade8bf3
Compare
|
Rebased, some code-style changes made, and merged in 30501d3 Thank you! |
Don't double-list modules that MP_REGISTER_MODULE
Over 16MB, 32-bit addressing is required rather than the standard 24-bit.
These changes were originally part of dpgeorge#11
@dpgeorge was your commit "stm32/qspi: Abort any transfers if peripheral is busy." intentionally left out from the merging of your pybd branch?
I haven't done any of the suggested cleanup / separating of the larger spiflash support yet, this is just pushing a rebased version of my previous attempt here.