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

[yxi] Rename calyx-axi-wrapper clock signal to ap_clk #2386

Merged
merged 7 commits into from
Jan 27, 2025

Conversation

nathanielnrn
Copy link
Contributor

@nathanielnrn nathanielnrn commented Jan 2, 2025

Another PR whittling down the changes present in #2267.
This PR changes the toplevel clock signal from clk to ap_clk. This should allow us to reuse the existing gen_xo.tcl and maintain backwards compatibility with the old verilog-axi-wrapper, without requiring a distinction between the two in the gen_xo.tcl file.

Also fixes a bsd vs gnu sed syntax error. The new command should work on both.
(bsd sed require terminating commands to end with a ;, gnu does not)

I'll also note that this PR touches a bunch of runt cocotb tests in tests/axi. I'm aware that there is a desire to get rid of large snapshots as they tend to get ignored. For now this setup is the best thing I have to make sure breaking changes aren't introduced to either axi-wrapper, so choosing to update them for now.

@nathanielnrn nathanielnrn changed the title [yxi] Rename [new] calyx-axi-wrapper clock signal to ap_clk [yxi] Rename calyx-axi-wrapper clock signal to ap_clk Jan 2, 2025
@@ -133,8 +133,8 @@ component m_read_channel(ARESETn: 1, RVALID: 1, RLAST: 1, RDATA: 32, RRESP: 2) -
n_RLAST = std_reg(1);
read_data_reg = std_reg(32);
bt_reg = std_reg(1);
curr_addr_internal_mem_incr = std_add(3);
curr_addr_axi_incr = std_add(64);
curr_addr_internal_mem_incr_1_1 = std_add(3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did these names start getting changed?

Copy link
Contributor

@rachitnigam rachitnigam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment but if there is a simple answer, please merge!

@nathanielnrn
Copy link
Contributor Author

Looks like the new numbers are a result of #2152. Not sure why these tests weren't caught there? That said, merging.

@nathanielnrn nathanielnrn merged commit 3160307 into main Jan 27, 2025
18 checks passed
@nathanielnrn nathanielnrn deleted the ap-clk-changes branch January 27, 2025 02:29
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.

2 participants