-
Notifications
You must be signed in to change notification settings - Fork 176
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
Added gatemate vendor and Updated init file #1460
base: main
Are you sure you want to change the base?
Conversation
amaranth/vendor/_gatemate.py
Outdated
""", | ||
"{{name}}.sdc": r""" | ||
# {{autogenerated}} | ||
{% for net_signal, port_signal, frequency in platform.iter_clock_constraints() -%} |
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.
There is no iter_clock_constraints
function in the main
branch (you can grep it with zero hits), how did you test sdc
generation functionality?
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 didn't test the .sdc function, to be honest. I based this part of the on the _quicklogic.py
vendor.
What do I need to do here?
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.
You need to make sure that a clock constraint, as specified in your board file at least, is actually applied. Whether a clock constraint is applied or not can be seen in the report of the PNR tool, in a format described in the vendor's documentation.
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.
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.
That has a clock constraint file in the top right, as I would expect.
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 think that is their pin constraint file, the .ccf
file?
amaranth/vendor/_gatemate.py
Outdated
if not hasattr(self, "osc_div"): | ||
raise ValueError("OSC divider (osc_div) must be an integer between 2 and 512") | ||
if not isinstance(self.osc_div, int) or self.osc_div < 2 or self.osc_div > 512: | ||
raise ValueError("OSC divider (osc_div) must be an integer between 2 and 512, not {!r}".format(self.osc_div)) | ||
if not hasattr(self, "osc_freq"): | ||
raise ValueError("OSC frequency (osc_freq) must be an integer between 2100000 and 80000000") | ||
if not isinstance(self.osc_freq, int) or self.osc_freq < 2100000 or self.osc_freq > 80000000: | ||
raise ValueError("OSC frequency (osc_freq) must be an integer between 2100000 and 80000000, not {!r}".format(self.osc_freq)) | ||
clk_i = Signal() |
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 doesn't look right--you aren't using osc_div
and osc_freq
anywhere in the instantiation and qlal4s3b_cell_macro
is a QuickLogic primitive. Do GateMate FPGAs have any internal oscillator at all, like RC oscillator or ring oscillator? If not then the create_missing_domain
function should only use an async reset synchronizer, or possibly just be absent.
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.
From what I have read on their documentation for the CCGM1A1 Chip, there are no oscillators(ring or RC) on the chip.
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.
Then you don't need any implementation of create_missing_domain
or default_clk_constraint
at all.
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.
Removed create_missing_domain
and default_clk_constraint
.
amaranth/vendor/_gatemate.py
Outdated
* ``yosys`` | ||
* ``p_r`` | ||
|
||
The environment is populated by setting the ``CC_TOOL`` environment variable to point to the toolchain directory. |
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 don't see how this would happen as nothing in this file touches CC_TOOL
. There is a default behavior that will use the platform.toolchain
property (defined here to be GateMate
) to derive AMARANTH_ENV_GATEMATE
from it.
If you wanted to use CC_TOOL
to populale the environment you have to write a custom template for the shell and batch scripts, which is possible but finicky and requires extensive testing and especially careful coding.
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.
Changed CC_TOOL
to AMARANTH_ENV_GATEMATE
What is "messy"? In general, the generated Verilog is intended to be human-readable (since you'll eventually have to read it, even if it is very rare with Amaranth in practice) but not beautiful (since you aren't going to be reading it the majority of the time). But I can point you to the difference between ULX3S and GateMate platforms. |
Two linebreaks between top-level items: Co-authored-by: Catherine <[email protected]>
Added verbose("-v") instead of -v for p_r tool Co-authored-by: Catherine <[email protected]>
This is the GateMate generated Verilog file
While this is for ULX3S:
The GateMate one is much longer and makes 7 modules, while the ULX3S is shorter and makes only 1. Could you explain further? |
These two files are not comparable. Your ULX3S Verilog file doesn't use an Amaranth platform, and if it did they'd look fairly similar. Amaranth extensively uses modules for code reuse. Some of the modules above are trivial because the GateMate platform doesn't customize I/O primitives (as do most other FPGA platforms, even the relatively simple SiliconBlue one); Amaranth typically uses a module per I/O pin group, which in turn can use a module per pin, etc. The reset synchronizer has similar properties. These modules get folded during syntesis and there's no downside to having them besides perhaps aesthetics. |
amaranth/vendor/_gatemate.py
Outdated
{% for file in platform.iter_files(".v") -%} | ||
read -sv {{get_override("read_verilog_opts")|options}} {{file}} | ||
{% endfor %} |
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 isn't right: you should not be reading .v
files in SystemVerilog mode. You should copy this part from e.g. SiliconBlue platform.
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.
After reading all user-provided files, you should read the top-level .il
file you should write above. This can again be copied from SiliconBlue essentially as-is.
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.
The .ys script is now:
"{{name}}.ys": r"""
# {{autogenerated}}
{% for file in platform.iter_files(".v") -%}
read_verilog {{get_override("read_verilog_opts")|options}} {{file}}
{% endfor %}
{% for file in platform.iter_files(".sv") -%}
read_verilog -sv {{get_override("read_verilog_opts")|options}} {{file}}
{% endfor %}
{% for file in platform.iter_files(".il") -%}
read_ilang {{file}}
{% endfor %}
read_ilang {{name}}.il
{{get_override("script_after_read")|default("# (script_after_read placeholder)")}}
synth_gatemate {{get_override("synth_opts")|options}} -top {{name}} -vlog {{name}}_synth.v
{{get_override("script_after_synth")|default("# (script_after_synth placeholder)")}}
""",
In the documentation provided here:
There is an option to write json, but is supported for a future release of the p_r
tool with nextpnr
that is planned next year.
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.
The command_template
for yosys is the same for the one in SilliconBlue:
r"""
{{invoke_tool("yosys")}}
{{quiet("-q")}}
{{get_override("yosys_opts")|options}}
-l {{name}}.rpt
{{name}}.ys
""",
But running the code for my Blinky example I get this:
python3 Blinky.py -b -p amaranth_boards.gatemate_a1_evb.GateMate_A1_EVB
ERROR: Module `top' not found!
Traceback (most recent call last):
File "/home/user/FPGA/tools/amaranth/examples/gatemate/Blinky.py", line 125, in <module>
plat.build(Blinky(num_leds, clock_divider), do_program=do_program)
File "/home/user/FPGA/tools/amaranth/amaranth/build/plat.py", line 103, in build
products = plan.execute_local(build_dir)
File "/home/user/FPGA/tools/amaranth/amaranth/build/run.py", line 118, in execute_local
subprocess.check_call(["sh", f"{self.script}.sh"],
File "/usr/lib/python3.10/subprocess.py", line 369, in check_call
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sh', 'build_top.sh']' returned non-zero exit status 1.
The top.ys
script is given below:
# Automatically generated by Amaranth 0.6.0.dev13. Do not edit.
# (script_after_read placeholder)
synth_gatemate -top top -vlog top_synth.v
# (script_after_synth placeholder)
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, at the moment -vlog
is the correct one.
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.
But running the code for my Blinky example I get this:
That is odd. If you push your changes I can look into it.
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 pushed the changes.
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 added my Blinky.py
example aswell in examples
for now so you can see.
Will delete later.
The command is
python3 Blinky.py -b -p amaranth_boards.gatemate_a1_evb.GateMate_A1_EVB
amaranth/vendor/_gatemate.py
Outdated
**TemplatedPlatform.build_script_templates, | ||
"{{name}}.v": r""" | ||
/* {{autogenerated}} */ | ||
{{emit_verilog()}} |
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.
For a Yosys-based platform, you should output a .il
file, not a .v
one.
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.
Added
"{{name}}.il": r"""
# {{autogenerated}}
{{emit_rtlil()}}
""",
Instead of
"{{name}}.v": r"""
/* {{autogenerated}} */
{{emit_verilog()}}
""",
amaranth/vendor/_gatemate.py
Outdated
-i {{name}}_synth.v | ||
-o {{name}} | ||
-ccf {{name}}.ccf | ||
-cCP > log/impl.log |
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.
There is no log directory any more. The redirection should probably be something like this:
-cCP > log/impl.log | |
-cCP | |
> {{name}}.tim |
The .rpt
/.tim
extension is something we've been using for Yosys based platforms and isn't set in stone, but it's nice to have consistency.
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.
Changed to
r"""
{{invoke_tool("p_r")}}
{{verbose("-v")}}
-i {{name}}_synth.v
-o {{name}}
-ccf {{name}}.ccf
-cCP
> {{name}}.tim
""",
In order for this PR to be complete and mergeable I expect the following items to be done:
Once all of the above is done the platform will be merged. I will also consider backporting it into the 0.5.x release series so that it can be used more early, on the stable branch. We can then review the amaranth-boards file in more detail, as it's time-consuming but doesn't need to block the platform acceptance. |
I just realized that the development board you are using and the board that I have aren't the same board. Since I need to be able to test ongoing development I guess I'll have to find time to write a board file for the one CologneChip sent me, or you could also contribute a second one to make that faster. |
You have the GateMate Evaluation Board from CologneChip? |
I have "GateMate FPGA Starter Kit" which is a blue board with a different layout than what you posted. |
My friend has that board at home with him, so I can also ask him for help with this atleast via testing by uploading. |
Sounds good to me. For now you testing on your own board is OK (managing review of two boards is a lot more difficult than one), I'll hack together a small board file with just a LED or something to test it myself. |
I have reviewed the CologneChip Primitive Library just now. In addition to the list above, the following primitives will need to be appropriately integrated into the platform: For the I/O buffers, the Lattice platform is quite similar and can be used as a template. For the reset generator I will probably implement it myself. This work should be done after the rest is finished. It's fairly finicky so I may end up doing it myself, but I will explain what needs to be done either way. |
If you put the following contents into a file export PATH=.../cc-toolchain-linux/bin/openFPGALoader:.../cc-toolchain-linux/bin/p_r:.../cc-toolchain-linux/bin/yosys:${PATH} and then put |
Added GateMate vendor
@TarikHamedovic After removing |
amaranth/vendor/_gatemate.py
Outdated
device = property(abstractmethod(lambda: None)) | ||
package = property(abstractmethod(lambda: None)) |
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 just realized that nothing in the platform file is using these variables. In that case they should be removed.
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.
Deleted those lines
I confirm that that was the problem. In the I tested it out on the board aswell and it blinks. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1460 +/- ##
==========================================
- Coverage 91.12% 91.11% -0.02%
==========================================
Files 43 43
Lines 10990 11150 +160
Branches 2665 2715 +50
==========================================
+ Hits 10015 10159 +144
- Misses 805 821 +16
Partials 170 170 ☔ View full report in Codecov by Sentry. |
A couple of questions to keep the pull request going. I was busy with other projects the last two weeks, but I'll try my best to finish this pull request completely, but I am going to need some guidance. Looking at the tasks said above, the following questions are:
How do I actually override commands? Where do I use it, if that makes sense. I have seen the documentation of other platforms and they have
When I do the reports and available overrides in the docstring, I should place them in the sphinx documentation akin to the quicklogic example?
Any guidance on the tasks above? I also added the GateMate Eval Board you are using to my fork of amaranth-boards where I only placed the clock, led and button. It should work, I'll test it out with my colleague tonight. Also trying it on windows gives me the following message:
|
You can use
All of the overrides I expect this platform will need are already in the file.
It is better to use the Lattice platform as an example as it's more complete (and I'm not really sure to be honest if anyone is even using QuickLogic). The format is the same, but Lattice has more of them, which might help.
The Lattice platform has examples for all of these tasks. At the time I don't have time available for a more detailed guidance.
I can't immediately see what's wrong with it. You should probably look into the Yosys log. |
Hello,
Following our discussion on the amaranth-lang/amaranth repository, we reached out to CologneChip and requested a modification to their constraint files. Specifically, they made it possible to
Net
instead ofPin_in
,Pin_out
, andPin_inout
, enabling the direction of the pin to be determined from the RTL file rather than explicitly declared in the constraint file.CologneChip implemented this change, and I tested it on the Olimex GateMateA1-EVB Board using a simple Blinky example. The new setup works fine now.
I added a
_gatemate.py
file to theamaranth/vendor
directory. This file leveragesyosys
and thep_r
tool, which can be downloaded as a pre-built binary from CologneChip's official site.For programming, I used
openFPGALoader
. I included this part in the amaranth-boards definition for the board, as I noticed that most other boards follow this approach.I just wanted to ask aswell, maybe I can open up a issue aswell. The generated Verilog file is pretty messy.
I have a ULX3S board and the generated Verilog file for the same Blinky example generates much cleaner code.
What can be the issue here? And can it be fixed?