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

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

[Addy Ke]

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

Re: [PATCH] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

[Doug Anderson]

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

[PATCH v2] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

[Addy Ke]

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

To fix this bug, min_low_ns should include fall time and min_high_ns
should include rise time too.

This patch merged the patch that Doug submitted to chromium, which
can get the rise and fall times for signals from the device tree.
This allows us to more accurately calculate timings. see:
https://chromium-review.googlesource.com/#/c/232774/

Signed-off-by: Addy Ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
Changes in v2:
- merged the patch that Doug submitted to chromium

 Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 10 ++++
 drivers/i2c/busses/i2c-rk3x.c                      | 55 +++++++++++++++-------
 2 files changed, 47 insertions(+), 18 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
index dde6c22..faf5997 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
@@ -21,6 +21,13 @@ Required on RK3066, RK3188 :
 Optional properties :
 
  - clock-frequency : SCL frequency to use (in Hz). If omitted, 100kHz is used.
+ - rise-ns : Number of nanoseconds the signal takes to rise (t(r) in i2c spec).
+	     If not specified this is assumed to be the max the spec allows
+	     (1000 ns for standard mode, 300 ns for fast mode) which might
+	     cause slightly slower communication.
+ - fall-ns : Number of nanoseconds the signal takes to fall (t(f) in the i2c0
+	     spec).  If not specified this is assumed to be the max the spec
+	     allows (300 ns) which might cause slightly slower communication.
 
 Example:
 
@@ -39,4 +46,7 @@ i2c0: i2c@2002d000 {
 
 	clock-names = "i2c";
 	clocks = <&cru PCLK_I2C0>;
+
+	rise-ns = <800>;
+	fall-ns = <100>;
 };
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 0ee5802..025d4b5 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -102,6 +102,8 @@ struct rk3x_i2c {
 
 	/* Settings */
 	unsigned int scl_frequency;
+	unsigned int rise_ns;
+	unsigned int fall_ns;
 
 	/* Synchronization & notification */
 	spinlock_t lock;
@@ -435,6 +437,8 @@ out:
  *
  * @clk_rate: I2C input clock rate
  * @scl_rate: Desired SCL rate
+ * @rise_ns: How many ns it takes for signals to rise.
+ * @fall_ns: How many ns it takes for signals to fall.
  * @div_low: Divider output for low
  * @div_high: Divider output for high
  *
@@ -443,11 +447,14 @@ out:
  * too high, we silently use the highest possible rate.
  */
 static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
+			      unsigned long rise_ns, unsigned long fall_ns,
 			      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 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 clk_rate_khz, scl_rate_khz;
@@ -470,28 +477,30 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
 
 	/*
 	 * min_low_ns:  The minimum number of ns we need to hold low
-	 *		to meet i2c spec
+	 *		to meet i2c spec, should include fall time.
 	 * min_high_ns: The minimum number of ns we need to hold high
-	 *		to meet i2c spec
+	 *		to meet i2c spec, should include rise time.
 	 * max_low_ns:  The maximum number of ns we can hold low
-	 *		to meet i2c spec
+	 *		to meet i2c spec.
 	 *
 	 * 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;
+		spec_min_low_ns = 4700;
+		spec_min_high_ns = 4000;
+		spec_max_data_hold_ns = 3450;
+		spec_data_hold_buffer_ns = 50;
 	} else {
-		min_low_ns = 1300;
-		min_high_ns = 600;
-		max_data_hold_ns = 900;
-		data_hold_buffer_ns = 50;
+		spec_min_low_ns = 1300;
+		spec_min_high_ns = 600;
+		spec_max_data_hold_ns = 900;
+		spec_data_hold_buffer_ns = 50;
 	}
-	max_low_ns = max_data_hold_ns * 2 - data_hold_buffer_ns;
+	min_low_ns = spec_min_low_ns + fall_ns;
+	min_high_ns = spec_min_high_ns + 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 */
@@ -588,8 +597,8 @@ static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
 	u64 t_low_ns, t_high_ns;
 	int ret;
 
-	ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, &div_low,
-				 &div_high);
+	ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, i2c->rise_ns,
+				 i2c->fall_ns, &div_low, &div_high);
 
 	WARN_ONCE(ret != 0, "Could not reach SCL freq %u", i2c->scl_frequency);
 
@@ -633,9 +642,9 @@ static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long
 	switch (event) {
 	case PRE_RATE_CHANGE:
 		if (rk3x_i2c_calc_divs(ndata->new_rate, i2c->scl_frequency,
-				      &div_low, &div_high) != 0) {
+				       i2c->rise_ns, i2c->fall_ns, &div_low,
+				       &div_high) != 0)
 			return NOTIFY_STOP;
-		}
 
 		/* scale up */
 		if (ndata->new_rate > ndata->old_rate)
@@ -859,6 +868,16 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
 		i2c->scl_frequency = DEFAULT_SCL_RATE;
 	}
 
+	/* Read rise and fall ns; if not there use the default max from spec */
+	if (of_property_read_u32(pdev->dev.of_node, "rise-ns", &i2c->rise_ns)) {
+		if (i2c->scl_frequency <= 100000)
+			i2c->rise_ns = 1000;
+		else
+			i2c->rise_ns = 300;
+	}
+	if (of_property_read_u32(pdev->dev.of_node, "fall-ns", &i2c->fall_ns))
+		i2c->fall_ns = 300;
+
 	strlcpy(i2c->adap.name, "rk3x-i2c", sizeof(i2c->adap.name));
 	i2c->adap.owner = THIS_MODULE;
 	i2c->adap.algo = &rk3x_i2c_algorithm;
-- 
1.8.3.2

Re: [PATCH v2] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

[Doug Anderson]

Addy,

On Tue, Dec 2, 2014 at 6:37 PM, Addy Ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org> 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).
>
> To fix this bug, min_low_ns should include fall time and min_high_ns
> should include rise time too.
>
> This patch merged the patch that Doug submitted to chromium, which
> can get the rise and fall times for signals from the device tree.
> This allows us to more accurately calculate timings. see:
> https://chromium-review.googlesource.com/#/c/232774/
>
> Signed-off-by: Addy Ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> Changes in v2:
> - merged the patch that Doug submitted to chromium
>
>  Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 10 ++++
>  drivers/i2c/busses/i2c-rk3x.c                      | 55 +++++++++++++++-------
>  2 files changed, 47 insertions(+), 18 deletions(-)

This looks good to me.  Thank you for spinning.

Reviewed-by: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Re: [PATCH v2] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 922 bytes --]


> + - rise-ns : Number of nanoseconds the signal takes to rise (t(r) in i2c spec).
> +	     If not specified this is assumed to be the max the spec allows
> +	     (1000 ns for standard mode, 300 ns for fast mode) which might
> +	     cause slightly slower communication.
> + - fall-ns : Number of nanoseconds the signal takes to fall (t(f) in the i2c0
> +	     spec).  If not specified this is assumed to be the max the spec
> +	     allows (300 ns) which might cause slightly slower communication.

We already have those bindings from the designware driver:

 - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds.
 - i2c-scl-falling-time : should contain the SCL falling time in nanoseconds.
 - i2c-sda-falling-time : should contain the SDA falling time in nanoseconds.

Can you reuse them? Or do you really need a specific rise-time property?
If so, please matche the style of the bindings above.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

[Doug Anderson]

Wolfram,

On Wed, Dec 3, 2014 at 3:15 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>
>> + - rise-ns : Number of nanoseconds the signal takes to rise (t(r) in i2c spec).
>> +          If not specified this is assumed to be the max the spec allows
>> +          (1000 ns for standard mode, 300 ns for fast mode) which might
>> +          cause slightly slower communication.
>> + - fall-ns : Number of nanoseconds the signal takes to fall (t(f) in the i2c0
>> +          spec).  If not specified this is assumed to be the max the spec
>> +          allows (300 ns) which might cause slightly slower communication.
>
> We already have those bindings from the designware driver:
>
>  - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds.
>  - i2c-scl-falling-time : should contain the SCL falling time in nanoseconds.
>  - i2c-sda-falling-time : should contain the SDA falling time in nanoseconds.
>
> Can you reuse them? Or do you really need a specific rise-time property?
> If so, please matche the style of the bindings above.

