Skip to content

[doc] Fix mhpmevent reset values in performance counters doc - #2570

Merged
SamuelRiedel merged 1 commit into
lowRISC:masterfrom
h-filali:doc-mhpmevent-reset-values
Nov 6, 2026
Merged

SamuelRiedel merged 1 commit into
lowRISC:masterfrom
h-filali:doc-mhpmevent-reset-values

Conversation

@h-filali

@h-filali h-filali commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Update the event selector table with the actual reset values and separate the event ID from the selector bit.

Fixes #2550, reported by WLHsu0827.

@h-filali
h-filali requested a review from SamuelRiedel November 6, 2026 12:22
@github-actions

github-actions Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@SamuelRiedel SamuelRiedel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! Just a few minor unrelated nits while we're at it.

| ``mhpmevent12(h)`` | 0x32C | 0x0000_1000 | 12 |
+----------------------+-------------+-------------+--------------+
+----------------------+-------------+-------------+----------+-----+
| Event Selector | CSR Address | Reset Value | Event ID | Bit |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we just say "Value" instaed of "reset value" to not suggest this can be modified later?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair point, I'll change it.

+----------------------+-------------+-------------+----------+-----+
| Event Selector | CSR Address | Reset Value | Event ID | Bit |
+======================+=============+=============+==========+=====+
| ``mhpmevent3(h)`` | 0x323 | 0x0000_0001 | 3 | 0 |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The (h) should be removed here, no? We don't have an mhpmeventXh register.

The event selector CSRs ``mhpmevent3`` - ``mhpmevent31`` define which of these events are counted by the event counters ``mhpmcounter3(h)`` - ``mhpmcounter31(h)``.
If a specific bit in an event selector CSR is set to 1, this means that events with this ID are being counted by the counter associated with that selector CSR.
The event selector CSRs are hardwired and writes to them are ignored.
Each implemented event selector CSR ``mhpmeventX`` has exactly one bit set, bit X-3, i.e., ``mhpmevent3`` reads as ``0x1``, ``mhpmevent4`` reads as ``0x2``, and so on.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe add that it will read zero if not implemented.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

@h-filali
h-filali force-pushed the doc-mhpmevent-reset-values branch from 20f9abb to 4159120 Compare November 6, 2026 15:45
@h-filali

h-filali commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for your input @SamuelRiedel. All valid. Your points should be addressed now.

Update the event selector table with the actual reset values and
separate the event ID from the selector bit.

Fixes lowRISC#2550, reported by WLHsu0827.

Signed-off-by: Hakim Filali <hfilali@lowrisc.org>
@h-filali
h-filali force-pushed the doc-mhpmevent-reset-values branch from 4159120 to 0b8a75c Compare November 6, 2026 15:47

@SamuelRiedel SamuelRiedel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great. Thanks a lot for this!

@SamuelRiedel
SamuelRiedel added this pull request to the merge queue Nov 6, 2026
Merged via the queue into lowRISC:master with commit 626034d Nov 6, 2026
13 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Nov 6, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Docs: mhpmevent reset values in performance_counters.rst do not match RTL (1 << i vs 1 << (i-3))

2 participants