zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.01k stars 6.16k forks source link

drivers: spi: spi_rpi_pico_pio: Byte order is incorrect #72954

Open ThreeEights opened 1 month ago

ThreeEights commented 1 month ago

Describe the bug The PIO-based SPI driver for the Raspberry Pi Pico (and the RP2040 in general) sends data out the SPI bus in the wrong byte order when SPI_WORD_SET() is 16 or 32 bits, and reverses the byte order of the input data. Data is passed to the SPI driver (tx buffer) in the byte order intended for transmission. However, the little-endian PIO SPI driver treats the data as 16- or 32-bit integers, loading the data into the transmit FIFO in the reverse of the correct order and not correcting the reversed data in the receive FIFO.

To Reproduce Steps to reproduce the behavior:

  1. The only readily available device that supports SPI_WORD_SET(32) in Zephyr is the TI TMAG5170 sensor.
  2. Wire the TMAG5170 to the Raspberry Pi Pico (see "Additional context" for configuration).
  3. Build samples/sensor/magn_polling, adding the boards/rpi_pico.overlay provided below, with the command west build -p auto -b rpi_pico
  4. While monitoring the SPI bus, run the generated sample.

Observed behavior The command to read the TEST_CONFIG register should be 0x8F000008, but is sent on the bus as 0x08 0x00 0x00 0x8F. The input line receives 0xE0 0x00 0x00 0x8B, but the status message delivered to the application is 0x8B0000E0. The TMAG5170 driver repeats the attempt to read the TEST_CONFIG register, never leaving the driver's init function.

Expected behavior The data should be transmitted in the order provided, regardless of the CPU byte order: 0x8F 0x00 0x00 0x08, and the received data should be placed in the rx buffer in the order expected: 0xE0 0x00 0x00 0x8B.

Impact The PIO SPI driver cannot operate correctly in either SPI_WORD_SET(16) or SPI_WORD_SET(32) configurations.

Logs and console output Data as exchanged on the SPI bus (first two 32-bit exchanges):

Time [s],Packet ID,MOSI,MISO
0.0000392500,0,0x08,0xE0
0.0000472500,0,0x00,0x00
0.0000552500,0,0x00,0x00
0.0000632500,0,0x8F,0x8B
0.0000892500,0,0x08,0xE0
0.0000972500,0,0x00,0x00
0.0001052500,0,0x00,0x00
0.0001132500,0,0x8F,0x8B

Environment (please complete the following information):

Additional context boards/rpi_pico.overlay used to build sample:

/ {
    aliases {
        magn0 = &tmag5170;
    };
};

&pinctrl {
    pio0_spi0_default: pio0_spi0_default {
        /* gpio 9 is used for chip select, not assigned to the PIO */
        group1 {
            pinmux = <PIO0_P10>, <PIO0_P11>;
        };
        group2 {
            pinmux = <PIO0_P8>;
            input-enable;
        };
    };

};

&pio0 {
    status = "okay";

    pio0_spi0: pio0_spi0 {
        pinctrl-0 = <&pio0_spi0_default>;
        pinctrl-names = "default";

        compatible = "raspberrypi,pico-spi-pio";
        status = "okay";
        #address-cells = <1>;
        #size-cells = <0>;
        clocks = <&clocks RPI_PICO_CLKID_CLK_SYS>;
        miso-gpios = <&gpio0 8 0>;
        cs-gpios = <&gpio0 9 GPIO_ACTIVE_LOW>;
        clk-gpios = <&gpio0 10 GPIO_ACTIVE_HIGH>;
        mosi-gpios = <&gpio0 11 GPIO_ACTIVE_HIGH>;
        tmag5170: tmag5170@0 {
            compatible = "ti,tmag5170";
            reg = <0>;
            operating-mode = <3>;
            spi-max-frequency = <1000000>; /* conservatively set to 1MHz */
        };
    };
};

TMAG5170 connections to RPI Pico:

ThreeEights commented 1 month ago

@morsisko - Feel free to comment on this issue.

morsisko commented 1 month ago

Hello, sorry for late response. When I was designing the driver I was using mainly ESP32 SPI driver. I also encountered bugs described here: https://github.com/zephyrproject-rtos/zephyr/issues/54746

ESP32 driver send bytes as they are in the buffer (so that byte[0] will be sent first on the SPI, then byte[1] and so on). I don't know if it is correct behavior or not, however definitely there needs to be some kind of agreement about the byte order between different SPI drivers.

ThreeEights commented 1 month ago

@morsisko - Thanks. I think I have the Pico PIO SPI driver working that way, too. It seems like a sensible convention: Send the bytes in the order in the buffer, whether sending 8, 16, or 32 bytes in a transaction. Once I have a pull request ready we should be able to close this issue.