diff mbox

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

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

Commit Message

Max Schwarz Sept. 8, 2014, 10:56 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!)
---

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 | 120 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 112 insertions(+), 8 deletions(-)

Comments

Max Schwarz Sept. 13, 2014, 11:26 a.m. UTC | #1
Hello Addy,

On Wednesday 10 September 2014 at 16:16:10, Addy wrote:
> > @@ -724,16 +816,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);
> 
> I have tested on rk3288-pinky board:
> I2c clock must be enabled before driver call rk3x_i2c_set_scl_rate()
> to write div value to CLKDIV register. If not, system will crash.
> 
> So we need call clk_enable() before rk3x_i2c_set_scl_rate().

Interesting. It works without problems on RK3188. Do you know if the clock has 
to be kept enabled for some time? Or is it okay to do it like this?

clk_enable(i2c->clk);
rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
clk_disable(i2c->clk);

Cheers,
  Max
addy ke Sept. 15, 2014, 3:11 a.m. UTC | #2
Hi Max

On 2014/9/13 19:26, Max Schwarz wrote:
> Hello Addy,
> 
> On Wednesday 10 September 2014 at 16:16:10, Addy wrote:
>>> @@ -724,16 +816,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);
>>
>> I have tested on rk3288-pinky board:
>> I2c clock must be enabled before driver call rk3x_i2c_set_scl_rate()
>> to write div value to CLKDIV register. If not, system will crash.
>>
>> So we need call clk_enable() before rk3x_i2c_set_scl_rate().
> 
> Interesting. It works without problems on RK3188. Do you know if the clock has 
> to be kept enabled for some time? Or is it okay to do it like this?
> 
> clk_enable(i2c->clk);
> rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
> clk_disable(i2c->clk);
>
> Cheers,
>   Max
>
Before write value to i2c register, it is necessary to enable i2c's clock and parent clock!

In the linux-rockchip branch code, the i2c's clock and parent clock are enabled by default.
So there are no problems on it.

But on rk388-pinky board, pclk_peri is disabled when i2c driver is probed.
(Maybe its reference count drops to 0 and it is disabled by clk driver)

Hi, Doug
Do you known why pclk_peri is disabled when i2c driver is probed?
My test branch is cros/chromeos-3.14.


> 
>
Max Schwarz Sept. 15, 2014, 8:29 a.m. UTC | #3
Hi Addy,

Am Montag, 15. September 2014, 11:11:24 schrieb addy ke:
> On 2014/9/13 19:26, Max Schwarz wrote:
> > On Wednesday 10 September 2014 at 16:16:10, Addy wrote:
> >>> @@ -724,16 +816,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);
> >> 
> >> I have tested on rk3288-pinky board:
> >> I2c clock must be enabled before driver call rk3x_i2c_set_scl_rate()
> >> to write div value to CLKDIV register. If not, system will crash.
> >> 
> >> So we need call clk_enable() before rk3x_i2c_set_scl_rate().
> > 
> > Interesting. It works without problems on RK3188. Do you know if the clock
> > has to be kept enabled for some time? Or is it okay to do it like this?
> > 
> > clk_enable(i2c->clk);
> > rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
> > clk_disable(i2c->clk);
> > 
> > Cheers,
> > 
> >   Max
> 
> Before write value to i2c register, it is necessary to enable i2c's clock
> and parent clock!

The parent clock is enabled by the clk framework when we enable the child 
clock. No need to do it explicitly.

My question was whether we can immediately *disable* the clock after writing 
the divider register. Could you test the above snippet in the i2c probe 
function? I don't have access to a RK3288 board.

> In the linux-rockchip branch code, the i2c's clock and parent clock are
> enabled by default. So there are no problems on it.

Yes, I saw that. I still think it's nice that we disable the clock when we 
don't need it. I'd like to keep that if possible.

> But on rk388-pinky board, pclk_peri is disabled when i2c driver is probed.
> (Maybe its reference count drops to 0 and it is disabled by clk driver)

Yes, that's probably because no one is using childs of the pclk_peri clock 
yet.

Cheers,
  Max
Heiko Stuebner Sept. 15, 2014, 8:38 a.m. UTC | #4
Am Montag, 15. September 2014, 10:29:21 schrieb Max Schwarz:
> Hi Addy,
> 
> Am Montag, 15. September 2014, 11:11:24 schrieb addy ke:
> > On 2014/9/13 19:26, Max Schwarz wrote:
> > > On Wednesday 10 September 2014 at 16:16:10, Addy wrote:
> > >>> @@ -724,16 +816,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);
> > >> 
> > >> I have tested on rk3288-pinky board:
> > >> I2c clock must be enabled before driver call rk3x_i2c_set_scl_rate()
> > >> to write div value to CLKDIV register. If not, system will crash.
> > >> 
> > >> So we need call clk_enable() before rk3x_i2c_set_scl_rate().
> > > 
> > > Interesting. It works without problems on RK3188. Do you know if the
> > > clock
> > > has to be kept enabled for some time? Or is it okay to do it like this?
> > > 
> > > clk_enable(i2c->clk);
> > > rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
> > > clk_disable(i2c->clk);
> > > 
> > > Cheers,
> > > 
> > >   Max
> > 
> > Before write value to i2c register, it is necessary to enable i2c's clock
> > and parent clock!
> 
> The parent clock is enabled by the clk framework when we enable the child
> clock. No need to do it explicitly.

