Skip to content

ONSEMI: Fix a few issues related to I2C #5394

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

Merged
merged 1 commit into from
Nov 6, 2017
Merged

ONSEMI: Fix a few issues related to I2C #5394

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
8 changes: 5 additions & 3 deletions targets/TARGET_ONSEMI/TARGET_NCS36510/i2c.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@
#define I2C_SPEED_400K_AT_8MHZ (uint8_t)0x03
#define I2C_SPEED_400K_AT_16MHZ (uint8_t)0x08


/* I2C commands */
#define I2C_CMD_NULL 0x00
#define I2C_CMD_WDAT0 0x10
Expand Down Expand Up @@ -93,7 +92,10 @@
#define I2C_API_STATUS_SUCCESS 0
#define PAD_REG_ADRS_BYTE_SIZE 4

#define SEND_COMMAND(cmd) while(!I2C_FIFO_EMPTY); wait_us(1); obj->membase->CMD_REG = cmd;
// The wait_us(0) command is needed so the I2C state machines have enough
// time for data to settle across all clock domain crossings in their
// synchronizers, both directions.
#define SEND_COMMAND(cmd) wait_us(0); obj->membase->CMD_REG = cmd; wait_us(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

It was wait_us(1), now it is 0? What is intention with 0 as a time waiting period?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @0xc0170, I ran the I2C tests for the CI test shield, all the mbed tests, and our own test program below with 0 failures after many runs, even at different system clock divider frequencies.

For the delay going to 0 instead of 1us, only 0 is needed. The system just needs a handful of extra clock cycles so giving the MCU something to do so the internal state machines can synchronize the data. There are clock domain crossings as the data crosses from APB bus to internal and then back. It's not exactly a bug but not exactly what was intended either.

Here is the test code. I've ran tens of thousands of transactions and no longer see any failures.

// check if I2C is supported on this device
#if !DEVICE_I2C
  #error [NOT_SUPPORTED] I2C not supported on this platform, add 'DEVICE_I2C' definition to your platform.
#endif

#include "mbed.h"
#include "I2CEeprom.h"
#include <string.h>

#define I2C_ADDRESS 0xA0
#define SIZE_DATA 33
#define ITERATIONS 512

int total_failures = 0;
int total_num_tests = 0;

Serial pc(USBTX, USBRX);
I2CEeprom memory(I2C_SDA, I2C_SCL, I2C_ADDRESS, 32, 0, 100000);

// Fill array with random characters
void init_string(char* buffer, int len)
{
    srand(time(NULL));
    int x = 0;
    for(x = 0; x < len; x++){
        buffer[x] = 'A' + (rand() % 26);
    }
    buffer[len-1] = 0; // add \0 to end of string
    //pc.printf("\r\n****\r\nBuffer Len = `%d`, String = `%s`\r\n****\r\n",len,buffer);
}

void flash_WR(PinName I2C_SDA, PinName I2C_SCL, int size, int eeprom_address)
{
    //I2CEeprom memory(I2C_SDA, I2C_SCL, eeprom_address, 32, 0);
    int num_read = 0;
    int num_written = 0;
    volatile char test_string[size] = {0};
    volatile char read_string[size] = {0};
    init_string((char *)test_string,size); // populate test_string with random characters
    for(int x = 0; x< size;x++){
        read_string[x] = 0;
    }
    //pc.printf("\r\n****\r\n Test String = `%s` \r\n****\r\n",test_string);

    num_written = memory.write(eeprom_address,(char *)test_string,size);
    num_read = memory.read(eeprom_address,(char *)read_string,size);

	pc.printf("\r\n****\r\n eeprom_address = 0x%x\r\n Len = %d\r\n Num Bytes Written = %d \r\n Num Bytes Read = %d \r\n Written String = %s \r\n Read String = %s \r\n****\r\n",eeprom_address,size,num_written,num_read,test_string,read_string);

	if(num_written != num_read) {
		total_failures++;
		pc.printf("\r\n****\r\n ERROR: Wrote %d bytes but only read back %d bytes...\r\n****\r\n", num_written, num_read);
		return;
	}

	if(strcmp((const char *)test_string, (const char *)read_string) != 0) {
		total_failures++;
		pc.printf("\r\n****\r\n ERROR: Strings don't match!...\r\n****\r\n ");
		return;
	}
}

// Test single byte R/W
void single_byte_WR_commands(PinName I2C_SDA, PinName I2C_SCL, int eeprom_address)
{
	srand(time(NULL));
    char test = 0;
    char read;
    int r = 0;
    int w = 0;

    for(int i = 0; i < 0x20; i++) {
    	test = i;
    	total_num_tests++;

		w = memory.write(eeprom_address,test);
		r = memory.read(eeprom_address,read);

		pc.printf("\r\n****\r\n eeprom_address = 0x%x\r\n Num Bytes Read = %d \r\n Num Bytes Written = %d \r\n Read byte = 0x%x \r\n Written Byte = 0x%x \r\n****\r\n",eeprom_address,r,w,read,test);
		if(w != r) {
			total_failures++;
			pc.printf("\r\n****\r\n ERROR: Wrote %d bytes but only read back %d bytes...\r\n****\r\n", w, r);
			return;
		}

		if(test != read) {
			total_failures++;
			pc.printf("\r\n****\r\n ERROR: Bytes don't match! Sent 0x%x, received 0x%x...\r\n****\r\n ", test, read);
			return;
		}
    }
}

// Test single byte R/W
void single_byte_WR(PinName I2C_SDA, PinName I2C_SCL, int eeprom_address)
{
	srand(time(NULL));
    char test = rand() % 0xFF;

    char read;
    int r = 0;
    int w = 0;

    total_num_tests++;

    w = memory.write(eeprom_address,test);
    r = memory.read(eeprom_address,read);

	pc.printf("\r\n****\r\n eeprom_address = 0x%x\r\n Num Bytes Read = %d \r\n Num Bytes Written = %d \r\n Read byte = 0x%x \r\n Written Byte = 0x%x \r\n****\r\n",eeprom_address,r,w,read,test);
	if(w != r) {
		total_failures++;
		pc.printf("\r\n****\r\n ERROR: Wrote %d bytes but only read back %d bytes...\r\n****\r\n", w, r);
		return;
	}

	if(test != read) {
		total_failures++;
		pc.printf("\r\n****\r\n ERROR: Bytes don't match! Sent 0x%x, received 0x%x...\r\n****\r\n ", test, read);
		return;
	}
}

int genRandAddress() {
	srand(time(NULL));
	return (rand() % 0x7FFF);
}

// Entry point into the tests
int main()
{	
	pc.printf("\r\n****\r\n Starting single byte tests... \r\n****\r\n ");
	single_byte_WR_commands(I2C_SDA, I2C_SCL, genRandAddress());
	if(1) {
		for(int i=0; i < ITERATIONS; i++) {
			total_num_tests++;	
			single_byte_WR(I2C_SDA, I2C_SCL, genRandAddress());
			pc.printf("\r\n Test iteration = %d \r\n", total_num_tests);
		}
	}
	pc.printf("\r\n****\r\n Starting multi-byte tests... \r\n****\r\n");
	if(1) {
		for(int i=0; i < ITERATIONS; i++) {
			total_num_tests++;
			flash_WR(I2C_SDA, I2C_SCL, SIZE_DATA, genRandAddress());
			pc.printf("\r\n Test iteration = %d \r\n", total_num_tests);
		}
	}
	pc.printf("\r\n****\r\n Done... \r\n****\r\n");
	pc.printf("\r\n****\r\n FAILED %d tests out of %d total tests \r\n****\r\n", total_failures, total_num_tests);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that explains it


/** Init I2C device.
* @details
Expand Down Expand Up @@ -158,4 +160,4 @@ extern int32_t fI2cReadB(i2c_t *d, char *buf, int len);
*/
extern int32_t fI2cWriteB(i2c_t *d, const char *buf, int len);

#endif /* I2C_H_ */
#endif /* I2C_H_ */
6 changes: 2 additions & 4 deletions targets/TARGET_ONSEMI/TARGET_NCS36510/i2c_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,7 @@ int i2c_byte_write(i2c_t *obj, int data)
return Count;
}

while(obj->membase->STATUS.WORD & I2C_STATUS_CMD_FIFO_OFL_BIT); /* Wait till command overflow ends */

if(obj->membase->STATUS.WORD & I2C_STATUS_BUS_ERR_BIT) {
if(I2C_BUS_ERR_CHECK) {
/* Bus error means NAK received */
return 0;
} else {
Expand All @@ -180,4 +178,4 @@ int i2c_byte_write(i2c_t *obj, int data)
}
}

#endif /* DEVICE_I2C */
#endif /* DEVICE_I2C */
40 changes: 16 additions & 24 deletions targets/TARGET_ONSEMI/TARGET_NCS36510/ncs36510_i2c.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@
/* See i2c.h for details */
void fI2cInit(i2c_t *obj,PinName sda,PinName scl)
{
uint32_t clockDivisor;
/* determine the I2C to use */
I2CName i2c_sda = (I2CName)pinmap_peripheral(sda, PinMap_I2C_SDA);
I2CName i2c_scl = (I2CName)pinmap_peripheral(scl, PinMap_I2C_SCL);
Expand Down Expand Up @@ -93,9 +92,7 @@ void fI2cInit(i2c_t *obj,PinName sda,PinName scl)
obj->membase->CR.BITS.I2C_APB_CD_EN = True;

/* set default baud rate at 100k */
clockDivisor = ((fClockGetPeriphClockfrequency() / 100000) >> 2) - 2;
obj->membase->CR.BITS.CD_VAL = (clockDivisor & I2C_CLOCKDIVEDER_VAL_MASK);
obj->membase->PRE_SCALE_REG = (clockDivisor & I2C_APB_CLK_DIVIDER_VAL_MASK) >> 5; /**< Zero pre-scale value not allowed */
fI2cFrequency(obj, 100000);

/* Cross bar setting */
pinmap_pinout(sda, PinMap_I2C_SDA);
Expand All @@ -110,8 +107,8 @@ void fI2cInit(i2c_t *obj,PinName sda,PinName scl)
PadReg_t *padRegScl = (PadReg_t*)(PADREG_BASE + (scl * PAD_REG_ADRS_BYTE_SIZE));

CLOCK_ENABLE(CLOCK_PAD);
padRegSda->PADIO0.BITS.POWER = 1; /* sda: Drive strength */
padRegScl->PADIO0.BITS.POWER = 1; /* scl: Drive strength */
padRegSda->PADIO0.BITS.POWER = 3; /* sda: Drive strength */
padRegScl->PADIO0.BITS.POWER = 3; /* scl: Drive strength */
CLOCK_DISABLE(CLOCK_PAD);

CLOCK_ENABLE(CLOCK_GPIO);
Expand Down Expand Up @@ -160,7 +157,10 @@ int32_t fI2cReadB(i2c_t *obj, char *buf, int len)
int32_t read = 0;

while (read < len) {
/* Send read command */

while(FIFO_OFL_CHECK); /* Wait till command overflow ends */

/* Send read command */
SEND_COMMAND(I2C_CMD_RDAT8);
while(!RD_DATA_READY) {
if (I2C_BUS_ERR_CHECK) {
Expand All @@ -170,16 +170,16 @@ int32_t fI2cReadB(i2c_t *obj, char *buf, int len)
}
buf[read++] = obj->membase->RD_FIFO_REG; /**< Reading 'read FIFO register' will clear status register */

if(!(read>=len)) { /* No ACK will be generated for the last read, upper level I2C protocol should generate */
SEND_COMMAND(I2C_CMD_WDAT0); /* TODO based on requirement generate ACK or NACK Based on the requirement. */
if(!(read>=len)) {
SEND_COMMAND(I2C_CMD_WDAT0);
} else {
/* No ack */
SEND_COMMAND(I2C_CMD_WDAT1);
}

/* check for FIFO underflow */
if(I2C_UFL_CHECK) {
return I2C_ERROR_NO_SLAVE; /* TODO No error available for this in i2c_api.h */
return I2C_EVENT_ERROR;
}
if(I2C_BUS_ERR_CHECK) {
/* Bus error */
Expand All @@ -196,44 +196,36 @@ int32_t fI2cWriteB(i2c_t *obj, const char *buf, int len)
int32_t write = 0;

while (write < len) {
/* Send write command */
SEND_COMMAND(I2C_CMD_WDAT8);

while(FIFO_OFL_CHECK); /* Wait till command overflow ends */

if(buf[write] == I2C_CMD_RDAT8) {
/* SW work around to counter FSM issue. If the only command in the CMD FIFO is the WDAT8 command (data of 0x13)
then as the command is read out (i.e. the FIFO goes empty), the WDAT8 command will be misinterpreted as a
RDAT8 command by the data FSM; resulting in an I2C bus error (NACK instead of an ACK). */
/* Send 0x13 bit wise */
SEND_COMMAND(I2C_CMD_WDAT0);

SEND_COMMAND(I2C_CMD_WDAT0);

SEND_COMMAND(I2C_CMD_WDAT0);

SEND_COMMAND(I2C_CMD_WDAT1);

SEND_COMMAND(I2C_CMD_WDAT0);

SEND_COMMAND(I2C_CMD_WDAT0);

SEND_COMMAND(I2C_CMD_WDAT1);

SEND_COMMAND(I2C_CMD_WDAT1);
write++;
} else {
/* Send data */
SEND_COMMAND(I2C_CMD_WDAT8);
SEND_COMMAND(buf[write++]);
}
SEND_COMMAND(I2C_CMD_VRFY_ACK); /* TODO Verify ACK based on requirement, Do we need? */
SEND_COMMAND(I2C_CMD_VRFY_ACK);

if (I2C_BUS_ERR_CHECK) {
/* Bus error */
return I2C_ERROR_BUS_BUSY;
}

while(FIFO_OFL_CHECK); /* Wait till command overflow ends */
}

return write;
}

#endif /* DEVICE_I2C */
#endif /* DEVICE_I2C */