[PATCHv2 1/2] i2c: stm32f7: fix clearing the control register

[PATCHv2 1/2] i2c: stm32f7: fix clearing the control register

[Jorge Ramirez-Ortiz]

Bits should be set to 0, not 1.

Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
---
 drivers/i2c/stm32f7_i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
index bf2a6c9b4b..3a727e68ac 100644
--- a/drivers/i2c/stm32f7_i2c.c
+++ b/drivers/i2c/stm32f7_i2c.c
@@ -413,7 +413,7 @@ static int stm32_i2c_check_end_of_message(struct stm32_i2c_priv *i2c_priv)
 		setbits_le32(&regs->icr, STM32_I2C_ICR_STOPCF);
 
 		/* Clear control register 2 */
-		setbits_le32(&regs->cr2, STM32_I2C_CR2_RESET_MASK);
+		clrbits_le32(&regs->cr2, STM32_I2C_CR2_RESET_MASK);
 	}
 
 	return ret;
-- 
2.34.1

[PATCHv2 2/2] i2c: stm32f7: do not set the STOP condition on error

[Jorge Ramirez-Ortiz]

Sending the stop condition without waiting for transfer complete
has been found to lock the bus (BUSY) when NACKF is raised.

Tested accessing the NXP SE05X I2C device.
https://www.nxp.com/docs/en/application-note/AN12399.pdf

Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
---
 drivers/i2c/stm32f7_i2c.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
index 3a727e68ac..14827e5cec 100644
--- a/drivers/i2c/stm32f7_i2c.c
+++ b/drivers/i2c/stm32f7_i2c.c
@@ -485,9 +485,11 @@ 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);
+	if (!ret) {
+		/* 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.34.1

Re: [PATCHv2 1/2] i2c: stm32f7: fix clearing the control register

[Patrice CHOTARD]

Hi Jorge

On 8/15/22 16:52, Jorge Ramirez-Ortiz wrote:
> Bits should be set to 0, not 1.
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> ---
>  drivers/i2c/stm32f7_i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
> index bf2a6c9b4b..3a727e68ac 100644
> --- a/drivers/i2c/stm32f7_i2c.c
> +++ b/drivers/i2c/stm32f7_i2c.c
> @@ -413,7 +413,7 @@ static int stm32_i2c_check_end_of_message(struct stm32_i2c_priv *i2c_priv)
>  		setbits_le32(&regs->icr, STM32_I2C_ICR_STOPCF);
>  
>  		/* Clear control register 2 */
> -		setbits_le32(&regs->cr2, STM32_I2C_CR2_RESET_MASK);
> +		clrbits_le32(&regs->cr2, STM32_I2C_CR2_RESET_MASK);
>  	}
>  
>  	return ret;


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

Re: [PATCHv2 2/2] i2c: stm32f7: do not set the STOP condition on error

[Patrice CHOTARD]

+Alain

Alain, can you have a look a this patch and give your feedback on it.

On my side i tested it on stm32mp157c-ev1 and stm32mp157c-dk2, i didn't see any regression
but i prefer to get expert feedback ;-)

Thanks
Patrice

