Re: [PATCH RFT 2/2] i2c: rcar: improve accuracy for R-Car Gen3+

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Sep 19, 2023 at 11:14 AM Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
> On Wed, Sep 13, 2023 at 11:38 AM Wolfram Sang
> <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
> > With some new registers, SCL can be calculated to be closer to the
> > desired rate. Apply the new formula for R-Car Gen3 device types.
> >
> > Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
>
> Thanks for your patch!
>
> > --- a/drivers/i2c/busses/i2c-rcar.c
> > +++ b/drivers/i2c/busses/i2c-rcar.c
> > @@ -301,24 +316,57 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
> >         round = DIV_ROUND_CLOSEST(ick, 1000000);
> >         round = DIV_ROUND_CLOSEST(round * sum, 1000);
> >
> > -       /*
> > -        * SCL  = ick / (20 + 8 * SCGD + F[(ticf + tr + intd) * ick])
> > -        * 20 + 8 * SCGD + F[...] = ick / SCL
> > -        * SCGD = ((ick / SCL) - 20 - F[...]) / 8
> > -        * Result (= SCL) should be less than bus_speed for hardware safety
> > -        */
> > -       scgd = DIV_ROUND_UP(ick, t.bus_freq_hz ?: 1);
> > -       scgd = DIV_ROUND_UP(scgd - 20 - round, 8);
> > -       scl = ick / (20 + 8 * scgd + round);
> > +       if (priv->devtype < I2C_RCAR_GEN3) {
> > +               u32 scgd;
> > +               /*
> > +                * SCL  = ick / (20 + 8 * SCGD + F[(ticf + tr + intd) * ick])
> > +                * 20 + 8 * SCGD + F[...] = ick / SCL
> > +                * SCGD = ((ick / SCL) - 20 - F[...]) / 8
> > +                * Result (= SCL) should be less than bus_speed for hardware safety
> > +                */
> > +               scgd = DIV_ROUND_UP(ick, t.bus_freq_hz ?: 1);
> > +               scgd = DIV_ROUND_UP(scgd - 20 - round, 8);
> > +               scl = ick / (20 + 8 * scgd + round);
> >
> > -       if (scgd > 0x3f)
> > -               goto err_no_val;
> > +               if (scgd > 0x3f)
> > +                       goto err_no_val;
> >
> > -       dev_dbg(dev, "clk %u/%u(%lu), round %u, CDF: %u, SCGD: %u\n",
> > -               scl, t.bus_freq_hz, rate, round, cdf, scgd);
> > +               dev_dbg(dev, "clk %u/%u(%lu), round %u, CDF: %u, SCGD: %u\n",
> > +                       scl, t.bus_freq_hz, rate, round, cdf, scgd);
> >
> > -       /* keep icccr value */
> > -       priv->icccr = scgd << cdf_width | cdf;
> > +               priv->icccr = scgd << cdf_width | cdf;
> > +       } else {
> > +               u32 x, sum_ratio = RCAR_SCHD_RATIO + RCAR_SCLD_RATIO;
> > +               /*
> > +                * SCLD/SCHD ratio and SMD default value are explained above
> > +                * where they are defined. With these definitions, we can compute
> > +                * x as a base value for the SCLD/SCHD ratio:
> > +                *
> > +                * SCL = clkp / (8 + 2 * SMD + SCLD + SCHD + F[(ticf + tr + intd) * clkp])
> > +                * SCL = clkp / (8 + 2 * RCAR_DEFAULT_SMD + RCAR_SCLD_RATIO * x
> > +                *               + RCAR_SCHD_RATIO * x + F[...])
> > +                *
> > +                * with: sum_ratio = RCAR_SCLD_RATIO + RCAR_SCHD_RATIO
> > +                * and:  smd = 2 * RCAR_DEFAULT_SMD
> > +                *
> > +                * SCL = clkp / (8 + smd + sum_ratio * x + F[...])
> > +                * 8 + smd + sum_ratio * x + F[...] = SCL / clkp
> > +                * x = ((SCL / clkp) - 8 - smd - F[...]) / sum_ratio
> > +                */
> > +               x = DIV_ROUND_UP(rate, t.bus_freq_hz ?: 1);
> > +               x = DIV_ROUND_UP(x - 8 - 2 * RCAR_DEFAULT_SMD - round, sum_ratio);
> > +               scl = rate / (8 + 2 * RCAR_DEFAULT_SMD + sum_ratio * x + round);
> > +
> > +               /* Bail out if values don't fit into 16 bit or SMD became too large */
> > +               if (x * RCAR_SCLD_RATIO > 0xffff || RCAR_DEFAULT_SMD > x * RCAR_SCHD_RATIO)
>
> The second part of the check looks wrong to me, as it would reject
> all the recommended register values for SMD and SCHD in the docs .
>
> What does "SMD became too large" mean here?

Nevermind, my mistake.

Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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