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

prims: do some cleanups #295

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

thoughtpolice
Copy link
Contributor

@thoughtpolice thoughtpolice commented Jan 16, 2021

  • Delete some primitives like GreyCntr. These aren't used anywhere.
  • Remove some ifdef workarounds for Icarus Verilog that no longer apply, I believe. (Icarus v11.0 seems to simulate the included generate blocks fine, as far as I can tell?)
  • Consolidate most of the Vivado primitives with the generic RTL primitives. These are all just copy/paste identical files, except with a few verilog annotations for clock buffers/RAM primitives. Users can now instead just define VIVADO in their synthesis tool (hopefully, uh, vivado) in order to enable these.

Point 3 might be the most controversial, but I think this is significantly better than requiring the search paths to be amended for a tool or different primitives selected transparently. It would be even better if vendor tools would define some macros by default for auto-detection, but, alas...

I also plan on doing the same thing for Quartus too, but I wanted to get the thrust of all this out there first off.

This is all done in preparation for a fix for #139.

Icarus seems to support the 'generate' constructs used here for BRAMs,
so there's no need to use this workaround anymore.

Signed-off-by: Austin Seipp <[email protected]>
The Verilog primitives for Vivado are identical to the generic
primitives for all synthesis tools, except they have some Verilog
annotations for the clocking and RAM primitives to use the right
underlying logic. So all of these can be consolidated by just using
`ifdef appropriately. Do this, and remove all the copies.

Signed-off-by: Austin Seipp <[email protected]>
@thoughtpolice thoughtpolice changed the title prims: delete some old verilog files prims: do some cleanups Jan 16, 2021
@thoughtpolice
Copy link
Contributor Author

Re: point 3 — there is an odd discrepancy I discovered in the GatedClock primitive where it behaves completely differently on Vivado. I don't know what should be done about this, but it seems these are in conflict?

--- Verilog/GatedClock.v        2021-01-16 02:42:30.358357579 -0600
+++ Verilog.Vivado/GatedClock.v 2021-01-15 23:41:08.262363786 -0600
@@ -17,7 +17,7 @@
 // To avoid glitches, CLK_GATE_OUT only changes  when CLK_IN is low.
 // CLK_GATE_OUT follows CLK_GATE_IN in the same cycle, but COND is first
 // registered, thus delaying the gate condition by one cycle.
-// In this model, the oscillator CLK_OUT stop when the CLK_GATE_IN or
+// In this model, the oscillator CLK_OUT does *not* stop when the CLK_GATE_IN or
 // COND are deasserted.
 module GatedClock(
                  // ports for the internal register
@@ -54,7 +54,7 @@

    assign COND_OUT = COND_reg;

-   assign CLK_OUT = CLK_IN & new_gate ;
+   assign CLK_OUT = CLK_IN; // & new_gate ;
    assign CLK_GATE_OUT = new_gate ;

    // Use latch to avoid glitches
@@ -90,4 +90,3 @@
 `endif // BSV_NO_INITIAL_BLOCKS

 endmodule // GatedClock
-

@quark17
Copy link
Collaborator

quark17 commented Jan 20, 2021

Thanks for contributing cleanups! I haven't yet had a close look, but I think it's probably good to use ifdefs when the only difference is an attribute. I've asked @czeck to have a look, too, to have a second (and better) set of eyes on the Verilog files. But I did want to reply to your comment about GatedClock: The difference in these files is not a problem.

For gated clocks in a BSC design, BSC expects an oscillator wire and a gate wire, but BSC is agnostic about whether the oscillator still oscillates when the gate is de-asserted. BSC does not use the wires in a way that it matters. This allows the user to decide what is better for their synthesis (or simulation) target. So it's OK for the generic and Vivado versions to differ -- although we can certainly revisit the decision. (Maybe @czeck can say something about the original choices that went into it.)

@czeck
Copy link
Collaborator

czeck commented Jan 21, 2021 via email

Base automatically changed from master to main March 16, 2021 05:42
@quark17
Copy link
Collaborator

quark17 commented Jul 30, 2021

Thank you again for these cleanups. As you can see, I have merged the first two commits, leaving the third. (You may want to rebase this PR on the head, with just that commit?)

When a version of a module differs just by an attribute, having them in one file with an ifdef around the attribute makes sense just to avoid duplication that can get out of sync. But I worry that it may be inevitable that different tools may need drastically different modules. (For example, the Quartus versions of BRAM modules look quite different -- but they were created with old versions of Quartus and the situation may be improved now.)

I want to understand if the idea here is that all versions of a primitive would be defined in the same file. That is, if the Quartus version is wildly different, then there may just be a second module..endmodule block in the file, and ifdef directives would pick the appropriate one? The more I mull it over, the more I think I could get behind that idea.

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.

3 participants