[U-Boot] [PATCH 1/2] fsl_i2c: Wait for STOP condition to propagate
After issuing a STOP one must wait until the STOP has completed on the bus before doing something new to the controller.
Also add an extra read of SR as the manual mentions doing that is a good idea.
Remove surplus write of CR just before a write, isn't required and could potentially disturb the I2C bus.
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se --- drivers/i2c/fsl_i2c.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c index 47bbf79..59bfab6 100644 --- a/drivers/i2c/fsl_i2c.c +++ b/drivers/i2c/fsl_i2c.c @@ -223,7 +223,7 @@ i2c_init(int speed, int slaveadd) #endif }
-static __inline__ int +static int i2c_wait4bus(void) { unsigned long long timeval = get_ticks(); @@ -248,6 +248,8 @@ i2c_wait(int write) csr = readb(&i2c_dev[i2c_bus_num]->sr); if (!(csr & I2C_SR_MIF)) continue; + /* Read again to allow register to stabilise */ + csr = readb(&i2c_dev[i2c_bus_num]->sr);
writeb(0x0, &i2c_dev[i2c_bus_num]->sr);
@@ -293,9 +295,6 @@ __i2c_write(u8 *data, int length) { int i;
- writeb(I2C_CR_MEN | I2C_CR_MSTA | I2C_CR_MTX, - &i2c_dev[i2c_bus_num]->cr); - for (i = 0; i < length; i++) { writeb(data[i], &i2c_dev[i2c_bus_num]->dr);
@@ -351,6 +350,9 @@ i2c_read(u8 dev, uint addr, int alen, u8 *data, int length) && i2c_write_addr(dev, I2C_READ_BIT, 1) != 0) i = __i2c_read(data, length);
+ if (i2c_wait4bus()) /* Wait until STOP */ + debug("i2c_read: wait4bus timed out\n"); + writeb(I2C_CR_MEN, &i2c_dev[i2c_bus_num]->cr);
if (i == length) @@ -372,6 +374,8 @@ i2c_write(u8 dev, uint addr, int alen, u8 *data, int length) }
writeb(I2C_CR_MEN, &i2c_dev[i2c_bus_num]->cr); + if (i2c_wait4bus()) /* Wait until STOP */ + debug("i2c_write: wait4bus timed out\n");
if (i == length) return 0;
Some boards need a higher DFSR value than the spec currently recommends so give these boards the means to define there own.
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se --- drivers/i2c/fsl_i2c.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c index 59bfab6..ea0146c 100644 --- a/drivers/i2c/fsl_i2c.c +++ b/drivers/i2c/fsl_i2c.c @@ -172,14 +172,17 @@ static unsigned int set_i2c_bus_speed(const struct fsl_i2c *dev, u8 fdr; #ifdef __PPC__ u8 dfsr; +#ifdef CONFIG_FSL_I2C_CUSTOM_DFSR + dfsr = CONFIG_FSL_I2C_CUSTOM_DFSR; +#else dfsr = fsl_i2c_speed_map[i].dfsr; #endif + writeb(dfsr, &dev->dfsrr); /* set default filter */ +#endif fdr = fsl_i2c_speed_map[i].fdr; speed = i2c_clk / fsl_i2c_speed_map[i].divider; writeb(fdr, &dev->fdr); /* set bus speed */ -#ifdef __PPC__ - writeb(dfsr, &dev->dfsrr); /* set default filter */ -#endif + break; }
Joakim Tjernlund wrote:
Some boards need a higher DFSR value than the spec currently recommends so give these boards the means to define there own.
If you're going to do this, then you need to also define CONFIG_FSL_I2C_CUSTOM_FSR and CONFIG_FSL_I2C_CUSTOM_SPEED, and disable the code that lets you change the speed. Otherwise, the code will have no idea what speed the bus is really running at.
Joakim Tjernlund wrote:
Some boards need a higher DFSR value than the spec currently recommends so give these boards the means to define there own.
Wow, that was fast :)
If you're going to do this, then you need to also define CONFIG_FSL_I2C_CUSTOM_FSR and CONFIG_FSL_I2C_CUSTOM_SPEED, and disable the code that lets you change the speed. Otherwise, the code will have no idea what speed the bus is really running at.
No, the impact on speed from DFSR is pretty small so it will be close enough. It will only complicate matters for the user I think.
Jocke
Timur Tabi timur@freescale.com wrote on 15/09/2009 21:04:47:
Joakim Tjernlund wrote:
No, the impact on speed from DFSR is pretty small so it will be close enough.
How small? From the app note:
divisor = B * (A + ((3*C)/B)*2);
C is dfsr and 10 <= A <= 30, 16 <= B <= 2048 Considering the actual speed may be way lower the requested speed I think this is small enough.
Jocke
Timur Tabi timur@freescale.com wrote on 15/09/2009 21:04:47:
Joakim Tjernlund wrote:
No, the impact on speed from DFSR is pretty small so it will be close enough.
How small?
From the app note: divisor = B * (A + ((3*C)/B)*2);
C is dfsr and 10 <= A <= 30, 16 <= B <= 2048 Considering the actual speed may be way lower the requested speed I think this is small enough.
Once we have the new procedure in place, we can calculate the exact divisor so the need for extra CONFIG_ options goes away.
Jocke
Joakim Tjernlund wrote:
Timur Tabi timur@freescale.com wrote on 15/09/2009 21:04:47:
Joakim Tjernlund wrote:
No, the impact on speed from DFSR is pretty small so it will be close enough.
How small?
From the app note: divisor = B * (A + ((3*C)/B)*2);
C is dfsr and 10 <= A <= 30, 16 <= B <= 2048 Considering the actual speed may be way lower the requested speed I think this is small enough.
Once we have the new procedure in place, we can calculate the exact divisor so the need for extra CONFIG_ options goes away.
As Timur pointed out, a new table/algorithm would require some real testing and also some feedback from the users. Who knows if "your" values do not make trouble. Therefore I vote for using custom settings for maximum flexibility:
CONFIG_FSL_I2C_CUSTOM_FDR CONFIG_FSL_I2C_CUSTOM_DFSR
or
CONFIG_FSL_I2C_CUSTOM_DFSR_FDR
Wolfgang.
Wolfgang Grandegger wg@denx.de wrote on 16/09/2009 12:22:05:
Joakim Tjernlund wrote:
Timur Tabi timur@freescale.com wrote on 15/09/2009 21:04:47:
Joakim Tjernlund wrote:
No, the impact on speed from DFSR is pretty small so it will be close enough.
How small?
From the app note: divisor = B * (A + ((3*C)/B)*2);
C is dfsr and 10 <= A <= 30, 16 <= B <= 2048 Considering the actual speed may be way lower the requested speed I think this is small enough.
Once we have the new procedure in place, we can calculate the exact divisor so the need for extra CONFIG_ options goes away.
As Timur pointed out, a new table/algorithm would require some real testing and also some feedback from the users. Who knows if "your" values do not make trouble. Therefore I vote for using custom settings for maximum flexibility:
CONFIG_FSL_I2C_CUSTOM_FDR CONFIG_FSL_I2C_CUSTOM_DFSR
Oh well, since you both wanted it I added it. Sent 3 patches, the last patch impl. the latest AN2819 spec.
Would you mind test it a little?
Jocke
Joakim Tjernlund wrote:
Wolfgang Grandegger wg@denx.de wrote on 16/09/2009 12:22:05:
Joakim Tjernlund wrote:
Timur Tabi timur@freescale.com wrote on 15/09/2009 21:04:47:
Joakim Tjernlund wrote:
No, the impact on speed from DFSR is pretty small so it will be close enough.
How small?
From the app note: divisor = B * (A + ((3*C)/B)*2);
C is dfsr and 10 <= A <= 30, 16 <= B <= 2048 Considering the actual speed may be way lower the requested speed I think this is small enough.
Once we have the new procedure in place, we can calculate the exact divisor so the need for extra CONFIG_ options goes away.
As Timur pointed out, a new table/algorithm would require some real testing and also some feedback from the users. Who knows if "your" values do not make trouble. Therefore I vote for using custom settings for maximum flexibility:
CONFIG_FSL_I2C_CUSTOM_FDR CONFIG_FSL_I2C_CUSTOM_DFSR
Oh well, since you both wanted it I added it. Sent 3 patches, the last patch impl. the latest AN2819 spec.
Would you mind test it a little?
OK, I will do some tests later this week. What CPU do you use and at what I2C bus frequency do you test?
Wolfgang.
Wolfgang Grandegger wg@grandegger.com wrote on 16/09/2009 13:45:03:
Joakim Tjernlund wrote:
Wolfgang Grandegger wg@denx.de wrote on 16/09/2009 12:22:05:
Joakim Tjernlund wrote:
Timur Tabi timur@freescale.com wrote on 15/09/2009 21:04:47:
Joakim Tjernlund wrote:
> No, the impact on speed from DFSR is pretty small so it will > be close enough. How small?
From the app note: divisor = B * (A + ((3*C)/B)*2);
C is dfsr and 10 <= A <= 30, 16 <= B <= 2048 Considering the actual speed may be way lower the requested speed I think this is small enough.
Once we have the new procedure in place, we can calculate the exact divisor so the need for extra CONFIG_ options goes away.
As Timur pointed out, a new table/algorithm would require some real testing and also some feedback from the users. Who knows if "your" values do not make trouble. Therefore I vote for using custom settings for maximum flexibility:
CONFIG_FSL_I2C_CUSTOM_FDR CONFIG_FSL_I2C_CUSTOM_DFSR
Oh well, since you both wanted it I added it. Sent 3 patches, the last patch impl. the latest AN2819 spec.
Would you mind test it a little?
OK, I will do some tests later this week. What CPU do you use and at what I2C bus frequency do you test?
mpc8321, I2C bus is between 34KHz and 100KHz, CSB is 133.332 MHz
Joakim Tjernlund wrote:
Wolfgang Grandegger wg@grandegger.com wrote on 16/09/2009 13:45:03:
Joakim Tjernlund wrote:
Wolfgang Grandegger wg@denx.de wrote on 16/09/2009 12:22:05:
Joakim Tjernlund wrote:
Timur Tabi timur@freescale.com wrote on 15/09/2009 21:04:47: > Joakim Tjernlund wrote: > >> No, the impact on speed from DFSR is pretty small so it will >> be close enough. > How small? From the app note: divisor = B * (A + ((3*C)/B)*2);
C is dfsr and 10 <= A <= 30, 16 <= B <= 2048 Considering the actual speed may be way lower the requested speed I think this is small enough.
Once we have the new procedure in place, we can calculate the exact divisor so the need for extra CONFIG_ options goes away.
As Timur pointed out, a new table/algorithm would require some real testing and also some feedback from the users. Who knows if "your" values do not make trouble. Therefore I vote for using custom settings for maximum flexibility:
CONFIG_FSL_I2C_CUSTOM_FDR CONFIG_FSL_I2C_CUSTOM_DFSR
Oh well, since you both wanted it I added it. Sent 3 patches, the last patch impl. the latest AN2819 spec.
Would you mind test it a little?
OK, I will do some tests later this week. What CPU do you use and at what I2C bus frequency do you test?
mpc8321, I2C bus is between 34KHz and 100KHz, CSB is 133.332 MHz
OK, where can I find the new AN2819? I found Document Number: AN2919, Rev. 5, 12/2008.
Wolfgang.
Wolfgang Grandegger wg@denx.de wrote on 17/09/2009 08:36:32:
mpc8321, I2C bus is between 34KHz and 100KHz, CSB is 133.332 MHz
OK, where can I find the new AN2819? I found Document Number: AN2919, Rev. 5, 12/2008.
That is the one I found too.
Jocke
participants (5)
-
Joakim Tjernlund -
Joakim Tjernlund
-
Timur Tabi -
Wolfgang Grandegger -
Wolfgang Grandegger