linux-kernel - Re: [PATCH 3/3] i2c: rcar: add FastMode+ support

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 6 Sep 2023 14:23:35 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Wolfram Sang <wsa+renesas@...g-engineering.com>
Cc:     linux-renesas-soc@...r.kernel.org,
        Andi Shyti <andi.shyti@...nel.org>, linux-i2c@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] i2c: rcar: add FastMode+ support

Hi Wolfram,

On Wed, Sep 6, 2023 at 2:11 PM Wolfram Sang
<wsa+renesas@...g-engineering.com> wrote:
> > >         ick = rate / (cdf + 1);
> >
> > In case of FM+, cdf will be zero, and ick == rate?
>
> Yes.
>
> > > @@ -292,34 +324,55 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
> > >         round = (ick + 500000) / 1000000 * sum;
> >
> > ick == rate if FM+
>
> Yes, does this induce a change here?

No, just pointing it out, and wondering if this is intended...

>
> > >         round = (round + 500) / 1000;
> >
> > DIV_ROUND_UP()
>
> DIV_ROUND_CLOSEST() I'd say, but I have a seperate patch for that.

Oops (it's too hot here for more coffee...)

> > > +       if (priv->flags & ID_P_FMPLUS) {
> >
> > IIUIC, on R-ar Gen3 and later you can use ICCCR2 without FM+, for
> > improved accuracy, too?
>
> Yeah, we could do that. It indeed improves accuracy:
>
>         old             new
> 100kHz: 97680/100000    99950/100000
> 400kHz: 373482/400000   399201/400000
>
> Caring about regressions here is a bit over the top, or?

Probably OK.

> > > +               /*
> > > +                * SMD should be smaller than SCLD and SCHD, we arbitrarily set
> > > +                * the ratio 1:3. SCHD:SCLD ratio is 1:1, thus:
> > > +                * SCL  = clkp / (8 + SMD * 2 + SCLD + SCHD + F[(ticf + tr + intd) * clkp])
> > > +                * SCL  = clkp / (8 + SMD * 2 + SMD * 3 + SMD * 3 + F[...])
> > > +                * SCL  = clkp / (8 + SMD * 8 + F[...])
> > > +                */
> > > +               smd = DIV_ROUND_UP(ick / t.bus_freq_hz - 8 - round, 8);
> >
> > Perhaps use rate instead of ick?
>
> That's probably cleaner.
>
> > DIV_ROUND_UP(ick, 8 * (t.bus_freq_hz - 8 - round));
>
> This looks like you assumed "ick / (t.bus_freq_hz - 8 - round)" but it
> is "(ick / t.bus_freq_hz) - 8 - round"?

Oops (again)

OK do you need rounding for the division of ick and t.bus_freq_hz,
or is the adjustment bij "- (round + 8)" already taking care of that?
I guess I just don't understand the intended formula here...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds