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

[Wolfram Sang]

Hi Geert,

> > -       u32 icccr;
> > +       u32 clock_val;
> 
> Perhaps use a union to store either icccr or smd?

Yup, can do.

> 
> >         u8 recovery_icmcr;      /* protected by adapter lock */
> >         enum rcar_i2c_type devtype;
> >         struct i2c_client *slave;
> > @@ -217,7 +228,17 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
> >         rcar_i2c_write(priv, ICMCR, MDBS);
> >         rcar_i2c_write(priv, ICMSR, 0);
> >         /* start clock */
> > -       rcar_i2c_write(priv, ICCCR, priv->icccr);
> > +       if (priv->flags & ID_P_FMPLUS) {
> > +               rcar_i2c_write(priv, ICCCR, 0);
> > +               rcar_i2c_write(priv, ICMPR, priv->clock_val);
> > +               rcar_i2c_write(priv, ICHPR, 3 * priv->clock_val);
> > +               rcar_i2c_write(priv, ICLPR, 3 * priv->clock_val);
> > +               rcar_i2c_write(priv, ICCCR2, FMPE | CDFD | HLSE | SME);
> 
> ICCCR2 note 1: "ICCCR2 should be written to prior to writing ICCCR."

Eeks, I remembered it the other way around :/

> >         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?

> >         round = (round + 500) / 1000;
> 
> DIV_ROUND_UP()

DIV_ROUND_CLOSEST() I'd say, but I have a seperate patch for that.

> > +       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?

> > +               /*
> > +                * 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"?

> > +               scl = ick / (8 + 8 * smd + round);
> 
> DIV_ROUND_UP()?

Okay.

> > +               if (smd > 0xff) {
> > +                       dev_err(dev, "it is impossible to calculate best SCL\n");
> > +                       return -EINVAL;
> 
> Perhaps some "goto error", to share with the error handling for non-FM+?

I will check with the refactored code.

> > -       dev_dbg(dev, "clk %d/%d(%lu), round %u, CDF:0x%x, SCGD: 0x%x\n",
> > -               scl, t.bus_freq_hz, rate, round, cdf, scgd);
> > +               dev_dbg(dev, "clk %d/%d(%lu), round %u, SMD:0x%x, SCHD: 0x%x\n",
> 
> %u/%u
> 
> Perhaps it makes more sense to print SMD and SCHD in decimal?
> 
> This also applies to the existing code (CDF/SCGD) you moved into
> the else branch.

Can do. I don't care it is debug output.

> > +               if (scgd == 0x40) {
> > +                       dev_err(dev, "it is impossible to calculate best SCL\n");
> > +                       return -EINVAL;
> 
> This was -EIO before.

I'll squash this into a seperate cleanup patch I have.

Thanks,

   Wolfram

Attachment: signature.asc
Description: PGP signature