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

Offer safe PLL's #2592

Merged
merged 7 commits into from Oct 25, 2023
Merged

Offer safe PLL's #2592

merged 7 commits into from Oct 25, 2023

Conversation

DigitalBrains1
Copy link
Member

We have the convention that if a function can lead to timing violations, we prefix the name of that function with unsafe. The functions in Clash.Intel.ClockGen and Clash.Xilinx.ClockGen before this PR do not follow that convention. The locked 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 an unsafe prefix, and we should offer an easy-to-use alternative. It was too easy for a user to forget to synchronise the locked 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-use Reset instead of the asynchronous locked 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.

  • All documentation and examples now shows the safe variants where that makes sense. Since we recommend people always use a PLL, in practice that meant we recommended people to use functions like unsafeFromActiveLow. Now people can follow the recommendation without being exposed to unsafe stuff.
  • All clock generators support multiple output clocks.
  • It is no longer required to give a clock generator a name. Instead, people can set the name of the instance with Clash.Magic.setName if they need a predictable name.
  • Intel PLL's now account for the ResetPolarity of the input domain instead of always expecting an active-high signal.

Still TODO:

  • Write a changelog entry (see changelog/README.md)
  • Check copyright notices are up to date in edited files

@DigitalBrains1 DigitalBrains1 added this to the 1.8 milestone Oct 20, 2023
Copy link
Member

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

clash-prelude/src/Clash/Intel/ClockGen.hs Outdated Show resolved Hide resolved
clash-prelude/src/Clash/Intel/ClockGen.hs Outdated Show resolved Hide resolved
clash-prelude/src/Clash/Intel/ClockGen.hs Outdated Show resolved Hide resolved
@DigitalBrains1
Copy link
Member Author

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.

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.
@DigitalBrains1
Copy link
Member Author

Since your review, I've made the following changes:

  • Finished documentation for Clash.Intel.ClockGen

  • For both Xilinx and Intel: switched the example to use the implicit Prelude as I think this is more familiar to a novice

  • Again for both: added an example of the use of the unsafe functions

  • Adjusted the commit message for the Improve readability of Clash.Clocks.Deriving commit, from No functional change to:

    No functional change, except that the clocks function is now annotated with a CLASH_OPAQUE "pragma" rather than a mere NOINLINE.

    We discovered this had been overlooked for PR Warn against non-opaque primitive functions on GHC >= 9.4 #2511, but since this PR fundamentally changed the code, we decided to adjust it here.

clash-prelude/src/Clash/Intel/ClockGen.hs Outdated Show resolved Hide resolved
clash-prelude/src/Clash/Xilinx/ClockGen.hs Outdated Show resolved Hide resolved
clash-prelude/src/Clash/Intel/ClockGen.hs Outdated Show resolved Hide resolved
All the examples in the documentation are adjusted to use safe PLL's.

Documents all the changes in this PR
@DigitalBrains1 DigitalBrains1 merged commit 428fa24 into master Oct 25, 2023
13 checks passed
@DigitalBrains1 DigitalBrains1 deleted the safe_pll branch October 25, 2023 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants