-
Notifications
You must be signed in to change notification settings - Fork 1
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
Clash 1.9 update #444
Clash 1.9 update #444
Conversation
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.
I think the difference between an improvised reset synchroniser with xpmCdcSingle
and the bog-standard resetSynchronizer
that is part of the safe PLL functions is that the former includes a max_delay
constraint and an ASYNC_REG
attribute whereas the latter includes none of those things. So I think the improvisation is qualitatively better...
@@ -14,14 +14,17 @@ import Bittide.Arithmetic.Ppm | |||
import Data.Proxy | |||
|
|||
createDomain vXilinxSystem{vName="Basic125", vPeriod= hzToPeriod 125e6} | |||
createDomain vXilinxSystem{vName="Ext125", vPeriod= hzToPeriod 125e6, vResetKind=Asynchronous} |
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.
This is more a matter of preference, but
createDomain vXilinxSystem{vName="Ext125", vPeriod= hzToPeriod 125e6, vResetKind=Asynchronous}
is fully identical to
createDomain vSystem{vName="Ext125", vPeriod= hzToPeriod 125e6}
(or vIntelSystem
instead of vSystem
for maximum confusion ;-P)
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 why the different records?
"SYSCLK_300" ::: DiffClock Basic300 -> | ||
"SYSCLK_300" ::: DiffClock Ext300 -> | ||
"SYNC_IN" ::: Signal Basic300 Bool -> | ||
"SYNC_OUT" ::: Signal Basic300 Bool |
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.
Oof, good catch. This used to declare that the input and output clocks of the clock wizard were 100% identical, which means you could have just piped a signal synchronous to the input clock to the domain of the output clock or vice versa. But they are obviously not identical, so this might introduce metastability.
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.
(I have to admit the odds of you doing this accidentally are pretty low. But usually the type system prevents it and I really think you should not lie about equality of clocks even if the odds of it going wrong are low.)
clash-lang/clash-compiler#2624 should allow clockControlDemo0 to build again |
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.
@kleinreact Could you check whether @leonschoorl's patch works?
Negative 😞. @leonschoorl: Is that new error related to your fixes? |
The output is confusing: it prints a warning (which we should be able to ignore if it doesn't come back later as an error). The real error is:
Do we connect dc fifo to an async reset? Because that would be an issue yeah... |
The only related change is the update of the external clock domains to use asynchronous resets, which is required for the safe variants of clock wizards and looks to me like the right way of doing it? So yes, if that's correct then we are connecting the dc fifo to an asynchronous reset somewhere in that instance. |
But it still means we are using a "free reset" with dcFifo, which is unintended behavior, right? So the error indication points to a true error then? |
Yes, it is unintended behavior. The reset should be synchronized. The error just crops up for the "wrong" reasons. |
541ed68
to
1cc33aa
Compare
This PR bumps Clash to the latest required 1.9 version (fixing the
xpmCdcHandshake
primitive) and applies the corresponding interface changes. In particular, this includesclockWizardDifferential
getting rid of the improvised reset syncers,BasicX
toExtX
for all clocks that originate from a differential input, andExtX
domains toAsynchronous
resets.Note that
clockControlDemo0
currently does not compile due to a Clash bug introduced since the previously used version, which is under investigation right now.