lkml.org 
[lkml]   [2023]   [Sep]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/3] i2c: rcar: add FastMode+ support
    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

    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2023-09-06 14:12    [W:3.735 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site