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

Gowin. Add the ability to place registers in IOB #1403

Merged
merged 8 commits into from
Jan 1, 2025

Conversation

yrabbit
Copy link
Contributor

@yrabbit yrabbit commented Dec 8, 2024

IO blocks have registers: for input, for output and for OutputEnable signal - IREG, OREG and TREG respectively.

Each of the registers has one implicit non-switched wire, which one depends on the type of register (IREG has a Q wire, OREG has a D wire). Although the registers can be activated independently of each other they share the CLK, ClockEnable and LocalSetReset wires and this places restrictions on the possible combinations of register types in a single IO.

Register placement in IO blocks is enabled by specifying the command line keys --vopt ireg_in_iob, --vopt oreg_in_iob, or --vopt ioreg_in_iob.

It should be noted that specifying these keys leads to attempts to place registers in IO blocks, but no errors are generated in case of failure.

IO blocks have registers: for input, for output and for OutputEnable
signal - IREG, OREG and TREG respectively.

Each of the registers has one implicit non-switched wire, which one
depends on the type of register (IREG has a Q wire, OREG has a D wire).
Although the registers can be activated independently of each other they
share the CLK, ClockEnable and LocalSetReset wires and this places
restrictions on the possible combinations of register types in a single
IO.

Register placement in IO blocks is enabled by specifying the command
line keys --vopt ireg_in_iob, --vopt oreg_in_iob, or --vopt ioreg_in_iob.

It should be noted that specifying these keys leads to attempts to place
registers in IO blocks, but no errors are generated in case of failure.

Signed-off-by: YRabbit <[email protected]>
Check for unconnected ports.

Signed-off-by: YRabbit <[email protected]>
@whitequark
Copy link
Member

It should be noted that specifying these keys leads to attempts to place registers in IO blocks, but no errors are generated in case of failure.

What about warnings? If I want registers in IOBs I really want to know when it fails.

@yrabbit
Copy link
Contributor Author

yrabbit commented Dec 9, 2024

Yeah, I should probably rework the diagnostics.

If an attempt to place an FF in an IO block fails, issue a warning
detailing the reason for the failure, whether it is a register type
conflict, a network requirement violation, or a control signal conflict.

Signed-off-by: YRabbit <[email protected]>
Flipflops with a fixed ClockEnable input cannot coexist with flipflops
with a variable one.

Signed-off-by: YRabbit <[email protected]>
@gatecat
Copy link
Member

gatecat commented Dec 17, 2024

How do the vendor tools deal with this? Warning on every register that can't be packed into an IOB if the mode is enabled seems annoying in the case of a register that incidentally happens to be connected to an IO, but was never intended to be packed. Perhaps a per-IO/per-register attribute would be the best option, if there's nothing we can crib from the vendor tools.

@rowanG077
Copy link
Contributor

Xilinx does use an attribute: https://adaptivesupport.amd.com/s/article/66668?language=en_US

@yrabbit
Copy link
Contributor Author

yrabbit commented Dec 17, 2024

How do the vendor tools deal with this? Warning on every register that can't be packed into an IOB if the mode is enabled seems annoying in the case of a register that incidentally happens to be connected to an IO, but was never intended to be packed. Perhaps a per-IO/per-register attribute would be the best option, if there's nothing we can crib from the vendor tools.

Vendor tool does not react in any way in case the mode is set to place registers in IO but it did not work.

On the other hand, there is a .CST file construct that specifies that a particular register should be placed in a particular IO block. In practice this is almost useless because you have to specify the name of the pimitive, i.e. an explicit DFF construct.

012

@rowanG077
Copy link
Contributor

rowanG077 commented Dec 17, 2024

Vivado does give a warning afaik. I don't have it installed currently so can't check. But googling says it does show something like:

CRITICAL WARNING: [Place 30-73] Invalid constraint on register 'i_system_wrapper/system_i/sc_fxgen_1/U0/fxout2_reg[0]'. It has the property IOB=TRUE, but it is not driving or driven by any IO element.

Apologies if you are only referring to the gowin tools, it's not clear if you meant only that.

Placement modes are still specified by the command line keys
ireg_in_iob/oreg_in_iob/ioreg_in_iob, but also introduces more granular
control in the form of attributes at I/O ports:

  (* NOIOBFF *) - registers are never placed in this IO,

  (* IOBFF *) - registers must be placed in this IO, in case of failure
  a warning (not an error) with the reason for nonplacement is issued,

  _attribute_absence_ - no diagnostics will be issued: managed to place - good, failed - not bad either.

Signed-off-by: YRabbit <[email protected]>
@yrabbit
Copy link
Contributor Author

yrabbit commented Dec 18, 2024

Placement modes are still specified by the command line keys
ireg_in_iob/oreg_in_iob/ioreg_in_iob, but also introduces more granular
control in the form of attributes at I/O ports:

  (* NOIOBFF *) - registers are never placed in this IO,

  (* IOBFF *) - registers must be placed in this IO, in case of failure
  a warning (not an error) with the reason for nonplacement is issued,

  _attribute_absence_ - no diagnostics will be issued: managed to place - good, failed - not bad either.

like:

module top (
    input clk,
    input key_i,
    input rst_i,
    (* IOBFF *) output [`LEDS_NR-1:0] led
);

@whitequark
Copy link
Member

    (* IOBFF *) output [`LEDS_NR-1:0] led

Would it be possible to make the attribute bit-granular? Otherwise it is not possible to pack only some FFs into IOBs in an output like this. It's been causing issues for Amaranth.

Another option to solve this is to also accept the attribute on explicit IO buffer instances.

@yrabbit
Copy link
Contributor Author

yrabbit commented Dec 18, 2024

Another option to solve this is to also accept the attribute on explicit IO buffer instances.

Well I'm in favour of the latter option as it just works already now without any changes.

shot-19

@whitequark
Copy link
Member

Ah I didn't realize. That is preferred.


// output reg in IO
CellInfo *iologic_o = nullptr;
if ((ci.type == id_OBUF && ctx->settings.count(id_OREG_IN_IOB)) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that FFs with the IOBFF attribute set should be packed into IO buffers regardless of the nextpnr configuration, this is perhaps the most useful approach when you only have a few flipflops that you are certain you want placed in them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Change the logic for handling command line keys and attributes -
attributes allow routines to be placed in IO regardless of global mode.

Signed-off-by: YRabbit <[email protected]>

// output enable reg in IO
if (ci.type == id_IOBUF && (ctx->settings.count(id_IOREG_IN_IOB) || ci.attrs.count(id_IOBFF))) {
while (1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, the while loop here is just to get a way of being able to use break - imo do { } while (false); would convey the semantics here better.

@gatecat gatecat merged commit c565e36 into YosysHQ:master Jan 1, 2025
8 checks passed
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

Successfully merging this pull request may close these issues.

4 participants