gh-raspberrypi/pico-sdk

Watch

Fork 0

`spi_write_*()` should disable interrupts when used with hardware chip select and CPHA=1 #3141

New issue

Open

opened 2026-08-16 07:16:13 -05:00 by kukrimate · 9 comments

kukrimate commented

2026-08-16 07:16:13 -05:00

(Migrated from github.com)

For context I am using SPI mode 3 with hardware chip select.

According to the datasheet, in this mode (and generally CPHA=1) the hardware chip select is toggled around the entire transfer as oppossed to per byte.
The chip select gets asserted (goes low) when the TX FIFO goes from empty to non-empty, and it gets deasserted (goes high) when the TX FIFO empties.

But I had the odd data corruption every ~1000 5 byte transfers or so. I've narrowed it down some 5 byte transfers getting broken up randomly mid way through with a logic analyzer.

After some more code debugging it turns out my USB ISR was firing mid-way the SPI transfer causing the TX FIFO to run empty and the chip select to deassert... The following code sequence fixed my issue:

uint32_t s = save_and_disable_interrupts();
spi_write_read_blocking(spi0, txbuf, rxbuf, sizeof(txbuf));
restore_interrupts(s);

I am not sure doing this internally in the SDK is the right approach, but the user should at the very least be warned about this possibly happening.

For context I am using SPI mode 3 with hardware chip select. According to the datasheet, in this mode (and generally CPHA=1) the hardware chip select is toggled around the entire transfer as oppossed to per byte. The chip select gets asserted (goes low) when the TX FIFO goes from empty to non-empty, and it gets deasserted (goes high) when the TX FIFO empties. But I had the odd data corruption every ~1000 5 byte transfers or so. I've narrowed it down some 5 byte transfers getting broken up randomly mid way through with a logic analyzer. After some more code debugging it turns out my USB ISR was firing mid-way the SPI transfer causing the TX FIFO to run empty and the chip select to deassert... The following code sequence fixed my issue: ```c uint32_t s = save_and_disable_interrupts(); spi_write_read_blocking(spi0, txbuf, rxbuf, sizeof(txbuf)); restore_interrupts(s); ``` I am not sure doing this internally in the SDK is the right approach, but the user should at the very least be warned about this possibly happening.

lurch commented

2026-08-16 09:29:13 -05:00

(Migrated from github.com)

I am not sure doing this internally in the SDK is the right approach

Yeah, I guess the SDK has no way of knowing if the SPI transfer is more important, or responding to the USB interrupt is more important 😉

Although the comment at https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/hardware_spi/spi.c#L88 suggests that this code is (lightly) interruptible, so does this mean that your USB ISR is taking more time to run than it takes for the SPI FIFO to drain? Is there any way you could speed up your USB ISR? Or maybe you could have the SPI-code running on one core and the USB-code running on the other core?

> I am not sure doing this internally in the SDK is the right approach Yeah, I guess the SDK has no way of knowing if the SPI transfer is more important, or responding to the USB interrupt is more important 😉 Although the comment at https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/hardware_spi/spi.c#L88 suggests that this code _is_ (lightly) interruptible, so does this mean that your USB ISR is taking more time to run than it takes for the SPI FIFO to drain? Is there any way you could speed up your USB ISR? Or maybe you could have the SPI-code running on one core and the USB-code running on the other core?

kukrimate commented

2026-08-16 09:38:22 -05:00

(Migrated from github.com)

Yeah, I guess the SDK has no way of knowing if the SPI transfer is more important, or responding to the USB interrupt is more important 😉

It is not just about importance, I would actually be fine with the SPI transfer as a whole being delayed.

The problem is the SPI transfer will simply have incorrect framing in this scenario, and devices using the CPHA=1 chip select scheme, they certainly expect the entire transfer to be part of one CS pulse.

Although the comment at https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/hardware_spi/spi.c#L88 suggests that this code is (lightly) interruptible, so does this mean that your USB ISR is taking more time to run than it takes for the SPI FIFO to drain? Is there any way you could speed up your USB ISR? Or maybe you could have the SPI-code running on one core and the USB-code running on the other core?

I am using TinyUSB here which I don't believe has a particularly heavy weight or uncommon USB implementation :)

I am also running fairly high speed SPI, the SPI hw transferring one byte is measured in single-to-double digit CPU cycles here, so anything but USB ISR written as few lines of hand-written ASM interrupting the SPI transfer will result in the same issue.

> Yeah, I guess the SDK has no way of knowing if the SPI transfer is more important, or responding to the USB interrupt is more important 😉 It is not just about importance, I would actually be fine with the SPI transfer as a _whole_ being delayed. The problem is the SPI transfer will simply have incorrect framing in this scenario, and devices using the CPHA=1 chip select scheme, they certainly expect the entire transfer to be part of one CS pulse. > Although the comment at https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/hardware_spi/spi.c#L88 suggests that this code is (lightly) interruptible, so does this mean that your USB ISR is taking more time to run than it takes for the SPI FIFO to drain? Is there any way you could speed up your USB ISR? Or maybe you could have the SPI-code running on one core and the USB-code running on the other core? I am using TinyUSB here which I don't believe has a particularly heavy weight or uncommon USB implementation :) I am also running fairly high speed SPI, the SPI hw transferring one byte is measured in single-to-double digit CPU cycles here, so anything but USB ISR written as few lines of hand-written ASM interrupting the SPI transfer will result in the same issue.

lurch commented

2026-08-16 14:39:34 -05:00

(Migrated from github.com)

If you do disable interrupts around SPI activity, I guess you'd want them disabled for as short a time as possible (to reduce the chance of missing your USB interrupts) ?
So given that the SPI hardware has an 8-deep FIFO, and you're only transmitting 5 bytes at a time, I wonder if you could create a custom version of spi_write_read_blocking which disables interrupts, writes all 5 bytes to spi_get_hw(spi)->dr (which loads them into the transmit FIFO), then re-enables interrupts, and then reads all 5 bytes out of the receive FIFO (as they become available) by reading from spi_get_hw(spi)->dr? Because presumably once all the bytes are in the SPI hardware's FIFOs, it doesn't matter if the CPU gets interrupted or not? 🤔 (This is a bit of a stab in the dark, from having had a quick browse through https://documentation-service.arm.com/static/5e8e3bc7fd977155116a9361 and https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/hardware_spi/spi.c )

If you **do** disable interrupts around SPI activity, I guess you'd want them disabled for as short a time as possible (to reduce the chance of missing your USB interrupts) ? So given that the SPI hardware has an 8-deep FIFO, and you're only transmitting 5 bytes at a time, I wonder if you could create a custom version of `spi_write_read_blocking` which disables interrupts, writes all 5 bytes to `spi_get_hw(spi)->dr` (which loads them into the transmit FIFO), then re-enables interrupts, and then reads all 5 bytes out of the receive FIFO (as they become available) by reading from `spi_get_hw(spi)->dr`? Because presumably once all the bytes are in the SPI hardware's FIFOs, it doesn't matter if the CPU gets interrupted or not? 🤔 (This is a bit of a stab in the dark, from having had a quick browse through https://documentation-service.arm.com/static/5e8e3bc7fd977155116a9361 and https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/hardware_spi/spi.c )

kukrimate commented

2026-08-16 16:08:00 -05:00

(Migrated from github.com)

I might have gone slightly overbroad by also putting the receive part inside the interrupt disable block but the function below does seem to resolve my problem of the transfer being broken up:

static void __not_in_flash_func(write_reg)(uint8_t reg, uint32_t val)
{
	uint32_t s = save_and_disable_interrupts();

	spi0_hw->dr = lsb2msb[(reg << 2) | 2];
	spi0_hw->dr = lsb2msb[val & 0xFF];
	spi0_hw->dr = lsb2msb[(val >> 8) & 0xFF];
	spi0_hw->dr = lsb2msb[(val >> 16) & 0xFF];
	spi0_hw->dr = lsb2msb[(val >> 24) & 0xFF];

	while (!(spi0_hw->sr & SPI_SSPSR_RNE_BITS));
	(void)spi0_hw->dr;
	while (!(spi0_hw->sr & SPI_SSPSR_RNE_BITS));
	(void)spi0_hw->dr;
	while (!(spi0_hw->sr & SPI_SSPSR_RNE_BITS));
	(void)spi0_hw->dr;
	while (!(spi0_hw->sr & SPI_SSPSR_RNE_BITS));
	(void)spi0_hw->dr;
	while (!(spi0_hw->sr & SPI_SSPSR_RNE_BITS));
	(void)spi0_hw->dr;

	restore_interrupts(s);
}

I might have gone slightly overbroad by also putting the receive part inside the interrupt disable block but the function below does seem to resolve my problem of the transfer being broken up: ```c static void __not_in_flash_func(write_reg)(uint8_t reg, uint32_t val) { uint32_t s = save_and_disable_interrupts(); spi0_hw->dr = lsb2msb[(reg << 2) | 2]; spi0_hw->dr = lsb2msb[val & 0xFF]; spi0_hw->dr = lsb2msb[(val >> 8) & 0xFF]; spi0_hw->dr = lsb2msb[(val >> 16) & 0xFF]; spi0_hw->dr = lsb2msb[(val >> 24) & 0xFF]; while (!(spi0_hw->sr & SPI_SSPSR_RNE_BITS)); (void)spi0_hw->dr; while (!(spi0_hw->sr & SPI_SSPSR_RNE_BITS)); (void)spi0_hw->dr; while (!(spi0_hw->sr & SPI_SSPSR_RNE_BITS)); (void)spi0_hw->dr; while (!(spi0_hw->sr & SPI_SSPSR_RNE_BITS)); (void)spi0_hw->dr; while (!(spi0_hw->sr & SPI_SSPSR_RNE_BITS)); (void)spi0_hw->dr; restore_interrupts(s); } ```

👍 1

lurch commented

2026-08-17 04:08:00 -05:00

(Migrated from github.com)

Oh! If you're not actually using the returned value, why was your original code using spi_write_read_blocking rather than spi_write_blocking ? 🤔 (But of course spi_write_blocking would still have the same problem, if interrupted while writing to spi_get_hw(spi)->dr)

I might have gone slightly overbroad by also putting the receive part inside the interrupt disable block

I guess you could also move the bit-shifting and masking to be before the interrupt disable block, if you really need to reduce the amount of time that interrupts are disabled for 😉

Oh! If you're not actually using the returned value, why was your original code using `spi_write_read_blocking` rather than `spi_write_blocking` ? 🤔 (But of course `spi_write_blocking` would still have the same problem, if interrupted while writing to `spi_get_hw(spi)->dr`) > I might have gone slightly overbroad by also putting the receive part inside the interrupt disable block I guess you could also move the bit-shifting and masking to be _before_ the interrupt disable block, if you _really_ need to reduce the amount of time that interrupts are disabled for 😉

kukrimate commented

2026-08-17 04:16:23 -05:00

(Migrated from github.com)

@lurch Oh sorry I have another operation that is 6 bytes called read_register that does use the returned value, it is almost identical besides that.

👍 1

Wren6991 commented

2026-08-19 04:18:53 -05:00

(Migrated from github.com)

I am not sure doing this internally in the SDK is the right approach

I think this would be a bit of a footgun for other users; disabling interrupts for (potentially) thousands of cycles would be fatal for anything that needs tightly bounded IRQ latency.

You can avoid the CSn deassertion by using a software-controlled chip select. That's one of the reasons software CSn is commonly used in practice, even though it's not pretty. You could also consider using DMA, so that processor IRQs don't interrupt the data flow.

Out of interest what was the reason for using the hardware chip select?

> I am not sure doing this internally in the SDK is the right approach I think this would be a bit of a footgun for other users; disabling interrupts for (potentially) thousands of cycles would be fatal for anything that needs tightly bounded IRQ latency. You can avoid the CSn deassertion by using a software-controlled chip select. That's one of the reasons software CSn is commonly used in practice, even though it's not pretty. You could also consider using DMA, so that processor IRQs don't interrupt the data flow. Out of interest what was the reason for using the hardware chip select?

Wren6991 commented

2026-08-19 04:20:55 -05:00

(Migrated from github.com)

We don't provide DMA accessors for SPI because it's hard to write one that covers all use cases without re-implementing most of the DMA API, but it doesn't take much code. For an example see here in the MicroPython source: github.com/micropython/micropython@1c3c201149/ports/rp2/machine_spi.c (L280-L320)

We don't provide DMA accessors for SPI because it's hard to write one that covers all use cases without re-implementing most of the DMA API, but it doesn't take much code. For an example see here in the MicroPython source: https://github.com/micropython/micropython/blob/1c3c201149f37fe8d81246191b3127bb198d6306/ports/rp2/machine_spi.c#L280-L320

kukrimate commented

2026-08-19 16:24:41 -05:00

(Migrated from github.com)

@Wren6991

I think this would be a bit of a footgun for other users; disabling interrupts for (potentially) thousands of cycles would be fatal for anything that needs tightly bounded IRQ latency.

Yeah I am not 100% sure what solution (if any at all) could be good here. With software chip select (or I guess even the CPHA=0 behavior) gaps between bytes in the transaction do not really matter, obviously disabling interrupts unless necessary is heavy handed.
Why I reported this is because the SPI transfer API feels broken when combined with hardware chip select and the CPHA=1 behaviour, or at least it is a bit of a footgun.

You can avoid the CSn deassertion by using a software-controlled chip select. That's one of the reasons software CSn is commonly used in practice, even though it's not pretty. You could also consider using DMA, so that processor IRQs don't interrupt the data flow.

Out of interest what was the reason for using the hardware chip select?

I did use software chip select originally, and it does work for my application. But to get maximum performance I switched to hardware chip select.
Basically what's going on here is few non-critical read_reg() and write_reg() operations happen first to program the flash controller for page read or page write, and then a sequence of identical read_reg() operations yield 512 bytes of data, unfortunately 4 bytes per each CS assertion.
To get maximum performance out of this at a given frequence, I found that hardware chip select both reduces the front and back porch of each transfer, and also allows me to fire off read_reg() operations with as small delay between them as possible.

We don't provide DMA accessors for SPI because it's hard to write one that covers all use cases without re-implementing most of the DMA API, but it doesn't take much code. For an example see here in the MicroPython source: github.com/micropython/micropython@1c3c201149/ports/rp2/machine_spi.c (L280-L320)

I can use DMA for the bulk part of above I suppose, but I still need the ability to do individual non-DMA read_reg() and write_reg() operations to not have to do the whole DMA setup for each single setup operation. Disabling the interrupts manually for those works for me for those I was just pondering about the API behaviour.

@Wren6991 > I think this would be a bit of a footgun for other users; disabling interrupts for (potentially) thousands of cycles would be fatal for anything that needs tightly bounded IRQ latency. Yeah I am not 100% sure what solution (if any at all) could be good here. With software chip select (or I guess even the CPHA=0 behavior) gaps between bytes in the transaction do not really matter, obviously disabling interrupts unless necessary is heavy handed. Why I reported this is because the SPI transfer API feels broken when combined with hardware chip select and the CPHA=1 behaviour, or at least it is a bit of a footgun. > You can avoid the CSn deassertion by using a software-controlled chip select. That's one of the reasons software CSn is commonly used in practice, even though it's not pretty. You could also consider using DMA, so that processor IRQs don't interrupt the data flow. > > Out of interest what was the reason for using the hardware chip select? I did use software chip select originally, and it does work for my application. But to get maximum performance I switched to hardware chip select. Basically what's going on here is few non-critical `read_reg()` and `write_reg()` operations happen first to program the flash controller for page read or page write, and then a sequence of identical `read_reg()` operations yield 512 bytes of data, unfortunately 4 bytes per each CS assertion. To get maximum performance out of this at a given frequence, I found that hardware chip select both reduces the front and back porch of each transfer, and also allows me to fire off `read_reg()` operations with as small delay between them as possible. > We don't provide DMA accessors for SPI because it's hard to write one that covers all use cases without re-implementing most of the DMA API, but it doesn't take much code. For an example see here in the MicroPython source: https://github.com/micropython/micropython/blob/1c3c201149f37fe8d81246191b3127bb198d6306/ports/rp2/machine_spi.c#L280-L320 I can use DMA for the bulk part of above I suppose, but I still need the ability to do individual non-DMA `read_reg()` and `write_reg()` operations to not have to do the whole DMA setup for each single setup operation. Disabling the interrupts manually for those works for me for those I was just pondering about the API behaviour.