[PATCH] i2c: imx: fix emulated smbus block read

[PATCH] i2c: imx: fix emulated smbus block read

[Lukasz Kucharczyk]

Acknowledge the byte count submitted by the target.
When I2C_SMBUS_BLOCK_DATA read operation is executed by
i2c_smbus_xfer_emulated(), the length of the second (read) message is set
to 1. Length of the block is supposed to be obtained from the target by the
underlying bus driver.
The i2c_imx_isr_read() function should emit the acknowledge on i2c bus
after reading the first byte (i.e., byte count) while processing such
message (as defined in Section 6.5.7 of System Management Bus
Specification [1]). Without this acknowledge, the target does not submit
subsequent bytes and the controller only reads 0xff's.

In addition, store the length of block data obtained from the target in
the buffer provided by i2c_smbus_xfer_emulated() - otherwise the first
byte of actual data is erroneously interpreted as length of the data
block.

[1] https://smbus.org/specs/SMBus_3_3_20240512.pdf

Fixes: 5f5c2d4579ca ("i2c: imx: prevent rescheduling in non dma mode")
Signed-off-by: Lukasz Kucharczyk <lukasz.kucharczyk@leica-geosystems.com>
---
 drivers/i2c/busses/i2c-imx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index ee0d25b498cb..4bf550a3b98d 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -1008,7 +1008,7 @@ static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
 	/* setup bus to read data */
 	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 	temp &= ~I2CR_MTX;
-	if (i2c_imx->msg->len - 1)
+	if ((i2c_imx->msg->len - 1) || (i2c_imx->msg->flags & I2C_M_RECV_LEN))
 		temp &= ~I2CR_TXAK;
 
 	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
@@ -1063,6 +1063,7 @@ static inline void i2c_imx_isr_read_block_data_len(struct imx_i2c_struct *i2c_im
 		wake_up(&i2c_imx->queue);
 	}
 	i2c_imx->msg->len += len;
+	i2c_imx->msg->buf[i2c_imx->msg_buf_idx++] = len;
 }
 
 static irqreturn_t i2c_imx_master_isr(struct imx_i2c_struct *i2c_imx, unsigned int status)
-- 
2.34.1

RE: [PATCH] i2c: imx: fix emulated smbus block read

[Carlos Song]

> -----Original Message-----
> From: Lukasz Kucharczyk <lukasz.kucharczyk@leica-geosystems.com>
> Sent: Tuesday, May 20, 2025 8:23 PM
> To: Oleksij Rempel <o.rempel@pengutronix.de>;
> stefan.eichenberger@toradex.com; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Andi Shyti <andi.shyti@kernel.org>; Shawn Guo
> <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>; Fabio
> Estevam <festevam@gmail.com>
> Cc: open list:FREESCALE IMX I2C DRIVER <linux-i2c@vger.kernel.org>; open
> list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE <imx@lists.linux.dev>;
> moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> <linux-arm-kernel@lists.infradead.org>; open list
> <linux-kernel@vger.kernel.org>; bsp-development.geo@leica-geosystems.com;
> customers.leicageo@pengutronix.de; Lukasz Kucharczyk
> <lukasz.kucharczyk@leica-geosystems.com>
> Subject: [EXT] [PATCH] i2c: imx: fix emulated smbus block read
> 
> [You don't often get email from lukasz.kucharczyk@leica-geosystems.com.
> Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> Acknowledge the byte count submitted by the target.
> When I2C_SMBUS_BLOCK_DATA read operation is executed by
> i2c_smbus_xfer_emulated(), the length of the second (read) message is set to 1.
> Length of the block is supposed to be obtained from the target by the
> underlying bus driver.
> The i2c_imx_isr_read() function should emit the acknowledge on i2c bus after
> reading the first byte (i.e., byte count) while processing such message (as
> defined in Section 6.5.7 of System Management Bus Specification [1]). Without
> this acknowledge, the target does not submit subsequent bytes and the
> controller only reads 0xff's.
> 
> In addition, store the length of block data obtained from the target in the
> buffer provided by i2c_smbus_xfer_emulated() - otherwise the first byte of
> actual data is erroneously interpreted as length of the data block.
> 
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsmbus.
> org%2Fspecs%2FSMBus_3_3_20240512.pdf&data=05%7C02%7Ccarlos.song%
> 40nxp.com%7Cd05bf48873224466159808dd97991d7d%7C686ea1d3bc2b4c
> 6fa92cd99c5c301635%7C0%7C1%7C638833406066875925%7CUnknown%7C
> TWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXa
> W4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=dmTE
> obQKzUzw03sEd%2BDtUo3se7kJ9ZTS4atfs0lGLC8%3D&reserved=0
> 
> Fixes: 5f5c2d4579ca ("i2c: imx: prevent rescheduling in non dma mode")
> Signed-off-by: Lukasz Kucharczyk <lukasz.kucharczyk@leica-geosystems.com>

Hi,

Looks this is a nice fix.

I2C SMBUS block read need first read one byte from data length offset then I2C will know how many bytes
need to continue read. For this issue you can meet " Error: Read failed " when using i2cget -f -y bus address offset s to test.

So you apply this change to make i2c-imx controller can behavior like this:

S Addr Wr [A] Comm [A] Sr Addr Rd [A] [Count] A [Data] A [Data] A ... A [Data] NA P

Do I understand this right?

Carlos
> ---
>  drivers/i2c/busses/i2c-imx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index
> ee0d25b498cb..4bf550a3b98d 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1008,7 +1008,7 @@ static inline int i2c_imx_isr_read(struct
> imx_i2c_struct *i2c_imx)
>         /* setup bus to read data */
>         temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>         temp &= ~I2CR_MTX;
> -       if (i2c_imx->msg->len - 1)
> +       if ((i2c_imx->msg->len - 1) || (i2c_imx->msg->flags &
> + I2C_M_RECV_LEN))
>                 temp &= ~I2CR_TXAK;
> 
>         imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); @@ -1063,6
> +1063,7 @@ static inline void i2c_imx_isr_read_block_data_len(struct
> imx_i2c_struct *i2c_im
>                 wake_up(&i2c_imx->queue);
>         }
>         i2c_imx->msg->len += len;
> +       i2c_imx->msg->buf[i2c_imx->msg_buf_idx++] = len;
>  }
> 
>  static irqreturn_t i2c_imx_master_isr(struct imx_i2c_struct *i2c_imx,
> unsigned int status)
> --
> 2.34.1
> 

RE: [PATCH] i2c: imx: fix emulated smbus block read

[KUCHARCZYK Lukasz]

> -----Original Message-----
> From: Carlos Song <carlos.song@nxp.com>
> Sent: 27 May 2025 12:46
> Subject: RE: [PATCH] i2c: imx: fix emulated smbus block read
> 
> I2C SMBUS block read need first read one byte from data length offset then
> I2C will know how many bytes need to continue read. For this issue you can
> meet " Error: Read failed " when using i2cget -f -y bus address offset s to test.
> 
> So you apply this change to make i2c-imx controller can behavior like this:
> 
> S Addr Wr [A] Comm [A] Sr Addr Rd [A] [Count] A [Data] A [Data] A ... A
> [Data] NA P
> 
> Do I understand this right?

hi Carlos; thanks for the message! Yes, exactly, that's correct.

I run into this issue while trying to integrate a smart battery into a
IMX8-based system. Fetching of properties that rely on data block read
operation (i.e., ManufacturerName, DeviceName,  DeviceChemistry and
ManufacturerData) was failing.

With the fix the block read looks just like you described.


Without the fix, the transaction on the bus looked somehow like:
S Addr Wr [A] Comm [A] Sr Addr Rd [A] [Count] NA [0xff] NA [0xff] NA ... [0xff] NA P

(i.e., the Count is not acknowledged and SDA remains high afterwards).

Lukasz

RE: [PATCH] i2c: imx: fix emulated smbus block read

[Carlos Song]

> -----Original Message-----
> From: KUCHARCZYK Lukasz <lukasz.kucharczyk@leica-geosystems.com>
> Sent: Tuesday, May 27, 2025 8:03 PM
> To: Carlos Song <carlos.song@nxp.com>; Oleksij Rempel
> <o.rempel@pengutronix.de>; stefan.eichenberger@toradex.com; Pengutronix
> Kernel Team <kernel@pengutronix.de>; Andi Shyti <andi.shyti@kernel.org>;
> Shawn Guo <shawnguo@kernel.org>; Sascha Hauer <s.hauer@pengutronix.de>;
> Fabio Estevam <festevam@gmail.com>
> Cc: open list:FREESCALE IMX I2C DRIVER <linux-i2c@vger.kernel.org>; open
> list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE <imx@lists.linux.dev>;
> moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
> <linux-arm-kernel@lists.infradead.org>; open list
> <linux-kernel@vger.kernel.org>; GEO-CHHER-bsp-development
> <bsp-development.geo@leica-geosystems.com>;
> customers.leicageo@pengutronix.de
> Subject: [EXT] RE: [PATCH] i2c: imx: fix emulated smbus block read
> 
> [You don't often get email from lukasz.kucharczyk@leica-geosystems.com.
> Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification ]
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> > -----Original Message-----
> > From: Carlos Song <carlos.song@nxp.com>
> > Sent: 27 May 2025 12:46
> > Subject: RE: [PATCH] i2c: imx: fix emulated smbus block read
> >
> > I2C SMBUS block read need first read one byte from data length offset
> > then I2C will know how many bytes need to continue read. For this
> > issue you can meet " Error: Read failed " when using i2cget -f -y bus address
> offset s to test.
> >
> > So you apply this change to make i2c-imx controller can behavior like this:
> >
> > S Addr Wr [A] Comm [A] Sr Addr Rd [A] [Count] A [Data] A [Data] A ...
> > A [Data] NA P
> >
> > Do I understand this right?
> 
> hi Carlos; thanks for the message! Yes, exactly, that's correct.
> 
> I run into this issue while trying to integrate a smart battery into a IMX8-based
> system. Fetching of properties that rely on data block read operation (i.e.,
> ManufacturerName, DeviceName,  DeviceChemistry and
> ManufacturerData) was failing.
> 
> With the fix the block read looks just like you described.
> 
> 
> Without the fix, the transaction on the bus looked somehow like:
> S Addr Wr [A] Comm [A] Sr Addr Rd [A] [Count] NA [0xff] NA [0xff] NA ... [0xff]
> NA P
> 
> (i.e., the Count is not acknowledged and SDA remains high afterwards).
> 

Good, thank you for your work!
Reviewed-by: Carlos Song <carlos.song@nxp.com>

> Lukasz

Re: [PATCH] i2c: imx: fix emulated smbus block read

[Stefan Eichenberger]

Hi Lukasz,

On Tue, May 20, 2025 at 02:22:52PM +0200, Lukasz Kucharczyk wrote:
> Acknowledge the byte count submitted by the target.
> When I2C_SMBUS_BLOCK_DATA read operation is executed by
> i2c_smbus_xfer_emulated(), the length of the second (read) message is set
> to 1. Length of the block is supposed to be obtained from the target by the
> underlying bus driver.
> The i2c_imx_isr_read() function should emit the acknowledge on i2c bus
> after reading the first byte (i.e., byte count) while processing such
> message (as defined in Section 6.5.7 of System Management Bus
> Specification [1]). Without this acknowledge, the target does not submit
> subsequent bytes and the controller only reads 0xff's.
> 
> In addition, store the length of block data obtained from the target in
> the buffer provided by i2c_smbus_xfer_emulated() - otherwise the first
> byte of actual data is erroneously interpreted as length of the data
> block.
> 
> [1] https://smbus.org/specs/SMBus_3_3_20240512.pdf
> 
> Fixes: 5f5c2d4579ca ("i2c: imx: prevent rescheduling in non dma mode")
> Signed-off-by: Lukasz Kucharczyk <lukasz.kucharczyk@leica-geosystems.com>
> ---
>  drivers/i2c/busses/i2c-imx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index ee0d25b498cb..4bf550a3b98d 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1008,7 +1008,7 @@ static inline int i2c_imx_isr_read(struct imx_i2c_struct *i2c_imx)
>  	/* setup bus to read data */
>  	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>  	temp &= ~I2CR_MTX;
> -	if (i2c_imx->msg->len - 1)
> +	if ((i2c_imx->msg->len - 1) || (i2c_imx->msg->flags & I2C_M_RECV_LEN))
>  		temp &= ~I2CR_TXAK;
>  
>  	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> @@ -1063,6 +1063,7 @@ static inline void i2c_imx_isr_read_block_data_len(struct imx_i2c_struct *i2c_im
>  		wake_up(&i2c_imx->queue);
>  	}
>  	i2c_imx->msg->len += len;
> +	i2c_imx->msg->buf[i2c_imx->msg_buf_idx++] = len;
>  }
>  
>  static irqreturn_t i2c_imx_master_isr(struct imx_i2c_struct *i2c_imx, unsigned int status)

