-
Notifications
You must be signed in to change notification settings - Fork 155
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
Offer safe PLL's #2592
Offer safe PLL's #2592
Conversation
5a36831
to
0bd8474
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.
LGTM
Only some minor details w.r.t. documentation: The Intel docs say things about Xilinx.
Ah, I had neglected to mention that I was still working on the Intel documentation, I simply copied the Xilinx one and still need to adjust those things. |
a8b0b5d
to
1785ba9
Compare
No functional change, except that the `clocks` function is now annotated with a `CLASH_OPAQUE` "pragma" rather than a mere `NOINLINE`. By using TH quotations where possible and doing basically everything in the Q monad instead of mixing it with pure constructors, the code is much easier to grok. Quotations and TH `Name`s don't interact very well. `Clash.Clocks.Deriving` was renamed to `...Internal`, and by moving the `Clocks` class to `Clash.Clocks.Internal`, we can use its names in the quotation in `deriveClocksInstance` without resorting to TH `Name`s. This means `Clash.Clocks` now contains the orphaned instances. This could unfortunately not be fixed with a .hs-boot file because Template Haskell doesn't play nice with that. `Clash.Clocks.Internal` is not exported by `clash-prelude` so users will not accidentally import the `Clocks` class without its instances and be confused. When building the Haddock, only emit instances for up to 3 clocks for the `Clocks` class to reduce the amount of instances under the `Clock` and `Signal` data types.
`altpll` and `alteraPll` did not account for the input domain's reset polarity. By rewriting the template functions in the DSL, we can simply use `DSL.unsafeToActiveHigh` to deal with this. A new function `Clash.Primitives.DSL.declareN` is added. By expressing that in terms of `Clash.Netlist.Id.nextN`, it will in the future benefit from a more efficient implementation when `nextN` is implemented in a more efficient manner.
The wizards in `Clash.Xilinx.ClockGen` now use the user-provided name as the name of the /instance/ rather than the name of the /IP core/. This change was also done for Intel in PR #1022. When the user is responsible for creating the IP core, it makes sense to always set the component name to the user-provided value. But when that is also generated by Clash, that is no longer needed. Allowing users to set the instance name makes it possible to match on the instance in SDC files and such. Instead of always needing a user-specified name for the Intel and Xilinx PLL functions, the instance name can now be set through `Clash.Magic.setName`. To accomodate the changed function arguments, the functions are split into the deprecated old interface and a new interface which marks the use _unsafe_. This is expanded upon in the next commits.
Since your review, I've made the following changes:
|
1785ba9
to
aadb416
Compare
All the examples in the documentation are adjusted to use safe PLL's. Documents all the changes in this PR
aadb416
to
de5e00b
Compare
We have the convention that if a function can lead to timing violations, we prefix the name of that function with
unsafe
. The functions inClash.Intel.ClockGen
andClash.Xilinx.ClockGen
before this PR do not follow that convention. Thelocked
output of these clock generators is an asynchronous signal. All our examples showed the correct way to handle this, but it should be called out by anunsafe
prefix, and we should offer an easy-to-use alternative. It was too easy for a user to forget to synchronise thelocked
signal.This PR deprecates the Intel functions and adds new functions, both a safe variant and, for advanced use cases, an unsafe variant. The safe variant incorporates
resetSynchronizer
so there's just a ready-to-useReset
instead of the asynchronouslocked
signal.For Xilinx, it was decided to not deprecate the functions but rather scrap them altogether and provide new, safe ones with the same name as the old unsafe ones. The functions in Clash 1.6 are so broken that keeping them around as deprecated functions doesn't add much utility for our users; better to bite the bullet. PR #2427 had already changed the API for Xilinx, by the way.
unsafeFromActiveLow
. Now people can follow the recommendation without being exposed tounsafe
stuff.Clash.Magic.setName
if they need a predictable name.ResetPolarity
of the input domain instead of always expecting an active-high signal.Still TODO: