lib.cdc: add optional reset to MultiReg, and document its use cases #44

Wren6991 · 2019-03-19T03:51:21Z

Following on from #37.

This patch adds a reset_less parameter to MultiReg. The default behaviour is the same as the original behaviour. It also adds a docstring to MultiReg which, besides usual docstring stuff, adds the following footnote:

    Note on Reset
    -------------
    MultiReg is non-resettable by default. Usually this is the safest option;
    on FPGA the MultiReg will still be initialised to its `reset` value,
    when the FPGA loads its configuration bitstream.

    However, in designs where the value of the MultiReg must be valid
    immediately after reset, consider `reset_less`=False if:
     - You are targeting an ASIC, or an FPGA that does not initialise
       flipflop values
     - Your design features warm (non-power-on) resets of `odomain`, so
       the one-time initialisation at power on is insufficient
     - Your design features a sequenced reset, and the MultiReg must maintain
       its reset value until `odomain` reset specifically is deasserted.

    MultiRegs are reset by the `odomain` reset only.

I made sure to make it explicit that the default behaviour relies on FPGA bitstream config. Not sure if this is exactly the kind of documentation you intended -- let me know and I will fix it up :)

Thanks

codecov · 2019-03-19T04:07:17Z

Codecov Report

Merging #44 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master     #44   +/-   ##
======================================
  Coverage    85.5%   85.5%           
======================================
  Files          27      27           
  Lines        3912    3912           
  Branches      764     764           
======================================
  Hits         3345    3345           
  Misses        478     478           
  Partials       89      89

Impacted Files	Coverage Δ
nmigen/lib/cdc.py	`86.66% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d69a4e2...8a062aa. Read the comment docs.

whitequark · 2019-03-28T05:24:07Z

Thank you! I've manually merged this PR, with the following changes:

Some typos (int instead of bool, etc).
In ReST, single backticks are used for emphasis, double backticks are used for code/monospace.
Clarified some details in the note on reset (all FPGAs allow initializing flip-flops to at least zero; any of the listed conditions can be true for the reset to become necessary).
Reflowed the text for 100 columns, which is what's used in this codebase.

Wren6991 · 2019-03-28T07:30:35Z

int instead of bool, etc

Oops, potentially bad C codebase at work leaking through.

Thanks for merging, and sorry for the mess; that's great feedback. I'll go and rebase + fix up my other PR based on this.

lib.cdc: add optional reset to MultiReg, and document its use cases

8a062aa

whitequark closed this Mar 28, 2019

nmigen-issue-migration mentioned this pull request Jan 27, 2020

lib.cdc: add optional reset to MultiReg, and document its use cases amaranth-lang/amaranth#10

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib.cdc: add optional reset to MultiReg, and document its use cases #44

lib.cdc: add optional reset to MultiReg, and document its use cases #44

Wren6991 commented Mar 19, 2019

codecov bot commented Mar 19, 2019 •

edited

whitequark commented Mar 28, 2019

Wren6991 commented Mar 28, 2019

lib.cdc: add optional reset to MultiReg, and document its use cases #44

lib.cdc: add optional reset to MultiReg, and document its use cases #44

Conversation

Wren6991 commented Mar 19, 2019

codecov bot commented Mar 19, 2019 • edited

Codecov Report

whitequark commented Mar 28, 2019

Wren6991 commented Mar 28, 2019

codecov bot commented Mar 19, 2019 •

edited