-
Notifications
You must be signed in to change notification settings - Fork 582
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
RAM negated enable #1855
base: main
Are you sure you want to change the base?
RAM negated enable #1855
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some suggestions for minor cleanup.
Note to @BFH-ktt1: This adds memory with inverted enables to the list of things that are not supported in hdl. That doesn't break anything that currently exists, but should the hdl generation be enhanced to cover these attributes? If so, we should create an issue about that.
ramInvertOutputEnable = Invert output enable | ||
ramInvertWriteEnable = Invert write enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, it would be clearer for the users if we speak in case of enable, reset, and set signals of activity instead of negated/inverted inputs, i.e., the attribute refers to the control signal and the user has the choice between "high-active" and "low-active".
@figurantpp, @davidhutchens, @BFH-ktt1: What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like "Output Enable Mode: High Active/Low Active" sounds ok to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it is clearer to state the active level rather than speak of inverting something. Active High/Active Low sounds better to my ear than High Active/Low Active.
IMHO, it would be nice if the activity is considered directly in the HDL generation. It should be fairly easy to add with the help of generic parameters, e.g., called |
There are two other sets of enables with RAM. There are the Byte Enables (BE) and the Line Enables (LE). Those could also be Active High or Active Low. There are up to 8 of each. The BE and LE are never active together so only one additional Attribute would be necessary assuming it makes sense to force all 8 to have the same Active Level. @maehne and @BFH-ktt1: Does what I wrote make sense? Remember, I am not a hardware person. If it does make sense, should it be added to this PR or done later? |
I'm not particularly used with HDL, but I can try working with it. |
This PR contains an implementation for a negated output and write enable for the RAM component as requested in issue #1510.
The implementation considers errors/unknowns as false values when the invert enable is activated (so it does enable read/write). When the output/write enable invert is on, a little circle shows up in the connection, just like the clock does.