[PATCH v5] i2c: rk3x: adjust the LOW divison based on characteristics of SCL

[PATCH v5] i2c: rk3x: adjust the LOW divison based on characteristics of SCL

[Addy Ke]

As show in I2C specification:
- Standard-mode: the minimum HIGH period of the scl clock is 4.0us
                 the minimum LOW period of the scl clock is 4.7us
- Fast-mode: the minimum HIGH period of the scl clock is 0.6us
             the minimum LOW period of the scl clock is 1.3us

I have measured i2c SCL waveforms in fast-mode by oscilloscope
on rk3288-pinky board. the LOW period of the scl clock is 1.3us.
It is so critical that we must adjust LOW division to increase
the LOW period of the scl clock.

Thanks Doug for the suggestion about division formulas.

Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Tested-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
---
Changes in v2:
- remove Fast-mode plus and HS-mode
- use new formulas suggested by Doug
Changes in V3:
- use new formulas (sep 30) suggested by Doug
Changes in V4:
- fix some wrong style
- WARN_ONCE if min_low_div > max_low_div
Changes in V5:
- round up for i2c_rate and round down for scl_rate, suggested by Max
- place a WARN_ON if scl_rate < 1000, suggested by Max
- add div_high and div_low overflow protection, suggested by Max

 drivers/i2c/busses/i2c-rk3x.c | 161 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 152 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index b41d979..a30a4fb 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -24,6 +24,7 @@
 #include <linux/wait.h>
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
+#include <linux/math64.h>
 
 
 /* Register Map */
@@ -428,18 +429,158 @@ out:
 	return IRQ_HANDLED;
 }
 
-static void rk3x_i2c_set_scl_rate(struct rk3x_i2c *i2c, unsigned long scl_rate)
+static int rk3x_i2c_calc_divs(unsigned long i2c_rate, unsigned long scl_rate,
+			       unsigned long *div_low, unsigned long *div_high)
 {
-	unsigned long i2c_rate = clk_get_rate(i2c->clk);
-	unsigned int div;
+	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;
+
+	unsigned long min_low_div, min_high_div;
+	unsigned long max_low_div;
+
+	unsigned long min_div_for_hold, min_total_div;
+	unsigned long extra_div, extra_low_div, ideal_low_div;
+
+	/* Only support standard-mode and fast-mode */
+	if (WARN_ON(scl_rate > 400000))
+		scl_rate = 400000;
+
+	/* prevent scl_rate_khz from becoming 0 */
+	if (WARN_ON(scl_rate < 1000))
+		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
+	 *
+	 * 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_total_ns = min_low_ns + min_high_ns;
+
+	/* Adjust to avoid overflow */
+	i2c_rate_khz = DIV_ROUND_UP(i2c_rate, 1000);
+	scl_rate_khz = scl_rate / 1000;
 
-	/* set DIV = DIVH = DIVL
-	 * SCL rate = (clk rate) / (8 * (DIVH + 1 + DIVL + 1))
-	 *          = (clk rate) / (16 * (DIV + 1))
+	/*
+	 * We need the total div to be >= this number
+	 * so we don't clock too fast.
+	 */
+	min_total_div = DIV_ROUND_UP(i2c_rate_khz, scl_rate_khz * 8);
+
+	/* These are the min dividers needed for min hold times. */
+	min_low_div = DIV_ROUND_UP(i2c_rate_khz * min_low_ns, 8 * 1000000);
+	min_high_div = DIV_ROUND_UP(i2c_rate_khz * min_high_ns, 8 * 1000000);
+	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.
 	 */
-	div = DIV_ROUND_UP(i2c_rate, scl_rate * 16) - 1;
+	max_low_div = i2c_rate_khz * max_low_ns / (8 * 1000000);
+
+	if (min_low_div > max_low_div) {
+		WARN_ONCE(true,
+			  "Conflicting, min_low_div %lu, max_low_div %lu\n",
+			  min_low_div, max_low_div);
+		max_low_div = min_low_div;
+	}
+
+	if (min_div_for_hold > min_total_div) {
+		/*
+		 * Time needed to meet hold requirements is important.
+		 * Just use that.
+		 */
+		*div_low = min_low_div;
+		*div_high = min_high_div;
+	} else {
+		/*
+		 * We've got to distribute some time among the low and high
+		 * so we don't run too fast.
+		 */
+		extra_div = min_total_div - min_div_for_hold;
+
+		/*
+		 * We'll try to split things up perfectly evenly,
+		 * biasing slightly towards having a higher div
+		 * for low (spend more time low).
+		 */
+		ideal_low_div = DIV_ROUND_UP(i2c_rate_khz * min_low_ns,
+					     scl_rate_khz * 8 * min_total_ns);
+
+		/* Don't allow it to go over the max */
+		if (ideal_low_div > max_low_div)
+			ideal_low_div = max_low_div;
+
+		/*
+		 * Handle when the ideal low div is going to take up
+		 * more than we have.
+		 */
+		if (ideal_low_div > min_low_div + extra_div)
+			ideal_low_div = min_low_div + extra_div;
+
+		/* Give low the "ideal" and give high whatever extra is left */
+		extra_low_div = ideal_low_div - min_low_div;
+		*div_low = ideal_low_div;
+		*div_high = min_high_div + (extra_div - extra_low_div);
+	}
+
+	/*
+	* Adjust to the fact that the hardware has an implicit "+1".
+	* NOTE: Above calculations always produce div_low > 0 and  div_high > 0.
+	*/
+	*div_low = *div_low - 1;
+	*div_high = *div_high - 1;
+
+	if (*div_low >= 0xffff || *div_high >= 0xffff)
+		return -EINVAL;
+	else
+		return 0;
+}
+
+static int rk3x_i2c_set_scl_rate(struct rk3x_i2c *i2c, unsigned long scl_rate)
+{
+	unsigned long i2c_rate = clk_get_rate(i2c->clk);
+	unsigned long div_low, div_high;
+	u64 t_low_ns, t_high_ns;
+	int ret = 0;
 
-	i2c_writel(i2c, (div << 16) | (div & 0xffff), REG_CLKDIV);
+	ret = rk3x_i2c_calc_divs(i2c_rate, scl_rate, &div_low, &div_high);
+	if (ret < 0)
+		return ret;
+
+	i2c_writel(i2c, (div_high << 16) | (div_low & 0xffff), REG_CLKDIV);
+
+	t_low_ns = div_u64(((u64)div_low + 1) * 8 * 1000000000, i2c_rate);
+	t_high_ns = div_u64(((u64)div_high + 1) * 8 * 1000000000, i2c_rate);
+	dev_dbg(i2c->dev,
+		"CLK %lukhz, Req %luns, Act low %lluns high %lluns\n",
+		i2c_rate / 1000,
+		1000000000 / scl_rate,
+		t_low_ns, t_high_ns);
+
+	return ret;
 }
 
 /**
@@ -537,7 +678,9 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
 	clk_enable(i2c->clk);
 
 	/* The clock rate might have changed, so setup the divider again */
-	rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
+	ret = rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
+	if (ret < 0)
+		return ret;
 
 	i2c->is_last_msg = false;
 
-- 
1.8.3.2

[PATCH v5] i2c: rk3x: adjust the LOW divison based on characteristics of SCL

[Max Schwarz]

Hi Addy,

