Re: [PATCH v7 8/9] i2c: rk3x: add i2c support for rk3399 soc

[Doug Anderson]

Hi

On Fri, May 6, 2016 at 2:32 AM, David.Wu <david.wu@xxxxxxxxxxxxxx> wrote:
>> Also (optional): once you do that, there becomes much less of a reason
>> to store "t_calc" in "struct rk3x_i2c".  Already you're never using
>> the "div_low" and "div_high" that you store in the "struct rk3x_i2c".
>> Of course, to do that you've got to change other places not to clobber
>> these bits in REG_CON.
>>
>
> So, I only just need to store tuning value in the "struct rk3x_i2c", but not
> to store the "div_low" and "div_high"?

I was suggesting not adding anything to what's stored in "struct
rk3x_i2c" (AKA don't add t_calc there).  Just set the value directly
in the register here then change all other bits of code to respect it.

That is:

In rk3x_i2c_start(), write:

  u32 val = i2c_readl(i2c, REG_CON) & REG_CON_TUNING_MASK;

In rk3x_i2c_xfer(), write:
  val = i2c_readl(i2c, REG_CON) & REG_CON_TUNING_MASK
  val |= REG_CON_EN | REG_CON_STOP, REG_CON;
  i2c_writel(i2c, val);


You could get fancy and add a new "i2c_update_bits", but that might be overkill.


> The patches we have already used in our projects, they are verified by the
> basic tests. I would ask them to do more tests. Because we didn't change the
> clock rate now, it was a fixed value when clk inited, so we could not find
> the bug here.

OK.  Presumably a rk3066 w/ DVS enabled would be a good test?  Reading
the description of commit 249051f49907 ("i2c: rk3x: handle dynamic
clock rate changes correctly"), I see:

    The i2c input clock can change dynamically, e.g. on the RK3066 where
    pclk_i2c0 and pclk_i2c1 are connected to the armclk, which changes
    rate on cpu frequency scaling.


>> If you take that advice, you can get rid of all of the "if
>> (i2c->pclk)" statements in your code.
>>
>
> It would make i2c->clk to be enabled and prepared twice when uses
> rk3x_i2c_v0_calc_timings for old hardware. But if do the opposite
> disabled and unprepated twice, that is okay.

Right.  The same clock will get an enable / prepare count of "2"
(instead of two different clocks getting a count of "1).  ...but
nothing should be hurt by that.

-Doug