[PATCH 1/2] i2c: imx-lpi2c: fix SMBus block read with PEC

[PATCH 1/2] i2c: imx-lpi2c: fix SMBus block read with PEC

[Michael Walle]

If PEC is enabled, the I2C core sets msgs->len to 2 instead of 1. The
length calculation for the second receive command assumes it's always
1. This will lead to a short read and eventually a timeout because the
PEC byte is never received.

Take msgs->len into account when calculating the remaining bytes.

This was tested on a board which uses an IPMI SSIF BMC.

Fixes: efdc383d1cc2 ("i2c: imx-lpi2c: fix SMBus block read NACK after byte count")
Signed-off-by: Michael Walle <mwalle@kernel.org>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 7302464c371a..f57e800ac3b5 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -563,7 +563,7 @@ static unsigned int lpi2c_SMBus_block_read_length_byte(struct lpi2c_imx_struct *
 static int lpi2c_imx_read_init(struct lpi2c_imx_struct *lpi2c_imx,
 			       struct i2c_msg *msgs)
 {
-	unsigned int temp, val, block_len;
+	unsigned int temp, val, block_len, remaining;
 	int ret;
 
 	lpi2c_imx->rx_buf = msgs->buf;
@@ -612,9 +612,13 @@ static int lpi2c_imx_read_init(struct lpi2c_imx_struct *lpi2c_imx,
 		 * keep MTDR non-empty and ensuring continuous ACKs. Only update command register
 		 * here. All block bytes will be read out at IRQ handler or lpi2c_imx_read_atomic()
 		 * function.
+		 *
+		 * msgs->len is 1 - or if PEC enabled msgs->len is 2. Two bytes
+		 * have already been requested.
 		 */
-		if (block_len > 1)
-			writel((RECV_DATA << 8) | (block_len - 2), lpi2c_imx->base + LPI2C_MTDR);
+		remaining = msgs->len + block_len - 2;
+		if (remaining)
+			writel((RECV_DATA << 8) | (remaining - 1), lpi2c_imx->base + LPI2C_MTDR);
 
 		lpi2c_imx->msglen += block_len;
 		msgs->len += block_len;
-- 
2.47.3

[PATCH 2/2] i2c: imx-lpi2c: don't treat a NACK in SMBus block read as an error

[Michael Walle]

An SMBus device might indicate that a value isn't ready yet by asserting
a NACK.
This will lead to the following error message which will flood the
kernel log for some device like an IPMI SSIF BMC:

   [    4.277005] i2c i2c-5: SMBus read count failed -110

Stop the polling if NDF is set and return -EIO, just like everywhere
else. While at it, drop the MSR_RDF_ASSERTED() macro, it's just one bit,
no need for FIELD_GET.

Fixes: efdc383d1cc2 ("i2c: imx-lpi2c: fix SMBus block read NACK after byte count")
Signed-off-by: Michael Walle <mwalle@kernel.org>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index f57e800ac3b5..583155802e8a 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -92,7 +92,6 @@
 #define MRDR_RXEMPTY	BIT(14)
 #define MDER_TDDE	BIT(0)
 #define MDER_RDDE	BIT(1)
-#define MSR_RDF_ASSERTED(x) FIELD_GET(MSR_RDF, (x))
 
 #define SCR_SEN		BIT(0)
 #define SCR_RST		BIT(1)
@@ -594,7 +593,10 @@ static int lpi2c_imx_read_init(struct lpi2c_imx_struct *lpi2c_imx,
 		writel((RECV_DATA << 8) | 0x01, lpi2c_imx->base + LPI2C_MTDR);
 
 		ret = readl_poll_timeout(lpi2c_imx->base + LPI2C_MSR, val,
-					 MSR_RDF_ASSERTED(val), 1, 1000);
+					 (val & (MSR_RDF | MSR_NDF)), 1, 1000);
+		if (val & MSR_NDF)
+			return -EIO;
+
 		if (ret) {
 			dev_err(&lpi2c_imx->adapter.dev, "SMBus read count failed %d\n", ret);
 			return ret;
-- 
2.47.3

Re: [PATCH 2/2] i2c: imx-lpi2c: don't treat a NACK in SMBus block read as an error

[Carlos Song]

> An SMBus device might indicate that a value isn't ready yet by asserting
> a NACK.
> This will lead to the following error message which will flood the
> kernel log for some device like an IPMI SSIF BMC:
> 
>    [    4.277005] i2c i2c-5: SMBus read count failed -110
> 
> Stop the polling if NDF is set and return -EIO, just like everywhere
> else. While at it, drop the MSR_RDF_ASSERTED() macro, it's just one bit,
> no need for FIELD_GET.
> 
> Fixes: efdc383d1cc2 ("i2c: imx-lpi2c: fix SMBus block read NACK after byte count")
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> ---
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index f57e800ac3b5..583155802e8a 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -92,7 +92,6 @@
>  #define MRDR_RXEMPTY	BIT(14)
>  #define MDER_TDDE	BIT(0)
>  #define MDER_RDDE	BIT(1)
> -#define MSR_RDF_ASSERTED(x) FIELD_GET(MSR_RDF, (x))
>  
>  #define SCR_SEN		BIT(0)
>  #define SCR_RST		BIT(1)
> @@ -594,7 +593,10 @@ static int lpi2c_imx_read_init(struct lpi2c_imx_struct *lpi2c_imx,
>  		writel((RECV_DATA << 8) | 0x01, lpi2c_imx->base + LPI2C_MTDR);
>  
>  		ret = readl_poll_timeout(lpi2c_imx->base + LPI2C_MSR, val,
> -					 MSR_RDF_ASSERTED(val), 1, 1000);
> +					 (val & (MSR_RDF | MSR_NDF)), 1, 1000);
> +		if (val & MSR_NDF)
> +			return -EIO;
> +
>  		if (ret) {
>  			dev_err(&lpi2c_imx->adapter.dev, "SMBus read count failed %d\n", ret);
>  			return ret;

LGTM,
Acked-by: Carlos Song <carlos.song@nxp.com>
-- 
Best Regards,
Carlos

Re: [PATCH 1/2] i2c: imx-lpi2c: fix SMBus block read with PEC

[Carlos Song]

> If PEC is enabled, the I2C core sets msgs->len to 2 instead of 1. The
> length calculation for the second receive command assumes it's always
> 1. This will lead to a short read and eventually a timeout because the
> PEC byte is never received.
> 
> Take msgs->len into account when calculating the remaining bytes.
> 
> This was tested on a board which uses an IPMI SSIF BMC.
> 
> Fixes: efdc383d1cc2 ("i2c: imx-lpi2c: fix SMBus block read NACK after byte count")
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> ---
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index 7302464c371a..f57e800ac3b5 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -563,7 +563,7 @@ static unsigned int lpi2c_SMBus_block_read_length_byte(struct lpi2c_imx_struct *
>  static int lpi2c_imx_read_init(struct lpi2c_imx_struct *lpi2c_imx,
>  			       struct i2c_msg *msgs)
>  {
> -	unsigned int temp, val, block_len;
> +	unsigned int temp, val, block_len, remaining;
>  	int ret;
>  
>  	lpi2c_imx->rx_buf = msgs->buf;
> @@ -612,9 +612,13 @@ static int lpi2c_imx_read_init(struct lpi2c_imx_struct *lpi2c_imx,
>  		 * keep MTDR non-empty and ensuring continuous ACKs. Only update command register
>  		 * here. All block bytes will be read out at IRQ handler or lpi2c_imx_read_atomic()
>  		 * function.
> +		 *
> +		 * msgs->len is 1 - or if PEC enabled msgs->len is 2. Two bytes
> +		 * have already been requested.
>  		 */
> -		if (block_len > 1)
> -			writel((RECV_DATA << 8) | (block_len - 2), lpi2c_imx->base + LPI2C_MTDR);
> +		remaining = msgs->len + block_len - 2;
> +		if (remaining)
> +			writel((RECV_DATA << 8) | (remaining - 1), lpi2c_imx->base + LPI2C_MTDR);

Hi Michael,

Just to confirm my understanding:

For SMBus block read with PEC enabled, the PEC (CRC-8) byte is appended
at the end of the transfer. In that case the I2C core sets msgs->len to
2 instead of 1.

The previous code still calculated the remaining receive count assuming
msgs->len == 1, which means the PEC byte was not included in the total
number of bytes requested from the controller.

As a result, the last PEC byte was never read out, leading to a short
read and eventually a timeout.

Is that the issue this patch is fixing?

>  
>  		lpi2c_imx->msglen += block_len;
>  		msgs->len += block_len;

-- 
Best Regards,
Carlos

Re: [PATCH 1/2] i2c: imx-lpi2c: fix SMBus block read with PEC

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 2631 bytes --]

Hi Carlos,

On Thu Nov 5, 2026 at 9:29 AM CET, Carlos Song wrote:
>
>
>> If PEC is enabled, the I2C core sets msgs->len to 2 instead of 1. The
>> length calculation for the second receive command assumes it's always
>> 1. This will lead to a short read and eventually a timeout because the
>> PEC byte is never received.
>> 
>> Take msgs->len into account when calculating the remaining bytes.
>> 
>> This was tested on a board which uses an IPMI SSIF BMC.
>> 
>> Fixes: efdc383d1cc2 ("i2c: imx-lpi2c: fix SMBus block read NACK after byte count")
>> Signed-off-by: Michael Walle <mwalle@kernel.org>
>> ---
>>  drivers/i2c/busses/i2c-imx-lpi2c.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
>> index 7302464c371a..f57e800ac3b5 100644
>> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
>> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
>> @@ -563,7 +563,7 @@ static unsigned int lpi2c_SMBus_block_read_length_byte(struct lpi2c_imx_struct *
>>  static int lpi2c_imx_read_init(struct lpi2c_imx_struct *lpi2c_imx,
>>  			       struct i2c_msg *msgs)
>>  {
>> -	unsigned int temp, val, block_len;
>> +	unsigned int temp, val, block_len, remaining;
>>  	int ret;
>>  
>>  	lpi2c_imx->rx_buf = msgs->buf;
>> @@ -612,9 +612,13 @@ static int lpi2c_imx_read_init(struct lpi2c_imx_struct *lpi2c_imx,
>>  		 * keep MTDR non-empty and ensuring continuous ACKs. Only update command register
>>  		 * here. All block bytes will be read out at IRQ handler or lpi2c_imx_read_atomic()
>>  		 * function.
>> +		 *
>> +		 * msgs->len is 1 - or if PEC enabled msgs->len is 2. Two bytes
>> +		 * have already been requested.
>>  		 */
>> -		if (block_len > 1)
>> -			writel((RECV_DATA << 8) | (block_len - 2), lpi2c_imx->base + LPI2C_MTDR);
>> +		remaining = msgs->len + block_len - 2;
>> +		if (remaining)
>> +			writel((RECV_DATA << 8) | (remaining - 1), lpi2c_imx->base + LPI2C_MTDR);
>
> Hi Michael,
>
> Just to confirm my understanding:
>
> For SMBus block read with PEC enabled, the PEC (CRC-8) byte is appended
> at the end of the transfer. In that case the I2C core sets msgs->len to
> 2 instead of 1.
>
> The previous code still calculated the remaining receive count assuming
> msgs->len == 1, which means the PEC byte was not included in the total
> number of bytes requested from the controller.
>
> As a result, the last PEC byte was never read out, leading to a short
> read and eventually a timeout.
>
> Is that the issue this patch is fixing?


Yes exactly.

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]