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.

For reasons of transparency: The work on I2C_SMBUS_I2C_BLOCK_DATA support
for the RTL8239-MCU was done on RTL931xx. All problem were therefore
detected with the patches from Jonas Jelonek [1] and not the vanilla Linux
driver. But looking through the code, it seems like these are NOT
regressions introduced by the RTL931x patchset.

[1] https://patchwork.ozlabs.org/project/linux-i2c/cover/20250727114800.3046-1-jelonek.jonas@gmail.com/

Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Changes in v2:
- add the missing transfer width and read length increase for the SMBus
  Write/Read
- Link to v1: https://lore.kernel.org/r/20250802-i2c-rtl9300-multi-byte-v1-0-5f687e0098e2@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 Ops

 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>

Sorry for the noise. I did test v2 but replied to v1.

On 04/08/2025 04:54, 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.
>
> For reasons of transparency: The work on I2C_SMBUS_I2C_BLOCK_DATA support
> for the RTL8239-MCU was done on RTL931xx. All problem were therefore
> detected with the patches from Jonas Jelonek [1] and not the vanilla Linux
> driver. But looking through the code, it seems like these are NOT
> regressions introduced by the RTL931x patchset.
>
> [1] https://scanmail.trustwave.com/?c=20988&d=npSP6NDPXTaQTClSdFmD6RMng5fg4WMz7KSCx5tgNw&u=https%3a%2f%2fpatchwork%2eozlabs%2eorg%2fproject%2flinux-i2c%2fcover%2f20250727114800%2e3046-1-jelonek%2ejonas%40gmail%2ecom%2f
>
> 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.

> ---
> Changes in v2:
> - add the missing transfer width and read length increase for the SMBus
>    Write/Read
> - Link to v1: https://scanmail.trustwave.com/?c=20988&d=npSP6NDPXTaQTClSdFmD6RMng5fg4WMz7KHQxMVsYg&u=https%3a%2f%2flore%2ekernel%2eorg%2fr%2f20250802-i2c-rtl9300-multi-byte-v1-0-5f687e0098e2%40narfation%2eorg
>
> ---
> 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 Ops
>
>   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,

On Monday, 4 August 2025 00:39:40 CEST Chris Packham wrote:
> For the series
> 
> Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Thank you.

> 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.

I can understand this problem quite well. We can all only try our best and 
then hope that someone with the actual HW can figure out the specific parts 
which we didn't had access to.


> 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.


No, I prepare something for downstream testing (with Jonas' patch): 
https://github.com/openwrt/openwrt/pull/19577#discussion_r2248520949

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

Thanks, I was not aware of this specific one. I don't exactly know the repo 
structure for I2C Host drivers. But 
git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git
i2c/i2c-host-fixes or i2c/i2c-host-next didn't had this patch. I've also 
checked linux-next and couldn't find the patch at the moment. 

I am guessing it is the best when I resent this patch as part of my patchset 
and modify my patches accordingly. The resent will then be done this evening 
(GMT+2). Preview can be found at
https://git.open-mesh.org/linux-merge.git/log/?h=b4/i2c-rtl9300-multi-byte

I've also checked linux-next and couldn't find the patch at the moment.

Kind regards,
	Sven

On 04/08/2025 20:35, Sven Eckelmann wrote:
> On Monday, 4 August 2025 00:39:40 CEST Chris Packham wrote:
>> For the series
>>
>> Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> Thank you.
>
>> 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.
> I can understand this problem quite well. We can all only try our best and
> then hope that someone with the actual HW can figure out the specific parts
> which we didn't had access to.
>
>
>> 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.
>
> No, I prepare something for downstream testing (with Jonas' patch):
> https://scanmail.trustwave.com/?c=20988&d=4PCQ6BHJEPBu-7BQIQUZquRTjfxQxIRM1wAL-n5mvg&u=https%3a%2f%2fgithub%2ecom%2fopenwrt%2fopenwrt%2fpull%2f19577%23discussion%5fr2248520949
>
>> Conflict appears to be with
>> https://scanmail.trustwave.com/?c=20988&d=4PCQ6BHJEPBu-7BQIQUZquRTjfxQxIRM11AI-nUytw&u=https%3a%2f%2flore%2ekernel%2eorg%2fall%2f20250615235248%2e529019-1-alexguo1023%40gmail%2ecom%2f
> Thanks, I was not aware of this specific one. I don't exactly know the repo
> structure for I2C Host drivers. But
> git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git
> i2c/i2c-host-fixes or i2c/i2c-host-next didn't had this patch. I've also
> checked linux-next and couldn't find the patch at the moment.
>
> I am guessing it is the best when I resent this patch as part of my patchset
> and modify my patches accordingly. The resent will then be done this evening
> (GMT+2). Preview can be found at
> https://scanmail.trustwave.com/?c=20988&d=4PCQ6BHJEPBu-7BQIQUZquRTjfxQxIRM11ZYrXdjvg&u=https%3a%2f%2fgit%2eopen-mesh%2eorg%2flinux-merge%2egit%2flog%2f%3fh%3db4%2fi2c-rtl9300-multi-byte
>
> I've also checked linux-next and couldn't find the patch at the moment.

Oops my bad. I'd applied Alex's patch to test locally then added more 
stuff on top of it and promptly forgot. When I came to test your patches 
I just rebased on top of net-next which I incorrectly assumed had picked 
up Alex's change via linux-next hence reporting the conflict which you 
can't see. Glad you managed to figure it out on your end for v3.

>
> Kind regards,
> 	Sven