Sdtrig: Clarify that tinfo.version is a global constant, not per-trigger

[JanMatCodasip]

The purpose of "tinfo.version" field is apparently to describe the version of the whole Sdtrig implementation. That is, it is a constant unaffected by the current value of tselect CSR. Judging form the original commit that introduced it (0374fd6) and from the related mailing list discussion
(https://lists.riscv.org/g/tech-debug/topic/96888390).

However, the sentences

This register provides access to the trigger selected by tselect.
The reset values listed here apply to every underlying trigger.

leave certain room for interpretation that "tinfo.version" is a per-trigger constant, not a global one, and could therefore change depending on "tselect".

Remove this ambiguity by rewording the description of "tinfo" CSR and "tinfo.version" field.

[en-sc]

I believe this to be a valuable clarification. Tnanks!

[pdonahue-ventana]

The confusion is that "the version of the Sdtrig extension implemented" might change dynamically because part of the trigger module might support 0.13 and another part supports 1.0?

[JanMatCodasip]

Yes. The sentence This register [tinfo] provides access to the trigger selected by tselect. may lead the reader to believe that tinfo.version may differ for individual triggers. That would mean that some triggers follow the implementation prior to spec revision 5a5c078 whereas others follow the spec after this commit.

[rtwfroody]

To me "This register provides access to the trigger selected by tselect." does mean that tinfo.version applies per trigger, and this change would change that behavior.

That would make this change not backwards compatible.

[JanMatCodasip]

rtwfroody: To me "This register provides access to the trigger selected by tselect." does mean that tinfo.version applies per trigger

One may argue that the sentence tinfo.version - Contains the version of the Sdtrig extension implemented means that tinfo.version refers to the whole Sdtrig extension, and as such it contains a constant that does not change with tselect. (Or can there be more than one instance of the Sdtrig extension implemented in a RISC-V hart?)

Could we please reach an agreement what we would like tinfo.version to mean, and then describe in it in clear words in the spec. I favor constant tinfo.version but do not mind either of the two options as long as the spec is unambiguous.

[pdonahue-ventana]

You're right that different triggers cannot implement different versions of Sdtrig and be compliant with Sdtrig. Of course, you can have a non-conforming extension that does so, but that's not officially Sdtrig.

Here's what's currently specified:

• Each separate trigger has a separate version value (because tselect determines which trigger the entire tinfo is read from).
• The tinfo.version values of each trigger must match the version of Sdtrig that is implemented.
• It doesn't explicitly say that Sdtrig is a monolithic spec and that you can't partially implement one version and partially implement another.

I believe that you are mainly concerned about the last point. Your MR proposes:

• Part of the tinfo register depends on tselect and part of it does not.
• The part that does not is the version of Sdtrig that is implemented.
• There is a singular version of Sdtrig that is implemented.

@rtwfroody points out that the first bullet is incompatible with the behavior in the ratified spec. Practically speaking, the current spec and your proposal seem equivalent (except, possibly, if a future version does want to have per-trigger version fields for some reason). But if they're equivalent then it seems like there's not a reason to change a ratified spec.

[JanMatCodasip]

@pdonahue-ventana, thank you for your reply - understood. So if I rephrase your comment: the current text of the spec already requires that tinfo.version contains the same value for all triggers.

However, I still believe that this should be stated in a clearer way so that mis-interpretations (like discussed in this thread) are avoided.

I have changed this merge request to add an explanatory note for the tinfo.version field. Would this look all right to you?

[pdonahue-ventana]

That's OK with me. I have removed the "Backwards Incompatible Change" label.

Since the spec is ratified, there's a process for updates. Other specs have gathered up several clarifications and then sent a whole batch through the process. So expect this to lie dormant for a while.

[JanMatCodasip]

Thank you.

pdonahue-ventana: Since the spec is ratified, there's a process for updates. [...] So expect this to lie dormant for a while.

That sounds perfectly fine to me. After all, it is a minor clarification only.