Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SDC plugin does not output the BUFG out clocks anymore #64

Open
acomodi opened this issue Dec 9, 2020 · 11 comments
Open

SDC plugin does not output the BUFG out clocks anymore #64

acomodi opened this issue Dec 9, 2020 · 11 comments
Assignees

Comments

@acomodi
Copy link
Contributor

acomodi commented Dec 9, 2020

With the clock propagation enhancements done in #54, the resulting SDC does not take into account the output clock from BUFGs connected to the PLL outputs.

This causes VPR to not correctly constrain clock signals, resulting in higher run-time and an nan CPD, such as in the following example:

PLLE2_ADV #(
        .CLKFBOUT_MULT(4'd12),
        .CLKIN1_PERIOD(10.0),
        .CLKOUT0_DIVIDE(5'd20),
        .CLKOUT0_PHASE(1'd0),
        .CLKOUT1_DIVIDE(3'd5),
        .CLKOUT1_PHASE(1'd0),
        .CLKOUT2_DIVIDE(3'd5),
        .CLKOUT2_PHASE(7'd90),
        .CLKOUT3_DIVIDE(3'd6),
        .CLKOUT3_PHASE(1'd0),
        .CLKOUT4_DIVIDE(6'd48),
        .CLKOUT4_PHASE(1'd0),
        .DIVCLK_DIVIDE(1'd1),
        .REF_JITTER1(0.01),
        .STARTUP_WAIT("FALSE")
) PLLE2_ADV (
        .CLKFBIN(pll_fb),
        .CLKIN1(crg_clkin),
        .RST(reset7),
        .CLKFBOUT(pll_fb),
        .CLKOUT0(crg_clkout0),
        .CLKOUT1(crg_clkout1),
        .CLKOUT2(crg_clkout2),
        .CLKOUT3(crg_clkout3),
        .CLKOUT4(crg_clkout4),
        .LOCKED(crg_locked)
);


BUFG BUFG(
        .I(crg_clkout0),
        .O(crg_clkout_buf0)
);

BUFG BUFG_1(
        .I(crg_clkout1),
        .O(crg_clkout_buf1)
);

BUFG BUFG_2(
        .I(crg_clkout2),
        .O(crg_clkout_buf2)
);

BUFG BUFG_3(
        .I(crg_clkout3),
        .O(crg_clkout_buf3)
);

BUFG BUFG_4(
        .I(crg_clkout4),
        .O(crg_clkout_buf4)
);

Resulting SDC:

create_clock -period 16.6667 -waveform {0 8.33333} crg_clkout0
create_clock -period 5 -waveform {0 2.5} crg_clkout3
create_clock -period 40 -waveform {0 20} crg_clkout4

This example is taken from the LiteX mini design, for which the crg_clkout1 and crg_clkout2 clock nets are unconnected, therefore they do not end up in the SDC, and this is expected.

The problem though is that the correct SDC for VPR should have also the crg_clkoutN_bufN clocks as well.
I notice also that the clock constraint for the input clock net to the PLL is missing as well.

acomodi added a commit to antmicro/fpga-tool-perf that referenced this issue Dec 9, 2020
See issue:
chipsalliance/yosys-f4pga-plugins#64

Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@tmichalak
Copy link
Collaborator

@acomodi So the current behavior is expected as the clock parameters (period, waveform) on the BUFGs are the same as on the PLL output wires that are driving them.
So what you expect is the following SDC:

create_clock -period 16.6667 -waveform {0 8.33333} crg_clkout0
create_clock -period 16.6667 -waveform {0 8.33333} crg_clkout_buf0
create_clock -period 5 -waveform {0 2.5} crg_clkout3
create_clock -period 5 -waveform {0 2.5} crg_clkout_buf3
create_clock -period 40 -waveform {0 20} crg_clkout4
create_clock -period 40 -waveform {0 20} crg_clkout_buf4

Of course what is worrying is the fact that the clocks for crg_clkout1 and crg_clkout2 so CLKOUT1 and CLKOUT2 outputs of the PLL are not in the SDC. Is the SDC you pasted complete?

@acomodi
Copy link
Contributor Author

acomodi commented Dec 9, 2020

Of course what is worrying is the fact that the clocks for crg_clkout1 and crg_clkout2 so CLKOUT1 and CLKOUT2 outputs of the PLL are not in the SDC. Is the SDC you pasted complete?

@tmichalak Those are absent as the test is the minilitex one, wihtout DDR and Ethernet. Yosys prunes away the BUFG related to CLKOUT1 and CLKOUT2, and the SDC plugin discards those nets and does not output them in the final SDC, so that SDC is complete indeed.

What is missing though is also the CLKIN to the PLL which I am not sure why was not added.

@tmichalak
Copy link
Collaborator

You mean CLKIN1? Well, it's driven by some explicitly added clock thus is itself neither an explicit (added by create_clock) or generated (propagated from the explicitly added clock wire but with different clock parameters).

acomodi added a commit to antmicro/fpga-tool-perf that referenced this issue Dec 10, 2020
See issue:
chipsalliance/yosys-f4pga-plugins#64

Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
@mithro
Copy link
Collaborator

mithro commented Dec 14, 2020

Poke?

@mithro
Copy link
Collaborator

mithro commented Dec 14, 2020

What is the status here?

@tmichalak
Copy link
Collaborator

@acomodi from my perspective the current behavior is expected. So adding this exception would be VPR specific. @litghost thoughts?

@acomodi
Copy link
Contributor Author

acomodi commented Dec 14, 2020

The main issue comes from the fact that VPR is not able to propagate the clocks. In fact, this is what results when running in VPR:

  160 # Build Timing Graph took 0.04 seconds (max_rss 176.2 MiB, delta_rss +0.0 MiB)                                                                                                                                                                                                      
  161 Netlist contains 8 clocks                                                                                                                                                                                                                                                           
  162   Netlist Clock 'crg_clkin' Fanout: 9 pins (0.0%), 9 blocks (0.1%)                                                                                                                                                                                                                  
  163   Netlist Clock 'sys_clk' Fanout: 2125 pins (4.3%), 2092 blocks (20.2%)                                                                                                                                                                                                             
  164   Netlist Clock 'idelay_clk' Fanout: 8 pins (0.0%), 8 blocks (0.1%)                                                                                                                                                                                                                 
  165   Netlist Clock 'eth_clk' Fanout: 1 pins (0.0%), 1 blocks (0.0%)                                                                                                                                                                                                                    
  166   Netlist Clock 'pll_fb' Fanout: 1 pins (0.0%), 1 blocks (0.0%)                                                                                                                                                                                                                     
  167   Netlist Clock 'crg_clkout0' Fanout: 1 pins (0.0%), 1 blocks (0.0%)                                                                                                                                                                                                                
  168   Netlist Clock 'crg_clkout3' Fanout: 1 pins (0.0%), 1 blocks (0.0%)                                                                                                                                                                                                                
  169   Netlist Clock 'crg_clkout4' Fanout: 1 pins (0.0%), 1 blocks (0.0%)                                                                                                                                                                                                                
  170 # Load Timing Constraints                                                                                                                                                                                                                                                           
  171                                                                                                                                                                                                                                                                                     
  172 Applied 3 SDC commands from '/data/symbiflow/symbiflow-arch-defs/build/xc/xc7/tests/soc/litex/mini/minilitex_arty/artix7-xc7a50t-virt-xc7a50t-test/top_synth.sdc'                                                                                                                   
  173 Timing constraints created 3 clocks                                                                                                                                                                                                                                                 
  174   Constrained Clock 'crg_clkout0' Source: 'PLLE2_ADV.CLKOUT0[0]'                                                                                                                                                                                                                    
  175   Constrained Clock 'crg_clkout3' Source: 'PLLE2_ADV.CLKOUT3[0]'                                                                                                                                                                                                                    
  176   Constrained Clock 'crg_clkout4' Source: 'PLLE2_ADV.CLKOUT4[0]'  

The only constrained nets are the PLL clock output nets.
If the SDC lacks sys_clk, the entire logic of the LiteX core will be unconstrained at the moment.
I think that, for the time being we should output the SDC relative to all the clcock different clock netlists that are outputs of clock tiles (namely, BUFG, PLL, etc.).

As a longer term solution we might need to add capabilities in VPR to understand that a clock net that, for instance, passes through a BUFG, has the same constraint as the PLL output.
In this case sys_clk will have the same constraint applied to crg_clkout0 applied.

@litghost
Copy link
Contributor

litghost commented Dec 14, 2020

So I believe the fixes in #54 are good, but we do need a "verbose" SDC output for VPR. So make write_sdc have a -include_propigated_clock option, and we use that for VPR?'

@acomodi / @tmichalak I believe that this would resolve the issue?

@tmichalak
Copy link
Collaborator

@acomodi @litghost Adding a switch will work.

@mithro
Copy link
Collaborator

mithro commented Dec 14, 2020

@tmichalak - Yes, we most certainly want to be able to write a SDC file with all clocks propagated, that was one of the points of pulling the data into Yosys in the first place...

@tmichalak
Copy link
Collaborator

@acomodi @litghost @mithro I created a PR with the include_propagated_clocks switch: #65

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants