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

modified run_phase -> main_phase #2458

Open
wants to merge 4 commits into
base: cv32e40p/dev
Choose a base branch
from

Conversation

subbu009
Copy link

As per issue #1933 , I have modified run_phase to main_phase to be in consistent with the use of other run time phases (reset_phase, confgure_phase) in the base test, firmware test and firmware riscof test.
Now the problem with line 154 in uvmt_cv32e40p_firmware_test.sv is statements below are waiting on posedge of resetn. If we use run_phase it works fine because run_phase starts at time 0 in parallel with reset_phase and senses the posedge of resetn whenever it happens. But when I use main_phase instead, I observed the line 154 waits on for resetn posedge and it never happens because the reset_phase has already completed. Due to this the test hangs and never finishes. So, to run ci_check, I have commented the line 154, and checks passed as expected. The question : is it okay if we do not wait for posedge of resetn? if we were to wait on reset, then it is better to fork off the current reset, configure and run phase tasks in only one run_phase so the reset posedge in one thread can be detected correctly by other forked thread.

On the other hand, if we would like to add on-the-fly reset feature as @MikeOpenHWGroup explained, then it is better to use reset, configure and main phases instead of run_phase.

@subbu009 subbu009 marked this pull request as draft May 31, 2024 09:43
@subbu009 subbu009 marked this pull request as ready for review May 31, 2024 09:43
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.

Hi @subbu009, this looks mostly good. I have a question about how the "first reset" is handled.

Hi @datum-dpoulin, can you comment?

@MikeOpenHWGroup
Copy link
Member

This all looks good to be @subbu009, and it passes a local CI regression. This branch is in active use by a team at Dolphin who are trying to close off coverage, so I will want their approval before merging.

@XavierAubert, @YoannPruvost, @dd-BeeNee, can one or more of you have a look?

@MikeOpenHWGroup
Copy link
Member

Great suggestion @dd-baoshan, thanks a lot!

I have approved this pull-request, so it can be merged at any time. However, I will defer to the Dolphin before doing so because I know the team is currently very focused on completing this project within the next couple of weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants