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.

Sign up for GitHub

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

Jump to bottom

Bikeshed: conventions for CDC primitives #97

Closed
whitequark opened this issue Jun 11, 2019 · 4 comments
Closed

Bikeshed: conventions for CDC primitives #97

whitequark opened this issue Jun 11, 2019 · 4 comments
Labels
question
Milestone
0.1

Comments

@whitequark
Copy link
Contributor

whitequark commented Jun 11, 2019 •
edited

One reason I've hesitated to merge #40 so far is that I think the oMigen conventions for CDC primitives are a bit annoying. I think a better situation than the current one would be:

  • Instead of arguments named domain, idomain, rdomain, etc, which only allow a single letter for domain designation, use domain, i_domain, r_domain, pll_domain, etc.
  • All modules which are likely to be transformed with DomainRenamer right after creation should allow changing the domain names through arguments. Notably this would include AsyncFIFO, which is probably responsible for most uses of DomainRenamer, but all other CDC primitives as well. Non-CDC modules may or may not allow that depending on intended use.

Thoughts? @sbourdeauducq @jordens @Wren6991
@sbourdeauducq
Copy link
Member

Sounds ok.

Wren6991 added a commit to Wren6991/nmigen that referenced this issue Jun 28, 2019
@Wren6991
lib.cdc: update clock domain naming conventions according to m-labs#97 …
c84cd8d 
…(bikeshed)
@Wren6991 Wren6991 mentioned this issue Jun 28, 2019
Open
@Wren6991
Copy link
Contributor

I like this, but find some of the results inconsistent (c84cd8d):

class Gearbox(Elaboratable):
   ...
    def __init__(self, iwidth, cd_i, owidth, cd_o):

Inconsistent use of suffix vs prefix is something I would trip over when typing too fast.

@whitequark
Copy link
Contributor Author

whitequark commented Jun 28, 2019 •
edited

I think Gearbox should of course use width_i and width_o here, for pretty much the same reasons as cd_i etc.

@whitequark whitequark added this to the 0.1 milestone Jun 28, 2019
whitequark added a commit that referenced this issue Sep 12, 2019
lib.cdc: adjust MultiReg for new CDC primitive conventions.
8f659b6 
Refs #97.
whitequark added a commit that referenced this issue Sep 12, 2019
lib.cdc: adjust ResetSynchronizer for new CDC primitive conventions.
9893e3c 
Refs #97.
whitequark added a commit that referenced this issue Sep 12, 2019
lib.fifo: adjust for new CDC primitive conventions.
da61076 
Refs #97.
@whitequark
Copy link
Contributor Author

whitequark commented Sep 12, 2019 •
edited

Since nMigen uses domain and not cd everywhere in the public API, I changed the convention slightly to match that, see original comment.

I've adjusted lib.cdc for this convention since it was not very invasive. I've also adjusted lib.fifo for this convention, and also have consistent naming elsewhere, but that is a larger diff, so I haven't committed it to master yet. Diff.

Feedback? @sbourdeauducq @Wren6991

Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
question
Projects
None yet
Milestone
0.1
Development

No branches or pull requests
3 participants
@whitequark @sbourdeauducq @Wren6991