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

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

[Wolfram Sang]

The newest generation of Renesas R-Car SoCs support FastMode+. This
series enables the driver to use it. After a cleanup (patch 1) and
adding the Gen4 devtype (patch 2), actual FM+ support gets added in
patch 3. Tested on a Falcon board with a R-Car V3U. Getting >16KB of
data from the PMIC was pretty much 2.5x faster than without FM+ which
pretty much matches the theoretical values. Actual scoping still needs
to be done as it needs some logistics because of the board being remote.
But here the patches already for review.

Note: I intend to remove the brute-force algorithm from the regular
clock calculation as well. This will be a separate series, though,
because more cleanups are possible.

Thanks and happy hacking!


Wolfram Sang (3):
  i2c: rcar: avoid non-standard use of goto
  i2c: rcar: introduce Gen4 devices
  i2c: rcar: add FastMode+ support

 drivers/i2c/busses/i2c-rcar.c | 142 +++++++++++++++++++++++-----------
 1 file changed, 95 insertions(+), 47 deletions(-)

-- 
2.35.1

[PATCH 1/3] i2c: rcar: avoid non-standard use of goto

[Wolfram Sang]

Kernel functions goto somewhere on error conditions. Using goto for the
default path is irritating. Let's bail out on error instead.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 2d9c37410ebd..f2b953df0c4d 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -317,12 +317,14 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
 	for (scgd = 0; scgd < 0x40; scgd++) {
 		scl = ick / (20 + (scgd * 8) + round);
 		if (scl <= t.bus_freq_hz)
-			goto scgd_find;
+			break;
+	}
+
+	if (scgd == 0x40) {
+		dev_err(dev, "it is impossible to calculate best SCL\n");
+		return -EIO;
 	}
-	dev_err(dev, "it is impossible to calculate best SCL\n");
-	return -EIO;
 
-scgd_find:
 	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);
 
-- 
2.35.1

[PATCH 2/3] i2c: rcar: introduce Gen4 devices

[Wolfram Sang]

So far, we treated Gen4 as Gen3. But we are soon adding FM+ as a Gen4
specific feature, so prepare the code for the new devtype.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index f2b953df0c4d..76aa16bf17b2 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -114,6 +114,7 @@ enum rcar_i2c_type {
 	I2C_RCAR_GEN1,
 	I2C_RCAR_GEN2,
 	I2C_RCAR_GEN3,
+	I2C_RCAR_GEN4,
 };
 
 struct rcar_i2c_priv {
@@ -218,7 +219,7 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
 	/* 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, ICFBSCR, TCYC17);
 
 }
@@ -251,22 +252,11 @@ 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);
 
-	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
@@ -1031,10 +1021,12 @@ static const struct of_device_id rcar_i2c_dt_ids[] = {
 	{ .compatible = "renesas,i2c-r8a7794", .data = (void *)I2C_RCAR_GEN2 },
 	{ .compatible = "renesas,i2c-r8a7795", .data = (void *)I2C_RCAR_GEN3 },
 	{ .compatible = "renesas,i2c-r8a7796", .data = (void *)I2C_RCAR_GEN3 },
+	/* S4 has no FM+ bit */
+	{ .compatible = "renesas,i2c-r8a779f0", .data = (void *)I2C_RCAR_GEN3 },
 	{ .compatible = "renesas,rcar-gen1-i2c", .data = (void *)I2C_RCAR_GEN1 },
 	{ .compatible = "renesas,rcar-gen2-i2c", .data = (void *)I2C_RCAR_GEN2 },
 	{ .compatible = "renesas,rcar-gen3-i2c", .data = (void *)I2C_RCAR_GEN3 },
-	{ .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN3 },
+	{ .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN4 },
 	{},
 };
 MODULE_DEVICE_TABLE(of, rcar_i2c_dt_ids);
@@ -1101,6 +1093,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 		irqhandler = rcar_i2c_gen2_irq;
 	}
 
