| Message ID | 1462372472-6396-1-git-send-email-david.wu@rock-chips.com |
|---|---|
| State | Superseded |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */
Signed-off-by: David Wu <david.wu@rock-chips.com> --- drivers/i2c/busses/i2c-rk3x.c | 100 ++++++++++++++++++++++++++++++------------ 1 file changed, 72 insertions(+), 28 deletions(-)