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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <D4RBC5VO5DLQ.PATCQSM33ORM@bootlin.com>
Date: Wed, 09 Oct 2024 15:34:47 +0200
From: Théo Lebrun <theo.lebrun@...tlin.com>
To: Théo Lebrun <theo.lebrun@...tlin.com>, "Linus Walleij"
 <linus.walleij@...aro.org>
Cc: "Andi Shyti" <andi.shyti@...nel.org>, "Rob Herring" <robh@...nel.org>,
 "Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Conor Dooley"
 <conor+dt@...nel.org>, <linux-arm-kernel@...ts.infradead.org>,
 <linux-i2c@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, "Vladimir Kondratiev"
 <vladimir.kondratiev@...ileye.com>, Grégory Clement
 <gregory.clement@...tlin.com>, "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>, "Tawfik Bayouk"
 <tawfik.bayouk@...ileye.com>
Subject: Re: [PATCH v2 5/6] i2c: nomadik: fix BRCR computation

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@...tlin.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