[v5,RFC] i2c: rk3x: handle dynamic clock rate changes correctly
diff mbox

Message ID 5044926.7bKhNq8BYr@typ
State New, archived
Headers show

Commit Message

Max Schwarz Sept. 23, 2014, 8:27 a.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
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.

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(-)

Comments

Doug Anderson Sept. 23, 2014, 3:25 p.m. UTC | #1
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
Max Schwarz Sept. 23, 2014, 3:32 p.m. UTC | #2
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
Max Schwarz Oct. 12, 2014, 7:12 p.m. UTC | #3
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.
Doug Anderson Nov. 12, 2014, 5:48 p.m. UTC | #4
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

Patch
diff mbox

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;