* [PATCH] i2c: fix i2c-sh_mobile timing issues
@ 2008-08-27 9:33 Magnus Damm
2008-08-27 11:09 ` Jean Delvare
2008-08-27 14:40 ` Paul Mundt
0 siblings, 2 replies; 6+ messages in thread
From: Magnus Damm @ 2008-08-27 9:33 UTC (permalink / raw)
To: i2c; +Cc: khali, Magnus Damm, lethal, linux-sh
From: Magnus Damm <damm@igel.co.jp>
This patch teaches the i2c-sh_mobile driver to make use of wait irqs.
Without this patch only dte irqs are used which may lead to overruns
and cases of missing stop and extra bytes being read on the i2c bus.
Use of wait irqs forces the hardware to pause and wait until the cpu
is ready. Polling is also reworked in this patch to fix ms delay issues.
Verified with bus analyzer and tested on MigoR and AP325RXA boards.
Signed-off-by: Magnus Damm <damm@igel.co.jp>
---
Paul, can you merge this for 2.6.27?
drivers/i2c/busses/i2c-sh_mobile.c | 271 +++++++++++++++++++++++++++---------
1 file changed, 208 insertions(+), 63 deletions(-)
--- 0001/drivers/i2c/busses/i2c-sh_mobile.c
+++ work/drivers/i2c/busses/i2c-sh_mobile.c 2008-08-13 17:40:23.000000000 +0900
@@ -31,13 +31,84 @@
#include <linux/clk.h>
#include <linux/io.h>
+/* Transmit operation: */
+/* */
+/* 0 byte transmit */
+/* BUS: S A8 ACK P */
+/* IRQ: DTE WAIT */
+/* ICIC: */
+/* ICCR: 0x94 0x90 */
+/* ICDR: A8 */
+/* */
+/* 1 byte transmit */
+/* BUS: S A8 ACK D8(1) ACK P */
+/* IRQ: DTE WAIT WAIT */
+/* ICIC: -DTE */
+/* ICCR: 0x94 0x90 */
+/* ICDR: A8 D8(1) */
+/* */
+/* 2 byte transmit */
+/* BUS: S A8 ACK D8(1) ACK D8(2) ACK P */
+/* IRQ: DTE WAIT WAIT WAIT */
+/* ICIC: -DTE */
+/* ICCR: 0x94 0x90 */
+/* ICDR: A8 D8(1) D8(2) */
+/* */
+/* 3 bytes or more, +---------+ gets repeated */
+/* */
+/* */
+/* Receive operation: */
+/* */
+/* 0 byte receive - not supported since slave may hold SDA low */
+/* */
+/* 1 byte receive [TX] | [RX] */
+/* BUS: S A8 ACK | D8(1) ACK P */
+/* IRQ: DTE WAIT | WAIT DTE */
+/* ICIC: -DTE | +DTE */
+/* ICCR: 0x94 0x81 | 0xc0 */
+/* ICDR: A8 | D8(1) */
+/* */
+/* 2 byte receive [TX]| [RX] */
+/* BUS: S A8 ACK | D8(1) ACK D8(2) ACK P */
+/* IRQ: DTE WAIT | WAIT WAIT DTE */
+/* ICIC: -DTE | +DTE */
+/* ICCR: 0x94 0x81 | 0xc0 */
+/* ICDR: A8 | D8(1) D8(2) */
+/* */
+/* 3 byte receive [TX] | [RX] */
+/* BUS: S A8 ACK | D8(1) ACK D8(2) ACK D8(3) ACK P */
+/* IRQ: DTE WAIT | WAIT WAIT WAIT DTE */
+/* ICIC: -DTE | +DTE */
+/* ICCR: 0x94 0x81 | 0xc0 */
+/* ICDR: A8 | D8(1) D8(2) D8(3) */
+/* */
+/* 4 bytes or more, this part is repeated +---------+ */
+/* */
+/* */
+/* Interrupt order and BUSY flag */
+/* ___ _ */
+/* SDA ___\___XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXAAAAAAAAA___/ */
+/* SCL \_/1\_/2\_/3\_/4\_/5\_/6\_/7\_/8\___/9\_____/ */
+/* */
+/* S D7 D6 D5 D4 D3 D2 D1 D0 P */
+/* ___ */
+/* WAIT IRQ ________________________________/ \___________ */
+/* TACK IRQ ____________________________________/ \_______ */
+/* DTE IRQ __________________________________________/ \_ */
+/* AL IRQ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX */
+/* _______________________________________________ */
+/* BUSY __/ \_ */
+/* */
+
enum sh_mobile_i2c_op {
OP_START = 0,
- OP_TX_ONLY,
+ OP_TX_FIRST,
+ OP_TX,
OP_TX_STOP,
OP_TX_TO_RX,
- OP_RX_ONLY,
+ OP_RX,
OP_RX_STOP,
+ OP_RX_STOP_DATA,
};
struct sh_mobile_i2c_data {
@@ -127,25 +198,34 @@ static unsigned char i2c_op(struct sh_mo
spin_lock_irqsave(&pd->lock, flags);
switch (op) {
- case OP_START:
+ case OP_START: /* issue start and trigger DTE interrupt */
iowrite8(0x94, ICCR(pd));
break;
- case OP_TX_ONLY:
+ case OP_TX_FIRST: /* disable DTE interrupt and write data */
+ iowrite8(ICIC_WAITE | ICIC_ALE | ICIC_TACKE, ICIC(pd));
iowrite8(data, ICDR(pd));
break;
- case OP_TX_STOP:
+ case OP_TX: /* write data */
iowrite8(data, ICDR(pd));
- iowrite8(0x90, ICCR(pd));
- iowrite8(ICIC_ALE | ICIC_TACKE, ICIC(pd));
break;
- case OP_TX_TO_RX:
+ case OP_TX_STOP: /* write data and issue a stop afterwards */
iowrite8(data, ICDR(pd));
+ iowrite8(0x90, ICCR(pd));
+ break;
+ case OP_TX_TO_RX: /* select read mode */
iowrite8(0x81, ICCR(pd));
break;
- case OP_RX_ONLY:
+ case OP_RX: /* just read data */
ret = ioread8(ICDR(pd));
break;
- case OP_RX_STOP:
+ case OP_RX_STOP: /* enable DTE interrupt, issue stop */
+ iowrite8(ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE,
+ ICIC(pd));
+ iowrite8(0xc0, ICCR(pd));
+ break;
+ case OP_RX_STOP_DATA: /* enable DTE interrupt, read data, issue stop */
+ iowrite8(ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE,
+ ICIC(pd));
ret = ioread8(ICDR(pd));
iowrite8(0xc0, ICCR(pd));
break;
@@ -157,58 +237,120 @@ static unsigned char i2c_op(struct sh_mo
return ret;
}
+static int sh_mobile_i2c_is_first_byte(struct sh_mobile_i2c_data *pd)
+{
+ if (pd->pos = -1)
+ return 1;
+
+ return 0;
+}
+
+static int sh_mobile_i2c_is_last_byte(struct sh_mobile_i2c_data *pd)
+{
+ if (pd->pos = (pd->msg->len - 1))
+ return 1;
+
+ return 0;
+}
+
+static void sh_mobile_i2c_get_data(struct sh_mobile_i2c_data *pd,
+ unsigned char *buf)
+{
+ switch (pd->pos) {
+ case -1:
+ *buf = (pd->msg->addr & 0x7f) << 1;
+ *buf |= (pd->msg->flags & I2C_M_RD) ? 1 : 0;
+ break;
+ default:
+ *buf = pd->msg->buf[pd->pos];
+ }
+}
+
+static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd)
+{
+ unsigned char data;
+
+ if (pd->pos = pd->msg->len)
+ return 1;
+
+ sh_mobile_i2c_get_data(pd, &data);
+
+ if (sh_mobile_i2c_is_last_byte(pd))
+ i2c_op(pd, OP_TX_STOP, data);
+ else if (sh_mobile_i2c_is_first_byte(pd))
+ i2c_op(pd, OP_TX_FIRST, data);
+ else
+ i2c_op(pd, OP_TX, data);
+
+ pd->pos++;
+ return 0;
+}
+
+static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
+{
+ unsigned char data;
+ int real_pos;
+
+ do {
+ if (pd->pos <= -1) {
+ sh_mobile_i2c_get_data(pd, &data);
+
+ if (sh_mobile_i2c_is_first_byte(pd))
+ i2c_op(pd, OP_TX_FIRST, data);
+ else
+ i2c_op(pd, OP_TX, data);
+ break;
+ }
+
+ if (pd->pos = 0) {
+ i2c_op(pd, OP_TX_TO_RX, 0);
+ break;
+ }
+
+ real_pos = pd->pos - 2;
+
+ if (pd->pos = pd->msg->len) {
+ if (real_pos < 0) {
+ i2c_op(pd, OP_RX_STOP, 0);
+ break;
+ }
+ data = i2c_op(pd, OP_RX_STOP_DATA, 0);
+ } else
+ data = i2c_op(pd, OP_RX, 0);
+
+ pd->msg->buf[real_pos] = data;
+ } while (0);
+
+ pd->pos++;
+ return pd->pos = (pd->msg->len + 2);
+}
+
static irqreturn_t sh_mobile_i2c_isr(int irq, void *dev_id)
{
struct platform_device *dev = dev_id;
struct sh_mobile_i2c_data *pd = platform_get_drvdata(dev);
- struct i2c_msg *msg = pd->msg;
- unsigned char data, sr;
- int wakeup = 0;
+ unsigned char sr;
+ int wakeup;
sr = ioread8(ICSR(pd));
- pd->sr |= sr;
+ pd->sr |= sr; /* remember state */
dev_dbg(pd->dev, "i2c_isr 0x%02x 0x%02x %s %d %d!\n", sr, pd->sr,
- (msg->flags & I2C_M_RD) ? "read" : "write",
- pd->pos, msg->len);
+ (pd->msg->flags & I2C_M_RD) ? "read" : "write",
+ pd->pos, pd->msg->len);
if (sr & (ICSR_AL | ICSR_TACK)) {
- iowrite8(0, ICIC(pd)); /* disable interrupts */
- wakeup = 1;
- goto do_wakeup;
- }
-
- if (pd->pos = msg->len) {
- i2c_op(pd, OP_RX_ONLY, 0);
- wakeup = 1;
- goto do_wakeup;
- }
-
- if (pd->pos = -1) {
- data = (msg->addr & 0x7f) << 1;
- data |= (msg->flags & I2C_M_RD) ? 1 : 0;
- } else
- data = msg->buf[pd->pos];
-
- if ((pd->pos = -1) || !(msg->flags & I2C_M_RD)) {
- if (msg->flags & I2C_M_RD)
- i2c_op(pd, OP_TX_TO_RX, data);
- else if (pd->pos = (msg->len - 1)) {
- i2c_op(pd, OP_TX_STOP, data);
- wakeup = 1;
- } else
- i2c_op(pd, OP_TX_ONLY, data);
- } else {
- if (pd->pos = (msg->len - 1))
- data = i2c_op(pd, OP_RX_STOP, 0);
- else
- data = i2c_op(pd, OP_RX_ONLY, 0);
+ /* don't interrupt transaction - continue to issue stop */
+ iowrite8(sr & ~(ICSR_AL | ICSR_TACK), ICSR(pd));
+ wakeup = 0;
+ } else if (pd->msg->flags & I2C_M_RD)
+ wakeup = sh_mobile_i2c_isr_rx(pd);
+ else
+ wakeup = sh_mobile_i2c_isr_tx(pd);
- msg->buf[pd->pos] = data;
- }
- pd->pos++;
+ if (sr & ICSR_WAIT) /* TODO: add delay here to support slow acks */
+ iowrite8(sr & ~ICSR_WAIT, ICSR(pd));
- do_wakeup:
if (wakeup) {
pd->sr |= SW_DONE;
wake_up(&pd->wait);
@@ -219,6 +361,11 @@ static irqreturn_t sh_mobile_i2c_isr(int
static int start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg)
{
+ if (usr_msg->len = 0 && (usr_msg->flags & I2C_M_RD)) {
+ dev_err(pd->dev, "Unsupported zero length i2c read\n");
+ return -EIO;
+ }
+
/* Initialize channel registers */
iowrite8(ioread8(ICCR(pd)) & ~ICCR_ICE, ICCR(pd));
@@ -233,9 +380,8 @@ static int start_ch(struct sh_mobile_i2c
pd->pos = -1;
pd->sr = 0;
- /* Enable all interrupts except wait */
- iowrite8(ioread8(ICIC(pd)) | ICIC_ALE | ICIC_TACKE | ICIC_DTEE,
- ICIC(pd));
+ /* Enable all interrupts to begin with */
+ iowrite8(ICIC_WAITE | ICIC_ALE | ICIC_TACKE | ICIC_DTEE, ICIC(pd));
return 0;
}
@@ -268,25 +414,18 @@ static int sh_mobile_i2c_xfer(struct i2c
if (!k)
dev_err(pd->dev, "Transfer request timed out\n");
- retry_count = 10;
+ retry_count = 1000;
again:
val = ioread8(ICSR(pd));
dev_dbg(pd->dev, "val 0x%02x pd->sr 0x%02x\n", val, pd->sr);
- if ((val | pd->sr) & (ICSR_TACK | ICSR_AL)) {
- err = -EIO;
- break;
- }
-
/* the interrupt handler may wake us up before the
* transfer is finished, so poll the hardware
* until we're done.
*/
-
- if (!(!(val & ICSR_BUSY) && (val & ICSR_SCLM) &&
- (val & ICSR_SDAM))) {
- msleep(1);
+ if (val & ICSR_BUSY) {
+ udelay(10);
if (retry_count--)
goto again;
@@ -294,6 +433,12 @@ again:
dev_err(pd->dev, "Polling timed out\n");
break;
}
+
+ /* handle missing acknowledge and arbitration lost */
+ if ((val | pd->sr) & (ICSR_TACK | ICSR_AL)) {
+ err = -EIO;
+ break;
+ }
}
deactivate_ch(pd);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: fix i2c-sh_mobile timing issues
2008-08-27 9:33 [PATCH] i2c: fix i2c-sh_mobile timing issues Magnus Damm
@ 2008-08-27 11:09 ` Jean Delvare
2008-08-28 11:57 ` Magnus Damm
2008-08-27 14:40 ` Paul Mundt
1 sibling, 1 reply; 6+ messages in thread
From: Jean Delvare @ 2008-08-27 11:09 UTC (permalink / raw)
To: Magnus Damm; +Cc: i2c, lethal, linux-sh, Ben Dooks
Hi Magnus,
On Wed, 27 Aug 2008 18:33:56 +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
>
> This patch teaches the i2c-sh_mobile driver to make use of wait irqs.
> Without this patch only dte irqs are used which may lead to overruns
> and cases of missing stop and extra bytes being read on the i2c bus.
>
> Use of wait irqs forces the hardware to pause and wait until the cpu
> is ready. Polling is also reworked in this patch to fix ms delay issues.
>
> Verified with bus analyzer and tested on MigoR and AP325RXA boards.
>
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
>
> Paul, can you merge this for 2.6.27?
>
> drivers/i2c/busses/i2c-sh_mobile.c | 271 +++++++++++++++++++++++++++---------
> 1 file changed, 208 insertions(+), 63 deletions(-)
>
I'm not sure why I am Cc'd on this but Ben Dooks isn't. Ben is the one
in charge of i2c bus drivers for embedded system. I'm adding him.
> --- 0001/drivers/i2c/busses/i2c-sh_mobile.c
> +++ work/drivers/i2c/busses/i2c-sh_mobile.c 2008-08-13 17:40:23.000000000 +0900
> @@ -31,13 +31,84 @@
> #include <linux/clk.h>
> #include <linux/io.h>
>
> +/* Transmit operation: */
> +/* */
> +/* 0 byte transmit */
> +/* BUS: S A8 ACK P */
> +/* IRQ: DTE WAIT */
> +/* ICIC: */
> +/* ICCR: 0x94 0x90 */
> +/* ICDR: A8 */
> +/* */
> +/* 1 byte transmit */
> +/* BUS: S A8 ACK D8(1) ACK P */
> +/* IRQ: DTE WAIT WAIT */
> +/* ICIC: -DTE */
> +/* ICCR: 0x94 0x90 */
> +/* ICDR: A8 D8(1) */
> +/* */
> +/* 2 byte transmit */
> +/* BUS: S A8 ACK D8(1) ACK D8(2) ACK P */
> +/* IRQ: DTE WAIT WAIT WAIT */
> +/* ICIC: -DTE */
> +/* ICCR: 0x94 0x90 */
> +/* ICDR: A8 D8(1) D8(2) */
> +/* */
> +/* 3 bytes or more, +---------+ gets repeated */
> +/* */
> +/* */
> +/* Receive operation: */
> +/* */
> +/* 0 byte receive - not supported since slave may hold SDA low */
> +/* */
> +/* 1 byte receive [TX] | [RX] */
> +/* BUS: S A8 ACK | D8(1) ACK P */
> +/* IRQ: DTE WAIT | WAIT DTE */
> +/* ICIC: -DTE | +DTE */
> +/* ICCR: 0x94 0x81 | 0xc0 */
> +/* ICDR: A8 | D8(1) */
> +/* */
> +/* 2 byte receive [TX]| [RX] */
> +/* BUS: S A8 ACK | D8(1) ACK D8(2) ACK P */
> +/* IRQ: DTE WAIT | WAIT WAIT DTE */
> +/* ICIC: -DTE | +DTE */
> +/* ICCR: 0x94 0x81 | 0xc0 */
> +/* ICDR: A8 | D8(1) D8(2) */
> +/* */
> +/* 3 byte receive [TX] | [RX] */
> +/* BUS: S A8 ACK | D8(1) ACK D8(2) ACK D8(3) ACK P */
> +/* IRQ: DTE WAIT | WAIT WAIT WAIT DTE */
> +/* ICIC: -DTE | +DTE */
> +/* ICCR: 0x94 0x81 | 0xc0 */
> +/* ICDR: A8 | D8(1) D8(2) D8(3) */
> +/* */
> +/* 4 bytes or more, this part is repeated +---------+ */
> +/* */
> +/* */
> +/* Interrupt order and BUSY flag */
> +/* ___ _ */
> +/* SDA ___\___XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXAAAAAAAAA___/ */
> +/* SCL \_/1\_/2\_/3\_/4\_/5\_/6\_/7\_/8\___/9\_____/ */
> +/* */
> +/* S D7 D6 D5 D4 D3 D2 D1 D0 P */
> +/* ___ */
> +/* WAIT IRQ ________________________________/ \___________ */
> +/* TACK IRQ ____________________________________/ \_______ */
> +/* DTE IRQ __________________________________________/ \_ */
> +/* AL IRQ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX */
> +/* _______________________________________________ */
> +/* BUSY __/ \_ */
> +/* */
> +
> enum sh_mobile_i2c_op {
> OP_START = 0,
> - OP_TX_ONLY,
> + OP_TX_FIRST,
> + OP_TX,
> OP_TX_STOP,
> OP_TX_TO_RX,
> - OP_RX_ONLY,
> + OP_RX,
> OP_RX_STOP,
> + OP_RX_STOP_DATA,
> };
>
> struct sh_mobile_i2c_data {
> @@ -127,25 +198,34 @@ static unsigned char i2c_op(struct sh_mo
> spin_lock_irqsave(&pd->lock, flags);
>
> switch (op) {
> - case OP_START:
> + case OP_START: /* issue start and trigger DTE interrupt */
> iowrite8(0x94, ICCR(pd));
> break;
> - case OP_TX_ONLY:
> + case OP_TX_FIRST: /* disable DTE interrupt and write data */
> + iowrite8(ICIC_WAITE | ICIC_ALE | ICIC_TACKE, ICIC(pd));
> iowrite8(data, ICDR(pd));
> break;
> - case OP_TX_STOP:
> + case OP_TX: /* write data */
> iowrite8(data, ICDR(pd));
> - iowrite8(0x90, ICCR(pd));
> - iowrite8(ICIC_ALE | ICIC_TACKE, ICIC(pd));
> break;
> - case OP_TX_TO_RX:
> + case OP_TX_STOP: /* write data and issue a stop afterwards */
> iowrite8(data, ICDR(pd));
> + iowrite8(0x90, ICCR(pd));
> + break;
> + case OP_TX_TO_RX: /* select read mode */
> iowrite8(0x81, ICCR(pd));
> break;
> - case OP_RX_ONLY:
> + case OP_RX: /* just read data */
> ret = ioread8(ICDR(pd));
> break;
> - case OP_RX_STOP:
> + case OP_RX_STOP: /* enable DTE interrupt, issue stop */
> + iowrite8(ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE,
> + ICIC(pd));
> + iowrite8(0xc0, ICCR(pd));
> + break;
> + case OP_RX_STOP_DATA: /* enable DTE interrupt, read data, issue stop */
> + iowrite8(ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE,
> + ICIC(pd));
> ret = ioread8(ICDR(pd));
> iowrite8(0xc0, ICCR(pd));
> break;
> @@ -157,58 +237,120 @@ static unsigned char i2c_op(struct sh_mo
> return ret;
> }
>
> +static int sh_mobile_i2c_is_first_byte(struct sh_mobile_i2c_data *pd)
> +{
> + if (pd->pos = -1)
> + return 1;
> +
> + return 0;
> +}
> +
> +static int sh_mobile_i2c_is_last_byte(struct sh_mobile_i2c_data *pd)
> +{
> + if (pd->pos = (pd->msg->len - 1))
> + return 1;
> +
> + return 0;
> +}
> +
> +static void sh_mobile_i2c_get_data(struct sh_mobile_i2c_data *pd,
> + unsigned char *buf)
> +{
> + switch (pd->pos) {
> + case -1:
> + *buf = (pd->msg->addr & 0x7f) << 1;
> + *buf |= (pd->msg->flags & I2C_M_RD) ? 1 : 0;
> + break;
> + default:
> + *buf = pd->msg->buf[pd->pos];
> + }
> +}
> +
> +static int sh_mobile_i2c_isr_tx(struct sh_mobile_i2c_data *pd)
> +{
> + unsigned char data;
> +
> + if (pd->pos = pd->msg->len)
> + return 1;
> +
> + sh_mobile_i2c_get_data(pd, &data);
> +
> + if (sh_mobile_i2c_is_last_byte(pd))
> + i2c_op(pd, OP_TX_STOP, data);
> + else if (sh_mobile_i2c_is_first_byte(pd))
> + i2c_op(pd, OP_TX_FIRST, data);
> + else
> + i2c_op(pd, OP_TX, data);
> +
> + pd->pos++;
> + return 0;
> +}
> +
> +static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data *pd)
> +{
> + unsigned char data;
> + int real_pos;
> +
> + do {
> + if (pd->pos <= -1) {
> + sh_mobile_i2c_get_data(pd, &data);
> +
> + if (sh_mobile_i2c_is_first_byte(pd))
> + i2c_op(pd, OP_TX_FIRST, data);
> + else
> + i2c_op(pd, OP_TX, data);
> + break;
> + }
> +
> + if (pd->pos = 0) {
> + i2c_op(pd, OP_TX_TO_RX, 0);
> + break;
> + }
> +
> + real_pos = pd->pos - 2;
> +
> + if (pd->pos = pd->msg->len) {
> + if (real_pos < 0) {
> + i2c_op(pd, OP_RX_STOP, 0);
> + break;
> + }
> + data = i2c_op(pd, OP_RX_STOP_DATA, 0);
> + } else
> + data = i2c_op(pd, OP_RX, 0);
> +
> + pd->msg->buf[real_pos] = data;
> + } while (0);
> +
> + pd->pos++;
> + return pd->pos = (pd->msg->len + 2);
> +}
> +
> static irqreturn_t sh_mobile_i2c_isr(int irq, void *dev_id)
> {
> struct platform_device *dev = dev_id;
> struct sh_mobile_i2c_data *pd = platform_get_drvdata(dev);
> - struct i2c_msg *msg = pd->msg;
> - unsigned char data, sr;
> - int wakeup = 0;
> + unsigned char sr;
> + int wakeup;
>
> sr = ioread8(ICSR(pd));
> - pd->sr |= sr;
> + pd->sr |= sr; /* remember state */
>
> dev_dbg(pd->dev, "i2c_isr 0x%02x 0x%02x %s %d %d!\n", sr, pd->sr,
> - (msg->flags & I2C_M_RD) ? "read" : "write",
> - pd->pos, msg->len);
> + (pd->msg->flags & I2C_M_RD) ? "read" : "write",
> + pd->pos, pd->msg->len);
>
> if (sr & (ICSR_AL | ICSR_TACK)) {
> - iowrite8(0, ICIC(pd)); /* disable interrupts */
> - wakeup = 1;
> - goto do_wakeup;
> - }
> -
> - if (pd->pos = msg->len) {
> - i2c_op(pd, OP_RX_ONLY, 0);
> - wakeup = 1;
> - goto do_wakeup;
> - }
> -
> - if (pd->pos = -1) {
> - data = (msg->addr & 0x7f) << 1;
> - data |= (msg->flags & I2C_M_RD) ? 1 : 0;
> - } else
> - data = msg->buf[pd->pos];
> -
> - if ((pd->pos = -1) || !(msg->flags & I2C_M_RD)) {
> - if (msg->flags & I2C_M_RD)
> - i2c_op(pd, OP_TX_TO_RX, data);
> - else if (pd->pos = (msg->len - 1)) {
> - i2c_op(pd, OP_TX_STOP, data);
> - wakeup = 1;
> - } else
> - i2c_op(pd, OP_TX_ONLY, data);
> - } else {
> - if (pd->pos = (msg->len - 1))
> - data = i2c_op(pd, OP_RX_STOP, 0);
> - else
> - data = i2c_op(pd, OP_RX_ONLY, 0);
> + /* don't interrupt transaction - continue to issue stop */
> + iowrite8(sr & ~(ICSR_AL | ICSR_TACK), ICSR(pd));
> + wakeup = 0;
> + } else if (pd->msg->flags & I2C_M_RD)
> + wakeup = sh_mobile_i2c_isr_rx(pd);
> + else
> + wakeup = sh_mobile_i2c_isr_tx(pd);
>
> - msg->buf[pd->pos] = data;
> - }
> - pd->pos++;
> + if (sr & ICSR_WAIT) /* TODO: add delay here to support slow acks */
> + iowrite8(sr & ~ICSR_WAIT, ICSR(pd));
>
> - do_wakeup:
> if (wakeup) {
> pd->sr |= SW_DONE;
> wake_up(&pd->wait);
> @@ -219,6 +361,11 @@ static irqreturn_t sh_mobile_i2c_isr(int
>
> static int start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg)
> {
> + if (usr_msg->len = 0 && (usr_msg->flags & I2C_M_RD)) {
> + dev_err(pd->dev, "Unsupported zero length i2c read\n");
> + return -EIO;
> + }
> +
> /* Initialize channel registers */
> iowrite8(ioread8(ICCR(pd)) & ~ICCR_ICE, ICCR(pd));
>
> @@ -233,9 +380,8 @@ static int start_ch(struct sh_mobile_i2c
> pd->pos = -1;
> pd->sr = 0;
>
> - /* Enable all interrupts except wait */
> - iowrite8(ioread8(ICIC(pd)) | ICIC_ALE | ICIC_TACKE | ICIC_DTEE,
> - ICIC(pd));
> + /* Enable all interrupts to begin with */
> + iowrite8(ICIC_WAITE | ICIC_ALE | ICIC_TACKE | ICIC_DTEE, ICIC(pd));
> return 0;
> }
>
> @@ -268,25 +414,18 @@ static int sh_mobile_i2c_xfer(struct i2c
> if (!k)
> dev_err(pd->dev, "Transfer request timed out\n");
>
> - retry_count = 10;
> + retry_count = 1000;
> again:
> val = ioread8(ICSR(pd));
>
> dev_dbg(pd->dev, "val 0x%02x pd->sr 0x%02x\n", val, pd->sr);
>
> - if ((val | pd->sr) & (ICSR_TACK | ICSR_AL)) {
> - err = -EIO;
> - break;
> - }
> -
> /* the interrupt handler may wake us up before the
> * transfer is finished, so poll the hardware
> * until we're done.
> */
> -
> - if (!(!(val & ICSR_BUSY) && (val & ICSR_SCLM) &&
> - (val & ICSR_SDAM))) {
> - msleep(1);
> + if (val & ICSR_BUSY) {
> + udelay(10);
> if (retry_count--)
> goto again;
>
> @@ -294,6 +433,12 @@ again:
> dev_err(pd->dev, "Polling timed out\n");
> break;
> }
> +
> + /* handle missing acknowledge and arbitration lost */
> + if ((val | pd->sr) & (ICSR_TACK | ICSR_AL)) {
> + err = -EIO;
> + break;
> + }
> }
>
> deactivate_ch(pd);
--
Jean Delvare
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: fix i2c-sh_mobile timing issues
2008-08-27 9:33 [PATCH] i2c: fix i2c-sh_mobile timing issues Magnus Damm
2008-08-27 11:09 ` Jean Delvare
@ 2008-08-27 14:40 ` Paul Mundt
2008-08-27 15:46 ` Jean Delvare
1 sibling, 1 reply; 6+ messages in thread
From: Paul Mundt @ 2008-08-27 14:40 UTC (permalink / raw)
To: Magnus Damm; +Cc: i2c, khali, linux-sh, ben-linux
On Wed, Aug 27, 2008 at 06:33:56PM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
>
> This patch teaches the i2c-sh_mobile driver to make use of wait irqs.
> Without this patch only dte irqs are used which may lead to overruns
> and cases of missing stop and extra bytes being read on the i2c bus.
>
> Use of wait irqs forces the hardware to pause and wait until the cpu
> is ready. Polling is also reworked in this patch to fix ms delay issues.
>
> Verified with bus analyzer and tested on MigoR and AP325RXA boards.
>
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
>
> Paul, can you merge this for 2.6.27?
>
> drivers/i2c/busses/i2c-sh_mobile.c | 271 +++++++++++++++++++++++++++---------
> 1 file changed, 208 insertions(+), 63 deletions(-)
>
I'll add it to my queue if Jean and Ben don't have any issues with the
patch. If the polling works as is, I would suggest that it is not 2.6.27
material, but if it's fixing operational bugs then it doesn't make sense
to leave 2.6.27 in a known broken state.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: fix i2c-sh_mobile timing issues
2008-08-27 14:40 ` Paul Mundt
@ 2008-08-27 15:46 ` Jean Delvare
2008-09-05 5:36 ` Paul Mundt
0 siblings, 1 reply; 6+ messages in thread
From: Jean Delvare @ 2008-08-27 15:46 UTC (permalink / raw)
To: Paul Mundt; +Cc: Magnus Damm, i2c, linux-sh, ben-linux
On Wed, 27 Aug 2008 23:40:48 +0900, Paul Mundt wrote:
> On Wed, Aug 27, 2008 at 06:33:56PM +0900, Magnus Damm wrote:
> > From: Magnus Damm <damm@igel.co.jp>
> >
> > This patch teaches the i2c-sh_mobile driver to make use of wait irqs.
> > Without this patch only dte irqs are used which may lead to overruns
> > and cases of missing stop and extra bytes being read on the i2c bus.
> >
> > Use of wait irqs forces the hardware to pause and wait until the cpu
> > is ready. Polling is also reworked in this patch to fix ms delay issues.
> >
> > Verified with bus analyzer and tested on MigoR and AP325RXA boards.
> >
> > Signed-off-by: Magnus Damm <damm@igel.co.jp>
> > ---
> >
> > Paul, can you merge this for 2.6.27?
> >
> > drivers/i2c/busses/i2c-sh_mobile.c | 271 +++++++++++++++++++++++++++---------
> > 1 file changed, 208 insertions(+), 63 deletions(-)
> >
> I'll add it to my queue if Jean and Ben don't have any issues with the
> patch. If the polling works as is, I would suggest that it is not 2.6.27
> material, but if it's fixing operational bugs then it doesn't make sense
> to leave 2.6.27 in a known broken state.
No objection from me, as this is outside of my jurisdiction.
--
Jean Delvare
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: fix i2c-sh_mobile timing issues
2008-08-27 11:09 ` Jean Delvare
@ 2008-08-28 11:57 ` Magnus Damm
0 siblings, 0 replies; 6+ messages in thread
From: Magnus Damm @ 2008-08-28 11:57 UTC (permalink / raw)
To: Jean Delvare; +Cc: i2c, lethal, linux-sh, Ben Dooks
Hi Jean,
On Wed, Aug 27, 2008 at 8:09 PM, Jean Delvare <khali@linux-fr.org> wrote:
> On Wed, 27 Aug 2008 18:33:56 +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm@igel.co.jp>
>>
>> This patch teaches the i2c-sh_mobile driver to make use of wait irqs.
>> Without this patch only dte irqs are used which may lead to overruns
>> and cases of missing stop and extra bytes being read on the i2c bus.
>>
>> Use of wait irqs forces the hardware to pause and wait until the cpu
>> is ready. Polling is also reworked in this patch to fix ms delay issues.
>>
>> Verified with bus analyzer and tested on MigoR and AP325RXA boards.
>>
>> Signed-off-by: Magnus Damm <damm@igel.co.jp>
>> ---
>>
>> Paul, can you merge this for 2.6.27?
>>
>> drivers/i2c/busses/i2c-sh_mobile.c | 271 +++++++++++++++++++++++++++---------
>> 1 file changed, 208 insertions(+), 63 deletions(-)
>>
>
> I'm not sure why I am Cc'd on this but Ben Dooks isn't. Ben is the one
> in charge of i2c bus drivers for embedded system. I'm adding him.
Sorry about that. My fault, I based my emails on my previous emails.
I'd like to classify this patch as bug fix, but it required quite a
big rewrite since I had to change the interrupt handling which
affected the main state machine. And while at it I decided to add some
clarifications which added even more lines.
So I'd like to see this included in 2.6.27 if possible. It's a pretty
isolated change.
Thanks for your help!
/ magnus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: fix i2c-sh_mobile timing issues
2008-08-27 15:46 ` Jean Delvare
@ 2008-09-05 5:36 ` Paul Mundt
0 siblings, 0 replies; 6+ messages in thread
From: Paul Mundt @ 2008-09-05 5:36 UTC (permalink / raw)
To: Jean Delvare; +Cc: Magnus Damm, i2c, linux-sh, ben-linux
On Wed, Aug 27, 2008 at 05:46:28PM +0200, Jean Delvare wrote:
> On Wed, 27 Aug 2008 23:40:48 +0900, Paul Mundt wrote:
> > On Wed, Aug 27, 2008 at 06:33:56PM +0900, Magnus Damm wrote:
> > > From: Magnus Damm <damm@igel.co.jp>
> > >
> > > This patch teaches the i2c-sh_mobile driver to make use of wait irqs.
> > > Without this patch only dte irqs are used which may lead to overruns
> > > and cases of missing stop and extra bytes being read on the i2c bus.
> > >
> > > Use of wait irqs forces the hardware to pause and wait until the cpu
> > > is ready. Polling is also reworked in this patch to fix ms delay issues.
> > >
> > > Verified with bus analyzer and tested on MigoR and AP325RXA boards.
> > >
> > > Signed-off-by: Magnus Damm <damm@igel.co.jp>
> > > ---
> > >
> > > Paul, can you merge this for 2.6.27?
> > >
> > > drivers/i2c/busses/i2c-sh_mobile.c | 271 +++++++++++++++++++++++++++---------
> > > 1 file changed, 208 insertions(+), 63 deletions(-)
> > >
> > I'll add it to my queue if Jean and Ben don't have any issues with the
> > patch. If the polling works as is, I would suggest that it is not 2.6.27
> > material, but if it's fixing operational bugs then it doesn't make sense
> > to leave 2.6.27 in a known broken state.
>
> No objection from me, as this is outside of my jurisdiction.
>
Well, it's been a week and there's been no comments, so I'll roll this in
to my queue.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-09-05 5:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-08-27 9:33 [PATCH] i2c: fix i2c-sh_mobile timing issues Magnus Damm
2008-08-27 11:09 ` Jean Delvare
2008-08-28 11:57 ` Magnus Damm
2008-08-27 14:40 ` Paul Mundt
2008-08-27 15:46 ` Jean Delvare
2008-09-05 5:36 ` Paul Mundt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).