Skip to content

Conversation

@glaserf
Copy link
Contributor

@glaserf glaserf commented Nov 24, 2025

This PR addresses a rare issue where ack_o of the push_pull_interface could get stuck high when asserted at the exact same time that the interface is reset. For the valid/ready flavor of the interface, a similar mechanism is already there.

@glaserf glaserf requested a review from a team as a code owner November 24, 2025 09:22
@glaserf glaserf requested review from h-filali, marnovandermaas, martin-velay and rswarbrick and removed request for a team and marnovandermaas November 24, 2025 09:22
Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

I think this looks reasonable to me. Just for my understanding can you point me to the equivalent race check for the valid/ready interface? Also, what task is the one that is racing against the drive task?

Copy link
Contributor

@martin-velay martin-velay left a comment

Choose a reason for hiding this comment

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

It sounds like a good catch, thanks for the fix!

@glaserf
Copy link
Contributor Author

glaserf commented Nov 24, 2025

I think this looks reasonable to me. Just for my understanding can you point me to the equivalent race check for the valid/ready interface? Also, what task is the one that is racing against the drive task?

Sure:

// In case there is race condition between the logic above and reset_signals task.
// We always set the valid_int again to 0 to make sure the data comes out of reset is not
// valid.
if (cfg.in_reset) `CB.valid_int <= '0;

Maybe we can update the comment there, I think it says the same, just in a bit different words.

As for the task, I am not sure where the "reset_task" in question resides, maybe someone from the DV team can comment?

@martin-velay
Copy link
Contributor

This reset_signals is defined in the dv_base_driver and implemented in each child.

I cannot see the implementation for req_int in pull_host_driver, if we follow the same logic I guess we should do the same for this variable too.

And at some point we plan to improve the DV base library to be more "resetable" and probably change slightly the logic.

Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation, looks good to me.

@glaserf glaserf force-pushed the push_pull_ack_reset branch from 89966f7 to 8085ffe Compare November 28, 2025 09:37
@glaserf
Copy link
Contributor Author

glaserf commented Nov 28, 2025

Thanks for your feedback. I now added handling of this case to all four flavors of this protocol and reworded the pre-existing comment.

@rswarbrick can you please have a look and get it merged, if you don't see any issues?

@martin-velay
Copy link
Contributor

It looks good to me, thanks for the latest changes!

This commit ensures that the ack_o signal is de-asserted when the
interface is reset under all conditions. A similar condition already
exists for the valid/ready mode of the interface.

Signed-off-by: Florian Glaser <[email protected]>
@glaserf glaserf force-pushed the push_pull_ack_reset branch from 8085ffe to b3a7d95 Compare November 28, 2025 12:19
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.

3 participants