Skip to content

rtl: connect uDMA I2C status event - #372

Merged
MikeOpenHWGroup merged 2 commits into
openhwfoundation:masterfrom
shivamtiwari020505:agent/i2c-status-event-356
Aug 12, 2026
Merged

MikeOpenHWGroup merged 2 commits into
openhwfoundation:masterfrom
shivamtiwari020505:agent/i2c-status-event-356

Conversation

@shivamtiwari020505

Copy link
Copy Markdown
Contributor

What changed

The uDMA I2C controller already generated a legacy err_o pulse as busy_rise | arbitration_lost_rise, but udma_i2c_top left it disconnected. The STATUS register inputs were also tied low, and the parent subsystem did not place the signal in the SoC event vector.

This PR completes that path while preserving the established signal meaning:

  • expose BUSY-rise and arbitration-lost-rise separately from udma_i2c_control
  • transfer the two causes and the composite event from periph_clk_i to sys_clk_i with the existing acknowledged edge_propagator CDC pattern
  • set the sticky STATUS.BUSY and STATUS.AL bits from the synchronized causes
  • give a new condition priority over a simultaneous STATUS clear-on-read
  • drive top-level err_o from the synchronized legacy composite event
  • route I2C 0 and I2C 1 status events to peripheral event IDs 19 and 23
  • document the STATUS semantics, event mapping, and focused tests

Although the port is named err_o, BUSY rises on a normal START. The documentation therefore describes it as a status event and tells software to read STATUS for the cause.

Backward compatibility

  • Existing I2C RX/TX event IDs 16/17 and 20/21 do not move.
  • Event IDs 18 and 22 remain reserved.
  • IDs 19 and 23 were previously reserved and are now defined as I2C status events.
  • STATUS and SETUP addresses, bit positions, and reset values do not change.
  • FC, cluster, and peripheral event masks reset to all ones, so the new event is masked after reset. Software must explicitly unmask ID 19 or 23 to receive it.

Software that intentionally unmasked a formerly reserved event slot can now observe the newly defined event; software using the documented existing events continues unchanged.

Verification

  • Icarus: udma_i2c_control_status_tb passes
    • normal external START produces BUSY/status notification
    • normal STOP produces no additional notification
    • unexpected STOP during an active command produces arbitration-lost/status notification
  • Icarus: udma_i2c_status_tb passes with asynchronous clocks
    • reset behavior
    • BUSY, arbitration-lost, and simultaneous causes
    • one-cycle synchronized event
    • sticky STATUS and clear-on-read
    • set priority over a simultaneous STATUS read
  • Verilator lint/elaboration passes for the complete modified udma_i2c_top hierarchy; reported warnings are pre-existing legacy/vendor warnings.
  • git diff --check passes.

The two tests are available as FuseSoC targets and documented under rtl/udma/udma_i2c/tb.

Fixes #356

Signed-off-by: Shivam Tiwari <shivamtiwari020505@gmail.com>
@MikeOpenHWGroup

Copy link
Copy Markdown
Contributor

Hi @shivamtiwari020505, thanks for this PR! This is big one so it will take several days to review. In the meantime, I have several comments:

  1. In order to accept this PR, you must sign the Eclipse Contributor Agreement. See CONTRIBUTING for details. Note that when you create you Eclipse ID for the ECA, you should use the same email address as you use for GitHub.
  2. Thank you for adding the license header to the new files (e.g. rtl/udma/udma_i2c/rtl/udma_i2c_status.sv). However, you should not assign the copyright to 'OpenHW', you should claim it for yourself since you are the author and are making it available as an open source artifact.
  3. The udma_i2c_status module (rtl/udma/udma_i2c/rtl/udma_i2c_status.sv) is really a CDC module - maybe a better name for this module would be udma_i2c_status_ep? Also, please add a comment header to that module to explain why it is needed for the status signals.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR completes the uDMA I2C “status event” path by exposing BUSY-rise and arbitration-lost-rise, synchronizing them into the system clock domain, wiring sticky STATUS bits, and routing the resulting status event into the SoC event vector (IDs 19 and 23).

Changes:

  • Added a dedicated udma_i2c_status CDC block (edge-propagator based) and connected it in udma_i2c_top to drive err_o and STATUS inputs.
  • Updated the I2C register interface so STATUS bits are sticky, clear-on-read, and “set” wins over simultaneous clear-on-read.
  • Added focused Icarus/FuseSoC test targets and updated documentation for STATUS semantics and event mapping.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
rtl/udma/udma_i2c/udma_i2c.core Adds new RTL source and two FuseSoC test targets/filesets for status verification.
rtl/udma/udma_i2c/tb/udma_i2c_status_tb.sv New testbench validating CDC behavior and sticky/clear-on-read STATUS semantics under async clocks.
rtl/udma/udma_i2c/tb/udma_i2c_control_status_tb.sv New testbench validating BUSY/AL source pulses and legacy err_o meaning at controller level.
rtl/udma/udma_i2c/tb/README.md Documents how to run the new FuseSoC/Icarus status tests.
rtl/udma/udma_i2c/rtl/udma_i2c_top.sv Wires new status CDC module, connects STATUS inputs, and drives err_o from synchronized event.
rtl/udma/udma_i2c/rtl/udma_i2c_status.sv New CDC module using edge_propagator to transfer pulses to sys_clk_i.
rtl/udma/udma_i2c/rtl/udma_i2c_reg_if.sv Ensures new status conditions are not lost on simultaneous STATUS read (set priority).
rtl/udma/udma_i2c/rtl/udma_i2c_control.sv Exposes BUSY-rise and arbitration-lost-rise separately while preserving legacy err_o.
rtl/core-v-mcu/udma_subsystem/udma_subsystem.sv Routes I2C status event into the SoC peripheral event vector slot for each I2C instance.
docs/doc-src/ip-blocks/udma_i2cm.rst Updates STATUS register and err_o semantics documentation and event ID mapping.
docs/doc-src/ip-blocks/apb_event_cntrl.rst Updates peripheral event table: IDs 19 and 23 now defined for I2C status event.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread rtl/udma/udma_i2c/udma_i2c.core
@shivamtiwari020505

Copy link
Copy Markdown
Contributor Author

Thanks Mike. I have signed the ECA and pushed the requested updates. The new files now use my copyright, and I renamed the CDC helper to udma_i2c_status_ep. I also added a module header explaining why the status pulses need acknowledged clock-domain crossing.

While checking the source lists, I added the renamed module to src_files.yml as well. Both focused simulations and full hierarchy lint still pass.

Claim the new-file copyright for Shivam Tiwari, rename the helper to udma_i2c_status_ep, document its clock-domain crossing role, and keep both source manifests in sync.

Signed-off-by: Shivam Tiwari <shivamtiwari020505@gmail.com>
@shivamtiwari020505
shivamtiwari020505 force-pushed the agent/i2c-status-event-356 branch from ee101b6 to c800628 Compare August 12, 2026 15:37
@MikeOpenHWGroup

Copy link
Copy Markdown
Contributor

Thanks @shivamtiwari020505, looks good. Once you have a look at the comment from Copilot I can approve and merge.

@shivamtiwari020505
shivamtiwari020505 marked this pull request as ready for review August 12, 2026 15:58
@MikeOpenHWGroup
MikeOpenHWGroup merged commit 7ee3679 into openhwfoundation:master Aug 12, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

err_o of uDMA I2CM block not connected to anything

3 participants