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

[q3k]

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]

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]

@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]

Have added a temporary fix for this, since some additional things needed to be added for generated bitstream. Can you please check if #76 works for you ?

[cr1901]

@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]

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 ***@***.***> 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 <#69 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADdKSONXNB6UoVMR_5D_88WC7uiQJDvFks5udoB2gaJpZM4WZ1wx> .

[cr1901]

@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]

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

[smunaut]

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

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]

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 do_not 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.