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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Wolfram,

On Tue, Sep 5, 2023 at 6:01 PM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
> Apply the different formula and register setting for activating FM+ on
> Gen4 devtypes.
>
> 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

> @@ -128,7 +139,7 @@ struct rcar_i2c_priv {
>         wait_queue_head_t wait;
>
>         int pos;
> -       u32 icccr;
> +       u32 clock_val;

Perhaps use a union to store either icccr or smd?

>         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."

> +       } else {
> +               rcar_i2c_write(priv, ICCCR, priv->clock_val);
> +               if (priv->devtype >= I2C_RCAR_GEN3)
> +                       rcar_i2c_write(priv, ICCCR2, 0);

Likewise.

> +       }
>
>         if (priv->devtype >= I2C_RCAR_GEN3)
>                 rcar_i2c_write(priv, ICFBSCR, TCYC17);
> @@ -242,7 +263,7 @@ static int rcar_i2c_bus_barrier(struct rcar_i2c_priv *priv)
>
>  static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
>  {
> -       u32 scgd, cdf, round, ick, sum, scl, cdf_width;
> +       u32 scgd, cdf = 0, round, ick, sum, scl, cdf_width, smd;
>         unsigned long rate;
>         struct device *dev = rcar_i2c_priv_to_dev(priv);
>         struct i2c_timings t = {
> @@ -252,19 +273,26 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
>                 .scl_int_delay_ns       = 50,
>         };
>
> -       cdf_width = (priv->devtype == I2C_RCAR_GEN1) ? 2 : 3;
> -
>         /* Fall back to previously used values if not supplied */
>         i2c_parse_fw_timings(dev, &t, false);
>
> +       if (t.bus_freq_hz > I2C_MAX_FAST_MODE_FREQ &&
> +           priv->devtype >= I2C_RCAR_GEN4)
> +               priv->flags |= ID_P_FMPLUS;
> +       else
> +               priv->flags &= ~ID_P_FMPLUS;
> +
>         /*
>          * calculate SCL clock
>          * see
> -        *      ICCCR
> +        *      ICCCR (and ICCCR2 for FastMode+)
>          *
>          * ick  = clkp / (1 + CDF)
>          * SCL  = ick / (20 + SCGD * 8 + F[(ticf + tr + intd) * ick])
>          *
> +        * for FastMode+:
> +        * SCL  = clkp / (8 + SMD * 2 + SCLD + SCHD +F[(ticf + tr + intd) * clkp])
> +        *
>          * ick  : I2C internal clock < 20 MHz
>          * ticf : I2C SCL falling time
>          * tr   : I2C SCL rising  time
> @@ -273,10 +301,14 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
>          * F[]  : integer up-valuation
>          */
>         rate = clk_get_rate(priv->clk);
> -       cdf = rate / 20000000;
> -       if (cdf >= 1U << cdf_width) {
> -               dev_err(dev, "Input clock %lu too high\n", rate);
> -               return -EIO;
> +
> +       if (!(priv->flags & ID_P_FMPLUS)) {
> +               cdf = rate / 20000000;
> +               cdf_width = (priv->devtype == I2C_RCAR_GEN1) ? 2 : 3;
> +               if (cdf >= 1U << cdf_width) {
> +                       dev_err(dev, "Input clock %lu too high\n", rate);
> +                       return -EIO;
> +               }
>         }
>         ick = rate / (cdf + 1);

In case of FM+, cdf will be zero, and ick == rate?

> @@ -292,34 +324,55 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
>         round = (ick + 500000) / 1000000 * sum;

ick == rate if FM+

>         round = (round + 500) / 1000;

DIV_ROUND_UP()

>
> -       /*
> -        * SCL  = ick / (20 + SCGD * 8 + F[(ticf + tr + intd) * ick])
> -        *
> -        * Calculation result (= SCL) should be less than
> -        * bus_speed for hardware safety
> -        *
> -        * We could use something along the lines of
> -        *      div = ick / (bus_speed + 1) + 1;
> -        *      scgd = (div - 20 - round + 7) / 8;
> -        *      scl = ick / (20 + (scgd * 8) + round);
> -        * (not fully verified) but that would get pretty involved
> -        */
> -       for (scgd = 0; scgd < 0x40; scgd++) {
> -               scl = ick / (20 + (scgd * 8) + round);
> -               if (scl <= t.bus_freq_hz)
> -                       break;
> -       }
> +       if (priv->flags & ID_P_FMPLUS) {

IIUIC, on R-ar Gen3 and later you can use ICCCR2 without FM+, for
improved accuracy, too?

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

DIV_ROUND_UP(ick, 8 * (t.bus_freq_hz - 8 - round));

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

DIV_ROUND_UP()?

>
> -       if (scgd == 0x40) {
> -               dev_err(dev, "it is impossible to calculate best SCL\n");
> -               return -EIO;
> -       }
> +               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+?

> +               }
>
> -       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.

> +                       scl, t.bus_freq_hz, rate, round, smd, 3 * smd);
>
> -       /* keep icccr value */
> -       priv->icccr = scgd << cdf_width | cdf;
> +               priv->clock_val = smd;
> +       } else {
> +               /*
> +                * SCL  = ick / (20 + SCGD * 8 + F[(ticf + tr + intd) * ick])
> +                *
> +                * Calculation result (= SCL) should be less than
> +                * bus_speed for hardware safety
> +                *
> +                * We could use something along the lines of
> +                *      div = ick / (bus_speed + 1) + 1;
> +                *      scgd = (div - 20 - round + 7) / 8;
> +                *      scl = ick / (20 + (scgd * 8) + round);
> +                * (not fully verified) but that would get pretty involved
> +                */
> +               for (scgd = 0; scgd < 0x40; scgd++) {
> +                       scl = ick / (20 + (scgd * 8) + round);
> +                       if (scl <= t.bus_freq_hz)
> +                               break;
> +               }
> +
> +               if (scgd == 0x40) {
> +                       dev_err(dev, "it is impossible to calculate best SCL\n");
> +                       return -EINVAL;

This was -EIO before.


> +               }
> +
> +               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);
> +
> +               priv->clock_val = scgd << cdf_width | cdf;
> +       }
>
>         return 0;
>  }

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



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux