Skip to content
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

hdl.mem: document ReadPort and WritePort. #499

Merged
merged 1 commit into from Sep 17, 2020

Conversation

jfng
Copy link
Member

@jfng jfng commented Sep 15, 2020

Fixes #496.

Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

Good work! There's a few clarifications I would add.

Read data.
en : Signal or Const, in
Read enable. If asserted, ``data`` is updated with the word stored at ``addr``. If the
port is transparent, it is hardwired to 1 and cannot be driven by user logic.
Copy link
Member

Choose a reason for hiding this comment

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

It's worth mentioning that this hardwiring behavior is not what we want to do, but in fact the consequence of a Yosys design issue.

Memory associated with the port.
domain : str
Clock domain. Defaults to ``"sync"``. If set to ``"comb"``, the port is asynchronous.
Otherwise, reads have a latency of 1 clock cycle.
Copy link
Member

Choose a reason for hiding this comment

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

I would rephrase this along the lines of "the read data becomes available on the next clock cycle", since otherwise there's a bit of ambiguity with pipelined reads where that'd be an extra clock cycle.

@jfng
Copy link
Member Author

jfng commented Sep 16, 2020

Updated to address previous comments.

@whitequark whitequark merged commit 69ed491 into amaranth-lang:master Sep 17, 2020
@jfng jfng deleted the add-mem-docs branch September 17, 2020 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

no doc string for en in ReadPort
2 participants