Skip to content
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.

Sign up for GitHub

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

Jump to bottom

drivers: spi: RPi Pico PIO SPI code size and byte order. #74117

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+116 −128
Open

drivers: spi: RPi Pico PIO SPI code size and byte order. #74117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
176 changes: 48 additions & 128 deletions drivers/spi/spi_rpi_pico_pio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

#include <zephyr/sys/util.h>
#include <zephyr/sys/sys_io.h>
#include <zephyr/sys/byteorder.h>
#include <zephyr/drivers/clock_control.h>
#include <zephyr/drivers/spi.h>
#include <zephyr/drivers/pinctrl.h>
Expand Down Expand Up @@ -90,80 +91,33 @@
/* ------------------- */

#define SPI_SIO_MODE_0_0_TX_WRAP_TARGET 0
#define SPI_SIO_MODE_0_0_TX_WRAP 2
#define SPI_SIO_MODE_0_0_TX_WRAP 1
#define SPI_SIO_MODE_0_0_TX_CYCLES 2

RPI_PICO_PIO_DEFINE_PROGRAM(spi_sio_mode_0_0_tx, SPI_SIO_MODE_0_0_TX_WRAP_TARGET,
SPI_SIO_MODE_0_0_TX_WRAP,
/* .wrap_target */
0x80a0, /* 0: pull block side 0 */
0x6001, /* 1: out pins, 1 side 0 */
0x10e1, /* 2: jmp !osre, 1 side 1 */
0x6001, /* 0: out pins, 1 side 0 */
0xb042, /* 1: nop side 1 */
/* .wrap */
);

Check notice on line 103 in drivers/spi/spi_rpi_pico_pio.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

drivers/spi/spi_rpi_pico_pio.c:103 
-		/*     .wrap_target */
-	0x6001,	/*  0: out    pins, 1         side 0  */
-	0xb042,	/*  1: nop                    side 1  */
-		/*     .wrap */
+			    /*     .wrap_target */
+			    0x6001, /*  0: out    pins, 1         side 0  */
+			    0xb042, /*  1: nop                    side 1  */
+				    /*     .wrap */

/* ------------------------- */
/* spi_sio_mode_0_0_8_bit_rx */
/* spi_sio_mode_0_0_rx */
/* ------------------------- */

#define SPI_SIO_MODE_0_0_8_BIT_RX_WRAP_TARGET 0
#define SPI_SIO_MODE_0_0_8_BIT_RX_WRAP 6
#define SPI_SIO_MODE_0_0_8_BIT_RX_CYCLES 2
#define SPI_SIO_MODE_0_0_RX_WRAP_TARGET 0
#define SPI_SIO_MODE_0_0_RX_WRAP 2
#define SPI_SIO_MODE_0_0_RX_CYCLES 2

