RE: [pacth] I2C bug fixes for L-O and L-Z

RE: [pacth] I2C bug fixes for L-O and L-Z

[Eero Nurkkala]

Could you please also address the question of the load on the SCL line?

In my understanding, many boards are using the experimental values
from TRM for SCLL, SCLH, HSSCLL and HSSCLH. I have notices that those
"default" values fail to provide a standard I2C clock for, at least,
boards that I have tested. Should those values be board specific and
taken off from the I2C driver? Or should a note be added there saying
"These values are board specific and they depend on the load on the SCL
line. Please measure and validate correct values for your board".

RE: [pacth] I2C bug fixes for L-O and L-Z

[Woodruff, Richard]

Hi,

> Could you please also address the question of the load on the SCL line?

Are you talking about rise/fall time?

I can comment on bits I picked up reading and talking to others.

The current data sheets I looked at give a couple kinds of guidance for resistor usage. One type has you measure bus capacitance then gives optimized resistor values per speed range in a capacitance range. The other type gives rise/fall which you should measure on board then choose the correct value for your board.

You need to have a low enough value resistor such that it pulls voltage quickly enough. One tradeoff is the lower the value the more current flows per bit.

> In my understanding, many boards are using the experimental values
> from TRM for SCLL, SCLH, HSSCLL and HSSCLH. I have notices that those
> "default" values fail to provide a standard I2C clock for, at least,
> boards that I have tested. Should those values be board specific and
> taken off from the I2C driver? Or should a note be added there saying
> "These values are board specific and they depend on the load on the SCL
> line. Please measure and validate correct values for your board".

So yes, how fast your I2C bus can safely run is board dependent.  However, depending on resistor choice you might have full range.

The way the resistor/capacitive ranges are defined if you're good for HS I guess you should be good for fast and standard speed.

You should be engaging your board people if what you are measuring is not to your needs. If the board has only put in a FS value then HS operation might be limited.  In that case you might need tables as you say in question.

* The idea I had when looking at this is you might be able to make use of the 3 internal pull-ups which exist on the path (and possible external pullup) to make sort of a resistive network.  This might allow you to lower resistance when running at high speeds and make it higher at slower ones (to save power).  This should expand speed range even if your board designer only put a FS resistor value in.

Does that help?

Regards,
Richard W.

RE: [pacth] I2C bug fixes for L-O and L-Z

[Eero Nurkkala]

On Mon, 2009-02-16 at 15:19 +0100, ext Woodruff, Richard wrote:
> Hi,
> 
> > Could you please also address the question of the load on the SCL line?
> 
> Are you talking about rise/fall time?
> 

Sorry for being unclear;

The I2C standard addresses also rise/fall times, but more interesting,
the tLow and tHigh (and a number of other parameters).

It seems with the open source drivers, that somehow they're after a
"balanced I2C clock" meaning tLow == tHigh, which is very, very
dangerous.

If you look at the I2C standard, you see that actually tLow is
about twice as large as tHigh! (that is closer to truth than
the balanced clock)

So I'm talking about the registers I2C_SCLL and I2C_SCLH. If they
have the TRM or "open source" values, then it's very likely 
the I2C clock is out of standard. The SCLL (tLow) is in danger
for being far too short.

The I2C chip manufacturers consider the I2C standard as basis for
any proper operation. They're not after "balanced" I2C clock.

So I wish I could get some comments on the SCLL and SCLH, TRM,
open source and obeying I2C standard.

RE: [pacth] I2C bug fixes for L-O and L-Z

[Woodruff, Richard]

> From: Eero Nurkkala [mailto:ext-eero.nurkkala@nokia.com]

> On Mon, 2009-02-16 at 15:19 +0100, ext Woodruff, Richard wrote:
> > Hi,
> >
> > > Could you please also address the question of the load on the SCL line?
> >
> > Are you talking about rise/fall time?
> >
> Sorry for being unclear;
>
> The I2C standard addresses also rise/fall times, but more interesting,
> the tLow and tHigh (and a number of other parameters).
>
> It seems with the open source drivers, that somehow they're after a
> "balanced I2C clock" meaning tLow == tHigh, which is very, very
> dangerous.

