[U-Boot] [PATCH 1/3] fsl_i2c: Wait for STOP condition to propagate

[U-Boot] [PATCH 1/3] fsl_i2c: Wait for STOP condition to propagate

[Joakim Tjernlund]

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;
-- 
1.6.4.3

[U-Boot] [PATCH 2/3] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_{DFSR/FDR}

[Joakim Tjernlund]

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_FDR 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;
 		}
 
-- 
1.6.4.3

[U-Boot] [PATCH 3/3] fsl_i2c: Impl. AN2919, rev 5 to calculate FDR/DFSR

[Joakim Tjernlund]

The latest AN2919 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 |   90 ++++++++++++++++++++++++++++++-------------------
 1 files changed, 55 insertions(+), 35 deletions(-)

diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
index 0c5f6be..78f21c7 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,72 @@ 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 */
+	/* a, b and dfsr matches identifiers A,B and C respectively in AN2919 */
+	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;
 }
 
-- 
1.6.4.3

[U-Boot] [PATCH 3/3] fsl_i2c: Impl. AN2919, rev 5 to calculate FDR/DFSR

[Heiko Schocher]

Hello Joakim,

Joakim Tjernlund wrote:
> The latest AN2919 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 |   90 ++++++++++++++++++++++++++++++-------------------
>  1 files changed, 55 insertions(+), 35 deletions(-)

Applied to u-boot-i2c testing with one fix:

Applying: fsl_i2c: Impl. AN2919, rev 5 to calculate FDR/DFSR
/home/hs/i2c/u-boot-i2c/.git/rebase-apply/patch:100: trailing whitespace.
                                debug("Tr <= %d ns\n",
warning: 1 line applied after fixing whitespace errors.

Thanks.

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 3/3] fsl_i2c: Impl. AN2919, rev 5 to calculate FDR/DFSR

[Wolfgang Grandegger]

Hi Joakim,

Joakim Tjernlund wrote:
> The latest AN2919 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.

I compared rev. 5 of AN2919 with rev. 3 and, as you pointed out, it puts
additional constraints on how to select dfsr and fdr. Especially dfsr
should not exceed a certain, frequency dependent value: dfsr <= 50 /
period-in-ns. Therefore, I expected problems with divider values from
the table which high dfsr values. I did your "=> date;date;date;date"
test on a MPC8548 board using dfsr=43 and fdr=7 but it did not fail.
According to the rev. 5, dfsr is not allowed to be greater than 8.
Your patch works fine on this board as well. I have no time for a more
thorough testing with different CPUs and frequencies. Anyhow...

> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Acked-by: Wolfgang Grandegger <wg@grandegger.com>

I realized some minor coding style issues, though. See below.

> ---
>  drivers/i2c/fsl_i2c.c |   90 ++++++++++++++++++++++++++++++-------------------
>  1 files changed, 55 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
> index 0c5f6be..78f21c7 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,72 @@ 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 */
> +	/* a, b and dfsr matches identifiers A,B and C respectively in AN2919 */
> +	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);

Please use spaces around "/" and "*".

> +			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", 

Please remove trailing white space.

Wolfgang.

[U-Boot] [PATCH 3/3] fsl_i2c: Impl. AN2919, rev 5 to calculate FDR/DFSR

[Joakim Tjernlund]

Wolfgang Grandegger <wg@denx.de> wrote on 21/09/2009 12:53:36:
>
> Hi Joakim,
>
> Joakim Tjernlund wrote:
> > The latest AN2919 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.
>
> I compared rev. 5 of AN2919 with rev. 3 and, as you pointed out, it puts
> additional constraints on how to select dfsr and fdr. Especially dfsr
> should not exceed a certain, frequency dependent value: dfsr <= 50 /
> period-in-ns. Therefore, I expected problems with divider values from
> the table which high dfsr values. I did your "=> date;date;date;date"
> test on a MPC8548 board using dfsr=43 and fdr=7 but it did not fail.
> According to the rev. 5, dfsr is not allowed to be greater than 8.
> Your patch works fine on this board as well. I have no time for a more
> thorough testing with different CPUs and frequencies. Anyhow...

Yes, I too notice that higher dfsr values than the spec says works, in
fact my board needs at least dfsr 8 to be 100% stable but the spec says
no more that 6 for my board. I suspect that we should enforce a minimum
value of 8 to be on the safe side, especially as the HW default is 0x10.
Anyhow it is better now than before.

>
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> Acked-by: Wolfgang Grandegger <wg@grandegger.com>

Thanks.

>
> I realized some minor coding style issues, though. See below.
Noted.

[U-Boot] [PATCH 3/3] fsl_i2c: Impl. AN2919, rev 5 to calculate FDR/DFSR

[Wolfgang Grandegger]

Joakim Tjernlund wrote:
> Wolfgang Grandegger <wg@denx.de> wrote on 21/09/2009 12:53:36:
>> Hi Joakim,
>>
>> Joakim Tjernlund wrote:
>>> The latest AN2919 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.
>> I compared rev. 5 of AN2919 with rev. 3 and, as you pointed out, it puts
>> additional constraints on how to select dfsr and fdr. Especially dfsr
>> should not exceed a certain, frequency dependent value: dfsr <= 50 /
>> period-in-ns. Therefore, I expected problems with divider values from
>> the table which high dfsr values. I did your "=> date;date;date;date"
>> test on a MPC8548 board using dfsr=43 and fdr=7 but it did not fail.
>> According to the rev. 5, dfsr is not allowed to be greater than 8.
>> Your patch works fine on this board as well. I have no time for a more
>> thorough testing with different CPUs and frequencies. Anyhow...
> 
> Yes, I too notice that higher dfsr values than the spec says works, in
> fact my board needs at least dfsr 8 to be 100% stable but the spec says
> no more that 6 for my board. I suspect that we should enforce a minimum
> value of 8 to be on the safe side, especially as the HW default is 0x10.

You say that the new constraints introduced by rev. 5 are not even
correct!? Well, these modifications are obscure anyhow.

> Anyhow it is better now than before.

At least for your board. Let's keep an eye on people reporting I2C
problems for these processors on the U-Boot and linuxppc-dev ML.

Wolfgang.

[U-Boot] [PATCH 3/3] fsl_i2c: Impl. AN2919, rev 5 to calculate FDR/DFSR

[Joakim Tjernlund]

Wolfgang Grandegger <wg@denx.de> wrote on 21/09/2009 13:59:04:
>
> Joakim Tjernlund wrote:
> > Wolfgang Grandegger <wg@denx.de> wrote on 21/09/2009 12:53:36:
> >> Hi Joakim,
> >>
> >> Joakim Tjernlund wrote:
> >>> The latest AN2919 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.
> >> I compared rev. 5 of AN2919 with rev. 3 and, as you pointed out, it puts
> >> additional constraints on how to select dfsr and fdr. Especially dfsr
> >> should not exceed a certain, frequency dependent value: dfsr <= 50 /
> >> period-in-ns. Therefore, I expected problems with divider values from
> >> the table which high dfsr values. I did your "=> date;date;date;date"
> >> test on a MPC8548 board using dfsr=43 and fdr=7 but it did not fail.
> >> According to the rev. 5, dfsr is not allowed to be greater than 8.
> >> Your patch works fine on this board as well. I have no time for a more
> >> thorough testing with different CPUs and frequencies. Anyhow...
> >
> > Yes, I too notice that higher dfsr values than the spec says works, in
> > fact my board needs at least dfsr 8 to be 100% stable but the spec says
> > no more that 6 for my board. I suspect that we should enforce a minimum
> > value of 8 to be on the safe side, especially as the HW default is 0x10.
>
> You say that the new constraints introduced by rev. 5 are not even
> correct!? Well, these modifications are obscure anyhow.

