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

[pinmux] Add strap sampling and TAP qualification logic #5301

Merged
merged 1 commit into from
Feb 20, 2021

Conversation

msfschaffner
Copy link
Contributor

This implements the strap sampling logic and the TAP qualification with life cycle signals.

Signed-off-by: Michael Schaffner [email protected]

@msfschaffner
Copy link
Contributor Author

Related feature tracker: #5221

@msfschaffner msfschaffner force-pushed the pinmux-straps branch 7 times, most recently from 53a4fa1 to 0a7cdf8 Compare February 19, 2021 19:42
@msfschaffner msfschaffner marked this pull request as ready for review February 19, 2021 19:49
@msfschaffner msfschaffner requested review from cindychip and weicaiyang and removed request for weicaiyang February 19, 2021 20:00
////////////////

// The strap sampling enable input shall be pulsed high exactly once after cold boot.
`ASSERT(PwrMgrStrapSampleOnce_A, strap_en_i |=> strong(!strap_en_i [*]))
Copy link
Contributor

@cindychip cindychip Feb 19, 2021

Choose a reason for hiding this comment

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

nice syntax!
I wonder if keyword strong is needed here.
I think strap_en_i |=> !strap_en_i [*], once found strap_en_i, from next cycle, strap_en_i has to stay low on every clock cycle until next reset.
Also a quick question: does the strap_en_i pulse only set high for one cycle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @cindychip, you are probably right that we do not need the strong modification here.
Let me remove that.
RE your question regarding the enable signal: yes, it is expected that this is a single cycle pulse.

Choose a reason for hiding this comment

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

dumb question from me... what would the strong modification imply here..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it ensures that the sequential property holds only if the sequence has matched (e.g., you can't have sequences that have started evaluation but have not matched yet at the end of a simulation).

this is needed for things like testing that eventually, an FSM returns to a certain state (especially in FPV, although simulation can sometimes also check this).

I placed it here although it was not necessary in this context, since the sequence always matches after pulse deassertion.

Copy link
Contributor

@cindychip cindychip 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 tagging me on the PR michael! I mainly read through and focused on the assertions. LGTM but would be good to have some RTL engineers approve it.

output jtag_pkg::jtag_req_t rv_jtag_o,
input jtag_pkg::jtag_rsp_t rv_jtag_i,
output jtag_pkg::jtag_req_t dft_jtag_o,
input jtag_pkg::jtag_rsp_t dft_jtag_i,

Choose a reason for hiding this comment

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

quick question, since the flash technically also has a jtag connection to pinmux (however it is muxed and nothing special), should i just do that as part of normal cio_'s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm... let me see... Afaik, the flash JTAG has a different LC qualification signal (the NVM debug one, right)? So we can't daisy chain the DFT TAP with the flash one?

We could technically add a strap pin bit to allow selection of other TAPs here as well...

Otherwise, if it is fine from a test-flow perspective that the JTAG is functionally muxed in the pinmux ( (and therefore requires a pinmux configuration), then we could go for that, yes.

Choose a reason for hiding this comment

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

yeah it's a different signal, and I do the controls for that directly in flash_phy already, so we don't have to expose anything to pinmux. Maybe that's better then, I just stick with the vanilla pinmux flow and we don't have to do anything extra here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok SGTM!

Copy link

@tjaychen tjaychen left a comment

Choose a reason for hiding this comment

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

lgtm with a few clarifying questions.
They don't need to be addressed in this PR.

@tjaychen
Copy link

tjaychen commented Feb 20, 2021 via email

@msfschaffner msfschaffner merged commit a706380 into lowRISC:master Feb 20, 2021
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