-
Notifications
You must be signed in to change notification settings - Fork 3
Initial AXI Module skeleton #31
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
base: main
Are you sure you want to change the base?
Conversation
Added some of the interfaces/ports as described in the one-pager for AXI Crossbar. Size and quantity of ports subject to change
rtl/bus/axi/axi_async_fifo.sv
Outdated
| input logic s_rst_ni, | ||
|
|
||
| // Destination clock domain | ||
| input logic m_clck_i, |
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.
Label Consistency: "m_clck_i" -> "m_clk_i"
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.
Fixed.
rtl/bus/axi/axi_crossbar.sv
Outdated
| )( | ||
| // TODO: add ports | ||
| input logic clk_i, | ||
| input logic rst_ni |
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.
Syntax Error: Add comma after port declaration
Meowcaroni
left a comment
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.
@XyzalAxel Please review comments, make changes accordingly, and resubmit PR (pull request).
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.
Syntax Error: Parameters should be listed with commas, not semicolons. Write in following style:
parameter int unsigned A = 1,
parameter int unsigned B = 2
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.
Syntax Error: Parameters should be listed with commas, not semicolons. Write in following style:
parameter int unsigned A = 1,
parameter int unsigned B = 2
| function automatic logic [PTR_SIZE-1:0] BinaryToGray(input logic [PTR_SIZE-1:0] binary); | ||
| BinaryToGray = (binary >> 1) ^ binary; | ||
| endfunction |
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.
Reference Call Error: Function is named "BinaryToGray" while references are named"binaryToGray". Make sure to update output variable within function as well if function name is changed.
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.
Style Issue: Move "begin" keywords to same lines associated components (always, if, etc.)
| parameter int unsigned N_S = 2, | ||
| parameter int unsigned ADDR_WIDTH = 32, | ||
| parameter int unsigned DATA_WIDTH = 64, | ||
| parameter int unsigned ID_WIDTH = 4, |
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.
Syntax Error: Extra comma
Meowcaroni
left a comment
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.
Overall pretty good, just minor syntax & style issues. Beyond what I already wrote, please leave anything that won't synthesize (EX: Writing "ports" with no definition) in comments in case someone is running simulations with all the files.
Added setups for AXI files and the Markdown file for AXI Crossbar. Implemented the one-pager by mapping each required documentation section directly onto the structure of the axi_crossbar module (Purpose & Role, Parameters, Ports, etc.). Also, using AMD's documentation regarding AXI Interconnect as a resource.