[PATCH RFT 0/2] i2c: rcar: improve Gen3 support

[PATCH RFT 0/2] i2c: rcar: improve Gen3 support

[Wolfram Sang]

Here is the second series paving the way for Gen4 support. This time we
properly apply the Gen3 specific features. See the patch comments for
further information. This has been tested on a Renesas Falcon board with
a R-Car V3U SoC at various bus speeds. Because the calculation formulas
are crucial, testing on board farms would be much appreciated!

Thanks and happy hacking!


Wolfram Sang (2):
  i2c: rcar: reset controller is mandatory for Gen3+
  i2c: rcar: improve accuracy for R-Car Gen3+

 drivers/i2c/busses/i2c-rcar.c | 148 ++++++++++++++++++++++------------
 1 file changed, 98 insertions(+), 50 deletions(-)

-- 
2.35.1

[PATCH RFT 1/2] i2c: rcar: reset controller is mandatory for Gen3+

[Wolfram Sang]

Initially, we only needed a reset controller to make sure RXDMA works at
least once per transfer. Meanwhile, documentation has been updated. It
now says that a reset has to be performed prior every transaction, also
if it is non-DMA. So, make the reset controller a requirement instead of
being optional.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 5e97635faf78..342c3747f415 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -838,12 +838,10 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
 
 	/* Gen3 needs a reset before allowing RXDMA once */
 	if (priv->devtype == I2C_RCAR_GEN3) {
-		priv->flags |= ID_P_NO_RXDMA;
-		if (!IS_ERR(priv->rstc)) {
-			ret = rcar_i2c_do_reset(priv);
-			if (ret == 0)
-				priv->flags &= ~ID_P_NO_RXDMA;
-		}
+		priv->flags &= ~ID_P_NO_RXDMA;
+		ret = rcar_i2c_do_reset(priv);
+		if (ret)
+			priv->flags |= ID_P_NO_RXDMA;
 	}
 
 	rcar_i2c_init(priv);
@@ -1096,11 +1094,13 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 
 	if (priv->devtype == I2C_RCAR_GEN3) {
 		priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
-		if (!IS_ERR(priv->rstc)) {
-			ret = reset_control_status(priv->rstc);
-			if (ret < 0)
-				priv->rstc = ERR_PTR(-ENOTSUPP);
-		}
+		if (IS_ERR(priv->rstc))
+			return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc),
+					     "couldn't get reset");
+
+		ret = reset_control_status(priv->rstc);
+		if (ret < 0)
+			return ret;
 	}
 
 	/* Stay always active when multi-master to keep arbitration working */
-- 
2.35.1

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

[Wolfram Sang]

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(-)

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;
 
-- 
2.35.1

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

[Geert Uytterhoeven]

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

Re: [PATCH RFT 1/2] i2c: rcar: reset controller is mandatory for Gen3+

[Geert Uytterhoeven]

Hi Wolfram,

Thanks for your patch!
On Wed, Sep 13, 2023 at 8:41 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Initially, we only needed a reset controller to make sure RXDMA works at
> least once per transfer. Meanwhile, documentation has been updated. It
> now says that a reset has to be performed prior every transaction, also
> if it is non-DMA. So, make the reset controller a requirement instead of
> being optional.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -838,12 +838,10 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap,
>
>         /* Gen3 needs a reset before allowing RXDMA once */
>         if (priv->devtype == I2C_RCAR_GEN3) {
> -               priv->flags |= ID_P_NO_RXDMA;
> -               if (!IS_ERR(priv->rstc)) {
> -                       ret = rcar_i2c_do_reset(priv);
> -                       if (ret == 0)
> -                               priv->flags &= ~ID_P_NO_RXDMA;
> -               }
> +               priv->flags &= ~ID_P_NO_RXDMA;
> +               ret = rcar_i2c_do_reset(priv);
> +               if (ret)
> +                       priv->flags |= ID_P_NO_RXDMA;

This is pre-existing, but if rcar_i2c_do_reset() returns an error,
that means the I2C block couldn't get out of reset.  Are we sure we
can still do PIO transfers in that case, or should this be considered
a fatal error?

>         }
>
>         rcar_i2c_init(priv);
> @@ -1096,11 +1094,13 @@ static int rcar_i2c_probe(struct platform_device *pdev)
>
>         if (priv->devtype == I2C_RCAR_GEN3) {
>                 priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> -               if (!IS_ERR(priv->rstc)) {
> -                       ret = reset_control_status(priv->rstc);
> -                       if (ret < 0)
> -                               priv->rstc = ERR_PTR(-ENOTSUPP);
> -               }
> +               if (IS_ERR(priv->rstc))
> +                       return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc),
> +                                            "couldn't get reset");
> +
> +               ret = reset_control_status(priv->rstc);
> +               if (ret < 0)
> +                       return ret;

This is a pre-existing check, but do you really need it?
This condition will be true if the reset is still asserted, which
could happen due to some glitch, or force-booting into a new kernel
using kexec.  And AFAIUI, that should be resolved by the call to
rcar_i2c_do_reset() above.

>         }
>
>         /* Stay always active when multi-master to keep arbitration working */

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

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

[Geert Uytterhoeven]

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

Re: [PATCH RFT 1/2] i2c: rcar: reset controller is mandatory for Gen3+

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1317 bytes --]

Hi Geert,

> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks!

> > +               priv->flags &= ~ID_P_NO_RXDMA;
> > +               ret = rcar_i2c_do_reset(priv);
> > +               if (ret)
> > +                       priv->flags |= ID_P_NO_RXDMA;
> 
> This is pre-existing, but if rcar_i2c_do_reset() returns an error,
> that means the I2C block couldn't get out of reset.  Are we sure we
> can still do PIO transfers in that case, or should this be considered
> a fatal error?

Makes sense. I will double check what to do here.

> > +               ret = reset_control_status(priv->rstc);
> > +               if (ret < 0)
> > +                       return ret;
> 
> This is a pre-existing check, but do you really need it?
> This condition will be true if the reset is still asserted, which
> could happen due to some glitch, or force-booting into a new kernel
> using kexec.  And AFAIUI, that should be resolved by the call to
> rcar_i2c_do_reset() above.

This check is needed to ensure reset_control_status() really works
because we need it in rcar_i2c_do_reset(). From the docs:

"reset_control_status - returns a negative errno if not supported,..."

The code only checks for that, not for the status of the reset line.

All the best,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFT 1/2] i2c: rcar: reset controller is mandatory for Gen3+

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, Sep 19, 2023 at 11:19 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > +               ret = reset_control_status(priv->rstc);
> > > +               if (ret < 0)
> > > +                       return ret;
> >
> > This is a pre-existing check, but do you really need it?
> > This condition will be true if the reset is still asserted, which
> > could happen due to some glitch, or force-booting into a new kernel
> > using kexec.  And AFAIUI, that should be resolved by the call to
> > rcar_i2c_do_reset() above.
>
> This check is needed to ensure reset_control_status() really works
> because we need it in rcar_i2c_do_reset(). From the docs:
>
> "reset_control_status - returns a negative errno if not supported,..."
>
> The code only checks for that, not for the status of the reset line.

Right, I missed the negative.
I don't think this can fail on R-Car Gen2+ (using the CPG/MSSR driver)
if devm_reset_control_get_exclusive() succeeded, but it's prudent, in
case the block is every reused on a different SoC family.

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

Re: [PATCH RFT 0/2] i2c: rcar: improve Gen3 support

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Sep 13, 2023 at 12:20 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Here is the second series paving the way for Gen4 support. This time we
> properly apply the Gen3 specific features. See the patch comments for
> further information. This has been tested on a Renesas Falcon board with
> a R-Car V3U SoC at various bus speeds. Because the calculation formulas
> are crucial, testing on board farms would be much appreciated!

Thanks for your series!

This (combined with the FM+ series) seems to work fine on all R-Car
boards I gave it a try on.

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

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

[Geert Uytterhoeven]

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

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Geert Uytterhoeven]

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

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

[Geert Uytterhoeven]

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

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