| 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: <2556360.42nCqUelUN@diego>
Date: Thu, 10 Dec 2015 12:03:28 +0100
From: Heiko Stübner <heiko@...ech.de>
To: David Wu <wdc@...k-chips.com>
Cc: wsa@...-dreams.de, huangtao@...k-chips.com, hl@...k-chips.com,
xjq@...k-chips.com, linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, linux-i2c@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] i2c: rk3x: Add two new features for rk3399
Hi David,
Am Mittwoch, 9. Dezember 2015, 17:11:46 schrieb David Wu:
> 1. support highspeed.
> 2. check i2c bus idle status.
Listing two separate changes in one patch is a big indicator that it should be
split up into two patches. Also please be more verbose (aka explain more) what
patches do - and especially why it's needed.
>From what I've seen below, my personal favorite would probably be:
patch1: introduce ops struct and move the current calc_divs to it
patch2: introduce v1, highspeed with that new calc_divs
patch3: introduce the idle status-check
> Change-Id: I9c22e752af621c0f8dbcbd399c86b34fd810ec38
no change-ids etc from external revision control systems please
> Signed-off-by: David Wu <wdc@...k-chips.com>
> ---
> drivers/i2c/busses/i2c-rk3x.c | 336
> ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 320
> insertions(+), 16 deletions(-)
> mode change 100644 => 100755 drivers/i2c/busses/i2c-rk3x.c
>
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> old mode 100644
> new mode 100755
> index c1935eb..b1aa702
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -25,6 +25,7 @@
> #include <linux/mfd/syscon.h>
> #include <linux/regmap.h>
> #include <linux/math64.h>
> +#include <linux/delay.h>
>
>
> /* Register Map */
> @@ -37,6 +38,7 @@
> #define REG_IEN 0x18 /* interrupt enable */
> #define REG_IPD 0x1c /* interrupt pending */
> #define REG_FCNT 0x20 /* finished count */
> +#define I2C_ST 0x220 /* i2c pin status */
while the registers are called I2C_* in the TRMs, please keep with the current
notation in the driver ... so REG_ST here
>
> /* Data buffer offsets */
> #define TXBUFFER_BASE 0x100
> @@ -58,6 +60,12 @@ enum {
> #define REG_CON_LASTACK BIT(5) /* 1: send NACK after last received byte
> */ #define REG_CON_ACTACK BIT(6) /* 1: stop if NACK is received */
>
> +#define VERSION_MASK 0xffff0000
> +#define VERSION_SHIFT 16
> +
> +#define RK3X_I2C_V0 0x0
> +#define RK3X_I2C_V1 0x1
> +
> /* REG_MRXADDR bits */
> #define REG_MRXADDR_VALID(x) BIT(24 + (x)) /* [x*8+7:x*8] of MRX[R]ADDR
> valid */
>
> @@ -71,6 +79,13 @@ enum {
> #define REG_INT_NAKRCV BIT(6) /* NACK received */
> #define REG_INT_ALL 0x7f
>
> +enum {
> + I2C_IDLE = 0,
> + I2C_SDA_LOW,
> + I2C_SCL_LOW,
> + BOTH_LOW,
> +};
> +
> /* Constants */
> #define WAIT_TIMEOUT 1000 /* ms */
> #define DEFAULT_SCL_RATE (100 * 1000) /* Hz */
> @@ -90,10 +105,23 @@ struct rk3x_i2c_soc_data {
> int grf_offset;
> };
>
> +struct rk3x_i2c_ops {
> + int (*check_idle)(void __iomem *);
> + int (*calc_divs)(unsigned long,
> + unsigned long,
> + unsigned long,
> + unsigned long,
> + unsigned long,
> + unsigned long *,
> + unsigned long *,
> + unsigned int *);
> +};
> +
> struct rk3x_i2c {
> struct i2c_adapter adap;
> struct device *dev;
> struct rk3x_i2c_soc_data *soc_data;
> + struct rk3x_i2c_ops ops;
>
> /* Hardware resources */
> void __iomem *regs;
> @@ -116,6 +144,7 @@ struct rk3x_i2c {
> u8 addr;
> unsigned int mode;
> bool is_last_msg;
> + unsigned int time_con;
>
> /* I2C state machine */
> enum rk3x_i2c_state state;
> @@ -151,7 +180,8 @@ static void rk3x_i2c_start(struct rk3x_i2c *i2c)
> i2c_writel(i2c, REG_INT_START, REG_IEN);
>
> /* enable adapter with correct mode, send START condition */
> - val = REG_CON_EN | REG_CON_MOD(i2c->mode) | REG_CON_START;
> + val = i2c->time_con | REG_CON_EN | REG_CON_MOD(i2c->mode)
> + | REG_CON_START;
>
> /* if we want to react to NACK, set ACTACK bit */
> if (!(i2c->msg->flags & I2C_M_IGNORE_NAK))
> @@ -443,16 +473,19 @@ out:
> * @sda_fall_ns: How many ns it takes for SDA to fall.
> * @div_low: Divider output for low
> * @div_high: Divider output for high
> + * @con: version0 is not used
> *
> * Returns: 0 on success, -EINVAL if the goal SCL rate is too slow. In that
> case * a best-effort divider value is returned in divs. If the target rate
> is * 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 scl_rise_ns,
> - unsigned long scl_fall_ns,
> - unsigned long sda_fall_ns,
> - unsigned long *div_low, unsigned long *div_high)
> +static int rk3x_i2c_v0_calc_divs(unsigned long clk_rate, unsigned long
> scl_rate, + unsigned long scl_rise_ns,
> + unsigned long scl_fall_ns,
> + unsigned long sda_fall_ns,
> + unsigned long *div_low,
> + unsigned long *div_high,
> + unsigned int *con)
> {
> unsigned long spec_min_low_ns, spec_min_high_ns;
> unsigned long spec_setup_start, spec_max_data_hold_ns;
> @@ -614,19 +647,244 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate,
> unsigned long scl_rate, return ret;
> }
>
> +/**
> + * Calculate divider values for desired SCL frequency
> + *
> + * @clk_rate: I2C input clock rate
> + * @scl_rate: Desired SCL rate
> + * @scl_rise_ns: How many ns it takes for SCL to rise.
> + * @scl_fall_ns: How many ns it takes for SCL to fall.
> + * @sda_fall_ns: How many ns it takes for SDA to fall.
> + * @div_low: Divider output for low
> + * @div_high: Divider output for high
> + * @con: SDA update point config used to adjust setup/hold time,
> + * start setup config for setup_start and hold_start time,
> + * stop_setup config for setup_stop time.
> + *
> + * Returns: 0 on success, -EINVAL if the goal SCL rate is too slow. In that
> case + * a best-effort divider value is returned in divs. If the target
> rate is + * too high, we silently use the highest possible rate.
> +
> + * l = divl + 1;
> + * h = divh + 1;
> + * s = data_upd_st + 1;
> + * u = start_setup_cnt + 1;
> + * p = stop_setup_cnt + 1;
> + * T:Tclk_i2c
> +
> + * tHigh = 8 * h * T;
> + * tLow = 8 * l * T;
> +
> + * tHD;sda = (l * s + 1) * T;
> + * tSU;sda = ((8 - l) * s + 1) * T;
> + * tI2C = 8 * (l + h) * T;
> +
> + * tSU;sta = (8h * u + 1) * T;
> + * tHD;sta = [8h * (u + 1) - 1]* T;
> + * tSU;sto =(8h * p + 1) * T;
> + */
> +static int rk3x_i2c_v1_calc_divs(unsigned long clk_rate, unsigned long
> scl_rate, + unsigned long scl_rise_ns,
> + unsigned long scl_fall_ns,
> + unsigned long sda_fall_ns,
> + unsigned long *div_low,
> + unsigned long *div_high,
> + unsigned int *con)
> +{
> + unsigned long spec_min_low_ns, spec_min_high_ns;
> + unsigned long spec_min_setup_start, spec_min_hold_start;
> + unsigned long spec_min_data_setup, spec_max_data_hold_ns;
> + unsigned long spec_min_stop_setup;
> +
> + unsigned long min_low_ns, min_high_ns, min_total_ns;
> + unsigned long min_setup_start_ns, min_hold_start_ns;
> + unsigned long min_stop_setup_ns, max_hold_data_ns, min_setup_data_ns;
> +
> + unsigned long clk_rate_khz, scl_rate_khz;
> +
> + unsigned long min_low_div, min_high_div;
> +
> + unsigned long min_div_for_hold, min_total_div;
> + unsigned long extra_div, extra_low_div;
> + unsigned long start_setup_cnt, stop_setup_cnt, data_upd_st;
> +
> + int ret = 0;
> +
> + if (WARN_ON(scl_rate > 3400000))
> + scl_rate = 3400000;
> +
> + if (WARN_ON(scl_rate < 100000))
> + scl_rate = 100000;
> +
> + if (scl_rate <= 100000) {
> + spec_min_low_ns = 4700;
> + spec_min_high_ns = 4000;
> +
> + spec_min_setup_start = 4700;
> + spec_min_hold_start = 4000;
> +
> + spec_max_data_hold_ns = 3450;
> + spec_min_data_setup = 250;
> + spec_min_stop_setup = 4000;
> +
> + start_setup_cnt = 0;
> + stop_setup_cnt = 0;
> + } else if (scl_rate <= 400000) {
> + spec_min_setup_start = 600;
> + spec_min_hold_start = 600;
> +
> + spec_min_low_ns = 1300;
> + spec_min_high_ns = 600;
> +
> + spec_min_data_setup = 100;
> + spec_max_data_hold_ns = 900;
> + spec_min_stop_setup = 600;
> +
> + start_setup_cnt = 0;
> + stop_setup_cnt = 0;
> + } else if (scl_rate <= 1700000) {
> + spec_min_low_ns = 320;
> + spec_min_high_ns = 120;
> +
> + spec_min_setup_start = 160;
> + spec_min_hold_start = 160;
> +
> + spec_max_data_hold_ns = 150;
> + spec_min_data_setup = 10;
> + spec_min_stop_setup = 160;
> +
> + start_setup_cnt = 1;
> + stop_setup_cnt = 1;
> + } else {
> + spec_min_low_ns = 160;
> + spec_min_high_ns = 60;
> +
> + spec_min_setup_start = 160;
> + spec_min_hold_start = 160;
> +
> + spec_min_data_setup = 10;
> + spec_max_data_hold_ns = 70;
> + spec_min_stop_setup = 160;
> +
> + start_setup_cnt = 2;
> + stop_setup_cnt = 2;
> + }
> +
> + clk_rate_khz = DIV_ROUND_UP(clk_rate, 1000);
> + scl_rate_khz = scl_rate / 1000;
> + min_total_div = DIV_ROUND_UP(clk_rate_khz, scl_rate_khz * 8);
> +
> + /*tHigh = 8 * h *T;*/
> + min_high_ns = scl_rise_ns + spec_min_high_ns;
> + min_high_div = DIV_ROUND_UP(clk_rate_khz * min_high_ns, 8 * 1000000);
> +
> + /*tSU;sta = (u*8*h + 4)*T + T;*/
> + min_setup_start_ns = scl_rise_ns + spec_min_setup_start;
> + min_high_div = max(min_high_div,
> + DIV_ROUND_UP(clk_rate_khz * min_setup_start_ns
> + - 1000000, 8 * 1000000 * (1 + start_setup_cnt)));
> +
> + /*tHD;sta = (u + 1) * 8h * T - T;*/
> + min_hold_start_ns = scl_rise_ns + spec_min_hold_start;
> + min_high_div = max(min_high_div,
> + DIV_ROUND_UP(clk_rate_khz * min_hold_start_ns
> + + 1000000, 8 * 1000000 * (2 + start_setup_cnt)));
> +
> + /*tSU;sto = (p*8*h + 4)*T + T;*/
> + min_stop_setup_ns = scl_rise_ns + spec_min_stop_setup;
> + min_high_div = max(min_high_div,
> + DIV_ROUND_UP(clk_rate_khz * min_stop_setup_ns
> + - 1000000, 8 * 1000000 * (1 + stop_setup_cnt)));
> +
> + min_low_ns = scl_fall_ns + spec_min_low_ns;
> +
> + /* These are the min dividers needed for min hold times. */
> + min_low_div = DIV_ROUND_UP(clk_rate_khz * min_low_ns, 8 * 1000000);
> +
> + min_div_for_hold = (min_low_div + min_high_div);
> + min_total_ns = min_low_ns + min_high_ns;
> +
> + /*
> + * This is the maximum divider so we don't go over the maximum.
> + * We don't round up here (we round down) since this is a maximum.
> + */
> + 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;
> + extra_low_div = DIV_ROUND_UP(min_low_div * extra_div,
> + min_div_for_hold);
> +
> + *div_low = min_low_div + extra_low_div;
> + *div_high = min_high_div + (extra_div - extra_low_div);
> + }
> +
> + /*
> + * tHD;sda = (l * s + 1) * T;
> + * tSU;sda = ((8 - l) * s + 1) * T;
> + */
> + for (data_upd_st = 2; data_upd_st >= 0; data_upd_st--) {
> + max_hold_data_ns = DIV_ROUND_UP(((data_upd_st + 1)
> + * (*div_low) + 1) * 1000000,
> + clk_rate_khz);
> + min_setup_data_ns = DIV_ROUND_UP(((9 - data_upd_st)
> + * (*div_low) + 1) * 1000000,
> + clk_rate_khz);
> + if ((max_hold_data_ns < spec_max_data_hold_ns) &&
> + (min_setup_data_ns > spec_min_data_setup))
> + break;
> + }
> +
> + /*
> + * 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 = *div_low - 1;
> + *div_high = *div_high - 1;
> +
> + /* Maximum divider supported by hw is 0xffff */
> + if (*div_low > 0xffff) {
> + *div_low = 0xffff;
> + ret = -EINVAL;
> + }
> +
> + if (*div_high > 0xffff) {
> + *div_high = 0xffff;
> + ret = -EINVAL;
> + }
> +
> + *con = *con & 0x00ff;
> + *con |= data_upd_st << 8;
> + *con |= start_setup_cnt << 12;
> + *con |= stop_setup_cnt << 14;
> +
> + return ret;
> +}
> +
> static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long
> clk_rate) {
> + unsigned int con = 0;
> unsigned long div_low, div_high;
> u64 t_low_ns, t_high_ns;
> int ret;
>
> - ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, i2c->scl_rise_ns,
> + ret = i2c->ops.calc_divs(clk_rate, i2c->scl_frequency, i2c->scl_rise_ns,
> i2c->scl_fall_ns, i2c->sda_fall_ns,
> - &div_low, &div_high);
> + &div_low, &div_high, &con);
> WARN_ONCE(ret != 0, "Could not reach SCL freq %u", i2c->scl_frequency);
>
> clk_enable(i2c->clk);
> i2c_writel(i2c, (div_high << 16) | (div_low & 0xffff), REG_CLKDIV);
> + i2c->time_con = con;
> clk_disable(i2c->clk);
>
> t_low_ns = div_u64(((u64)div_low + 1) * 8 * 1000000000, clk_rate);
> @@ -661,13 +919,14 @@ static int rk3x_i2c_clk_notifier_cb(struct
> notifier_block *nb, unsigned long struct clk_notifier_data *ndata = data;
> struct rk3x_i2c *i2c = container_of(nb, struct rk3x_i2c, clk_rate_nb);
> unsigned long div_low, div_high;
> + unsigned int con = 0;
>
> switch (event) {
> case PRE_RATE_CHANGE:
> - if (rk3x_i2c_calc_divs(ndata->new_rate, i2c->scl_frequency,
> + if (i2c->ops.calc_divs(ndata->new_rate, i2c->scl_frequency,
> i2c->scl_rise_ns, i2c->scl_fall_ns,
> i2c->sda_fall_ns,
> - &div_low, &div_high) != 0)
> + &div_low, &div_high, &con) != 0)
> return NOTIFY_STOP;
>
> /* scale up */
> @@ -690,6 +949,11 @@ static int rk3x_i2c_clk_notifier_cb(struct
> notifier_block *nb, unsigned long }
> }
>
> +static int rockchip_i2c_v1_check_idle(void __iomem *regs)
> +{
> + return readl(regs + I2C_ST) & 0x03;
> +}
> +
> /**
> * Setup I2C registers for an I2C operation specified by msgs, num.
> *
> @@ -777,6 +1041,7 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
> {
> struct rk3x_i2c *i2c = (struct rk3x_i2c *)adap->algo_data;
> unsigned long timeout, flags;
> + int state, retry = 10;
> int ret = 0;
> int i;
>
> @@ -784,6 +1049,21 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
>
> clk_enable(i2c->clk);
>
> + if (i2c->ops.check_idle) {
> + while (retry) {
> + state = i2c->ops.check_idle(i2c);
> + if (state == I2C_IDLE)
> + break;
> + mdelay(10);
> + retry--;
> + }
> + if (retry == 0) {
> + dev_err(i2c->dev, "i2c is not in idle(state = %d)\n",
> + state);
> + return -EIO;
> + }
> + }
> +
> i2c->is_last_msg = false;
>
> /*
> @@ -816,7 +1096,8 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
>
> /* Force a STOP condition without interrupt */
> i2c_writel(i2c, 0, REG_IEN);
> - i2c_writel(i2c, REG_CON_EN | REG_CON_STOP, REG_CON);
> + i2c_writel(i2c, i2c->time_con | REG_CON_EN |
> + REG_CON_STOP, REG_CON);
>
> i2c->state = STATE_IDLE;
>
> @@ -871,6 +1152,7 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
> u32 value;
> int irq;
> unsigned long clk_rate;
> + unsigned int version;
>
> i2c = devm_kzalloc(&pdev->dev, sizeof(struct rk3x_i2c), GFP_KERNEL);
> if (!i2c)
shouldn't you do something to the scl_frequency here too? If I'm not blind,
even with your patch, the code will limit sclk_frequency to 400kHZ?
So I guess you should your version check+ops-assignment here instead and also
cap the scl_frequency accordingly if necessary.
> @@ -901,15 +1183,29 @@ static int rk3x_i2c_probe(struct platform_device
> *pdev) &i2c->scl_rise_ns)) {
> if (i2c->scl_frequency <= 100000)
> i2c->scl_rise_ns = 1000;
> - else
> + else if (i2c->scl_frequency <= 400000)
> i2c->scl_rise_ns = 300;
> + else if (i2c->scl_frequency <= 1700000)
> + i2c->scl_rise_ns = 80;
> + else
> + i2c->scl_rise_ns = 40;
> }
> if (of_property_read_u32(pdev->dev.of_node, "i2c-scl-falling-time-ns",
> - &i2c->scl_fall_ns))
> - i2c->scl_fall_ns = 300;
> + &i2c->scl_fall_ns)) {
> + if (i2c->scl_frequency <= 400000)
> + i2c->scl_fall_ns = 300;
> + else if (i2c->scl_frequency <= 1700000)
> + i2c->scl_fall_ns = 80;
> + else
> + i2c->scl_fall_ns = 40;
> + }
> if (of_property_read_u32(pdev->dev.of_node, "i2c-sda-falling-time-ns",
> - &i2c->scl_fall_ns))
> - i2c->sda_fall_ns = i2c->scl_fall_ns;
> + &i2c->scl_fall_ns)) {
> + if (i2c->scl_frequency <= 400000)
> + i2c->sda_fall_ns = i2c->scl_fall_ns;
> + else
> + i2c->sda_fall_ns = 2 * i2c->scl_fall_ns;
> + }
>
> strlcpy(i2c->adap.name, "rk3x-i2c", sizeof(i2c->adap.name));
> i2c->adap.owner = THIS_MODULE;
Thanks
Heiko
--
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