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

[Kever Yang]

Add Addy to make sure he get this mail,  it's wrong mail cc address
in Max's mail, and Addy didn't subscribe linux-rockchip mailing list at 
that time.


On 09/16/2014 03:06 AM, Max Schwarz 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;
> +	}
> +
> +	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;
> +}
> +
> +/**
> + * 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
> +		);
> +	}
> +
> +	/*
> +	 * 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 +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);
>   err_clk:
>   	clk_unprepare(i2c->clk);
>   	return ret;