diff mbox series

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

Message ID 20230913062950.4968-3-wsa+renesas@sang-engineering.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series i2c: rcar: improve Gen3 support | expand

Commit Message

Wolfram Sang Sept. 13, 2023, 6:29 a.m. UTC
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@sang-engineering.com>
---

The base for this patch was a BSP patch adding FM+ support. However,
this clock calculation is significantly different:

* it calculates the divider instead of trying values until it fits
* as there was no information about reasonable SMD calculation, it uses
  a fixed value suggested from recommended settings in the docs
* the SCL low/high ratio is 5:4 instead of 1:1. Otherwise the specs for
  Fastmode are slightly not met
* it doesn't use soc_device_match

 drivers/i2c/busses/i2c-rcar.c | 126 +++++++++++++++++++++++-----------
 1 file changed, 87 insertions(+), 39 deletions(-)

Comments

Geert Uytterhoeven Sept. 18, 2023, 2:44 p.m. UTC | #1
Hi Wolfram,

On Wed, Sep 13, 2023 at 11:38 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> 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@sang-engineering.com>

Thanks for your patch!

> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c

> @@ -84,11 +88,25 @@
>  #define RMDMAE BIT(1)  /* DMA Master Received Enable */
>  #define TMDMAE BIT(0)  /* DMA Master Transmitted Enable */
>
> +/* ICCCR2 */
> +#define CDFD   BIT(2)  /* CDF Disable */
> +#define HLSE   BIT(1)  /* HIGH/LOW Separate Control Enable */
> +#define SME    BIT(0)  /* SCL Mask Enable */
> +
>  /* ICFBSCR */
>  #define TCYC17 0x0f            /* 17*Tcyc delay 1st bit between SDA and SCL */
>
>  #define RCAR_MIN_DMA_LEN       8
>
> +/* SCL low/high ratio 5:4 to meet all I2C timing specs (incl safety margin) */
> +#define RCAR_SCLD_RATIO                5
> +#define RCAR_SCHD_RATIO                4
> +/*
> + * SMD should be smaller than SCLD/SCHD and is always around 20 in the docs.
> + * Thus, we simply use 20 which works for low and high speeds.
> +*/

(checkpatch) WARNING: Block comments should align the * on each line

> +#define RCAR_DEFAULT_SMD       20
> +
>  #define RCAR_BUS_PHASE_START   (MDBS | MIE | ESG)
>  #define RCAR_BUS_PHASE_DATA    (MDBS | MIE)
>  #define RCAR_BUS_PHASE_STOP    (MDBS | MIE | FSB)

> @@ -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[...])

(checkpatch) WARNING: please, no space before tabs

I hope to give this a full test-run on my farm soon...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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
Geert Uytterhoeven Sept. 19, 2023, 9:14 a.m. UTC | #2
Hi Wolfram,

On Wed, Sep 13, 2023 at 11:38 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> 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@sang-engineering.com>

Thanks for your patch!

> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -84,11 +88,25 @@
>  #define RMDMAE BIT(1)  /* DMA Master Received Enable */
>  #define TMDMAE BIT(0)  /* DMA Master Transmitted Enable */
>
> +/* ICCCR2 */
> +#define CDFD   BIT(2)  /* CDF Disable */
> +#define HLSE   BIT(1)  /* HIGH/LOW Separate Control Enable */
> +#define SME    BIT(0)  /* SCL Mask Enable */
> +
>  /* ICFBSCR */
>  #define TCYC17 0x0f            /* 17*Tcyc delay 1st bit between SDA and SCL */
>
>  #define RCAR_MIN_DMA_LEN       8
>
> +/* SCL low/high ratio 5:4 to meet all I2C timing specs (incl safety margin) */
> +#define RCAR_SCLD_RATIO                5
> +#define RCAR_SCHD_RATIO                4
> +/*
> + * SMD should be smaller than SCLD/SCHD and is always around 20 in the docs.
> + * Thus, we simply use 20 which works for low and high speeds.
> +*/
> +#define RCAR_DEFAULT_SMD       20
> +
>  #define RCAR_BUS_PHASE_START   (MDBS | MIE | ESG)
>  #define RCAR_BUS_PHASE_DATA    (MDBS | MIE)
>  #define RCAR_BUS_PHASE_STOP    (MDBS | MIE | FSB)
> @@ -128,6 +146,7 @@ struct rcar_i2c_priv {
>
>         int pos;
>         u32 icccr;
> +       u32 scl_gran;

You may want to store just u16 schd and scld instead, so you don't
have to calculate them over and over again in rcar_i2c_init().
They are calculated in rcar_i2c_clock_calculate() (called from .probe())
anyway to validate ranges.

That would also avoid the need to come up with a better name for
scl_gran ;-)

>         u8 recovery_icmcr;      /* protected by adapter lock */
>         enum rcar_i2c_type devtype;
>         struct i2c_client *slave;
> @@ -216,11 +235,16 @@ 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->devtype == I2C_RCAR_GEN3)
> +       if (priv->devtype < I2C_RCAR_GEN3) {
> +               rcar_i2c_write(priv, ICCCR, priv->icccr);
> +       } else {
> +               rcar_i2c_write(priv, ICCCR2, CDFD | HLSE | SME);
> +               rcar_i2c_write(priv, ICCCR, priv->icccr);
> +               rcar_i2c_write(priv, ICMPR, RCAR_DEFAULT_SMD);
> +               rcar_i2c_write(priv, ICHPR, RCAR_SCHD_RATIO * priv->scl_gran);
> +               rcar_i2c_write(priv, ICLPR, RCAR_SCLD_RATIO * priv->scl_gran);
>                 rcar_i2c_write(priv, ICFBSCR, TCYC17);
> -
> +       }
>  }
>
>  static int rcar_i2c_bus_barrier(struct rcar_i2c_priv *priv)

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

> +                       goto err_no_val;
> +
> +               dev_dbg(dev, "clk %u/%u(%lu), round %u, CDF: %u SCL gran %u\n",
> +                       scl, t.bus_freq_hz, rate, round, cdf, x);
> +
> +               priv->icccr = cdf;
> +               priv->scl_gran = x;
> +       }
>
>         return 0;

The rest LGTM.

BTW, Note 2 in the docs for the Clock Control Register 2 (ICCCR2) seems
to be incorrect:

    SCLD: I2C SCL low clock period (set by the SCL HIGH control register)
                  ^^^                              ^^^^
    SCHD: I2C SCL high clock period (set by the SCL LOW control register)
                  ^^^^                              ^^^

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven Sept. 19, 2023, 9:32 a.m. UTC | #3
Hi Wolfram,

On Tue, Sep 19, 2023 at 11:14 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Sep 13, 2023 at 11:38 AM Wolfram Sang
> <wsa+renesas@sang-engineering.com> 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@sang-engineering.com>
>
> 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@glider.be>

Gr{oetje,eeting}s,

                        Geert
Wolfram Sang Sept. 19, 2023, 9:36 a.m. UTC | #4
Hi Geert,

> > +       u32 scl_gran;
> 
> You may want to store just u16 schd and scld instead, so you don't
> have to calculate them over and over again in rcar_i2c_init().
> They are calculated in rcar_i2c_clock_calculate() (called from .probe())
> anyway to validate ranges.
> 
> That would also avoid the need to come up with a better name for
> scl_gran ;-)

I like that idea! Will do.

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

Seems you clarified this on your own.

I will resend this series with the u16 variables and the cosmetic issues
fixed. And figure out how this slipped through my checkpatch hooks...

Thanks for the review!

   Wolfram
Geert Uytterhoeven Sept. 19, 2023, 9:42 a.m. UTC | #5
Hi Wolfram,

On Wed, Sep 13, 2023 at 11:38 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> 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@sang-engineering.com>

> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c

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

Woops, I missed that both "SCL / clkp" above should be "clkp / SCL".

Fortunately I noticed your "[PATCH 2/2] i2c: rcar: add FastMode+
support for Gen4" fixed that, but I guess it's better to fix that in
this patch instead.

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven Sept. 19, 2023, 9:44 a.m. UTC | #6
On Tue, Sep 19, 2023 at 11:42 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Sep 13, 2023 at 11:38 AM Wolfram Sang
> <wsa+renesas@sang-engineering.com> 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@sang-engineering.com>
>
> > --- a/drivers/i2c/busses/i2c-rcar.c
> > +++ b/drivers/i2c/busses/i2c-rcar.c
>
> > -       /* 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
>
> Woops, I missed that both "SCL / clkp" above should be "clkp / SCL".
>
> Fortunately I noticed your "[PATCH 2/2] i2c: rcar: add FastMode+
> support for Gen4" fixed that, but I guess it's better to fix that in
> this patch instead.

And while at it, please move the "2 *" to replace "smd" by "2 * smd", to
match the docs, and the result after FM+?

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 342c3747f415..bc5c7a0050eb 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -41,6 +41,10 @@ 
 #define ICSAR	0x1C	/* slave address */
 #define ICMAR	0x20	/* master address */
 #define ICRXTX	0x24	/* data port */
+#define ICCCR2	0x28	/* Clock control 2 */
+#define ICMPR	0x2C	/* SCL mask control */
+#define ICHPR	0x30	/* SCL HIGH control */
+#define ICLPR	0x34	/* SCL LOW control */
 #define ICFBSCR	0x38	/* first bit setup cycle (Gen3) */
 #define ICDMAER	0x3c	/* DMA enable (Gen3) */
 
