[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Timur Tabi]

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?

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Heiko Schocher]

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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Don't put off for tomorrow what you can  do  today,  because  if  you
enjoy it today you can do it again tomorrow.

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"If you are afraid of loneliness, don't marry."             - Chekhov

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Timur Tabi]

On Fri, Sep 4, 2009 at 4:25 AM, Wolfgang Denk<wd@denx.de> wrote:

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

Can you explain?  I don't know enough about the I2C protocol.  Why is
i2c_wait4bus unnecessary?

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

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?

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Virtual" means never knowing where your next byte is coming from.

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Peter Tyser]

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

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I've been programming for (35-9=) 24 years myself,  and  I  find  the
best way to learn a new language is to *read* every example snippet I
can find, with a reference book close by. Eventually, the *rhythm* of
the language starts pounding in my head... and my fluency is close at
hand. That's how I learned Perl, and now I'm one of the drummers. :-)
         -- Randal L. Schwartz in <8cvi6p2tir.fsf@gadget.cscaper.com>

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Timur Tabi]

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?

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Peter Tyser]

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

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A consultant is a person who borrows your watch, tells you what  time
it is, pockets the watch, and sends you a bill for it.

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Very ugly or very beautiful women should be flattered on their
understanding, and mediocre ones on their beauty.
                                       -- Philip Earl of Chesterfield

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Scott Wood]

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

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Scott Wood]

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

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Timur Tabi]

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()?

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Misquotation is, in fact, the pride and privilege of the  learned.  A
widely-read  man  never  quotes  accurately,  for  the rather obvious
reason that he has read too widely.
                - Hesketh Pearson _Common Misquotations_ introduction

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
To get something done, a committee should consist  of  no  more  than
three men, two of them absent.

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Scott Wood]

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

[U-Boot] Odd value for I2C_TIMEOUT in fsl_i2c.c

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The biggest difference between time and space is that you can't reuse
time.                                                 - Merrick Furst