| Message ID | 1373283927-21677-1-git-send-email-mika.westerberg@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
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. > 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? > With this patch we let the platform code to specify more accurate, optimal > *CNT values if they are known beforehand. > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/i2c/busses/i2c-designware-core.c | 46 +++++++++++++++++++------------- > drivers/i2c/busses/i2c-designware-core.h | 12 +++++++++ > 2 files changed, 40 insertions(+), 18 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c > index ad46616..f31469e 100644 > --- a/drivers/i2c/busses/i2c-designware-core.c > +++ b/drivers/i2c/busses/i2c-designware-core.c > @@ -308,29 +308,39 @@ int i2c_dw_init(struct dw_i2c_dev *dev) > /* set standard and fast speed deviders for high/low periods */ > > /* Standard-mode */ > - hcnt = i2c_dw_scl_hcnt(input_clock_khz, > - 40, /* tHD;STA = tHIGH = 4.0 us */ > - 3, /* tf = 0.3 us */ > - 0, /* 0: DW default, 1: Ideal */ > - 0); /* No offset */ > - lcnt = i2c_dw_scl_lcnt(input_clock_khz, > - 47, /* tLOW = 4.7 us */ > - 3, /* tf = 0.3 us */ > - 0); /* No offset */ > + if (dev->ss_hcnt && dev->ss_lcnt) { > + hcnt = dev->ss_hcnt; > + lcnt = dev->ss_lcnt; > + } else { > + hcnt = i2c_dw_scl_hcnt(input_clock_khz, > + 40, /* tHD;STA = tHIGH = 4.0 us */ > + 3, /* tf = 0.3 us */ > + 0, /* 0: DW default, 1: Ideal */ > + 0); /* No offset */ > + lcnt = i2c_dw_scl_lcnt(input_clock_khz, > + 47, /* tLOW = 4.7 us */ > + 3, /* tf = 0.3 us */ > + 0); /* No offset */ > + } > dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT); > dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT); > dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt); > > /* Fast-mode */ > - hcnt = i2c_dw_scl_hcnt(input_clock_khz, > - 6, /* tHD;STA = tHIGH = 0.6 us */ > - 3, /* tf = 0.3 us */ > - 0, /* 0: DW default, 1: Ideal */ > - 0); /* No offset */ > - lcnt = i2c_dw_scl_lcnt(input_clock_khz, > - 13, /* tLOW = 1.3 us */ > - 3, /* tf = 0.3 us */ > - 0); /* No offset */ > + if (dev->fs_hcnt && dev->fs_lcnt) { > + hcnt = dev->fs_hcnt; > + lcnt = dev->fs_lcnt; > + } else { > + hcnt = i2c_dw_scl_hcnt(input_clock_khz, > + 6, /* tHD;STA = tHIGH = 0.6 us */ > + 3, /* tf = 0.3 us */ > + 0, /* 0: DW default, 1: Ideal */ > + 0); /* No offset */ > + lcnt = i2c_dw_scl_lcnt(input_clock_khz, > + 13, /* tLOW = 1.3 us */ > + 3, /* tf = 0.3 us */ > + 0); /* No offset */ > + } > dw_writel(dev, hcnt, DW_IC_FS_SCL_HCNT); > dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT); > dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt); > diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h > index 912aa22..e8a7565 100644 > --- a/drivers/i2c/busses/i2c-designware-core.h > +++ b/drivers/i2c/busses/i2c-designware-core.h > @@ -61,6 +61,14 @@ > * @tx_fifo_depth: depth of the hardware tx fifo > * @rx_fifo_depth: depth of the hardware rx fifo > * @rx_outstanding: current master-rx elements in tx fifo > + * @ss_hcnt: standard speed HCNT value > + * @ss_lcnt: standard speed LCNT value > + * @fs_hcnt: fast speed HCNT value > + * @fs_lcnt: fast speed LCNT value > + * > + * HCNT and LCNT parameters can be used if the platform knows more accurate > + * values than the one computed based only on the input clock frequency. > + * Leave them to be %0 if not used. > */ > struct dw_i2c_dev { > struct device *dev; > @@ -91,6 +99,10 @@ struct dw_i2c_dev { > unsigned int rx_fifo_depth; > int rx_outstanding; > u32 sda_hold_time; > + u16 ss_hcnt; > + u16 ss_lcnt; > + u16 fs_hcnt; > + u16 fs_lcnt; > }; > > #define ACCESS_SWAP 0x00000001 > -- > 1.8.3.2 >
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). > > 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? At least on Intel hardware we have an ACPI method that returns directly the optimum *CNT values. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote: > 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. If I understand the above, it leaves Tf and Tr to be PCB specific and then these values are passed to the core driver from platform data, right? I'm thinking that passing directly the "measured" *CNT values from the platform data makes this even more accurate (given that information is available). And if you only have the Tf and Tr for your board, you can have custom calculation done in the platform part of the driver that takes them into account, and then passes these custom *CNT values to the core. > > 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. Our FW people decided to have a custom ACPI method that returns the correct values to be used in the Windows I2C driver. It returns direct *CNT register values that have been measured to be good for a given PCB. > 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. It would be good if we can fix this for non-ACPI devices as well. Is it hard to add similar properties to the driver specific Device Tree bindings? Wolfram, do you have any input on this? -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 10, 2013 at 01:52:15PM +0300, Mika Westerberg wrote: > On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote: > > 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. > > If I understand the above, it leaves Tf and Tr to be PCB specific and then > these values are passed to the core driver from platform data, right? That would be the idea: Calculate Th' and Tl' in function of the desired clock frequency and duty cycle and then adapt these values using measured transition times. What prevented me from implementing this rather academic approach are the following comments in i2c-designware-core.c: /* * DesignWare I2C core doesn't seem to have solid strategy to meet * the tHD;STA timing spec. Configuring _HCNT based on tHIGH spec * will result in violation of the tHD;STA spec. */ /* ... * This is just experimental rule; the tHD;STA period * turned out to be proportinal to (_HCNT + 3). With this setting, * we could meet both tHIGH and tHD;STA timing specs. * ... */ If I interpret this right, the slow down of the clock is intentional to meet tHD;STA timing constraints. > I'm thinking that passing directly the "measured" *CNT values from the > platform data makes this even more accurate (given that information is > available). And if you only have the Tf and Tr for your board, you can have > custom calculation done in the platform part of the driver that takes them > into account, and then passes these custom *CNT values to the core. Well, as in the previous discussion on SDA hold time, Tf and Tr are physical values whereas the register values are clock dependent and probably not appropriate at least for device tree usage (or cases where the clock speed might change). If I understand you correctly, ACPI is more register oriented and able to cope with this outside of the OS? > > > 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. > > Our FW people decided to have a custom ACPI method that returns the correct > values to be used in the Windows I2C driver. It returns direct *CNT > register values that have been measured to be good for a given PCB. > > > 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. > > It would be good if we can fix this for non-ACPI devices as well. Is it > hard to add similar properties to the driver specific Device Tree bindings? I think it would be quite trivial to add properties for either the register values or the transition time values. For SDA hold time we concluded that time values would be more adequate which brings us to the question of better understanding the hack for tHD;SDA. Otherwise we might break something. Do you think your team which determines the tHD;SDA values could give us some guidance on that? > Wolfram, do you have any input on this?
On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote: > On Wed, Jul 10, 2013 at 01:52:15PM +0300, Mika Westerberg wrote: > > On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote: > > > 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. > > > > If I understand the above, it leaves Tf and Tr to be PCB specific and then > > these values are passed to the core driver from platform data, right? > > That would be the idea: Calculate Th' and Tl' in function of the desired > clock frequency and duty cycle and then adapt these values using > measured transition times. What prevented me from implementing this > rather academic approach are the following comments in > i2c-designware-core.c: > > /* > * DesignWare I2C core doesn't seem to have solid strategy to meet > * the tHD;STA timing spec. Configuring _HCNT based on tHIGH spec > * will result in violation of the tHD;STA spec. > */ > > /* ... > * This is just experimental rule; the tHD;STA period > * turned out to be proportinal to (_HCNT + 3). With this setting, > * we could meet both tHIGH and tHD;STA timing specs. > * ... > */ > > If I interpret this right, the slow down of the clock is intentional to > meet tHD;STA timing constraints. Yeah, looks like so. tHD;STA is the SDA hold time. I wonder if the above comments apply to some earlier version of the IP that didn't have the SDA hold register? > > I'm thinking that passing directly the "measured" *CNT values from the > > platform data makes this even more accurate (given that information is > > available). And if you only have the Tf and Tr for your board, you can have > > custom calculation done in the platform part of the driver that takes them > > into account, and then passes these custom *CNT values to the core. > > Well, as in the previous discussion on SDA hold time, Tf and Tr are > physical values whereas the register values are clock dependent and > probably not appropriate at least for device tree usage (or cases where > the clock speed might change). If I understand you correctly, ACPI is > more register oriented and able to cope with this outside of the OS? Well, ACPI doesn't care what its methods return (unless the methods are specfied in the ACPI spec). In this case it just returns a package of three values: HCNT, LCNT and SDA hold time. I guess it is supposed to work like this: * If there is no vendor specific SSCN/FMCN methods for the device we can just use the default and calculate *CNT from the clock speed. * If there exists a method, we use the values there instead. I think the Windows driver does something like above. IMHO we should to the same in Linux driver to be able to take advantage of the measured *CNT values. > > > > 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. > > > > Our FW people decided to have a custom ACPI method that returns the correct > > values to be used in the Windows I2C driver. It returns direct *CNT > > register values that have been measured to be good for a given PCB. > > > > > 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. > > > > It would be good if we can fix this for non-ACPI devices as well. Is it > > hard to add similar properties to the driver specific Device Tree bindings? > > I think it would be quite trivial to add properties for either the > register values or the transition time values. For SDA hold time we > concluded that time values would be more adequate which brings us to the > question of better understanding the hack for tHD;SDA. Otherwise we > might break something. Do you think your team which determines the > tHD;SDA values could give us some guidance on that? I2C spec says following about the tHD;STA: Standard mode : min 4.0 us Fast mode : min 0.6 us But I can try to ask from our HW guys for an answer. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 11, 2013 at 10:36:00AM +0300, Mika Westerberg wrote: > On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote: > > On Wed, Jul 10, 2013 at 01:52:15PM +0300, Mika Westerberg wrote: > > > On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote: > > > > 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. > > > > > > If I understand the above, it leaves Tf and Tr to be PCB specific and then > > > these values are passed to the core driver from platform data, right? > > > > That would be the idea: Calculate Th' and Tl' in function of the desired > > clock frequency and duty cycle and then adapt these values using > > measured transition times. What prevented me from implementing this > > rather academic approach are the following comments in > > i2c-designware-core.c: > > > > /* > > * DesignWare I2C core doesn't seem to have solid strategy to meet > > * the tHD;STA timing spec. Configuring _HCNT based on tHIGH spec > > * will result in violation of the tHD;STA spec. > > */ > > > > /* ... > > * This is just experimental rule; the tHD;STA period > > * turned out to be proportinal to (_HCNT + 3). With this setting, > > * we could meet both tHIGH and tHD;STA timing specs. > > * ... > > */ > > > > If I interpret this right, the slow down of the clock is intentional to > > meet tHD;STA timing constraints. > > Yeah, looks like so. tHD;STA is the SDA hold time. I wonder if the above > comments apply to some earlier version of the IP that didn't have the SDA > hold register? Scratch that. I re-read the spec and tHD;STA is hold time for (repeated) start. There is a constraint that says that the device must internally provide a hold time of at least 300ns for the SDA signal. Maybe that's the constraint the comment above is referring to? -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/11/13 7:13 PM, Mika Westerberg wrote: > On Thu, Jul 11, 2013 at 10:36:00AM +0300, Mika Westerberg wrote: >> On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote: >>> On Wed, Jul 10, 2013 at 01:52:15PM +0300, Mika Westerberg wrote: >>>> On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote: >>>>> 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. >>>> >>>> If I understand the above, it leaves Tf and Tr to be PCB specific and then >>>> these values are passed to the core driver from platform data, right? >>> >>> That would be the idea: Calculate Th' and Tl' in function of the desired >>> clock frequency and duty cycle and then adapt these values using >>> measured transition times. What prevented me from implementing this >>> rather academic approach are the following comments in >>> i2c-designware-core.c: When we talk about I2C timing specs, we should not bring up "clock speed" things. All we have to do is to strictly meet timing constraints of tHIGH, tLOW, tHD;SATA, tr, tf, etc. The resulting "clock speed" is not a goal. >>> /* >>> * DesignWare I2C core doesn't seem to have solid strategy to meet >>> * the tHD;STA timing spec. Configuring _HCNT based on tHIGH spec >>> * will result in violation of the tHD;STA spec. >>> */ >>> >>> /* ... >>> * This is just experimental rule; the tHD;STA period >>> * turned out to be proportinal to (_HCNT + 3). With this setting, >>> * we could meet both tHIGH and tHD;STA timing specs. >>> * ... >>> */ >>> >>> If I interpret this right, the slow down of the clock is intentional to >>> meet tHD;STA timing constraints. Correct. >> Yeah, looks like so. tHD;STA is the SDA hold time. I wonder if the above >> comments apply to some earlier version of the IP that didn't have the SDA >> hold register? If I remember DesignWare APB I2C spec correctly, SDA hold time register doesn't help to meet tHD;STA spec. Could someone confirm it really so with a real hardware, please? Shinya > Scratch that. > > I re-read the spec and tHD;STA is hold time for (repeated) start. There is > a constraint that says that the device must internally provide a hold time > of at least 300ns for the SDA signal. Maybe that's the constraint the > comment above is referring to? > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 12, 2013 at 04:56:49PM +0900, Shinya Kuribayashi wrote: > On 7/11/13 7:13 PM, Mika Westerberg wrote: > >On Thu, Jul 11, 2013 at 10:36:00AM +0300, Mika Westerberg wrote: > >>On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote: > >>>On Wed, Jul 10, 2013 at 01:52:15PM +0300, Mika Westerberg wrote: > >>>>On Tue, Jul 09, 2013 at 06:19:28PM +0200, Christian Ruppert wrote: > >>>>>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. > >>>> > >>>>If I understand the above, it leaves Tf and Tr to be PCB specific and then > >>>>these values are passed to the core driver from platform data, right? > >>> > >>>That would be the idea: Calculate Th' and Tl' in function of the desired > >>>clock frequency and duty cycle and then adapt these values using > >>>measured transition times. What prevented me from implementing this > >>>rather academic approach are the following comments in > >>>i2c-designware-core.c: > > When we talk about I2C timing specs, we should not bring up "clock speed" > things. All we have to do is to strictly meet timing constraints of > tHIGH, tLOW, tHD;SATA, tr, tf, etc. The resulting "clock speed" is not > a goal. OK, thanks for the explanation. I'm relatively new with I2C bus even though I've adapted the DW I2C driver to work on our HW. > >>>/* > >>> * DesignWare I2C core doesn't seem to have solid strategy to meet > >>> * the tHD;STA timing spec. Configuring _HCNT based on tHIGH spec > >>> * will result in violation of the tHD;STA spec. > >>> */ > >>> > >>>/* ... > >>> * This is just experimental rule; the tHD;STA period > >>> * turned out to be proportinal to (_HCNT + 3). With this setting, > >>> * we could meet both tHIGH and tHD;STA timing specs. > >>> * ... > >>> */ > >>> > >>>If I interpret this right, the slow down of the clock is intentional to > >>>meet tHD;STA timing constraints. > > Correct. > > >>Yeah, looks like so. tHD;STA is the SDA hold time. I wonder if the above > >>comments apply to some earlier version of the IP that didn't have the SDA > >>hold register? > > If I remember DesignWare APB I2C spec correctly, SDA hold time register > doesn't help to meet tHD;STA spec. Could someone confirm it really so > with a real hardware, please? Indeed, SDA hold in the DesignWare I2C is not the same as tHD;STA according the databook. Unfortunately I don't have means to actually measure that here. Anyway, the HCNT, LCNT and SDA hold time values we get from ACPI SSCN/FMCN methods are measured by our HW guys on a certain board and they have verified that using those we meet all the I2C timing specs. In order to take advantage of those we need some way to pass those values to the i2c designware core. I have two suggestions: - Use the method outlined in this patch and let the interface driver override *CNT values. - Allow interface drivers to override the function that does calculations for these values like: if (dev->std_scl_cnt) dev->std_scl_cnt(dev, &hcnt, &lcnt); else /* Fallback to the calculation based solely on iclk */ Any comments on these? -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Now I've had a look at the whole discussion. Basically, DW I2C core provides a good enough (and quite direct) way to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers. But from my experience (with a slightly old version of DW I2C core around 2005, version 1.06a or so), DW I2C core was apparently lacking in an appropriate hardware mechanism to meet tHD;STA timing spec. We then found that we could meet tHD;STA by increasing *HCNT values, so that's what currently we have in i2c-designware.c I know with that workaround applied, tHIGH is to be configured with a larger value than necessary, but we have no choice. For I2C bus systems, we must meet every timing constraint strictly as required. If DW I2C core cannot meet tHD;STA without the sacrifice of tHIGH, and I would call it a hardware bug. On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote: >> If I understand the above, it leaves Tf and Tr to be PCB specific and then >> these values are passed to the core driver from platform data, right? > > That would be the idea: Calculate Th' and Tl' in function of the desired > clock frequency and duty cycle and then adapt these values using > measured transition times. I think this would be a good solution. On 7/8/13 10:42 PM, Christian Ruppert wrote: > Anyway, the HCNT, LCNT and SDA hold time values we get from ACPI SSCN/FMCN > methods are measured by our HW guys on a certain board and they have > verified that using those we meet all the I2C timing specs. > > In order to take advantage of those we need some way to pass those values > to the i2c designware core. I have two suggestions: > > - Use the method outlined in this patch and let the interface driver > override *CNT values. With *HCNT and *LCNT registers, we can control tHIGH, tLOW, tHD;STA quite accurately. On the other hand, what we can't control with DW I2C core is tr and tf. I assumed that it could never be longer than 300nsec (it's defined as a max. in the I2C specification), so I used it for calculation. I agree that tr and tf are PCB specific, and it would a good first step toward timing optimization to make them configurable through platform data. Second step is that if current i2c_dw_scl_hcnt and i2c_dw_scl_lcnt calculations don't suit with later DW I2C cores, then it would be nice for someone who can access to the data book to update formulas, or provide alternative formulas and make them selectable depending on DW core versions. It always helps us to have a way to calculate *HCNT and *LCNT values automatically. As said above, DW I2C core can control tHIGH, tLOW, tHD;STA, etc. quite _accurate_, if HCNT/LCNT values were calculated with proper formulas. It also helps hardware people as well to provide reference HCNT/LCNT values. And as a third step, if we want to use optimized HCNT/LCNT values which can not be obtained from proper formulas + user-requested tf/tr, or if we want to use HCNT/LCNT settings verified by vendors or provided from hardware team, then I'm fine with having a way to _override_ HCNT/LCNT values. Such direct way might be useful. > - Allow interface drivers to override the function that does calculations > for these values like: > > if (dev->std_scl_cnt) > dev->std_scl_cnt(dev, &hcnt, &lcnt); > else > /* Fallback to the calculation based solely on iclk */ To make the code having less indentations and be more clear that *CNT values are being overridden, something like this would be nice (leave more good comments if necessary I'll leave it to you): /* set standard and fast speed deviders for high/low periods */ /* Standard-mode */ hcnt = i2c_dw_scl_hcnt(input_clock_khz, 40, /* tHD;STA = tHIGH = 4.0 us */ 3, /* tf = 0.3 us */ 0, /* 0: DW default, 1: Ideal */ 0); /* No offset */ lcnt = i2c_dw_scl_lcnt(input_clock_khz, 47, /* tLOW = 4.7 us */ 3, /* tf = 0.3 us */ 0); /* No offset */ + if (dev->ss_hcnt && dev->ss_lcnt) { + /* give preference to immediate values over optimal ones */ + hcnt = dev->ss_hcnt; + lcnt = dev->ss_lcnt; + } dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT); dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT); dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt); Shinya -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jul 13, 2013 at 02:36:43PM +0900, Shinya Kuribayashi wrote: > Hi, > > Now I've had a look at the whole discussion. > > Basically, DW I2C core provides a good enough (and quite direct) way > to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers. > > But from my experience (with a slightly old version of DW I2C core > around 2005, version 1.06a or so), DW I2C core was apparently lacking > in an appropriate hardware mechanism to meet tHD;STA timing spec. We > then found that we could meet tHD;STA by increasing *HCNT values, so > that's what currently we have in i2c-designware.c I know with that > workaround applied, tHIGH is to be configured with a larger value > than necessary, but we have no choice. For I2C bus systems, we must > meet every timing constraint strictly as required. If DW I2C core > cannot meet tHD;STA without the sacrifice of tHIGH, and I would call > it a hardware bug. I agree. However, tHD;STA [min] is defined to the same value as tHIGH [min] for all modes in the I2C specification. Do I understand you correctly that the SCL_HCNT parameters control at the same time tHIGH and tHD;STA? Concerning SDA_HOLD_TIME, we have done a few measurements in our lab and it looks like the delay between the falling edge clock of the start condition and the rising edge of the first data bit of the start byte is controlled by this parameter. I conclude that in the drawing below (1) is controlled by SCL_HCNT whereas (2) is controlled by SDA_HOLD_TIME. _________ scl \__________ ... ___ ______ sda \_________/ ... |-----|---| (1) (2) > On Wed, Jul 10, 2013 at 06:56:35PM +0200, Christian Ruppert wrote: > >>If I understand the above, it leaves Tf and Tr to be PCB specific and then > >>these values are passed to the core driver from platform data, right? > > > >That would be the idea: Calculate Th' and Tl' in function of the desired > >clock frequency and duty cycle and then adapt these values using > >measured transition times. > > I think this would be a good solution. > > On 7/8/13 10:42 PM, Christian Ruppert wrote: > >Anyway, the HCNT, LCNT and SDA hold time values we get from ACPI SSCN/FMCN > >methods are measured by our HW guys on a certain board and they have > >verified that using those we meet all the I2C timing specs. > > > >In order to take advantage of those we need some way to pass those values > >to the i2c designware core. I have two suggestions: > > > > - Use the method outlined in this patch and let the interface driver > > override *CNT values. > > With *HCNT and *LCNT registers, we can control tHIGH, tLOW, tHD;STA > quite accurately. On the other hand, what we can't control with DW > I2C core is tr and tf. I assumed that it could never be longer than > 300nsec (it's defined as a max. in the I2C specification), so I used > it for calculation. > > I agree that tr and tf are PCB specific, and it would a good first > step toward timing optimization to make them configurable through > platform data. > > Second step is that if current i2c_dw_scl_hcnt and i2c_dw_scl_lcnt > calculations don't suit with later DW I2C cores, then it would be > nice for someone who can access to the data book to update formulas, > or provide alternative formulas and make them selectable depending > on DW core versions. I'm not having the impression there is a huge difference between the different generations of DW blocks. Probably we can find one formula that suits all blocks. We just have to be careful (in doubt rather conservative) with the transition times. This seems to be currently the case and if I understand Mika correctly, his objective is to remove some of this conservatism. > It always helps us to have a way to calculate *HCNT and *LCNT values > automatically. As said above, DW I2C core can control tHIGH, tLOW, > tHD;STA, etc. quite _accurate_, if HCNT/LCNT values were calculated > with proper formulas. It also helps hardware people as well to > provide reference HCNT/LCNT values. > > And as a third step, if we want to use optimized HCNT/LCNT values > which can not be obtained from proper formulas + user-requested > tf/tr, or if we want to use HCNT/LCNT settings verified by vendors > or provided from hardware team, then I'm fine with having a way to > _override_ HCNT/LCNT values. Such direct way might be useful. I agree. Probably it is best to have both, a default method based on formulas and timing parameters (the formulas are quite simple anyway) which works with device tree and such and a second method based on register values which works with mechanisms like ACPI. Christian -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/16/13 8:16 PM, Christian Ruppert wrote:> On Sat, Jul 13, 2013 at 02:36:43PM +0900, Shinya Kuribayashi wrote: >> Basically, DW I2C core provides a good enough (and quite direct) way >> to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers. >> >> But from my experience (with a slightly old version of DW I2C core >> around 2005, version 1.06a or so), DW I2C core was apparently lacking >> in an appropriate hardware mechanism to meet tHD;STA timing spec. We >> then found that we could meet tHD;STA by increasing *HCNT values, so >> that's what currently we have in i2c-designware.c I know with that >> workaround applied, tHIGH is to be configured with a larger value >> than necessary, but we have no choice. For I2C bus systems, we must >> meet every timing constraint strictly as required. If DW I2C core >> cannot meet tHD;STA without the sacrifice of tHIGH, and I would call >> it a hardware bug. > > I agree. However, tHD;STA [min] is defined to the same value as tHIGH > [min] for all modes in the I2C specification. Do I understand you > correctly that the SCL_HCNT parameters control at the same time tHIGH > and tHD;STA? *HCNT value does affect both tHIGH and tHD;STA at the same time. But we have to make sure that composition of tHIGH is different from the one of tHD;STA. For tHIGH ---------- DW I2C core is aware of the SCL rising transition (tr) and can ignore it. It starts counting the HIGH period of the SCL signal (tHIGH) after the SCL input voltage increases at VIH. This is described in the data book as an ideal configuration, and the resulting formula would be: HCNT + (1+4+3) >= IC_CLK * tHIGH ... (a) please refer to the data book for details about (1+4+3) part. For tLOW -------- DW I2C core starts counting the SCL CNTs for the LOW period of the SCL signal (tLOW) as soon as it pulls the SCL line _without_ _confirming_ the SCL input voltage decreases at VIL. In order to meet the tLOW timing spec, we need to take into account the fall time of SCL signal (tf), so the resulting formula should be: LCNT + 1 >= IC_CLK * (tLOW + tf) please refer to the data book for details about '+1' part. For tHD;STA ----------- There is no explanation about tHD;STA in the data book. We only have (my) experimental result; tHD;STA turned out to be proportional to 'HCNT + 3'. The formula is: HCNT + 3 >= IC_CLK * (tHD;STA + tf) ... (b) DW I2C core seems to start counting the SCL CNTs for the HIGH period of the SCL signel (tHD;STA) as soon as it pulls the _SDA_ line without confirming the SDA input voltage decreases at VIL, so we have to take into account the SDA falling transition (tf) here. As can be expected from (a) and (b), if tHD;STA [min] is defined to the same value as tHIGH [min], we need to have larger HCNT value than necessary for tHIGH, to meet tHD;STA constraint. > Concerning SDA_HOLD_TIME, we have done a few measurements in our lab and > it looks like the delay between the falling edge clock of the start > condition and the rising edge of the first data bit of the start byte is > controlled by this parameter. I conclude that in the drawing below (1) > is controlled by SCL_HCNT whereas (2) is controlled by SDA_HOLD_TIME. > > _________ > scl \__________ ... > ___ ______ > sda \_________/ ... > > |-----|---| > (1) (2) Thanks for the clarification, understood. Shinya >>> In order to take advantage of those we need some way to pass those values >>> to the i2c designware core. I have two suggestions: >>> >>> - Use the method outlined in this patch and let the interface driver >>> override *CNT values. >> >> With *HCNT and *LCNT registers, we can control tHIGH, tLOW, tHD;STA >> quite accurately. On the other hand, what we can't control with DW >> I2C core is tr and tf. I assumed that it could never be longer than >> 300nsec (it's defined as a max. in the I2C specification), so I used >> it for calculation. >> >> I agree that tr and tf are PCB specific, and it would a good first >> step toward timing optimization to make them configurable through >> platform data. >> >> Second step is that if current i2c_dw_scl_hcnt and i2c_dw_scl_lcnt >> calculations don't suit with later DW I2C cores, then it would be >> nice for someone who can access to the data book to update formulas, >> or provide alternative formulas and make them selectable depending >> on DW core versions. > > I'm not having the impression there is a huge difference between the > different generations of DW blocks. Probably we can find one formula > that suits all blocks. We just have to be careful (in doubt rather > conservative) with the transition times. This seems to be currently > the case and if I understand Mika correctly, his objective is to remove > some of this conservatism. > >> It always helps us to have a way to calculate *HCNT and *LCNT values >> automatically. As said above, DW I2C core can control tHIGH, tLOW, >> tHD;STA, etc. quite _accurate_, if HCNT/LCNT values were calculated >> with proper formulas. It also helps hardware people as well to >> provide reference HCNT/LCNT values. >> >> And as a third step, if we want to use optimized HCNT/LCNT values >> which can not be obtained from proper formulas + user-requested >> tf/tr, or if we want to use HCNT/LCNT settings verified by vendors >> or provided from hardware team, then I'm fine with having a way to >> _override_ HCNT/LCNT values. Such direct way might be useful. > > I agree. Probably it is best to have both, a default method based on > formulas and timing parameters (the formulas are quite simple anyway) > which works with device tree and such and a second method based on > register values which works with mechanisms like ACPI. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 17, 2013 at 11:39:58PM +0900, Shinya Kuribayashi wrote: > On 7/16/13 8:16 PM, Christian Ruppert wrote:> On Sat, Jul 13, 2013 at 02:36:43PM +0900, Shinya Kuribayashi wrote: > >>Basically, DW I2C core provides a good enough (and quite direct) way > >>to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers. > >> > >>But from my experience (with a slightly old version of DW I2C core > >>around 2005, version 1.06a or so), DW I2C core was apparently lacking > >>in an appropriate hardware mechanism to meet tHD;STA timing spec. We > >>then found that we could meet tHD;STA by increasing *HCNT values, so > >>that's what currently we have in i2c-designware.c I know with that > >>workaround applied, tHIGH is to be configured with a larger value > >>than necessary, but we have no choice. For I2C bus systems, we must > >>meet every timing constraint strictly as required. If DW I2C core > >>cannot meet tHD;STA without the sacrifice of tHIGH, and I would call > >>it a hardware bug. > > > >I agree. However, tHD;STA [min] is defined to the same value as tHIGH > >[min] for all modes in the I2C specification. Do I understand you > >correctly that the SCL_HCNT parameters control at the same time tHIGH > >and tHD;STA? > > *HCNT value does affect both tHIGH and tHD;STA at the same time. > But we have to make sure that composition of tHIGH is different > from the one of tHD;STA. > > For tHIGH > ---------- > > DW I2C core is aware of the SCL rising transition (tr) and can > ignore it. It starts counting the HIGH period of the SCL signal > (tHIGH) after the SCL input voltage increases at VIH. > > This is described in the data book as an ideal configuration, > and the resulting formula would be: > > HCNT + (1+4+3) >= IC_CLK * tHIGH ... (a) > > please refer to the data book for details about (1+4+3) part. > > For tLOW > -------- > > DW I2C core starts counting the SCL CNTs for the LOW period of > the SCL signal (tLOW) as soon as it pulls the SCL line _without_ > _confirming_ the SCL input voltage decreases at VIL. > > In order to meet the tLOW timing spec, we need to take into > account the fall time of SCL signal (tf), so the resulting > formula should be: > > LCNT + 1 >= IC_CLK * (tLOW + tf) > > please refer to the data book for details about '+1' part. > > For tHD;STA > ----------- > > There is no explanation about tHD;STA in the data book. We > only have (my) experimental result; tHD;STA turned out to be > proportional to 'HCNT + 3'. The formula is: > > HCNT + 3 >= IC_CLK * (tHD;STA + tf) ... (b) > > DW I2C core seems to start counting the SCL CNTs for the HIGH > period of the SCL signel (tHD;STA) as soon as it pulls the _SDA_ > line without confirming the SDA input voltage decreases at VIL, > so we have to take into account the SDA falling transition (tf) > here. > > As can be expected from (a) and (b), if tHD;STA [min] is defined > to the same value as tHIGH [min], we need to have larger HCNT > value than necessary for tHIGH, to meet tHD;STA constraint. > > [...] Hrmmm... This makes my head spin. Do you think the following code snippet represents an accurate method to calculate the register values? If you agree I'll prepare a patch based on that for the DW I2C. The question will be, however, how to obtain the transition times. Would it make sense to add generic I2C device tree properties for those parameters? These parameters are independent of the actual bus driver, rather a PCB property... And as such the correct place would be device tree or ACPI or similar. Greetings, Christian --- SNIP --- /* * t_f;SDA * |-| * _____ _____ . . . * \ / * SDA \ / * \________/ t_r;SCL t_f;SCL * |-| |-| * ______________ ________ * \ / \ * SCL \ / \ * \_________/ \_______ . . . * |------| |---------| |--------| * t_HD;STA t_LOW t_HIGH * * |--------|-----------| |--------| * ( HCNT LCNT HCNT ) * 1/f_SYS * * * HCNT = f_SYS * max(t_HD;STA + t_f;SDA , t_HIGH) * = f_SYS * (t_HIGH + t_f;SDA) because T_HD;STA == T_HIGH * LCNT = f_SYS * (t_f;SCL + t_LOW) * HCNT + LCNT + t_r;SCL = 1/f_SCL = t_SCL */ #define I2C_STD_MODE (0) #define I2C_FAST_MODE (1) #define I2C_FAST_MODE_PLUS (2) static const struct i2c_timing_params { unsigned int t_SCL; /* t_SCL in (ms * 65536) */ unsigned int t_LOW; /* t_LOW in (ms * 65536) */ unsigned int t_HIGH; /* t_HIGH = t_HD;STA in (ms * 65536) */ } i2c_timing_params[] = { { /* I2C_STD_MODE */ .t_SCL = 10.0e-3 * 65536 + 0.5, .t_LOW = 4.7e-3 * 65536 + 0.5, .t_HIGH = 4.0e-3 * 65536 + 0.5, }, { /* I2C_FAST_MODE */ .t_SCL = 2.5e-3 * 65536 + 0.5, .t_LOW = 1.3e-3 * 65536 + 0.5, .t_HIGH = 0.6e-3 * 65536 + 0.5, }, { /* I2C_FAST_MODE_PLUS */ .t_SCL = 1.0e-3 * 65536 + 0.5, .t_LOW = 0.5e-3 * 65536 + 0.5, .t_HIGH = 0.26e-3 * 65536 + 0.5, }, }; #define DW_CNT_OFFS (1) #define DW_FILT_OFFS (2) #define DW_SCL_LATENCY (3) /* * all timings in ns f_SYS in kHz * For worst case scenario assume tf_SCL = tf_SDA = 300ns, tr_SCL = 0ns */ void calc_timing_params(u16 *HCNT, u16 *LCNT, unsigned int f_SYS unsigned int tf_SCL, unsigned int tr_SCL, unsigned int tf_SDA, unsigned int mode, unsigned int IC_SPKLEN) { unsigned int H, L; int D; /* * The worst case High count timing includes the SDA falling * transition of start condition */ H = f_SYS * (tf_SDA + i2c_timing_params[mode].t_HIGH); /* The Low count timing always includes the preceding falling edge. */ L = f_SYS * (tf_SCL + i2c_timing_params[mode].t_LOW); /* * The clock timings must not exceed the maximum frequencies * from the I2C specification, slow down if required. */ D = (f_SYS * i2c_timing_params[mode].t_scl - H - L - tr_SCL) / 2; if (D > 0) { H += D; L += D; } /* * Round the HIGH count and subtract * (DW_CNT_OFFS + DW_FILT_OFFS + DW_SCL_LATENCY + IC_SPKLEN) * which are added to the register value inside the DW IP. */ H = ((H + 32768) >> 16) - DW_CNT_OFFS - DW_FILT_OFFS - DW_SCL_LATENCY - IC_SPKLEN; /* * Round the LOW count and subtract DW_CNT_OFFS which is added to * the register value inside the DW IP. */ L = ((L + 32768) >> 16) - DW_CNT_OFFS; /* Respect the DW IP minimum timings */ if (H < (IC_SPKLEN + 5)) H = IC_SPKLEN + 5; if (L < (IC_SPKLEN + 7)) L = IC_SPKLEN + 7; *HCNT = H; *LCNT = L; } -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/22/13 10:17 PM, Christian Ruppert wrote:> On Wed, Jul 17, 2013 at 11:39:58PM +0900, Shinya Kuribayashi wrote: >> On 7/16/13 8:16 PM, Christian Ruppert wrote:> On Sat, Jul 13, 2013 at 02:36:43PM +0900, Shinya Kuribayashi wrote: >>>> Basically, DW I2C core provides a good enough (and quite direct) way >>>> to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers. >>>> >>>> But from my experience (with a slightly old version of DW I2C core >>>> around 2005, version 1.06a or so), DW I2C core was apparently lacking >>>> in an appropriate hardware mechanism to meet tHD;STA timing spec. We >>>> then found that we could meet tHD;STA by increasing *HCNT values, so >>>> that's what currently we have in i2c-designware.c I know with that >>>> workaround applied, tHIGH is to be configured with a larger value >>>> than necessary, but we have no choice. For I2C bus systems, we must >>>> meet every timing constraint strictly as required. If DW I2C core >>>> cannot meet tHD;STA without the sacrifice of tHIGH, and I would call >>>> it a hardware bug. >>> >>> I agree. However, tHD;STA [min] is defined to the same value as tHIGH >>> [min] for all modes in the I2C specification. Do I understand you >>> correctly that the SCL_HCNT parameters control at the same time tHIGH >>> and tHD;STA? >> >> *HCNT value does affect both tHIGH and tHD;STA at the same time. >> But we have to make sure that composition of tHIGH is different >> from the one of tHD;STA. >> >> For tHIGH >> ---------- >> >> DW I2C core is aware of the SCL rising transition (tr) and can >> ignore it. It starts counting the HIGH period of the SCL signal >> (tHIGH) after the SCL input voltage increases at VIH. >> >> This is described in the data book as an ideal configuration, >> and the resulting formula would be: >> >> HCNT + (1+4+3) >= IC_CLK * tHIGH ... (a) >> >> please refer to the data book for details about (1+4+3) part. >> >> For tLOW >> -------- >> >> DW I2C core starts counting the SCL CNTs for the LOW period of >> the SCL signal (tLOW) as soon as it pulls the SCL line _without_ >> _confirming_ the SCL input voltage decreases at VIL. >> >> In order to meet the tLOW timing spec, we need to take into >> account the fall time of SCL signal (tf), so the resulting >> formula should be: >> >> LCNT + 1 >= IC_CLK * (tLOW + tf) >> >> please refer to the data book for details about '+1' part. >> >> For tHD;STA >> ----------- >> >> There is no explanation about tHD;STA in the data book. We >> only have (my) experimental result; tHD;STA turned out to be >> proportional to 'HCNT + 3'. The formula is: >> >> HCNT + 3 >= IC_CLK * (tHD;STA + tf) ... (b) >> >> DW I2C core seems to start counting the SCL CNTs for the HIGH >> period of the SCL signel (tHD;STA) as soon as it pulls the _SDA_ >> line without confirming the SDA input voltage decreases at VIL, >> so we have to take into account the SDA falling transition (tf) >> here. >> >> As can be expected from (a) and (b), if tHD;STA [min] is defined >> to the same value as tHIGH [min], we need to have larger HCNT >> value than necessary for tHIGH, to meet tHD;STA constraint. >> >> [...] > > Hrmmm... This makes my head spin. Do you think the following code > snippet represents an accurate method to calculate the register values? > If you agree I'll prepare a patch based on that for the DW I2C. The > question will be, however, how to obtain the transition times. Please find my comments below. > /* > * t_f;SDA > * |-| > * _____ _____ . . . > * \ / > * SDA \ / > * \________/ t_r;SCL t_f;SCL > * |-| |-| > * ______________ ________ > * \ / \ > * SCL \ / \ > * \_________/ \_______ . . . > * |------| |---------| |--------| > * t_HD;STA t_LOW t_HIGH > * > * |--------|-----------| |--------| > * ( HCNT LCNT HCNT ) * 1/f_SYS Composition is: HCNT+3 LCNT+1 HCNT+(1+4+3) The offsets for the HCNT are different in the cases of tHD;STA and t_HIGH. It's based on my experimental result and we can't know how DW I2C core actually counts those period, but it can be easily imagined that for DW I2C core, the way to count t_HD;STA is similar to the way to count t_LOW; it starts counting CNTs as soon as it pulls the SCL/SDA line, then waits for given CNTs. I think your equations and snippet code are based on the assumption that DW I2C core must be counting the number of CNTs for the HIGH period of the SCL signal (that's t_HD;STA and t_HIGH) in the same way, but I don't think so. From my observation and experimental results, it must be different. We couldn't know what actually is, though. > * > * HCNT = f_SYS * max(t_HD;STA + t_f;SDA , t_HIGH) > * = f_SYS * (t_HIGH + t_f;SDA) because T_HD;STA == T_HIGH > * LCNT = f_SYS * (t_f;SCL + t_LOW) > * HCNT + LCNT + t_r;SCL = 1/f_SCL = t_SCL As said before, all t_SCL things should go away. Please forget about 100kbps, 400kbps, and so on. Bus/clock speed is totally pointless concept for the I2C bus systems. For example, as long as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a certain I2C bus, it doesn't matter that the resulting clock speed is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode. Nobody in the I2C bus doesn't care about actual bus/clock speed. We don't have to care about the resulting bus speed, or rather we should/must not check to see if it's within the proper range. > #define I2C_STD_MODE (0) > #define I2C_FAST_MODE (1) > #define I2C_FAST_MODE_PLUS (2) > > static const struct i2c_timing_params { > unsigned int t_SCL; /* t_SCL in (ms * 65536) */ > unsigned int t_LOW; /* t_LOW in (ms * 65536) */ > unsigned int t_HIGH; /* t_HIGH = t_HD;STA in (ms * 65536) */ > } i2c_timing_params[] = { > { /* I2C_STD_MODE */ > .t_SCL = 10.0e-3 * 65536 + 0.5, > .t_LOW = 4.7e-3 * 65536 + 0.5, > .t_HIGH = 4.0e-3 * 65536 + 0.5, > }, > { /* I2C_FAST_MODE */ > .t_SCL = 2.5e-3 * 65536 + 0.5, > .t_LOW = 1.3e-3 * 65536 + 0.5, > .t_HIGH = 0.6e-3 * 65536 + 0.5, > }, > { /* I2C_FAST_MODE_PLUS */ > .t_SCL = 1.0e-3 * 65536 + 0.5, > .t_LOW = 0.5e-3 * 65536 + 0.5, > .t_HIGH = 0.26e-3 * 65536 + 0.5, > }, > }; I just can't take to these 65536s, 0.5s, and {(x + 32768) >> 16}, as it's not intuitive. Would be nice to come up with another way! > #define DW_CNT_OFFS (1) > #define DW_FILT_OFFS (2) > #define DW_SCL_LATENCY (3) > > /* > * all timings in ns f_SYS in kHz > * For worst case scenario assume tf_SCL = tf_SDA = 300ns, tr_SCL = 0ns > */ > void calc_timing_params(u16 *HCNT, u16 *LCNT, unsigned int f_SYS > unsigned int tf_SCL, unsigned int tr_SCL, > unsigned int tf_SDA, unsigned int mode, > unsigned int IC_SPKLEN) > { > unsigned int H, L; > int D; > /* > * The worst case High count timing includes the SDA falling > * transition of start condition > */ > H = f_SYS * (tf_SDA + i2c_timing_params[mode].t_HIGH); > > /* The Low count timing always includes the preceding falling edge. */ > L = f_SYS * (tf_SCL + i2c_timing_params[mode].t_LOW); > > /* > * The clock timings must not exceed the maximum frequencies > * from the I2C specification, slow down if required. > */ > D = (f_SYS * i2c_timing_params[mode].t_scl - H - L - tr_SCL) / 2; > if (D > 0) { > H += D; > L += D; > } These H/L adjustments based on clock timings are not necessary. > /* > * Round the HIGH count and subtract > * (DW_CNT_OFFS + DW_FILT_OFFS + DW_SCL_LATENCY + IC_SPKLEN) > * which are added to the register value inside the DW IP. > */ > H = ((H + 32768) >> 16) - DW_CNT_OFFS - DW_FILT_OFFS > - DW_SCL_LATENCY - IC_SPKLEN; And this adjustment for HCNT has to be altered to suit t_HIGH or t_HD;STA. It can not be processed in a single form like this. > /* > * Round the LOW count and subtract DW_CNT_OFFS which is added to > * the register value inside the DW IP. > */ > L = ((L + 32768) >> 16) - DW_CNT_OFFS; This is Ok. > /* Respect the DW IP minimum timings */ > if (H < (IC_SPKLEN + 5)) > H = IC_SPKLEN + 5; > if (L < (IC_SPKLEN + 7)) > L = IC_SPKLEN + 7; > *HCNT = H; > *LCNT = L; > } I'm not familiar with this SPKLEN (spike length?) as it didn't appear in the data book around 2005, but it's fine to try to make sure H/L values are within the range supported. > Would it make sense to add generic I2C device tree properties for those > parameters? These parameters are independent of the actual bus driver, > rather a PCB property... And as such the correct place would be device > tree or ACPI or similar. If there are other bus drivers that make use of tr/tf transition times, I think it makes sense. Hope this helps, Shinya -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote: > On 7/22/13 10:17 PM, Christian Ruppert wrote:> On Wed, Jul 17, 2013 at 11:39:58PM +0900, Shinya Kuribayashi wrote: > >>On 7/16/13 8:16 PM, Christian Ruppert wrote:> On Sat, Jul 13, 2013 at 02:36:43PM +0900, Shinya Kuribayashi wrote: > >>>>Basically, DW I2C core provides a good enough (and quite direct) way > >>>>to control tHIGH and tLOW timing specs, *HCNT and *LCNT registers. > >>>> > >>>>But from my experience (with a slightly old version of DW I2C core > >>>>around 2005, version 1.06a or so), DW I2C core was apparently lacking > >>>>in an appropriate hardware mechanism to meet tHD;STA timing spec. We > >>>>then found that we could meet tHD;STA by increasing *HCNT values, so > >>>>that's what currently we have in i2c-designware.c I know with that > >>>>workaround applied, tHIGH is to be configured with a larger value > >>>>than necessary, but we have no choice. For I2C bus systems, we must > >>>>meet every timing constraint strictly as required. If DW I2C core > >>>>cannot meet tHD;STA without the sacrifice of tHIGH, and I would call > >>>>it a hardware bug. > >>> > >>>I agree. However, tHD;STA [min] is defined to the same value as tHIGH > >>>[min] for all modes in the I2C specification. Do I understand you > >>>correctly that the SCL_HCNT parameters control at the same time tHIGH > >>>and tHD;STA? > >> > >>*HCNT value does affect both tHIGH and tHD;STA at the same time. > >>But we have to make sure that composition of tHIGH is different > >>from the one of tHD;STA. > >> > >>For tHIGH > >>---------- > >> > >>DW I2C core is aware of the SCL rising transition (tr) and can > >>ignore it. It starts counting the HIGH period of the SCL signal > >>(tHIGH) after the SCL input voltage increases at VIH. > >> > >>This is described in the data book as an ideal configuration, > >>and the resulting formula would be: > >> > >> HCNT + (1+4+3) >= IC_CLK * tHIGH ... (a) > >> > >>please refer to the data book for details about (1+4+3) part. > >> > >>For tLOW > >>-------- > >> > >>DW I2C core starts counting the SCL CNTs for the LOW period of > >>the SCL signal (tLOW) as soon as it pulls the SCL line _without_ > >>_confirming_ the SCL input voltage decreases at VIL. > >> > >>In order to meet the tLOW timing spec, we need to take into > >>account the fall time of SCL signal (tf), so the resulting > >>formula should be: > >> > >> LCNT + 1 >= IC_CLK * (tLOW + tf) > >> > >>please refer to the data book for details about '+1' part. > >> > >>For tHD;STA > >>----------- > >> > >>There is no explanation about tHD;STA in the data book. We > >>only have (my) experimental result; tHD;STA turned out to be > >>proportional to 'HCNT + 3'. The formula is: > >> > >> HCNT + 3 >= IC_CLK * (tHD;STA + tf) ... (b) > >> > >>DW I2C core seems to start counting the SCL CNTs for the HIGH > >>period of the SCL signel (tHD;STA) as soon as it pulls the _SDA_ > >>line without confirming the SDA input voltage decreases at VIL, > >>so we have to take into account the SDA falling transition (tf) > >>here. > >> > >>As can be expected from (a) and (b), if tHD;STA [min] is defined > >>to the same value as tHIGH [min], we need to have larger HCNT > >>value than necessary for tHIGH, to meet tHD;STA constraint. > >> > >>[...] > > > >Hrmmm... This makes my head spin. Do you think the following code > >snippet represents an accurate method to calculate the register values? > >If you agree I'll prepare a patch based on that for the DW I2C. The > >question will be, however, how to obtain the transition times. > > Please find my comments below. > > >/* > > * t_f;SDA > > * |-| > > * _____ _____ . . . > > * \ / > > * SDA \ / > > * \________/ t_r;SCL t_f;SCL > > * |-| |-| > > * ______________ ________ > > * \ / \ > > * SCL \ / \ > > * \_________/ \_______ . . . > > * |------| |---------| |--------| > > * t_HD;STA t_LOW t_HIGH > > * > > * |--------|-----------| |--------| > > * ( HCNT LCNT HCNT ) * 1/f_SYS > > Composition is: HCNT+3 LCNT+1 HCNT+(1+4+3) OK > The offsets for the HCNT are different in the cases of tHD;STA and > t_HIGH. It's based on my experimental result and we can't know how > DW I2C core actually counts those period, but it can be easily > imagined that for DW I2C core, the way to count t_HD;STA is similar > to the way to count t_LOW; it starts counting CNTs as soon as it > pulls the SCL/SDA line, then waits for given CNTs. > > I think your equations and snippet code are based on the assumption > that DW I2C core must be counting the number of CNTs for the HIGH > period of the SCL signal (that's t_HD;STA and t_HIGH) in the same > way, but I don't think so. From my observation and experimental > results, it must be different. We couldn't know what actually is, > though. > > > * > > * HCNT = f_SYS * max(t_HD;STA + t_f;SDA , t_HIGH) > > * = f_SYS * (t_HIGH + t_f;SDA) because T_HD;STA == T_HIGH > > * LCNT = f_SYS * (t_f;SCL + t_LOW) > > * HCNT + LCNT + t_r;SCL = 1/f_SCL = t_SCL So the corrected HCNT would be: HCNT = max(f_SYS * (t_HD;STA + t_f;SDA) - 3, f_SYS * t_HIGH - 7) = f_SYS * (t_HD;STA + t_f;SDA) - 3 > As said before, all t_SCL things should go away. Please forget > about 100kbps, 400kbps, and so on. Bus/clock speed is totally > pointless concept for the I2C bus systems. For example, as long > as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a > certain I2C bus, it doesn't matter that the resulting clock speed > is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode. > Nobody in the I2C bus doesn't care about actual bus/clock speed. > > We don't have to care about the resulting bus speed, or rather > we should/must not check to see if it's within the proper range. Actually, the I2C specification clearly defines f_SCL;max (and thus implies t_SCL;min), both in the tables and the timing diagrams. Why can we ignore this constraint while having to meet all the others? > >#define I2C_STD_MODE (0) > >#define I2C_FAST_MODE (1) > >#define I2C_FAST_MODE_PLUS (2) > > > >static const struct i2c_timing_params { > > unsigned int t_SCL; /* t_SCL in (ms * 65536) */ > > unsigned int t_LOW; /* t_LOW in (ms * 65536) */ > > unsigned int t_HIGH; /* t_HIGH = t_HD;STA in (ms * 65536) */ > >} i2c_timing_params[] = { > > { /* I2C_STD_MODE */ > > .t_SCL = 10.0e-3 * 65536 + 0.5, > > .t_LOW = 4.7e-3 * 65536 + 0.5, > > .t_HIGH = 4.0e-3 * 65536 + 0.5, > > }, > > { /* I2C_FAST_MODE */ > > .t_SCL = 2.5e-3 * 65536 + 0.5, > > .t_LOW = 1.3e-3 * 65536 + 0.5, > > .t_HIGH = 0.6e-3 * 65536 + 0.5, > > }, > > { /* I2C_FAST_MODE_PLUS */ > > .t_SCL = 1.0e-3 * 65536 + 0.5, > > .t_LOW = 0.5e-3 * 65536 + 0.5, > > .t_HIGH = 0.26e-3 * 65536 + 0.5, > > }, > >}; > > I just can't take to these 65536s, 0.5s, and {(x + 32768) >> 16}, > as it's not intuitive. Would be nice to come up with another way! OK, I'll make this more readable in the next version. > >#define DW_CNT_OFFS (1) > >#define DW_FILT_OFFS (2) > >#define DW_SCL_LATENCY (3) > > > >/* > > * all timings in ns f_SYS in kHz > > * For worst case scenario assume tf_SCL = tf_SDA = 300ns, tr_SCL = 0ns > > */ > >void calc_timing_params(u16 *HCNT, u16 *LCNT, unsigned int f_SYS > > unsigned int tf_SCL, unsigned int tr_SCL, > > unsigned int tf_SDA, unsigned int mode, > > unsigned int IC_SPKLEN) > >{ > > unsigned int H, L; > > int D; > > /* > > * The worst case High count timing includes the SDA falling > > * transition of start condition > > */ > > H = f_SYS * (tf_SDA + i2c_timing_params[mode].t_HIGH); > > > > /* The Low count timing always includes the preceding falling edge. */ > > L = f_SYS * (tf_SCL + i2c_timing_params[mode].t_LOW); > > > > /* > > * The clock timings must not exceed the maximum frequencies > > * from the I2C specification, slow down if required. > > */ > > D = (f_SYS * i2c_timing_params[mode].t_scl - H - L - tr_SCL) / 2; > > if (D > 0) { > > H += D; > > L += D; > > } > > These H/L adjustments based on clock timings are not necessary. > > > /* > > * Round the HIGH count and subtract > > * (DW_CNT_OFFS + DW_FILT_OFFS + DW_SCL_LATENCY + IC_SPKLEN) > > * which are added to the register value inside the DW IP. > > */ > > H = ((H + 32768) >> 16) - DW_CNT_OFFS - DW_FILT_OFFS > > - DW_SCL_LATENCY - IC_SPKLEN; > > And this adjustment for HCNT has to be altered to suit t_HIGH or > t_HD;STA. It can not be processed in a single form like this. I agree. > > /* > > * Round the LOW count and subtract DW_CNT_OFFS which is added to > > * the register value inside the DW IP. > > */ > > L = ((L + 32768) >> 16) - DW_CNT_OFFS; > > This is Ok. > > > /* Respect the DW IP minimum timings */ > > if (H < (IC_SPKLEN + 5)) > > H = IC_SPKLEN + 5; > > if (L < (IC_SPKLEN + 7)) > > L = IC_SPKLEN + 7; > > *HCNT = H; > > *LCNT = L; > >} > > I'm not familiar with this SPKLEN (spike length?) as it didn't > appear in the data book around 2005, but it's fine to try to make > sure H/L values are within the range supported. Yes, SPKLEN is a register defining the number of cycles to filter out in spike filtering. If this has appeared sometime between 2005 and today we must also check the version for this. This is really starting to become complicated. > >Would it make sense to add generic I2C device tree properties for those > >parameters? These parameters are independent of the actual bus driver, > >rather a PCB property... And as such the correct place would be device > >tree or ACPI or similar. > > If there are other bus drivers that make use of tr/tf transition > times, I think it makes sense. Wolfram, what's your opinion on this? Greetings, Christian
> > >Would it make sense to add generic I2C device tree properties for those > > >parameters? These parameters are independent of the actual bus driver, > > >rather a PCB property... And as such the correct place would be device > > >tree or ACPI or similar. > > > > If there are other bus drivers that make use of tr/tf transition > > times, I think it makes sense. > > Wolfram, what's your opinion on this? If it is a PCB property, it makes sense to have generic bindings for it. Can they have very-safe defaults and thus be optional?
On Mon, Aug 05, 2013 at 12:02:26PM +0200, Wolfram Sang wrote: > > > > >Would it make sense to add generic I2C device tree properties for those > > > >parameters? These parameters are independent of the actual bus driver, > > > >rather a PCB property... And as such the correct place would be device > > > >tree or ACPI or similar. > > > > > > If there are other bus drivers that make use of tr/tf transition > > > times, I think it makes sense. > > > > Wolfram, what's your opinion on this? > > If it is a PCB property, it makes sense to have generic bindings for > it. Can they have very-safe defaults and thus be optional? We can definitely have safe defaults that work for a given driver/hardware. I don't think the same defaults would be safe for all drivers/hardware: The timing strategies of different I2C hardware seems to vary widely (which edges of the clock are sampled, how does different hardware deal with hold times etc) and depending on which parameters are available to the driver to control these timings, the safe values would either have to be the minimum or the maximum in the range allowed by the I2C specification. Every driver would thus have to implement its own defaults in case the properties are not defined. Christian
> Every driver would thus have to implement its own defaults in case the > properties are not defined. Placing the defaults at driver level sounds fine to me. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 8/5/13 6:31 PM, Christian Ruppert wrote:> On Wed, Jul 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote: >> As said before, all t_SCL things should go away. Please forget >> about 100kbps, 400kbps, and so on. Bus/clock speed is totally >> pointless concept for the I2C bus systems. For example, as long >> as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a >> certain I2C bus, it doesn't matter that the resulting clock speed >> is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode. >> Nobody in the I2C bus doesn't care about actual bus/clock speed. >> >> We don't have to care about the resulting bus speed, or rather >> we should/must not check to see if it's within the proper range. > > Actually, the I2C specification clearly defines f_SCL;max (and thus > implies t_SCL;min), both in the tables and the timing diagrams. Why can > we ignore this constraint while having to meet all the others? If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case), f_SCL;max will be met by itself. And again, all I2C master and slave devices in the bus don't care about f_SCL; what they do care are t_f, t_r, t_HIGH, t_LOW, and so on. That's why I'm saying f_SCL is pointless and has no value for HCNT/LCNT calculations. Is that clear? What is the point to make sure whether f_SCL constraint is met or not? Is there any combination where t_f, t_r, t_HIGH, t_LOW, t_HD;SATA are met, but f_SCL is out of range? I don't think there is. I'd make a compromise proposal; it's fine to make sure whether the resulting f_SCL is within a supported range, but should not make a correction of HCNT/LCNT values. Just report warning messages that some parameters/calculations might be mis-configured an/or wrong. Shinya -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sorry for the slooow response, I've been on vacation. On Tue, Jul 16, 2013 at 01:16:18PM +0200, Christian Ruppert wrote: > > Second step is that if current i2c_dw_scl_hcnt and i2c_dw_scl_lcnt > > calculations don't suit with later DW I2C cores, then it would be > > nice for someone who can access to the data book to update formulas, > > or provide alternative formulas and make them selectable depending > > on DW core versions. > > I'm not having the impression there is a huge difference between the > different generations of DW blocks. Probably we can find one formula > that suits all blocks. We just have to be careful (in doubt rather > conservative) with the transition times. This seems to be currently > the case and if I understand Mika correctly, his objective is to remove > some of this conservatism. What I had originally in mind was that we could just pass whatever HCNT/LCNT values we get from system FW (with the help of ACPI or DT). However, if we can make the HCNT/LCNT calculation more accurate using tf and tr, that are passed from DT or platform data, we should implement that as well (as a separate patch). > > It always helps us to have a way to calculate *HCNT and *LCNT values > > automatically. As said above, DW I2C core can control tHIGH, tLOW, > > tHD;STA, etc. quite _accurate_, if HCNT/LCNT values were calculated > > with proper formulas. It also helps hardware people as well to > > provide reference HCNT/LCNT values. > > > > And as a third step, if we want to use optimized HCNT/LCNT values > > which can not be obtained from proper formulas + user-requested > > tf/tr, or if we want to use HCNT/LCNT settings verified by vendors > > or provided from hardware team, then I'm fine with having a way to > > _override_ HCNT/LCNT values. Such direct way might be useful. > > I agree. Probably it is best to have both, a default method based on > formulas and timing parameters (the formulas are quite simple anyway) > which works with device tree and such and a second method based on > register values which works with mechanisms like ACPI. I agree. I'm going to post a new version of this patch (and the SDA hold patch) that takes care of the ACPI case if there are no objections. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 16, 2013 at 11:15:12AM +0900, Shinya Kuribayashi wrote: > >Actually, the I2C specification clearly defines f_SCL;max (and thus > >implies t_SCL;min), both in the tables and the timing diagrams. Why can > >we ignore this constraint while having to meet all the others? > > If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case), > f_SCL;max will be met by itself. And again, all I2C master and > slave devices in the bus don't care about f_SCL; what they do care > are t_f, t_r, t_HIGH, t_LOW, and so on. That's why I'm saying > f_SCL is pointless and has no value for HCNT/LCNT calculations. One thing that comes to mind regarding the bus speed is that even if we have all the minimal timing requirements met we still prefer resulting bus speeds closer to 400kHz than 315.41kHz for the reasons that we get more data transferred that way, no? -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 8/19/13 8:36 PM, Mika Westerberg wrote: > On Fri, Aug 16, 2013 at 11:15:12AM +0900, Shinya Kuribayashi wrote: >>> Actually, the I2C specification clearly defines f_SCL;max (and thus >>> implies t_SCL;min), both in the tables and the timing diagrams. Why can >>> we ignore this constraint while having to meet all the others? >> >> If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case), >> f_SCL;max will be met by itself. And again, all I2C master and >> slave devices in the bus don't care about f_SCL; what they do care >> are t_f, t_r, t_HIGH, t_LOW, and so on. That's why I'm saying >> f_SCL is pointless and has no value for HCNT/LCNT calculations. > > One thing that comes to mind regarding the bus speed is that even if we > have all the minimal timing requirements met we still prefer resulting bus > speeds closer to 400kHz than 315.41kHz for the reasons that we get more > data transferred that way, no? That depends I2C slave devices in the bus in your target systems. As long as your slave devices can detect START/STOP conditions and recognize SDA/SCL transitions properly, that should be Ok (you can use HCNT/LCNT settings for 400 kHz without having all the minimal timing requirements met). My comments above was a reply to Christian's snippet code and how to treat f_SCL;mas constraints, and unrelated to your case in question. I'm for having a way to override HCNT/LCNT values as said before, and that should nicely work for you. Shinya -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 16, 2013 at 11:15:12AM +0900, Shinya Kuribayashi wrote: > Hi, > > On 8/5/13 6:31 PM, Christian Ruppert wrote:> On Wed, Jul 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote: > >>As said before, all t_SCL things should go away. Please forget > >>about 100kbps, 400kbps, and so on. Bus/clock speed is totally > >>pointless concept for the I2C bus systems. For example, as long > >>as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a > >>certain I2C bus, it doesn't matter that the resulting clock speed > >>is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode. > >>Nobody in the I2C bus doesn't care about actual bus/clock speed. > >> > >>We don't have to care about the resulting bus speed, or rather > >>we should/must not check to see if it's within the proper range. > > > >Actually, the I2C specification clearly defines f_SCL;max (and thus > >implies t_SCL;min), both in the tables and the timing diagrams. Why can > >we ignore this constraint while having to meet all the others? > > If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case), > f_SCL;max will be met by itself. I'm not sure if I agree with this: Standard mode: t_r;min 0ns t_f;min + 0ns t_HIGH;min + 4000ns t_LOW;min + 4700ns 1/f_SCL = 8700ns ==> f_SCL = 115kHz ==> violation of f_SCL;max=100kHz Fast mode (let's assume V_DD = 5.5V): t_r;min 20ns t_f;min + 20ns t_HIGH;min + 600ns t_LIW;min + 1300ns 1/f_SCL = 1940ns ==> f_SCL = 515kHz ==> violation of f_SCL;max=400kHz In my understanding, f_SCL;max condition is only met a) either if t_HIGH = t_HIGH;min and t_LOW = t_LOW;min then t_r must be t_r;max and t_f must be t_f;max b) or if t_r < t_r;max and t_f < t_f;max then t_HIGH must be > t_HIGH;min and T_LOW must be T_LOW;min Given that we cannot easily influence t_r and t_f we must adjust t_HIGH and t_LOW. What am I missing here? > And again, all I2C master and > slave devices in the bus don't care about f_SCL; what they do care > are t_f, t_r, t_HIGH, t_LOW, and so on. That's why I'm saying > f_SCL is pointless and has no value for HCNT/LCNT calculations. I partially agree: If I2C devices don't care about f_SCL but only about t_r, t_f, t_HIGH and t_LOW there's no need to respect the f_SCL;max constraint. If this is the case, I'm wondering why it is part of the specification, though. > Is that clear? What is the point to make sure whether f_SCL > constraint is met or not? Is there any combination where t_f, > t_r, t_HIGH, t_LOW, t_HD;SATA are met, but f_SCL is out of range? > I don't think there is. See above. > I'd make a compromise proposal; it's fine to make sure whether the > resulting f_SCL is within a supported range, but should not make a > correction of HCNT/LCNT values. Just report warning messages that > some parameters/calculations might be mis-configured an/or wrong. Not sure if this is a good idea: If f_SCL is met by design I'm perfectly happy with dropping the t_HIGH/t_LOW adjustment code, no need to bloat the kernel with confusing warnings. If f_SCL is not automatically met we must either make sure t_HIGH/t_LOW are adjusted or we must take the decision to ignore that constraint and document the reasons behind that decision accordingly. Greetings, Christian
On 8/21/13 11:39 PM, Christian Ruppert wrote: > On Fri, Aug 16, 2013 at 11:15:12AM +0900, Shinya Kuribayashi wrote: >> On 8/5/13 6:31 PM, Christian Ruppert wrote: >>> On Wed, Jul 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote: >>>> As said before, all t_SCL things should go away. Please forget >>>> about 100kbps, 400kbps, and so on. Bus/clock speed is totally >>>> pointless concept for the I2C bus systems. For example, as long >>>> as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a >>>> certain I2C bus, it doesn't matter that the resulting clock speed >>>> is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode. >>>> Nobody in the I2C bus doesn't care about actual bus/clock speed. >>>> >>>> We don't have to care about the resulting bus speed, or rather >>>> we should/must not check to see if it's within the proper range. >>> >>> Actually, the I2C specification clearly defines f_SCL;max (and thus >>> implies t_SCL;min), both in the tables and the timing diagrams. Why can >>> we ignore this constraint while having to meet all the others? >> >> If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case), >> f_SCL;max will be met by itself. > > I'm not sure if I agree with this: > > Standard mode: > t_r;min 0ns > t_f;min + 0ns > t_HIGH;min + 4000ns > t_LOW;min + 4700ns > 1/f_SCL = 8700ns > ==> f_SCL = 115kHz ==> violation of f_SCL;max=100kHz > > Fast mode (let's assume V_DD = 5.5V): > t_r;min 20ns > t_f;min + 20ns > t_HIGH;min + 600ns > t_LIW;min + 1300ns > 1/f_SCL = 1940ns > ==> f_SCL = 515kHz ==> violation of f_SCL;max=400kHz It's more realistic to calculate with say 50ns < tr,tf < 300ns, than with tt = tf = 0ns or <20ns. Even if with such real tf/tr times, there is cases where f_SCL can be greater than 100/400Hz. I understand what you mean, but that was not my point. See below. >> And again, all I2C master and >> slave devices in the bus don't care about f_SCL; what they do care >> are t_f, t_r, t_HIGH, t_LOW, and so on. That's why I'm saying >> f_SCL is pointless and has no value for HCNT/LCNT calculations. > > I partially agree: If I2C devices don't care about f_SCL but only about > t_r, t_f, t_HIGH and t_LOW there's no need to respect the f_SCL;max > constraint. If this is the case, I'm wondering why it is part of the > specification, though. With t_r;max and t_f;max, Standard mode: t_r;max 1000ns (max time applied) t_f;max + 300ns (max time applied) t_HIGH;min + 4000ns t_LOW;min + 4700ns 1/f_SCL =10000ns ==> f_SCL = 100kHz ==> f_SCL;max for Standard-mode Fast mode: t_r;max 300ns (max time applied) t_f;max + 300ns (max time applied) t_HIGH;min + 600ns t_LIW;min + 1300ns 1/f_SCL = 2500ns ==> f_SCL = 400kHz ==> f_SCL;max for Fast-mode f_SCL;max is defined as a resulting clock frequency with the combination of: (1) the max. conditions of t_r and t_f (2) the min. conditions of t_HIGH and t_LOW We can try to meet t_HIGH;min and t_LOW;min, but we can't meet t_r;min nor t_f;min in the actual systems. The t_r and t_f are _minimum requisites_ for the I/O buffer characteristic of the master and the board designs, hence necessarily contain some time margin. f_SCL is anything more than the resulting speed of (1) + (2), so I don't think we need to adjust HCNT/LCNT values at all. If with t_r < t_r;max and t_f < t_f;max, and you've got a faster clock speed than f_SCL;max, then it's a bonus and we can accept it gratefully. >> I'd make a compromise proposal; it's fine to make sure whether the >> resulting f_SCL is within a supported range, but should not make a >> correction of HCNT/LCNT values. Just report warning messages that >> some parameters/calculations might be mis-configured an/or wrong. > > Not sure if this is a good idea: If f_SCL is met by design I'm perfectly > happy with dropping the t_HIGH/t_LOW adjustment code, no need to bloat > the kernel with confusing warnings. If f_SCL is not automatically met we > must either make sure t_HIGH/t_LOW are adjusted or we must take the > decision to ignore that constraint and document the reasons behind that > decision accordingly. I tried to write my thought down, not sure well-explained, though. Notes: * As long as tHD;SDA issue remains in the DW I2C core, we need to have t_HIGH with a relatively lager value than necessary. In such a case, the resulting f_SCL can never exceed f_SCL;max. * I also wonder which values do you think should be adjusted to meet f_SCL;max, HCNT or LCNT, and why is that? I think it's hard to explain, isn't it? Shinya -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Aug 24, 2013 at 01:58:47PM +0900, Shinya Kuribayashi wrote: > On 8/21/13 11:39 PM, Christian Ruppert wrote: > >On Fri, Aug 16, 2013 at 11:15:12AM +0900, Shinya Kuribayashi wrote: > >>On 8/5/13 6:31 PM, Christian Ruppert wrote: > >>>On Wed, Jul 24, 2013 at 11:31:44PM +0900, Shinya Kuribayashi wrote: > >>>>As said before, all t_SCL things should go away. Please forget > >>>>about 100kbps, 400kbps, and so on. Bus/clock speed is totally > >>>>pointless concept for the I2C bus systems. For example, as long > >>>>as tr/tf, tHIGH/tLOW, tHD;STA, etc. are met by _all_ devices in a > >>>>certain I2C bus, it doesn't matter that the resulting clock speed > >>>>is, say 120 kbps with Standard-mode, or even 800 kbps for Fast-mode. > >>>>Nobody in the I2C bus doesn't care about actual bus/clock speed. > >>>> > >>>>We don't have to care about the resulting bus speed, or rather > >>>>we should/must not check to see if it's within the proper range. > >>> > >>>Actually, the I2C specification clearly defines f_SCL;max (and thus > >>>implies t_SCL;min), both in the tables and the timing diagrams. Why can > >>>we ignore this constraint while having to meet all the others? > >> > >>If we meet t_r, t_f, t_HIGH, t_LOW (and t_HIGH in this DW case), > >>f_SCL;max will be met by itself. > > > >I'm not sure if I agree with this: > > > >Standard mode: > > t_r;min 0ns > > t_f;min + 0ns > > t_HIGH;min + 4000ns > > t_LOW;min + 4700ns > > 1/f_SCL = 8700ns > > ==> f_SCL = 115kHz ==> violation of f_SCL;max=100kHz > > > >Fast mode (let's assume V_DD = 5.5V): > > t_r;min 20ns > > t_f;min + 20ns > > t_HIGH;min + 600ns > > t_LIW;min + 1300ns > > 1/f_SCL = 1940ns > > ==> f_SCL = 515kHz ==> violation of f_SCL;max=400kHz > > It's more realistic to calculate with say 50ns < tr,tf < 300ns, > than with tt = tf = 0ns or <20ns. Even if with such real tf/tr > times, there is cases where f_SCL can be greater than 100/400Hz. I agree. I just used these values to illustrate my point. > I understand what you mean, but that was not my point. See below. > > >>And again, all I2C master and > >>slave devices in the bus don't care about f_SCL; what they do care > >>are t_f, t_r, t_HIGH, t_LOW, and so on. That's why I'm saying > >>f_SCL is pointless and has no value for HCNT/LCNT calculations. > > > >I partially agree: If I2C devices don't care about f_SCL but only about > >t_r, t_f, t_HIGH and t_LOW there's no need to respect the f_SCL;max > >constraint. If this is the case, I'm wondering why it is part of the > >specification, though. > > With t_r;max and t_f;max, > > Standard mode: > t_r;max 1000ns (max time applied) > t_f;max + 300ns (max time applied) > t_HIGH;min + 4000ns > t_LOW;min + 4700ns > 1/f_SCL =10000ns > ==> f_SCL = 100kHz ==> f_SCL;max for Standard-mode > > Fast mode: > t_r;max 300ns (max time applied) > t_f;max + 300ns (max time applied) > t_HIGH;min + 600ns > t_LIW;min + 1300ns > 1/f_SCL = 2500ns > ==> f_SCL = 400kHz ==> f_SCL;max for Fast-mode > > f_SCL;max is defined as a resulting clock frequency with the > combination of: > > (1) the max. conditions of t_r and t_f > (2) the min. conditions of t_HIGH and t_LOW > > We can try to meet t_HIGH;min and t_LOW;min, but we can't meet > t_r;min nor t_f;min in the actual systems. The t_r and t_f are > _minimum requisites_ for the I/O buffer characteristic of the > master and the board designs, hence necessarily contain some > time margin. I agree. > f_SCL is anything more than the resulting speed of (1) + (2), > so I don't think we need to adjust HCNT/LCNT values at all. > If with t_r < t_r;max and t_f < t_f;max, and you've got a faster > clock speed than f_SCL;max, then it's a bonus and we can accept > it gratefully. Here I'm not sure: The I2C specification clearly states f_SCL min/max requirements in table 10. I'm feeling a bit uneasy patching a driver with (to date) rather pessimistic bus timings to something which might be over-optimistic. If I submit a patch for this I would like to be sure not to break existing systems. How do other drivers manage this? > >>I'd make a compromise proposal; it's fine to make sure whether the > >>resulting f_SCL is within a supported range, but should not make a > >>correction of HCNT/LCNT values. Just report warning messages that > >>some parameters/calculations might be mis-configured an/or wrong. > > > >Not sure if this is a good idea: If f_SCL is met by design I'm perfectly > >happy with dropping the t_HIGH/t_LOW adjustment code, no need to bloat > >the kernel with confusing warnings. If f_SCL is not automatically met we > >must either make sure t_HIGH/t_LOW are adjusted or we must take the > >decision to ignore that constraint and document the reasons behind that > >decision accordingly. > > I tried to write my thought down, not sure well-explained, though. > > Notes: > > * As long as tHD;SDA issue remains in the DW I2C core, we need to > have t_HIGH with a relatively lager value than necessary. In > such a case, the resulting f_SCL can never exceed f_SCL;max. Not in my calculations (Let's assume fast mode with all rise and fall times being 50ns as you say above and f_sys = 50MHz): HCNT = f_SYS * (t_HD;STA + t_f;SDA) - 3 -> t_HIGH = t_HD_STA + t_f;SDA + 4 / f_sys = 0.6us + 0.05 us + 0.08 us = 0.73us LCNT = f_SYS * (t_f;SCL + t_LOW) -> t_LOW = 0.05us + 1.3us = 1.35us -> f_SCL = 1 / (t_HIGH + t_LOW + t_r;SCL + t_f;SCL) = 1 / (0.73us + 1.35 us + 0.05 us + 0.05us) = 458MHz > 400MHz > * I also wonder which values do you think should be adjusted to meet > f_SCL;max, HCNT or LCNT, and why is that? I think it's hard to > explain, isn't it? Originally I thought it would be best to adjust both in the same way. Thinking about your question I guess it might be more in the spirit of I2C to only adjust LCNT. The specification (table 10) allows for both, there are no max values specified for either. Greetings, Christian
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index ad46616..f31469e 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -308,29 +308,39 @@ int i2c_dw_init(struct dw_i2c_dev *dev) /* set standard and fast speed deviders for high/low periods */ /* Standard-mode */ - hcnt = i2c_dw_scl_hcnt(input_clock_khz, - 40, /* tHD;STA = tHIGH = 4.0 us */ - 3, /* tf = 0.3 us */ - 0, /* 0: DW default, 1: Ideal */ - 0); /* No offset */ - lcnt = i2c_dw_scl_lcnt(input_clock_khz, - 47, /* tLOW = 4.7 us */ - 3, /* tf = 0.3 us */ - 0); /* No offset */ + if (dev->ss_hcnt && dev->ss_lcnt) { + hcnt = dev->ss_hcnt; + lcnt = dev->ss_lcnt; + } else { + hcnt = i2c_dw_scl_hcnt(input_clock_khz, + 40, /* tHD;STA = tHIGH = 4.0 us */ + 3, /* tf = 0.3 us */ + 0, /* 0: DW default, 1: Ideal */ + 0); /* No offset */ + lcnt = i2c_dw_scl_lcnt(input_clock_khz, + 47, /* tLOW = 4.7 us */ + 3, /* tf = 0.3 us */ + 0); /* No offset */ + } dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT); dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT); dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt); /* Fast-mode */ - hcnt = i2c_dw_scl_hcnt(input_clock_khz, - 6, /* tHD;STA = tHIGH = 0.6 us */ - 3, /* tf = 0.3 us */ - 0, /* 0: DW default, 1: Ideal */ - 0); /* No offset */ - lcnt = i2c_dw_scl_lcnt(input_clock_khz, - 13, /* tLOW = 1.3 us */ - 3, /* tf = 0.3 us */ - 0); /* No offset */ + if (dev->fs_hcnt && dev->fs_lcnt) { + hcnt = dev->fs_hcnt; + lcnt = dev->fs_lcnt; + } else { + hcnt = i2c_dw_scl_hcnt(input_clock_khz, + 6, /* tHD;STA = tHIGH = 0.6 us */ + 3, /* tf = 0.3 us */ + 0, /* 0: DW default, 1: Ideal */ + 0); /* No offset */ + lcnt = i2c_dw_scl_lcnt(input_clock_khz, + 13, /* tLOW = 1.3 us */ + 3, /* tf = 0.3 us */ + 0); /* No offset */ + } dw_writel(dev, hcnt, DW_IC_FS_SCL_HCNT); dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT); dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt); diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 912aa22..e8a7565 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -61,6 +61,14 @@ * @tx_fifo_depth: depth of the hardware tx fifo * @rx_fifo_depth: depth of the hardware rx fifo * @rx_outstanding: current master-rx elements in tx fifo + * @ss_hcnt: standard speed HCNT value + * @ss_lcnt: standard speed LCNT value + * @fs_hcnt: fast speed HCNT value + * @fs_lcnt: fast speed LCNT value + * + * HCNT and LCNT parameters can be used if the platform knows more accurate + * values than the one computed based only on the input clock frequency. + * Leave them to be %0 if not used. */ struct dw_i2c_dev { struct device *dev; @@ -91,6 +99,10 @@ struct dw_i2c_dev { unsigned int rx_fifo_depth; int rx_outstanding; u32 sda_hold_time; + u16 ss_hcnt; + u16 ss_lcnt; + u16 fs_hcnt; + u16 fs_lcnt; }; #define ACCESS_SWAP 0x00000001
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. 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. With this patch we let the platform code to specify more accurate, optimal *CNT values if they are known beforehand. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/i2c/busses/i2c-designware-core.c | 46 +++++++++++++++++++------------- drivers/i2c/busses/i2c-designware-core.h | 12 +++++++++ 2 files changed, 40 insertions(+), 18 deletions(-)