Sonic, The issue that the system sometimes freezes is fixed now, due to proper timeout handling.
The i2c driver as it is today is therefore better than before. However with the ADXL345 driver in I2C mode, SCL set to 400kHz - I sometimes see that wait_for_completion_timeout() runs into timeout, due to a lost interrupt. bfin_twi_handle_interrupt() returns without signaling complete() and the HW doesn't issue a follow up interrupt, as the implementation would assume. As far as I can tell this happens with the block read transfers, and only at high SCL. I will send you an ADXL board you can test with. -Michael >-----Original Message----- >From: Zhang, Sonic >Sent: Tuesday, June 16, 2009 4:15 AM >To: Zhang, Sonic; Hennerich, Michael; [email protected] >Cc: [email protected] >Subject: RE: [Linux-kernel-commits] [6649] trunk/drivers/i2c/busses/i2c-bfin-twi.c: More replace >timeout timer with wait_for_completion_timeout > >Hi Michael, > >I ran the complete regression after applying your patch. Both twi_smbus_test and twi_master_test >passed on the regression machine. Could you file a bug with the error information under ADXL34x >driver? Maybe the problem is the default timeout value is too short. You can changed it in ADXL34x >driver at runtime. > > >Sonic > > >-----Original Message----- >From: Zhang, Sonic [mailto:[email protected]] >Sent: Monday, June 15, 2009 5:48 PM >To: Hennerich, Michael; [email protected] >Cc: [email protected] >Subject: RE: [Linux-kernel-commits] [6649] trunk/drivers/i2c/busses/i2c-bfin-twi.c: More replace >timeout timer with wait_for_completion_timeout > >I am still looking at the regression result. > >Sonic > >-----Original Message----- >From: Hennerich, Michael [mailto:[email protected]] >Sent: Monday, June 15, 2009 5:06 PM >To: [email protected] >Cc: [email protected] >Subject: RE: [Linux-kernel-commits] [6649] >trunk/drivers/i2c/busses/i2c-bfin-twi.c: More replace timeout timer with wait_for_completion_timeout > >Sonic, > >Did you test these changes? > >i2c-bfin-twi i2c-bfin-twi.0: Blackfin BF5xx on-chip I2C TWI Contoller, regs_b...@ffc00700 >schedule_timeout: wrong timeout value fffedda6 Hardware Trace: > 0 Target : <0x00004c50> { __etext + 0xffe50890 } > > >IIRC: > >Timeout is in jiffies but not jiffies + timeout. > >Index: drivers/i2c/busses/i2c-bfin-twi.c >=================================================================== >--- drivers/i2c/busses/i2c-bfin-twi.c (revision 6714) >+++ drivers/i2c/busses/i2c-bfin-twi.c (working copy) >@@ -335,7 +335,7 @@ > > while (!iface->result) { > if (!wait_for_completion_timeout(&iface->complete, >- jiffies + adap->timeout*HZ)) >+ adap->timeout*HZ)) > iface->result = -1; > } > >@@ -551,7 +551,7 @@ > > while (!iface->result) { > if (!wait_for_completion_timeout(&iface->complete, >- jiffies + adap->timeout*HZ)) >+ adap->timeout*HZ)) > iface->result = -1; > } > > >With this being fixed - the kernel boots without an error messages. >However the ADXL34x driver doesn't work at all now. > >-Michael > > > >________________________________________ >From: [email protected] >[mailto:[email protected]] On Behalf Of [email protected] >Sent: Thursday, June 11, 2009 11:30 AM >To: [email protected] >Subject: [Linux-kernel-commits] [6649] >trunk/drivers/i2c/busses/i2c-bfin-twi.c: More replace timeout timer with wait_for_completion_timeout > >Revision >6649 >Author >sonicz >Date >2009-06-11 04:29:59 -0500 (Thu, 11 Jun 2009) Log Message More replace timeout timer with >wait_for_completion_timeout > >1. retry i2c transfer according to user defined parameter retires in struct i2c_adapter. >2. wait timeout based on user defined parameter timeout in struct i2c_adapter. >3. wake up i2c transfer process in each twi interrupt. >Modified Paths >* trunk/drivers/i2c/busses/i2c-bfin-twi.c >Diff >Modified: trunk/drivers/i2c/busses/i2c-bfin-twi.c (6648 => 6649) > >--- trunk/drivers/i2c/busses/i2c-bfin-twi.c 2009-06-11 09:26:47 UTC >(rev 6648) >+++ trunk/drivers/i2c/busses/i2c-bfin-twi.c 2009-06-11 09:29:59 UTC >(rev 6649) >@@ -23,8 +23,6 @@ > #include <asm/portmux.h> > #include <asm/irq.h> > >-#define POLL_TIMEOUT (20 * HZ) >- > /* SMBus mode*/ > #define TWI_I2C_MODE_STANDARD 1 > #define TWI_I2C_MODE_STANDARDSUB 2 >@@ -165,16 +163,13 @@ > write_INT_MASK(iface, 0); > write_MASTER_CTL(iface, 0); > SSYNC(); >- /* If it is a quick transfer, only address bug >no data, >+ /* If it is a quick transfer, only address >without data, > * not an err, return 1. >+ * If address is acknowledged return 1. > */ >- if (iface->writeNum == 0 && (mast_stat & >BUFRDERR)) >+ if ((iface->writeNum == 0 && (mast_stat & >BUFRDERR)) >+ || !(mast_stat & ANAK)) > iface->result = 1; >- /* If address not acknowledged return -1, >- * else return 0. >- */ >- else if (!(mast_stat & ANAK)) >- iface->result = 0; > } > complete(&iface->complete); > return; >@@ -246,9 +241,9 @@ > write_INT_MASK(iface, 0); > write_MASTER_CTL(iface, 0); > SSYNC(); >- complete(&iface->complete); > } > } >+ complete(&iface->complete); > } > > /* Interrupt handler */ >@@ -264,9 +259,9 @@ > } > > /* >- * Generic i2c master transfer entrypoint >+ * One i2c master transfer > */ >-static int bfin_twi_master_xfer(struct i2c_adapter *adap, >+static int bfin_twi_do_master_xfer(struct i2c_adapter *adap, > struct i2c_msg *msgs, int num) > { > struct bfin_twi_iface *iface = adap->algo_data; @@ -338,23 >+333,41 @@ > ((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ > 100) ? FAST : 0)); > SSYNC(); > >- if (!wait_for_completion_timeout(&iface->complete, >- jiffies + POLL_TIMEOUT)) >- iface->result = -1; >+ while (!iface->result) { >+ if (!wait_for_completion_timeout(&iface->complete, >+ jiffies + adap->timeout*HZ)) >+ iface->result = -1; >+ } > >- rc = iface->result; >+ if (iface->result == 1) >+ rc = iface->cur_msg + 1; >+ else >+ rc = iface->result; > >- if (rc == 1) >- return num; >- else >- return rc; >+ return rc; > } > > /* >- * SMBus type transfer entrypoint >+ * Generic i2c master transfer entrypoint > */ >+static int bfin_twi_master_xfer(struct i2c_adapter *adap, >+ struct i2c_msg *msgs, int num) >+{ >+ int i, ret = 0; > >-int bfin_twi_smbus_xfer(struct i2c_adapter *adap, u16 addr, >+ for (i = 0; i < adap->retries; i++) { >+ ret = bfin_twi_do_master_xfer(adap, msgs, num); >+ if (ret > 0) >+ break; >+ } >+ >+ return ret; >+} >+ >+/* >+ * One I2C SMBus transfer >+ */ >+int bfin_twi_do_smbus_xfer(struct i2c_adapter *adap, u16 addr, > unsigned short flags, char read_write, > u8 command, int size, union i2c_smbus_data >*data) > { >@@ -536,9 +549,11 @@ > } > SSYNC(); > >- if (!wait_for_completion_timeout(&iface->complete, >- jiffies + POLL_TIMEOUT)) >- iface->result = -1; >+ while (!iface->result) { >+ if (!wait_for_completion_timeout(&iface->complete, >+ jiffies + adap->timeout*HZ)) >+ iface->result = -1; >+ } > > rc = (iface->result >= 0) ? 0 : -1; > >@@ -546,6 +561,25 @@ > } > > /* >+ * Generic I2C SMBus transfer entrypoint */ int >+bfin_twi_smbus_xfer(struct i2c_adapter *adap, u16 addr, >+ unsigned short flags, char read_write, >+ u8 command, int size, union i2c_smbus_data >*data) >+{ >+ int i, ret = 0; >+ >+ for (i = 0; i < adap->retries; i++) { >+ ret = bfin_twi_do_smbus_xfer(adap, addr, flags, >+ read_write, command, size, data); >+ if (ret == 0) >+ break; >+ } >+ >+ return ret; >+} >+ >+/* > * Return what the adapter supports > */ > static u32 bfin_twi_functionality(struct i2c_adapter *adap) @@ -643,6 >+677,8 @@ > p_adap->algo_data = iface; > p_adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD; > p_adap->dev.parent = &pdev->dev; >+ p_adap->timeout = 1; >+ p_adap->retries = 3; > > rc = peripheral_request_list(pin_req[pdev->id], "i2c-bfin-twi"); > if (rc) { _______________________________________________ Linux-kernel-commits mailing list [email protected] https://blackfin.uclinux.org/mailman/listinfo/linux-kernel-commits
