SYS/RTIO clock merge #129

Spaqin · 2022-07-26T09:52:08Z

So for Kasli we'd like to use the RTIO clock (output of Si5324) as SYS clock, avoiding clock domain transfers, with a simple bootstrap clock before the bootloader changes the clock. It seems possible in general, but comes with its own challenges - e.g. no support for 100/150MHz clocks (unless we set the MMCM/PLL parameters to match these frequencies? change in gateware + software, essentially, different build).

Marking this PR as WIP as I've been having difficulties with it and it's nowhere near finished. There's also kc705 support in there to be done as well, and I suppose it should be eventually merged in tandem with ARTIQ part.

Few things that I encountered though that I would like an explanation for:

why is the div2 clock used in MMCM/PLL, rather than the 125MHz one?
why was sysclk running at ~114MHz instead of 125 (see clkfbout_mult_f @ 14.5 instead of 16)? this looks like an oversight?
Kasli QPLL took the buffered 125MHz from the reference directly (see line 209/256 of kasli.py) - I cannot use the "chosen clock" from BUFGMUX, as Vivado complains about unbuffered output, and sending the BUFGMUX output through BUFG/BUFH/BUFR resulted in unroutable design. Which should it use, is there a way to choose a clock for it too?

At the point of making this PR this method doesn't meet timing constraints and I can't seem to find a way to improve it (e.g. putting the si5324 clock through MMCM and then to BUFGMUX makes it even worse). Unless there's a different, better way to switch between the two clocks (probably is). I managed to run it at some point, but more by luck I suppose. I get garbage output on serial most of the time, possibly due to unmet timing, quite badly too.

------------------------------------------------------------------------------------------------
| Design Timing Summary
| ---------------------
------------------------------------------------------------------------------------------------

    WNS(ns)      TNS(ns)  TNS Failing Endpoints  TNS Total Endpoints      WHS(ns)      THS(ns)  THS Failing Endpoints  THS Total Endpoints     WPWS(ns)     TPWS(ns)  TPWS Failing Endpoints  TPWS Total Endpoints  
    -------      -------  ---------------------  -------------------      -------      -------  ---------------------  -------------------     --------     --------  ----------------------  --------------------  
     -3.074       -5.992                      2                36450        0.056        0.000                      0                36446        0.264        0.000                       0                 15627  


Timing constraints are not met.

Open for suggestions, changes, criticism.

sbourdeauducq · 2022-07-26T10:16:09Z

why is the div2 clock used in MMCM/PLL, rather than the 125MHz one?

Because of routing restrictions of IBUFDS_GTE2. You can't have the same output to the QPLL and to the fabric.

why was sysclk running at ~114MHz instead of 125 (see clkfbout_mult_f @ 14.5 instead of 16)? this looks like an oversight?

Kasli 1 with the lower speed grade FPGA cannot meet timing at 125MHz. I guess we will have to introduce a slower RTIO clock to support legacy systems.

Kasli QPLL took the buffered 125MHz from the reference directly (see line 209/256 of kasli.py) - I cannot use the "chosen clock" from BUFGMUX, as Vivado complains about unbuffered output, and sending the BUFGMUX output through BUFG/BUFH/BUFR resulted in unroutable design.

Indeed this is unroutable - the QPLL/transceiver needs to use the dedicated clocking pins and dedicated clocking network.
And you don't want to switch that clock, it needs to be exactly 125MHz for Ethernet. And the Ethernet clock domain is asynchronous to all other domains. So just keep that direct IBUFDS_GTE2 -> QPLL connection, it is fine (and there aren't many other options anyway).

Your code cannot work reliably because the system PLL/MMCM may unlock/glitch when you switch clocks. You cannot put a BUFGMUX at the input of a MMCM without precautions, which are difficult to take here because the MMCM needs to be continuously clocking the CPU. It seems to confuse the Vivado timing analyzer as well.

I suggest you redesign it based on this scheme from Xilinx UG472:

