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

[WIP] Add QuickLogic EOS-S3 support #1758

Open
wants to merge 465 commits into
base: main
Choose a base branch
from
Open

Conversation

rw1nkler
Copy link
Contributor

@rw1nkler rw1nkler commented Nov 6, 2020

This PR adds support for QuickLogic devices. The codebase comes from the main QuickLogic repository, with additional changes that are required for proper integration with the SymbiFlow mainline.

At this point, these issues need to be addressed to provide the QuickLogic support:

I believe that the integration can be speeded up by replacing the Yosys and v2x conda packages in the CI. Then when all the changes are available in the main repository, we can remove the package substitution. In this PR, I proposed the necessary changes for this solution.

@rw1nkler rw1nkler changed the title Add QuickLogic support [WIP] Add QuickLogic support Nov 6, 2020
@rw1nkler
Copy link
Contributor Author

rw1nkler commented Nov 6, 2020

@litghost, can you provide any information on what should be added to integrate the QuickLogic support with SymbiFlow?
Probably, your help is also required to set up proper CI.

Do you think that the solution with overwriting certain packages temporarily in the CI is a good approach?
9a90d28#diff-d9eb2e25be835d9549f2c62e1b9717dba5bce9f76572de38ca4e595ff9c4c60dR17

I'm also aware of these DCO errors, I will do my best to resolve the problem.

get_rel_target(REL_QUICKLOGIC_FASM_NAME quicklogic_fasm ${QUICKLOGIC_FASM_NAME})
add_custom_target(
${REL_QUICKLOGIC_FASM_NAME}
DEPENDS ${QUICKLOGIC_FASM_OUTPUT}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use add_file_dependency, not directly depend on the output filename.

Copy link
Contributor

@litghost litghost left a comment

Choose a reason for hiding this comment

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

Anytime an add_custom_command depends on something that has add_file_target, it needs to use add_file_dependency, not just depending on the filename.

get_target_property(YOSYS_DEVICE_CELLS_MAP ${DEVICE_TYPE} CELLS_MAP)

if (NOT "${YOSYS_DEVICE_CELLS_SIM}" MATCHES ".*NOTFOUND")
get_file_target(YOSYS_DEVICE_CELLS_SIM_TARGET ${YOSYS_DEVICE_CELLS_SIM})
Copy link
Contributor

Choose a reason for hiding this comment

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

Just use "add_file_dependency" which does the same thing as these 3 lines?

@@ -13,6 +13,7 @@ dependencies:
- symbiflow::icestorm
- symbiflow::capnproto-java
- litex-hub::gcc-riscv64-elf-newlib
- python=3.7
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the drop to python 3.7?

Copy link
Contributor

@litghost litghost left a comment

Choose a reason for hiding this comment

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

Most of the new CMake functions need better documentation. At a minimum, what do they do and how to call them. A why would be good too

Copy link
Contributor

@litghost litghost left a comment

Choose a reason for hiding this comment

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

Please remove all * imports, e.g.:

quicklogic/common/utils/rr_utils.py:

from data_structs import *

Copy link
Contributor

@litghost litghost left a comment

Choose a reason for hiding this comment

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

This PR should probably be broken up, it is very large, and hard to review.

Suggestions for places to split the PR:

  • A PR to setup the basic directory structure and run v2x on Verilog. I believe this PR would be relatively straight forward, and the generated output would be all the pb_types, etc.
  • A PR adding the python support libraries
  • A PR adding the new CMake scripts

@litghost
Copy link
Contributor

litghost commented Nov 6, 2020

Do you think that the solution with overwriting certain packages temporarily in the CI is a good approach?
9a90d28#diff-d9eb2e25be835d9549f2c62e1b9717dba5bce9f76572de38ca4e595ff9c4c60dR17

I think that is a terrible idea.

@litghost
Copy link
Contributor

litghost commented Nov 6, 2020

@litghost, can you provide any information on what should be added to integrate the QuickLogic support with SymbiFlow?
Probably, your help is also required to set up proper CI.

I think the first step here is to make a small PR that adds the QuickLogic configuration and a dummy QL CI script that is an empty shell script. We can then add that config to kokoro. I then recommend we incrementally stage PR's, per my earlier suggestion.

I know this isn't a surprise, but it is practically impossible to review and merge a PR with 120k lines and ~500 files. We need to break this into small PRs with more modest goals. For example, having a PR that generates all the pb_types by invoking v2x is probably a good PR once we have the initial CI stuff setup.

@litghost
Copy link
Contributor

litghost commented Nov 6, 2020

I think the first step here is to make a small PR that adds the QuickLogic configuration and a dummy QL CI script that is an empty shell script. We can then add that config to kokoro. I then recommend we incrementally stage PR's, per my earlier suggestion.

Specifically this PR would have the following files:

.github/kokoro/continuous-ql.cfg
.github/kokoro/presubmit-ql.cfg
.github/kokoro/ql.sh
.github/kokoro/kokoro-cfg.py

I notice that in this PR you didn't modify kokoro-cfg.py, which seems wrong? presubmit-ql.cfg / continuous-ql.cfg should be generated from kokoro-cfg.py

@rw1nkler
Copy link
Contributor Author

rw1nkler commented Nov 9, 2020

This PR should probably be broken up, it is very large, and hard to review.

Ok, I will split the PR then into smaller chunks.

I think the first step here is to make a small PR that adds the QuickLogic configuration and a dummy QL CI script that is an empty shell script. We can then add that config to kokoro. I then recommend we incrementally stage PR's, per my earlier suggestion.

Here is the first PR with the initial CI setup: #1760

Suggestions for places to split the PR:

  • A PR to setup the basic directory structure and run v2x on Verilog. I believe this PR would be relatively straight forward, and the generated output would be all the pb_types, etc.

This PR will require all of the v2x PRs to be merged:
- chipsalliance/f4pga-v2x#75
- chipsalliance/f4pga-v2x#32

From the discussions in the v2x PRs, I see that still some of the issues are unresolved. What's more, the solutions proposed there will require a lot of changes in the existing QuickLogic support.

Signed-off-by: Maciej Kurc <[email protected]>
…e IP input is unconnected (no CLOCK pad)

Signed-off-by: Maciej Kurc <[email protected]>
Signed-off-by: Maciej Kurc <[email protected]>
kkumar23 and others added 26 commits November 9, 2020 15:15
Removing wpiflash, due to licensing terms

Signed-off-by: Kishor Kumar <[email protected]>
Syntax update symbiflo_write_fasm to symbiflow_write_fasm

Signed-off-by: Kishor Kumar <[email protected]>
Signed-off-by: Jan Kowalewski <[email protected]>
Signed-off-by: Kishor Kumar <[email protected]>
Signed-off-by: Kishor Kumar <[email protected]>
correcting command

Signed-off-by: Kishor Kumar <[email protected]>
Signed-off-by: Kishor Kumar <[email protected]>
adding openocd

Signed-off-by: Kishor Kumar <[email protected]>
Signed-off-by: Jan Kowalewski <[email protected]>
Signed-off-by: Robert Winkler <[email protected]>
Signed-off-by: Robert Winkler <[email protected]>
This changes enables the option of using custom v2x package.

Signed-off-by: Robert Winkler <[email protected]>
This change is required by the quicklogic-yosys conda package,
used in the quicklogic kokoro CI.

Signed-off-by: Robert Winkler <[email protected]>
@litghost
Copy link
Contributor

litghost commented Nov 9, 2020

This PR will require all of the v2x PRs to be merged:

From the discussions in the v2x PRs, I see that still some of the issues are unresolved. What's more, the solutions proposed there will require a lot of changes in the existing QuickLogic support.

Given your statement, that needs to be fixed first then? So order for now should be:

  • Get relevant v2x PR's merged
  • Start creating PR's for the relevant python scripts
  • Work on updating verilog per v2x PR changes
  • etc

@litghost
Copy link
Contributor

litghost commented Nov 9, 2020

@rw1nkler New QuickLogic job is up and running. I tested the presubmit job in #1762 and hopefully the master job will be green too.

@mkurc-ant mkurc-ant changed the title [WIP] Add QuickLogic support [WIP] Add QuickLogic EOS-S3 support Jun 14, 2021
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.

9 participants