diff mbox series

[v3,3/3] i2c: stm32: do not set the STOP condition on error

Message ID 20220909160627.1793406-4-alain.volmat@foss.st.com
State Superseded
Delegated to: Patrick Delaunay
Headers show
Series i2c: stm32: cleanup & stop handling fix | expand

Commit Message

Alain Volmat Sept. 9, 2022, 4:06 p.m. UTC 
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(-)

Comments

Patrick DELAUNAY Sept. 12, 2022, 8:19 a.m. UTC | #1 
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
Patrice CHOTARD Sept. 12, 2022, 8:27 a.m. UTC | #2 
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
Jorge Ramirez-Ortiz, Foundries Sept. 12, 2022, 8:35 a.m. UTC | #3 
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
> 
>
Patrick DELAUNAY Sept. 19, 2022, 3:46 p.m. UTC | #4 
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
diff mbox series

Patch

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);
 }