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

Improve GHDL Tool #412

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

Improve GHDL Tool #412

wants to merge 6 commits into from

Conversation

psi-chuffine
Copy link

This works with my simple builds, but needs additional testing and validation for libraries and more complex GHDL usage.
I will correct any problems as I encounter them.

Correct commands.add formatting
Add steps to perform GHDL action
Clean up ghdl.py
Add support for libraries
* Needs validation and testing
@psi-chuffine
Copy link
Author

Apologies for the overbroad PR, I should have branched the cocotb addition.

@olofk
Copy link
Owner

olofk commented Jan 25, 2024

There seems to be issues still I tried it with an old VHDL example that I put together for other reasons a couple of years ago. Attaching the example project if that is of any use for debugging.

Run fusesoc run --target=simold libaxis for the old Tool API or fusesoc run --target=sim libaxis for the new Flow API that you are working on.

vhdl_example.tar.gz

@psi-chuffine
Copy link
Author

I'm concatenating GHDL calls incorrectly. I should have a fix shortly.

Break each library import step into its own build step
@psi-chuffine
Copy link
Author

I have it building, almost, with the test set you sent.
I'm currently getting

  shared variable words : t_mem(0 to TRANSACTIONS-1)(WIDTH-1 downto 0);
                  ^
src/libaxis_0/libaxis/bench/vhdl/suv/tb_axis_sync_fifo.vhd:56:19:note: (you can use -frelaxed to turn this error into a warning)
  shared variable words : t_mem(0 to TRANSACTIONS-1)(WIDTH-1 downto 0);

This is coming from analyze_options. The old GHDL tool picks up analyze_options from libaxis_0.eda.yml, but the new tool doesn't. Why would that be the case?
I'm also assuming that each command in the flow api philosophy should be one action, so I've broken the library imports into independent stages.

@alexfanqi
Copy link

alexfanqi commented Feb 22, 2024

This is coming from analyze_options

I think you need to add those into TOOL_OPTIONS, tool_options are filtered through it.

@alexfanqi
Copy link

Does it make sense to also support having top level entity in a library other than work? Right now, I get No rule to make target 'work-obj08.cf', needed by 'run'.

@psi-chuffine
Copy link
Author

I should be able to make that change, I set the flow to require f"work-obj{standard}.cf" to ensure the build order is handled correctly as I have always used work as my top level.

@olofk
Copy link
Owner

olofk commented Mar 7, 2024

Sorry. I never got back to this one. Are we all good at this point, or are there any issues remaining? Did I understand correctly that this requires work to be the toplevel lib? If so, I think we might need to reconsider. I know code out in the wild that uses a specific name for the toplevel lib too.

@psi-chuffine
Copy link
Author

I've still got an action here. I'll post an update when I have made the changes.

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

3 participants