Skip to content
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

Merged
merged 1 commit into from Apr 6, 2022
Merged

Lattice: Add support for MachXO2/XO3L internal oscillator #575

merged 1 commit into from Apr 6, 2022

Conversation

jreyesr
Copy link
Contributor

@jreyesr jreyesr commented Jan 13, 2021

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.

@codecov
Copy link

codecov bot commented Jan 13, 2021

Codecov Report

Merging #575 (0a66153) into master (9af8201) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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     
Impacted Files Coverage Δ
nmigen/build/run.py 22.05% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9af8201...0a66153. Read the comment docs.

Copy link
Member

@whitequark whitequark left a 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.

@jreyesr
Copy link
Contributor Author

jreyesr commented Jan 30, 2021

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?

@whitequark
Copy link
Member

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?

No, with this additional information, I believe your approach is correct. I'll take another look soon.

Lunaphied
Lunaphied previously approved these changes Dec 11, 2021
Copy link
Contributor

@Lunaphied Lunaphied left a 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-commenter
Copy link

codecov-commenter commented Dec 11, 2021

Codecov Report

Merging #575 (213a2d6) into main (7c16195) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c16195...213a2d6. Read the comment docs.

@jreyesr
Copy link
Contributor Author

jreyesr commented Dec 11, 2021

LGTM, just needs to be rebased I think.

Done. I just made a brand new commit, since my code was outdated by almost a year.

Copy link
Contributor

@Lunaphied Lunaphied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for rebasing.

Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@whitequark whitequark enabled auto-merge (squash) April 6, 2022 04:12
@whitequark whitequark disabled auto-merge April 6, 2022 04:12
@whitequark whitequark merged commit 9b8354e into amaranth-lang:main Apr 6, 2022
@whitequark
Copy link
Member

Thank you, and apologies for the delay!

@jreyesr
Copy link
Contributor Author

jreyesr commented Apr 6, 2022

Thank you, and apologies for the delay!

Of course, no worries. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants