-
Notifications
You must be signed in to change notification settings - Fork 175
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
Initial support for gowin vendor via proprietary and apicula toolchain. #734
Conversation
Does the Apicula toolchain not support IOB registers at all currently? |
Unclear. The blinky did not want them. I can check how far it goes. @pepijndevos certainly knows more. Also, the fuzzer code mentions IOBUF. |
I will merge the PR once all of the features of the Apicula toolchain are supported, similar to Lattice, Xilinx, etc ones. |
Codecov Report
@@ Coverage Diff @@
## main #734 +/- ##
=======================================
Coverage 82.74% 82.74%
=======================================
Files 52 52
Lines 7089 7089
Branches 1705 1705
=======================================
Hits 5866 5866
+ Misses 1025 1024 -1
- Partials 198 199 +1 see 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Exciting stuff. Looks like the IOB question has been resolved right? |
@pepijndevos thanks! What else is supported in apicula? Is there a list of supported and tested primitives somewhere? |
Tested you say? Bwaha |
Ok, so DDR is something I could at least add and see if it compiles. |
I've rebased and added support for xdr>0 from https://github.com/tcjie/Gowin repo. |
@pepijndevos if you have time, could you perhaps have a look here? Is anything still missing? Testing is still pending for the differential IO, since I have limited means to do this. |
@bl0x The answer is yes. |
Eh, not too familiar with amaranth but I guess it basically looks fine. What do you need for testing differential IO? IIRC we basically run this example as a sanity check https://github.com/YosysHQ/apicula/blob/master/examples/blinky-oddr.v |
@tcjie I've used some code from your repository https://github.com/tcjie/Gowin to add support for the proprietary gowin toolchain. |
Support for on chip oscillators is now also in. |
Here is the result with
|
Exact same result as above with the Gowin toolchain, after doing this:
from amaranth_boards.tang_nano_9k import *
from amaranth_boards.test.blinky import *
TangNano9kPlatform(toolchain="Gowin").build(Blinky(), do_program=True)
|
@bl0x that's very useful |
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.
Thank you for your contribution! I do not see any technical issues with this PR. I'm going to test it locally and once everything passes, merge 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.
On second look I was a bit too enthusiastic earlier, sorry about that!
Therefore, it is probably best (now and for the future) to use these two as input and check that they are somwhat sane and at least agree with each other. To make a parts list in python use this gist. |
@whitequark I'm happier now with the PR =) |
@bl0x Thanks so much, you're a hero! I did one last round of small changes, mostly focused on making the regexp use a little more idiomatic, though I think Could you check that my edits are correct? Then I'm happy to merge this at last! |
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.
Really happy with this PR!
a688600
to
89960fd
Compare
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.
All fine with me! Thanks!
It's good to go then?
@bl0x Yep! I'm going to squash all the commits in this PR and then merge. |
89960fd
to
78ce3ad
Compare
Did it blink before at all? You've showed some video demo before. Was that the same device? |
I've never used the built-in oscillator before since it was kind of awkward. I decided to try it out now that the support for it seems to be done properly. |
I'll try and check with tang nano 9k later. |
Using "OSC" and 25 MHz with tang nano 9k the 6 LEDs flash. With 12.5 MHz the flash speed increases (by factor 2?) |
This means you need to pass
This seems wrong; the LEDs are supposed to always flash with 1 Hz period. |
This looks like an upstream bug: YosysHQ/apicula#193 @bl0x I'll fix the filename overwrite issue. Could you take a look at why blinky doesn't get the right frequency? |
Co-authored-by: Catherine <[email protected]>
512d8bf
to
efc0860
Compare
As blinky works on both 1k and 9k, I'm merging this 🎉 |
@whitequark nice work! |
Thank you! |
self.package = match.group(5) | ||
self.speed = match.group(6) | ||
|
||
match = re.match(reg_series+reg_size+reg_subseries+"$", self.family) |
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.
Hold on. This matches series, size, and subseries, all of which are also matched above. So what information does family
actually add?
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.
It's... Complicated. Iirc device names don't actually uniquely define the actual device. I think I have an email from their support that's supposed to explain it but honestly it just doesn't make any sense.
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.
Have you looked at the code 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.
So this suggests there could be a "series" in the part name that is not the same as the "series_f" in the part family?
In the datasheet, "Family" seems to be LittleBee or Arora, and "Series" seems to be names like "GW1N"
For convenience:
http://cdn.gowinsemi.com.cn/DS100E.pdf#page=85
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.
It might be related to revisions of the chips, as shown on the package marking, as opposed to the datasheets:
So while a full part order name might be GW1N-LV9 MG100C6/I5
the device family name as shown in GOWIN EDA software tool might be different according to... the extra chip marking information "xxxxCA0N" also shown here: GW1N-9C
Many thanks to @siliconwitch who found out about it.
And as you can see, this would also have a different Device ID.
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.
@josuah That is exactly why I'm doing the double-matching and why one also has to supply family
to uniquely identify the part.
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.
Just take a look at the contents of <path-to-gowin-IDE>/share/device/device_info.csv
.
Run this:
awk -F"," '{print $2}' device_info.csv | sort | uniq -c
and you'll see that the part name in column 2 is not unique enough.
You need in addition the family name in column 4. The below command returns 0 lines:
awk -F"," '{print $2, $4}' device_info.csv | sort | uniq -c | awk '$1!=1' | wc -l
while the not taking into account the family name gives >100 non-unique parts:
awk -F"," '{print $2}' device_info.csv | sort | uniq -c | awk '$1!=1' | wc -l
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.
And the device family lacks the package information used for the pin mapping.
I suppose they would have preferred to avoid this split themselves.
Maybe a difference in the process with the same masks.
Maybe a fix that lead to more consequences than intended.
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.
@bl0x But the RE that matches family
is a subset of the RE that matches part
... what am I missing?
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.
@whitequark Nothing! Because part can also contain all information (and oftentimes does), but not always. We could use the same RE to also match family
, but that would be too general.
Supported devices: gw1n 9c / tang nano 9k