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

[PATCH v2 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

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: only send a STOP upon transfer completion

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

-- 
2.25.1

[PATCH v2 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

[PATCH v2 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

[PATCH v2 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).

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 | 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 v2 1/3] i2c: stm32: fix comment and remove unused AUTOEND bit

[Patrick DELAUNAY]

Hi,

On 9/8/22 12:59, 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

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

[Patrick DELAUNAY]

Hi,

On 9/8/22 12:59, 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

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

[Patrick DELAUNAY]

Hi,

On 9/8/22 12:59, 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).
>
> 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 | 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);
>   }
>   


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

Thanks
Patrick

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

[Jorge Ramirez-Ortiz, Foundries]

On 08/09/22, Patrick DELAUNAY wrote:
> Hi,
> 
> On 9/8/22 12:59, 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).
> >

Cmon no need to thank me, that is kind of excessive :)
Just the sign-off or codevelop tags for reference


if you can wait before merging I will test the series before monday

thanks
Jorge

> > 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 | 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);
> >   }
> 
> 
> Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> 
> Thanks
> Patrick
> 

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

[Heiko Schocher]

Hello Jorge,

On 09.09.22 10:30, Jorge Ramirez-Ortiz, Foundries wrote:
> On 08/09/22, Patrick DELAUNAY wrote:
>> Hi,
>>
>> On 9/8/22 12:59, 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).
>>>
> 
> Cmon no need to thank me, that is kind of excessive :)
> Just the sign-off or codevelop tags for reference
> 
> 
> if you can wait before merging I will test the series before monday

I would love to see a test before we merge this.

@Patrick: feel free to merge it through stm32 repo.

Thanks!

bye,
Heiko
> 
> thanks
> Jorge
> 
>>> 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 | 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);
>>>   }
>>
>>
>> Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>
>> Thanks
>> Patrick
>>

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

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

[Patrick DELAUNAY]

Hi,

On 9/9/22 10:43, Heiko Schocher wrote:
> Hello Jorge,
>
> On 09.09.22 10:30, Jorge Ramirez-Ortiz, Foundries wrote:
>> On 08/09/22, Patrick DELAUNAY wrote:
>>> Hi,
>>>
>>> On 9/8/22 12:59, 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).
>>>>
>> Cmon no need to thank me, that is kind of excessive :)
>> Just the sign-off or codevelop tags for reference
>>
>>
>> if you can wait before merging I will test the series before monday
> I would love to see a test before we merge this.
>
> @Patrick: feel free to merge it through stm32 repo.


Ok, I will take this serie in my next pull request for stm32


>
> Thanks!
>
> bye,
> Heiko


By Patrick


>> thanks
>> Jorge
>>
>>>> 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 | 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);
>>>>    }
>>>
>>> Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>>
>>> Thanks
>>> Patrick
>>>

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

[Patrick DELAUNAY]

Hi Alain

On 9/8/22 12:59, 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).
>
> 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 | 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);
>   }
>   


Boot on DK2 failed with the traces:


U-Boot 2022.10-rc4-00043-g5b118161055 (Sep 09 2022 - 14:19:12 +0200)

CPU: STM32MP157CAC Rev.B
Model: STMicroelectronics STM32MP157C-DK2 Discovery Board
Board: stm32mp1 in trusted mode (st,stm32mp157c-dk2)
Board: MB1272 Var2.0 Rev.C-01
DRAM:  512 MiB
Clocks:
- MPU : 650 MHz
- MCU : 208.878 MHz
- AXI : 266.500 MHz
- PER : 24 MHz
- DDR : 533 MHz
stpmic1_pmic stpmic@33: stpmic1_read: failed to read register 0x25 : 
-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x25 
:-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x25 
:-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x2a 
:-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x26 
:-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x21 
:-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x22 
:-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x23 
:-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x25 
:-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x26 
:-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x29 
:-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write regi�Core:  
275 devices, 40 uclasses, devicetree: board
WDT:   Started watchdog@5a002000 with servicing (32s timeout)
NAND:  0 MiB
MMC:   STM32 SD/MMC: 0
Loading Environment from MMC... OK
In:    serial
Out:   serial
Err:   serial
Net:   eth0: ethernet@5800a000
Hit any key to stop autoboot:  0


I think the code should be inserted AFTER the test "if (!stop)"

I modify the patch with

-------------------------- drivers/i2c/stm32f7_i2c.c 
-------------------------- index aac592860e1..cd3bcdf8d99 100644 @@ 
-477,13 +477,12 @@ 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 */ 
+setbits_le32(&regs->cr2, STM32_I2C_CR2_STOP); } }


