cores/clocks/lattice_ecp5: Fix phase calculation to match Diamond output

[zeldin]

Disclaimer: I am not a Lattice employee and do not have access to internal documentation -- anything below regarding internal workings of the PLL should be taken as conjecture. PLLs are not complicated things though, so it's not that difficult to suss out how they work. 😄

The current phase calculation for ECP5 EHXPLLL has two problems:

1. It doesn't set FPHASE at all. This value is a phase offset in eighths of a VCO cycle added to CPHASE, so the calculation of CPHASE should be done as fix-point with three fractional bits. Diamond also performs correct rounding based on the fourth fractional bit.
2. The CPHASE calculation uses div+1 as the scale factor for a complete clock period. This is obviously wrong as a complete period is div VCO cycles, not div+1. I believe the confusion here stems from the fact that the divisor value in the bitstream is actually a counter reload value, and thus one smaller than the divisor. This is not visible in the verilog though; the subtraction is performed in the P&R tool.¹ Therefore any assertion that div+1 should be used is likely talking about the bitstream value, not the div value used in LiteX which is the verilog value and thus already "+1".

With these changes, the following clock setup:

  pll = ECP5PLL()
  clk48 = Signal()
  cd_a = ClockDomain()
  cd_b = ClockDomain()

  pll.register_clkin(clk48, 48e6)
  pll.create_clkout(cd_a, 64e6, margin = 0, phase = 315)
  pll.create_clkout(cd_b, 128e6, margin = 0, phase = 315)

generates

    (* FREQUENCY_PIN_CLKI = "48.0", FREQUENCY_PIN_CLKOP = "64.0", FREQUENCY_PIN_CLKOS = "128.0", ICP_CURRENT = "6", LPF_RESISTOR = "16", MFG_ENABLE_FILTEROPAMP = "1", MFG_GMCREF_SEL = "2" *)
