-
Notifications
You must be signed in to change notification settings - Fork 11
ot_i2c_transport
: implement device for external I2C communication
#190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: ot-9.2.0
Are you sure you want to change the base?
Conversation
4aa1651
to
c990a45
Compare
c990a45
to
f759510
Compare
e89e6b2
to
d0289a0
Compare
d0289a0
to
d9c2532
Compare
0e7efaa
to
d716e69
Compare
docs/opentitan/i2c_transport.md
Outdated
|
||
* During a read transaction, a `B` byte can be written to read a byte from the device. | ||
|
||
* During a write transaction, written bytes are sent to the device. These will be written back on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure to exactly get it:
A real I2C device respond with a ACK/NACK on each written byte, i.e. a NACK may happen at anytime, in which case the master should stop the on-going request. I'm not sure to understand if the protocol fulfills this chronology, as I understand it is more transaction like, but I'm not sure to get it right.
I think it would be nice to have a temporal view of the example, such as
BYTE 0 1 2 3
-----|---------|------|-----|-----|
MAS 51W AA BB CC
DEV 51W AA BB !!
Not sure my understanding is correct.
Trying to write this make me wonder why we are not sticking to something closer to the actual I2C protocol.
For example, the first "byte" which requires 3 characters cannot be replied to before the 3 chars, which differs from the payload bytes where the reply can be emitted after 2 chars. I'm wondering if this is not prone to ouf of sync conditions.
The intention of this device is to be an I2C host device that is driven externally by OpenTitanTool. It will be used to issue read and write operations to other I2C devices on its bus - of primary interest is issuing commands to the OpenTitan I2C host/target block. This "bus device" is present on the bus, but it cannot be interacted with by other bus devices and cannot have transactions targetting it. Signed-off-by: Alice Ziuziakowska <[email protected]>
Signed-off-by: Alice Ziuziakowska <[email protected]>
Signed-off-by: Alice Ziuziakowska <[email protected]>
Needed for `ot_i2c_transport` to be able to communicate with `ot_i2c` in target mode. Signed-off-by: Alice Ziuziakowska <[email protected]>
d716e69
to
f3c8d36
Compare
I've made the wire protocol use raw bytes now instead of hex encoded ones, as I now have a functional implementation to talk to this in I have also made the device throttle processing using a timer to allow control to be returned to the vCPU so that it can observe the transfers taking place and prepare response bytes. I thought to do this manually in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is something I deeply do not understand with how the I2C stack is managed, but I do not want to delay this PR any further, as I guess the feature is needed.
I'll try to find some time to dig deeper.
s->parser_state = CMD_I2C_WRITE; | ||
break; | ||
case CMD_I2C_ERR: | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a trace/warning?
nit pick: do not forget to update the format of the PR title |
ot_i2c_transport
: implement device for external I2C communication
I had a very quick look at the I2C target mode documentation, and I think the QEMUTimer is not required, as the target implements clock stretching. I think what is required is that the ACK/NACK (controller to target write) or payload (controller from target read) should only be send when the respective HW FIFO are in the proper state. If the FIFO is full (write) no ACK/NACK should be reported (and obviously not data should be send if the FIFO is empty (read). It looks like everything can be managed with the communication "transport" protocol, and I'm still not sure it covers all use cases and stick to the actual I2C implementation, such as:
If the remote host performs an illegal request (e.g. pushing a new byte whereas it has not received a ACK or NACK), the state machine should be moved to ERROR, and only restarted on a new connection for example. To sum up, I keep thinking it would help to draft some kind of chronogram to compare the I2C physical bus signalling vs. its translation into byte exchange over the comm. socket, which should cover all standard cases. |
the 7-bit target address of the transaction and the read/write bit as the least significant bit | ||
(0 indicates a write transfer, 1 indicates a read transfer). | ||
|
||
- On a repeated start condition, the I2C target address of the first transfer will be used, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure to understand how you discriminate signaling (START/STOP/RESTART/ACK/NACK) from payload.
For example in b's123s123S'
, is the second s
a restart or the 4th byte of the payload?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you started a write transaction, before each payload byte you send a W
first to indicate the next byte is payload, or instead of W
you can use repeated start s
or stop S
.
This would be ideal, however clock stretching is unimplemented in I2C, as QEMU's I2C model is quite simplistic and I2C operations cannot be deferred until some data ready condition (e.g As a workaround, the timer is used to throttle the I/O to yield control back to the OT I2C, so that it can observe the transactions going on. Otherwise read transfers will all complete immediately on the I/O thread when it decides to process the I2C transfer, and at no point will OT I2C be aware a read transfer is taking place and have no opportunity to fill the TX FIFO.
This is already the case for transfers in both directions - once a read transfer is started, the controller issues a read by sending an At some point I would like to implement a better I2C layer to model this better, there are already some wrappers such as |
Yes you are right, the I2C API is far too limited, we will need to extend it at some point. |
This is an I2C device that allows external software such as
opentitanlib
to communicate with and issue commands to devices on a QEMU I2C bus. The communication is done through achardev
.See
docs/i2c_transport.md
for usage and protocol information.The device can be instantiated with these command line parameters:
-chardev pty,id=i2ctp -device ot-i2c-transport,bus=<bus>,chardev=i2c0tp
, to instantiate on an I2C bus.See also lowRISC/opentitan#28450 for an implementation and test harness using this device.
Nice to haves: