| Message ID | 1378742120-11135-3-git-send-email-g.liakhovetski@gmx.de |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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);
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(-)