And the boot is OK, I2C read/tested is OK

test with the 2 available device on the board = STPMIC1 & STUSB1600

STM32MP> i2c bus
Bus 4:    i2c@40012000
Bus 3:    i2c@5c002000  (active 3)
    28: stusb1600@28, offset len 1, flags 0
    33: stpmic@33, offset len 1, flags 0

STM32MP> pmic dev stpmic@33

STM32MP> pmic dump
Dump pmic: stpmic@33 registers

0x00: 00 10 00 00 00 01 10 00 00 00 00 00 00 00 00 00
0x10: 04 00 00 00 00 00 80 00 00 00 00 00 00 00 00 00
0x20: 61 79 d9 d9 01 25 61 7d 00 51 0d 00 00 00 00 00
0x30: 61 50 d9 d9 00 24 24 24 01 51 04 00 00 00 00 00
0x40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x80: ff ff ff cf 00 00 00 00 00 00 00 00 00 00 00 00
0x90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0xa0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0xb0: 00 00 00 00 00 00 00 00 08 00 00 00 00 00 01 02
0xc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0xd0: 00 00 00 00 00 00 00 00 00 00 0c 00 00 00 00 00
0xe0: aa 55 01 19 f0 81 84 00 31 03 0b 00 00 00 01 08
0xf0: 40 10 00 00 20 52 fd 0e ee 92 c0 02 f2 80 02 33

STM32MP> regulator status -a
Name                 Enabled            uV         mA Mode
reg11                disabled      1100000          - -
reg18                disabled      1800000          - -
usb33                disabled      3300000          - -
vrefbuf@50025000     enabled       2500000          - -
vddcore              enabled       1200000          - HP
vdd_ddr              enabled       1350000          - HP
vdd                  enabled       3300000          - HP
v3v3                 enabled       3300000          - HP
v1v8_audio           enabled       1800000          - -
v3v3_hdmi            enabled       3300000          - -
vtt_ddr              enabled        675000          - SINK SOURCE
vdd_usb              disabled      3300000          - -
vdda                 enabled       2900000          - -
v1v2_hdmi            enabled       1200000          - -
vref_ddr             enabled        675000          - -
bst_out              disabled            -          - -
vbus_otg             disabled            -          - -
vbus_sw              disabled            -          - -
vin                  enabled       5000000          -


I2C write is OK: tested with :

STM32MP> regulator dev vbus_otg
dev: vbus_otg @ pwr_sw1
STM32MP> regulator enable

+ USB cable deconnection detection in 'ums command'


So I think you need to modify the patch

regards


Patrick

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

[Alain Volmat]

Hi Patrick

On Fri, Sep 09, 2022 at 02:53:23PM +0200, Patrick DELAUNAY wrote:
> Hi Alain
> 
> On 9/8/22 12:59, 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).
> > 
> > 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 | 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);
> >   }
> 
> 
> Boot on DK2 failed with the traces:

