[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Joakim Tjernlund]

I wonder if this hides another problem too.
if the timeout hits, -1 is returned.

Then in i2c_read()/i2c_write() you have:
	if (i2c_wait4bus() >= 0
	    && i2c_write_addr(dev, I2C_WRITE_BIT, 0) != 0
	    && __i2c_write(&a[4 - alen], alen) == alen)
		i = 0; /* No error so far */
notice how the latter part, i2c_write_addr(dev, I2C_WRITE_BIT, 0) != 0
 && __i2c_write(&a[4 - alen], alen) == alen)
is ignored(never called) when i2c_wait4bus()  returns -1

 Jocke

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Timur Tabi]

On Wed, Sep 9, 2009 at 4:19 AM, Joakim
Tjernlund<joakim.tjernlund@transmode.se> wrote:
>
> I wonder if this hides another problem too.
> if the timeout hits, -1 is returned.
>
> Then in i2c_read()/i2c_write() you have:
> ? ? ? ?if (i2c_wait4bus() >= 0
> ? ? ? ? ? ?&& i2c_write_addr(dev, I2C_WRITE_BIT, 0) != 0
> ? ? ? ? ? ?&& __i2c_write(&a[4 - alen], alen) == alen)
> ? ? ? ? ? ? ? ?i = 0; /* No error so far */
> notice how the latter part, i2c_write_addr(dev, I2C_WRITE_BIT, 0) != 0
> ?&& __i2c_write(&a[4 - alen], alen) == alen)
> is ignored(never called) when i2c_wait4bus() ?returns -1

Yeah, that is a bit odd.  It looks like it was supposed to be some
short-cut way to avoid multiple if-then-else clauses.

I wouldn't say my patch is *hiding* another problem -- that code looks
broken either way.

Someone should probably fix it to look like this:

if (i2c_wait4bus() < 0)
    return -1;

if (!i2c_write_addr(dev, I2C_WRITE_BIT, 0))
    return -1;

if (__i2c_write(&a[4 - alen], alen) != alen)
    return -1;

and so on.
		i = 0; /* No error so far */

-- 
Timur Tabi
Linux kernel developer@Freescale

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Joakim Tjernlund]

timur.tabi at gmail.com wrote on 09/09/2009 16:24:15:
>
> On Wed, Sep 9, 2009 at 4:19 AM, Joakim
> Tjernlund<joakim.tjernlund@transmode.se> wrote:
> >
> > I wonder if this hides another problem too.
> > if the timeout hits, -1 is returned.
> >
> > Then in i2c_read()/i2c_write() you have:
> > ? ? ? ?if (i2c_wait4bus() >= 0
> > ? ? ? ? ? ?&& i2c_write_addr(dev, I2C_WRITE_BIT, 0) != 0
> > ? ? ? ? ? ?&& __i2c_write(&a[4 - alen], alen) == alen)
> > ? ? ? ? ? ? ? ?i = 0; /* No error so far */
> > notice how the latter part, i2c_write_addr(dev, I2C_WRITE_BIT, 0) != 0
> > ?&& __i2c_write(&a[4 - alen], alen) == alen)
> > is ignored(never called) when i2c_wait4bus() ?returns -1
>
> Yeah, that is a bit odd.  It looks like it was supposed to be some
> short-cut way to avoid multiple if-then-else clauses.
>
> I wouldn't say my patch is *hiding* another problem -- that code looks
> broken either way.

Yes, bad wording on my part.

>
> Someone should probably fix it to look like this:
>
> if (i2c_wait4bus() < 0)
>     return -1;

I suspect that this won't work in the long run. If
i2c_wait4bus() times out, the bus is likely stuck and will
never recover. Probably best to ignore these errors and reset the controller
and try again or something ..

>
> if (!i2c_write_addr(dev, I2C_WRITE_BIT, 0))
>     return -1;
>
> if (__i2c_write(&a[4 - alen], alen) != alen)
>     return -1;
>
> and so on.
>       i = 0; /* No error so far */
>
> --
> Timur Tabi
> Linux kernel developer at Freescale
>

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Joakim Tjernlund]