My board has a higher rise time than the I2C spec(1 us) so it may
be the reason for me. It *looks* like a higher dfsr makes the controller
tolerate more "noise". Where does the dfsr=1 come from in the first place?
It seems random and is very far from HW default.

[U-Boot] [PATCH 2/3] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_{DFSR/FDR}

[Heiko Schocher]

Hello Joakim,

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.
> 
> For completeness, add CONFIG_FSL_I2C_CUSTOM_FDR too.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  drivers/i2c/fsl_i2c.c |   14 +++++++++++---
>  1 files changed, 11 insertions(+), 3 deletions(-)

Applied to u-boot-i2c testing

Thanks.

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 2/3] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_{DFSR/FDR}

[Timur Tabi]

Joakim Tjernlund wrote:

> +#ifdef CONFIG_FSL_I2C_CUSTOM_FDR
> +			fdr = CONFIG_FSL_I2C_CUSTOM_FDR;
> +			speed = i2c_clk / divider; /* Fake something */

How about setting 'speed' to CONFIG_SYS_I2C_SPEED?

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 2/3] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_{DFSR/FDR}

[Joakim Tjernlund]

>
> Joakim Tjernlund wrote:
>
> > +#ifdef CONFIG_FSL_I2C_CUSTOM_FDR
> > +         fdr = CONFIG_FSL_I2C_CUSTOM_FDR;
> > +         speed = i2c_clk / divider; /* Fake something */
>
> How about setting 'speed' to CONFIG_SYS_I2C_SPEED?

Naa, if you want that you just pass CONFIG_SYS_I2C_SPEED to
i2c_init(), which is the default.

[U-Boot] [PATCH 1/3] fsl_i2c: Wait for STOP condition to propagate

[Heiko Schocher]

Hello Joakim,

Joakim Tjernlund wrote:
> 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(-)

Applied to u-boot-i2c testing

I applied your 3 patches at the moment to the u-boot-i2c testing branch,
in the hope to see some "Tested-by" ;-)

Thanks.

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 1/3] fsl_i2c: Wait for STOP condition to propagate

[Joakim Tjernlund]

>
> Hello Joakim,
>
> Joakim Tjernlund wrote:
> > 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(-)
>
> Applied to u-boot-i2c testing
>
> I applied your 3 patches at the moment to the u-boot-i2c testing branch,
> in the hope to see some "Tested-by" ;-)

Great, hopefully you will get some Tested-by from Wolfgang G. and Timur

[U-Boot] [PATCH 1/3] fsl_i2c: Wait for STOP condition to propagate

[Wolfgang Denk]

Dear Joakim Tjernlund,

In message <1253178437-32398-1-git-send-email-Joakim.Tjernlund@transmode.se> you wrote:
> 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(-)

Could you _please_ start adding some version information to your
patches, and a list of changes between the versions?

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Crash programs fail because they are based on the theory  that,  with
nine women pregnant, you can get a baby a month.  - Wernher von Braun

[U-Boot] [PATCH 1/3] fsl_i2c: Wait for STOP condition to propagate

[Joakim Tjernlund]

>
> Dear Joakim Tjernlund,
>
> In message <1253178437-32398-1-git-send-email-Joakim.Tjernlund@transmode.se> you wrote:
> > 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(-)
>
> Could you _please_ start adding some version information to your
> patches, and a list of changes between the versions?
>
> Thanks.

Yes, that was a bit sloppy on my part, sorry. What do you want
me to do now?
Resend the whole series or do I get away with it this time?
All but the last patch is in u-boot-i2c testing(by Heiko) repo already.

[U-Boot] [PATCH 1/3] fsl_i2c: Wait for STOP condition to propagate

[Heiko Schocher]

Hello Joakim,

