[PATCH v5] i2c: rk3x: adjust the LOW divison based on characteristics of SCL
As show in I2C specification:
- Standard-mode: the minimum HIGH period of the scl clock is 4.0us
the minimum LOW period of the scl clock is 4.7us
- Fast-mode: the minimum HIGH period of the scl clock is 0.6us
the minimum LOW period of the scl clock is 1.3us
I have measured i2c SCL waveforms in fast-mode by oscilloscope
on rk3288-pinky board. the LOW period of the scl clock is 1.3us.
It is so critical that we must adjust LOW division to increase
the LOW period of the scl clock.
Thanks Doug for the suggestion about division formulas.
Tested-by: Heiko Stuebner
Reviewed-by: Doug Anderson
Tested-by: Doug Anderson
Signed-off-by: Addy Ke
---
Changes in v2:
- remove Fast-mode plus and HS-mode
- use new formulas suggested by Doug
Changes in V3:
- use new formulas (sep 30) suggested by Doug
Changes in V4:
- fix some wrong style
- WARN_ONCE if min_low_div > max_low_div
Changes in V5:
- round up for i2c_rate and round down for scl_rate, suggested by Max
- place a WARN_ON if scl_rate < 1000, suggested by Max
- add div_high and div_low overflow protection, suggested by Max
drivers/i2c/busses/i2c-rk3x.c | 161 +++---
1 file changed, 152 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index b41d979..a30a4fb 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
/* Register Map */
@@ -428,18 +429,158 @@ out:
return IRQ_HANDLED;
}
-static void rk3x_i2c_set_scl_rate(struct rk3x_i2c *i2c, unsigned long scl_rate)
+static int rk3x_i2c_calc_divs(unsigned long i2c_rate, unsigned long scl_rate,
+ unsigned long *div_low, unsigned long *div_high)
{
- unsigned long i2c_rate = clk_get_rate(i2c->clk);
- unsigned int div;
+ unsigned long min_low_ns, min_high_ns;
+ unsigned long max_data_hold_ns;
+ unsigned long data_hold_buffer_ns;
+ unsigned long max_low_ns, min_total_ns;
+
+ unsigned long i2c_rate_khz, scl_rate_khz;
+
+ unsigned long min_low_div, min_high_div;
+ unsigned long max_low_div;
+
+ unsigned long min_div_for_hold, min_total_div;
+ unsigned long extra_div, extra_low_div, ideal_low_div;
+
+ /* Only support standard-mode and fast-mode */
+ if (WARN_ON(scl_rate > 40))
+ scl_rate = 40;
+
+ /* prevent scl_rate_khz from becoming 0 */
+ if (WARN_ON(scl_rate < 1000))
+ scl_rate = 1000;
+
+ /*
+* min_low_ns: The minimum number of ns we need to hold low
+* to meet i2c spec
+* min_high_ns: The minimum number of ns we need to hold high
+* to meet i2c spec
+* max_low_ns: The maximum number of ns we can hold low
+* to meet i2c spec
+*
+* Note: max_low_ns should be (max data hold time * 2 - buffer)
+* This is because the i2c host on Rockchip holds the data line
+* for half the low time.
+*/
+ if (scl_rate <= 10) {
+ min_low_ns = 4700;
+ min_high_ns = 4000;
+ max_data_hold_ns = 3450;
+ data_hold_buffer_ns = 50;
+ } else {
+ min_low_ns = 1300;
+ min_high_ns = 600;
+ max_data_hold_ns = 900;
+ data_hold_buffer_ns = 50;
+ }
+ max_low_ns = max_data_hold_ns * 2 - data_hold_buffer_ns;
+ min_total_ns = min_low_ns + min_high_ns;
+
+ /* Adjust to avoid overflow */
+ i2c_rate_khz = DIV_ROUND_UP(i2c_rate, 1000);
+ scl_rate_khz = scl_rate / 1000;
- /* set DIV = DIVH = DIVL
-* SCL rate = (clk rate) / (8 * (DIVH + 1 + DIVL + 1))
-* = (clk rate) / (16 * (DIV + 1))
+ /*
+* We need the total div to be >= this number
+* so we don't clock too fast.
+*/
+ min_total_div = DIV_ROUND_UP(i2c_rate_khz, scl_rate_khz * 8);
+
+ /* These are the min dividers needed for min hold times. */
+ min_low_div = DIV_ROUND_UP(i2c_rate_khz * min_low_ns, 8 * 100);
+ min_high_div = DIV_ROUND_UP(i2c_rate_khz * min_high_ns, 8 * 100);
+ min_div_for_hold = (min_low_div + min_high_div);
+
+ /*
+* This is the maximum divider so we don't go over the max.
+* We don't round up here (we round down) since this is a max.
*/
- div = DIV_ROUND_UP(i2c_rate, scl_rate * 16) - 1;
+ max_low_div = i2c_rate_khz * max_low_ns / (8 * 100);
+
+ if (min_low_div > max_low_div) {
+ WARN_ONCE(true,
+ "Conflicting, min_low_div %lu, max_low_div %lu\n",
+ min_low_div, max_low_div);
+ max_low_div = min_low_div;
+ }
+
+ if (min_div_for_hold > min_total_div) {
+ /*
+
Re: [PATCH v4] i2c: rk3x: adjust the LOW divison based on characteristics of SCL
Max, On Sun, Oct 12, 2014 at 5:10 AM, Max Schwarz wrote: >> + /* Adjust to avoid overflow */ >> + i2c_rate_khz = DIV_ROUND_UP(i2c_rate, 1000); >> + scl_rate_khz = DIV_ROUND_UP(scl_rate, 1000); > > I'm not really comfortable with using DIV_ROUND_UP on the last line, since > this may violate the user's set SCL rate. Rounding up 1.1kHz SCL rate to 2kHz > is not good. Ah, good point. So we round up for i2c_rate and down for scl_rate: i2c_rate_khz = DIV_ROUND_UP(i2c_rate, 1000); scl_rate_khz = scl_rate / 1000; > I suggest using scl_rate / 1000 and placing a > > if(WARN_ON(scl_rate < 1000)) > scl_rate = 1000; > > somewhere to prevent scl_rate_khz from becoming 0. Ah, makes sense. Yeah, that should be at the top. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 3/3] i2c: show and change bus frequency via sysfs
On Sun, Oct 12, 2014 at 12:32:56PM +0300, Octavian Purdila wrote: > On Sat, Oct 11, 2014 at 11:14 PM, Mark Roszko wrote: > > > This seems limiting to arches with peripherals that can support a range of > > frequencies rather than fixed numbers. > > Also it creates some portability quirkiness between platforms when all the > > i2c bus drivers have different supported freq lists and you have to match > > exactly the right frequency. I.e. one guy does 60khz but another only has > > 80khz. > > Sorry, I don't understand your points here. If this limitations exists > they are not introduced by this patch. This patch just exposes the > frequency so that it can be read or changed in userspace. > > > Another issue is in systems where you have i2c devices on the same bus as > > the sysfs user space driver. User space could set a bus frequency that > > prevents operation with a system i2c device. > > Changing the frequency is limited to root. Also, bus drivers do not > have to implement set_freq if it is thought not to be safe. > > On a different not, I have noticed that a fixed set of frequencies > might not be the best API, since multiple drivers rather support a > rather large set of frequencies in a range. A better API might be to > expose a min-max range and let the bus driver adjust the requested > frequency. I will follow up with a second version that does that. As two separate sysfs attributes, maybe ? sysfs is supposed to provide one value per attribute. For the patch itself, I would find it better if you used is_visible to determine if the new attributes should be visible (and/or writable) instead of returning -EOPNOTSUPP. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] i2c: rk3x: adjust the LOW divison based on characteristics of SCL
Hi Addy,
sorry for the long delay, I finally had time to look at your work.
On Thursday 09 October 2014 at 14:47:15, Addy Ke wrote:
> As show in I2C specification:
> - Standard-mode: the minimum HIGH period of the scl clock is 4.0us
> the minimum LOW period of the scl clock is 4.7us
> - Fast-mode: the minimum HIGH period of the scl clock is 0.6us
> the minimum LOW period of the scl clock is 1.3us
>
> I have measured i2c SCL waveforms in fast-mode by oscilloscope
> on rk3288-pinky board. the LOW period of the scl clock is 1.3us.
> It is so critical that we must adjust LOW division to increase
> the LOW period of the scl clock.
>
> Thanks Doug for the suggestion about division formulas.
>
> Reviewed-by: Doug Anderson
> Tested-by: Doug Anderson
> Signed-off-by: Addy Ke
> ---
> Changes in v2:
> - remove Fast-mode plus and HS-mode
> - use new formulas suggested by Doug
> Changes in V3:
> - use new formulas (sep 30) suggested by Doug
> Changes in V3:
> - fix some wrong style
> - WARN_ONCE if min_low_div > max_low_div
>
> drivers/i2c/busses/i2c-rk3x.c | 140
> +++--- 1 file changed, 133
> insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index b41d979..f17f7a2 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -24,6 +24,7 @@
> #include
> #include
> #include
> +#include
>
>
> /* Register Map */
> @@ -428,18 +429,143 @@ out:
> return IRQ_HANDLED;
> }
>
> +static void rk3x_i2c_calc_divs(unsigned long i2c_rate, unsigned long
> scl_rate, + unsigned long *div_low, unsigned long
> *div_high)
> +{
> + unsigned long min_low_ns, min_high_ns;
> + unsigned long max_data_hold_ns;
> + unsigned long data_hold_buffer_ns;
> + unsigned long max_low_ns, min_total_ns;
> +
> + unsigned long i2c_rate_khz, scl_rate_khz;
> +
> + unsigned long min_low_div, min_high_div;
> + unsigned long max_low_div;
> +
> + unsigned long min_div_for_hold, min_total_div;
> + unsigned long extra_div, extra_low_div, ideal_low_div;
> +
> + /* Only support standard-mode and fast-mode */
> + WARN_ON(scl_rate > 40);
> +
> + /*
> + * min_low_ns: The minimum number of ns we need to hold low
> + * to meet i2c spec
> + * min_high_ns: The minimum number of ns we need to hold high
> + * to meet i2c spec
> + * max_low_ns: The maximum number of ns we can hold low
> + * to meet i2c spec
> + *
> + * Note: max_low_ns should be (max data hold time * 2 - buffer)
> + * This is because the i2c host on Rockchip holds the data line
> + * for half the low time.
> + */
> + if (scl_rate <= 10) {
> + min_low_ns = 4700;
> + min_high_ns = 4000;
> + max_data_hold_ns = 3450;
> + data_hold_buffer_ns = 50;
> + } else {
> + min_low_ns = 1300;
> + min_high_ns = 600;
> + max_data_hold_ns = 900;
> + data_hold_buffer_ns = 50;
> + }
> + max_low_ns = max_data_hold_ns * 2 - data_hold_buffer_ns;
> + min_total_ns = min_low_ns + min_high_ns;
> +
> + /* Adjust to avoid overflow */
> + i2c_rate_khz = DIV_ROUND_UP(i2c_rate, 1000);
> + scl_rate_khz = DIV_ROUND_UP(scl_rate, 1000);
I'm not really comfortable with using DIV_ROUND_UP on the last line, since
this may violate the user's set SCL rate. Rounding up 1.1kHz SCL rate to 2kHz
is not good.
I suggest using scl_rate / 1000 and placing a
if(WARN_ON(scl_rate < 1000))
scl_rate = 1000;
somewhere to prevent scl_rate_khz from becoming 0.
> +
> + /*
> + * We need the total div to be >= this number
> + * so we don't clock too fast.
> + */
> + min_total_div = DIV_ROUND_UP(i2c_rate_khz, scl_rate_khz * 8);
> +
> + /* These are the min dividers needed for min hold times. */
> + min_low_div = DIV_ROUND_UP(i2c_rate_khz * min_low_ns, 8 * 100);
> + min_high_div = DIV_ROUND_UP(i2c_rate_khz * min_high_ns, 8 * 100);
> + min_div_for_hold = (min_low_div + min_high_div);
> +
> + /*
> + * This is the maximum divider so we don't go over the max.
> + * We don't round up here (we round down) since this is a max.
> + */
> + max_low_div = i2c_rate_khz * max_low_ns / (8 * 100);
> +
> + if (min_low_div > max_low_div) {
> + WARN_ONCE(true,
> + "Conflicting, min_low_div %lu, max_low_div %lu\n",
> + min_low_div, max_low_div);
> + max_low_div = min_low_div;
> + }
> +
> + if (min_div_for_hold > min_total_div) {
> + /*
> + * Time needed to meet hold requirements is important.
> + * Just use that.
> + */
> + *div_low = min_low_div;
> + *div_high = min_h
Re: [RFC PATCH 3/3] i2c: show and change bus frequency via sysfs
On Sat, Oct 11, 2014 at 11:14 PM, Mark Roszko wrote: > This seems limiting to arches with peripherals that can support a range of > frequencies rather than fixed numbers. > Also it creates some portability quirkiness between platforms when all the > i2c bus drivers have different supported freq lists and you have to match > exactly the right frequency. I.e. one guy does 60khz but another only has > 80khz. Sorry, I don't understand your points here. If this limitations exists they are not introduced by this patch. This patch just exposes the frequency so that it can be read or changed in userspace. > Another issue is in systems where you have i2c devices on the same bus as > the sysfs user space driver. User space could set a bus frequency that > prevents operation with a system i2c device. Changing the frequency is limited to root. Also, bus drivers do not have to implement set_freq if it is thought not to be safe. On a different not, I have noticed that a fixed set of frequencies might not be the best API, since multiple drivers rather support a rather large set of frequencies in a range. A better API might be to expose a min-max range and let the bus driver adjust the requested frequency. I will follow up with a second version that does that. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