Ouch, I am very sorry about that. I think I might have made a mistake
during testing / removing debug traces, leading to this mistake ;-(
Very sorry about that, thanks a lot Patrick for the test.

> 
> 
> U-Boot 2022.10-rc4-00043-g5b118161055 (Sep 09 2022 - 14:19:12 +0200)
> 
> CPU: STM32MP157CAC Rev.B
> Model: STMicroelectronics STM32MP157C-DK2 Discovery Board
> Board: stm32mp1 in trusted mode (st,stm32mp157c-dk2)
> Board: MB1272 Var2.0 Rev.C-01
> DRAM:  512 MiB
> Clocks:
> - MPU : 650 MHz
> - MCU : 208.878 MHz
> - AXI : 266.500 MHz
> - PER : 24 MHz
> - DDR : 533 MHz
> stpmic1_pmic stpmic@33: stpmic1_read: failed to read register 0x25 :
> -16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x25
> :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x25
> :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x2a
> :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x26
> :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x21
> :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x22
> :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x23
> :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x25
> :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x26
> :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write register 0x29
> :-16stpmic1_pmic stpmic@33: stpmic1_write: failed to write regi�Core:  275
> devices, 40 uclasses, devicetree: board
> WDT:   Started watchdog@5a002000 with servicing (32s timeout)
> NAND:  0 MiB
> MMC:   STM32 SD/MMC: 0
> Loading Environment from MMC... OK
> In:    serial
> Out:   serial
> Err:   serial
> Net:   eth0: ethernet@5800a000
> Hit any key to stop autoboot:  0
> 
> 
> I think the code should be inserted AFTER the test "if (!stop)"
> 
> I modify the patch with
> 
> -------------------------- drivers/i2c/stm32f7_i2c.c
> -------------------------- index aac592860e1..cd3bcdf8d99 100644 @@ -477,13
> +477,12 @@ 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 */ +setbits_le32(&regs->cr2, STM32_I2C_CR2_STOP); } }
> 
> 
> And the boot is OK, I2C read/tested is OK
> 
> test with the 2 available device on the board = STPMIC1 & STUSB1600
> 
> STM32MP> i2c bus
> Bus 4:    i2c@40012000
> Bus 3:    i2c@5c002000  (active 3)
>    28: stusb1600@28, offset len 1, flags 0
>    33: stpmic@33, offset len 1, flags 0
> 
> STM32MP> pmic dev stpmic@33
> 
> STM32MP> pmic dump
> Dump pmic: stpmic@33 registers
> 
> 0x00: 00 10 00 00 00 01 10 00 00 00 00 00 00 00 00 00
> 0x10: 04 00 00 00 00 00 80 00 00 00 00 00 00 00 00 00
> 0x20: 61 79 d9 d9 01 25 61 7d 00 51 0d 00 00 00 00 00
> 0x30: 61 50 d9 d9 00 24 24 24 01 51 04 00 00 00 00 00
> 0x40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x80: ff ff ff cf 00 00 00 00 00 00 00 00 00 00 00 00
> 0x90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0xa0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0xb0: 00 00 00 00 00 00 00 00 08 00 00 00 00 00 01 02
> 0xc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0xd0: 00 00 00 00 00 00 00 00 00 00 0c 00 00 00 00 00
> 0xe0: aa 55 01 19 f0 81 84 00 31 03 0b 00 00 00 01 08
> 0xf0: 40 10 00 00 20 52 fd 0e ee 92 c0 02 f2 80 02 33
> 
> STM32MP> regulator status -a
> Name                 Enabled            uV         mA Mode
> reg11                disabled      1100000          - -
> reg18                disabled      1800000          - -
> usb33                disabled      3300000          - -
> vrefbuf@50025000     enabled       2500000          - -
> vddcore              enabled       1200000          - HP
> vdd_ddr              enabled       1350000          - HP
> vdd                  enabled       3300000          - HP
> v3v3                 enabled       3300000          - HP
> v1v8_audio           enabled       1800000          - -
> v3v3_hdmi            enabled       3300000          - -
> vtt_ddr              enabled        675000          - SINK SOURCE
> vdd_usb              disabled      3300000          - -
> vdda                 enabled       2900000          - -
> v1v2_hdmi            enabled       1200000          - -
> vref_ddr             enabled        675000          - -
> bst_out              disabled            -          - -
> vbus_otg             disabled            -          - -
> vbus_sw              disabled            -          - -
> vin                  enabled       5000000          -
> 
> 
> I2C write is OK: tested with :
> 
> STM32MP> regulator dev vbus_otg
> dev: vbus_otg @ pwr_sw1
> STM32MP> regulator enable
> 
> + USB cable deconnection detection in 'ums command'
> 
> 
> So I think you need to modify the patch

In fact, moving the set STOP at this place right after the TC flag
has an issue leading to breaking the i2c probe.  As I wrote originaly,
we have to take into consideration the first NACK check in the while
loop, so finally, the best solution seems to me as making the set STOP
conditionnal right at the end of the function (actually as Jorge patch
was doing in his patch) but also checking for the NACK or ERROR as well.

Basically, as below:

+    /* 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);

Sorry for all the noise with this problem.  I tested it again and with
that I don't see issues after a NACK and also the probe is still behaving
correctly.  Let me update the series with a v3.

Regards,
Alain
> 
> regards
> 
> 
> Patrick

> 

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

[Jorge Ramirez-Ortiz, Foundries]

On 09/09/22, Alain Volmat wrote:
> Hi Patrick
> 
> On Fri, Sep 09, 2022 at 02:53:23PM +0200, Patrick DELAUNAY wrote:
> > Hi Alain
> > 
> > On 9/8/22 12:59, 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).
> > > 
> > > 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 | 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);
> > >   }
> > 
> > 
> > Boot on DK2 failed with the traces:
> 
> Ouch, I am very sorry about that. I think I might have made a mistake
> during testing / removing debug traces, leading to this mistake ;-(
> Very sorry about that, thanks a lot Patrick for the test.
> 

Just did a quick verification on my end and at least for my use case it is all
good.

My use case is enabling SCP03 on the NXP SE05x using the U-boot i2c driver from
OP-TEE via the trampoline.

Also, xould you mind also including the fix below in your series Alain? I think
it is better if you send them all together.

many thanks for steping up for these fixes

Jorge



Author: Jorge Ramirez-Ortiz <jorge@foundries.io>
Date:   3 minutes ago

    i2c: stm32: fix usage of rise/fall device tree properties
    
    These two device tree properties were not being applied.
    
    Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>

diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
index 505d27afe8..5231055be0 100644
--- a/drivers/i2c/stm32f7_i2c.c
+++ b/drivers/i2c/stm32f7_i2c.c
@@ -910,17 +910,18 @@ static int stm32_of_to_plat(struct udevice *dev)
 {
        const struct stm32_i2c_data *data;
        struct stm32_i2c_priv *i2c_priv = dev_get_priv(dev);
-   u32 rise_time, fall_time;
        int ret;
 
        data = (const struct stm32_i2c_data *)dev_get_driver_data(dev);
        if (!data)
                return -EINVAL;
 
-   rise_time = dev_read_u32_default(dev, "i2c-scl-rising-time-ns",
+ i2c_priv->setup.rise_time = dev_read_u32_default(dev,
+                                  "i2c-scl-rising-time-ns",
                                         STM32_I2C_RISE_TIME_DEFAULT);
 
-   fall_time = dev_read_u32_default(dev, "i2c-scl-falling-time-ns",
+ i2c_priv->setup.fall_time = dev_read_u32_default(dev,
+                                 "i2c-scl-falling-time-ns",
                                         STM32_I2C_FALL_TIME_DEFAULT);
 
        i2c_priv->dnf_dt = dev_read_u32_default(dev, "i2c-digital-filter-width-ns", 0);

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

[Alain Volmat]

Hi Jorge

On Sun, Sep 11, 2022 at 08:57:17PM +0200, Jorge Ramirez-Ortiz, Foundries wrote:
> On 09/09/22, Alain Volmat wrote:
> > Hi Patrick
> > 
> > On Fri, Sep 09, 2022 at 02:53:23PM +0200, Patrick DELAUNAY wrote:
> > > Hi Alain
> > > 
> > > On 9/8/22 12:59, 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).
> > > > 
> > > > 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 | 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);
> > > >   }
> > > 
> > > 
> > > Boot on DK2 failed with the traces:
> > 
> > Ouch, I am very sorry about that. I think I might have made a mistake
> > during testing / removing debug traces, leading to this mistake ;-(
> > Very sorry about that, thanks a lot Patrick for the test.
> > 
> 
> Just did a quick verification on my end and at least for my use case it is all
> good.
> 
> My use case is enabling SCP03 on the NXP SE05x using the U-boot i2c driver from
> OP-TEE via the trampoline.
> 
> Also, xould you mind also including the fix below in your series Alain? I think
> it is better if you send them all together.

Sure, just added it in my tree and will put the serie v4 in few seconds.
Thanks a lot for the patch.

Alain

> 
> many thanks for steping up for these fixes
> 
> Jorge
> 
> 
> 
> Author: Jorge Ramirez-Ortiz <jorge@foundries.io>
> Date:   3 minutes ago
> 
>     i2c: stm32: fix usage of rise/fall device tree properties
>     
>     These two device tree properties were not being applied.
>     
>     Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> 
> diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
> index 505d27afe8..5231055be0 100644
> --- a/drivers/i2c/stm32f7_i2c.c
> +++ b/drivers/i2c/stm32f7_i2c.c
> @@ -910,17 +910,18 @@ static int stm32_of_to_plat(struct udevice *dev)
>  {
>         const struct stm32_i2c_data *data;
>         struct stm32_i2c_priv *i2c_priv = dev_get_priv(dev);
> -   u32 rise_time, fall_time;
>         int ret;
>  
>         data = (const struct stm32_i2c_data *)dev_get_driver_data(dev);
>         if (!data)
>                 return -EINVAL;
>  
> -   rise_time = dev_read_u32_default(dev, "i2c-scl-rising-time-ns",
> + i2c_priv->setup.rise_time = dev_read_u32_default(dev,
> +                                  "i2c-scl-rising-time-ns",
>                                          STM32_I2C_RISE_TIME_DEFAULT);
>  
> -   fall_time = dev_read_u32_default(dev, "i2c-scl-falling-time-ns",
> + i2c_priv->setup.fall_time = dev_read_u32_default(dev,
> +                                 "i2c-scl-falling-time-ns",
>                                          STM32_I2C_FALL_TIME_DEFAULT);
>  
>         i2c_priv->dnf_dt = dev_read_u32_default(dev, "i2c-digital-filter-width-ns", 0);