Update pulp_riscv_dbg to pulp-platform/riscv-dbg@9155bfc #15436
Conversation
andreaskurth
requested review from
moidx,
msfschaffner and
tjaychen
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| endtask | ||
| endclass | ||
|
|
||
| endpackage |
There was a problem hiding this comment.
File must end with a newline. [Style: posix-file-endings] [posix-eof]
| /// They replace the functionality of `dmi_jtag_tap.sv`. The file is | ||
| /// pin-compatible so that by selecting the appropriate file for the target it | ||
| /// can be transparently managed without relying on tick defines. | ||
| module dmi_jtag_tap #( |
There was a problem hiding this comment.
Declared module does not match the first dot-delimited component of file name: "dmi_bscane_tap" [Style: file-names] [module-filename]
| // Solderpad Hardware License, Version 0.51, see LICENSE for details. | ||
| // SPDX-License-Identifier: SHL-0.51 | ||
|
|
||
| interface JTAG_DV ( |
There was a problem hiding this comment.
Interface names must use lower_snake_case naming convention and end with _if. [Style: interface-conventions] [interface-name-style]
|
|
||
| class jtag_driver #( | ||
| parameter int IrLength = 0, | ||
| parameter IDCODE = 'h1, |
There was a problem hiding this comment.
Explicitly define a storage type for every parameter and localparam, (IDCODE). [Style: constants] [explicit-parameter-storage-type]
| // abstracts the debug module | ||
| class riscv_dbg #( | ||
| parameter int IrLength = 5, | ||
| parameter IDCODE = 'h1, |
There was a problem hiding this comment.
Explicitly define a storage type for every parameter and localparam, (IDCODE). [Style: constants] [explicit-parameter-storage-type]
| assert(req.addr == req_mon.addr) else | ||
| $error("Invalid dmi request. Got address %0x instead of %0x.", req_mon.addr, req.addr); | ||
| assert(req.op == req_mon.op) else | ||
| $error("Invalid dmi request. Got op %0x instead of %0x.", req_mon.op, req.op);; |
There was a problem hiding this comment.
Do not use consecutive null statements like ';;'. [Style: redundant-semicolons] [forbid-consecutive-null-statements]
|
|
||
|
|
||
| // The DV interface additionally carries a clock signal. | ||
| interface DMI_BUS_DV #( |
There was a problem hiding this comment.
Interface names must use lower_snake_case naming convention and end with _if. [Style: interface-conventions] [interface-name-style]
There was a problem hiding this comment.
LG for vendored C code. Will leave others to approve RTL. Thanks @andreaskurth!
|
Would you please run a full rv_dm regression locally to see if there are any fallouts? |
Sure 👍 All tests except the ones based on Test Results
Failure Buckets
|
andreaskurth
force-pushed
the
rv_dm-update
branch
from
ed80396
to
c2db22f
Compare
Awesome, thanks! |
andreaskurth
force-pushed
the
rv_dm-update
branch
from
c2db22f
to
2ad2fe0
Compare
|
I'm struggling to waive the lint error
I added this: opentitan/hw/ip/rv_dm/lint/rv_dm.waiver Lines 84 to 86 in 2ad2fe0 but the tool still flags an error. We don't have access to that linter, and I don't want to flood CI with trial & error. Any inputs would be highly appreciated |
I remember a similar issue in the past. I think you need to change |
| @@ -0,0 +1,2 @@ | |||
| .bender/ | |||
There was a problem hiding this comment.
Do we usually exclude such files from the vendoring? I guess not.
There was a problem hiding this comment.
We can instruct the vendor tool to exclude these files by enumerating the exclusion paths in the *.vendor.hjson:
opentitan/hw/vendor/pulp_riscv_dbg.vendor.hjson
Lines 20 to 25 in bcab2e1
| @@ -0,0 +1,45 @@ | |||
| From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | |||
There was a problem hiding this comment.
There is already a lint fix patch. I think the new fixes of this patch could have been merged into the other patch. But in the interest of time, I think this is okay.
There was a problem hiding this comment.
Hm since we actually have a patch repository we should move the patches there if possible.
This will make it easier to modify&replay these patches in the future.
Sorry I don't want to unnecessarily draw this out - let me know if I can help with this.
| @@ -37,20 +37,20 @@ index 350c6c5..38d7b51 100644 | |||
| - logic resp_queue_empty; | |||
There was a problem hiding this comment.
Did you directly edit this file instead of regenerating the patch? In the interest of time, I think this is okay.
There was a problem hiding this comment.
I think it would still be good to use the patching repository for this so that this can easily be generated automatically. It looks like this is already set up in the vendor file:
opentitan/hw/vendor/pulp_riscv_dbg.vendor.hjson
Lines 14 to 18 in bcab2e1
So it would just be a matter of moving the patch to the ot branch here and invoking the vendor tool with the
--refresh-patches switch:
$ util/vendor.py --refresh-patches --update --commit
There was a problem hiding this comment.
I tried that but it failed. I think our patch repository does not include patches from at least the last two PRs updating hw/vendor/pulp_riscv_dbg: #12613, #12728. In the interest of time, I did not spend the effort to first update the patch repository and port the patches there. But if you think that should be done now, I'll do it.
There was a problem hiding this comment.
I understand. Your approach is reasonable. Let's move forward and leave the cleanup for another time.
There was a problem hiding this comment.
I created #15506 to track this
I think you need to escape brackets when using -regexp since they are otherwise interpreted. If no regex'ing is needed here, switching to -msg as @vogelpi suggested should do the trick. |
andreaskurth
force-pushed
the
rv_dm-update
branch
from
2ad2fe0
to
873953d
Compare
That did the trick, thanks for your help! |
Update code from upstream repository https://github.com/pulp- platform/riscv-dbg to revision 9155bfc3006589d255cbfc4aa4df83e1567acbf3 * Update CHANGELOG.md (bluew) * tb: Ignore comb warnings (bluew) * dmi_jtag: Update `dmi` `op` field based on DMI response (Andreas Kurth) * dmi_jtag: Add mechanism for capturing failed DM op (Andreas Kurth) * dmi_jtag: Set busy error only if no sticky error is set (Andreas Kurth) * tb: Fix make vcsify (bluew) * tb: Rewrite buggy openocd test script in Python (bluew) * tb/Makefile: Use tabs (bluew) * tb: Fix testbench build (bluew) * dm_mem: Clear state of hart upon ndmreset (Andreas Kurth) * dmi_jtag_tap: Bring all state to initial value in Test-Logic-Reset (Andreas Kurth) * dmi_jtag: Take DMI response into account for reads (Andreas Kurth) * dm_csrs: Return busy DMI response if SBA is busy (Andreas Kurth) * dm_csrs: Return busy DMI response if command is busy (Andreas Kurth) * dm_csrs: Put entire `dmi_resp_o` through FIFO (Andreas Kurth) * dmi_jtag_tap: Use generic tech cells (Luca Colagrande) * jtag_test: Add `read_dmi_exp_backoff()` and `sba_read_double()` functions (Luca Colagrande) * Fix r/s/t/u-reset commands (epsilon) * Fix for 64-bit accesses (Arjan Bink) * Alternative fix for pull request 27 (Arjan Bink) Signed-off-by: Andreas Kurth <[email protected]>
Signed-off-by: Andreas Kurth <[email protected]>
andreaskurth
force-pushed
the
rv_dm-update
branch
from
873953d
to
d1c9be2
Compare
|
Rebased on |
Update code from upstream repository https://github.com/pulp-platform/riscv-dbg to revision
9155bfc3006589d255cbfc4aa4df83e1567acbf3
This includes the fixes from upstream PRs pulp-platform/riscv-dbg#138, pulp-platform/riscv-dbg#139, and pulp-platform/riscv-dbg#140, which are relevant for M2.
The auto-generated overview of the changes is:
dmiopfield based on DMI response (Andreas Kurth)dmi_resp_othrough FIFO (Andreas Kurth)read_dmi_exp_backoff()andsba_read_double()functions (Luca Colagrande)