RPI_PICO_PIO_DEFINE_PROGRAM(spi_sio_mode_0_0_8_bit_rx, SPI_SIO_MODE_0_0_8_BIT_RX_WRAP_TARGET,
SPI_SIO_MODE_0_0_8_BIT_RX_WRAP,
RPI_PICO_PIO_DEFINE_PROGRAM(spi_sio_mode_0_0_rx, SPI_SIO_MODE_0_0_RX_WRAP_TARGET,
SPI_SIO_MODE_0_0_RX_WRAP,
/* .wrap_target */
0x80a0, /* 0: pull block side 0 */
0x6020, /* 1: out x, 32 side 0 */
0xe047, /* 2: set y, 7 side 0 */
0x5001, /* 3: in pins, 1 side 1 */
0x0083, /* 4: jmp y--, 3 side 0 */
0x8020, /* 5: push block side 0 */
0x0042, /* 6: jmp x--, 2 side 0 */
/* .wrap */
);

/* -------------------------- */
/* spi_sio_mode_0_0_16_bit_rx */
/* -------------------------- */

#define SPI_SIO_MODE_0_0_16_BIT_RX_WRAP_TARGET 0
#define SPI_SIO_MODE_0_0_16_BIT_RX_WRAP 6
#define SPI_SIO_MODE_0_0_16_BIT_RX_CYCLES 2

RPI_PICO_PIO_DEFINE_PROGRAM(spi_sio_mode_0_0_16_bit_rx, SPI_SIO_MODE_0_0_16_BIT_RX_WRAP_TARGET,
SPI_SIO_MODE_0_0_16_BIT_RX_WRAP,
/* .wrap_target */
0x80a0, /* 0: pull block side 0 */
0x6020, /* 1: out x, 32 side 0 */
0xe04f, /* 2: set y, 15 side 0 */
0x5001, /* 3: in pins, 1 side 1 */
0x0083, /* 4: jmp y--, 3 side 0 */
0x8020, /* 5: push block side 0 */
0x0042, /* 6: jmp x--, 2 side 0 */
/* .wrap */
);

/* -------------------------- */
/* spi_sio_mode_0_0_32_bit_rx */
/* -------------------------- */

#define SPI_SIO_MODE_0_0_32_BIT_RX_WRAP_TARGET 0
#define SPI_SIO_MODE_0_0_32_BIT_RX_WRAP 6
#define SPI_SIO_MODE_0_0_32_BIT_RX_CYCLES 2

RPI_PICO_PIO_DEFINE_PROGRAM(spi_sio_mode_0_0_32_bit_rx, SPI_SIO_MODE_0_0_32_BIT_RX_WRAP_TARGET,
SPI_SIO_MODE_0_0_32_BIT_RX_WRAP,
/* .wrap_target */
0x80a0, /* 0: pull block side 0 */
0x6020, /* 1: out x, 32 side 0 */
0xe05f, /* 2: set y, 31 side 0 */
0x5001, /* 3: in pins, 1 side 1 */
0x0083, /* 4: jmp y--, 3 side 0 */
0x8020, /* 5: push block side 0 */
0x0042, /* 6: jmp x--, 2 side 0 */
0x6020, /* 0: out x, 32 side 0 */
0x5001, /* 1: in pins, 1 side 1 */
0x0041, /* 2: jmp x--, 1 side 0 */
/* .wrap */
);

Check notice on line 120 in drivers/spi/spi_rpi_pico_pio.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

drivers/spi/spi_rpi_pico_pio.c:120 
-		/*     .wrap_target */
-	0x6020, /*  0: out    x, 32           side 0 */
-	0x5001, /*  1: in     pins, 1         side 1 */
-	0x0041, /*  2: jmp    x--, 1          side 0 */
-		/*     .wrap */
+			    /*     .wrap_target */
+			    0x6020, /*  0: out    x, 32           side 0 */
+			    0x5001, /*  1: in     pins, 1         side 1 */
+			    0x0041, /*  2: jmp    x--, 1          side 0 */
+				    /*     .wrap */
#endif /* SPI_RPI_PICO_PIO_HALF_DUPLEX_ENABLED */

static float spi_pico_pio_clock_divisor(const uint32_t clock_freq, int cycles,
Expand Down Expand Up @@ -237,8 +191,9 @@

static inline int spi_pico_pio_sm_complete(struct spi_pico_pio_data *data)
{
return (data->pio->sm[data->pio_sm].addr == data->pio_tx_offset);
return ((data->pio->sm[data->pio_sm].addr == data->pio_tx_offset)
&& pio_sm_is_tx_fifo_empty(data->pio, data->pio_sm));
}

Check notice on line 196 in drivers/spi/spi_rpi_pico_pio.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

drivers/spi/spi_rpi_pico_pio.c:196 
-	return ((data->pio->sm[data->pio_sm].addr == data->pio_tx_offset)
-		&& pio_sm_is_tx_fifo_empty(data->pio, data->pio_sm));
+	return ((data->pio->sm[data->pio_sm].addr == data->pio_tx_offset) &&
+		pio_sm_is_tx_fifo_empty(data->pio, data->pio_sm));

static int spi_pico_pio_configure(const struct spi_pico_pio_config *dev_cfg,
struct spi_pico_pio_data *data, const struct spi_config *spi_cfg)
Expand Down Expand Up @@ -349,55 +304,27 @@

float clock_div = spi_pico_pio_clock_divisor(clock_freq, SPI_SIO_MODE_0_0_TX_CYCLES,
spi_cfg->frequency);
data->tx_period_ticks = DIV_ROUND_UP((data->bits * CONFIG_SYS_CLOCK_TICKS_PER_SEC),
data->tx_period_ticks = DIV_ROUND_UP(CONFIG_SYS_CLOCK_TICKS_PER_SEC,
spi_cfg->frequency);

data->pio_tx_offset =
pio_add_program(data->pio, RPI_PICO_PIO_GET_PROGRAM(spi_sio_mode_0_0_tx));

switch (data->dfs) {
case 4:
data->pio_rx_offset = pio_add_program(
data->pio, RPI_PICO_PIO_GET_PROGRAM(spi_sio_mode_0_0_32_bit_rx));
data->pio_rx_wrap_target =
data->pio_rx_offset +
RPI_PICO_PIO_GET_WRAP_TARGET(spi_sio_mode_0_0_32_bit_rx);
data->pio_rx_wrap = data->pio_rx_offset +
RPI_PICO_PIO_GET_WRAP(spi_sio_mode_0_0_32_bit_rx);
break;

case 2:
data->pio_rx_offset = pio_add_program(
data->pio, RPI_PICO_PIO_GET_PROGRAM(spi_sio_mode_0_0_16_bit_rx));
data->pio_rx_wrap_target =
data->pio_rx_offset +
RPI_PICO_PIO_GET_WRAP_TARGET(spi_sio_mode_0_0_16_bit_rx);
data->pio_rx_wrap = data->pio_rx_offset +
RPI_PICO_PIO_GET_WRAP(spi_sio_mode_0_0_16_bit_rx);
break;

case 1:
data->pio_rx_offset = pio_add_program(
data->pio, RPI_PICO_PIO_GET_PROGRAM(spi_sio_mode_0_0_8_bit_rx));
data->pio_rx_wrap_target =
data->pio_rx_offset +
RPI_PICO_PIO_GET_WRAP_TARGET(spi_sio_mode_0_0_8_bit_rx);
data->pio_rx_wrap = data->pio_rx_offset +
RPI_PICO_PIO_GET_WRAP(spi_sio_mode_0_0_8_bit_rx);
break;

default:
LOG_ERR("Support for %d transfer size not enabled", (data->dfs * 8));
return -EINVAL;
}
data->pio_rx_offset = pio_add_program(
data->pio, RPI_PICO_PIO_GET_PROGRAM(spi_sio_mode_0_0_rx));
data->pio_rx_wrap_target =
data->pio_rx_offset +
RPI_PICO_PIO_GET_WRAP_TARGET(spi_sio_mode_0_0_rx);
data->pio_rx_wrap = data->pio_rx_offset +
RPI_PICO_PIO_GET_WRAP(spi_sio_mode_0_0_rx);

Check notice on line 320 in drivers/spi/spi_rpi_pico_pio.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

drivers/spi/spi_rpi_pico_pio.c:320 
-		data->tx_period_ticks = DIV_ROUND_UP(CONFIG_SYS_CLOCK_TICKS_PER_SEC,
-						     spi_cfg->frequency);
+		data->tx_period_ticks =
+			DIV_ROUND_UP(CONFIG_SYS_CLOCK_TICKS_PER_SEC, spi_cfg->frequency);
 
 		data->pio_tx_offset =
 			pio_add_program(data->pio, RPI_PICO_PIO_GET_PROGRAM(spi_sio_mode_0_0_tx));
 
-		data->pio_rx_offset = pio_add_program(
-			data->pio, RPI_PICO_PIO_GET_PROGRAM(spi_sio_mode_0_0_rx));
+		data->pio_rx_offset =
+			pio_add_program(data->pio, RPI_PICO_PIO_GET_PROGRAM(spi_sio_mode_0_0_rx));
 		data->pio_rx_wrap_target =
-			data->pio_rx_offset +
-			RPI_PICO_PIO_GET_WRAP_TARGET(spi_sio_mode_0_0_rx);
-		data->pio_rx_wrap = data->pio_rx_offset +
-					RPI_PICO_PIO_GET_WRAP(spi_sio_mode_0_0_rx);
+			data->pio_rx_offset + RPI_PICO_PIO_GET_WRAP_TARGET(spi_sio_mode_0_0_rx);
+		data->pio_rx_wrap =
+			data->pio_rx_offset + RPI_PICO_PIO_GET_WRAP(spi_sio_mode_0_0_rx);
sm_config = pio_get_default_sm_config();

sm_config_set_clkdiv(&sm_config, clock_div);
sm_config_set_in_pins(&sm_config, sio->pin);
sm_config_set_in_shift(&sm_config, lsb, false, data->bits);
sm_config_set_in_shift(&sm_config, lsb, true, data->bits);
sm_config_set_out_pins(&sm_config, sio->pin, 1);
sm_config_set_out_shift(&sm_config, lsb, false, data->bits);
sm_config_set_out_shift(&sm_config, lsb, true, data->bits);
hw_set_bits(&data->pio->input_sync_bypass, 1u << sio->pin);

sm_config_set_sideset_pins(&sm_config, clk->pin);
Expand Down Expand Up @@ -496,8 +423,8 @@
{
struct spi_pico_pio_data *data = dev->data;
const size_t chunk_len = spi_context_max_continuous_chunk(&data->spi_ctx);
const void *txbuf = data->spi_ctx.tx_buf;
void *rxbuf = data->spi_ctx.rx_buf;
const uint8_t *txbuf = data->spi_ctx.tx_buf;
uint8_t *rxbuf = data->spi_ctx.rx_buf;
uint32_t txrx;
size_t fifo_cnt = 0;

Expand All @@ -516,32 +443,30 @@
switch (data->dfs) {
case 4: {
if (txbuf) {
txrx = ((uint32_t *)txbuf)[data->tx_count];
txrx = sys_get_be32(txbuf + (data->tx_count * 4));
}
spi_pico_pio_sm_put32(data->pio, data->pio_sm, txrx);
data->tx_count += 4;
} break;

case 2: {
if (txbuf) {
txrx = ((uint16_t *)txbuf)[data->tx_count];
txrx = sys_get_be16(txbuf + (data->tx_count * 2));
}
spi_pico_pio_sm_put16(data->pio, data->pio_sm, txrx);
data->tx_count += 2;
} break;

case 1: {
if (txbuf) {
txrx = ((uint8_t *)txbuf)[data->tx_count];
}
spi_pico_pio_sm_put8(data->pio, data->pio_sm, txrx);
data->tx_count++;
} break;

default:
LOG_ERR("Support fot %d bits not enabled", (data->dfs * 8));
break;
}
data->tx_count++;
fifo_cnt++;
}

Expand All @@ -553,19 +478,17 @@

/* Discard received data if rx buffer not assigned */
if (rxbuf) {
((uint32_t *)rxbuf)[data->rx_count] = (uint32_t)txrx;
sys_put_be32(txrx, rxbuf + (data->rx_count * 4));
}
data->rx_count += 4;
} break;

case 2: {
txrx = spi_pico_pio_sm_get16(data->pio, data->pio_sm);

/* Discard received data if rx buffer not assigned */
if (rxbuf) {
((uint16_t *)rxbuf)[data->rx_count] = (uint16_t)txrx;
sys_put_be16(txrx, rxbuf + (data->rx_count * 2));
}
data->rx_count += 2;
} break;

case 1: {
Expand All @@ -575,13 +498,13 @@
if (rxbuf) {
((uint8_t *)rxbuf)[data->rx_count] = (uint8_t)txrx;
}
data->rx_count++;
} break;

default:
LOG_ERR("Support fot %d bits not enabled", (data->dfs * 8));
break;
}
data->rx_count++;
fifo_cnt--;
}
}
Expand All @@ -592,8 +515,8 @@
#if SPI_RPI_PICO_PIO_HALF_DUPLEX_ENABLED
struct spi_pico_pio_data *data = dev->data;
const struct spi_pico_pio_config *dev_cfg = dev->config;
const void *txbuf = data->spi_ctx.tx_buf;
void *rxbuf = data->spi_ctx.rx_buf;
const uint8_t *txbuf = data->spi_ctx.tx_buf;
uint8_t *rxbuf = data->spi_ctx.rx_buf;
uint32_t txrx;
int sio_pin = dev_cfg->sio_gpio.pin;
uint32_t tx_size = data->spi_ctx.tx_len; /* Number of WORDS to send */
Expand Down Expand Up @@ -622,33 +545,33 @@

