On Tue, Jul 09, 2013 at 11:44:02AM +0300, Mika Westerberg wrote: > On Mon, Jul 08, 2013 at 03:42:17PM +0200, Christian Ruppert wrote: > > On Mon, Jul 08, 2013 at 02:45:26PM +0300, Mika Westerberg wrote: > > > The DesignWare I2C controller has high count (HCNT) and low count (LCNT) > > > registers for each of the I2C speed modes (standard and fast). These > > > registers are programmed based on the input clock speed in the driver. > > > > > > However, that is not always the most accurate way. For example on Intel > > > BayTrail we have 100MHz input clock and calculated *CNT values result as > > > measured by one of our team: > > > > > > Standard mode: 100.25kHz > > > Fast mode : 315.41kHZ > > > > > > The fast mode speed is over 20% lower than what is expected. > > > > We have observed similar issues and I am wondering if this effect is due > > to the overly-pessimistic formulas in i2c_dw_scl_lcnt and > > i2c_dw_scl_hcnt. The comments in these functions are a bit confusing and > > I don't pretend having fully understood what the intention is. I'm > > having the impression that defining the arguments tf in function of the > > hardware would be the "correct" way to achieve better clock accuracy. > > Well, that's not the only timing parameter specified in the I2C spec. We > also have tr among other things (even though the dw driver doesn't use it). Exactly. Ant T_HD;STA which is mentioned somewhere in the comment and one of the reasons why I never dared touching these functions. The fact that the designware IP doesn't seem to manage all timings makes this function quite difficult to understand. > > > It might be > > > that there are things like strenght of the pull-up resistors, bus > > > capacitance etc. that are very platform specific and have an effect on the > > > clock signal. > > > > I believe tf is supposed to model these things. I just haven't clearly > > understood how. In my understanding, transition times should be > > subtracted rather than added to cycle times or am I getting something > > fundamentally wrong here? > > I'm not sure, honestly :-) I believe tf is the same tf that is in the I2C > spec, meaning fall time of both SCL and SDA signals and the spec measures > one clock cycle to be from end of the first tf to the end of the second > (fig. 27 in the I2C spec). If I'm reading it right then it means adding > instead of substracting. > Do you have any suggestions how we could use tf here? What I meant is the following: The clock cycle time Tc is composed of the four components Tc = Th + Tf + Tl + Tr where Th: Time during which the signal is high Tf: Falling edge transition time Tl: Time during which the signal is low Tr: Rising edge transition time The I2C specification specifies a minimum for Tl and Th and a range (or maximum) for Tr and Tf. A maximum frequency is specified as the frequency obtained by adding the minima for Th and Tl to the maxima of Tr ant Tf. Since as you said, transition times are very much PCB dependent, one way to guarantee the max. frequency constraint (and to achieve a constant frequency at its max) is to define the constants Th' = Th + Tf := Th_min + Tf_max Tl' = Tl + Tr := Tl_min + Tr_max and to calculate the variables Th = Th' - Tf Tl = Tl' - Tr in function of Tf and Tr of the given PCB. > At least on Intel > hardware we have an ACPI method that returns directly the optimum *CNT > values. I don't know ACPI very well and if it deals with register values directly your patch is probably the right thing. The timing calculation in the driver seems a bit strange to me, however (see above), but I never dared touching it because I never had time to dive into the code deep enough and I was just wondering if it was possible to fix it at the same time. If ACPI bypasses the function alltogether this is probably not applicable. -- Christian Ruppert , <christian.ruppert@xxxxxxxxxx> /| Tel: +41/(0)22 816 19-42 //| 3, Chemin du Pré-Fleuri _// | bilis Systems CH-1228 Plan-les-Ouates -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html