[PATCH] i2c: stm32f7: Fix PEC handling in case of SMBUS transfers

[PATCH] i2c: stm32f7: Fix PEC handling in case of SMBUS transfers

[Alain Volmat]

The PECBYTE bit allows to generate (in case of write) or
compute/compare the PEC byte (in case of read).  In case
of reading a value (performed by first sending a write
command, then followed by a read command) the PECBYTE should
only be set before starting the read command and not before
the first write command.

Fixes: 9e48155f6bfe ("i2c: i2c-stm32f7: Add initial SMBus protocols support")

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

diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
index 579b30581725..0d3c9a041b56 100644
--- a/drivers/i2c/busses/i2c-stm32f7.c
+++ b/drivers/i2c/busses/i2c-stm32f7.c
@@ -1059,9 +1059,10 @@ static int stm32f7_i2c_smbus_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,
 	/* Configure PEC */
 	if ((flags & I2C_CLIENT_PEC) && f7_msg->size != I2C_SMBUS_QUICK) {
 		cr1 |= STM32F7_I2C_CR1_PECEN;
-		cr2 |= STM32F7_I2C_CR2_PECBYTE;
-		if (!f7_msg->read_write)
+		if (!f7_msg->read_write) {
+			cr2 |= STM32F7_I2C_CR2_PECBYTE;
 			f7_msg->count++;
+		}
 	} else {
 		cr1 &= ~STM32F7_I2C_CR1_PECEN;
 		cr2 &= ~STM32F7_I2C_CR2_PECBYTE;
@@ -1149,8 +1150,10 @@ static void stm32f7_i2c_smbus_rep_start(struct stm32f7_i2c_dev *i2c_dev)
 	f7_msg->stop = true;
 
 	/* Add one byte for PEC if needed */
-	if (cr1 & STM32F7_I2C_CR1_PECEN)
+	if (cr1 & STM32F7_I2C_CR1_PECEN) {
+		cr2 |= STM32F7_I2C_CR2_PECBYTE;
 		f7_msg->count++;
+	}
 
 	/* Set number of bytes to be transferred */
 	cr2 &= ~(STM32F7_I2C_CR2_NBYTES_MASK);
-- 
2.25.1

Re: [PATCH] i2c: stm32f7: Fix PEC handling in case of SMBUS transfers

[Pierre Yves MORDRET]

Hi Alain,

Sounds good to me

Reviewed-by: Pierre-Yves MORDRET <pierre-yves.mordret@foss.st.com>

Regards

On 10/2/23 10:42, Alain Volmat wrote:
> The PECBYTE bit allows to generate (in case of write) or
> compute/compare the PEC byte (in case of read).  In case
> of reading a value (performed by first sending a write
> command, then followed by a read command) the PECBYTE should
> only be set before starting the read command and not before
> the first write command.
> 
> Fixes: 9e48155f6bfe ("i2c: i2c-stm32f7: Add initial SMBus protocols support")
> 
> Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
> ---
>  drivers/i2c/busses/i2c-stm32f7.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
> index 579b30581725..0d3c9a041b56 100644
> --- a/drivers/i2c/busses/i2c-stm32f7.c
> +++ b/drivers/i2c/busses/i2c-stm32f7.c
> @@ -1059,9 +1059,10 @@ static int stm32f7_i2c_smbus_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,
>  	/* Configure PEC */
>  	if ((flags & I2C_CLIENT_PEC) && f7_msg->size != I2C_SMBUS_QUICK) {
>  		cr1 |= STM32F7_I2C_CR1_PECEN;
> -		cr2 |= STM32F7_I2C_CR2_PECBYTE;
> -		if (!f7_msg->read_write)
> +		if (!f7_msg->read_write) {
> +			cr2 |= STM32F7_I2C_CR2_PECBYTE;
>  			f7_msg->count++;
> +		}
>  	} else {
>  		cr1 &= ~STM32F7_I2C_CR1_PECEN;
>  		cr2 &= ~STM32F7_I2C_CR2_PECBYTE;
> @@ -1149,8 +1150,10 @@ static void stm32f7_i2c_smbus_rep_start(struct stm32f7_i2c_dev *i2c_dev)
>  	f7_msg->stop = true;
>  
>  	/* Add one byte for PEC if needed */
> -	if (cr1 & STM32F7_I2C_CR1_PECEN)
> +	if (cr1 & STM32F7_I2C_CR1_PECEN) {
> +		cr2 |= STM32F7_I2C_CR2_PECBYTE;
>  		f7_msg->count++;
> +	}
>  
>  	/* Set number of bytes to be transferred */
>  	cr2 &= ~(STM32F7_I2C_CR2_NBYTES_MASK);

-- 
--
~ Py MORDRET
--

Re: [PATCH] i2c: stm32f7: Fix PEC handling in case of SMBUS transfers

[Andi Shyti]

Hi Alain,

On Mon, Oct 02, 2023 at 10:42:10AM +0200, Alain Volmat wrote:
> The PECBYTE bit allows to generate (in case of write) or
> compute/compare the PEC byte (in case of read).  In case
> of reading a value (performed by first sending a write
> command, then followed by a read command) the PECBYTE should
> only be set before starting the read command and not before
> the first write command.

What is this patch fixing?

Can you please point this detail in the documentation, I haven't
found it[*]

> Fixes: 9e48155f6bfe ("i2c: i2c-stm32f7: Add initial SMBus protocols support")
> 
> Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>

please, don't leave blank lines between tags.

Thanks,
Andi

[*] Hope this is the correct one:
https://www.st.com/resource/en/reference_manual/rm0385-stm32f75xxx-and-stm32f74xxx-advanced-armbased-32bit-mcus-stmicroelectronics.pdf

> ---
>  drivers/i2c/busses/i2c-stm32f7.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
> index 579b30581725..0d3c9a041b56 100644
> --- a/drivers/i2c/busses/i2c-stm32f7.c
> +++ b/drivers/i2c/busses/i2c-stm32f7.c
> @@ -1059,9 +1059,10 @@ static int stm32f7_i2c_smbus_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,
>  	/* Configure PEC */
>  	if ((flags & I2C_CLIENT_PEC) && f7_msg->size != I2C_SMBUS_QUICK) {
>  		cr1 |= STM32F7_I2C_CR1_PECEN;
> -		cr2 |= STM32F7_I2C_CR2_PECBYTE;
> -		if (!f7_msg->read_write)
> +		if (!f7_msg->read_write) {
> +			cr2 |= STM32F7_I2C_CR2_PECBYTE;
>  			f7_msg->count++;
> +		}
>  	} else {
>  		cr1 &= ~STM32F7_I2C_CR1_PECEN;
>  		cr2 &= ~STM32F7_I2C_CR2_PECBYTE;
> @@ -1149,8 +1150,10 @@ static void stm32f7_i2c_smbus_rep_start(struct stm32f7_i2c_dev *i2c_dev)
>  	f7_msg->stop = true;
>  
>  	/* Add one byte for PEC if needed */
> -	if (cr1 & STM32F7_I2C_CR1_PECEN)
> +	if (cr1 & STM32F7_I2C_CR1_PECEN) {
> +		cr2 |= STM32F7_I2C_CR2_PECBYTE;
>  		f7_msg->count++;
> +	}
>  
>  	/* Set number of bytes to be transferred */
>  	cr2 &= ~(STM32F7_I2C_CR2_NBYTES_MASK);
> -- 
> 2.25.1
> 

Re: [PATCH] i2c: stm32f7: Fix PEC handling in case of SMBUS transfers

[Alain Volmat]

Hi Andi,

Thanks for the review.

On Tue, Oct 03, 2023 at 07:42:46PM +0200, Andi Shyti wrote:
> Hi Alain,
> 
> On Mon, Oct 02, 2023 at 10:42:10AM +0200, Alain Volmat wrote:
> > The PECBYTE bit allows to generate (in case of write) or
> > compute/compare the PEC byte (in case of read).  In case
> > of reading a value (performed by first sending a write
> > command, then followed by a read command) the PECBYTE should
> > only be set before starting the read command and not before
> > the first write command.
> 
> What is this patch fixing?
> 
> Can you please point this detail in the documentation, I haven't
> found it[*]

This is about the handling of the PECBYTE bit of the I2C_CR2 register
(cf page 1010 of the spec you pointed).  There were no issue in case
of performing SMBUS write (with PEC), however read was not working.
PECBYTE was set from the very beginning of the transaction, but since
SMBUS read is first made of a write transfer, followed by a read transfer,
the PECBYTE was appended to the end of the write transfer (instead of the read
transfer), leading to lose of the last byte of the write transfer.
(in addition to the fact that the PEC byte should NOT be placed at the
end of the write transfer).
(cf Figure 30 of SMBUS specification [1]).

I could add more information within the commit log if you prefer.

[1] http://www.smbus.org/specs/SMBus_3_2_20220112.pdf

> 
> > Fixes: 9e48155f6bfe ("i2c: i2c-stm32f7: Add initial SMBus protocols support")
> > 
> > Signed-off-by: Alain Volmat <alain.volmat@foss.st.com>
> 
> please, don't leave blank lines between tags.

Ok,  will remove this blank line within a v2.

Thanks,
Alain

> 
> Thanks,
> Andi
> 
> [*] Hope this is the correct one:
> https://www.st.com/resource/en/reference_manual/rm0385-stm32f75xxx-and-stm32f74xxx-advanced-armbased-32bit-mcus-stmicroelectronics.pdf
> 
> > ---
> >  drivers/i2c/busses/i2c-stm32f7.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-stm32f7.c b/drivers/i2c/busses/i2c-stm32f7.c
> > index 579b30581725..0d3c9a041b56 100644
> > --- a/drivers/i2c/busses/i2c-stm32f7.c
> > +++ b/drivers/i2c/busses/i2c-stm32f7.c
> > @@ -1059,9 +1059,10 @@ static int stm32f7_i2c_smbus_xfer_msg(struct stm32f7_i2c_dev *i2c_dev,
> >  	/* Configure PEC */
> >  	if ((flags & I2C_CLIENT_PEC) && f7_msg->size != I2C_SMBUS_QUICK) {
> >  		cr1 |= STM32F7_I2C_CR1_PECEN;
> > -		cr2 |= STM32F7_I2C_CR2_PECBYTE;
> > -		if (!f7_msg->read_write)
> > +		if (!f7_msg->read_write) {
> > +			cr2 |= STM32F7_I2C_CR2_PECBYTE;
> >  			f7_msg->count++;
> > +		}
> >  	} else {
> >  		cr1 &= ~STM32F7_I2C_CR1_PECEN;
> >  		cr2 &= ~STM32F7_I2C_CR2_PECBYTE;
> > @@ -1149,8 +1150,10 @@ static void stm32f7_i2c_smbus_rep_start(struct stm32f7_i2c_dev *i2c_dev)
> >  	f7_msg->stop = true;
> >  
> >  	/* Add one byte for PEC if needed */
> > -	if (cr1 & STM32F7_I2C_CR1_PECEN)
> > +	if (cr1 & STM32F7_I2C_CR1_PECEN) {
> > +		cr2 |= STM32F7_I2C_CR2_PECBYTE;
> >  		f7_msg->count++;
> > +	}
> >  
> >  	/* Set number of bytes to be transferred */
> >  	cr2 &= ~(STM32F7_I2C_CR2_NBYTES_MASK);
> > -- 
> > 2.25.1
> >