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

[mithro]

FWIW - There is guidance on how to evolve your protocol at https://capnproto.org/language.html#evolving-your-protocol

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

[mithro]

Given this is now backwards compatible and has dependent changes, I'm okay with moving forward.

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

[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

[mithro]

Looks good enough. If Chris confirms he is happy, go ahead and merge.

[clavin-xlnx]

Overall, I think I can live with this approach. I'm assuming that cell pin defaults don't change based on which BEL they might be placed on, but that is an underlying assumption.

I added a couple minor considerations/suggestions.

[clavin-xlnx]

I believe the python-fpga-interchange project depends on the $stringRef() annotation so that it can map the string enumerations into human-readable names. You'll probably want to add them to anything that is of type StringIdx. Example is on line 384 of the original file:

name @11 : StringIdx $stringRef();

[gatecat]

good catch, fixed

[clavin-xlnx]

I'm assuming we are intentionally being explicit here as I imagine (for Xilinx devices at least), the majority of pins will be float. If we think spending the extra space is worth it, I can live with this decision. Otherwise, we could just declare float as the implicit default.

[gatecat]

A small difference between float and no entry at all, is that float will create the cell pin if it's missing together but leave it floating which might matter in some cases and so I'd rather factor in.

In terms of space, I am not so sure that the majority of pins will be float. Several of the large hard IPs like the PS7/8/9 have a default of ground for the vast majority of their pins (and vcc for most of the others, iirc); and they have thousands of pins.

[clavin-xlnx]

That make sense, Let's keep it as is.

[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