> timur.tabi at gmail.com wrote on 09/09/2009 16:24:15:
> >
> > On Wed, Sep 9, 2009 at 4:19 AM, Joakim
> > Tjernlund<joakim.tjernlund@transmode.se> wrote:
> > >
> > > I wonder if this hides another problem too.
> > > if the timeout hits, -1 is returned.
> > >
> > > Then in i2c_read()/i2c_write() you have:
> > > ? ? ? ?if (i2c_wait4bus() >= 0
> > > ? ? ? ? ? ?&& i2c_write_addr(dev, I2C_WRITE_BIT, 0) != 0
> > > ? ? ? ? ? ?&& __i2c_write(&a[4 - alen], alen) == alen)
> > > ? ? ? ? ? ? ? ?i = 0; /* No error so far */
> > > notice how the latter part, i2c_write_addr(dev, I2C_WRITE_BIT, 0) != 0
> > > ?&& __i2c_write(&a[4 - alen], alen) == alen)
> > > is ignored(never called) when i2c_wait4bus() ?returns -1
> >
> > Yeah, that is a bit odd.  It looks like it was supposed to be some
> > short-cut way to avoid multiple if-then-else clauses.
> >
> > I wouldn't say my patch is *hiding* another problem -- that code looks
> > broken either way.
>
> Yes, bad wording on my part.
>
> >
> > Someone should probably fix it to look like this:
> >
> > if (i2c_wait4bus() < 0)
> >     return -1;
>
> I suspect that this won't work in the long run. If
> i2c_wait4bus() times out, the bus is likely stuck and will
> never recover. Probably best to ignore these errors and reset the controller
> and try again or something ..
>
> >
> > if (!i2c_write_addr(dev, I2C_WRITE_BIT, 0))
> >     return -1;
> >
> > if (__i2c_write(&a[4 - alen], alen) != alen)
> >     return -1;
> >
> > and so on.
> >       i = 0; /* No error so far */
> >
> > --
> > Timur Tabi
> > Linux kernel developer at Freescale
> >

BTW, the fdr and dfsr calculations appears totally bogus. It seems
like the table is taken from some examples in AN2919 and it is pure luck
that it works most of the time. For me it does not work 100%, instead I get
random errors which hangs both the controller and the RTC device.

       Jocke

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Timur Tabi]

Joakim Tjernlund wrote:

> BTW, the fdr and dfsr calculations appears totally bogus. It seems
> like the table is taken from some examples in AN2919 and it is pure luck
> that it works most of the time. For me it does not work 100%, instead I get
> random errors which hangs both the controller and the RTC device.

Well, the problem is that for a given frequency, there are several values of fdr/dfsr that theoretically work.  So I picked what I thought would be good values for dfsr, but maybe it's not possible to pick such values.

A while back, someone posted a version of this code that computed the values of fdr/dfsr.  I nack'd that patch because I thought the algorithm was too convoluted, but perhaps what we really need is a combination of sorts.  The code should read the value of DFSR from the register, and then calculate an appropriate FDR to go with it.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Joakim Tjernlund]

Timur Tabi <timur@freescale.com> wrote on 10/09/2009 15:07:36:
>
> Joakim Tjernlund wrote:
>
> > BTW, the fdr and dfsr calculations appears totally bogus. It seems
> > like the table is taken from some examples in AN2919 and it is pure luck
> > that it works most of the time. For me it does not work 100%, instead I get
> > random errors which hangs both the controller and the RTC device.
>
> Well, the problem is that for a given frequency, there are several values of
> fdr/dfsr that theoretically work.  So I picked what I thought would be good
> values for dfsr, but maybe it's not possible to pick such values.

I don't think it is. The current values only works for good i2c buses.
Our have too high pullups so rise time is horrible. It will be fixed
but we need to handle the old boards too.

>
> A while back, someone posted a version of this code that computed the values
> of fdr/dfsr.  I nack'd that patch because I thought the algorithm was too

Not so sure about that, but I haven't tried to calc it generally.

> convoluted, but perhaps what we really need is a combination of sorts.  The
> code should read the value of DFSR from the register, and then calculate an
> appropriate FDR to go with it.

Yes, and a way to #define wanted DFSR. From there one can select
one of the 4 tables listed in AN2919. The sum of tabels will be rather big though
so I suspect it will less code to calculate the FDR/DFSR.

   Jocke

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Timur Tabi]

Joakim Tjernlund wrote:

>> A while back, someone posted a version of this code that computed the values
>> of fdr/dfsr.  I nack'd that patch because I thought the algorithm was too
> 
> Not so sure about that, but I haven't tried to calc it generally.

A quick way to check this is to figure out which dfsr/fdr values the i2c code uses, and then modify that entry in the table with a different pair.  You'd have to manually calculate the correct value of fdr for the given dfsr.

> Yes, and a way to #define wanted DFSR. From there one can select
> one of the 4 tables listed in AN2919. The sum of tabels will be rather big though
> so I suspect it will less code to calculate the FDR/DFSR.

In this case, you're right.  But so far, you're the only person who's complained to me that the current code doesn't work, and you already admitted that your board is broken, so I don't really see the need to change the code.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Joakim Tjernlund]

Timur Tabi <timur@freescale.com> wrote on 10/09/2009 15:29:35:
>
> Joakim Tjernlund wrote:
>
> >> A while back, someone posted a version of this code that computed the values
> >> of fdr/dfsr.  I nack'd that patch because I thought the algorithm was too
> >
> > Not so sure about that, but I haven't tried to calc it generally.
>
> A quick way to check this is to figure out which dfsr/fdr values the i2c code
> uses, and then modify that entry in the table with a different pair.  You'd
> have to manually calculate the correct value of fdr for the given dfsr.

The code was using 1, most entries has 1 as DFSR so that is no surprise.
I have calculated DFSR and max is 6 for my 8321, CSB=133.332 MHz, board.
Using this value works for me.

>
> > Yes, and a way to #define wanted DFSR. From there one can select
> > one of the 4 tables listed in AN2919. The sum of tabels will be rather big though
> > so I suspect it will less code to calculate the FDR/DFSR.
>
> In this case, you're right.  But so far, you're the only person who's
> complained to me that the current code doesn't work, and you already admitted
> that your board is broken, so I don't really see the need to change the code.

Come on, just because my board is somewhat broken, it doesn't mean the
driver is correct. If I define my speed to 100KHz I get
a DFSR of 22, way over what is allowed for my board.

It "works" because nobody has noticed the occasional error and/or doesn't
operate in a noisy env.

     Jocke

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Timur Tabi]

Joakim Tjernlund wrote:

> Come on, just because my board is somewhat broken, it doesn't mean the
> driver is correct. If I define my speed to 100KHz I get
> a DFSR of 22, way over what is allowed for my board.

Why is a value of 22 over what is allowed on the board?  I was under the impression that DFSR and FDR are just two values that work together to create a divider.  Is there something special about DFSR?



-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Joakim Tjernlund]

Timur Tabi <timur@freescale.com> wrote on 10/09/2009 17:22:38:
>
> Joakim Tjernlund wrote:
>
> > Come on, just because my board is somewhat broken, it doesn't mean the
> > driver is correct. If I define my speed to 100KHz I get
> > a DFSR of 22, way over what is allowed for my board.
>
> Why is a value of 22 over what is allowed on the board?  I was under the
> impression that DFSR and FDR are just two values that work together to create
> a divider.  Is there something special about DFSR?

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Timur Tabi]

Joakim Tjernlund wrote:
> From AN2919, chap. 4.1:
> C <= 50*T, C is dfsr and T is i2c_period in nano seconds.

Argh, my copy of AN2919 is old!  Mine doesn't have any of this stuff in it.

-- 
Timur Tabi
Linux kernel developer@Freescale

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Joakim Tjernlund]

Timur Tabi <timur@freescale.com> wrote on 10/09/2009 15:29:35:
>
> Joakim Tjernlund wrote:
>
> >> A while back, someone posted a version of this code that computed the values
> >> of fdr/dfsr.  I nack'd that patch because I thought the algorithm was too
> >
> > Not so sure about that, but I haven't tried to calc it generally.
>
> A quick way to check this is to figure out which dfsr/fdr values the i2c code
> uses, and then modify that entry in the table with a different pair.  You'd
> have to manually calculate the correct value of fdr for the given dfsr.
>
> > Yes, and a way to #define wanted DFSR. From there one can select
> > one of the 4 tables listed in AN2919. The sum of tabels will be rather big though
> > so I suspect it will less code to calculate the FDR/DFSR.
>
> In this case, you're right.  But so far, you're the only person who's
> complained to me that the current code doesn't work, and you already admitted
> that your board is broken, so I don't really see the need to change the code.

Looking a bit harder at the table I don't understand some entries, where does
the entries with dfsr != 1 come from? They don't look like any table in AN2919

  Jocke

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Timur Tabi]

Joakim Tjernlund wrote:

> Looking a bit harder at the table I don't understand some entries, where does
> the entries with dfsr != 1 come from? They don't look like any table in AN2919

They're all calculated.  I entered the algorithm into a spreadsheet and determined every possible combination of DFSR and FDR.  The values of DFSR==22 are for frequencies that are normally not published.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Joakim Tjernlund]

Timur Tabi <timur@freescale.com> wrote on 10/09/2009 17:26:29:
>
> Joakim Tjernlund wrote:
>
> > Looking a bit harder at the table I don't understand some entries, where does
> > the entries with dfsr != 1 come from? They don't look like any table in AN2919
>
> They're all calculated.  I entered the algorithm into a spreadsheet and
> determined every possible combination of DFSR and FDR.  The values of DFSR==22
> are for frequencies that are normally not published.

This calculation does not seem to match AN2919.

Suppose one used only Table 7(almost what we have if you exclude dfsr!= 1)
Table 7 is valid for 1 <= dfsr <=5 so how about replacing the current dfsr
with:
#ifdef __PPC__
			u8 dfsr;
			dfsr = (5*(i2c_clk/1000))/(100000);
			if (dfsr > 5)
				dfsr = 5;
			if (!dfsr)
				dfsr = 1;
			debug("i2c_clk:%d, dfsr:%d\n", i2c_clk, dfsr);
			writeb(dfsr, &dev->dfsrr);	/* set default filter */
