SDC: Handle input clock phase shift on clock generators

[tmichalak]

Opening this PR for discussion purposes.

For the initial version of the SDC plugin we had a discussion around phase shift and validation of the waveform.
In the current implementation we have a 1ns delay on BUFGs and clock synth's (like MMCM and PLL) output a clock with the computed frequency, and a phase shift equal to the phase specified. For example if the PLL input has a 20 degree phase shift, and the request to the PLL is to add another 45 degree phase shift, the output waveform will have a 65 degree phase shift.

However, this sometimes leads to sanity checks being raised:

`8. Executing Verilog-2005 frontend: /tmpfs/src/github/symbiflow-arch-defs-presubmit-xc7/env/conda/envs/symbiflow_arch_def_base/bin/../share/yosys//plugins/fasm_extra_modules/BANK.v
ERROR: Phase shift on clock sys4x_dqs_clk exceeds 360 degrees
Rising edge: 3.041667, Falling edge: 5.125000, Clock period:4.166667

Not sure how vivado handles these situations though.

[mkurc-ant]

On page 66 in https://www.xilinx.com/support/documentation/user_guides/ug472_7Series_Clocking.pdf are details on how the phase detector of a PLL works. Judging from that I conclude that the whole thing behaves as an "integrating regulator" so it can synchronize both phases and frequencies of CLKINn and CLKFB.

I also think that just adding phases is wrong as the frequency of a PLL output may be different than of CLKINn. With that phases on both clocks have different meanings and cannot be added/compared directly.

[litghost]

I also think that just adding phases is wrong as the frequency of a PLL output may be different than of CLKINn. With that phases on both clocks have different meanings and cannot be added/compared directly.

The PLL's output phase should be related to one of the input phases in some way. I agree that it isn't a straight summation isn't the right answer, but having the output clocks have no relation to the input phase is wrong too? The output of the VCO in a PLL matches the phase of the input clock. The PLL output registers can then be phase shifted as needed.

See page 74 and eq 3-3, 3-4, 3-5. They are all speaking about shifting phase with respect to the input clock.

[litghost]

So I think I understand the problem, and know half of the solution. Let's ignore frequency changes for a minute. If a 10 ns period, 0 deg 50% wave form enters a PLL that is the following create clock:

create_clock -period 10 -waveform {0 5} clk

So the clock raises on 0 and 1*period and 2*period, etc.
The clock falls on 5 and 5+1*period, etc.

If we apply a 270 degree phase shift to clk, it will be:

create_clock -period 10 -waveform {7.5 2.5} clk

So the clock raises at 7.5 and 7.5+1*period and 7.5+2*period, etc.
The failing clock case is interesting. It could be thought as having either a 12.5 falling edge, or a 2.5 falling edge, they are equivalent. I believe SDC requires the edge time be modulus the period. For clarity: 2.5 + 1*period == 12.5 + 0*period.

So one bug that needs to be fixed is the waveform values need to be modulus the period.

That still leaves the meaning of the phase relationship through a PLL when there is a frequency shift. I believe I know the answer, but want to think on it more.

[litghost]

I've done some work, and I believe I understand how to relate the input clock phase and the output clock phase on the PLL.

If the PLL output phase shift = 0 deg, then the rising edge of the output clock matches the rising edge of the VCO. When FBCLKOUT phase shift = 0, this implies that the VCO and input clock rising edge are phase matched. This means the rising edge of the input clock and the rising edge of the output clock should be equal given the following equation:

input clock rising time + N1 * input clock period = output clock rising time + N2 * output clock period

where N1 and N2 are some natural number (e.g. some integer >=0).

If the PLL output phase shift != 0, the output clock is phase shifted per it's output frequency, with respect to the output phase shift = 0 case.

So for example:

If the input clock is 100 MHz, with a phase shift of 90 degree, that implies the following create clock:

create_clock -period 10 -waveform {2.5 7.5} clk

An output clock of 200 MHz with an output phase shift of 0 should have a rising edge at 2.5 ns, so the create clock is:

create_clock -period 5 -waveform {2.5 0} clkout

An output clock of 200 MHz with an output phase shift of 90 deg would have a create clock of:

create_clock -period 5 -waveform {3.75 1.25} clkout

So I believe this is how to apply input phase to output phase:

1. Compute the input clocks rising edge time
2. Create a non-phase shifted output at the new frequency that shares that rising edge, e.g. the remainder of the rising edge time and the new output clock period
3. If the PLL is apply a phase shift, shift the non-shifted waveform by (phase shift / 360) * period ns. If either the rising or falling edge are greater than period, then subtract one period from that wave form.
   3a. For output clocks with duty cycle != 50%, compute the falling edge time from the rising edge time + duty cycle * period

@mkurc-ant / @tmichalak Do you disagree with anything here? I'm going to do some testing to figure out how to accommodate feedback phase shifts.

[litghost]

Okay, looks like CLKFBOUT_PHASE works pretty much like CLKOUT<n>_PHASE, but on the input clock frequency and negative.

So I think the equations for the output clock wave forms are:

output clock period = input clock period * (DIVCLK_DIVIDE * CLKOUT<n>_DIVIDE) / CLKFBOUT_MULT
output clock rising edge = (input clock rising edge - (CLKFBOUT_PHASE / 360) * input clock period + (CLKOUT<n>_PHASE / 360) * output clock period) % output clock period
output clock falling edge = (output clock rising edge + CLKOUT<n>_DUTY_CYCLE * output clock period) % output clock period

[litghost]

Proposed fix for PLL phase shifts

[tmichalak]

@litghost All of the above makes sense. Thanks a lot for the analysis. Judging from it the only things missing in the original implementation were the period modulo and negative phase shift from the CLKFBOUT_PHASE.
BTW in the equation for the period:
output clock period = input clock period * (CLKFBOUT_MULT / DIVCLK_DIVIDE) / CLKOUT<n>_DIVIDE
it should be more like:
output clock period = input clock period * (DIVCLK_DIVIDE * CLKOUT<n>_DIVIDE) / CLKFBOUT_MULT
otherwise bigger frequency multiplier values would result in bigger periods

[litghost]

BTW in the equation for the period:
output clock period = input clock period * (CLKFBOUT_MULT / DIVCLK_DIVIDE) / CLKOUT<n>_DIVIDE
it should be more like:
output clock period = input clock period * (DIVCLK_DIVIDE * CLKOUT<n>_DIVIDE) / CLKFBOUT_MULT
otherwise bigger frequency multiplier values would result in bigger periods

Of course. Higher period -> more division.

[litghost]

Why is the delay through a Bufg still 1 ns? We should either set it to 0, or a more reasonable value. 0 is probably better for now.

[litghost]

Should have a FIXME/TODO about the CLKFBOUT_PHASE not being supported. Maybe we should error if CLKFBOUT_PHASE is non-zero?

[tmichalak]

Added support for CLKFBOUT_PHASE and addressed all above mentioned comments regarding shift calculations.
The period and waveform is calculated according to the equations discussed in the discussion above.
Removed the temporary delay values on IBUF, BUFG (set them to 0).
Updated the references of all tests and added new ones for DIVLK_DIVIDE and CLKFBOUT_PHASE.

Regarding CLKFBOUT_PHASE the waveforms have negative values and it's fine this way, at least according to vivado.
I tested in vivado the following waveforms: {-5 0}, {5 10}, {5 0} and the reported clock trees were identical.

[litghost]

This should be a approx equal check, e.g. fabs(a - b) < max(a, b)*10*epsilon. This prevents non-relevant rounding errors from triggering the error.

[tmichalak]

Done. However I am not sure what is meant by the 10 in 10*epsilon.
I implemented it as fabs(a-b) < max(a, b) * kApproxEqualFactor where the kApproxEqualFactor is expressed in float, i.e. 0.01 means +/- 1%

[litghost]

https://en.cppreference.com/w/cpp/types/numeric_limits/epsilon

[tmichalak]

Thanks, fixed.

[mithro]

For future developers I feel like this could benefit from a write up with a few diagrams (maybe wavedrom based)?

[tmichalak]

For future developers I feel like this could benefit from a write up with a few diagrams (maybe wavedrom based)?

Wavedrom looks awesome!
I think at some point we will have to document what we have done in all of the Yosys plugins.
Wavedrom for explaining the SDC plugin will be indispensable.

[litghost]

LGTM