Patchwork [2/5] i2c: rcar: get clock rate only once and simplify calculation

login
register
mail settings
Submitter Guennadi Liakhovetski
Date Sept. 9, 2013, 3:55 p.m.
Message ID <1378742120-11135-3-git-send-email-g.liakhovetski@gmx.de>
Download mbox | patch
Permalink /patch/273608/
State Superseded
Headers show

Comments

Guennadi Liakhovetski - Sept. 9, 2013, 3:55 p.m.
There is no need to repeatedly query clock frequency, where it is not
expected to change. The complete loop can also trivially be replaced with
a simple division. A further loop below the one, being simplified, could
also be replaced, but that would get more complicated.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
---
 drivers/i2c/busses/i2c-rcar.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)
Magnus Damm - Sept. 10, 2013, 10:37 p.m.
Hi Guennadi,

On Tue, Sep 10, 2013 at 12:55 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> There is no need to repeatedly query clock frequency, where it is not
> expected to change. The complete loop can also trivially be replaced with
> a simple division. A further loop below the one, being simplified, could
> also be replaced, but that would get more complicated.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>

Thanks for your efforts. What makes you think that you should assume
that the clock frequency doesn't change?

As you already know, we want drivers to be reusable across multiple
SoCs. Assuming it that it would be fixed based on one particular SoC
doesn't guarantee that that's the case on other SoCs. Which SoCs did
you take into consideration?

Thanks,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski - Sept. 11, 2013, 7:03 a.m.
Hi Magnus

Thanks for your comments

On Wed, 11 Sep 2013, Magnus Damm wrote:

> Hi Guennadi,
> 
> On Tue, Sep 10, 2013 at 12:55 AM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > There is no need to repeatedly query clock frequency, where it is not
> > expected to change. The complete loop can also trivially be replaced with
> > a simple division. A further loop below the one, being simplified, could
> > also be replaced, but that would get more complicated.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
> 
> Thanks for your efforts. What makes you think that you should assume
> that the clock frequency doesn't change?

Sorry, maybe I didn't explain well enough in the patch description. 
Please, have a look at the patch. It removes repeated clock rate readings 
inside a tight loop. I'm aware that the clock rate can change, but there 
should be a way to make sure, that beginning now and until a certain later 
point of time the rate doesn't change. Presumably this should be done by 
locking the clock. I'm not sure this is currently supported by the clock 
API. Neither clk_get(), nor clk_prepare() nor clk_enable() seem to have 
the semantics of locking the clock's frequency. Maybe the one who actually 
prepared the clock should become its owner, but I don't think this is 
currently the expected behaviour. In either case, I don't think calling 
clk_get_rate() repeatedly inside _that_ loop makes sense - the rate can 
anyway be changed after the loop, so, it doesn't add any security.

Thanks
Guennadi

> As you already know, we want drivers to be reusable across multiple
> SoCs. Assuming it that it would be fixed based on one particular SoC
> doesn't guarantee that that's the case on other SoCs. Which SoCs did
> you take into consideration?

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 15eef94..9325db4 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -231,6 +231,7 @@  static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv,
 	u32 round, ick;
 	u32 scl;
 	u32 cdf_width;
+	unsigned long rate;
 
 	if (!clkp) {
 		dev_err(dev, "there is no peripheral_clk\n");
@@ -264,15 +265,14 @@  static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv,
 	 * clkp : peripheral_clk
 	 * F[]  : integer up-valuation
 	 */
-	for (cdf = 0; cdf < (1 << cdf_width); cdf++) {
-		ick = clk_get_rate(clkp) / (1 + cdf);
-		if (ick < 20000000)
-			goto ick_find;
+	rate = clk_get_rate(clkp);
+	cdf = rate / 20000000;
+	if (cdf >= 1 << cdf_width) {
+		dev_err(dev, "Input clock %lu too high\n", rate);
+		return -EIO;
 	}
-	dev_err(dev, "there is no best CDF\n");
-	return -EIO;
+	ick = rate / (cdf + 1);
 
-ick_find:
 	/*
 	 * it is impossible to calculate large scale
 	 * number on u32. separate it
@@ -290,6 +290,12 @@  ick_find:
 	 *
 	 * Calculation result (= SCL) should be less than
 	 * bus_speed for hardware safety
+	 *
+	 * We could use something along the lines of
+	 *	div = ick / (bus_speed + 1) + 1;
+	 *	scgd = (div - 20 - round + 7) / 8;
+	 *	scl = ick / (20 + (scgd * 8) + round);
+	 * (not fully verified) but that would get pretty involved
 	 */
 	for (scgd = 0; scgd < 0x40; scgd++) {
 		scl = ick / (20 + (scgd * 8) + round);