pulp-platform / common_cells

Common SystemVerilog components
Other
488 stars 138 forks source link

Clk int div static #183

Closed meggiman closed 1 year ago

meggiman commented 1 year ago

This PR adds a convenience wrapper for the clk_int_div module that ties of the inputs to provide a static clock divider with a fixed division value. The PR also contains a TB for this new module that verifies the clock divider for various different parametrizations.

meggiman commented 1 year ago

This is a valid input. The only issue I could see with this approach is, that there might be IPs that rely on the faulty duty cycle behavior of the old clk_div as a feature. What do you think @bluewww?

bluewww commented 1 year ago

The "problem" with the clk_div module is that it has a skewed duty cycle since it basically lets through one short clock pulse for every RATIO clock ticks. I want to mention here that the idea to have one clock pulse being propagated every N cycles is totally viable (and being done in real-life so it can't really be called faulty).

Unfortunately, clk_div does this it the wrong way. The correct way to do this would be to use a down counter and a latch-based clock gate. Some people refer to that design as "punch through divider". This divider design is advantageous if you want to do at-speed testing (in DFT).

Now whether it is a good idea to replace this skewed (and buggy divider withouth my patch I believe) with a balanced divider I don't know. It might be better to implement a punch through divider and replace it with that.

I have been using a patched version of clk_div but I will switch to clk_int_div myself.

meggiman commented 1 year ago

Thanks for your feedback @bluewww. In that case I propose we patch the deprecated clk_div module to at least avoid glitches but keep the naming scheme as is. Would that be fine with you @niwis?

niwis commented 1 year ago

I honestly would prefer to replace clk_div with this module and un-deprecate it. I don't think the skewed duty cycle or the exponential divisor are intended features of clk_div or that anyone relies on them (they aren't even documented). And its implementation has more flaws, so I think this would be the safest approach. Of course, we could still add a module clk_div_skew (or similar) that would skew the clock explicitly.

But I don't want to stall this for much longer. If both of you agree that we should keep the old clk_div as is, I'm also okay with that.

meggiman commented 1 year ago

I have no strong opinion here. But if we change the existing clk_div module it has to be released in a major version bump since it will change the behaviour of an existing module (and the interface).