1. Patchew
  2. linux
  3. [PATCH v2 0/6] i2c: nomadik: support >=1MHz & Mobileye EyeQ6H platform
  4. View patch

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

Théo Lebrun posted 6 patches 1 month, 2 weeks ago
There is a newer version of this series
[PATCH v2 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

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.
 	 */
 	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);

-- 
2.46.2
Re: [PATCH v2 5/6] i2c: nomadik: fix BRCR computation
Posted by Linus Walleij 1 month, 2 weeks ago 
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
Re: [PATCH v2 5/6] i2c: nomadik: fix BRCR computation
Posted by Théo Lebrun 1 month, 2 weeks ago 
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
Re: [PATCH v2 5/6] i2c: nomadik: fix BRCR computation
Posted by Théo Lebrun 1 month, 2 weeks ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.