All of lore.kernel.org
 help / color / mirror / Atom feed
From: Addy Ke <addy.ke@rock-chips.com>
To: wsa@the-dreams.de, max.schwarz@online.de, heiko@sntech.de,
	olof@lixom.net, dianders@chromium.org
Cc: linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, cf@rock-chips.com,
	xjq@rock-chips.com, huangtao@rock-chips.com, zyw@rock-chips.com,
	yzq@rock-chips.com, hj@rock-chips.com, kever.yang@rock-chips.com,
	hl@rock-chips.com, caesar.wang@rock-chips.com,
	zhengsq@rock-chips.com, Addy Ke <addy.ke@rock-chips.com>
Subject: [PATCH] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec
Date: Thu,  6 Nov 2014 16:11:54 +0800	[thread overview]
Message-ID: <1415261514-4051-1-git-send-email-addy.ke@rock-chips.com> (raw)

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

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 */
-- 
1.8.3.2



WARNING: multiple messages have this Message-ID (diff)
From: Addy Ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
To: wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org,
	max.schwarz-BGeptl67XyCzQB+pC5nmwQ@public.gmane.org,
	heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org,
	olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org,
	dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	cf-TNX95d0MmH7DzftRWevZcw@public.gmane.org,
	xjq-TNX95d0MmH7DzftRWevZcw@public.gmane.org,
	huangtao-TNX95d0MmH7DzftRWevZcw@public.gmane.org,
	zyw-TNX95d0MmH7DzftRWevZcw@public.gmane.org,
	yzq-TNX95d0MmH7DzftRWevZcw@public.gmane.org,
	hj-TNX95d0MmH7DzftRWevZcw@public.gmane.org,
	kever.yang-TNX95d0MmH7DzftRWevZcw@public.gmane.org,
	hl-TNX95d0MmH7DzftRWevZcw@public.gmane.org,
	caesar.wang-TNX95d0MmH7DzftRWevZcw@public.gmane.org,
	zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org,
	Addy Ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Subject: [PATCH] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec
Date: Thu,  6 Nov 2014 16:11:54 +0800	[thread overview]
Message-ID: <1415261514-4051-1-git-send-email-addy.ke@rock-chips.com> (raw)

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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
 drivers/i2c/busses/i2c-rk3x.c | 58 +++++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 21 deletions(-)

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 */
-- 
1.8.3.2

WARNING: multiple messages have this Message-ID (diff)
From: addy.ke@rock-chips.com (Addy Ke)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec
Date: Thu,  6 Nov 2014 16:11:54 +0800	[thread overview]
Message-ID: <1415261514-4051-1-git-send-email-addy.ke@rock-chips.com> (raw)

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

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 */
-- 
1.8.3.2

             reply	other threads:[~2014-11-06  8:12 UTC|newest]

Thread overview: 63+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-06  8:11 Addy Ke [this message]
2014-11-06  8:11 ` [PATCH] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec Addy Ke
2014-11-06  8:11 ` Addy Ke
2014-12-02 23:02 ` Doug Anderson
2014-12-02 23:02   ` Doug Anderson
2014-12-03  2:37 ` [PATCH v2] " Addy Ke
2014-12-03  2:37   ` Addy Ke
2014-12-03  2:37   ` Addy Ke
2014-12-03  5:13   ` Doug Anderson
2014-12-03  5:13     ` Doug Anderson
2014-12-03  5:13     ` Doug Anderson
2014-12-03 11:15   ` Wolfram Sang
2014-12-03 11:15     ` Wolfram Sang
2014-12-03 11:15     ` Wolfram Sang
2014-12-03 17:53     ` Doug Anderson
2014-12-03 17:53       ` Doug Anderson
2014-12-03 17:53       ` Doug Anderson
2014-12-04 18:40       ` Wolfram Sang
2014-12-04 18:40         ` Wolfram Sang
2014-12-04 18:40         ` Wolfram Sang
2014-12-04 18:43         ` Doug Anderson
2014-12-04 18:43           ` Doug Anderson
2014-12-04 18:43           ` Doug Anderson
2014-12-04 19:03           ` Wolfram Sang
2014-12-04 19:03             ` Wolfram Sang
2014-12-04 19:03             ` Wolfram Sang
2014-12-05 19:31             ` Doug Anderson
2014-12-05 19:31               ` Doug Anderson
2014-12-05 19:31               ` Doug Anderson
2014-12-08  2:59 ` [PATCH v3] " Addy Ke
2014-12-08  2:59   ` Addy Ke
2014-12-08  2:59   ` Addy Ke
2014-12-08  3:06   ` addy ke
2014-12-08  3:06     ` addy ke
2014-12-08  3:06     ` addy ke
2014-12-08  8:52   ` Uwe Kleine-König
2014-12-08  8:52     ` Uwe Kleine-König
2014-12-08 17:13     ` Doug Anderson
2014-12-08 17:13       ` Doug Anderson
2014-12-08 17:13       ` Doug Anderson
2014-12-08 17:34       ` Wolfram Sang
2014-12-08 17:34         ` Wolfram Sang
2014-12-08 17:34         ` Wolfram Sang
2014-12-08 18:53         ` Doug Anderson
2014-12-08 18:53           ` Doug Anderson
2014-12-08 18:53           ` Doug Anderson
2014-12-08 20:04           ` Uwe Kleine-König
2014-12-08 20:04             ` Uwe Kleine-König
2014-12-08 20:04             ` Uwe Kleine-König
2014-12-11  6:00   ` [PATCH v4] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C specification Addy Ke
2014-12-11  6:00     ` Addy Ke
2014-12-11  6:00     ` Addy Ke
2014-12-11  7:47     ` Uwe Kleine-König
2014-12-11  7:47       ` Uwe Kleine-König
2014-12-11  7:47       ` Uwe Kleine-König
2014-12-11 11:02     ` [PATCH v5] " Addy Ke
2014-12-11 11:02       ` Addy Ke
2014-12-11 19:22       ` Doug Anderson
2014-12-11 19:22         ` Doug Anderson
2014-12-11 19:22         ` Doug Anderson
2015-01-13 11:42       ` Wolfram Sang
2015-01-13 11:42         ` Wolfram Sang
2015-01-13 11:42         ` Wolfram Sang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1415261514-4051-1-git-send-email-addy.ke@rock-chips.com \
    --to=addy.ke@rock-chips.com \
    --cc=caesar.wang@rock-chips.com \
    --cc=cf@rock-chips.com \
    --cc=dianders@chromium.org \
    --cc=heiko@sntech.de \
    --cc=hj@rock-chips.com \
    --cc=hl@rock-chips.com \
    --cc=huangtao@rock-chips.com \
    --cc=kever.yang@rock-chips.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=max.schwarz@online.de \
    --cc=olof@lixom.net \
    --cc=wsa@the-dreams.de \
    --cc=xjq@rock-chips.com \
    --cc=yzq@rock-chips.com \
    --cc=zhengsq@rock-chips.com \
    --cc=zyw@rock-chips.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.