Re: [PATCH] i2c: rk3x: adjust the LOW divison based on characteristics of SCL

From: Doug Anderson
Date: Wed Sep 24 2014 - 00:10:56 EST

Addy,

On Tue, Sep 23, 2014 at 6:55 PM, Addy Ke <addy.ke@xxxxxxxxxxxxxx> 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
> - Fast-mode plus:
> the minimum HIGH period of the scl clock is 0.26us
> the minimum LOW period of the scl clock is 0.5us
> - HS-mode(<1.7MHz):
> the minimum HIGH period of the scl clock is 0.12us
> the minimum LOW period of the scl clock is 0.32us
> - HS-mode(<3.4MHz):
> the minimum HIGH period of the scl clock is 0.06us
> the minimum LOW period of the scl clock is 0.16us
>
> 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 formula.
>
> Signed-off-by: Addy Ke <addy.ke@xxxxxxxxxxxxxx>
> ---
> drivers/i2c/busses/i2c-rk3x.c | 79 +++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 72 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 93cfc83..49d67b7 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -428,18 +428,83 @@ out:
> return IRQ_HANDLED;
> }
>
> +static void rk3x_i2c_get_ratios(unsigned long scl_rate,
> + unsigned long *high_ratio,
> + unsigned long *low_ratio)
> +{
> + /* 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
> + * - Fast-mode plus:
> + * the minimum HIGH period of the scl clock is 0.26us
> + * the minimum LOW period of the scl clock is 0.5us
> + * - HS-mode(<1.7MHz):
> + * the minimum HIGH period of the scl clock is 0.12us
> + * the minimum LOW period of the scl clock is 0.32us
> + * - HS-mode(<3.4MHz):
> + * the minimum HIGH period of the scl clock is 0.06us
> + * the minimum LOW period of the scl clock is 0.16us

Is the rest of the driver ready for Fast-mode plus or HS mode? If not
then maybe leave those off? If nothing else the commit message should
indicate that this is just being forward thinking.

> + */
> + if (scl_rate <= 100000) {
> + *high_ratio = 40;
> + *low_ratio = 47;
> + } else if (scl_rate <= 400000) {
> + *high_ratio = 6;
> + *low_ratio = 13;
> + } else if (scl_rate <= 1000000) {
> + *high_ratio = 26;
> + *low_ratio = 50;
> + } else if (scl_rate <= 1700000) {
> + *high_ratio = 12;
> + *low_ratio = 32;
> + } else {
> + *high_ratio = 6;
> + *low_ratio = 16;

Since it's only the ratio of high to low that matters, you can combine
the last two. 12 : 32 == 6 : 16

> + }
> +}
> +
> +static void rk3x_i2c_calc_divs(unsigned long i2c_rate, unsigned long scl_rate,
> + unsigned long *divh, unsigned long *divl)
> +{
> + unsigned long high_ratio, low_ratio;
> + unsigned long ratio_sum;
> +
> + rk3x_i2c_get_ratios(scl_rate, &high_ratio, &low_ratio);
> + ratio_sum = high_ratio + low_ratio;
> +
> + /* T_high = T_clk * (divh + 1) * 8
> + * T_low = T_clk * (divl + 1) * 8
> + * T_scl = T_high + T_low
> + * T_scl = 1 / scl_rate
> + * T_clk = 1 / i2c_rate
> + * T_high : T_low = high_ratio : low_ratio
> + * ratio_sum = high_ratio + low_ratio
> + *
> + * so:
> + * divh = (i2c_rate * high_ratio) / (scl_rate * ratio_sum * 8) - 1
> + * divl = (i2c_rate * low_ratio) / (scl_rate * ratio_sum * 8) - 1
> + */
> + *divh = DIV_ROUND_UP(i2c_rate * high_ratio, scl_rate * ratio_sum * 8);
> + if (*divh)
> + *divh = *divh - 1;
> +
> + *divl = DIV_ROUND_UP(i2c_rate * low_ratio, scl_rate * ratio_sum * 8);
> + if (*divl)
> + *divl = *divl - 1;

When I sent you the sample formulas I purposely did it differently
than this. Any reason you changed from my formulas?

div_low = DIV_ROUND_UP(clk_rate * low_ratio, scl_rate * 8 * ratio_sum)
div_high = DIV_ROUND_UP(clk_rate, scl_rate * 8) - div_low

div_low -= 1
if div_high:
div_high -= 1

Why did I do it that way?

* Assuming i2c_rate and the ratio is non-zero then you can assume that
DIV_ROUND_UP gives a value that is >= 1. No need to test the result
against 0.

* (I think) you'll get a more accurate clock rate by subtracting.

Try running your formula vs. my formula with a ratio of 13 : 6, an i2c
rate of 12800000, and an scl rate of 400000

Mine will get:
Req = 400000, act = 400000, 1.88 us low, 0.62 us high, low/high = 3.00

Yours will get:
Req = 400000, act = 320000, 1.88 us low, 1.25 us high, low/high = 1.50


> +}
> +
> static void rk3x_i2c_set_scl_rate(struct rk3x_i2c *i2c, unsigned long scl_rate)
> {
> unsigned long i2c_rate = clk_get_rate(i2c->clk);
> - unsigned int div;
> + unsigned long divh, divl;
>
> - /* set DIV = DIVH = DIVL
> - * SCL rate = (clk rate) / (8 * (DIVH + 1 + DIVL + 1))
> - * = (clk rate) / (16 * (DIV + 1))
> - */
> - div = DIV_ROUND_UP(i2c_rate, scl_rate * 16) - 1;
> + rk3x_i2c_calc_divs(i2c_rate, scl_rate, &divh, &divl);
>
> - i2c_writel(i2c, (div << 16) | (div & 0xffff), REG_CLKDIV);
> + i2c_writel(i2c, (divh << 16) | (divl & 0xffff), REG_CLKDIV);
> }
>
> /**
> --
> 1.8.3.2
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/