The top-left IBUFG would take the Si5324 output, and the top-right BUFG would become a BUFGMUX with the main input from the MMCM (as shown) and the IBUFDS_GTE2 ODIV2 output on its other input. It is fine if the system bootstraps at 62.5MHz, just correct the UART baudrate register. Note that BUFG and BUFGMUX correspond to the same physical resource inside the FPGA (BUFG is just a software abstraction with some BUFGMUX signals hidden), so the deskew should also work. The feedback BUFG would stay as on the UG472 diagram.

- no more 115mhz - si5324 to pll - bufgmux takes raw bootstrap or pll output

This reverts commit 31f11d6.

sbourdeauducq · 2022-07-28T09:49:33Z

misoc/targets/kasli.py

+
+        self.specials += Instance("IBUFDS_GTE2",
+            i_CEB=0,
+            i_I=si5324_out.p, i_IB=si5324_out.n,


Should be called cdr_clk_... since it may not come from the Si5324 after WRPLL.

sbourdeauducq · 2022-07-28T09:49:58Z

misoc/targets/kasli.py

@@ -61,44 +99,56 @@ def __init__(self, platform):
        pll_clk200 = Signal()
        self.specials += [
            Instance("MMCME2_BASE",
-                p_CLKIN1_PERIOD=16.0,
-                i_CLKIN1=self.clk125_div2,
+                # 150mhz not supported anymore


This is not the place to mention this.

sbourdeauducq · 2022-07-28T09:51:57Z

misoc/targets/kasli.py

-        SoCSDRAM.__init__(self, platform,
-                          clk_freq=125e6*14.5/16, cpu_reset_address=0x400000,
-                          **kwargs)
+        kwargs["uart_initial_clk_freq"] = 62.5e6


Why mutate kwargs?

Just do uart_initial_clk_freq=62.5e6 below?

sbourdeauducq · 2022-07-28T09:59:03Z

misoc/targets/kasli.py

                p_CLKOUT1_DIVIDE=2, p_CLKOUT1_PHASE=0.0, o_CLKOUT1=mmcm_sys4x,
                p_CLKOUT2_DIVIDE=2, p_CLKOUT2_PHASE=90.0, o_CLKOUT2=mmcm_sys4x_dqs,
            ),
            Instance("PLLE2_BASE",
-                p_CLKIN1_PERIOD=16.0,
-                i_CLKIN1=self.clk125_div2,
+                p_CLKIN1_PERIOD=si5324_period*2,


Why not just keep that one on clk125?
The IDELAYCTRL 200MHz clock is asynchronous to the others.

sbourdeauducq · 2022-07-28T09:59:32Z

misoc/targets/kasli.py

-            i_I=clk125.p, i_IB=clk125.n,
-            o_O=self.clk125_buf,
-            o_ODIV2=self.clk125_div2)
+            i_I=bootstrap.p, i_IB=bootstrap.n,


You don't need to rename this, it could still be called clk125 to match the hw and migen pin names.

sbourdeauducq · 2022-07-28T10:00:06Z

misoc/targets/kasli.py

+            o_ODIV2=si5324_div2)
+
+        # required for qpll
+        self.clk125_buf = bootstrap_buf


Remove after https://github.com/m-labs/misoc/pull/129/files#r932022819

Spaqin · 2022-08-24T07:20:31Z

Although this looks to be what we need, I cannot get the system to boot anymore (no code is run at all, no UART output). Probably something I missed in the log.

With the BUFGMUX for cd_sys and just BUFG from MMCM for cd_sys4x, there will be a slew of warnings:

WARNING: [DRC REQP-1580] Phase alignment: Unsupported clocking topology used for ISERDESE2 ISERDESE2. This can result in corrupted data. The ISERDESE2/CLK / ISERDESE2/CLKDIV pins should be driven by the same source through the same buffer type or by a BUFIO / BUFR combination in order to have a proper phase relationship. Please refer to the Select I/O User Guide for supported clocking topologies of the chosen INTERFACE_TYPE mode.

For all ISERDESE2 and OSERDESE2 - makes sense, Vivado doesn't know the ISERDESE2 will be used after the clock hand-over, with proper relationship - but it does clutter the log.

sbourdeauducq · 2022-08-24T07:24:10Z

misoc/targets/kasli.py

+            cdr_clk_out = platform.request("si5324_clkout")
+
+        if freq == 125.0e6:
+           cdr_clk_period = 8.


indentation

sbourdeauducq · 2022-08-24T07:26:33Z

misoc/targets/kasli.py

+
+        platform.add_false_path_constraints(
+            self.clk125_buf,
+            self.cd_sys.clk, self.cd_sys4x.clk, self.cd_sys4x_dqs.clk, self.cd_clk200.clk)


False path between cd_sys and cd_sys4x sounds wrong.

sbourdeauducq · 2022-08-24T07:27:13Z

Although this looks to be what we need, I cannot get the system to boot anymore

Can you blink an LED with a counter in the sys domain?

sbourdeauducq · 2022-08-24T07:28:57Z

misoc/targets/kasli.py

        clk125 = platform.request("clk125_gtp")
        platform.add_period_constraint(clk125, 8.)
        self.clk125_buf = Signal()
-        self.clk125_div2 = Signal()


ARTIQ is using this. https://github.com/m-labs/artiq/blob/3535d0f1ae98829538b013714aa1a63c0621f9ce/artiq/gateware/targets/kasli.py#L593

sbourdeauducq · 2022-08-24T07:31:43Z

misoc/targets/kasli.py

+        self.pll_locked = CSRStatus()
+        self.mmcm_locked = CSRStatus()
+
+        # old "sys" clock, now bootstrap


Just say "bootstrap clock"

sbourdeauducq · 2022-08-24T07:32:22Z

misoc/targets/kasli.py

+        else:
+            raise NotImplementedError
+
+        self.cdr_clk_buf = Signal()


Where is this attribute used?

OK, I see, you are moving the IBUFDS_GTE2 from ARTIQ kasli.py to here?

Simpler to use cdr_clk_clean_fabric / si5324_clkout_fabric.
BTW this (or your current code) will break DRTIO since you won't meet timing at the transceiver's TXDATA pins anymore. On DRTIO systems, the post-bootstrap system clock needs to be driven from the transceiver's TXOUTCLK pin. But we can deal with that later.

You will need something like this (from UG482), but with both TXUSRCLK2 and TXUSRCLK on the same clock (as done currently).

BUFH if you can use it would be lighter on resources.

restore async reset for clk200

sbourdeauducq · 2022-09-26T11:28:06Z

misoc/targets/kasli.py

+
+        # at 62.5MHz bootstrap cd, will get around 1ms
+        # todo: check if can do it faster
+        delay_counter = Signal(16, reset=0xFFFF_FFFF)


That reset value is 32-bit.

sbourdeauducq · 2022-09-26T11:29:06Z

misoc/targets/kasli.py

+        # todo: check if can do it faster
+        delay_counter = Signal(16, reset=0xFFFF_FFFF)
+
+        # register to prevent glitches


also add no_retiming

sbourdeauducq · 2022-09-26T11:29:27Z

misoc/targets/kasli.py

+            i_switch.eq(clock_switch_sync.o)
+        ]
+
+        #2-reg the done signal to prevent glitches