Ah, doh!  I should have thought to look for existing bindings.  Sorry
about that.  :(

If you don't read all the below, my belief is that we should simply
rename the strings in Addy's patch.  We should change "rise-ns" to
"i2c-scl-rising-time" and "fall-ns" to "i2c-scl-falling-time".
Wolfram: can you confirm this is OK?  I'm voting to leave the "-ns"
off the end of both to avoid asymmetry.

---

Details:

Hrm, we seem to need different parameters than designware i2c.  The
designware bus is specifying "i2c-sda-hold-time-ns".  On Rockchip i2c
we specify just the number of cycles that the clk line should be high
and the number of cycles that it should be low.  The adapter does the
rest of the work.  As I understand it, the data hold time on Rockchip
i2c is equal to half the low time, for instance.  That was indicated
by Addy who talked to the IC engineer.

Because of the above, I _think_ that means that specifying
"i2c-sda-hold-time-ns" is not appropriate for us, or at least not easy
to convert in a sane way.

We could add a "i2c-scl-hold-time-ns", but if I understand correctly I
think that the "rise-time" describes the hardware in a cleaner way.
If you specify the hold time then (I think) that it requires the user
to tweak it whenever he/she adjusts the bus speed.  In other words if
you have a bus and you decide to move it from running at 400kHz to
running at 300kHz (signal integrity issues?) or 100kHz, you need to
manually modify the hold time.  ...on the other hand the rise time is
a property of the hardware I think (size of resistor, etc).

For the falling times I guess we should use the "i2c-scl-falling-time"
and not list the "i2c-sda-falling-time" for now?  As per above the
controller takes in the high/low period of the clocks and figures out
the rest itself.  Possibly we will need to account for
i2c-sda-falling-time eventually, but maybe that can come later?


-Doug

Re: [PATCH v2] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]


> If you don't read all the below, my belief is that we should simply
> rename the strings in Addy's patch.  We should change "rise-ns" to
> "i2c-scl-rising-time" and "fall-ns" to "i2c-scl-falling-time".
> Wolfram: can you confirm this is OK?  I'm voting to leave the "-ns"
> off the end of both to avoid asymmetry.

New binding should have the "-ns" suffix, right? So, I'd vote to add the
suffix to the new bindings and deprecate the ones used in the designware
driver: "i2c-scl-rising-time-ns" and "i2c-scl-falling-time-ns"

It might be a little more work now, but it will help us in the future,
because it is the correct way to do it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

[Doug Anderson]

Wolfram,

On Thu, Dec 4, 2014 at 10:40 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>
>> If you don't read all the below, my belief is that we should simply
>> rename the strings in Addy's patch.  We should change "rise-ns" to
>> "i2c-scl-rising-time" and "fall-ns" to "i2c-scl-falling-time".
>> Wolfram: can you confirm this is OK?  I'm voting to leave the "-ns"
>> off the end of both to avoid asymmetry.
>
> New binding should have the "-ns" suffix, right? So, I'd vote to add the
> suffix to the new bindings and deprecate the ones used in the designware
> driver: "i2c-scl-rising-time-ns" and "i2c-scl-falling-time-ns"
>
> It might be a little more work now, but it will help us in the future,
> because it is the correct way to do it.

OK, that sounds fair.  You're OK with my proposal otherwise?

Are you looking for Addy or me to submit a patch supporting the "-ns"
suffix on the Designware i2c driver, or should we just use the "-ns"
suffix in the Rockchip driver and assume someone will later add
support for the new binding in the Designware i2c driver?

-Doug

Re: [PATCH v2] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]


> > New binding should have the "-ns" suffix, right? So, I'd vote to add the
> > suffix to the new bindings and deprecate the ones used in the designware
> > driver: "i2c-scl-rising-time-ns" and "i2c-scl-falling-time-ns"
> >
> > It might be a little more work now, but it will help us in the future,
> > because it is the correct way to do it.
> 
> OK, that sounds fair.  You're OK with my proposal otherwise?

Yes.

> Are you looking for Addy or me to submit a patch supporting the "-ns"
> suffix on the Designware i2c driver, or should we just use the "-ns"
> suffix in the Rockchip driver and assume someone will later add
> support for the new binding in the Designware i2c driver?

I'd be really happy if you can do it. I'd love to have consistency right
away.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

[Doug Anderson]

Hi,

On Thu, Dec 4, 2014 at 11:03 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> > New binding should have the "-ns" suffix, right? So, I'd vote to add the
>> > suffix to the new bindings and deprecate the ones used in the designware
>> > driver: "i2c-scl-rising-time-ns" and "i2c-scl-falling-time-ns"
>> >
>> > It might be a little more work now, but it will help us in the future,
>> > because it is the correct way to do it.
>>
>> OK, that sounds fair.  You're OK with my proposal otherwise?
>
> Yes.

OK, for simplicity I put my requested changes in
<https://chromium-review.googlesource.com/#/c/232774/3>


>> Are you looking for Addy or me to submit a patch supporting the "-ns"
>> suffix on the Designware i2c driver, or should we just use the "-ns"
>> suffix in the Rockchip driver and assume someone will later add
>> support for the new binding in the Designware i2c driver?
>
> I'd be really happy if you can do it. I'd love to have consistency right
> away.

Well that was easy.  https://patchwork.kernel.org/patch/5445231/

-Doug

[PATCH v3] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

[Addy Ke]

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

To fix this bug, min_low_ns should include fall time and min_high_ns
should include rise time too.

This patch merged the patch that Doug submitted to chromium, which
can get the rise and fall times for signals from the device tree.
This allows us to more accurately calculate timings. see:
https://chromium-review.googlesource.com/#/c/232774/

Signed-off-by: Addy Ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
Changes in v2:
- merged the patch that Doug submitted to chromium
Changes in v3:
- merged the patch that Doug submitted to chromium to change bindins
  see: https://chromium-review.googlesource.com/#/c/232775/

 Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 11 +++++
 drivers/i2c/busses/i2c-rk3x.c                      | 57 +++++++++++++++-------
 2 files changed, 50 insertions(+), 18 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
index dde6c22..925d6eb 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
@@ -21,6 +21,14 @@ Required on RK3066, RK3188 :
 Optional properties :
 
  - clock-frequency : SCL frequency to use (in Hz). If omitted, 100kHz is used.
+ - i2c-scl-rising-time-ns : Number of nanoseconds the signal takes to rise
+	(t(r) in i2c spec). If not specified this is assumed to be the max the
+	spec allows(1000 ns for standard mode, 300 ns for fast mode) which
+	might cause slightly slower communication.
+ - i2c-scl-falling-time-ns : Number of nanoseconds the signal takes to fall
+	(t(f) in the i2c0 spec).  If not specified this is assumed to be the
+	max the spec allows (300 ns) which might cause slightly slower
+	communication.
 
 Example:
 
@@ -39,4 +47,7 @@ i2c0: i2c@2002d000 {
 
 	clock-names = "i2c";
 	clocks = <&cru PCLK_I2C0>;
+
+	i2c-scl-rising-time-ns = <800>;
+	i2c-scl-falling-time-ns = <100>;
 };
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 0ee5802..50c1678 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -102,6 +102,8 @@ struct rk3x_i2c {
 
 	/* Settings */
 	unsigned int scl_frequency;
+	unsigned int rise_ns;
+	unsigned int fall_ns;
 
 	/* Synchronization & notification */
 	spinlock_t lock;
@@ -435,6 +437,8 @@ out:
  *
  * @clk_rate: I2C input clock rate
  * @scl_rate: Desired SCL rate
+ * @rise_ns: How many ns it takes for signals to rise.
+ * @fall_ns: How many ns it takes for signals to fall.
  * @div_low: Divider output for low
  * @div_high: Divider output for high
  *
@@ -443,11 +447,14 @@ out:
  * too high, we silently use the highest possible rate.
  */
 static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
+			      unsigned long rise_ns, unsigned long fall_ns,
 			      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 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 clk_rate_khz, scl_rate_khz;
@@ -470,28 +477,30 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
 
 	/*
 	 * min_low_ns:  The minimum number of ns we need to hold low
-	 *		to meet i2c spec
+	 *		to meet i2c spec, should include fall time.
 	 * min_high_ns: The minimum number of ns we need to hold high
-	 *		to meet i2c spec
+	 *		to meet i2c spec, should include rise time.
 	 * max_low_ns:  The maximum number of ns we can hold low
-	 *		to meet i2c spec
+	 *		to meet i2c spec.
 	 *
 	 * 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;
+		spec_min_low_ns = 4700;
+		spec_min_high_ns = 4000;
+		spec_max_data_hold_ns = 3450;
+		spec_data_hold_buffer_ns = 50;
 	} else {
-		min_low_ns = 1300;
-		min_high_ns = 600;
-		max_data_hold_ns = 900;
-		data_hold_buffer_ns = 50;
+		spec_min_low_ns = 1300;
+		spec_min_high_ns = 600;
+		spec_max_data_hold_ns = 900;
+		spec_data_hold_buffer_ns = 50;
 	}
-	max_low_ns = max_data_hold_ns * 2 - data_hold_buffer_ns;
+	min_low_ns = spec_min_low_ns + fall_ns;
+	min_high_ns = spec_min_high_ns + 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 */
@@ -588,8 +597,8 @@ static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
 	u64 t_low_ns, t_high_ns;
 	int ret;
 
-	ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, &div_low,
-				 &div_high);
+	ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, i2c->rise_ns,
+				 i2c->fall_ns, &div_low, &div_high);
 
 	WARN_ONCE(ret != 0, "Could not reach SCL freq %u", i2c->scl_frequency);
 
@@ -633,9 +642,9 @@ static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long
 	switch (event) {
 	case PRE_RATE_CHANGE:
 		if (rk3x_i2c_calc_divs(ndata->new_rate, i2c->scl_frequency,
-				      &div_low, &div_high) != 0) {
+				       i2c->rise_ns, i2c->fall_ns, &div_low,
+				       &div_high) != 0)
 			return NOTIFY_STOP;
-		}
 
 		/* scale up */
 		if (ndata->new_rate > ndata->old_rate)
@@ -859,6 +868,18 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
 		i2c->scl_frequency = DEFAULT_SCL_RATE;
 	}
 
+	/* Read rise and fall ns; if not there use the default max from spec */
+	if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-rising-time-ns",
+				 &i2c->rise_ns)) {
+		if (i2c->scl_frequency <= 100000)
+			i2c->rise_ns = 1000;
+		else
+			i2c->rise_ns = 300;
+	}
+	if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-falling-time-ns",
+				 &i2c->fall_ns))
+		i2c->fall_ns = 300;
+
 	strlcpy(i2c->adap.name, "rk3x-i2c", sizeof(i2c->adap.name));
 	i2c->adap.owner = THIS_MODULE;
 	i2c->adap.algo = &rk3x_i2c_algorithm;
-- 
1.8.3.2

Re: [PATCH v3] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

[addy ke]

On 2014/12/8 10:59, Addy Ke 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).
> 
> To fix this bug, min_low_ns should include fall time and min_high_ns
> should include rise time too.
> 
> This patch merged the patch that Doug submitted to chromium, which
> can get the rise and fall times for signals from the device tree.
> This allows us to more accurately calculate timings. see:
> https://chromium-review.googlesource.com/#/c/232774/
> 
> Signed-off-by: Addy Ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> Changes in v2:
> - merged the patch that Doug submitted to chromium
> Changes in v3:
> - merged the patch that Doug submitted to chromium to change bindins
>   see: https://chromium-review.googlesource.com/#/c/232775/
Sorry, see https://chromium-review.googlesource.com/#/c/232774/ not 232775
> 
>  Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 11 +++++
>  drivers/i2c/busses/i2c-rk3x.c                      | 57 +++++++++++++++-------
>  2 files changed, 50 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> index dde6c22..925d6eb 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> @@ -21,6 +21,14 @@ Required on RK3066, RK3188 :
>  Optional properties :
>  
>   - clock-frequency : SCL frequency to use (in Hz). If omitted, 100kHz is used.
> + - i2c-scl-rising-time-ns : Number of nanoseconds the signal takes to rise
> +	(t(r) in i2c spec). If not specified this is assumed to be the max the
> +	spec allows(1000 ns for standard mode, 300 ns for fast mode) which
> +	might cause slightly slower communication.
> + - i2c-scl-falling-time-ns : Number of nanoseconds the signal takes to fall
> +	(t(f) in the i2c0 spec).  If not specified this is assumed to be the
> +	max the spec allows (300 ns) which might cause slightly slower
> +	communication.
>  
>  Example:
>  
> @@ -39,4 +47,7 @@ i2c0: i2c@2002d000 {
>  
>  	clock-names = "i2c";
>  	clocks = <&cru PCLK_I2C0>;
> +
> +	i2c-scl-rising-time-ns = <800>;
> +	i2c-scl-falling-time-ns = <100>;
>  };
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 0ee5802..50c1678 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -102,6 +102,8 @@ struct rk3x_i2c {
>  
>  	/* Settings */
>  	unsigned int scl_frequency;
> +	unsigned int rise_ns;
> +	unsigned int fall_ns;
>  
>  	/* Synchronization & notification */
>  	spinlock_t lock;
> @@ -435,6 +437,8 @@ out:
>   *
>   * @clk_rate: I2C input clock rate
>   * @scl_rate: Desired SCL rate
> + * @rise_ns: How many ns it takes for signals to rise.
> + * @fall_ns: How many ns it takes for signals to fall.
>   * @div_low: Divider output for low
>   * @div_high: Divider output for high
>   *
> @@ -443,11 +447,14 @@ out:
>   * too high, we silently use the highest possible rate.
>   */
>  static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
> +			      unsigned long rise_ns, unsigned long fall_ns,
>  			      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 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 clk_rate_khz, scl_rate_khz;
> @@ -470,28 +477,30 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
>  
>  	/*
>  	 * min_low_ns:  The minimum number of ns we need to hold low
> -	 *		to meet i2c spec
> +	 *		to meet i2c spec, should include fall time.
>  	 * min_high_ns: The minimum number of ns we need to hold high
> -	 *		to meet i2c spec
> +	 *		to meet i2c spec, should include rise time.
>  	 * max_low_ns:  The maximum number of ns we can hold low
> -	 *		to meet i2c spec
> +	 *		to meet i2c spec.
>  	 *
>  	 * 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;
> +		spec_min_low_ns = 4700;
> +		spec_min_high_ns = 4000;
> +		spec_max_data_hold_ns = 3450;
> +		spec_data_hold_buffer_ns = 50;
>  	} else {
> -		min_low_ns = 1300;
> -		min_high_ns = 600;
> -		max_data_hold_ns = 900;
> -		data_hold_buffer_ns = 50;
> +		spec_min_low_ns = 1300;
> +		spec_min_high_ns = 600;
> +		spec_max_data_hold_ns = 900;
> +		spec_data_hold_buffer_ns = 50;
>  	}
> -	max_low_ns = max_data_hold_ns * 2 - data_hold_buffer_ns;
> +	min_low_ns = spec_min_low_ns + fall_ns;
> +	min_high_ns = spec_min_high_ns + 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 */
> @@ -588,8 +597,8 @@ static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
>  	u64 t_low_ns, t_high_ns;
>  	int ret;
>  
> -	ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, &div_low,
> -				 &div_high);
> +	ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, i2c->rise_ns,
> +				 i2c->fall_ns, &div_low, &div_high);
>  
>  	WARN_ONCE(ret != 0, "Could not reach SCL freq %u", i2c->scl_frequency);
>  
> @@ -633,9 +642,9 @@ static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long
>  	switch (event) {
>  	case PRE_RATE_CHANGE:
>  		if (rk3x_i2c_calc_divs(ndata->new_rate, i2c->scl_frequency,
> -				      &div_low, &div_high) != 0) {
> +				       i2c->rise_ns, i2c->fall_ns, &div_low,
> +				       &div_high) != 0)
>  			return NOTIFY_STOP;
> -		}
>  
>  		/* scale up */
>  		if (ndata->new_rate > ndata->old_rate)
> @@ -859,6 +868,18 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
>  		i2c->scl_frequency = DEFAULT_SCL_RATE;
>  	}
>  
> +	/* Read rise and fall ns; if not there use the default max from spec */
> +	if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-rising-time-ns",
> +				 &i2c->rise_ns)) {
> +		if (i2c->scl_frequency <= 100000)
> +			i2c->rise_ns = 1000;
> +		else
> +			i2c->rise_ns = 300;
> +	}
> +	if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-falling-time-ns",
> +				 &i2c->fall_ns))
> +		i2c->fall_ns = 300;
> +
>  	strlcpy(i2c->adap.name, "rk3x-i2c", sizeof(i2c->adap.name));
>  	i2c->adap.owner = THIS_MODULE;
>  	i2c->adap.algo = &rk3x_i2c_algorithm;
> 

[PATCH v4] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C specification

[Addy Ke]

The number of clock cycles to be written into the CLKDIV register
that determines the I2C clk high phase includes the rise time.
So to meet the timing requirements defined in the I2C specification
which defines the minimal time SCL has to be high, the rise time
has to taken into account. The same applies to the low phase with
falling time.

In my test on RK3288-Pink2 board, which is not an upstream board yet,
if external pull-up resistor is 4.7K, rise_ns is about 700ns.
So the measured high_ns is about 3900ns, which is less than 4000ns
(the minimum high_ns in I2C specification for Standard-mode).

To fix this bug, min_low_ns should include fall time and min_high_ns
should include rise time too.

This patch merged the patch that Doug submitted to chromium project,
which can get the rise and fall times for signals from the device tree.
This allows us to more accurately calculate timings. see:
https://chromium-review.googlesource.com/#/c/232774/

Signed-off-by: Addy Ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
Changes in v2:
- merged the patch that Doug submitted to chromium project
Changes in v3:
- merged the patch that Doug submitted to chromium to projectchange bindins
  see: https://chromium-review.googlesource.com/#/c/232774/
Changes in v4:
- Fix some typos pointed out by Uwe
- adjust "i2c" capitalization to "I2C"

 Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 11 ++++
 drivers/i2c/busses/i2c-rk3x.c                      | 76 +++++++++++++++-------
 2 files changed, 62 insertions(+), 25 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
index dde6c22..1885bd8 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
@@ -21,6 +21,14 @@ Required on RK3066, RK3188 :
 Optional properties :
 
  - clock-frequency : SCL frequency to use (in Hz). If omitted, 100kHz is used.
+ - i2c-scl-rising-time-ns : Number of nanoseconds the signal takes to rise
+	(t(r) in I2C specification). If not specified this is assumed to be
+	the maximum the specification allows(1000 ns for Standard-mode,
+	300 ns for Fast-mode) which might cause slightly slower communication.
+ - i2c-scl-falling-time-ns : Number of nanoseconds the signal takes to fall
+	(t(f) in the I2C specification). If not specified this is assumed to
+	be the maximum the specification allows (300 ns) which might cause
+	slightly slowercommunication.
 
 Example:
 
@@ -39,4 +47,7 @@ i2c0: i2c@2002d000 {
 
 	clock-names = "i2c";
 	clocks = <&cru PCLK_I2C0>;
+
+	i2c-scl-rising-time-ns = <800>;
+	i2c-scl-falling-time-ns = <100>;
 };
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 0ee5802..f8eb77e 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -102,6 +102,8 @@ struct rk3x_i2c {
 
 	/* Settings */
 	unsigned int scl_frequency;
+	unsigned int rise_ns;
+	unsigned int fall_ns;
 
 	/* Synchronization & notification */
 	spinlock_t lock;
@@ -435,6 +437,8 @@ out:
  *
  * @clk_rate: I2C input clock rate
  * @scl_rate: Desired SCL rate
+ * @rise_ns: How many ns it takes for signals to rise.
+ * @fall_ns: How many ns it takes for signals to fall.
  * @div_low: Divider output for low
  * @div_high: Divider output for high
  *
@@ -443,11 +447,14 @@ out:
  * too high, we silently use the highest possible rate.
  */
 static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
+			      unsigned long rise_ns, unsigned long fall_ns,
 			      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 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 clk_rate_khz, scl_rate_khz;
@@ -469,29 +476,33 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
 		scl_rate = 1000;
 
 	/*
-	 * 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 to
+	 *		meet I2C specification, should include fall time.
+	 * min_high_ns: The minimum number of ns we need to hold high to
+	 *		meet I2C specification, should include rise time.
+	 * max_low_ns:  The maximum number of ns we can hold low to meet
+	 *		I2C specification.
 	 *
-	 * Note: max_low_ns should be (max data hold time * 2 - buffer)
+	 * Note: max_low_ns should be (maximum 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;
+		/* Standard-mode */
+		spec_min_low_ns = 4700;
+		spec_min_high_ns = 4000;
+		spec_max_data_hold_ns = 3450;
+		spec_data_hold_buffer_ns = 50;
 	} else {
-		min_low_ns = 1300;
-		min_high_ns = 600;
-		max_data_hold_ns = 900;
-		data_hold_buffer_ns = 50;
+		/* Fast-mode */
+		spec_min_low_ns = 1300;
+		spec_min_high_ns = 600;
+		spec_max_data_hold_ns = 900;
+		spec_data_hold_buffer_ns = 50;
 	}
-	max_low_ns = max_data_hold_ns * 2 - data_hold_buffer_ns;
+	min_low_ns = spec_min_low_ns + fall_ns;
+	min_high_ns = spec_min_high_ns + 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 */
@@ -510,8 +521,8 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
 	min_div_for_hold = (min_low_div + min_high_div);
 
 	/*
-	 * This is the maximum divider so we don't go over the max.
-	 * We don't round up here (we round down) since this is a max.
+	 * This is the maximum divider so we don't go over the maximum.
+	 * We don't round up here (we round down) since this is a maximum.
 	 */
 	max_low_div = clk_rate_khz * max_low_ns / (8 * 1000000);
 
@@ -544,7 +555,7 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
 		ideal_low_div = DIV_ROUND_UP(clk_rate_khz * min_low_ns,
 					     scl_rate_khz * 8 * min_total_ns);
 
-		/* Don't allow it to go over the max */
+		/* Don't allow it to go over the maximum */
 		if (ideal_low_div > max_low_div)
 			ideal_low_div = max_low_div;
 
@@ -588,8 +599,8 @@ static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
 	u64 t_low_ns, t_high_ns;
 	int ret;
 
-	ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, &div_low,
-				 &div_high);
+	ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, i2c->rise_ns,
+				 i2c->fall_ns, &div_low, &div_high);
 
 	WARN_ONCE(ret != 0, "Could not reach SCL freq %u", i2c->scl_frequency);
 
@@ -633,9 +644,9 @@ static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long
 	switch (event) {
 	case PRE_RATE_CHANGE:
 		if (rk3x_i2c_calc_divs(ndata->new_rate, i2c->scl_frequency,
-				      &div_low, &div_high) != 0) {
+				       i2c->rise_ns, i2c->fall_ns, &div_low,
+				       &div_high) != 0)
 			return NOTIFY_STOP;
-		}
 
 		/* scale up */
 		if (ndata->new_rate > ndata->old_rate)
@@ -859,6 +870,21 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
 		i2c->scl_frequency = DEFAULT_SCL_RATE;
 	}
 
+	/*
+	 * Read rise and fall ns.
+	 * If not, there use the default maximum from specification.
+	 */
+	if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-rising-time-ns",
+				 &i2c->rise_ns)) {
+		if (i2c->scl_frequency <= 100000)
+			i2c->rise_ns = 1000;
+		else
+			i2c->rise_ns = 300;
+	}
+	if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-falling-time-ns",
+				 &i2c->fall_ns))
+		i2c->fall_ns = 300;
+
 	strlcpy(i2c->adap.name, "rk3x-i2c", sizeof(i2c->adap.name));
 	i2c->adap.owner = THIS_MODULE;
 	i2c->adap.algo = &rk3x_i2c_algorithm;
-- 
1.8.3.2

Re: [PATCH v4] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C specification

[Uwe Kleine-König]

Hello,

I like it now. There are only a few small nitpicks, not sure its worth
to respin if noone else has concerns. See below.

On Thu, Dec 11, 2014 at 02:00:49PM +0800, Addy Ke wrote:
> The number of clock cycles to be written into the CLKDIV register
> that determines the I2C clk high phase includes the rise time.
> So to meet the timing requirements defined in the I2C specification
> which defines the minimal time SCL has to be high, the rise time
> has to taken into account. The same applies to the low phase with
> falling time.
> 
> In my test on RK3288-Pink2 board, which is not an upstream board yet,
> if external pull-up resistor is 4.7K, rise_ns is about 700ns.
> So the measured high_ns is about 3900ns, which is less than 4000ns
> (the minimum high_ns in I2C specification for Standard-mode).
> 
> To fix this bug, min_low_ns should include fall time and min_high_ns
s/,//

> should include rise time too.
I'd skip the "too". If you want to keep it, s/time/time,/.
 
> This patch merged the patch that Doug submitted to chromium project,
AFAIK s/,//

For correctness, does this patch needs Doug's Sob?

> which can get the rise and fall times for signals from the device tree.
> This allows us to more accurately calculate timings. see:
> https://chromium-review.googlesource.com/#/c/232774/
> 
> Signed-off-by: Addy Ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> [...]
> @@ -469,29 +476,33 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
> [...]
>  	if (scl_rate <= 100000) {
> -		min_low_ns = 4700;
> -		min_high_ns = 4000;
> -		max_data_hold_ns = 3450;
> -		data_hold_buffer_ns = 50;
> +		/* Standard-mode */
> +		spec_min_low_ns = 4700;
> +		spec_min_high_ns = 4000;
> +		spec_max_data_hold_ns = 3450;
> +		spec_data_hold_buffer_ns = 50;
>  	} else {
> -		min_low_ns = 1300;
> -		min_high_ns = 600;
> -		max_data_hold_ns = 900;
> -		data_hold_buffer_ns = 50;
> +		/* Fast-mode */
> +		spec_min_low_ns = 1300;
> +		spec_min_high_ns = 600;
> +		spec_max_data_hold_ns = 900;
> +		spec_data_hold_buffer_ns = 50;
The background of my question regarding data_hold_buffer_ns in the last
round was: If data_hold_buffer_ns doesn't appear in the I2C
specification, should it be renamed to spec_... ? *shrug*

>  	}
> -	max_low_ns = max_data_hold_ns * 2 - data_hold_buffer_ns;
> +	min_low_ns = spec_min_low_ns + fall_ns;
> +	min_high_ns = spec_min_high_ns + 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 */
> @@ -510,8 +521,8 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
>  	min_div_for_hold = (min_low_div + min_high_div);
>  
>  	/*
> -	 * This is the maximum divider so we don't go over the max.
> -	 * We don't round up here (we round down) since this is a max.
> +	 * This is the maximum divider so we don't go over the maximum.
> +	 * We don't round up here (we round down) since this is a maximum.
>  	 */
>  	max_low_div = clk_rate_khz * max_low_ns / (8 * 1000000);
>  
> @@ -544,7 +555,7 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
>  		ideal_low_div = DIV_ROUND_UP(clk_rate_khz * min_low_ns,
>  					     scl_rate_khz * 8 * min_total_ns);
>  
> -		/* Don't allow it to go over the max */
> +		/* Don't allow it to go over the maximum */
>  		if (ideal_low_div > max_low_div)
>  			ideal_low_div = max_low_div;
>  
> @@ -588,8 +599,8 @@ static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
>  	u64 t_low_ns, t_high_ns;
>  	int ret;
>  
> -	ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, &div_low,
> -				 &div_high);
> +	ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, i2c->rise_ns,
> +				 i2c->fall_ns, &div_low, &div_high);
>  
>  	WARN_ONCE(ret != 0, "Could not reach SCL freq %u", i2c->scl_frequency);
>  
> @@ -633,9 +644,9 @@ static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long
>  	switch (event) {
>  	case PRE_RATE_CHANGE:
>  		if (rk3x_i2c_calc_divs(ndata->new_rate, i2c->scl_frequency,
> -				      &div_low, &div_high) != 0) {
> +				       i2c->rise_ns, i2c->fall_ns, &div_low,
> +				       &div_high) != 0)
>  			return NOTIFY_STOP;
> -		}
>  
>  		/* scale up */
>  		if (ndata->new_rate > ndata->old_rate)
> @@ -859,6 +870,21 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
>  		i2c->scl_frequency = DEFAULT_SCL_RATE;
>  	}
>  
> +	/*
> +	 * Read rise and fall ns.
> +	 * If not, there use the default maximum from specification.
I'd write:

	Read rise and fall time from device tree. If not available use
	the default maximum timing from the specification.

(Otherwise I think the comma needs to go after "there" in your
sentence.)

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v5] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C specification

[Addy Ke]

The number of clock cycles to be written into the CLKDIV register
that determines the I2C clk high phase includes the rise time.
So to meet the timing requirements defined in the I2C specification
which defines the minimal time SCL has to be high, the rise time
has to taken into account. The same applies to the low phase with
falling time.

In my test on RK3288-Pink2 board, which is not an upstream board yet,
if external pull-up resistor is 4.7K, rise_ns is about 700ns.
So the measured high_ns is about 3900ns, which is less than 4000ns
(the minimum high_ns in I2C specification for Standard-mode).

To fix this bug min_low_ns should include fall time and min_high_ns
should include rise time.

This patch merged the patch from chromium project which can get the
rise and fall times for signals from the device tree. This allows us
to more accurately calculate timings. see:
https://chromium-review.googlesource.com/#/c/232774/

Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
---
Changes in v2:
- merged the patch that Doug submitted to chromium project
Changes in v3:
- merged the patch that Doug submitted to chromium to projectchange bindins
  see: https://chromium-review.googlesource.com/#/c/232774/
Changes in v4:
- Fix some typos pointed out by Uwe
- adjust "i2c" capitalization to "I2C"
Changes in v5:
- Fix some typos pointed out by Uwe in patch v4

 Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 11 ++++
 drivers/i2c/busses/i2c-rk3x.c                      | 72 +++++++++++++++-------
 2 files changed, 60 insertions(+), 23 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
index dde6c22..1885bd8 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
@@ -21,6 +21,14 @@ Required on RK3066, RK3188 :
 Optional properties :
 
  - clock-frequency : SCL frequency to use (in Hz). If omitted, 100kHz is used.
+ - i2c-scl-rising-time-ns : Number of nanoseconds the signal takes to rise
+	(t(r) in I2C specification). If not specified this is assumed to be
+	the maximum the specification allows(1000 ns for Standard-mode,
+	300 ns for Fast-mode) which might cause slightly slower communication.
+ - i2c-scl-falling-time-ns : Number of nanoseconds the signal takes to fall
+	(t(f) in the I2C specification). If not specified this is assumed to
+	be the maximum the specification allows (300 ns) which might cause
+	slightly slowercommunication.
 
 Example:
 
@@ -39,4 +47,7 @@ i2c0: i2c@2002d000 {
 
 	clock-names = "i2c";
 	clocks = <&cru PCLK_I2C0>;
+
+	i2c-scl-rising-time-ns = <800>;
+	i2c-scl-falling-time-ns = <100>;
 };
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 0ee5802..6fe2bab 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -102,6 +102,8 @@ struct rk3x_i2c {
 
 	/* Settings */
 	unsigned int scl_frequency;
