i2c: rk3x: handle dynamic clock rate changes correctly
Subsystems (2)
PATCH v7 1/1 i2c: rk3x: handle dynamic clock rate changes correctly
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.
Also rename all misleading "i2c_rate" variables to "clk_rate", as they describe the *input* clk rate.
---
Changes since v6:
- "best effort" logic in rk3x_i2c_calc_divs() (sugg. by Doug Anderson)
- fix range check in rk3x_i2c_calc_divs() (sugg. by Doug Anderson)
Changes since v5:
- document rk3x_i2c_calc_divs (Doug Anderson)
- rename all i2c_rate to clk_rate (Doug Anderson)
- remove obsolete braces in rk3x_i2c_clk_notifier_cb (Doug Anderson)
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.
Here are waveforms from rate change tests:
Rate change from default rate to half rate:
http://x-quadraht.de/fast_to_slow.png
Quoted 1 line
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.
Cheers,
Max
drivers/i2c/busses/i2c-rk3x.c | 153 ++++++++++++++++++++++++++++++++---------- 1 file changed, 118 insertions(+), 35 deletions(-)
drivers/i2c/busses/i2c-rk3x.c +118 -35
@@ -98,6 +98,7 @@ struct rk3x_i2c {/* Hardware resources */ void __iomem *regs;struct clk *clk; +struct notifier_block clk_rate_nb;/* Settings */ unsigned int scl_frequency;
@@ -429,15 +430,27 @@ out:return IRQ_HANDLED ; } -static int rk3x_i2c_calc_divs (unsigned long i2c_rate,unsigned long scl_rate, -unsigned long *div_low,unsigned long *div_high) +/** +* Calculate divider values for desired SCL frequency +* +* @clk_rate: I2C input clock rate +* @scl_rate: Desired SCL rate +* @div_low: Divider output for low +* @div_high: Divider output for high +* +* Returns: 0 on success, -EINVAL if the goal SCL rate is too slow. In that case +* a best-effort divider value is returned in divs. If the target rate is +* too high, we silently use the highest possible rate. +*/ +static int rk3x_i2c_calc_divs (unsigned long clk_rate,unsigned long scl_rate, +unsigned long *div_low,unsigned long *div_high) {unsigned long min_low_ns, min_high_ns;unsigned long max_data_hold_ns;unsigned long data_hold_buffer_ns;unsigned long max_low_ns, min_total_ns; -unsigned long i2c_rate_khz, scl_rate_khz; +unsigned long clk_rate_khz, scl_rate_khz;unsigned long min_low_div, min_high_div;unsigned long max_low_div;
@@ -445,6 +458,8 @@ static int rk3x_i2c_calc_divs(unsigned long i2c_rate, unsigned long scl_rate,unsigned long min_div_for_hold, min_total_div;unsigned long extra_div, extra_low_div, ideal_low_div; +int ret =0 ; +/* Only support standard-mode and fast-mode */ if (WARN_ON (scl_rate >400000 )) scl_rate =400000 ;
@@ -480,25 +495,25 @@ static int rk3x_i2c_calc_divs(unsigned long i2c_rate, unsigned long scl_rate, min_total_ns = min_low_ns + min_high_ns;/* Adjust to avoid overflow */ - i2c_rate_khz =DIV_ROUND_UP (i2c_rate,1000 ); + clk_rate_khz =DIV_ROUND_UP (clk_rate,1000 ); scl_rate_khz = scl_rate /1000 ;/* * We need the total div to be >= this number * so we don't clock too fast. */ - min_total_div =DIV_ROUND_UP (i2c_rate_khz, scl_rate_khz *8 ); + min_total_div =DIV_ROUND_UP (clk_rate_khz, scl_rate_khz *8 );/* These are the min dividers needed for min hold times. */ - min_low_div =DIV_ROUND_UP (i2c_rate_khz * min_low_ns,8 *1000000 ); - min_high_div =DIV_ROUND_UP (i2c_rate_khz * min_high_ns,8 *1000000 ); + min_low_div =DIV_ROUND_UP (clk_rate_khz * min_low_ns,8 *1000000 ); + min_high_div =DIV_ROUND_UP (clk_rate_khz * min_high_ns,8 *1000000 ); min_div_for_hold = (min_low_div + min_high_div);/* * This is the maximum divider so we don't go over the max. * We don't round up here (we round down) since this is a max. */ - max_low_div = i2c_rate_khz * max_low_ns / (8 *1000000 ); + max_low_div = clk_rate_khz * max_low_ns / (8 *1000000 );if (min_low_div > max_low_div) { WARN_ONCE(true,
@@ -526,7 +541,7 @@ static int rk3x_i2c_calc_divs(unsigned long i2c_rate, unsigned long scl_rate,* biasing slightly towards having a higher div * for low (spend more time low). */ - ideal_low_div =DIV_ROUND_UP (i2c_rate_khz * min_low_ns, + ideal_low_div =DIV_ROUND_UP (clk_rate_khz * min_low_ns, scl_rate_khz *8 * min_total_ns);/* Don't allow it to go over the max */
@@ -547,40 +562,99 @@ static int rk3x_i2c_calc_divs(unsigned long i2c_rate, unsigned long scl_rate, }/* -* Adjust to the fact that the hardware has an implicit "+1". -* NOTE: Above calculations always produce div_low > 0 and div_high > 0. -*/ +* Adjust to the fact that the hardware has an implicit "+1". +* NOTE: Above calculations always produce div_low > 0 and div_high > 0. +*/ *div_low = *div_low -1 ; *div_high = *div_high -1 ; -if (*div_low >=0xffff || *div_high >=0xffff ) -return -EINVAL ; -else -return 0 ; +/* Maximum divider supported by hw is 0xffff */ +if (*div_low >0xffff ) { + *div_low =0xffff ; + ret = -EINVAL ; + } + +if (*div_high >0xffff ) { + *div_high =0xffff ; + ret = -EINVAL ; + } + +return ret; } -static int rk3x_i2c_set_scl_rate (struct rk3x_i2c *i2c,unsigned long scl_rate) +static void rk3x_i2c_adapt_div (struct rk3x_i2c *i2c,unsigned long clk_rate) { -unsigned long i2c_rate =clk_get_rate (i2c->clk);unsigned long div_low, div_high;u64 t_low_ns, t_high_ns; -int ret =0 ; +int ret; - ret =rk3x_i2c_calc_divs (i2c_rate, scl_rate, &div_low, &div_high); -if (ret <0 ) -return ret; + ret =rk3x_i2c_calc_divs (clk_rate, i2c->scl_frequency, &div_low, + &div_high); +WARN_ONCE (ret !=0 ,"Could not reach SCL freq %u" , i2c->scl_frequency); + +clk_enable (i2c->clk);i2c_writel (i2c, (div_high <<16 ) | (div_low &0xffff ),REG_CLKDIV ); +clk_disable (i2c->clk); - t_low_ns =div_u64 (((u64 )div_low +1 ) *8 *1000000000 , i2c_rate); - t_high_ns =div_u64 (((u64 )div_high +1 ) *8 *1000000000 , i2c_rate); + t_low_ns =div_u64 (((u64 )div_low +1 ) *8 *1000000000 , clk_rate); + t_high_ns =div_u64 (((u64 )div_high +1 ) *8 *1000000000 , clk_rate);dev_dbg (i2c->dev, -"CLK %lukhz, Req %luns, Act low %lluns high %lluns\n" , - i2c_rate /1000 , -1000000000 / scl_rate, +"CLK %lukhz, Req %uns, Act low %lluns high %lluns\n" , + clk_rate /1000 , +1000000000 / i2c->scl_frequency, t_low_ns, t_high_ns); +} -return ret; +/** +* 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 long div_low, div_high; + +switch (event) { +case PRE_RATE_CHANGE : +if (rk3x_i2c_calc_divs (ndata->new_rate, i2c->scl_frequency, + &div_low, &div_high) !=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 ; + } }/**
@@ -677,11 +751,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 */ - ret =rk3x_i2c_set_scl_rate (i2c, i2c->scl_frequency); -if (ret <0 ) - goto exit; - i2c->is_last_msg = false;/*
@@ -728,7 +797,6 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap, } } -exit:clk_disable (i2c->clk);spin_unlock_irqrestore (&i2c->lock, flags);
@@ -768,6 +836,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)
@@ -868,16 +937,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;
@@ -888,6 +969,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 ;
Signature · 1 line
1.9.1
Quoted 8 lines · Max Schwarz
The i2c input clock can change dynamically, e.g. on the RK3066 where
pclk_i2c0andpclk_i2c1are connected to the armclk, which changes rate on cpu frequency scaling.Until now, we incorrectly called
clk_get_rate()while holding thei2c->lockinrk3x_i2c_xfer()to adapt to clock rate changes. Thanks to Huang Tao for reporting this issue.
Quoted 8 lines
Do it properly now using the clk notifier framework. The callback logic was taken from
i2c-cadence.c.Also rename all misleading "
i2c_rate" variables to "clk_rate", as they describe the *input* clk rate.Signed-off-by Max Schwarz <[email protected]>
Superfluous, I assume that you tested a patch when you send it :)
Quoted 2 lines
Reviewed-by Doug Anderson <[email protected]>
Please drop such tags when there are significant changes between the revisions (which happened here). They need to be sent again explicitly, so I know really this version has been worked on.
So, Doug, please confirm those tags.
Wolfram,
Quoted 20 lines · Wolfram Sang
Quoted 8 lines · Max Schwarz
The i2c input clock can change dynamically, e.g. on the RK3066 where
pclk_i2c0andpclk_i2c1are connected to the armclk, which changes rate on cpu frequency scaling.Until now, we incorrectly called
clk_get_rate()while holding thei2c->lockinrk3x_i2c_xfer()to adapt to clock rate changes. Thanks to Huang Tao for reporting this issue.Quoted 8 lines
Do it properly now using the clk notifier framework. The callback logic was taken from
i2c-cadence.c.Also rename all misleading "
i2c_rate" variables to "clk_rate", as they describe the *input* clk rate.Signed-off-by Max Schwarz <[email protected]>Superfluous, I assume that you tested a patch when you send it :)
Quoted 8 lines
Quoted 2 lines
Reviewed-by Doug Anderson <[email protected]>Please drop such tags when there are significant changes between the revisions (which happened here). They need to be sent again explicitly, so I know really this version has been worked on.
So, Doug, please confirm those tags.
Consider them confirmed. I like this new version and it seems to be working well for me. I don't use the dynamic clock changes but it at least doesn't crash or break in any other ways. I've landed it locally in my tree.
-Doug
Wolfram Sang
AppliedQuoted 11 lines · Max Schwarz
The i2c input clock can change dynamically, e.g. on the RK3066 where
pclk_i2c0andpclk_i2c1are connected to the armclk, which changes rate on cpu frequency scaling.Until now, we incorrectly called
clk_get_rate()while holding thei2c->lockinrk3x_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.
Quoted 7 lines
Also rename all misleading "
i2c_rate" variables to "clk_rate", as they describe the *input* clk rate.Signed-off-by Max Schwarz <[email protected]>Reviewed-by Doug Anderson <[email protected]>
Applied to for-next, thanks!