Oh, I see. I didn't look at that angle. I can inquire/look.

For sure wave forms on O-Scope and LA showed variable data line times, sometimes longer then one would expect (for stop/start/ack).  But yes, clocks were pretty even iirc.

Regards,
Richard W.

RE: [pacth] I2C bug fixes for L-O and L-Z

[Woodruff, Richard]

Eero,

> > From: Eero Nurkkala [mailto:ext-eero.nurkkala@nokia.com]
>
> > On Mon, 2009-02-16 at 15:19 +0100, ext Woodruff, Richard wrote:
> > > Hi,
> > >
> > > > Could you please also address the question of the load on the SCL line?
> > >
> > > Are you talking about rise/fall time?
> > >
> > Sorry for being unclear;
> >
> > The I2C standard addresses also rise/fall times, but more interesting,
> > the tLow and tHigh (and a number of other parameters).
> >
> > It seems with the open source drivers, that somehow they're after a
> > "balanced I2C clock" meaning tLow == tHigh, which is very, very
> > dangerous.
>
> Oh, I see. I didn't look at that angle. I can inquire/look.
>
> For sure wave forms on O-Scope and LA showed variable data line times,
> sometimes longer then one would expect (for stop/start/ack).  But yes, clocks
> were pretty even iirc.

I received the following comment from a HW Apps person whom has dealt with this at the board level.

<comment start>
Attached is the I2C spec that I have.  As I understand it, the I2C only specify the minimum tLow and tHigh (which is not "balanced").  However what is more important is that the appropriate setup and hold time are followed.

If the equal time still meets the setup/hold time then there should not be any issue from a compliance standpoint.
<comment end>

Regards,
Richard W.

RE: [pacth] I2C bug fixes for L-O and L-Z

[Eero Nurkkala]

On Fri, 2009-02-20 at 21:59 +0100, ext Woodruff, Richard wrote:
> 
> I received the following comment from a HW Apps person whom has dealt with this at the board level.
> 
> <comment start>
> Attached is the I2C spec that I have.  As I understand it, the I2C only specify the minimum tLow and tHigh (which is not "balanced").  However what is more important is that the appropriate setup and hold time are followed.
> 
> If the equal time still meets the setup/hold time then there should not be any issue from a compliance standpoint.
> <comment end>
> 
> Regards,
> Richard W.

Hi,

With out board, the tLow and tHigh values are in line with I2C standard
with the patch:
http://marc.info/?l=linux-omap&m=122770723311340&w=2

Would that risk the setup and hold times? (If I remember correctly, the
values (setup, hold) were within the I2C standard even with the patch.)

I think it's a risk not to meet tLow and tHigh. I'm saying, with open
source values with our board, the tLow was not in standard. If using
TRM minimum values, things get even worse. Why? because it states, for
example, Fast Mode SCLL = 5 and SCLH = 7. This means that the low
period is smaller than the high! Shouldn't it be vice versa? (scope
verified).

- Eero

Re: tHigh tLow discussion (was [pacth] I2C bug fixes for L-O and L-Z)

[Nishanth Menon]

