Add specification of default cell pin values

[gatecat]

This adds structures for DeviceResources for the default values for cell pins that are missing or disconnected.

Previously the contract was that the synthesis tool would always give cell pins a value, but this has several problems:

• existing flows aren't doing this
• cells might want to be created by the PnR tool or anything else reading the interchange format
• sometimes, floating is a valid default value (for example certain dedicated pins) and the existing contract didn't recognise that
• setting all disconnected pins to ground is definitely not correct, vendor tools usually connect disconnected clock enables to Vcc for example.

[gatecat]

It's a bit tricky because the same fields are also being used to encode other info like inversion, but this is how nextpnr-nexus is encoding this information at the moment:

• https://github.com/YosysHQ/nextpnr/blob/2912860c9788033a7501726e77bb4962b394280d/nexus/pins.cc#L27
• https://github.com/YosysHQ/nextpnr/blob/2912860c9788033a7501726e77bb4962b394280d/nexus/arch.h#L767-L814

likewise for nextpnr-xilinx:

• https://github.com/daveshah1/nextpnr-xilinx/blob/d40ffba7b34d0bd6ae7d62d1bf849f8ec4d1da78/xilinx/pins.cc#L364-L626

GitHub

YosysHQ/nextpnr

nextpnr portable FPGA place and route tool. Contribute to YosysHQ/nextpnr development by creating an account on GitHub.

GitHub

YosysHQ/nextpnr

nextpnr portable FPGA place and route tool. Contribute to YosysHQ/nextpnr development by creating an account on GitHub.

GitHub

daveshah1/nextpnr-xilinx

Experimental flows using nextpnr for Xilinx devices - daveshah1/nextpnr-xilinx

[acomodi]

It's a bit tricky because the same fields are also being used to encode other info like inversion

In fact, a question I have is whether it made sense to have a unified CellPin parameters field, that would comprehend inversion and defaults when the pin is left unconnected.

[gatecat]

Yes, I think that makes sense

[gatecat]

Updated - this is a breaking change due to some renaming, I'm not sure how best we want to handle that wrt RapidWright and python-fpga-interchange changes?

[mithro]

@gatecat - I need to think a bit more about this. There are a couple of potential complications around this idea. I believe (?) we made an explicit decision that the place and route tools should not be doing transforms on the netlist which this kind of looks like and we have also generally eared on the side of being more strict as well. On the flip side, describing the "default state" of things seems like a potentially good idea too....

[clavin-xlnx]

@gatecat - Perhaps you have a broader perspective when seeing how things are handled through other architectures. However, from my understanding unused cell pins receive a default pin mapping through the CelBelMapping construct. Some of those defaults are dictated by the type of cell mapping used. Broadly, I can see three levels of specifying pin defaults:

1) Exceptional, situation-dependent cases where a particular use of a UNISIM necessitates a particular default setting. 2) A broader (architecture/vendor specific) mode of defaulting pins (e.g. dangling inputs connected to GND/VCC) to avoid DRC issues. 3) Hardware defaults where no specification is necessary (bitstream default settings that are implicit)

As you noted, the changes will break compatibility with previous versions. I'm ok to accommodate breaking changes if needed, but we probably want to make it an exceptional process. This topic, however, probably requires some more discussion before moving to that point.

[gatecat]

To give some more context here, consider running the following Verilog through Vivado:

module top();
(* keep, dont_touch *) RAMB18E1 ram_i ();
endmodule

despite there being no ports given on the RAMB18E1, in practice Vivado will connect some ports to ground, some to Vcc, and leave others floating (I appreciate in hardware connecting to Vcc and left floating are the same, although this may not be the same for non-Xilinx devices):

There is currently no logic in Yosys to do this, so it would pass a RAMB18E1 instantiation with no ports over to nextpnr (or whatever downstream tool you are using). Essentially there are two options to fix it:

• add logic and data to Yosys in order to support this
• add data to the interchange format to support this, and implement the logic to fixup the ports either during place and route (as we do for pre-existing nextpnr flows at the moment), or perhaps more practically in the Yosys JSON to logical netlist importer tool.

It's notable that the interchange format already includes default values for parameters (which Yosys could also handle, and indeed has the data for already), so including the default values for ports doesn't seem such a great step. This would also be needed if cells are created at (post-)PnR time - imagine the equivalent of doing create_cell in a post-PnR Vivado design.

[gatecat]

It's entirely possible to add this as a new field, separate from the inversion stuff as in the original version of this PR, which wouldn't be a breaking change btw - it's a question of whether we prefer a breaking change at this point, or a (possibly) slightly less neat format.

[mithro]

@gatecat - On the topic of breaking change verse slightly less neat format, I think we should just get into the habit of never making a breaking change if possible. We should be able to just retire the old inversion stuff and replace it with the new pin properties follow https://capnproto.org/language.html#evolving-your-protocol right?

Cap'n Proto: Schema Language

[clavin-xlnx]

@gatecat - Thanks for the RAMB18E1 example, I think I better understand what you are getting at. I can see that in this case there is a lack of default information when no user connectivity information is provided.

The straight forward location (for me, at least) to add the default information would be on the CellBelPinEntry, for example:

  # Map one cell pin to one BEL pin.
  # Note: There may be more than one BEL pin mapped to one cell pin.
  struct CellBelPinEntry {
    cellPin @0 : StringIdx $stringRef();
    belPin  @1 : StringIdx $stringRef();
    # *NEW*
    default @2 : CellPinValue; 
  }

  # These structures are used to define default constant values for unused
  # or missing cell pins
  enum CellPinValue {
    # leave floating
    float        @0;
    # connect to ground
    gnd          @1;
    # connect to vcc
    vcc          @2;
  }

This change would not break backwards compatibility and would provide the most relevant context to the information in my opinion.

[gatecat]

I think CellBelPinEntry is definitely a neat place to put this; however, the disadvantage with that is in terms of the flow order it is possible that you'd want to resolve these before doing cell-bel mappings, and therefore before the bel that the cell is being mapped to is known. If we decide that resolving these while doing cell-bel mappings is fair, then that would work though.

[mkurc-ant]

I am in favor of making the default value property of a cell pin and having them defined for cells rather than within CellBelPinEntry. That's because default pin connections are essentially cell properties.

If we don't want a breaking change we can define another list next to CellInversions, for example DefaultCellConnections or something similar and store the default values there.

[gatecat]

I am in favor of making the default value property of a cell pin and having them defined for cells rather than within CellBelPinEntry. That's because default pin connections are essentially cell properties.

If we don't want a breaking change we can define another list next to CellInversions, for example DefaultCellConnections or something similar and store the default values there.

Done, how does this look? This is no longer a breaking change at all either, which I think is probably better all things considered.

[mkurc-ant]

@gatecat Looks good to me.

[gatecat]

The addition could use a more comprehensive comment (see the rest of the file for examples).

I've improved the comments a bit in my most recent force-push

[gatecat]

@acomodi / @mkurc-ant do you have permissions on this repo to merge this?

[acomodi]

@gatecat Actually, I do not. I guess we need to wait for @mithro