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

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

[Addy Ke]

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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
 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
+	 */
+	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;
+	}
+}
+
+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;
+}
+
 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

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

[Doug Anderson]

Addy,

On Tue, Sep 23, 2014 at 6:55 PM, Addy Ke <addy.ke@rock-chips.com> 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@rock-chips.com>
> ---
>  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
>
>

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

[addy ke]

On 2014/9/24 12:10, Doug Anderson wrote:
> Addy,
> 
> On Tue, Sep 23, 2014 at 6:55 PM, Addy Ke <addy.ke@rock-chips.com> 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@rock-chips.com>
>> ---
>>  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
> 
yes, you are right. yours is closer to the scl clock what we want to set.

But if (clk_rate * low_ratio) can not be divisible by (scl_rate * 8 * ratio_sum),
div_low will be round up, and div _high will be round down.
The gap between div_low and div_high is increased.

so maybe we can set:
div_high = DIV_ROUND_UP(clk_rate * high_ratio, scl_rate * 8 * ratio_sum)
div_low = DIV_ROUND_UP(clk_rate, scl_rate * 8) - div_low

i2c rate is 128Mhz:
1) calculate div_high first:
div_high = DIV_ROUND_UP(clk_rate * high_ratio, scl_rate * 8 * ratio_sum)
div_low = DIV_ROUND_UP(clk_rate, scl_rate * 8) - div_low

req = 400000, act = 400000, div_high = 13, div_low = 27

2) calculate div_low first:
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

req = 400000, act = 400000, div_high = 12, div_high = 28

I think that the first is more appropriate.

> 
>> +}
>> +
>>  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
>>
>>
> 
> 
> 

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

[Doug Anderson]

Addy,

On Wed, Sep 24, 2014 at 1:23 AM, addy ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>
>
> On 2014/9/24 12:10, Doug Anderson wrote:
>> Addy,
>>
>> On Tue, Sep 23, 2014 at 6:55 PM, Addy Ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org> 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-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>> ---
>>>  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
>>
> yes, you are right. yours is closer to the scl clock what we want to set.
>
> But if (clk_rate * low_ratio) can not be divisible by (scl_rate * 8 * ratio_sum),
> div_low will be round up, and div _high will be round down.
> The gap between div_low and div_high is increased.

Is that important?  As far as I can tell as long as we are meeting the
minimum requirements for low and high hold times then we're OK.


> so maybe we can set:
> div_high = DIV_ROUND_UP(clk_rate * high_ratio, scl_rate * 8 * ratio_sum)
> div_low = DIV_ROUND_UP(clk_rate, scl_rate * 8) - div_low
>
> i2c rate is 128Mhz:
> 1) calculate div_high first:
> div_high = DIV_ROUND_UP(clk_rate * high_ratio, scl_rate * 8 * ratio_sum)
> div_low = DIV_ROUND_UP(clk_rate, scl_rate * 8) - div_low
>
> req = 400000, act = 400000, div_high = 13, div_low = 27
>
> 2) calculate div_low first:
> 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
>
> req = 400000, act = 400000, div_high = 12, div_high = 28
>
> I think that the first is more appropriate.

I guess I was biasing towards making low a little longer instead of
biasing towards keeping high and low the same.  I'm not sure the bias
is terribly critical.

In any case, I spent more time testing and I realized that my old
formulas could potentially violate hold times because I wasn't careful
enough.  I've crafted some new code that is more careful to ensure
_both_ minimum hold times and minimum clock rate times.

Do you want to spin your patch with these new formulas, or do you
think I should?


(anyone else on the thread listening, feel free to comment).

---

def test_it(min_low_ns, min_high_ns, clk_rate, scl_rate):
  min_total_ns = min_low_ns + min_high_ns

  # We need the total div to be >= this number so we don't clock too fast.
  min_total_div = DIV_ROUND_UP(clk_rate, scl_rate * 8);

  # These are the min dividers needed for hold times.
  min_low_div = DIV_ROUND_UP(clk_rate * min_low_ns, 8 * 1000000000)
  min_high_div = DIV_ROUND_UP(clk_rate * min_high_ns, 8 * 1000000000)
  min_div_for_hold = (min_low_div + min_high_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_high_div
  else:
    # We've got to distribute some time among the low and high so we
    # don't run too fast.
    extra_div = min_total_div - min_div_for_hold

    # We'll try to split things up perfectly evenly, biasing slightly
    # towards having a higher div for low (spend more time low).
    ideal_low_div = DIV_ROUND_UP(clk_rate * min_low_ns,
                                 scl_rate * 8 * min_total_ns)

    # Handle when the ideal low div is going to take up more than we have
    if ideal_low_div > min_low_div + extra_div:
      assert ideal_low_div == min_low_div + extra_div + 1
      ideal_low_div = min_low_div + extra_div

    # Give low the "ideal" and give high whatever extra is left.
    div_low = ideal_low_div
    div_high = min_high_div + (extra_div - (ideal_low_div - min_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 -= 1
  div_high -= 1

  T_pclk_us = 1000000. / clk_rate
  T_sclk_us = 1000000. / scl_rate

  T_low_us = T_pclk_us * (div_low + 1) * 8
  T_high_us = T_pclk_us * (div_high + 1) * 8

  T_tot_us = (T_high_us + T_low_us)
  freq = 1000000. / T_tot_us

  if T_low_us * 1000 < min_low_ns:
    print "ERROR: not low long enough"
  if T_high_us * 1000 < min_high_ns:
    print "ERROR: not high long enough"

  print "CLK = %d, Req = %d, act = %.2f, %.2f us low, " \
        "%.2f us high, low/high = %.2f" % (
        clk_rate, scl_rate, freq, T_low_us,
        T_high_us, T_low_us / T_high_us)

  return (clk_rate, scl_rate, freq, T_low_us, T_high_us)

test_it(4700, 4000, 1484000, 100000)
test_it(4700, 4000, 2000001, 100000)
test_it(4700, 4000, 74250000, 99799)
test_it(4700, 4000, 74250000, 99798)
test_it(4700, 4000, 74250000, 99797)
test_it(4700, 4000, 74250000, 100000)

test_it(1300, 600,   5000000, 400000)
test_it(1300, 600,   9400000, 400000)

test_it(1300,  600, 74250000, 400000)
test_it(1300,  600, 12800000, 400000)
test_it(1300,  600,  6400000, 400000)
test_it(1300,  600,  3200000, 400000)
test_it(1300,  600,  1600000, 400000)
test_it(1300,  600,   800000, 400000)

for i in xrange(800000, 74250000, 100):
  test_it(4700, 4000, i, 100000)

for i in xrange(800000, 74250000, 100):
  test_it(1300, 600, i, 400000)

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

[addy ke]

Hi, Doug

On 2014/9/25 1:13, Doug Anderson wrote:
> Addy,
> 
> On Wed, Sep 24, 2014 at 1:23 AM, addy ke <addy.ke@rock-chips.com> wrote:
>>
>>
>> On 2014/9/24 12:10, Doug Anderson wrote:
>>> Addy,
>>>
>>> On Tue, Sep 23, 2014 at 6:55 PM, Addy Ke <addy.ke@rock-chips.com> 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@rock-chips.com>
>>>> ---
>>>>  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
>>>
>> yes, you are right. yours is closer to the scl clock what we want to set.
>>
>> But if (clk_rate * low_ratio) can not be divisible by (scl_rate * 8 * ratio_sum),
>> div_low will be round up, and div _high will be round down.
>> The gap between div_low and div_high is increased.
> 
> Is that important?  As far as I can tell as long as we are meeting the
> minimum requirements for low and high hold times then we're OK.
> 
> 
In my measurement,all paramter but "Data hold time" are match the characteristics of SCL bus line.
the measured value is 0.928us("data hold time on RK3X"  ~=  "the low period / 2")
but the maximum value described in table is 0.9us

About "Data hold time", there are described in I2C specification:
- for CBUS compatible masters for I2C-bus deivices
- the maximum data hold time has only be met if the device does not stretch the LOW period of the SCL signal.

I have tested on RK3288-Pinky board, there are no error.
But I don't known whether this paramter will affect i2c communications.

>> so maybe we can set:
>> div_high = DIV_ROUND_UP(clk_rate * high_ratio, scl_rate * 8 * ratio_sum)
>> div_low = DIV_ROUND_UP(clk_rate, scl_rate * 8) - div_low
>>
>> i2c rate is 128Mhz:
>> 1) calculate div_high first:
>> div_high = DIV_ROUND_UP(clk_rate * high_ratio, scl_rate * 8 * ratio_sum)
>> div_low = DIV_ROUND_UP(clk_rate, scl_rate * 8) - div_low
>>
>> req = 400000, act = 400000, div_high = 13, div_low = 27
>>
>> 2) calculate div_low first:
>> 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
>>
>> req = 400000, act = 400000, div_high = 12, div_high = 28
>>
>> I think that the first is more appropriate.
> 
> I guess I was biasing towards making low a little longer instead of
> biasing towards keeping high and low the same.  I'm not sure the bias
> is terribly critical.
> 
> In any case, I spent more time testing and I realized that my old
> formulas could potentially violate hold times because I wasn't careful
> enough.  I've crafted some new code that is more careful to ensure
> _both_ minimum hold times and minimum clock rate times.
> 
> Do you want to spin your patch with these new formulas, or do you
> think I should?
> 
I think the new formulas is reasonable, so I will send patch v2 with it today.
Thank you very much.
> 
> (anyone else on the thread listening, feel free to comment).
> 
> ---
> 
> def test_it(min_low_ns, min_high_ns, clk_rate, scl_rate):
>   min_total_ns = min_low_ns + min_high_ns
> 
>   # We need the total div to be >= this number so we don't clock too fast.
>   min_total_div = DIV_ROUND_UP(clk_rate, scl_rate * 8);
> 
>   # These are the min dividers needed for hold times.
>   min_low_div = DIV_ROUND_UP(clk_rate * min_low_ns, 8 * 1000000000)
>   min_high_div = DIV_ROUND_UP(clk_rate * min_high_ns, 8 * 1000000000)
>   min_div_for_hold = (min_low_div + min_high_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_high_div
>   else:
>     # We've got to distribute some time among the low and high so we
>     # don't run too fast.
>     extra_div = min_total_div - min_div_for_hold
> 
>     # We'll try to split things up perfectly evenly, biasing slightly
>     # towards having a higher div for low (spend more time low).
>     ideal_low_div = DIV_ROUND_UP(clk_rate * min_low_ns,
>                                  scl_rate * 8 * min_total_ns)
> 
>     # Handle when the ideal low div is going to take up more than we have
>     if ideal_low_div > min_low_div + extra_div:
>       assert ideal_low_div == min_low_div + extra_div + 1
>       ideal_low_div = min_low_div + extra_div
> 
>     # Give low the "ideal" and give high whatever extra is left.
>     div_low = ideal_low_div
>     div_high = min_high_div + (extra_div - (ideal_low_div - min_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 -= 1
>   div_high -= 1
> 
>   T_pclk_us = 1000000. / clk_rate
>   T_sclk_us = 1000000. / scl_rate
> 
>   T_low_us = T_pclk_us * (div_low + 1) * 8
>   T_high_us = T_pclk_us * (div_high + 1) * 8
> 
>   T_tot_us = (T_high_us + T_low_us)
>   freq = 1000000. / T_tot_us
> 
>   if T_low_us * 1000 < min_low_ns:
>     print "ERROR: not low long enough"
>   if T_high_us * 1000 < min_high_ns:
>     print "ERROR: not high long enough"
> 
>   print "CLK = %d, Req = %d, act = %.2f, %.2f us low, " \
>         "%.2f us high, low/high = %.2f" % (
>         clk_rate, scl_rate, freq, T_low_us,
>         T_high_us, T_low_us / T_high_us)
> 
>   return (clk_rate, scl_rate, freq, T_low_us, T_high_us)
> 
> test_it(4700, 4000, 1484000, 100000)
> test_it(4700, 4000, 2000001, 100000)
> test_it(4700, 4000, 74250000, 99799)
> test_it(4700, 4000, 74250000, 99798)
> test_it(4700, 4000, 74250000, 99797)
> test_it(4700, 4000, 74250000, 100000)
> 
> test_it(1300, 600,   5000000, 400000)
> test_it(1300, 600,   9400000, 400000)
> 
> test_it(1300,  600, 74250000, 400000)
> test_it(1300,  600, 12800000, 400000)
> test_it(1300,  600,  6400000, 400000)
> test_it(1300,  600,  3200000, 400000)
> test_it(1300,  600,  1600000, 400000)
> test_it(1300,  600,   800000, 400000)
> 
> for i in xrange(800000, 74250000, 100):
>   test_it(4700, 4000, i, 100000)
> 
> for i in xrange(800000, 74250000, 100):
>   test_it(1300, 600, i, 400000)
> 
> 
> 

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

[Doug Anderson]

Addy,

On Wed, Sep 24, 2014 at 6:56 PM, addy ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> In my measurement,all paramter but "Data hold time" are match the characteristics of SCL bus line.
> the measured value is 0.928us("data hold time on RK3X"  ~=  "the low period / 2")
> but the maximum value described in table is 0.9us
>
> About "Data hold time", there are described in I2C specification:
> - for CBUS compatible masters for I2C-bus deivices
> - the maximum data hold time has only be met if the device does not stretch the LOW period of the SCL signal.
>
> I have tested on RK3288-Pinky board, there are no error.
> But I don't known whether this paramter will affect i2c communications.

I'll have to spend more time tomorrow to really understand this, but
if changing the code to bias towards slightly longer "high" times
instead of "low" times helps fix it then that's fine with me.

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

[Doug Anderson]

Addy,

On Wed, Sep 24, 2014 at 9:36 PM, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> Addy,
>
> On Wed, Sep 24, 2014 at 6:56 PM, addy ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>> In my measurement,all paramter but "Data hold time" are match the characteristics of SCL bus line.
>> the measured value is 0.928us("data hold time on RK3X"  ~=  "the low period / 2")
>> but the maximum value described in table is 0.9us
>>
>> About "Data hold time", there are described in I2C specification:
>> - for CBUS compatible masters for I2C-bus deivices
>> - the maximum data hold time has only be met if the device does not stretch the LOW period of the SCL signal.
>>
>> I have tested on RK3288-Pinky board, there are no error.
>> But I don't known whether this paramter will affect i2c communications.
>
> I'll have to spend more time tomorrow to really understand this, but
> if changing the code to bias towards slightly longer "high" times
> instead of "low" times helps fix it then that's fine with me.

So what you're saying is that you're seeing a case where the clock
goes low and the data is not valid until .928us.  Is this data that is
being driven by the master or data that is being driven by the slave?

Do you know why the data takes so long to be valid?  Maybe you can
email me some of the waveforms and I can try to help you debug it.


In any case it sounds like the the "data hold time" problem is
unrelated to the clock ratio problem (right?), so maybe you could send
out patch v2?

-Doug

P.S. I checked the Rockchip TRM and it claims 400kHz maximum i2c.  I
think that means you can just remove all of the "fast mode plus" and
"high speed mode" clock rates from your table.

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

[addy ke]

Hi, Doug

On 2014/9/26 5:52, Doug Anderson wrote:
> Addy,
> 
> On Wed, Sep 24, 2014 at 9:36 PM, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>> Addy,
>>
>> On Wed, Sep 24, 2014 at 6:56 PM, addy ke <addy.ke-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>>> In my measurement,all paramter but "Data hold time" are match the characteristics of SCL bus line.
>>> the measured value is 0.928us("data hold time on RK3X"  ~=  "the low period / 2")
>>> but the maximum value described in table is 0.9us
>>>
>>> About "Data hold time", there are described in I2C specification:
>>> - for CBUS compatible masters for I2C-bus deivices
>>> - the maximum data hold time has only be met if the device does not stretch the LOW period of the SCL signal.
>>>
>>> I have tested on RK3288-Pinky board, there are no error.
>>> But I don't known whether this paramter will affect i2c communications.
>>
>> I'll have to spend more time tomorrow to really understand this, but
>> if changing the code to bias towards slightly longer "high" times
>> instead of "low" times helps fix it then that's fine with me.
> 
> So what you're saying is that you're seeing a case where the clock
> goes low and the data is not valid until .928us.  Is this data that is
> being driven by the master or data that is being driven by the slave?
> 
It is driven by the master and will be release at half of LOW period in our IC design.

> Do you know why the data takes so long to be valid?  Maybe you can
> email me some of the waveforms and I can try to help you debug it.
> 
sure, I will email the I2C signal test report table right now.
> 
> In any case it sounds like the the "data hold time" problem is
> unrelated to the clock ratio problem (right?), so maybe you could send
> out patch v2?
> 
Ok, I will send patch v2 today.
thanks.

> -Doug
> 
> P.S. I checked the Rockchip TRM and it claims 400kHz maximum i2c.  I
> think that means you can just remove all of the "fast mode plus" and
> "high speed mode" clock rates from your table.
> 
Ok.
> 
> 

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

[Doug Anderson]

Addy,

On Thu, Sep 25, 2014 at 6:40 PM, addy ke <addy.ke@rock-chips.com> wrote:
> Hi, Doug
>
> On 2014/9/26 5:52, Doug Anderson wrote:
>> Addy,
>>
>> On Wed, Sep 24, 2014 at 9:36 PM, Doug Anderson <dianders@chromium.org> wrote:
>>> Addy,
>>>
>>> On Wed, Sep 24, 2014 at 6:56 PM, addy ke <addy.ke@rock-chips.com> wrote:
>>>> In my measurement,all paramter but "Data hold time" are match the characteristics of SCL bus line.
>>>> the measured value is 0.928us("data hold time on RK3X"  ~=  "the low period / 2")
>>>> but the maximum value described in table is 0.9us
>>>>
>>>> About "Data hold time", there are described in I2C specification:
>>>> - for CBUS compatible masters for I2C-bus deivices
>>>> - the maximum data hold time has only be met if the device does not stretch the LOW period of the SCL signal.
>>>>
>>>> I have tested on RK3288-Pinky board, there are no error.
>>>> But I don't known whether this paramter will affect i2c communications.
>>>
>>> I'll have to spend more time tomorrow to really understand this, but
>>> if changing the code to bias towards slightly longer "high" times
>>> instead of "low" times helps fix it then that's fine with me.
>>
>> So what you're saying is that you're seeing a case where the clock
>> goes low and the data is not valid until .928us.  Is this data that is
>> being driven by the master or data that is being driven by the slave?
>>
> It is driven by the master and will be release at half of LOW period in our IC design.

Ah, I didn't know this.  Is that in the TRM somewhere?

...so does it work to just replace:

    ideal_low_div = DIV_ROUND_UP(clk_rate * min_low_ns,
                                 scl_rate * 8 * min_total_ns)

...with:

    ideal_low_div = min_low_div


That will assign all "extra" time to the "high" part.

-Doug

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

[addy ke]

On 2014/9/26 10:08, Doug Anderson wrote:
> Addy,
> 
> On Thu, Sep 25, 2014 at 6:40 PM, addy ke <addy.ke@rock-chips.com> wrote:
>> Hi, Doug
>>
>> On 2014/9/26 5:52, Doug Anderson wrote:
>>> Addy,
>>>
>>> On Wed, Sep 24, 2014 at 9:36 PM, Doug Anderson <dianders@chromium.org> wrote:
>>>> Addy,
>>>>
>>>> On Wed, Sep 24, 2014 at 6:56 PM, addy ke <addy.ke@rock-chips.com> wrote:
>>>>> In my measurement,all paramter but "Data hold time" are match the characteristics of SCL bus line.
>>>>> the measured value is 0.928us("data hold time on RK3X"  ~=  "the low period / 2")
>>>>> but the maximum value described in table is 0.9us
>>>>>
>>>>> About "Data hold time", there are described in I2C specification:
>>>>> - for CBUS compatible masters for I2C-bus deivices
>>>>> - the maximum data hold time has only be met if the device does not stretch the LOW period of the SCL signal.
>>>>>
>>>>> I have tested on RK3288-Pinky board, there are no error.
>>>>> But I don't known whether this paramter will affect i2c communications.
>>>>
>>>> I'll have to spend more time tomorrow to really understand this, but
>>>> if changing the code to bias towards slightly longer "high" times
>>>> instead of "low" times helps fix it then that's fine with me.
>>>
>>> So what you're saying is that you're seeing a case where the clock
>>> goes low and the data is not valid until .928us.  Is this data that is
>>> being driven by the master or data that is being driven by the slave?
>>>
>> It is driven by the master and will be release at half of LOW period in our IC design.
> 
> Ah, I didn't know this.  Is that in the TRM somewhere?
> 
No, it was my test and confirmed by IC engineer.
You can find that there is a pulse at half of LOW period of the ninth SCL clock each bytes.

> ...so does it work to just replace:
> 
>     ideal_low_div = DIV_ROUND_UP(clk_rate * min_low_ns,
>                                  scl_rate * 8 * min_total_ns)
> 
> ...with:
> 
>     ideal_low_div = min_low_div
> 
> 
> That will assign all "extra" time to the "high" part.
> 
I Think it is more reasonable.
> -Doug
> 
> 
>