All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/3] i2c: stm32: cleanup & stop handling fix
@ 2022-09-09 16:06 Alain Volmat
  2022-09-09 16:06 ` [PATCH v3 1/3] i2c: stm32: fix comment and remove unused AUTOEND bit Alain Volmat
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Alain Volmat @ 2022-09-09 16:06 UTC (permalink / raw)
  To: uboot-stm32, u-boot
  Cc: patrice.chotard, patrick.delaunay, jorge, hs, oleksandr.suvorov

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

v3: fix stop handling in patch 3/3
v2: update commit message in patch 3/3

Alain Volmat (3):
  i2c: stm32: fix comment and remove unused AUTOEND bit
  i2c: stm32: remove unused stop parameter in start & reload handling
  i2c: stm32: do not set the STOP condition on error

 drivers/i2c/stm32f7_i2c.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3 1/3] i2c: stm32: fix comment and remove unused AUTOEND bit
  2022-09-09 16:06 [PATCH v3 0/3] i2c: stm32: cleanup & stop handling fix Alain Volmat
@ 2022-09-09 16:06 ` Alain Volmat
  2022-09-12  7:59   ` Patrice CHOTARD
  2022-09-09 16:06 ` [PATCH v3 2/3] i2c: stm32: remove unused stop parameter in start & reload handling Alain Volmat
  2022-09-09 16:06 ` [PATCH v3 3/3] i2c: stm32: do not set the STOP condition on error Alain Volmat
  2 siblings, 1 reply; 10+ messages in thread
From: Alain Volmat @ 2022-09-09 16:06 UTC (permalink / raw)
  To: uboot-stm32, u-boot
  Cc: patrice.chotard, patrick.delaunay, jorge, hs, oleksandr.suvorov

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>
Reviewed-by: Patrick Delaunay <patrick.delaunay@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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 2/3] i2c: stm32: remove unused stop parameter in start & reload handling
  2022-09-09 16:06 [PATCH v3 0/3] i2c: stm32: cleanup & stop handling fix Alain Volmat
  2022-09-09 16:06 ` [PATCH v3 1/3] i2c: stm32: fix comment and remove unused AUTOEND bit Alain Volmat
@ 2022-09-09 16:06 ` Alain Volmat
  2022-09-12  8:00   ` Patrice CHOTARD
  2022-09-09 16:06 ` [PATCH v3 3/3] i2c: stm32: do not set the STOP condition on error Alain Volmat
  2 siblings, 1 reply; 10+ messages in thread
From: Alain Volmat @ 2022-09-09 16:06 UTC (permalink / raw)
  To: uboot-stm32, u-boot
  Cc: patrice.chotard, patrick.delaunay, jorge, hs, oleksandr.suvorov

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>
Reviewed-by: Patrick Delaunay <patrick.delaunay@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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 3/3] i2c: stm32: do not set the STOP condition on error
  2022-09-09 16:06 [PATCH v3 0/3] i2c: stm32: cleanup & stop handling fix Alain Volmat
  2022-09-09 16:06 ` [PATCH v3 1/3] i2c: stm32: fix comment and remove unused AUTOEND bit Alain Volmat
  2022-09-09 16:06 ` [PATCH v3 2/3] i2c: stm32: remove unused stop parameter in start & reload handling Alain Volmat
@ 2022-09-09 16:06 ` Alain Volmat
  2022-09-12  8:19   ` Patrick DELAUNAY
  2022-09-12  8:27   ` Patrice CHOTARD
  2 siblings, 2 replies; 10+ messages in thread
From: Alain Volmat @ 2022-09-09 16:06 UTC (permalink / raw)
  To: uboot-stm32, u-boot
  Cc: patrice.chotard, patrick.delaunay, jorge, hs, oleksandr.suvorov

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.

Thanks to Jorge Ramirez-Ortiz for diagnosing and proposing a first
fix for this. [1]

[1] https://lore.kernel.org/u-boot/20220815145211.31342-2-jorge@foundries.io/

Reported-by: Jorge Ramirez-Ortiz, Foundries <jorge@foundries.io>
Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
---
 drivers/i2c/stm32f7_i2c.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
index 0ec67b5c12..2db7f44d44 100644
--- a/drivers/i2c/stm32f7_i2c.c
+++ b/drivers/i2c/stm32f7_i2c.c
@@ -483,9 +483,9 @@ static int stm32_i2c_message_xfer(struct stm32_i2c_priv *i2c_priv,
 		}
 	}
 