@@ -84,11 +88,25 @@ 
 #define RMDMAE	BIT(1)	/* DMA Master Received Enable */
 #define TMDMAE	BIT(0)	/* DMA Master Transmitted Enable */
 
+/* ICCCR2 */
+#define CDFD	BIT(2)	/* CDF Disable */
+#define HLSE	BIT(1)	/* HIGH/LOW Separate Control Enable */
+#define SME	BIT(0)	/* SCL Mask Enable */
+
 /* ICFBSCR */
 #define TCYC17	0x0f		/* 17*Tcyc delay 1st bit between SDA and SCL */
 
 #define RCAR_MIN_DMA_LEN	8
 
+/* SCL low/high ratio 5:4 to meet all I2C timing specs (incl safety margin) */
+#define RCAR_SCLD_RATIO		5
+#define RCAR_SCHD_RATIO		4
+/*
+ * SMD should be smaller than SCLD/SCHD and is always around 20 in the docs.
+ * Thus, we simply use 20 which works for low and high speeds.
+*/
+#define RCAR_DEFAULT_SMD	20
+
 #define RCAR_BUS_PHASE_START	(MDBS | MIE | ESG)
 #define RCAR_BUS_PHASE_DATA	(MDBS | MIE)
 #define RCAR_BUS_PHASE_STOP	(MDBS | MIE | FSB)
@@ -128,6 +146,7 @@  struct rcar_i2c_priv {
 
 	int pos;
 	u32 icccr;
+	u32 scl_gran;
 	u8 recovery_icmcr;	/* protected by adapter lock */
 	enum rcar_i2c_type devtype;
 	struct i2c_client *slave;
@@ -216,11 +235,16 @@  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->devtype == I2C_RCAR_GEN3)
+	if (priv->devtype < I2C_RCAR_GEN3) {
+		rcar_i2c_write(priv, ICCCR, priv->icccr);
+	} else {
+		rcar_i2c_write(priv, ICCCR2, CDFD | HLSE | SME);
+		rcar_i2c_write(priv, ICCCR, priv->icccr);
+		rcar_i2c_write(priv, ICMPR, RCAR_DEFAULT_SMD);
+		rcar_i2c_write(priv, ICHPR, RCAR_SCHD_RATIO * priv->scl_gran);
+		rcar_i2c_write(priv, ICLPR, RCAR_SCLD_RATIO * priv->scl_gran);
 		rcar_i2c_write(priv, ICFBSCR, TCYC17);
-
+	}
 }
 
 static int rcar_i2c_bus_barrier(struct rcar_i2c_priv *priv)
@@ -241,7 +265,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 cdf, round, ick, sum, scl, cdf_width;
 	unsigned long rate;
 	struct device *dev = rcar_i2c_priv_to_dev(priv);
 	struct i2c_timings t = {
@@ -254,27 +278,17 @@  static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
 	/* Fall back to previously used values if not supplied */
 	i2c_parse_fw_timings(dev, &t, false);
 
-	switch (priv->devtype) {
-	case I2C_RCAR_GEN1:
-		cdf_width = 2;
-		break;
-	case I2C_RCAR_GEN2:
-	case I2C_RCAR_GEN3:
-		cdf_width = 3;
-		break;
-	default:
-		dev_err(dev, "device type error\n");
-		return -EIO;
-	}
-
 	/*
 	 * calculate SCL clock
 	 * see
-	 *	ICCCR
+	 *	ICCCR (and ICCCR2 for Gen3+)
 	 *
 	 * ick	= clkp / (1 + CDF)
 	 * SCL	= ick / (20 + SCGD * 8 + F[(ticf + tr + intd) * ick])
 	 *
+	 * for Gen3+:
+	 * 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
@@ -284,11 +298,12 @@  static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
 	 */
 	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;
-	}
-	ick = rate / (cdf + 1);
+	cdf_width = (priv->devtype == I2C_RCAR_GEN1) ? 2 : 3;
+	if (cdf >= 1U << cdf_width)
+		goto err_no_val;
+
+	/* On Gen3+, we use cdf only for the filters, not as a SCL divider */
+	ick = rate / (priv->devtype < I2C_RCAR_GEN3 ? (cdf + 1) : 1);
 
 	/*
 	 * It is impossible to calculate a large scale number on u32. Separate it.
@@ -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)
+			goto err_no_val;
+
+		dev_dbg(dev, "clk %u/%u(%lu), round %u, CDF: %u SCL gran %u\n",
+			scl, t.bus_freq_hz, rate, round, cdf, x);
+
+		priv->icccr = cdf;
+		priv->scl_gran = x;
+	}
 
 	return 0;