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

[Linus Walleij]

Hi Theo,

thanks for your patch!

On Wed, Oct 9, 2024 at 12:23 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
>
> 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>
> ---
>  drivers/i2c/busses/i2c-nomadik.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index 8f52ae4d6285af2dd2b3dc7070672757e831a019..b2b9da0b32ed903c080f4bdc427ea0dd7b031b49 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -454,9 +454,12 @@ 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.
>          */

You could push in some more details from the commit message here so it's not
so terse.

>         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, i2c_clk / (priv->clk_freq * div) + 1);

Doesn't the last part correspond to something like
#include <linux/math.h>
u64 scaler = DIV_ROUND_DOWN_ULL(i2c_clk, (priv->clk_freq * div));
brcr2 = FIELD_PREP(I2C_BRCR_BRCNT2, (u32)scaler);

Certianly one of the in-kernel division helpers like DIV_ROUND_DOWN
round_up() etc are better to use IMO, but I might not be understanding the
fine details of the math here.

Yours,
Linus Walleij