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.

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

[EZ#10853] spi transfer bug #363

Closed
long585 opened this issue Feb 20, 2017 · 40 comments
Closed

[EZ#10853] spi transfer bug #363

long585 opened this issue Feb 20, 2017 · 40 comments
Assignees

Comments

@long585
Copy link

long585 commented Feb 20, 2017

spi_transaction_t t;
t.length=TEST_SIZE*8;
t.tx_buffer=send_buf;
t.rx_buffer=recv_buf;
spi_device_transmit(spi[0], &t);

if TEST_SIZE >31 the rx_buffer can`t recv right data.

@Spritetm
Copy link
Member

Do you happen to have some more information than 'can't recv right data'? What data do you expect and what data actually ends up in the receive buffer?

@Spritetm Spritetm self-assigned this Feb 20, 2017
@long585
Copy link
Author

long585 commented Feb 20, 2017

const char send_buf[50] =
{
0x01,0x02,0x03,0x04,0x05,0x06,0x07,0x08,0x09,0x0A,
0x0B,0x0C,0x0D,0x0E,0x0F,0x10,0x11,0x12,0x13,0x14,
0x15,0x16,0x17,0x18,0x19,0x1A,0x1B,0x1C,0x1D,0x1E, 
0x1F,0x20,0x21,0x22,0x23,0x24,0x25,0x26,0x27,0x28,
0x29,0x2A,0x2B,0x2C,0x2D,0x2E,0x2F,0x30,0x31,0x32
};
char recv_buf[50]={0};

above are data buf

if t.length >31 the recv_buf data are messy code,like this:

Transmit...
Send vs recv:
1 2 3 4 5 6 7 8 9 a b c d e f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 <sent
8e db 33 ef fd fe d4 73 d7 7f 7e 3d ff 1d e3 8f be f4 a a5 f9 7a fe 5b ed ff f6 f4 62 cf ff fb <recv

@long585
Copy link
Author

long585 commented Feb 20, 2017

I get through read the spi driver code,i look the dma threshhold

//If a transaction is smaller than or equal to of bits, we do not use DMA; instead, we directly copy/paste
//bits from/to the work registers. Keep between 32 and (8*32) please.
#define THRESH_DMA_TRANS (8*32)

now,i want to know the threshhold can freedom configure?
@Spritetm

@negativekelvin
Copy link
Contributor

Possibly related #336

@methodmissing
Copy link
Contributor

Also worth noting is that the constant https://github.com/espressif/esp-idf/blob/master/components/driver/spi_master.c#L449 should be used here https://github.com/espressif/esp-idf/blob/master/components/driver/spi_master.c#L628 too to possibly avoid inline values leaking in the future.

@Spritetm
Copy link
Member

methodmissing: Check. I've included it in the code change for #336.

@long585
Copy link
Author

long585 commented Feb 22, 2017

new issue
when spi master clock is 10Mhz the recv data is right:

Send vs recv:
1 2 3 4 5 6 7 8 9 a b c d e f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e <sent
1 2 3 4 5 6 7 8 9 a b c d e f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e <recv

but spi master clock change to 20Mhz the recv data is wrong:

Send vs recv:
1 2 3 4 5 6 7 8 9 a b c d e f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e <sent
0 81 1 82 2 83 3 84 4 85 5 86 6 87 7 88 8 89 9 8a a 8b b 8c c 8d d 8e e 8f <recv

@negativekelvin @methodmissing

@Spritetm
Copy link
Member

I just fixed some SPI code, commit c057f06 . Can you check if this fixes the issue you originally had?

@long585
Copy link
Author

long585 commented Feb 23, 2017

I had fixed the code, but the issue still exist. At the moment I found that DMA can not be sent and transmitted at the same time. Can you test this issue ?
@Spritetm @negativekelvin @methodmissing

@Spritetm
Copy link
Member

'sent and transmitted'? Do you mean 'sent and received'? If so, you need to enable full-duplex mode :)

In general, as far as I can see, SPI works perfectly well with 10MHz. I'm using the testcase in components/driver/test/test_spi_master.c and modified the clock to 10 (and later even 20) MHz, and I receive the transmitted data without issue.

@long585
Copy link
Author

long585 commented Feb 23, 2017

whether you test the data size >32 bytes? E.g 50 bytes?
You have provided the test case I have tried, really no problem,
@Spritetm

@Spritetm
Copy link
Member

Spritetm commented Feb 23, 2017

Ah derp, sorry, forgot the length is in bits. Lemme try...

Edit: Nope, 160 bytes, 10MHz and I still get back what I sent.
2nd edit: Aaaaah, no wait, I see what you mean now. Hmmm, that is strange.

@Spritetm
Copy link
Member

Seems like this is a hardware thing. I'm discussing it with the HW guys. I can work around the symptoms in the driver, but I'd rather understand what's going on first.

@long585
Copy link
Author

long585 commented Feb 23, 2017

I hope to find the issue as soon as possible, and then give the solution, my project is very urgent to use SPI DMA.thanks!

@Spritetm
Copy link
Member

Hmm. Can you check once more if transmitting <31 bytes of data at 20MHz does not give you any problems?

@negativekelvin
Copy link
Contributor

negativekelvin commented Feb 23, 2017

If you adjust clkcnt_h then it works up to 20mhz but breaks again at 26mhz aka clkcnt_n < 3. Happens with 16 bytes too.

@negativekelvin
Copy link
Contributor

negativekelvin commented Feb 23, 2017

Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=726, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 110041
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=570, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 140105
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=420, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 190023
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=228, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 349344
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=210, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 379146
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=166, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 479041
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=162, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 490797
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=156, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 509554
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=150, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 529801
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=130, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 610687
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=126, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 629921
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=112, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 707964
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=106, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 747663
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=102, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 776699
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=100, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 792079
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=88, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 898876
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=82, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 963855
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=2, hw->clock.clkdiv_pre=0, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=2
SPI Frequency: 26666666
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=0, hw->clock.clkcnt_n=1, hw->clock.clkdiv_pre=0, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=1
SPI Frequency: 40000000
Data does not match!
Clock registers: hw->clock.clk_equ_sysclk=1, hw->clock.clkcnt_n=0, hw->clock.clkdiv_pre=0, hw->clock.clkcnt_h=0, hw->clock.clkcnt_l=0
SPI Frequency: 80000000

Here is a scan of all the clk freq in 10khz increments. Where trouble happens clkdiv_pre >> 0 && clkcnt_n = 0 or clkdiv_pre = 0 && clkcnt_n < 3

We can fix all the errors below 26MHz by restricting clkcnt_n > 0. Then we are left with just errors when freq > fapb/4

@negativekelvin
Copy link
Contributor

negativekelvin commented Feb 23, 2017

All freq work using the native mux pins including clk_equ_sysclk.

@negativekelvin
Copy link
Contributor

negativekelvin commented Feb 23, 2017

26mhz working now using matrix pins. 40mhz needs dummy cycle? Can't get it to work. Enable dummy cycle stalls it.

@Spritetm
Copy link
Member

Spritetm commented Feb 24, 2017

negativekelvin: I get the same result here: everything up to 40MHz works, but 40MHz hangs when enabling dummy bits. Will ask the digital guys, but it looks like the SPI host controller is not happy with the combination of dummy bits and full-duplex mode.

@long585
Copy link
Author

long585 commented Feb 24, 2017

What is the current progress of this issue?

@negativekelvin
Copy link
Contributor

@long585 negativekelvin@1385bf0 seems to fix everything but 40mhz

@Spritetm
Copy link
Member

I'm actually working on a similar fix, but using the registers meant for it instead of messing with the clock rate calculations... 40MHz AND full-duplex AND using the GPIO matrix is still an issue, though: seems the engineer didn't have that usage scenario in mind. We're seeing if we can work around that limitation somehow, but that may take a while.

@long585
Copy link
Author

long585 commented Feb 24, 2017

Well, do not pay attention to this issue first. Another problem when I send the data size is 50 bytes, the received data is wrong。

ERROR timer_count:180
0 be 92 1f b7 fe 75 26 db fb 99 7b fe f9 de 61 37 be 92 1f b7 fe 75 26 db fb 99 7b fe f9 de 61 0 be 92 1f b7 fe 75 26 db fb 99 7b fe f9 de 61 37 0 

@negativekelvin
Copy link
Contributor

50 bytes working for me, post your code

@long585
Copy link
Author

long585 commented Feb 24, 2017

next week post code

@long585
Copy link
Author

long585 commented Feb 27, 2017

my test hw environment is self send self recv

#define PIN_NUM_MISO 16
#define PIN_NUM_MOSI 17
#define PIN_NUM_CLK  25
#define PIN_NUM_CS   27

#define PIN_NUM_DC   21
#define PIN_NUM_RST  18
#define PIN_NUM_BCKL 5



#if 1
#define BUF_COUNT 50
#define BUF_SIZE  50
#define TEST_SIZE 30

char tmp_buf[50]={0};
const char send_buf[BUF_SIZE] ={0x01,0x02,0x03,0x04,0x05,0x06,0x07,0x08,0x09,0x0A,
					  			0x0B,0x0C,0x0D,0x0E,0x0F,0x10,0x11,0x12,0x13,0x14,
					  			0x15,0x16,0x17,0x18,0x19,0x1A,0x1B,0x1C,0x1D,0x1E,
					  			0x1F,0x20,0x21,0x22,0x23,0x24,0x25,0x26,0x27,0x28,
					  			0x29,0x2A,0x2B,0x2C,0x2D,0x2E,0x2F,0x30,0x31,0x32};



#define TRANS_BUFFER 1
#ifdef TRANS_BUFFER
typedef struct{
	char data_buf[BUF_COUNT][BUF_SIZE];
	int read_index;
	int write_index;
	int valid_count;
}spi_recv_buf;
spi_recv_buf recv_buf;
#endif

#endif
spi_transaction_t t;

spi_device_handle_t spi;



//This function is called (in irq context!) just before a transmission starts. It will
//set the D/C line to the value indicated in the user field.
void ili_spi_pre_transfer_callback(spi_transaction_t *t) 
{
    int dc=(int)t->user;
    gpio_set_level(PIN_NUM_DC, dc);
}



void spi2_recive_callback(spi_transaction_t *t)
{

		static int callback_count =0;
		char * add = (char *)(t->rx_buffer);
		callback_count++;
		if(memcmp(add,send_buf,BUF_SIZE)!=0)
		{
			printf("ERROR timer_count:%d\n",callback_count);

			for(int i =0;i< BUF_SIZE;i++)
			{
				printf("%x ",add[i]);
			}
			printf("\n");

		}

		memset(&recv_buf,0,sizeof(recv_buf));
		
	
}
void spi_trans(void)
{

		esp_err_t ret;

	
	    memset(&t, 0, sizeof(t));

	    t.length=BUF_SIZE*8;
		t.rxlength = BUF_SIZE*8;
	    t.tx_buffer=send_buf;
	    t.rx_buffer=&recv_buf.data_buf[recv_buf.write_index];
		recv_buf.write_index++;
		recv_buf.write_index %=BUF_COUNT;
				
	    ret=spi_device_transmit(spi, &t);


}
void app_main()
{
    esp_err_t ret;
    
    spi_bus_config_t buscfg={
        .miso_io_num=PIN_NUM_MISO,
        .mosi_io_num=PIN_NUM_MOSI,
        .sclk_io_num=PIN_NUM_CLK,
        .quadwp_io_num=-1,
        .quadhd_io_num=-1
    };
    spi_device_interface_config_t devcfg={
        .clock_speed_hz=10000000,               //Clock out at 10 MHz
        .mode=0,                                //SPI mode 0
        .spics_io_num=PIN_NUM_CS,               //CS pin
        .queue_size=7,                          //We want to be able to queue 7 transactions at a time
        //.pre_cb=ili_spi_pre_transfer_callback,  //Specify pre-transfer callback to handle D/C line
        .post_cb = spi2_recive_callback
    };
    //Initialize the SPI bus
    ret=spi_bus_initialize(HSPI_HOST, &buscfg, 1);
    assert(ret==ESP_OK);
    //Attach the LCD to the SPI bus
    ret=spi_bus_add_device(HSPI_HOST, &devcfg, &spi);
    assert(ret==ESP_OK);

	while(1)
	{
		vTaskDelay(1000 / portTICK_PERIOD_MS);
		spi_trans();
	}

}

@negativekelvin @Spritetm

@long585
Copy link
Author

long585 commented Mar 1, 2017

@negativekelvin @Spritetm Have you ever seen this code?

@Spritetm
Copy link
Member

Spritetm commented Mar 2, 2017

Please do not use the callbacks for something like that. They're called as soon as the physical transfer is complete, in interrupt context. They're not meant for anything more than twiddle a GPIO or something.

@long585
Copy link
Author

long585 commented Mar 3, 2017

I know, but I would like to know why this code receives 50 bytes will be wrong
@Spritetm

@negativekelvin
Copy link
Contributor

negativekelvin commented Mar 3, 2017

@long585 I think when DMA is active your send_buf needs to be placed in ram with DRAM_ATTR

@long585
Copy link
Author

long585 commented Mar 6, 2017

Your idea is right,I modify the send_buf to be place in ram with DRAM_ATTR,but the last one byte date still wrong. like this:

recv:
1 2 3 4 5 6 7 8 9 a b c d e f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 0 

@negativekelvin

@long585
Copy link
Author

long585 commented Mar 6, 2017

This is my test case, send size and recv size from 45 bytes to 55 bytes.The test results are as follows:

recv_size:45
1 2 3 4 5 6 7 8 9 a b c d e f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 
recv_size:46
1 2 3 4 5 6 7 8 9 a b c d e f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 0 
recv_size:47
1 2 3 4 5 6 7 8 9 a b c d e f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 0 0 
recv_size:48
1 2 3 4 5 6 7 8 9 a b c d e f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 
recv_size:49
1 2 3 4 5 6 7 8 9 a b c d e f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 
recv_size:50
1 2 3 4 5 6 7 8 9 a b c d e f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 0 
recv_size:51
1 2 3 4 5 6 7 8 9 a b c d e f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 0 0 
recv_size:52
1 2 3 4 5 6 7 8 9 a b c d e f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 
recv_size:53
1 2 3 4 5 6 7 8 9 a b c d e f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 35 
recv_size:54
1 2 3 4 5 6 7 8 9 a b c d e f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 35 0 
recv_size:55
1 2 3 4 5 6 7 8 9 a b c d e f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 30 31 32 33 34 35 0 0 

@negativekelvin
Copy link
Contributor

negativekelvin commented Mar 6, 2017

Not sure, it appears to work when setting RX DMA size to be 32 bit aligned.

@Spritetm
https://github.com/espressif/esp-idf/blob/master/components/driver/spi_master.c#L607

@long585
Copy link
Author

long585 commented Mar 8, 2017

What is the progress of this issue?
@negativekelvin @Spritetm

@negativekelvin
Copy link
Contributor

@Spritetm any comment about DMA and RX fifo copy issue?

@Spritetm
Copy link
Member

Spritetm commented Mar 9, 2017

Gonna look into it, but at the moment I'm slightly distracted by some other (internal) issues.

@long585
Copy link
Author

long585 commented Mar 13, 2017

What is the progress of this issue?
@negativekelvin @Spritetm

igrr pushed a commit that referenced this issue Mar 31, 2017
Fix timing adjustment needed for higher speeds of SPI master bus.

Ref #363 . It was found out the master SPI driver didn't exactly calculate the delay compensation needed, breaking 20 and 26MHz full-duplex mode. This fixes these use cases. We also found out 40MHz full-duplex routed over the GPIO matrix does not work because of a hardware quirk; this MR adds a check/error for that case until we find a workaround.

See merge request !547
@FayeY FayeY changed the title spi transfer bug [EZ#10853] spi transfer bug Apr 28, 2017
@igrr
Copy link
Member

igrr commented May 1, 2017

Fixed in a4acf7b

@igrr igrr closed this as completed May 1, 2017
@Spritetm
Copy link
Member

Spritetm commented May 1, 2017

Actually, it's been fixed in 46fa2cf or one of its subcommits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants