[PATCH 0/4] i2c: rcar: handle Gen2 and Gen3 V3U quirks better

[PATCH 0/4] i2c: rcar: handle Gen2 and Gen3 V3U quirks better

[Wolfram Sang]

The new V3U SoC needs handling of spurious interrupts which is handled
in patch #4. However, this extra check is bad for Gen2 and earlier SoCs,
so we need seperate interrupts now. While working on this, further
improvements to avoid the HW race condition on Gen2 and earlier have
been found, see patches 1-3.

My measurements have shown that patches 1+2 really improve the
situation. Before, I could see doubled messages after adding 2us of
delay to the interrupt handler. After, they only started to appear after
7us. I can't say much about the spurious interrupts on V3U. The BSP team
experienced them, I did not so far.

Let me know what you think...


Wolfram Sang (4):
  i2c: rcar: faster irq code to minimize HW race condition
  i2c: rcar: optimize cacheline to minimize HW race condition
  i2c: rcar: make sure irq is not threaded on Gen2 and earlier
  i2c: rcar: protect against supurious interrupts on V3U

 drivers/i2c/busses/i2c-rcar.c | 66 +++++++++++++++++++++++++----------
 1 file changed, 48 insertions(+), 18 deletions(-)

-- 
2.28.0

[PATCH 1/4] i2c: rcar: faster irq code to minimize HW race condition

[Wolfram Sang]

To avoid the HW race condition on R-Car Gen2 and earlier, we need to
write to ICMCR as soon as possible in the interrupt handler. We can
improve this by writing a static value instead of masking out bits.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 217def2d7cb4..824586d7ee56 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -91,7 +91,6 @@
 
 #define RCAR_BUS_PHASE_START	(MDBS | MIE | ESG)
 #define RCAR_BUS_PHASE_DATA	(MDBS | MIE)
-#define RCAR_BUS_MASK_DATA	(~(ESG | FSB) & 0xFF)
 #define RCAR_BUS_PHASE_STOP	(MDBS | MIE | FSB)
 
 #define RCAR_IRQ_SEND	(MNR | MAL | MST | MAT | MDE)
@@ -621,7 +620,7 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
 /*
  * This driver has a lock-free design because there are IP cores (at least
  * R-Car Gen2) which have an inherent race condition in their hardware design.
- * There, we need to clear RCAR_BUS_MASK_DATA bits as soon as possible after
+ * There, we need to switch to RCAR_BUS_PHASE_DATA as soon as possible after
  * the interrupt was generated, otherwise an unwanted repeated message gets
  * generated. It turned out that taking a spinlock at the beginning of the ISR
  * was already causing repeated messages. Thus, this driver was converted to
@@ -630,13 +629,11 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
 static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
 {
 	struct rcar_i2c_priv *priv = ptr;
-	u32 msr, val;
+	u32 msr;
 
 	/* Clear START or STOP immediately, except for REPSTART after read */
-	if (likely(!(priv->flags & ID_P_REP_AFTER_RD))) {
-		val = rcar_i2c_read(priv, ICMCR);
-		rcar_i2c_write(priv, ICMCR, val & RCAR_BUS_MASK_DATA);
-	}
+	if (likely(!(priv->flags & ID_P_REP_AFTER_RD)))
+		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
 
 	msr = rcar_i2c_read(priv, ICMSR);
 
-- 
2.28.0

Re: [PATCH 1/4] i2c: rcar: faster irq code to minimize HW race condition

[Wolfram Sang]

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

On Wed, Dec 23, 2020 at 06:21:51PM +0100, Wolfram Sang wrote:
> To avoid the HW race condition on R-Car Gen2 and earlier, we need to
> write to ICMCR as soon as possible in the interrupt handler. We can
> improve this by writing a static value instead of masking out bits.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


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

[PATCH 2/4] i2c: rcar: optimize cacheline to minimize HW race condition

[Wolfram Sang]

'flags' and 'io' are needed first, so they should be at the beginning of
the private struct.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 824586d7ee56..ad6630e3cc77 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -119,6 +119,7 @@ enum rcar_i2c_type {
 };
 
 struct rcar_i2c_priv {
+	u32 flags;
 	void __iomem *io;
 	struct i2c_adapter adap;
 	struct i2c_msg *msg;
@@ -129,7 +130,6 @@ struct rcar_i2c_priv {
 
 	int pos;
 	u32 icccr;
-	u32 flags;
 	u8 recovery_icmcr;	/* protected by adapter lock */
 	enum rcar_i2c_type devtype;
 	struct i2c_client *slave;
-- 
2.28.0

Re: [PATCH 2/4] i2c: rcar: optimize cacheline to minimize HW race condition

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Dec 23, 2020 at 6:22 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> 'flags' and 'io' are needed first, so they should be at the beginning of
> the private struct.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -119,6 +119,7 @@ enum rcar_i2c_type {
>  };
>
>  struct rcar_i2c_priv {
> +       u32 flags;

Note that this adds a hole on 64-bit.

But if this really can make a difference, IMHO it is still broken

>         void __iomem *io;
>         struct i2c_adapter adap;
>         struct i2c_msg *msg;

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/4] i2c: rcar: optimize cacheline to minimize HW race condition

[Wolfram Sang]

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


> But if this really can make a difference, IMHO it is still broken

It *is* broken on Gen2, no way around that. We can only minimze the race
best as we can.


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

Re: [PATCH 2/4] i2c: rcar: optimize cacheline to minimize HW race condition

[Wolfram Sang]

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

On Wed, Dec 23, 2020 at 06:21:52PM +0100, Wolfram Sang wrote:
> 'flags' and 'io' are needed first, so they should be at the beginning of
> the private struct.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


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

[PATCH 3/4] i2c: rcar: make sure irq is not threaded on Gen2 and earlier

[Wolfram Sang]

Ensure this irq runs as fast as possible.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index ad6630e3cc77..3c9c3a6f7ac8 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -928,6 +928,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	struct rcar_i2c_priv *priv;
 	struct i2c_adapter *adap;
 	struct device *dev = &pdev->dev;
+	unsigned long irqflags = 0;
 	int ret;
 
 	/* Otherwise logic will break because some bytes must always use PIO */
@@ -976,6 +977,9 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 
 	rcar_i2c_write(priv, ICSAR, 0); /* Gen2: must be 0 if not using slave */
 
+	if (priv->devtype < I2C_RCAR_GEN3)
+		irqflags |= IRQF_NO_THREAD;
+
 	if (priv->devtype == I2C_RCAR_GEN3) {
 		priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
 		if (!IS_ERR(priv->rstc)) {
@@ -995,7 +999,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 		priv->flags |= ID_P_HOST_NOTIFY;
 
 	priv->irq = platform_get_irq(pdev, 0);
-	ret = devm_request_irq(dev, priv->irq, rcar_i2c_irq, 0, dev_name(dev), priv);
+	ret = devm_request_irq(dev, priv->irq, rcar_i2c_irq, irqflags, dev_name(dev), priv);
 	if (ret < 0) {
 		dev_err(dev, "cannot get irq %d\n", priv->irq);
 		goto out_pm_disable;
-- 
2.28.0

Re: [PATCH 3/4] i2c: rcar: make sure irq is not threaded on Gen2 and earlier

[Wolfram Sang]

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

On Wed, Dec 23, 2020 at 06:21:53PM +0100, Wolfram Sang wrote:
> Ensure this irq runs as fast as possible.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


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

[PATCH 4/4] i2c: rcar: protect against supurious interrupts on V3U

[Wolfram Sang]

V3U creates spurious interrupts which we need to handle. This costs time
until BUS_PHASE_DATA can be activated which is problematic for Gen2 SoCs
and earlier. Because of this we introduce two interrupt handlers here
which will call a generic main irq function once the timing critical
stuff is done.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 3c9c3a6f7ac8..12f6d452c0f7 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -625,20 +625,11 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
  * generated. It turned out that taking a spinlock at the beginning of the ISR
  * was already causing repeated messages. Thus, this driver was converted to
  * the now lockless behaviour. Please keep this in mind when hacking the driver.
+ * R-Car Gen3 seems to have this fixed but earlier versions than R-Car Gen2 are
+ * likely affected. Therefore, we have different interrupt handler entries.
  */
-static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
+static irqreturn_t rcar_i2c_irq(int irq, struct rcar_i2c_priv *priv, u32 msr)
 {
-	struct rcar_i2c_priv *priv = ptr;
-	u32 msr;
-
-	/* Clear START or STOP immediately, except for REPSTART after read */
-	if (likely(!(priv->flags & ID_P_REP_AFTER_RD)))
-		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
-
-	msr = rcar_i2c_read(priv, ICMSR);
-
-	/* Only handle interrupts that are currently enabled */
-	msr &= rcar_i2c_read(priv, ICMIER);
 	if (!msr) {
 		if (rcar_i2c_slave_irq(priv))
 			return IRQ_HANDLED;
@@ -682,6 +673,41 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t rcar_i2c_gen2_irq(int irq, void *ptr)
+{
+	struct rcar_i2c_priv *priv = ptr;
+	u32 msr;
+
+	/* Clear START or STOP immediately, except for REPSTART after read */
+	if (likely(!(priv->flags & ID_P_REP_AFTER_RD)))
+		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
+
+	/* Only handle interrupts that are currently enabled */
+	msr = rcar_i2c_read(priv, ICMSR);
+	msr &= rcar_i2c_read(priv, ICMIER);
+
+	return rcar_i2c_irq(irq, priv, msr);
+}
+
+static irqreturn_t rcar_i2c_gen3_irq(int irq, void *ptr)
+{
+	struct rcar_i2c_priv *priv = ptr;
+	u32 msr;
+
+	/* Only handle interrupts that are currently enabled */
+	msr = rcar_i2c_read(priv, ICMSR);
+	msr &= rcar_i2c_read(priv, ICMIER);
+
+	/*
+	 * Clear START or STOP immediately, except for REPSTART after read or
+	 * if a spurious interrupt was detected.
+	 */
+	if (likely(!(priv->flags & ID_P_REP_AFTER_RD) && msr))
+		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_DATA);
+
+	return rcar_i2c_irq(irq, priv, msr);
+}
+
 static struct dma_chan *rcar_i2c_request_dma_chan(struct device *dev,
 					enum dma_transfer_direction dir,
 					dma_addr_t port_addr)
@@ -929,6 +955,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	struct i2c_adapter *adap;
 	struct device *dev = &pdev->dev;
 	unsigned long irqflags = 0;
+	irqreturn_t (*irqhandler)(int irq, void *ptr) = rcar_i2c_gen3_irq;
 	int ret;
 
 	/* Otherwise logic will break because some bytes must always use PIO */
@@ -977,8 +1004,10 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 
 	rcar_i2c_write(priv, ICSAR, 0); /* Gen2: must be 0 if not using slave */
 
-	if (priv->devtype < I2C_RCAR_GEN3)
+	if (priv->devtype < I2C_RCAR_GEN3) {
 		irqflags |= IRQF_NO_THREAD;
+		irqhandler = rcar_i2c_gen2_irq;
+	}
 
 	if (priv->devtype == I2C_RCAR_GEN3) {
 		priv->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
@@ -999,7 +1028,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 		priv->flags |= ID_P_HOST_NOTIFY;
 
 	priv->irq = platform_get_irq(pdev, 0);
-	ret = devm_request_irq(dev, priv->irq, rcar_i2c_irq, irqflags, dev_name(dev), priv);
+	ret = devm_request_irq(dev, priv->irq, irqhandler, irqflags, dev_name(dev), priv);
 	if (ret < 0) {
 		dev_err(dev, "cannot get irq %d\n", priv->irq);
 		goto out_pm_disable;
-- 
2.28.0

Re: [PATCH 4/4] i2c: rcar: protect against supurious interrupts on V3U

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Dec 23, 2020 at 6:24 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> V3U creates spurious interrupts which we need to handle. This costs time
> until BUS_PHASE_DATA can be activated which is problematic for Gen2 SoCs
> and earlier. Because of this we introduce two interrupt handlers here
> which will call a generic main irq function once the timing critical
> stuff is done.
>
> 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
> @@ -625,20 +625,11 @@ static bool rcar_i2c_slave_irq(struct rcar_i2c_priv *priv)
>   * generated. It turned out that taking a spinlock at the beginning of the ISR
>   * was already causing repeated messages. Thus, this driver was converted to
>   * the now lockless behaviour. Please keep this in mind when hacking the driver.
> + * R-Car Gen3 seems to have this fixed but earlier versions than R-Car Gen2 are

s/than/like/?

> + * likely affected. Therefore, we have different interrupt handler entries.
>   */

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 4/4] i2c: rcar: protect against supurious interrupts on V3U

[Wolfram Sang]

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


> >   * the now lockless behaviour. Please keep this in mind when hacking the driver.
> > + * R-Car Gen3 seems to have this fixed but earlier versions than R-Car Gen2 are
> 
> s/than/like/?

What I meant to say was: We know Gen2 is affected (first sentence of the
paragraph). We know Gen3 is fixed. Earlier than Gen2 is likely affected
but we don't know. So, I'd like to keep 'than'.


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

Re: [PATCH 4/4] i2c: rcar: protect against supurious interrupts on V3U

[Wolfram Sang]

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

On Wed, Dec 23, 2020 at 06:21:54PM +0100, Wolfram Sang wrote:
> V3U creates spurious interrupts which we need to handle. This costs time
> until BUS_PHASE_DATA can be activated which is problematic for Gen2 SoCs
> and earlier. Because of this we introduce two interrupt handlers here
> which will call a generic main irq function once the timing critical
> stuff is done.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-next, thanks!


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

Re: [PATCH 0/4] i2c: rcar: handle Gen2 and Gen3 V3U quirks better

[Niklas Söderlund]

Hi Wolfram,

Thanks for your series.

On 2020-12-23 18:21:50 +0100, Wolfram Sang wrote:
> The new V3U SoC needs handling of spurious interrupts which is handled
> in patch #4. However, this extra check is bad for Gen2 and earlier SoCs,
> so we need seperate interrupts now. While working on this, further
> improvements to avoid the HW race condition on Gen2 and earlier have
> been found, see patches 1-3.
> 
> My measurements have shown that patches 1+2 really improve the
> situation. Before, I could see doubled messages after adding 2us of
> delay to the interrupt handler. After, they only started to appear after
> 7us. I can't say much about the spurious interrupts on V3U. The BSP team
> experienced them, I did not so far.
> 
> Let me know what you think...

I like it, for the whole series

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> 
> 
> Wolfram Sang (4):
>   i2c: rcar: faster irq code to minimize HW race condition
>   i2c: rcar: optimize cacheline to minimize HW race condition
>   i2c: rcar: make sure irq is not threaded on Gen2 and earlier
>   i2c: rcar: protect against supurious interrupts on V3U
> 
>  drivers/i2c/busses/i2c-rcar.c | 66 +++++++++++++++++++++++++----------
>  1 file changed, 48 insertions(+), 18 deletions(-)
> 
> -- 
> 2.28.0
> 

-- 
Regards,
Niklas Söderlund