-
Notifications
You must be signed in to change notification settings - Fork 58
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
Spartan3A Support #138
Spartan3A Support #138
Conversation
Codecov Report
@@ Coverage Diff @@
## master #138 +/- ##
=======================================
Coverage 80.74% 80.74%
=======================================
Files 32 32
Lines 5111 5111
Branches 1106 1106
=======================================
Hits 4127 4127
Misses 848 848
Partials 136 136 Continue to review full report at Codecov.
|
nmigen/vendor/xilinx_spartan_3_6.py
Outdated
{{name}}_par.ncd | ||
{{name}}.bit | ||
""" | ||
] | ||
|
||
@property | ||
def family(self): | ||
if self.device.startswith("xc3s"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should normalize the case with something like device = self.device.upcase()
.
@@ -76,6 +81,10 @@ class XilinxSpartan6Platform(TemplatedPlatform): | |||
-ifn {{name}}.prj | |||
-ofn {{name}}.ngc | |||
-top {{name}} | |||
{% if platform.family in ["3", "3E", "3A"] %} | |||
-use_new_parser yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this switch does for Spartan 6? Should we not always use it? Is that a part of ISE or just its Spartan support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was under the impression that you don't add any options you don't have to the templates (e.g. that are default for the family). E.g. while -use_new_parser yes
is the default for S6 already (and thus harmless to put on the command line), it is not the default for S3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, that's fine then.
nmigen/vendor/xilinx_spartan_3_6.py
Outdated
@@ -126,12 +135,28 @@ class XilinxSpartan6Platform(TemplatedPlatform): | |||
""", | |||
r""" | |||
{{get_tool("bitgen")}} | |||
{{get_override("bitgen_opts")|default(["-w"])|options}} | |||
{{get_override("bitgen_opts")|default(["-g Binary:Yes -w"])|options}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a defaulted option? This means that anyone who provides NMIGEN_bitgen_opts
for a board that requires bitstream.bin
will break the programmer for that board unless they specifically know the default. That seems bad. What are the drawbacks of -g Binary:Yes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the drawbacks of -g Binary:Yes
It generates bitstream.bin
in addition to bitstream.bit
. I don't see any drawbacks besides the one you mentioned.
In my case, Mercury needs -g Binary:Yes
unconditionally- the programmer for that board cannot handle bitstream.bit
. I suppose I could do an override of command_templates
at the platform level to add in the -g Binary:Yes
option and change the defaults back to the existing behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just generate bitstream.bin
on every board? Unless it's extremely slow or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just generate bitstream.bin on every board? Unless it's extremely slow or something.
That is exactly what -g Binary:Yes
already does. The issue, AIUI, is an override would disable this option unless the user already knew that the option -g Binary:Yes
is required to get .bin
files from bitgen
, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. If boards rely on it it shouldn't be an override. It should either be a per-board switch based on a property, or always enabled. If it's not too slow, always enabled is preferable.
As per discussion on IRC, Spartan3(A) support for trailing-edge-but-still-made boards is a minimally invasive addition to the already-existing Spartan 6 support, so I added it for my own purposes :).
In addition to changing the module name to
xilinx_spartan_3_6
, I added two other features I feel usefulfor ISE that were in
omigen
but didn't make their way tonmigen
:bitgen -g Binary:Yes
by default to get a raw binary bitstream usable beyond Xilinx tools.xst -register_balancing yes
. This is off by default in both Spartan6 and Spartan 3A, per the XST User Guides.-ifmt MIXED
is also passed toxst
inomigen
, but this option is ignored in ISE 14.7 and is assumed default.