use MultiReg instead

...which you are already doing below. Here one register with no_retiming is enough.

sbourdeauducq · 2022-09-26T11:29:52Z

misoc/targets/kasli.py

+        self.submodules += fsm
+
+        fsm.act("START",
+            If(i_switch == 1,


just If(i_switch, ...

sbourdeauducq · 2022-09-26T11:33:07Z

misoc/targets/kasli.py

+        )
+
+        fsm.act("RESET_START",
+            NextValue(reset, 1),


You can simply drive signals using eq() in fsm.act(). They are driven combinatorially then, with the current FSM state as input.

NextValue adds a register.

sbourdeauducq · 2022-09-26T11:39:22Z

misoc/targets/kasli.py

-    def __init__(self, sdram_controller_type="minicon", hw_rev=None,
-                 **kwargs):
+    def __init__(self, sdram_controller_type="minicon", hw_rev=None, rtio_sys_merge=False, 
+                 clk_freq=125e6*14.5/16, **kwargs):


Shouldn't clk_freq be 125e6 now?

backwards compatibility - by default it should go back to old frequency matching p_CLKFBOUT_MULT_F=14.5

Hmm, OK. But changing this in ARTIQ at the same time as we update MiSoC shouldn't be difficult.
Also the timing usually passes anyway on Kasli 2 at 125MHz.

- use multireg instead of manual registers - use combinatorial FSM where aplicable instead of reg

sbourdeauducq · 2022-10-05T05:26:30Z

misoc/targets/kasli.py

+
+        fsm.act("CLOCK_SWITCH",
+            reset.eq(1),
+            NextValue(o_switch, 1),


or just use done? It will definitely take more than a few cycles before the CPU reads the register, so it'll work.

sbourdeauducq · 2022-10-05T05:26:59Z

misoc/targets/kasli.py

+        reset = Signal()
+
+        # at 62.5MHz bootstrap cd, will get around 1ms
+        # todo: check if can do it faster


It doesn't really matter, remove the todo.

Spaqin · 2022-10-06T04:42:28Z

Still in WIP because after moving the reboot to the firmware it seems rather unreliable - sometimes it reboots after firmware sets the clock switch bit, sometimes it doesn't and just continues onward.

- refactor 14.5/16 clk_freq

Spaqin · 2022-10-06T08:22:31Z

PulseSynchronizer sometimes wouldn't synchronize the pulse - not sure why, but ~half the time it would fail. An even simpler approach seemed to have worked, though - reboots every time.

sbourdeauducq · 2022-10-07T10:36:38Z

misoc/targets/kasli.py

-            state[2].eq(o_switch),
-        ]
+        # latch the clock switch (no going back)
+        self.sync.bootstrap += If(self.i_clk_sw, i_switch.eq(1))


That's a register not a latch and it won't work reliably (clocked by bootstrap, input in sys).
If you do want a latch use Instance of something like FDPE (check Xilinx docs) and the PRE input, and make sure PRE does not glitch (e.g. register output with no_retiming).

Spaqin added 5 commits July 18, 2022 16:48

initial changes

b87a5dc

si5324 from gte output is required

9787050

restore div2 clocks and use them in pll/mmcm

3b5d134

expose clk125_buf after all for qpll

0789b11

kasli: fix 125mhz pll output

61c8821

Spaqin added 9 commits July 27, 2022 14:48

kasli:

8832f34

- no more 115mhz - si5324 to pll - bufgmux takes raw bootstrap or pll output

mmcme2 driven by div2

5ae28b0

si5324: ibufg the output before mmcme2/pll

cc69d4a

gte clock -> fabric clock

31f11d6

kasli: quick fix pll multiplier

5b65a03

Revert "gte clock -> fabric clock"

1472229

This reverts commit 31f11d6.

use si5324 div2 due to routing issues

9de4ea7

kasli: add bufg for mmcme

ffd8c3b

set uart initial speed

9c33b40

sbourdeauducq reviewed Jul 28, 2022

View reviewed changes

Spaqin force-pushed the clk_merge branch from e85f9cd to 7d2b128 Compare August 24, 2022 07:11

sbourdeauducq reviewed Aug 24, 2022

View reviewed changes

sbourdeauducq mentioned this pull request Sep 2, 2022

Added support for Digilent Genesys 2 m-labs/artiq#1961

Open

7 tasks

Spaqin added 4 commits September 21, 2022 14:45

rename clocks, cosmetics

5b75ce2

restore async reset for clk200

shorten false path constraints

836d03e

restore divided bootstrap clock

4d6eeb2

add constraint between sys and sys4x

3a7a76e

Spaqin force-pushed the clk_merge branch from 9a29b89 to 8f9a0bc Compare September 21, 2022 06:47

Spaqin added 2 commits September 23, 2022 12:27

implement fsm for changing the clock through mmcm_adv

50e7bd1

make it backwards compatible

98b5d20

Spaqin force-pushed the clk_merge branch from 8f9a0bc to 98b5d20 Compare September 23, 2022 07:24

sbourdeauducq reviewed Sep 26, 2022

View reviewed changes

misoc/targets/kasli.py Outdated

self.submodules += fsm

fsm.act("START",

If(i_switch == 1,

Copy link

Member

sbourdeauducq Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just If(i_switch, ...

sbourdeauducq reviewed Sep 26, 2022

View reviewed changes

minor simplifications:

5ac8071

- use multireg instead of manual registers - use combinatorial FSM where aplicable instead of reg

sbourdeauducq reviewed Oct 5, 2022

View reviewed changes

Spaqin changed the title ~~[WIP] SYS/RTIO clock merge~~ SYS/RTIO clock merge Oct 5, 2022

Spaqin changed the title ~~SYS/RTIO clock merge~~ [WIP] SYS/RTIO clock merge Oct 5, 2022

Spaqin added 3 commits October 6, 2022 16:04

- remove o_done, use o_switch instead

aabef0b

- refactor 14.5/16 clk_freq

remove done state, allow resetting

ab1c714

remove pulsesynchronizer as it fails randomly

a451829

Spaqin force-pushed the clk_merge branch from 720ce36 to a451829 Compare October 6, 2022 08:19

Spaqin changed the title ~~[WIP] SYS/RTIO clock merge~~ SYS/RTIO clock merge Oct 6, 2022

sbourdeauducq reviewed Oct 7, 2022

View reviewed changes

Spaqin added 2 commits October 10, 2022 14:43

fdpe i_clk_sw

2475b67

fdpe -> just multireg

ecb8d40

sbourdeauducq merged commit 4fb0730 into m-labs:master Oct 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SYS/RTIO clock merge #129

SYS/RTIO clock merge #129

Spaqin commented Jul 26, 2022

sbourdeauducq commented Jul 26, 2022 •

edited

sbourdeauducq Jul 28, 2022

sbourdeauducq Jul 28, 2022

sbourdeauducq Jul 28, 2022

sbourdeauducq Jul 28, 2022

sbourdeauducq Jul 28, 2022

sbourdeauducq Jul 28, 2022

Spaqin commented Aug 24, 2022

sbourdeauducq Aug 24, 2022

sbourdeauducq Aug 24, 2022

sbourdeauducq commented Aug 24, 2022

sbourdeauducq Aug 24, 2022

sbourdeauducq Aug 24, 2022

sbourdeauducq Aug 24, 2022

sbourdeauducq Aug 24, 2022 •

edited

sbourdeauducq Aug 24, 2022 •

edited

sbourdeauducq Aug 24, 2022 •

edited

sbourdeauducq Sep 26, 2022

sbourdeauducq Sep 26, 2022

sbourdeauducq Sep 26, 2022

sbourdeauducq Sep 26, 2022

sbourdeauducq Sep 26, 2022

sbourdeauducq Sep 26, 2022

sbourdeauducq Sep 26, 2022

sbourdeauducq Sep 26, 2022

Spaqin Sep 30, 2022

sbourdeauducq Sep 30, 2022

sbourdeauducq Oct 5, 2022

sbourdeauducq Oct 5, 2022

Spaqin commented Oct 6, 2022 •

edited

Spaqin commented Oct 6, 2022

sbourdeauducq Oct 7, 2022

SYS/RTIO clock merge #129

SYS/RTIO clock merge #129

Conversation

Spaqin commented Jul 26, 2022

sbourdeauducq commented Jul 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spaqin commented Aug 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbourdeauducq commented Aug 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbourdeauducq Aug 24, 2022 • edited

Choose a reason for hiding this comment

sbourdeauducq Aug 24, 2022 • edited

Choose a reason for hiding this comment

sbourdeauducq Aug 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spaqin commented Oct 6, 2022 • edited

Spaqin commented Oct 6, 2022

Choose a reason for hiding this comment

sbourdeauducq commented Jul 26, 2022 •

edited

sbourdeauducq Aug 24, 2022 •

edited

sbourdeauducq Aug 24, 2022 •

edited

sbourdeauducq Aug 24, 2022 •

edited

Spaqin commented Oct 6, 2022 •

edited