Re: [RFC PATCH v2 0/6] hw/i2c: i2c slave mode support

[Jae Hyun Yoo <quic_jaehyoo@quicinc.com>]

Hi Klaus,

On 6/2/2022 6:50 AM, Cédric Le Goater wrote:
> On 6/2/22 10:21, Klaus Jensen wrote:
>> On Jun  2 09:52, Cédric Le Goater wrote:
>>> On 6/1/22 23:08, Klaus Jensen wrote:
>>>> From: Klaus Jensen <k.jensen@samsung.com>
>>>>
>>>> Hi all,
>>>>
>>>> This RFC series adds I2C "slave mode" support for the Aspeed I2C
>>>
>>> I think you can remove the RFC prefix.
>>>
>>>> controller as well as the necessary infrastructure in the i2c core to
>>>> support this.
>>>>
>>>> v2 changes
>>>> ~~~~~~~~~~
>>>> I finally got around to working on this again. I'm sorry for not
>>>> bringing a v2 to the table earlier.
>>>>
>>>> Mad props to Peter and Jonathan for putting this series to work and
>>>> pushing it forward! Thanks!
>>>>
>>>> This series is based off Cédric's aspeed-7.1 tree, so it includes the
>>>> register fields. This is all "old register mode", but Peter seems to
>>>> have added support in new mode.
>>>>
>>>> There are some loose ends of course, i.e send_async doesn't handle
>>>> broadcast and asynchronous slaves being sent stuff can't nack. But I
>>>> wanted to get some feedback on the interface before I tackle that.
>>>>
>>>> This series
>>>> ~~~~~~~~~~~
>>>> Patch 1 and 2 are small Aspeed I2C changes/additions.
>>>>
>>>> Patch 3 adds support for multiple masters in the i2c core, allowing
>>>> slaves to master the bus and (safely) issue i2c_send/recv().
>>>>
>>>> Patch 4 adds an asynchronous send i2c_send_async(I2CBus *, uint8) on 
>>>> the
>>>> bus that must be paired with an explicit ack using i2c_ack(I2CBus 
>>>> *). We
>>>> have previously discussed how we wanted to handle the issue that some
>>>> slaves implement this and some do not. Using a QOM interface was up, 
>>>> but
>>>> couldn't figure out a good way to do it. I ended up decided against it
>>>> since I believe this have to be a run-time check anyway. The problem is
>>>> that a slave can master the bus and try to communicate with *anyone* on
>>>> the bus - and there is no reason why we should only allow asynchronous
>>>> slaves on the bus in that case, or whatever we would want to do when
>>>> devices are plugged. So, instead, the current master can issue an
>>>> i2c_start_send() and if that fails (because it isnt implemented by the
>>>> target slave) it can either bail out or use i2c_start_send_async() 
>>>> if it
>>>> itself supports it. This works the other way around as well of course,
>>>> but it is probably simpler to handle slaves that respond to
>>>> i2c_start_send(). This approach relies on adding a new i2c_event, which
>>>> is why a bunch of other devices needs changes in their event handling.
>>>>
>>>> Patch 5 adds *partial* slave mode functionality to the emulated Aspeed
>>>> I2C controller, that is, it only supports asynchronous sends started by
>>>> another slave that is currently mastering the bus. No asynchronous
>>>> receive.
>>>
>>> If there are no objections, I think this is a good way to move forward
>>> and improve this initial implementation when the need arises.
>>>
>>
>> There is an outstanding issue with the SLAVE_ADDR_RX_MATCH interrupt bit
>> (bit 7). Remember from my first series I had a workaround to make sure
>> it wasnt masked.
>>
>> I posted this upstream to linux
>>
>>    
>> https://lore.kernel.org/lkml/20220602054842.122271-1-its@irrelevant.dk/
>>
>> Not sure if that is the right way to fix it. 
> 
> That's weird. I would have thought it was already enabled [ Adding Jae ]

Slave mode support in Aspeed I2C driver is already enabled and it has
worked well so far. The fix Klaus made in the link is incorrect.

https://lore.kernel.org/lkml/20220602054842.122271-1-its@irrelevant.dk/

The patch is adding ASPEED_I2CD_INTR_SLAVE_MATCH as a mask bit for
I2CD0C (Interrupt Control Register) but actually this bit is part of
I2CD10 (Interrupt Status Register). Means that the slave match interrupt
can be enabled without enabling any mask bit in I2CD0C.

Thanks,
Jae

>> You mentioned something about "fixing" a mask on the ast2600?
> 
> This can be addressed later.
> 
> The model could be more precise since the driver is masking the value
> already we should be fine. See commit 3fb2e2aeafb2 ("i2c: aspeed: disable
> additional device addresses on ast2[56]xx") from Zeiv.
> 
>  From the datasheet.
> On the AST2400 (only 1 slave address)
> 
>    * no upper bits
> 
> On the AST2500 (2 possible slave addresses),
> 
>    * bit[31] : Slave Address match indicator
>    * bit[30] : Slave Address Receiving pending
> 
> On the AST2600 (3 possible slave addresses),
> 
>    * bit[31-30] : Slave Address match indicator
>    * bit[29] : Slave Address Receiving pending
> 
> Thanks,
> 
> C.
> 
>>
>> But with the above patch, all works an intended and no "workaround"
>> required.
>>
>>>> Finally, patch 6 adds an example device using this new API. The device
>>>> is a simple "echo" device that upon being sent a set of bytes uses the
>>>> first byte as the address of the slave to echo to.
>>>>
>>>> With this combined I am able to boot up Linux on an emulated Aspeed 
>>>> 2600
>>>> evaluation board and have the i2c echo device write into a Linux slave
>>>> EEPROM. Assuming the echo device is on address 0x42:
>>>>
>>>>     # echo slave-24c02 0x1064 > /sys/bus/i2c/devices/i2c-15/new_device
>>>>     i2c i2c-15: new_device: Instantiated device slave-24c02 at 0x64
>>>>     # i2cset -y 15 0x42 0x64 0x00 0xaa i
>>>>     # hexdump /sys/bus/i2c/devices/15-1064/slave-eeprom
>>>>     0000000 ffaa ffff ffff ffff ffff ffff ffff ffff
>>>>     0000010 ffff ffff ffff ffff ffff ffff ffff ffff
>>>>     *
>>>>     0000100
>>>
>>> I have started working on buildroot images  :
>>>
>>>    https://github.com/legoater/buildroot/commits/aspeed
>>>
>>> The resulting files are quite small :
>>>
>>>      $ ll output/images/
>>>      total 86040
>>>      drwxr-xr-x 2 legoater legoater     4096 Jun  1 20:01 ./
>>>      drwxrwxr-x 6 legoater legoater     4096 Jun  1 19:40 ../
>>>      -rwxr-xr-x 1 legoater legoater    36837 Jun  1 20:01 
>>> aspeed-ast2600-evb.dtb*
>>>      -rw-r--r-- 1 legoater legoater 67108864 Jun  1 20:01 flash.img
>>>      -rw-r--r-- 1 legoater legoater  6682796 Jun  1 20:01 image.itb
>>>      -rw-r--r-- 1 legoater legoater     1846 Jun  1 20:01 image.its
>>>      -rw-r--r-- 1 legoater legoater  3168768 Jun  1 20:01 rootfs.cpio
>>>      -rw-r--r-- 1 legoater legoater  1026660 Jun  1 20:01 rootfs.cpio.xz
>>>      -rw-r--r-- 1 legoater legoater  3788800 Jun  1 20:01 rootfs.tar
>>>      -rw-r--r-- 1 legoater legoater   653777 Jun  1 20:00 u-boot.bin
>>>      -rw-r--r-- 1 legoater legoater  5617280 Jun  1 20:01 zImage
>>>
>>> I will probably host them on GH and we could use them under avocado
>>> to extend the tests.
>>>
>>>
>>> They should boot real HW. I will submit the defconfigs to buildroot
>>> after more tests and cleanups.
>>>
>>
>> Nice!
>