[U-Boot] fsl_i2c: increase I2C timeout values and make them configurable
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
On Wed, Sep 9, 2009 at 4:19 AM, Joakim Tjernlundjoakim.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@gmail.com wrote on 09/09/2009 16:24:15:
On Wed, Sep 9, 2009 at 4:19 AM, Joakim Tjernlundjoakim.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
timur.tabi@gmail.com wrote on 09/09/2009 16:24:15:
On Wed, Sep 9, 2009 at 4:19 AM, Joakim Tjernlundjoakim.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
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 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
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 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
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 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?
From AN2919, chap. 4.1:
C <= 50*T, C is dfsr and T is i2c_period in nano seconds.
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
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 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
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 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.
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
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?
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.
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
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.
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.
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
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.
JockePS. 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/i2...
Wolfgang.
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 nanosecondsand 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 theseconditions 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.
JockePS. 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 AN2819Where 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/i2...
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
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
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.
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
participants (5)
-
Detlev Zundel -
Joakim Tjernlund -
Timur Tabi -
Wolfgang Grandegger -
Wolfgang Grandegger