+	unsigned int rise_ns;
+	unsigned int fall_ns;
 
 	/* Synchronization & notification */
 	spinlock_t lock;
@@ -435,6 +437,8 @@ out:
  *
  * @clk_rate: I2C input clock rate
  * @scl_rate: Desired SCL rate
+ * @rise_ns: How many ns it takes for signals to rise.
+ * @fall_ns: How many ns it takes for signals to fall.
  * @div_low: Divider output for low
  * @div_high: Divider output for high
  *
@@ -443,11 +447,14 @@ out:
  * too high, we silently use the highest possible rate.
  */
 static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
+			      unsigned long rise_ns, unsigned long fall_ns,
 			      unsigned long *div_low, unsigned long *div_high)
 {
-	unsigned long min_low_ns, min_high_ns;
-	unsigned long max_data_hold_ns;
+	unsigned long spec_min_low_ns, spec_min_high_ns;
+	unsigned long spec_max_data_hold_ns;
 	unsigned long data_hold_buffer_ns;
+
+	unsigned long min_low_ns, min_high_ns;
 	unsigned long max_low_ns, min_total_ns;
 
 	unsigned long clk_rate_khz, scl_rate_khz;
@@ -469,29 +476,33 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
 		scl_rate = 1000;
 
 	/*
-	 * 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 to
+	 *		meet I2C specification, should include fall time.
+	 * min_high_ns: The minimum number of ns we need to hold high to
+	 *		meet I2C specification, should include rise time.
+	 * max_low_ns:  The maximum number of ns we can hold low to meet
+	 *		I2C specification.
 	 *
-	 * Note: max_low_ns should be (max data hold time * 2 - buffer)
+	 * Note: max_low_ns should be (maximum 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;
+		/* Standard-mode */
+		spec_min_low_ns = 4700;
+		spec_min_high_ns = 4000;
+		spec_max_data_hold_ns = 3450;
 		data_hold_buffer_ns = 50;
 	} else {
-		min_low_ns = 1300;
-		min_high_ns = 600;
-		max_data_hold_ns = 900;
+		/* Fast-mode */
+		spec_min_low_ns = 1300;
+		spec_min_high_ns = 600;
+		spec_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 + fall_ns;
+	min_high_ns = spec_min_high_ns + rise_ns;
+	max_low_ns = spec_max_data_hold_ns * 2 - data_hold_buffer_ns;
 	min_total_ns = min_low_ns + min_high_ns;
 
 	/* Adjust to avoid overflow */
@@ -510,8 +521,8 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
 	min_div_for_hold = (min_low_div + min_high_div);
 
 	/*
-	 * This is the maximum divider so we don't go over the max.
-	 * We don't round up here (we round down) since this is a max.
+	 * This is the maximum divider so we don't go over the maximum.
+	 * We don't round up here (we round down) since this is a maximum.
 	 */
 	max_low_div = clk_rate_khz * max_low_ns / (8 * 1000000);
 
@@ -544,7 +555,7 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
 		ideal_low_div = DIV_ROUND_UP(clk_rate_khz * min_low_ns,
 					     scl_rate_khz * 8 * min_total_ns);
 
-		/* Don't allow it to go over the max */
+		/* Don't allow it to go over the maximum */
 		if (ideal_low_div > max_low_div)
 			ideal_low_div = max_low_div;
 
@@ -588,8 +599,8 @@ static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
 	u64 t_low_ns, t_high_ns;
 	int ret;
 
-	ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, &div_low,
-				 &div_high);
+	ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, i2c->rise_ns,
+				 i2c->fall_ns, &div_low, &div_high);
 
 	WARN_ONCE(ret != 0, "Could not reach SCL freq %u", i2c->scl_frequency);
 
@@ -633,9 +644,9 @@ static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long
 	switch (event) {
 	case PRE_RATE_CHANGE:
 		if (rk3x_i2c_calc_divs(ndata->new_rate, i2c->scl_frequency,
-				      &div_low, &div_high) != 0) {
+				       i2c->rise_ns, i2c->fall_ns, &div_low,
+				       &div_high) != 0)
 			return NOTIFY_STOP;
-		}
 
 		/* scale up */
 		if (ndata->new_rate > ndata->old_rate)
@@ -859,6 +870,21 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
 		i2c->scl_frequency = DEFAULT_SCL_RATE;
 	}
 
+	/*
+	 * Read rise and fall time from device tree. If not available use
+	 * the default maximum timing from the specification.
+	 */
+	if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-rising-time-ns",
+				 &i2c->rise_ns)) {
+		if (i2c->scl_frequency <= 100000)
+			i2c->rise_ns = 1000;
+		else
+			i2c->rise_ns = 300;
+	}
+	if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-falling-time-ns",
+				 &i2c->fall_ns))
+		i2c->fall_ns = 300;
+
 	strlcpy(i2c->adap.name, "rk3x-i2c", sizeof(i2c->adap.name));
 	i2c->adap.owner = THIS_MODULE;
 	i2c->adap.algo = &rk3x_i2c_algorithm;
-- 
1.8.3.2

Re: [PATCH v5] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C specification

[Doug Anderson]

Hi,

On Thu, Dec 11, 2014 at 3:02 AM, Addy Ke <addy.ke@rock-chips.com> wrote:
>   - clock-frequency : SCL frequency to use (in Hz). If omitted, 100kHz is used.
> + - i2c-scl-rising-time-ns : Number of nanoseconds the signal takes to rise
> +       (t(r) in I2C specification). If not specified this is assumed to be
> +       the maximum the specification allows(1000 ns for Standard-mode,
> +       300 ns for Fast-mode) which might cause slightly slower communication.
> + - i2c-scl-falling-time-ns : Number of nanoseconds the signal takes to fall
> +       (t(f) in the I2C specification). If not specified this is assumed to
> +       be the maximum the specification allows (300 ns) which might cause
> +       slightly slowercommunication.

nit: you forgot a space between "slower" and "communication".

...this is the type of thing I think Wolfram can fixup when applying,
so I'd suggest against respinning unless something else is needed.

Overall I continue to be happy with this patch.

Reviewed-by: Doug Anderson <dianders@chromium.org>
Tested-by: Doug Anderson <dianders@chromium.org>

You'll notice that I posted a patch atop it at
<https://patchwork.kernel.org/patch/5477201/>.

-Doug

Re: [PATCH v5] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C specification

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]

On Thu, Dec 11, 2014 at 07:02:40PM +0800, Addy Ke wrote:
> The number of clock cycles to be written into the CLKDIV register
> that determines the I2C clk high phase includes the rise time.
> So to meet the timing requirements defined in the I2C specification
> which defines the minimal time SCL has to be high, the rise time
> has to taken into account. The same applies to the low phase with
> falling time.
> 
> In my test on RK3288-Pink2 board, which is not an upstream board yet,
> if external pull-up resistor is 4.7K, rise_ns is about 700ns.
> So the measured high_ns is about 3900ns, which is less than 4000ns
> (the minimum high_ns in I2C specification for Standard-mode).
> 
> To fix this bug min_low_ns should include fall time and min_high_ns
> should include rise time.
> 
> This patch merged the patch from chromium project which can get the
> rise and fall times for signals from the device tree. This allows us
> to more accurately calculate timings. see:
> https://chromium-review.googlesource.com/#/c/232774/
> 
> Signed-off-by: Addy Ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

Applied to for-next, thanks!

I fixed the typo Doug mentioned.

Please send new patches as seperate threads, not as a reply to the old
patch.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

[Uwe Kleine-König]

Hello Addy,

On Mon, Dec 08, 2014 at 10:59:49AM +0800, Addy Ke 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
I think "actual measured" is misleading here. (The driver doesn't
dermine these parameters automatically, right?)

Maybe something like the following is more correct and understandable?:

	The number of clock cycles to be written into the XYZ register
	that determines the i2c clk high phase includes the rise time.
	So to meet the timing requirements defined in the i2c
	specification which defines the minimal time SCL has to be high,
	the rise time has to taken into account. The same applies to the
	low phase with falling time.

> 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
It would be nice to point out the actual board you used (if it's a
publically available machine).

> 700ns. So the actual measured high_ns is about 3900ns, which is less
> than 4000ns(the minimum high_ns in I2C spec).
s/s(/s (/;
s/spec/specification for Standard-mode/

There are different capitalisation of i2c in the patch and the commit log. I
don't know what Wolfram prefers here, but using the same spelling
everywhere would be nice.

> To fix this bug, min_low_ns should include fall time and min_high_ns
> should include rise time too.
> 
> This patch merged the patch that Doug submitted to chromium, which
Isn't Chromium a web browser? Does it really contain i2c stuff?
I bet it's about Chromium OS.

> can get the rise and fall times for signals from the device tree.
> This allows us to more accurately calculate timings. see:
> https://chromium-review.googlesource.com/#/c/232774/
> 
> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
> ---
> Changes in v2:
> - merged the patch that Doug submitted to chromium
> Changes in v3:
> - merged the patch that Doug submitted to chromium to change bindins
>   see: https://chromium-review.googlesource.com/#/c/232775/
> 
>  Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 11 +++++
>  drivers/i2c/busses/i2c-rk3x.c                      | 57 +++++++++++++++-------
>  2 files changed, 50 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> index dde6c22..925d6eb 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> @@ -21,6 +21,14 @@ Required on RK3066, RK3188 :
>  Optional properties :
>  
>   - clock-frequency : SCL frequency to use (in Hz). If omitted, 100kHz is used.
> + - i2c-scl-rising-time-ns : Number of nanoseconds the signal takes to rise
> +	(t(r) in i2c spec). If not specified this is assumed to be the max the
s/spec/specification/; s/max/maximum/

> +	spec allows(1000 ns for standard mode, 300 ns for fast mode) which
s/s(/s (/; s/standard mode/Standard-mode/; s/fast mode/Fast-mode/;


> +	might cause slightly slower communication.
> + - i2c-scl-falling-time-ns : Number of nanoseconds the signal takes to fall
> +	(t(f) in the i2c0 spec).  If not specified this is assumed to be the
s/i2c0/i2c/

> +	max the spec allows (300 ns) which might cause slightly slower
> +	communication.
>  
>  Example:
>  
> @@ -39,4 +47,7 @@ i2c0: i2c@2002d000 {
>  
>  	clock-names = "i2c";
>  	clocks = <&cru PCLK_I2C0>;
> +
> +	i2c-scl-rising-time-ns = <800>;
> +	i2c-scl-falling-time-ns = <100>;
>  };
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 0ee5802..50c1678 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -470,28 +477,30 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
>  
>  	/*
>  	 * min_low_ns:  The minimum number of ns we need to hold low
> -	 *		to meet i2c spec
> +	 *		to meet i2c spec, should include fall time.
>  	 * min_high_ns: The minimum number of ns we need to hold high
> -	 *		to meet i2c spec
> +	 *		to meet i2c spec, should include rise time.
>  	 * max_low_ns:  The maximum number of ns we can hold low
> -	 *		to meet i2c spec
> +	 *		to meet i2c spec.
>  	 *
>  	 * 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) {
		/* Standard-mode */
> -		min_low_ns = 4700;
> -		min_high_ns = 4000;
> -		max_data_hold_ns = 3450;
> -		data_hold_buffer_ns = 50;
> +		spec_min_low_ns = 4700;
> +		spec_min_high_ns = 4000;
> +		spec_max_data_hold_ns = 3450;
The specification calls this "data valid time" (t_(VD;DAT)).
> +		spec_data_hold_buffer_ns = 50;
I didn't find this parameter in the spec, also it doesn't differ for
Standard-mode vs. Fast-mode. What is this?

>  	} else {
		/* Fast-mode */
> -		min_low_ns = 1300;
> -		min_high_ns = 600;
> -		max_data_hold_ns = 900;
> -		data_hold_buffer_ns = 50;
> +		spec_min_low_ns = 1300;
> +		spec_min_high_ns = 600;
> +		spec_max_data_hold_ns = 900;
> +		spec_data_hold_buffer_ns = 50;
>  	}
> -	max_low_ns = max_data_hold_ns * 2 - data_hold_buffer_ns;
> +	min_low_ns = spec_min_low_ns + fall_ns;
> +	min_high_ns = spec_min_high_ns + 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 */
> @@ -588,8 +597,8 @@ static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
>  	u64 t_low_ns, t_high_ns;
>  	int ret;
>  
> -	ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, &div_low,
> -				 &div_high);
> +	ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, i2c->rise_ns,
> +				 i2c->fall_ns, &div_low, &div_high);
>  
>  	WARN_ONCE(ret != 0, "Could not reach SCL freq %u", i2c->scl_frequency);
>  
> @@ -633,9 +642,9 @@ static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long
>  	switch (event) {
>  	case PRE_RATE_CHANGE:
>  		if (rk3x_i2c_calc_divs(ndata->new_rate, i2c->scl_frequency,
> -				      &div_low, &div_high) != 0) {
> +				       i2c->rise_ns, i2c->fall_ns, &div_low,
> +				       &div_high) != 0)
>  			return NOTIFY_STOP;
> -		}
>  
>  		/* scale up */
>  		if (ndata->new_rate > ndata->old_rate)
> @@ -859,6 +868,18 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
>  		i2c->scl_frequency = DEFAULT_SCL_RATE;
>  	}
>  
> +	/* Read rise and fall ns; if not there use the default max from spec */
> +	if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-rising-time-ns",
> +				 &i2c->rise_ns)) {
> +		if (i2c->scl_frequency <= 100000)
> +			i2c->rise_ns = 1000;
> +		else
> +			i2c->rise_ns = 300;
> +	}
> +	if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-falling-time-ns",
> +				 &i2c->fall_ns))
> +		i2c->fall_ns = 300;
> +
I don't know if other drivers do the same (I assume they should). If so,
moving this logic into the core would be nice. I guess this can still be
done later.

The i2c specification also has a minimum for t(r) and t(f). I don't know
why these are limited, but I think it would be good to check for these
limits, too.

>  	strlcpy(i2c->adap.name, "rk3x-i2c", sizeof(i2c->adap.name));
>  	i2c->adap.owner = THIS_MODULE;
>  	i2c->adap.algo = &rk3x_i2c_algorithm;

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v3] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

[Doug Anderson]

Hi,

On Mon, Dec 8, 2014 at 12:52 AM, Uwe Kleine-König
<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> Hello Addy,
>
> On Mon, Dec 08, 2014 at 10:59:49AM +0800, Addy Ke 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
> I think "actual measured" is misleading here. (The driver doesn't
> dermine these parameters automatically, right?)
>
> Maybe something like the following is more correct and understandable?:
>
>         The number of clock cycles to be written into the XYZ register
>         that determines the i2c clk high phase includes the rise time.
>         So to meet the timing requirements defined in the i2c
>         specification which defines the minimal time SCL has to be high,
>         the rise time has to taken into account. The same applies to the
>         low phase with falling time.

I agree that the prose of the commit message could be cleaned up.  I
don't think English is Addy's primary language.  Your proposals sound
fine to me, though I would probably replace XYZ by "CLKDIV".  You
should capitalize I2C here, too.


>> 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
> It would be nice to point out the actual board you used (if it's a
> publically available machine).

He probably tested on rk3288-pinky, which is not an upstream board yet.


>> 700ns. So the actual measured high_ns is about 3900ns, which is less
>> than 4000ns(the minimum high_ns in I2C spec).
> s/s(/s (/;
> s/spec/specification for Standard-mode/
>
> There are different capitalisation of i2c in the patch and the commit log. I
> don't know what Wolfram prefers here, but using the same spelling
> everywhere would be nice.

Can you please point out?  IIRC you should always capitalize I2C in
prose (descriptions, comments, documentation, etc).  However when used
in places which specific capitalization standards it should be
lowercase.  That means file names, directory names, device tree
property names, etc should be lower case.

I think Addy got it right in most cases and places where it's wrong
are probably my fault.  I think I missed a few in my patches.  :(


>> To fix this bug, min_low_ns should include fall time and min_high_ns
>> should include rise time too.
>>
>> This patch merged the patch that Doug submitted to chromium, which
> Isn't Chromium a web browser? Does it really contain i2c stuff?
> I bet it's about Chromium OS.

Chromium OS is part of the Chromium project.


>> +     (t(r) in i2c spec). If not specified this is assumed to be the max the
> s/spec/specification/; s/max/maximum/

This should have been I2C, not i2c (capital).


>> +     might cause slightly slower communication.
>> + - i2c-scl-falling-time-ns : Number of nanoseconds the signal takes to fall
>> +     (t(f) in the i2c0 spec).  If not specified this is assumed to be the
> s/i2c0/i2c/

Same here: I2C.


>>        * min_low_ns:  The minimum number of ns we need to hold low
>> -      *              to meet i2c spec
>> +      *              to meet i2c spec, should include fall time.
>>        * min_high_ns: The minimum number of ns we need to hold high
>> -      *              to meet i2c spec
>> +      *              to meet i2c spec, should include rise time.
>>        * max_low_ns:  The maximum number of ns we can hold low
>> -      *              to meet i2c spec
>> +      *              to meet i2c spec.

...and those should be I2C.


>>        *
>>        * 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) {
>                 /* Standard-mode */
>> -             min_low_ns = 4700;
>> -             min_high_ns = 4000;
>> -             max_data_hold_ns = 3450;
>> -             data_hold_buffer_ns = 50;
>> +             spec_min_low_ns = 4700;
>> +             spec_min_high_ns = 4000;
>> +             spec_max_data_hold_ns = 3450;
> The specification calls this "data valid time" (t_(VD;DAT)).

Could change this if need be, but the old code had it as "data hold".
Maybe we could argue about it in a future patch?


>> +             spec_data_hold_buffer_ns = 50;
> I didn't find this parameter in the spec, also it doesn't differ for
> Standard-mode vs. Fast-mode. What is this?

I believe I put it in an earlier bit of sample code when Addy was
worried about things being too close to the margins.  I'm not sure
what specifically Addy was worried about.  Maybe this should actually
be the falling time for data?

Again, this is not something new so perhaps we could debate in a future patch?


>> +     /* Read rise and fall ns; if not there use the default max from spec */
>> +     if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-rising-time-ns",
>> +                              &i2c->rise_ns)) {
>> +             if (i2c->scl_frequency <= 100000)
>> +                     i2c->rise_ns = 1000;
>> +             else
>> +                     i2c->rise_ns = 300;
>> +     }
>> +     if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-falling-time-ns",
>> +                              &i2c->fall_ns))
>> +             i2c->fall_ns = 300;
>> +
> I don't know if other drivers do the same (I assume they should). If so,
> moving this logic into the core would be nice. I guess this can still be
> done later.
>
> The i2c specification also has a minimum for t(r) and t(f). I don't know
> why these are limited, but I think it would be good to check for these
> limits, too.

One of these two properties is in another driver (the Designware one).
However, different hardware seems to take in different parameters for
I2C.  It seems like for now the best thing to do is to keep drivers
consistent.  If we notice a pattern then stuff should move to the
core.

-Doug

Re: [PATCH v3] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1484 bytes --]


> > There are different capitalisation of i2c in the patch and the commit log. I
> > don't know what Wolfram prefers here, but using the same spelling
> > everywhere would be nice.
> 
> Can you please point out?  IIRC you should always capitalize I2C in
> prose (descriptions, comments, documentation, etc).  However when used
> in places which specific capitalization standards it should be
> lowercase.  That means file names, directory names, device tree
> property names, etc should be lower case.

This is what I prefer, too. However, I am not too strict about it.
I mean, both are readable.

> >> +     /* Read rise and fall ns; if not there use the default max from spec */
> >> +     if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-rising-time-ns",
> >> +                              &i2c->rise_ns)) {
> >> +             if (i2c->scl_frequency <= 100000)
> >> +                     i2c->rise_ns = 1000;
> >> +             else
> >> +                     i2c->rise_ns = 300;
> >> +     }
> >> +     if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-falling-time-ns",
> >> +                              &i2c->fall_ns))
> >> +             i2c->fall_ns = 300;
> >> +
> > I don't know if other drivers do the same (I assume they should). If so,
> > moving this logic into the core would be nice. I guess this can still be
> > done later.

Let's leave that for later. If we do it, we should start with bus speed
setting first, probably.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

[Doug Anderson]

Hi,

On Mon, Dec 8, 2014 at 9:34 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>
>> > There are different capitalisation of i2c in the patch and the commit log. I
>> > don't know what Wolfram prefers here, but using the same spelling
>> > everywhere would be nice.
>>
>> Can you please point out?  IIRC you should always capitalize I2C in
>> prose (descriptions, comments, documentation, etc).  However when used
>> in places which specific capitalization standards it should be
>> lowercase.  That means file names, directory names, device tree
>> property names, etc should be lower case.
>
> This is what I prefer, too. However, I am not too strict about it.
> I mean, both are readable.

Glad my understanding is OK.  :)

>
>> >> +     /* Read rise and fall ns; if not there use the default max from spec */
>> >> +     if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-rising-time-ns",
>> >> +                              &i2c->rise_ns)) {
>> >> +             if (i2c->scl_frequency <= 100000)
>> >> +                     i2c->rise_ns = 1000;
>> >> +             else
>> >> +                     i2c->rise_ns = 300;
>> >> +     }
>> >> +     if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-falling-time-ns",
>> >> +                              &i2c->fall_ns))
>> >> +             i2c->fall_ns = 300;
>> >> +
>> > I don't know if other drivers do the same (I assume they should). If so,
>> > moving this logic into the core would be nice. I guess this can still be
>> > done later.
>
> Let's leave that for later. If we do it, we should start with bus speed
> setting first, probably.

OK, great.


So just to summarize for Addy, I think you're being asked to spin one more time.

1. Update the patch description as per Uwe.  Fix other typos pointed out by him.

2. Since you're spinning anyway, adjust "i2c" capitalization to "I2C".

3. If you agree with Uwe, rename spec_max_data_hold_ns to
spec_data_valid_ns.  If not, we should debate in a followup patch.

4. If you think spec_data_hold_buffer_ns could be better represented
in another way (should this be i2c-sda-falling-time-ns"?), please do
so.  If not, we should debate in a followup patch.

-Doug

Re: [PATCH v3] i2c: rk3x: fix bug that cause measured high_ns doesn't meet I2C spec

[Uwe Kleine-König]

Hello,

On Mon, Dec 08, 2014 at 10:53:44AM -0800, Doug Anderson wrote:
> So just to summarize for Addy, I think you're being asked to spin one more time.
> 
> 1. Update the patch description as per Uwe.  Fix other typos pointed out by him.
> 
> 2. Since you're spinning anyway, adjust "i2c" capitalization to "I2C".
> 
> 3. If you agree with Uwe, rename spec_max_data_hold_ns to
> spec_data_valid_ns.  If not, we should debate in a followup patch.
> 
> 4. If you think spec_data_hold_buffer_ns could be better represented
> in another way (should this be i2c-sda-falling-time-ns"?), please do
> so.  If not, we should debate in a followup patch.
Sounds all reasonable. I'd not address 3 and 4 in this patch, but do
this in separate one. (That's what I intended from the beginning, just
failed to point that out explicitly.)

Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |