[PATCH 0/2] i2c: rcar: implement STOP and REP_START according to docs

[PATCH 0/2] i2c: rcar: implement STOP and REP_START according to docs

[Wolfram Sang]

Details are in the patch descriptions. Thanks to Yamasaki-san, Shimoda-san, and
all other people involved.

Wolfram Sang (2):
  i2c: rcar: refactor private flags
  i2c: rcar: implement STOP and REP_START according to docs

 drivers/i2c/busses/i2c-rcar.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

-- 
2.11.0

[PATCH 1/2] i2c: rcar: refactor private flags

[Wolfram Sang]

Use BIT macro to avoid shift-31-problem, indent a little more and use
GENMASK to make it easier to add new flags.

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

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 791a4aa34fdd..a9f1880e2eae 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -19,6 +19,7 @@
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  */
+#include <linux/bitops.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/dmaengine.h>
@@ -112,9 +113,9 @@
 #define ID_ARBLOST	(1 << 3)
 #define ID_NACK		(1 << 4)
 /* persistent flags */
-#define ID_P_NO_RXDMA	(1 << 30) /* HW forbids RXDMA sometimes */
-#define ID_P_PM_BLOCKED	(1 << 31)
-#define ID_P_MASK	(ID_P_PM_BLOCKED | ID_P_NO_RXDMA)
+#define ID_P_NO_RXDMA		BIT(30) /* HW forbids RXDMA sometimes */
+#define ID_P_PM_BLOCKED		BIT(31)
+#define ID_P_MASK		GENMASK(31, 30)
 
 enum rcar_i2c_type {
 	I2C_RCAR_GEN1,
-- 
2.11.0

Re: [PATCH 1/2] i2c: rcar: refactor private flags

[Ulrich Hecht]

> On August 8, 2018 at 9:59 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
> 
> 
> Use BIT macro to avoid shift-31-problem, indent a little more and use
> GENMASK to make it easier to add new flags.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-rcar.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index 791a4aa34fdd..a9f1880e2eae 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -19,6 +19,7 @@
>   * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>   * GNU General Public License for more details.
>   */
> +#include <linux/bitops.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/dmaengine.h>
> @@ -112,9 +113,9 @@
>  #define ID_ARBLOST	(1 << 3)
>  #define ID_NACK		(1 << 4)
>  /* persistent flags */
> -#define ID_P_NO_RXDMA	(1 << 30) /* HW forbids RXDMA sometimes */
> -#define ID_P_PM_BLOCKED	(1 << 31)
> -#define ID_P_MASK	(ID_P_PM_BLOCKED | ID_P_NO_RXDMA)
> +#define ID_P_NO_RXDMA		BIT(30) /* HW forbids RXDMA sometimes */
> +#define ID_P_PM_BLOCKED		BIT(31)
> +#define ID_P_MASK		GENMASK(31, 30)
>  
>  enum rcar_i2c_type {
>  	I2C_RCAR_GEN1,
> -- 
> 2.11.0
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

Re: [PATCH 1/2] i2c: rcar: refactor private flags

[Wolfram Sang]

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

On Wed, Aug 08, 2018 at 09:59:27AM +0200, Wolfram Sang wrote:
> Use BIT macro to avoid shift-31-problem, indent a little more and use
> GENMASK to make it easier to add new flags.
> 
> 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 1/2] i2c: rcar: refactor private flags

[Eugeniu Rosca]

Hi Wolfram,

> On August 8, 2018 at 9:59 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
> 
> Use BIT macro to avoid shift-31-problem, 

May I ask how exactly you spotted the "shift-31-problem" in
drivers/i2c/busses/i2c-rcar.c:
 - visual code review?
 - static analysis, special compiler flags?
 - run-time testing with UBSAN=y?

The background of my question is seeing a lot of UBSAN errors caused
by (1 << 31) in U-Boot with UBSAN=y [1]. Based on my experiments, the
Linux UBSAN simply doesn't complain about (1 << 31). I traced the
difference to be caused by the -std=gnu{89,11} value passed to gcc,
which varies between U-Boot and Linux.

According to feedback from GCC community [2], with 'gcc -std=gnu89',
shifting into (not past) the sign bit is "defined behavior" which is why
UBSAN doesn't report this as an issue in Linux kernel. That makes me
curious about the background of this patch, since it might shed some
light onto how to further handle the (1 << 31) UBSAN warnings in
U-Boot.

Thank you very much for your feedback.

[1] https://patchwork.ozlabs.org/cover/962307/
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87392

Best regards,
Eugeniu.

Re: [PATCH 1/2] i2c: rcar: refactor private flags

[Wolfram Sang]

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


> May I ask how exactly you spotted the "shift-31-problem" in
> drivers/i2c/busses/i2c-rcar.c:
>  - visual code review?
>  - static analysis, special compiler flags?

This one. I run a set of static code analyziers when applying patches.
One of them is 'cppcheck' which reported it.

> According to feedback from GCC community [2], with 'gcc -std=gnu89',
> shifting into (not past) the sign bit is "defined behavior" which is why
> UBSAN doesn't report this as an issue in Linux kernel. That makes me

I see. I guess it can be argued. Yet, BIT() solves other issues as well
('1' vs '1u'), so this was probably a reasonable move nonetheless, plus
we are super-super-sure about the shifting now.


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

Re: [PATCH 1/2] i2c: rcar: refactor private flags

[Eugeniu Rosca]

Hi Wolfram,

On Fri, Oct 05, 2018 at 06:27:28PM +0200, Wolfram Sang wrote:
> 
> > May I ask how exactly you spotted the "shift-31-problem" in
> > drivers/i2c/busses/i2c-rcar.c:
> >  - visual code review?
> >  - static analysis, special compiler flags?
> 
> This one. I run a set of static code analyziers when applying patches.
> One of them is 'cppcheck' which reported it.

Indeed, cppcheck reports w/o this patch:

[drivers/i2c/busses/i2c-rcar.c:972]: (error) Shifting signed 32-bit value by 31 bits is undefined behaviour
[drivers/i2c/busses/i2c-rcar.c:1008]: (error) Shifting signed 32-bit value by 31 bits is undefined behaviour

> 
> > According to feedback from GCC community [2], with 'gcc -std=gnu89',
> > shifting into (not past) the sign bit is "defined behavior" which is why
> > UBSAN doesn't report this as an issue in Linux kernel. That makes me
> 
> I see. I guess it can be argued. Yet, BIT() solves other issues as well
> ('1' vs '1u'), so this was probably a reasonable move nonetheless, plus
> we are super-super-sure about the shifting now.
> 

I agree. There is no doubt that avoiding/fixing shifting into the sign
bit makes the code more portable and will lessen the pain when
switching Kbuild to C99/C11 (if ever needed). I still have open
questions, but since they go beyond i2c framework and beyond kernel
itself (as said, they originate from porting UBSan to U-Boot), I will
discuss them elsewhere.

Thanks again for the reply.

Best regards,
Eugeniu.

[PATCH 2/2] i2c: rcar: implement STOP and REP_START according to docs

[Wolfram Sang]

When doing a REP_START after a read message, the driver used to trigger
a STOP first which would then be overwritten by REP_START. This was the
only stable method found when doing the last refactoring. However, this
was not in accordance with the documentation.

After research from our BSP team and myself, we now can implement a
version which works and is according to the documentation. The new
approach ensures the ICMCR register is only changed when really needed.

Tested on a R-Car Gen2 (H2) and Gen3 with DMA (M3N).

Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index a9f1880e2eae..43ad933df0f0 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -113,9 +113,10 @@
 #define ID_ARBLOST	(1 << 3)
 #define ID_NACK		(1 << 4)
 /* persistent flags */
+#define ID_P_REP_AFTER_RD	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, 30)
+#define ID_P_MASK		GENMASK(31, 29)
 
 enum rcar_i2c_type {
 	I2C_RCAR_GEN1,
@@ -345,7 +346,10 @@ static void rcar_i2c_prepare_msg(struct rcar_i2c_priv *priv)
 		rcar_i2c_write(priv, ICMSR, 0);
 		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
 	} else {
-		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
+		if (priv->flags & ID_P_REP_AFTER_RD)
+			priv->flags &= ~ID_P_REP_AFTER_RD;
+		else
+			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
 		rcar_i2c_write(priv, ICMSR, 0);
 	}
 	rcar_i2c_write(priv, ICMIER, read ? RCAR_IRQ_RECV : RCAR_IRQ_SEND);
@@ -550,15 +554,15 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
 		priv->pos++;
 	}
 
-	/*
-	 * If next received data is the _LAST_, go to STOP phase. Might be
-	 * overwritten by REP START when setting up a new msg. Not elegant
-	 * but the only stable sequence for REP START I have found so far.
-	 * If you want to change this code, make sure sending one transfer with
-	 * four messages (WR-RD-WR-RD) works!
-	 */
-	if (priv->pos + 1 >= msg->len)
-		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
+	/* If next received data is the _LAST_, go to new phase. */
+	if (priv->pos + 1 == msg->len) {
+		if (priv->flags & ID_LAST_MSG) {
+			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
+		} else {
+			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
+			priv->flags |= ID_P_REP_AFTER_RD;
+		}
+	}
 
 	if (priv->pos == msg->len && !(priv->flags & ID_LAST_MSG))
 		rcar_i2c_next_msg(priv);
@@ -626,9 +630,11 @@ static irqreturn_t rcar_i2c_irq(int irq, void *ptr)
 	struct rcar_i2c_priv *priv = ptr;
 	u32 msr, val;
 
-	/* Clear START or STOP as soon as we can */
-	val = rcar_i2c_read(priv, ICMCR);
-	rcar_i2c_write(priv, ICMCR, val & RCAR_BUS_MASK_DATA);
+	/* 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);
+	}
 
 	msr = rcar_i2c_read(priv, ICMSR);
 
-- 
2.11.0

Re: [PATCH 2/2] i2c: rcar: implement STOP and REP_START according to docs

[Ulrich Hecht]

Thank you for your patch.

> On August 8, 2018 at 9:59 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
> 
> 
> When doing a REP_START after a read message, the driver used to trigger
> a STOP first which would then be overwritten by REP_START. This was the
> only stable method found when doing the last refactoring. However, this
> was not in accordance with the documentation.
> 
> After research from our BSP team and myself, we now can implement a
> version which works and is according to the documentation. The new
> approach ensures the ICMCR register is only changed when really needed.
> 
> Tested on a R-Car Gen2 (H2) and Gen3 with DMA (M3N).
> 
> Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-rcar.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index a9f1880e2eae..43ad933df0f0 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -113,9 +113,10 @@
>  #define ID_ARBLOST	(1 << 3)
>  #define ID_NACK		(1 << 4)
>  /* persistent flags */
> +#define ID_P_REP_AFTER_RD	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, 30)
> +#define ID_P_MASK		GENMASK(31, 29)
>  
>  enum rcar_i2c_type {
>  	I2C_RCAR_GEN1,
> @@ -345,7 +346,10 @@ static void rcar_i2c_prepare_msg(struct rcar_i2c_priv *priv)
>  		rcar_i2c_write(priv, ICMSR, 0);
>  		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
>  	} else {
> -		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
> +		if (priv->flags & ID_P_REP_AFTER_RD)
> +			priv->flags &= ~ID_P_REP_AFTER_RD;
> +		else
> +			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
>  		rcar_i2c_write(priv, ICMSR, 0);
>  	}
>  	rcar_i2c_write(priv, ICMIER, read ? RCAR_IRQ_RECV : RCAR_IRQ_SEND);
> @@ -550,15 +554,15 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
>  		priv->pos++;
>  	}
>  
> -	/*
> -	 * If next received data is the _LAST_, go to STOP phase. Might be
> -	 * overwritten by REP START when setting up a new msg. Not elegant
> -	 * but the only stable sequence for REP START I have found so far.
> -	 * If you want to change this code, make sure sending one transfer with
> -	 * four messages (WR-RD-WR-RD) works!
> -	 */
> -	if (priv->pos + 1 >= msg->len)
> -		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
> +	/* If next received data is the _LAST_, go to new phase. */
> +	if (priv->pos + 1 == msg->len) {
> +		if (priv->flags & ID_LAST_MSG) {
> +			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
> +		} else {
> +			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
> +			priv->flags |= ID_P_REP_AFTER_RD;
> +		}
> +	}

So "priv->pos + 1 <= msg->len" is an invariant? (The current code seems to imply that it isn't.)

If it is,
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

Re: [PATCH 2/2] i2c: rcar: implement STOP and REP_START according to docs

[Wolfram Sang]

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


> > -	if (priv->pos + 1 >= msg->len)
> > -		rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
> > +	/* If next received data is the _LAST_, go to new phase. */
> > +	if (priv->pos + 1 == msg->len) {
> > +		if (priv->flags & ID_LAST_MSG) {
> > +			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
> > +		} else {
> > +			rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_START);
> > +			priv->flags |= ID_P_REP_AFTER_RD;
> > +		}
> > +	}
> 
> So "priv->pos + 1 <= msg->len" is an invariant? (The current code seems to imply that it isn't.)

I think it is, we increment pos by 1 right before this 'if'. Do you
agree? IIRC the old '>=' came from a 'well, won't hurt' attitude. It was
not precise, sadly.

> If it is,
> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

Thanks.


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

Re: [PATCH 2/2] i2c: rcar: implement STOP and REP_START according to docs

[Wolfram Sang]

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

On Wed, Aug 08, 2018 at 09:59:28AM +0200, Wolfram Sang wrote:
> When doing a REP_START after a read message, the driver used to trigger
> a STOP first which would then be overwritten by REP_START. This was the
> only stable method found when doing the last refactoring. However, this
> was not in accordance with the documentation.
> 
> After research from our BSP team and myself, we now can implement a
> version which works and is according to the documentation. The new
> approach ensures the ICMCR register is only changed when really needed.
> 
> Tested on a R-Car Gen2 (H2) and Gen3 with DMA (M3N).
> 
> Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
> 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 --]