* [PATCH v7 1/9] i2c: rk3x: add documentation to fields in "struct rk3x_i2c"
2016-05-04 14:13 [PATCH v7 0/9] add i2c driver supported for rk3399 David Wu
@ 2016-05-04 14:13 ` David Wu
[not found] ` <1462371194-5809-2-git-send-email-david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2016-05-04 14:13 ` [PATCH v7 2/9] i2c: rk3x: use struct "rk3x_i2c_calced_timings" David Wu
` (7 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: David Wu @ 2016-05-04 14:13 UTC (permalink / raw)
To: heiko, wsa
Cc: robh+dt, dianders, andy.shevchenko, pawel.moll, mark.rutland,
ijc+devicetree, galak, briannorris, davidriley, huangtao, hl,
xjq, zyw, cf, linux-arm-kernel, linux-rockchip, linux-i2c,
devicetree, linux-kernel, 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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v7 2/9] i2c: rk3x: use struct "rk3x_i2c_calced_timings"
2016-05-04 14:13 [PATCH v7 0/9] add i2c driver supported for rk3399 David Wu
2016-05-04 14:13 ` [PATCH v7 1/9] i2c: rk3x: add documentation to fields in "struct rk3x_i2c" David Wu
@ 2016-05-04 14:13 ` David Wu
[not found] ` <1462371194-5809-3-git-send-email-david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2016-05-04 14:13 ` [PATCH v7 3/9] i2c: rk3x: Remove redundant rk3x_i2c_clean_ipd() David Wu
` (6 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: David Wu @ 2016-05-04 14:13 UTC (permalink / raw)
To: heiko, wsa
Cc: robh+dt, dianders, andy.shevchenko, pawel.moll, mark.rutland,
ijc+devicetree, galak, briannorris, davidriley, huangtao, hl,
xjq, zyw, cf, linux-arm-kernel, linux-rockchip, linux-i2c,
devicetree, linux-kernel, 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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v7 3/9] i2c: rk3x: Remove redundant rk3x_i2c_clean_ipd()
2016-05-04 14:13 [PATCH v7 0/9] add i2c driver supported for rk3399 David Wu
2016-05-04 14:13 ` [PATCH v7 1/9] i2c: rk3x: add documentation to fields in "struct rk3x_i2c" David Wu
2016-05-04 14:13 ` [PATCH v7 2/9] i2c: rk3x: use struct "rk3x_i2c_calced_timings" David Wu
@ 2016-05-04 14:13 ` David Wu
2016-05-05 22:55 ` Doug Anderson
2016-05-04 14:13 ` [PATCH v7 4/9] i2c: rk3x: Move setting STATE_START and add STATE_SETUP David Wu
` (5 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: David Wu @ 2016-05-04 14:13 UTC (permalink / raw)
To: heiko, wsa
Cc: robh+dt, dianders, andy.shevchenko, pawel.moll, mark.rutland,
ijc+devicetree, galak, briannorris, davidriley, huangtao, hl,
xjq, zyw, cf, linux-arm-kernel, linux-rockchip, linux-i2c,
devicetree, linux-kernel, 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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v7 3/9] i2c: rk3x: Remove redundant rk3x_i2c_clean_ipd()
2016-05-04 14:13 ` [PATCH v7 3/9] i2c: rk3x: Remove redundant rk3x_i2c_clean_ipd() David Wu
@ 2016-05-05 22:55 ` Doug Anderson
0 siblings, 0 replies; 29+ messages in thread
From: Doug Anderson @ 2016-05-05 22:55 UTC (permalink / raw)
To: David Wu
Cc: Heiko Stübner, Wolfram Sang, Rob Herring, Andy Shevchenko,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Brian Norris,
David Riley, Tao Huang, Lin Huang, Jianqun Xu, Chris, Eddie Cai,
linux-arm-kernel, open list:ARM/Rockchip SoC...,
linux-i2c, devicetree, linux-kernel
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>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v7 4/9] i2c: rk3x: Move setting STATE_START and add STATE_SETUP
2016-05-04 14:13 [PATCH v7 0/9] add i2c driver supported for rk3399 David Wu
` (2 preceding siblings ...)
2016-05-04 14:13 ` [PATCH v7 3/9] i2c: rk3x: Remove redundant rk3x_i2c_clean_ipd() David Wu
@ 2016-05-04 14:13 ` David Wu
2016-05-05 22:56 ` Doug Anderson
2016-05-04 14:33 ` [PATCH v7 5/9] i2c: rk3x: Change SoC data to not use array David Wu
` (4 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: David Wu @ 2016-05-04 14:13 UTC (permalink / raw)
To: heiko, wsa
Cc: robh+dt, dianders, andy.shevchenko, pawel.moll, mark.rutland,
ijc+devicetree, galak, briannorris, davidriley, huangtao, hl,
xjq, zyw, cf, linux-arm-kernel, linux-rockchip, linux-i2c,
devicetree, linux-kernel, 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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v7 4/9] i2c: rk3x: Move setting STATE_START and add STATE_SETUP
2016-05-04 14:13 ` [PATCH v7 4/9] i2c: rk3x: Move setting STATE_START and add STATE_SETUP David Wu
@ 2016-05-05 22:56 ` Doug Anderson
2016-05-06 3:20 ` David.Wu
0 siblings, 1 reply; 29+ messages in thread
From: Doug Anderson @ 2016-05-05 22:56 UTC (permalink / raw)
To: David Wu
Cc: Heiko Stübner, Wolfram Sang, Mark Rutland, Tao Huang,
Jianqun Xu, Lin Huang, Pawel Moll, Ian Campbell, devicetree,
open list:ARM/Rockchip SoC...,
linux-kernel, Eddie Cai, Andy Shevchenko, Rob Herring, linux-i2c,
Kumar Gala, Chris, Brian Norris, David Riley, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v7 4/9] i2c: rk3x: Move setting STATE_START and add STATE_SETUP
2016-05-05 22:56 ` Doug Anderson
@ 2016-05-06 3:20 ` David.Wu
0 siblings, 0 replies; 29+ messages in thread
From: David.Wu @ 2016-05-06 3:20 UTC (permalink / raw)
To: Doug Anderson
Cc: Heiko Stübner, Wolfram Sang, Mark Rutland, Tao Huang,
Jianqun Xu, Lin Huang, Pawel Moll, Ian Campbell, devicetree,
open list:ARM/Rockchip SoC...,
linux-kernel, Eddie Cai, Andy Shevchenko, Rob Herring, linux-i2c,
Kumar Gala, Chris, Brian Norris, David Riley, linux-arm-kernel
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
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v7 5/9] i2c: rk3x: Change SoC data to not use array
2016-05-04 14:13 [PATCH v7 0/9] add i2c driver supported for rk3399 David Wu
` (3 preceding siblings ...)
2016-05-04 14:13 ` [PATCH v7 4/9] i2c: rk3x: Move setting STATE_START and add STATE_SETUP David Wu
@ 2016-05-04 14:33 ` David Wu
[not found] ` <1462372418-6349-1-git-send-email-david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2016-05-04 14:34 ` [PATCH v7 6/9] i2c: rk3x: Move spec timing data to "static const" structs David Wu
` (3 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: David Wu @ 2016-05-04 14:33 UTC (permalink / raw)
To: heiko, wsa
Cc: robh+dt, dianders, andy.shevchenko, pawel.moll, mark.rutland,
ijc+devicetree, galak, briannorris, davidriley, huangtao, hl,
xjq, zyw, cf, linux-arm-kernel, linux-rockchip, linux-i2c,
devicetree, linux-kernel, 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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v7 6/9] i2c: rk3x: Move spec timing data to "static const" structs
2016-05-04 14:13 [PATCH v7 0/9] add i2c driver supported for rk3399 David Wu
` (4 preceding siblings ...)
2016-05-04 14:33 ` [PATCH v7 5/9] i2c: rk3x: Change SoC data to not use array David Wu
@ 2016-05-04 14:34 ` David Wu
2016-05-05 22:58 ` Doug Anderson
2016-05-04 14:35 ` [PATCH v7 7/9] dt-bindings: i2c: rk3x: add support for rk3399 David Wu
` (2 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: David Wu @ 2016-05-04 14:34 UTC (permalink / raw)
To: heiko, wsa
Cc: robh+dt, dianders, andy.shevchenko, pawel.moll, mark.rutland,
ijc+devicetree, galak, briannorris, davidriley, huangtao, hl,
xjq, zyw, cf, linux-arm-kernel, linux-rockchip, linux-i2c,
devicetree, linux-kernel, 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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v7 6/9] i2c: rk3x: Move spec timing data to "static const" structs
2016-05-04 14:34 ` [PATCH v7 6/9] i2c: rk3x: Move spec timing data to "static const" structs David Wu
@ 2016-05-05 22:58 ` Doug Anderson
2016-05-06 4:55 ` David.Wu
0 siblings, 1 reply; 29+ messages in thread
From: Doug Anderson @ 2016-05-05 22:58 UTC (permalink / raw)
To: David Wu
Cc: Heiko Stübner, Wolfram Sang, Rob Herring, Andy Shevchenko,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Brian Norris,
David Riley, Tao Huang, Lin Huang, Jianqun Xu, Chris, Eddie Cai,
linux-arm-kernel, open list:ARM/Rockchip SoC...,
linux-i2c, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v7 6/9] i2c: rk3x: Move spec timing data to "static const" structs
2016-05-05 22:58 ` Doug Anderson
@ 2016-05-06 4:55 ` David.Wu
0 siblings, 0 replies; 29+ messages in thread
From: David.Wu @ 2016-05-06 4:55 UTC (permalink / raw)
To: Doug Anderson
Cc: Heiko Stübner, Wolfram Sang, Rob Herring, Andy Shevchenko,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Brian Norris,
David Riley, Tao Huang, Lin Huang, Jianqun Xu, Chris, Eddie Cai,
linux-arm-kernel, open list:ARM/Rockchip SoC...,
linux-i2c, devicetree, linux-kernel
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
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v7 7/9] dt-bindings: i2c: rk3x: add support for rk3399
2016-05-04 14:13 [PATCH v7 0/9] add i2c driver supported for rk3399 David Wu
` (5 preceding siblings ...)
2016-05-04 14:34 ` [PATCH v7 6/9] i2c: rk3x: Move spec timing data to "static const" structs David Wu
@ 2016-05-04 14:35 ` David Wu
2016-05-05 22:12 ` Rob Herring
2016-05-05 22:59 ` Doug Anderson
2016-05-04 14:36 ` [PATCH v7 8/9] i2c: rk3x: add i2c support for rk3399 soc David Wu
2016-05-04 14:37 ` [PATCH v7 9/9] i2c: rk3x: support fast-mode plus for rk3399 David Wu
8 siblings, 2 replies; 29+ messages in thread
From: David Wu @ 2016-05-04 14:35 UTC (permalink / raw)
To: heiko, wsa
Cc: robh+dt, dianders, andy.shevchenko, pawel.moll, mark.rutland,
ijc+devicetree, galak, briannorris, davidriley, huangtao, hl,
xjq, zyw, cf, linux-arm-kernel, linux-rockchip, linux-i2c,
devicetree, linux-kernel, 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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v7 7/9] dt-bindings: i2c: rk3x: add support for rk3399
2016-05-04 14:35 ` [PATCH v7 7/9] dt-bindings: i2c: rk3x: add support for rk3399 David Wu
@ 2016-05-05 22:12 ` Rob Herring
2016-05-06 6:09 ` David.Wu
2016-05-05 22:59 ` Doug Anderson
1 sibling, 1 reply; 29+ messages in thread
From: Rob Herring @ 2016-05-05 22:12 UTC (permalink / raw)
To: David Wu
Cc: heiko, wsa, dianders, andy.shevchenko, pawel.moll, mark.rutland,
ijc+devicetree, galak, briannorris, davidriley, huangtao, hl,
xjq, zyw, cf, linux-arm-kernel, linux-rockchip, linux-i2c,
devicetree, linux-kernel
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
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v7 7/9] dt-bindings: i2c: rk3x: add support for rk3399
2016-05-05 22:12 ` Rob Herring
@ 2016-05-06 6:09 ` David.Wu
0 siblings, 0 replies; 29+ messages in thread
From: David.Wu @ 2016-05-06 6:09 UTC (permalink / raw)
To: Rob Herring
Cc: heiko, wsa, dianders, andy.shevchenko, pawel.moll, mark.rutland,
ijc+devicetree, galak, briannorris, davidriley, huangtao, hl,
xjq, zyw, cf, linux-arm-kernel, linux-rockchip, linux-i2c,
devicetree, linux-kernel
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
>>
>>
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v7 7/9] dt-bindings: i2c: rk3x: add support for rk3399
2016-05-04 14:35 ` [PATCH v7 7/9] dt-bindings: i2c: rk3x: add support for rk3399 David Wu
2016-05-05 22:12 ` Rob Herring
@ 2016-05-05 22:59 ` Doug Anderson
1 sibling, 0 replies; 29+ messages in thread
From: Doug Anderson @ 2016-05-05 22:59 UTC (permalink / raw)
To: David Wu
Cc: Heiko Stübner, Wolfram Sang, Rob Herring, Andy Shevchenko,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Brian Norris,
David Riley, Tao Huang, Lin Huang, Jianqun Xu, Chris, Eddie Cai,
linux-arm-kernel, open list:ARM/Rockchip SoC...,
linux-i2c, devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v7 8/9] i2c: rk3x: add i2c support for rk3399 soc
2016-05-04 14:13 [PATCH v7 0/9] add i2c driver supported for rk3399 David Wu
` (6 preceding siblings ...)
2016-05-04 14:35 ` [PATCH v7 7/9] dt-bindings: i2c: rk3x: add support for rk3399 David Wu
@ 2016-05-04 14:36 ` David Wu
[not found] ` <1462372609-6535-1-git-send-email-david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2016-05-04 14:37 ` [PATCH v7 9/9] i2c: rk3x: support fast-mode plus for rk3399 David Wu
8 siblings, 1 reply; 29+ messages in thread
From: David Wu @ 2016-05-04 14:36 UTC (permalink / raw)
To: heiko, wsa
Cc: robh+dt, dianders, andy.shevchenko, pawel.moll, mark.rutland,
ijc+devicetree, galak, briannorris, davidriley, huangtao, hl,
xjq, zyw, cf, linux-arm-kernel, linux-rockchip, linux-i2c,
devicetree, linux-kernel, 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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v7 9/9] i2c: rk3x: support fast-mode plus for rk3399
2016-05-04 14:13 [PATCH v7 0/9] add i2c driver supported for rk3399 David Wu
` (7 preceding siblings ...)
2016-05-04 14:36 ` [PATCH v7 8/9] i2c: rk3x: add i2c support for rk3399 soc David Wu
@ 2016-05-04 14:37 ` David Wu
2016-05-05 23:02 ` Doug Anderson
8 siblings, 1 reply; 29+ messages in thread
From: David Wu @ 2016-05-04 14:37 UTC (permalink / raw)
To: heiko, wsa
Cc: robh+dt, dianders, andy.shevchenko, pawel.moll, mark.rutland,
ijc+devicetree, galak, briannorris, davidriley, huangtao, hl,
xjq, zyw, cf, linux-arm-kernel, linux-rockchip, linux-i2c,
devicetree, linux-kernel, 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
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v7 9/9] i2c: rk3x: support fast-mode plus for rk3399
2016-05-04 14:37 ` [PATCH v7 9/9] i2c: rk3x: support fast-mode plus for rk3399 David Wu
@ 2016-05-05 23:02 ` Doug Anderson
2016-05-06 2:12 ` David.Wu
0 siblings, 1 reply; 29+ messages in thread
From: Doug Anderson @ 2016-05-05 23:02 UTC (permalink / raw)
To: David Wu
Cc: Heiko Stübner, Wolfram Sang, Rob Herring, Andy Shevchenko,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Brian Norris,
David Riley, Tao Huang, Lin Huang, Jianqun Xu, Chris, Eddie Cai,
linux-arm-kernel, open list:ARM/Rockchip SoC...,
linux-i2c, devicetree, linux-kernel
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>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v7 9/9] i2c: rk3x: support fast-mode plus for rk3399
2016-05-05 23:02 ` Doug Anderson
@ 2016-05-06 2:12 ` David.Wu
0 siblings, 0 replies; 29+ messages in thread
From: David.Wu @ 2016-05-06 2:12 UTC (permalink / raw)
To: Doug Anderson
Cc: Heiko Stübner, Wolfram Sang, Rob Herring, Andy Shevchenko,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Brian Norris,
David Riley, Tao Huang, Lin Huang, Jianqun Xu, Chris, Eddie Cai,
linux-arm-kernel, open list:ARM/Rockchip SoC...,
linux-i2c, devicetree, linux-kernel
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>
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread