Re: [PATCH v7 8/9] i2c: rk3x: add i2c support for rk3399 soc
From: David.Wu
Date: Fri May 06 2016 - 05:32:30 EST
Hi Doug,
å 2016/5/6 7:00, Doug Anderson åé:
David,
On Wed, May 4, 2016 at 7:36 AM, David Wu <david.wu@xxxxxxxxxxxxxx> wrote:
+/**
+ * Calculate timing values for desired SCL frequency
+ *
+ * @clk_rate: I2C input clock rate
+ * @t: Known I2C timing information
+ * @t_calc: Caculated rk3x private timings that would be written into regs
+
+ * 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.
+ * The following formulas are v1's method to calculate timings.
+ *
+ * l = divl + 1;
+ * h = divh + 1;
+ * s = sda_update_config + 1;
+ * u = start_setup_config + 1;
+ * p = stop_setup_config + 1;
+ * T = Tclk_i2c;
+
+ * tHigh = 8 * h * T;
+ * tLow = 8 * l * T;
+
+ * tHD;sda = (l * s + 1) * T;
+ * tSU;sda = [(8 - s) * l + 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_timings(unsigned long clk_rate,
+ struct i2c_timings *t,
+ struct rk3x_i2c_calced_timings *t_calc)
+{
I don't think I'm going to try to understand all the math here. I'll
trust you that this does something sane.
+ /* Final divh and divl must be greater than 0, otherwise the
+ * hardware would not output the i2c clk.
+ */
nit: multiline comment style doesn't match rest of file.
static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
{
struct i2c_timings *t = &i2c->t;
- struct rk3x_i2c_calced_timings calc;
+ struct rk3x_i2c_calced_timings *calc = &i2c->t_calc;
u64 t_low_ns, t_high_ns;
int ret;
- ret = rk3x_i2c_calc_divs(clk_rate, t, &calc);
+ ret = i2c->soc_data->calc_timings(clk_rate, t, calc);
WARN_ONCE(ret != 0, "Could not reach SCL freq %u", t->bus_freq_hz);
- clk_enable(i2c->clk);
- i2c_writel(i2c, (calc.div_high << 16) | (calc.div_low & 0xffff),
+ if (i2c->pclk)
+ clk_enable(i2c->pclk);
+ else
+ clk_enable(i2c->clk);
+ i2c_writel(i2c, (calc->div_high << 16) | (calc->div_low & 0xffff),
REG_CLKDIV);
There is a subtle bug here, though it likely doesn't manifest in any
current hardware configurations.
Specifically if you get a clock change on a device with a "v1"
controller while an i2c transaction is happening then you will likely
get i2c errors.
The clock change notifications work like this:
* Before the clock change, adjust the timings based on the faster of
the old/new clock.
* Let the clock change happen.
* If we didn't adjust the timings before, adjust them now.
With the logic above there will be a period where the i2c transaction
is happening slower than ideal, but that should be OK. ...and you can
imagine the speed of the transaction changing midway through the
transaction--even midway through a single byte.
With v1 some of the timing information is _not_updated by
rk3x_i2c_adapt_div()--it's only set at the start of a transaction.
That breaks all the above assumptions.
So you should probably be updating the the RKI2C_CON register here by
doing a read-modify-write, like:
ctrl = i2c_readl(i2c, REG_CON);
ctrl &= ~REG_CON_TUNING_MASK;
ctrl |= i2c->t_calc.tuning;
i2c_writel(i2c, ctrl, REG_CON);
Yeap, it seems it is a bug when a clock changes, but not update the
regs, it might make transfer failed. It was not enough to just store
tuning value.
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"?
@@ -728,11 +910,11 @@ 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);
- struct rk3x_i2c_calced_timings calc;
switch (event) {
case PRE_RATE_CHANGE:
- if (rk3x_i2c_calc_divs(ndata->new_rate, &i2c->t, &calc) != 0)
+ if (i2c->soc_data->calc_timings(ndata->new_rate, &i2c->t,
+ &i2c->t_calc) != 0)
This change is incorrect. Please change it back to being calculated
in a local variable. Optionally also add a comment that says:
/*
* Try the calculation (but don't store the result) ahead of
* time to see if we need to block the clock change. Timings
* shouldn't actually take effect until rk3x_i2c_adapt_div().
*/
Specifically in the case that we return NOTIFY_STOP here we _don't_
want to have modified our internal timings. We also _don't_ want to
have modified our internal timings in the case that the old_rate > the
new_rate.
Okay, use &i2c->t_calc is an error here, timings shouldn't actually take
effect until rk3x_i2c_adapt_div().
BTW: Did you manage to find anyone using an old Rockchip SoC that can
test your patches?
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.
@@ -1042,17 +1236,38 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, i2c);
+ i2c->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(i2c->clk)) {
+ dev_err(&pdev->dev, "cannot get clock\n");
+ return PTR_ERR(i2c->clk);
+ }
+
ret = clk_prepare(i2c->clk);
if (ret < 0) {
dev_err(&pdev->dev, "Could not prepare clock\n");
return ret;
}
+ if (i2c->soc_data->calc_timings == rk3x_i2c_v1_calc_timings) {
+ i2c->pclk = devm_clk_get(&pdev->dev, "pclk");
+ if (IS_ERR(i2c->pclk)) {
+ dev_err(i2c->dev, "Could not get i2c pclk\n");
+ ret = PTR_ERR(i2c->pclk);
+ goto err_clk;
+ }
+
+ ret = clk_prepare(i2c->pclk);
+ if (ret) {
+ dev_err(i2c->dev, "Could not prepare pclk\n");
+ goto err_clk;
+ }
+ }
This is not matching the bindings. You are still assuming that "i2c"
clock is the first clock and "pclk" is the one named "pclk". Said
another way, if you had the following in your device tree:
clocks = <&pmucru PCLK_I2C0_PMU>, <&pmucru SCLK_I2C0_PMU>;
clock-names = "pclk", "i2c";
...you'll find that you'll get back "pclk" twice. The first time
you'll get it because you asked for the first clock listed, the second
time because you asked for the clock named "pclk".
I'd also say that your life will probably be easier if you always
setup both "clk" and "pclk", even on old CPUs. It's OK to call
"clk_prepare" twice and OK to call "clk_enable" twice.
Thus, I'd probably write all the code as this (untested):
if (i2c->soc_data->calc_timings == rk3x_i2c_v0_calc_timings) {
/* Only one clock to use for bus clock and peripheral clock */
i2c->clk = devm_clk_get(&pdev->dev, NULL);
i2c->pclk = i2c->clk;
} else {
i2c->clk = devm_clk_get(&pdev->dev, "i2c");
i2c->pclk = devm_clk_get(&pdev->dev, "pclk");
}
if (IS_ERR(i2c->clk)) {
ret = PTR_ERR(i2c->clk);
if (ret != -EPROBE_DEFER)
dev_err(&pdev->dev, "Can't get bus clk: %d\n", ret);
return ret;
}
if (IS_ERR(i2c->pclk)) {
ret = PTR_ERR(i2c->pclk);
if (ret != -EPROBE_DEFER)
dev_err(&pdev->dev, "Can't get periph clk: %d\n", ret);
return ret;
}
ret = clk_prepare(i2c->clk);
if (ret < 0) {
dev_err(&pdev->dev, "Can't prepare bus clk: %d\n", ret);
return ret;
}
ret = clk_prepare(i2c->pclk);
if (ret < 0) {
dev_err(&pdev->dev, "Can't prepare periph clock: %d\n", ret);
goto err_clk;
}
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.
-Doug