[EV3] Implement a SPI flash driver for the EV3 #314

ArcaneNibble · 2025-07-04T21:55:19Z

This PR implements a driver for the EV3 SPI flash memory.

ArcaneNibble · 2025-07-05T03:07:08Z

Hmm, there seems to be a strange heisenbug here. I'm currently working on integrating the ADC into this code, but something I've done seems to have made code that (I promise!) was previously working no longer reach the REPL (hanging without any debug output).

Still investigating...

ArcaneNibble · 2025-07-05T04:24:18Z

With the latest commit the flash + ADC seem to both work (not at the same time but instead using the suggested hack).

I rewrote the entire ADC sampling logic to use DMA.

dlech · 2025-07-05T04:52:29Z

but something I've done seems to have made code that (I promise!) was previously working no longer reach the REPL (hanging without any debug output).

Rebasing on some changes I just pushed might help as I found and fixed a bug with the wrong entry point address in the uImage. Different builds could have the linker put the Entry at different places. See pybricks/support#2256 (comment)

dlech

I didn't do an in-depth review since I know this is still a work in progress, but I like what I see so far. 😄

lib/pbio/drv/adc/adc_ev3.c

dlech · 2025-07-05T05:11:01Z

lib/pbio/drv/block_device/block_device_ev3.c

+    SPIModeConfigure(SOC_SPI_0_REGS, SPI_MASTER_MODE);
+    unsigned int  spipc0 = SPI_SPIPC0_SOMIFUN | SPI_SPIPC0_SIMOFUN | SPI_SPIPC0_CLKFUN | SPI_SPIPC0_SCS0FUN0 | SPI_SPIPC0_SCS0FUN3;
+    SPIPinControl(SOC_SPI_0_REGS, 0, 0, &spipc0);
+    SPIDefaultCSSet(SOC_SPI_0_REGS, (1 << 0) | (1 << 3));


Let's use macros here so we know what these bits are.

SPI_DATA_FORMATx are not really better though. I would introduce a new one that says SPI_BITS_PER_WORD(x)

I added macros for 0 and 3

Not sure what you want to do about SPI_DATA_FORMATx? There is the large comment explaining how format 0 is allocated for the SPI flash while format 1 is allocated for the ADC.

dlech · 2025-07-05T05:18:03Z

lib/pbio/drv/adc/adc_ev3.c

+    SPIConfigClkFormat(SOC_SPI_0_REGS, SPI_CLK_POL_LOW | SPI_CLK_OUTOFPHASE, SPI_DATA_FORMAT1);
+    SPIShiftMsbFirst(SOC_SPI_0_REGS, SPI_DATA_FORMAT1);
+    SPICharLengthSet(SOC_SPI_0_REGS, 16, SPI_DATA_FORMAT1);
+}


I would also set other config explicitly here even if it is the same as flash.

For example, SPIDefaultCSSet() is an interesting one. Omitting SPI_CSHOLD there is important for using the ADC with DMA so that it toggles the CS line between each word.

Which other config settings were you thinking of? SPIDefaultCSSet doesn't configure the CSHOLD bit. That bit is in fact cleared by the DMA commands to SPIDAT1

CSHOLD was the main one. I guess I copied and pasted the wrong fucntion name. But it is nice to see what the full config is all in once place without relying on the flash memory driver setting up some of this for us. (And could save us some trouble if we ever tweak the flash memory driver without realizing it also affects the ADC).

laurensvalk · 2025-07-05T11:55:17Z

With the latest commit the flash + ADC seem to both work (not at the same time but instead using the suggested hack).

I rewrote the entire ADC sampling logic to use DMA.

Sounds great! Looking forward to check it out after the weekend and help you get it merged.

ArcaneNibble · 2025-07-07T15:21:20Z

I've cleaned up the history of this PR and marking it as ready for review.

We still need to add some logic to selectively enable the block device driver and the ADC driver, which both use SPI0 on EV3.

Can be used as a starting point for new contributors for the EV3 driver.

…ash driver. This is derived from the W25Qxx_STM32 driver, but with most of the logic removed to make room for the EV3 implementation.

…tion.

…performing flash commands. This is all of the logic needed to program the EDMA3 controller and the SPI peripheral, but the logic to read/write flash is not implemented yet.

…riting. This completes the storage implementation for EV3.

The ADC driver will be rewritten to use DMA, but the code for setting it up is placed in the SPI flash driver in order to centralize the logic in one place.

This stub driver uses pbio rather than Contiki, and it is ready to be integrated with the SPI flash driver (including waiting for the SPI bus to be available), but the actual logic for talking to the ADC chip has not been implemented yet.

This ensures that the specified frequency is never exceeced.

laurensvalk · 2025-07-08T12:17:06Z

Thanks! Rebased and applied ./tools/codeformat.py to each commit along the way. I've tested storage with hub.system.storage. The analog touch sensor is detected and works.

I've been working on pybricks/support#2264, but I propose to merge yours first for simplicity.

Since EV3 is under heavy development, I think we can merge it as-is and address open TODOs in follow up pull requests.

coveralls · 2025-07-08T12:24:19Z

coverage: 57.091%. remained the same
when pulling 6f88e2d on ArcaneNibble:spiflash
into dd0dfbc on pybricks:master.

github-actions · 2025-07-08T12:27:23Z

Download the artifacts for this pull request:

dlech · 2025-07-09T00:23:24Z

lib/pbio/drv/adc/adc_ev3.c

+    (1 << 12) |                                                                     \
+    (1 << 11) |                                                                     \
+    (((x) & 0xf) << 7) |                                                            \
+    (1 << 6)


Would be nice to have some macros or comments to explain these bits.

dlech · 2025-07-09T00:26:28Z

lib/pbio/drv/adc/adc_ev3.c

+    // XXX We probably need to figure out how atomicity works between the DMA and the CPU.
+    // For now, read the value twice and assume it's good (not torn) if the values are the same.


Instead of worrying about tearing, I would do a double buffer. Just before we call pbdrv_adc_callbacks in the protothread where we drive the sampling, copy the DMA buffer to a 2nd buffer. We know we are not in the middle of a DMA operation at this point. Then have this function use the values from the 2nd buffer so that we know the data is good.

dlech · 2025-07-09T00:33:09Z

lib/pbio/drv/adc/adc_ev3.c

+    SPIConfigClkFormat(SOC_SPI_0_REGS, SPI_CLK_POL_LOW | SPI_CLK_OUTOFPHASE, SPI_DATA_FORMAT1);
+    SPIShiftMsbFirst(SOC_SPI_0_REGS, SPI_DATA_FORMAT1);
+    SPICharLengthSet(SOC_SPI_0_REGS, 16, SPI_DATA_FORMAT1);
+}


CSHOLD was the main one. I guess I copied and pasted the wrong fucntion name. But it is nice to see what the full config is all in once place without relying on the flash memory driver setting up some of this for us. (And could save us some trouble if we ever tweak the flash memory driver without realizing it also affects the ADC).

dlech · 2025-07-09T00:39:52Z

lib/pbio/drv/block_device/block_device_ev3.c

+static inline void spi0_setup_for_flash() {
+    HWREG(SOC_SPI_0_REGS + SPI_SPIDELAY) = 0;
+
+    *(volatile uint16_t *)(SOC_SPI_0_REGS + SPI_SPIDAT1 + 2) =


Why not use SPIDat1Config()?

dlech · 2025-07-09T00:40:51Z

lib/pbio/drv/block_device/block_device_ev3.c

+
+// Helper functions for setting up the high control bits of a data transfer
+static inline void spi0_setup_for_flash() {
+    HWREG(SOC_SPI_0_REGS + SPI_SPIDELAY) = 0;


Why not use SPIDelayConfigure()?

dlech · 2025-07-09T00:47:59Z

lib/pbio/drv/block_device/block_device_ev3.c

+
+    // SPI module data formats
+    SPIClkConfigure(SOC_SPI_0_REGS, SOC_SPI_0_MODULE_FREQ, SPI_CLK_SPEED_FLASH, SPI_DATA_FORMAT0);
+    // For reasons which have not yet been fully investigated, attempting to switch between


In the TRM on the DEFSEL field description of SPIDAT1, it says:

Note: Preselecting a Format Register. Writing to just the control field (using byte writes) does
not initiate any SPI transfer in master mode. This feature can be used to set up SPIx_CLK phase
or polarity before actually starting the transfer by just updating the DFSEL fields in the control field
to select the required phase/polarity combination.

so maybe not a glitch, but intentional?

So it sounds like we should swap the order of the config here.

dlech · 2025-07-09T00:54:31Z

lib/pbio/drv/block_device/block_device_ev3.c

+    SPIOutOfReset(SOC_SPI_0_REGS);
+    SPIModeConfigure(SOC_SPI_0_REGS, SPI_MASTER_MODE);
+    unsigned int spipc0 = SPI_SPIPC0_SOMIFUN | SPI_SPIPC0_SIMOFUN | SPI_SPIPC0_CLKFUN | SPI_SPIPC0_SCS0FUN0 | SPI_SPIPC0_SCS0FUN3;
+    SPIPinControl(SOC_SPI_0_REGS, 0, 0, &spipc0);


Would be nice to split SPIPinControl() into two function so we don't have this goofy pass by reference when writing.

dlech · 2025-07-09T00:57:50Z

lib/pbio/drv/block_device/block_device_ev3.c

+    SPIReset(SOC_SPI_0_REGS);
+    SPIOutOfReset(SOC_SPI_0_REGS);
+    SPIModeConfigure(SOC_SPI_0_REGS, SPI_MASTER_MODE);
+    unsigned int spipc0 = SPI_SPIPC0_SOMIFUN | SPI_SPIPC0_SIMOFUN | SPI_SPIPC0_CLKFUN | SPI_SPIPC0_SCS0FUN0 | SPI_SPIPC0_SCS0FUN3;


This is another place where I would rather just have the ADC reconfigure things here instead. Or at least add some more comments on what parts of this are for the ADC. I.e. one chip select is for this driver and one is for the ADC driver.

laurensvalk self-requested a review July 4, 2025 22:23

dlech reviewed Jul 5, 2025

View reviewed changes

ArcaneNibble force-pushed the spiflash branch from 36887f4 to 34c88ae Compare July 7, 2025 15:13

ArcaneNibble marked this pull request as ready for review July 7, 2025 15:20

ArcaneNibble changed the title ~~[WIP] [EV3] Implement a SPI flash driver for the EV3~~ [EV3] Implement a SPI flash driver for the EV3 Jul 7, 2025

laurensvalk and others added 14 commits July 8, 2025 13:31

pbio/platform/ev3: Disable ADC driver temporarily.

db4567a

We still need to add some logic to selectively enable the block device driver and the ADC driver, which both use SPI0 on EV3.

pbio/drv/block_device/block_device_ev3: Model after W25Qxx_STM32 driver.

07dbbd7

Can be used as a starting point for new contributors for the EV3 driver.

pbio/drv/block_device/block_device_ev3: Implement skeleton for EV3 fl…

9b18eb4

…ash driver. This is derived from the W25Qxx_STM32 driver, but with most of the logic removed to make room for the EV3 implementation.

pbio/drv/block_device/block_device_ev3: Implement hardware initializa…

5cd1e7e

…tion.

pbio/drv/block_device/block_device_ev3: Hook up interrupt handlers.

ee30608

pbio/drv/block_device/block_device_ev3: Implement raw primitives for …

85072c6

…performing flash commands. This is all of the logic needed to program the EDMA3 controller and the SPI peripheral, but the logic to read/write flash is not implemented yet.

pbio/drv/block_device/block_device_ev3: Implement flash reading and w…

070953d

…riting. This completes the storage implementation for EV3.

pbio/drv/block_device/block_device_ev3: Implement functions for ADC use.

76723c0

The ADC driver will be rewritten to use DMA, but the code for setting it up is placed in the SPI flash driver in order to centralize the logic in one place.

tiam1808/drivers/spi: Round *up* divisor for clock prescaler.

0fb8389

This ensures that the specified frequency is never exceeced.

pbio/drv/adc/adc_ev3: Initialize SPI data format.

4d2be45

pbio/drv/adc/adc_ev3: Implement new DMA-based ADC sampling.

bbfa7f5

pbio/drv/adc/adc_ev3: Extend delays for more accurate analog readings.

214ad51

pbio/drv/adc/adc_ev3: Use proper naming convention for local define.

6f88e2d

laurensvalk force-pushed the spiflash branch from 05f721c to 6f88e2d Compare July 8, 2025 12:10

laurensvalk merged commit 6f88e2d into pybricks:master Jul 8, 2025
17 checks passed

dlech reviewed Jul 9, 2025

View reviewed changes

		// XXX We probably need to figure out how atomicity works between the DMA and the CPU.
		// For now, read the value twice and assume it's good (not torn) if the values are the same.

Uh oh!

[EV3] Implement a SPI flash driver for the EV3 #314

[EV3] Implement a SPI flash driver for the EV3 #314

Uh oh!

Conversation

ArcaneNibble commented Jul 4, 2025

Uh oh!

ArcaneNibble commented Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArcaneNibble commented Jul 5, 2025

Uh oh!

dlech commented Jul 5, 2025

Uh oh!

dlech left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurensvalk commented Jul 5, 2025

Uh oh!

ArcaneNibble commented Jul 7, 2025

Uh oh!

laurensvalk commented Jul 8, 2025

Uh oh!

coveralls commented Jul 8, 2025

Uh oh!

github-actions bot commented Jul 8, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ArcaneNibble commented Jul 5, 2025 •

edited

Loading