[PATCH 0/3] i2c: stm32: cleanup & stop handling fix

[PATCH 0/3] i2c: stm32: cleanup & stop handling fix

[Alain Volmat]

This series corrects the handling of the stop condition and
cleanup few bits in the driver stm32f7_i2c.c

Alain Volmat (3):
  i2c: stm32: fix comment and remove unused AUTOEND bit
  i2c: stm32: remove unused stop parameter in start & reload handling
  i2c: stm32: only send a STOP upon transfer completion

 drivers/i2c/stm32f7_i2c.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

-- 
2.25.1

[PATCH 1/3] i2c: stm32: fix comment and remove unused AUTOEND bit

[Alain Volmat]

Comment within stm32_i2c_message_start is misleading, indicating
that AUTOEND bit is setted while it is actually cleared.
Moreover, the bit is actually never setted so there is no need
to clear it hence get rid of this bit clear and the bit macro
as well.

Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
---
 drivers/i2c/stm32f7_i2c.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
index bf2a6c9b4b..78d7156492 100644
--- a/drivers/i2c/stm32f7_i2c.c
+++ b/drivers/i2c/stm32f7_i2c.c
@@ -57,7 +57,6 @@ struct stm32_i2c_regs {
 #define STM32_I2C_CR1_PE			BIT(0)
 
 /* STM32 I2C control 2 */
-#define STM32_I2C_CR2_AUTOEND			BIT(25)
 #define STM32_I2C_CR2_RELOAD			BIT(24)
 #define STM32_I2C_CR2_NBYTES_MASK		GENMASK(23, 16)
 #define STM32_I2C_CR2_NBYTES(n)			((n & 0xff) << 16)
@@ -304,9 +303,8 @@ static void stm32_i2c_message_start(struct stm32_i2c_priv *i2c_priv,
 		cr2 |= STM32_I2C_CR2_SADD7(msg->addr);
 	}
 
-	/* Set nb bytes to transfer and reload or autoend bits */
-	cr2 &= ~(STM32_I2C_CR2_NBYTES_MASK | STM32_I2C_CR2_RELOAD |
-		 STM32_I2C_CR2_AUTOEND);
+	/* Set nb bytes to transfer and reload (if needed) */
+	cr2 &= ~(STM32_I2C_CR2_NBYTES_MASK | STM32_I2C_CR2_RELOAD);
 	if (msg->len > STM32_I2C_MAX_LEN) {
 		cr2 |= STM32_I2C_CR2_NBYTES(STM32_I2C_MAX_LEN);
 		cr2 |= STM32_I2C_CR2_RELOAD;
-- 
2.25.1

Re: [PATCH 1/3] i2c: stm32: fix comment and remove unused AUTOEND bit

[Patrick DELAUNAY]

Hi,

On 9/8/22 10:06, Alain Volmat wrote:
> Comment within stm32_i2c_message_start is misleading, indicating
> that AUTOEND bit is setted while it is actually cleared.
> Moreover, the bit is actually never setted so there is no need
> to clear it hence get rid of this bit clear and the bit macro
> as well.
>
> Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
> ---
>   drivers/i2c/stm32f7_i2c.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
> index bf2a6c9b4b..78d7156492 100644
> --- a/drivers/i2c/stm32f7_i2c.c
> +++ b/drivers/i2c/stm32f7_i2c.c
> @@ -57,7 +57,6 @@ struct stm32_i2c_regs {
>   #define STM32_I2C_CR1_PE			BIT(0)
>   
>   /* STM32 I2C control 2 */
> -#define STM32_I2C_CR2_AUTOEND			BIT(25)
>   #define STM32_I2C_CR2_RELOAD			BIT(24)
>   #define STM32_I2C_CR2_NBYTES_MASK		GENMASK(23, 16)
>   #define STM32_I2C_CR2_NBYTES(n)			((n & 0xff) << 16)
> @@ -304,9 +303,8 @@ static void stm32_i2c_message_start(struct stm32_i2c_priv *i2c_priv,
>   		cr2 |= STM32_I2C_CR2_SADD7(msg->addr);
>   	}
>   
> -	/* Set nb bytes to transfer and reload or autoend bits */
> -	cr2 &= ~(STM32_I2C_CR2_NBYTES_MASK | STM32_I2C_CR2_RELOAD |
> -		 STM32_I2C_CR2_AUTOEND);
> +	/* Set nb bytes to transfer and reload (if needed) */
> +	cr2 &= ~(STM32_I2C_CR2_NBYTES_MASK | STM32_I2C_CR2_RELOAD);
>   	if (msg->len > STM32_I2C_MAX_LEN) {
>   		cr2 |= STM32_I2C_CR2_NBYTES(STM32_I2C_MAX_LEN);
>   		cr2 |= STM32_I2C_CR2_RELOAD;


Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>

Thanks
Patrick

[PATCH 2/3] i2c: stm32: remove unused stop parameter in start & reload handling

[Alain Volmat]

Functions stm32_i2c_message_start and stm32_i2c_handle_reload
both get a stop boolean indicating if the transfer should end with
a STOP or not.  However no specific handling is needed in those
functions hence remove the parameter.

Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
---
 drivers/i2c/stm32f7_i2c.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
index 78d7156492..0ec67b5c12 100644
--- a/drivers/i2c/stm32f7_i2c.c
+++ b/drivers/i2c/stm32f7_i2c.c
@@ -282,7 +282,7 @@ static int stm32_i2c_check_device_busy(struct stm32_i2c_priv *i2c_priv)
 }
 
 static void stm32_i2c_message_start(struct stm32_i2c_priv *i2c_priv,
-				    struct i2c_msg *msg, bool stop)
+				    struct i2c_msg *msg)
 {
 	struct stm32_i2c_regs *regs = i2c_priv->regs;
 	u32 cr2 = readl(&regs->cr2);
@@ -325,7 +325,7 @@ static void stm32_i2c_message_start(struct stm32_i2c_priv *i2c_priv,
  */
 
 static void stm32_i2c_handle_reload(struct stm32_i2c_priv *i2c_priv,
-				    struct i2c_msg *msg, bool stop)
+				    struct i2c_msg *msg)
 {
 	struct stm32_i2c_regs *regs = i2c_priv->regs;
 	u32 cr2 = readl(&regs->cr2);
@@ -431,7 +431,7 @@ static int stm32_i2c_message_xfer(struct stm32_i2c_priv *i2c_priv,
 	/* Add errors */
 	mask |= STM32_I2C_ISR_ERRORS;
 
-	stm32_i2c_message_start(i2c_priv, msg, stop);
+	stm32_i2c_message_start(i2c_priv, msg);
 
 	while (msg->len) {
 		/*
@@ -469,7 +469,7 @@ static int stm32_i2c_message_xfer(struct stm32_i2c_priv *i2c_priv,
 			mask = msg->flags & I2C_M_RD ? STM32_I2C_ISR_RXNE :
 			       STM32_I2C_ISR_TXIS | STM32_I2C_ISR_NACKF;
 
-			stm32_i2c_handle_reload(i2c_priv, msg, stop);
+			stm32_i2c_handle_reload(i2c_priv, msg);
 		} else if (!bytes_to_rw) {
 			/* Wait until TC flag is set */
 			mask = STM32_I2C_ISR_TC;
-- 
2.25.1

Re: [PATCH 2/3] i2c: stm32: remove unused stop parameter in start & reload handling

[Patrick DELAUNAY]

Hi

On 9/8/22 10:06, Alain Volmat wrote:
> Functions stm32_i2c_message_start and stm32_i2c_handle_reload
> both get a stop boolean indicating if the transfer should end with
> a STOP or not.  However no specific handling is needed in those
> functions hence remove the parameter.
>
> Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
> ---
>   drivers/i2c/stm32f7_i2c.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
> index 78d7156492..0ec67b5c12 100644
> --- a/drivers/i2c/stm32f7_i2c.c
> +++ b/drivers/i2c/stm32f7_i2c.c
> @@ -282,7 +282,7 @@ static int stm32_i2c_check_device_busy(struct stm32_i2c_priv *i2c_priv)
>   }
>   
>   static void stm32_i2c_message_start(struct stm32_i2c_priv *i2c_priv,
> -				    struct i2c_msg *msg, bool stop)
> +				    struct i2c_msg *msg)
>   {
>   	struct stm32_i2c_regs *regs = i2c_priv->regs;
>   	u32 cr2 = readl(&regs->cr2);
> @@ -325,7 +325,7 @@ static void stm32_i2c_message_start(struct stm32_i2c_priv *i2c_priv,
>    */
>   
>   static void stm32_i2c_handle_reload(struct stm32_i2c_priv *i2c_priv,
> -				    struct i2c_msg *msg, bool stop)
> +				    struct i2c_msg *msg)
>   {
>   	struct stm32_i2c_regs *regs = i2c_priv->regs;
>   	u32 cr2 = readl(&regs->cr2);
> @@ -431,7 +431,7 @@ static int stm32_i2c_message_xfer(struct stm32_i2c_priv *i2c_priv,
>   	/* Add errors */
>   	mask |= STM32_I2C_ISR_ERRORS;
>   
> -	stm32_i2c_message_start(i2c_priv, msg, stop);
> +	stm32_i2c_message_start(i2c_priv, msg);
>   
>   	while (msg->len) {
>   		/*
> @@ -469,7 +469,7 @@ static int stm32_i2c_message_xfer(struct stm32_i2c_priv *i2c_priv,
>   			mask = msg->flags & I2C_M_RD ? STM32_I2C_ISR_RXNE :
>   			       STM32_I2C_ISR_TXIS | STM32_I2C_ISR_NACKF;
>   
> -			stm32_i2c_handle_reload(i2c_priv, msg, stop);
> +			stm32_i2c_handle_reload(i2c_priv, msg);
>   		} else if (!bytes_to_rw) {
>   			/* Wait until TC flag is set */
>   			mask = STM32_I2C_ISR_TC;


Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>

Thanks
Patrick

[PATCH 3/3] i2c: stm32: only send a STOP upon transfer completion

[Alain Volmat]

Current function stm32_i2c_message_xfer is sending a STOP
whatever the result of the transaction is.  This can cause issues
such as making the bus busy since the controller itself is already
sending automatically a STOP when a NACK is generated.  This can
be especially seen when the processing get slower (ex: enabling lots
of debug messages), ending up send 2 STOP (one automatically by the
controller and a 2nd one at the end of the stm32_i2c_message_xfer
function).

Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
---
 drivers/i2c/stm32f7_i2c.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
index 0ec67b5c12..8803979d3e 100644
--- a/drivers/i2c/stm32f7_i2c.c
+++ b/drivers/i2c/stm32f7_i2c.c
@@ -477,16 +477,16 @@ static int stm32_i2c_message_xfer(struct stm32_i2c_priv *i2c_priv,
 			if (ret)
 				break;
 
+			/* End of transfer, send stop condition */
+			mask = STM32_I2C_CR2_STOP;
+			setbits_le32(&regs->cr2, mask);
+
 			if (!stop)
 				/* Message sent, new message has to be sent */
 				return 0;
 		}
 	}
 
-	/* End of transfer, send stop condition */
-	mask = STM32_I2C_CR2_STOP;
-	setbits_le32(&regs->cr2, mask);
-
 	return stm32_i2c_check_end_of_message(i2c_priv);
 }
 
-- 
2.25.1

Re: [PATCH 3/3] i2c: stm32: only send a STOP upon transfer completion

[Jorge Ramirez-Ortiz, Foundries]

On 08/09/22, Alain Volmat wrote:
> Current function stm32_i2c_message_xfer is sending a STOP
> whatever the result of the transaction is.  This can cause issues
> such as making the bus busy since the controller itself is already
> sending automatically a STOP when a NACK is generated.  This can
> be especially seen when the processing get slower (ex: enabling lots
> of debug messages), ending up send 2 STOP (one automatically by the
> controller and a 2nd one at the end of the stm32_i2c_message_xfer
> function).

um I debugged this - took me a couple of days - and I proposed a fix that has just
been massaged a little in this PR.

IMO the best thing to do is either adding me - or to be fair yourself since you
invested almost zero time on it - as a co-author. The hard bit was to find the
issue in the first place.

what do you think?

I'll try to find some time to test this.

> 
> Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
> ---
>  drivers/i2c/stm32f7_i2c.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
> index 0ec67b5c12..8803979d3e 100644
> --- a/drivers/i2c/stm32f7_i2c.c
> +++ b/drivers/i2c/stm32f7_i2c.c
> @@ -477,16 +477,16 @@ static int stm32_i2c_message_xfer(struct stm32_i2c_priv *i2c_priv,
>  			if (ret)
>  				break;
>  
> +			/* End of transfer, send stop condition */
> +			mask = STM32_I2C_CR2_STOP;
> +			setbits_le32(&regs->cr2, mask);
> +
>  			if (!stop)
>  				/* Message sent, new message has to be sent */
>  				return 0;
>  		}
>  	}
>  
> -	/* End of transfer, send stop condition */
> -	mask = STM32_I2C_CR2_STOP;
> -	setbits_le32(&regs->cr2, mask);
> -
>  	return stm32_i2c_check_end_of_message(i2c_priv);
>  }
>  
> -- 
> 2.25.1
> 

Re: [PATCH 3/3] i2c: stm32: only send a STOP upon transfer completion

[Alain Volmat]

Hi Jorge,

On Thu, Sep 08, 2022 at 10:36:22AM +0200, Jorge Ramirez-Ortiz, Foundries wrote:
> On 08/09/22, Alain Volmat wrote:
> > Current function stm32_i2c_message_xfer is sending a STOP
> > whatever the result of the transaction is.  This can cause issues
> > such as making the bus busy since the controller itself is already
> > sending automatically a STOP when a NACK is generated.  This can
> > be especially seen when the processing get slower (ex: enabling lots
> > of debug messages), ending up send 2 STOP (one automatically by the
> > controller and a 2nd one at the end of the stm32_i2c_message_xfer
> > function).
> 
> um I debugged this - took me a couple of days - and I proposed a fix that has just
> been massaged a little in this PR.
> 
> IMO the best thing to do is either adding me - or to be fair yourself since you
> invested almost zero time on it - as a co-author. The hard bit was to find the
> issue in the first place.
> 
> what do you think?

I'm very sorry, it wasn't my intention at all but indeed I should have
taken more care to highlight your work.  Sorry about that.
I'm going to push a v2 for the serie, highlighting that and putting
your Signed-of first.

> 
> I'll try to find some time to test this.
> 
> > 
> > Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
> > ---
> >  drivers/i2c/stm32f7_i2c.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
> > index 0ec67b5c12..8803979d3e 100644
> > --- a/drivers/i2c/stm32f7_i2c.c
> > +++ b/drivers/i2c/stm32f7_i2c.c
> > @@ -477,16 +477,16 @@ static int stm32_i2c_message_xfer(struct stm32_i2c_priv *i2c_priv,
> >  			if (ret)
> >  				break;
> >  
> > +			/* End of transfer, send stop condition */
> > +			mask = STM32_I2C_CR2_STOP;
> > +			setbits_le32(&regs->cr2, mask);
> > +
> >  			if (!stop)
> >  				/* Message sent, new message has to be sent */
> >  				return 0;
> >  		}
> >  	}
> >  
> > -	/* End of transfer, send stop condition */
> > -	mask = STM32_I2C_CR2_STOP;
> > -	setbits_le32(&regs->cr2, mask);
> > -
> >  	return stm32_i2c_check_end_of_message(i2c_priv);
> >  }
> >  
> > -- 
> > 2.25.1
> >