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

[U-Boot] [PATCH 1/2] 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..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;
-- 
1.6.4.2

[U-Boot] [PATCH 2/2] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_DFSR

[Joakim Tjernlund]

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

[U-Boot] [PATCH 2/2] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_DFSR

[Timur Tabi]

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.


-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 2/2] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_DFSR

[Joakim Tjernlund]

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

[U-Boot] [PATCH 2/2] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_DFSR

[Timur Tabi]

Joakim Tjernlund wrote:

> No, the impact on speed from DFSR is pretty small so it will
> be close enough. 

How small?

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 2/2] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_DFSR

[Joakim Tjernlund]

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?

[U-Boot] [PATCH 2/2] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_DFSR

[Joakim Tjernlund]

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

[U-Boot] [PATCH 2/2] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_DFSR

[Wolfgang Grandegger]

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.

[U-Boot] [PATCH 2/2] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_DFSR

[Joakim Tjernlund]

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

[U-Boot] [PATCH 2/2] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_DFSR

[Wolfgang Grandegger]

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.

[U-Boot] [PATCH 2/2] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_DFSR

[Joakim Tjernlund]

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

[U-Boot] [PATCH 2/2] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_DFSR

[Wolfgang Grandegger]

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.

[U-Boot] [PATCH 2/2] fsl_i2c: Add CONFIG_FSL_I2C_CUSTOM_DFSR

[Joakim Tjernlund]

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