This makes sense, we never tested the actual SMBus emulation. Thanks a lot for the fix.

Reviewed-by: Stefan Eichenberger <eichest@gmail.com>

Re: [PATCH] i2c: imx: fix emulated smbus block read

[Oleksij Rempel]

On Tue, May 20, 2025 at 02:22:52PM +0200, Lukasz Kucharczyk wrote:
> Acknowledge the byte count submitted by the target.
> When I2C_SMBUS_BLOCK_DATA read operation is executed by
> i2c_smbus_xfer_emulated(), the length of the second (read) message is set
> to 1. Length of the block is supposed to be obtained from the target by the
> underlying bus driver.
> The i2c_imx_isr_read() function should emit the acknowledge on i2c bus
> after reading the first byte (i.e., byte count) while processing such
> message (as defined in Section 6.5.7 of System Management Bus
> Specification [1]). Without this acknowledge, the target does not submit
> subsequent bytes and the controller only reads 0xff's.
> 
> In addition, store the length of block data obtained from the target in
> the buffer provided by i2c_smbus_xfer_emulated() - otherwise the first
> byte of actual data is erroneously interpreted as length of the data
> block.
> 
> [1] https://smbus.org/specs/SMBus_3_3_20240512.pdf
> 
> Fixes: 5f5c2d4579ca ("i2c: imx: prevent rescheduling in non dma mode")
> Signed-off-by: Lukasz Kucharczyk <lukasz.kucharczyk@leica-geosystems.com>
 
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you!

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] i2c: imx: fix emulated smbus block read

[Andi Shyti]

Hi Lukasz,

On Tue, May 20, 2025 at 02:22:52PM +0200, Lukasz Kucharczyk wrote:
> Acknowledge the byte count submitted by the target.
> When I2C_SMBUS_BLOCK_DATA read operation is executed by
> i2c_smbus_xfer_emulated(), the length of the second (read) message is set
> to 1. Length of the block is supposed to be obtained from the target by the
> underlying bus driver.
> The i2c_imx_isr_read() function should emit the acknowledge on i2c bus
> after reading the first byte (i.e., byte count) while processing such
> message (as defined in Section 6.5.7 of System Management Bus
> Specification [1]). Without this acknowledge, the target does not submit
> subsequent bytes and the controller only reads 0xff's.
> 
> In addition, store the length of block data obtained from the target in
> the buffer provided by i2c_smbus_xfer_emulated() - otherwise the first
> byte of actual data is erroneously interpreted as length of the data
> block.
> 
> [1] https://smbus.org/specs/SMBus_3_3_20240512.pdf
> 
> Fixes: 5f5c2d4579ca ("i2c: imx: prevent rescheduling in non dma mode")
> Signed-off-by: Lukasz Kucharczyk <lukasz.kucharczyk@leica-geosystems.com>

merged to i2c/i2c-host-fixes.

Thanks,
Andi