-	/* End of transfer, send stop condition */
-	mask = STM32_I2C_CR2_STOP;
-	setbits_le32(&regs->cr2, mask);
+	/* End of transfer, send stop condition if appropriate */
+	if (!ret && !(status & (STM32_I2C_ISR_NACKF | STM32_I2C_ISR_ERRORS)))
+		setbits_le32(&regs->cr2, STM32_I2C_CR2_STOP);
 
 	return stm32_i2c_check_end_of_message(i2c_priv);
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 1/3] i2c: stm32: fix comment and remove unused AUTOEND bit
  2022-09-09 16:06 ` [PATCH v3 1/3] i2c: stm32: fix comment and remove unused AUTOEND bit Alain Volmat
@ 2022-09-12  7:59   ` Patrice CHOTARD
  0 siblings, 0 replies; 10+ messages in thread
From: Patrice CHOTARD @ 2022-09-12  7:59 UTC (permalink / raw)
  To: Alain Volmat, uboot-stm32, u-boot
  Cc: patrick.delaunay, jorge, hs, oleksandr.suvorov

Hi Alain

On 9/9/22 18: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>
> Reviewed-by: Patrick Delaunay <patrick.delaunay@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: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/3] i2c: stm32: remove unused stop parameter in start & reload handling
  2022-09-09 16:06 ` [PATCH v3 2/3] i2c: stm32: remove unused stop parameter in start & reload handling Alain Volmat
@ 2022-09-12  8:00   ` Patrice CHOTARD
  0 siblings, 0 replies; 10+ messages in thread
From: Patrice CHOTARD @ 2022-09-12  8:00 UTC (permalink / raw)
  To: Alain Volmat, uboot-stm32, u-boot
  Cc: patrick.delaunay, jorge, hs, oleksandr.suvorov

Hi Alain

On 9/9/22 18: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>
> Reviewed-by: Patrick Delaunay <patrick.delaunay@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: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 3/3] i2c: stm32: do not set the STOP condition on error
  2022-09-09 16:06 ` [PATCH v3 3/3] i2c: stm32: do not set the STOP condition on error Alain Volmat
@ 2022-09-12  8:19   ` Patrick DELAUNAY
  2022-09-12  8:35     ` Jorge Ramirez-Ortiz, Foundries
  2022-09-12  8:27   ` Patrice CHOTARD
  1 sibling, 1 reply; 10+ messages in thread
From: Patrick DELAUNAY @ 2022-09-12  8:19 UTC (permalink / raw)
  To: Alain Volmat, uboot-stm32, u-boot
  Cc: patrice.chotard, jorge, hs, oleksandr.suvorov

Hi Alain,

On 9/9/22 18:06, 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.
>
> Thanks to Jorge Ramirez-Ortiz for diagnosing and proposing a first
> fix for this. [1]
>
> [1] https://lore.kernel.org/u-boot/20220815145211.31342-2-jorge@foundries.io/
>
> Reported-by: Jorge Ramirez-Ortiz, Foundries <jorge@foundries.io>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
> ---
>   drivers/i2c/stm32f7_i2c.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
> index 0ec67b5c12..2db7f44d44 100644
> --- a/drivers/i2c/stm32f7_i2c.c
> +++ b/drivers/i2c/stm32f7_i2c.c
> @@ -483,9 +483,9 @@ static int stm32_i2c_message_xfer(struct stm32_i2c_priv *i2c_priv,
>   		}
>   	}
>   
> -	/* End of transfer, send stop condition */
> -	mask = STM32_I2C_CR2_STOP;
> -	setbits_le32(&regs->cr2, mask);
> +	/* End of transfer, send stop condition if appropriate */
> +	if (!ret && !(status & (STM32_I2C_ISR_NACKF | STM32_I2C_ISR_ERRORS)))
> +		setbits_le32(&regs->cr2, STM32_I2C_CR2_STOP);
>   
>   	return stm32_i2c_check_end_of_message(i2c_priv);
>   }



Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
Tested-by: Patrick Delaunay <patrick.delaunay@foss.st.com> [stm32mp157c-dk2]

No regression detection on ST Microelectonics board.

- No error trace on boot

- I2C probe command is OK

   STM32MP> i2c probe
   Valid chip addresses: 28 33

- And other tests done with the 2 I2C devices STPMIC1 & STUSB1600 are ok:
   regulalor command

   pmic status command

   USB type C connection/deconnection


@Jorge: can you test also for your use-case, thanks


Thanks
Patrick



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 3/3] i2c: stm32: do not set the STOP condition on error
  2022-09-09 16:06 ` [PATCH v3 3/3] i2c: stm32: do not set the STOP condition on error Alain Volmat
  2022-09-12  8:19   ` Patrick DELAUNAY
