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

Support for IDDR and ODDR #1502

Merged
merged 35 commits into from
Aug 18, 2020
Merged

Support for IDDR and ODDR #1502

merged 35 commits into from
Aug 18, 2020

Conversation

mkurc-ant
Copy link
Collaborator

@mkurc-ant mkurc-ant commented May 25, 2020

This PR adds support for IDDR and ODDR primitives.

  • IDDR architecture
  • IDDR techmap(s)
  • IDDR fasm2bels
  • IDDR tests
  • ODDR architecture
  • ODDR techmaps
  • ODDR fasm2bels
  • ODDR tests
  • Timings for both
  • How to map IDDR to IDDR_2CLK
  • More pack patterns
  • Add a test that can be run on HW

The IDDR is added as another mode of the ILOGICE3 pb_type. The connection IDELAY->IDDR is modeled as a mux - no need for separate pb_types.

An ODDR can be used to drive either O or T of an (I)OBUFT(DS). Two independent ODDRs can be packed into the same OLOGIC site to drive O and T independently.

For that the OLOGICE3 pb_type has two modes: OSERDESE2 and OLOGIC. In the second mode there are two almost identical child pb_types - one for ODDR for OQ and one for ODDR for TQ. These pb_types can operate independently either in the ODDR mode or a pass-through mode. This allows to have all combinations of ODDR uses.

Depending on where an ODDR is used, different fasm features have to be emitted. Fortunately for O and T they have one-to-one correspondences. The same blif models for both could be used. They are however nested i different pb_types that provide necessary parameters to fasm features mappings.

For the updated OLOGICE3 model to work the T connection for OBUF had to be made explicit. The IOB architecture has been modified to have the OBUFT by default. That allowed 'O' and 'T' paths to be split and made fully independent.

The issue with Vivado routing OBUFT.T to const1 + enabling inversion while it is logically connected to const0 has been fixed according to the suggestion in the issue: #1514

@probot-autolabeler probot-autolabeler bot added lang-python Issue uses (or requires) Python language. lang-verilog Issue uses (or requires) Verilog language. lang-xml Issues uses (or requires) XML language. type-utils Issues is related to the scripts inside the repo. type-vpr labels May 25, 2020
@mkurc-ant mkurc-ant force-pushed the iddr_oddr branch 5 times, most recently from c113787 to 77eb3f4 Compare July 7, 2020 07:30
@mkurc-ant mkurc-ant changed the title [WIP] Support for IDDR and ODDR Support for IDDR and ODDR Jul 7, 2020
@mkurc-ant mkurc-ant linked an issue Jul 7, 2020 that may be closed by this pull request
@mkurc-ant
Copy link
Collaborator Author

I didn't see any CI errors besides #1400 so I rebased it on top of master. Now the CI should pass.

- Detects whether there is an OBUFT with T input routed to const1. If so
then checks whether the ZINV_T1 inverter is enabled in the neighboring
OLOGIC site. If both cases are true then the OBUFT is replaced with an
OBUF.
Copy link
Contributor

Choose a reason for hiding this comment

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

Good comment, thanks!

return

# Get the neighboring IOI3 tile prefix and loc
ioi_tile_loc = site.tile.rsplit("_", maxsplit=1)[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than doing a string replace here, why not walk 1 tile to the left or right? That would be more robust?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

data_rate = '"DDR"'
bel.parameters['DATA_RATE'] = data_rate

# TODO: There shouldn't be mixed width in FASM features.
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like it could crop up in a new fasm2bels failure in the future. Is it possible to add a failing test now to demostrate the issue you are mentioning here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@litghost You mean add it and then disable? (we want green CI). I can think of a test with data width of eg. 4 that cannot be decoded now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@litghost You mean add it and then disable? (we want green CI). I can think of a test with data width of eg. 4 that cannot be decoded now.

That is one way to go. I think the valid options are:

  • Create and issue, and attach a failing test case so that it can be fixed in the future
  • Create the failing test case and fix it in this PR

The TODO by itself is insufficient.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll go with an issue then. Solving it probably requires adjustment to prjxray fuzzers, and I think that this PR is already kind of big.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There: #1589

xc/xc7/techmap/cells_map.v Outdated Show resolved Hide resolved
@litghost
Copy link
Contributor

litghost commented Jul 8, 2020

This PR needs to be rebased.

@mkurc-ant
Copy link
Collaborator Author

mkurc-ant commented Jul 10, 2020

@litghost I've discovered an issue with handling of IDDR vs. IDDR_2CLK.

In the techmap I map IDDR to the two clock version by connecting both C and CB to the same signal and setting the IS_CB_INVERTER parameter. Since there are no bits for that I wonder how should I handle this. For an IDDR Vivado always show that both CLK and CLKB of the site are connected to the same net and that the CLKB inverter is enabled.

Similarly, there is an issue in fasm2bels - whenever I decode a IDDR_2CLK with C and CB connected to the same net, Vivado complains that this is illegal.

@acomodi You have dealt with ISERDES stuff which appears to be similar. Have you encountered issues with CLKB inversion ?

@acomodi
Copy link
Contributor

acomodi commented Jul 10, 2020

@mkurc-ant , as far as I understood with the ISERDES, the CLKB inverter is not enabled by any feature, with, or without it, the bitstream remains the same. What I have done in fasm2bels, at the time, was to add the IS_CLKB_INVERTED parameter to the ISERDES, but still assign the same net to CLK and CLKB. This avoids Vivado complaining about the clock signals being the same.

@mkurc-ant
Copy link
Collaborator Author

@acomodi Thanks, I'll try that.

@mkurc-ant
Copy link
Collaborator Author

@acomodi That did the trick! I now have IDDR and ODDR working in hardware as well as correct decoding in fasm2bels.

Although there is a fasm difference:

 HCLK_CMT_X8Y26.HCLK_CMT_CK_BUFHCLK9_USED

 HCLK_CMT_L_X106Y26.HCLK_CMT_CCIO0_ACTIVE
-HCLK_CMT_L_X106Y26.HCLK_CMT_CCIO0_USED
 HCLK_CMT_L_X106Y26.HCLK_CMT_CK_BUFHCLK6_USED
 HCLK_CMT_L_X106Y26.HCLK_CMT_CK_IN10.HCLK_CMT_MUX_CLK_PLL0

But I probably know the cause, we shouldn't emit CCIOn_USED feature for routes through HCLK_CMT_MUX_OUT_FREQ_REFn.

@probot-autolabeler probot-autolabeler bot added the type-docs Issue is related to documentation. label Jul 10, 2020
mkurc-ant and others added 25 commits August 13, 2020 11:18
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
…ey have OSERDES connected.

Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
This reverts commit 1a50773.

Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
…SERDES

Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Maciej Kurc <mkurc@antmicro.com>
Signed-off-by: Alessandro Comodi <acomodi@antmicro.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang-python Issue uses (or requires) Python language. lang-verilog Issue uses (or requires) Verilog language. lang-xml Issues uses (or requires) XML language. third-party type-docs Issue is related to documentation. type-utils Issues is related to the scripts inside the repo. type-vpr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for IDDR primitives
4 participants