Skip to content

[proj] Debug Module D1 sign-off - #741

Draft
PRugg-Cap wants to merge 1 commit into
lowRISC:mainfrom
Capabilities-Limited:pdr32-debug-module-d1-signoff
Draft

PRugg-Cap wants to merge 1 commit into
lowRISC:mainfrom
Capabilities-Limited:pdr32-debug-module-d1-signoff

Conversation

@PRugg-Cap

Copy link
Copy Markdown

No description provided.

Comment thread doc/proj/debug_module.md
| Type | Item | Status | Note/Collaterals |
|---------------|----------------------------|--------|------------------|
| Documentation | SPEC_COMPLETED | Done | [Debug Module specification][block doc].
| Documentation | CSR_DEFINED | TODO | The DMI registers are defined here: [DMI registers][dmi-registers]. The memory-mapped registers from the core are defined here: [Debug Memory Map][bus-registers]. TODO: two additional registers are required for CHERI, to indicate the program buffer is in integral pointer mode or capability pointer mode. These are currently allocated offsets 0x120 and 0x128.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Should I update the docs to include these registers?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The CHERI registers can be added to the doc once we integrate the new block. For now it's fine to document the functionality of the current block without CHERI.

Comment thread doc/proj/debug_module.md
| RTL | IP_INSTANTIABLE | Done | It is instantiated in top chip system.
| RTL | PHYSICAL_MACROS_DEFINED_80 | TODO | ...
| RTL | FUNC_IMPLEMENTED | Done | Debug functionality is already impelemented and smoke tested within Mocha. Small changes are expected to support capability debugging.
| RTL | ASSERT_KNOWN_ADDED | TODO | ...

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Were these already done for OpenTitan somewhere?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Comment thread doc/proj/debug_module.md
| RTL | PHYSICAL_MACROS_DEFINED_80 | TODO | ...
| RTL | FUNC_IMPLEMENTED | Done | Debug functionality is already impelemented and smoke tested within Mocha. Small changes are expected to support capability debugging.
| RTL | ASSERT_KNOWN_ADDED | TODO | ...
| Code Quality | LINT_SETUP | TODO | ...

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I believe these are already being checked by the system linting?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, and I don't see any debug or JTAG related lint waives in this file: https://github.com/lowRISC/mocha/blob/main/hw/top_chip/lint/top_chip_system.vlt

Comment thread doc/proj/debug_module.md
### D1

The Debug Module used for D1 sign-off is revision [358f9011][debug-hash], the same as used in OpenTitan.
The sign-off checklist items are described in the [D1 design sign-off checklist][D1 checklist].

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oops, the "D1 checklist" link is broken

Comment thread doc/proj/debug_module.md
| RTL | CLKRST_CONNECTED | Done | `dm_top.sv` correctly connects `clk_i` and `rst_ni` to `dm_csrs.sv`, `dm_sba.sv`, and `dm_mem.sv`. `dm_csrs.sv` instantiates a `prim_fifo_sync` that connects up the right clock, but changes reset domain to `dmi_rst_ni`, with a comment confirming this is intended. `dm_mem.sv` and `dm_sba.sv` have no further submodules.
| RTL | IP_TOP | Done | This module is defined in `dm_top.sv`.
| RTL | IP_INSTANTIABLE | Done | It is instantiated in top chip system.
| RTL | PHYSICAL_MACROS_DEFINED_80 | TODO | ...

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The ROM size here, and the CHERI one shouldn't increase by that much right?

@marnovandermaas marnovandermaas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I left some initial comments here, please address the TODOs as best you can. The CHERI register documentation, ROM additions, etc. can be deferred to D2. Please also mark this as ready to review once the TODOs are done.

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.

2 participants