[PATCH v2 0/3] i2c: rcar: avoid spurious irqs

[PATCH v2 0/3] i2c: rcar: avoid spurious irqs

[Wolfram Sang]

Thanks to a report from Dirk, the first patch clears a window in which
spurious irqs might happen in probe. The second patch makes the
detection of spurious irqs louder. The third is a cleanup found while
developing this series. The first should go to for-current, the others
to for-next. No dependencies, luckily.


Wolfram Sang (3):
  i2c: rcar: bring hardware to known state when probing
  i2c: rcar: WARN about spurious irqs
  i2c: rcar: minor changes to adhere to coding style

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

-- 
2.43.0

[PATCH v2 1/3] i2c: rcar: bring hardware to known state when probing

[Wolfram Sang]

When probing, the hardware is not brought into a known state. This may
be a problem when a hypervisor restarts Linux without resetting the
hardware, leaving an old state running. Make sure the hardware gets
initialized, especially interrupts should be cleared and disabled.

Reported-by: Dirk Behme <dirk.behme@de.bosch.com>
Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
Closes: https://lore.kernel.org/r/20240702045535.2000393-1-dirk.behme@de.bosch.com
Fixes: 6ccbe607132b ("i2c: add Renesas R-Car I2C driver")
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Should go to for-current.

Changes since v1:
* renamed new function to *_reset_slave() instead of *_init_slave()
  which is more appropriate
* do not mention refactorization in commit message
* added Fixes tag

 drivers/i2c/busses/i2c-rcar.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 828aa2ea0fe4..ec73463ea9b5 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -257,6 +257,14 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
 	}
 }
 
+static void rcar_i2c_reset_slave(struct rcar_i2c_priv *priv)
+{
+	rcar_i2c_write(priv, ICSIER, 0);
+	rcar_i2c_write(priv, ICSSR, 0);
+	rcar_i2c_write(priv, ICSCR, SDBS);
+	rcar_i2c_write(priv, ICSAR, 0); /* Gen2: must be 0 if not using slave */
+}
+
 static int rcar_i2c_bus_barrier(struct rcar_i2c_priv *priv)
 {
 	int ret;
@@ -1033,11 +1041,8 @@ static int rcar_unreg_slave(struct i2c_client *slave)
 
 	/* ensure no irq is running before clearing ptr */
 	disable_irq(priv->irq);
-	rcar_i2c_write(priv, ICSIER, 0);
-	rcar_i2c_write(priv, ICSSR, 0);
+	rcar_i2c_reset_slave(priv);
 	enable_irq(priv->irq);
-	rcar_i2c_write(priv, ICSCR, SDBS);
-	rcar_i2c_write(priv, ICSAR, 0); /* Gen2: must be 0 if not using slave */
 
 	priv->slave = NULL;
 
@@ -1152,7 +1157,9 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 		goto out_pm_disable;
 	}
 
-	rcar_i2c_write(priv, ICSAR, 0); /* Gen2: must be 0 if not using slave */
+	/* Bring hardware to known state */
+	rcar_i2c_init(priv);
+	rcar_i2c_reset_slave(priv);
 
 	if (priv->devtype < I2C_RCAR_GEN3) {
 		irqflags |= IRQF_NO_THREAD;
-- 
2.43.0

[PATCH v2 2/3] i2c: rcar: WARN about spurious irqs

[Wolfram Sang]

The FIXME is very old and probably needed because of some driver bug
like insufficient initialization. It may well be that it was fixed
meanwhile but we never know because the spurious irq is silently
ignored. Add now a call trace when this happens so we have more
information in case the issue still exists.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Should go to for-next.

Changes since v1:
* new patch

 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 ec73463ea9b5..d7688d702b65 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -553,7 +553,7 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv *priv, u32 msr)
 	u32 irqs_to_clear = MDE;
 
 	/* FIXME: sometimes, unknown interrupt happened. Do nothing */
-	if (!(msr & MDE))
+	if (WARN(!(msr & MDE), "spurious irq"))
 		return;
 
 	if (msr & MAT)
-- 
2.43.0

[PATCH v2 3/3] i2c: rcar: minor changes to adhere to coding style

[Wolfram Sang]

A newline was missing and closing braces of functions do not need a
semicolon.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
Should go to for-next.

Changes since v1:
* new patch

 drivers/i2c/busses/i2c-rcar.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index d7688d702b65..29c7fafeb8a9 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -192,7 +192,7 @@ static int rcar_i2c_get_scl(struct i2c_adapter *adap)
 
 	return !!(rcar_i2c_read(priv, ICMCR) & FSCL);
 
-};
+}
 
 static void rcar_i2c_set_scl(struct i2c_adapter *adap, int val)
 {
@@ -204,7 +204,7 @@ static void rcar_i2c_set_scl(struct i2c_adapter *adap, int val)
 		priv->recovery_icmcr &= ~FSCL;
 
 	rcar_i2c_write(priv, ICMCR, priv->recovery_icmcr);
-};
+}
 
 static void rcar_i2c_set_sda(struct i2c_adapter *adap, int val)
 {
@@ -216,7 +216,7 @@ static void rcar_i2c_set_sda(struct i2c_adapter *adap, int val)
 		priv->recovery_icmcr &= ~FSDA;
 
 	rcar_i2c_write(priv, ICMCR, priv->recovery_icmcr);
-};
+}
 
 static int rcar_i2c_get_bus_free(struct i2c_adapter *adap)
 {
@@ -224,7 +224,7 @@ static int rcar_i2c_get_bus_free(struct i2c_adapter *adap)
 
 	return !(rcar_i2c_read(priv, ICMCR) & FSDA);
 
-};
+}
 
 static struct i2c_bus_recovery_info rcar_i2c_bri = {
 	.get_scl = rcar_i2c_get_scl,
@@ -233,6 +233,7 @@ static struct i2c_bus_recovery_info rcar_i2c_bri = {
 	.get_bus_free = rcar_i2c_get_bus_free,
 	.recover_bus = i2c_generic_scl_recovery,
 };
+
 static void rcar_i2c_init(struct rcar_i2c_priv *priv)
 {
 	/* reset master mode */
-- 
2.43.0

Re: [PATCH v2 3/3] i2c: rcar: minor changes to adhere to coding style

[Markus Elfring]

> A newline was missing and closing braces of functions do not need a
> semicolon.

Can there be a need to offer such changes by separate update steps?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc6#n81


…
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -192,7 +192,7 @@ static int rcar_i2c_get_scl(struct i2c_adapter *adap)
>
>  	return !!(rcar_i2c_read(priv, ICMCR) & FSCL);
>
> -};
> +}
…

How do you think about to omit any blank lines at such source code places?

Regards,
Markus

Re: [PATCH v2 3/3] i2c: rcar: minor changes to adhere to coding style

[Wolfram Sang]

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

On Sun, Jul 07, 2024 at 12:34:36PM +0200, Markus Elfring wrote:
> > A newline was missing and closing braces of functions do not need a
> > semicolon.
> 
> Can there be a need to offer such changes by separate update steps?

That would be too fine grained in my book.

> >  	return !!(rcar_i2c_read(priv, ICMCR) & FSCL);
> >
> > -};
> > +}
> …
> 
> How do you think about to omit any blank lines at such source code places?

Oh yes, that newline should go as well. Thanks!


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

Re: [PATCH v2 3/3] i2c: rcar: minor changes to adhere to coding style

[Wolfram Sang]

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


> > > -};
> > > +}
> > …
> > 
> > How do you think about to omit any blank lines at such source code places?
> 
> Oh yes, that newline should go as well. Thanks!

Andi, is it okay if I only resend this patch?


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

Re: [PATCH v2 3/3] i2c: rcar: minor changes to adhere to coding style

[Andi Shyti]

Hi Wolfram,

On Mon, Jul 08, 2024 at 11:50:59AM GMT, Wolfram Sang wrote:
> 
> > > > -};
> > > > +}
> > > …
> > > 
> > > How do you think about to omit any blank lines at such source code places?
> > 
> > Oh yes, that newline should go as well. Thanks!
> 
> Andi, is it okay if I only resend this patch?

That's OK... if you want I can remove those blank lines before
applying them, it's just two cases in your patch.

Andi

Re: [PATCH v2 1/3] i2c: rcar: bring hardware to known state when probing

[Andi Shyti]

Hi Wolfram,

On Sun, Jul 07, 2024 at 10:28:46AM GMT, Wolfram Sang wrote:
> When probing, the hardware is not brought into a known state. This may
> be a problem when a hypervisor restarts Linux without resetting the
> hardware, leaving an old state running. Make sure the hardware gets
> initialized, especially interrupts should be cleared and disabled.
> 
> Reported-by: Dirk Behme <dirk.behme@de.bosch.com>
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Closes: https://lore.kernel.org/r/20240702045535.2000393-1-dirk.behme@de.bosch.com
> Fixes: 6ccbe607132b ("i2c: add Renesas R-Car I2C driver")
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Should go to for-current.

merged in i2c/i2c-host-fixes.

Thanks,
Andi

Re: [PATCH v2 3/3] i2c: rcar: minor changes to adhere to coding style

[Wolfram Sang]

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


> That's OK... if you want I can remove those blank lines before
> applying them, it's just two cases in your patch.

Then, please do that. Thanks!


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

Re: [PATCH v2 3/3] i2c: rcar: minor changes to adhere to coding style

[Andi Shyti]

Hi Wolfram,

> > That's OK... if you want I can remove those blank lines before
> > applying them, it's just two cases in your patch.
> 
> Then, please do that. Thanks!

done! Applied 2 and 3 to i2c/i2c-host.

Thanks,
Andi