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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAD=FV=XOOAbKxqJsxGU4pXbNJtbq_Q4Pj2=9P48NCze5NTJyrw@mail.gmail.com>
Date:	Fri, 6 May 2016 11:00:02 -0700
From:	Doug Anderson <dianders@...omium.org>
To:	"David.Wu" <david.wu@...k-chips.com>
Cc:	Heiko Stübner <heiko@...ech.de>,
	Wolfram Sang <wsa@...-dreams.de>,
	Rob Herring <robh+dt@...nel.org>,
	Andy Shevchenko <andy.shevchenko@...il.com>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Brian Norris <briannorris@...gle.com>,
	David Riley <davidriley@...gle.com>,
	Tao Huang <huangtao@...k-chips.com>,
	Lin Huang <hl@...k-chips.com>, Jianqun Xu <xjq@...k-chips.com>,
	Chris <zyw@...k-chips.com>, Eddie Cai <cf@...k-chips.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v7 8/9] i2c: rk3x: add i2c support for rk3399 soc

Hi

On Fri, May 6, 2016 at 2:32 AM, David.Wu <david.wu@...k-chips.com> 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