| Message ID | 5044926.7bKhNq8BYr@typ (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Max, On Tue, Sep 23, 2014 at 1:27 AM, Max Schwarz <max.schwarz@online.de> wrote: > 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. > > Until now, we incorrectly called clk_get_rate() while holding the > i2c->lock in rk3x_i2c_xfer() to adapt to clock rate changes. > Thanks to Huang Tao for reporting this issue. > > Do it properly now using the clk notifier framework. The callback > logic was taken from i2c-cadence.c. > > Signed-off-by: Max Schwarz <max.schwarz@online.de> > Tested-by: Max Schwarz <max.schwarz@online.de> on RK3188 > Tested-by: Doug Anderson <dianders@chromium.org> on RK3288 > (dynamic rate changes are untested!) > --- > > This is based on Wolframs' i2c/for-current branch, since that > includes the recent divisor fix by Addy Ke (b4a7bd7a38). > > Doug, I'm keeping your Tested-By since the changes should not touch > the static clock case, but you might want to confirm that the > Reviewed-By still stands. No problem. I just re-tested your v5 and it still boots. You can add my Reviewed-by back though I have one nit below. > Changes since v4: > - fixed a bug in rk3x_i2c_clk_notifier_cb() which would feed > the input clock frequency as the desired SCL frequency into > rk3x_i2c_set_scl_frequency(i2c, scl_rate). Rename & change to > rk3x_i2c_adapt_div(i2c, clk_rate) to make things clear. Whew, glad you caught this! Since the notifier block wasn't used on rk3288 which I'm using, I didn't spend nearly enough time check that part of the code... > +static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long > + event, void *data) > +{ > + struct clk_notifier_data *ndata = data; > + struct rk3x_i2c *i2c = container_of(nb, struct rk3x_i2c, clk_rate_nb); > + unsigned int div; > + > + switch (event) { > + case PRE_RATE_CHANGE: > + { Now that you're not declaring any variables here, you don't need braces in this case. -Doug
Hi Doug, Am Dienstag, 23. September 2014, 08:25:11 schrieb Doug Anderson: > > This is based on Wolframs' i2c/for-current branch, since that > > includes the recent divisor fix by Addy Ke (b4a7bd7a38). > > > > Doug, I'm keeping your Tested-By since the changes should not touch > > the static clock case, but you might want to confirm that the > > Reviewed-By still stands. > > No problem. I just re-tested your v5 and it still boots. You can add > my Reviewed-by back though I have one nit below. Thanks! > > Changes since v4: > > - fixed a bug in rk3x_i2c_clk_notifier_cb() which would feed > > > > the input clock frequency as the desired SCL frequency into > > rk3x_i2c_set_scl_frequency(i2c, scl_rate). Rename & change to > > rk3x_i2c_adapt_div(i2c, clk_rate) to make things clear. > > Whew, glad you caught this! Since the notifier block wasn't used on > rk3288 which I'm using, I didn't spend nearly enough time check that > part of the code... Yeah, it just highlights we that we need some way to test this. Apart from my buggy code we also don't really know how the hw reacts to writing the divider in the middle of a transfer... I'll try to write something that forces clock rate changes even on RK3188/RK3288. > > +static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned > > long + event, void *data) > > +{ > > + struct clk_notifier_data *ndata = data; > > + struct rk3x_i2c *i2c = container_of(nb, struct rk3x_i2c, > > clk_rate_nb); + unsigned int div; > > + > > + switch (event) { > > + case PRE_RATE_CHANGE: > > + { > > Now that you're not declaring any variables here, you don't need > braces in this case. Thanks, will remove them. Cheers, Max
Hi, > Yeah, it just highlights we that we need some way to test this. Apart from > my buggy code we also don't really know how the hw reacts to writing the > divider in the middle of a transfer... I finally got around to stress-testing this patch with rate changes forced from a test driver [1]. Here are waveforms for rate transitions: Rate change from default rate to half rate: http://x-quadraht.de/fast_to_slow.png From half rate to default rate: http://x-quadraht.de/slow_to_fast.png The third and fourth line show the begin and end of the clk_set_rate() call, respectively. You can see nicely that both rate transitions result in a few slower SCL cycles, but never in faster ones. As soon as Addy's divider calculation patch is accepted, I will rebase the patch and send it again. Cheers, Max [1]: https://gist.github.com/xqms/086bec7269df64f46b14 Note: The driver requires exporting the __clk_lookup function from clk.c.
Max, On Sun, Oct 12, 2014 at 12:12 PM, Max Schwarz <max.schwarz@online.de> wrote: > As soon as Addy's divider calculation patch is accepted, I will rebase the > patch and send it again. Willing to do this now? I believe Addy's patch landed. -Doug
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c index 93cfc83..70d2dc3 100644 --- a/drivers/i2c/busses/i2c-rk3x.c +++ b/drivers/i2c/busses/i2c-rk3x.c @@ -97,6 +97,7 @@ struct rk3x_i2c { /* Hardware resources */ void __iomem *regs; struct clk *clk; + struct notifier_block clk_rate_nb; /* Settings */ unsigned int scl_frequency; @@ -428,18 +429,113 @@ out: return IRQ_HANDLED; } -static void rk3x_i2c_set_scl_rate(struct rk3x_i2c *i2c, unsigned long scl_rate) +/** + * Calculate divider value for desired SCL frequency + * + * @clk_rate: I2C input clock rate + * @scl_rate: Desired SCL rate + * @div: Divider output + * + * Return: 0 on success, -EINVAL on unreachable SCL rate. In that case + * a best-effort divider value is returned in div. + **/ +static int rk3x_i2c_calc_div(unsigned long clk_rate, unsigned long scl_rate, + unsigned int *div) { - unsigned long i2c_rate = clk_get_rate(i2c->clk); - unsigned int div; + unsigned long div_tmp; /* set DIV = DIVH = DIVL * SCL rate = (clk rate) / (8 * (DIVH + 1 + DIVL + 1)) * = (clk rate) / (16 * (DIV + 1)) */ - div = DIV_ROUND_UP(i2c_rate, scl_rate * 16) - 1; + div_tmp = DIV_ROUND_UP(clk_rate, scl_rate * 16) - 1; + + if (div_tmp > 0xFFFF) { + /* The input clock is too fast. Reject this rate change. */ + *div = 0xFFFF; + return -EINVAL; + } + + *div = div_tmp; + + return 0; +} + +/** + * Setup divider register for (changed) input clk rate + * + * @clk_rate: Input clock rate + **/ +static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate) +{ + unsigned int div; + int ret; + + ret = rk3x_i2c_calc_div(clk_rate, i2c->scl_frequency, &div); + + WARN_ONCE(ret != 0, "Could not reach SCL freq %u", i2c->scl_frequency); + + /* + * Writing the register with halted clock crashes the system at least on + * RK3288. + */ + clk_enable(i2c->clk); i2c_writel(i2c, (div << 16) | (div & 0xffff), REG_CLKDIV); + clk_disable(i2c->clk); +} + +/** + * rk3x_i2c_clk_notifier_cb - Clock rate change callback + * @nb: Pointer to notifier block + * @event: Notification reason + * @data: Pointer to notification data object + * + * The callback checks whether a valid bus frequency can be generated after the + * change. If so, the change is acknowledged, otherwise the change is aborted. + * New dividers are written to the HW in the pre- or post change notification + * depending on the scaling direction. + * + * Code adapted from i2c-cadence.c. + * + * Return: NOTIFY_STOP if the rate change should be aborted, NOTIFY_OK + * to acknowedge the change, NOTIFY_DONE if the notification is + * considered irrelevant. + */ +static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long + event, void *data) +{ + struct clk_notifier_data *ndata = data; + struct rk3x_i2c *i2c = container_of(nb, struct rk3x_i2c, clk_rate_nb); + unsigned int div; + + switch (event) { + case PRE_RATE_CHANGE: + { + if (rk3x_i2c_calc_div(ndata->new_rate, i2c->scl_frequency, + &div) != 0) { + return NOTIFY_STOP; + } + + /* scale up */ + if (ndata->new_rate > ndata->old_rate) + rk3x_i2c_adapt_div(i2c, ndata->new_rate); + + return NOTIFY_OK; + } + case POST_RATE_CHANGE: + /* scale down */ + if (ndata->new_rate < ndata->old_rate) + rk3x_i2c_adapt_div(i2c, ndata->new_rate); + return NOTIFY_OK; + case ABORT_RATE_CHANGE: + /* scale up */ + if (ndata->new_rate > ndata->old_rate) + rk3x_i2c_adapt_div(i2c, ndata->old_rate); + return NOTIFY_OK; + default: + return NOTIFY_DONE; + } } /** @@ -536,9 +632,6 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap, clk_enable(i2c->clk); - /* The clock rate might have changed, so setup the divider again */ - rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency); - i2c->is_last_msg = false; /* @@ -624,6 +717,7 @@ static int rk3x_i2c_probe(struct platform_device *pdev) int bus_nr; u32 value; int irq; + unsigned long clk_rate; i2c = devm_kzalloc(&pdev->dev, sizeof(struct rk3x_i2c), GFP_KERNEL); if (!i2c) @@ -724,16 +818,28 @@ static int rk3x_i2c_probe(struct platform_device *pdev) return ret; } + i2c->clk_rate_nb.notifier_call = rk3x_i2c_clk_notifier_cb; + ret = clk_notifier_register(i2c->clk, &i2c->clk_rate_nb); + if (ret != 0) { + dev_err(&pdev->dev, "Unable to register clock notifier\n"); + goto err_clk; + } + + clk_rate = clk_get_rate(i2c->clk); + rk3x_i2c_adapt_div(i2c, clk_rate); + ret = i2c_add_adapter(&i2c->adap); if (ret < 0) { dev_err(&pdev->dev, "Could not register adapter\n"); - goto err_clk; + goto err_clk_notifier; } dev_info(&pdev->dev, "Initialized RK3xxx I2C bus at %p\n", i2c->regs); return 0; +err_clk_notifier: + clk_notifier_unregister(i2c->clk, &i2c->clk_rate_nb); err_clk: clk_unprepare(i2c->clk); return ret; @@ -744,6 +850,8 @@ static int rk3x_i2c_remove(struct platform_device *pdev) struct rk3x_i2c *i2c = platform_get_drvdata(pdev); i2c_del_adapter(&i2c->adap); + + clk_notifier_unregister(i2c->clk, &i2c->clk_rate_nb); clk_unprepare(i2c->clk); return 0;