[PATCH 0/4] i2c: rtl9300: Fix multi-byte I2C operations

[PATCH 0/4] i2c: rtl9300: Fix multi-byte I2C operations

[Sven Eckelmann]

During the integration of the RTL8239 POE chip + its frontend MCU, it was
noticed that multi-byte operations were basically broken in the current
driver.

Tests using SMBus Block Writes showed that the data (after the Wr + Ack
marker) was mixed up on the wire. At first glance, it looked like an
endianness problem. But for transfers were the number of count + data bytes
was not divisible by 4, the last bytes were not looking like an endianness
problem because they were were in the wrong order but not for example 0 -
which would be the case for an endianness problem with 32 bit registers. At
the end, it turned out to be a the way how i2c_write tried to add the bytes
to the send registers.

Each 32 bit register was used similar to a shift register - shifting the
various bytes up the register while the next one is added to the least
significant byte. But the I2C controller expects the first byte of the
tranmission in the least significant byte of the first register. And the
last byte (assuming it is a 16 byte transfer) in the most significant byte
of the fourth register.

While doing these tests, it was also observed that the count byte was
missing from the SMBus Block Writes. The driver just removed them from the
data->block (from the I2C subsystem). But the I2C controller DOES NOT
automatically add this byte - for example by using the configured
transmission length.

The RTL8239 MCU is not actually an SMBus compliant device. Instead, it
expects I2C Block Reads + I2C Block Writes. But according to the already
identified bugs in the driver, it was clear that the I2C controller can
simply be modified to not send the count byte for I2C_SMBUS_I2C_BLOCK_DATA.
The receive part, just needs to write the content of the receive buffer to
the correct position in data->block.

While the on-wire formwat was now correct, reads were still not possible
against the MCU (for the RTL8239 POE chip). It was always timing out
because the 2ms were not enough for sending the read request and then
receiving the 12 byte answer.

These changes were originally submitted to OpenWrt. But there are plans to
migrate OpenWrt to the upstream Linux driver. As result, the pull request
was stopped and the changes were redone against this driver.

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Harshal Gohel (2):
      i2c: rtl9300: Fix multi-byte I2C write
      i2c: rtl9300: Implement I2C block read and write

Sven Eckelmann (2):
      i2c: rtl9300: Increase timeout for transfer polling
      i2c: rtl9300: Add missing count byte for SMBus Block Write

 drivers/i2c/busses/i2c-rtl9300.c | 43 +++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)
---
base-commit: b9ddaa95fd283bce7041550ddbbe7e764c477110
change-id: 20250802-i2c-rtl9300-multi-byte-edaa1fb0872c

Best regards,
-- 
Sven Eckelmann <sven@narfation.org>

[PATCH 1/4] i2c: rtl9300: Fix multi-byte I2C write

[Sven Eckelmann]

From: Harshal Gohel <hg@simonwunderlich.de>

The RTL93xx I2C controller has 4 32 bit registers to store the bytes for
the upcoming I2C transmission. The first byte is stored in the
least-significant byte of the first register. And the last byte in the most
significant byte of the last register. A map of the transferred bytes to
their order in the registers is:

reg 0: 04-03-02-01
reg 1: 08-07-06-05
reg 2: 0c-0b-0a-09
reg 3: 10-0f-0e-0d

The i2c_read() function basically demonstrates how the hardware would pick
up bytes from this register set. But the i2c_write() function was just
pushing bytes one after another to the least significant byte of a register
AFTER shifting the last one to the next more significant byte position.

If you would then have tried to send a buffer with numbers 1-11 using
i2c_write(), you would have ended up with following register content:

reg 0: 01-02-03-04
reg 1: 05-06-07-08
reg 2: 00-09-0a-0b
reg 3: 00-00-00-00

On the wire, you would then have seen:

  Sr Addr Rd/Wr [A] 04 A 03 A 02 A 01 A 08 A 07 A 06 A 05 A 0b A 0a A 09 A/NA P

But the correct data transmission was expected to be

  Sr Addr Rd/Wr [A] 01 A 02 A 03 A 04 A 05 A 06 A 07 A 08 A 09 A 0a A 0b A/NA P

Because of this multi-byte ordering problem, only single byte i2c_write()
operations were executed correctly (on the wire).

By shifting the byte directly to the correct end position in the register,
it is possible to avoid this incorrect byte ordering and fix multi-byte
transmissions.

