[stm32wb, spi] Initial SPI implementation

[AlexLanzano]

No description provided.

[Copilot]

Pull request overview

This PR implements the initial SPI driver for STM32WB microcontrollers, transitioning from stub functions to a functional implementation. The driver supports master mode with configurable clock polarity/phase (SPI modes 0-3), 8-bit data frames, and software-managed chip select via GPIO.

Changes:

• Implemented full SPI master mode driver with register configuration, initialization, deinitialization, and data transfer functions
• Added SPI1 peripheral configuration and GPIO pin definitions for SCK, MISO, MOSI, and CS pins in example board configuration
• Modified GPIO driver to support multiple clock descriptors, enabling configuration of pins across different GPIO ports

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
wolfHAL/spi/stm32wb_spi.h	Added SPI mode enumerations, updated configuration structure with clock controller fields, removed chipSelect from per-transaction config
wolfHAL/platform/st/stm32wb55xx.h	Added SPI1 peripheral include and clock descriptor definition
wolfHAL/gpio/stm32wb_gpio.h	Changed GPIO configuration to accept array of clock descriptors instead of single clock
src/spi/stm32wb_spi.c	Implemented SPI initialization, configuration, and transfer functions with register-level control
src/gpio/stm32wb_gpio.c	Updated to iterate over multiple clock descriptors for GPIO port initialization
examples/stm32wb/stm32wb55xx_nucleo.h	Added SPI1 pin enumerations and SPI device instance declaration
examples/stm32wb/stm32wb55xx_nucleo.c	Added GPIO configurations for SPI1 pins and SPI device instance definition

Comments suppressed due to low confidence (1)

src/spi/stm32wb_spi.c:211

• The SPI driver implementation lacks test coverage. Other STM32WB peripherals in this codebase have corresponding test files (test_gpio.c, test_clock.c, test_flash.c, test_timer.c in tests/stm32wb/). Consider adding test_spi.c to verify SPI initialization, configuration, and data transfer functionality.

#include <stdint.h>
#include <wolfHAL/spi/stm32wb_spi.h>
#include <wolfHAL/spi/spi.h>
#include <wolfHAL/clock/clock.h>
#include <wolfHAL/error.h>
#include <wolfHAL/regmap.h>
#include <wolfHAL/bitops.h>

/*
 * STM32WB SPI Register Definitions
 *
 * The SPI peripheral provides full-duplex synchronous serial communication.
 * This driver configures it in master mode with software slave management
 * and 8-bit data frames.
 */

/* Control Register 1 - master config, clock, enable */
#define SSPI_CR1_REG  0x00
#define SSPI_CR1_CPHA WHAL_MASK(0)           /* Clock phase */
#define SSPI_CR1_CPOL WHAL_MASK(1)           /* Clock polarity */
#define SSPI_CR1_MSTR WHAL_MASK(2)           /* Master selection */
#define SSPI_CR1_BR   WHAL_MASK_RANGE(5, 3)  /* Baud rate prescaler */
#define SSPI_CR1_SPE  WHAL_MASK(6)           /* SPI enable */
#define SSPI_CR1_SSI  WHAL_MASK(8)           /* Internal slave select */
#define SSPI_CR1_SSM  WHAL_MASK(9)           /* Software slave management */

/* Control Register 2 - data size, FIFO threshold */
#define SSPI_CR2_REG   0x04
#define SSPI_CR2_DS    WHAL_MASK_RANGE(11, 8) /* Data size (0111 = 8-bit) */
#define SSPI_CR2_FRXTH WHAL_MASK(12)          /* FIFO reception threshold */

/* Status Register */
#define SSPI_SR_REG  0x08
#define SSPI_SR_RXNE WHAL_MASK(0) /* Receive buffer not empty */
#define SSPI_SR_TXE  WHAL_MASK(1) /* Transmit buffer empty */
#define SSPI_SR_BSY  WHAL_MASK(7) /* Busy flag */