On 8/15/22 16:52, Jorge Ramirez-Ortiz wrote:
> Sending the stop condition without waiting for transfer complete
> has been found to lock the bus (BUSY) when NACKF is raised.
> 
> Tested accessing the NXP SE05X I2C device.
> https://www.nxp.com/docs/en/application-note/AN12399.pdf
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> ---
>  drivers/i2c/stm32f7_i2c.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
> index 3a727e68ac..14827e5cec 100644
> --- a/drivers/i2c/stm32f7_i2c.c
> +++ b/drivers/i2c/stm32f7_i2c.c
> @@ -485,9 +485,11 @@ 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);
> +	if (!ret) {
> +		/* 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);
>  }

Re: [PATCHv2 2/2] i2c: stm32f7: do not set the STOP condition on error

[Patrice CHOTARD]

+Alain (with the correct email address ;-))

Alain, can you have a look a this patch and give your feedback on it.

On my side i tested it on stm32mp157c-ev1 and stm32mp157c-dk2, i didn't see any regression
but i prefer to get expert feedback 

Thanks
Patrice

On 8/15/22 16:52, Jorge Ramirez-Ortiz wrote:
> Sending the stop condition without waiting for transfer complete
> has been found to lock the bus (BUSY) when NACKF is raised.
> 
> Tested accessing the NXP SE05X I2C device.
> https://www.nxp.com/docs/en/application-note/AN12399.pdf
> 
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> ---
>  drivers/i2c/stm32f7_i2c.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
> index 3a727e68ac..14827e5cec 100644
> --- a/drivers/i2c/stm32f7_i2c.c
> +++ b/drivers/i2c/stm32f7_i2c.c
> @@ -485,9 +485,11 @@ 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);
> +	if (!ret) {
> +		/* 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);
>  }

Re: [PATCHv2 1/2] i2c: stm32f7: fix clearing the control register

[Patrick DELAUNAY]

Hi,

On 8/15/22 16:52, Jorge Ramirez-Ortiz wrote:
> Bits should be set to 0, not 1.
>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> ---
>   drivers/i2c/stm32f7_i2c.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
> index bf2a6c9b4b..3a727e68ac 100644
> --- a/drivers/i2c/stm32f7_i2c.c
> +++ b/drivers/i2c/stm32f7_i2c.c
> @@ -413,7 +413,7 @@ static int stm32_i2c_check_end_of_message(struct stm32_i2c_priv *i2c_priv)
>   		setbits_le32(&regs->icr, STM32_I2C_ICR_STOPCF);
>   
>   		/* Clear control register 2 */
> -		setbits_le32(&regs->cr2, STM32_I2C_CR2_RESET_MASK);
> +		clrbits_le32(&regs->cr2, STM32_I2C_CR2_RESET_MASK);
>   	}
>   
>   	return ret;


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

Thanks
Patrick

Re: [PATCHv2 2/2] i2c: stm32f7: do not set the STOP condition on error

[Alain Volmat]

Hi,

I confirm that a fix is necessary regarding this setting of the stop
condition.  As a matter of fact, the controller is already sending
the stop condition in case of NACK so there is no need to send the
stop condition.
However, this fix is not enough since the nack could be detected
few lines above 

	if (status & (STM32_I2C_ISR_NACKF | STM32_I2C_ISR_ERRORS))
		break;

and in this case the current check would not catch it.

I propose to set the STOP condition upon handling of the transfer
complete.

I've put this fix within a small 3 patches series that I'm going to
send, could you check it to confirm this fixes the issue ?

Regards,
Alain

On Thu, Aug 25, 2022 at 03:36:36PM +0200, Patrice CHOTARD wrote:
> +Alain (with the correct email address ;-))
> 
> Alain, can you have a look a this patch and give your feedback on it.
> 
> On my side i tested it on stm32mp157c-ev1 and stm32mp157c-dk2, i didn't see any regression
> but i prefer to get expert feedback 
> 
> Thanks
> Patrice
> 
> On 8/15/22 16:52, Jorge Ramirez-Ortiz wrote:
> > Sending the stop condition without waiting for transfer complete
> > has been found to lock the bus (BUSY) when NACKF is raised.
> > 
> > Tested accessing the NXP SE05X I2C device.
> > https://www.nxp.com/docs/en/application-note/AN12399.pdf
> > 
> > Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> > Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> > ---
> >  drivers/i2c/stm32f7_i2c.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
> > index 3a727e68ac..14827e5cec 100644
> > --- a/drivers/i2c/stm32f7_i2c.c
> > +++ b/drivers/i2c/stm32f7_i2c.c
> > @@ -485,9 +485,11 @@ 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);
> > +	if (!ret) {
> > +		/* 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);
> >  }

Re: [PATCHv2 2/2] i2c: stm32f7: do not set the STOP condition on error

[Patrick DELAUNAY]

Hi,

On 9/7/22 11:20, Alain Volmat wrote:
> Hi,
>
> I confirm that a fix is necessary regarding this setting of the stop
> condition.  As a matter of fact, the controller is already sending
> the stop condition in case of NACK so there is no need to send the
> stop condition.
> However, this fix is not enough since the nack could be detected
> few lines above
>
> 	if (status & (STM32_I2C_ISR_NACKF | STM32_I2C_ISR_ERRORS))
> 		break;
>
> and in this case the current check would not catch it.
>
> I propose to set the STOP condition upon handling of the transfer
> complete.
>
> I've put this fix within a small 3 patches series that I'm going to
> send, could you check it to confirm this fixes the issue ?
>
> Regards,
> Alain
>
> On Thu, Aug 25, 2022 at 03:36:36PM +0200, Patrice CHOTARD wrote:
>> +Alain (with the correct email address ;-))
>>
>> Alain, can you have a look a this patch and give your feedback on it.
>>
>> On my side i tested it on stm32mp157c-ev1 and stm32mp157c-dk2, i didn't see any regression
>> but i prefer to get expert feedback
>>
>> Thanks
>> Patrice
>>
>> On 8/15/22 16:52, Jorge Ramirez-Ortiz wrote:
>>> Sending the stop condition without waiting for transfer complete
>>> has been found to lock the bus (BUSY) when NACKF is raised.
>>>
>>> Tested accessing the NXP SE05X I2C device.
>>> https://www.nxp.com/docs/en/application-note/AN12399.pdf
>>>
>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
>>> Reviewed-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
>>> ---
>>>   drivers/i2c/stm32f7_i2c.c | 8 +++++---
>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>>

For reference, this patch is superseded by Alain Volmat patch:

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

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

in the serie "i2c: stm32: cleanup & stop handling fix"

http://patchwork.ozlabs.org/project/uboot/list/?series=317443&state=*


Regards


Patrick

Re: [PATCHv2 1/2] i2c: stm32f7: fix clearing the control register

[Patrick DELAUNAY]

Hi,

On 8/15/22 16:52, Jorge Ramirez-Ortiz wrote:
> Bits should be set to 0, not 1.
>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
> ---
>   drivers/i2c/stm32f7_i2c.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
> index bf2a6c9b4b..3a727e68ac 100644
> --- a/drivers/i2c/stm32f7_i2c.c
> +++ b/drivers/i2c/stm32f7_i2c.c
> @@ -413,7 +413,7 @@ static int stm32_i2c_check_end_of_message(struct stm32_i2c_priv *i2c_priv)
>   		setbits_le32(&regs->icr, STM32_I2C_ICR_STOPCF);
>   
>   		/* Clear control register 2 */
> -		setbits_le32(&regs->cr2, STM32_I2C_CR2_RESET_MASK);
> +		clrbits_le32(&regs->cr2, STM32_I2C_CR2_RESET_MASK);
>   	}
>   
>   	return ret;


Applied to u-boot-stm/master, thanks!

Regards
Patrick

[PATCHv2 1/2] i2c: stm32f7: fix clearing the control register

[Jorge Ramirez-Ortiz]

Bits should be set to 0, not 1.

Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

---
 drivers/i2c/stm32f7_i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/stm32f7_i2c.c b/drivers/i2c/stm32f7_i2c.c
index bf2a6c9b4b..3a727e68ac 100644
--- a/drivers/i2c/stm32f7_i2c.c
+++ b/drivers/i2c/stm32f7_i2c.c
@@ -413,7 +413,7 @@ static int stm32_i2c_check_end_of_message(struct stm32_i2c_priv *i2c_priv)
 		setbits_le32(&regs->icr, STM32_I2C_ICR_STOPCF);
 
 		/* Clear control register 2 */
-		setbits_le32(&regs->cr2, STM32_I2C_CR2_RESET_MASK);
+		clrbits_le32(&regs->cr2, STM32_I2C_CR2_RESET_MASK);
 	}
 
 	return ret;
-- 
2.34.1