Joakim Tjernlund wrote:
>> Dear Joakim Tjernlund,
>>
>> In message <1253178437-32398-1-git-send-email-Joakim.Tjernlund@transmode.se> you wrote:
>>> 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(-)
>> Could you _please_ start adding some version information to your
>> patches, and a list of changes between the versions?
>>
>> Thanks.
> 
> Yes, that was a bit sloppy on my part, sorry. What do you want
> me to do now?
> Resend the whole series or do I get away with it this time?
> All but the last patch is in u-boot-i2c testing(by Heiko) repo already.

You mean with last patch, this patch:?

http://lists.denx.de/pipermail/u-boot/2009-September/061187.html

If, so, I think, I add it also to the u-boot-i2c testing branch,
and it is OK for now, but please for further patches, add some
information as Wolfgang suggested.

thanks

bye
Heiko

[U-Boot] [PATCH 1/3] fsl_i2c: Wait for STOP condition to propagate

[Joakim Tjernlund]

Heiko Schocher <heiko.schocher@invitel.hu> wrote on 23/09/2009 11:02:09:
>
> Hello Joakim,
>
> Joakim Tjernlund wrote:
> >> Dear Joakim Tjernlund,
> >>
> >> In message <1253178437-32398-1-git-send-email-
> Joakim.Tjernlund at transmode.se> you wrote:
> >>> 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(-)
> >> Could you _please_ start adding some version information to your
> >> patches, and a list of changes between the versions?
> >>
> >> Thanks.
> >
> > Yes, that was a bit sloppy on my part, sorry. What do you want
> > me to do now?
> > Resend the whole series or do I get away with it this time?
> > All but the last patch is in u-boot-i2c testing(by Heiko) repo already.
>
> You mean with last patch, this patch:?
>
> http://lists.denx.de/pipermail/u-boot/2009-September/061187.html

Yes, that is the last one. I don't see it in testing though.
I also think that
fsl_i2c: Wait for STOP condition to propagate, fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_{DFSR/FDR}
and fsl_i2c: Wait for STOP condition to propagate can go to WD

The fsl_i2c: Impl. AN2919, rev 5 to calculate FDR/DFSR can wait a bit longer to
get some more testing if you think it is needed.

    Jocke

[U-Boot] [PATCH 1/3] fsl_i2c: Wait for STOP condition to propagate

[Heiko Schocher]

Hello Joakim,

Joakim Tjernlund wrote:
> Heiko Schocher <heiko.schocher@invitel.hu> wrote on 23/09/2009 11:02:09:
>> Hello Joakim,
>>
>> Joakim Tjernlund wrote:
>>>> Dear Joakim Tjernlund,
>>>>
>>>> In message <1253178437-32398-1-git-send-email-
>> Joakim.Tjernlund at transmode.se> you wrote:
>>>>> 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(-)
>>>> Could you _please_ start adding some version information to your
>>>> patches, and a list of changes between the versions?
>>>>
>>>> Thanks.
>>> Yes, that was a bit sloppy on my part, sorry. What do you want
>>> me to do now?
>>> Resend the whole series or do I get away with it this time?
>>> All but the last patch is in u-boot-i2c testing(by Heiko) repo already.
>> You mean with last patch, this patch:?
>>
>> http://lists.denx.de/pipermail/u-boot/2009-September/061187.html
> 
> Yes, that is the last one. I don't see it in testing though.

applied to u-boot-i2c testing

> I also think that
> fsl_i2c: Wait for STOP condition to propagate, fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_{DFSR/FDR}
> and fsl_i2c: Wait for STOP condition to propagate can go to WD
> 
> The fsl_i2c: Impl. AN2919, rev 5 to calculate FDR/DFSR can wait a bit longer to
> get some more testing if you think it is needed.

I wait for the end of the week (I hope, I find time to make some tests
too), and send a pull request on monday to Wolfgang.

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 1/3] fsl_i2c: Wait for STOP condition to propagate

[Joakim Tjernlund]

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;
-- 
1.6.4.2