diff mbox

i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

Message ID 1415261514-4051-1-git-send-email-addy.ke@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

addy ke Nov. 6, 2014, 8:11 a.m. UTC
high_ns calculated from the low division of CLKDIV register is the sum of
actual measured high_ns and rise_ns. The rise time which related to
external pull-up resistor can be up to the maximum rise time in I2C spec.

In my test, if external pull-up resistor is 4.7K, rise_ns is about 700ns.
So the actual measured high_ns is about 3900ns, which is less than 4000ns
(the minimum high_ns in I2C spec).

Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
---
 drivers/i2c/busses/i2c-rk3x.c | 58 +++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 21 deletions(-)

Comments

Doug Anderson Dec. 2, 2014, 11:02 p.m. UTC | #1
Addy,

On Thu, Nov 6, 2014 at 12:11 AM, Addy Ke <addy.ke@rock-chips.com> wrote:
> high_ns calculated from the low division of CLKDIV register is the sum of
> actual measured high_ns and rise_ns. The rise time which related to
> external pull-up resistor can be up to the maximum rise time in I2C spec.
>
> In my test, if external pull-up resistor is 4.7K, rise_ns is about 700ns.
> So the actual measured high_ns is about 3900ns, which is less than 4000ns
> (the minimum high_ns in I2C spec).

It's a little unfortunate to have to make the assumption that the rise
time for a board is going to be the maximum the spec allows.  I think
this is something that's better to let a board specify in the device
tree.  Allowing us to specify it also gives us a little extra slop and
makes it easier to specify a "fall time" without affecting the
resulting frequency.  Right now your code assumes that the fall time
is 0.

I've prototyped what things could look like if the rise and fall times
were specified in the device tree.  You can see my patch (atop yours)
at:

https://chromium-review.googlesource.com/#/c/232774/

...perhaps you could squash that into your patch and post up v2?

-Doug
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index e276ffb..8e1cc2b 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -432,9 +432,12 @@  out:
 static int rk3x_i2c_calc_divs(unsigned long i2c_rate, unsigned long scl_rate,
 			       unsigned long *div_low, unsigned long *div_high)
 {
+	unsigned long spec_min_low_ns, spec_min_high_ns;
+	unsigned long spec_max_data_hold_ns;
+	unsigned long spec_data_hold_buffer_ns;
+	unsigned long spec_max_rise_ns;
+
 	unsigned long min_low_ns, min_high_ns;
-	unsigned long max_data_hold_ns;
-	unsigned long data_hold_buffer_ns;
 	unsigned long max_low_ns, min_total_ns;
 
 	unsigned long i2c_rate_khz, scl_rate_khz;
@@ -453,30 +456,43 @@  static int rk3x_i2c_calc_divs(unsigned long i2c_rate, unsigned long scl_rate,
 	if (WARN_ON(scl_rate < 1000))
 		scl_rate = 1000;
 
+	if (scl_rate <= 100000) {
+		spec_min_low_ns = 4700;
+		spec_min_high_ns = 4000;
+		spec_max_rise_ns = 1000;
+		spec_max_data_hold_ns = 3450;
+		spec_data_hold_buffer_ns = 50;
+	} else {
+		spec_min_low_ns = 1300;
+		spec_min_high_ns = 600;
+		spec_max_rise_ns = 300;
+		spec_max_data_hold_ns = 900;
+		spec_data_hold_buffer_ns = 50;
+	}
+
 	/*
-	 * min_low_ns:  The minimum number of ns we need to hold low
-	 *		to meet i2c spec
-	 * min_high_ns: The minimum number of ns we need to hold high
-	 *		to meet i2c spec
-	 * max_low_ns:  The maximum number of ns we can hold low
-	 *		to meet i2c spec
+	 * min_low_ns:  The minimum number of ns we need to hold low.
+	 *		The fall time in RK3X's I2C controller is approximately
+	 *		equal to 0. So min_low_ns = spec_min_low_ns.
+	 * Note: low_ns should be (measured_low_ns + measured_fall_time)
+	 *	 and measured_low_ns must meet I2C spec.
 	 *
-	 * Note: max_low_ns should be (max data hold time * 2 - buffer)
+	 * min_high_ns: The minimum number of ns we need to hold high.
+	 *		The rise time which related to external pull-up resistor
+	 *		can be up to spec_max_rise_ns.
+	 *		So min_high_ns = spec_min_high_ns + spec_max_rise_ns
+	 * Note: high_ns should be (measured_high_ns + measured_rise_time)
+	 *	 and measured_high_ns must meet I2C spec.
+	 *
+	 * max_low_ns:  The maximum number of ns we can hold low.
+	 * Note: max_low_ns should be (max_data_hold_time * 2 - buffer)
 	 *	 This is because the i2c host on Rockchip holds the data line
 	 *	 for half the low time.
 	 */
-	if (scl_rate <= 100000) {
-		min_low_ns = 4700;
-		min_high_ns = 4000;
-		max_data_hold_ns = 3450;
-		data_hold_buffer_ns = 50;
-	} else {
-		min_low_ns = 1300;
-		min_high_ns = 600;
-		max_data_hold_ns = 900;
-		data_hold_buffer_ns = 50;
-	}
-	max_low_ns = max_data_hold_ns * 2 - data_hold_buffer_ns;
+	min_low_ns = spec_min_low_ns;
+	min_high_ns = spec_min_high_ns + spec_max_rise_ns;
+	max_low_ns = spec_max_data_hold_ns * 2 - spec_data_hold_buffer_ns;
+
 	min_total_ns = min_low_ns + min_high_ns;
 
 	/* Adjust to avoid overflow */