Opentitan EG 1.0.0 I2C implementation #162

ziuziakowska · 2025-08-28T09:00:14Z

This PR implements the Opentitan Earlgrey 1.0.0 I2C controller, based on the current Darjeeling I2C implementation which to my understanding is similar to Earlgrey ES.

The main differences are a different register interface with some moved fields, most FIFO overflow interrupts are replaced with threshold interrupts, and FIFO thresholds can be any amount of bytes.

Unsupported or partially implemented features:

Only 7-bit addressing is supported, with no address bit masking. (TARGET_ID only supports ADDRESS0 with MASK0 == 0x7f)
Loopback mode (CTRL.LLPBK) is unsupported.
Timing-related registers or physical layer controls are unimplemented:
- TIMING0-TIMING4 registers. (besides the changes in earlgrey-9.2.0 that at least check the values are within spec.)
- OVRD to manually drive SDA/SCL. VAL, ACQ_FIFO_NEXT_DATA.
- Clock stretching controls. (HOST_TIMEOUT_CTRL, TARGET_TIMEOUT_CTRL, TARGET_ACK_CTRL, HOST_NACK_HANDLER_TIMEOUT)

AlexJones0 · 2025-08-28T10:06:09Z

I've not had a chance to look in detail yet, but one quick high-level comment: as I understand, although the current QEMU Darjeeling I2C implementation (i.e. what is already in ot_i2c_dj.c) is similar to Earlgrey ES, the current OpenTitan Darjeeling I2C (i.e. what we want to emulate) should be similar to the current Earlgrey I2C.

So, I think in this PR it would be appropriate to just rename ot_i2c_dj to ot_i2c and use it for both tops, as I don't think there are any notable differences for emulation at the moment, so duplicating the device and having 2 I2C implementations will just be harder to maintain I think.

Aside: there do seem to have been 1 or 2 RTL Changes since earlgrey_1.0.0 was taken, but only to hardware internals like e.g. the DMA interface - these could be added to Darjeeling if necessary with versioning later on, like is done in e.g. the pwrmgr.

AlexJones0 · 2025-08-28T10:13:19Z

Also another note: if you could add a commit that quickly updates the status of I2C in docs/opentitan/earlgrey.md that would be great. It doesn't have to be super descriptive, just mention any notable supported/missing features as is done for the other IP.

loiclefort · 2025-08-28T15:27:25Z

I think in this PR it would be appropriate to just rename ot_i2c_dj to ot_i2c and use it for both tops

Agreed. Earlgrey and Darjeeling should share the same I2C implementation.

engdoreis

Thanks @ziuziakowska for this PR.
I'm not very familiar with the code base, so I'll leave in deth reviews for Alex and Loïc.
The only thing that I beleive that would make sense to change is to squash the commits:

"[ot] hw/riscv: ot_earlgrey: copy ot_i2c_dj into ot_i2c, add ot_i2c to eg"
"[ot] hw: riscv,opentitan: replace ot_i2c_dj with ot_i2c"

I would also suggest running clang-format/tidy on every commit.

rivos-eblot

Note: I'm not familiar enough with the I2C implementation to provide feedback about HW emulation, so comments are mostly about generic QEMU OT idioms.

hw/opentitan/ot_i2c.c

configs/devices/riscv32-softmmu/default.mak

hw/opentitan/ot_i2c.c

hw/riscv/Kconfig

hw/opentitan/ot_i2c.c

AlexJones0

Thanks for doing this @ziuziakowska, sorry it took me a while to get to.

I feel like I'm lacking the background to comment on the correct build configuration options so I'll leave that to @rivos-eblot, but I've left some comments on both the HW emulation and general QEMU idioms.

hw/opentitan/ot_i2c.c

AlexJones0 · 2025-09-04T11:23:08Z

hw/opentitan/ot_i2c.c

+         * threshold. */
+        ot_i2c_irq_set_state(s, ACQ_THRESHOLD, ot_i2c_acq_threshold_intr(s));
+        break;
+    case R_HOST_FIFO_STATUS:


Nit: really the masking here and in R_TARGET_FIFO_STATUS reads is not necessary due to the FIFO capacity, and if we did want to check this it might be more appropriate as a g_assert? (because it would mean something has gone horribly wrong about our understanding of the FIFOs).

AlexJones0 · 2025-09-04T11:39:44Z

hw/opentitan/ot_i2c.c

    IRQ_NAME_ENTRY(UNEXP_STOP),
    IRQ_NAME_ENTRY(HOST_TIMEOUT),
    /* clang-format on */
 };
 #undef IRQ_NAME_ENTRY

-#define OT_I2C_DJ_FIFO_SIZE 64u
+#define OT_I2C_FIFO_SIZE 64u


To give you more work: looking at OpenTitan's I2C, the acquisition FIFO actually has a different size of 268 bytes compared to the 64 bytes of the other FIFOs. Annoyingly, if this is documented anywhere I can't find it - maybe we should open an issue/PR for improving the I2C documentation?

If this change is going to be painful it might be a good idea to leave it to a future PR - after all, the I2C is already outdated, so there is no regression in this PR.

hw/opentitan/ot_i2c.c

rivos-eblot · 2025-09-04T14:15:17Z

I feel like I'm lacking the background to comment on the correct build configuration options so I'll leave that to @rivos-eblot, but I've left some comments on both the HW emulation and general QEMU idioms.

Build config seems ok to me, thanks for the changes.

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

taken from `hw/top_earlgrey/sw/autogen/top_earlgrey.h` Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

This updates the interrupts and registers for the I2C block from the earlgrey-es interface to earlgrey 1.0.0. `FIFO_STATUS` has been split off into `HOST_FIFO_STATUS` and `TARGET_FIFO_STATUS`, and some fields from `FIFO_CTRL` have been moved to the new `HOST_FIFO_CONFIG` and `TARGET_FIFO_CONFIG`. The remaining newly added registers are not yet implemented. Interrupts are now caused by exceeding a per-fifo threshold instead on overflow. Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

This commit makes each I2C controller create its own I2C bus using its `ot_id` in the bus name ('ot-i2c0`, `ot-i2c1`, `ot-i2c2`). Previously, each instantiation would overwrite the previous and leave only one `ot-i2c` bus. Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

This commit changes the names of `ot_i2c_{host,target}_get_{rx,tx}_threshold` to use the names of the FIFOs it is checking. It also adds `ot_i2c_{fmt,acq,rx,tx}_threshold_intr` functions to compare against the threshold to determine if the corresponding level interrupt should be asserted, as some FIFOs assert the interrupt below their threshold and others above. Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

With the removal of the `FMTILVL` and `RXILVL` fields, this register is now write-only. Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

These registers contain latched event bits that are RW1C. All bits must be cleared to deassert the interrupt. Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

This implements the `NAKOK` field when sending bytes in host mode. If `NAKOK` is set, then the current byte in a write transaction will not cause a controller halt interrupt if the byte is not ACK'd. The behaviour if READB and NAKOK are both set is unspecified in the docs so this is rejected. Since this flag needs to be stored for each byte, the FMT FIFO is upgraded from a `Fifo8` to a `OtFifo32` to store the extra bit. This bit is not sent over I2C as it only affects controller interrupts. Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

The I2C_NACK event seems to be unreachable as we do not use `i2c_send_async`, `i2c_send` returns a NACK by return value, `i2c_recv` is infallible, and we ACK every byte we receive in target mode so we cannot receive a byte that was NACK'd. Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

AlexJones0

Thanks for the changes - I'm happy to merge this after my comment is addressed.

@rivos-eblot is this okay to merge for you?
(edit: nevermind, I can't because of the requested changes anyway 😅)

AlexJones0 · 2025-09-04T14:54:55Z

hw/opentitan/ot_i2c.c

+    FIELD(ACQ_FIFO_NEXT_DATA, ACQ_FIFO_NEXT_DATA, 0u, 8u)
+REG32(HOST_NACK_HANDLER_TIMEOUT, 0x74u)
+    FIELD(HOST_NACK_HANDLER_TIMEOUT, VAL, 0u, 31u)
+    FIELD(HOST_NACK_ENABLER_TIMEOUT, EN, 31u, 1u)


Suggested change

FIELD(HOST_NACK_ENABLER_TIMEOUT, EN, 31u, 1u)

FIELD(HOST_NACK_HANDLER_TIMEOUT, EN, 31u, 1u)

There's also a typo here:
FIELD(TARGET_TIMOUT_CTRL, VAL, 0u, 31u)

rivos-eblot

LGTM

rivos-eblot · 2025-09-05T07:34:08Z

@rivos-eblot is this okay to merge for you? (edit: nevermind, I can't because of the requested changes anyway 😅)
Yes thanks.

ziuziakowska requested review from engdoreis and AlexJones0 August 28, 2025 09:01

ziuziakowska force-pushed the ot_i2c branch 2 times, most recently from 873b2e4 to 5d61b85 Compare August 28, 2025 09:28

ziuziakowska changed the base branch from ot-earlgrey_1.0.0-9.2.0 to ot-earlgrey-9.2.0 August 29, 2025 09:54

ziuziakowska force-pushed the ot_i2c branch 8 times, most recently from bb95860 to 713c8c8 Compare August 29, 2025 13:31

ziuziakowska marked this pull request as ready for review August 29, 2025 13:46

ziuziakowska force-pushed the ot_i2c branch from 713c8c8 to 86679b9 Compare September 1, 2025 08:45

engdoreis reviewed Sep 1, 2025

View reviewed changes

ziuziakowska force-pushed the ot_i2c branch from 86679b9 to 2429460 Compare September 1, 2025 10:22

ziuziakowska changed the title ~~DRAFT: Opentitan EG 1.0.0 I2C implementation~~ Opentitan EG 1.0.0 I2C implementation Sep 1, 2025

ziuziakowska requested a review from loiclefort September 1, 2025 14:07

ziuziakowska mentioned this pull request Sep 3, 2025

Create I2C QEMU test lowRISC/opentitan#28134

Open

ziuziakowska force-pushed the ot_i2c branch from 2429460 to 8a6400e Compare September 3, 2025 10:48

ziuziakowska marked this pull request as draft September 3, 2025 11:18

ziuziakowska force-pushed the ot_i2c branch 2 times, most recently from 5c49a29 to 743d259 Compare September 3, 2025 13:09

rivos-eblot requested changes Sep 3, 2025

View reviewed changes

ziuziakowska force-pushed the ot_i2c branch from 743d259 to 8096505 Compare September 3, 2025 14:07

ziuziakowska requested a review from rivos-eblot September 3, 2025 14:11

ziuziakowska marked this pull request as ready for review September 3, 2025 15:24

ziuziakowska requested a review from engdoreis September 3, 2025 15:24

rivos-eblot requested changes Sep 3, 2025

View reviewed changes

hw/riscv/Kconfig Outdated Show resolved Hide resolved

hw/riscv/Kconfig Outdated Show resolved Hide resolved

hw/opentitan/ot_i2c.c Outdated Show resolved Hide resolved

hw/opentitan/ot_i2c.c Outdated Show resolved Hide resolved

ziuziakowska force-pushed the ot_i2c branch from 8096505 to 52c6370 Compare September 4, 2025 10:12

ziuziakowska requested a review from rivos-eblot September 4, 2025 11:33

AlexJones0 reviewed Sep 4, 2025

View reviewed changes

rivos-eblot reviewed Sep 4, 2025

View reviewed changes

hw/opentitan/ot_i2c.c Outdated Show resolved Hide resolved

hw/opentitan/ot_i2c.c Outdated Show resolved Hide resolved

hw/opentitan/ot_i2c.c Outdated Show resolved Hide resolved

ziuziakowska force-pushed the ot_i2c branch from 52c6370 to 34f1e50 Compare September 4, 2025 14:26

ziuziakowska added 12 commits September 4, 2025 15:37

[ot] hw/riscv: ot_earlgrey: move ot_i2c_dj to ot_i2c, add to eg

798365e

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

[ot] hw/riscv: ot_earlgrey: add i2c plic irqs numbers

9b97bc6

taken from `hw/top_earlgrey/sw/autogen/top_earlgrey.h` Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

[ot] hw/opentitan: ot_i2c: impl HOST_FIFO_CONFIG,TARGET_FIFO_CONFIG

7975d94

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

[ot] hw/opentitan: ot_i2c: make FIFO_CTRL register write-only

a2ec62b

With the removal of the `FMTILVL` and `RXILVL` fields, this register is now write-only. Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

[ot] hw/opentitan: ot_i2c: impl CONTROLLER_EVENTS,TARGET_EVENTS

eeaed59

These registers contain latched event bits that are RW1C. All bits must be cleared to deassert the interrupt. Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

[ot] hw/opentitan: ot_i2c: add some additional comments and logging

e2c9f71

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

[ot] docs/opentitan: earlgrey: document I2C controller features

d91e0be

Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>

ziuziakowska force-pushed the ot_i2c branch from 34f1e50 to d91e0be Compare September 4, 2025 14:38

ziuziakowska requested review from rivos-eblot and AlexJones0 September 4, 2025 14:38

AlexJones0 approved these changes Sep 4, 2025

View reviewed changes

rivos-eblot approved these changes Sep 5, 2025

View reviewed changes

rivos-eblot self-requested a review September 5, 2025 07:53

rivos-eblot approved these changes Sep 5, 2025

View reviewed changes

	FIELD(HOST_NACK_ENABLER_TIMEOUT, EN, 31u, 1u)
	FIELD(HOST_NACK_HANDLER_TIMEOUT, EN, 31u, 1u)

Opentitan EG 1.0.0 I2C implementation #162

Are you sure you want to change the base?

Opentitan EG 1.0.0 I2C implementation #162

Conversation

ziuziakowska commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexJones0 commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexJones0 commented Aug 28, 2025

Uh oh!

loiclefort commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

engdoreis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rivos-eblot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexJones0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexJones0 Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

AlexJones0 Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rivos-eblot commented Sep 4, 2025

Uh oh!

AlexJones0 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexJones0 Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

rivos-eblot Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

rivos-eblot left a comment

Choose a reason for hiding this comment

Uh oh!

rivos-eblot commented Sep 5, 2025

Uh oh!

Uh oh!

ziuziakowska commented Aug 28, 2025 •

edited

Loading

AlexJones0 commented Aug 28, 2025 •

edited

Loading

loiclefort commented Aug 28, 2025 •

edited

Loading

engdoreis left a comment •

edited

Loading

AlexJones0 left a comment •

edited

Loading