[U-Boot] [PATCH v3 1/3] ARM: I2C: I2C Multi byte address support
T Krishnamoorthy, Balaji
balajitk at ti.com
Fri Feb 17 08:05:58 CET 2012
On Mon, Jan 23, 2012 at 3:14 PM, Patil, Rachna <rachna at ti.com> wrote:
> commit 2faa76196af4b3e93bcb9e38ed9090cbd3b06db3
> Author: Patil, Rachna <rachna at ti.com>
> Date: Sun Jan 22 23:44:12 2012 +0000
>
> ARM: I2C: I2C Multi byte address support
>
> Existing OMAP I2C driver does not support address
> length greater than one. Hence this patch is to
> add support for 2 byte address read/write.
>
> Signed-off-by: Philip, Avinash <avinashphilip at ti.com>
> Signed-off-by: Hebbar, Gururaja <gururaja.hebbar at ti.com>
> Signed-off-by: Patil, Rachna <rachna at ti.com>
>
> diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c
> index a7ffd95..80932ef 100644
> --- a/drivers/i2c/omap24xx_i2c.c
> +++ b/drivers/i2c/omap24xx_i2c.c
> @@ -29,10 +29,11 @@
>
> DECLARE_GLOBAL_DATA_PTR;
>
> -#define I2C_TIMEOUT 1000
> +#define I2C_STAT_TIMEO (1 << 31)
> +#define I2C_TIMEOUT 10
>
> -static void wait_for_bb(void);
> -static u16 wait_for_pin(void);
> +static u32 wait_for_bb(void);
> +static u32 wait_for_status_mask(u16 mask);
> static void flush_fifo(void);
>
> /*
> @@ -50,7 +51,6 @@ void i2c_init(int speed, int slaveadd)
> int psc, fsscll, fssclh;
> int hsscll = 0, hssclh = 0;
> u32 scll, sclh;
> - int timeout = I2C_TIMEOUT;
>
> /* Only handle standard, fast and high speeds */
> if ((speed != OMAP_I2C_STANDARD) &&
> @@ -112,24 +112,14 @@ void i2c_init(int speed, int slaveadd)
> sclh = (unsigned int)fssclh;
> }
>
> + if (gd->flags & GD_FLG_RELOC)
> + bus_initialized[current_bus] = 1;
> +
> if (readw(&i2c_base->con) & I2C_CON_EN) {
> writew(0, &i2c_base->con);
> udelay(50000);
> }
>
> - writew(0x2, &i2c_base->sysc); /* for ES2 after soft reset */
> - udelay(1000);
> -
> - writew(I2C_CON_EN, &i2c_base->con);
> - while (!(readw(&i2c_base->syss) & I2C_SYSS_RDONE) && timeout--) {
> - if (timeout <= 0) {
> - puts("ERROR: Timeout in soft-reset\n");
> - return;
> - }
> - udelay(1000);
> - }
> -
> - writew(0, &i2c_base->con);
This change in not related to multi byte address
and why is this removed
> writew(psc, &i2c_base->psc);
> writew(scll, &i2c_base->scll);
> writew(sclh, &i2c_base->sclh);
> @@ -145,81 +135,6 @@ void i2c_init(int speed, int slaveadd)
> flush_fifo();
> writew(0xFFFF, &i2c_base->stat);
> writew(0, &i2c_base->cnt);
> -
> - if (gd->flags & GD_FLG_RELOC)
> - bus_initialized[current_bus] = 1;
why is this section of code moved above ?
> -}
> -
> -static int i2c_read_byte(u8 devaddr, u8 regoffset, u8 *value)
> -{
> - int i2c_error = 0;
> - u16 status;
> -
> - /* wait until bus not busy */
> - wait_for_bb();
> -
> - /* one byte only */
> - writew(1, &i2c_base->cnt);
> - /* set slave address */
> - writew(devaddr, &i2c_base->sa);
> - /* no stop bit needed here */
> - writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT |
> - I2C_CON_TRX, &i2c_base->con);
> -
> - /* send register offset */
> - while (1) {
> - status = wait_for_pin();
> - if (status == 0 || status & I2C_STAT_NACK) {
> - i2c_error = 1;
> - goto read_exit;
> - }
> - if (status & I2C_STAT_XRDY) {
> - /* Important: have to use byte access */
> - writeb(regoffset, &i2c_base->data);
> - writew(I2C_STAT_XRDY, &i2c_base->stat);
> - }
> - if (status & I2C_STAT_ARDY) {
> - writew(I2C_STAT_ARDY, &i2c_base->stat);
> - break;
> - }
> - }
> -
> - /* set slave address */
> - writew(devaddr, &i2c_base->sa);
> - /* read one byte from slave */
> - writew(1, &i2c_base->cnt);
> - /* need stop bit here */
> - writew(I2C_CON_EN | I2C_CON_MST |
> - I2C_CON_STT | I2C_CON_STP,
> - &i2c_base->con);
> -
> - /* receive data */
> - while (1) {
> - status = wait_for_pin();
> - if (status == 0 || status & I2C_STAT_NACK) {
> - i2c_error = 1;
> - goto read_exit;
> - }
> - if (status & I2C_STAT_RRDY) {
> -#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) || \
> - defined(CONFIG_OMAP44XX)
Note you have removed CONFIG_OMAP44XX here
and not added in any other place in this patch
was this patch developed on latest baseline or something went wrong
while rebasing ?
> - *value = readb(&i2c_base->data);
> -#else
> - *value = readw(&i2c_base->data);
> -#endif
> - writew(I2C_STAT_RRDY, &i2c_base->stat);
> - }
> - if (status & I2C_STAT_ARDY) {
> - writew(I2C_STAT_ARDY, &i2c_base->stat);
> - break;
> - }
> - }
> -
> -read_exit:
> - flush_fifo();
> - writew(0xFFFF, &i2c_base->stat);
> - writew(0, &i2c_base->cnt);
> - return i2c_error;
> }
>
> static void flush_fifo(void)
> @@ -246,32 +161,42 @@ static void flush_fifo(void)
>
> int i2c_probe(uchar chip)
> {
> - u16 status;
> + u32 status;
> int res = 1; /* default = fail */
>
> if (chip == readw(&i2c_base->oa))
> return res;
>
> /* wait until bus not busy */
> - wait_for_bb();
> + status = wait_for_bb();
> + /* exit on BUS busy */
> + if (status & I2C_STAT_TIMEO)
> + return res;
>
> /* try to write one byte */
> writew(1, &i2c_base->cnt);
> /* set slave address */
> writew(chip, &i2c_base->sa);
> /* stop bit needed here */
> - writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX |
> - I2C_CON_STP, &i2c_base->con);
> -
> - status = wait_for_pin();
> -
> - /* check for ACK (!NAK) */
> - if (!(status & I2C_STAT_NACK))
> - res = 0;
> -
> - /* abort transfer (force idle state) */
> - writew(0, &i2c_base->con);
> -
> + writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT
> + | I2C_CON_STP, &i2c_base->con);
> + /* enough delay for the NACK bit set */
> + udelay(9000);
> +
> + if (!(readw(&i2c_base->stat) & I2C_STAT_NACK)) {
> + res = 0; /* success case */
> + flush_fifo();
> + writew(0xFFFF, &i2c_base->stat);
flush_fifo and clearing stat is anyway at the end of function
why is this repeated
> + } else {
> + /* failure, clear sources*/
> + writew(0xFFFF, &i2c_base->stat);
> + /* finish up xfer */
> + writew(readw(&i2c_base->con) | I2C_CON_STP, &i2c_base->con);
> + status = wait_for_bb();
> + /* exit on BUS busy */
> + if (status & I2C_STAT_TIMEO)
> + return res;
> + }
> flush_fifo();
> /* don't allow any more data in... we don't want it. */
> writew(0, &i2c_base->cnt);
> @@ -281,111 +206,309 @@ int i2c_probe(uchar chip)
>
> int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
> {
> - int i;
> + int i2c_error = 0, i;
> + u32 status;
> +
> + if ((alen > 2) || (alen < 0))
> + return 1;
>
> - if (alen > 1) {
> - printf("I2C read: addr len %d not supported\n", alen);
> + if (alen < 2) {
> + if (addr + len > 256)
> + return 1;
> + } else if (addr + len > 0xFFFF) {
> return 1;
> }
>
> - if (addr + len > 256) {
> - puts("I2C read: address out of range\n");
> + /* wait until bus not busy */
> + status = wait_for_bb();
> +
> + /* exit on BUS busy */
> + if (status & I2C_STAT_TIMEO)
> return 1;
> +
> + writew((alen & 0xFF), &i2c_base->cnt);
> + /* set slave address */
> + writew(chip, &i2c_base->sa);
> + /* Clear the Tx & Rx FIFOs */
> + writew((readw(&i2c_base->buf) | I2C_RXFIFO_CLEAR |
> + I2C_TXFIFO_CLEAR), &i2c_base->buf);
> + /* no stop bit needed here */
> + writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_TRX |
> + I2C_CON_STT, &i2c_base->con);
> +
> + /* wait for Transmit ready condition */
> + status = wait_for_status_mask(I2C_STAT_XRDY | I2C_STAT_NACK);
> +
> + if (status & (I2C_STAT_NACK | I2C_STAT_TIMEO))
> + i2c_error = 1;
> +
> + if (!i2c_error) {
> + if (status & I2C_STAT_XRDY) {
> + switch (alen) {
> + case 2:
> + /* Send address MSByte */
> +#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)
> + writew(((addr >> 8) & 0xFF), &i2c_base->data);
writew or writeb??
> +
> + /* Clearing XRDY event */
> + writew((status & I2C_STAT_XRDY),
> + &i2c_base->stat);
> + /* wait for Transmit ready condition */
> + status = wait_for_status_mask(I2C_STAT_XRDY |
> + I2C_STAT_NACK);
> +
> + if (status & (I2C_STAT_NACK |
> + I2C_STAT_TIMEO)) {
> + i2c_error = 1;
> + break;
> + }
> +#endif
> + case 1:
> +#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)
> + /* Send address LSByte */
> + writew((addr & 0xFF), &i2c_base->data);
writew or writeb??
> +#else
> + /* Send address Short word */
> + writew((addr & 0xFFFF), &i2c_base->data);
> +#endif
> + /* Clearing XRDY event */
> + writew((status & I2C_STAT_XRDY),
> + &i2c_base->stat);
> + /*wait for Transmit ready condition */
> + status = wait_for_status_mask(I2C_STAT_ARDY |
> + I2C_STAT_NACK);
> +
> + if (status & (I2C_STAT_NACK |
> + I2C_STAT_TIMEO)) {
> + i2c_error = 1;
> + break;
> + }
> + }
> + } else
> + i2c_error = 1;
> }
>
> - for (i = 0; i < len; i++) {
> - if (i2c_read_byte(chip, addr + i, &buffer[i])) {
> - puts("I2C read: I/O error\n");
> - i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
> - return 1;
> + /* Wait for ARDY to set */
> + status = wait_for_status_mask(I2C_STAT_ARDY | I2C_STAT_NACK
> + | I2C_STAT_AL);
> +
> + if (!i2c_error) {
> + /* set slave address */
> + writew(chip, &i2c_base->sa);
> + writew((len & 0xFF), &i2c_base->cnt);
> + /* Clear the Tx & Rx FIFOs */
> + writew((readw(&i2c_base->buf) | I2C_RXFIFO_CLEAR |
> + I2C_TXFIFO_CLEAR), &i2c_base->buf);
> + /* need stop bit here */
> + writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_STP,
> + &i2c_base->con);
> +
> + for (i = 0; i < len; i++) {
> + /* wait for Receive condition */
> + status = wait_for_status_mask(I2C_STAT_RRDY |
> + I2C_STAT_NACK);
> + if (status & (I2C_STAT_NACK | I2C_STAT_TIMEO)) {
> + i2c_error = 1;
> + break;
> + }
> +
> + if (status & I2C_STAT_RRDY) {
> +#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)
> + buffer[i] = readb(&i2c_base->data);
> +#else
> + *((u16 *)&buffer[i]) =
> + readw(&i2c_base->data) & 0xFFFF;
> + i++;
> +#endif
> + writew((status & I2C_STAT_RRDY),
> + &i2c_base->stat);
> + udelay(1000);
> + } else {
> + i2c_error = 1;
> + }
> }
> }
>
> + /* Wait for ARDY to set */
> + status = wait_for_status_mask(I2C_STAT_ARDY | I2C_STAT_NACK
> + | I2C_STAT_AL);
why do you have to wait for ARDY when transaction is done?
> +
> + if (i2c_error) {
> + writew(0, &i2c_base->con);
> + return 1;
> + }
> +
> + writew(I2C_CON_EN, &i2c_base->con);
> +
> + while (readw(&i2c_base->stat)
> + || (readw(&i2c_base->con) & I2C_CON_MST)) {
> + udelay(10000);
> + writew(0xFFFF, &i2c_base->stat);
> + }
> +
> + writew(I2C_CON_EN, &i2c_base->con);
> + flush_fifo();
> + writew(0xFFFF, &i2c_base->stat);
> + writew(0, &i2c_base->cnt);
> +
> return 0;
> }
>
> int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
> {
> - int i;
> - u16 status;
> - int i2c_error = 0;
>
> - if (alen > 1) {
> - printf("I2C write: addr len %d not supported\n", alen);
> + int i, i2c_error = 0;
> + u32 status;
> + u16 writelen;
> +
> + if (alen > 2)
> return 1;
> - }
>
> - if (addr + len > 256) {
> - printf("I2C write: address 0x%x + 0x%x out of range\n",
> - addr, len);
> + if (alen < 2) {
> + if (addr + len > 256)
> + return 1;
> + } else if (addr + len > 0xFFFF) {
> return 1;
> }
>
> /* wait until bus not busy */
> - wait_for_bb();
> + status = wait_for_bb();
> +
> + /* exiting on BUS busy */
> + if (status & I2C_STAT_TIMEO)
> + return 1;
>
> - /* start address phase - will write regoffset + len bytes data */
> - /* TODO consider case when !CONFIG_OMAP243X/34XX/44XX */
> - writew(alen + len, &i2c_base->cnt);
> + writelen = (len & 0xFFFF) + alen;
> +
> + /* two bytes */
> + writew((writelen & 0xFFFF), &i2c_base->cnt);
where else is this variable writelen used ?
> + /* Clear the Tx & Rx FIFOs */
> + writew((readw(&i2c_base->buf) | I2C_RXFIFO_CLEAR |
> + I2C_TXFIFO_CLEAR), &i2c_base->buf);
> /* set slave address */
> writew(chip, &i2c_base->sa);
> /* stop bit needed here */
> writew(I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | I2C_CON_TRX |
> I2C_CON_STP, &i2c_base->con);
>
> - /* Send address byte */
> - status = wait_for_pin();
> + /* wait for Transmit ready condition */
> + status = wait_for_status_mask(I2C_STAT_XRDY | I2C_STAT_NACK);
>
> - if (status == 0 || status & I2C_STAT_NACK) {
> + if (status & (I2C_STAT_NACK | I2C_STAT_TIMEO))
> i2c_error = 1;
> - printf("error waiting for i2c address ACK (status=0x%x)\n",
> - status);
> - goto write_exit;
> - }
>
> - if (status & I2C_STAT_XRDY) {
> - writeb(addr & 0xFF, &i2c_base->data);
> - writew(I2C_STAT_XRDY, &i2c_base->stat);
> - } else {
> - i2c_error = 1;
> - printf("i2c bus not ready for transmit (status=0x%x)\n",
> - status);
> - goto write_exit;
> - }
> + if (!i2c_error) {
> + if (status & I2C_STAT_XRDY) {
> + switch (alen) {
> +#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)
> + case 2:
what is the logical reason behind keeping case statement under
compilation flag ?
> + /* send out MSB byte */
> + writeb(((addr >> 8) & 0xFF), &i2c_base->data);
> +#else
> + writeb((addr & 0xFFFF), &i2c_base->data);
bit field is masked for 16 bits but byte write is used !!
> + break;
> +#endif
> + /* Clearing XRDY event */
> + writew((status & I2C_STAT_XRDY),
> + &i2c_base->stat);
> + /*waiting for Transmit ready * condition */
> + status = wait_for_status_mask(I2C_STAT_XRDY |
> + I2C_STAT_NACK);
> +
> + if (status & (I2C_STAT_NACK | I2C_STAT_TIMEO)) {
> + i2c_error = 1;
> + break;
> + }
This should be in #if part for better readablity
> + case 1:
> +#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)
> + /* send out MSB byte */
> + writeb((addr & 0xFF), &i2c_base->data);
> +#else
> + writew(((buffer[0] << 8) | (addr & 0xFF)),
> + &i2c_base->data);
can you explain the logic behind using buffer[0]
> +#endif
> + }
>
> - /* address phase is over, now write data */
> - for (i = 0; i < len; i++) {
> - status = wait_for_pin();
> + /* Clearing XRDY event */
> + writew((status & I2C_STAT_XRDY), &i2c_base->stat);
> + }
> +
> + /* waiting for Transmit ready condition */
> + status = wait_for_status_mask(I2C_STAT_XRDY | I2C_STAT_NACK);
>
> - if (status == 0 || status & I2C_STAT_NACK) {
> + if (status & (I2C_STAT_NACK | I2C_STAT_TIMEO))
> i2c_error = 1;
> - printf("i2c error waiting for data ACK (status=0x%x)\n",
> - status);
> - goto write_exit;
> +
> + if (!i2c_error) {
> + for (i = ((alen > 1) ? 0 : 1); i < len; i++) {
this is again not correct
Did you test this patch for alen = 1 ?
> + if (status & I2C_STAT_XRDY) {
> +#if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)
> + writeb((buffer[i] & 0xFF),
> + &i2c_base->data);
> +#else
> + writew((((buffer[i] << 8) |
> + buffer[i + 1]) & 0xFFFF),
> + &i2c_base->data);
> + i++;
> +#endif
> + } else
> + i2c_error = 1;
> + /* Clearing XRDY event */
> + writew((status & I2C_STAT_XRDY),
> + &i2c_base->stat);
wrong indent
> + /* waiting for XRDY condition */
> + status = wait_for_status_mask(
> + I2C_STAT_XRDY |
> + I2C_STAT_ARDY |
> + I2C_STAT_NACK);
> + if (status & (I2C_STAT_NACK |
> + I2C_STAT_TIMEO)) {
> + i2c_error = 1;
> + break;
> + }
> + if (status & I2C_STAT_ARDY)
> + break;
> + }
> }
> + }
>
> - if (status & I2C_STAT_XRDY) {
> - writeb(buffer[i], &i2c_base->data);
> - writew(I2C_STAT_XRDY, &i2c_base->stat);
> - } else {
> - i2c_error = 1;
> - printf("i2c bus not ready for Tx (i=%d)\n", i);
> - goto write_exit;
> + status = wait_for_status_mask(I2C_STAT_ARDY | I2C_STAT_NACK |
> + I2C_STAT_AL);
why do have to wait for ARDY here?
> +
> + if (status & (I2C_STAT_NACK | I2C_STAT_TIMEO))
> + i2c_error = 1;
> +
> + if (i2c_error) {
> + writew(0, &i2c_base->con);
> + return 1;
> + }
> +
> + if (!i2c_error) {
> + int eout = 200;
> +
> + writew(I2C_CON_EN, &i2c_base->con);
> + while ((status = readw(&i2c_base->stat)) ||
> + (readw(&i2c_base->con) & I2C_CON_MST)) {
is this a workaround or errata ? if yes can you please add some comment?
why should MST bit get set after writing I2C_CON_EN ??
> + udelay(1000);
> + /* have to read to clear intrrupt */
wrong comment and typo
This patch series is not really tested for other platform
like (CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX)
and the patch is buggy for OMAP4430 panda
<snip>
More information about the U-Boot
mailing list