| Message ID | 20241009-mbly-i2c-v2-5-ac9230a8dac5@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: nomadik: support >=1MHz & Mobileye EyeQ6H platform | expand |
Hi Theo, thanks for your patch! On Wed, Oct 9, 2024 at 12:23 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 > > 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 | 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
Hello Linus, On Wed Oct 9, 2024 at 1:34 PM CEST, Linus Walleij wrote: > On Wed, Oct 9, 2024 at 12:23 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > --- 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. Most of the details from the commit message come from behavior changes: what was done previously versus what is the new behavior we implement. Having a clock divider picking the bus rate that is below the target speed rather than above sounds rather intuitive. Eg when you ask for 400kHz you want <=400kHz, not >=400kHz. I'll add that last sentence "Eg when you ask for 400kHz you want a bus rate <=400kHz (and not >=400kHz)". It is straight forward and easy to understand. > > 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. Indeed what we want is: DIV_ROUND_DOWN(i2c_clk, priv->clk_freq * div) I see no reason to use DIV_ROUND_DOWN_ULL(). It would be useful if i2c_clk + (priv->clk_freq * div) had a chance to overflow. Worst case is: 3_400_000 + (48_000_000 * 3) = 147_400_000 Will send v3 straight away as this is a significant change, thanks Linus! -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Wed Oct 9, 2024 at 3:31 PM CEST, Théo Lebrun wrote: > On Wed Oct 9, 2024 at 1:34 PM CEST, Linus Walleij wrote: > > On Wed, Oct 9, 2024 at 12:23 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > --- 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. > > Most of the details from the commit message come from behavior changes: > what was done previously versus what is the new behavior we implement. > > Having a clock divider picking the bus rate that is below the target > speed rather than above sounds rather intuitive. Eg when you ask for > 400kHz you want <=400kHz, not >=400kHz. > > I'll add that last sentence "Eg when you ask for 400kHz you want a bus > rate <=400kHz (and not >=400kHz)". It is straight forward and easy to > understand. > > > > 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. > > Indeed what we want is: > DIV_ROUND_DOWN(i2c_clk, priv->clk_freq * div) s/DIV_ROUND_DOWN/DIV_ROUND_UP/ (sorry for the confusion) > > I see no reason to use DIV_ROUND_DOWN_ULL(). It would be useful if s/DIV_ROUND_DOWN_ULL/DIV_ROUND_UP_ULL/ > i2c_clk + (priv->clk_freq * div) > had a chance to overflow. > > Worst case is: > 3_400_000 + (48_000_000 * 3) = 147_400_000 > > Will send v3 straight away as this is a significant change, > thanks Linus! > > -- > Théo Lebrun, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
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. */ 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); /* set the baud rate counter register */ writel((brcr1 | brcr2), priv->virtbase + I2C_BRCR);
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@bootlin.com> --- drivers/i2c/busses/i2c-nomadik.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)