On Monday 13 October 2014 at 10:44:04, Addy Ke wrote:
> As show in I2C specification:
> - Standard-mode: the minimum HIGH period of the scl clock is 4.0us
>                  the minimum LOW period of the scl clock is 4.7us
> - Fast-mode: the minimum HIGH period of the scl clock is 0.6us
>              the minimum LOW period of the scl clock is 1.3us
> 
> I have measured i2c SCL waveforms in fast-mode by oscilloscope
> on rk3288-pinky board. the LOW period of the scl clock is 1.3us.
> It is so critical that we must adjust LOW division to increase
> the LOW period of the scl clock.
> 
> Thanks Doug for the suggestion about division formulas.
> 
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Reviewed-by: Doug Anderson <dianders@chromium.org>
> Tested-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
> ---
> Changes in v2:
> - remove Fast-mode plus and HS-mode
> - use new formulas suggested by Doug
> Changes in V3:
> - use new formulas (sep 30) suggested by Doug
> Changes in V4:
> - fix some wrong style
> - WARN_ONCE if min_low_div > max_low_div
> Changes in V5:
> - round up for i2c_rate and round down for scl_rate, suggested by Max
> - place a WARN_ON if scl_rate < 1000, suggested by Max
> - add div_high and div_low overflow protection, suggested by Max
> 
>  drivers/i2c/busses/i2c-rk3x.c | 161
> +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 152
> insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index b41d979..a30a4fb 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -24,6 +24,7 @@
>  #include <linux/wait.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/regmap.h>
> +#include <linux/math64.h>
> 
> 
>  /* Register Map */
> @@ -428,18 +429,158 @@ out:
>  	return IRQ_HANDLED;
>  }
> 
> -static void rk3x_i2c_set_scl_rate(struct rk3x_i2c *i2c, unsigned long
> scl_rate) +static int rk3x_i2c_calc_divs(unsigned long i2c_rate, unsigned
> long scl_rate, +			       unsigned long *div_low, unsigned long 
*div_high)
>  {
> -	unsigned long i2c_rate = clk_get_rate(i2c->clk);
> -	unsigned int div;
> +	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;
> +
> +	unsigned long min_low_div, min_high_div;
> +	unsigned long max_low_div;
> +
> +	unsigned long min_div_for_hold, min_total_div;
> +	unsigned long extra_div, extra_low_div, ideal_low_div;
> +
> +	/* Only support standard-mode and fast-mode */
> +	if (WARN_ON(scl_rate > 400000))
> +		scl_rate = 400000;
> +
> +	/* prevent scl_rate_khz from becoming 0 */
> +	if (WARN_ON(scl_rate < 1000))
> +		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
> +	 *
> +	 * 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_total_ns = min_low_ns + min_high_ns;
> +
> +	/* Adjust to avoid overflow */
> +	i2c_rate_khz = DIV_ROUND_UP(i2c_rate, 1000);
> +	scl_rate_khz = scl_rate / 1000;
> 
> -	/* set DIV = DIVH = DIVL
> -	 * SCL rate = (clk rate) / (8 * (DIVH + 1 + DIVL + 1))
> -	 *          = (clk rate) / (16 * (DIV + 1))
> +	/*
> +	 * We need the total div to be >= this number
> +	 * so we don't clock too fast.
> +	 */
> +	min_total_div = DIV_ROUND_UP(i2c_rate_khz, scl_rate_khz * 8);
> +
> +	/* These are the min dividers needed for min hold times. */
> +	min_low_div = DIV_ROUND_UP(i2c_rate_khz * min_low_ns, 8 * 1000000);
> +	min_high_div = DIV_ROUND_UP(i2c_rate_khz * min_high_ns, 8 * 1000000);
> +	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.
>  	 */
> -	div = DIV_ROUND_UP(i2c_rate, scl_rate * 16) - 1;
> +	max_low_div = i2c_rate_khz * max_low_ns / (8 * 1000000);
> +
> +	if (min_low_div > max_low_div) {
> +		WARN_ONCE(true,
> +			  "Conflicting, min_low_div %lu, max_low_div %lu\n",
> +			  min_low_div, max_low_div);
> +		max_low_div = min_low_div;
> +	}
> +
> +	if (min_div_for_hold > min_total_div) {
> +		/*
> +		 * Time needed to meet hold requirements is important.
> +		 * Just use that.
> +		 */
> +		*div_low = min_low_div;
> +		*div_high = min_high_div;
> +	} else {
> +		/*
> +		 * We've got to distribute some time among the low and high
> +		 * so we don't run too fast.
> +		 */
> +		extra_div = min_total_div - min_div_for_hold;
> +
> +		/*
> +		 * We'll try to split things up perfectly evenly,
> +		 * biasing slightly towards having a higher div
> +		 * for low (spend more time low).
> +		 */
> +		ideal_low_div = DIV_ROUND_UP(i2c_rate_khz * min_low_ns,
> +					     scl_rate_khz * 8 * min_total_ns);
> +
> +		/* Don't allow it to go over the max */
> +		if (ideal_low_div > max_low_div)
> +			ideal_low_div = max_low_div;
> +
> +		/*
> +		 * Handle when the ideal low div is going to take up
> +		 * more than we have.
> +		 */
> +		if (ideal_low_div > min_low_div + extra_div)
> +			ideal_low_div = min_low_div + extra_div;
> +
> +		/* Give low the "ideal" and give high whatever extra is left */
> +		extra_low_div = ideal_low_div - min_low_div;
> +		*div_low = ideal_low_div;
> +		*div_high = min_high_div + (extra_div - extra_low_div);
> +	}
> +
> +	/*
> +	* Adjust to the fact that the hardware has an implicit "+1".
> +	* NOTE: Above calculations always produce div_low > 0 and  div_high > 0.
> +	*/
> +	*div_low = *div_low - 1;
> +	*div_high = *div_high - 1;
> +
> +	if (*div_low >= 0xffff || *div_high >= 0xffff)
> +		return -EINVAL;
> +	else
> +		return 0;
> +}
> +
> +static int rk3x_i2c_set_scl_rate(struct rk3x_i2c *i2c, unsigned long
> scl_rate) +{
> +	unsigned long i2c_rate = clk_get_rate(i2c->clk);
> +	unsigned long div_low, div_high;
> +	u64 t_low_ns, t_high_ns;
> +	int ret = 0;
> 
> -	i2c_writel(i2c, (div << 16) | (div & 0xffff), REG_CLKDIV);
> +	ret = rk3x_i2c_calc_divs(i2c_rate, scl_rate, &div_low, &div_high);
> +	if (ret < 0)
> +		return ret;
> +
> +	i2c_writel(i2c, (div_high << 16) | (div_low & 0xffff), REG_CLKDIV);
> +
> +	t_low_ns = div_u64(((u64)div_low + 1) * 8 * 1000000000, i2c_rate);
> +	t_high_ns = div_u64(((u64)div_high + 1) * 8 * 1000000000, i2c_rate);
> +	dev_dbg(i2c->dev,
> +		"CLK %lukhz, Req %luns, Act low %lluns high %lluns\n",
> +		i2c_rate / 1000,
> +		1000000000 / scl_rate,
> +		t_low_ns, t_high_ns);
> +
> +	return ret;
>  }
> 
>  /**
> @@ -537,7 +678,9 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
>  	clk_enable(i2c->clk);
> 
>  	/* The clock rate might have changed, so setup the divider again */
> -	rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
> +	ret = rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
> +	if (ret < 0)
> +		return ret;

You are leaving the i2c->lock locked and the clock enabled if you return here. 
Please use goto to jump to the clk_disable(i2c->clk) at the end of the 
function.

Cheers,
  Max

[PATCH v6] i2c: rk3x: adjust the LOW divison based on characteristics of SCL

[Addy Ke]

As show in I2C specification:
- Standard-mode: the minimum HIGH period of the scl clock is 4.0us
                 the minimum LOW period of the scl clock is 4.7us
- Fast-mode: the minimum HIGH period of the scl clock is 0.6us
             the minimum LOW period of the scl clock is 1.3us

I have measured i2c SCL waveforms in fast-mode by oscilloscope
on rk3288-pinky board. the LOW period of the scl clock is 1.3us.
It is so critical that we must adjust LOW division to increase
the LOW period of the scl clock.

Thanks Doug for the suggestion about division formulas.

Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Tested-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
---
Changes in v2:
- remove Fast-mode plus and HS-mode
- use new formulas suggested by Doug
Changes in V3:
- use new formulas (sep 30) suggested by Doug
Changes in V4:
- fix some wrong style
- WARN_ONCE if min_low_div > max_low_div
Changes in V5:
- round up for i2c_rate and round down for scl_rate, suggested by Max
- place a WARN_ON if scl_rate < 1000, suggested by Max
- add div_high and div_low overflow protection, suggested by Max
Changes in V6:
- goto to jump to clk_disable if set_scl_rate return error

 drivers/i2c/busses/i2c-rk3x.c | 162 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 153 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index b41d979..47e85dc 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -24,6 +24,7 @@
 #include <linux/wait.h>
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
+#include <linux/math64.h>
 
 
 /* Register Map */
@@ -428,18 +429,158 @@ out:
 	return IRQ_HANDLED;
 }
 
-static void rk3x_i2c_set_scl_rate(struct rk3x_i2c *i2c, unsigned long scl_rate)
+static int rk3x_i2c_calc_divs(unsigned long i2c_rate, unsigned long scl_rate,
+			       unsigned long *div_low, unsigned long *div_high)
 {
-	unsigned long i2c_rate = clk_get_rate(i2c->clk);
-	unsigned int div;
+	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;
+
+	unsigned long min_low_div, min_high_div;
+	unsigned long max_low_div;
+
+	unsigned long min_div_for_hold, min_total_div;
+	unsigned long extra_div, extra_low_div, ideal_low_div;
+
+	/* Only support standard-mode and fast-mode */
+	if (WARN_ON(scl_rate > 400000))
+		scl_rate = 400000;
+
+	/* prevent scl_rate_khz from becoming 0 */
+	if (WARN_ON(scl_rate < 1000))
+		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
+	 *
+	 * 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_total_ns = min_low_ns + min_high_ns;
+
+	/* Adjust to avoid overflow */
+	i2c_rate_khz = DIV_ROUND_UP(i2c_rate, 1000);
+	scl_rate_khz = scl_rate / 1000;
 
-	/* set DIV = DIVH = DIVL
-	 * SCL rate = (clk rate) / (8 * (DIVH + 1 + DIVL + 1))
-	 *          = (clk rate) / (16 * (DIV + 1))
+	/*
+	 * We need the total div to be >= this number
+	 * so we don't clock too fast.
+	 */
+	min_total_div = DIV_ROUND_UP(i2c_rate_khz, scl_rate_khz * 8);
+
+	/* These are the min dividers needed for min hold times. */
+	min_low_div = DIV_ROUND_UP(i2c_rate_khz * min_low_ns, 8 * 1000000);
+	min_high_div = DIV_ROUND_UP(i2c_rate_khz * min_high_ns, 8 * 1000000);
+	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.
 	 */
-	div = DIV_ROUND_UP(i2c_rate, scl_rate * 16) - 1;
+	max_low_div = i2c_rate_khz * max_low_ns / (8 * 1000000);
+
+	if (min_low_div > max_low_div) {
+		WARN_ONCE(true,
+			  "Conflicting, min_low_div %lu, max_low_div %lu\n",
+			  min_low_div, max_low_div);
+		max_low_div = min_low_div;
+	}
 
-	i2c_writel(i2c, (div << 16) | (div & 0xffff), REG_CLKDIV);
+	if (min_div_for_hold > min_total_div) {
+		/*
+		 * Time needed to meet hold requirements is important.
+		 * Just use that.
+		 */
+		*div_low = min_low_div;
+		*div_high = min_high_div;
+	} else {
+		/*
+		 * We've got to distribute some time among the low and high
+		 * so we don't run too fast.
+		 */
+		extra_div = min_total_div - min_div_for_hold;
+
+		/*
+		 * We'll try to split things up perfectly evenly,
+		 * biasing slightly towards having a higher div
+		 * for low (spend more time low).
+		 */
+		ideal_low_div = DIV_ROUND_UP(i2c_rate_khz * min_low_ns,
+					     scl_rate_khz * 8 * min_total_ns);
+
+		/* Don't allow it to go over the max */
+		if (ideal_low_div > max_low_div)
+			ideal_low_div = max_low_div;
+
+		/*
+		 * Handle when the ideal low div is going to take up
+		 * more than we have.
+		 */
+		if (ideal_low_div > min_low_div + extra_div)
+			ideal_low_div = min_low_div + extra_div;
+
+		/* Give low the "ideal" and give high whatever extra is left */
+		extra_low_div = ideal_low_div - min_low_div;
+		*div_low = ideal_low_div;
+		*div_high = min_high_div + (extra_div - extra_low_div);
+	}
+
+	/*
+	* Adjust to the fact that the hardware has an implicit "+1".
+	* NOTE: Above calculations always produce div_low > 0 and  div_high > 0.
+	*/
+	*div_low = *div_low - 1;
+	*div_high = *div_high - 1;
+
+	if (*div_low >= 0xffff || *div_high >= 0xffff)
+		return -EINVAL;
+	else
+		return 0;
+}
+
+static int rk3x_i2c_set_scl_rate(struct rk3x_i2c *i2c, unsigned long scl_rate)
+{
+	unsigned long i2c_rate = clk_get_rate(i2c->clk);
+	unsigned long div_low, div_high;
+	u64 t_low_ns, t_high_ns;
+	int ret = 0;
+
+	ret = rk3x_i2c_calc_divs(i2c_rate, scl_rate, &div_low, &div_high);
+	if (ret < 0)
+		return ret;
+
+	i2c_writel(i2c, (div_high << 16) | (div_low & 0xffff), REG_CLKDIV);
+
+	t_low_ns = div_u64(((u64)div_low + 1) * 8 * 1000000000, i2c_rate);
+	t_high_ns = div_u64(((u64)div_high + 1) * 8 * 1000000000, i2c_rate);
+	dev_dbg(i2c->dev,
+		"CLK %lukhz, Req %luns, Act low %lluns high %lluns\n",
+		i2c_rate / 1000,
+		1000000000 / scl_rate,
+		t_low_ns, t_high_ns);
+
+	return ret;
 }
 
 /**
@@ -537,7 +678,9 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
 	clk_enable(i2c->clk);
 
 	/* The clock rate might have changed, so setup the divider again */
-	rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
+	ret = rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
+	if (ret < 0)
+		goto exit;
 
 	i2c->is_last_msg = false;
 
@@ -585,6 +728,7 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
 		}
 	}
 
+exit:
 	clk_disable(i2c->clk);
 	spin_unlock_irqrestore(&i2c->lock, flags);
 
-- 
1.8.3.2

[PATCH v6] i2c: rk3x: adjust the LOW divison based on characteristics of SCL

[Max Schwarz]

Hi Addy,

On Tuesday 14 October 2014 at 14:09:21, Addy Ke wrote:
> As show in I2C specification:
> - Standard-mode: the minimum HIGH period of the scl clock is 4.0us
>                  the minimum LOW period of the scl clock is 4.7us
> - Fast-mode: the minimum HIGH period of the scl clock is 0.6us
>              the minimum LOW period of the scl clock is 1.3us
> 
> I have measured i2c SCL waveforms in fast-mode by oscilloscope
> on rk3288-pinky board. the LOW period of the scl clock is 1.3us.
> It is so critical that we must adjust LOW division to increase
> the LOW period of the scl clock.
> 
> Thanks Doug for the suggestion about division formulas.
> 
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Reviewed-by: Doug Anderson <dianders@chromium.org>
> Tested-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>

Reviewed-by: Max Schwarz <max.schwarz@online.de>
Tested-by: Max Schwarz <max.schwarz@online.de>

Cheers,
  Max

[PATCH v6] i2c: rk3x: adjust the LOW divison based on characteristics of SCL

[Wolfram Sang]

On Tue, Oct 14, 2014 at 02:09:21PM +0800, Addy Ke wrote:
> As show in I2C specification:
> - Standard-mode: the minimum HIGH period of the scl clock is 4.0us
>                  the minimum LOW period of the scl clock is 4.7us
> - Fast-mode: the minimum HIGH period of the scl clock is 0.6us
>              the minimum LOW period of the scl clock is 1.3us
> 
> I have measured i2c SCL waveforms in fast-mode by oscilloscope
> on rk3288-pinky board. the LOW period of the scl clock is 1.3us.
> It is so critical that we must adjust LOW division to increase
> the LOW period of the scl clock.
> 
> Thanks Doug for the suggestion about division formulas.
> 
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Reviewed-by: Doug Anderson <dianders@chromium.org>
> Tested-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>

Applied to for-next, thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141110/d2cf78fb/attachment.sig>