Add dynamic clock periods

[martijnbastiaan]

Still TODO:

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

[DigitalBrains1]

This doesn't contain the halfway option yet, where you have a static clock but specify explicitly the time of the first active edge (which enables you to adjust the phase of the clock without all the extra processing true dynamic clocks incur). Was this a conscious decision or oversight? I'm fine with the former :-).

[edit] I'd suggest always having a time for the first active edge, for both static and dynamic clocks. And have it default to 0 (which would mean an edge at ~LONGESTPERIOD). [/edit]

[martijnbastiaan]

Was this a conscious decision

Somewhat. I wanted to get the absolute minimum in so we can use it in the project that shall not be named. However, I also think it's largely unrelated to this PR as I'd like to make that change on another level, as per @rowanG077's suggestion a while ago: https://github.com/clash-lang/clash-compiler/issues/1215.

[DigitalBrains1]

Yeah, I mainly thought that as you were fiddling with periods anyway you might as well add phase. "On another level": that's also a valid criticism of this PR, all the information used to be in DomainConfiguration, now it's partly in Clock and partly in DomainConfiguration. Eventually we might want to move it all into Clock perhaps.

If we do implement something like #1215, we should think a bit about the unit we express it in. An integral amount of degrees might not be the best option. It seems Vivado only lets you specify it in degrees, and Quartus allows you to specify it in degrees, nanoseconds or picoseconds (neither force an integral amount, mind you). Of course, if Clash also generates the instantiation of the PLL, it doesn't matter much what unit they choose, but it's instructive to see what others have done with the same subject.

[martijnbastiaan]

Hmmyeah, I think the proper way to do this is to change _period to Maybe Nat, instead of just Nat. However, this would also break the current API for a feature that, approximately, 0% of Clash users are interested in :-). So I think we should evaluate it this way - if it proves useful for a larger audience we can make it break the public API. I'm therefore hopeful that this PR gets accepted as is - its general approach anyway.

Eventually we might want to move it all into Clock perhaps.

I think this gets you into trouble with initBehavior and resetPolarity. At some point we did encode it on a clock/reset level, but that turned out to be quite painful after we started adding properties.

If we do implement something like https://github.com/clash-lang/clash-compiler/issues/1215, we should think a bit about the unit we express it in. An integral amount of degrees might not be the best option.

Why not as picoseconds†, like period?

† should be refactored into femtoseconds at some point

[DigitalBrains1]

So I think we should evaluate it this way - if it proves useful for a larger audience we can make it break the public API. I'm therefore hopeful that this PR gets accepted as is - its general approach anyway.

Sounds good.

Eventually we might want to move it all into Clock perhaps.

I think this gets you into trouble with initBehavior and resetPolarity. At some point we did encode it on a clock/reset level, but that turned out to be quite painful after we started adding properties.

I didn't define "it all" ;-). You could also make just the period and phase information something encoded in Clock. Even the active edge definition seems more a property of a domain, since it affects a lot of generated HDL. It's more that a clock also needs to obey the active edge than that it is a property of the clock signal.

Anyway, we are getting a bit sidetracked thanks to my comments. I don't think it is going to affect this PR.

Why not as picoseconds†, like period?

Sounds fine to me. I can't properly pin it down, but it feels more appropriate to me than an angular measure like degrees or radians. However, it is much more common to express phase in an angular measure, so I feel like I should consider that more appropriate? :-) The main difference is the scaling I suppose, degrees or radians scale with the frequency. With degrees, you are always dividing one period into 360 equally sized units. With picoseconds, you're dividing a 500 MHz clock into 2000 equally sized units, but a 50 MHz clock into 20,000 equally sized units. So in a sense you lose resolution as frequency increases. In another sense, you do not: it's still a picosecond.

Again, sidetracked.

[DigitalBrains1]

I removed the superfluous NumericUnderscores. Furthermore, I noticed that an earlier commit still exported the Clock constructor, but then a later commit removed it again. I also moved that in the right place. However, because I accidentally rebased on a newer master, the Compare links are unhelpful. Here you can actually compare my changes:

https://github.com/clash-lang/clash-compiler/compare/7b28ffa..a4c309f5c2bee9bf89d55b1e77abc42be8672938

[martijnbastiaan]

@mergify rebase

[mergify[bot]]

rebase

✅ Branch has been successfully rebased

[martijnbastiaan]

@mergify rebase

[mergify[bot]]

rebase

✅ Branch has been successfully rebased

[martijnbastiaan]

@mergify queue

[mergify[bot]]

queue

🟠 The pull request is the 2nd in the queue to be merged

#2295 is queued for merge.

Required conditions of queue default for merge:

The following pull requests are queued:

	Pull request	Queue/Priority	Speculative checks	Queued
1	Add missing changelog entries (#2298)	default/medium	in place	2022-08-16 10:00 UTC
2	Add dynamic clock periods (#2295)	default/medium		2022-08-16 10:01 UTC

---

More informations about Mergify merge queue can be found in the documentation.

Mergify commands

You can also trigger Mergify actions by commenting on this pull request:

• @Mergifyio refresh will re-evaluate the queue rules

Additionally, on Mergify dashboard you can:

• look at your merge queues
• generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com

[mergify[bot]]

queue default squash

🛑 The pull request has been removed from the queue

The queue conditions cannot be satisfied due to failing checks or checks timeout. You can take a look at Queue: Embarked in merge train check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI. Then, re-embark the pull request into the merge queue by posting the comment @mergifyio refresh on the pull request.

[mergify[bot]]

queue

🛑 The pull request has been removed from the queue

The queue conditions cannot be satisfied due to failing checks or checks timeout. You can take a look at Queue: Embarked in merge train check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI. Then, re-embark the pull request into the merge queue by posting the comment @mergifyio refresh on the pull request.

[martijnbastiaan]

@mergify rebase

[mergify[bot]]

rebase

✅ Branch has been successfully rebased

[martijnbastiaan]

@mergify rebase

[mergify[bot]]

rebase

✅ Branch has been successfully rebased