[PATCH v3 5/6] i2c: nomadik: fix BRCR computation

Théo Lebrun posted 6 patches 1 month, 2 weeks ago
[PATCH v3 5/6] i2c: nomadik: fix BRCR computation
Posted by Théo Lebrun 1 month, 2 weeks ago
Current BRCR computation is:

    brcr = floor(i2cclk / (clkfreq * div))

With brcr: "baud rate counter", an internal clock divider,
 and i2cclk: input clock rate (24MHz, 38.4MHz or 48MHz),
 and clkfreq: desired bus rate,
 and div: speed-mode dependent divider (2 for standard, 3 otherwise).

Assume i2cclk=48MHz, clkfreq=3.4MHz, div=3,
  then brcr = floor(48MHz / (3.4MHz * 3)) = 4
   and resulting bus rate = 48MHz / (4 * 3) = 4MHz

Assume i2cclk=38.4MHz, clkfreq=1.0MHz, div=3,
  then brcr = floor(38.4MHz / (1.0MHz * 3)) = 12
   and resulting bus rate = 38.4MHz / (12 * 3) = 1066kHz

The current computation means we always pick the smallest divider that
gives a bus rate above target. We should instead pick the largest
divider that gives a bus rate below target, using:

    brcr = floor(i2cclk / (clkfreq * div)) + 1

If we redo the above examples:

Assume i2cclk=48MHz, clkfreq=3.4MHz, div=3,
  then brcr = floor(48MHz / (3.4MHz * 3)) + 1 = 5
   and resulting bus rate = 48MHz / (5 * 3) = 3.2MHz

Assume i2cclk=38.4MHz, clkfreq=1.0MHz, div=3,
  then brcr = floor(38.4MHz / (1.0MHz * 3)) + 1 = 13
   and resulting bus rate = 38.4MHz / (13 * 3) = 985kHz

In kernel C code, floor(x)   is DIV_ROUND_DOWN() and,
                  floor(x)+1 is DIV_ROUND_UP().

This is much less of an issue with slower bus rates (ie those currently
supported), because the gap from one divider to the next is much
smaller. It however keeps us from always using bus rates superior to
the target.

This fix is required for later on supporting faster bus rates:
I2C_FREQ_MODE_FAST_PLUS (1MHz) and I2C_FREQ_MODE_HIGH_SPEED (3.4MHz).

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 8f52ae4d6285af2dd2b3dc7070672757e831a019..90e3390cd66de5b3aa47d94de7fb30e1d67ac70e 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -454,9 +454,13 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
 	 * operation, and the other is for std, fast mode, fast mode
 	 * plus operation. Currently we do not supprt high speed mode
 	 * so set brcr1 to 0.
+	 *
+	 * BRCR is a clock divider amount. Pick highest value that
+	 * leads to rate strictly below target. Eg when asking for
+	 * 400kHz you want a bus rate <=400kHz (and not >=400kHz).
 	 */
 	brcr1 = FIELD_PREP(I2C_BRCR_BRCNT1, 0);
-	brcr2 = FIELD_PREP(I2C_BRCR_BRCNT2, i2c_clk / (priv->clk_freq * div));
+	brcr2 = FIELD_PREP(I2C_BRCR_BRCNT2, DIV_ROUND_UP(i2c_clk, priv->clk_freq * div));
 
 	/* set the baud rate counter register */
 	writel((brcr1 | brcr2), priv->virtbase + I2C_BRCR);

-- 
2.46.2

Re: [PATCH v3 5/6] i2c: nomadik: fix BRCR computation
Posted by Linus Walleij 1 month, 2 weeks ago
On Wed, Oct 9, 2024 at 4:01 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> Current BRCR computation is:
>
>     brcr = floor(i2cclk / (clkfreq * div))
>
> With brcr: "baud rate counter", an internal clock divider,
>  and i2cclk: input clock rate (24MHz, 38.4MHz or 48MHz),
>  and clkfreq: desired bus rate,
>  and div: speed-mode dependent divider (2 for standard, 3 otherwise).
>
> Assume i2cclk=48MHz, clkfreq=3.4MHz, div=3,
>   then brcr = floor(48MHz / (3.4MHz * 3)) = 4
>    and resulting bus rate = 48MHz / (4 * 3) = 4MHz
>
> Assume i2cclk=38.4MHz, clkfreq=1.0MHz, div=3,
>   then brcr = floor(38.4MHz / (1.0MHz * 3)) = 12
>    and resulting bus rate = 38.4MHz / (12 * 3) = 1066kHz
>
> The current computation means we always pick the smallest divider that
> gives a bus rate above target. We should instead pick the largest
> divider that gives a bus rate below target, using:
>
>     brcr = floor(i2cclk / (clkfreq * div)) + 1
>
> If we redo the above examples:
>
> Assume i2cclk=48MHz, clkfreq=3.4MHz, div=3,
>   then brcr = floor(48MHz / (3.4MHz * 3)) + 1 = 5
>    and resulting bus rate = 48MHz / (5 * 3) = 3.2MHz
>
> Assume i2cclk=38.4MHz, clkfreq=1.0MHz, div=3,
>   then brcr = floor(38.4MHz / (1.0MHz * 3)) + 1 = 13
>    and resulting bus rate = 38.4MHz / (13 * 3) = 985kHz
>
> In kernel C code, floor(x)   is DIV_ROUND_DOWN() and,
>                   floor(x)+1 is DIV_ROUND_UP().
>
> This is much less of an issue with slower bus rates (ie those currently
> supported), because the gap from one divider to the next is much
> smaller. It however keeps us from always using bus rates superior to
> the target.
>
> This fix is required for later on supporting faster bus rates:
> I2C_FREQ_MODE_FAST_PLUS (1MHz) and I2C_FREQ_MODE_HIGH_SPEED (3.4MHz).
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Excellent, thanks for iterating!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij