diff mbox

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

Message ID 1411801888-3152-1-git-send-email-addy.ke@rock-chips.com
State New, archived
Headers show

Commit Message

addy ke Sept. 27, 2014, 7:11 a.m. UTC
From: Addy <addy.ke@rock-chips.com>

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.

After put this patch, we can find that min_low_ns is about
5.4us in Standard-mode and 1.6us in Fast-mode.

Thanks Doug for the suggestion about division formulas.

Signed-off-by: Addy <addy.ke@rock-chips.com>
---
Changes in v2:
- remove Fast-mode plus and HS-mode
- use new formulas suggested by Doug

 drivers/i2c/busses/i2c-rk3x.c | 104 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 97 insertions(+), 7 deletions(-)

Comments

Doug Anderson Sept. 29, 2014, 4:39 a.m. UTC | #1
Addy,

On Sat, Sep 27, 2014 at 12:11 AM, Addy Ke <addy.ke@rock-chips.com> wrote:
> From: Addy <addy.ke@rock-chips.com>
>
> 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.
>
> After put this patch, we can find that min_low_ns is about
> 5.4us in Standard-mode and 1.6us in Fast-mode.
>
> Thanks Doug for the suggestion about division formulas.
>
> Signed-off-by: Addy <addy.ke@rock-chips.com>
> ---
> Changes in v2:
> - remove Fast-mode plus and HS-mode
> - use new formulas suggested by Doug
>
>  drivers/i2c/busses/i2c-rk3x.c | 104 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 97 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index e637c32..81672a8 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -428,19 +428,109 @@ out:
>         return IRQ_HANDLED;
>  }
>
> +static void rk3x_i2c_get_min_ns(unsigned int scl_rate,
> +                               unsigned long *min_low_ns,
> +                               unsigned long *min_high_ns)
> +{
> +       WARN_ON(scl_rate > 400000);
> +
> +       /* As show in I2C specification:
> +        * - Standard-mode(100KHz):
> +        *   min_low_ns is 4700ns
> +        *   min_high_ns is 4000ns
> +        * - Fast-mode(400KHz):
> +        *   min_low_ns is 1300ns
> +        *   min_high_ns is 600ns
> +        *
> +        * (min_low_ns + min_high_ns) up to 10000ns in Standard-mode
> +        * and 2500ns in Fast-mode
> +        */
> +       if (scl_rate <= 100000) {
> +               *min_low_ns = 4700 + 650;
> +               *min_high_ns = 4000 + 650;
> +       } else {
> +               *min_low_ns = 1300 + 300;
> +               *min_high_ns = 600 + 300;

Wait, where did the extra 650 and 300 come from here?  Are you just
trying to balance things out?  That's not the right way to do things.
Please explain what you were trying to accomplish and we can figure
out a better way.

...maybe this helped make thins nicer in the clock rates you tried,
but I'd bet that I can find clock rates that this is broken for...


> +       }
> +}
> +
> +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, min_total_ns;
> +       unsigned long min_low_div, min_high_div;
> +       unsigned long min_total_div, min_div_for_hold;
> +       unsigned long extra_div, extra_low_div, ideal_low_div;
> +       unsigned long i2c_rate_khz, scl_rate_khz;
> +
> +       rk3x_i2c_get_min_ns(scl_rate, &min_low_ns, &min_high_ns);
> +       min_total_ns = min_low_ns + min_high_ns;
> +
> +       /* To avoid from overflow warning */
> +       i2c_rate_khz = i2c_rate / 1000;
> +       scl_rate_khz = scl_rate / 1000;

DIV_ROUND_UP?

> +
> +       /*We need the total div to be >= this number
> +        * so we don't clock too fast.
> +        */

Please match the commenting style in the rest of the file.  That's:

/*
 * A
 * B
 * C
 */

Not:

/* A
 * B
 * C
 */

