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

[rv_dm] Fix RDC and DFT issue with resets #23621

Merged
merged 1 commit into from
Jun 11, 2024
Merged

Conversation

a-will
Copy link
Contributor

@a-will a-will commented Jun 10, 2024

Convert the JTAG combined resets to have a synchronous assertion timing in the core clock domain, placing them on the D input of the first flop of the prim_flop_2sync, instead of the reset inputs of both flops. This removes the asynchronous reset assertion that may cause RDC issues, and it restores scan reset as the only reset for those 2 flops when in scan mode.

Note that prim_fifo_async_simple guarantees that a prior transaction in flight will at least maintain its data values, since the holding register is not reset. This mechanism does not guarantee that the request will be squashed, but it does guarantee that the request will not be malformed by the reset. In addition, if the JTAG reset is held for long enough (i.e. >= the round-trip delay), the response for any previously in-flight request will be dropped.

Convert the JTAG combined resets to have a synchronous assertion timing
in the core clock domain, placing them on the D input of the first flop
of the prim_flop_2sync, instead of the reset inputs of both flops. This
removes the asynchronous reset assertion that may cause RDC issues, and
it restores scan reset as the only reset for those 2 flops.

Note that prim_fifo_async_simple guarantees that a prior transaction in
flight will at least maintain its data values, since the holding
register is *not* reset. This mechanism does not guarantee that the
request will be squashed, but it *does* guarantee that the request will
not be malformed by the reset. In addition, if the JTAG reset is held
for long enough (i.e. >= the round-trip delay), the response for any
previously in-flight request will be dropped.

Signed-off-by: Alexander Williams <[email protected]>
@a-will
Copy link
Contributor Author

a-will commented Jun 10, 2024

Note that the guarantee of not mangling the request, provided by prim_fifo_async_simple + long enough reset, is what's new since this was last proposed, in https://github.com/lowRISC/riscv-dbg/pull/5.

Copy link
Contributor

@vogelpi vogelpi left a comment

Choose a reason for hiding this comment

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

Thanks @a-will . I've read up on the linked issue (and related issues and PRs) and I think this is reasonable. However, I haven't been involved in most of previous discussions around RDC inside RV_DM. Ideally, also @msfschaffner or @andreaskurth could chime in.

@a-will a-will requested a review from moidx June 11, 2024 03:36
@a-will
Copy link
Contributor Author

a-will commented Jun 11, 2024

Please merge if there is agreement.

The failure is unrelated to this PR. The test was brought in via #23378, where it experienced merge skew and started failing when rebased atop 3238897 (from #23544)

@vogelpi
Copy link
Contributor

vogelpi commented Jun 11, 2024

Let's merge this to include it in the next RC following today. If additional review feedback comes in, we can still do a follow-up PR if needed. But based on the discussion in the linked issue and @a-will 's expertise with the matter, I don't think this will be necessary.

@vogelpi vogelpi merged commit bc05bca into lowRISC:master Jun 11, 2024
29 of 31 checks passed
@a-will a-will deleted the rv-dm-dft branch June 11, 2024 13:50
Copy link
Contributor

@msfschaffner msfschaffner left a comment

Choose a reason for hiding this comment

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

I believe there is a potential corner case where a TRST may be missed if

  1. the JTAG clock is faster than the RV_DM clock, and the TRST is not asserted for more than a cycle.
  2. or the RV_DM core clock is not enabled (e.g. during sleep).

These may be reasons why the combined reset has been made async in the first place.

Now, I think these corner cases are mostly theoretical and do not have any negative practical implications, given that we usually hold TRST low for many cycles, and since we can retry to TRST and re-connect after sleep exit.

The change clearly makes DFT insertion and CDC analysis more convenient, so merging LGTM!

@a-will
Copy link
Contributor Author

a-will commented Jun 29, 2024

Just some notes (reinforcing your reasoning here in one case):

I believe there is a potential corner case where a TRST may be missed if

  1. the JTAG clock is faster than the RV_DM clock, and the TRST is not asserted for more than a cycle.

A TRST minimum width in the spec is par for the course, though. Ideally, we would filter short pulses, even, so glitches don't count. At least we have the schmitt triggers. :)

  1. or the RV_DM core clock is not enabled (e.g. during sleep).

If we wanted to fix that, I don't think the answer is that async feedthrough reset, though. Probably, I'd choose to have this become a synchronous reset request that sticks until acknowledged by the RV_DM clock domain (via good, old prim_sync_reqack, perhaps). DMI would be blocked and maybe yield "busy" until it cleared.

We'd need something like an SR latch for the TRST_N pin for that, since the pin is async to any clock. Without the latch, I think I'd only have the TRST_N pin reset the TAP FSM, then leave DTM and DMI reset duties to just dtmcs.dmihardreset.

This should work because there are many more TCK cycles needed to set up a DMI request than are needed to clear prim_sync_reqack's round-trip.

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