switch (data->dfs) {
case 4: {
txrx = ((uint32_t *)txbuf)[data->tx_count];
txrx = sys_get_be32(txbuf + (data->tx_count * 4));
spi_pico_pio_sm_put32(data->pio, data->pio_sm, txrx);
data->tx_count += 4;
} break;

case 2: {
txrx = ((uint16_t *)txbuf)[data->tx_count];
txrx = sys_get_be16(txbuf + (data->tx_count * 2));
spi_pico_pio_sm_put16(data->pio, data->pio_sm, txrx);
data->tx_count += 2;
} break;

case 1: {
txrx = ((uint8_t *)txbuf)[data->tx_count];
spi_pico_pio_sm_put8(data->pio, data->pio_sm, txrx);
data->tx_count++;
} break;

default:
LOG_ERR("Support fot %d bits not enabled", (data->dfs * 8));
break;
}
data->tx_count++;
}
}
while ((!pio_sm_is_tx_fifo_empty(data->pio, data->pio_sm)) ||
(!spi_pico_pio_sm_complete(data))) {
/* Wait for the state machine to complete the cycle */
/* before resetting the PIO for reading. */
do {
k_sleep(K_TICKS(data->tx_period_ticks));
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at other drivers it seems that waiting on a bit without sleeping between iterations is legitimate. If any higher priority task needs to run it will get its runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, but I'm not following you. Are you suggesting using a timer instead of k_sleep()? Or perhaps I didn't explain the problem clearly: Even after the last word has been shifted from the FIFO, clocked out by the PIO program, and the PIO program counter has returned to the starting point, a delay is still needed to allow the SPI bus signals to complete before the CPU can reset the PIO state machine to start the receive cycle. I can't think of any way for the CPU to detect the state of the SPI bus, which is why I added the delay here.

I did try to minimize the needed delay, though perhaps waiting the full word size is excessive. I'll test this with just one SPI clock cycle and see if that's enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only one SPI bit time is needed; I tested it at both the fast end and the slow end of the BME280 clock range. (The issue is more of a problem at slow SPI clock rates.)

}
} while ((!pio_sm_is_tx_fifo_empty(data->pio, data->pio_sm)) ||
(!spi_pico_pio_sm_complete(data)));
}

if (rxbuf) {
Expand All @@ -659,8 +582,7 @@
pio_sm_set_pindirs_with_mask(data->pio, data->pio_sm, 0, BIT(sio_pin));
pio_sm_restart(data->pio, data->pio_sm);
pio_sm_clkdiv_restart(data->pio, data->pio_sm);
pio_sm_put(data->pio, data->pio_sm, rx_size - 1);
pio_sm_exec(data->pio, data->pio_sm, pio_encode_out(pio_x, 32));
pio_sm_put(data->pio, data->pio_sm, (rx_size * data->bits) - 1);
pio_sm_exec(data->pio, data->pio_sm, pio_encode_jmp(data->pio_rx_offset));
pio_sm_set_enabled(data->pio, data->pio_sm, true);

Expand All @@ -671,26 +593,24 @@
switch (data->dfs) {
case 4: {
txrx = spi_pico_pio_sm_get32(data->pio, data->pio_sm);
((uint32_t *)rxbuf)[data->rx_count] = (uint32_t)txrx;
data->rx_count += 4;
sys_put_be32(txrx, rxbuf + (data->rx_count * 4));
} break;

case 2: {
txrx = spi_pico_pio_sm_get16(data->pio, data->pio_sm);
((uint16_t *)rxbuf)[data->rx_count] = (uint16_t)txrx;
data->rx_count += 2;
sys_put_be16(txrx, rxbuf + (data->rx_count * 2));
} break;

case 1: {
txrx = spi_pico_pio_sm_get8(data->pio, data->pio_sm);
((uint8_t *)rxbuf)[data->rx_count] = (uint8_t)txrx;
data->rx_count++;
rxbuf[data->rx_count] = (uint8_t)txrx;
} break;

default:
LOG_ERR("Support fot %d bits not enabled", (data->dfs * 8));
break;
}
data->rx_count++;
}
}
}
Expand Down
24 changes: 24 additions & 0 deletions samples/sensor/magn_polling/README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,27 @@ Overview
Sample application that periodically reads magnetometer (X, Y, Z) data from
the first available device that implements SENSOR_CHAN_MAGN_* (predefined array
of device names).

Board-specific overlays
***********************

TMAG5170 via Raspberry Pi Pico
==============================

The Zephyr driver for the :dtcompatible:`ti,tmag5170`` requires an SPI driver
that supports 32-bit SPI_WORD_SIZE. On the :ref:`rpi_pico`, the
:dtcompatible:`raspberrypi,pico-spi-pio` SPI driver provides this support,
demonstrated with the
:zephyr_file:`samples/sensor/magn_polling/boards/rpi_pico.overlay`.

The GPIO pin assignments in the overlay file are arbitrary. The PIO SPI
driver allows using any four GPIO pins for the SPI bus. Just keep in mind
that the pin assignments in the pinctrl block and the pio0_spi0 block
must match.

With the sensor wired to the desired pins, build and flash with:

.. zephyr-app-commands::
:zephyr-app: samples/sensor/magn_polling
:goals: build flash
:board: rpi_pico
Loading
Loading