diff mbox

[v3,RFC] i2c: rk3x: handle dynamic clock rate changes correctly

Message ID 4919014.RxQIyefZLS@typ (mailing list archive)
State New, archived
Headers show

Commit Message

Max Schwarz Sept. 16, 2014, 11:13 p.m. UTC
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
(dynamic rate changes are untested!)
---

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.

Can somebody with access to a RK3066 test this or tell me how to
produce dynamic i2c clk rate changes on my RK3188?

I guess the only good way to test this is with a logic analyzer
or I2C sniffer during frequency changes.

Cheers,
  Max

 drivers/i2c/busses/i2c-rk3x.c | 134 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 126 insertions(+), 8 deletions(-)

Comments

Douglas Anderson Sept. 17, 2014, 4:26 p.m. UTC | #1
Max,

On Tue, Sep 16, 2014 at 4:13 PM, Max Schwarz <max.schwarz@online.de> wrote:
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 93cfc83..c13fabc 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -97,6 +97,8 @@ struct rk3x_i2c {
>         /* Hardware resources */
>         void __iomem *regs;
>         struct clk *clk;
> +       struct notifier_block clk_rate_nb;
> +       unsigned long clk_freq;

Is it just me, or is "clk_freq" a write-only member of this structure?
 If it is, perhaps it could be removed.

I guess if it were not write only I'd expect it would be used in
rk3x_i2c_set_scl_rate(), but it's not.


> +static void rk3x_i2c_set_scl_rate(struct rk3x_i2c *i2c, unsigned long scl_rate)
> +{
> +       unsigned long i2c_rate = clk_get_rate(i2c->clk);
> +       unsigned int div;
> +       int ret;
> +
> +       ret = rk3x_i2c_calc_div(i2c_rate, scl_rate, &div);
> +
> +       WARN_ONCE(ret != 0, "Could not reach desired SCL freq %lu", scl_rate);
> +
> +       /*
> +        * We might be called from rk3x_i2c_probe or from the clk rate change
> +        * notifier. In that case, the i2c clk is not running. So enable it
> +        * explicitly here during the register write.

It's now in _both_ cases that the i2c clk is not running, right?
...so we could just remove this comment totally?  Enabling a clock
around an i2c_write() doesn't seem incredibly noteworthy and it's now
consistent among the two callers...
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 93cfc83..c13fabc 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -97,6 +97,8 @@  struct rk3x_i2c {
 	/* Hardware resources */
 	void __iomem *regs;
 	struct clk *clk;
+	struct notifier_block clk_rate_nb;
+	unsigned long clk_freq;
 
 	/* Settings */
 	unsigned int scl_frequency;
@@ -428,18 +430,123 @@  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 desired SCL frequency
+ *
+ * @scl_rate: Desired SCL Rate
+ **/
+static void rk3x_i2c_set_scl_rate(struct rk3x_i2c *i2c, unsigned long scl_rate)
+{
+	unsigned long i2c_rate = clk_get_rate(i2c->clk);
+	unsigned int div;
+	int ret;
+
+	ret = rk3x_i2c_calc_div(i2c_rate, scl_rate, &div);
+
+	WARN_ONCE(ret != 0, "Could not reach desired SCL freq %lu", scl_rate);
+
+	/*
+	 * We might be called from rk3x_i2c_probe or from the clk rate change
+	 * notifier. In that case, the i2c clk is not running. So enable it
+	 * explicitly here during the register write.
+	 *
+	 * 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);
+
+	switch (event) {
+	case PRE_RATE_CHANGE:
+	{
+		unsigned int div;
+
+		if (rk3x_i2c_calc_div(ndata->new_rate, i2c->scl_frequency,
+				      &div) != 0) {
+			return NOTIFY_STOP;
+		}
+
+		i2c->clk_freq = ndata->new_rate;
+
+		/* scale up */
+		if (ndata->new_rate > ndata->old_rate)
+			rk3x_i2c_set_scl_rate(i2c, ndata->new_rate);
+
+		return NOTIFY_OK;
+	}
+	case POST_RATE_CHANGE:
+		i2c->clk_freq = ndata->new_rate;
+
+		/* scale down */
+		if (ndata->new_rate < ndata->old_rate)
+			rk3x_i2c_set_scl_rate(i2c, ndata->new_rate);
+		return NOTIFY_OK;
+	case ABORT_RATE_CHANGE:
+		/* scale up */
+		if (ndata->new_rate > ndata->old_rate)
+			rk3x_i2c_set_scl_rate(i2c, ndata->old_rate);
+		return NOTIFY_OK;
+	default:
+		return NOTIFY_DONE;
+	}
 }
 
 /**
@@ -536,9 +643,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;
 
 	/*
@@ -724,16 +828,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;
+	}
+
+	i2c->clk_freq = clk_get_rate(i2c->clk);
+	rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
+
 	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 +860,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;