| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aRyieUbSqofjEm3a@lizhi-Precision-Tower-5810>
Date: Tue, 18 Nov 2025 11:44:41 -0500
From: Frank Li <Frank.li@....com>
To: Carlos Song <carlos.song@....com>
Cc: aisheng.dong@....com, andi.shyti@...nel.org, shawnguo@...nel.org,
s.hauer@...gutronix.de, kernel@...gutronix.de, festevam@...il.com,
pandy.gao@....com, wsa@...nel.org, vz@...ia.com,
B38611@...escale.com, linux-i2c@...r.kernel.org,
imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] i2c: imx-lpi2c: support smbus block read feature
On Tue, Nov 18, 2025 at 07:13:23PM +0800, Carlos Song wrote:
> The LPI2C controller automatically transmits a NACK on the last byte
> of a receive data command. It transmits the NACK unless the next
> command in the TXFIFO is also a receive data command. If the transmit
> FIFO is empty when a receive data command completes, a NACK is also
> automatically transmitted.
>
> Specially set read 2 bytes command initially. When the RXFIFO receives
> count data, controller should immediately read out this value and update
> MTDR register to keep the TXFIFO not empty. Set new receive data command
> to read other data before the 2nd byte is returned.
>
> Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> Signed-off-by: Carlos Song <carlos.song@....com>
>
> ---
> I setup an env to test this feature change:
>
> On I.MX93-EVK:
> LPI2C5 as slave device and use i2ctool to redesign the slave-eeprom mem:
>
> hexdump -C /sys/bus/i2c/devices/5-1064/slave-eeprom
>
> root@...93evk:~# hexdump -C /sys/bus/i2c/devices/5-1064/slave-eeprom
> 00000000 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
> 00000010 20 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e | ...............|
> 00000020 0f 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e |................|
> 00000030 1f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
> 00000040 08 00 01 02 03 04 05 06 07 ff ff ff ff ff ff ff |................|
> 00000050 00 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
> 00000060 40 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |@...............|
> 00000070 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
>
> case1: SMBus block read 0x10 register. SMBus len = 32 bytes.
> case2: SMBus block read 0x40 register. SMBus len = 8 bytes.
> case3: SMBus block read 0x50 register. SMBus len = 0 bytes. It break the
> SMBus spec(len = 0).
> case4: SMBus block read 0x60 register. SMBus len = 64 bytes. It break
> the SMBus spec(len >= 32bytes).
>
> LPI2C3 as master controller to smbus block tead the slave device at 0x64.
> Not apply this fix:
> root@...93evk:~# i2cget -f -y 3 0x64 0x10 s
> Error: Read failed
> root@...93evk:~# i2cget -f -y 3 0x64 0x40 s
> Error: Read failed
> root@...93evk:~# i2cget -f -y 3 0x64 0x50 s
> Error: Read failed
> root@...93evk:~# i2cget -f -y 3 0x64 0x60 s
> 0x00
>
> After apply this fix:
> root@...93evk:~# i2cget -f -y 3 0x64 0x10 s
where show your read 32 byte, do you means "i2cget -f -y 3 0x64 0x10 s 0x20"?
> 0x00 0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08 0x09 0x0a 0x0b 0x0c 0x0d 0x0e 0x0f
> 0x10 0x11 0x12 0x13 0x14 0x15 0x16 0x17 0x18 0x19 0x1a 0x1b 0x1c 0x1d 0x1e 0x1f
according to above dump, the data from 0x11, not 0x10.
Frank
> root@...93evk:~# i2cget -f -y 3 0x64 0x40 s
> 0x00 0x01 0x02 0x03 0x04 0x05 0x06 0x07
> root@...93evk:~# i2cget -f -y 3 0x64 0x50 s
> [ 184.098094] i2c i2c-3: Invalid SMBus block read length
> Error: Read failed
> root@...93evk:~# i2cget -f -y 3 0x64 0x60 s
> [ 179.722105] i2c i2c-3: Invalid SMBus block read length
> Error: Read failed
>
> ---
> drivers/i2c/busses/i2c-imx-lpi2c.c | 93 +++++++++++++++++++++++-------
> 1 file changed, 73 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index d882126c1778..39088567db59 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -90,6 +90,7 @@
> #define MRDR_RXEMPTY BIT(14)
> #define MDER_TDDE BIT(0)
> #define MDER_RDDE BIT(1)
> +#define MSR_RDF_ASSERT(x) FIELD_GET(MSR_RDF, (x))
>
> #define SCR_SEN BIT(0)
> #define SCR_RST BIT(1)
> @@ -461,7 +462,7 @@ static bool lpi2c_imx_write_txfifo(struct lpi2c_imx_struct *lpi2c_imx, bool atom
>
> static bool lpi2c_imx_read_rxfifo(struct lpi2c_imx_struct *lpi2c_imx, bool atomic)
> {
> - unsigned int blocklen, remaining;
> + unsigned int remaining;
> unsigned int temp, data;
>
> do {
> @@ -472,15 +473,6 @@ static bool lpi2c_imx_read_rxfifo(struct lpi2c_imx_struct *lpi2c_imx, bool atomi
> lpi2c_imx->rx_buf[lpi2c_imx->delivered++] = data & 0xff;
> } while (1);
>
> - /*
> - * First byte is the length of remaining packet in the SMBus block
> - * data read. Add it to msgs->len.
> - */
> - if (lpi2c_imx->block_data) {
> - blocklen = lpi2c_imx->rx_buf[0];
> - lpi2c_imx->msglen += blocklen;
> - }
> -
> remaining = lpi2c_imx->msglen - lpi2c_imx->delivered;
>
> if (!remaining) {
> @@ -493,12 +485,7 @@ static bool lpi2c_imx_read_rxfifo(struct lpi2c_imx_struct *lpi2c_imx, bool atomi
> lpi2c_imx_set_rx_watermark(lpi2c_imx);
>
> /* multiple receive commands */
> - if (lpi2c_imx->block_data) {
> - lpi2c_imx->block_data = 0;
> - temp = remaining;
> - temp |= (RECV_DATA << 8);
> - writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> - } else if (!(lpi2c_imx->delivered & 0xff)) {
> + if (!(lpi2c_imx->delivered & 0xff)) {
> temp = (remaining > CHUNK_DATA ? CHUNK_DATA : remaining) - 1;
> temp |= (RECV_DATA << 8);
> writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> @@ -536,18 +523,80 @@ static int lpi2c_imx_write_atomic(struct lpi2c_imx_struct *lpi2c_imx,
> return err;
> }
>
> +static unsigned int lpi2c_SMBus_block_read_single_byte(struct lpi2c_imx_struct *lpi2c_imx)
> +{
> + unsigned int val, data;
> + int ret;
> +
> + ret = readl_poll_timeout(lpi2c_imx->base + LPI2C_MSR, val,
> + MSR_RDF_ASSERT(val), 1, 1000);
> + if (ret) {
> + dev_err(&lpi2c_imx->adapter.dev, "SMBus read count timeout %d\n", ret);
> + return ret;
> + }
> +
> + data = readl(lpi2c_imx->base + LPI2C_MRDR);
> + lpi2c_imx->rx_buf[lpi2c_imx->delivered++] = data & 0xff;
> +
> + return data;
> +}
> +
> static void lpi2c_imx_read_init(struct lpi2c_imx_struct *lpi2c_imx,
> struct i2c_msg *msgs)
> {
> - unsigned int temp;
> + unsigned int temp, ret, block_len;
>
> lpi2c_imx->rx_buf = msgs->buf;
> lpi2c_imx->block_data = msgs->flags & I2C_M_RECV_LEN;
>
> lpi2c_imx_set_rx_watermark(lpi2c_imx);
> - temp = msgs->len > CHUNK_DATA ? CHUNK_DATA - 1 : msgs->len - 1;
> - temp |= (RECV_DATA << 8);
> - writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> +
> + if (!lpi2c_imx->block_data) {
> + temp = msgs->len > CHUNK_DATA ? CHUNK_DATA - 1 : msgs->len - 1;
> + temp |= (RECV_DATA << 8);
> + writel(temp, lpi2c_imx->base + LPI2C_MTDR);
> + } else {
> + /*
> + * The LPI2C controller automatically transmits a NACK on the last byte
> + * of a receive data command. It transmits the NACK unless the next
> + * command in the TXFIFO is also a receive data command. If the transmit
> + * FIFO is empty when a receive data command completes, a NACK is also
> + * automatically transmitted.
> + * So specially set read 2 bytes read command initially. First byte in
> + * RXFIFO is SMBus block data length, when the data enter the RXFIFO,
> + * controller should immediately read out and update MTDR register to keep
> + * the TXFIFO not empty. Second byte is the first byte of block data.
> + * So the data length of the subsequent block data read command should be
> + * block_len - 1, because in the first read command, the first byte of block
> + * data has already been stored in RXFIFO.
> + */
> + writel((RECV_DATA << 8) | 0x01, lpi2c_imx->base + LPI2C_MTDR);
> +
> + /* Read the first byte as block len */
> + block_len = lpi2c_SMBus_block_read_single_byte(lpi2c_imx);
> + if (block_len < 0) {
> + dev_err(&lpi2c_imx->adapter.dev, "SMBus read data length timeout\n");
> + return;
> + }
> +
> + /* Confirm SMBus transfer meets protocol */
> + if (block_len == 0 || block_len > I2C_SMBUS_BLOCK_MAX) {
> + dev_err(&lpi2c_imx->adapter.dev, "Invalid SMBus block read length\n");
> + return;
> + }
> +
> + /* If just read 1 byte then read out from fifo. No need new command update */
> + if (block_len == 1) {
> + ret = lpi2c_SMBus_block_read_single_byte(lpi2c_imx);
> + if (ret < 0)
> + dev_err(&lpi2c_imx->adapter.dev, "SMBus read data timeout\n");
> + return;
> + }
> +
> + /* Block read other length data need to update command again*/
> + writel((RECV_DATA << 8) | (block_len - 2), lpi2c_imx->base + LPI2C_MTDR);
> + lpi2c_imx->msglen += block_len;
> + }
> }
>
> static bool lpi2c_imx_read_chunk_atomic(struct lpi2c_imx_struct *lpi2c_imx)
> @@ -599,6 +648,10 @@ static bool is_use_dma(struct lpi2c_imx_struct *lpi2c_imx, struct i2c_msg *msg)
> if (pm_suspend_in_progress())
> return false;
>
> + /* DMA is not suitable for SMBus block read */
> + if (msg->flags & I2C_M_RECV_LEN)
> + return false;
> +
> /*
> * When the length of data is less than I2C_DMA_THRESHOLD,
> * cpu mode is used directly to avoid low performance.
> --
> 2.34.1
>
Powered by blists - more mailing lists