EHXPLLL #(
	.CLKFB_DIV(5'd16),
	.CLKI_DIV(1'd1),
	.CLKOP_CPHASE(5'd21),
	.CLKOP_DIV(4'd12),
	.CLKOP_ENABLE("ENABLED"),
	.CLKOP_FPHASE(3'd4),
	.CLKOS2_CPHASE(1'd0),
	.CLKOS2_DIV(1'd1),
	.CLKOS2_ENABLE("ENABLED"),
	.CLKOS2_FPHASE(1'd0),
	.CLKOS_CPHASE(4'd10),
	.CLKOS_DIV(3'd6),
	.CLKOS_ENABLE("ENABLED"),
	.CLKOS_FPHASE(2'd2),
	.FEEDBK_PATH("INT_OS2")
) EHXPLLL ( ... );

which is identical to Diamond output:

/* Verilog netlist generated by SCUBA Diamond (64-bit) 3.13.0.56.2 */
/* Module Version: 5.7 */
/* /usr/local/diamond/3.13/ispfpga/bin/lin64/scuba -n pll -lang verilog -arch sa5p00 -type pll -fin 48 -fclkop 64 -fclkop_tol 0.0 -phasep 315 -fclkos 128 -fclkos_tol 0.0 -phases 315 -fclkos2 768 -fclkos2_tol 0.0 -phase_cntl STATIC -fb_mode 7  */
[...]
    defparam PLLInst_0.PLLRST_ENA = "DISABLED" ;
    defparam PLLInst_0.INTFB_WAKE = "DISABLED" ;
    defparam PLLInst_0.STDBY_ENABLE = "DISABLED" ;
    defparam PLLInst_0.DPHASE_SOURCE = "DISABLED" ;
    defparam PLLInst_0.CLKOS3_FPHASE = 0 ;
    defparam PLLInst_0.CLKOS3_CPHASE = 0 ;
    defparam PLLInst_0.CLKOS2_FPHASE = 0 ;
    defparam PLLInst_0.CLKOS2_CPHASE = 0 ;
    defparam PLLInst_0.CLKOS_FPHASE = 2 ;
    defparam PLLInst_0.CLKOS_CPHASE = 10 ;
    defparam PLLInst_0.CLKOP_FPHASE = 4 ;
    defparam PLLInst_0.CLKOP_CPHASE = 21 ;
    defparam PLLInst_0.PLL_LOCK_MODE = 0 ;
    defparam PLLInst_0.CLKOS_TRIM_DELAY = 0 ;
    defparam PLLInst_0.CLKOS_TRIM_POL = "FALLING" ;
    defparam PLLInst_0.CLKOP_TRIM_DELAY = 0 ;
    defparam PLLInst_0.CLKOP_TRIM_POL = "FALLING" ;
    defparam PLLInst_0.OUTDIVIDER_MUXD = "DIVD" ;
    defparam PLLInst_0.CLKOS3_ENABLE = "DISABLED" ;
    defparam PLLInst_0.OUTDIVIDER_MUXC = "DIVC" ;
    defparam PLLInst_0.CLKOS2_ENABLE = "ENABLED" ;
    defparam PLLInst_0.OUTDIVIDER_MUXB = "DIVB" ;
    defparam PLLInst_0.CLKOS_ENABLE = "ENABLED" ;
    defparam PLLInst_0.OUTDIVIDER_MUXA = "DIVA" ;
    defparam PLLInst_0.CLKOP_ENABLE = "ENABLED" ;
    defparam PLLInst_0.CLKOS3_DIV = 1 ;
    defparam PLLInst_0.CLKOS2_DIV = 1 ;
    defparam PLLInst_0.CLKOS_DIV = 6 ;
    defparam PLLInst_0.CLKOP_DIV = 12 ;
    defparam PLLInst_0.CLKFB_DIV = 16 ;
    defparam PLLInst_0.CLKI_DIV = 1 ;
    defparam PLLInst_0.FEEDBK_PATH = "INT_OS2" ;

(I created the reference clock manually in this case.)
With the old LiteX code the output was instead

(* FREQUENCY_PIN_CLKI = "48.0", FREQUENCY_PIN_CLKOP = "64.0", FREQUENCY_PIN_CLKOS = "128.0", ICP_CURRENT = "6", LPF_RESISTOR = "16", MFG_ENABLE_FILTEROPAMP = "1", MFG_GMCREF_SEL = "2" *)
EHXPLLL #(
	.CLKFB_DIV(5'd16),
	.CLKI_DIV(1'd1),
	.CLKOP_CPHASE(5'd22),
	.CLKOP_DIV(4'd12),
	.CLKOP_ENABLE("ENABLED"),
	.CLKOP_FPHASE(1'd0),
	.CLKOS2_CPHASE(1'd0),
	.CLKOS2_DIV(1'd1),
	.CLKOS2_ENABLE("ENABLED"),
	.CLKOS2_FPHASE(1'd0),
	.CLKOS_CPHASE(4'd11),
	.CLKOS_DIV(3'd6),
	.CLKOS_ENABLE("ENABLED"),
	.CLKOS_FPHASE(1'd0),
	.FEEDBK_PATH("INT_OS2")
) EHXPLLL ( ... );

i.e. OP_CPHASE=22, OP_FPHASE=0, OS_CPHASE=11, OS_FPHASE=0 when it should have been OP_CPHASE=21, OP_FPHASE=4, OS_CPHASE=10, OS_FPHASE=2.

Footnotes

1. This is also why CPHASE should be set to div-1 for a zero phase -- CPHASE is used as the counter load value on the first cycle, so setting it to the normal reload value of div-1 gives a cycle or normal length, a higher number will result in a longer first cycle giving a phase offset to the following cycles. ↩

[enjoy-digital]

Thanks @zeldin! I don't think previous ECP5/Phase support has been used/tested extensively. Your PR/Explanations make sense so let's merge.