YosysHQ / nextpnr

nextpnr portable FPGA place and route tool
ISC License
1.25k stars 237 forks source link

An option to reject inputs where some of the clock nets have no defined or derived constraints #1335

Open whitequark opened 6 days ago

whitequark commented 6 days ago

Due to a combination of factors, I've realized recently that I've been building Glasgow bitstreams with the default 12 MHz constraint, which of course did not work at 80 MHz post-PLL.

We have --pcf-allow-unconstrained, I propose --clk-allow-unconstrained and similarly switching the default (or at the very least, --clk-deny-unconstrained to be available as an option).

rowanG077 commented 6 days ago

I would rather propose making an unconstrained clock a warning. And then you can enable warning as error.

whitequark commented 6 days ago

I would rather propose making an unconstrained clock a warning.

When would you ever want an unconstrained clock in a real design?

Unconstrained pins are reasonably common, like if you haven't done PCB layout yet. Unconstrained clocks essentially result in garbage-in, garbage-out, unless your clock is 12 MHz or less, which is fairly uncommon in amaranth-boards at least (which I think is representative of what people use).

Also, I'm not sure if the default 12 MHz frequency of unconstrained clocks propagates through PLLs, which can be even worse.

rowanG077 commented 6 days ago

I agree that you never want an unconstrained clock in a real design. But breaking peoples builds for relying on something that is bad practice but works is kinda heavy handed. A warning on the short term, maybe with a notice the warning will be upgraded to an error in the future, will at least give people time to move.

whitequark commented 6 days ago

A warning on the short term, maybe with a notice the warning will be upgraded to an error in the future will at least give people time to move.

That's fine by me. What I want to avoid is any situation where a substring is matched in the warning to elevate it to an error, like Yosys does. It should be a dedicated option.