@ 2022-09-12  8:27   ` Patrice CHOTARD
  1 sibling, 0 replies; 10+ messages in thread
From: Patrice CHOTARD @ 2022-09-12  8:27 UTC (permalink / raw)
  To: Alain Volmat, uboot-stm32, u-boot
  Cc: patrick.delaunay, jorge, hs, oleksandr.suvorov

Hi Alain

On 9/9/22 18:06, 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.
> 
> Thanks to Jorge Ramirez-Ortiz for diagnosing and proposing a first
> fix for this. [1]
> 
> [1] https://lore.kernel.org/u-boot/20220815145211.31342-2-jorge@foundries.io/
> 
> Reported-by: Jorge Ramirez-Ortiz, Foundries <jorge@foundries.io>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
> ---
>  drivers/i2c/stm32f7_i2c.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
> index 0ec67b5c12..2db7f44d44 100644
> --- a/drivers/i2c/stm32f7_i2c.c
> +++ b/drivers/i2c/stm32f7_i2c.c
> @@ -483,9 +483,9 @@ static int stm32_i2c_message_xfer(struct stm32_i2c_priv *i2c_priv,
>  		}
>  	}
>  
> -	/* End of transfer, send stop condition */
> -	mask = STM32_I2C_CR2_STOP;
> -	setbits_le32(&regs->cr2, mask);
> +	/* End of transfer, send stop condition if appropriate */
> +	if (!ret && !(status & (STM32_I2C_ISR_NACKF | STM32_I2C_ISR_ERRORS)))
> +		setbits_le32(&regs->cr2, STM32_I2C_CR2_STOP);
>  
>  	return stm32_i2c_check_end_of_message(i2c_priv);
>  }

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 3/3] i2c: stm32: do not set the STOP condition on error
  2022-09-12  8:19   ` Patrick DELAUNAY
@ 2022-09-12  8:35     ` Jorge Ramirez-Ortiz, Foundries
  2022-09-19 15:46       ` Patrick DELAUNAY
  0 siblings, 1 reply; 10+ messages in thread
From: Jorge Ramirez-Ortiz, Foundries @ 2022-09-12  8:35 UTC (permalink / raw)
  To: Patrick DELAUNAY
  Cc: Alain Volmat, uboot-stm32, u-boot, patrice.chotard, jorge, hs,
	oleksandr.suvorov

On 12/09/22, Patrick DELAUNAY wrote:
> Hi Alain,
> 
> On 9/9/22 18:06, 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.
> > 
> > Thanks to Jorge Ramirez-Ortiz for diagnosing and proposing a first
> > fix for this. [1]
> > 
> > [1] https://lore.kernel.org/u-boot/20220815145211.31342-2-jorge@foundries.io/
> > 
> > Reported-by: Jorge Ramirez-Ortiz, Foundries <jorge@foundries.io>
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
> > ---
> >   drivers/i2c/stm32f7_i2c.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
> > index 0ec67b5c12..2db7f44d44 100644
> > --- a/drivers/i2c/stm32f7_i2c.c
> > +++ b/drivers/i2c/stm32f7_i2c.c
> > @@ -483,9 +483,9 @@ static int stm32_i2c_message_xfer(struct stm32_i2c_priv *i2c_priv,
> >   		}
> >   	}
> > -	/* End of transfer, send stop condition */
> > -	mask = STM32_I2C_CR2_STOP;
> > -	setbits_le32(&regs->cr2, mask);
> > +	/* End of transfer, send stop condition if appropriate */
> > +	if (!ret && !(status & (STM32_I2C_ISR_NACKF | STM32_I2C_ISR_ERRORS)))
> > +		setbits_le32(&regs->cr2, STM32_I2C_CR2_STOP);
> >   	return stm32_i2c_check_end_of_message(i2c_priv);
> >   }
> 
> 
> 
> Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Tested-by: Patrick Delaunay <patrick.delaunay@foss.st.com> [stm32mp157c-dk2]
> 
> No regression detection on ST Microelectonics board.
> 
> - No error trace on boot
> 
> - I2C probe command is OK
> 
>   STM32MP> i2c probe
>   Valid chip addresses: 28 33
> 
> - And other tests done with the 2 I2C devices STPMIC1 & STUSB1600 are ok:
>   regulalor command
> 
>   pmic status command
> 
>   USB type C connection/deconnection
> 
> 
> @Jorge: can you test also for your use-case, thanks

yes I did test a few hours ago and it is good on my end.
can add my tested tag if needed

Tested-by: Jorge Ramirez-Ortiz <jorge@foundries.io>

btw I also sent a patch to fix the way some dts properties are handled.
shall I submit separately or will it be includeed in this set?


> 
> 
> Thanks
> Patrick
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 3/3] i2c: stm32: do not set the STOP condition on error
  2022-09-12  8:35     ` Jorge Ramirez-Ortiz, Foundries
@ 2022-09-19 15:46       ` Patrick DELAUNAY
  0 siblings, 0 replies; 10+ messages in thread
From: Patrick DELAUNAY @ 2022-09-19 15:46 UTC (permalink / raw)
  To: Jorge Ramirez-Ortiz, Foundries
  Cc: Alain Volmat, uboot-stm32, u-boot, patrice.chotard, hs,
	oleksandr.suvorov

Hi Jorge,

On 9/12/22 10:35, Jorge Ramirez-Ortiz, Foundries wrote:
> On 12/09/22, Patrick DELAUNAY wrote:
>> Hi Alain,
>>
>> On 9/9/22 18:06, 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.
>>>
>>> Thanks to Jorge Ramirez-Ortiz for diagnosing and proposing a first
>>> fix for this. [1]
>>>
>>> [1] https://lore.kernel.org/u-boot/20220815145211.31342-2-jorge@foundries.io/
>>>
>>> Reported-by: Jorge Ramirez-Ortiz, Foundries <jorge@foundries.io>
>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
>>> Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
>>> ---
>>>    drivers/i2c/stm32f7_i2c.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
>>> index 0ec67b5c12..2db7f44d44 100644
>>> --- a/drivers/i2c/stm32f7_i2c.c
>>> +++ b/drivers/i2c/stm32f7_i2c.c
>>> @@ -483,9 +483,9 @@ static int stm32_i2c_message_xfer(struct stm32_i2c_priv *i2c_priv,
>>>    		}
>>>    	}
>>> -	/* End of transfer, send stop condition */
>>> -	mask = STM32_I2C_CR2_STOP;
>>> -	setbits_le32(&regs->cr2, mask);
>>> +	/* End of transfer, send stop condition if appropriate */
>>> +	if (!ret && !(status & (STM32_I2C_ISR_NACKF | STM32_I2C_ISR_ERRORS)))
>>> +		setbits_le32(&regs->cr2, STM32_I2C_CR2_STOP);
>>>    	return stm32_i2c_check_end_of_message(i2c_priv);
>>>    }
>>
>>
>> Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>> Tested-by: Patrick Delaunay <patrick.delaunay@foss.st.com> [stm32mp157c-dk2]
>>
>> No regression detection on ST Microelectonics board.
>>
>> - No error trace on boot
>>
>> - I2C probe command is OK
>>
>>    STM32MP> i2c probe
>>    Valid chip addresses: 28 33
>>
>> - And other tests done with the 2 I2C devices STPMIC1 & STUSB1600 are ok:
>>    regulalor command
>>
>>    pmic status command
>>
>>    USB type C connection/deconnection
>>
>>
>> @Jorge: can you test also for your use-case, thanks
> yes I did test a few hours ago and it is good on my end.
> can add my tested tag if needed
>
> Tested-by: Jorge Ramirez-Ortiz <jorge@foundries.io>


Thanks for the test

but sorry I don't see your message when I made my pull request for 
v2022.10-rc5

so I don't add your tag "Tested-by" when I merge this commit in serie V4.


> btw I also sent a patch to fix the way some dts properties are handled.
> shall I submit separately or will it be includeed in this set?

Yes pushed by Alain in V4 = 
http://patchwork.ozlabs.org/project/uboot/list/?series=317940&state=*

See[v4,4/4] i2c: stm32: fix usage of rise/fall device tree properties

http://patchwork.ozlabs.org/project/uboot/patch/20220912084201.1826979-5-alain.volmat@foss.st.com/

it is part of my last pull request = u-boot-stm32-20220915

it is now merged in master branch and part of the next v2022.10-rc5

Thanks
Patrick


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-09-19 15:46 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-09 16:06 [PATCH v3 0/3] i2c: stm32: cleanup & stop handling fix Alain Volmat
2022-09-09 16:06 ` [PATCH v3 1/3] i2c: stm32: fix comment and remove unused AUTOEND bit Alain Volmat
2022-09-12  7:59   ` Patrice CHOTARD
2022-09-09 16:06 ` [PATCH v3 2/3] i2c: stm32: remove unused stop parameter in start & reload handling Alain Volmat
2022-09-12  8:00   ` Patrice CHOTARD
2022-09-09 16:06 ` [PATCH v3 3/3] i2c: stm32: do not set the STOP condition on error Alain Volmat
2022-09-12  8:19   ` Patrick DELAUNAY
2022-09-12  8:35     ` Jorge Ramirez-Ortiz, Foundries
2022-09-19 15:46       ` Patrick DELAUNAY
2022-09-12  8:27   ` Patrice CHOTARD

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.