#endif

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Timur Tabi]

Joakim Tjernlund wrote:

> This calculation does not seem to match AN2919.

When I wrote the code, AN2919 was much smaller than what you have today.

> Suppose one used only Table 7(almost what we have if you exclude dfsr!= 1)
> Table 7 is valid for 1 <= dfsr <=5 so how about replacing the current dfsr
> with:
> #ifdef __PPC__
> 			u8 dfsr;
> 			dfsr = (5*(i2c_clk/1000))/(100000);
> 			if (dfsr > 5)
> 				dfsr = 5;
> 			if (!dfsr)
> 				dfsr = 1;
> 			debug("i2c_clk:%d, dfsr:%d\n", i2c_clk, dfsr);
> 			writeb(dfsr, &dev->dfsrr);	/* set default filter */
> #endif

The value of FDR is dependent on the value of DFSR, so if I calculate DFSR, I have to also calculate FDR.  This means the table goes away.  I'm okay with that (since my table is no longer a viable approach, it seems), but it's more work than I'm willing to do at the moment.  Especically since this is going to need a lot of testing before I'm willing to push it.

Another way of handling this is to edit the table so that it only includes values of DFSR between 1 and 5, which is (unfortunately) *every* entry with a DFSR != 1.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Joakim Tjernlund]

Timur Tabi <timur@freescale.com> wrote on 10/09/2009 18:13:03:
>
> Joakim Tjernlund wrote:
>
> > This calculation does not seem to match AN2919.
>
> When I wrote the code, AN2919 was much smaller than what you have today.
>
> > Suppose one used only Table 7(almost what we have if you exclude dfsr!= 1)
> > Table 7 is valid for 1 <= dfsr <=5 so how about replacing the current dfsr
> > with:
> > #ifdef __PPC__
> >          u8 dfsr;
> >          dfsr = (5*(i2c_clk/1000))/(100000);
> >          if (dfsr > 5)
> >             dfsr = 5;
> >          if (!dfsr)
> >             dfsr = 1;
> >          debug("i2c_clk:%d, dfsr:%d\n", i2c_clk, dfsr);
> >          writeb(dfsr, &dev->dfsrr);   /* set default filter */
> > #endif
>
> The value of FDR is dependent on the value of DFSR, so if I calculate DFSR, I
> have to also calculate FDR.  This means the table goes away.  I'm okay with
> that (since my table is no longer a viable approach, it seems), but it's more
> work than I'm willing to do at the moment.  Especically since this is going to
> need a lot of testing before I'm willing to push it.

You can manage with the 4 tables listed in the end, they cover all dfsr's, but if
you can swing an algorithm that is even better.

>
> Another way of handling this is to edit the table so that it only includes
> values of DFSR between 1 and 5, which is (unfortunately) *every* entry with a DFSR != 1.

Exactly, that is what I am proposing and that is what the code above does.
The entries with DFSR != 1 are likely wrong anyway and is a better fit than todays
method.

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Joakim Tjernlund]

Timur Tabi <timur@freescale.com> wrote on 10/09/2009 18:13:03:
>
> Joakim Tjernlund wrote:
>
> > This calculation does not seem to match AN2919.
>
> When I wrote the code, AN2919 was much smaller than what you have today.
>
> > Suppose one used only Table 7(almost what we have if you exclude dfsr!= 1)
> > Table 7 is valid for 1 <= dfsr <=5 so how about replacing the current dfsr
> > with:
> > #ifdef __PPC__
> >          u8 dfsr;
> >          dfsr = (5*(i2c_clk/1000))/(100000);
> >          if (dfsr > 5)
> >             dfsr = 5;
> >          if (!dfsr)
> >             dfsr = 1;
> >          debug("i2c_clk:%d, dfsr:%d\n", i2c_clk, dfsr);
> >          writeb(dfsr, &dev->dfsrr);   /* set default filter */
> > #endif
>
> The value of FDR is dependent on the value of DFSR, so if I calculate DFSR, I
> have to also calculate FDR.  This means the table goes away.  I'm okay with
> that (since my table is no longer a viable approach, it seems), but it's more
> work than I'm willing to do at the moment.  Especically since this is going to
> need a lot of testing before I'm willing to push it.