Eero Nurkkala said the following on 02/23/2009 03:37 PM:
> On Fri, 2009-02-20 at 21:59 +0100, ext Woodruff, Richard wrote:
>   
>> I received the following comment from a HW Apps person whom has dealt with this at the board level.
>>
>> <comment start>
>> Attached is the I2C spec that I have.  As I understand it, the I2C only specify the minimum tLow and tHigh (which is not "balanced").  However what is more important is that the appropriate setup and hold time are followed.
>>
>> If the equal time still meets the setup/hold time then there should not be any issue from a compliance standpoint.
>> <comment end>
>>
>> Regards,
>> Richard W.
>>     
> With out board, the tLow and tHigh values are in line with I2C standard
> with the patch:
> http://marc.info/?l=linux-omap&m=122770723311340&w=2
>
> Would that risk the setup and hold times? (If I remember correctly, the
> values (setup, hold) were within the I2C standard even with the patch.)
>
> I think it's a risk not to meet tLow and tHigh. I'm saying, with open
> source values with our board, the tLow was not in standard. If using
> TRM minimum values, things get even worse. Why? because it states, for
> example, Fast Mode SCLL = 5 and SCLH = 7. This means that the low
> period is smaller than the high! Shouldn't it be vice versa? (scope
> verified).
>   
just couple of views: (Disclaimer - it might be good if someone could re
validate my math :( )..
a) tHigh and tLow could be device dependent. for an I2C bus with
multiple devices, we probably need a common denominator. the question is
- is there a min and max for the tHigh and tLow? some of the datasheets
I have seen seem to have this.
b) the drivers/i2c/busses/i2c-omap.c does computation based on
dev->speed. the equation is based on:
tHigh  = ( sclh +6 )*iclk period
tLow  = ( scll +6 )*iclk period
the TRM ([2] table18-12(page 2942)) instead speaks of:
tHigh  = ( sclh +5 )*iclk period
tLow  = ( scll +7 )*iclk period
current i2c driver attempts to put the same value to scll and sclh reg.
The result would be that tHigh != tLow, unless I am missing something.

I am not entirely sure about the basis of Eero's equation:

scll = (scl+3) * iclk
sclh = (scl+9) * iclk


