lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.57k stars 769 forks source link

[i2c,rtl] Compress ACQ FIFO entries #22028

Open marnovandermaas opened 7 months ago

marnovandermaas commented 7 months ago

Description

This issue is based on a TODO that I added in this pull request: https://github.com/lowRISC/opentitan/pull/21857

Encode i2c_acq_byte_id_e more efficiently in the ACQ FIFO. Each entry in the ACQ FIFO does not need to contain both an 8 bit data field and a 3 bit identifier. We should have the ACQ FIFO be 9 bits wide where the MSB indicates whether it is a data byte or a control byte. This way we can add more values to this enum without having to widen the ACQ FIFO width.

The affected file: https://github.com/lowRISC/opentitan/blob/master/hw/ip/i2c/rtl/i2c_pkg.sv

andreaskurth commented 7 months ago

2 ID bits might be necessary to encode the six enum values together with the 8-bit data field. Something like this could work:

id     abyte
2'b00  <abyte> is a byte (could be address+r/w or data) that we NACK'd 
2'b01  <abyte> is a data byte that we ACK'd
2'b10  <abyte> contains the address+r/w that was ACK'd, but the transfer will continue and NACK subsequent data bytes (this only happens for writes)
2'b11  Start, Restart, or Stop -- depending on the value of <abyte>

Each entry in the ACQ FIFO can be up to 10 bit wide, so 2 ID bits are okay, but not more.

andreaskurth commented 7 months ago

@marnovandermaas may I ask you to please prioritize this (or let me know if you cannot work on it at the moment)? This is a prerequisite for using a 10-bit-wide SRAM to store ACQ FIFO entries (part of #22159).

marnovandermaas commented 7 months ago

@andreaskurth I won't have time to look at this before next week. One thing that was mentioned to me is that for a regular start it would be good to have an encoding of which I2C address it went to (at the moment this could be done with one bit) and also whether it was is a read or a write. I still think we can do it in 9 bits:

andreaskurth commented 7 months ago

Okay, thanks for the info. Let's hold this for now: With #22216 the SRAM that now backs all FIFOs is 13 bit wide (due to the width of the FMT FIFO; see #22166). If we don't need to further optimize the area, we'll close this issue (or at least defer it to a future revision).

andreaskurth commented 7 months ago

This may be easier than compressing the entries of the FMT FIFO (#22166), but as in the current architecture all FIFOs are stored in the same SRAM, there's no value in compressing just the ACQ FIFO entries. Potential future release and backlog.