+	/* Gen3 needs reset for RXDMA */
 	if (priv->devtype == I2C_RCAR_GEN3) {
 		priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
 		if (!IS_ERR(priv->rstc)) {
-- 
2.35.1

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

[Wolfram Sang]

Apply the different formula and register setting for activating FM+ on
Gen4 devtypes.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 76aa16bf17b2..a48849b393a3 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,6 +88,12 @@
 #define RMDMAE	BIT(1)	/* DMA Master Received Enable */
 #define TMDMAE	BIT(0)	/* DMA Master Transmitted Enable */
 
+/* ICCCR2 */
+#define FMPE	BIT(7)	/* Fast Mode Plus Enable */
+#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 */
 
@@ -104,11 +114,12 @@
 #define ID_NACK			BIT(4)
 #define ID_EPROTO		BIT(5)
 /* persistent flags */
+#define ID_P_FMPLUS		BIT(27)
 #define ID_P_NOT_ATOMIC		BIT(28)
 #define ID_P_HOST_NOTIFY	BIT(29)
 #define ID_P_NO_RXDMA		BIT(30) /* HW forbids RXDMA sometimes */
 #define ID_P_PM_BLOCKED		BIT(31)
-#define ID_P_MASK		GENMASK(31, 28)
+#define ID_P_MASK		GENMASK(31, 27)
 
 enum rcar_i2c_type {
 	I2C_RCAR_GEN1,
@@ -128,7 +139,7 @@ struct rcar_i2c_priv {
 	wait_queue_head_t wait;
 
 	int pos;
-	u32 icccr;
+	u32 clock_val;
 	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);
+	} else {
+		rcar_i2c_write(priv, ICCCR, priv->clock_val);
+		if (priv->devtype >= I2C_RCAR_GEN3)
+			rcar_i2c_write(priv, ICCCR2, 0);
+	}
 
 	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);
 
@@ -292,34 +324,55 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
 	round = (ick + 500000) / 1000000 * sum;
 	round = (round + 500) / 1000;
 
-	/*
-	 * 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) {
+		/*
+		 * 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);
+		scl = ick / (8 + 8 * smd + round);
 
-	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;
+		}
 
-	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",
+			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;
+		}
+
+		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;
 }
-- 
2.35.1

Re: [PATCH 1/3] i2c: rcar: avoid non-standard use of goto

[Andi Shyti]

Hi Wolfram,

On Mon, Sep 04, 2023 at 03:58:49PM +0200, Wolfram Sang wrote:
> Kernel functions goto somewhere on error conditions. Using goto for the
> default path is irritating. Let's bail out on error instead.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

Re: [PATCH 2/3] i2c: rcar: introduce Gen4 devices

[Andi Shyti]

Hi Wolfram,

> @@ -1031,10 +1021,12 @@ static const struct of_device_id rcar_i2c_dt_ids[] = {
>  	{ .compatible = "renesas,i2c-r8a7794", .data = (void *)I2C_RCAR_GEN2 },
>  	{ .compatible = "renesas,i2c-r8a7795", .data = (void *)I2C_RCAR_GEN3 },
>  	{ .compatible = "renesas,i2c-r8a7796", .data = (void *)I2C_RCAR_GEN3 },
> +	/* S4 has no FM+ bit */
> +	{ .compatible = "renesas,i2c-r8a779f0", .data = (void *)I2C_RCAR_GEN3 },

is this I2C_RCAR_GEN3 or I2C_RCAR_GEN4?

Rest looks good.

Andi

>  	{ .compatible = "renesas,rcar-gen1-i2c", .data = (void *)I2C_RCAR_GEN1 },
>  	{ .compatible = "renesas,rcar-gen2-i2c", .data = (void *)I2C_RCAR_GEN2 },
>  	{ .compatible = "renesas,rcar-gen3-i2c", .data = (void *)I2C_RCAR_GEN3 },
> -	{ .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN3 },
> +	{ .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN4 },
>  	{},

Re: [PATCH 2/3] i2c: rcar: introduce Gen4 devices

[Wolfram Sang]

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

On Tue, Sep 05, 2023 at 01:36:24PM +0200, Andi Shyti wrote:
> Hi Wolfram,
> 
> > @@ -1031,10 +1021,12 @@ static const struct of_device_id rcar_i2c_dt_ids[] = {
> >  	{ .compatible = "renesas,i2c-r8a7794", .data = (void *)I2C_RCAR_GEN2 },
> >  	{ .compatible = "renesas,i2c-r8a7795", .data = (void *)I2C_RCAR_GEN3 },
> >  	{ .compatible = "renesas,i2c-r8a7796", .data = (void *)I2C_RCAR_GEN3 },
> > +	/* S4 has no FM+ bit */
> > +	{ .compatible = "renesas,i2c-r8a779f0", .data = (void *)I2C_RCAR_GEN3 },
> 
> is this I2C_RCAR_GEN3 or I2C_RCAR_GEN4?

Technically, it is Gen4. But its I2C controller behaves like Gen3. This
is why it has a seperate entry to avoid the generic Gen4 fallback...

> > -	{ .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN3 },
> > +	{ .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN4 },

... here.


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

Re: [PATCH 2/3] i2c: rcar: introduce Gen4 devices

[Andi Shyti]

Hi Wolfram,

> > >  	{ .compatible = "renesas,i2c-r8a7794", .data = (void *)I2C_RCAR_GEN2 },
> > >  	{ .compatible = "renesas,i2c-r8a7795", .data = (void *)I2C_RCAR_GEN3 },
> > >  	{ .compatible = "renesas,i2c-r8a7796", .data = (void *)I2C_RCAR_GEN3 },
> > > +	/* S4 has no FM+ bit */
> > > +	{ .compatible = "renesas,i2c-r8a779f0", .data = (void *)I2C_RCAR_GEN3 },
> > 
> > is this I2C_RCAR_GEN3 or I2C_RCAR_GEN4?
> 
> Technically, it is Gen4. But its I2C controller behaves like Gen3. This
> is why it has a seperate entry to avoid the generic Gen4 fallback...
> 
> > > -	{ .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN3 },
> > > +	{ .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN4 },
> 
> ... here.

got it... I just wanted to be sure... thanks!

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

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

[Andi Shyti]

Hi Wolfram,

[...]

> @@ -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);
> +	} else {
> +		rcar_i2c_write(priv, ICCCR, priv->clock_val);
> +		if (priv->devtype >= I2C_RCAR_GEN3)
> +			rcar_i2c_write(priv, ICCCR2, 0);

is this last bit part of the FM+ enabling or is it part of the
GEN4 support?

> +	}

