-
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
Lattice: Add support for MachXO2/XO3L internal oscillator #575
Conversation
Codecov Report
@@ Coverage Diff @@
## master #575 +/- ##
=======================================
Coverage 81.50% 81.50%
=======================================
Files 49 49
Lines 6461 6461
Branches 1287 1287
=======================================
Hits 5266 5266
+ Misses 1008 1007 -1
- Partials 187 188 +1
Continue to review full report at Codecov.
|
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.
Our other platforms that support a configurable internal clock source, iCE40, ECP5, and QuickLogic, use a divider to set the frequency. This simplifies the logic, in particular it means that any input configuration exactly (within the limits of the hardware) specifies output frequency. In addition, ECP5 uses the same cell library as MachXO2/XO3L and is a broadly similar family of devices, so they should definitely use the same general approach.
Please change your implemenation to closely follow the logic in the ECP5 platform.
There's a difference between iCE40, ECP5, and QuickLogic devices, on the one hand, and MachXO2, on the other. On MachXO2 devices, the oscillator is configured by explicitly setting the frequency as a Verilog parameter. See pages 35 to 37 here for an example. The allowed frequencies do seem to come from a 266 MHz oscillator with a configurable divider with ratios between 2 to 128 (this is not in the documentation, it simply fits the list provided and you can't set the oscillator frequency by passing a divider). Hovewer, not all ratios are valid. Ratios 2 to 32 are all valid, ratios between 32 to 64 are only valid if divisible by 2, and ratios between 64 and 128 are valid if divisible by 4. Should I still go ahead and use a divider ratio as the configurable parameter, as opposed to the raw frequency that is now being used? If so, there will still be "dead spots" in the input values (i.e., it won't be enough to just "enter a divider between 2 and 128", as in the other families, since, say, 65 would request a frequency of 4.09 MHz, which is not in the list of valid frequencies). Is that acceptable? |
No, with this additional information, I believe your approach is correct. I'll take another look soon. |
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.
LGTM, just needs to be rebased I think.
Codecov Report
@@ Coverage Diff @@
## main #575 +/- ##
=======================================
Coverage 81.53% 81.53%
=======================================
Files 49 49
Lines 6468 6468
Branches 1531 1531
=======================================
Hits 5274 5274
Misses 1003 1003
Partials 191 191 Continue to review full report at Codecov.
|
Done. I just made a brand new commit, since my code was outdated by almost a 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.
Thanks for rebasing.
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.
LGTM
Thank you, and apologies for the delay! |
Of course, no worries. Thank you! |
This PR adds support for the internal high-speed oscillator on MachXO2/XO3L devices, called OSCH. Migen had support for the oscillator, but nMigen doesn't have it yet.
It essentially replicates the changes that #530 made on Intel Cyclone V devices and #338 on the iCE40, with the difference that MachXO2/XO3 devices have a configurable frequency and a list of allowed freqs, as opposed to the Cyclone V which has a fixed frequency with a maximum of 100MHz, and the iCE40 which uses a fixed 48MHz oscillator and a divider.
This PR is accompanied by (and would allow) PR amaranth-lang/amaranth-boards#136 in nmigen-boards, which changes the TinyFPGAAX2Platform to use the internal clock (at 2.08 MHz) by default, since the TinyFPGA AX2 has no external clock and thus no better default alternative for the clock signal.