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
