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 at online.de>
Tested-by: Max Schwarz <max.schwarz at online.de> on RK3188
Tested-by: Doug Anderson <dianders at 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.
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.
- trimmed comment in rk3x_i2c_adapt_div() (sugg. by Doug Anderson)
Changes since v3:
- drop leftover write-only clk_freq variable (sugg. by Doug Anderson)
Changes since v2:
- allow rate changes which result in lower than
desired SCL frequencies (sugg. by Doug Anderson)
- simplified divider range checks (Doug Anderson)
- removed duplicate clk_enable()/disable() (Doug Anderson)
- added missing unregister in rk3x_i2c_remove() (Doug Anderson)
Changes since v1:
- make sure the i2c input clock is active during prescaler
register write by explicitly enabling/disabling it in
rk3x_i2c_set_scl_frequency(). Bug found by Addy Ke.
It would still be awesome if someone with an RK3066 could test this.
Heiko suggested using a script by Doug for stress-testing frequency
changes:
cd /sys/devices/system/cpu/cpu0/cpufreq
echo userspace > scaling_governor
unset FREQS
read -a FREQS < scaling_available_frequencies
RANDOM=$$$(date +%s)
while true; do
FREQ=${FREQS[$RANDOM % ${#FREQS[@]} ]}
echo Now ${FREQ}
echo ${FREQ} > scaling_setspeed
done
If you run some I2C transactions at the same time (e.g. using
i2cget) that would be enough to confirm that everything still
works.
I'll also try to write a test driver for RK3188/RK3288 which can
force i2c input clock rate changes.
Cheers,
Max
drivers/i2c/busses/i2c-rk3x.c | 124 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 116 insertions(+), 8 deletions(-)
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;
--
1.9.1