Cc: <stable@vger.kernel.org>
Fixes: c366be720235 ("i2c: Add driver for the RTL9300 I2C controller")
Signed-off-by: Harshal Gohel <hg@simonwunderlich.de>
Co-developed-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 drivers/i2c/busses/i2c-rtl9300.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
index e064e8a4a1f0824abc82fa677866b85f99fbe4a7..1b3cbe3ea84a4fa480c5c00438eecc551d047348 100644
--- a/drivers/i2c/busses/i2c-rtl9300.c
+++ b/drivers/i2c/busses/i2c-rtl9300.c
@@ -143,10 +143,13 @@ static int rtl9300_i2c_write(struct rtl9300_i2c *i2c, u8 *buf, int len)
 		return -EIO;
 
 	for (i = 0; i < len; i++) {
+		unsigned int shift = (i % 4) * 8;
+		unsigned int reg = i / 4;
+
 		if (i % 4 == 0)
-			vals[i/4] = 0;
-		vals[i/4] <<= 8;
-		vals[i/4] |= buf[i];
+			vals[reg] = 0;
+
+		vals[reg] |= buf[i] << shift;
 	}
 
 	return regmap_bulk_write(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_DATA_WORD0,

-- 
2.47.2

[PATCH 2/4] i2c: rtl9300: Increase timeout for transfer polling

[Sven Eckelmann]

The timeout for transfers was only set to 2ms. Because of this relatively
low limit, 12-byte read operations to the frontend MCU of a RTL8239 POE PSE
chip cluster was consistently resulting in a timeout.

The original OpenWrt downstream driver [1] was not using any timeout limit
at all. This is also possible by setting the timeout_us parameter of
regmap_read_poll_timeout() to 0. But since the driver currently is
implements the ETIMEDOUT error, it is more sensible to increase the timeout
in such a way that the communication with the (quite common) Realtek
I2C-connected POE management solution is possible.

[1] https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/realtek/files-6.12/drivers/i2c/busses/i2c-rtl9300.c;h=c4d973195ef39dc56d6207e665d279745525fcac#l202

Cc: <stable@vger.kernel.org>
Fixes: c366be720235 ("i2c: Add driver for the RTL9300 I2C controller")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 drivers/i2c/busses/i2c-rtl9300.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
index 1b3cbe3ea84a4fa480c5c00438eecc551d047348..a10e5e6e00075fabb8906d56f09f5b9141fbc06e 100644
--- a/drivers/i2c/busses/i2c-rtl9300.c
+++ b/drivers/i2c/busses/i2c-rtl9300.c
@@ -178,7 +178,7 @@ static int rtl9300_i2c_execute_xfer(struct rtl9300_i2c *i2c, char read_write,
 		return ret;
 
 	ret = regmap_read_poll_timeout(i2c->regmap, i2c->reg_base + RTL9300_I2C_MST_CTRL1,
-				       val, !(val & RTL9300_I2C_MST_CTRL1_I2C_TRIG), 100, 2000);
+				       val, !(val & RTL9300_I2C_MST_CTRL1_I2C_TRIG), 100, 100000);
 	if (ret)
 		return ret;
 

-- 
2.47.2

[PATCH 3/4] i2c: rtl9300: Add missing count byte for SMBus Block Write

[Sven Eckelmann]

The expected on-wire format of an SMBus Block Write is

  S Addr Wr [A] Comm [A] Count [A] Data [A] Data [A] ... [A] Data [A] P

Everything starting from the Count byte is provided by the I2C subsystem in
the array data->block. But the driver was skipping the Count byte
(data->block[0]) when sending it to the RTL93xx I2C controller.

Only the actual data could be seen on the wire:

  S Addr Wr [A] Comm [A] Data [A] Data [A] ... [A] Data [A] P

This wire format is not SMBus Block Write compatible but matches the format
of an I2C Block Write. Simply adding the count byte to the buffer for the
I2C controller enough to fix the transmission.

Cc: <stable@vger.kernel.org>
Fixes: c366be720235 ("i2c: Add driver for the RTL9300 I2C controller")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 drivers/i2c/busses/i2c-rtl9300.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
index a10e5e6e00075fabb8906d56f09f5b9141fbc06e..ed82e6c21aaf46000a263740123ffba833578cc2 100644
--- a/drivers/i2c/busses/i2c-rtl9300.c
+++ b/drivers/i2c/busses/i2c-rtl9300.c
@@ -288,7 +288,7 @@ static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned s
 		if (ret)
 			goto out_unlock;
 		if (read_write == I2C_SMBUS_WRITE) {
-			ret = rtl9300_i2c_write(i2c, &data->block[1], data->block[0]);
+			ret = rtl9300_i2c_write(i2c, &data->block[0], data->block[0] + 1);
 			if (ret)
 				goto out_unlock;
 		}

-- 
2.47.2

[PATCH 4/4] i2c: rtl9300: Implement I2C block read and write

[Sven Eckelmann]

From: Harshal Gohel <hg@simonwunderlich.de>

It was noticed that the original implementation of SMBus Block Write in the
driver was actually an I2C Block Write. Both differ only in the Count byte
before the actual data:

  S Addr Wr [A] Comm [A] Count [A] Data [A] Data [A] ... [A] Data [A] P

The I2C Block Write is just skipping this Count byte and starts directly
with the data:

  S Addr Wr [A] Comm [A] Data [A] Data [A] ... [A] Data [A] P

The I2C controller of RTL93xx doesn't handle this Count byte special and it
is simply another one of (16 possible) data bytes. Adding support for the
I2C Block Write therefore only requires to skip the count byte (0) in
data->block.

It is similar for reads. The SMBUS Block read is having a Count byte before
the data:

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

And the I2C Block Read is directly starting with the actual data:

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

The I2C controller is also not handling this byte in a special way. It
simply provides every byte after the Rd marker + Ack as part of the 16 byte
receive buffer (registers). The content of this buffer just has to be
copied to the right position in the receive data->block.

Signed-off-by: Harshal Gohel <hg@simonwunderlich.de>
Co-developed-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 drivers/i2c/busses/i2c-rtl9300.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
index ed82e6c21aaf46000a263740123ffba833578cc2..e136f774fd1a6be03339a3c1b70c1a93a92ee55c 100644
--- a/drivers/i2c/busses/i2c-rtl9300.c
+++ b/drivers/i2c/busses/i2c-rtl9300.c
@@ -186,22 +186,32 @@ static int rtl9300_i2c_execute_xfer(struct rtl9300_i2c *i2c, char read_write,
 		return -EIO;
 
 	if (read_write == I2C_SMBUS_READ) {
-		if (size == I2C_SMBUS_BYTE || size == I2C_SMBUS_BYTE_DATA) {
+		switch (size) {
+		case I2C_SMBUS_BYTE:
+		case I2C_SMBUS_BYTE_DATA:
 			ret = regmap_read(i2c->regmap,
 					  i2c->reg_base + RTL9300_I2C_MST_DATA_WORD0, &val);
 			if (ret)
 				return ret;
 			data->byte = val & 0xff;
-		} else if (size == I2C_SMBUS_WORD_DATA) {
+			break;
+		case I2C_SMBUS_WORD_DATA:
 			ret = regmap_read(i2c->regmap,
 					  i2c->reg_base + RTL9300_I2C_MST_DATA_WORD0, &val);
 			if (ret)
 				return ret;
 			data->word = val & 0xffff;
-		} else {
+			break;
+		case I2C_SMBUS_I2C_BLOCK_DATA:
+			ret = rtl9300_i2c_read(i2c, &data->block[1], len);
+			if (ret)
+				return ret;
+			break;
+		default:
 			ret = rtl9300_i2c_read(i2c, &data->block[0], len);
 			if (ret)
 				return ret;
+			break;
 		}
 	}
 
@@ -295,6 +305,21 @@ static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned s
 		len = data->block[0];
 		break;
 
+	case I2C_SMBUS_I2C_BLOCK_DATA:
+		ret = rtl9300_i2c_reg_addr_set(i2c, command, 1);
+		if (ret)
+			goto out_unlock;
+		ret = rtl9300_i2c_config_xfer(i2c, chan, addr, data->block[0]);
+		if (ret)
+			goto out_unlock;
+		if (read_write == I2C_SMBUS_WRITE) {
+			ret = rtl9300_i2c_write(i2c, &data->block[1], data->block[0]);
+			if (ret)
+				goto out_unlock;
+		}
+		len = data->block[0];
+		break;
+
 	default:
 		dev_err(&adap->dev, "Unsupported transaction %d\n", size);
 		ret = -EOPNOTSUPP;
@@ -313,6 +338,7 @@ static u32 rtl9300_i2c_func(struct i2c_adapter *a)
 {
 	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
 	       I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
+	       I2C_FUNC_SMBUS_READ_I2C_BLOCK | I2C_FUNC_SMBUS_WRITE_I2C_BLOCK |
 	       I2C_FUNC_SMBUS_BLOCK_DATA;
 }
 

-- 
2.47.2

Re: [PATCH 3/4] i2c: rtl9300: Add missing count byte for SMBus Block Write

[Sven Eckelmann]

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

On Saturday, 2 August 2025 12:32:02 CEST Sven Eckelmann wrote:
[...]
> diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
> index a10e5e6e00075fabb8906d56f09f5b9141fbc06e..ed82e6c21aaf46000a263740123ffba833578cc2 100644
> --- a/drivers/i2c/busses/i2c-rtl9300.c
> +++ b/drivers/i2c/busses/i2c-rtl9300.c
> @@ -288,7 +288,7 @@ static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned s
>  		if (ret)
>  			goto out_unlock;
>  		if (read_write == I2C_SMBUS_WRITE) {
> -			ret = rtl9300_i2c_write(i2c, &data->block[1], data->block[0]);
> +			ret = rtl9300_i2c_write(i2c, &data->block[0], data->block[0] + 1);
>  			if (ret)
>  				goto out_unlock;
>  		}
> 
> 

Yes, there is as always this "2 seconds" after the send "wtf. were did the 
line go" situation:

diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
index a10e5e6e00075fabb8906d56f09f5b9141fbc06e..4d109ebd02f0cad86f50e4e148966b3fa68b0196 100644
--- a/drivers/i2c/busses/i2c-rtl9300.c
+++ b/drivers/i2c/busses/i2c-rtl9300.c
@@ -284,11 +284,11 @@ static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned s
 		ret = rtl9300_i2c_reg_addr_set(i2c, command, 1);
 		if (ret)
 			goto out_unlock;
-		ret = rtl9300_i2c_config_xfer(i2c, chan, addr, data->block[0]);
+		ret = rtl9300_i2c_config_xfer(i2c, chan, addr, data->block[0] + 1);
 		if (ret)
 			goto out_unlock;
 		if (read_write == I2C_SMBUS_WRITE) {
-			ret = rtl9300_i2c_write(i2c, &data->block[1], data->block[0]);
+			ret = rtl9300_i2c_write(i2c, &data->block[0], data->block[0] + 1);
 			if (ret)
 				goto out_unlock;
 		}

v2 will be send tomorrow. Sorry about the noise.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 0/4] i2c: rtl9300: Fix multi-byte I2C operations

[Chris Packham]

Hi Sven,

On 02/08/2025 22:31, Sven Eckelmann wrote:
> During the integration of the RTL8239 POE chip + its frontend MCU, it was
> noticed that multi-byte operations were basically broken in the current
> driver.
>
> Tests using SMBus Block Writes showed that the data (after the Wr + Ack
> marker) was mixed up on the wire. At first glance, it looked like an
> endianness problem. But for transfers were the number of count + data bytes
> was not divisible by 4, the last bytes were not looking like an endianness
> problem because they were were in the wrong order but not for example 0 -
> which would be the case for an endianness problem with 32 bit registers. At
> the end, it turned out to be a the way how i2c_write tried to add the bytes
> to the send registers.
>
> Each 32 bit register was used similar to a shift register - shifting the
> various bytes up the register while the next one is added to the least
> significant byte. But the I2C controller expects the first byte of the
> tranmission in the least significant byte of the first register. And the
> last byte (assuming it is a 16 byte transfer) in the most significant byte
> of the fourth register.
>
> While doing these tests, it was also observed that the count byte was
> missing from the SMBus Block Writes. The driver just removed them from the
> data->block (from the I2C subsystem). But the I2C controller DOES NOT
> automatically add this byte - for example by using the configured
> transmission length.
>
> The RTL8239 MCU is not actually an SMBus compliant device. Instead, it
> expects I2C Block Reads + I2C Block Writes. But according to the already
> identified bugs in the driver, it was clear that the I2C controller can
> simply be modified to not send the count byte for I2C_SMBUS_I2C_BLOCK_DATA.
> The receive part, just needs to write the content of the receive buffer to
> the correct position in data->block.
>
> While the on-wire formwat was now correct, reads were still not possible
> against the MCU (for the RTL8239 POE chip). It was always timing out
> because the 2ms were not enough for sending the read request and then
> receiving the 12 byte answer.
>
> These changes were originally submitted to OpenWrt. But there are plans to
> migrate OpenWrt to the upstream Linux driver. As result, the pull request
> was stopped and the changes were redone against this driver.

Thanks. I was only really able to test with basic eeprom like devices so 
it's not entirely surprising that the SMBUS stuff was wrong.

Is you series intended to apply on top of Jonas's? I'm trying to apply 
yours alone (for various reasons happens to be on top of net-next/main) 
and I'm getting conflicts.

At a cursory glance your changes all look sensible I'll reply with a 
proper r-by when I've been able to tests them.

>
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
> Harshal Gohel (2):
>        i2c: rtl9300: Fix multi-byte I2C write
>        i2c: rtl9300: Implement I2C block read and write
>
> Sven Eckelmann (2):
>        i2c: rtl9300: Increase timeout for transfer polling
>        i2c: rtl9300: Add missing count byte for SMBus Block Write
>
>   drivers/i2c/busses/i2c-rtl9300.c | 43 +++++++++++++++++++++++++++++++++-------
>   1 file changed, 36 insertions(+), 7 deletions(-)
> ---
> base-commit: b9ddaa95fd283bce7041550ddbbe7e764c477110
> change-id: 20250802-i2c-rtl9300-multi-byte-edaa1fb0872c
>
> Best regards,

Re: [PATCH 0/4] i2c: rtl9300: Fix multi-byte I2C operations

[Chris Packham]

On 04/08/2025 10:12, Chris Packham wrote:
> Hi Sven,
>
> On 02/08/2025 22:31, Sven Eckelmann wrote:
>> During the integration of the RTL8239 POE chip + its frontend MCU, it 
>> was
>> noticed that multi-byte operations were basically broken in the current
>> driver.
>>
>> Tests using SMBus Block Writes showed that the data (after the Wr + Ack
>> marker) was mixed up on the wire. At first glance, it looked like an
>> endianness problem. But for transfers were the number of count + data 
>> bytes
>> was not divisible by 4, the last bytes were not looking like an 
>> endianness
>> problem because they were were in the wrong order but not for example 
>> 0 -
>> which would be the case for an endianness problem with 32 bit 
>> registers. At
>> the end, it turned out to be a the way how i2c_write tried to add the 
>> bytes
>> to the send registers.
>>
>> Each 32 bit register was used similar to a shift register - shifting the
>> various bytes up the register while the next one is added to the least
>> significant byte. But the I2C controller expects the first byte of the
>> tranmission in the least significant byte of the first register. And the
>> last byte (assuming it is a 16 byte transfer) in the most significant 
>> byte
>> of the fourth register.
>>
>> While doing these tests, it was also observed that the count byte was
>> missing from the SMBus Block Writes. The driver just removed them 
>> from the
>> data->block (from the I2C subsystem). But the I2C controller DOES NOT
>> automatically add this byte - for example by using the configured
>> transmission length.
>>
>> The RTL8239 MCU is not actually an SMBus compliant device. Instead, it
>> expects I2C Block Reads + I2C Block Writes. But according to the already
>> identified bugs in the driver, it was clear that the I2C controller can
>> simply be modified to not send the count byte for 
>> I2C_SMBUS_I2C_BLOCK_DATA.
>> The receive part, just needs to write the content of the receive 
>> buffer to
>> the correct position in data->block.
>>
>> While the on-wire formwat was now correct, reads were still not possible
>> against the MCU (for the RTL8239 POE chip). It was always timing out
>> because the 2ms were not enough for sending the read request and then
>> receiving the 12 byte answer.
>>
>> These changes were originally submitted to OpenWrt. But there are 
>> plans to
>> migrate OpenWrt to the upstream Linux driver. As result, the pull 
>> request
>> was stopped and the changes were redone against this driver.
>
> Thanks. I was only really able to test with basic eeprom like devices 
> so it's not entirely surprising that the SMBUS stuff was wrong.
>
> Is you series intended to apply on top of Jonas's? I'm trying to apply 
> yours alone (for various reasons happens to be on top of 
> net-next/main) and I'm getting conflicts.

Conflict appears to be with 
https://lore.kernel.org/all/20250615235248.529019-1-alexguo1023@gmail.com/

>
> At a cursory glance your changes all look sensible I'll reply with a 
> proper r-by when I've been able to tests them.
>
>>
>> Signed-off-by: Sven Eckelmann <sven@narfation.org>
>> ---
>> Harshal Gohel (2):
>>        i2c: rtl9300: Fix multi-byte I2C write
>>        i2c: rtl9300: Implement I2C block read and write
>>
>> Sven Eckelmann (2):
>>        i2c: rtl9300: Increase timeout for transfer polling
>>        i2c: rtl9300: Add missing count byte for SMBus Block Write
>>
>>   drivers/i2c/busses/i2c-rtl9300.c | 43 
>> +++++++++++++++++++++++++++++++++-------
>>   1 file changed, 36 insertions(+), 7 deletions(-)
>> ---
>> base-commit: b9ddaa95fd283bce7041550ddbbe7e764c477110
>> change-id: 20250802-i2c-rtl9300-multi-byte-edaa1fb0872c
>>
>> Best regards,

Re: [PATCH 0/4] i2c: rtl9300: Fix multi-byte I2C operations

[Chris Packham]

On 02/08/2025 22:31, Sven Eckelmann wrote:
> During the integration of the RTL8239 POE chip + its frontend MCU, it was
> noticed that multi-byte operations were basically broken in the current
> driver.
>
> Tests using SMBus Block Writes showed that the data (after the Wr + Ack
> marker) was mixed up on the wire. At first glance, it looked like an
> endianness problem. But for transfers were the number of count + data bytes
> was not divisible by 4, the last bytes were not looking like an endianness
> problem because they were were in the wrong order but not for example 0 -
> which would be the case for an endianness problem with 32 bit registers. At
> the end, it turned out to be a the way how i2c_write tried to add the bytes
> to the send registers.
>
> Each 32 bit register was used similar to a shift register - shifting the
> various bytes up the register while the next one is added to the least
> significant byte. But the I2C controller expects the first byte of the
> tranmission in the least significant byte of the first register. And the
> last byte (assuming it is a 16 byte transfer) in the most significant byte
> of the fourth register.
>
> While doing these tests, it was also observed that the count byte was
> missing from the SMBus Block Writes. The driver just removed them from the
> data->block (from the I2C subsystem). But the I2C controller DOES NOT
> automatically add this byte - for example by using the configured
> transmission length.
>
> The RTL8239 MCU is not actually an SMBus compliant device. Instead, it
> expects I2C Block Reads + I2C Block Writes. But according to the already
> identified bugs in the driver, it was clear that the I2C controller can
> simply be modified to not send the count byte for I2C_SMBUS_I2C_BLOCK_DATA.
> The receive part, just needs to write the content of the receive buffer to
> the correct position in data->block.
>
> While the on-wire formwat was now correct, reads were still not possible
> against the MCU (for the RTL8239 POE chip). It was always timing out
> because the 2ms were not enough for sending the read request and then
> receiving the 12 byte answer.
>
> These changes were originally submitted to OpenWrt. But there are plans to
> migrate OpenWrt to the upstream Linux driver. As result, the pull request
> was stopped and the changes were redone against this driver.
>
> Signed-off-by: Sven Eckelmann <sven@narfation.org>

For the series

Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Note that I've only got the same simple eeprom devices that I did the 
initial development on so I don't think I've really exercised the block 
data paths but I can say the changes don't appear to have regressed 
anything.

> ---
> Harshal Gohel (2):
>        i2c: rtl9300: Fix multi-byte I2C write
>        i2c: rtl9300: Implement I2C block read and write
>
> Sven Eckelmann (2):
>        i2c: rtl9300: Increase timeout for transfer polling
>        i2c: rtl9300: Add missing count byte for SMBus Block Write
>
>   drivers/i2c/busses/i2c-rtl9300.c | 43 +++++++++++++++++++++++++++++++++-------
>   1 file changed, 36 insertions(+), 7 deletions(-)
> ---
> base-commit: b9ddaa95fd283bce7041550ddbbe7e764c477110
> change-id: 20250802-i2c-rtl9300-multi-byte-edaa1fb0872c
>
> Best regards,