-
Notifications
You must be signed in to change notification settings - Fork 147
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
Fix and document Altera PLL's #2136
Conversation
bfac719
to
9fd204e
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. While we're editing around here though: is it worth referring users to look at the data sheet to see what clocks they can ask for. I assume the speed of clocks you can generate is determined by the board you're using or something? Even if it's obvious to hardware engineers, it feels like something that makes it friendlier to hobbyists etc.
This is what you get for recently doing a documentation PR, I actually thought about the documentation while reviewing 😉 |
9fd204e
to
13a307c
Compare
Okay, you motivated me to write more documentation for this module. How's this? (note: build artifact of CI, will disappear next time CI runs for this PR) |
This looks much better to me, but I think @leonschoorl or @christiaanb should give it a quick skim too in case I've missed something obvious (or worse, something subtle) |
13a307c
to
1880545
Compare
alteraPll
primitive1880545
to
dda0f63
Compare
Commit d325557 introduced an off-by-one error in the number of clocks to generate for 'alteraPll'. Additionally, the internal `locked` signal for `alteraPll` is changed to be of type `Bit` like in `altpll`. The borrowing of the type of the `Reset` input probably came about out of convenience back when resets were still a simple type so it did not matter. Now it looked odd but worked fine nonetheless. Also adjusted the type in `CondAssignment` in `altpll`, which showed the same pattern.
dda0f63
to
9b10ae2
Compare
Commit d325557 introduced an off-by-one error in the number of clocks to generate for `alteraPll`. Additionally, the internal `locked` signal for `alteraPll` is changed to be of type `Bit` like in `altpll`. The borrowing of the type of the `Reset` input probably came about out of convenience back when resets were still a simple type so it did not matter. Now it looked odd but worked fine nonetheless. Also adjusted the type in `CondAssignment` in `altpll`, which showed the same pattern. (cherry picked from commit 9417cb6)
Commit d325557 introduced an off-by-one error in the number of clocks to generate for `alteraPll`. Additionally, the internal `locked` signal for `alteraPll` is changed to be of type `Bit` like in `altpll`. The borrowing of the type of the `Reset` input probably came about out of convenience back when resets were still a simple type so it did not matter. Now it looked odd but worked fine nonetheless. Also adjusted the type in `CondAssignment` in `altpll`, which showed the same pattern. (cherry picked from commit 9417cb6) Co-authored-by: Peter Lebbing <peter@digitalbrains.com>
Commit d325557 introduced an off-by-one error in the number of clocks
to generate for
alteraPll
.Additionally, the internal
locked
signal foralteraPll
is changed tobe of type
Bit
like inaltpll
. The borrowing of the type of theReset
input probably came about out of convenience back when resetswere still a simple type so it did not matter. Now it looked odd but
worked fine nonetheless.
Also adjusted the type in
CondAssignment
inaltpll
, which showed thesame pattern.
Still TODO: