[PATCH v7 0/9] add i2c driver supported for rk3399

[PATCH v7 0/9] add i2c driver supported for rk3399

[David Wu]

There are two points differert from others:
 - new method to caculate i2c timings for rk3399
 - pclk and function clk are separated at rk3399

David Wu (9):
  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: Move setting STATE_START and add STATE_SETUP
  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                      | 482 +++++++++++++++++----
 2 files changed, 419 insertions(+), 79 deletions(-)

-- 
1.9.1

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

[David Wu]

Signed-off-by: David Wu <david.wu@rock-chips.com>
---
Change in v7:
- 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 v7 1/9] i2c: rk3x: add documentation to fields in "struct rk3x_i2c"

[Doug Anderson]

David,

On Wed, May 4, 2016 at 7:13 AM, David Wu <david.wu@rock-chips.com> wrote:
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
> Change in v7:
> - 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

You're a little ahead of yourself.  The @t element doesn't exist as of
this patch.  Instead all the sub-elements are there.  This in this
patch you need to document scl_frequency, scl_rise_ns, ....  In a
later patch you'd change it to t.


-Doug

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

[Doug Anderson]

David,

On Wed, May 4, 2016 at 4:45 PM, Doug Anderson <dianders@chromium.org> wrote:
> David,
>
> On Wed, May 4, 2016 at 7:13 AM, David Wu <david.wu@rock-chips.com> wrote:
>> Signed-off-by: David Wu <david.wu@rock-chips.com>
>> ---
>> Change in v7:
>> - 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
>
> You're a little ahead of yourself.  The @t element doesn't exist as of
> this patch.  Instead all the sub-elements are there.  This in this
> patch you need to document scl_frequency, scl_rise_ns, ....  In a
> later patch you'd change it to t.

Or I'm an idiot and applied to the wrong branch.  Your patch is fine.

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

[PATCH v7 2/9] i2c: rk3x: use struct "rk3x_i2c_calced_timings"

[David Wu]

Signed-off-by: David Wu <david.wu@rock-chips.com>
---
Change in v7:
- none

 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 v7 2/9] i2c: rk3x: use struct "rk3x_i2c_calced_timings"

[Doug Anderson]

David,

On Wed, May 4, 2016 at 7:13 AM, David Wu <david.wu@rock-chips.com> wrote:
> Signed-off-by: David Wu <david.wu@rock-chips.com>

Usually folks like a description and not just a subject line.  I'd add
a description like:

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.

> ---
> Change in v7:
> - none
>
>  drivers/i2c/busses/i2c-rk3x.c | 55 +++++++++++++++++++++++++------------------
>  1 file changed, 32 insertions(+), 23 deletions(-)

Other than the description, I think this looks fine.

IMHO this can be applied any time independent of any earlier patches.

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

[PATCH v7 2/9] i2c: rk3x: use struct "rk3x_i2c_calced_timings"

[David.Wu]

Hi Doug,

? 2016/5/6 6:55, Doug Anderson ??:
> David,
>
> On Wed, May 4, 2016 at 7:13 AM, David Wu <david.wu@rock-chips.com> wrote:
>> Signed-off-by: David Wu <david.wu@rock-chips.com>
>
> Usually folks like a description and not just a subject line.  I'd add
> a description like:
>
> 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.
>

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

>> ---
>> Change in v7:
>> - none
>>
>>   drivers/i2c/busses/i2c-rk3x.c | 55 +++++++++++++++++++++++++------------------
>>   1 file changed, 32 insertions(+), 23 deletions(-)
>
> Other than the description, I think this looks fine.
>
> IMHO this can be applied any time independent of any earlier patches.
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
>
>

[PATCH v7 3/9] 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>
---
Change in v7:
- 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 v7 3/9] i2c: rk3x: Remove redundant rk3x_i2c_clean_ipd()

[Doug Anderson]

David,

On Wed, May 4, 2016 at 7:13 AM, David Wu <david.wu@rock-chips.com> wrote:
> 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>
> ---
> Change in v7:
> - none
>
>  drivers/i2c/busses/i2c-rk3x.c | 1 -
>  1 file changed, 1 deletion(-)

Looks great.  Thanks for splitting this out from other patches--makes
it much more obvious what's happening!  :)

IMHO this can be applied any time independent of any earlier patches.

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

[PATCH v7 4/9] i2c: rk3x: Move setting STATE_START and add STATE_SETUP

[David Wu]

Signed-off-by: David Wu <david.wu@rock-chips.com>
---
Change in v7:
- none

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

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 9eeb4e5..0838fcf 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -87,6 +87,7 @@ struct rk3x_i2c_calced_timings {
 
 enum rk3x_i2c_state {
 	STATE_IDLE,
+	STATE_SETUP,
 	STATE_START,
 	STATE_READ,
 	STATE_WRITE,
@@ -174,6 +175,7 @@ static void rk3x_i2c_start(struct rk3x_i2c *i2c)
 {
 	u32 val;
 
+	i2c->state = STATE_START;
 	i2c_writel(i2c, REG_INT_START, REG_IEN);
 
 	/* enable adapter with correct mode, send START condition */
@@ -451,6 +453,7 @@ static irqreturn_t rk3x_i2c_irq(int irqno, void *dev_id)
 		rk3x_i2c_handle_stop(i2c, ipd);
 		break;
 	case STATE_IDLE:
+	case STATE_SETUP:
 		break;
 	}
 
@@ -781,7 +784,7 @@ static int rk3x_i2c_setup(struct rk3x_i2c *i2c, struct i2c_msg *msgs, int num)
 
 	i2c->addr = msgs[0].addr;
 	i2c->busy = true;
-	i2c->state = STATE_START;
+	i2c->state = STATE_SETUP;
 	i2c->processed = 0;
 	i2c->error = 0;
 
-- 
1.9.1

[PATCH v7 4/9] i2c: rk3x: Move setting STATE_START and add STATE_SETUP

[Doug Anderson]

David,

On Wed, May 4, 2016 at 7:13 AM, David Wu <david.wu@rock-chips.com> wrote:
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
> Change in v7:
> - none
>
>  drivers/i2c/busses/i2c-rk3x.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Probably this change could be dropped now.  Switching the location of
setting START_START was much more important when you were supporting
HIGH SPEED mode.  I don't think we need it anymore, right?

...if we want to keep this change, I'd say:

1. Add a description, like maybe:

To help with debugging add a STATE_SETUP between STATE_IDLE and
STATE_START to make it more obvious that we're not actually idle but we
also haven't initiated the start bit.  This change is not expected to
have any impact but it does delay the changing of state to STATE_START.
If previously we were getting an erroneous interrupt before we actually
sent the start bit we'll now be treating that differently.  The new
behavior (catching the erroneous interrupt) should be better.

2. Change "i2c->state = STATE_SETUP" to the _start_ of
rk3x_i2c_setup().  That would have a better chance of catching a
spurious interrupt.

3. Add an error check at the start of rk3x_i2c_irq() similar to the
check for STATE_IDLE (or use the same check and modify the printk).
Specifically the justification for adding STATE_SETUP is to help with
debugging (catch interrupts that were unexpected and print more info
about our state), so we should make it useful for this.


-Doug

[PATCH v7 4/9] i2c: rk3x: Move setting STATE_START and add STATE_SETUP

[David.Wu]

Hi Doug,

? 2016/5/6 6:56, Doug Anderson ??:
> David,
>
> On Wed, May 4, 2016 at 7:13 AM, David Wu <david.wu@rock-chips.com> wrote:
>> Signed-off-by: David Wu <david.wu@rock-chips.com>
>> ---
>> Change in v7:
>> - none
>>
>>   drivers/i2c/busses/i2c-rk3x.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> Probably this change could be dropped now.  Switching the location of
> setting START_START was much more important when you were supporting
> HIGH SPEED mode.  I don't think we need it anymore, right?
>

Yes, I would drop it next version?STATE_SETUP didn't make sense, it was
not much more important, because there was a error printk for 
rk3x_i2c_setup() called failed.

> ...if we want to keep this change, I'd say:
>
> 1. Add a description, like maybe:
>
> To help with debugging add a STATE_SETUP between STATE_IDLE and
> STATE_START to make it more obvious that we're not actually idle but we
> also haven't initiated the start bit.  This change is not expected to
> have any impact but it does delay the changing of state to STATE_START.
> If previously we were getting an erroneous interrupt before we actually
> sent the start bit we'll now be treating that differently.  The new
> behavior (catching the erroneous interrupt) should be better.
>
> 2. Change "i2c->state = STATE_SETUP" to the _start_ of
> rk3x_i2c_setup().  That would have a better chance of catching a
> spurious interrupt.
>
> 3. Add an error check at the start of rk3x_i2c_irq() similar to the
> check for STATE_IDLE (or use the same check and modify the printk).
> Specifically the justification for adding STATE_SETUP is to help with
> debugging (catch interrupts that were unexpected and print more info
> about our state), so we should make it useful for this.
>
>
> -Doug
>
>
>

[PATCH v7 5/9] i2c: rk3x: Change SoC data to not use array

[David Wu]

Signed-off-by: David Wu <david.wu@rock-chips.com>
---
 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 0838fcf..9686c81 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -867,17 +867,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 v7 5/9] i2c: rk3x: Change SoC data to not use array

[Heiko Stübner]

Am Mittwoch, 4. Mai 2016, 22:33:38 schrieb David Wu:
> Signed-off-by: David Wu <david.wu@rock-chips.com>

that makes the code look nicer (and of course prepares for you further 
changes), so

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

> ---
>  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 0838fcf..9686c81 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -867,17 +867,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);

[PATCH v7 5/9] i2c: rk3x: Change SoC data to not use array

[Doug Anderson]

Hi,

On Wed, May 4, 2016 at 7:33 AM, David Wu <david.wu@rock-chips.com> wrote:
> Signed-off-by: David Wu <david.wu@rock-chips.com>

nit: $SUBJECT should probably mention that this also constifies the SoC data.


Again, needs description, like maybe:

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.


IMHO this can be applied any time independent of any earlier patches.

Suggested-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>

[PATCH v7 6/9] i2c: rk3x: Move spec timing data to "static const" structs

[David Wu]

Signed-off-by: David Wu <david.wu@rock-chips.com>
---
 drivers/i2c/busses/i2c-rk3x.c | 100 ++++++++++++++++++++++++++++++------------
 1 file changed, 72 insertions(+), 28 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 9686c81..408f9ab 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -76,6 +76,51 @@ enum {
 #define DEFAULT_SCL_RATE  (100 * 1000) /* Hz */
 
 /**
+ * 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
@@ -463,6 +508,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
@@ -477,10 +537,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;
 
@@ -492,6 +548,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 */
@@ -514,22 +572,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:
@@ -539,14 +583,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 v7 6/9] i2c: rk3x: Move spec timing data to "static const" structs

[Doug Anderson]

David,

On Wed, May 4, 2016 at 7:34 AM, David Wu <david.wu@rock-chips.com> wrote:
> Signed-off-by: David Wu <david.wu@rock-chips.com>

As you can probably guess, again a description would be nice.  Like maybe:

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.

That helps someone reading the patch understand the motivation.


> @@ -76,6 +76,51 @@ enum {
>  #define DEFAULT_SCL_RATE  (100 * 1000) /* Hz */
>
>  /**
> + * 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,

There are more spec values than are currently used in this patch.
Personally I'm OK with that, but if you wanted to be totally clean
this patch would only include the spec values that were needed, then
introduce the additional values in the rk3399 patch.


> @@ -492,6 +548,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;

aside (feel free to ignore): Gosh, I kinda forgot what the heck this
value was for.  I guess it's not anything in the spec.  I have a
feeling it was some sort of slop value that someone felt was
necessary, but I don't quite remember.  Oh well, I guess we leave it
there since I'd rather not mess with timings on old hardware that are
apparently working for everyone.  :-P


In any case, aside from the missing description:

Suggested-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>

IMHO this can be applied any time independent of any earlier patches.


-Doug

[PATCH v7 6/9] i2c: rk3x: Move spec timing data to "static const" structs

[David.Wu]

Hi Doug,

? 2016/5/6 6:58, Doug Anderson ??:
> David,
>
> On Wed, May 4, 2016 at 7:34 AM, David Wu <david.wu@rock-chips.com> wrote:
>> Signed-off-by: David Wu <david.wu@rock-chips.com>
>
> As you can probably guess, again a description would be nice.  Like maybe:
>
> 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.
>
> That helps someone reading the patch understand the motivation.
>
>
>> @@ -76,6 +76,51 @@ enum {
>>   #define DEFAULT_SCL_RATE  (100 * 1000) /* Hz */
>>
>>   /**
>> + * 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,
>
> There are more spec values than are currently used in this patch.
> Personally I'm OK with that, but if you wanted to be totally clean
> this patch would only include the spec values that were needed, then
> introduce the additional values in the rk3399 patch.
>
>
>> @@ -492,6 +548,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;
>
> aside (feel free to ignore): Gosh, I kinda forgot what the heck this
> value was for.  I guess it's not anything in the spec.  I have a
> feeling it was some sort of slop value that someone felt was
> necessary, but I don't quite remember.  Oh well, I guess we leave it
> there since I'd rather not mess with timings on old hardware that are
> apparently working for everyone.  :-P
>
>

I thing it was a tuning value for max_t_low_ns. :-P

> In any case, aside from the missing description:
>
> Suggested-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> IMHO this can be applied any time independent of any earlier patches.
>
>
> -Doug
>
>
>

[PATCH v7 7/9] 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>
---
 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..82b6f6b 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 followings
+   - "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.  REQUIRED.
+   - For newer hardware (rk3399): specify by name
+     - "i2c": REQUIRED. This is used to derive the functional clock.
+     - "pclk": REQUIRED. This is the bus clock.equired on RK3066, RK3188 :
 
 Required on RK3066, RK3188 :
 
-- 
1.9.1

[PATCH v7 7/9] dt-bindings: i2c: rk3x: add support for rk3399

[Rob Herring]

On Wed, May 04, 2016 at 10:35:23PM +0800, David Wu 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>
> ---
>  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..82b6f6b 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 followings

s/followings/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.  REQUIRED.

This is the required section, so REQUIRED is redundant.

> +   - For newer hardware (rk3399): specify by name
> +     - "i2c": REQUIRED. This is used to derive the functional clock.
> +     - "pclk": REQUIRED. This is the bus clock.equired on RK3066, RK3188 :

This doesn't make sense. The first line says this applies to rk3399, but 
then here it is talking about other parts.

And there's a typo.

>  
>  Required on RK3066, RK3188 :
>  
> -- 
> 1.9.1
> 
> 

[PATCH v7 7/9] dt-bindings: i2c: rk3x: add support for rk3399

[David.Wu]

Hi Rob,

? 2016/5/6 6:12, Rob Herring ??:
> On Wed, May 04, 2016 at 10:35:23PM +0800, David Wu 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>
>> ---
>>   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..82b6f6b 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 followings
>
> s/followings/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.  REQUIRED.
>
> This is the required section, so REQUIRED is redundant.


>
>> +   - For newer hardware (rk3399): specify by name
>> +     - "i2c": REQUIRED. This is used to derive the functional clock.
>> +     - "pclk": REQUIRED. This is the bus clock.equired on RK3066, RK3188 :
>
> This doesn't make sense. The first line says this applies to rk3399, but
> then here it is talking about other parts.
>
> And there's a typo.
>

Okay, I will remove the "equired on RK3066, RK3188 :".

>>
>>   Required on RK3066, RK3188 :
>>
>> --
>> 1.9.1
>>
>>
>
>
>

[PATCH v7 7/9] dt-bindings: i2c: rk3x: add support for rk3399

[Doug Anderson]

Hi,

On Wed, May 4, 2016 at 7:35 AM, 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>
> ---
>  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..82b6f6b 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 followings
> +   - "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.  REQUIRED.
> +   - For newer hardware (rk3399): specify by name
> +     - "i2c": REQUIRED. This is used to derive the functional clock.
> +     - "pclk": REQUIRED. This is the bus clock.equired on RK3066, RK3188 :

Looks great except that you need to delete the "equired on RK3066,
RK3188 :" part, which looks like it was leftover from some sort of
copy/paste.

Once that is done, feel free to add my Reviewed-by tag.


-Doug

[PATCH v7 8/9] 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 v7:
- transform into a 9 series patches (Doug)
- drop highspeed with mastercode, and support fast-mode plus (Doug)

Changes in v6:
- add master code send for HS mode
- use assigned-clocks mechanism for clock rate set form DT (Heiko)

Changes in v5:
- use compatible to seperate from different version
- can caculate highspeed clk rate

Changes in v4:
- pclk and sclk are treated individually
- use switch-case to seperate from different version (Andy)
- fix dead loop form Julia's notice

Change in v3:
- Too many arguments for ops func, use struct for them (Andy)

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

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 408f9ab..47368c4 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -58,6 +58,10 @@ 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_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 */
 
@@ -124,10 +128,15 @@ static const struct i2c_spec_values fast_mode_spec = {
  * 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 {
@@ -141,9 +150,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 *);
 };
 
 /**
@@ -152,9 +164,11 @@ 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
+ * @t_calc: I2C timing information need to be calculated
  * @lock: spinlock for the i2c bus
  * @wait: the waitqueue to wait for i2c transfer
  * @busy: the condition for the event to wait for
@@ -174,10 +188,12 @@ struct rk3x_i2c {
 	/* Hardware resources */
 	void __iomem *regs;
 	struct clk *clk;
+	struct clk *pclk;
 	struct notifier_block clk_rate_nb;
 
 	/* Settings */
 	struct i2c_timings t;
+	struct rk3x_i2c_calced_timings t_calc;
 
 	/* Synchronization & notification */
 	spinlock_t lock;
@@ -218,13 +234,13 @@ 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->t_calc.tuning;
 
 	i2c->state = STATE_START;
 	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))
@@ -533,9 +549,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;
@@ -681,23 +697,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;
+	struct rk3x_i2c_calced_timings *calc = &i2c->t_calc;
 	u64 t_low_ns, t_high_ns;
 	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);
-	i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
+	if (i2c->pclk)
+		clk_enable(i2c->pclk);
+	else
+		clk_enable(i2c->clk);
+	i2c_writel(i2c, (calc->div_high << 16) | (calc->div_low & 0xffff),
 		   REG_CLKDIV);
-	clk_disable(i2c->clk);
+	if (i2c->pclk)
+		clk_disable(i2c->pclk);
+	else
+		clk_disable(i2c->clk);
 
-	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,
+	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",
@@ -728,11 +910,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);
-	struct rk3x_i2c_calced_timings calc;
 
 	switch (event) {
 	case PRE_RATE_CHANGE:
-		if (rk3x_i2c_calc_divs(ndata->new_rate, &i2c->t, &calc) != 0)
+		if (i2c->soc_data->calc_timings(ndata->new_rate, &i2c->t,
+						&i2c->t_calc) != 0)
 			return NOTIFY_STOP;
 
 		/* scale up */
@@ -847,6 +1029,8 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
 
 	spin_lock_irqsave(&i2c->lock, flags);
 
+	if (i2c->pclk)
+		clk_enable(i2c->pclk);
 	clk_enable(i2c->clk);
 
 	i2c->is_last_msg = false;
@@ -881,7 +1065,8 @@ 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);
+			i2c_writel(i2c, i2c->t_calc.tuning | REG_CON_EN |
+				   REG_CON_STOP, REG_CON);
 
 			i2c->state = STATE_IDLE;
 
@@ -896,6 +1081,8 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
 	}
 
 	clk_disable(i2c->clk);
+	if (i2c->pclk)
+		clk_disable(i2c->pclk);
 	spin_unlock_irqrestore(&i2c->lock, flags);
 
 	return ret < 0 ? ret : num;
@@ -913,18 +1100,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[] = {
@@ -944,6 +1140,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);
@@ -983,12 +1183,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))
@@ -1042,17 +1236,38 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, i2c);
 
+	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);
+	}
+
 	ret = clk_prepare(i2c->clk);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Could not prepare clock\n");
 		return ret;
 	}
 
+	if (i2c->soc_data->calc_timings == rk3x_i2c_v1_calc_timings) {
+		i2c->pclk = devm_clk_get(&pdev->dev, "pclk");
+		if (IS_ERR(i2c->pclk)) {
+			dev_err(i2c->dev, "Could not get i2c pclk\n");
+			ret = PTR_ERR(i2c->pclk);
+			goto err_clk;
+		}
+
+		ret = clk_prepare(i2c->pclk);
+		if (ret) {
+			dev_err(i2c->dev, "Could not prepare pclk\n");
+			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);
@@ -1070,6 +1285,9 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
 
 err_clk_notifier:
 	clk_notifier_unregister(i2c->clk, &i2c->clk_rate_nb);
+err_pclk:
+	if (i2c->pclk)
+		clk_unprepare(i2c->pclk);
 err_clk:
 	clk_unprepare(i2c->clk);
 	return ret;
@@ -1082,6 +1300,8 @@ static int rk3x_i2c_remove(struct platform_device *pdev)
 	i2c_del_adapter(&i2c->adap);
 
 	clk_notifier_unregister(i2c->clk, &i2c->clk_rate_nb);
+	if (i2c->pclk)
+		clk_unprepare(i2c->pclk);
 	clk_unprepare(i2c->clk);
 
 	return 0;
-- 
1.9.1

[PATCH v7 8/9] i2c: rk3x: add i2c support for rk3399 soc

[Doug Anderson]

David,

On Wed, May 4, 2016 at 7:36 AM, David Wu <david.wu@rock-chips.com> wrote:
> +/**
> + * 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)
> +{

I don't think I'm going to try to understand all the math here.  I'll
trust you that this does something sane.


> +       /* Final divh and divl must be greater than 0, otherwise the
> +        * hardware would not output the i2c clk.
> +        */

nit: multiline comment style doesn't match rest of file.


>  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;
> +       struct rk3x_i2c_calced_timings *calc = &i2c->t_calc;
>         u64 t_low_ns, t_high_ns;
>         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);
> -       i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
> +       if (i2c->pclk)
> +               clk_enable(i2c->pclk);
> +       else
> +               clk_enable(i2c->clk);
> +       i2c_writel(i2c, (calc->div_high << 16) | (calc->div_low & 0xffff),
>                    REG_CLKDIV);

There is a subtle bug here, though it likely doesn't manifest in any
current hardware configurations.

Specifically if you get a clock change on a device with a "v1"
controller while an i2c transaction is happening then you will likely
get i2c errors.

The clock change notifications work like this:
* Before the clock change, adjust the timings based on the faster of
the old/new clock.
* Let the clock change happen.
* If we didn't adjust the timings before, adjust them now.

With the logic above there will be a period where the i2c transaction
is happening slower than ideal, but that should be OK.  ...and you can
imagine the speed of the transaction changing midway through the
transaction--even midway through a single byte.


With v1 some of the timing information is _not_updated by
rk3x_i2c_adapt_div()--it's only set at the start of a transaction.
That breaks all the above assumptions.

So you should probably be updating the the RKI2C_CON register here by
doing a read-modify-write, like:

ctrl = i2c_readl(i2c, REG_CON);
ctrl &= ~REG_CON_TUNING_MASK;
ctrl |= i2c->t_calc.tuning;
i2c_writel(i2c, ctrl, REG_CON);


Also (optional): once you do that, there becomes much less of a reason
to store "t_calc" in "struct rk3x_i2c".  Already you're never using
the "div_low" and "div_high" that you store in the "struct rk3x_i2c".
Of course, to do that you've got to change other places not to clobber
these bits in REG_CON.


