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

[hw/pulp_riscv_dbg] Fix reset value of tap_state_q #24018

Merged

Conversation

andreaskurth
Copy link
Contributor

@andreaskurth andreaskurth commented Jul 15, 2024

This fixes upstream issue pulp-platform/riscv-dbg#165 by applying upstream PR pulp-platform/riscv-dbg#169. The change gets applied in a patch to avoid re-vendoring, which would be inconvenient at the current time.

This also reverts commit bd76d92 "[jtag,dv] Allow the jtag monitor to "reset" to a different TAP state" and commit 3b41400 "[rv_dm,dv] Teach environment about TAP's reset FSM state", which together replicated this problem in our DV code.

This fixes #24019.

This fixes upstream issue pulp-platform/riscv-dbg#165 by applying
upstream PR pulp-platform/riscv-dbg#169.  The change gets applied in a
patch to avoid re-vendoring, which would be inconvenient at the current
time.

This also reverts commit bd76d92
"[jtag,dv] Allow the jtag monitor to "reset" to a different TAP state"
and commit 3b41400 "[rv_dm,dv] Teach
environment about TAP's reset FSM state", which together replicated this
problem in our DV code.

Signed-off-by: Andreas Kurth <adk@lowrisc.org>
@andreaskurth andreaskurth self-assigned this Jul 15, 2024
@andreaskurth andreaskurth requested a review from a team as a code owner July 15, 2024 16:53
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.

LGTM, thanks @andreaskurth !

@vogelpi
Copy link
Contributor

vogelpi commented Jul 15, 2024

CHANGE AUTHORIZED: hw/vendor/pulp_riscv_dbg/src/dmi_jtag_tap.sv

@vogelpi vogelpi added the ECO Accepted as an ECO label Jul 15, 2024
@andreaskurth
Copy link
Contributor Author

CHANGE AUTHORIZED: hw/vendor/pulp_riscv_dbg/src/dmi_jtag_tap.sv

@andreaskurth
Copy link
Contributor Author

I just ran a full block-level regression, and results seem comparable to current nightlies:

