[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c
Currently we define I2C_TIMEOUT like this:
#define I2C_TIMEOUT (CONFIG_SYS_HZ / 4)
I'm seeing some I2C instability on a new board I'm working on, especially with SPD. If I change the above to
#define I2C_TIMEOUT (CONFIG_SYS_HZ / 2)
The problems go away (or at least, so far appear to). Can someone tell me why we choose (CONFIG_SYS_HZ / 4) to begin with? The way we use I2C_TIMEOUT is confusing:
while (readb(&i2c_dev[i2c_bus_num]->sr) & I2C_SR_MBB) { if ((get_ticks() - timeval) > usec2ticks(I2C_TIMEOUT)) return -1; }
CONFIG_HZ is 1000, so I2C_TIMEOUT is equal to 250. However, the way it's used, 250 isn't the number of ticks per second, it's used as number of microseconds. If CONFIG_HZ is changed to 100, does that mean that we want to call usec2ticks(25)?
I think what we should be doing is this:
#define I2C_TIMEOUT 1000
Surely, one millisecond is not too long of a timeout?
Hello Timur,
Timur Tabi wrote:
Currently we define I2C_TIMEOUT like this:
#define I2C_TIMEOUT (CONFIG_SYS_HZ / 4)
I'm seeing some I2C instability on a new board I'm working on, especially with SPD. If I change the above to
#define I2C_TIMEOUT (CONFIG_SYS_HZ / 2)
The problems go away (or at least, so far appear to). Can someone tell me why we choose (CONFIG_SYS_HZ / 4) to begin with? The way we use I2C_TIMEOUT is confusing:
while (readb(&i2c_dev[i2c_bus_num]->sr) & I2C_SR_MBB) { if ((get_ticks() - timeval) > usec2ticks(I2C_TIMEOUT)) return -1; }
CONFIG_HZ is 1000, so I2C_TIMEOUT is equal to 250. However, the way it's used, 250 isn't the number of ticks per second, it's used as number of microseconds. If CONFIG_HZ is changed to 100, does that mean that we want to call usec2ticks(25)?
This seems like a Bug to me.
I think what we should be doing is this:
#define I2C_TIMEOUT 1000
Surely, one millisecond is not too long of a timeout?
I think this should be Ok. Can you provide a patch? Thanks for catching this.
bye Heiko
Dear Heiko Schocher,
In message 4AA0BEC5.3010505@denx.de you wrote:
CONFIG_HZ is 1000, so I2C_TIMEOUT is equal to 250. However, the way it's used, 250 isn't the number of ticks per second, it's used as number of microseconds. If CONFIG_HZ is changed to 100, does that mean that we want to call usec2ticks(25)?
This seems like a Bug to me.
I think what we should be doing is this:
#define I2C_TIMEOUT 1000
Surely, one millisecond is not too long of a timeout?
I think this should be Ok. Can you provide a patch?
There are actually two parts in Timur's mail:
1) First part is the question if the timeout, which is currently set to 250 us, should be raised to 1,000 us.
I cannot answer this question. I don't even understand why the i2c_wait4bus() function is needed at all.
2) The second part is if the timeout definition should be changed from being based on CONFIG_SYS_HZ to a plain numeric constant. Assuming I2C_TIMEOUT is intended to be a period of time (as the name suggests), then "(CONFIG_SYS_HZ / 4)" is clearly wrong as it's a frequency and not a time.
In this case, a comment should be added explainign what I2C_TIMEOUT means.
Best regards,
Wolfgang Denk
On Fri, Sep 4, 2009 at 4:25 AM, Wolfgang Denkwd@denx.de wrote:
There are actually two parts in Timur's mail:
- First part is the question if the timeout, which is currently set
to 250 us, should be raised to 1,000 us.
I cannot answer this question. I don't even understand why the i2c_wait4bus() function is needed at all.
Can you explain? I don't know enough about the I2C protocol. Why is i2c_wait4bus unnecessary?
- The second part is if the timeout definition should be changed
from being based on CONFIG_SYS_HZ to a plain numeric constant. Assuming I2C_TIMEOUT is intended to be a period of time (as the name suggests), then "(CONFIG_SYS_HZ / 4)" is clearly wrong as it's a frequency and not a time.
In this case, a comment should be added explainign what I2C_TIMEOUT means.
It took me a while to track it down, but I finally found the commit that make the change: SHA 09d318a8bb1444ec92e31cafcdba877eb9409e58, from Kumar about a year ago:
fsl_i2c: Use timebase timer functions instead of get_timer()
The current implementation of get_timer() is only really useful after we have relocated u-boot to memory. The i2c code is used before that as part of the SPD DDR setup.
We actually have a bug when using the get_timer() code before relocation because the .bss hasn't been setup and thus we could be reading/writing a random location (probably in flash).
Signed-off-by: Kumar Gala galak@kernel.crashing.org
Specifically, it does this:
while (readb(&i2c_dev[i2c_bus_num]->sr) & I2C_SR_MBB) { - if (get_timer(timeval) > I2C_TIMEOUT) { + if ((get_ticks() - timeval) > usec2ticks(I2C_TIMEOUT)) return -1; - } }
Kumar, any thoughts? Is there something sneaky going on here, or did you just misinterpret the value of I2C_TIMEOUT?
Dear Timur Tabi,
In message ed82fe3e0909040709u16adec47id77693ed53193db5@mail.gmail.com you wrote:
I cannot answer this question. I don't even understand why the i2c_wait4bus() function is needed at all.
Can you explain? I don't know enough about the I2C protocol. Why is i2c_wait4bus unnecessary?
Wrong Question. I don't know enough about the I2C protocol. Why is i2c_wait4bus necessary?
Kumar, any thoughts? Is there something sneaky going on here, or did you just misinterpret the value of I2C_TIMEOUT?
I guess I2C_TIMEOUT might always have been misinterpeted.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Wrong Question. I don't know enough about the I2C protocol. Why is i2c_wait4bus necessary?
Ok, why is it necessary?
Kumar, any thoughts? Is there something sneaky going on here, or did you just misinterpret the value of I2C_TIMEOUT?
I guess I2C_TIMEOUT might always have been misinterpeted.
I think the original code was correct, because it was counting clock ticks.
On Fri, 2009-09-04 at 10:12 -0500, Timur Tabi wrote:
Wolfgang Denk wrote:
Wrong Question. I don't know enough about the I2C protocol. Why is i2c_wait4bus necessary?
Ok, why is it necessary?
Freescale's I2C core supports multiple masters. I'd guess that i2c_wait4bus() is used to ensure the bus is not in use by a different master before initiating a read or write. Its polling the MBB status bit, which is automatically set/cleared when the controller sees a START/STOP which supports this.
If this is the case, the timeout should be the maximum (or reasonable maximum) time an I2C transaction could take.
Best, Peter
On Fri, 2009-09-04 at 10:30 -0500, Timur Tabi wrote:
Peter Tyser wrote:
If this is the case, the timeout should be the maximum (or reasonable maximum) time an I2C transaction could take.
How long is that? Is one millisecond good enough?
The timeout in i2c_wait4bus() could potentially be pretty large. The I2C bus could in theory be ran at very slow speeds, 1 cycle could be many bytes, etc. You'd have to dig into the spec to get a definitive answer about how long a maximum cycle is (if there is even a value), but I think it would have to be much longer than 1ms. Looks like the linux driver has a timeout of 1 second (for the equivalent of i2c_wait4bus()). I'd be fine with that for U-Boot too. 99% of boards don't have multiple masters so a large timeout shouldn't affect them at all.
As far as the I2C_TIMEOUT in general I'm not sure either:) 1ms might be OK, but for reference the SMBus has a specified timeout of minimum 25ms, max 35ms. I'd tend to lean towards the conservative side as for devices that are working correctly, they will never timeout. If i2c transactions are timing out, I'd be more concerned about why than the extra 2 seconds my board took to boot:) I guess the i2c probe command might take a while too, but that personally doesn't bother me.
In any case, I'd vote for a different, very large timeout value for i2c_wait4bus() and a few millisecond (or larger) timeout for I2C_TIMEOUT.
Best, Peter
Dear Timur Tabi,
In message 4AA132B3.3050004@freescale.com you wrote:
Peter Tyser wrote:
If this is the case, the timeout should be the maximum (or reasonable maximum) time an I2C transaction could take.
How long is that? Is one millisecond good enough?
Probably not. If you place a read request to a slow device it may take tens of milliseconds, or even longer - I have no idea. IIRC we had a box with a LCD display connected over I2C, which didn't ent into production as originally designed because writing the content took over 100 millisec.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Probably not. If you place a read request to a slow device it may take tens of milliseconds, or even longer - I have no idea. IIRC we had a box with a LCD display connected over I2C, which didn't ent into production as originally designed because writing the content took over 100 millisec.
Well, that's an extreme case that is board-specific. Perhaps I should do this:
#ifndef CONFIG_I2C_TIMEOUT #define CONFIG_I2C_TIMEOUT 1000 #endif
Keep in mind that so far, the number 250 has been good enough for every board to date. Why my current board is happier with 500 is a mystery to me.
Also, should we be using the same value for the timeout in i2c_wait4bus() and i2c_wait()? It looks like i2c_wait() should have a much shorter timeout than i2c_wait4bus()?
Dear Timur Tabi,
In message 4AA15EDE.6080706@freescale.com you wrote:
Well, that's an extreme case that is board-specific. Perhaps I should do this:
#ifndef CONFIG_I2C_TIMEOUT #define CONFIG_I2C_TIMEOUT 1000 #endif
OK.
Also, should we be using the same value for the timeout in i2c_wait4bus() and i2c_wait()? It looks like i2c_wait() should have a much shorter timeout than i2c_wait4bus()?
I'd follow Peter's suggestion here. It sounds sane to me.
Best regards,
Wolfgang Denk
Dear Peter Tyser,
In message 1252078092.6005.63.camel@localhost.localdomain you wrote:
Wrong Question. I don't know enough about the I2C protocol. Why is i2c_wait4bus necessary?
Ok, why is it necessary?
Freescale's I2C core supports multiple masters. I'd guess that i2c_wait4bus() is used to ensure the bus is not in use by a different master before initiating a read or write. Its polling the MBB status bit, which is automatically set/cleared when the controller sees a START/STOP which supports this.
If this is the case, the timeout should be the maximum (or reasonable maximum) time an I2C transaction could take.
Thanks for the explanation - are there actually any boards out there using more than a single I2C master (i. e. the CPU) on the same I2C bus?
Best regards,
Wolfgang Denk
Dear Timur Tabi,
In message 4AA12E52.2080403@freescale.com you wrote:
Wrong Question. I don't know enough about the I2C protocol. Why is i2c_wait4bus necessary?
Ok, why is it necessary?
Maybe we should remove it, when nobody knows why it's needed.
Kumar, any thoughts? Is there something sneaky going on here, or did you just misinterpret the value of I2C_TIMEOUT?
I guess I2C_TIMEOUT might always have been misinterpeted.
I think the original code was correct, because it was counting clock ticks.
It cannot have been correct. get_timer() takes an argument of milliseconds, i. e. a time. "(CONFIG_SYS_HZ / 4)" is a frequency, i. e. not a time, but the inverse of it.
It is plain wront to write "250 per second" when you mean "250 milliseconds"
Best regards,
Wolfgang Denk
On Fri, Sep 04, 2009 at 05:29:48PM +0200, Wolfgang Denk wrote:
Kumar, any thoughts? Is there something sneaky going on here, or did you just misinterpret the value of I2C_TIMEOUT?
I guess I2C_TIMEOUT might always have been misinterpeted.
I think the original code was correct, because it was counting clock ticks.
It cannot have been correct. get_timer() takes an argument of milliseconds, i. e. a time. "(CONFIG_SYS_HZ / 4)" is a frequency, i. e. not a time, but the inverse of it.
It is plain wront to write "250 per second" when you mean "250 milliseconds"
It is not a frequency, it is a number of ticks. This is a very common idiom.
The "milliseconds" interpretation of get_timer() is not documented anywhere in the code that I can see. Neither, again as far as I can see from a quick grep, is the requirement that CONFIG_SYS_HZ be 1000, other than in some board- or arch-specific files (some actually say that CONFIG_SYS_HZ must be *less* than 1000), or in mailing list archives.
-Scott
Dear Scott Wood,
In message 20090904183437.GA20066@b07421-ec1.am.freescale.net you wrote:
milliseconds, i. e. a time. "(CONFIG_SYS_HZ / 4)" is a frequency, i. e. not a time, but the inverse of it.
It is plain wront to write "250 per second" when you mean "250 milliseconds"
It is not a frequency, it is a number of ticks. This is a very common idiom.
CONFIG_SYS_HZ _is_ a frequenzy. It is the number of ticks _per_ _second_. That is the _inverse_ of a time unit, not a time unit.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Dear Scott Wood,
In message 20090904183437.GA20066@b07421-ec1.am.freescale.net you wrote:
milliseconds, i. e. a time. "(CONFIG_SYS_HZ / 4)" is a frequency, i. e. not a time, but the inverse of it.
It is plain wront to write "250 per second" when you mean "250 milliseconds"
It is not a frequency, it is a number of ticks. This is a very common idiom.
CONFIG_SYS_HZ _is_ a frequenzy. It is the number of ticks _per_ _second_. That is the _inverse_ of a time unit, not a time unit.
Yes, CONFIG_SYS_HZ is a frequency. And when you multiply a _frequency_, which is _ticks_ per _second_, by a number _seconds_ (in this case, 1/4 sec), you get a number of _ticks_.
-Scott
Dear Timur Tabi,
In message 4A9FDF1E.4090908@freescale.com you wrote:
Currently we define I2C_TIMEOUT like this:
#define I2C_TIMEOUT (CONFIG_SYS_HZ / 4)
= 250, that is.
CONFIG_HZ is 1000, so I2C_TIMEOUT is equal to 250. However, the way it's used, 250 isn't the number of ticks per second, it's used as number of microseconds. If CONFIG_HZ is changed to 100, does that mean that we want to call usec2ticks(25)?
CONFIG_SYS_HZ is a constant of 1000. We do not change constants.
Surely, one millisecond is not too long of a timeout?
It definitely depends which sort of timeout this is.
I guess there is some specification?
Best regards,
Wolfgang Denk
On Fri, Sep 04, 2009 at 10:31:00AM +0200, Wolfgang Denk wrote:
CONFIG_HZ is 1000, so I2C_TIMEOUT is equal to 250. However, the way it's used, 250 isn't the number of ticks per second, it's used as number of microseconds. If CONFIG_HZ is changed to 100, does that mean that we want to call usec2ticks(25)?
CONFIG_SYS_HZ is a constant of 1000. We do not change constants.
We shouldn't call them CONFIGurable, then. :-)
Out of curiosity, what is the reason why CONFIG_SYS_HZ must be 1000?
-Scott
Dear Scott Wood,
In message 20090904183645.GB20066@b07421-ec1.am.freescale.net you wrote:
CONFIG_SYS_HZ is a constant of 1000. We do not change constants.
We shouldn't call them CONFIGurable, then. :-)
Agreed. This should never have made it into public code. But it slipped through some 9 years ago, and I cannot turn back the clocks (at least not that far).
Out of curiosity, what is the reason why CONFIG_SYS_HZ must be 1000?
IIRC there are a number of places which make such an assumption, incorrectly.
Best regards,
Wolfgang Denk
participants (5)
-
Heiko Schocher -
Peter Tyser -
Scott Wood -
Timur Tabi -
Wolfgang Denk