feat(spi): Add support for SPI slave interface - #125
Conversation
9a90f16 to
9c63500
Compare
|
It is hard for me to decide if this feature is worth the additional 1kB of stub, but when we make it ECO version independent it might be nice for parallel flashing so I believe we can proceed. If stub size becomes an issue we can build stubs with only one interface. If we do not need This is just the high level review of the functionality, when we decide on this, I will check the small things, but overall great work, thanks. WDYT about this? BTW we might consider something like |
9c63500 to
e9bdf63
Compare
Dzarda7
left a comment
Dzarda7
left a comment
There was a problem hiding this comment.
Thanks a lot for the effort, left a few comments. I would also suggest renaming spi to something like slave_spi (I should have done it for sdio too, but for spi it is even more confusing as we use it for flashing).
One extra thing, I meant by DRAM_DMA_ALIGNED_ATTR and DRAM_ATTR is something like DMA_ALIGNED_ATTR __attribute__((aligned(4))) for most and L1_CACHE_LINE_SIZE for the chips with L1 cache. If you do not need it here, I think you will definitely need it in esp-flasher-stub for the buffer definition without #ifdefs
@erhankur could you please help explain proper DMA aligning? I found that not only esp32p4 might need L1 cache alignment https://github.com/search?q=repo%3Aespressif%2Fesp-idf%20CACHE_L1_CACHE_LINE_SIZE&type=code
| * The ROM DMA loaders (spi_slave_rom_txdma_load / _rxdma_load) reject any | ||
| * length above LLDESC_SPI_MAX_BUFFER_SIZE (4096 - 4), so this is the true | ||
| * single-descriptor limit, not the 12-bit descriptor field max (0xFFF). */ | ||
| #define SPI_DMA_DESC_MAX_LEN (4096U - 4U) |
There was a problem hiding this comment.
Is this still relevant when we have our own descriptor? Isn't it 16KB?
| * If the receive DMA is already armed, this is a no-op. Call after freeing or | ||
| * claiming a frame buffer to provide the next DMA destination. | ||
| * | ||
| * @param buf Writable 4-byte-aligned receive buffer. |
There was a problem hiding this comment.
Does not alignment depend on the cache also? For P4 I believe it might be 64 bytes but I might be wrong here.
| * @param data Pointer to frame bytes (must be 4-byte aligned). | ||
| * @param len Number of frame bytes, up to the DMA descriptor limit (4092). | ||
| * @return STUB_LIB_OK on success, STUB_LIB_ERR_* on failure. | ||
| */ |
There was a problem hiding this comment.
same here, also with descriptor limit
| * Manually added ROM symbols for esp32c2 | ||
| * These symbols are not in the auto-generated ROM linker scripts | ||
| * but are needed for ECO version handling | ||
| ***************************************/ | ||
|
|
There was a problem hiding this comment.
this file is not necessary now right?
| break; | ||
| case SPI_SLV_CMD_REBOOT: | ||
| esp_rom_software_reset_cpu(0); | ||
| break; |
There was a problem hiding this comment.
do we plan to use this kind of reset? If not, maybe nice place to save some IRAM.
| int stub_target_spi_rearm(uint8_t *buf, size_t max_size) | ||
| { | ||
| if (s_rx_armed) { | ||
| return STUB_LIB_OK; | ||
| } | ||
| if (buf == NULL || max_size == 0) { | ||
| return STUB_LIB_ERR_INVALID_ARG; | ||
| } | ||
|
|
||
| WRITE_PERI_REG(SPI_DMA_INT_CLR_REG, SPI_SLV_RX_DONE); | ||
| spi_slave_rom_rxdma_load(SPI_SLV_HW, buf, (uint32_t)max_size); | ||
|
|
||
| s_seq_rx ^= SPI_SLV_STA_TOGGLE; | ||
| uint32_t rxsta = s_seq_rx | ((uint32_t)max_size << SPI_SLV_STA_LEN_SHIFT); | ||
| s_seq_rx &= ~SPI_SLV_STA_INIT; | ||
| WRITE_PERI_REG(SPI_SLV_REG_RXSTA, rxsta); | ||
|
|
||
| s_rx_armed = true; | ||
| return STUB_LIB_OK; | ||
| } |
There was a problem hiding this comment.
isn't S2 now then limited to the ROM descriptor number instead of the one defined for others?
|
|
||
| int stub_lib_spi_rearm(uint8_t *buf, size_t max_size) | ||
| { | ||
| if (buf == NULL || max_size == 0 || !IS_ALIGNED((uintptr_t)buf, SPI_DMA_ALIGN)) { |
There was a problem hiding this comment.
is SPI_DMA_ALIGN even for P4? I am not really sure about it, I think it might be 64 bytes.
| uint32_t aligned_len = (chunk + 3U) & ~3U; /* RX needs word-aligned length */ | ||
| dmadesc_rx[desc_idx].size = aligned_len & LLDESC_SPI_SIZE_MASK; | ||
| dmadesc_rx[desc_idx].length = aligned_len & LLDESC_SPI_SIZE_MASK; |
There was a problem hiding this comment.
Isn't this potential buffer overflow? Anyway this should not be necessary because the upper function does if (buf == NULL || max_size == 0 || !IS_ALIGNED((uintptr_t)buf, SPI_DMA_ALIGN))
| uint32_t desc_idx = 0; | ||
| while (remaining) { | ||
| uint32_t chunk = (remaining > LLDESC_SPI_MAX_BUFFER_SIZE) ? LLDESC_SPI_MAX_BUFFER_SIZE : remaining; | ||
| uint32_t aligned_len = (chunk + 3U) & ~3U; /* 4-byte DMA granularity */ |
There was a problem hiding this comment.
Here I am still not sure about the 4 byte alignment, if it does not have to be 64.
2 participants
Description
include/esp-stub-lib/spi.hexposes the usual transport surface:stub_lib_spi_is_active,_init,_take_rx_frame,_rearm, and_tx_frame.src/spi.c) that validates arguments (non-NULL, non-zero length, 4-byte DMA alignment, TX size limit) and delegates to weak per-target hooks. Each target drives the ROM's shared W0..W3 handshake registers (VER / RXSTA / TXSTA / CMD) and offloads the actual receive/transmit DMA to ROM SPI-slave helper routines. The design is fully polled (no interrupts): the command handshake is serviced by polling, receive completion is detected via the DMA-done flag, and a host reboot request is handled through the CMD register.*.rom.extra.ld. Targets without an implementation keep the common weak stubs, which simply report SPI as inactive.(
esp-serial-flasher/src/protocol_spi.c), wiressrc/spi.cinto the core, common, and per-target CMake builds, and adds atest_spismoke path to the example stub. Most of the +6585 line count is generated SPI register headers (spi_reg.h) for c2/c3/p4.Speed measurement
Related
Testing
Checklist
Before submitting a Pull Request, please ensure the following: