Using `PLLOUTGLOBAL` on PLL results in :Assertion failure"

cr1901 commented 6 years ago

Actual Behavior

The following Verilog module:

William@William-THINK MINGW64 ~/src/tinyfpga-soc
$ cat pll-crash/build/top.v
/* Machine-generated using Migen */
module top(
        input clk16,
        output out
);

wire sys_clk;
wire clk16_1;
wire clk32;

// Adding a dummy event (using a dummy signal 'dummy_s') to get the simulator
// to run the combinatorial process once at the beginning.
// synthesis translate_off
reg dummy_s;
initial dummy_s <= 1'd0;
// synthesis translate_on

assign clk16_1 = clk16;
assign out = sys_clk;

SB_PLL40_CORE #(
        .DIVF(6'd63),
        .DIVQ(3'd5),
        .DIVR(1'd0),
        .FILTER_RANGE(3'd1)
) SB_PLL40_CORE (
        .REFERENCECLK(clk16_1),
        .RESETB(1'd1),
        .PLLOUTGLOBAL(clk32)
);

SB_GB SB_GB(
        .USER_SIGNAL_TO_GLOBAL_BUFFER(clk32),
        .GLOBAL_BUFFER_OUTPUT(sys_clk)
);

endmodule

results in the following assertion failure:

William@William-THINK MINGW64 ~/src/tinyfpga-soc/pll-crash
$ python3 pll-crash.py
Warning: Found one of those horrible `(synopsys|synthesis) translate_off' comments.
Yosys does support them but it is recommended to use `ifdef constructs instead!
Info: Importing module top
Info: Rule checker, Verifying pre-placed design
Info: Checksum: 0x191aa3eb

Info: constrained 'clk16' to bel 'X9/Y33/io0'
Info: constrained 'out' to bel 'X0/Y30/io0'

Info: Packing constants..
Info: Promoting globals..
Info: Packing IOs..
Info: Packing LUT-FFs..
Info: Packing non-LUT FFs..
Info: Packing carries..
Info: Packing RAMs..
Info: Packing special functions..
Warning: PLL 'SB_PLL40_CORE' has unknown connected port 'PLLOUTGLOBAL' - ignoring
Info:   constrained PLL 'SB_PLL40_CORE_PLL' to X16/Y0/pll_3
Info:   constrained PLL 'SB_PLL40_CORE_PLL' to X16/Y33/pll_3
Info: Constraining chains...
Info: Checksum: 0x28c010d3

Info: Annotating ports with timing budgets for target frequency 16.00 MHz

This application has requested the Runtime to terminate it in an unusual way.
Please contact the application's support team for more information.
terminate called after throwing an instance of 'nextpnr_ice40::assertion_failure'
  what():  Assertion failure: port_fanin.empty() (../common/timing.cc:167)

Expected behavior

PLLOUTGLOBAL followed by SB_GB should be supported in nextpnr, even if it's an obsolete hint (?) to the pnr tool.

Other information

William@William-THINK MINGW64 ~/src/tinyfpga-soc
$ uname -a
MINGW64_NT-6.1 William-THINK 2.10.0(0.325/5/3) 2018-02-09 15:25 x86_64 Msys

MCVE

I have provided an MCVE: pll-crash.zip

q3k commented 6 years ago

First of all, whoops, these branches should be swapped so that we fail early instead of crashing later.

Second of all, seems like PLLOUTGLOBAL is not a real Bel port, same as PLLOUTCORE. I think we should support designs that use this port via the following logic:

IF the out port PLLOUTGLOBAL is used
THEN
- remap the port to PLLOUT_A
- IF if the PLLOUT_A port drives any cell that is not a SB_GB
- THEN insert an SB_GB between the PLLOUT_A and downhill cell ports

Similar to what we do with LOCK and ICESTORM_LCs...

@daveshah1 , what do you think?

daveshah1 commented 6 years ago

PLLOUTGLOBAL has a dedicated connection to a global network via the padin, as if an SB_GB_IO was placed at the relevant location. This isn't the same as following the PLL output with a SB_GB (in arachne and the vendor tools, PLLOUTGLOBAL followed by SB_GB will waste a precious global buffer, and this is correct behaviour imo).

The proper solution is to add support for padin, which requires small changes to the bitstream gen as these are "extra bits" not part of a tile, implement PLLOUTGLOBAL properly and SB_GB_IO at the same time.

q3k commented 6 years ago

@daveshah1 Makes sense! Thank you for the info.

I'll implement this properly and also add a warning if the design uses a PLLOUTGLOBAL followed by an SB_GB.

mmicko commented 6 years ago

Have added a temporary fix for this, since some additional things needed to be added for generated bitstream. Can you please check if https://github.com/YosysHQ/nextpnr/pull/76 works for you ?

cr1901 commented 6 years ago

@mmicko This solves the crash, but then I get the following warning:

Warning: PLL 'SB_PLL40_CORE' is using port PLLOUTGLOBAL but implementation does not actually use the global clock output of the PLL

Does the implementation refer to nextpnr or my design? Because I route PLLOUTGLOBAL directly to my intended system clock.

mmicko commented 6 years ago

Warning is due to missing part in implementation, afraid it needs to wait for David to be fully resolved. As I understood it should even work with this change but not fully.

On Sat, Sep 22, 2018, 20:24 William D. Jones notifications@github.com wrote:

@mmicko https://github.com/mmicko This solves the crash, but then I get the following warning:

Warning: PLL 'SB_PLL40_CORE' is using port PLLOUTGLOBAL but implementation does not actually use the global clock output of the PLL

Does the implementation refer to nextpnr or my design? Because I route PLLOUTGLOBAL directly to my intended system clock.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/YosysHQ/nextpnr/issues/69#issuecomment-423763675, or mute the thread https://github.com/notifications/unsubscribe-auth/ADdKSONXNB6UoVMR_5D_88WC7uiQJDvFks5udoB2gaJpZM4WZ1wx .

cr1901 commented 6 years ago

@mmicko Well, the crash is gone, but my generated bitstream doesn't actually work. I guess for now that means this particular issue is resolved, but PLL support is still in progress.

mmicko commented 6 years ago

Agree. It seams your other PLL issue is connected to global buffer bitstream handling as well.

smunaut commented 6 years ago

I've been looking into this and this is what I have so far :

https://github.com/YosysHQ/nextpnr/compare/master...smunaut:ice40_pll_global

The only relevant commits are:

ice40: Add GlobalNetowkrInfo in the chip database
ice40: Add support for PLL global outputs via PADIN
[WIP] Complete hack to make it work ...

The first two are looking good to me. Basically first add some more info to the chipdb and then actually use that information.

The general idea, as discussed on IRC, is that when the PLL global output is used, we create a 'fake' SB_GB cell and we force it to the location that will drive the same global network than the PLL output will drive. We also mark that SB_GB as being 'fake' and during bitstream generation, we use that info to set the extra bits that will actually padin. Doing it that way also ensures that this global network / global buffer won't be used by anything else. (Just had to move global promotion after special packing).

And this is what the two first commit do.

The remaining hack is needed because when the PLL doesn't have any output connected, the current bitstream generation code will disable it and the associated IO ...

I'm trying to work out how to deal with that properly but the current code seems to rely on the order the SB_IO and ICESTORM_PLL are processed in the loop over the cells in write_asc which is definitely not good since AFAICT, nothing guarantee this order ...

smunaut commented 6 years ago

I've updated my tree with my latest changes, cleaning it all up. I think it's ready to be reviewed / tested.

For the IO config what I ended up doing is :

Do a prescan of all the cells to find the PLL and collects the SB_IO that are used by that PLL. Either as clock input because it's a PAD type PLL. Or for the output. Note that it doesn't matter if there is anything actually connected to the output port or not, it's purely the PLL type that determines if an SB_IO is unavailable because of the PLL. (i.e. 1 or 2 outputs). Even if the output is global, the SB_IO is still unavailable.
During the normal scan of the cells, for SB_IO, if a cell is used as PAD input, we make sure to enable its input buffer. If the cell is used as part of the output path of the pll we make sure the user isn't trying to use the input as well. In both cases, we _donot configure PINTYPE[1:0] there, we omit them.
During the normal scan of the cell, for the PLL, we take all the SB_IO locations used for the output path and we make sure to set the PINTYPE[1:0] appropriately.

So the IE/REN bits are always configured as part of the SB_IO config. That's because it's only needed if that site is used as an actual input (either pad or otherwise) and in that case, there will be a SB_IO cell in the list of cells for that site.

The PINTYPE bits are configured during the PLL config because for some of the SB_IO used in the output path, it's possible we don't have any SB_IO that correspond to that site in the design.

YosysHQ / nextpnr