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

[Linus Walleij]

On Wed, Oct 9, 2024 at 4:01 PM Théo Lebrun <theo.lebrun@xxxxxxxxxxx> 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@xxxxxxxxxxx>

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

Yours,
Linus Walleij