diff mbox series

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

Message ID 20201223172154.34462-5-wsa+renesas@sang-engineering.com
State Accepted
Commit 9c975c432bc0aa53a90438fc80b369cb35134a48
Headers show
Series i2c: rcar: handle Gen2 and Gen3 V3U quirks better | expand

Commit Message

Wolfram Sang Dec. 23, 2020, 5:21 p.m. UTC
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(-)

Comments

Geert Uytterhoeven Dec. 28, 2020, 1:37 p.m. UTC | #1
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
Wolfram Sang Dec. 28, 2020, 2:46 p.m. UTC | #2
> >   * 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'.
Wolfram Sang Jan. 5, 2021, 3:35 p.m. UTC | #3
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!
diff mbox series

Patch

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;