Moving slowFPGA build to separate repo #53
Conversation
* Prior to splitting SlowFPGA to own repo
* Replace 'buffer' ports with copied 'out' ports * Replace case stmt using 'TO' in choice with if-else * Fix wrong TS constaintin timing constaints
* In recent versions of Vivado a licence is no longer for webpack parts. * If targeting a non-webpack device or using licenced IP, the * LM_LICENSE_FILE or XILINXD_LICENSE_FILE env var can, and should, * be set in the shell or user profile, prior to running make.
glennchid
requested review from
Araneidae,
thomascobb,
shu-soleil,
tomtrafford and
MichaelStubbings
There was a problem hiding this comment.
A few questions. I think one of the changes to CONFIG.example doesn't belong here, and I'm a bit puzzled by the move of a chunk of definitions from targets/PandABox/hdl/slow_defines.vhd to modules/system/hdl/system_registers.vhd.
Other than that looks fine. I do think changing bits to bit for the bit-file Makefile identifier is mostly churn, as a bitfile contains bits... but that's not really worth a review comment.
| export ISE = /dls_sw/FPGA/Xilinx/14.7/ISE_DS/settings64.sh | ||
| export LM_LICENSE_FILE = [email protected] |
There was a problem hiding this comment.
Why have these been removed? The LM_LICENSE_FILE is definitely still needed...
There was a problem hiding this comment.
Xilinx ISE is no longer required by this repo.
LM_LICENCE_FILE - I am not so bothered about this. It is actually no longer needed for the regular PandA builds using recent versions of Vivado, as webpack devices no longer require a licence file. If targeting non-webpack devices or using licenced IP, the licence server can be set in the personal CONFIG file (I have this set in my bash profile). As CI builds in the cloud do not need to know about our local licence server, I would prefer to have no mention of licencing in the public repos at all.
Here is suggestion though, if you prefer:
# Specify path to licence server, if required
# export LM_LICENCE_FILE =
There was a problem hiding this comment.
I think it probably is worth adding just the empty LM_LICENCE_FILE then for those applications which will need a licence.
| -- Input Encoder Address List | ||
| constant INPROT_ADDR_LIST : page_array(ENC_NUM-1 downto 0) := ( | ||
| TO_SVECTOR(INENC4_PROTOCOL, PAGE_AW), | ||
| TO_SVECTOR(INENC3_PROTOCOL, PAGE_AW), | ||
| TO_SVECTOR(INENC2_PROTOCOL, PAGE_AW), | ||
| TO_SVECTOR(INENC1_PROTOCOL, PAGE_AW) | ||
| ); | ||
|
|
||
| -- Output Encoder Address List | ||
| constant OUTPROT_ADDR_LIST : page_array(ENC_NUM-1 downto 0) := ( | ||
| TO_SVECTOR(OUTENC4_PROTOCOL, PAGE_AW), | ||
| TO_SVECTOR(OUTENC3_PROTOCOL, PAGE_AW), | ||
| TO_SVECTOR(OUTENC2_PROTOCOL, PAGE_AW), | ||
| TO_SVECTOR(OUTENC1_PROTOCOL, PAGE_AW) | ||
| ); | ||
|
|
||
| -- TTLIN TERM Address List | ||
| constant TTLTERM_ADDR_LIST : page_array(TTLIN_NUM-1 downto 0) := ( | ||
| TO_SVECTOR(TTLIN6_TERM, PAGE_AW), | ||
| TO_SVECTOR(TTLIN5_TERM, PAGE_AW), | ||
| TO_SVECTOR(TTLIN4_TERM, PAGE_AW), | ||
| TO_SVECTOR(TTLIN3_TERM, PAGE_AW), | ||
| TO_SVECTOR(TTLIN2_TERM, PAGE_AW), | ||
| TO_SVECTOR(TTLIN1_TERM, PAGE_AW) | ||
| ); | ||
|
|
There was a problem hiding this comment.
What has this got to do with moving the slowFPGA build out?
Oh, I see. Tricky ... this list of definitions is also needed by the slow FPGA, as it's very architecture specific and is managed by the slow FPGA. Hmmm.
| -- Input Encoder Address List | ||
| constant INPROT_ADDR_LIST : page_array(ENC_NUM-1 downto 0) := ( | ||
| TO_SVECTOR(INENC4_PROTOCOL, PAGE_AW), | ||
| TO_SVECTOR(INENC3_PROTOCOL, PAGE_AW), | ||
| TO_SVECTOR(INENC2_PROTOCOL, PAGE_AW), | ||
| TO_SVECTOR(INENC1_PROTOCOL, PAGE_AW) | ||
| ); | ||
|
|
||
| -- Output Encoder Address List | ||
| constant OUTPROT_ADDR_LIST : page_array(ENC_NUM-1 downto 0) := ( | ||
| TO_SVECTOR(OUTENC4_PROTOCOL, PAGE_AW), | ||
| TO_SVECTOR(OUTENC3_PROTOCOL, PAGE_AW), | ||
| TO_SVECTOR(OUTENC2_PROTOCOL, PAGE_AW), | ||
| TO_SVECTOR(OUTENC1_PROTOCOL, PAGE_AW) | ||
| ); | ||
|
|
||
| -- TTLIN TERM Address List | ||
| constant TTLTERM_ADDR_LIST : page_array(TTLIN_NUM-1 downto 0) := ( | ||
| TO_SVECTOR(TTLIN6_TERM, PAGE_AW), | ||
| TO_SVECTOR(TTLIN5_TERM, PAGE_AW), | ||
| TO_SVECTOR(TTLIN4_TERM, PAGE_AW), | ||
| TO_SVECTOR(TTLIN3_TERM, PAGE_AW), | ||
| TO_SVECTOR(TTLIN2_TERM, PAGE_AW), | ||
| TO_SVECTOR(TTLIN1_TERM, PAGE_AW) | ||
| ); | ||
|
|
There was a problem hiding this comment.
No, I don't get it. Why have these moved? These are indeed specific to the slow FPGA because it's in control of these items.
There was a problem hiding this comment.
As mentioned in the PR above, 'slow_defines.vhd' is needed by both FPGA repos and needs to be kept in sync between the two. These three constants used to be defined in 'slow_defines' but are used only by the Zynq FPGA, and refer to other constants which are not used or known about by the Slow FPGA. As these three constants are only used in 'system_registers.vhd', the obvious solution was to move them to there.
To make it easier to build the FPGA zpkg with CI actions in github, we decided to try separating the slow-fpga from the Zynq build. Recent versions of Vivado no longer require a licence file or server for webpack parts, whereas Xilinx ISE does. The slow-fpga will be built locally, and as it is changed only infrequently, CI is unnecessary for this.
Please review the following changes in association with the new repo for the Slow FPGA:
https://github.com/PandABlocks/PandABlocks-slowFPGA
There are now several source files that are now duplicated in both repos, mainly support modules found in common/hdl.
'slow_defines.vhd' must be kept in both repos and in sync, as it contains the register mapping for the slow-fpga registers.