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

jasper SLEC script changes #1003

Merged
merged 13 commits into from
Jun 18, 2024
Merged

jasper SLEC script changes #1003

merged 13 commits into from
Jun 18, 2024

Conversation

mret55
Copy link
Contributor

@mret55 mret55 commented Jun 12, 2024

Updated jasper SLEC script for parameters usage. Added assumption files and bind file scripts/slec/cadence folder.

Updated jasper SLEC script for parameters usage. Added assumption files and bind file scripts/slec/cadence folder.
Copy link
Member

@MikeOpenHWGroup MikeOpenHWGroup left a comment

Choose a reason for hiding this comment

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

  • Are the .DS_Store files required as inputs to Jasper, or are they tool artefacts? If they are artefacts, they should not be added to the repository.
  • Please add the appropriate license headers to the clear-text files. The following "two liner" is sufficient:
// Copyright 2024 <your-organization>
// SPDX-License-Identifier: Apache-2.0 WITH SHL-2.1

Added license headers
@MikeOpenHWGroup
Copy link
Member

Hi @mret55, thanks for this PR. I have requested a couple of changes.

Also, the Eclipse ECA check is failing, but I checked and Eclipse reports that There is a valid ECA on file for [email protected]. I will investigate.

@pascalgouedo pascalgouedo self-requested a review June 12, 2024 16:36
@MikeOpenHWGroup
Copy link
Member

Hi @mret55, the Eclipse ECA check is now passing.

scripts/slec/cadence/README.rtf Outdated Show resolved Hide resolved
Copy link

@pascalgouedo pascalgouedo left a comment

Choose a reason for hiding this comment

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

I only found assume files but no file containing row4 assertion.
Once added, it should be added to cv32e40p_tb_src.flist.

scripts/slec/tb_src/cv32e40p_bind2.sv Outdated Show resolved Hide resolved
scripts/slec/tb_src/data_assert2.sv Outdated Show resolved Hide resolved
scripts/slec/tb_src/insn_assert2.sv Outdated Show resolved Hide resolved
scripts/formal/fpv.tcl Show resolved Hide resolved
@mret55
Copy link
Contributor Author

mret55 commented Jun 17, 2024

I only found assume files but no file containing row4 assertion. Once added, it should be added to cv32e40p_tb_src.flist.

I have uncommented the row4 assertion from cv32e40p/scripts/formal/src/cv32e40p_controller_assert.sv.
This file is already part of cv32e40p/scripts/formal/cv32e40p_formal.flist

@mret55
Copy link
Contributor Author

mret55 commented Jun 17, 2024

Hi @pascalgouedo,
I responded to your comments and fixed them accordingly.
Please let me know if there are any additional questions.

@pascalgouedo
Copy link

I only found assume files but no file containing row4 assertion. Once added, it should be added to cv32e40p_tb_src.flist.

I have uncommented the row4 assertion from cv32e40p/scripts/formal/src/cv32e40p_controller_assert.sv. This file is already part of cv32e40p/scripts/formal/cv32e40p_formal.flist

Are data_assert2 and insn_assert2 really useful as they are duplicates of data_assert and insn_assert?
If you were able to reuse cv32e40p_controller_assert you should be able to reuse those 2 files?

@mret55
Copy link
Contributor Author

mret55 commented Jun 17, 2024

I only found assume files but no file containing row4 assertion. Once added, it should be added to cv32e40p_tb_src.flist.

I have uncommented the row4 assertion from cv32e40p/scripts/formal/src/cv32e40p_controller_assert.sv. This file is already part of cv32e40p/scripts/formal/cv32e40p_formal.flist

Are data_assert2 and insn_assert2 really useful as they are duplicates of data_assert and insn_assert? If you were able to reuse cv32e40p_controller_assert you should be able to reuse those 2 files?

Hi @pascalgouedo,
I think there are differences between the two. insn_assert.sv at cv32e40p/scripts/formal/src/ imports two packages (cv32e40p_pkg and cv32e40p_tracer_pkg), which are not there in insn_assert2.sv, which you had given me.

Also, these are used for 2 different applications. One in cv32e40p/scripts/formal/ is for FPV (property verification), while one in cv32e40p/scripts/slec/cadence/tb_src is for SLEC.

I will need to run tweak run.sh for those packages, remove existing files and rerun jasper again if we were to consolidate the files. Not sure, if I will be able to get around to doing that quick.

In its existing setup (as per this pull request), it works fine.
Maybe we can merge my current changes in dev for now, and do the consolidation later?

@MikeOpenHWGroup
Copy link
Member

Approving and merging as per @pascalgouedo request. (Note that I have no way to test this from my current location.)

@MikeOpenHWGroup MikeOpenHWGroup merged commit b2eebc5 into openhwgroup:dev Jun 18, 2024
3 checks passed
@pascalgouedo pascalgouedo added Component:Tool-and-build For issues in the tool and build flow (e.g. Makefile, FuseSoc, etc.) Component:Verif For issues in the verification environment or test cases (e.g. for testbench, C code, etc.) labels Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:Tool-and-build For issues in the tool and build flow (e.g. Makefile, FuseSoc, etc.) Component:Verif For issues in the verification environment or test cases (e.g. for testbench, C code, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants