[U-Boot] [PATCH 1/3] 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..56f9680 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 (length && 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.
For completeness, add CONFIG_FSL_I2C_CUSTOM_DFR too.
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se --- drivers/i2c/fsl_i2c.c | 14 +++++++++++--- 1 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c index 56f9680..0c5f6be 100644 --- a/drivers/i2c/fsl_i2c.c +++ b/drivers/i2c/fsl_i2c.c @@ -172,14 +172,22 @@ 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 +#ifdef CONFIG_FSL_I2C_CUSTOM_FDR + fdr = CONFIG_FSL_I2C_CUSTOM_FDR; + speed = i2c_clk / divider; /* Fake something */ +#else 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 + writeb(fdr, &dev->fdr); /* set bus speed */ + break; }
The latest AN2819 has changed the way FDR/DFSR should be calculated. Update the driver according to spec. However, Condition 2 is not accounted for as it is not clear how to do so.
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se --- drivers/i2c/fsl_i2c.c | 88 +++++++++++++++++++++++++++++------------------- 1 files changed, 53 insertions(+), 35 deletions(-)
diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c index 0c5f6be..0491a69 100644 --- a/drivers/i2c/fsl_i2c.c +++ b/drivers/i2c/fsl_i2c.c @@ -100,29 +100,9 @@ static const struct fsl_i2c *i2c_dev[2] = { */ static const struct { unsigned short divider; -#ifdef __PPC__ - u8 dfsr; -#endif u8 fdr; } fsl_i2c_speed_map[] = { -#ifdef __PPC__ - {160, 1, 32}, {192, 1, 33}, {224, 1, 34}, {256, 1, 35}, - {288, 1, 0}, {320, 1, 1}, {352, 6, 1}, {384, 1, 2}, {416, 6, 2}, - {448, 1, 38}, {480, 1, 3}, {512, 1, 39}, {544, 11, 3}, {576, 1, 4}, - {608, 22, 3}, {640, 1, 5}, {672, 32, 3}, {704, 11, 5}, {736, 43, 3}, - {768, 1, 6}, {800, 54, 3}, {832, 11, 6}, {896, 1, 42}, {960, 1, 7}, - {1024, 1, 43}, {1088, 22, 7}, {1152, 1, 8}, {1216, 43, 7}, {1280, 1, 9}, - {1408, 22, 9}, {1536, 1, 10}, {1664, 22, 10}, {1792, 1, 46}, - {1920, 1, 11}, {2048, 1, 47}, {2176, 43, 11}, {2304, 1, 12}, - {2560, 1, 13}, {2816, 43, 13}, {3072, 1, 14}, {3328, 43, 14}, - {3584, 1, 50}, {3840, 1, 15}, {4096, 1, 51}, {4608, 1, 16}, - {5120, 1, 17}, {6144, 1, 18}, {7168, 1, 54}, {7680, 1, 19}, - {8192, 1, 55}, {9216, 1, 20}, {10240, 1, 21}, {12288, 1, 22}, - {14336, 1, 58}, {15360, 1, 23}, {16384, 1, 59}, {18432, 1, 24}, - {20480, 1, 25}, {24576, 1, 26}, {28672, 1, 62}, {30720, 1, 27}, - {32768, 1, 63}, {36864, 1, 28}, {40960, 1, 29}, {49152, 1, 30}, - {61440, 1, 31}, {-1, 1, 31} -#elif defined(__M68K__) +#ifdef __M68K__ {20, 32}, {22, 33}, {24, 34}, {26, 35}, {28, 0}, {28, 36}, {30, 1}, {32, 37}, {34, 2}, {36, 38}, {40, 3}, {40, 39}, @@ -158,7 +138,6 @@ static unsigned int set_i2c_bus_speed(const struct fsl_i2c *dev, unsigned int i2c_clk, unsigned int speed) { unsigned short divider = min(i2c_clk / speed, (unsigned short) -1); - unsigned int i;
/* * We want to choose an FDR/DFSR that generates an I2C bus speed that @@ -166,31 +145,70 @@ static unsigned int set_i2c_bus_speed(const struct fsl_i2c *dev, * want the first divider that is equal to or greater than the * calculated divider. */ - - for (i = 0; i < ARRAY_SIZE(fsl_i2c_speed_map); i++) - if (fsl_i2c_speed_map[i].divider >= divider) { - u8 fdr; #ifdef __PPC__ - u8 dfsr; + u8 dfsr, fdr = 0x31; /* Default if no FDR found */ + unsigned short A, B, Ga, Gb; + unsigned long c_div, est_div; + #ifdef CONFIG_FSL_I2C_CUSTOM_DFSR - dfsr = 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 */ + /* Condition 1: dfsr <= 50/T */ + dfsr = (5*(i2c_clk/1000))/(100000); #endif #ifdef CONFIG_FSL_I2C_CUSTOM_FDR - fdr = CONFIG_FSL_I2C_CUSTOM_FDR; - speed = i2c_clk / divider; /* Fake something */ + fdr = CONFIG_FSL_I2C_CUSTOM_FDR; + speed = i2c_clk / divider; /* Fake something */ #else + debug("Requested speed:%d, i2c_clk:%d\n", speed, i2c_clk); + if (!dfsr) + dfsr = 1; + + est_div = ~0; + for(Ga=0x4, A=10; A<=30; Ga++, A+=2) { + for (Gb=0; Gb<8; Gb++) { + B = 16 << Gb; + c_div = B * (A + ((3*dfsr)/B)*2); + if (c_div > divider && c_div < est_div) { + unsigned short bin_Gb, bin_Ga; + + est_div = c_div; + bin_Gb = Gb << 2; + bin_Ga = (Ga & 0x3) | ((Ga & 0x4) << 3); + fdr = bin_Gb | bin_Ga; + speed = i2c_clk/est_div; + debug("FDR:0x%.2x, div:%ld, Ga:0x%x, Gb:0x%x, " + "A:%d, B:%d, speed:%d\n", + fdr, est_div, Ga, Gb, A, B, speed); + /* Condition 2 not accounted for */ + debug("Tr <= %d ns\n", + (B-3*dfsr)* 1000000/(i2c_clk/1000)); + } + } + if (A == 20) + A+=2; + if (A == 24) + A+=4; + } + debug("divider:%d, est_div:%ld, DFSR:%d\n", divider, est_div, dfsr); + debug("FDR:0x%.2x, speed:%d\n", fdr, speed); +#endif + writeb(dfsr, &dev->dfsrr); /* set default filter */ + writeb(fdr, &dev->fdr); /* set bus speed */ +#else + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(fsl_i2c_speed_map); i++) + if (fsl_i2c_speed_map[i].divider >= divider) { + u8 fdr; + fdr = fsl_i2c_speed_map[i].fdr; speed = i2c_clk / fsl_i2c_speed_map[i].divider; -#endif writeb(fdr, &dev->fdr); /* set bus speed */
break; } - +#endif return speed; }
Hello Joakim,
Joakim Tjernlund wrote:
The latest AN2819 has changed the way FDR/DFSR should be calculated. Update the driver according to spec. However, Condition 2 is not accounted for as it is not clear how to do so.
Thanks for your work, just some minor Codingstyle comments:
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se
drivers/i2c/fsl_i2c.c | 88 +++++++++++++++++++++++++++++------------------- 1 files changed, 53 insertions(+), 35 deletions(-)
diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c index 0c5f6be..0491a69 100644 --- a/drivers/i2c/fsl_i2c.c +++ b/drivers/i2c/fsl_i2c.c @@ -100,29 +100,9 @@ static const struct fsl_i2c *i2c_dev[2] = { */ static const struct { unsigned short divider; -#ifdef __PPC__
- u8 dfsr;
-#endif u8 fdr; } fsl_i2c_speed_map[] = { -#ifdef __PPC__
- {160, 1, 32}, {192, 1, 33}, {224, 1, 34}, {256, 1, 35},
- {288, 1, 0}, {320, 1, 1}, {352, 6, 1}, {384, 1, 2}, {416, 6, 2},
- {448, 1, 38}, {480, 1, 3}, {512, 1, 39}, {544, 11, 3}, {576, 1, 4},
- {608, 22, 3}, {640, 1, 5}, {672, 32, 3}, {704, 11, 5}, {736, 43, 3},
- {768, 1, 6}, {800, 54, 3}, {832, 11, 6}, {896, 1, 42}, {960, 1, 7},
- {1024, 1, 43}, {1088, 22, 7}, {1152, 1, 8}, {1216, 43, 7}, {1280, 1, 9},
- {1408, 22, 9}, {1536, 1, 10}, {1664, 22, 10}, {1792, 1, 46},
- {1920, 1, 11}, {2048, 1, 47}, {2176, 43, 11}, {2304, 1, 12},
- {2560, 1, 13}, {2816, 43, 13}, {3072, 1, 14}, {3328, 43, 14},
- {3584, 1, 50}, {3840, 1, 15}, {4096, 1, 51}, {4608, 1, 16},
- {5120, 1, 17}, {6144, 1, 18}, {7168, 1, 54}, {7680, 1, 19},
- {8192, 1, 55}, {9216, 1, 20}, {10240, 1, 21}, {12288, 1, 22},
- {14336, 1, 58}, {15360, 1, 23}, {16384, 1, 59}, {18432, 1, 24},
- {20480, 1, 25}, {24576, 1, 26}, {28672, 1, 62}, {30720, 1, 27},
- {32768, 1, 63}, {36864, 1, 28}, {40960, 1, 29}, {49152, 1, 30},
- {61440, 1, 31}, {-1, 1, 31}
-#elif defined(__M68K__) +#ifdef __M68K__ {20, 32}, {22, 33}, {24, 34}, {26, 35}, {28, 0}, {28, 36}, {30, 1}, {32, 37}, {34, 2}, {36, 38}, {40, 3}, {40, 39}, @@ -158,7 +138,6 @@ static unsigned int set_i2c_bus_speed(const struct fsl_i2c *dev, unsigned int i2c_clk, unsigned int speed) { unsigned short divider = min(i2c_clk / speed, (unsigned short) -1);
unsigned int i;
/*
- We want to choose an FDR/DFSR that generates an I2C bus speed that
@@ -166,31 +145,70 @@ static unsigned int set_i2c_bus_speed(const struct fsl_i2c *dev, * want the first divider that is equal to or greater than the * calculated divider. */
- for (i = 0; i < ARRAY_SIZE(fsl_i2c_speed_map); i++)
if (fsl_i2c_speed_map[i].divider >= divider) {u8 fdr;#ifdef __PPC__
u8 dfsr;
- u8 dfsr, fdr = 0x31; /* Default if no FDR found */
- unsigned short A, B, Ga, Gb;
Please do not use mixed-case variables, thanks.
- unsigned long c_div, est_div;
#ifdef CONFIG_FSL_I2C_CUSTOM_DFSR
dfsr = 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 */
- /* Condition 1: dfsr <= 50/T */
- dfsr = (5*(i2c_clk/1000))/(100000);
Please use one space around (on each side of) most binary and ternary operators.
#endif #ifdef CONFIG_FSL_I2C_CUSTOM_FDR
fdr = CONFIG_FSL_I2C_CUSTOM_FDR;speed = i2c_clk / divider; /* Fake something */
- fdr = CONFIG_FSL_I2C_CUSTOM_FDR;
- speed = i2c_clk / divider; /* Fake something */
#else
- debug("Requested speed:%d, i2c_clk:%d\n", speed, i2c_clk);
- if (!dfsr)
dfsr = 1;- est_div = ~0;
- for(Ga=0x4, A=10; A<=30; Ga++, A+=2) {
spaces her too.
for (Gb=0; Gb<8; Gb++) {
and here too. Please check the whole patch.
B = 16 << Gb;c_div = B * (A + ((3*dfsr)/B)*2);if (c_div > divider && c_div < est_div) {
Can we make
if ((c_div > divider) && (c_div < est_div)) {
unsigned short bin_Gb, bin_Ga;
here too, please no mixed-case vars.
est_div = c_div;bin_Gb = Gb << 2;bin_Ga = (Ga & 0x3) | ((Ga & 0x4) << 3);fdr = bin_Gb | bin_Ga;speed = i2c_clk/est_div;debug("FDR:0x%.2x, div:%ld, Ga:0x%x, Gb:0x%x, ""A:%d, B:%d, speed:%d\n",fdr, est_div, Ga, Gb, A, B, speed);/* Condition 2 not accounted for */debug("Tr <= %d ns\n",(B-3*dfsr)* 1000000/(i2c_clk/1000));}}if (A == 20)A+=2;if (A == 24)A+=4;- }
- debug("divider:%d, est_div:%ld, DFSR:%d\n", divider, est_div, dfsr);
- debug("FDR:0x%.2x, speed:%d\n", fdr, speed);
+#endif
- writeb(dfsr, &dev->dfsrr); /* set default filter */
- writeb(fdr, &dev->fdr); /* set bus speed */
+#else
- unsigned int i;
- for (i = 0; i < ARRAY_SIZE(fsl_i2c_speed_map); i++)
if (fsl_i2c_speed_map[i].divider >= divider) {u8 fdr;fdr = fsl_i2c_speed_map[i].fdr; speed = i2c_clk / fsl_i2c_speed_map[i].divider;-#endif writeb(fdr, &dev->fdr); /* set bus speed */
break; }
+#endif return speed; }
bye Heiko
Heiko Schocher hs@denx.de wrote on 17/09/2009 08:00:34:
Hello Joakim,
Hi Heiko
Joakim Tjernlund wrote:
The latest AN2819 has changed the way FDR/DFSR should be calculated. Update the driver according to spec. However, Condition 2 is not accounted for as it is not clear how to do so.
Thanks for your work, just some minor Codingstyle comments:
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se
drivers/i2c/fsl_i2c.c | 88 +++++++++++++++++++++++++++++------------------- 1 files changed, 53 insertions(+), 35 deletions(-)
diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c index 0c5f6be..0491a69 100644 --- a/drivers/i2c/fsl_i2c.c +++ b/drivers/i2c/fsl_i2c.c @@ -100,29 +100,9 @@ static const struct fsl_i2c *i2c_dev[2] = {
#ifdef __PPC__
u8 dfsr;
- u8 dfsr, fdr = 0x31; /* Default if no FDR found */
- unsigned short A, B, Ga, Gb;
Please do not use mixed-case variables, thanks.
A and B are from the AN2819 spec and I used the same names to ease identify with the spec. I rather keep them.
- unsigned long c_div, est_div;
#ifdef CONFIG_FSL_I2C_CUSTOM_DFSR
dfsr = 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 */
- /* Condition 1: dfsr <= 50/T */
- dfsr = (5*(i2c_clk/1000))/(100000);
Please use one space around (on each side of) most binary and ternary operators.
Like so? dfsr = (5 * (i2c_clk / 1000)) / 100000);
#endif #ifdef CONFIG_FSL_I2C_CUSTOM_FDR
fdr = CONFIG_FSL_I2C_CUSTOM_FDR;speed = i2c_clk / divider; /* Fake something */
- fdr = CONFIG_FSL_I2C_CUSTOM_FDR;
- speed = i2c_clk / divider; /* Fake something */
#else
- debug("Requested speed:%d, i2c_clk:%d\n", speed, i2c_clk);
- if (!dfsr)
dfsr = 1;- est_div = ~0;
- for(Ga=0x4, A=10; A<=30; Ga++, A+=2) {
spaces her too.
Like so? for(Ga = 0x4, A = 10; A <= 30; Ga++, A += 2) {
for (Gb=0; Gb<8; Gb++) {and here too. Please check the whole patch.
B = 16 << Gb;c_div = B * (A + ((3*dfsr)/B)*2);if (c_div > divider && c_div < est_div) {Can we make
if ((c_div > divider) && (c_div < est_div)) {
Sure.
Dear Joakim Tjernlund,
In message OF0E2B8150.47DDCD99-ONC1257634.0023E2E7-C1257634.002481BA@transmode.se you wrote:
- u8 dfsr, fdr = 0x31; /* Default if no FDR found */
- unsigned short A, B, Ga, Gb;
Please do not use mixed-case variables, thanks.
A and B are from the AN2819 spec and I used the same names to ease identify with the spec. I rather keep them.
Feel free to add a comment to explain the relation, but please use only lower-case identifiers.
...
Like so? for(Ga = 0x4, A = 10; A <= 30; Ga++, A += 2) {
Please also here:
for (...)
Best regards,
Wolfgang Denk
Hello Joakim,
Joakim Tjernlund wrote:
Heiko Schocher hs@denx.de wrote on 17/09/2009 08:00:34:
Hello Joakim,
Hi Heiko
Joakim Tjernlund wrote:
The latest AN2819 has changed the way FDR/DFSR should be calculated. Update the driver according to spec. However, Condition 2 is not accounted for as it is not clear how to do so.
Thanks for your work, just some minor Codingstyle comments:
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se
drivers/i2c/fsl_i2c.c | 88 +++++++++++++++++++++++++++++------------------- 1 files changed, 53 insertions(+), 35 deletions(-)
diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c index 0c5f6be..0491a69 100644 --- a/drivers/i2c/fsl_i2c.c +++ b/drivers/i2c/fsl_i2c.c @@ -100,29 +100,9 @@ static const struct fsl_i2c *i2c_dev[2] = {
#ifdef __PPC__
u8 dfsr;
- u8 dfsr, fdr = 0x31; /* Default if no FDR found */
- unsigned short A, B, Ga, Gb;
Please do not use mixed-case variables, thanks.
A and B are from the AN2819 spec and I used the same names to ease identify with the spec. I rather keep them.
I am fine with that, just please do not mix upper and lower case in one variable name. So please use "ga" and "gb" ...
- unsigned long c_div, est_div;
#ifdef CONFIG_FSL_I2C_CUSTOM_DFSR
dfsr = 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 */
- /* Condition 1: dfsr <= 50/T */
- dfsr = (5*(i2c_clk/1000))/(100000);
Please use one space around (on each side of) most binary and ternary operators.
Like so? dfsr = (5 * (i2c_clk / 1000)) / 100000);
Yep.
#endif #ifdef CONFIG_FSL_I2C_CUSTOM_FDR
fdr = CONFIG_FSL_I2C_CUSTOM_FDR;speed = i2c_clk / divider; /* Fake something */
- fdr = CONFIG_FSL_I2C_CUSTOM_FDR;
- speed = i2c_clk / divider; /* Fake something */
#else
- debug("Requested speed:%d, i2c_clk:%d\n", speed, i2c_clk);
- if (!dfsr)
dfsr = 1;- est_div = ~0;
- for(Ga=0x4, A=10; A<=30; Ga++, A+=2) {
spaces her too.
Like so? for(Ga = 0x4, A = 10; A <= 30; Ga++, A += 2) {
for (Ga = 0x4, A = 10; A <= 30; Ga++, A += 2) {
for (Gb=0; Gb<8; Gb++) {and here too. Please check the whole patch.
B = 16 << Gb;c_div = B * (A + ((3*dfsr)/B)*2);if (c_div > divider && c_div < est_div) {Can we make
if ((c_div > divider) && (c_div < est_div)) {
Sure.
Thanks!
bye Heiko
participants (4)
-
Heiko Schocher -
Joakim Tjernlund -
Joakim Tjernlund
-
Wolfgang Denk