|  Stage  |                   Name                    | Tests                            |  Max Job Runtime  |  Simulated Time  |  Passing  |  Total  |  Pass Rate  |                                                                                                 
|:-------:|:-----------------------------------------:|:---------------------------------|:-----------------:|:----------------:|:---------:|:-------:|:-----------:|                                                                                                 
|   V1    |                   smoke                   | rv_dm_smoke                      |      2.440s       |     2.507ms      |     2     |    2    |  100.00 %   |                                                                                                 
|   V1    |           jtag_dtm_csr_hw_reset           | rv_dm_jtag_dtm_csr_hw_reset      |      2.140s       |    858.782us     |     5     |    5    |  100.00 %   |                                                                                                 
|   V1    |              jtag_dtm_csr_rw              | rv_dm_jtag_dtm_csr_rw            |      2.890s       |    667.699us     |    20     |   20    |  100.00 %   |                                                                                                 
|   V1    |           jtag_dtm_csr_bit_bash           | rv_dm_jtag_dtm_csr_bit_bash      |      1.136m       |     23.593ms     |     5     |    5    |  100.00 %   |                                                                                                 
|   V1    |           jtag_dtm_csr_aliasing           | rv_dm_jtag_dtm_csr_aliasing      |      3.530s       |     1.372ms      |     5     |    5    |  100.00 %   |                                                                                                 
|   V1    |           jtag_dmi_csr_hw_reset           | rv_dm_jtag_dmi_csr_hw_reset      |      20.720s      |     11.143ms     |     5     |    5    |  100.00 %   |                                                                                                 
|   V1    |              jtag_dmi_csr_rw              | rv_dm_jtag_dmi_csr_rw            |      18.860s      |     11.337ms     |    20     |   20    |  100.00 %   |                                                                                                 
|   V1    |           jtag_dmi_csr_bit_bash           | rv_dm_jtag_dmi_csr_bit_bash      |      2.078m       |     59.706ms     |    20     |   20    |  100.00 %   |                                                                                                 
|   V1    |           jtag_dmi_csr_aliasing           | rv_dm_jtag_dmi_csr_aliasing      |      3.935m       |     89.818ms     |     5     |    5    |  100.00 %   |                                                                                                 
|   V1    |           jtag_dmi_cmderr_busy            | rv_dm_cmderr_busy                |      3.120s       |    915.674us     |     2     |    2    |  100.00 %   |                                                                                                 
|   V1    |       jtag_dmi_cmderr_not_supported       | rv_dm_cmderr_not_supported       |      1.720s       |    414.110us     |     2     |    2    |  100.00 %   |                                                                                                 
|   V1    |             cmderr_exception              | rv_dm_cmderr_exception           |      4.310s       |     3.345ms      |     2     |    2    |  100.00 %   |                                                                                                 
|   V1    |          mem_tl_access_resuming           | rv_dm_mem_tl_access_resuming     |      1.260s       |    195.522us     |     2     |    2    |  100.00 %   |                                                                                                 
|   V1    |           mem_tl_access_halted            | rv_dm_mem_tl_access_halted       |      1.260s       |     1.063ms      |     2     |    2    |  100.00 %   |
|   V1    |            cmderr_halt_resume             | rv_dm_cmderr_halt_resume         |      2.830s       |    780.722us     |     2     |    2    |  100.00 %   |                                                                                                 
|   V1    |            dataaddr_rw_access             | rv_dm_dataaddr_rw_access         |      1.140s       |    348.886us     |     2     |    2    |  100.00 %   |
|   V1    |                halt_resume                | rv_dm_halt_resume_whereto        |      1.370s       |     1.042ms      |     2     |    2    |  100.00 %   |
|   V1    |               progbuf_busy                | rv_dm_progbuf_busy               |      2.040s       |    718.817us     |     2     |    2    |  100.00 %   |                                                         
|   V1    |            abstractcmd_status             | rv_dm_abstractcmd_status         |      2.680s       |    618.383us     |     2     |    2    |  100.00 %   |                                                                                                 
|   V1    |        progbuf_read_write_execute         | rv_dm_progbuf_read_write_execute |      1.310s       |    814.072us     |     2     |    2    |  100.00 %   |
|   V1    |             progbuf_exception             | rv_dm_cmderr_exception           |      4.310s       |     3.345ms      |     2     |    2    |  100.00 %   |                                                                                                 
|   V1    |              rom_read_access              | rv_dm_rom_read_access            |      0.850s       |    137.029us     |     2     |    2    |  100.00 %   |                                                                                                 
|   V1    |               csr_hw_reset                | rv_dm_csr_hw_reset               |      2.500s       |    394.229us     |     5     |    5    |  100.00 %   |
|   V1    |                  csr_rw                   | rv_dm_csr_rw                     |      2.650s       |    181.606us     |    20     |   20    |  100.00 %   |                                                                                                 
|   V1    |               csr_bit_bash                | rv_dm_csr_bit_bash               |      53.270s      |     1.458ms      |     5     |    5    |  100.00 %   |
|   V1    |               csr_aliasing                | rv_dm_csr_aliasing               |      1.461m       |     5.575ms      |     5     |    5    |  100.00 %   |                                                                                                 
|   V1    |        csr_mem_rw_with_rand_reset         | rv_dm_csr_mem_rw_with_rand_reset |      11.130s      |     4.920ms      |    20     |   20    |  100.00 %   |
|   V1    | regwen_csr_and_corresponding_lockable_csr | rv_dm_csr_aliasing               |      1.461m       |     5.575ms      |     5     |    5    |  100.00 %   |
|         |                                           | rv_dm_csr_rw                     |      2.650s       |    181.606us     |    20     |   20    |  100.00 %   |
|   V1    |                 mem_walk                  | rv_dm_mem_walk                   |      1.060s       |    139.473us     |     5     |    5    |  100.00 %   |
|   V1    |            mem_partial_access             | rv_dm_mem_partial_access         |      0.820s       |     64.849us     |     5     |    5    |  100.00 %   |                                                                                                 
|   V1    |                                           | **TOTAL**                        |                   |                  |    176    |   176   |  100.00 %   |                                                                                                 
|   V2    |                  idcode                   | rv_dm_smoke                      |      2.440s       |     2.507ms      |     2     |    2    |  100.00 %   |                                                                                                 
|   V2    |            jtag_dtm_hard_reset            | rv_dm_jtag_dtm_hard_reset        |      4.130s       |     1.397ms      |     2     |    2    |  100.00 %   |
|   V2    |            jtag_dtm_idle_hint             | rv_dm_jtag_dtm_idle_hint         |      2.090s       |    731.076us     |     2     |    2    |  100.00 %   |                                                                                                 
|   V2    |            jtag_dmi_failed_op             | rv_dm_dmi_failed_op              |      1.680s       |    363.982us     |     2     |    2    |  100.00 %   |
|   V2    |           jtag_dmi_dm_inactive            | rv_dm_jtag_dmi_dm_inactive       |      2.040s       |    885.029us     |     2     |    2    |  100.00 %   |                                                                                                 
|   V2    |                    sba                    | rv_dm_sba_tl_access              |      26.660s      |     8.185ms      |    20     |   20    |  100.00 %   |
|         |                                           | rv_dm_delayed_resp_sba_tl_access |      20.950s      |     13.853ms     |    20     |   20    |  100.00 %   |                                                                                                 
|   V2    |                  bad_sba                  | rv_dm_bad_sba_tl_access          |      43.200s      |     14.075ms     |    20     |   20    |  100.00 %   |
|   V2    |             sba_autoincrement             | rv_dm_autoincr_sba_tl_access     |      2.842m       |    160.385ms     |    20     |   20    |  100.00 %   |
|   V2    |          jtag_dmi_debug_disabled          | rv_dm_jtag_dmi_debug_disabled    |      1.010s       |    542.156us     |     2     |    2    |  100.00 %   |                                                                                                 
|   V2    |            sba_debug_disabled             | rv_dm_sba_debug_disabled         |      3.530s       |     1.624ms      |     2     |    2    |  100.00 %   |
|   V2    |               ndmreset_req                | rv_dm_ndmreset_req               |      1.000s       |    455.040us     |     2     |    2    |  100.00 %   |                                                                                                 
|   V2    |               hart_unavail                | rv_dm_hart_unavail               |      2.210s       |    996.622us     |     5     |    5    |  100.00 %   |
|   V2    |           tap_ctrl_transitions            | rv_dm_tap_fsm                    |      5.760s       |     3.053ms      |     0     |    1    |   0.00 %    |
|         |                                           | rv_dm_tap_fsm_rand_reset         |      2.258m       |     49.428ms     |     9     |   10    |   90.00 %   |
|   V2    |                stress_all                 | rv_dm_stress_all                 |      37.930s      |     16.836ms     |    36     |   50    |   72.00 %   |
|   V2    |                alert_test                 | rv_dm_alert_test                 |      1.100s       |    132.631us     |    50     |   50    |  100.00 %   |
|   V2    |           tl_d_oob_addr_access            | rv_dm_tl_errors                  |      5.430s       |    277.495us     |    20     |   20    |  100.00 %   |
|   V2    |            tl_d_illegal_access            | rv_dm_tl_errors                  |      5.430s       |    277.495us     |    20     |   20    |  100.00 %   |                            
|   V2    |          tl_d_outstanding_access          | rv_dm_csr_aliasing               |      1.461m       |     5.575ms      |     5     |    5    |  100.00 %   |
|         |                                           | rv_dm_csr_hw_reset               |      2.500s       |    394.229us     |     5     |    5    |  100.00 %   |                                                                                                 
|         |                                           | rv_dm_csr_rw                     |      2.650s       |    181.606us     |    20     |   20    |  100.00 %   |
|         |                                           | rv_dm_same_csr_outstanding       |      9.520s       |     6.289ms      |    20     |   20    |  100.00 %   |
|   V2    |            tl_d_partial_access            | rv_dm_csr_aliasing               |      1.461m       |     5.575ms      |     5     |    5    |  100.00 %   |
|         |                                           | rv_dm_csr_hw_reset               |      2.500s       |    394.229us     |     5     |    5    |  100.00 %   |
|         |                                           | rv_dm_csr_rw                     |      2.650s       |    181.606us     |    20     |   20    |  100.00 %   |
|         |                                           | rv_dm_same_csr_outstanding       |      9.520s       |     6.289ms      |    20     |   20    |  100.00 %   |                                                                                                 
|   V2    |                                           | **TOTAL**                        |                   |                  |    234    |   250   |   93.60 %   |                            
|   V2S   |                tl_intg_err                | rv_dm_sec_cm                     |      3.540s       |     2.358ms      |     5     |    5    |  100.00 %   |
|         |                                           | rv_dm_tl_intg_err                |      23.850s      |     8.996ms      |    20     |   20    |  100.00 %   |
|   V2S   |           sec_cm_bus_integrity            | rv_dm_tl_intg_err                |      23.850s      |     8.996ms      |    20     |   20    |  100.00 %   |
|   V2S   |    sec_cm_lc_hw_debug_en_intersig_mubi    | rv_dm_sba_debug_disabled         |      3.530s       |     1.624ms      |     2     |    2    |  100.00 %   |
|         |                                           | rv_dm_debug_disabled             |      0.890s       |     80.225us     |     2     |    2    |  100.00 %   |
|   V2S   |                                           | **TOTAL**                        |                   |                  |    27     |   27    |  100.00 %   |
|   V3    |        stress_all_with_rand_reset         | rv_dm_stress_all_with_rand_reset |      59.760s      |     3.492ms      |     0     |   10    |   0.00 %    |
|   V3    |                                           | **TOTAL**                        |                   |                  |     0     |   10    |   0.00 %    |
|         |                                           | **TOTAL**                        |                   |                  |    437    |   463   |   94.38 %   |

jtag_tap_fsm passed 0/1, so I re-ran it separately with more seeds and got 19/28 (current nightlies pass 1/1).

@andreaskurth andreaskurth merged commit 663be28 into lowRISC:master Jul 15, 2024
33 checks passed
@andreaskurth andreaskurth deleted the fix-dmi_jtag_tap-tap_state_q branch July 15, 2024 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ECO Accepted as an ECO
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[hw/pulp_riscv_dbg] Reset value of tap_state_q is incorrect
2 participants