/* Data Register - 8/16-bit access */
#define SSPI_DR_REG  0x0C
#define SSPI_DR_MASK WHAL_MASK_RANGE(7, 0)

/* 8-bit data size value for DS field */
#define SSPI_DS_8BIT 0x7

/*
 * Calculate the baud rate prescaler index for a target baud rate.
 *
 * SPI baud rate = fPCLK / (2 ^ (BR + 1))
 *   BR=0 -> /2,  BR=1 -> /4,  BR=2 -> /8, ... BR=7 -> /256
 *
 * Returns the smallest prescaler that does not exceed the target baud.
 */
static uint32_t whal_Stm32wbSpi_CalcBr(size_t pclk, uint32_t targetBaud)
{
    uint32_t br;

    for (br = 0; br < 7; br++) {
        if ((pclk / (2u << br)) <= targetBaud) {
            return br;
        }
    }

    return 7;
}

/*
 * Configure SPI mode and baud rate, then enable the peripheral.
 * Must be called with SPE disabled.
 */
static void whal_Stm32wbSpi_ApplyComCfg(const whal_Regmap *reg,
                                        whal_Stm32wbSpi_Cfg *cfg,
                                        whal_Stm32wbSpi_ComCfg *comCfg)
{
    size_t pclk;
    uint32_t cpol, cpha, br;

    whal_Clock_GetRate(cfg->clkCtrl, &pclk);

    br = whal_Stm32wbSpi_CalcBr(pclk, comCfg->baud);

    cpol = (comCfg->mode >> 1) & 1;
    cpha = comCfg->mode & 1;

    /* Disable SPE before reconfiguring */
    whal_Reg_Update(reg->base, SSPI_CR1_REG, SSPI_CR1_SPE,
                    whal_SetBits(SSPI_CR1_SPE, 0));

    /* Set mode and baud rate */
    whal_Reg_Update(reg->base, SSPI_CR1_REG,
                    SSPI_CR1_CPOL | SSPI_CR1_CPHA | SSPI_CR1_BR,
                    whal_SetBits(SSPI_CR1_CPOL, cpol) |
                    whal_SetBits(SSPI_CR1_CPHA, cpha) |
                    whal_SetBits(SSPI_CR1_BR, br));

    /* Re-enable SPE */
    whal_Reg_Update(reg->base, SSPI_CR1_REG, SSPI_CR1_SPE,
                    whal_SetBits(SSPI_CR1_SPE, 1));
}

whal_Error whal_Stm32wbSpi_Init(whal_Spi *spiDev)
{
    whal_Error err;
    whal_Stm32wbSpi_Cfg *cfg;
    const whal_Regmap *reg = &spiDev->regmap;

    cfg = (whal_Stm32wbSpi_Cfg *)spiDev->cfg;

    err = whal_Clock_Enable(cfg->clkCtrl, cfg->clk);
    if (err != WHAL_SUCCESS) {
        return err;
    }

    /* Master mode with software slave management */
    whal_Reg_Update(reg->base, SSPI_CR1_REG,
                    SSPI_CR1_MSTR | SSPI_CR1_SSM | SSPI_CR1_SSI,
                    whal_SetBits(SSPI_CR1_MSTR, 1) |
                    whal_SetBits(SSPI_CR1_SSM, 1) |
                    whal_SetBits(SSPI_CR1_SSI, 1));

    /* 8-bit data size and FIFO receive threshold for 8-bit */
    whal_Reg_Update(reg->base, SSPI_CR2_REG,
                    SSPI_CR2_DS | SSPI_CR2_FRXTH,
                    whal_SetBits(SSPI_CR2_DS, SSPI_DS_8BIT) |
                    whal_SetBits(SSPI_CR2_FRXTH, 1));

    return WHAL_SUCCESS;
}

whal_Error whal_Stm32wbSpi_Deinit(whal_Spi *spiDev)
{
    whal_Error err;
    const whal_Regmap *reg = &spiDev->regmap;
    whal_Stm32wbSpi_Cfg *cfg = (whal_Stm32wbSpi_Cfg *)spiDev->cfg;

    /* Disable SPI */
    whal_Reg_Update(reg->base, SSPI_CR1_REG, SSPI_CR1_SPE,
                    whal_SetBits(SSPI_CR1_SPE, 0));

    err = whal_Clock_Disable(cfg->clkCtrl, cfg->clk);
    if (err) {
        return err;
    }

    return WHAL_SUCCESS;
}

whal_Error whal_Stm32wbSpi_SendRecv(whal_Spi *spiDev, void *spiComCfg, const uint8_t *tx,
                                    size_t txLen, uint8_t *rx, size_t rxLen)
{
    const whal_Regmap *reg = &spiDev->regmap;
    whal_Stm32wbSpi_Cfg *cfg = (whal_Stm32wbSpi_Cfg *)spiDev->cfg;
    whal_Stm32wbSpi_ComCfg *comCfg = (whal_Stm32wbSpi_ComCfg *)spiComCfg;
    size_t totalLen = txLen > rxLen ? txLen : rxLen;
    size_t status;
    size_t d;

    whal_Stm32wbSpi_ApplyComCfg(reg, cfg, comCfg);

    for (size_t i = 0; i < totalLen; i++) {
        if (txLen) {
            /* Wait for TX buffer empty */
            do {
                whal_Reg_Get(reg->base, SSPI_SR_REG, SSPI_SR_TXE, &status);
            } while (!status);

            /* Write data or dummy byte */
            uint8_t txByte = (i >= txLen) ? tx[txLen-1] : tx[i];
            *(volatile uint8_t *)(reg->base + SSPI_DR_REG) = txByte;
        }

        if (rxLen) {
            /* Wait for RX buffer not empty */
            do {
                whal_Reg_Get(reg->base, SSPI_SR_REG, SSPI_SR_RXNE, &status);
            } while (!status);

            /* Read received byte */
            d = *(volatile uint8_t *)(reg->base + SSPI_DR_REG);
            if (i < rxLen) {
                rx[i] = (uint8_t)d;
            }
        }
    }

    /* Wait for not busy */
    do {
        whal_Reg_Get(reg->base, SSPI_SR_REG, SSPI_SR_BSY, &status);
    } while (status);

    return WHAL_SUCCESS;
}

whal_Error whal_Stm32wbSpi_Send(whal_Spi *spiDev, void *spiComCfg, const uint8_t *data,
                                size_t dataSz)
{
    return whal_Stm32wbSpi_SendRecv(spiDev, spiComCfg, data, dataSz, NULL, 0);
}

whal_Error whal_Stm32wbSpi_Recv(whal_Spi *spiDev, void *spiComCfg, uint8_t *data,
                                size_t dataSz)
{
    return whal_Stm32wbSpi_SendRecv(spiDev, spiComCfg, NULL, 0, data, dataSz);
}

const whal_SpiDriver whal_Stm32wbSpi_Driver = {
    .Init = whal_Stm32wbSpi_Init,
    .Deinit = whal_Stm32wbSpi_Deinit,
    .SendRecv = whal_Stm32wbSpi_SendRecv,
    .Send = whal_Stm32wbSpi_Send,
    .Recv = whal_Stm32wbSpi_Recv,
};

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

When txLen is 0 in a receive-only operation, no data is written to the SPI data register to trigger the clock generation necessary for receiving data. In SPI master mode, the master must transmit data (or dummy bytes) to generate clock pulses for the slave to send data back. Add logic to transmit dummy bytes (e.g., 0xFF) when tx is NULL or i >= txLen.

Suggested change

	if (txLen) {
	/* Wait for TX buffer empty */
	do {
	whal_Reg_Get(reg->base, SSPI_SR_REG, SSPI_SR_TXE, &status);
	} while (!status);
	/* Write data or dummy byte */
	uint8_t txByte = (i >= txLen) ? tx[txLen-1] : tx[i];
	*(volatile uint8_t *)(reg->base + SSPI_DR_REG) = txByte;
	}
	/* Always transmit a byte to generate clock pulses (dummy if no TX data) */
	/* Wait for TX buffer empty */
	do {
	whal_Reg_Get(reg->base, SSPI_SR_REG, SSPI_SR_TXE, &status);
	} while (!status);
	/* Write data or dummy byte */
	uint8_t txByte;
	if (tx != NULL && i < txLen) {
	txByte = tx[i];
	} else {
	txByte = 0xFFu; /* Dummy byte for clock generation */
	}
	*(volatile uint8_t *)(reg->base + SSPI_DR_REG) = txByte;

[Copilot]

Null pointer checks are missing for critical parameters. If spiComCfg is NULL, the cast and subsequent access will cause undefined behavior. Add validation: if (!spiComCfg || !cfg) return WHAL_EINVAL. Similarly, when tx or rx are expected to be non-NULL based on txLen or rxLen, validate them to prevent dereferencing NULL pointers.

[Copilot]

The return value of whal_Clock_GetRate is not checked for errors. If the clock rate retrieval fails, pclk will have an undefined value, leading to incorrect baud rate calculation. Add error checking and return early on failure, similar to how whal_Stm32wbUart_Init handles this at lines 43-46 in src/uart/stm32wb_uart.c.

[Copilot]

When tx is NULL (in receive-only mode), accessing tx[txLen-1] at line 167 will cause a null pointer dereference and crash. The condition should check if tx is NULL before accessing it, and send a dummy byte (typically 0xFF) when tx is NULL.

Suggested change

	if (txLen) {
	/* Wait for TX buffer empty */
	do {
	whal_Reg_Get(reg->base, SSPI_SR_REG, SSPI_SR_TXE, &status);
	} while (!status);
	/* Write data or dummy byte */
	uint8_t txByte = (i >= txLen) ? tx[txLen-1] : tx[i];
	*(volatile uint8_t *)(reg->base + SSPI_DR_REG) = txByte;
	}
	/* Always transmit a byte to generate clock pulses.
	* If tx is NULL (receive-only), send dummy 0xFF.
	*/
	/* Wait for TX buffer empty */
	do {
	whal_Reg_Get(reg->base, SSPI_SR_REG, SSPI_SR_TXE, &status);
	} while (!status);
	/* Write data or dummy byte */
	uint8_t txByte = 0xFF;
	if (tx && txLen) {
	txByte = (i >= txLen) ? tx[txLen - 1] : tx[i];
	}
	*(volatile uint8_t *)(reg->base + SSPI_DR_REG) = txByte;

[Copilot]

The SPI peripheral is not enabled during initialization. The SPE (SPI Enable) bit must be set for the SPI peripheral to function. Add a call to enable SPE after configuring CR1 and CR2 registers: whal_Reg_Update(reg->base, SSPI_CR1_REG, SSPI_CR1_SPE, whal_SetBits(SSPI_CR1_SPE, 1));

Suggested change

	/* Enable SPI peripheral */
	whal_Reg_Update(reg->base, SSPI_CR1_REG, SSPI_CR1_SPE,
	whal_SetBits(SSPI_CR1_SPE, 1));

[Copilot]

The SPI peripheral is reconfigured (mode and baud rate) on every call to SendRecv, which includes disabling and re-enabling SPE. This is inefficient for consecutive transactions with the same device. Consider caching the current configuration and only reconfiguring when it changes, or providing separate configuration and transfer functions to allow the user to configure once and transfer multiple times.

[Copilot]

The logic for determining txByte is incorrect. When i >= txLen, the condition evaluates to true and accesses tx[txLen-1], but when i < txLen, it should use tx[i]. The condition should be reversed to: (i < txLen) ? tx[i] : tx[txLen-1]. Currently, this will always use the last byte of the transmit buffer instead of sending data sequentially.

Suggested change

	uint8_t txByte = (i >= txLen) ? tx[txLen-1] : tx[i];
	uint8_t txByte = (i < txLen) ? tx[i] : tx[txLen - 1];