[PATCH v8 0/8] add i2c driver supported for rk3399

[PATCH v8 0/8] add i2c driver supported for rk3399

[David Wu]

There are three points differert from others:
 - new method to caculate i2c timings for rk3399
 - pclk and function clk are separated at rk3399
 - add fast-plus mode supported for rk3399

David Wu (8):
  i2c: rk3x: add documentation to fields in "struct rk3x_i2c"
  i2c: rk3x: use struct "rk3x_i2c_calced_timings"
  i2c: rk3x: Remove redundant rk3x_i2c_clean_ipd()
  i2c: rk3x: Change SoC data to not use array
  i2c: rk3x: Move spec timing data to "static const" structs
  dt-bindings: i2c: rk3x: add support for rk3399
  i2c: rk3x: add i2c support for rk3399 soc
  i2c: rk3x: support fast-mode plus for rk3399

 Documentation/devicetree/bindings/i2c/i2c-rk3x.txt |  16 +-
 drivers/i2c/busses/i2c-rk3x.c                      | 493 +++++++++++++++++----
 2 files changed, 430 insertions(+), 79 deletions(-)

-- 
1.9.1

[PATCH v8 0/8] add i2c driver supported for rk3399

[David Wu]

There are three points differert from others:
 - new method to caculate i2c timings for rk3399
 - pclk and function clk are separated at rk3399
 - add fast-plus mode supported for rk3399

David Wu (8):
  i2c: rk3x: add documentation to fields in "struct rk3x_i2c"
  i2c: rk3x: use struct "rk3x_i2c_calced_timings"
  i2c: rk3x: Remove redundant rk3x_i2c_clean_ipd()
  i2c: rk3x: Change SoC data to not use array
  i2c: rk3x: Move spec timing data to "static const" structs
  dt-bindings: i2c: rk3x: add support for rk3399
  i2c: rk3x: add i2c support for rk3399 soc
  i2c: rk3x: support fast-mode plus for rk3399

 Documentation/devicetree/bindings/i2c/i2c-rk3x.txt |  16 +-
 drivers/i2c/busses/i2c-rk3x.c                      | 493 +++++++++++++++++----
 2 files changed, 430 insertions(+), 79 deletions(-)

-- 
1.9.1

[PATCH v8 1/8] i2c: rk3x: add documentation to fields in "struct rk3x_i2c"

[David Wu]

Signed-off-by: David Wu <david.wu@rock-chips.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Change in v8:
- none

 drivers/i2c/busses/i2c-rk3x.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 80bed02..7e45d51 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -90,6 +90,26 @@ struct rk3x_i2c_soc_data {
 	int grf_offset;
 };
 
+/**
+ * struct rk3x_i2c - private data of the controller
+ * @adap: corresponding I2C adapter
+ * @dev: device for this controller
+ * @soc_data: related soc data struct
+ * @regs: virtual memory area
+ * @clk: clock of i2c bus
+ * @clk_rate_nb: i2c clk rate change notify
+ * @t: I2C known timing information
+ * @lock: spinlock for the i2c bus
+ * @wait: the waitqueue to wait for i2c transfer
+ * @busy: the condition for the event to wait for
+ * @msg: current i2c message
+ * @addr: addr of i2c slave device
+ * @mode: mode of i2c transfer
+ * @is_last_msg: flag determines whether it is the last msg in this transfer
+ * @state: state of i2c transfer
+ * @processed: byte length which has been send or received
+ * @error: error code for i2c transfer
+ */
 struct rk3x_i2c {
 	struct i2c_adapter adap;
 	struct device *dev;
@@ -116,7 +136,7 @@ struct rk3x_i2c {
 
 	/* I2C state machine */
 	enum rk3x_i2c_state state;
-	unsigned int processed; /* sent/received bytes */
+	unsigned int processed;
 	int error;
 };
 
-- 
1.9.1

[PATCH v8 1/8] i2c: rk3x: add documentation to fields in "struct rk3x_i2c"

[David Wu]

Signed-off-by: David Wu <david.wu@rock-chips.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Change in v8:
- none

 drivers/i2c/busses/i2c-rk3x.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 80bed02..7e45d51 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -90,6 +90,26 @@ struct rk3x_i2c_soc_data {
 	int grf_offset;
 };
 
+/**
+ * struct rk3x_i2c - private data of the controller
+ * @adap: corresponding I2C adapter
+ * @dev: device for this controller
+ * @soc_data: related soc data struct
+ * @regs: virtual memory area
+ * @clk: clock of i2c bus
+ * @clk_rate_nb: i2c clk rate change notify
+ * @t: I2C known timing information
+ * @lock: spinlock for the i2c bus
+ * @wait: the waitqueue to wait for i2c transfer
+ * @busy: the condition for the event to wait for
+ * @msg: current i2c message
+ * @addr: addr of i2c slave device
+ * @mode: mode of i2c transfer
+ * @is_last_msg: flag determines whether it is the last msg in this transfer
+ * @state: state of i2c transfer
+ * @processed: byte length which has been send or received
+ * @error: error code for i2c transfer
+ */
 struct rk3x_i2c {
 	struct i2c_adapter adap;
 	struct device *dev;
@@ -116,7 +136,7 @@ struct rk3x_i2c {
 
 	/* I2C state machine */
 	enum rk3x_i2c_state state;
-	unsigned int processed; /* sent/received bytes */
+	unsigned int processed;
 	int error;
 };
 
-- 
1.9.1

[PATCH v8 2/8] i2c: rk3x: use struct "rk3x_i2c_calced_timings"

[David Wu]

The "div_high" and "div_low" values are always used together.  Group them
into a structure to make it easier to pass them both around.  This
structure also provides a place for future calculated timings.

Signed-off-by: David Wu <david.wu@rock-chips.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Change in v8:
- add commit description.

 drivers/i2c/busses/i2c-rk3x.c | 55 +++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 23 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 7e45d51..1e2677a 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -75,6 +75,16 @@ enum {
 #define WAIT_TIMEOUT      1000 /* ms */
 #define DEFAULT_SCL_RATE  (100 * 1000) /* Hz */
 
+/**
+ * struct rk3x_i2c_calced_timings:
+ * @div_low: Divider output for low
+ * @div_high: Divider output for high
+ */
+struct rk3x_i2c_calced_timings {
+	unsigned long div_low;
+	unsigned long div_high;
+};
+
 enum rk3x_i2c_state {
 	STATE_IDLE,
 	STATE_START,
@@ -454,9 +464,8 @@ out:
  * Calculate divider values for desired SCL frequency
  *
  * @clk_rate: I2C input clock rate
- * @t: Known I2C timing information.
- * @div_low: Divider output for low
- * @div_high: Divider output for high
+ * @t: Known I2C timing information
+ * @t_calc: Caculated rk3x private timings that would be written into regs
  *
  * Returns: 0 on success, -EINVAL if the goal SCL rate is too slow. In that case
  * a best-effort divider value is returned in divs. If the target rate is
@@ -464,8 +473,7 @@ out:
  */
 static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 			      struct i2c_timings *t,
-			      unsigned long *div_low,
-			      unsigned long *div_high)
+			      struct rk3x_i2c_calced_timings *t_calc)
 {
 	unsigned long spec_min_low_ns, spec_min_high_ns;
 	unsigned long spec_setup_start, spec_max_data_hold_ns;
@@ -572,8 +580,8 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 		 * Time needed to meet hold requirements is important.
 		 * Just use that.
 		 */
-		*div_low = min_low_div;
-		*div_high = min_high_div;
+		t_calc->div_low = min_low_div;
+		t_calc->div_high = min_high_div;
 	} else {
 		/*
 		 * We've got to distribute some time among the low and high
@@ -602,25 +610,25 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 
 		/* 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);
+		t_calc->div_low = ideal_low_div;
+		t_calc->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;
+	t_calc->div_low--;
+	t_calc->div_high--;
 
 	/* Maximum divider supported by hw is 0xffff */
-	if (*div_low > 0xffff) {
-		*div_low = 0xffff;
+	if (t_calc->div_low > 0xffff) {
+		t_calc->div_low = 0xffff;
 		ret = -EINVAL;
 	}
 
-	if (*div_high > 0xffff) {
-		*div_high = 0xffff;
+	if (t_calc->div_high > 0xffff) {
+		t_calc->div_high = 0xffff;
 		ret = -EINVAL;
 	}
 
@@ -630,19 +638,21 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
 {
 	struct i2c_timings *t = &i2c->t;
-	unsigned long div_low, div_high;
+	struct rk3x_i2c_calced_timings calc;
 	u64 t_low_ns, t_high_ns;
 	int ret;
 
-	ret = rk3x_i2c_calc_divs(clk_rate, t, &div_low, &div_high);
+	ret = rk3x_i2c_calc_divs(clk_rate, t, &calc);
 	WARN_ONCE(ret != 0, "Could not reach SCL freq %u", t->bus_freq_hz);
 
 	clk_enable(i2c->clk);
-	i2c_writel(i2c, (div_high << 16) | (div_low & 0xffff), REG_CLKDIV);
+	i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
+		   REG_CLKDIV);
 	clk_disable(i2c->clk);
 
-	t_low_ns = div_u64(((u64)div_low + 1) * 8 * 1000000000, clk_rate);
-	t_high_ns = div_u64(((u64)div_high + 1) * 8 * 1000000000, clk_rate);
+	t_low_ns = div_u64(((u64)calc.div_low + 1) * 8 * 1000000000, clk_rate);
+	t_high_ns = div_u64(((u64)calc.div_high + 1) * 8 * 1000000000,
+			    clk_rate);
 	dev_dbg(i2c->dev,
 		"CLK %lukhz, Req %uns, Act low %lluns high %lluns\n",
 		clk_rate / 1000,
@@ -672,12 +682,11 @@ static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long
 {
 	struct clk_notifier_data *ndata = data;
 	struct rk3x_i2c *i2c = container_of(nb, struct rk3x_i2c, clk_rate_nb);
-	unsigned long div_low, div_high;
+	struct rk3x_i2c_calced_timings calc;
 
 	switch (event) {
 	case PRE_RATE_CHANGE:
-		if (rk3x_i2c_calc_divs(ndata->new_rate, &i2c->t,
-				       &div_low, &div_high) != 0)
+		if (rk3x_i2c_calc_divs(ndata->new_rate, &i2c->t, &calc) != 0)
 			return NOTIFY_STOP;
 
 		/* scale up */
-- 
1.9.1

[PATCH v8 2/8] i2c: rk3x: use struct "rk3x_i2c_calced_timings"

[David Wu]

The "div_high" and "div_low" values are always used together.  Group them
into a structure to make it easier to pass them both around.  This
structure also provides a place for future calculated timings.

Signed-off-by: David Wu <david.wu@rock-chips.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Change in v8:
- add commit description.

 drivers/i2c/busses/i2c-rk3x.c | 55 +++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 23 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 7e45d51..1e2677a 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -75,6 +75,16 @@ enum {
 #define WAIT_TIMEOUT      1000 /* ms */
 #define DEFAULT_SCL_RATE  (100 * 1000) /* Hz */
 
+/**
+ * struct rk3x_i2c_calced_timings:
+ * @div_low: Divider output for low
+ * @div_high: Divider output for high
+ */
+struct rk3x_i2c_calced_timings {
+	unsigned long div_low;
+	unsigned long div_high;
+};
+
 enum rk3x_i2c_state {
 	STATE_IDLE,
 	STATE_START,
@@ -454,9 +464,8 @@ out:
  * Calculate divider values for desired SCL frequency
  *
  * @clk_rate: I2C input clock rate
- * @t: Known I2C timing information.
- * @div_low: Divider output for low
- * @div_high: Divider output for high
+ * @t: Known I2C timing information
+ * @t_calc: Caculated rk3x private timings that would be written into regs
  *
  * Returns: 0 on success, -EINVAL if the goal SCL rate is too slow. In that case
  * a best-effort divider value is returned in divs. If the target rate is
@@ -464,8 +473,7 @@ out:
  */
 static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 			      struct i2c_timings *t,
-			      unsigned long *div_low,
-			      unsigned long *div_high)
+			      struct rk3x_i2c_calced_timings *t_calc)
 {
 	unsigned long spec_min_low_ns, spec_min_high_ns;
 	unsigned long spec_setup_start, spec_max_data_hold_ns;
@@ -572,8 +580,8 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 		 * Time needed to meet hold requirements is important.
 		 * Just use that.
 		 */
-		*div_low = min_low_div;
-		*div_high = min_high_div;
+		t_calc->div_low = min_low_div;
+		t_calc->div_high = min_high_div;
 	} else {
 		/*
 		 * We've got to distribute some time among the low and high
@@ -602,25 +610,25 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 
 		/* 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);
+		t_calc->div_low = ideal_low_div;
+		t_calc->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;
+	t_calc->div_low--;
+	t_calc->div_high--;
 
 	/* Maximum divider supported by hw is 0xffff */
-	if (*div_low > 0xffff) {
-		*div_low = 0xffff;
+	if (t_calc->div_low > 0xffff) {
+		t_calc->div_low = 0xffff;
 		ret = -EINVAL;
 	}
 
-	if (*div_high > 0xffff) {
-		*div_high = 0xffff;
+	if (t_calc->div_high > 0xffff) {
+		t_calc->div_high = 0xffff;
 		ret = -EINVAL;
 	}
 
@@ -630,19 +638,21 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
 {
 	struct i2c_timings *t = &i2c->t;
-	unsigned long div_low, div_high;
+	struct rk3x_i2c_calced_timings calc;
 	u64 t_low_ns, t_high_ns;
 	int ret;
 
-	ret = rk3x_i2c_calc_divs(clk_rate, t, &div_low, &div_high);
+	ret = rk3x_i2c_calc_divs(clk_rate, t, &calc);
 	WARN_ONCE(ret != 0, "Could not reach SCL freq %u", t->bus_freq_hz);
 
 	clk_enable(i2c->clk);
-	i2c_writel(i2c, (div_high << 16) | (div_low & 0xffff), REG_CLKDIV);
+	i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
+		   REG_CLKDIV);
 	clk_disable(i2c->clk);
 
-	t_low_ns = div_u64(((u64)div_low + 1) * 8 * 1000000000, clk_rate);
-	t_high_ns = div_u64(((u64)div_high + 1) * 8 * 1000000000, clk_rate);
+	t_low_ns = div_u64(((u64)calc.div_low + 1) * 8 * 1000000000, clk_rate);
+	t_high_ns = div_u64(((u64)calc.div_high + 1) * 8 * 1000000000,
+			    clk_rate);
 	dev_dbg(i2c->dev,
 		"CLK %lukhz, Req %uns, Act low %lluns high %lluns\n",
 		clk_rate / 1000,
@@ -672,12 +682,11 @@ static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long
 {
 	struct clk_notifier_data *ndata = data;
 	struct rk3x_i2c *i2c = container_of(nb, struct rk3x_i2c, clk_rate_nb);
-	unsigned long div_low, div_high;
+	struct rk3x_i2c_calced_timings calc;
 
 	switch (event) {
 	case PRE_RATE_CHANGE:
-		if (rk3x_i2c_calc_divs(ndata->new_rate, &i2c->t,
-				       &div_low, &div_high) != 0)
+		if (rk3x_i2c_calc_divs(ndata->new_rate, &i2c->t, &calc) != 0)
 			return NOTIFY_STOP;
 
 		/* scale up */
-- 
1.9.1

[PATCH v8 3/8] i2c: rk3x: Remove redundant rk3x_i2c_clean_ipd()

[David Wu]

Call rk3x_i2c_setup() before rk3x_i2c_start()
and the last thing in setup was to clean the IPD,
so no reason to do it at the beginning of start.

Signed-off-by: David Wu <david.wu@rock-chips.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Change in v8:
- none

 drivers/i2c/busses/i2c-rk3x.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 1e2677a..9eeb4e5 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -174,7 +174,6 @@ static void rk3x_i2c_start(struct rk3x_i2c *i2c)
 {
 	u32 val;
 
-	rk3x_i2c_clean_ipd(i2c);
 	i2c_writel(i2c, REG_INT_START, REG_IEN);
 
 	/* enable adapter with correct mode, send START condition */
-- 
1.9.1

[PATCH v8 3/8] i2c: rk3x: Remove redundant rk3x_i2c_clean_ipd()

[David Wu]

Call rk3x_i2c_setup() before rk3x_i2c_start()
and the last thing in setup was to clean the IPD,
so no reason to do it at the beginning of start.

Signed-off-by: David Wu <david.wu@rock-chips.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Change in v8:
- none

 drivers/i2c/busses/i2c-rk3x.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 1e2677a..9eeb4e5 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -174,7 +174,6 @@ static void rk3x_i2c_start(struct rk3x_i2c *i2c)
 {
 	u32 val;
 
-	rk3x_i2c_clean_ipd(i2c);
 	i2c_writel(i2c, REG_INT_START, REG_IEN);
 
 	/* enable adapter with correct mode, send START condition */
-- 
1.9.1

[PATCH v8 4/8] i2c: rk3x: Change SoC data to not use array

[David Wu]

Specifying the i2c SoC data in an array provides very little benefit and
gets unwieldly / confusing as the array grows since the next bit of code
needs to refer to elements in the array by their raw integral index.

Let's just create a single 'static const' structure for each SoC so that
we can refer to these structures by ID.

Signed-off-by: David Wu <david.wu@rock-chips.com>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Suggested-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Change in v8:
- add commit description.

 drivers/i2c/busses/i2c-rk3x.c | 38 ++++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 9eeb4e5..d7a871f 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -864,17 +864,39 @@ static const struct i2c_algorithm rk3x_i2c_algorithm = {
 	.functionality		= rk3x_i2c_func,
 };
 
-static struct rk3x_i2c_soc_data soc_data[3] = {
-	{ .grf_offset = 0x154 }, /* rk3066 */
-	{ .grf_offset = 0x0a4 }, /* rk3188 */
-	{ .grf_offset = -1 },    /* no I2C switching needed */
+static const struct rk3x_i2c_soc_data rk3066_soc_data = {
+	.grf_offset = 0x154,
+};
+
+static const struct rk3x_i2c_soc_data rk3188_soc_data = {
+	.grf_offset = 0x0a4,
+};
+
+static const struct rk3x_i2c_soc_data rk3228_soc_data = {
+	.grf_offset = -1,
+};
+
+static const struct rk3x_i2c_soc_data rk3288_soc_data = {
+	.grf_offset = -1,
 };
 
 static const struct of_device_id rk3x_i2c_match[] = {
-	{ .compatible = "rockchip,rk3066-i2c", .data = (void *)&soc_data[0] },
-	{ .compatible = "rockchip,rk3188-i2c", .data = (void *)&soc_data[1] },
-	{ .compatible = "rockchip,rk3228-i2c", .data = (void *)&soc_data[2] },
-	{ .compatible = "rockchip,rk3288-i2c", .data = (void *)&soc_data[2] },
+	{
+		.compatible = "rockchip,rk3066-i2c",
+		.data = (void *)&rk3066_soc_data
+	},
+	{
+		.compatible = "rockchip,rk3188-i2c",
+		.data = (void *)&rk3188_soc_data
+	},
+	{
+		.compatible = "rockchip,rk3228-i2c",
+		.data = (void *)&rk3228_soc_data
+	},
+	{
+		.compatible = "rockchip,rk3288-i2c",
+		.data = (void *)&rk3288_soc_data
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, rk3x_i2c_match);
-- 
1.9.1

[PATCH v8 4/8] i2c: rk3x: Change SoC data to not use array

[David Wu]

Specifying the i2c SoC data in an array provides very little benefit and
gets unwieldly / confusing as the array grows since the next bit of code
needs to refer to elements in the array by their raw integral index.

Let's just create a single 'static const' structure for each SoC so that
we can refer to these structures by ID.

Signed-off-by: David Wu <david.wu@rock-chips.com>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Suggested-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Change in v8:
- add commit description.

 drivers/i2c/busses/i2c-rk3x.c | 38 ++++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 9eeb4e5..d7a871f 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -864,17 +864,39 @@ static const struct i2c_algorithm rk3x_i2c_algorithm = {
 	.functionality		= rk3x_i2c_func,
 };
 
-static struct rk3x_i2c_soc_data soc_data[3] = {
-	{ .grf_offset = 0x154 }, /* rk3066 */
-	{ .grf_offset = 0x0a4 }, /* rk3188 */
-	{ .grf_offset = -1 },    /* no I2C switching needed */
+static const struct rk3x_i2c_soc_data rk3066_soc_data = {
+	.grf_offset = 0x154,
+};
+
+static const struct rk3x_i2c_soc_data rk3188_soc_data = {
+	.grf_offset = 0x0a4,
+};
+
+static const struct rk3x_i2c_soc_data rk3228_soc_data = {
+	.grf_offset = -1,
+};
+
+static const struct rk3x_i2c_soc_data rk3288_soc_data = {
+	.grf_offset = -1,
 };
 
 static const struct of_device_id rk3x_i2c_match[] = {
-	{ .compatible = "rockchip,rk3066-i2c", .data = (void *)&soc_data[0] },
-	{ .compatible = "rockchip,rk3188-i2c", .data = (void *)&soc_data[1] },
-	{ .compatible = "rockchip,rk3228-i2c", .data = (void *)&soc_data[2] },
-	{ .compatible = "rockchip,rk3288-i2c", .data = (void *)&soc_data[2] },
+	{
+		.compatible = "rockchip,rk3066-i2c",
+		.data = (void *)&rk3066_soc_data
+	},
+	{
+		.compatible = "rockchip,rk3188-i2c",
+		.data = (void *)&rk3188_soc_data
+	},
+	{
+		.compatible = "rockchip,rk3228-i2c",
+		.data = (void *)&rk3228_soc_data
+	},
+	{
+		.compatible = "rockchip,rk3288-i2c",
+		.data = (void *)&rk3288_soc_data
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, rk3x_i2c_match);
-- 
1.9.1

[PATCH v8 5/8] i2c: rk3x: Move spec timing data to "static const" structs

[David Wu]

The i2c timing specs are really just constant data.  There's no reason
to write code to init them, so move them out to structures.  This not
only is a cleaner solution but it will reduce code duplication when we
introduce a new variant of rk3x_i2c_calc_divs() in a future patch.

Signed-off-by: David Wu <david.wu@rock-chips.com>
Suggested-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v8:
- add commit description.
- remove the spec values that were not needed, then
  introduce the additional values in the rk3399 patch. 

 drivers/i2c/busses/i2c-rk3x.c | 83 ++++++++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 28 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index d7a871f..9791797 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -76,6 +76,34 @@ enum {
 #define DEFAULT_SCL_RATE  (100 * 1000) /* Hz */
 
 /**
+ * struct i2c_spec_values:
+ * @min_low_ns: min LOW period of the SCL clock
+ * @min_high_ns: min HIGH period of the SCL cloc
+ * @min_setup_start_ns: min set-up time for a repeated START conditio
+ * @max_data_hold_ns: max data hold time
+ */
+struct i2c_spec_values {
+	unsigned long min_low_ns;
+	unsigned long min_high_ns;
+	unsigned long min_setup_start_ns;
+	unsigned long max_data_hold_ns;
+};
+
+static const struct i2c_spec_values standard_mode_spec = {
+	.min_low_ns = 4700,
+	.min_high_ns = 4000,
+	.min_setup_start_ns = 4700,
+	.max_data_hold_ns = 3450,
+};
+
+static const struct i2c_spec_values fast_mode_spec = {
+	.min_low_ns = 1300,
+	.min_high_ns = 600,
+	.min_setup_start_ns = 600,
+	.max_data_hold_ns = 900,
+};
+
+/**
  * struct rk3x_i2c_calced_timings:
  * @div_low: Divider output for low
  * @div_high: Divider output for high
@@ -460,6 +488,21 @@ out:
 }
 
 /**
+ * Get timing values of I2C specification
+ *
+ * @speed: Desired SCL frequency
+ *
+ * Returns: Matched i2c spec values.
+ */
+static const struct i2c_spec_values *rk3x_i2c_get_spec(unsigned int speed)
+{
+	if (speed <= 100000)
+		return &standard_mode_spec;
+	else
+		return &fast_mode_spec;
+}
+
+/**
  * Calculate divider values for desired SCL frequency
  *
  * @clk_rate: I2C input clock rate
@@ -474,10 +517,6 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 			      struct i2c_timings *t,
 			      struct rk3x_i2c_calced_timings *t_calc)
 {
-	unsigned long spec_min_low_ns, spec_min_high_ns;
-	unsigned long spec_setup_start, 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;
 
@@ -489,6 +528,8 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 	unsigned long min_div_for_hold, min_total_div;
 	unsigned long extra_div, extra_low_div, ideal_low_div;
 
+	unsigned long data_hold_buffer_ns = 50;
+	const struct i2c_spec_values *spec;
 	int ret = 0;
 
 	/* Only support standard-mode and fast-mode */
@@ -511,22 +552,8 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 	 *	 This is because the i2c host on Rockchip holds the data line
 	 *	 for half the low time.
 	 */
-	if (t->bus_freq_hz <= 100000) {
-		/* Standard-mode */
-		spec_min_low_ns = 4700;
-		spec_setup_start = 4700;
-		spec_min_high_ns = 4000;
-		spec_max_data_hold_ns = 3450;
-		data_hold_buffer_ns = 50;
-	} else {
-		/* Fast-mode */
-		spec_min_low_ns = 1300;
-		spec_setup_start = 600;
-		spec_min_high_ns = 600;
-		spec_max_data_hold_ns = 900;
-		data_hold_buffer_ns = 50;
-	}
-	min_high_ns = t->scl_rise_ns + spec_min_high_ns;
+	spec = rk3x_i2c_get_spec(t->bus_freq_hz);
+	min_high_ns = t->scl_rise_ns + spec->min_high_ns;
 
 	/*
 	 * Timings for repeated start:
@@ -536,14 +563,14 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 	 * We need to account for those rules in picking our "high" time so
 	 * we meet tSU;STA and tHD;STA times.
 	 */
-	min_high_ns = max(min_high_ns,
-		DIV_ROUND_UP((t->scl_rise_ns + spec_setup_start) * 1000, 875));
-	min_high_ns = max(min_high_ns,
-		DIV_ROUND_UP((t->scl_rise_ns + spec_setup_start +
-			      t->sda_fall_ns + spec_min_high_ns), 2));
-
-	min_low_ns = t->scl_fall_ns + spec_min_low_ns;
-	max_low_ns = spec_max_data_hold_ns * 2 - data_hold_buffer_ns;
+	min_high_ns = max(min_high_ns, DIV_ROUND_UP(
+		(t->scl_rise_ns + spec->min_setup_start_ns) * 1000, 875));
+	min_high_ns = max(min_high_ns, DIV_ROUND_UP(
+		(t->scl_rise_ns + spec->min_setup_start_ns + t->sda_fall_ns +
+		spec->min_high_ns), 2));
+
+	min_low_ns = t->scl_fall_ns + spec->min_low_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 */
-- 
1.9.1

[PATCH v8 5/8] i2c: rk3x: Move spec timing data to "static const" structs

[David Wu]

The i2c timing specs are really just constant data.  There's no reason
to write code to init them, so move them out to structures.  This not
only is a cleaner solution but it will reduce code duplication when we
introduce a new variant of rk3x_i2c_calc_divs() in a future patch.

Signed-off-by: David Wu <david.wu@rock-chips.com>
Suggested-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v8:
- add commit description.
- remove the spec values that were not needed, then
  introduce the additional values in the rk3399 patch. 

 drivers/i2c/busses/i2c-rk3x.c | 83 ++++++++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 28 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index d7a871f..9791797 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -76,6 +76,34 @@ enum {
 #define DEFAULT_SCL_RATE  (100 * 1000) /* Hz */
 
 /**
+ * struct i2c_spec_values:
+ * @min_low_ns: min LOW period of the SCL clock
+ * @min_high_ns: min HIGH period of the SCL cloc
+ * @min_setup_start_ns: min set-up time for a repeated START conditio
+ * @max_data_hold_ns: max data hold time
+ */
+struct i2c_spec_values {
+	unsigned long min_low_ns;
+	unsigned long min_high_ns;
+	unsigned long min_setup_start_ns;
+	unsigned long max_data_hold_ns;
+};
+
+static const struct i2c_spec_values standard_mode_spec = {
+	.min_low_ns = 4700,
+	.min_high_ns = 4000,
+	.min_setup_start_ns = 4700,
+	.max_data_hold_ns = 3450,
+};
+
+static const struct i2c_spec_values fast_mode_spec = {
+	.min_low_ns = 1300,
+	.min_high_ns = 600,
+	.min_setup_start_ns = 600,
+	.max_data_hold_ns = 900,
+};
+
+/**
  * struct rk3x_i2c_calced_timings:
  * @div_low: Divider output for low
  * @div_high: Divider output for high
@@ -460,6 +488,21 @@ out:
 }
 
 /**
+ * Get timing values of I2C specification
+ *
+ * @speed: Desired SCL frequency
+ *
+ * Returns: Matched i2c spec values.
+ */
+static const struct i2c_spec_values *rk3x_i2c_get_spec(unsigned int speed)
+{
+	if (speed <= 100000)
+		return &standard_mode_spec;
+	else
+		return &fast_mode_spec;
+}
+
+/**
  * Calculate divider values for desired SCL frequency
  *
  * @clk_rate: I2C input clock rate
@@ -474,10 +517,6 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 			      struct i2c_timings *t,
 			      struct rk3x_i2c_calced_timings *t_calc)
 {
-	unsigned long spec_min_low_ns, spec_min_high_ns;
-	unsigned long spec_setup_start, 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;
 
@@ -489,6 +528,8 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 	unsigned long min_div_for_hold, min_total_div;
 	unsigned long extra_div, extra_low_div, ideal_low_div;
 
+	unsigned long data_hold_buffer_ns = 50;
+	const struct i2c_spec_values *spec;
 	int ret = 0;
 
 	/* Only support standard-mode and fast-mode */
@@ -511,22 +552,8 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 	 *	 This is because the i2c host on Rockchip holds the data line
 	 *	 for half the low time.
 	 */
-	if (t->bus_freq_hz <= 100000) {
-		/* Standard-mode */
-		spec_min_low_ns = 4700;
-		spec_setup_start = 4700;
-		spec_min_high_ns = 4000;
-		spec_max_data_hold_ns = 3450;
-		data_hold_buffer_ns = 50;
-	} else {
-		/* Fast-mode */
-		spec_min_low_ns = 1300;
-		spec_setup_start = 600;
-		spec_min_high_ns = 600;
-		spec_max_data_hold_ns = 900;
-		data_hold_buffer_ns = 50;
-	}
-	min_high_ns = t->scl_rise_ns + spec_min_high_ns;
+	spec = rk3x_i2c_get_spec(t->bus_freq_hz);
+	min_high_ns = t->scl_rise_ns + spec->min_high_ns;
 
 	/*
 	 * Timings for repeated start:
@@ -536,14 +563,14 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 	 * We need to account for those rules in picking our "high" time so
 	 * we meet tSU;STA and tHD;STA times.
 	 */
-	min_high_ns = max(min_high_ns,
-		DIV_ROUND_UP((t->scl_rise_ns + spec_setup_start) * 1000, 875));
-	min_high_ns = max(min_high_ns,
-		DIV_ROUND_UP((t->scl_rise_ns + spec_setup_start +
-			      t->sda_fall_ns + spec_min_high_ns), 2));
-
-	min_low_ns = t->scl_fall_ns + spec_min_low_ns;
-	max_low_ns = spec_max_data_hold_ns * 2 - data_hold_buffer_ns;
+	min_high_ns = max(min_high_ns, DIV_ROUND_UP(
+		(t->scl_rise_ns + spec->min_setup_start_ns) * 1000, 875));
+	min_high_ns = max(min_high_ns, DIV_ROUND_UP(
+		(t->scl_rise_ns + spec->min_setup_start_ns + t->sda_fall_ns +
+		spec->min_high_ns), 2));
+
+	min_low_ns = t->scl_fall_ns + spec->min_low_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 */
-- 
1.9.1

[PATCH v8 6/8] dt-bindings: i2c: rk3x: add support for rk3399

[David Wu]

The bus clock and function clock are separated at rk3399,
and others use one clock as the bus clock and function clock.

Signed-off-by: David Wu <david.wu@rock-chips.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Change in v8:
- remove error description.

 Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
index 0b4a85f..5429301 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
@@ -6,10 +6,20 @@ RK3xxx SoCs.
 Required properties :
 
  - reg : Offset and length of the register set for the device
- - compatible : should be "rockchip,rk3066-i2c", "rockchip,rk3188-i2c",
-		"rockchip,rk3228-i2c" or "rockchip,rk3288-i2c".
+ - compatible: should be one of the following:
+   - "rockchip,rk3066-i2c": for rk3066
+   - "rockchip,rk3188-i2c": for rk3188
+   - "rockchip,rk3228-i2c": for rk3228
+   - "rockchip,rk3288-i2c": for rk3288
+   - "rockchip,rk3399-i2c": for rk3399
  - interrupts : interrupt number
- - clocks : parent clock
+ - clocks: See ../clock/clock-bindings.txt
+   - For older hardware (rk3066, rk3188, rk3228, rk3288):
+     - There is one clock that's used both to derive the functional clock
+       for the device and as the bus clock.
+   - For newer hardware (rk3399): specified by name
+     - "i2c": REQUIRED. This is used to derive the functional clock.
+     - "pclk": REQUIRED. This is the bus clock.
 
 Required on RK3066, RK3188 :
 
-- 
1.9.1

[PATCH v8 6/8] dt-bindings: i2c: rk3x: add support for rk3399

[David Wu]

The bus clock and function clock are separated at rk3399,
and others use one clock as the bus clock and function clock.

Signed-off-by: David Wu <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Reviewed-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
Change in v8:
- remove error description.

 Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
index 0b4a85f..5429301 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
@@ -6,10 +6,20 @@ RK3xxx SoCs.
 Required properties :
 
  - reg : Offset and length of the register set for the device
- - compatible : should be "rockchip,rk3066-i2c", "rockchip,rk3188-i2c",
-		"rockchip,rk3228-i2c" or "rockchip,rk3288-i2c".
+ - compatible: should be one of the following:
+   - "rockchip,rk3066-i2c": for rk3066
+   - "rockchip,rk3188-i2c": for rk3188
+   - "rockchip,rk3228-i2c": for rk3228
+   - "rockchip,rk3288-i2c": for rk3288
+   - "rockchip,rk3399-i2c": for rk3399
  - interrupts : interrupt number
- - clocks : parent clock
+ - clocks: See ../clock/clock-bindings.txt
+   - For older hardware (rk3066, rk3188, rk3228, rk3288):
+     - There is one clock that's used both to derive the functional clock
+       for the device and as the bus clock.
+   - For newer hardware (rk3399): specified by name
+     - "i2c": REQUIRED. This is used to derive the functional clock.
+     - "pclk": REQUIRED. This is the bus clock.
 
 Required on RK3066, RK3188 :
 
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v8 6/8] dt-bindings: i2c: rk3x: add support for rk3399

[David Wu]

The bus clock and function clock are separated at rk3399,
and others use one clock as the bus clock and function clock.

Signed-off-by: David Wu <david.wu@rock-chips.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Change in v8:
- remove error description.

 Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
index 0b4a85f..5429301 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
@@ -6,10 +6,20 @@ RK3xxx SoCs.
 Required properties :
 
  - reg : Offset and length of the register set for the device
- - compatible : should be "rockchip,rk3066-i2c", "rockchip,rk3188-i2c",
-		"rockchip,rk3228-i2c" or "rockchip,rk3288-i2c".
+ - compatible: should be one of the following:
+   - "rockchip,rk3066-i2c": for rk3066
+   - "rockchip,rk3188-i2c": for rk3188
+   - "rockchip,rk3228-i2c": for rk3228
+   - "rockchip,rk3288-i2c": for rk3288
+   - "rockchip,rk3399-i2c": for rk3399
  - interrupts : interrupt number
- - clocks : parent clock
+ - clocks: See ../clock/clock-bindings.txt
+   - For older hardware (rk3066, rk3188, rk3228, rk3288):
+     - There is one clock that's used both to derive the functional clock
+       for the device and as the bus clock.
+   - For newer hardware (rk3399): specified by name
+     - "i2c": REQUIRED. This is used to derive the functional clock.
+     - "pclk": REQUIRED. This is the bus clock.
 
 Required on RK3066, RK3188 :
 
-- 
1.9.1

[PATCH v8 7/8] i2c: rk3x: add i2c support for rk3399 soc

[David Wu]

- new method to caculate i2c timings for rk3399:
  There was an timing issue about "repeated start" time at the I2C
  controller of version0, controller appears to drop SDA at .875x (7/8)
  programmed clk high. On version 1 of the controller, the rule(.875x)
  isn't enough to meet tSU;STA
  requirements on 100k's Standard-mode. To resolve this issue,
  sda_update_config, start_setup_config and stop_setup_config for I2C
  timing information are added, new rules are designed to calculate
  the timing information at new v1.
- pclk and function clk are separated at rk3399

Signed-off-by: David Wu <david.wu@rock-chips.com>
---
Changes in v8:
- update tuning in RKI2C_CON register by doing a read-modify-write (Doug)
- new method to use pclk and clk (Doug)

Changes in v7:
- transform into a 9 series patches (Doug)
- drop highspeed with mastercode, and support fast-mode plus (Doug)

 drivers/i2c/busses/i2c-rk3x.c | 289 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 270 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 9791797..25ed1ad 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -58,6 +58,12 @@ enum {
 #define REG_CON_LASTACK   BIT(5) /* 1: send NACK after last received byte */
 #define REG_CON_ACTACK    BIT(6) /* 1: stop if NACK is received */
 
+#define REG_CON_TUNING_MASK GENMASK(15, 8)
+
+#define REG_CON_SDA_CFG(cfg) ((cfg) << 8)
+#define REG_CON_STA_CFG(cfg) ((cfg) << 12)
+#define REG_CON_STO_CFG(cfg) ((cfg) << 14)
+
 /* REG_MRXADDR bits */
 #define REG_MRXADDR_VALID(x) BIT(24 + (x)) /* [x*8+7:x*8] of MRX[R]ADDR valid */
 
@@ -77,40 +83,62 @@ enum {
 
 /**
  * struct i2c_spec_values:
+ * @min_hold_start_ns: min hold time (repeated) START condition
  * @min_low_ns: min LOW period of the SCL clock
  * @min_high_ns: min HIGH period of the SCL cloc
  * @min_setup_start_ns: min set-up time for a repeated START conditio
  * @max_data_hold_ns: max data hold time
+ * @min_data_setup_ns: min data set-up time
+ * @min_setup_stop_ns: min set-up time for STOP condition
+ * @min_hold_buffer_ns: min bus free time between a STOP and
+ * START condition
  */
 struct i2c_spec_values {
+	unsigned long min_hold_start_ns;
 	unsigned long min_low_ns;
 	unsigned long min_high_ns;
 	unsigned long min_setup_start_ns;
 	unsigned long max_data_hold_ns;
+	unsigned long min_data_setup_ns;
+	unsigned long min_setup_stop_ns;
+	unsigned long min_hold_buffer_ns;
 };
 
 static const struct i2c_spec_values standard_mode_spec = {
+	.min_hold_start_ns = 4000,
 	.min_low_ns = 4700,
 	.min_high_ns = 4000,
 	.min_setup_start_ns = 4700,
 	.max_data_hold_ns = 3450,
+	.min_data_setup_ns = 250,
+	.min_setup_stop_ns = 4000,
+	.min_hold_buffer_ns = 4700,
 };
 
 static const struct i2c_spec_values fast_mode_spec = {
+	.min_hold_start_ns = 600,
 	.min_low_ns = 1300,
 	.min_high_ns = 600,
 	.min_setup_start_ns = 600,
 	.max_data_hold_ns = 900,
+	.min_data_setup_ns = 100,
+	.min_setup_stop_ns = 600,
+	.min_hold_buffer_ns = 1300,
 };
 
 /**
  * struct rk3x_i2c_calced_timings:
  * @div_low: Divider output for low
  * @div_high: Divider output for high
+ * @tuning: Used to adjust setup/hold data time,
+ * setup/hold start time and setup stop time for
+ * v1's calc_timings, the tuning should all be 0
+ * for old hardware anyone using v0's calc_timings.
  */
 struct rk3x_i2c_calced_timings {
 	unsigned long div_low;
 	unsigned long div_high;
+	unsigned int tuning;
 };
 
 enum rk3x_i2c_state {
@@ -123,9 +151,12 @@ enum rk3x_i2c_state {
 
 /**
  * @grf_offset: offset inside the grf regmap for setting the i2c type
+ * @calc_timings: Callback function for i2c timing information calculated
  */
 struct rk3x_i2c_soc_data {
 	int grf_offset;
+	int (*calc_timings)(unsigned long, struct i2c_timings *,
+			    struct rk3x_i2c_calced_timings *);
 };
 
 /**
@@ -134,7 +165,8 @@ struct rk3x_i2c_soc_data {
  * @dev: device for this controller
  * @soc_data: related soc data struct
  * @regs: virtual memory area
- * @clk: clock of i2c bus
+ * @clk: function clk for rk3399 or function & Bus clks for others
+ * @pclk: Bus clk for rk3399
  * @clk_rate_nb: i2c clk rate change notify
  * @t: I2C known timing information
  * @lock: spinlock for the i2c bus
@@ -156,6 +188,7 @@ struct rk3x_i2c {
 	/* Hardware resources */
 	void __iomem *regs;
 	struct clk *clk;
+	struct clk *pclk;
 	struct notifier_block clk_rate_nb;
 
 	/* Settings */
@@ -200,12 +233,12 @@ static inline void rk3x_i2c_clean_ipd(struct rk3x_i2c *i2c)
  */
 static void rk3x_i2c_start(struct rk3x_i2c *i2c)
 {
-	u32 val;
+	u32 val = i2c_readl(i2c, REG_CON) & REG_CON_TUNING_MASK;
 
 	i2c_writel(i2c, REG_INT_START, REG_IEN);
 
 	/* enable adapter with correct mode, send START condition */
-	val = REG_CON_EN | REG_CON_MOD(i2c->mode) | REG_CON_START;
+	val |= REG_CON_EN | REG_CON_MOD(i2c->mode) | REG_CON_START;
 
 	/* if we want to react to NACK, set ACTACK bit */
 	if (!(i2c->msg->flags & I2C_M_IGNORE_NAK))
@@ -513,9 +546,9 @@ static const struct i2c_spec_values *rk3x_i2c_get_spec(unsigned int speed)
  * a best-effort divider value is returned in divs. If the target rate is
  * too high, we silently use the highest possible rate.
  */
-static int rk3x_i2c_calc_divs(unsigned long clk_rate,
-			      struct i2c_timings *t,
-			      struct rk3x_i2c_calced_timings *t_calc)
+static int rk3x_i2c_v0_calc_timings(unsigned long clk_rate,
+				    struct i2c_timings *t,
+				    struct rk3x_i2c_calced_timings *t_calc)
 {
 	unsigned long min_low_ns, min_high_ns;
 	unsigned long max_low_ns, min_total_ns;
@@ -661,20 +694,189 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 	return ret;
 }
 
+/**
+ * Calculate timing values for desired SCL frequency
+ *
+ * @clk_rate: I2C input clock rate
+ * @t: Known I2C timing information
+ * @t_calc: Caculated rk3x private timings that would be written into regs
+
+ * Returns: 0 on success, -EINVAL if the goal SCL rate is too slow. In that case
+ * a best-effort divider value is returned in divs. If the target rate is
+ * too high, we silently use the highest possible rate.
+ * The following formulas are v1's method to calculate timings.
+ *
+ * l = divl + 1;
+ * h = divh + 1;
+ * s = sda_update_config + 1;
+ * u = start_setup_config + 1;
+ * p = stop_setup_config + 1;
+ * T = Tclk_i2c;
+
+ * tHigh = 8 * h * T;
+ * tLow = 8 * l * T;
+
+ * tHD;sda = (l * s + 1) * T;
+ * tSU;sda = [(8 - s) * l + 1] * T;
+ * tI2C = 8 * (l + h) * T;
+
+ * tSU;sta = (8h * u + 1) * T;
+ * tHD;sta = [8h * (u + 1) - 1] * T;
+ * tSU;sto = (8h * p + 1) * T;
+ */
+static int rk3x_i2c_v1_calc_timings(unsigned long clk_rate,
+				    struct i2c_timings *t,
+				    struct rk3x_i2c_calced_timings *t_calc)
+{
+	unsigned long min_low_ns, min_high_ns, min_total_ns;
+	unsigned long min_setup_start_ns, min_setup_data_ns;
+	unsigned long min_setup_stop_ns, max_hold_data_ns;
+
+	unsigned long clk_rate_khz, scl_rate_khz;
+
+	unsigned long min_low_div, min_high_div;
+
+	unsigned long min_div_for_hold, min_total_div;
+	unsigned long extra_div, extra_low_div;
+	unsigned long sda_update_cfg, stp_sta_cfg, stp_sto_cfg;
+
+	const struct i2c_spec_values *spec;
+	int ret = 0;
+
+	/* Support standard-mode and fast-mode */
+	if (WARN_ON(t->bus_freq_hz > 400000))
+		t->bus_freq_hz = 400000;
+
+	/* prevent scl_rate_khz from becoming 0 */
+	if (WARN_ON(t->bus_freq_hz < 1000))
+		t->bus_freq_hz = 1000;
+
+	/*
+	 * 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.
+	 */
+	spec = rk3x_i2c_get_spec(t->bus_freq_hz);
+
+	/* calculate min-divh and min-divl */
+	clk_rate_khz = DIV_ROUND_UP(clk_rate, 1000);
+	scl_rate_khz = t->bus_freq_hz / 1000;
+	min_total_div = DIV_ROUND_UP(clk_rate_khz, scl_rate_khz * 8);
+
+	min_high_ns = t->scl_rise_ns + spec->min_high_ns;
+	min_high_div = DIV_ROUND_UP(clk_rate_khz * min_high_ns, 8 * 1000000);
+
+	min_low_ns = t->scl_fall_ns + spec->min_low_ns;
+	min_low_div = DIV_ROUND_UP(clk_rate_khz * min_low_ns, 8 * 1000000);
+
+	/*
+	 * Final divh and divl must be greater than 0, otherwise the
+	 * hardware would not output the i2c clk.
+	 */
+	min_high_div = (min_high_div < 1) ? 2 : min_high_div;
+	min_low_div = (min_low_div < 1) ? 2 : min_low_div;
+
+	/* These are the min dividers needed for min hold times. */
+	min_div_for_hold = (min_low_div + min_high_div);
+	min_total_ns = min_low_ns + min_high_ns;
+
+	/*
+	 * 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.
+	 */
+	 if (min_div_for_hold >= min_total_div) {
+		/*
+		 * Time needed to meet hold requirements is important.
+		 * Just use that.
+		 */
+		t_calc->div_low = min_low_div;
+		t_calc->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.
+		 * We'll try to split things up by the scale of min_low_div and
+		 * min_high_div, biasing slightly towards having a higher div
+		 * for low (spend more time low).
+		 */
+		extra_div = min_total_div - min_div_for_hold;
+		extra_low_div = DIV_ROUND_UP(min_low_div * extra_div,
+					     min_div_for_hold);
+
+		t_calc->div_low = min_low_div + extra_low_div;
+		t_calc->div_high = min_high_div + (extra_div - extra_low_div);
+	}
+
+	/*
+	 * calculate sda data hold count by the rules, data_upd_st:3
+	 * is a appropriate value to reduce calculated times.
+	 */
+	for (sda_update_cfg = 3; sda_update_cfg > 0; sda_update_cfg--) {
+		max_hold_data_ns =  DIV_ROUND_UP((sda_update_cfg
+						 * (t_calc->div_low) + 1)
+						 * 1000000, clk_rate_khz);
+		min_setup_data_ns =  DIV_ROUND_UP(((8 - sda_update_cfg)
+						 * (t_calc->div_low) + 1)
+						 * 1000000, clk_rate_khz);
+		if ((max_hold_data_ns < spec->max_data_hold_ns) &&
+		    (min_setup_data_ns > spec->min_data_setup_ns))
+			break;
+	}
+
+	/* calculate setup start config */
+	min_setup_start_ns = t->scl_rise_ns + spec->min_setup_start_ns;
+	stp_sta_cfg = DIV_ROUND_UP(clk_rate_khz * min_setup_start_ns
+			   - 1000000, 8 * 1000000 * (t_calc->div_high));
+
+	/* calculate setup stop config */
+	min_setup_stop_ns = t->scl_rise_ns + spec->min_setup_stop_ns;
+	stp_sto_cfg = DIV_ROUND_UP(clk_rate_khz * min_setup_stop_ns
+			   - 1000000, 8 * 1000000 * (t_calc->div_high));
+
+	t_calc->tuning = REG_CON_SDA_CFG(--sda_update_cfg) |
+			 REG_CON_STA_CFG(--stp_sta_cfg) |
+			 REG_CON_STO_CFG(--stp_sto_cfg);
+
+	t_calc->div_low--;
+	t_calc->div_high--;
+
+	/* Maximum divider supported by hw is 0xffff */
+	if (t_calc->div_low > 0xffff) {
+		t_calc->div_low = 0xffff;
+		ret = -EINVAL;
+	}
+
+	if (t_calc->div_high > 0xffff) {
+		t_calc->div_high = 0xffff;
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
 static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
 {
 	struct i2c_timings *t = &i2c->t;
 	struct rk3x_i2c_calced_timings calc;
 	u64 t_low_ns, t_high_ns;
+	u32 val;
 	int ret;
 
-	ret = rk3x_i2c_calc_divs(clk_rate, t, &calc);
+	ret = i2c->soc_data->calc_timings(clk_rate, t, &calc);
 	WARN_ONCE(ret != 0, "Could not reach SCL freq %u", t->bus_freq_hz);
 
-	clk_enable(i2c->clk);
+	clk_enable(i2c->pclk);
+
 	i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
 		   REG_CLKDIV);
-	clk_disable(i2c->clk);
+
+	val = i2c_readl(i2c, REG_CON);
+	val &= ~REG_CON_TUNING_MASK;
+	val |= calc.tuning;
+	i2c_writel(i2c, val, REG_CON);
+
+	clk_disable(i2c->pclk);
 
 	t_low_ns = div_u64(((u64)calc.div_low + 1) * 8 * 1000000000, clk_rate);
 	t_high_ns = div_u64(((u64)calc.div_high + 1) * 8 * 1000000000,
@@ -712,7 +914,13 @@ 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->t, &calc) != 0)
+		/*
+		 * Try the calculation (but don't store the result) ahead of
+		 * time to see if we need to block the clock change.  Timings
+		 * shouldn't actually take effect until rk3x_i2c_adapt_div().
+		 */
+		if (i2c->soc_data->calc_timings(ndata->new_rate, &i2c->t,
+						&calc) != 0)
 			return NOTIFY_STOP;
 
 		/* scale up */
@@ -822,12 +1030,14 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
 {
 	struct rk3x_i2c *i2c = (struct rk3x_i2c *)adap->algo_data;
 	unsigned long timeout, flags;
+	u32 val;
 	int ret = 0;
 	int i;
 
 	spin_lock_irqsave(&i2c->lock, flags);
 
 	clk_enable(i2c->clk);
+	clk_enable(i2c->pclk);
 
 	i2c->is_last_msg = false;
 
@@ -861,7 +1071,9 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
 
 			/* Force a STOP condition without interrupt */
 			i2c_writel(i2c, 0, REG_IEN);
-			i2c_writel(i2c, REG_CON_EN | REG_CON_STOP, REG_CON);
+			val = i2c_readl(i2c, REG_CON) & REG_CON_TUNING_MASK;
+			val |= REG_CON_EN | REG_CON_STOP;
+			i2c_writel(i2c, val, REG_CON);
 
 			i2c->state = STATE_IDLE;
 
@@ -875,7 +1087,9 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
 		}
 	}
 
+	clk_disable(i2c->pclk);
 	clk_disable(i2c->clk);
+
 	spin_unlock_irqrestore(&i2c->lock, flags);
 
 	return ret < 0 ? ret : num;
@@ -893,18 +1107,27 @@ static const struct i2c_algorithm rk3x_i2c_algorithm = {
 
 static const struct rk3x_i2c_soc_data rk3066_soc_data = {
 	.grf_offset = 0x154,
+	.calc_timings = rk3x_i2c_v0_calc_timings,
 };
 
 static const struct rk3x_i2c_soc_data rk3188_soc_data = {
 	.grf_offset = 0x0a4,
+	.calc_timings = rk3x_i2c_v0_calc_timings,
 };
 
 static const struct rk3x_i2c_soc_data rk3228_soc_data = {
 	.grf_offset = -1,
+	.calc_timings = rk3x_i2c_v0_calc_timings,
 };
 
 static const struct rk3x_i2c_soc_data rk3288_soc_data = {
 	.grf_offset = -1,
+	.calc_timings = rk3x_i2c_v0_calc_timings,
+};
+
+static const struct rk3x_i2c_soc_data rk3399_soc_data = {
+	.grf_offset = -1,
+	.calc_timings = rk3x_i2c_v1_calc_timings,
 };
 
 static const struct of_device_id rk3x_i2c_match[] = {
@@ -924,6 +1147,10 @@ static const struct of_device_id rk3x_i2c_match[] = {
 		.compatible = "rockchip,rk3288-i2c",
 		.data = (void *)&rk3288_soc_data
 	},
+	{
+		.compatible = "rockchip,rk3399-i2c",
+		.data = (void *)&rk3399_soc_data
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, rk3x_i2c_match);
@@ -963,12 +1190,6 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
 	spin_lock_init(&i2c->lock);
 	init_waitqueue_head(&i2c->wait);
 
-	i2c->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(i2c->clk)) {
-		dev_err(&pdev->dev, "cannot get clock\n");
-		return PTR_ERR(i2c->clk);
-	}
-
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	i2c->regs = devm_ioremap_resource(&pdev->dev, mem);
 	if (IS_ERR(i2c->regs))
@@ -1022,17 +1243,44 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, i2c);
 
+	if (i2c->soc_data->calc_timings == rk3x_i2c_v0_calc_timings) {
+		/* Only one clock to use for bus clock and peripheral clock */
+		i2c->clk = devm_clk_get(&pdev->dev, NULL);
+		i2c->pclk = i2c->clk;
+	} else {
+		i2c->clk = devm_clk_get(&pdev->dev, "i2c");
+		i2c->pclk = devm_clk_get(&pdev->dev, "pclk");
+	}
+
+	if (IS_ERR(i2c->clk)) {
+		ret = PTR_ERR(i2c->clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Can't get bus clk: %d\n", ret);
+		return ret;
+	}
+	if (IS_ERR(i2c->pclk)) {
+		ret = PTR_ERR(i2c->pclk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Can't get periph clk: %d\n", ret);
+		return ret;
+	}
+
 	ret = clk_prepare(i2c->clk);
 	if (ret < 0) {
-		dev_err(&pdev->dev, "Could not prepare clock\n");
+		dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);
 		return ret;
 	}
+	ret = clk_prepare(i2c->pclk);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Can't prepare periph clock: %d\n", ret);
+		goto err_clk;
+	}
 
 	i2c->clk_rate_nb.notifier_call = rk3x_i2c_clk_notifier_cb;
 	ret = clk_notifier_register(i2c->clk, &i2c->clk_rate_nb);
 	if (ret != 0) {
 		dev_err(&pdev->dev, "Unable to register clock notifier\n");
-		goto err_clk;
+		goto err_pclk;
 	}
 
 	clk_rate = clk_get_rate(i2c->clk);
@@ -1050,6 +1298,8 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
 
 err_clk_notifier:
 	clk_notifier_unregister(i2c->clk, &i2c->clk_rate_nb);
+err_pclk:
+	clk_unprepare(i2c->pclk);
 err_clk:
 	clk_unprepare(i2c->clk);
 	return ret;
@@ -1062,6 +1312,7 @@ static int rk3x_i2c_remove(struct platform_device *pdev)
 	i2c_del_adapter(&i2c->adap);
 
 	clk_notifier_unregister(i2c->clk, &i2c->clk_rate_nb);
+	clk_unprepare(i2c->pclk);
 	clk_unprepare(i2c->clk);
 
 	return 0;
-- 
1.9.1

[PATCH v8 7/8] i2c: rk3x: add i2c support for rk3399 soc

[David Wu]

- new method to caculate i2c timings for rk3399:
  There was an timing issue about "repeated start" time at the I2C
  controller of version0, controller appears to drop SDA at .875x (7/8)
  programmed clk high. On version 1 of the controller, the rule(.875x)
  isn't enough to meet tSU;STA
  requirements on 100k's Standard-mode. To resolve this issue,
  sda_update_config, start_setup_config and stop_setup_config for I2C
  timing information are added, new rules are designed to calculate
  the timing information at new v1.
- pclk and function clk are separated at rk3399

Signed-off-by: David Wu <david.wu@rock-chips.com>
---
Changes in v8:
- update tuning in RKI2C_CON register by doing a read-modify-write (Doug)
- new method to use pclk and clk (Doug)

Changes in v7:
- transform into a 9 series patches (Doug)
- drop highspeed with mastercode, and support fast-mode plus (Doug)

 drivers/i2c/busses/i2c-rk3x.c | 289 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 270 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 9791797..25ed1ad 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -58,6 +58,12 @@ enum {
 #define REG_CON_LASTACK   BIT(5) /* 1: send NACK after last received byte */
 #define REG_CON_ACTACK    BIT(6) /* 1: stop if NACK is received */
 
+#define REG_CON_TUNING_MASK GENMASK(15, 8)
+
+#define REG_CON_SDA_CFG(cfg) ((cfg) << 8)
+#define REG_CON_STA_CFG(cfg) ((cfg) << 12)
+#define REG_CON_STO_CFG(cfg) ((cfg) << 14)
+
 /* REG_MRXADDR bits */
 #define REG_MRXADDR_VALID(x) BIT(24 + (x)) /* [x*8+7:x*8] of MRX[R]ADDR valid */
 
@@ -77,40 +83,62 @@ enum {
 
 /**
  * struct i2c_spec_values:
+ * @min_hold_start_ns: min hold time (repeated) START condition
  * @min_low_ns: min LOW period of the SCL clock
  * @min_high_ns: min HIGH period of the SCL cloc
  * @min_setup_start_ns: min set-up time for a repeated START conditio
  * @max_data_hold_ns: max data hold time
+ * @min_data_setup_ns: min data set-up time
+ * @min_setup_stop_ns: min set-up time for STOP condition
+ * @min_hold_buffer_ns: min bus free time between a STOP and
+ * START condition
  */
 struct i2c_spec_values {
+	unsigned long min_hold_start_ns;
 	unsigned long min_low_ns;
 	unsigned long min_high_ns;
 	unsigned long min_setup_start_ns;
 	unsigned long max_data_hold_ns;
+	unsigned long min_data_setup_ns;
+	unsigned long min_setup_stop_ns;
+	unsigned long min_hold_buffer_ns;
 };
 
 static const struct i2c_spec_values standard_mode_spec = {
+	.min_hold_start_ns = 4000,
 	.min_low_ns = 4700,
 	.min_high_ns = 4000,
 	.min_setup_start_ns = 4700,
 	.max_data_hold_ns = 3450,
+	.min_data_setup_ns = 250,
+	.min_setup_stop_ns = 4000,
+	.min_hold_buffer_ns = 4700,
 };
 
 static const struct i2c_spec_values fast_mode_spec = {
+	.min_hold_start_ns = 600,
 	.min_low_ns = 1300,
 	.min_high_ns = 600,
 	.min_setup_start_ns = 600,
 	.max_data_hold_ns = 900,
+	.min_data_setup_ns = 100,
+	.min_setup_stop_ns = 600,
+	.min_hold_buffer_ns = 1300,
 };
 
 /**
  * struct rk3x_i2c_calced_timings:
  * @div_low: Divider output for low
  * @div_high: Divider output for high
+ * @tuning: Used to adjust setup/hold data time,
+ * setup/hold start time and setup stop time for
+ * v1's calc_timings, the tuning should all be 0
+ * for old hardware anyone using v0's calc_timings.
  */
 struct rk3x_i2c_calced_timings {
 	unsigned long div_low;
 	unsigned long div_high;
+	unsigned int tuning;
 };
 
 enum rk3x_i2c_state {
@@ -123,9 +151,12 @@ enum rk3x_i2c_state {
 
 /**
  * @grf_offset: offset inside the grf regmap for setting the i2c type
+ * @calc_timings: Callback function for i2c timing information calculated
  */
 struct rk3x_i2c_soc_data {
 	int grf_offset;
+	int (*calc_timings)(unsigned long, struct i2c_timings *,
+			    struct rk3x_i2c_calced_timings *);
 };
 
 /**
@@ -134,7 +165,8 @@ struct rk3x_i2c_soc_data {
  * @dev: device for this controller
  * @soc_data: related soc data struct
  * @regs: virtual memory area
- * @clk: clock of i2c bus
+ * @clk: function clk for rk3399 or function & Bus clks for others
+ * @pclk: Bus clk for rk3399
  * @clk_rate_nb: i2c clk rate change notify
  * @t: I2C known timing information
  * @lock: spinlock for the i2c bus
@@ -156,6 +188,7 @@ struct rk3x_i2c {
 	/* Hardware resources */
 	void __iomem *regs;
 	struct clk *clk;
+	struct clk *pclk;
 	struct notifier_block clk_rate_nb;
 
 	/* Settings */
@@ -200,12 +233,12 @@ static inline void rk3x_i2c_clean_ipd(struct rk3x_i2c *i2c)
  */
 static void rk3x_i2c_start(struct rk3x_i2c *i2c)
 {
-	u32 val;
+	u32 val = i2c_readl(i2c, REG_CON) & REG_CON_TUNING_MASK;
 
 	i2c_writel(i2c, REG_INT_START, REG_IEN);
 
 	/* enable adapter with correct mode, send START condition */
-	val = REG_CON_EN | REG_CON_MOD(i2c->mode) | REG_CON_START;
+	val |= REG_CON_EN | REG_CON_MOD(i2c->mode) | REG_CON_START;
 
 	/* if we want to react to NACK, set ACTACK bit */
 	if (!(i2c->msg->flags & I2C_M_IGNORE_NAK))
@@ -513,9 +546,9 @@ static const struct i2c_spec_values *rk3x_i2c_get_spec(unsigned int speed)
  * a best-effort divider value is returned in divs. If the target rate is
  * too high, we silently use the highest possible rate.
  */
-static int rk3x_i2c_calc_divs(unsigned long clk_rate,
-			      struct i2c_timings *t,
-			      struct rk3x_i2c_calced_timings *t_calc)
+static int rk3x_i2c_v0_calc_timings(unsigned long clk_rate,
+				    struct i2c_timings *t,
+				    struct rk3x_i2c_calced_timings *t_calc)
 {
 	unsigned long min_low_ns, min_high_ns;
 	unsigned long max_low_ns, min_total_ns;
@@ -661,20 +694,189 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
 	return ret;
 }
 
+/**
+ * Calculate timing values for desired SCL frequency
+ *
+ * @clk_rate: I2C input clock rate
+ * @t: Known I2C timing information
+ * @t_calc: Caculated rk3x private timings that would be written into regs
+
+ * Returns: 0 on success, -EINVAL if the goal SCL rate is too slow. In that case
+ * a best-effort divider value is returned in divs. If the target rate is
+ * too high, we silently use the highest possible rate.
+ * The following formulas are v1's method to calculate timings.
+ *
+ * l = divl + 1;
+ * h = divh + 1;
+ * s = sda_update_config + 1;
+ * u = start_setup_config + 1;
+ * p = stop_setup_config + 1;
+ * T = Tclk_i2c;
+
+ * tHigh = 8 * h * T;
+ * tLow = 8 * l * T;
+
+ * tHD;sda = (l * s + 1) * T;
+ * tSU;sda = [(8 - s) * l + 1] * T;
+ * tI2C = 8 * (l + h) * T;
+
+ * tSU;sta = (8h * u + 1) * T;
+ * tHD;sta = [8h * (u + 1) - 1] * T;
+ * tSU;sto = (8h * p + 1) * T;
+ */
+static int rk3x_i2c_v1_calc_timings(unsigned long clk_rate,
+				    struct i2c_timings *t,
+				    struct rk3x_i2c_calced_timings *t_calc)
+{
+	unsigned long min_low_ns, min_high_ns, min_total_ns;
+	unsigned long min_setup_start_ns, min_setup_data_ns;
+	unsigned long min_setup_stop_ns, max_hold_data_ns;
+
+	unsigned long clk_rate_khz, scl_rate_khz;
+
+	unsigned long min_low_div, min_high_div;
+
+	unsigned long min_div_for_hold, min_total_div;
+	unsigned long extra_div, extra_low_div;
+	unsigned long sda_update_cfg, stp_sta_cfg, stp_sto_cfg;
+
+	const struct i2c_spec_values *spec;
+	int ret = 0;
+
+	/* Support standard-mode and fast-mode */
+	if (WARN_ON(t->bus_freq_hz > 400000))
+		t->bus_freq_hz = 400000;
+
+	/* prevent scl_rate_khz from becoming 0 */
+	if (WARN_ON(t->bus_freq_hz < 1000))
+		t->bus_freq_hz = 1000;
+
+	/*
+	 * 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.
+	 */
+	spec = rk3x_i2c_get_spec(t->bus_freq_hz);
+
+	/* calculate min-divh and min-divl */
+	clk_rate_khz = DIV_ROUND_UP(clk_rate, 1000);
+	scl_rate_khz = t->bus_freq_hz / 1000;
+	min_total_div = DIV_ROUND_UP(clk_rate_khz, scl_rate_khz * 8);
+
+	min_high_ns = t->scl_rise_ns + spec->min_high_ns;
+	min_high_div = DIV_ROUND_UP(clk_rate_khz * min_high_ns, 8 * 1000000);
+
+	min_low_ns = t->scl_fall_ns + spec->min_low_ns;
+	min_low_div = DIV_ROUND_UP(clk_rate_khz * min_low_ns, 8 * 1000000);
+
+	/*
+	 * Final divh and divl must be greater than 0, otherwise the
+	 * hardware would not output the i2c clk.
+	 */
+	min_high_div = (min_high_div < 1) ? 2 : min_high_div;
+	min_low_div = (min_low_div < 1) ? 2 : min_low_div;
+
+	/* These are the min dividers needed for min hold times. */
+	min_div_for_hold = (min_low_div + min_high_div);
+	min_total_ns = min_low_ns + min_high_ns;
+
+	/*
+	 * 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.
+	 */
+	 if (min_div_for_hold >= min_total_div) {
+		/*
+		 * Time needed to meet hold requirements is important.
+		 * Just use that.
+		 */
+		t_calc->div_low = min_low_div;
+		t_calc->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.
+		 * We'll try to split things up by the scale of min_low_div and
+		 * min_high_div, biasing slightly towards having a higher div
+		 * for low (spend more time low).
+		 */
+		extra_div = min_total_div - min_div_for_hold;
+		extra_low_div = DIV_ROUND_UP(min_low_div * extra_div,
+					     min_div_for_hold);
+
+		t_calc->div_low = min_low_div + extra_low_div;
+		t_calc->div_high = min_high_div + (extra_div - extra_low_div);
+	}
+
+	/*
+	 * calculate sda data hold count by the rules, data_upd_st:3
+	 * is a appropriate value to reduce calculated times.
+	 */
+	for (sda_update_cfg = 3; sda_update_cfg > 0; sda_update_cfg--) {
+		max_hold_data_ns =  DIV_ROUND_UP((sda_update_cfg
+						 * (t_calc->div_low) + 1)
+						 * 1000000, clk_rate_khz);
+		min_setup_data_ns =  DIV_ROUND_UP(((8 - sda_update_cfg)
+						 * (t_calc->div_low) + 1)
+						 * 1000000, clk_rate_khz);
+		if ((max_hold_data_ns < spec->max_data_hold_ns) &&
+		    (min_setup_data_ns > spec->min_data_setup_ns))
+			break;
+	}
+
+	/* calculate setup start config */
+	min_setup_start_ns = t->scl_rise_ns + spec->min_setup_start_ns;
+	stp_sta_cfg = DIV_ROUND_UP(clk_rate_khz * min_setup_start_ns
+			   - 1000000, 8 * 1000000 * (t_calc->div_high));
+
+	/* calculate setup stop config */
+	min_setup_stop_ns = t->scl_rise_ns + spec->min_setup_stop_ns;
+	stp_sto_cfg = DIV_ROUND_UP(clk_rate_khz * min_setup_stop_ns
+			   - 1000000, 8 * 1000000 * (t_calc->div_high));
+
+	t_calc->tuning = REG_CON_SDA_CFG(--sda_update_cfg) |
+			 REG_CON_STA_CFG(--stp_sta_cfg) |
+			 REG_CON_STO_CFG(--stp_sto_cfg);
+
+	t_calc->div_low--;
+	t_calc->div_high--;
+
+	/* Maximum divider supported by hw is 0xffff */
+	if (t_calc->div_low > 0xffff) {
+		t_calc->div_low = 0xffff;
+		ret = -EINVAL;
+	}
+
+	if (t_calc->div_high > 0xffff) {
+		t_calc->div_high = 0xffff;
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
 static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
 {
 	struct i2c_timings *t = &i2c->t;
 	struct rk3x_i2c_calced_timings calc;
 	u64 t_low_ns, t_high_ns;
+	u32 val;
 	int ret;
 
-	ret = rk3x_i2c_calc_divs(clk_rate, t, &calc);
+	ret = i2c->soc_data->calc_timings(clk_rate, t, &calc);
 	WARN_ONCE(ret != 0, "Could not reach SCL freq %u", t->bus_freq_hz);
 
-	clk_enable(i2c->clk);
+	clk_enable(i2c->pclk);
+
 	i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
 		   REG_CLKDIV);
-	clk_disable(i2c->clk);
+
+	val = i2c_readl(i2c, REG_CON);
+	val &= ~REG_CON_TUNING_MASK;
+	val |= calc.tuning;
+	i2c_writel(i2c, val, REG_CON);
+
+	clk_disable(i2c->pclk);
 
 	t_low_ns = div_u64(((u64)calc.div_low + 1) * 8 * 1000000000, clk_rate);
 	t_high_ns = div_u64(((u64)calc.div_high + 1) * 8 * 1000000000,
@@ -712,7 +914,13 @@ 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->t, &calc) != 0)
+		/*
+		 * Try the calculation (but don't store the result) ahead of
+		 * time to see if we need to block the clock change.  Timings
+		 * shouldn't actually take effect until rk3x_i2c_adapt_div().
+		 */
+		if (i2c->soc_data->calc_timings(ndata->new_rate, &i2c->t,
+						&calc) != 0)
 			return NOTIFY_STOP;
 
 		/* scale up */
@@ -822,12 +1030,14 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
 {
 	struct rk3x_i2c *i2c = (struct rk3x_i2c *)adap->algo_data;
 	unsigned long timeout, flags;
+	u32 val;
 	int ret = 0;
 	int i;
 
 	spin_lock_irqsave(&i2c->lock, flags);
 
 	clk_enable(i2c->clk);
+	clk_enable(i2c->pclk);
 
 	i2c->is_last_msg = false;
 
@@ -861,7 +1071,9 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
 
 			/* Force a STOP condition without interrupt */
 			i2c_writel(i2c, 0, REG_IEN);
-			i2c_writel(i2c, REG_CON_EN | REG_CON_STOP, REG_CON);
+			val = i2c_readl(i2c, REG_CON) & REG_CON_TUNING_MASK;
+			val |= REG_CON_EN | REG_CON_STOP;
+			i2c_writel(i2c, val, REG_CON);
 
 			i2c->state = STATE_IDLE;
 
@@ -875,7 +1087,9 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
 		}
 	}
 
+	clk_disable(i2c->pclk);
 	clk_disable(i2c->clk);
+
 	spin_unlock_irqrestore(&i2c->lock, flags);
 
 	return ret < 0 ? ret : num;
@@ -893,18 +1107,27 @@ static const struct i2c_algorithm rk3x_i2c_algorithm = {
 
 static const struct rk3x_i2c_soc_data rk3066_soc_data = {
 	.grf_offset = 0x154,
+	.calc_timings = rk3x_i2c_v0_calc_timings,
 };
 
 static const struct rk3x_i2c_soc_data rk3188_soc_data = {
 	.grf_offset = 0x0a4,
+	.calc_timings = rk3x_i2c_v0_calc_timings,
 };
 
 static const struct rk3x_i2c_soc_data rk3228_soc_data = {
 	.grf_offset = -1,
+	.calc_timings = rk3x_i2c_v0_calc_timings,
 };
 
 static const struct rk3x_i2c_soc_data rk3288_soc_data = {
 	.grf_offset = -1,
+	.calc_timings = rk3x_i2c_v0_calc_timings,
+};
+
+static const struct rk3x_i2c_soc_data rk3399_soc_data = {
+	.grf_offset = -1,
+	.calc_timings = rk3x_i2c_v1_calc_timings,
 };
 
 static const struct of_device_id rk3x_i2c_match[] = {
@@ -924,6 +1147,10 @@ static const struct of_device_id rk3x_i2c_match[] = {
 		.compatible = "rockchip,rk3288-i2c",
 		.data = (void *)&rk3288_soc_data
 	},
+	{
+		.compatible = "rockchip,rk3399-i2c",
+		.data = (void *)&rk3399_soc_data
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, rk3x_i2c_match);
@@ -963,12 +1190,6 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
 	spin_lock_init(&i2c->lock);
 	init_waitqueue_head(&i2c->wait);
 
-	i2c->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(i2c->clk)) {
-		dev_err(&pdev->dev, "cannot get clock\n");
-		return PTR_ERR(i2c->clk);
-	}
-
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	i2c->regs = devm_ioremap_resource(&pdev->dev, mem);
 	if (IS_ERR(i2c->regs))
@@ -1022,17 +1243,44 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, i2c);
 
+	if (i2c->soc_data->calc_timings == rk3x_i2c_v0_calc_timings) {
+		/* Only one clock to use for bus clock and peripheral clock */
+		i2c->clk = devm_clk_get(&pdev->dev, NULL);
+		i2c->pclk = i2c->clk;
+	} else {
+		i2c->clk = devm_clk_get(&pdev->dev, "i2c");
+		i2c->pclk = devm_clk_get(&pdev->dev, "pclk");
+	}
+
+	if (IS_ERR(i2c->clk)) {
+		ret = PTR_ERR(i2c->clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Can't get bus clk: %d\n", ret);
+		return ret;
+	}
+	if (IS_ERR(i2c->pclk)) {
+		ret = PTR_ERR(i2c->pclk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Can't get periph clk: %d\n", ret);
+		return ret;
+	}
+
 	ret = clk_prepare(i2c->clk);
 	if (ret < 0) {
-		dev_err(&pdev->dev, "Could not prepare clock\n");
+		dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);
 		return ret;
 	}
+	ret = clk_prepare(i2c->pclk);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Can't prepare periph clock: %d\n", ret);
+		goto err_clk;
+	}
 
 	i2c->clk_rate_nb.notifier_call = rk3x_i2c_clk_notifier_cb;
 	ret = clk_notifier_register(i2c->clk, &i2c->clk_rate_nb);
 	if (ret != 0) {
 		dev_err(&pdev->dev, "Unable to register clock notifier\n");
-		goto err_clk;
+		goto err_pclk;
 	}
 
 	clk_rate = clk_get_rate(i2c->clk);
@@ -1050,6 +1298,8 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
 
 err_clk_notifier:
 	clk_notifier_unregister(i2c->clk, &i2c->clk_rate_nb);
+err_pclk:
+	clk_unprepare(i2c->pclk);
 err_clk:
 	clk_unprepare(i2c->clk);
 	return ret;
@@ -1062,6 +1312,7 @@ static int rk3x_i2c_remove(struct platform_device *pdev)
 	i2c_del_adapter(&i2c->adap);
 
 	clk_notifier_unregister(i2c->clk, &i2c->clk_rate_nb);
+	clk_unprepare(i2c->pclk);
 	clk_unprepare(i2c->clk);
 
 	return 0;
-- 
1.9.1

[PATCH v8 8/8] i2c: rk3x: support fast-mode plus for rk3399

[David Wu]

Signed-off-by: David Wu <david.wu@rock-chips.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Change in v8:
- None

 drivers/i2c/busses/i2c-rk3x.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 25ed1ad..0ba25ee 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -126,6 +126,17 @@ static const struct i2c_spec_values fast_mode_spec = {
 	.min_hold_buffer_ns = 1300,
 };
 
+static const struct i2c_spec_values fast_mode_plus_spec = {
+	.min_hold_start_ns = 260,
+	.min_low_ns = 500,
+	.min_high_ns = 260,
+	.min_setup_start_ns = 260,
+	.max_data_hold_ns = 400,
+	.min_data_setup_ns = 50,
+	.min_setup_stop_ns = 260,
+	.min_hold_buffer_ns = 500,
+};
+
 /**
  * struct rk3x_i2c_calced_timings:
  * @div_low: Divider output for low
@@ -531,8 +542,10 @@ static const struct i2c_spec_values *rk3x_i2c_get_spec(unsigned int speed)
 {
 	if (speed <= 100000)
 		return &standard_mode_spec;
-	else
+	else if (speed <= 400000)
 		return &fast_mode_spec;
+	else
+		return &fast_mode_plus_spec;
 }
 
 /**
@@ -743,9 +756,9 @@ static int rk3x_i2c_v1_calc_timings(unsigned long clk_rate,
 	const struct i2c_spec_values *spec;
 	int ret = 0;
 
-	/* Support standard-mode and fast-mode */
-	if (WARN_ON(t->bus_freq_hz > 400000))
-		t->bus_freq_hz = 400000;
+	/* Support standard-mode, fast-mode and fast-mode plus */
+	if (WARN_ON(t->bus_freq_hz > 1000000))
+		t->bus_freq_hz = 1000000;
 
 	/* prevent scl_rate_khz from becoming 0 */
 	if (WARN_ON(t->bus_freq_hz < 1000))
-- 
1.9.1

[PATCH v8 8/8] i2c: rk3x: support fast-mode plus for rk3399

[David Wu]

Signed-off-by: David Wu <david.wu@rock-chips.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Change in v8:
- None

 drivers/i2c/busses/i2c-rk3x.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 25ed1ad..0ba25ee 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -126,6 +126,17 @@ static const struct i2c_spec_values fast_mode_spec = {
 	.min_hold_buffer_ns = 1300,
 };
 
+static const struct i2c_spec_values fast_mode_plus_spec = {
+	.min_hold_start_ns = 260,
+	.min_low_ns = 500,
+	.min_high_ns = 260,
+	.min_setup_start_ns = 260,
+	.max_data_hold_ns = 400,
+	.min_data_setup_ns = 50,
+	.min_setup_stop_ns = 260,
+	.min_hold_buffer_ns = 500,
+};
+
 /**
  * struct rk3x_i2c_calced_timings:
  * @div_low: Divider output for low
@@ -531,8 +542,10 @@ static const struct i2c_spec_values *rk3x_i2c_get_spec(unsigned int speed)
 {
 	if (speed <= 100000)
 		return &standard_mode_spec;
-	else
+	else if (speed <= 400000)
 		return &fast_mode_spec;
+	else
+		return &fast_mode_plus_spec;
 }
 
 /**
@@ -743,9 +756,9 @@ static int rk3x_i2c_v1_calc_timings(unsigned long clk_rate,
 	const struct i2c_spec_values *spec;
 	int ret = 0;
 
-	/* Support standard-mode and fast-mode */
-	if (WARN_ON(t->bus_freq_hz > 400000))
-		t->bus_freq_hz = 400000;
+	/* Support standard-mode, fast-mode and fast-mode plus */
+	if (WARN_ON(t->bus_freq_hz > 1000000))
+		t->bus_freq_hz = 1000000;
 
 	/* prevent scl_rate_khz from becoming 0 */
 	if (WARN_ON(t->bus_freq_hz < 1000))
-- 
1.9.1

Re: [PATCH v8 7/8] i2c: rk3x: add i2c support for rk3399 soc

[Caesar Wang]

在 2016年05月11日 03:31, David Wu 写道:
> - new method to caculate i2c timings for rk3399:
>    There was an timing issue about "repeated start" time at the I2C
>    controller of version0, controller appears to drop SDA at .875x (7/8)
>    programmed clk high. On version 1 of the controller, the rule(.875x)
>    isn't enough to meet tSU;STA
>    requirements on 100k's Standard-mode. To resolve this issue,
>    sda_update_config, start_setup_config and stop_setup_config for I2C
>    timing information are added, new rules are designed to calculate
>    the timing information at new v1.
> - pclk and function clk are separated at rk3399
>
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
> Changes in v8:
> - update tuning in RKI2C_CON register by doing a read-modify-write (Doug)
> - new method to use pclk and clk (Doug)
>
> Changes in v7:
> - transform into a 9 series patches (Doug)
> - drop highspeed with mastercode, and support fast-mode plus (Doug)

Tested-by: Caesar Wang <wxt@rock-chips.com>
Few comments below:


I picked David's patches up for my rk3399 evb board.
Add the needed i2c devices for dts.

localhost / #modprobe i2c-dev
localhost / # i2cdump -f -y 0 0x1b
No size specified (using byte-data access)
      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f 0123456789abcdef
00: 18 01 09 21 01 13 01 00 00 00 00 01 01 00 00 00 ???!???....??...
10: 80 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ??..............
20: 00 5f 00 6f ff 00 00 00 10 00 ff 0f ff 02 01 0f ._.o....?..?.???
30: 00 00 01 0f 00 00 02 03 00 00 09 00 00 0c 00 0a ..??..??..?..?.?
40: 00 0c 00 0c 00 07 00 0a 00 0c 00 00 00 5f 00 03 .?.?.?.?.?..._.?
50: 06 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ??..............
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
70: 00 cf 03 00 28 00 0c 1c 80 19 00 34 12 00 71 00 .??.(.????.4?.q.
80: 10 50 1f ac 00 40 00 49 00 00 c0 44 41 01 00 00 ?P??.@.I..?DA?..
90: 55 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 U...............
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
localhost / # i2cset -f -y 0 0x1b 0x90 0xff
localhost / # i2cdump -f -y 0 0x1b |grep 90
  No size specified (using byte-data access)
90: ff 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................

The i2c devices are working for my board.

run:
modprobe i2c-dev; for i in $(seq 0 8); do echo ==BUS $i; i2cdetect -y -q 
$i; done
See the output message,and mesure the i2c frequency.





>
>   drivers/i2c/busses/i2c-rk3x.c | 289 +++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 270 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 9791797..25ed1ad 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -58,6 +58,12 @@ enum {
>   #define REG_CON_LASTACK   BIT(5) /* 1: send NACK after last received byte */
>   #define REG_CON_ACTACK    BIT(6) /* 1: stop if NACK is received */
>   
> +#define REG_CON_TUNING_MASK GENMASK(15, 8)
> +
> +#define REG_CON_SDA_CFG(cfg) ((cfg) << 8)
> +#define REG_CON_STA_CFG(cfg) ((cfg) << 12)
> +#define REG_CON_STO_CFG(cfg) ((cfg) << 14)
> +
>   /* REG_MRXADDR bits */
>   #define REG_MRXADDR_VALID(x) BIT(24 + (x)) /* [x*8+7:x*8] of MRX[R]ADDR valid */
>   
> @@ -77,40 +83,62 @@ enum {
>   
>   /**
>    * struct i2c_spec_values:
> + * @min_hold_start_ns: min hold time (repeated) START condition
>    * @min_low_ns: min LOW period of the SCL clock
>    * @min_high_ns: min HIGH period of the SCL cloc
>    * @min_setup_start_ns: min set-up time for a repeated START conditio
>    * @max_data_hold_ns: max data hold time
> + * @min_data_setup_ns: min data set-up time
> + * @min_setup_stop_ns: min set-up time for STOP condition
> + * @min_hold_buffer_ns: min bus free time between a STOP and
> + * START condition
>    */
>   struct i2c_spec_values {
> +	unsigned long min_hold_start_ns;
>   	unsigned long min_low_ns;
>   	unsigned long min_high_ns;
>   	unsigned long min_setup_start_ns;
>   	unsigned long max_data_hold_ns;
> +	unsigned long min_data_setup_ns;
> +	unsigned long min_setup_stop_ns;
> +	unsigned long min_hold_buffer_ns;
>   };
>   
>   static const struct i2c_spec_values standard_mode_spec = {
> +	.min_hold_start_ns = 4000,
>   	.min_low_ns = 4700,
>   	.min_high_ns = 4000,
>   	.min_setup_start_ns = 4700,
>   	.max_data_hold_ns = 3450,
> +	.min_data_setup_ns = 250,
> +	.min_setup_stop_ns = 4000,
> +	.min_hold_buffer_ns = 4700,
>   };
>   
>   static const struct i2c_spec_values fast_mode_spec = {
> +	.min_hold_start_ns = 600,
>   	.min_low_ns = 1300,
>   	.min_high_ns = 600,
>   	.min_setup_start_ns = 600,
>   	.max_data_hold_ns = 900,
> +	.min_data_setup_ns = 100,
> +	.min_setup_stop_ns = 600,
> +	.min_hold_buffer_ns = 1300,
>   };
>   
>   /**
>    * struct rk3x_i2c_calced_timings:
>    * @div_low: Divider output for low
>    * @div_high: Divider output for high
> + * @tuning: Used to adjust setup/hold data time,
> + * setup/hold start time and setup stop time for
> + * v1's calc_timings, the tuning should all be 0
> + * for old hardware anyone using v0's calc_timings.
>    */
>   struct rk3x_i2c_calced_timings {
>   	unsigned long div_low;
>   	unsigned long div_high;
> +	unsigned int tuning;
>   };
>   
>   enum rk3x_i2c_state {
> @@ -123,9 +151,12 @@ enum rk3x_i2c_state {
>   
>   /**
>    * @grf_offset: offset inside the grf regmap for setting the i2c type
> + * @calc_timings: Callback function for i2c timing information calculated
>    */
>   struct rk3x_i2c_soc_data {
>   	int grf_offset;
> +	int (*calc_timings)(unsigned long, struct i2c_timings *,
> +			    struct rk3x_i2c_calced_timings *);
>   };
>   
>   /**
> @@ -134,7 +165,8 @@ struct rk3x_i2c_soc_data {
>    * @dev: device for this controller
>    * @soc_data: related soc data struct
>    * @regs: virtual memory area
> - * @clk: clock of i2c bus
> + * @clk: function clk for rk3399 or function & Bus clks for others
> + * @pclk: Bus clk for rk3399
>    * @clk_rate_nb: i2c clk rate change notify
>    * @t: I2C known timing information
>    * @lock: spinlock for the i2c bus
> @@ -156,6 +188,7 @@ struct rk3x_i2c {
>   	/* Hardware resources */
>   	void __iomem *regs;
>   	struct clk *clk;
> +	struct clk *pclk;
>   	struct notifier_block clk_rate_nb;
>   
>   	/* Settings */
> @@ -200,12 +233,12 @@ static inline void rk3x_i2c_clean_ipd(struct rk3x_i2c *i2c)
>    */
>   static void rk3x_i2c_start(struct rk3x_i2c *i2c)
>   {
> -	u32 val;
> +	u32 val = i2c_readl(i2c, REG_CON) & REG_CON_TUNING_MASK;
>   
>   	i2c_writel(i2c, REG_INT_START, REG_IEN);
>   
>   	/* enable adapter with correct mode, send START condition */
> -	val = REG_CON_EN | REG_CON_MOD(i2c->mode) | REG_CON_START;
> +	val |= REG_CON_EN | REG_CON_MOD(i2c->mode) | REG_CON_START;
>   
>   	/* if we want to react to NACK, set ACTACK bit */
>   	if (!(i2c->msg->flags & I2C_M_IGNORE_NAK))
> @@ -513,9 +546,9 @@ static const struct i2c_spec_values *rk3x_i2c_get_spec(unsigned int speed)
>    * a best-effort divider value is returned in divs. If the target rate is
>    * too high, we silently use the highest possible rate.
>    */
> -static int rk3x_i2c_calc_divs(unsigned long clk_rate,
> -			      struct i2c_timings *t,
> -			      struct rk3x_i2c_calced_timings *t_calc)
> +static int rk3x_i2c_v0_calc_timings(unsigned long clk_rate,
> +				    struct i2c_timings *t,
> +				    struct rk3x_i2c_calced_timings *t_calc)
>   {
>   	unsigned long min_low_ns, min_high_ns;
>   	unsigned long max_low_ns, min_total_ns;
> @@ -661,20 +694,189 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
>   	return ret;
>   }
>   
> +/**
> + * Calculate timing values for desired SCL frequency
> + *
> + * @clk_rate: I2C input clock rate
> + * @t: Known I2C timing information
> + * @t_calc: Caculated rk3x private timings that would be written into regs
> +

miss the '* ' , i guess
> + * Returns: 0 on success, -EINVAL if the goal SCL rate is too slow. In that case
> + * a best-effort divider value is returned in divs. If the target rate is
> + * too high, we silently use the highest possible rate.
> + * The following formulas are v1's method to calculate timings.
> + *
> + * l = divl + 1;
> + * h = divh + 1;
> + * s = sda_update_config + 1;
> + * u = start_setup_config + 1;
> + * p = stop_setup_config + 1;
> + * T = Tclk_i2c;
> +

Ditto
> + * tHigh = 8 * h * T;
> + * tLow = 8 * l * T;
> +

Ditto
> + * tHD;sda = (l * s + 1) * T;
> + * tSU;sda = [(8 - s) * l + 1] * T;
> + * tI2C = 8 * (l + h) * T;
> +

Ditto
> + * tSU;sta = (8h * u + 1) * T;
> + * tHD;sta = [8h * (u + 1) - 1] * T;
> + * tSU;sto = (8h * p + 1) * T;
> + */
> +static int rk3x_i2c_v1_calc_timings(unsigned long clk_rate,
> +				    struct i2c_timings *t,
> +				    struct rk3x_i2c_calced_timings *t_calc)
> +{
> +	unsigned long min_low_ns, min_high_ns, min_total_ns;
> +	unsigned long min_setup_start_ns, min_setup_data_ns;
> +	unsigned long min_setup_stop_ns, max_hold_data_ns;
> +
> +	unsigned long clk_rate_khz, scl_rate_khz;
> +
> +	unsigned long min_low_div, min_high_div;
> +
> +	unsigned long min_div_for_hold, min_total_div;
> +	unsigned long extra_div, extra_low_div;
> +	unsigned long sda_update_cfg, stp_sta_cfg, stp_sto_cfg;
> +
> +	const struct i2c_spec_values *spec;
> +	int ret = 0;
> +
> +	/* Support standard-mode and fast-mode */
> +	if (WARN_ON(t->bus_freq_hz > 400000))
> +		t->bus_freq_hz = 400000;
> +
> +	/* prevent scl_rate_khz from becoming 0 */
> +	if (WARN_ON(t->bus_freq_hz < 1000))
> +		t->bus_freq_hz = 1000;
> +
> +	/*
> +	 * 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.
> +	 */
> +	spec = rk3x_i2c_get_spec(t->bus_freq_hz);
> +
> +	/* calculate min-divh and min-divl */
> +	clk_rate_khz = DIV_ROUND_UP(clk_rate, 1000);
> +	scl_rate_khz = t->bus_freq_hz / 1000;
> +	min_total_div = DIV_ROUND_UP(clk_rate_khz, scl_rate_khz * 8);
> +
> +	min_high_ns = t->scl_rise_ns + spec->min_high_ns;
> +	min_high_div = DIV_ROUND_UP(clk_rate_khz * min_high_ns, 8 * 1000000);
> +
> +	min_low_ns = t->scl_fall_ns + spec->min_low_ns;
> +	min_low_div = DIV_ROUND_UP(clk_rate_khz * min_low_ns, 8 * 1000000);
> +
> +	/*
> +	 * Final divh and divl must be greater than 0, otherwise the
> +	 * hardware would not output the i2c clk.
> +	 */
> +	min_high_div = (min_high_div < 1) ? 2 : min_high_div;
> +	min_low_div = (min_low_div < 1) ? 2 : min_low_div;
> +
> +	/* These are the min dividers needed for min hold times. */
> +	min_div_for_hold = (min_low_div + min_high_div);
> +	min_total_ns = min_low_ns + min_high_ns;
> +
> +	/*
> +	 * 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.
> +	 */
> +	 if (min_div_for_hold >= min_total_div) {
> +		/*
> +		 * Time needed to meet hold requirements is important.
> +		 * Just use that.
> +		 */
> +		t_calc->div_low = min_low_div;
> +		t_calc->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.
> +		 * We'll try to split things up by the scale of min_low_div and
> +		 * min_high_div, biasing slightly towards having a higher div
> +		 * for low (spend more time low).
> +		 */
> +		extra_div = min_total_div - min_div_for_hold;
> +		extra_low_div = DIV_ROUND_UP(min_low_div * extra_div,
> +					     min_div_for_hold);
> +
> +		t_calc->div_low = min_low_div + extra_low_div;
> +		t_calc->div_high = min_high_div + (extra_div - extra_low_div);
> +	}
> +
> +	/*
> +	 * calculate sda data hold count by the rules, data_upd_st:3
> +	 * is a appropriate value to reduce calculated times.
> +	 */
> +	for (sda_update_cfg = 3; sda_update_cfg > 0; sda_update_cfg--) {
> +		max_hold_data_ns =  DIV_ROUND_UP((sda_update_cfg
> +						 * (t_calc->div_low) + 1)
> +						 * 1000000, clk_rate_khz);
> +		min_setup_data_ns =  DIV_ROUND_UP(((8 - sda_update_cfg)
> +						 * (t_calc->div_low) + 1)
> +						 * 1000000, clk_rate_khz);
> +		if ((max_hold_data_ns < spec->max_data_hold_ns) &&
> +		    (min_setup_data_ns > spec->min_data_setup_ns))
> +			break;
> +	}
> +
> +	/* calculate setup start config */
> +	min_setup_start_ns = t->scl_rise_ns + spec->min_setup_start_ns;
> +	stp_sta_cfg = DIV_ROUND_UP(clk_rate_khz * min_setup_start_ns
> +			   - 1000000, 8 * 1000000 * (t_calc->div_high));
> +
> +	/* calculate setup stop config */
> +	min_setup_stop_ns = t->scl_rise_ns + spec->min_setup_stop_ns;
> +	stp_sto_cfg = DIV_ROUND_UP(clk_rate_khz * min_setup_stop_ns
> +			   - 1000000, 8 * 1000000 * (t_calc->div_high));
> +
> +	t_calc->tuning = REG_CON_SDA_CFG(--sda_update_cfg) |
> +			 REG_CON_STA_CFG(--stp_sta_cfg) |
> +			 REG_CON_STO_CFG(--stp_sto_cfg);
> +
> +	t_calc->div_low--;
> +	t_calc->div_high--;
> +
> +	/* Maximum divider supported by hw is 0xffff */
> +	if (t_calc->div_low > 0xffff) {
> +		t_calc->div_low = 0xffff;
> +		ret = -EINVAL;
> +	}
> +
> +	if (t_calc->div_high > 0xffff) {
> +		t_calc->div_high = 0xffff;
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>   static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
>   {
>   	struct i2c_timings *t = &i2c->t;
>   	struct rk3x_i2c_calced_timings calc;
>   	u64 t_low_ns, t_high_ns;
> +	u32 val;
>   	int ret;
>   
> -	ret = rk3x_i2c_calc_divs(clk_rate, t, &calc);
> +	ret = i2c->soc_data->calc_timings(clk_rate, t, &calc);
>   	WARN_ONCE(ret != 0, "Could not reach SCL freq %u", t->bus_freq_hz);
>   
> -	clk_enable(i2c->clk);
> +	clk_enable(i2c->pclk);
> +
>   	i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
>   		   REG_CLKDIV);
> -	clk_disable(i2c->clk);
> +
> +	val = i2c_readl(i2c, REG_CON);
> +	val &= ~REG_CON_TUNING_MASK;
> +	val |= calc.tuning;
> +	i2c_writel(i2c, val, REG_CON);
> +
> +	clk_disable(i2c->pclk);
>   
>   	t_low_ns = div_u64(((u64)calc.div_low + 1) * 8 * 1000000000, clk_rate);
>   	t_high_ns = div_u64(((u64)calc.div_high + 1) * 8 * 1000000000,
> @@ -712,7 +914,13 @@ 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->t, &calc) != 0)
> +		/*
> +		 * Try the calculation (but don't store the result) ahead of
> +		 * time to see if we need to block the clock change.  Timings
> +		 * shouldn't actually take effect until rk3x_i2c_adapt_div().
> +		 */
> +		if (i2c->soc_data->calc_timings(ndata->new_rate, &i2c->t,
> +						&calc) != 0)
>   			return NOTIFY_STOP;
>   
>   		/* scale up */
> @@ -822,12 +1030,14 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
>   {
>   	struct rk3x_i2c *i2c = (struct rk3x_i2c *)adap->algo_data;
>   	unsigned long timeout, flags;
> +	u32 val;
>   	int ret = 0;
>   	int i;
>   
>   	spin_lock_irqsave(&i2c->lock, flags);
>   
>   	clk_enable(i2c->clk);
> +	clk_enable(i2c->pclk);
>   
>   	i2c->is_last_msg = false;
>   
> @@ -861,7 +1071,9 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
>   
>   			/* Force a STOP condition without interrupt */
>   			i2c_writel(i2c, 0, REG_IEN);
> -			i2c_writel(i2c, REG_CON_EN | REG_CON_STOP, REG_CON);
> +			val = i2c_readl(i2c, REG_CON) & REG_CON_TUNING_MASK;
> +			val |= REG_CON_EN | REG_CON_STOP;
> +			i2c_writel(i2c, val, REG_CON);
>   
>   			i2c->state = STATE_IDLE;
>   
> @@ -875,7 +1087,9 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
>   		}
>   	}
>   
> +	clk_disable(i2c->pclk);
>   	clk_disable(i2c->clk);
> +
>   	spin_unlock_irqrestore(&i2c->lock, flags);
>   
>   	return ret < 0 ? ret : num;
> @@ -893,18 +1107,27 @@ static const struct i2c_algorithm rk3x_i2c_algorithm = {
>   
>   static const struct rk3x_i2c_soc_data rk3066_soc_data = {
>   	.grf_offset = 0x154,
> +	.calc_timings = rk3x_i2c_v0_calc_timings,
>   };
>   
>   static const struct rk3x_i2c_soc_data rk3188_soc_data = {
>   	.grf_offset = 0x0a4,
> +	.calc_timings = rk3x_i2c_v0_calc_timings,
>   };
>   
>   static const struct rk3x_i2c_soc_data rk3228_soc_data = {
>   	.grf_offset = -1,
> +	.calc_timings = rk3x_i2c_v0_calc_timings,
>   };
>   
>   static const struct rk3x_i2c_soc_data rk3288_soc_data = {
>   	.grf_offset = -1,
> +	.calc_timings = rk3x_i2c_v0_calc_timings,
> +};
> +
> +static const struct rk3x_i2c_soc_data rk3399_soc_data = {
> +	.grf_offset = -1,
> +	.calc_timings = rk3x_i2c_v1_calc_timings,
>   };
>   
>   static const struct of_device_id rk3x_i2c_match[] = {
> @@ -924,6 +1147,10 @@ static const struct of_device_id rk3x_i2c_match[] = {
>   		.compatible = "rockchip,rk3288-i2c",
>   		.data = (void *)&rk3288_soc_data
>   	},
> +	{
> +		.compatible = "rockchip,rk3399-i2c",
> +		.data = (void *)&rk3399_soc_data
> +	},
>   	{},
>   };
>   MODULE_DEVICE_TABLE(of, rk3x_i2c_match);
> @@ -963,12 +1190,6 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
>   	spin_lock_init(&i2c->lock);
>   	init_waitqueue_head(&i2c->wait);
>   
> -	i2c->clk = devm_clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(i2c->clk)) {
> -		dev_err(&pdev->dev, "cannot get clock\n");
> -		return PTR_ERR(i2c->clk);
> -	}
> -
>   	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   	i2c->regs = devm_ioremap_resource(&pdev->dev, mem);
>   	if (IS_ERR(i2c->regs))
> @@ -1022,17 +1243,44 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
>   
>   	platform_set_drvdata(pdev, i2c);
>   
> +	if (i2c->soc_data->calc_timings == rk3x_i2c_v0_calc_timings) {
> +		/* Only one clock to use for bus clock and peripheral clock */
> +		i2c->clk = devm_clk_get(&pdev->dev, NULL);
> +		i2c->pclk = i2c->clk;
> +	} else {
> +		i2c->clk = devm_clk_get(&pdev->dev, "i2c");
> +		i2c->pclk = devm_clk_get(&pdev->dev, "pclk");
> +	}
> +
> +	if (IS_ERR(i2c->clk)) {
> +		ret = PTR_ERR(i2c->clk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "Can't get bus clk: %d\n", ret);
> +		return ret;
> +	}
> +	if (IS_ERR(i2c->pclk)) {
> +		ret = PTR_ERR(i2c->pclk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "Can't get periph clk: %d\n", ret);
> +		return ret;
> +	}
> +
>   	ret = clk_prepare(i2c->clk);
>   	if (ret < 0) {
> -		dev_err(&pdev->dev, "Could not prepare clock\n");
> +		dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);
>   		return ret;
>   	}
> +	ret = clk_prepare(i2c->pclk);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Can't prepare periph clock: %d\n", ret);
> +		goto err_clk;
> +	}
>   
>   	i2c->clk_rate_nb.notifier_call = rk3x_i2c_clk_notifier_cb;
>   	ret = clk_notifier_register(i2c->clk, &i2c->clk_rate_nb);
>   	if (ret != 0) {
>   		dev_err(&pdev->dev, "Unable to register clock notifier\n");
> -		goto err_clk;
> +		goto err_pclk;
>   	}
>   
>   	clk_rate = clk_get_rate(i2c->clk);
> @@ -1050,6 +1298,8 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
>   
>   err_clk_notifier:
>   	clk_notifier_unregister(i2c->clk, &i2c->clk_rate_nb);
> +err_pclk:
> +	clk_unprepare(i2c->pclk);
>   err_clk:
>   	clk_unprepare(i2c->clk);
>   	return ret;
> @@ -1062,6 +1312,7 @@ static int rk3x_i2c_remove(struct platform_device *pdev)
>   	i2c_del_adapter(&i2c->adap);
>   
>   	clk_notifier_unregister(i2c->clk, &i2c->clk_rate_nb);
> +	clk_unprepare(i2c->pclk);
>   	clk_unprepare(i2c->clk);
>   
>   	return 0;


-- 
Thanks,
Caesar

[PATCH v8 7/8] i2c: rk3x: add i2c support for rk3399 soc

[Caesar Wang]

? 2016?05?11? 03:31, David Wu ??:
> - new method to caculate i2c timings for rk3399:
>    There was an timing issue about "repeated start" time at the I2C
>    controller of version0, controller appears to drop SDA at .875x (7/8)
>    programmed clk high. On version 1 of the controller, the rule(.875x)
>    isn't enough to meet tSU;STA
>    requirements on 100k's Standard-mode. To resolve this issue,
>    sda_update_config, start_setup_config and stop_setup_config for I2C
>    timing information are added, new rules are designed to calculate
>    the timing information at new v1.
> - pclk and function clk are separated at rk3399
>
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
> Changes in v8:
> - update tuning in RKI2C_CON register by doing a read-modify-write (Doug)
> - new method to use pclk and clk (Doug)
>
> Changes in v7:
> - transform into a 9 series patches (Doug)
> - drop highspeed with mastercode, and support fast-mode plus (Doug)

Tested-by: Caesar Wang <wxt@rock-chips.com>
Few comments below:


I picked David's patches up for my rk3399 evb board.
Add the needed i2c devices for dts.

localhost / #modprobe i2c-dev
localhost / # i2cdump -f -y 0 0x1b
No size specified (using byte-data access)
      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f 0123456789abcdef
00: 18 01 09 21 01 13 01 00 00 00 00 01 01 00 00 00 ???!???....??...
10: 80 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ??..............
20: 00 5f 00 6f ff 00 00 00 10 00 ff 0f ff 02 01 0f ._.o....?..?.???
30: 00 00 01 0f 00 00 02 03 00 00 09 00 00 0c 00 0a ..??..??..?..?.?
40: 00 0c 00 0c 00 07 00 0a 00 0c 00 00 00 5f 00 03 .?.?.?.?.?..._.?
50: 06 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ??..............
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
70: 00 cf 03 00 28 00 0c 1c 80 19 00 34 12 00 71 00 .??.(.????.4?.q.
80: 10 50 1f ac 00 40 00 49 00 00 c0 44 41 01 00 00 ?P??. at .I..?DA?..
90: 55 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 U...............
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
localhost / # i2cset -f -y 0 0x1b 0x90 0xff
localhost / # i2cdump -f -y 0 0x1b |grep 90
  No size specified (using byte-data access)
90: ff 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................

The i2c devices are working for my board.

run:
modprobe i2c-dev; for i in $(seq 0 8); do echo ==BUS $i; i2cdetect -y -q 
$i; done
See the output message,and mesure the i2c frequency.





>
>   drivers/i2c/busses/i2c-rk3x.c | 289 +++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 270 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 9791797..25ed1ad 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -58,6 +58,12 @@ enum {
>   #define REG_CON_LASTACK   BIT(5) /* 1: send NACK after last received byte */
>   #define REG_CON_ACTACK    BIT(6) /* 1: stop if NACK is received */
>   
> +#define REG_CON_TUNING_MASK GENMASK(15, 8)
> +
> +#define REG_CON_SDA_CFG(cfg) ((cfg) << 8)
> +#define REG_CON_STA_CFG(cfg) ((cfg) << 12)
> +#define REG_CON_STO_CFG(cfg) ((cfg) << 14)
> +
>   /* REG_MRXADDR bits */
>   #define REG_MRXADDR_VALID(x) BIT(24 + (x)) /* [x*8+7:x*8] of MRX[R]ADDR valid */
>   
> @@ -77,40 +83,62 @@ enum {
>   
>   /**
>    * struct i2c_spec_values:
> + * @min_hold_start_ns: min hold time (repeated) START condition
>    * @min_low_ns: min LOW period of the SCL clock
>    * @min_high_ns: min HIGH period of the SCL cloc
>    * @min_setup_start_ns: min set-up time for a repeated START conditio
>    * @max_data_hold_ns: max data hold time
> + * @min_data_setup_ns: min data set-up time
> + * @min_setup_stop_ns: min set-up time for STOP condition
> + * @min_hold_buffer_ns: min bus free time between a STOP and
> + * START condition
>    */
>   struct i2c_spec_values {
> +	unsigned long min_hold_start_ns;
>   	unsigned long min_low_ns;
>   	unsigned long min_high_ns;
>   	unsigned long min_setup_start_ns;
>   	unsigned long max_data_hold_ns;
> +	unsigned long min_data_setup_ns;
> +	unsigned long min_setup_stop_ns;
> +	unsigned long min_hold_buffer_ns;
>   };
>   
>   static const struct i2c_spec_values standard_mode_spec = {
> +	.min_hold_start_ns = 4000,
>   	.min_low_ns = 4700,
>   	.min_high_ns = 4000,
>   	.min_setup_start_ns = 4700,
>   	.max_data_hold_ns = 3450,
> +	.min_data_setup_ns = 250,
> +	.min_setup_stop_ns = 4000,
> +	.min_hold_buffer_ns = 4700,
>   };
>   
>   static const struct i2c_spec_values fast_mode_spec = {
> +	.min_hold_start_ns = 600,
>   	.min_low_ns = 1300,
>   	.min_high_ns = 600,
>   	.min_setup_start_ns = 600,
>   	.max_data_hold_ns = 900,
> +	.min_data_setup_ns = 100,
> +	.min_setup_stop_ns = 600,
> +	.min_hold_buffer_ns = 1300,
>   };
>   
>   /**
>    * struct rk3x_i2c_calced_timings:
>    * @div_low: Divider output for low
>    * @div_high: Divider output for high
> + * @tuning: Used to adjust setup/hold data time,
> + * setup/hold start time and setup stop time for
> + * v1's calc_timings, the tuning should all be 0
> + * for old hardware anyone using v0's calc_timings.
>    */
>   struct rk3x_i2c_calced_timings {
>   	unsigned long div_low;
>   	unsigned long div_high;
> +	unsigned int tuning;
>   };
>   
>   enum rk3x_i2c_state {
> @@ -123,9 +151,12 @@ enum rk3x_i2c_state {
>   
>   /**
>    * @grf_offset: offset inside the grf regmap for setting the i2c type
> + * @calc_timings: Callback function for i2c timing information calculated
>    */
>   struct rk3x_i2c_soc_data {
>   	int grf_offset;
> +	int (*calc_timings)(unsigned long, struct i2c_timings *,
> +			    struct rk3x_i2c_calced_timings *);
>   };
>   
>   /**
> @@ -134,7 +165,8 @@ struct rk3x_i2c_soc_data {
>    * @dev: device for this controller
>    * @soc_data: related soc data struct
>    * @regs: virtual memory area
> - * @clk: clock of i2c bus
> + * @clk: function clk for rk3399 or function & Bus clks for others
> + * @pclk: Bus clk for rk3399
>    * @clk_rate_nb: i2c clk rate change notify
>    * @t: I2C known timing information
>    * @lock: spinlock for the i2c bus
> @@ -156,6 +188,7 @@ struct rk3x_i2c {
>   	/* Hardware resources */
>   	void __iomem *regs;
>   	struct clk *clk;
> +	struct clk *pclk;
>   	struct notifier_block clk_rate_nb;
>   
>   	/* Settings */
> @@ -200,12 +233,12 @@ static inline void rk3x_i2c_clean_ipd(struct rk3x_i2c *i2c)
>    */
>   static void rk3x_i2c_start(struct rk3x_i2c *i2c)
>   {
> -	u32 val;
> +	u32 val = i2c_readl(i2c, REG_CON) & REG_CON_TUNING_MASK;
>   
>   	i2c_writel(i2c, REG_INT_START, REG_IEN);
>   
>   	/* enable adapter with correct mode, send START condition */
> -	val = REG_CON_EN | REG_CON_MOD(i2c->mode) | REG_CON_START;
> +	val |= REG_CON_EN | REG_CON_MOD(i2c->mode) | REG_CON_START;
>   
>   	/* if we want to react to NACK, set ACTACK bit */
>   	if (!(i2c->msg->flags & I2C_M_IGNORE_NAK))
> @@ -513,9 +546,9 @@ static const struct i2c_spec_values *rk3x_i2c_get_spec(unsigned int speed)
>    * a best-effort divider value is returned in divs. If the target rate is
>    * too high, we silently use the highest possible rate.
>    */
> -static int rk3x_i2c_calc_divs(unsigned long clk_rate,
> -			      struct i2c_timings *t,
> -			      struct rk3x_i2c_calced_timings *t_calc)
> +static int rk3x_i2c_v0_calc_timings(unsigned long clk_rate,
> +				    struct i2c_timings *t,
> +				    struct rk3x_i2c_calced_timings *t_calc)
>   {
>   	unsigned long min_low_ns, min_high_ns;
>   	unsigned long max_low_ns, min_total_ns;
> @@ -661,20 +694,189 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
>   	return ret;
>   }
>   
> +/**
> + * Calculate timing values for desired SCL frequency
> + *
> + * @clk_rate: I2C input clock rate
> + * @t: Known I2C timing information
> + * @t_calc: Caculated rk3x private timings that would be written into regs
> +

miss the '* ' , i guess
> + * Returns: 0 on success, -EINVAL if the goal SCL rate is too slow. In that case
> + * a best-effort divider value is returned in divs. If the target rate is
> + * too high, we silently use the highest possible rate.
> + * The following formulas are v1's method to calculate timings.
> + *
> + * l = divl + 1;
> + * h = divh + 1;
> + * s = sda_update_config + 1;
> + * u = start_setup_config + 1;
> + * p = stop_setup_config + 1;
> + * T = Tclk_i2c;
> +

Ditto
> + * tHigh = 8 * h * T;
> + * tLow = 8 * l * T;
> +

Ditto
> + * tHD;sda = (l * s + 1) * T;
> + * tSU;sda = [(8 - s) * l + 1] * T;
> + * tI2C = 8 * (l + h) * T;
> +

Ditto
> + * tSU;sta = (8h * u + 1) * T;
> + * tHD;sta = [8h * (u + 1) - 1] * T;
> + * tSU;sto = (8h * p + 1) * T;
> + */
> +static int rk3x_i2c_v1_calc_timings(unsigned long clk_rate,
> +				    struct i2c_timings *t,
> +				    struct rk3x_i2c_calced_timings *t_calc)
> +{
> +	unsigned long min_low_ns, min_high_ns, min_total_ns;
> +	unsigned long min_setup_start_ns, min_setup_data_ns;
> +	unsigned long min_setup_stop_ns, max_hold_data_ns;
> +
> +	unsigned long clk_rate_khz, scl_rate_khz;
> +
> +	unsigned long min_low_div, min_high_div;
> +
> +	unsigned long min_div_for_hold, min_total_div;
> +	unsigned long extra_div, extra_low_div;
> +	unsigned long sda_update_cfg, stp_sta_cfg, stp_sto_cfg;
> +
> +	const struct i2c_spec_values *spec;
> +	int ret = 0;
> +
> +	/* Support standard-mode and fast-mode */
> +	if (WARN_ON(t->bus_freq_hz > 400000))
> +		t->bus_freq_hz = 400000;
> +
> +	/* prevent scl_rate_khz from becoming 0 */
> +	if (WARN_ON(t->bus_freq_hz < 1000))
> +		t->bus_freq_hz = 1000;
> +
> +	/*
> +	 * 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.
> +	 */
> +	spec = rk3x_i2c_get_spec(t->bus_freq_hz);
> +
> +	/* calculate min-divh and min-divl */
> +	clk_rate_khz = DIV_ROUND_UP(clk_rate, 1000);
> +	scl_rate_khz = t->bus_freq_hz / 1000;
> +	min_total_div = DIV_ROUND_UP(clk_rate_khz, scl_rate_khz * 8);
> +
> +	min_high_ns = t->scl_rise_ns + spec->min_high_ns;
> +	min_high_div = DIV_ROUND_UP(clk_rate_khz * min_high_ns, 8 * 1000000);
> +
> +	min_low_ns = t->scl_fall_ns + spec->min_low_ns;
> +	min_low_div = DIV_ROUND_UP(clk_rate_khz * min_low_ns, 8 * 1000000);
> +
> +	/*
> +	 * Final divh and divl must be greater than 0, otherwise the
> +	 * hardware would not output the i2c clk.
> +	 */
> +	min_high_div = (min_high_div < 1) ? 2 : min_high_div;
> +	min_low_div = (min_low_div < 1) ? 2 : min_low_div;
> +
> +	/* These are the min dividers needed for min hold times. */
> +	min_div_for_hold = (min_low_div + min_high_div);
> +	min_total_ns = min_low_ns + min_high_ns;
> +
> +	/*
> +	 * 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.
> +	 */
> +	 if (min_div_for_hold >= min_total_div) {
> +		/*
> +		 * Time needed to meet hold requirements is important.
> +		 * Just use that.
> +		 */
> +		t_calc->div_low = min_low_div;
> +		t_calc->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.
> +		 * We'll try to split things up by the scale of min_low_div and
> +		 * min_high_div, biasing slightly towards having a higher div
> +		 * for low (spend more time low).
> +		 */
> +		extra_div = min_total_div - min_div_for_hold;
> +		extra_low_div = DIV_ROUND_UP(min_low_div * extra_div,
> +					     min_div_for_hold);
> +
> +		t_calc->div_low = min_low_div + extra_low_div;
> +		t_calc->div_high = min_high_div + (extra_div - extra_low_div);
> +	}
> +
> +	/*
> +	 * calculate sda data hold count by the rules, data_upd_st:3
> +	 * is a appropriate value to reduce calculated times.
> +	 */
> +	for (sda_update_cfg = 3; sda_update_cfg > 0; sda_update_cfg--) {
> +		max_hold_data_ns =  DIV_ROUND_UP((sda_update_cfg
> +						 * (t_calc->div_low) + 1)
> +						 * 1000000, clk_rate_khz);
> +		min_setup_data_ns =  DIV_ROUND_UP(((8 - sda_update_cfg)
> +						 * (t_calc->div_low) + 1)
> +						 * 1000000, clk_rate_khz);
> +		if ((max_hold_data_ns < spec->max_data_hold_ns) &&
> +		    (min_setup_data_ns > spec->min_data_setup_ns))
> +			break;
> +	}
> +
> +	/* calculate setup start config */
> +	min_setup_start_ns = t->scl_rise_ns + spec->min_setup_start_ns;
> +	stp_sta_cfg = DIV_ROUND_UP(clk_rate_khz * min_setup_start_ns
> +			   - 1000000, 8 * 1000000 * (t_calc->div_high));
> +
> +	/* calculate setup stop config */
> +	min_setup_stop_ns = t->scl_rise_ns + spec->min_setup_stop_ns;
> +	stp_sto_cfg = DIV_ROUND_UP(clk_rate_khz * min_setup_stop_ns
> +			   - 1000000, 8 * 1000000 * (t_calc->div_high));
> +
> +	t_calc->tuning = REG_CON_SDA_CFG(--sda_update_cfg) |
> +			 REG_CON_STA_CFG(--stp_sta_cfg) |
> +			 REG_CON_STO_CFG(--stp_sto_cfg);
> +
> +	t_calc->div_low--;
> +	t_calc->div_high--;
> +
> +	/* Maximum divider supported by hw is 0xffff */
> +	if (t_calc->div_low > 0xffff) {
> +		t_calc->div_low = 0xffff;
> +		ret = -EINVAL;
> +	}
> +
> +	if (t_calc->div_high > 0xffff) {
> +		t_calc->div_high = 0xffff;
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>   static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
>   {
>   	struct i2c_timings *t = &i2c->t;
>   	struct rk3x_i2c_calced_timings calc;
>   	u64 t_low_ns, t_high_ns;
> +	u32 val;
>   	int ret;
>   
> -	ret = rk3x_i2c_calc_divs(clk_rate, t, &calc);
> +	ret = i2c->soc_data->calc_timings(clk_rate, t, &calc);
>   	WARN_ONCE(ret != 0, "Could not reach SCL freq %u", t->bus_freq_hz);
>   
> -	clk_enable(i2c->clk);
> +	clk_enable(i2c->pclk);
> +
>   	i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
>   		   REG_CLKDIV);
> -	clk_disable(i2c->clk);
> +
> +	val = i2c_readl(i2c, REG_CON);
> +	val &= ~REG_CON_TUNING_MASK;
> +	val |= calc.tuning;
> +	i2c_writel(i2c, val, REG_CON);
> +
> +	clk_disable(i2c->pclk);
>   
>   	t_low_ns = div_u64(((u64)calc.div_low + 1) * 8 * 1000000000, clk_rate);
>   	t_high_ns = div_u64(((u64)calc.div_high + 1) * 8 * 1000000000,
> @@ -712,7 +914,13 @@ 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->t, &calc) != 0)
> +		/*
> +		 * Try the calculation (but don't store the result) ahead of
> +		 * time to see if we need to block the clock change.  Timings
> +		 * shouldn't actually take effect until rk3x_i2c_adapt_div().
> +		 */
> +		if (i2c->soc_data->calc_timings(ndata->new_rate, &i2c->t,
> +						&calc) != 0)
>   			return NOTIFY_STOP;
>   
>   		/* scale up */
> @@ -822,12 +1030,14 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
>   {
>   	struct rk3x_i2c *i2c = (struct rk3x_i2c *)adap->algo_data;
>   	unsigned long timeout, flags;
> +	u32 val;
>   	int ret = 0;
>   	int i;
>   
>   	spin_lock_irqsave(&i2c->lock, flags);
>   
>   	clk_enable(i2c->clk);
> +	clk_enable(i2c->pclk);
>   
>   	i2c->is_last_msg = false;
>   
> @@ -861,7 +1071,9 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
>   
>   			/* Force a STOP condition without interrupt */
>   			i2c_writel(i2c, 0, REG_IEN);
> -			i2c_writel(i2c, REG_CON_EN | REG_CON_STOP, REG_CON);
> +			val = i2c_readl(i2c, REG_CON) & REG_CON_TUNING_MASK;
> +			val |= REG_CON_EN | REG_CON_STOP;
> +			i2c_writel(i2c, val, REG_CON);
>   
>   			i2c->state = STATE_IDLE;
>   
> @@ -875,7 +1087,9 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
>   		}
>   	}
>   
> +	clk_disable(i2c->pclk);
>   	clk_disable(i2c->clk);
> +
>   	spin_unlock_irqrestore(&i2c->lock, flags);
>   
>   	return ret < 0 ? ret : num;
> @@ -893,18 +1107,27 @@ static const struct i2c_algorithm rk3x_i2c_algorithm = {
>   
>   static const struct rk3x_i2c_soc_data rk3066_soc_data = {
>   	.grf_offset = 0x154,
> +	.calc_timings = rk3x_i2c_v0_calc_timings,
>   };
>   
>   static const struct rk3x_i2c_soc_data rk3188_soc_data = {
>   	.grf_offset = 0x0a4,
> +	.calc_timings = rk3x_i2c_v0_calc_timings,
>   };
>   
>   static const struct rk3x_i2c_soc_data rk3228_soc_data = {
>   	.grf_offset = -1,
> +	.calc_timings = rk3x_i2c_v0_calc_timings,
>   };
>   
>   static const struct rk3x_i2c_soc_data rk3288_soc_data = {
>   	.grf_offset = -1,
> +	.calc_timings = rk3x_i2c_v0_calc_timings,
> +};
> +
> +static const struct rk3x_i2c_soc_data rk3399_soc_data = {
> +	.grf_offset = -1,
> +	.calc_timings = rk3x_i2c_v1_calc_timings,
>   };
>   
>   static const struct of_device_id rk3x_i2c_match[] = {
> @@ -924,6 +1147,10 @@ static const struct of_device_id rk3x_i2c_match[] = {
>   		.compatible = "rockchip,rk3288-i2c",
>   		.data = (void *)&rk3288_soc_data
>   	},
> +	{
> +		.compatible = "rockchip,rk3399-i2c",
> +		.data = (void *)&rk3399_soc_data
> +	},
>   	{},
>   };
>   MODULE_DEVICE_TABLE(of, rk3x_i2c_match);
> @@ -963,12 +1190,6 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
>   	spin_lock_init(&i2c->lock);
>   	init_waitqueue_head(&i2c->wait);
>   
> -	i2c->clk = devm_clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(i2c->clk)) {
> -		dev_err(&pdev->dev, "cannot get clock\n");
> -		return PTR_ERR(i2c->clk);
> -	}
> -
>   	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   	i2c->regs = devm_ioremap_resource(&pdev->dev, mem);
>   	if (IS_ERR(i2c->regs))
> @@ -1022,17 +1243,44 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
>   
>   	platform_set_drvdata(pdev, i2c);
>   
> +	if (i2c->soc_data->calc_timings == rk3x_i2c_v0_calc_timings) {
> +		/* Only one clock to use for bus clock and peripheral clock */
> +		i2c->clk = devm_clk_get(&pdev->dev, NULL);
> +		i2c->pclk = i2c->clk;
> +	} else {
> +		i2c->clk = devm_clk_get(&pdev->dev, "i2c");
> +		i2c->pclk = devm_clk_get(&pdev->dev, "pclk");
> +	}
> +
> +	if (IS_ERR(i2c->clk)) {
> +		ret = PTR_ERR(i2c->clk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "Can't get bus clk: %d\n", ret);
> +		return ret;
> +	}
> +	if (IS_ERR(i2c->pclk)) {
> +		ret = PTR_ERR(i2c->pclk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "Can't get periph clk: %d\n", ret);
> +		return ret;
> +	}
> +
>   	ret = clk_prepare(i2c->clk);
>   	if (ret < 0) {
> -		dev_err(&pdev->dev, "Could not prepare clock\n");
> +		dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);
>   		return ret;
>   	}
> +	ret = clk_prepare(i2c->pclk);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Can't prepare periph clock: %d\n", ret);
> +		goto err_clk;
> +	}
>   
>   	i2c->clk_rate_nb.notifier_call = rk3x_i2c_clk_notifier_cb;
>   	ret = clk_notifier_register(i2c->clk, &i2c->clk_rate_nb);
>   	if (ret != 0) {
>   		dev_err(&pdev->dev, "Unable to register clock notifier\n");
> -		goto err_clk;
> +		goto err_pclk;
>   	}
>   
>   	clk_rate = clk_get_rate(i2c->clk);
> @@ -1050,6 +1298,8 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
>   
>   err_clk_notifier:
>   	clk_notifier_unregister(i2c->clk, &i2c->clk_rate_nb);
> +err_pclk:
> +	clk_unprepare(i2c->pclk);
>   err_clk:
>   	clk_unprepare(i2c->clk);
>   	return ret;
> @@ -1062,6 +1312,7 @@ static int rk3x_i2c_remove(struct platform_device *pdev)
>   	i2c_del_adapter(&i2c->adap);
>   
>   	clk_notifier_unregister(i2c->clk, &i2c->clk_rate_nb);
> +	clk_unprepare(i2c->pclk);
>   	clk_unprepare(i2c->clk);
>   
>   	return 0;


-- 
Thanks,
Caesar

Re: [PATCH v8 8/8] i2c: rk3x: support fast-mode plus for rk3399

[Caesar Wang]

在 2016年05月11日 03:33, David Wu 写道:
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Tested-by: Caesar Wang <wxt@rock-chips.com>
> ---
> Change in v8:
> - None
>
>   drivers/i2c/busses/i2c-rk3x.c | 21 +++++++++++++++++----
>   1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 25ed1ad..0ba25ee 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -126,6 +126,17 @@ static const struct i2c_spec_values fast_mode_spec = {
>   	.min_hold_buffer_ns = 1300,
>   };
>   
> +static const struct i2c_spec_values fast_mode_plus_spec = {
> +	.min_hold_start_ns = 260,
> +	.min_low_ns = 500,
> +	.min_high_ns = 260,
> +	.min_setup_start_ns = 260,
> +	.max_data_hold_ns = 400,
> +	.min_data_setup_ns = 50,
> +	.min_setup_stop_ns = 260,
> +	.min_hold_buffer_ns = 500,
> +};
> +
>   /**
>    * struct rk3x_i2c_calced_timings:
>    * @div_low: Divider output for low
> @@ -531,8 +542,10 @@ static const struct i2c_spec_values *rk3x_i2c_get_spec(unsigned int speed)
>   {
>   	if (speed <= 100000)
>   		return &standard_mode_spec;
> -	else
> +	else if (speed <= 400000)
>   		return &fast_mode_spec;
> +	else
> +		return &fast_mode_plus_spec;
>   }
>   
>   /**
> @@ -743,9 +756,9 @@ static int rk3x_i2c_v1_calc_timings(unsigned long clk_rate,
>   	const struct i2c_spec_values *spec;
>   	int ret = 0;
>   
> -	/* Support standard-mode and fast-mode */
> -	if (WARN_ON(t->bus_freq_hz > 400000))
> -		t->bus_freq_hz = 400000;
> +	/* Support standard-mode, fast-mode and fast-mode plus */
> +	if (WARN_ON(t->bus_freq_hz > 1000000))
> +		t->bus_freq_hz = 1000000;
>   
>   	/* prevent scl_rate_khz from becoming 0 */
>   	if (WARN_ON(t->bus_freq_hz < 1000))


-- 
Thanks,
Caesar

[PATCH v8 8/8] i2c: rk3x: support fast-mode plus for rk3399

[Caesar Wang]

? 2016?05?11? 03:33, David Wu ??:
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Tested-by: Caesar Wang <wxt@rock-chips.com>
> ---
> Change in v8:
> - None
>
>   drivers/i2c/busses/i2c-rk3x.c | 21 +++++++++++++++++----
>   1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 25ed1ad..0ba25ee 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -126,6 +126,17 @@ static const struct i2c_spec_values fast_mode_spec = {
>   	.min_hold_buffer_ns = 1300,
>   };
>   
> +static const struct i2c_spec_values fast_mode_plus_spec = {
> +	.min_hold_start_ns = 260,
> +	.min_low_ns = 500,
> +	.min_high_ns = 260,
> +	.min_setup_start_ns = 260,
> +	.max_data_hold_ns = 400,
> +	.min_data_setup_ns = 50,
> +	.min_setup_stop_ns = 260,
> +	.min_hold_buffer_ns = 500,
> +};
> +
>   /**
>    * struct rk3x_i2c_calced_timings:
>    * @div_low: Divider output for low
> @@ -531,8 +542,10 @@ static const struct i2c_spec_values *rk3x_i2c_get_spec(unsigned int speed)
>   {
>   	if (speed <= 100000)
>   		return &standard_mode_spec;
> -	else
> +	else if (speed <= 400000)
>   		return &fast_mode_spec;
> +	else
> +		return &fast_mode_plus_spec;
>   }
>   
>   /**
> @@ -743,9 +756,9 @@ static int rk3x_i2c_v1_calc_timings(unsigned long clk_rate,
>   	const struct i2c_spec_values *spec;
>   	int ret = 0;
>   
> -	/* Support standard-mode and fast-mode */
> -	if (WARN_ON(t->bus_freq_hz > 400000))
> -		t->bus_freq_hz = 400000;
> +	/* Support standard-mode, fast-mode and fast-mode plus */
> +	if (WARN_ON(t->bus_freq_hz > 1000000))
> +		t->bus_freq_hz = 1000000;
>   
>   	/* prevent scl_rate_khz from becoming 0 */
>   	if (WARN_ON(t->bus_freq_hz < 1000))


-- 
Thanks,
Caesar

Re: [PATCH v8 1/8] i2c: rk3x: add documentation to fields in "struct rk3x_i2c"

[Heiko Stuebner]

Am Mittwoch, 11. Mai 2016, 03:24:05 schrieb David Wu:

In general, all patches should have a commit message (even if it's obvious) 
and some maintainers even enforce this in a hard way.

How about something like:

----
Add kernel-doc documentation for the elements of the previously
undocumented struct rk3x_i2c.
----

Some minor nitpicks below, otherwise
Reviewed-by: Heiko Stuebner <heiko@sntech.de>

> Signed-off-by: David Wu <david.wu@rock-chips.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> ---
> Change in v8:
> - none
> 
>  drivers/i2c/busses/i2c-rk3x.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 80bed02..7e45d51 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -90,6 +90,26 @@ struct rk3x_i2c_soc_data {
>  	int grf_offset;
>  };
> 
> +/**
> + * struct rk3x_i2c - private data of the controller
> + * @adap: corresponding I2C adapter
> + * @dev: device for this controller
> + * @soc_data: related soc data struct
> + * @regs: virtual memory area
> + * @clk: clock of i2c bus
> + * @clk_rate_nb: i2c clk rate change notify
		^ i2c clk rate change notifier block

> + * @t: I2C known timing information
> + * @lock: spinlock for the i2c bus
> + * @wait: the waitqueue to wait for i2c transfer
> + * @busy: the condition for the event to wait for
> + * @msg: current i2c message
> + * @addr: addr of i2c slave device
> + * @mode: mode of i2c transfer
> + * @is_last_msg: flag determines whether it is the last msg in this
> transfer + * @state: state of i2c transfer
> + * @processed: byte length which has been send or received
										^ sent (with a "t")

> + * @error: error code for i2c transfer
> + */
>  struct rk3x_i2c {
>  	struct i2c_adapter adap;
>  	struct device *dev;
> @@ -116,7 +136,7 @@ struct rk3x_i2c {
> 
>  	/* I2C state machine */
>  	enum rk3x_i2c_state state;
> -	unsigned int processed; /* sent/received bytes */
> +	unsigned int processed;
>  	int error;
>  };

[PATCH v8 1/8] i2c: rk3x: add documentation to fields in "struct rk3x_i2c"

[Heiko Stuebner]

Am Mittwoch, 11. Mai 2016, 03:24:05 schrieb David Wu:

In general, all patches should have a commit message (even if it's obvious) 
and some maintainers even enforce this in a hard way.

How about something like:

----
Add kernel-doc documentation for the elements of the previously
undocumented struct rk3x_i2c.
----

Some minor nitpicks below, otherwise
Reviewed-by: Heiko Stuebner <heiko@sntech.de>

> Signed-off-by: David Wu <david.wu@rock-chips.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> ---
> Change in v8:
> - none
> 
>  drivers/i2c/busses/i2c-rk3x.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 80bed02..7e45d51 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -90,6 +90,26 @@ struct rk3x_i2c_soc_data {
>  	int grf_offset;
>  };
> 
> +/**
> + * struct rk3x_i2c - private data of the controller
> + * @adap: corresponding I2C adapter
> + * @dev: device for this controller
> + * @soc_data: related soc data struct
> + * @regs: virtual memory area
> + * @clk: clock of i2c bus
> + * @clk_rate_nb: i2c clk rate change notify
		^ i2c clk rate change notifier block

> + * @t: I2C known timing information
> + * @lock: spinlock for the i2c bus
> + * @wait: the waitqueue to wait for i2c transfer
> + * @busy: the condition for the event to wait for
> + * @msg: current i2c message
> + * @addr: addr of i2c slave device
> + * @mode: mode of i2c transfer
> + * @is_last_msg: flag determines whether it is the last msg in this
> transfer + * @state: state of i2c transfer
> + * @processed: byte length which has been send or received
										^ sent (with a "t")

> + * @error: error code for i2c transfer
> + */
>  struct rk3x_i2c {
>  	struct i2c_adapter adap;
>  	struct device *dev;
> @@ -116,7 +136,7 @@ struct rk3x_i2c {
> 
>  	/* I2C state machine */
>  	enum rk3x_i2c_state state;
> -	unsigned int processed; /* sent/received bytes */
> +	unsigned int processed;
>  	int error;
>  };

Re: [PATCH v8 6/8] dt-bindings: i2c: rk3x: add support for rk3399

[Doug Anderson]

Hi,

On Tue, May 10, 2016 at 12:30 PM, David Wu <david.wu@rock-chips.com> wrote:
> The bus clock and function clock are separated at rk3399,
> and others use one clock as the bus clock and function clock.
>
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> ---
> Change in v8:
> - remove error description.
>
>  Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> index 0b4a85f..5429301 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> @@ -6,10 +6,20 @@ RK3xxx SoCs.
>  Required properties :
>
>   - reg : Offset and length of the register set for the device
> - - compatible : should be "rockchip,rk3066-i2c", "rockchip,rk3188-i2c",
> -               "rockchip,rk3228-i2c" or "rockchip,rk3288-i2c".
> + - compatible: should be one of the following:
> +   - "rockchip,rk3066-i2c": for rk3066
> +   - "rockchip,rk3188-i2c": for rk3188
> +   - "rockchip,rk3228-i2c": for rk3228
> +   - "rockchip,rk3288-i2c": for rk3288
> +   - "rockchip,rk3399-i2c": for rk3399
>   - interrupts : interrupt number
> - - clocks : parent clock
> + - clocks: See ../clock/clock-bindings.txt
> +   - For older hardware (rk3066, rk3188, rk3228, rk3288):
> +     - There is one clock that's used both to derive the functional clock
> +       for the device and as the bus clock.
> +   - For newer hardware (rk3399): specified by name
> +     - "i2c": REQUIRED. This is used to derive the functional clock.
> +     - "pclk": REQUIRED. This is the bus clock.

Depending on what Rob thinks, it might make sense to remove the above
two "REQUIRED" bits.  That would match his earlier feedback since
we're still in the "Required" section and thus it is redundant.

-Doug

[PATCH v8 6/8] dt-bindings: i2c: rk3x: add support for rk3399

[Doug Anderson]

Hi,

On Tue, May 10, 2016 at 12:30 PM, David Wu <david.wu@rock-chips.com> wrote:
> The bus clock and function clock are separated at rk3399,
> and others use one clock as the bus clock and function clock.
>
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> ---
> Change in v8:
> - remove error description.
>
>  Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> index 0b4a85f..5429301 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> @@ -6,10 +6,20 @@ RK3xxx SoCs.
>  Required properties :
>
>   - reg : Offset and length of the register set for the device
> - - compatible : should be "rockchip,rk3066-i2c", "rockchip,rk3188-i2c",
> -               "rockchip,rk3228-i2c" or "rockchip,rk3288-i2c".
> + - compatible: should be one of the following:
> +   - "rockchip,rk3066-i2c": for rk3066
> +   - "rockchip,rk3188-i2c": for rk3188
> +   - "rockchip,rk3228-i2c": for rk3228
> +   - "rockchip,rk3288-i2c": for rk3288
> +   - "rockchip,rk3399-i2c": for rk3399
>   - interrupts : interrupt number
> - - clocks : parent clock
> + - clocks: See ../clock/clock-bindings.txt
> +   - For older hardware (rk3066, rk3188, rk3228, rk3288):
> +     - There is one clock that's used both to derive the functional clock
> +       for the device and as the bus clock.
> +   - For newer hardware (rk3399): specified by name
> +     - "i2c": REQUIRED. This is used to derive the functional clock.
> +     - "pclk": REQUIRED. This is the bus clock.

Depending on what Rob thinks, it might make sense to remove the above
two "REQUIRED" bits.  That would match his earlier feedback since
we're still in the "Required" section and thus it is redundant.

-Doug

Re: [PATCH v8 7/8] i2c: rk3x: add i2c support for rk3399 soc

[Doug Anderson]

Hi,

On Tue, May 10, 2016 at 12:31 PM, David Wu <david.wu@rock-chips.com> wrote:
>  static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
>  {
>         struct i2c_timings *t = &i2c->t;
>         struct rk3x_i2c_calced_timings calc;
>         u64 t_low_ns, t_high_ns;
> +       u32 val;
>         int ret;
>
> -       ret = rk3x_i2c_calc_divs(clk_rate, t, &calc);
> +       ret = i2c->soc_data->calc_timings(clk_rate, t, &calc);
>         WARN_ONCE(ret != 0, "Could not reach SCL freq %u", t->bus_freq_hz);
>
> -       clk_enable(i2c->clk);
> +       clk_enable(i2c->pclk);
> +
>         i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
>                    REG_CLKDIV);
> -       clk_disable(i2c->clk);
> +
> +       val = i2c_readl(i2c, REG_CON);
> +       val &= ~REG_CON_TUNING_MASK;
> +       val |= calc.tuning;
> +       i2c_writel(i2c, val, REG_CON);

Another subtle bug here.  You need to be holding the spinlock here
since you're doing a read-modify-write of a register that is also
touched by the interrupt handler.  We never needed it before because
the previous register update wasn't touched by anyone else and it was
a single atomic write.

Also: technically if we are midway through a transfer when all this
happens then there will be a very short period of time when the two
timing-related registers won't match with each other.  I have no idea
how much that would matter, but in the very least it seems wise to
minimize the time where they mismatch.  So I'd probably write:

       spin_lock_irqsave(&i2c->lock, flags);
       val = i2c_readl(i2c, REG_CON);
       val &= ~REG_CON_TUNING_MASK;
       val |= calc.tuning;
       i2c_writel(i2c, val, REG_CON);
       i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
                  REG_CLKDIV);
       spin_unlock_irqrestore(&i2c->lock, flags);

...if we really end up with on a system with a dynamically changing
clock that uses the new-style timing and we see real problems, we can
always try to come up with a way to avoid any problems.  Sound OK?


Otherwise, I think things look good to me.  Caesar's comments would
also be good to fix.


-Doug

[PATCH v8 7/8] i2c: rk3x: add i2c support for rk3399 soc

[Doug Anderson]

Hi,

On Tue, May 10, 2016 at 12:31 PM, David Wu <david.wu@rock-chips.com> wrote:
>  static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
>  {
>         struct i2c_timings *t = &i2c->t;
>         struct rk3x_i2c_calced_timings calc;
>         u64 t_low_ns, t_high_ns;
> +       u32 val;
>         int ret;
>
> -       ret = rk3x_i2c_calc_divs(clk_rate, t, &calc);
> +       ret = i2c->soc_data->calc_timings(clk_rate, t, &calc);
>         WARN_ONCE(ret != 0, "Could not reach SCL freq %u", t->bus_freq_hz);
>
> -       clk_enable(i2c->clk);
> +       clk_enable(i2c->pclk);
> +
>         i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
>                    REG_CLKDIV);
> -       clk_disable(i2c->clk);
> +
> +       val = i2c_readl(i2c, REG_CON);
> +       val &= ~REG_CON_TUNING_MASK;
> +       val |= calc.tuning;
> +       i2c_writel(i2c, val, REG_CON);

Another subtle bug here.  You need to be holding the spinlock here
since you're doing a read-modify-write of a register that is also
touched by the interrupt handler.  We never needed it before because
the previous register update wasn't touched by anyone else and it was
a single atomic write.

Also: technically if we are midway through a transfer when all this
happens then there will be a very short period of time when the two
timing-related registers won't match with each other.  I have no idea
how much that would matter, but in the very least it seems wise to
minimize the time where they mismatch.  So I'd probably write:

       spin_lock_irqsave(&i2c->lock, flags);
       val = i2c_readl(i2c, REG_CON);
       val &= ~REG_CON_TUNING_MASK;
       val |= calc.tuning;
       i2c_writel(i2c, val, REG_CON);
       i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
                  REG_CLKDIV);
       spin_unlock_irqrestore(&i2c->lock, flags);

...if we really end up with on a system with a dynamically changing
clock that uses the new-style timing and we see real problems, we can
always try to come up with a way to avoid any problems.  Sound OK?


Otherwise, I think things look good to me.  Caesar's comments would
also be good to fix.


-Doug

Re: [PATCH v8 6/8] dt-bindings: i2c: rk3x: add support for rk3399

[Heiko Stuebner]

Am Mittwoch, 11. Mai 2016, 03:30:47 schrieb David Wu:
> The bus clock and function clock are separated at rk3399,
> and others use one clock as the bus clock and function clock.
> 
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

I'd second what Doug said wrt. the removal of the unnecessary "REQUIRED" 
statements of the clock names, especially as the surrounding text makes that 
requirement pretty clear.

Otherwiese looks nice, so

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

> ---
> Change in v8:
> - remove error description.
> 
>  Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt index
> 0b4a85f..5429301 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> @@ -6,10 +6,20 @@ RK3xxx SoCs.
>  Required properties :
> 
>   - reg : Offset and length of the register set for the device
> - - compatible : should be "rockchip,rk3066-i2c", "rockchip,rk3188-i2c",
> -		"rockchip,rk3228-i2c" or "rockchip,rk3288-i2c".
> + - compatible: should be one of the following:
> +   - "rockchip,rk3066-i2c": for rk3066
> +   - "rockchip,rk3188-i2c": for rk3188
> +   - "rockchip,rk3228-i2c": for rk3228
> +   - "rockchip,rk3288-i2c": for rk3288
> +   - "rockchip,rk3399-i2c": for rk3399
>   - interrupts : interrupt number
> - - clocks : parent clock
> + - clocks: See ../clock/clock-bindings.txt
> +   - For older hardware (rk3066, rk3188, rk3228, rk3288):
> +     - There is one clock that's used both to derive the functional clock
> +       for the device and as the bus clock.
> +   - For newer hardware (rk3399): specified by name
> +     - "i2c": REQUIRED. This is used to derive the functional clock.
> +     - "pclk": REQUIRED. This is the bus clock.
> 
>  Required on RK3066, RK3188 :

[PATCH v8 6/8] dt-bindings: i2c: rk3x: add support for rk3399

[Heiko Stuebner]

Am Mittwoch, 11. Mai 2016, 03:30:47 schrieb David Wu:
> The bus clock and function clock are separated at rk3399,
> and others use one clock as the bus clock and function clock.
> 
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

I'd second what Doug said wrt. the removal of the unnecessary "REQUIRED" 
statements of the clock names, especially as the surrounding text makes that 
requirement pretty clear.

Otherwiese looks nice, so

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

> ---
> Change in v8:
> - remove error description.
> 
>  Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt index
> 0b4a85f..5429301 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> @@ -6,10 +6,20 @@ RK3xxx SoCs.
>  Required properties :
> 
>   - reg : Offset and length of the register set for the device
> - - compatible : should be "rockchip,rk3066-i2c", "rockchip,rk3188-i2c",
> -		"rockchip,rk3228-i2c" or "rockchip,rk3288-i2c".
> + - compatible: should be one of the following:
> +   - "rockchip,rk3066-i2c": for rk3066
> +   - "rockchip,rk3188-i2c": for rk3188
> +   - "rockchip,rk3228-i2c": for rk3228
> +   - "rockchip,rk3288-i2c": for rk3288
> +   - "rockchip,rk3399-i2c": for rk3399
>   - interrupts : interrupt number
> - - clocks : parent clock
> + - clocks: See ../clock/clock-bindings.txt
> +   - For older hardware (rk3066, rk3188, rk3228, rk3288):
> +     - There is one clock that's used both to derive the functional clock
> +       for the device and as the bus clock.
> +   - For newer hardware (rk3399): specified by name
> +     - "i2c": REQUIRED. This is used to derive the functional clock.
> +     - "pclk": REQUIRED. This is the bus clock.
> 
>  Required on RK3066, RK3188 :

Re: [PATCH v8 3/8] i2c: rk3x: Remove redundant rk3x_i2c_clean_ipd()

[Heiko Stuebner]

Am Mittwoch, 11. Mai 2016, 03:24:07 schrieb David Wu:
> Call rk3x_i2c_setup() before rk3x_i2c_start()

That beginning of the sentence could use a tiny improvement, like

----
rk3x_i2c_setup() gets called directly before rk3x_i2c_start()
----

to make it obvious what happens in the code.

But a nice small improvement
Reviewed-by: Heiko Stuebner <heiko@sntech.de>

> and the last thing in setup was to clean the IPD,
> so no reason to do it at the beginning of start.
> 
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> ---
> Change in v8:
> - none
> 
>  drivers/i2c/busses/i2c-rk3x.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 1e2677a..9eeb4e5 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -174,7 +174,6 @@ static void rk3x_i2c_start(struct rk3x_i2c *i2c)
>  {
>  	u32 val;
> 
> -	rk3x_i2c_clean_ipd(i2c);
>  	i2c_writel(i2c, REG_INT_START, REG_IEN);
> 
>  	/* enable adapter with correct mode, send START condition */

[PATCH v8 3/8] i2c: rk3x: Remove redundant rk3x_i2c_clean_ipd()

[Heiko Stuebner]

Am Mittwoch, 11. Mai 2016, 03:24:07 schrieb David Wu:
> Call rk3x_i2c_setup() before rk3x_i2c_start()

That beginning of the sentence could use a tiny improvement, like

----
rk3x_i2c_setup() gets called directly before rk3x_i2c_start()
----

to make it obvious what happens in the code.

But a nice small improvement
Reviewed-by: Heiko Stuebner <heiko@sntech.de>

> and the last thing in setup was to clean the IPD,
> so no reason to do it at the beginning of start.
> 
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> ---
> Change in v8:
> - none
> 
>  drivers/i2c/busses/i2c-rk3x.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 1e2677a..9eeb4e5 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -174,7 +174,6 @@ static void rk3x_i2c_start(struct rk3x_i2c *i2c)
>  {
>  	u32 val;
> 
> -	rk3x_i2c_clean_ipd(i2c);
>  	i2c_writel(i2c, REG_INT_START, REG_IEN);
> 
>  	/* enable adapter with correct mode, send START condition */

Re: [PATCH v8 8/8] i2c: rk3x: support fast-mode plus for rk3399

[Heiko Stuebner]

Am Mittwoch, 11. Mai 2016, 03:33:14 schrieb David Wu:

please always try to also provide some sort of commit message.

---
Implement fast mode plus that allows bus speeds of up to 1MHz.
.....

---

Additionally, the i2c-noob in me would wish for a short sentence on where 
the timing values come from (measured, somewhere in the TRM I didn't find or 
something completely different).


Patch content itself looks good, so with a suitable commit message,
Reviewed-by: Heiko Stuebner <heiko@sntech.de>


> Signed-off-by: David Wu <david.wu@rock-chips.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> ---
> Change in v8:
> - None
> 
>  drivers/i2c/busses/i2c-rk3x.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 25ed1ad..0ba25ee 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -126,6 +126,17 @@ static const struct i2c_spec_values fast_mode_spec =
> { .min_hold_buffer_ns = 1300,
>  };
> 
> +static const struct i2c_spec_values fast_mode_plus_spec = {
> +	.min_hold_start_ns = 260,
> +	.min_low_ns = 500,
> +	.min_high_ns = 260,
> +	.min_setup_start_ns = 260,
> +	.max_data_hold_ns = 400,
> +	.min_data_setup_ns = 50,
> +	.min_setup_stop_ns = 260,
> +	.min_hold_buffer_ns = 500,
> +};
> +
>  /**
>   * struct rk3x_i2c_calced_timings:
>   * @div_low: Divider output for low
> @@ -531,8 +542,10 @@ static const struct i2c_spec_values
> *rk3x_i2c_get_spec(unsigned int speed) {
>  	if (speed <= 100000)
>  		return &standard_mode_spec;
> -	else
> +	else if (speed <= 400000)
>  		return &fast_mode_spec;
> +	else
> +		return &fast_mode_plus_spec;
>  }
> 
>  /**
> @@ -743,9 +756,9 @@ static int rk3x_i2c_v1_calc_timings(unsigned long
> clk_rate, const struct i2c_spec_values *spec;
>  	int ret = 0;
> 
> -	/* Support standard-mode and fast-mode */
> -	if (WARN_ON(t->bus_freq_hz > 400000))
> -		t->bus_freq_hz = 400000;
> +	/* Support standard-mode, fast-mode and fast-mode plus */
> +	if (WARN_ON(t->bus_freq_hz > 1000000))
> +		t->bus_freq_hz = 1000000;
> 
>  	/* prevent scl_rate_khz from becoming 0 */
>  	if (WARN_ON(t->bus_freq_hz < 1000))

[PATCH v8 8/8] i2c: rk3x: support fast-mode plus for rk3399

[Heiko Stuebner]

Am Mittwoch, 11. Mai 2016, 03:33:14 schrieb David Wu:

please always try to also provide some sort of commit message.

---
Implement fast mode plus that allows bus speeds of up to 1MHz.
.....

---

Additionally, the i2c-noob in me would wish for a short sentence on where 
the timing values come from (measured, somewhere in the TRM I didn't find or 
something completely different).


Patch content itself looks good, so with a suitable commit message,
Reviewed-by: Heiko Stuebner <heiko@sntech.de>


> Signed-off-by: David Wu <david.wu@rock-chips.com>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> ---
> Change in v8:
> - None
> 
>  drivers/i2c/busses/i2c-rk3x.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 25ed1ad..0ba25ee 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -126,6 +126,17 @@ static const struct i2c_spec_values fast_mode_spec =
> { .min_hold_buffer_ns = 1300,
>  };
> 
> +static const struct i2c_spec_values fast_mode_plus_spec = {
> +	.min_hold_start_ns = 260,
> +	.min_low_ns = 500,
> +	.min_high_ns = 260,
> +	.min_setup_start_ns = 260,
> +	.max_data_hold_ns = 400,
> +	.min_data_setup_ns = 50,
> +	.min_setup_stop_ns = 260,
> +	.min_hold_buffer_ns = 500,
> +};
> +
>  /**
>   * struct rk3x_i2c_calced_timings:
>   * @div_low: Divider output for low
> @@ -531,8 +542,10 @@ static const struct i2c_spec_values
> *rk3x_i2c_get_spec(unsigned int speed) {
>  	if (speed <= 100000)
>  		return &standard_mode_spec;
> -	else
> +	else if (speed <= 400000)
>  		return &fast_mode_spec;
> +	else
> +		return &fast_mode_plus_spec;
>  }
> 
>  /**
> @@ -743,9 +756,9 @@ static int rk3x_i2c_v1_calc_timings(unsigned long
> clk_rate, const struct i2c_spec_values *spec;
>  	int ret = 0;
> 
> -	/* Support standard-mode and fast-mode */
> -	if (WARN_ON(t->bus_freq_hz > 400000))
> -		t->bus_freq_hz = 400000;
> +	/* Support standard-mode, fast-mode and fast-mode plus */
> +	if (WARN_ON(t->bus_freq_hz > 1000000))
> +		t->bus_freq_hz = 1000000;
> 
>  	/* prevent scl_rate_khz from becoming 0 */
>  	if (WARN_ON(t->bus_freq_hz < 1000))

Re: [PATCH v8 8/8] i2c: rk3x: support fast-mode plus for rk3399

[Doug Anderson]

Heiko,

On Wed, May 11, 2016 at 2:09 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> Additionally, the i2c-noob in me would wish for a short sentence on where
> the timing values come from (measured, somewhere in the TRM I didn't find or
> something completely different).

If you search for "UM10204" you'll find the first link is
<http://www.nxp.com/documents/user_manual/UM10204.pdf>.  That appears
to be the official standard.  At least that's what
<http://www.i2c-bus.org/fast-mode-plus/> says (though they have an old
link).

Agree that it wouldn't hurt to mention that in the commit message.

-Doug

Re: [PATCH v8 8/8] i2c: rk3x: support fast-mode plus for rk3399

[Doug Anderson]

Heiko,

On Wed, May 11, 2016 at 2:09 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> Additionally, the i2c-noob in me would wish for a short sentence on where
> the timing values come from (measured, somewhere in the TRM I didn't find or
> something completely different).

If you search for "UM10204" you'll find the first link is
<http://www.nxp.com/documents/user_manual/UM10204.pdf>.  That appears
to be the official standard.  At least that's what
<http://www.i2c-bus.org/fast-mode-plus/> says (though they have an old
link).

Agree that it wouldn't hurt to mention that in the commit message.

-Doug

[PATCH v8 8/8] i2c: rk3x: support fast-mode plus for rk3399

[Doug Anderson]

Heiko,

On Wed, May 11, 2016 at 2:09 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> Additionally, the i2c-noob in me would wish for a short sentence on where
> the timing values come from (measured, somewhere in the TRM I didn't find or
> something completely different).

If you search for "UM10204" you'll find the first link is
<http://www.nxp.com/documents/user_manual/UM10204.pdf>.  That appears
to be the official standard.  At least that's what
<http://www.i2c-bus.org/fast-mode-plus/> says (though they have an old
link).

Agree that it wouldn't hurt to mention that in the commit message.

-Doug

Re: [PATCH v8 7/8] i2c: rk3x: add i2c support for rk3399 soc

[David.Wu]

Hi Doug,

在 2016/5/12 1:37, Doug Anderson 写道:
> Hi,
>
> On Tue, May 10, 2016 at 12:31 PM, David Wu <david.wu@rock-chips.com> wrote:
>>   static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
>>   {
>>          struct i2c_timings *t = &i2c->t;
>>          struct rk3x_i2c_calced_timings calc;
>>          u64 t_low_ns, t_high_ns;
>> +       u32 val;
>>          int ret;
>>
>> -       ret = rk3x_i2c_calc_divs(clk_rate, t, &calc);
>> +       ret = i2c->soc_data->calc_timings(clk_rate, t, &calc);
>>          WARN_ONCE(ret != 0, "Could not reach SCL freq %u", t->bus_freq_hz);
>>
>> -       clk_enable(i2c->clk);
>> +       clk_enable(i2c->pclk);
>> +
>>          i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
>>                     REG_CLKDIV);
>> -       clk_disable(i2c->clk);
>> +
>> +       val = i2c_readl(i2c, REG_CON);
>> +       val &= ~REG_CON_TUNING_MASK;
>> +       val |= calc.tuning;
>> +       i2c_writel(i2c, val, REG_CON);
>
> Another subtle bug here.  You need to be holding the spinlock here
> since you're doing a read-modify-write of a register that is also
> touched by the interrupt handler.  We never needed it before because
> the previous register update wasn't touched by anyone else and it was
> a single atomic write.
>
> Also: technically if we are midway through a transfer when all this
> happens then there will be a very short period of time when the two
> timing-related registers won't match with each other.  I have no idea
> how much that would matter, but in the very least it seems wise to
> minimize the time where they mismatch.  So I'd probably write:
>
>         spin_lock_irqsave(&i2c->lock, flags);
>         val = i2c_readl(i2c, REG_CON);
>         val &= ~REG_CON_TUNING_MASK;
>         val |= calc.tuning;
>         i2c_writel(i2c, val, REG_CON);
>         i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
>                    REG_CLKDIV);
>         spin_unlock_irqrestore(&i2c->lock, flags);
>
> ...if we really end up with on a system with a dynamically changing
> clock that uses the new-style timing and we see real problems, we can
> always try to come up with a way to avoid any problems.  Sound OK?
>
>

Good, add spin_lock is very necessary for atomic write here, thanks for 
your advice.

> Otherwise, I think things look good to me.  Caesar's comments would
> also be good to fix.
>
>
> -Doug
>
>
>

[PATCH v8 7/8] i2c: rk3x: add i2c support for rk3399 soc

[David.Wu]

Hi Doug,

? 2016/5/12 1:37, Doug Anderson ??:
> Hi,
>
> On Tue, May 10, 2016 at 12:31 PM, David Wu <david.wu@rock-chips.com> wrote:
>>   static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
>>   {
>>          struct i2c_timings *t = &i2c->t;
>>          struct rk3x_i2c_calced_timings calc;
>>          u64 t_low_ns, t_high_ns;
>> +       u32 val;
>>          int ret;
>>
>> -       ret = rk3x_i2c_calc_divs(clk_rate, t, &calc);
>> +       ret = i2c->soc_data->calc_timings(clk_rate, t, &calc);
>>          WARN_ONCE(ret != 0, "Could not reach SCL freq %u", t->bus_freq_hz);
>>
>> -       clk_enable(i2c->clk);
>> +       clk_enable(i2c->pclk);
>> +
>>          i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
>>                     REG_CLKDIV);
>> -       clk_disable(i2c->clk);
>> +
>> +       val = i2c_readl(i2c, REG_CON);
>> +       val &= ~REG_CON_TUNING_MASK;
>> +       val |= calc.tuning;
>> +       i2c_writel(i2c, val, REG_CON);
>
> Another subtle bug here.  You need to be holding the spinlock here
> since you're doing a read-modify-write of a register that is also
> touched by the interrupt handler.  We never needed it before because
> the previous register update wasn't touched by anyone else and it was
> a single atomic write.
>
> Also: technically if we are midway through a transfer when all this
> happens then there will be a very short period of time when the two
> timing-related registers won't match with each other.  I have no idea
> how much that would matter, but in the very least it seems wise to
> minimize the time where they mismatch.  So I'd probably write:
>
>         spin_lock_irqsave(&i2c->lock, flags);
>         val = i2c_readl(i2c, REG_CON);
>         val &= ~REG_CON_TUNING_MASK;
>         val |= calc.tuning;
>         i2c_writel(i2c, val, REG_CON);
>         i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
>                    REG_CLKDIV);
>         spin_unlock_irqrestore(&i2c->lock, flags);
>
> ...if we really end up with on a system with a dynamically changing
> clock that uses the new-style timing and we see real problems, we can
> always try to come up with a way to avoid any problems.  Sound OK?
>
>

Good, add spin_lock is very necessary for atomic write here, thanks for 
your advice.

> Otherwise, I think things look good to me.  Caesar's comments would
> also be good to fix.
>
>
> -Doug
>
>
>

Re: [PATCH v8 3/8] i2c: rk3x: Remove redundant rk3x_i2c_clean_ipd()

[David.Wu]

Hi Heiko

在 2016/5/12 2:26, Heiko Stuebner 写道:
> Am Mittwoch, 11. Mai 2016, 03:24:07 schrieb David Wu:
>> Call rk3x_i2c_setup() before rk3x_i2c_start()
>
> That beginning of the sentence could use a tiny improvement, like
>
> ----
> rk3x_i2c_setup() gets called directly before rk3x_i2c_start()
> ----
>

Okay, i will add it in next version, Thanks.

> to make it obvious what happens in the code.
>
> But a nice small improvement
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
>
>> and the last thing in setup was to clean the IPD,
>> so no reason to do it at the beginning of start.
>>
>> Signed-off-by: David Wu <david.wu@rock-chips.com>
>> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>> ---
>> Change in v8:
>> - none
>>
>>   drivers/i2c/busses/i2c-rk3x.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
>> index 1e2677a..9eeb4e5 100644
>> --- a/drivers/i2c/busses/i2c-rk3x.c
>> +++ b/drivers/i2c/busses/i2c-rk3x.c
>> @@ -174,7 +174,6 @@ static void rk3x_i2c_start(struct rk3x_i2c *i2c)
>>   {
>>   	u32 val;
>>
>> -	rk3x_i2c_clean_ipd(i2c);
>>   	i2c_writel(i2c, REG_INT_START, REG_IEN);
>>
>>   	/* enable adapter with correct mode, send START condition */
>
>
>
>

[PATCH v8 3/8] i2c: rk3x: Remove redundant rk3x_i2c_clean_ipd()

[David.Wu]

Hi Heiko

? 2016/5/12 2:26, Heiko Stuebner ??:
> Am Mittwoch, 11. Mai 2016, 03:24:07 schrieb David Wu:
>> Call rk3x_i2c_setup() before rk3x_i2c_start()
>
> That beginning of the sentence could use a tiny improvement, like
>
> ----
> rk3x_i2c_setup() gets called directly before rk3x_i2c_start()
> ----
>

Okay, i will add it in next version, Thanks.

> to make it obvious what happens in the code.
>
> But a nice small improvement
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
>
>> and the last thing in setup was to clean the IPD,
>> so no reason to do it at the beginning of start.
>>
>> Signed-off-by: David Wu <david.wu@rock-chips.com>
>> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>> ---
>> Change in v8:
>> - none
>>
>>   drivers/i2c/busses/i2c-rk3x.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
>> index 1e2677a..9eeb4e5 100644
>> --- a/drivers/i2c/busses/i2c-rk3x.c
>> +++ b/drivers/i2c/busses/i2c-rk3x.c
>> @@ -174,7 +174,6 @@ static void rk3x_i2c_start(struct rk3x_i2c *i2c)
>>   {
>>   	u32 val;
>>
>> -	rk3x_i2c_clean_ipd(i2c);
>>   	i2c_writel(i2c, REG_INT_START, REG_IEN);
>>
>>   	/* enable adapter with correct mode, send START condition */
>
>
>
>

Re: [PATCH v8 6/8] dt-bindings: i2c: rk3x: add support for rk3399

[David.Wu]

Hi Doug,

在 2016/5/12 0:35, Doug Anderson 写道:
> Hi,
>
> On Tue, May 10, 2016 at 12:30 PM, David Wu <david.wu@rock-chips.com> wrote:
>> The bus clock and function clock are separated at rk3399,
>> and others use one clock as the bus clock and function clock.
>>
>> Signed-off-by: David Wu <david.wu@rock-chips.com>
>> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>> ---
>> Change in v8:
>> - remove error description.
>>
>>   Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
>> index 0b4a85f..5429301 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
>> @@ -6,10 +6,20 @@ RK3xxx SoCs.
>>   Required properties :
>>
>>    - reg : Offset and length of the register set for the device
>> - - compatible : should be "rockchip,rk3066-i2c", "rockchip,rk3188-i2c",
>> -               "rockchip,rk3228-i2c" or "rockchip,rk3288-i2c".
>> + - compatible: should be one of the following:
>> +   - "rockchip,rk3066-i2c": for rk3066
>> +   - "rockchip,rk3188-i2c": for rk3188
>> +   - "rockchip,rk3228-i2c": for rk3228
>> +   - "rockchip,rk3288-i2c": for rk3288
>> +   - "rockchip,rk3399-i2c": for rk3399
>>    - interrupts : interrupt number
>> - - clocks : parent clock
>> + - clocks: See ../clock/clock-bindings.txt
>> +   - For older hardware (rk3066, rk3188, rk3228, rk3288):
>> +     - There is one clock that's used both to derive the functional clock
>> +       for the device and as the bus clock.
>> +   - For newer hardware (rk3399): specified by name
>> +     - "i2c": REQUIRED. This is used to derive the functional clock.
>> +     - "pclk": REQUIRED. This is the bus clock.
>
> Depending on what Rob thinks, it might make sense to remove the above
> two "REQUIRED" bits.  That would match his earlier feedback since
> we're still in the "Required" section and thus it is redundant.
>

Okay, i make a little misunderstand for that, so i will fix it in next 
version.

> -Doug
>
>
>

[PATCH v8 6/8] dt-bindings: i2c: rk3x: add support for rk3399

[David.Wu]

Hi Doug,

? 2016/5/12 0:35, Doug Anderson ??:
> Hi,
>
> On Tue, May 10, 2016 at 12:30 PM, David Wu <david.wu@rock-chips.com> wrote:
>> The bus clock and function clock are separated at rk3399,
>> and others use one clock as the bus clock and function clock.
>>
>> Signed-off-by: David Wu <david.wu@rock-chips.com>
>> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>> ---
>> Change in v8:
>> - remove error description.
>>
>>   Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
>> index 0b4a85f..5429301 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
>> @@ -6,10 +6,20 @@ RK3xxx SoCs.
>>   Required properties :
>>
>>    - reg : Offset and length of the register set for the device
>> - - compatible : should be "rockchip,rk3066-i2c", "rockchip,rk3188-i2c",
>> -               "rockchip,rk3228-i2c" or "rockchip,rk3288-i2c".
>> + - compatible: should be one of the following:
>> +   - "rockchip,rk3066-i2c": for rk3066
>> +   - "rockchip,rk3188-i2c": for rk3188
>> +   - "rockchip,rk3228-i2c": for rk3228
>> +   - "rockchip,rk3288-i2c": for rk3288
>> +   - "rockchip,rk3399-i2c": for rk3399
>>    - interrupts : interrupt number
>> - - clocks : parent clock
>> + - clocks: See ../clock/clock-bindings.txt
>> +   - For older hardware (rk3066, rk3188, rk3228, rk3288):
>> +     - There is one clock that's used both to derive the functional clock
>> +       for the device and as the bus clock.
>> +   - For newer hardware (rk3399): specified by name
>> +     - "i2c": REQUIRED. This is used to derive the functional clock.
>> +     - "pclk": REQUIRED. This is the bus clock.
>
> Depending on what Rob thinks, it might make sense to remove the above
> two "REQUIRED" bits.  That would match his earlier feedback since
> we're still in the "Required" section and thus it is redundant.
>

Okay, i make a little misunderstand for that, so i will fix it in next 
version.

> -Doug
>
>
>

Re: [PATCH v8 8/8] i2c: rk3x: support fast-mode plus for rk3399

[Heiko Stuebner]

Am Mittwoch, 11. Mai 2016, 16:41:54 schrieb Doug Anderson:
> Heiko,
> 
> On Wed, May 11, 2016 at 2:09 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> > Additionally, the i2c-noob in me would wish for a short sentence on
> > where
> > the timing values come from (measured, somewhere in the TRM I didn't
> > find or something completely different).
> 
> If you search for "UM10204" you'll find the first link is
> <http://www.nxp.com/documents/user_manual/UM10204.pdf>.  That appears
> to be the official standard.  At least that's what
> <http://www.i2c-bus.org/fast-mode-plus/> says (though they have an old
> link).

I did find that document yesterday, but was to blind and somehow only saw the 
separate table for highspeed-timings. On a closer look, I now also found the 
timing-table for the other modes [due to the bigger font-size used there? 
;-) ]

Re: [PATCH v8 8/8] i2c: rk3x: support fast-mode plus for rk3399

[Heiko Stuebner]

Am Mittwoch, 11. Mai 2016, 16:41:54 schrieb Doug Anderson:
> Heiko,
> 
> On Wed, May 11, 2016 at 2:09 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> > Additionally, the i2c-noob in me would wish for a short sentence on
> > where
> > the timing values come from (measured, somewhere in the TRM I didn't
> > find or something completely different).
> 
> If you search for "UM10204" you'll find the first link is
> <http://www.nxp.com/documents/user_manual/UM10204.pdf>.  That appears
> to be the official standard.  At least that's what
> <http://www.i2c-bus.org/fast-mode-plus/> says (though they have an old
> link).

I did find that document yesterday, but was to blind and somehow only saw the 
separate table for highspeed-timings. On a closer look, I now also found the 
timing-table for the other modes [due to the bigger font-size used there? 
;-) ]

[PATCH v8 8/8] i2c: rk3x: support fast-mode plus for rk3399

[Heiko Stuebner]

Am Mittwoch, 11. Mai 2016, 16:41:54 schrieb Doug Anderson:
> Heiko,
> 
> On Wed, May 11, 2016 at 2:09 PM, Heiko Stuebner <heiko@sntech.de> wrote:
> > Additionally, the i2c-noob in me would wish for a short sentence on
> > where
> > the timing values come from (measured, somewhere in the TRM I didn't
> > find or something completely different).
> 
> If you search for "UM10204" you'll find the first link is
> <http://www.nxp.com/documents/user_manual/UM10204.pdf>.  That appears
> to be the official standard.  At least that's what
> <http://www.i2c-bus.org/fast-mode-plus/> says (though they have an old
> link).

I did find that document yesterday, but was to blind and somehow only saw the 
separate table for highspeed-timings. On a closer look, I now also found the 
timing-table for the other modes [due to the bigger font-size used there? 
;-) ]

Re: [PATCH v8 7/8] i2c: rk3x: add i2c support for rk3399 soc

[David.Wu]

Hi Doug，

在 2016/5/12 9:08, David.Wu 写道:
> Hi Doug,
>
> 在 2016/5/12 1:37, Doug Anderson 写道:
>> Hi,
>>
>> On Tue, May 10, 2016 at 12:31 PM, David Wu <david.wu@rock-chips.com>
>> wrote:
>>>   static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long
>>> clk_rate)
>>>   {
>>>          struct i2c_timings *t = &i2c->t;
>>>          struct rk3x_i2c_calced_timings calc;
>>>          u64 t_low_ns, t_high_ns;
>>> +       u32 val;
>>>          int ret;
>>>
>>> -       ret = rk3x_i2c_calc_divs(clk_rate, t, &calc);
>>> +       ret = i2c->soc_data->calc_timings(clk_rate, t, &calc);
>>>          WARN_ONCE(ret != 0, "Could not reach SCL freq %u",
>>> t->bus_freq_hz);
>>>
>>> -       clk_enable(i2c->clk);
>>> +       clk_enable(i2c->pclk);
>>> +
>>>          i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low &
>>> 0xffff),
>>>                     REG_CLKDIV);
>>> -       clk_disable(i2c->clk);
>>> +
>>> +       val = i2c_readl(i2c, REG_CON);
>>> +       val &= ~REG_CON_TUNING_MASK;
>>> +       val |= calc.tuning;
>>> +       i2c_writel(i2c, val, REG_CON);
>>
>> Another subtle bug here.  You need to be holding the spinlock here
>> since you're doing a read-modify-write of a register that is also
>> touched by the interrupt handler.  We never needed it before because
>> the previous register update wasn't touched by anyone else and it was
>> a single atomic write.
>>
>> Also: technically if we are midway through a transfer when all this
>> happens then there will be a very short period of time when the two
>> timing-related registers won't match with each other.  I have no idea
>> how much that would matter, but in the very least it seems wise to
>> minimize the time where they mismatch.  So I'd probably write:
>>
>>         spin_lock_irqsave(&i2c->lock, flags);
>>         val = i2c_readl(i2c, REG_CON);
>>         val &= ~REG_CON_TUNING_MASK;
>>         val |= calc.tuning;
>>         i2c_writel(i2c, val, REG_CON);
>>         i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
>>                    REG_CLKDIV);
>>         spin_unlock_irqrestore(&i2c->lock, flags);
>>
>> ...if we really end up with on a system with a dynamically changing
>> clock that uses the new-style timing and we see real problems, we can
>> always try to come up with a way to avoid any problems.  Sound OK?
>>
>>
>
> Good, add spin_lock is very necessary for atomic write here, thanks for
> your advice.

I also found a subtle bug, it would clean the con register by use 
"i2c_writel(i2c, 0, REG_CON)" in rk3x_i2c_stop(). So i think it would be 
fixed by using read-modify-write way here in next version.

>
>> Otherwise, I think things look good to me.  Caesar's comments would
>> also be good to fix.
>>
>>
>> -Doug
>>
>>
>>

[PATCH v8 7/8] i2c: rk3x: add i2c support for rk3399 soc

[David.Wu]

Hi Doug?

? 2016/5/12 9:08, David.Wu ??:
> Hi Doug,
>
> ? 2016/5/12 1:37, Doug Anderson ??:
>> Hi,
>>
>> On Tue, May 10, 2016 at 12:31 PM, David Wu <david.wu@rock-chips.com>
>> wrote:
>>>   static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long
>>> clk_rate)
>>>   {
>>>          struct i2c_timings *t = &i2c->t;
>>>          struct rk3x_i2c_calced_timings calc;
>>>          u64 t_low_ns, t_high_ns;
>>> +       u32 val;
>>>          int ret;
>>>
>>> -       ret = rk3x_i2c_calc_divs(clk_rate, t, &calc);
>>> +       ret = i2c->soc_data->calc_timings(clk_rate, t, &calc);
>>>          WARN_ONCE(ret != 0, "Could not reach SCL freq %u",
>>> t->bus_freq_hz);
>>>
>>> -       clk_enable(i2c->clk);
>>> +       clk_enable(i2c->pclk);
>>> +
>>>          i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low &
>>> 0xffff),
>>>                     REG_CLKDIV);
>>> -       clk_disable(i2c->clk);
>>> +
>>> +       val = i2c_readl(i2c, REG_CON);
>>> +       val &= ~REG_CON_TUNING_MASK;
>>> +       val |= calc.tuning;
>>> +       i2c_writel(i2c, val, REG_CON);
>>
>> Another subtle bug here.  You need to be holding the spinlock here
>> since you're doing a read-modify-write of a register that is also
>> touched by the interrupt handler.  We never needed it before because
>> the previous register update wasn't touched by anyone else and it was
>> a single atomic write.
>>
>> Also: technically if we are midway through a transfer when all this
>> happens then there will be a very short period of time when the two
>> timing-related registers won't match with each other.  I have no idea
>> how much that would matter, but in the very least it seems wise to
>> minimize the time where they mismatch.  So I'd probably write:
>>
>>         spin_lock_irqsave(&i2c->lock, flags);
>>         val = i2c_readl(i2c, REG_CON);
>>         val &= ~REG_CON_TUNING_MASK;
>>         val |= calc.tuning;
>>         i2c_writel(i2c, val, REG_CON);
>>         i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
>>                    REG_CLKDIV);
>>         spin_unlock_irqrestore(&i2c->lock, flags);
>>
>> ...if we really end up with on a system with a dynamically changing
>> clock that uses the new-style timing and we see real problems, we can
>> always try to come up with a way to avoid any problems.  Sound OK?
>>
>>
>
> Good, add spin_lock is very necessary for atomic write here, thanks for
> your advice.

I also found a subtle bug, it would clean the con register by use 
"i2c_writel(i2c, 0, REG_CON)" in rk3x_i2c_stop(). So i think it would be 
fixed by using read-modify-write way here in next version.

>
>> Otherwise, I think things look good to me.  Caesar's comments would
>> also be good to fix.
>>
>>
>> -Doug
>>
>>
>>

Re: [PATCH v8 7/8] i2c: rk3x: add i2c support for rk3399 soc

[Doug Anderson]

David,

On Thu, May 12, 2016 at 8:07 AM, David.Wu <david.wu@rock-chips.com> wrote:
> I also found a subtle bug, it would clean the con register by use
> "i2c_writel(i2c, 0, REG_CON)" in rk3x_i2c_stop(). So i think it would be
> fixed by using read-modify-write way here in next version.

Good catch.  I look forward to the next version (maybe the last
version, since this series is looking really good).

-Doug

Re: [PATCH v8 7/8] i2c: rk3x: add i2c support for rk3399 soc

[Doug Anderson]

David,

On Thu, May 12, 2016 at 8:07 AM, David.Wu <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> I also found a subtle bug, it would clean the con register by use
> "i2c_writel(i2c, 0, REG_CON)" in rk3x_i2c_stop(). So i think it would be
> fixed by using read-modify-write way here in next version.

Good catch.  I look forward to the next version (maybe the last
version, since this series is looking really good).

-Doug
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v8 7/8] i2c: rk3x: add i2c support for rk3399 soc

[Doug Anderson]

David,

On Thu, May 12, 2016 at 8:07 AM, David.Wu <david.wu@rock-chips.com> wrote:
> I also found a subtle bug, it would clean the con register by use
> "i2c_writel(i2c, 0, REG_CON)" in rk3x_i2c_stop(). So i think it would be
> fixed by using read-modify-write way here in next version.

Good catch.  I look forward to the next version (maybe the last
version, since this series is looking really good).

-Doug