| 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
| ||
|
Message-ID: <548515A7.9050702@rock-chips.com>
Date: Mon, 08 Dec 2014 11:06:15 +0800
From: addy ke <addy.ke@...k-chips.com>
To: wsa@...-dreams.de, max.schwarz@...ine.de, heiko@...ech.de,
olof@...om.net, dianders@...omium.org, robh+dt@...nel.org,
pawel.moll@....com, ijc+devicetree@...lion.org.uk,
galak@...eaurora.org
CC: linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, cf@...k-chips.com,
xjq@...k-chips.com, huangtao@...k-chips.com, zyw@...k-chips.com,
yzq@...k-chips.com, hj@...k-chips.com, kever.yang@...k-chips.com,
hl@...k-chips.com, caesar.wang@...k-chips.com,
zhengsq@...k-chips.com
Subject: Re: [PATCH v3] i2c: rk3x: fix bug that cause measured high_ns doesn't
meet I2C spec
On 2014/12/8 10:59, Addy Ke wrote:
> high_ns calculated from the low division of CLKDIV register is the sum
> of actual measured high_ns and rise_ns. The rise time which related to
> external pull-up resistor can be up to the maximum rise time in I2C spec.
>
> In my test, if external pull-up resistor is 4.7K, rise_ns is about
> 700ns. So the actual measured high_ns is about 3900ns, which is less
> than 4000ns(the minimum high_ns in I2C spec).
>
> To fix this bug, min_low_ns should include fall time and min_high_ns
> should include rise time too.
>
> This patch merged the patch that Doug submitted to chromium, which
> can get the rise and fall times for signals from the device tree.
> This allows us to more accurately calculate timings. see:
> https://chromium-review.googlesource.com/#/c/232774/
>
> Signed-off-by: Addy Ke <addy.ke@...k-chips.com>
> ---
> Changes in v2:
> - merged the patch that Doug submitted to chromium
> Changes in v3:
> - merged the patch that Doug submitted to chromium to change bindins
> see: https://chromium-review.googlesource.com/#/c/232775/
Sorry, see https://chromium-review.googlesource.com/#/c/232774/ not 232775
>
> Documentation/devicetree/bindings/i2c/i2c-rk3x.txt | 11 +++++
> drivers/i2c/busses/i2c-rk3x.c | 57 +++++++++++++++-------
> 2 files changed, 50 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> index dde6c22..925d6eb 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-rk3x.txt
> @@ -21,6 +21,14 @@ Required on RK3066, RK3188 :
> Optional properties :
>
> - clock-frequency : SCL frequency to use (in Hz). If omitted, 100kHz is used.
> + - i2c-scl-rising-time-ns : Number of nanoseconds the signal takes to rise
> + (t(r) in i2c spec). If not specified this is assumed to be the max the
> + spec allows(1000 ns for standard mode, 300 ns for fast mode) which
> + might cause slightly slower communication.
> + - i2c-scl-falling-time-ns : Number of nanoseconds the signal takes to fall
> + (t(f) in the i2c0 spec). If not specified this is assumed to be the
> + max the spec allows (300 ns) which might cause slightly slower
> + communication.
>
> Example:
>
> @@ -39,4 +47,7 @@ i2c0: i2c@...2d000 {
>
> clock-names = "i2c";
> clocks = <&cru PCLK_I2C0>;
> +
> + i2c-scl-rising-time-ns = <800>;
> + i2c-scl-falling-time-ns = <100>;
> };
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 0ee5802..50c1678 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -102,6 +102,8 @@ struct rk3x_i2c {
>
> /* Settings */
> unsigned int scl_frequency;
> + unsigned int rise_ns;
> + unsigned int fall_ns;
>
> /* Synchronization & notification */
> spinlock_t lock;
> @@ -435,6 +437,8 @@ out:
> *
> * @clk_rate: I2C input clock rate
> * @scl_rate: Desired SCL rate
> + * @rise_ns: How many ns it takes for signals to rise.
> + * @fall_ns: How many ns it takes for signals to fall.
> * @div_low: Divider output for low
> * @div_high: Divider output for high
> *
> @@ -443,11 +447,14 @@ out:
> * too high, we silently use the highest possible rate.
> */
> static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
> + unsigned long rise_ns, unsigned long fall_ns,
> unsigned long *div_low, unsigned long *div_high)
> {
> + unsigned long spec_min_low_ns, spec_min_high_ns;
> + unsigned long spec_max_data_hold_ns;
> + unsigned long spec_data_hold_buffer_ns;
> +
> unsigned long min_low_ns, min_high_ns;
> - unsigned long max_data_hold_ns;
> - unsigned long data_hold_buffer_ns;
> unsigned long max_low_ns, min_total_ns;
>
> unsigned long clk_rate_khz, scl_rate_khz;
> @@ -470,28 +477,30 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
>
> /*
> * min_low_ns: The minimum number of ns we need to hold low
> - * to meet i2c spec
> + * to meet i2c spec, should include fall time.
> * min_high_ns: The minimum number of ns we need to hold high
> - * to meet i2c spec
> + * to meet i2c spec, should include rise time.
> * max_low_ns: The maximum number of ns we can hold low
> - * to meet i2c spec
> + * 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.
> */
> if (scl_rate <= 100000) {
> - min_low_ns = 4700;
> - min_high_ns = 4000;
> - max_data_hold_ns = 3450;
> - data_hold_buffer_ns = 50;
> + spec_min_low_ns = 4700;
> + spec_min_high_ns = 4000;
> + spec_max_data_hold_ns = 3450;
> + spec_data_hold_buffer_ns = 50;
> } else {
> - min_low_ns = 1300;
> - min_high_ns = 600;
> - max_data_hold_ns = 900;
> - data_hold_buffer_ns = 50;
> + spec_min_low_ns = 1300;
> + spec_min_high_ns = 600;
> + spec_max_data_hold_ns = 900;
> + spec_data_hold_buffer_ns = 50;
> }
> - max_low_ns = max_data_hold_ns * 2 - data_hold_buffer_ns;
> + min_low_ns = spec_min_low_ns + fall_ns;
> + min_high_ns = spec_min_high_ns + rise_ns;
> + max_low_ns = spec_max_data_hold_ns * 2 - spec_data_hold_buffer_ns;
> min_total_ns = min_low_ns + min_high_ns;
>
> /* Adjust to avoid overflow */
> @@ -588,8 +597,8 @@ static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
> u64 t_low_ns, t_high_ns;
> int ret;
>
> - ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, &div_low,
> - &div_high);
> + ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, i2c->rise_ns,
> + i2c->fall_ns, &div_low, &div_high);
>
> WARN_ONCE(ret != 0, "Could not reach SCL freq %u", i2c->scl_frequency);
>
> @@ -633,9 +642,9 @@ static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long
> switch (event) {
> case PRE_RATE_CHANGE:
> if (rk3x_i2c_calc_divs(ndata->new_rate, i2c->scl_frequency,
> - &div_low, &div_high) != 0) {
> + i2c->rise_ns, i2c->fall_ns, &div_low,
> + &div_high) != 0)
> return NOTIFY_STOP;
> - }
>
> /* scale up */
> if (ndata->new_rate > ndata->old_rate)
> @@ -859,6 +868,18 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
> i2c->scl_frequency = DEFAULT_SCL_RATE;
> }
>
> + /* Read rise and fall ns; if not there use the default max from spec */
> + if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-rising-time-ns",
> + &i2c->rise_ns)) {
> + if (i2c->scl_frequency <= 100000)
> + i2c->rise_ns = 1000;
> + else
> + i2c->rise_ns = 300;
> + }
> + if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-falling-time-ns",
> + &i2c->fall_ns))
> + i2c->fall_ns = 300;
> +
> strlcpy(i2c->adap.name, "rk3x-i2c", sizeof(i2c->adap.name));
> i2c->adap.owner = THIS_MODULE;
> i2c->adap.algo = &rk3x_i2c_algorithm;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists