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

[xilinx] Variable AXI requirement support #888

Merged
merged 15 commits into from
Mar 17, 2022
Merged

[xilinx] Variable AXI requirement support #888

merged 15 commits into from
Mar 17, 2022

Conversation

yn224
Copy link
Contributor

@yn224 yn224 commented Jan 31, 2022

This PR lays out the initial idea for enabling generation of xclbin files for examples with different number of external memory declaration (Issue #853).

fud/bitstream/gen_xo.tcl Outdated Show resolved Hide resolved
Copy link
Contributor

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Seems like a good first step; thanks!!

I'm not sure what your current workflow is, but one way to check whether everything is still working (i.e., we haven't broken anything yet) is to use the tests I set up in #866. You can do go tests/xilinx and type runt there to see if it still works.

Worth noting: this approach differs slightly from the plan I sketched in #853 (comment) in that it uses a number, rather than a list of AXI interface names. I assume you did this because it's a bit simpler to implement on the Tcl side? If so, that's true, but it's also a little less flexible (i.e., fud can't dictate the exact names of the AXI interfaces). I don't know if that will be limiting in the future. It's probably fine, but it would be good to leave plenty of clear comments here and there describing exactly what's going on, i.e., that interfaces must be called mN_axi for some integer N.

If you want to do the more flexible "pass the names" approach, I think you'll want to use Tcl's foreach loop to iterate over $::argc, skipping the first element because that's the output filename.

fud/fud/stages/xilinx/xclbin.py Outdated Show resolved Hide resolved
@rachitnigam
Copy link
Contributor

@yn224 my changes to fud conflict with this PR so you'll have to resolve them. Sorry about that! It should be straightforward.

@rachitnigam
Copy link
Contributor

@yn224 is this ready to merge?

@yn224
Copy link
Contributor Author

yn224 commented Feb 9, 2022

@rachitnigam @EclecticGriffin Sorry for the late response. I think this branch is ready to be merged. I'll try to work on a function that would return the arguments to Tcl file from the generated XML next. If you have any better suggestions for the next step, let me know.

Copy link
Contributor

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Thanks, @yn224! Can you please merge with or rebase on the master branch to resolve conflicts so this PR can be merged?

Also, can you please confirm that the Runt tests in #866 still work after this change? (I think that might require returning to hard-coding one interface.)

fud/fud/stages/xilinx/xclbin.py Outdated Show resolved Hide resolved
fud/bitstream/gen_xo.tcl Show resolved Hide resolved
@rachitnigam
Copy link
Contributor

Ah, github is being silly. It assumed that PR closed this one.

@rachitnigam rachitnigam reopened this Mar 9, 2022
@sampsyo
Copy link
Contributor

sampsyo commented Mar 9, 2022

Oops; indeed—thanks!

@yn224
Copy link
Contributor Author

yn224 commented Mar 14, 2022

@rachitnigam I have currently verified that the most recent commit to the master branch fixes testing issue so I integrated the logic for parsing XML file to extract out axi port names in xclbin.py file. However, I am not so sure where the logic should be placed to be able to take in the XML file as input.

@sampsyo
Copy link
Contributor

sampsyo commented Mar 14, 2022

Cool! In the spirit of taking this one step at a time, let's merge this PR without the XML parsing stuff and then tackle that in a separate PR.

@yn224
Copy link
Contributor Author

yn224 commented Mar 14, 2022

Done!

Copy link
Contributor

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Great; I think this is good to go!

@sampsyo
Copy link
Contributor

sampsyo commented Mar 17, 2022

Having heard no objections, I'm going to hit that green button now!

@sampsyo sampsyo merged commit 9fb873d into master Mar 17, 2022
@sampsyo sampsyo deleted the multi-axi branch March 17, 2022 19:43
@sampsyo
Copy link
Contributor

sampsyo commented Mar 17, 2022

If you have that XML-parsing stuff sitting around somewhere, @yn224, would you mind pushing it to a branch? I can help finagle it into the right location.

@yn224
Copy link
Contributor Author

yn224 commented Mar 18, 2022

If you have that XML-parsing stuff sitting around somewhere, @yn224, would you mind pushing it to a branch? I can help finagle it into the right location.

Sorry for the late response. The logic is currently pushed on the branch dedicated for PR #958.

@sampsyo
Copy link
Contributor

sampsyo commented Mar 18, 2022

Great!!

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.

4 participants