[...]

> +	if (priv->flags & ID_P_FMPLUS) {
> +		/*
> +		 * 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);
> +		scl = ick / (8 + 8 * smd + round);
>  
> -	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;
> +		}
>  
> -	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",
> +			scl, t.bus_freq_hz, rate, round, smd, 3 * smd);

I trust the formula application is right here... I can't say much :)
I don't see anything odd here.

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

would be nice to give a meaning to this 0x40 constant... either
having it in a define or a comment, at least.

Andi

> +			dev_err(dev, "it is impossible to calculate best SCL\n");
> +			return -EINVAL;
> +		}

Re: [PATCH 1/3] i2c: rcar: avoid non-standard use of goto

[Geert Uytterhoeven]

On Tue, Sep 5, 2023 at 6:22 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> Kernel functions goto somewhere on error conditions. Using goto for the
> default path is irritating. Let's bail out on error instead.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

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 3/3] i2c: rcar: add FastMode+ support

[Wolfram Sang]

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

Hi Andi,

> > @@ -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);
> > +	} else {
> > +		rcar_i2c_write(priv, ICCCR, priv->clock_val);
> > +		if (priv->devtype >= I2C_RCAR_GEN3)
> > +			rcar_i2c_write(priv, ICCCR2, 0);
> 
> is this last bit part of the FM+ enabling or is it part of the
> GEN4 support?

It is "disabling FM+" for lower speeds. Since we never used ICCCR2
before FM+, we need to make sure it is cleared properly.

> > +		for (scgd = 0; scgd < 0x40; scgd++) {
> > +			scl = ick / (20 + (scgd * 8) + round);
> > +			if (scl <= t.bus_freq_hz)
> > +				break;
> > +		}
> > +
> > +		if (scgd == 0x40) {
> 
> would be nice to give a meaning to this 0x40 constant... either
> having it in a define or a comment, at least.

This code existed before and was just moved into an if-body. It will be
updated in another series following this one.

Thanks for the review,

   Wolfram


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

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

[Andi Shyti]

Hi Wolfram,

> > > @@ -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);
> > > +	} else {
> > > +		rcar_i2c_write(priv, ICCCR, priv->clock_val);
> > > +		if (priv->devtype >= I2C_RCAR_GEN3)
> > > +			rcar_i2c_write(priv, ICCCR2, 0);
> > 
> > is this last bit part of the FM+ enabling or is it part of the
> > GEN4 support?
> 
> It is "disabling FM+" for lower speeds. Since we never used ICCCR2
> before FM+, we need to make sure it is cleared properly.

OK... I'm missing some hardware details here :)

> > > +		for (scgd = 0; scgd < 0x40; scgd++) {
> > > +			scl = ick / (20 + (scgd * 8) + round);
> > > +			if (scl <= t.bus_freq_hz)
> > > +				break;
> > > +		}
> > > +
> > > +		if (scgd == 0x40) {
> > 
> > would be nice to give a meaning to this 0x40 constant... either
> > having it in a define or a comment, at least.
> 
> This code existed before and was just moved into an if-body. It will be
> updated in another series following this one.

OK, thanks!

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

Re: [PATCH 2/3] i2c: rcar: introduce Gen4 devices

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, Sep 4, 2023 at 3:59 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> So far, we treated Gen4 as Gen3. But we are soon adding FM+ as a Gen4
> specific feature, so prepare the code for the new devtype.
>
> 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

> @@ -218,7 +219,7 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
>         /* 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, ICFBSCR, TCYC17);

Note that R-Car Gen4 (incl. R-Car S4) has ICFBSCR bits related to
Slave Clock Stretch Select (which is not yet supported by the driver).

> @@ -1031,10 +1021,12 @@ static const struct of_device_id rcar_i2c_dt_ids[] = {
>         { .compatible = "renesas,i2c-r8a7794", .data = (void *)I2C_RCAR_GEN2 },
>         { .compatible = "renesas,i2c-r8a7795", .data = (void *)I2C_RCAR_GEN3 },
>         { .compatible = "renesas,i2c-r8a7796", .data = (void *)I2C_RCAR_GEN3 },
> +       /* S4 has no FM+ bit */
> +       { .compatible = "renesas,i2c-r8a779f0", .data = (void *)I2C_RCAR_GEN3 },
>         { .compatible = "renesas,rcar-gen1-i2c", .data = (void *)I2C_RCAR_GEN1 },
>         { .compatible = "renesas,rcar-gen2-i2c", .data = (void *)I2C_RCAR_GEN2 },
>         { .compatible = "renesas,rcar-gen3-i2c", .data = (void *)I2C_RCAR_GEN3 },
> -       { .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN3 },
> +       { .compatible = "renesas,rcar-gen4-i2c", .data = (void *)I2C_RCAR_GEN4 },
>         {},
>  };
>  MODULE_DEVICE_TABLE(of, rcar_i2c_dt_ids);
> @@ -1101,6 +1093,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
>                 irqhandler = rcar_i2c_gen2_irq;
>         }
>
> +       /* Gen3 needs reset for RXDMA */
>         if (priv->devtype == I2C_RCAR_GEN3) {

According to the Programming Examples in the docs for R-Car Gen3,
R-Car V3U, S4-8, and V4H, I2C must be reset "at the beginning of
transmission and reception procedure", so not only for DMA.

>                 priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
>                 if (!IS_ERR(priv->rstc)) {

Also, you didn't the touch the checks in rcar_i2c_cleanup_dma():

    /* Gen3 can only do one RXDMA per transfer and we just completed it */
    if (priv->devtype == I2C_RCAR_GEN3 && ...) ...

and rcar_i2c_master_xfer():

    /* Gen3 needs a reset before allowing RXDMA once */
    if (priv->devtype == I2C_RCAR_GEN3) {
            ...
    }

Don't these apply to R-Car Gen4? I can't easily find where this quirk
is documented (perhaps just as a commit in the BSP?), but at least the
"Usage note for DMA mode of Receive Operation" looks identical for
R-Car Gen3 and for the various R-Car Gen4 variants.

So either:
  1. These checks must be updated, too, or
  2. The commit description must explain why this is not needed, as
     switching to I2C_RCAR_GEN4 changes behavior for R-Car Gen4 SoCs
     using the family-specific fallback.

BTW, depending on the answers to my questions above, you may want to
replace the rcar_i2c_type enum by a feature mask...

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 2/3] i2c: rcar: introduce Gen4 devices

[Wolfram Sang]

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

Hi Geert,

thank you for the review!

> Note that R-Car Gen4 (incl. R-Car S4) has ICFBSCR bits related to
> Slave Clock Stretch Select (which is not yet supported by the driver).

Thanks for the heads up. I'd need more information about the use case of
these bits. Seperate task.

> According to the Programming Examples in the docs for R-Car Gen3,
> R-Car V3U, S4-8, and V4H, I2C must be reset "at the beginning of
> transmission and reception procedure", so not only for DMA.

Sadly, this is vague. If you look at the example for a combined
write-then-read transfer, then you see that only one reset is done,

i.e.: reset -> write -> rep_start -> read

That would mean that we don't need a reset per read/write message of a
transfer. But a reset per transfer then? I would wonder why because we
could also have a super long transfer with lots of read/write messages
in it. Do we need a reset then inbetween? Or is it really dependant on
the STOP bit being transferred? I guess these are all questions for the
HW team, though.

I was reluctant to add the reset too often because my measurements back
then showed that it costs around 5us every time. Annoying. Maybe I
should take it easy and follow the documentation. But then I am still
not sure if a large transfer with way more than two messages are OK
without reset? I will ask the HW team.

> Also, you didn't the touch the checks in rcar_i2c_cleanup_dma():
...
> and rcar_i2c_master_xfer():
...
> 
> Don't these apply to R-Car Gen4? I can't easily find where this quirk
> is documented (perhaps just as a commit in the BSP?), but at least the
> "Usage note for DMA mode of Receive Operation" looks identical for
> R-Car Gen3 and for the various R-Car Gen4 variants.

My memory played a trick on me here. I asked Shimoda-san about this
issue on Gen4. I thought I got an answer that it was fixed, so I left
the code Gen3 only. But he actually never got a reply and I forgot to
ping about it.

The latest documentation has now a "usage note for DMA mode" about it
implying that the issue is still present on Gen4 :(

> BTW, depending on the answers to my questions above, you may want to
> replace the rcar_i2c_type enum by a feature mask...

That might be an option. I need to reshuffle my I2C patches first,
though. I'll send some cleanups first to have them out of the way. Then,
I will respin Gen4 support and take care of the DMA RX issue and the new
reset handling there. Thank you for your input!

All the best,

   Wolfram


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

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

[Geert Uytterhoeven]

Hi Wolfram,

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

> @@ -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@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 3/3] i2c: rcar: add FastMode+ support

[Wolfram Sang]

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

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


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

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

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Sep 6, 2023 at 2:11 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > >         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?

No, just pointing it out, and wondering if this is intended...

>
> > >         round = (round + 500) / 1000;
> >
> > DIV_ROUND_UP()
>
> DIV_ROUND_CLOSEST() I'd say, but I have a seperate patch for that.

Oops (it's too hot here for more coffee...)

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

Probably OK.

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

Oops (again)

OK do you need rounding for the division of ick and t.bus_freq_hz,
or is the adjustment bij "- (round + 8)" already taking care of that?
I guess I just don't understand the intended formula here...

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 3/3] i2c: rcar: add FastMode+ support

[Wolfram Sang]

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


> OK do you need rounding for the division of ick and t.bus_freq_hz,
> or is the adjustment bij "- (round + 8)" already taking care of that?

Unless I overlooked something, it is the latter. The new formula
produces the same values for 100 and 400kHz as the old one.

> I guess I just don't understand the intended formula here...

Ok, needs more documentation then.


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

Re: [PATCH 2/3] i2c: rcar: introduce Gen4 devices

[Wolfram Sang]

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


> I was reluctant to add the reset too often because my measurements back
> then showed that it costs around 5us every time. Annoying. Maybe I
> should take it easy and follow the documentation. But then I am still
> not sure if a large transfer with way more than two messages are OK
> without reset? I will ask the HW team.

Stupid me, we are following the documentation. Except that we treat the
reset property as optional while it should be mandatory for >= Gen3.


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

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

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, Sep 7, 2023 at 7:39 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > > @@ -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);
> > > +   } else {
> > > +           rcar_i2c_write(priv, ICCCR, priv->clock_val);
> > > +           if (priv->devtype >= I2C_RCAR_GEN3)
> > > +                   rcar_i2c_write(priv, ICCCR2, 0);
> >
> > is this last bit part of the FM+ enabling or is it part of the
> > GEN4 support?
>
> It is "disabling FM+" for lower speeds. Since we never used ICCCR2
> before FM+, we need to make sure it is cleared properly.

This may become clearer if you first introduce support for ICCCR2
on R-Car Gen3 and later, to improve i2c rate accuracy, and add
support for FM+ in a separate patch?

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 3/3] i2c: rcar: add FastMode+ support

[Wolfram Sang]

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


> This may become clearer if you first introduce support for ICCCR2
> on R-Car Gen3 and later, to improve i2c rate accuracy, and add
> support for FM+ in a separate patch?

That's my plan for the next revision of this series.


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