MediaTek-Labs / mt3620_m4_software

mt3620_m4_driver
Other
33 stars 29 forks source link

SPI Master Ignores First Received Byte #11

Closed brandonhanner closed 4 years ago

brandonhanner commented 4 years ago

Hey guys,

I have a project that communicates with an SPI device and the command structure is such that you send an 32 bit transaction that looks like [opcode-8bits][upper half of address-8bits][lower half of address-8bits][num words-8bits] followed by [num words] of 32 bit words, which seems to work fine.

When querying the device for information, you send it the same [opcode][upper half of address][lower half of address][num words] with a different opcode (one to read instead of write), and the device replies with [num words] of data. This part, again, works correctly. I have verified using a logic analyzer that the device is replying exactly as the data sheet says.

The problem is when using the SPIM in synchronous FIFO mode (havent tried async, but have reproduced in dma mode), the first byte of the reply from the slave device is not placed in the RX buffer, due to what looks like in the source code something to do with an opcode (see here). I have the buffer length at 4 bytes and the xfer.len set to 4 (because thats what the device replies with)

The examples for SPIM show that the sending and receiving only occurs on index 1 through the size of the send and receive buffers. Why doesn't send/receive start at buffer index 0? Are there some magic numbers that have to go in index 0 that I missed in the documentation?

brandonhanner commented 4 years ago

For clarification I have posted my user code snippet below. I am trying to accomplish a raw 32bit read from the bus.

The transfer is initiated by this snippet

//create a transfer structure and initialize it to all zeros
    struct mtk_spi_transfer xfer;
    memset(&xfer, 0, sizeof(xfer));

    //fill out the structure with relevant information
    xfer.tx_buf = spim_tx_buf;
    xfer.rx_buf = NULL;
    xfer.use_dma = 0; //no dma
    xfer.speed_khz = spi_master_speed;
    xfer.len = 4; //length of 3 bytes to send (address and number of proceeding bytes to expect)

    //put the bytes to be sent into the buffer
    uint8_t *buf = (uint8_t *)xfer.tx_buf;
    buf[0] = AHB_READ_OPCODE; //tell the tcan that we want to write to its special registers
    uint8_t *p = (uint8_t*)&address;
    buf[1] = p[1]; //spread the 16bit "address" variable across two 8 bit containers
    buf[2] = p[0]; //see comment on line above
    buf[3] = words;

    uint32_t test = (buf[0] << 24) | (buf[1] << 16) | (buf[2] << 8) | buf[3];

    //set the aux cs pin low HERE
    mtk_os_hal_gpio_set_output(gpio_ncs, OS_HAL_GPIO_DATA_LOW);
    mtk_os_hal_gpio_set_output(gpio_led_green, OS_HAL_GPIO_DATA_LOW);
    //send the bytes down the spi bus, and wait for the transfer to complete
    ret = mtk_os_hal_spim_transfer((spim_num) spi_master_port_num, &spi_default_config, &xfer);

now that we've told the chip which address we want to read, we can initiate the read sequence below:

    //create a transfer structure and initialize it to all zeros
    struct mtk_spi_transfer xfer;
    memset(&xfer, 0, sizeof(xfer));

    //fill out the structure with relevant information
    xfer.tx_buf = NULL;
    xfer.rx_buf = spim_rx_buf;
    xfer.use_dma = 0; //no dma
    xfer.speed_khz = spi_master_speed;
    xfer.len = 4; //length of 4 bytes to read

    //cast the buffer into uint8 array type so that it can be accessed with brackets
    uint8_t* buf = (uint8_t*)xfer.rx_buf;

    //initiate the transfer and wait for the transfer to complete
    ret = mtk_os_hal_spim_transfer((spim_num) spi_master_port_num, &spi_default_config, &xfer);

    uint32_t data = 0;

    data = (((uint32_t)buf[0]) << 24) | (((uint32_t)buf[1]) << 16) | (((uint32_t)buf[2] << 8)) | buf[3];
    return data;
LawranceLiu commented 4 years ago

Hi Brandon, Thank you very much for reporting the issue. The symptom you see is an expected behavior.

You could refer to the OS_HAL API description in: https://github.com/MediaTek-Labs/mt3620_m4_software/blob/master/MT3620_M4_Sample_Code/FreeRTOS/OS_HAL/inc/os_hal_spim.h#L129

When you try to read 4 bytes, you could configure the xfer data structure as following: xfer.tx_buf = NULL; xfer.rx_buf = rx_buf; //rx_buf[5] xfer.len = 5; // 1 opcode + 4byte rx data

You are welcome to let me know if you have any further question.

brandonhanner commented 4 years ago

Hey Lawrance,

The problem still persists. I have attached some pictures to help illustrate the issue. I have a third party device whose SPI protocol I cannot change. Therefore, I can only receive 4 bytes from the device and I need all 4 bytes but Sphere is throwing away the leading byte regardless of 4 or 5 byte transfers.

The device is replying the same bit pattern whether I set the transfer to 4 or 5 bytes, the only difference is that sphere leaves the clock running for 5 byte-times rather than 4. It still fails to place the bits into rx buf.

Here's the 4 byte waveform and debug window: 4byte_waveform

4byte_debug

Notice the clock looks correct, but of the 4 bytes I need, 80 96 28 FF, only the latter three show up in the buffer (see "tester" variable)

Now take a look at the 5 byte waveform and debug window:

5byte_waveform 5byte_debug

Notice the clock runs for 5 byte times, however the byte 80 is never placed in the buffer.

brandonhanner commented 4 years ago

I was able to hack together a solution by modifying the driver once I got ahold of the real datasheet but I have to use a 5 length transfer and set the opcode len = 0 for it to work...

Can we not just configure the peripheral to send (len) number of bytes from the tx buffer starting at 0 index and receive (len) number of bytes into rx buffer starting at 0 index? That would make this whole operation so much easier, no messing around with opcodes and not being able to use all of the tx and rx buffers. I understand that some devices use opcodes but it would be so much easier for the programmer to get the raw data into the buffers and just pull from index 1+ if you don't want it.

LawranceLiu commented 4 years ago

Hi Brandon, Thanks for all the detailed information you provided, we could understand the situation now.

Currently, the mtk_os_hal_spim_async_transfer( ) only supports "read with opcode". We are planning to update the mtk_spi_transfer data structure to let APP define the opcode length when invoking mtk_os_hal_spim_async_transfer( ). This modification is supposed to be able to support your case (set opcode length to 0), and would be ready by 3/20.

brandonhanner commented 4 years ago

Ok awesome thank you!

brandonhanner commented 4 years ago

Hey Lawrance,

I modified the half duplex portion of the "mtk_hdl_spim_fifo_handle_rx" function in hdl_spim.c by changing the limits of the for loop to 0->len, as well as removing the +1 from the memcopy within the loop (so it will start at index zero and fill as many bytes as received) Here is the resulting code snippet:

    if ((!tx_buf) && rx_buf) {  /* half duplex */
        for (i = 0; i < len; i++) {
            q = i / 4;
            r = i % 4;
            reg_val = osai_readl(SPI_REG_SDIR(base, q));
            val_tmp = (u8) (reg_val >> (r * 8));

            spim_debug("i:%d,rx_data:0x%x, val_tmp:0x%x\n",
                   i, reg_val, val_tmp);

            memcpy(rx_buf + i, &val_tmp, 1);
        }

Second, in the call to "mtk_mhal_spim_fifo_transfer_one", I modified the snippet dealing with half duplex receive calls to look like this:

    if ((!xfer->tx_buf) && xfer->rx_buf) {//if we are only receiving
        //memcpy(&opcode, xfer->rx_buf, 1); 
        //opcode &= 0xff;
        spim_debug("rx opcode(0x%x)\n", opcode);
        mtk_hdl_spim_enable_fifo_transfer(ctlr->base, 0, 0, 0,
                          xfer->rx_buf + 1,
                          xfer->len, tx_enable,
                          rx_enable);

And now I have a proper (len) byte clock, and it fills the rx buffer from 0. This might break the ability to "receive with opcode" for anyone who sees this and wants to adopt it, but does exactly what I need it to for my application, which is to receive (len) bytes into the rx buffer starting at index 0.

LawranceLiu commented 4 years ago

Hi Brandon, Thank you very much for the sharing.

LawranceLiu commented 4 years ago

The SPI API is updated in release_200320, APP could control the opcode length (0~4). Please reopen this issue if any further issue found.