openXC7 / nextpnr-xilinx

Experimental flows using nextpnr for Xilinx devices
ISC License
38 stars 14 forks source link

Contribute xc7k update to upstream #3

Closed unbtorsten closed 2 years ago

unbtorsten commented 2 years ago

This issue is to pursue propagating updates for Kintex7 to upstream. At this time, this seems to be only a couple of changes to Python scripts, cf. https://github.com/kintex-chatter/nextpnr-xilinx/commit/1c89de2d7f7025e870a82c3ad5788fc58bf0f48a @hansfbaier: Based on your changes, does the code need to be further abstracted before are pull request is possible? Why was it necessary to distinguish between the variables name and basepart?

The second difference is the submodule prjxray-db which has been changed. In this case, an update of upstream prjxray is already pending (subject to their CI).

hansfbaier commented 2 years ago

name is the full name including footprint-speedgrade, basepart is only the part name without the footprint. Got ideas for better names?

unbtorsten commented 2 years ago

Thank you for clarifying. I am not so much concerned with the naming of the variables, just wanted to make sure I understand their contents.

My next question is: why is it necessary to remove the speedgrade when dealing with Kintex7, while other families use it to specify the package (or at least are not bothered by the presence of this suffix)?

hansfbaier commented 2 years ago

The current solution should work for any part, and I can't really see how it could have worked before for other parts like artix7 too. Because the database directory structure is like this: image With the kintex directory looking similar to this

unbtorsten commented 2 years ago

Update: this comment is superseded. See my post below for results of further investigation.

After a close look at the Python scripts and documentation (both here and at nextpnr-xilinx), I found that:

Next steps I don't have the code at hand right now, but I can look into this tomorrow. Meanwhile, feel free to test my hypothesis and adjust steps 9 and 10 of our make procedure to read

python3 xilinx/python/bbaexport.py --device xc7k325t --bba xilinx/xc7k325t.bba
build/bbasm -l xilinx/xc7k325t.bba xilinx/xc7k325t.bin

using the original nextpnr-xilinx code. If this yields the expected results, the changes to xilinx/python/xilinx_device.py in https://github.com/kintex-chatter/nextpnr-xilinx/commit/1c89de2d7f7025e870a82c3ad5788fc58bf0f48a would no longer be necessary.

hansfbaier commented 2 years ago

So how are you going to handle different chipdbs for different footprints with this approach?

unbtorsten commented 2 years ago

Looking further into the script and the files that it reads, I agree with you that this would never have been compatible with (the latest?) prjxray-db file structure:

Further investigation shows that prjxray's directory structure used to place these files in the same folder (January 2020): https://github.com/f4pga/prjxray/commit/e44027bcafa943a0f5e492412acf26fae3a3a780#diff-175c465d4f4da9458bab1c732b558ee8b10b087d14d052c160bb827fb7d668f2R17 and https://github.com/f4pga/prjxray/commit/e44027bcafa943a0f5e492412acf26fae3a3a780#diff-2aea961e96499d96f980a17a1110978f3bf66bcc1e4ec1e772aaec71f6932614R43

In Januar 2021, a commit changed the structure and effectively broke nextpnr-xilinx: https://github.com/f4pga/prjxray/commit/e60b0d587574764e5838f0ef844dd0fc9c460fd9

What's next? Based on this, I will create a new branch, cherry-pick your (@hansfbaier) commit, and send a pull request to upstream nextpnr-xilinx.

Last but not least, prjxray seems to have adopted this naming convention in its Makefiles:

Family (e.g. kintex7) > Fabric (e.g. xc7k325t) > Part (e.g. xc7k325tffg900-2)

I will change the basepart to fabricname and name to partname to align naming conventions.

unbtorsten commented 2 years ago

Changes to nextpnr-xilinx are identified as bugfix and have been sent upstream as a pull request.

The second difference of this fork is the submodule prjxray-db which has been changed to accommodate the latest k325 data. An update of upstream prjxray is already pending (subject to prjxray's CI).

hansfbaier commented 2 years ago

@unbtorsten Great, thanks!