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

Remove global package imports #70

Merged
merged 7 commits into from
Mar 11, 2020

Conversation

WRoenninger
Copy link
Collaborator

@WRoenninger WRoenninger commented Mar 9, 2020

This PR removes the global package imports of axi_pkg in synthesizable modules. I ran into issues with Verilator, where the global namespace was broken due to the package defining resp_t and subsequest modules defining a parameter resp_t in thier definition.
I further did some port naming cleanup and removed interfaces which are no longer used by the modules in this repo.
Changes:

Added

  • axi_cdc: Add AXI4-Lite interface wrap.
  • axi_intf: Add AXI4-Lite prot_t signal, adapted macros accordingly.

Changed

  • axi_cdc: Change ports src and dst to slv and mst.
  • axi_cut: Change ports in and out to slv and mst.
  • axi_multicut: Change ports in and out to slv and mst.
  • axi_intf: Removed global import axi_pkg::*;, made package calls explicit.
  • tb_axi_cdc: Removed global import, add typedef macros.

Fixed

  • axi_cut: Add missing AXI_LITE_ASSIGN macros in axi_lite_cut_intf.

Removed

  • axi_cdc: Removed unused global import of axi_pkg.
  • axi_intf: unused interface AXI_ROUTING_RULES
  • axi_intf: unused interface AXI_ARBITRATION

Currently the tb_atop_filter is broken in this PR, should however be related to #67.

Copy link
Contributor

@zarubaf zarubaf left a comment

Choose a reason for hiding this comment

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

Thanks. I am unsure about the re-naming of the ports. I can see two paths:

  • Either we agree on some common port names (that should then go into CONTRIBUTING.md)
  • or we keep them as they are right now.

In any case I would suggest we don't try to fix too many "problems" in one PR. I would re-factor the changes and only address the Verilator things in a first short (this can go out as a hot-fix). Then we can have a separate discussion about the port naming convention.

@andreaskurth
Copy link
Contributor

I agree with @zarubaf. Please split the changes.

@fabianschuiki
Copy link
Collaborator

I also agree on having a quick hotfix, and then get back to any naming conventions later.

@WRoenninger
Copy link
Collaborator Author

Removed the port naming changes from this PR.
However I left the removal of the two interfaces in as they were used in the old axi_lite_xbar and should no longer be used.

Copy link
Contributor

@andreaskurth andreaskurth left a comment

Choose a reason for hiding this comment

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

Thanks for these fixes, highly appreciated!

Several of your commits mixed changes in one file (e.g., adding _intf variant and fixing macro spacing). That makes it hard to split them if we are in agreement on one part of the changes but not on the other.

I have refactored your macro spacing fixes to a separate branch. I also agree with them, but we need to fix all occurrences, not just some. Please finish that work in #71.

@andreaskurth
Copy link
Contributor

Benches passing. Merging.

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.

None yet

4 participants