> @@ -728,11 +910,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);
> -       struct rk3x_i2c_calced_timings calc;
>
>         switch (event) {
>         case PRE_RATE_CHANGE:
> -               if (rk3x_i2c_calc_divs(ndata->new_rate, &i2c->t, &calc) != 0)
> +               if (i2c->soc_data->calc_timings(ndata->new_rate, &i2c->t,
> +                                               &i2c->t_calc) != 0)

This change is incorrect.  Please change it back to being calculated
in a local variable.  Optionally also add a comment that says:

/*
 * 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().
 */

Specifically in the case that we return NOTIFY_STOP here we _don't_
want to have modified our internal timings.  We also _don't_ want to
have modified our internal timings in the case that the old_rate > the
new_rate.

BTW: Did you manage to find anyone using an old Rockchip SoC that can
test your patches?


> @@ -1042,17 +1236,38 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, i2c);
>
> +       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);
> +       }
> +
>         ret = clk_prepare(i2c->clk);
>         if (ret < 0) {
>                 dev_err(&pdev->dev, "Could not prepare clock\n");
>                 return ret;
>         }
>
> +       if (i2c->soc_data->calc_timings == rk3x_i2c_v1_calc_timings) {
> +               i2c->pclk = devm_clk_get(&pdev->dev, "pclk");
> +               if (IS_ERR(i2c->pclk)) {
> +                       dev_err(i2c->dev, "Could not get i2c pclk\n");
> +                       ret = PTR_ERR(i2c->pclk);
> +                       goto err_clk;
> +               }
> +
> +               ret = clk_prepare(i2c->pclk);
> +               if (ret) {
> +                       dev_err(i2c->dev, "Could not prepare pclk\n");
> +                       goto err_clk;
> +               }
> +       }

This is not matching the bindings.  You are still assuming that "i2c"
clock is the first clock and "pclk" is the one named "pclk".  Said
another way, if you had the following in your device tree:

  clocks =  <&pmucru PCLK_I2C0_PMU>, <&pmucru SCLK_I2C0_PMU>;
  clock-names = "pclk", "i2c";

...you'll find that you'll get back "pclk" twice.  The first time
you'll get it because you asked for the first clock listed, the second
time because you asked for the clock named "pclk".

I'd also say that your life will probably be easier if you always
setup both "clk" and "pclk", even on old CPUs.  It's OK to call
"clk_prepare" twice and OK to call "clk_enable" twice.

Thus, I'd probably write all the code as this (untested):

  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, "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;
  }

If you take that advice, you can get rid of all of the "if
(i2c->pclk)" statements in your code.


-Doug

[PATCH v7 8/9] i2c: rk3x: add i2c support for rk3399 soc

[David.Wu]

Hi Doug,

? 2016/5/6 7:00, Doug Anderson ??:
> David,
>
> On Wed, May 4, 2016 at 7:36 AM, David Wu <david.wu@rock-chips.com> wrote:
>> +/**
>> + * 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)
>> +{
>
> I don't think I'm going to try to understand all the math here.  I'll
> trust you that this does something sane.
>
>
>> +       /* Final divh and divl must be greater than 0, otherwise the
>> +        * hardware would not output the i2c clk.
>> +        */
>
> nit: multiline comment style doesn't match rest of file.
>
>
>>   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;
>> +       struct rk3x_i2c_calced_timings *calc = &i2c->t_calc;
>>          u64 t_low_ns, t_high_ns;
>>          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);
>> -       i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
>> +       if (i2c->pclk)
>> +               clk_enable(i2c->pclk);
>> +       else
>> +               clk_enable(i2c->clk);
>> +       i2c_writel(i2c, (calc->div_high << 16) | (calc->div_low & 0xffff),
>>                     REG_CLKDIV);
>
> There is a subtle bug here, though it likely doesn't manifest in any
> current hardware configurations.
>
> Specifically if you get a clock change on a device with a "v1"
> controller while an i2c transaction is happening then you will likely
> get i2c errors.
>
> The clock change notifications work like this:
> * Before the clock change, adjust the timings based on the faster of
> the old/new clock.
> * Let the clock change happen.
> * If we didn't adjust the timings before, adjust them now.
>
> With the logic above there will be a period where the i2c transaction
> is happening slower than ideal, but that should be OK.  ...and you can
> imagine the speed of the transaction changing midway through the
> transaction--even midway through a single byte.
>
>
> With v1 some of the timing information is _not_updated by
> rk3x_i2c_adapt_div()--it's only set at the start of a transaction.
> That breaks all the above assumptions.
>
> So you should probably be updating the the RKI2C_CON register here by
> doing a read-modify-write, like:
>
> ctrl = i2c_readl(i2c, REG_CON);
> ctrl &= ~REG_CON_TUNING_MASK;
> ctrl |= i2c->t_calc.tuning;
> i2c_writel(i2c, ctrl, REG_CON);
>
>

Yeap, it seems it is a bug when a clock changes, but not update the 
regs, it might make transfer failed. It was not enough to just store 
tuning value.

> Also (optional): once you do that, there becomes much less of a reason
> to store "t_calc" in "struct rk3x_i2c".  Already you're never using
> the "div_low" and "div_high" that you store in the "struct rk3x_i2c".
> Of course, to do that you've got to change other places not to clobber
> these bits in REG_CON.
>

So, I only just need to store tuning value in the "struct rk3x_i2c", but 
not to store the "div_low" and "div_high"?
>
>> @@ -728,11 +910,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);
>> -       struct rk3x_i2c_calced_timings calc;
>>
>>          switch (event) {
>>          case PRE_RATE_CHANGE:
>> -               if (rk3x_i2c_calc_divs(ndata->new_rate, &i2c->t, &calc) != 0)
>> +               if (i2c->soc_data->calc_timings(ndata->new_rate, &i2c->t,
>> +                                               &i2c->t_calc) != 0)
>
> This change is incorrect.  Please change it back to being calculated
> in a local variable.  Optionally also add a comment that says:
>
> /*
>   * 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().
>   */
>
> Specifically in the case that we return NOTIFY_STOP here we _don't_
> want to have modified our internal timings.  We also _don't_ want to
> have modified our internal timings in the case that the old_rate > the
> new_rate.

Okay, use &i2c->t_calc is an error here, timings shouldn't actually take 
effect until rk3x_i2c_adapt_div().

>
> BTW: Did you manage to find anyone using an old Rockchip SoC that can
> test your patches?
>

The patches we have already used in our projects, they are verified by 
the basic tests. I would ask them to do more tests. Because we didn't 
change the clock rate now, it was a fixed value when clk inited, so we 
could not find the bug here.

>
>> @@ -1042,17 +1236,38 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
>>
>>          platform_set_drvdata(pdev, i2c);
>>
>> +       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);
>> +       }
>> +
>>          ret = clk_prepare(i2c->clk);
>>          if (ret < 0) {
>>                  dev_err(&pdev->dev, "Could not prepare clock\n");
>>                  return ret;
>>          }
>>
>> +       if (i2c->soc_data->calc_timings == rk3x_i2c_v1_calc_timings) {
>> +               i2c->pclk = devm_clk_get(&pdev->dev, "pclk");
>> +               if (IS_ERR(i2c->pclk)) {
>> +                       dev_err(i2c->dev, "Could not get i2c pclk\n");
>> +                       ret = PTR_ERR(i2c->pclk);
>> +                       goto err_clk;
>> +               }
>> +
>> +               ret = clk_prepare(i2c->pclk);
>> +               if (ret) {
>> +                       dev_err(i2c->dev, "Could not prepare pclk\n");
>> +                       goto err_clk;
>> +               }
>> +       }
>
> This is not matching the bindings.  You are still assuming that "i2c"
> clock is the first clock and "pclk" is the one named "pclk".  Said
> another way, if you had the following in your device tree:
>
>    clocks =  <&pmucru PCLK_I2C0_PMU>, <&pmucru SCLK_I2C0_PMU>;
>    clock-names = "pclk", "i2c";
>
> ...you'll find that you'll get back "pclk" twice.  The first time
> you'll get it because you asked for the first clock listed, the second
> time because you asked for the clock named "pclk".
>
> I'd also say that your life will probably be easier if you always
> setup both "clk" and "pclk", even on old CPUs.  It's OK to call
> "clk_prepare" twice and OK to call "clk_enable" twice.
>
> Thus, I'd probably write all the code as this (untested):
>
>    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, "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;
>    }
>
> If you take that advice, you can get rid of all of the "if
> (i2c->pclk)" statements in your code.
>

It would make i2c->clk to be enabled and prepared twice when uses 
rk3x_i2c_v0_calc_timings for old hardware. But if do the opposite
disabled and unprepated twice, that is okay.

>
> -Doug
>
>
>

[PATCH v7 8/9] i2c: rk3x: add i2c support for rk3399 soc

[Doug Anderson]

Hi

On Fri, May 6, 2016 at 2:32 AM, David.Wu <david.wu@rock-chips.com> wrote:
>> Also (optional): once you do that, there becomes much less of a reason
>> to store "t_calc" in "struct rk3x_i2c".  Already you're never using
>> the "div_low" and "div_high" that you store in the "struct rk3x_i2c".
>> Of course, to do that you've got to change other places not to clobber
>> these bits in REG_CON.
>>
>
> So, I only just need to store tuning value in the "struct rk3x_i2c", but not
> to store the "div_low" and "div_high"?

I was suggesting not adding anything to what's stored in "struct
rk3x_i2c" (AKA don't add t_calc there).  Just set the value directly
in the register here then change all other bits of code to respect it.

That is:

In rk3x_i2c_start(), write:

  u32 val = i2c_readl(i2c, REG_CON) & REG_CON_TUNING_MASK;

In rk3x_i2c_xfer(), write:
  val = i2c_readl(i2c, REG_CON) & REG_CON_TUNING_MASK
  val |= REG_CON_EN | REG_CON_STOP, REG_CON;
  i2c_writel(i2c, val);


You could get fancy and add a new "i2c_update_bits", but that might be overkill.


> The patches we have already used in our projects, they are verified by the
> basic tests. I would ask them to do more tests. Because we didn't change the
> clock rate now, it was a fixed value when clk inited, so we could not find
> the bug here.

OK.  Presumably a rk3066 w/ DVS enabled would be a good test?  Reading
the description of commit 249051f49907 ("i2c: rk3x: handle dynamic
clock rate changes correctly"), I see:

    The i2c input clock can change dynamically, e.g. on the RK3066 where
    pclk_i2c0 and pclk_i2c1 are connected to the armclk, which changes
    rate on cpu frequency scaling.


>> If you take that advice, you can get rid of all of the "if
>> (i2c->pclk)" statements in your code.
>>
>
> It would make i2c->clk to be enabled and prepared twice when uses
> rk3x_i2c_v0_calc_timings for old hardware. But if do the opposite
> disabled and unprepated twice, that is okay.

Right.  The same clock will get an enable / prepare count of "2"
(instead of two different clocks getting a count of "1).  ...but
nothing should be hurt by that.

-Doug

[PATCH v7 9/9] i2c: rk3x: support fast-mode plus for rk3399

[David Wu]

Signed-off-by: David Wu <david.wu@rock-chips.com>
---
 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 47368c4..c66cc39 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -124,6 +124,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
@@ -534,8 +545,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;
 }
 
 /**
@@ -746,9 +759,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 v7 9/9] i2c: rk3x: support fast-mode plus for rk3399

[Doug Anderson]

David,

On Wed, May 4, 2016 at 7:37 AM, David Wu <david.wu@rock-chips.com> wrote:
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
>  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 47368c4..c66cc39 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -124,6 +124,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,

I'm curious where you got the data_hold_ns.  I can't quite remember
what this parameter does / how the timing function works anymore, but
the doc I have (search for UM10204 and click the first link) shows
values for Standard-mode and Fast-mode but not Fast-mode Plus.  It
seems to imply that this is a bit of a bogus number anyway because it
only matters if we don't stretch the tLOW to go along with the longer
data hold.

As I have said in the previous patch, how all this stuff works has
totally left my brain, so if you understand it that's probably good
enough.  If you feel like I should try to re-understand this again so
I can review it more deeply, let me know.


Since I assume that you had some sane reason to include max_data_hold_ns:

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

[PATCH v7 9/9] i2c: rk3x: support fast-mode plus for rk3399

[David.Wu]

Hi Doug,

? 2016/5/6 7:02, Doug Anderson ??:
> David,
>
> On Wed, May 4, 2016 at 7:37 AM, David Wu <david.wu@rock-chips.com> wrote:
>> Signed-off-by: David Wu <david.wu@rock-chips.com>
>> ---
>>   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 47368c4..c66cc39 100644
>> --- a/drivers/i2c/busses/i2c-rk3x.c
>> +++ b/drivers/i2c/busses/i2c-rk3x.c
>> @@ -124,6 +124,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,
>
> I'm curious where you got the data_hold_ns.  I can't quite remember
> what this parameter does / how the timing function works anymore, but
> the doc I have (search for UM10204 and click the first link) shows
> values for Standard-mode and Fast-mode but not Fast-mode Plus.  It
> seems to imply that this is a bit of a bogus number anyway because it
> only matters if we don't stretch the tLOW to go along with the longer
> data hold.
>
> As I have said in the previous patch, how all this stuff works has
> totally left my brain, so if you understand it that's probably good
> enough.  If you feel like I should try to re-understand this again so
> I can review it more deeply, let me know.
>
>

Yes, I could not get the description of fast-mode plus data_hold_ns, but 
I saw that the maximum tHD;DAT must be less than the maximum of 
tVD;DATor tVD;ACKby for Standard-mode and Fast-mode.

So I think the maximum tHD;DAT for Fast-mode Plus should be less than 
the maximum of tVD;DATor tVD;ACKby too, the maximum of tVD;DATor 
tVD;ACKby is 450ns, and 400ns is my taking a conservative value.

Description form UM10204
[4] The maximum tHD;DATcould be 3.45?s and 0.9?s for Standard-mode and 
Fast-mode, but must be less than the maximum of tVD;DATor tVD;ACKby a 
transition time. This maximum must only be met if the device does not 
stretch the LOW period (tLOW) of the SCL signal. If the clock stretches 
the SCL, the data must be valid by the set-up time before it releases
the clock.

> Since I assume that you had some sane reason to include max_data_hold_ns:
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
>
>