> +       min_total_div = DIV_ROUND_UP(i2c_rate_khz, scl_rate_khz * 8);
> +
> +       /* These are the min dividers needed for hold times */
> +       min_low_div = DIV_ROUND_UP(i2c_rate_khz * min_low_ns, 8 * 1000000);
> +       min_high_div = DIV_ROUND_UP(i2c_rate_khz * min_high_ns, 8 * 1000000);
> +       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
> +                * */

Extra "*" in "* */"


> +               *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).
> +                */

Last I remember you said that this wasn't going to work quite right
and you were just going to assign all of the "extra" to the high part.
What changed?


> +               ideal_low_div = DIV_ROUND_UP(i2c_rate_khz * min_low_ns,
> +                                            scl_rate_khz * 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)
> +                       ideal_low_div = min_low_div + extra_div;
> +               /* 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);
> +       }
> +
> +       /* Adjust to the fact that the hardware has an implicit "+1".*/
> +       *div_low = *div_low - 1;
> +       *div_high = *div_high - 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 div_low, div_high;
>
> -       /* SCL rate = (clk rate) / (8 * DIV) */
> -       div = DIV_ROUND_UP(i2c_rate, scl_rate * 8);

Strangely this now seems to be based upon an older version of the
file.  I had to revert (b4a7bd7 i2c: rk3x: fix divisor calculation for
SCL frequency) in order to apply v2.  Please submit your code against
the latest accepted code.


> +       /* The formulas in rk3x TRM:
> +        * - T_scl_high = T_clk * (divh + 1) * 8
> +        * - T_scl_low = T_clk * (divl + 1) * 8
> +        * */
> +       rk3x_i2c_calc_divs(i2c_rate, scl_rate, &div_low, &div_high);
>
> -       /* The lower and upper half of the CLKDIV reg describe the length of
> -        * SCL low & high periods. */
> -       div = DIV_ROUND_UP(div, 2);
> +       i2c_writel(i2c, (div_high << 16) | (div_low & 0xffff), REG_CLKDIV);
>
> -       i2c_writel(i2c, (div << 16) | (div & 0xffff), REG_CLKDIV);
> +       dev_dbg(i2c->dev, "scl_ns: %luns, scl_low_ns: %luns, scl_high_ns: %luns\n",
> +               1000000000/scl_rate,
> +               (1000000000 / i2c_rate) * (div_low + 1) * 8,
> +               (1000000000 / i2c_rate) * (div_high + 1) * 8);
>  }
>
>  /**
> --
> 1.8.3.2
>
>
addy ke Sept. 29, 2014, 7:45 a.m. UTC | #2
Hi Doug

On 2014/9/29 12:39, Doug Anderson wrote:
> Addy,
> 
> On Sat, Sep 27, 2014 at 12:11 AM, Addy Ke <addy.ke@rock-chips.com> wrote:
>> From: Addy <addy.ke@rock-chips.com>
>>
>> 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.
>>
>> After put this patch, we can find that min_low_ns is about
>> 5.4us in Standard-mode and 1.6us in Fast-mode.
>>
>> Thanks Doug for the suggestion about division formulas.
>>
>> Signed-off-by: Addy <addy.ke@rock-chips.com>
>> ---
>> Changes in v2:
>> - remove Fast-mode plus and HS-mode
>> - use new formulas suggested by Doug
>>
>>  drivers/i2c/busses/i2c-rk3x.c | 104 +++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 97 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
>> index e637c32..81672a8 100644
>> --- a/drivers/i2c/busses/i2c-rk3x.c
>> +++ b/drivers/i2c/busses/i2c-rk3x.c
>> @@ -428,19 +428,109 @@ out:
>>         return IRQ_HANDLED;
>>  }
>>
>> +static void rk3x_i2c_get_min_ns(unsigned int scl_rate,
>> +                               unsigned long *min_low_ns,
>> +                               unsigned long *min_high_ns)
>> +{
>> +       WARN_ON(scl_rate > 400000);
>> +
>> +       /* As show in I2C specification:
>> +        * - Standard-mode(100KHz):
>> +        *   min_low_ns is 4700ns
>> +        *   min_high_ns is 4000ns
>> +        * - Fast-mode(400KHz):
>> +        *   min_low_ns is 1300ns
>> +        *   min_high_ns is 600ns
>> +        *
>> +        * (min_low_ns + min_high_ns) up to 10000ns in Standard-mode
>> +        * and 2500ns in Fast-mode
>> +        */
>> +       if (scl_rate <= 100000) {
>> +               *min_low_ns = 4700 + 650;
>> +               *min_high_ns = 4000 + 650;
>> +       } else {
>> +               *min_low_ns = 1300 + 300;
>> +               *min_high_ns = 600 + 300;
> 
> Wait, where did the extra 650 and 300 come from here?  Are you just
> trying to balance things out?  That's not the right way to do things.
> Please explain what you were trying to accomplish and we can figure
> out a better way.
> 
> ...maybe this helped make thins nicer in the clock rates you tried,
> but I'd bet that I can find clock rates that this is broken for...
> 
> 
650 = [(1000000000/100K) ns - min_low_ns_100k - min_high_ns_100k] / 2
300 = [(1000000000/300K) ns - min_low_ns_400k - min_high_ns_400k] / 2

if there is no extra 650 and 300:
extra_div is 3 in fast-mode and 11 in standard-mode.

  1)if all of the "extra" is asssigned to the high part, the LOW period is too close to min_low.
    In my test:
    400k, scl_low_ns: 1440ns, scl_high_ns: 1120ns
    100k, scl_low_ns: 4800ns, scl_high_ns: 5120ns

  2)if not, dato hold time is too close to min_hold_time(400khz, 900ns)
    In my test:
    400k, scl_low_ns: 1760ns, scl_high_ns: 800ns
    100k, scl_low_ns: 5304ns, scl_high_ns: 4368ns

    hold_time_ns ~= scl_low_ns / 2 = 880ns

So I think we must decrease min_low_ns:min_high_ns to decrease scl_low in case 2

So I set the extra 650 and 300.

After this, in my test:
   400k, scl_low_ns: 1600ns, scl_high_ns: 960ns
   100k, scl_low_ns: 5200ns, scl_hign_ns: 4576ns

I think this value is more reasonable.

ps:
min_low_ns/min_high_ns >  (min_low_ns + extra) / (min_high_ns + extra) (if min_low_ns > min_high_ns)

>> +       }
>> +}
>> +
>> +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, min_total_ns;
>> +       unsigned long min_low_div, min_high_div;
>> +       unsigned long min_total_div, min_div_for_hold;
>> +       unsigned long extra_div, extra_low_div, ideal_low_div;
>> +       unsigned long i2c_rate_khz, scl_rate_khz;
>> +
>> +       rk3x_i2c_get_min_ns(scl_rate, &min_low_ns, &min_high_ns);
>> +       min_total_ns = min_low_ns + min_high_ns;
>> +
>> +       /* To avoid from overflow warning */
>> +       i2c_rate_khz = i2c_rate / 1000;
>> +       scl_rate_khz = scl_rate / 1000;
> 
> DIV_ROUND_UP?
> 
>> +
>> +       /*We need the total div to be >= this number
>> +        * so we don't clock too fast.
>> +        */
> 
> Please match the commenting style in the rest of the file.  That's:
> 
> /*
>  * A
>  * B
>  * C
>  */
> 
> Not:
> 
> /* A
>  * B
>  * C
>  */
> 
>> +       min_total_div = DIV_ROUND_UP(i2c_rate_khz, scl_rate_khz * 8);
>> +
>> +       /* These are the min dividers needed for hold times */
>> +       min_low_div = DIV_ROUND_UP(i2c_rate_khz * min_low_ns, 8 * 1000000);
>> +       min_high_div = DIV_ROUND_UP(i2c_rate_khz * min_high_ns, 8 * 1000000);
>> +       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
>> +                * */
> 
> Extra "*" in "* */"
> 
> 
>> +               *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).
>> +                */
> 
> Last I remember you said that this wasn't going to work quite right
> and you were just going to assign all of the "extra" to the high part.
> What changed?
if all of the "extra" is asssigned to the high part, the LOW period is too close to min_low.
As shown in the above.
> 
> 
>> +               ideal_low_div = DIV_ROUND_UP(i2c_rate_khz * min_low_ns,
>> +                                            scl_rate_khz * 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)
>> +                       ideal_low_div = min_low_div + extra_div;
>> +               /* 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);
>> +       }
>> +
>> +       /* Adjust to the fact that the hardware has an implicit "+1".*/
>> +       *div_low = *div_low - 1;
>> +       *div_high = *div_high - 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 div_low, div_high;
>>
>> -       /* SCL rate = (clk rate) / (8 * DIV) */
>> -       div = DIV_ROUND_UP(i2c_rate, scl_rate * 8);
> 
> Strangely this now seems to be based upon an older version of the
> file.  I had to revert (b4a7bd7 i2c: rk3x: fix divisor calculation for
> SCL frequency) in order to apply v2.  Please submit your code against
> the latest accepted code.
> 
> 
This patch is based on ../kernel/git/wsa/linux.git, master branch.
It is my mistake.
I will checkout for_next branch.
Thank you.

>> +       /* The formulas in rk3x TRM:
>> +        * - T_scl_high = T_clk * (divh + 1) * 8
>> +        * - T_scl_low = T_clk * (divl + 1) * 8
>> +        * */
>> +       rk3x_i2c_calc_divs(i2c_rate, scl_rate, &div_low, &div_high);
>>
>> -       /* The lower and upper half of the CLKDIV reg describe the length of
>> -        * SCL low & high periods. */
>> -       div = DIV_ROUND_UP(div, 2);
>> +       i2c_writel(i2c, (div_high << 16) | (div_low & 0xffff), REG_CLKDIV);
>>
>> -       i2c_writel(i2c, (div << 16) | (div & 0xffff), REG_CLKDIV);
>> +       dev_dbg(i2c->dev, "scl_ns: %luns, scl_low_ns: %luns, scl_high_ns: %luns\n",
>> +               1000000000/scl_rate,
>> +               (1000000000 / i2c_rate) * (div_low + 1) * 8,
>> +               (1000000000 / i2c_rate) * (div_high + 1) * 8);
>>  }
>>
>>  /**
>> --
>> 1.8.3.2
>>
>>
> 
> 
>
Doug Anderson Sept. 30, 2014, 10:23 p.m. UTC | #3
Addy,

On Mon, Sep 29, 2014 at 12:45 AM, addy ke <addy.ke@rock-chips.com> wrote:
> Hi Doug
>
> On 2014/9/29 12:39, Doug Anderson wrote:
>> Addy,
>>
>> On Sat, Sep 27, 2014 at 12:11 AM, Addy Ke <addy.ke@rock-chips.com> wrote:
>>> From: Addy <addy.ke@rock-chips.com>
>>>
>>> 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.
>>>
>>> After put this patch, we can find that min_low_ns is about
>>> 5.4us in Standard-mode and 1.6us in Fast-mode.
>>>
>>> Thanks Doug for the suggestion about division formulas.
>>>
>>> Signed-off-by: Addy <addy.ke@rock-chips.com>
>>> ---
>>> Changes in v2:
>>> - remove Fast-mode plus and HS-mode
>>> - use new formulas suggested by Doug
>>>
>>>  drivers/i2c/busses/i2c-rk3x.c | 104 +++++++++++++++++++++++++++++++++++++++---
>>>  1 file changed, 97 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
>>> index e637c32..81672a8 100644
>>> --- a/drivers/i2c/busses/i2c-rk3x.c
>>> +++ b/drivers/i2c/busses/i2c-rk3x.c
>>> @@ -428,19 +428,109 @@ out:
>>>         return IRQ_HANDLED;
>>>  }
>>>
>>> +static void rk3x_i2c_get_min_ns(unsigned int scl_rate,
>>> +                               unsigned long *min_low_ns,
>>> +                               unsigned long *min_high_ns)
>>> +{
>>> +       WARN_ON(scl_rate > 400000);
>>> +
>>> +       /* As show in I2C specification:
>>> +        * - Standard-mode(100KHz):
>>> +        *   min_low_ns is 4700ns
>>> +        *   min_high_ns is 4000ns
>>> +        * - Fast-mode(400KHz):
>>> +        *   min_low_ns is 1300ns
>>> +        *   min_high_ns is 600ns
>>> +        *
>>> +        * (min_low_ns + min_high_ns) up to 10000ns in Standard-mode
>>> +        * and 2500ns in Fast-mode
>>> +        */
>>> +       if (scl_rate <= 100000) {
>>> +               *min_low_ns = 4700 + 650;
>>> +               *min_high_ns = 4000 + 650;
>>> +       } else {
>>> +               *min_low_ns = 1300 + 300;
>>> +               *min_high_ns = 600 + 300;
>>
>> Wait, where did the extra 650 and 300 come from here?  Are you just
>> trying to balance things out?  That's not the right way to do things.
>> Please explain what you were trying to accomplish and we can figure
>> out a better way.
>>
>> ...maybe this helped make thins nicer in the clock rates you tried,
>> but I'd bet that I can find clock rates that this is broken for...
>>
>>
> 650 = [(1000000000/100K) ns - min_low_ns_100k - min_high_ns_100k] / 2
> 300 = [(1000000000/300K) ns - min_low_ns_400k - min_high_ns_400k] / 2
>
> if there is no extra 650 and 300:
> extra_div is 3 in fast-mode and 11 in standard-mode.

Umm, OK.  Except that "min_low_ns" is no longer the actual minimum
"ns" we need according to the spec.  That will mean that you're
sometimes going to end up with a clock rate that's slower than you
want.


>   1)if all of the "extra" is asssigned to the high part, the LOW period is too close to min_low.
>     In my test:
>     400k, scl_low_ns: 1440ns, scl_high_ns: 1120ns
>     100k, scl_low_ns: 4800ns, scl_high_ns: 5120ns

Why does it matter if it's close to "min_low"?  It's above the minimum
so it's fine, right?  If you need a buffer, add an explicit buffer to
the calculations.


>   2)if not, dato hold time is too close to min_hold_time(400khz, 900ns)
>     In my test:
>     400k, scl_low_ns: 1760ns, scl_high_ns: 800ns
>     100k, scl_low_ns: 5304ns, scl_high_ns: 4368ns
>
>     hold_time_ns ~= scl_low_ns / 2 = 880ns

It's still under, though.  Why does this matter?  Again, if you need a
buffer then add a buffer.


> So I think we must decrease min_low_ns:min_high_ns to decrease scl_low in case 2
>
> So I set the extra 650 and 300.
>
> After this, in my test:
>    400k, scl_low_ns: 1600ns, scl_high_ns: 960ns
>    100k, scl_low_ns: 5200ns, scl_hign_ns: 4576ns
>
> I think this value is more reasonable.

You're testing with a very particular clock input rate.  The function
needs to be general and work across a lot of different input clock
rates.  That's why my python code has loops in it to test the results
over a huge variety of input clock rates...

---

Here are a few examples of differences (SEP 30 is my new proposal below):

With this example your code produces a slightly slower clock rate than
desirable:

SEP 30: CLK = 74250000, Req = 100000, act = 99798.39, 5.49 us low,
4.53 us high, low/high = 1.21 (50 41)
AddyV2: CLK = 74250000, Req = 100000, act = 98736.70, 5.39 us low,
4.74 us high, low/high = 1.14 (49 43)

Here's another example where your patch doesn't produce ideal timings
(in this case the difference is more pronounced)

SEP 30: CLK = 66380000, Req = 400000, act = 395119.05, 1.69 us low,
0.84 us high, low/high = 2.00 (13 6)
AddyV2: CLK = 66380000, Req = 400000, act = 377159.09, 1.69 us low,
0.96 us high, low/high = 1.75 (13 7)

Here's an example your code violates timings (true, a 1.61MHz clock
isn't super realistic in rk3288, but it does show a non-ideal case):

SEP 30: CLK = 1610000, Req = 100000, act = 67083.33, 4.97 us low, 9.94
us high, low/high = 0.50 (0 1)
AddyV2: CLK = 1610000, Req = 100000, act = 67083.33, 9.94 us low, 4.97
us high, low/high = 2.00 (1 0)
...ERROR: low too long 9.94 us > 6.85 us (/2 4.97 us > 3.42 us) (conflicting=0)

---

I'll paste new code here with a proposal that adds a buffer (could be
0) and also send you an attachment in a separate email (I can't quite
believe that the lists will handle attachments).

One thing you'll notice is that with sufficiently slow input clocks
it's possible to get into a state where we can't meet both the minimum
and maximum times for SCL being low.  This probably isn't super
realistic, but the code could certainly have a warning.

---

def DIV_ROUND_UP(a, b): return (a + b - 1) // b

# This is bassed on v2 sent by Addy (fixed DIV_ROUND_UP)
def test_it_addy_v2(min_low_ns, min_high_ns, max_low_ns, i2c_rate, scl_rate):
  # Add the buffer in suggested by addy
  if min_low_ns == 4700:
    min_low_ns += 650
    min_high_ns += 650
  else:
    min_low_ns += 300
    min_high_ns += 300

  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)

  # 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 * 1000000)
  min_high_div = DIV_ROUND_UP(i2c_rate_khz * min_high_ns, 8 * 1000000)
  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(i2c_rate_khz * min_low_ns,
                                 scl_rate_khz * 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

  return div_low, div_high, False # Note: we don't calculate "conflicting"

# test_it_sep30 - Sept 30th proposal for i2c stuff
#
# 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.
def test_it_sep30(min_low_ns, min_high_ns, max_low_ns, i2c_rate, scl_rate):
  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)

  # 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 * 1000000)
  min_high_div = DIV_ROUND_UP(i2c_rate_khz * min_high_ns, 8 * 1000000)
  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 * 1000000)

  # Giving different rounding, it's possible that min > max (!?!)  We're
  # totally out of luck in that case, so let's go with getting clocks
  # right I guess...
  if min_low_div > max_low_div:
    # print "WARNING: conflicting restrictions"
    conflicting = True
    max_low_div = min_low_div
  else:
    conflicting = False

  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(i2c_rate_khz * min_low_ns,
                                 scl_rate_khz * 8 * min_total_ns)

    # Don't allow it to go over the max
    if ideal_low_div > max_low_div:
      ideal_low_div = max_low_div

    # 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

  return div_low, div_high, conflicting

def print_it(label, min_low_ns, min_high_ns, max_low_ns, i2c_rate, scl_rate,
             div_low, div_high, conflicting):
  T_pclk_us = 1000000. / i2c_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

  print "%s: CLK = %d, Req = %d, act = %.2f, %.2f us low, " \
        "%.2f us high, low/high = %.2f (%d %d)" % (
        label,
        i2c_rate, scl_rate, freq, T_low_us,
        T_high_us, T_low_us / T_high_us, div_low, div_high)

  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"

  if T_low_us * 1000 > max_low_ns:
    print "...ERROR: low too long %.2f us > %.2f us " \
      "(/2 %.2f us > %.2f us) (conflicting=%d)" % (
      T_low_us, max_low_ns / 1000.,
      T_low_us / 2., max_low_ns / 2000.,
      conflicting)
  elif conflicting:
    print "...Conflicting, but not low too long?"

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


def test_it(min_low_ns, min_high_ns, max_low_ns, i2c_rate, scl_rate):
  sep30 = test_it_sep30(min_low_ns, min_high_ns, max_low_ns,
                        i2c_rate, scl_rate)
  addy_v2 = test_it_addy_v2(min_low_ns, min_high_ns, max_low_ns,
                            i2c_rate, scl_rate)

  print_it("SEP 30", min_low_ns, min_high_ns, max_low_ns,
           i2c_rate, scl_rate, sep30[0], sep30[1], sep30[2])
  print_it("AddyV2", min_low_ns, min_high_ns, max_low_ns,
           i2c_rate, scl_rate, addy_v2[0], addy_v2[1], addy_v2[2])

  # Test to see where results are differet
  #if (sep30[0], sep30[1]) != (addy_v2[0], addy_v2[1]):
  #  print_it("SEP 30", min_low_ns, min_high_ns, max_low_ns,
  #           i2c_rate, scl_rate, sep30[0], sep30[1], sep30[2])
  #  print_it("AddyV2", min_low_ns, min_high_ns, max_low_ns,
  #           i2c_rate, scl_rate, addy_v2[0], addy_v2[1], addy_v2[2])

  # Test to see where clock rates are different.
  #if (sep30[0] + sep30[1]) != (addy_v2[0] + addy_v2[1]):
  #  print_it("SEP 30", min_low_ns, min_high_ns, max_low_ns,
  #           i2c_rate, scl_rate, sep30[0], sep30[1], sep30[2])
  #  print_it("AddyV2", min_low_ns, min_high_ns, max_low_ns,
  #           i2c_rate, scl_rate, addy_v2[0], addy_v2[1], addy_v2[2])


min_low_std_ns = 4700
min_high_std_ns = 4000
max_data_hold_std_ns = 3450
data_hold_buffer_std_ns = 50

min_low_fast_ns =  1300
min_high_fast_ns = 600
max_data_hold_fast_ns = 900
data_hold_buffer_fast_ns = 50

test_it(min_low_std_ns, min_high_std_ns,
        max_data_hold_std_ns * 2 - data_hold_buffer_std_ns, 74250000, 100000)
test_it(min_low_std_ns, min_high_std_ns,
        max_data_hold_std_ns * 2 - data_hold_buffer_std_ns,  2000001, 100000)
test_it(min_low_std_ns, min_high_std_ns,
        max_data_hold_std_ns * 2 - data_hold_buffer_std_ns,  1610000, 100000)
test_it(min_low_std_ns, min_high_std_ns,
        max_data_hold_std_ns * 2 - data_hold_buffer_std_ns,  1484000, 100000)

test_it(min_low_fast_ns, min_high_fast_ns,
        max_data_hold_fast_ns * 2 - data_hold_buffer_fast_ns, 66380000, 400000)
test_it(min_low_fast_ns, min_high_fast_ns,
        max_data_hold_fast_ns * 2 - data_hold_buffer_fast_ns, 74250000, 400000)
test_it(min_low_fast_ns, min_high_fast_ns,
        max_data_hold_fast_ns * 2 - data_hold_buffer_fast_ns, 12800000, 400000)
test_it(min_low_fast_ns, min_high_fast_ns,
        max_data_hold_fast_ns * 2 - data_hold_buffer_fast_ns,  9400000, 400000)
test_it(min_low_fast_ns, min_high_fast_ns,
        max_data_hold_fast_ns * 2 - data_hold_buffer_fast_ns,  6400000, 400000)
test_it(min_low_fast_ns, min_high_fast_ns,
        max_data_hold_fast_ns * 2 - data_hold_buffer_fast_ns,  5000000, 400000)
test_it(min_low_fast_ns, min_high_fast_ns,
        max_data_hold_fast_ns * 2 - data_hold_buffer_fast_ns,  3200000, 400000)
test_it(min_low_fast_ns, min_high_fast_ns,
        max_data_hold_fast_ns * 2 - data_hold_buffer_fast_ns,  1600000, 400000)
test_it(min_low_fast_ns, min_high_fast_ns,
        max_data_hold_fast_ns * 2 - data_hold_buffer_fast_ns,   800000, 400000)

#for i in xrange(74250000, 800000, -100):
#  test_it(min_low_std_ns, min_high_std_ns,
#          max_data_hold_std_ns * 2 - data_hold_buffer_std_ns,  i, 100000)
#
#for i in xrange(74250000, 800000, -100):
#  test_it(min_low_fast_ns, min_high_fast_ns,
#          max_data_hold_fast_ns * 2 - data_hold_buffer_fast_ns, i, 400000)

---

-Doug
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index e637c32..81672a8 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -428,19 +428,109 @@  out:
 	return IRQ_HANDLED;
 }
 
+static void rk3x_i2c_get_min_ns(unsigned int scl_rate,
+				unsigned long *min_low_ns,
+				unsigned long *min_high_ns)
+{
+	WARN_ON(scl_rate > 400000);
+
+	/* As show in I2C specification:
+	 * - Standard-mode(100KHz):
+	 *   min_low_ns is 4700ns
+	 *   min_high_ns is 4000ns
+	 * - Fast-mode(400KHz):
+	 *   min_low_ns is 1300ns
+	 *   min_high_ns is 600ns
+	 *
+	 * (min_low_ns + min_high_ns) up to 10000ns in Standard-mode
+	 * and 2500ns in Fast-mode
+	 */
+	if (scl_rate <= 100000) {
+		*min_low_ns = 4700 + 650;
+		*min_high_ns = 4000 + 650;
+	} else {
+		*min_low_ns = 1300 + 300;
+		*min_high_ns = 600 + 300;
+	}
+}
+
+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, min_total_ns;
+	unsigned long min_low_div, min_high_div;
+	unsigned long min_total_div, min_div_for_hold;
+	unsigned long extra_div, extra_low_div, ideal_low_div;
+	unsigned long i2c_rate_khz, scl_rate_khz;
+
+	rk3x_i2c_get_min_ns(scl_rate, &min_low_ns, &min_high_ns);
+	min_total_ns = min_low_ns + min_high_ns;
+
+	/* To avoid from overflow warning */
+	i2c_rate_khz = i2c_rate / 1000;
+	scl_rate_khz = scl_rate / 1000;
+
+	/*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 hold times */
+	min_low_div = DIV_ROUND_UP(i2c_rate_khz * min_low_ns, 8 * 1000000);
+	min_high_div = DIV_ROUND_UP(i2c_rate_khz * min_high_ns, 8 * 1000000);
+	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(i2c_rate_khz * min_low_ns,
+					     scl_rate_khz * 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)
+			ideal_low_div = min_low_div + extra_div;
+		/* 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);
+	}
+
+	/* Adjust to the fact that the hardware has an implicit "+1".*/
+	*div_low = *div_low - 1;
+	*div_high = *div_high - 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 div_low, div_high;
 
-	/* SCL rate = (clk rate) / (8 * DIV) */
-	div = DIV_ROUND_UP(i2c_rate, scl_rate * 8);
+	/* The formulas in rk3x TRM:
+	 * - T_scl_high = T_clk * (divh + 1) * 8
+	 * - T_scl_low = T_clk * (divl + 1) * 8
+	 * */
+	rk3x_i2c_calc_divs(i2c_rate, scl_rate, &div_low, &div_high);
 
-	/* The lower and upper half of the CLKDIV reg describe the length of
-	 * SCL low & high periods. */
-	div = DIV_ROUND_UP(div, 2);
+	i2c_writel(i2c, (div_high << 16) | (div_low & 0xffff), REG_CLKDIV);
 
-	i2c_writel(i2c, (div << 16) | (div & 0xffff), REG_CLKDIV);
+	dev_dbg(i2c->dev, "scl_ns: %luns, scl_low_ns: %luns, scl_high_ns: %luns\n",
+		1000000000/scl_rate,
+		(1000000000 / i2c_rate) * (div_low + 1) * 8,
+		(1000000000 / i2c_rate) * (div_high + 1) * 8);
 }
 
 /**