Setup target

[rwhitby]

With these changes, the build instructions simplify to:

1. git clone --recurse https://github.com/kintex-chatter/xc7k325t-blinky-nextpnr.git
2. cd xc7k325t-blinky-nextpnr
3. make BOARD=qmtech setup
4. make BOARD=qmtech all

I will do a separate pull request with a target to install the prerequisite distribution packages for various platforms.

[unbtorsten]

Is there a prefix ${XRAY_DIR} missing for the path in https://github.com/rwhitby/xc7k325t-blinky-nextpnr/pull/1/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R40?

[unbtorsten]

This line https://github.com/rwhitby/xc7k325t-blinky-nextpnr/pull/1/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R50 and the following lines make the Makefile specific to the qmtech-package (xc7k325tffg676-1). The README procedure lays out all the steps and makes it more apparent, where this may be adapted to xc7k325tffg900-2 (package used on Digilent Genesys2). I see an issue with hiding this.

[unbtorsten]

I see a third unresolved issue in line https://github.com/rwhitby/xc7k325t-blinky-nextpnr/pull/1/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R60, which makes the update of the chipdb conditional. @hansfbaier noted at the end of the README procedure that this has to be updated upon changes in nextpnr. Building the entire environment from scratch will not be confronted with this. A user that chooses to update some the repos might end up in confusion, however.

[rwhitby]

Is there a prefix ${XRAY_DIR} missing for the path in https://github.com/rwhitby/xc7k325t-blinky-nextpnr/pull/1/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R40?

I did not find the fasm2frames in the install location, so I had to run it from the build area.

Is there another way to install prjxray which would put fasm2frames into the install location?

[rwhitby]

This line https://github.com/rwhitby/xc7k325t-blinky-nextpnr/pull/1/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R50 and the following lines make the Makefile specific to the qmtech-package (xc7k325tffg676-1). The README procedure lays out all the steps and makes it more apparent, where this may be adapted to xc7k325tffg900-2 (package used on Digilent Genesys2). I see an issue with hiding this.

That is a very good point. Shall the setup target require the BOARD definition and do the right thing for each board?

[unbtorsten]

Is there a prefix ${XRAY_DIR} missing for the path in https://github.com/rwhitby/xc7k325t-blinky-nextpnr/pull/1/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R40?

I did not find the fasm2frames in the install location, so I had to run it from the build area.

Is there another way to install prjxray which would put fasm2frames into the install location?

See lines 27 and 28 in a0a6d09#diff-bc037111b143ddd9c78ac9112f228a2aa13eb3fd4f37cf9381bdd82280d16454R27
I think this part was lost in the transition from makeit.sh to Makefile.

[rwhitby]

I see a third unresolved issue in line https://github.com/rwhitby/xc7k325t-blinky-nextpnr/pull/1/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R60, which makes the update of the chipdb conditional. @hansfbaier noted at the end of the README procedure that this has to be updated upon changes in nextpnr. Building the entire environment from scratch will not be confronted with this. A user that chooses to update some the repos might end up in confusion, however.

How about we add a "FORCE=1" variable which overrides all these conditionals and makes them unconditional?

Alternatively, the "clobber" target is intended to re-do everything, but I'm not sure if it should remove the .bba and .bin file as they take ages to rebuild.

I'm open to any suggestions ...

[rwhitby]

Is there a prefix ${XRAY_DIR} missing for the path in https://github.com/rwhitby/xc7k325t-blinky-nextpnr/pull/1/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R40?

I did not find the fasm2frames in the install location, so I had to run it from the build area.
Is there another way to install prjxray which would put fasm2frames into the install location?

See lines 27 and 28 in a0a6d09#diff-bc037111b143ddd9c78ac9112f228a2aa13eb3fd4f37cf9381bdd82280d16454R27 I think this part was lost in the transition from makeit.sh to Makefile.

I suspect why it was lost is because a source operation in a Makefile doesn't apply to subsequent lines unless you join all the lines together. The current Makefile has this issue. I will attempt to fix this and incorporate those lines from makeit.sh in the next version.

[unbtorsten]

[conditional update of the chipdb]

How about we add a "FORCE=1" variable which overrides all these conditionals and makes them unconditional?

+1
Sounds like a practical solution to me.

[unbtorsten]

[selection of target Kintex package during make]

Shall the setup target require the BOARD definition and do the right thing for each board?

I think, that this is the central question. Maybe an issue in kintex-chatter/xc7k325t-blinky-nextpnr would allow for deeper discussion of this aspect?

I guess that Makefiles can naturally handle this by means of different targets. However, I am concerned about the clarity of the make-process. For example, prjxray's hierarchical Makefile structure is highly integrated, but not always easy to understand. A list of commands for different Kintex packages may be a lower barrier for those, that would like to add a new Kintex package. Finally, it should be easy to remove them from our forks once upstream is updated. What do you think?

[unbtorsten]

Thank you for the update! I have a few more thoughts that I will add directly based off of the code.

[unbtorsten]

Why add prjxray as a submodule? I consider it auxiliary set of binaries and data just like openFPGALoader or yosys that one would have to make available. I agree that setup of the toolchain is a pain these days, but see this to be in the scope of F4PGA (formerly known as Symbiflow), cf. openXC7#18 (comment).

[rwhitby]

The main reason to add prjxray as a submodule is so that XRAY_DIR can be automatically set to a default value based on this know relative location for prjxray.

[unbtorsten]

Change to ${XRAY_DIR}/env/bin/fasm2frames --part ${PART} --db-root ${DB_DIR}/kintex7 $< > $@ ?

[rwhitby]

env/bin does not exist in the installed XRAY_DIR location (~/opt/prjxray) - it only exists in the local build location (./prjxray), so such a change would break the build.
Is there a way to make fasm2frames get installed into the installed XRAY_DIR location (~/opt/prjxray) ?

[rwhitby]

I will add XRAY_UTILS_DIR and XRAY_TOOLS_DIR variables. The former points to the local build area, the latter points to the installed location.

[unbtorsten]

I think we've had different scenarios in mind.. I had been assuming that an independent clone of prjxray be available in XRAY_DIR=~/opt/prjxray, i.e. cloning prjxray would not be controlled through this Makefile. This way, we would have both the XRAY_UTILS_DIR and XRAY_TOOLS_DIR in the same location. Either way, I think we don't need make install for our use of prjxray.

I don't know if fasm2frames can get installed to XRAY_DIR. In fact, make install seems to only be concerned with

• $INSTALL_DIR/usr/local/bin/xc7frames2bit
• $INSTALL_DIR/usr/local/bin/xc7patch
• $INSTALL_DIR/usr/local/bin/gen_part_base_yaml
• $INSTALL_DIR/usr/local/bin/frame_address_decoder
• $INSTALL_DIR/usr/local/bin/bittool
• $INSTALL_DIR/usr/local/bin/segmatch
• $INSTALL_DIR/usr/local/bin/bitread

fasm2frames, on the other hand, is part of the build environment.

[rwhitby]

Yes, I wonder why one is installed and one is left in the build area. If we need to use fasm2frames for this project, then one would expect others to also need to use it. Or is there some other way that prjxray is intended to be used which does not require fasm2frames to be called independently?

[unbtorsten]

There is a related discussion from 2020 at f4pga/prjxray#1384 that resulted in an independent repo for fasm2frames: https://github.com/SymbiFlow/f4pga-xc-fasm I have not tested the code therein.

I opened an issue for further clarification at chipsalliance/f4pga-xc-fasm#19

[unbtorsten]

The location of fasm2frames seems to be the only (and seemingly unresolved) open question at this time. Why don't you send the pull request to update the make process at kintex-chatter, and we file an issue against https://github.com/rwhitby/xc7k325t-blinky-nextpnr/pull/1/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R45 to track and resolve the need for prjxray sources in the future?

[rwhitby]

Opened openXC7#25 with these notes included.

[rwhitby]

Oh, sorry for any confusion. All I did was rebase against upstream changes. I have not made any intentional updates to address the important items you have raised previously yet. Will get to all this on the weekend.

[rwhitby]

I believe that my latest push now addresses all comments, and adds board-specific filenames so that multiple boards can be supported from the same project directory.

Once there is agreement here, I will submit the same pull request to the kintex-chatter upstream.

[unbtorsten]

I have a couple more thoughts:

• See my comment on locating prjxray and fasm2frames in the above code review, cf. Setup target #1 (comment) -- I can definitely live with a local clone of prjxray. I'm just wondering whether it is really necessary. After all, we are just using the original upstream code.
• Make PROJECT_NAME compatible with overwrite by an environment variable, cf. Multi-project support in same or separate repo? openXC7/xc7k325t-blinky-nextpnr#21