Correct.

To make it even more explicit, kernel/driver code should _never_ rely on a 
clock magically just being on, but instead must handle it properly.

CLK_IGNORE_UNUSED and friends are just an intermediate measures when parts of 
the clock tree are still unhandled.


Heiko

> My question was whether we can immediately *disable* the clock after writing
> the divider register. Could you test the above snippet in the i2c probe
> function? I don't have access to a RK3288 board.
>
> > In the linux-rockchip branch code, the i2c's clock and parent clock are
> > enabled by default. So there are no problems on it.
> 
> Yes, I saw that. I still think it's nice that we disable the clock when we
> don't need it. I'd like to keep that if possible.
> 
> > But on rk388-pinky board, pclk_peri is disabled when i2c driver is probed.
> > (Maybe its reference count drops to 0 and it is disabled by clk driver)
> 
> Yes, that's probably because no one is using childs of the pclk_peri clock
> yet.
> 
> Cheers,
>   Max
addy ke Sept. 15, 2014, 8:51 a.m. UTC | #5
On 2014/9/15 16:29, Max Schwarz wrote:
> Hi Addy,
> 
> Am Montag, 15. September 2014, 11:11:24 schrieb addy ke:
>> On 2014/9/13 19:26, Max Schwarz wrote:
>>> On Wednesday 10 September 2014 at 16:16:10, Addy wrote:
>>>>> @@ -724,16 +816,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);
>>>>
>>>> I have tested on rk3288-pinky board:
>>>> I2c clock must be enabled before driver call rk3x_i2c_set_scl_rate()
>>>> to write div value to CLKDIV register. If not, system will crash.
>>>>
>>>> So we need call clk_enable() before rk3x_i2c_set_scl_rate().
>>>
>>> Interesting. It works without problems on RK3188. Do you know if the clock
>>> has to be kept enabled for some time? Or is it okay to do it like this?
>>>
>>> clk_enable(i2c->clk);
>>> rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
>>> clk_disable(i2c->clk);
>>>
>>> Cheers,
>>>
>>>   Max
>>
>> Before write value to i2c register, it is necessary to enable i2c's clock
>> and parent clock!
> 
> The parent clock is enabled by the clk framework when we enable the child 
> clock. No need to do it explicitly.
> 
> My question was whether we can immediately *disable* the clock after writing 
> the divider register. Could you test the above snippet in the i2c probe 
> function? I don't have access to a RK3288 board.
> 
sure, we can.
I have comfirmed on rk3288-pinky board.

>> In the linux-rockchip branch code, the i2c's clock and parent clock are
>> enabled by default. So there are no problems on it.
> 
> Yes, I saw that. I still think it's nice that we disable the clock when we 
> don't need it. I'd like to keep that if possible.
> 
>> But on rk388-pinky board, pclk_peri is disabled when i2c driver is probed.
>> (Maybe its reference count drops to 0 and it is disabled by clk driver)
> 
> Yes, that's probably because no one is using childs of the pclk_peri clock 
> yet.
> 
> Cheers,
>   Max
> 
> 
>
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 93cfc83..825632a 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,21 +430,114 @@  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;
+	/* Range checks */
+	if (scl_rate > clk_rate / 16) {
+		/* Return a best effort divider */
+		*div = 0;
+		return -EINVAL;
+	}
+
+	if (scl_rate < clk_rate / (16 * 65536)) {
+		/* Return a best effort divider */
+		*div = 0xFFFF;
+		return -EINVAL;
+	}
 
 	/* 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 = DIV_ROUND_UP(clk_rate, scl_rate * 16) - 1;
+
+	return 0;
+}
+
+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;
+
+	if (rk3x_i2c_calc_div(i2c_rate, scl_rate, &div) != 0) {
+		dev_warn(i2c->dev, "Could not reach desired SCL freq %lu Hz\n",
+			 scl_rate
+		);
+	}
 
 	i2c_writel(i2c, (div << 16) | (div & 0xffff), REG_CLKDIV);
 }
 
 /**
+ * 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;
+	}
+}
+
+/**
  * Setup I2C registers for an I2C operation specified by msgs, num.
  *
  * Must be called with i2c->lock held.
@@ -536,9 +631,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 +816,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;