Now computing a bit, i2c spec [1] says in table 5 for minimum for FS as
tLow as 1.3uS and tHigh as 0.6uS. max is not mentioned :(.. OMAP3430 TRM
([2] table 18-13 (page 2943)) says:
psc=9, scl = 5, sch =7 - for fclk = 96Mhz.
translating this:
iclk =96M/(9+1) = 9.6Mhz = 0.104167uSec.
tHigh  = ( sclh +5 )*iclk = (7+5)*0.104167 = 1.250004 uSec (spec min is
0.6uSec)
tLow  = ( scll +7 )*iclk =(5+7)*0.104167 = 1.250004 uSec (spec min is
1.3uSec <== I might be missing something here - but does that look like
some sort of violation?

Now, from a s/w perspective, If we would like to have it so that we can
configure tHigh and tLow based on devices, electrical characteristics on
a bus, not just speed of the bus, the current implementation would
probably need to change(I think).

Regards,
Nishanth Menon

Ref:
[1] http://www.nxp.com/acrobat_download/literature/9398/39340011.pdf
[2] http://focus.ti.com/pdfs/wtbu/SWPU114I_PrelimFinalEPDF_06_10_2008.pdf

RE: tHigh tLow discussion (was [pacth] I2C bug fixes for L-O and L-Z)

[ext-Eero.Nurkkala]

> I am not entirely sure about the basis of Eero's equation:
>
> scll = (scl+3) * iclk
> sclh = (scl+9) * iclk

No, no:

Exactly the opposite,
+                       fsscll = internal_clk / (dev->speed * 2) - 3;
+                       fssclh = internal_clk / (dev->speed * 2) - 9;

means a longer period for scll. (and shorter for sclh)

- Eero

Re: tHigh tLow discussion (was [pacth] I2C bug fixes for L-O and L-Z)

[Nishanth Menon]

On Mon, Feb 23, 2009 at 8:10 PM,  <ext-Eero.Nurkkala@nokia.com> wrote:
>> I am not entirely sure about the basis of Eero's equation:
>>
>> scll = (scl+3) * iclk
>> sclh = (scl+9) * iclk
>
> No, no:
>
> Exactly the opposite,
> +                       fsscll = internal_clk / (dev->speed * 2) - 3;
> +                       fssclh = internal_clk / (dev->speed * 2) - 9;
>
> means a longer period for scll. (and shorter for sclh)
Oops.. copy-paste typo.. :(
tLow = (scll+3) * iclk
tHigh = (sclh+9) * iclk
Vs:
TRM:
tHigh  = ( sclh +5 )*iclk period
tLow  = ( scll +7 )*iclk period

But my question is this: why are we trying to a different equation
here compared to the equation in the TRM? if the reason is that the
tLow and tHigh should be a variant based on board, probably
precomputed values or run time computed values wont work for all
platforms. the board file probably should give it as a platform
specific data. if this is not provided, use the run time computation..

just a thought..

Regards,
Nishanth Menon

RE: tHigh tLow discussion (was [pacth] I2C bug fixes for L-O and L-Z)

[ext-Eero.Nurkkala]

> But my question is this: why are we trying to a different equation
> here compared to the equation in the TRM?

Don't just blindly trust the TRM, all that matters is the real signal
caught in the scope. That is what you want to verify.

- Eero

Re: tHigh tLow discussion (was [pacth] I2C bug fixes for L-O and L-Z)

[Nishanth Menon]

On Mon, Feb 23, 2009 at 8:20 PM,  <ext-Eero.Nurkkala@nokia.com> wrote:
>
> Don't just blindly trust the TRM, all that matters is the real signal
> caught in the scope. That is what you want to verify.

;) Aye Aye.... I wonder if the equation you provided is based on
emperical measurement? if so would it vary based on electrical
characteristics of a platform? I mean beagleboard Vs Zoom Vs SDP
atleast have variances of i2c trace lengths and number of devices per
i2c bus.. wont the equation change based on board configuration?

Regards,
Nishanth Menon

RE: tHigh tLow discussion (was [pacth] I2C bug fixes for L-O and L-Z)

[ext-Eero.Nurkkala]

> ;) Aye Aye.... I wonder if the equation you provided is based on
> emperical measurement? if so would it vary based on electrical
> characteristics of a platform? I mean beagleboard Vs Zoom Vs SDP
> atleast have variances of i2c trace lengths and number of devices per
> i2c bus.. wont the equation change based on board configuration?

Nothing empirical. It's just for one board.

They say it differs from board to board. I haven't had time to spend with different boards.
They call it the "load on scl line", that makes the numbers different on other
boards. (I'm not so sure about that, but agreed it may vary little)

That patch is reverted these days, as it is not really empirical, and doesn't work on omap2430.

- Eero

Re: tHigh tLow discussion (was [pacth] I2C bug fixes for L-O and L-Z)

[Nishanth Menon]

On Mon, Feb 23, 2009 at 8:27 PM,  <ext-Eero.Nurkkala@nokia.com> wrote:
>
>
>> ;) Aye Aye.... I wonder if the equation you provided is based on
>> emperical measurement? if so would it vary based on electrical
>> characteristics of a platform? I mean beagleboard Vs Zoom Vs SDP
>> atleast have variances of i2c trace lengths and number of devices per
>> i2c bus.. wont the equation change based on board configuration?
>
> Nothing empirical. It's just for one board.
>
> They say it differs from board to board. I haven't had time to spend with different boards.
> They call it the "load on scl line", that makes the numbers different on other
> boards. (I'm not so sure about that, but agreed it may vary little)
>
So, it might be worth considering my following proposition?"
Now, from a s/w perspective, If we would like to have it so that we can
configure tHigh and tLow based on devices, electrical characteristics on
a bus, not just speed of the bus, the current implementation would
probably need to change(I think).
"
maybe take tHigh, tLow as a platform data input to the driver? that
could allow scaling for all boards I wonder?

Regards,
Nishanth Menon

RE: tHigh tLow discussion (was [pacth] I2C bug fixes for L-O and L-Z)

[ext-Eero.Nurkkala]

> So, it might be worth considering my following proposition?"
> Now, from a s/w perspective, If we would like to have it so that we can
> configure tHigh and tLow based on devices, electrical characteristics on
> a bus, not just speed of the bus, the current implementation would
> probably need to change(I think).
> "
> maybe take tHigh, tLow as a platform data input to the driver? that
> could allow scaling for all boards I wonder?

Something like that I'd say. Something not happening soon, because
not everybody has a scope and the time =)

- Eero

Re: tHigh tLow discussion (was [pacth] I2C bug fixes for L-O and L-Z)

[Aaro Koskinen]

Hello,

ext Nishanth Menon wrote:
> Oops.. copy-paste typo.. :(
> tLow = (scll+3) * iclk
> tHigh = (sclh+9) * iclk
> Vs:
> TRM:
> tHigh  = ( sclh +5 )*iclk period
> tLow  = ( scll +7 )*iclk period
> 
> But my question is this: why are we trying to a different equation
> here compared to the equation in the TRM?

The problem with TRM (the table 18-13 you referred earlier) is that it 
assumes 50% duty cycle while the correct one is more like 33%. This is 
corrected by Eero's patch:

+                       fsscll = internal_clk / (dev->speed * 2) - 3;
+                       fssclh = internal_clk / (dev->speed * 2) - 9;

this is same as (with internal_clk == 9600 and dev->speed == 400):

scl = internal_clk / dev->speed;
fsscll = scl - scl/3 - 7;
fssclh = scl/3 - 5;

If the code would be like this, then I guess the readers of both TRM 
_and_ the I2C spec would be happy?

The problem with Eero's patch is that it changes the internal clock 
(again thanks to that confusing table). You should be able to use same 
for all modes. Also note that the noise filter is one internel clock 
period. Currently the driver uses 19.2 MHz which exceeds the I2C spec 
max. So 24 MHz would be a safer choice.

A.

RE: tHigh tLow discussion (was [pacth] I2C bug fixes for L-O and L-Z)

[ext-Eero.Nurkkala]

> The problem with Eero's patch is that it changes the internal clock
> (again thanks to that confusing table). You should be able to use same
> for all modes. Also note that the noise filter is one internel clock
> period. Currently the driver uses 19.2 MHz which exceeds the I2C spec
> max. So 24 MHz would be a safer choice.

Noise filter exceeding the I2C is "a bonus provided by the device".
Should not harm any. (what they say =)

- Eero

Re: tHigh tLow discussion (was [pacth] I2C bug fixes for L-O and L-Z)

[Nishanth Menon]

Aaro Koskinen said the following on 02/24/2009 11:09 AM:
> ext Nishanth Menon wrote:
>> Oops.. copy-paste typo.. :(
>> tLow = (scll+3) * iclk
>> tHigh = (sclh+9) * iclk
>> Vs:
>> TRM:
>> tHigh  = ( sclh +5 )*iclk period
>> tLow  = ( scll +7 )*iclk period
>>
>> But my question is this: why are we trying to a different equation
>> here compared to the equation in the TRM?
>
> The problem with TRM (the table 18-13 you referred earlier) is that it
> assumes 50% duty cycle while the correct one is more like 33%. This is
> corrected by Eero's patch:
Gentle query - could you point me to the place where the 33% duty cycle
is mentioned in i2c spec? spec mentions minimum timing, but I don't seem
to find a constraint on duty cycle requirement.. :(

Regards,
Nishanth Menon

Re: tHigh tLow discussion (was [pacth] I2C bug fixes for L-O and L-Z)

[Aaro Koskinen]

Hello,

ext Nishanth Menon wrote:
> Aaro Koskinen said the following on 02/24/2009 11:09 AM:
>>> TRM:
>>> tHigh  = ( sclh +5 )*iclk period
>>> tLow  = ( scll +7 )*iclk period
>>>
>>> But my question is this: why are we trying to a different equation
>>> here compared to the equation in the TRM?
>> The problem with TRM (the table 18-13 you referred earlier) is that it
>> assumes 50% duty cycle while the correct one is more like 33%. This is
>> corrected by Eero's patch:
> Gentle query - could you point me to the place where the 33% duty cycle
> is mentioned in i2c spec? spec mentions minimum timing, but I don't seem
> to find a constraint on duty cycle requirement.. :(

Well, it's implicite... Check the i2c spec table 7 for speeds 3400 and 
1700. If you use TRM calculcations so that tLow == tHigh you get too 
tLow that's below the minimum in both cases. So you must increase tLow, 
and when you do that you must make tHigh shorter, otherwise you don't 
match the rate.

A.

RE: tHigh tLow discussion (was [pacth] I2C bug fixes for L-O and L-Z)

[Woodruff, Richard]

> From: Aaro Koskinen [mailto:aaro.koskinen@nokia.com]
> Sent: Tuesday, February 24, 2009 6:47 AM

> ext Nishanth Menon wrote:
> > Aaro Koskinen said the following on 02/24/2009 11:09 AM:
> >>> TRM:
> >>> tHigh  = ( sclh +5 )*iclk period
> >>> tLow  = ( scll +7 )*iclk period
> >>>
> >>> But my question is this: why are we trying to a different equation
> >>> here compared to the equation in the TRM?
> >> The problem with TRM (the table 18-13 you referred earlier) is that it
> >> assumes 50% duty cycle while the correct one is more like 33%. This is
> >> corrected by Eero's patch:
> > Gentle query - could you point me to the place where the 33% duty cycle
> > is mentioned in i2c spec? spec mentions minimum timing, but I don't seem
> > to find a constraint on duty cycle requirement.. :(
>
> Well, it's implicite... Check the i2c spec table 7 for speeds 3400 and
> 1700. If you use TRM calculcations so that tLow == tHigh you get too
> tLow that's below the minimum in both cases. So you must increase tLow,
> and when you do that you must make tHigh shorter, otherwise you don't
> match the rate.

... I didn't take a deep study but I did just skim a couple sections.

And it looks like it I2C spec does NOT require the clock high/low to be unbalanced (or balanced).  It would seem to recommend it being unbalanced because doing so should allow timing requirements to be more easily met for a range of frequencies. This would allow a more general equation.

In the end it is a question if rise/fall times and relative clock-line/data-line requirements are met.

It does seem to say in the spec that after 100pf of bus capacitance all bets are off and you need to measure and degrade speed unit conditions are met.

Regards,
Richard W.

RE: tHigh tLow discussion (was [pacth] I2C bug fixes for L-O and L-Z)

[Woodruff, Richard]

> > Well, it's implicite... Check the i2c spec table 7 for speeds 3400 and
> > 1700. If you use TRM calculcations so that tLow == tHigh you get too
> > tLow that's below the minimum in both cases. So you must increase tLow,
> > and when you do that you must make tHigh shorter, otherwise you don't
> > match the rate.
>
> ... I didn't take a deep study but I did just skim a couple sections.
>
> And it looks like it I2C spec does NOT require the clock high/low to be
> unbalanced (or balanced).  It would seem to recommend it being unbalanced
> because doing so should allow timing requirements to be more easily met for a
> range of frequencies. This would allow a more general equation.
>
> In the end it is a question if rise/fall times and relative clock-line/data-
> line requirements are met.
>
> It does seem to say in the spec that after 100pf of bus capacitance all bets
> are off and you need to measure and degrade speed unit conditions are met.

BTW, I'll reemphasize the controller bugs I pointed out last week are real and did impact a couple different boards.  Adding those fixes made what once would trip up after a few million near-back-to-back read/writes work for with out error so far.

I've not spent enough time on low/high to know anything absolutely.  It does seem like a more board friendly way to go but may not be required.

Regards,
Richard W.

RE: tHigh tLow discussion (was [pacth] I2C bug fixes for L-O and L-Z)

[Pakaravoor, Jagadeesh]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of
> Woodruff, Richard
> Sent: Tuesday, February 24, 2009 6:47 PM
> To: Aaro Koskinen; ext Nishanth Menon
> Cc: Nurkkala Eero.An (EXT-Offcode/Oulu); linux-omap@vger.kernel.org
> Subject: RE: tHigh tLow discussion (was [pacth] I2C bug fixes for L-O and L-Z)
> 
> 
> 
> BTW, I'll reemphasize the controller bugs I pointed out last week are real and did impact a couple
> different boards.  Adding those fixes made what once would trip up after a few million near-back-to-
> back read/writes work for with out error so far.
> 
> I've not spent enough time on low/high to know anything absolutely.  It does seem like a more board
> friendly way to go but may not be required.
> 

Unlike SDP, LDP was giving errors when i2c-1 bus was set to 2600 (like spurious and some times missing keypad interrupts and such). I tested the patch and with this patch all those issues does go away.

This patch is essential to make i2c-1 bus work at error-free at 2600.

ACKing the patch.

> Regards,
> Richard W.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html