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
