Add valid & done to DataPortInterface by RossComputerGuy · Pull Request #274 · intel/rohd-hcl

RossComputerGuy · 2025-12-14T06:33:36Z

Description & Motivation

Adds a valid output to DataPortInterface. This makes it possible to know whether the read or write has completed successfully.

Adds a done output to DataPortInterface. This makes it possible to know whether the read or write has completed.

Related Issue(s)

Seems like #146 is somewhat related since this allows for polling if valid is set and knowing that marks things are valid.

Testing

Ensure test/memory_model.dart passes

Backwards-compatibility

Is this a breaking change that will not be backwards-compatible? If yes, how so?

Documentation

Does the change require any updates to documentation? If so, where? Are they included?

mkorbel1

Thank you for the thoughtful contribution! A few questions and comments

lib/src/models/memory_model.dart

lib/src/memory/memory.dart

mkorbel1 · 2026-01-03T00:34:40Z

lib/src/memory/memory.dart

      DataPortGroup.data
    ]);
+
+    setPorts([


thought: do we want to make these integrity signals optional/nullable via a flag and hardware components can dynamically choose whether to generated related logic based on their presence?

Maybe, though would that just be not connecting those up then? If they're not connected, I think ROHD would just not generate those.

One other consideration here is backwards compatibility. Making them nullable allows for existing implementations using DataPortInterface not to have to do anything should they not want to.

mkorbel1 · 2026-01-03T00:36:16Z

lib/src/memory/register_file.dart

+                  If(
+                wrPort.en & wrPort.addr.eq(entry),
+                then: [
+                  wrPort.valid < 1,


I wonder if including valid and done as part of the RF would impact synthesis' ability to identify these as RFs? Also, I wonder if this generates more logic than is necessary (i.e., can we just do an address range check and a shift-register instead of a per-entry case statement, and would that synthesize differently)?

Afaict, it doesn't impact yosys's ability to generate RF's. Yosys doesn't even recognize RF's as memories. Also, there's a chance that yosys would just optimize it better than how we have it. Though, idk if we even need an address range check. We might be able to get away with just having wrPort.valid < wrPort.en.

mkorbel1 · 2026-01-03T00:37:45Z

lib/src/memory/memory.dart

  Logic get data => port('data');

+  /// Whether the data was successfully read or written
+  Logic get valid => port('valid');


thought: does calling this valid overload the terminology related to "ready/valid" protocols in a confusing way? @desmonddak what do you think?

I think valid is the most sensible name since it lets you know whether the request was able to complete. In terms of reading, that means data is valid. In terms of writing, it means the address which was written to is valid.

mkorbel1 · 2026-01-03T00:41:58Z

test/memory_test.dart

it would probably be good to add some testing for when valid and done should be false

Been trying to but the sim is being picky.

lib/src/memory/csr/csr_block.dart

kimmeljo

Per the discussion on the ports being required versus optional, for backward compatibility reasons, I'd make the case that they should be optional. This can (probably will) simplify the change set for this PR.

kimmeljo · 2026-01-20T17:49:40Z

lib/src/memory/memory.dart

      DataPortGroup.data
    ]);
+
+    setPorts([


One other consideration here is backwards compatibility. Making them nullable allows for existing implementations using DataPortInterface not to have to do anything should they not want to.

desmonddak · 2026-01-20T18:51:57Z

The dilemma we face is that we are going to start using the DataPortInterface in many different ways for components and we may need to architect it so that there are derived from a base Interface class representing the simplest possible memory accessor. Right now, we are overloading it with byte output control, for example, whereas the simplest interface probably should not have that.

Furthermore, we are extending it with a valid bit to call it ValidDataPortInterface in https://github.com/intel/rohd-hcl/pull/259/files#diff-1db666ec943a7d409d08eda2bc00935cd71160c8ae7ae14b471fe5458fc5dde7 (see the first 100 lines of the cache.dart file).

In that same PR, we are also extending the Fifo to be ready/valid.

If we add valid/done to the base memory, we may later find it difficult to create a broad array of memory components (CAM/TCAM/RF/FIFO/SRAM/WriteBuffer) with the various configurations they need. So I want to be careful incrementally extending DataPortInterface as we probably should architect an Interface hierarchy of data interfaces that will give us this more general capability to our data interfaces across this spectrum of components (to be built eventually). Already, I think we should factor out the byte enable somehow and only configure it for components that support byte-enable.

I think the fork in the road is: do we add nullable Logic and parameters to this DataPortInterface or do we inherit from a simpler Interface class of address+data and build up the various kinds of interfaces for memory components, as well as a hierarchy of memory components that inherit from Memory or RF or SRAM and have more and more features.

For now, I would propose a nullable interface to keep this PR simple, but I am a bit concerned about the concept of 'done' when we want to build a read/valid protocol set of components and 'done' is introducing new semantics potentially.

RossComputerGuy · 2026-01-25T05:09:52Z

Per the discussion on the ports being required versus optional, for backward compatibility reasons, I'd make the case that they should be optional. This can (probably will) simplify the change set for this PR.

Yeah, that is a good idea.

The dilemma we face is that we are going to start using the DataPortInterface in many different ways for components and we may need to architect it so that there are derived from a base Interface class representing the simplest possible memory accessor. Right now, we are overloading it with byte output control, for example, whereas the simplest interface probably should not have that.

I wouldn't really consider this overloading since having a way to indicate the success & completion of an access is good.

Furthermore, we are extending it with a valid bit to call it ValidDataPortInterface in https://github.com/intel/rohd-hcl/pull/259/files#diff-1db666ec943a7d409d08eda2bc00935cd71160c8ae7ae14b471fe5458fc5dde7 (see the first 100 lines of the cache.dart file).

In that same PR, we are also extending the Fifo to be ready/valid.

Ah, I wasn't aware of that PR doing it.

If we add valid/done to the base memory, we may later find it difficult to create a broad array of memory components (CAM/TCAM/RF/FIFO/SRAM/WriteBuffer) with the various configurations they need. So I want to be careful incrementally extending DataPortInterface as we probably should architect an Interface hierarchy of data interfaces that will give us this more general capability to our data interfaces across this spectrum of components (to be built eventually). Already, I think we should factor out the byte enable somehow and only configure it for components that support byte-enable.

I think the fork in the road is: do we add nullable Logic and parameters to this DataPortInterface or do we inherit from a simpler Interface class of address+data and build up the various kinds of interfaces for memory components, as well as a hierarchy of memory components that inherit from Memory or RF or SRAM and have more and more features.

I think the nullable way is simpler for this PR. I am not sure about the other way. The thing that I mainly needed from this was knowing when memory, registers, and CSRs access was done and valid.

For now, I would propose a nullable interface to keep this PR simple, but I am a bit concerned about the concept of 'done' when we want to build a read/valid protocol set of components and 'done' is introducing new semantics potentially.

Imo, ready isn't a great name either since it sounds like it mostly fits read since a write being ready sounds like the data port is ready to be written into and not that it is done writing.

desmonddak · 2026-03-31T15:10:45Z

I think in the intervening interval, we merged a PR with significant filesystem reorganization, which means you currently have merge conflicts. I think you should consider a new interface that inherits (which may make it easier for the future I mentioned, which was to consider a tree of data-oriented interfaces for memory-like devices. Or you can proceed with a Logic? add to the existing. The interface is a contract, so if we push too much into the bottom, then there are more exceptions to document for rudimentary components (like a flop-based RF would probably have few features supported).

RossComputerGuy changed the title ~~Add valid to DataPortInterface~~ Add valid & done to DataPortInterface Dec 14, 2025

RossComputerGuy force-pushed the feat/dataport-valid branch 2 times, most recently from 6f3c66b to e08aeef Compare December 21, 2025 00:52

RossComputerGuy force-pushed the feat/dataport-valid branch 2 times, most recently from daaacdd to d62d867 Compare January 1, 2026 01:00

mkorbel1 requested review from desmonddak and kimmeljo January 3, 2026 00:26

mkorbel1 reviewed Jan 3, 2026

View reviewed changes

Add valid to DataPortInterface

660d6e8

RossComputerGuy force-pushed the feat/dataport-valid branch from d62d867 to 66bb37a Compare January 3, 2026 02:50

Add done to DataPortInterface

811db00

RossComputerGuy force-pushed the feat/dataport-valid branch from 66bb37a to 811db00 Compare January 3, 2026 03:05

kimmeljo reviewed Jan 20, 2026

View reviewed changes

Conversation

RossComputerGuy commented Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description & Motivation

Related Issue(s)

Testing

Backwards-compatibility

Documentation

Uh oh!

mkorbel1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kimmeljo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

desmonddak commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RossComputerGuy commented Jan 25, 2026

Uh oh!

desmonddak commented Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RossComputerGuy commented Dec 14, 2025 •

edited

Loading

desmonddak commented Jan 20, 2026 •

edited

Loading