[PATCH v2 RFC] i2c: rk3x: handle dynamic clock rate changes correctly
Doug Anderson
dianders at chromium.org
Tue Sep 16 15:40:21 PDT 2014
Max,
On Mon, Sep 15, 2014 at 12:06 PM, Max Schwarz <max.schwarz at 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 at online.de>
> Tested-by: Max Schwarz <max.schwarz at online.de> on RK3188
> (dynamic rate changes are untested!)
> ---
>
> 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 | 139 +++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 131 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 93cfc83..4c35526 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,127 @@ 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;
IMHO this isn't an error case. i2c will still run at slower rates and
it doesn't seem like you should block clock changes based on this.
In fact your code is already assuming that running a little slow is OK
since you'll set dividers based on a faster clock while still running
on the slower one.
> + }
> +
> + if (scl_rate < clk_rate / (16 * 65536)) {
> + /* Return a best effort divider */
> + *div = 0xFFFF;
> + return -EINVAL;
> + }
If I were writing this I'd avoid making the reader try to figure out
whether this matches all the corner cases DIV_ROUND_UP. I'd just do
the DIV_ROUND_UP and consider it an error if the resulting value is >
0xffff.
>
> /* 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;
> +}
> +
> +/**
> + * 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;
> +
> + 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
> + );
It seems likely this will spam the user and make the system unusable.
You're envisioning a case where this is based on the arm clock and
thus changing constantly.
I know not everyone likes WARN_ON, but I'd personally do this as
WARN_ON_ONCE so it gets the user's attention with a nice big stack
crawl and then stops spamming (in case things are actually working).
> + }
> +
> + /*
> + * 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);
You've got a clk_enable here, but you also have it in probe right
around this function call. Can't you remove the ones from probe()?
> 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 +647,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 +832,31 @@ 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);
> +
> + clk_enable(i2c->clk);
> + rk3x_i2c_set_scl_rate(i2c, i2c->scl_frequency);
> + clk_disable(i2c->clk);
> +
> 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);
I think you need this in rk3x_i2c_remove(), don't you?
> err_clk:
> clk_unprepare(i2c->clk);
> return ret;
> --
> 1.9.1
>
More information about the Linux-rockchip
mailing list