I could not resist so I did a quick start:
#include <stdlib.h>
#include <stdio.h>
#define I2C_CLK 133332000
int main(int argc, char *argv[])
{
  unsigned long A,B,C;
  unsigned long divisor, req_div;
  unsigned long curr_div = ~0;
  unsigned long speed;

  if (argc != 2) {
    printf("%s <speed in HZ>\n", argv[0]);
    exit(1);
  }
  speed = atol(argv[1]);
  req_div = I2C_CLK/speed;
  C = (5*(I2C_CLK/1000))/(100000);
  if (!C)
    C = 1;

  for(A=10;A <= 30; A+=2) {
    for (B=16; B<=2048;B*=2) {
      divisor = B * (A + (3*C/B)*2);
      if (divisor > req_div && divisor < curr_div) {
	printf("div:%d, A:%d, B:%d\n", divisor, A, B);
	curr_div = divisor;
      }
    }
    if (A == 20)
      A+=2;
    if (A == 24)
      A+=4;
  }
  printf("\nreq_div:%d, curr_div:%d, DFSR:%d\n", req_div, curr_div, C);
}

This should be useful I hope. The special treatment for A == 20 and A == 24
is a bit strange though, do they really jump like that?

     Jocke

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Joakim Tjernlund]

>
> Timur Tabi <timur@freescale.com> wrote on 10/09/2009 18:13:03:
> >
> > Joakim Tjernlund wrote:
> >
> > > This calculation does not seem to match AN2919.
> >
> > When I wrote the code, AN2919 was much smaller than what you have today.
> >
> > > Suppose one used only Table 7(almost what we have if you exclude dfsr!= 1)
> > > Table 7 is valid for 1 <= dfsr <=5 so how about replacing the current dfsr
> > > with:
> > > #ifdef __PPC__
> > >          u8 dfsr;
> > >          dfsr = (5*(i2c_clk/1000))/(100000);
> > >          if (dfsr > 5)
> > >             dfsr = 5;
> > >          if (!dfsr)
> > >             dfsr = 1;
> > >          debug("i2c_clk:%d, dfsr:%d\n", i2c_clk, dfsr);
> > >          writeb(dfsr, &dev->dfsrr);   /* set default filter */
> > > #endif
> >
> > The value of FDR is dependent on the value of DFSR, so if I calculate DFSR, I
> > have to also calculate FDR.  This means the table goes away.  I'm okay with
> > that (since my table is no longer a viable approach, it seems), but it's more
> > work than I'm willing to do at the moment.  Especically since this is going to
> > need a lot of testing before I'm willing to push it.
>
> I could not resist so I did a quick start:
[SNIP]

So I completed the function, here it is:

#include <stdlib.h>
#include <stdio.h>
#define I2C_CLK 133332000

int main(int argc, char *argv[])
{
  unsigned long A,B,C;
  unsigned long Ga,Gb;
  unsigned long divisor, req_div;
  unsigned long est_div, bin_Gb, bin_Ga, est_FDR;
  unsigned long speed;

  if (argc != 2) {
    printf("%s <speed in HZ>\n", argv[0]);
    exit(1);
  }
  speed = atol(argv[1]);
  req_div = I2C_CLK/speed;
  C = (5*(I2C_CLK/1000))/(100000);
  if (!C)
    C = 1;
  est_div = ~0;
  for(Ga=0x4, A=10; A<=30; Ga++, A+=2) {
    for (Gb=0; Gb<8; Gb++) {
      B = 16 << Gb;
      divisor = B * (A + ((3*C)/B)*2);
      if (divisor >= req_div && divisor < est_div) {
	est_div = divisor;
	bin_Gb = Gb << 2;
	bin_Ga = (Ga & 0x3) | ((Ga & 0x4) << 3);
	est_FDR = bin_Gb | bin_Ga;
	//printf("div:%d, A:%d, B:%d b:%x, a:%x\n", divisor, A, B, Gb, Ga);
	//printf("bin_Gb:0x%x, bin_Ga:0x%x\n", bin_Gb, bin_Ga);
	//printf("FDR:0x%.2x, div:%d\n", est_FDR, est_div);
	//printf("speed:%d\n\n", I2C_CLK/est_div);
#if 0
/* Condition 2 not accounted for */
	{
	  unsigned long T = 1000000000/I2C_CLK;

	  printf("%d*%d >= Tr\n", (B-3*C), T);
	  printf("%d >= Tr\n", (B-3*C)* T);
	}
#endif
      }
    }
/* The old table in u-boot miss this */
    if (A == 20)
      A+=2;
    if (A == 24)
      A+=4;
  }

#if 1
  printf("\nreq_div:%d, est_div:%d, DFSR:%d\n", req_div, est_div, C);
  printf("bin_Gb:0x%x, bin_Ga:0x%x\n", bin_Gb, bin_Ga);
  printf("FDR:0x%.2x\n", est_FDR);
  printf("speed:%d\n", I2C_CLK/est_div);
#endif
}

This will generate the same divisor tables as AN2919, tables 6-9.
I do not take condition 2 into consideration as it not clear how to
deal with it and it does not seem to have an significant impact.

What do you think?

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Timur Tabi]

Joakim Tjernlund wrote:

> This will generate the same divisor tables as AN2919, tables 6-9.
> I do not take condition 2 into consideration as it not clear how to
> deal with it and it does not seem to have an significant impact.
> 
> What do you think?

I really don't have time to deal with it right now.  Sorry.


-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Detlev Zundel]

Hi Timur,

> Joakim Tjernlund wrote:
>
>> This will generate the same divisor tables as AN2919, tables 6-9.
>> I do not take condition 2 into consideration as it not clear how to
>> deal with it and it does not seem to have an significant impact.
>> 
>> What do you think?
>
> I really don't have time to deal with it right now.  Sorry.

Wolfgang (I put him on CC) was the last to propose such an algorithm
IIRC.  So maybe he has an opinion?

Thanks
  Detlev

-- 
Some people seem to think that C is a real programming language, but they are
sadly mistaken.  It really is about writing almost-portable assembly language
[...]               -- Linus Torvalds 10404265599082718160noreply at blogger.com
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Wolfgang Grandegger]

Detlev Zundel wrote:
> Hi Timur,
> 
>> Joakim Tjernlund wrote:
>>
>>> This will generate the same divisor tables as AN2919, tables 6-9.
>>> I do not take condition 2 into consideration as it not clear how to
>>> deal with it and it does not seem to have an significant impact.
>>>
>>> What do you think?
>> I really don't have time to deal with it right now.  Sorry.
> 
> Wolfgang (I put him on CC) was the last to propose such an algorithm
> IIRC.  So maybe he has an opinion?

I did not follow the thread yet, sorry. I implemented AN2819 for Linux
(see http://lxr.linux.no/#linux+v2.6.31/drivers/i2c/busses/i2c-mpc.c)
some time ago using Timur's table approach. But there is no difference
between the table and the algorithm to calculate the value. The table is
actually derived from the same algorithm.

Wolfgang.

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Timur Tabi]

Wolfgang Grandegger wrote:

> I did not follow the thread yet, sorry. I implemented AN2819 for Linux
> (see http://lxr.linux.no/#linux+v2.6.31/drivers/i2c/busses/i2c-mpc.c)
> some time ago using Timur's table approach. But there is no difference
> between the table and the algorithm to calculate the value. The table is
> actually derived from the same algorithm.

The problem with the table is that it does not allow for flexibility in choosing dfsr.  When I implemented the table code, I did not think that this was a problem, but apparently it is.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Joakim Tjernlund]

>
> Wolfgang Grandegger wrote:
>
> > I did not follow the thread yet, sorry. I implemented AN2819 for Linux
> > (see http://lxr.linux.no/#linux+v2.6.31/drivers/i2c/busses/i2c-mpc.c)
> > some time ago using Timur's table approach. But there is no difference
> > between the table and the algorithm to calculate the value. The table is
> > actually derived from the same algorithm.
>
> The problem with the table is that it does not allow for flexibility in
> choosing dfsr.  When I implemented the table code, I did not think that this
> was a problem, but apparently it is.

Yes, it is a problem for us as our board is out of spec. The rise time is way off
spec. By trial and error with the DFSR/FDR I managed to get a stable connection.
What is less funny though is that I need to program FDR/DFSR differently
in u-boot resp. kernel. to make it work.
I suspect it is due to kernel being IRQ driven and has longer pause
between chars, but it is a guess.

In any case, the revised AN2819 dictates a different algorithm but I feel
it is a bit incomplete w.r.t Condition 2:

    ? Condition 2: B ? T ? tI2CRM + 3 ? C ? T.
Given a suitable value of DFSR, chosen to satisfy Condition 1, this inequality must also be met to guarantee
that the SCL frequency matches the SCL frequency calculated by the divider equation. It is important to
note that tI2CRM is the measured rise time of the SCL signal, which is defined as the time for the signal to
rise from 10% to 70% of VCC.

                                                      NOTE
                   Note that the rise time must not exceed 300 nanoseconds and that the above
                   two conditions must both be satisfied to ensure that the actual SCL
                   frequency values align with the calculated values. By meeting these
                   conditions, the measured SCL frequency will match the calculated
                   frequency to within 5 kHz. Ignoring either of these conditions may result in
                   larger discrepancies between these frequency values.

How important is Condition 2 and what to do with rise times > 300 ns? The MAX rise time
for 100 KHz is 1000 ns so there is a gap here.

My testing suggests that this is not important. Bigger DFSR, in my case 0x6 or 0x10, is key
to get a stable I2C bus.

        Jocke
PS.
    Wolfgang, I sent a test program to calculate the new DFSR/FDR values in the thread, you
    might find it useful if you are going to try out the new AN2819

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Wolfgang Grandegger]

Joakim Tjernlund wrote:
>> Wolfgang Grandegger wrote:
>>
>>> I did not follow the thread yet, sorry. I implemented AN2819 for Linux
>>> (see http://lxr.linux.no/#linux+v2.6.31/drivers/i2c/busses/i2c-mpc.c)
>>> some time ago using Timur's table approach. But there is no difference
>>> between the table and the algorithm to calculate the value. The table is
>>> actually derived from the same algorithm.
>> The problem with the table is that it does not allow for flexibility in
>> choosing dfsr.  When I implemented the table code, I did not think that this
>> was a problem, but apparently it is.
> 
> Yes, it is a problem for us as our board is out of spec. The rise time is way off
> spec. By trial and error with the DFSR/FDR I managed to get a stable connection.
> What is less funny though is that I need to program FDR/DFSR differently
> in u-boot resp. kernel. to make it work.
> I suspect it is due to kernel being IRQ driven and has longer pause
> between chars, but it is a guess.
> 
> In any case, the revised AN2819 dictates a different algorithm but I feel
> it is a bit incomplete w.r.t Condition 2:
> 
>     ? Condition 2: B ? T ? tI2CRM + 3 ? C ? T.
> Given a suitable value of DFSR, chosen to satisfy Condition 1, this inequality must also be met to guarantee
> that the SCL frequency matches the SCL frequency calculated by the divider equation. It is important to
> note that tI2CRM is the measured rise time of the SCL signal, which is defined as the time for the signal to
> rise from 10% to 70% of VCC.
> 
>                                                       NOTE
>                    Note that the rise time must not exceed 300 nanoseconds and that the above
>                    two conditions must both be satisfied to ensure that the actual SCL
>                    frequency values align with the calculated values. By meeting these
>                    conditions, the measured SCL frequency will match the calculated
>                    frequency to within 5 kHz. Ignoring either of these conditions may result in
>                    larger discrepancies between these frequency values.
> 
> How important is Condition 2 and what to do with rise times > 300 ns? The MAX rise time
> for 100 KHz is 1000 ns so there is a gap here.
> 
> My testing suggests that this is not important. Bigger DFSR, in my case 0x6 or 0x10, is key
> to get a stable I2C bus.
> 
>         Jocke
> PS.
>     Wolfgang, I sent a test program to calculate the new DFSR/FDR values in the thread, you
>     might find it useful if you are going to try out the new AN2819

Where do I find this test program. I just dig out my program to
calculate the table entries for the Linux i2c-mpc.c. It actually
reproduced Timur's (old) U-Boot values. Unfortunately, finding *good*
dfsr/fdr settings is no trivial and takes time. Till recently, the
i2c-mpc driver of Linux did use *fixed* save values as shown here:

  http://lxr.linux.no/#linux+v2.6.29/drivers/i2c/busses/i2c-mpc.c

And also with newer kernels, the table is only used if one of the
following I2C DTS properties is defined:

- fsl,preserve-clocking;
- clock-frequency = <400000>;

See
http://lxr.linux.no/#linux+v2.6.31/Documentation/powerpc/dts-bindings/fsl/i2c.txt

Wolfgang.

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Joakim Tjernlund]

Wolfgang Grandegger <wg@grandegger.com> wrote on 15/09/2009 13:53:13:
>
> Joakim Tjernlund wrote:
> >> Wolfgang Grandegger wrote:
> >>
> >>> I did not follow the thread yet, sorry. I implemented AN2819 for Linux
> >>> (see http://lxr.linux.no/#linux+v2.6.31/drivers/i2c/busses/i2c-mpc.c)
> >>> some time ago using Timur's table approach. But there is no difference
> >>> between the table and the algorithm to calculate the value. The table is
> >>> actually derived from the same algorithm.
> >> The problem with the table is that it does not allow for flexibility in
> >> choosing dfsr.  When I implemented the table code, I did not think that this
> >> was a problem, but apparently it is.
> >
> > Yes, it is a problem for us as our board is out of spec. The rise time is way off
> > spec. By trial and error with the DFSR/FDR I managed to get a stable connection.
> > What is less funny though is that I need to program FDR/DFSR differently
> > in u-boot resp. kernel. to make it work.
> > I suspect it is due to kernel being IRQ driven and has longer pause
> > between chars, but it is a guess.
> >
> > In any case, the revised AN2819 dictates a different algorithm but I feel
> > it is a bit incomplete w.r.t Condition 2:
> >
> >     ? Condition 2: B ? T ? tI2CRM + 3 ? C ? T.
> > Given a suitable value of DFSR, chosen to satisfy Condition 1, this
> inequality must also be met to guarantee
> > that the SCL frequency matches the SCL frequency calculated by the divider
> equation. It is important to
> > note that tI2CRM is the measured rise time of the SCL signal, which is
> defined as the time for the signal to
> > rise from 10% to 70% of VCC.
> >
> >                                                       NOTE
> >                    Note that the rise time must not exceed 300 nanoseconds
> and that the above
> >                    two conditions must both be satisfied to ensure that the actual SCL
> >                    frequency values align with the calculated values. By meeting these
> >                    conditions, the measured SCL frequency will match the calculated
> >                    frequency to within 5 kHz. Ignoring either of these
> conditions may result in
> >                    larger discrepancies between these frequency values.
> >
> > How important is Condition 2 and what to do with rise times > 300 ns? The
> MAX rise time
> > for 100 KHz is 1000 ns so there is a gap here.
> >
> > My testing suggests that this is not important. Bigger DFSR, in my case 0x6
> or 0x10, is key
> > to get a stable I2C bus.
> >
> >         Jocke
> > PS.
> >     Wolfgang, I sent a test program to calculate the new DFSR/FDR values in
> the thread, you
> >     might find it useful if you are going to try out the new AN2819
>
> Where do I find this test program.

In this thread. Attached for you convenience.

> I just dig out my program to
> calculate the table entries for the Linux i2c-mpc.c. It actually
> reproduced Timur's (old) U-Boot values. Unfortunately, finding *good*
> dfsr/fdr settings is no trivial and takes time.

That was what my program intends to do. Works quite well but isn't perfect(See attached file: fdr.c)

> Till recently, the
> i2c-mpc driver of Linux did use *fixed* save values as shown here:
>
>   http://lxr.linux.no/#linux+v2.6.29/drivers/i2c/busses/i2c-mpc.c
>
> And also with newer kernels, the table is only used if one of the
> following I2C DTS properties is defined:
>
> - fsl,preserve-clocking;

ehh, I figured preserve-clocking meant use whatever fdr/dfsr is already
set to.

> - clock-frequency = <400000>;
>
> See
> http://lxr.linux.no/#linux+v2.6.31/Documentation/powerpc/dts-bindings/fsl/i2c.txt

I am using 2.6.30 and I think it is fairly equal to yours.
I am not using either property above so the linux i2c-mpc. driver falls back
to fdr=0x31 and dfsr=0x10 and this works well. It is u-boot that isn't working.
However, I have found a few driver bugs in the u-boot driver and fixing those
makes the fsl-i2c.c driver work well again.

You can easily stress test I2C in u-boot by entering
date;date;date;date;date
and then press and hold Enter for a while.

      Jocke
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fdr.c
Type: application/octet-stream
Size: 1739 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090915/13fa2782/attachment.obj 

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Joakim Tjernlund]

>
> I am using 2.6.30 and I think it is fairly equal to yours.
> I am not using either property above so the linux i2c-mpc. driver falls back
> to fdr=0x31 and dfsr=0x10 and this works well. It is u-boot that isn't working.
> However, I have found a few driver bugs in the u-boot driver and fixing those
> makes the fsl-i2c.c driver work well again.

I just sent you two patches that addresses my problems, I hope you can have
a look.

The kernel driver should also be updated with the "wait for STOP on the bus"
patch.

  Jocke

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Wolfgang Grandegger]

Timur Tabi wrote:
> Wolfgang Grandegger wrote:
> 
>> I did not follow the thread yet, sorry. I implemented AN2819 for Linux
>> (see http://lxr.linux.no/#linux+v2.6.31/drivers/i2c/busses/i2c-mpc.c)
>> some time ago using Timur's table approach. But there is no difference
>> between the table and the algorithm to calculate the value. The table is
>> actually derived from the same algorithm.
> 
> The problem with the table is that it does not allow for flexibility in choosing dfsr.  When I implemented the table code, I did not think that this was a problem, but apparently it is.

What would be the criteria to choose dfsr, especially for a defined bus
frequency.

Wolfgang.

[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable

[Joakim Tjernlund]

>
> Timur Tabi wrote:
> > Wolfgang Grandegger wrote:
> >
> >> I did not follow the thread yet, sorry. I implemented AN2819 for Linux
> >> (see http://lxr.linux.no/#linux+v2.6.31/drivers/i2c/busses/i2c-mpc.c)
> >> some time ago using Timur's table approach. But there is no difference
> >> between the table and the algorithm to calculate the value. The table is
> >> actually derived from the same algorithm.
> >
> > The problem with the table is that it does not allow for flexibility in
> choosing dfsr.  When I implemented the table code, I did not think that this
> was a problem, but apparently it is.
>
> What would be the criteria to choose dfsr, especially for a defined bus
> frequency.

It is in the new AN2919:
   ? Condition 1: C ? 50 ? T.
This means that the product of the decimal equivalent of the value in the DFSR and the source clock period
must not exceed 50 ns. Thus, given a specific source clock period, the value in the DFSR must not be so
high that it violates this condition.

I just compared the u-boot fsl-i2c.c driver with the kernel one and I think u-boot is buggy.
One cannot disable I2C directly after a read/write of last byte. There has to be some time
for the controller to generate STOP.
There are other differences too that I am not sure if they are significant or not.

          Jocke