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

Doc edits #29

Merged
merged 17 commits into from
May 23, 2024
Merged

Doc edits #29

merged 17 commits into from
May 23, 2024

Conversation

kersten1
Copy link
Contributor

@kersten1 kersten1 commented May 14, 2024

Added some edits. Use "can" or "might" rather than "may" because "may" generally indicates permission. I added in a few commas. Finally, if you use the words "this, that, those, or these" you need to have a noun after them. Otherwise, they are hard to translate and also can lead to confusion as to what "this" is referring to.

Also, I found this para in the intro.adoc to be confusing:
The CBQRI registers are defined so that the software can perform two individual 4 byte accesses, or the hardware can perform two independent 4 byte transactions, resulting from an 8 byte access, to the high and low halves of the register as long as the register semantics, with regards to side-effects, are respected between the two software accesses, or two hardware transactions, respectively.

I took out the two clauses. If those clauses are needed, let me know and I'll take another pass.

One of the general complaints that we've gotten for docs is that we do not fully explain the fields in a register. So for example, In qos_bandwidth.adoc after the wavedrom, VER and NBWBLKS are defined really well. But the descriptions tail off then and I'm not quite sure what RPFX and P are, even though they are mentioned in paragraph. Not sure what the rest are either. Can you add more description here?

Same with next register and those in qos_capacity.adoc.

The term access-type is used throughout. I changed most of them to be access-type identifier. Another choice could be access type. The hyphen makes access-type an adjective so it needs a noun. Continuing with this idea. I found access-type (AT) indicator in the qos_identifiers.adoc file and Access-type identifiers in the qos_intro.adoc file. Are these two different things?

Added some edits. Use "can" or "might" rather than "may" because "may" generally indicates permission. I added in a few commas. Finally, if you use the words "this, that, those, or these" you need to have a noun after them. Otherwise, they are hard to translate and also can lead to confusion as to what "this" is referring to.

Also, I found this para: 
The CBQRI registers are defined so that the software can perform two
individual 4 byte accesses, or the hardware can perform two independent 4 byte
transactions, resulting from an 8 byte access, to the high and low halves of the
register as long as the register semantics, with regards to side-effects, are
respected between the two software accesses, or two hardware transactions,
respectively.

to be confusing until I took out the two clauses. If those clauses are needed, let me know and I'll take another pass.

Signed-off-by: Kersten Richter <[email protected]>
qos_intro.adoc Outdated Show resolved Hide resolved
Signed-off-by: Kersten Richter <[email protected]>
qos_intro.adoc Outdated Show resolved Hide resolved
Signed-off-by: Kersten Richter <[email protected]>
Signed-off-by: Kersten Richter <[email protected]>
Signed-off-by: Kersten Richter <[email protected]>
Signed-off-by: Kersten Richter <[email protected]>
Signed-off-by: Kersten Richter <[email protected]>
Signed-off-by: Kersten Richter <[email protected]>
Signed-off-by: Kersten Richter <[email protected]>
qos_bandwidth.adoc Outdated Show resolved Hide resolved
qos_bandwidth.adoc Outdated Show resolved Hide resolved
Signed-off-by: Kersten Richter <[email protected]>
Signed-off-by: Kersten Richter <[email protected]>
qos_capacity.adoc Outdated Show resolved Hide resolved
qos_capacity.adoc Outdated Show resolved Hide resolved
qos_capacity.adoc Outdated Show resolved Hide resolved
qos_capacity.adoc Outdated Show resolved Hide resolved
Signed-off-by: Kersten Richter <[email protected]>
qos_guidelines.adoc Outdated Show resolved Hide resolved
qos_guidelines.adoc Outdated Show resolved Hide resolved
qos_intro.adoc Outdated Show resolved Hide resolved
Added "by ways" back in but italicized it to make it stand out a bit more. Also added "multiway" to "Set associative cache".

Signed-off-by: Kersten Richter <[email protected]>
Signed-off-by: Kersten Richter <[email protected]>
@kersten1
Copy link
Contributor Author

@ved-rivos Thank you for your comments. I believe I have incorporated fixes for them all. The only thing left from my review was a general statement:

One of the general complaints that we've gotten for docs is that we do not fully explain the fields in a register. So for example, In qos_bandwidth.adoc after the wavedrom, VER and NBWBLKS are defined really well. But the descriptions tail off then and I'm not quite sure what RPFX and P are, even though they are mentioned in paragraph. Not sure what the rest are either. Can you add more description here?

Same with next register and those in qos_capacity.adoc.

Just something to think about. Not really expecting a fix immediately :)

@kersten1
Copy link
Contributor Author

kersten1 commented May 15, 2024

@ved-rivos I also think that this spec is well written and want to hold it up as an example so if you DO fix those register fields or come up with rationale for why descriptions are not needed (commonly known, explained elsewhere), then that would make it even better.

Signed-off-by: Kersten Richter <[email protected]>
@ved-rivos
Copy link
Collaborator

But the descriptions tail off then and I'm not quite sure what RPFX and P are, even though they are mentioned in paragraph. Not sure what the rest are either. Can you add more description here?

@kersten1 The section 2.1 provides the details and the equation for how the P parameter is used to compute the effective MCID.

@kersten1
Copy link
Contributor Author

But the descriptions tail off then and I'm not quite sure what RPFX and P are, even though they are mentioned in paragraph. Not sure what the rest are either. Can you add more description here?

@kersten1 The section 2.1 provides the details and the equation for how the P parameter is used to compute the effective MCID.

But P itself is never defined. I know that it is a number between 0 to 12, but not what it is. I know that VER field holds the version of the specification implemented by the capacity controller. The NCBLKS field holds the total number of allocatable capacity blocks in the controller. I can infer that the CUNITS refers to the limit on the capacity units and FRCID is the operation to flush and deallocate the capacity blocks. But then RPFX and P are talked about, but never defined.

@ved-rivos
Copy link
Collaborator

ved-rivos commented May 16, 2024

@kersten1 The section 2.1 explains both but we can add this one statement to make it more explicit:
"When RCID-prefixed mode (RPFX) is 1 the controller operates in RCID-prefixed mode and the parameter P (prefixed bits) indicates the number of least significant bits of the MCID carried in the request that should be prefixed by the RCID.
If RPFX is 1, the controller uses RCID in the requests along with P least significant bits of the MCID carried in the request to compute an effective MCID ([[EMCID]] to identify the monitoring counter."

@kersten1
Copy link
Contributor Author

@ved-rivos Perfect! Want me to add it to my PR?

@ved-rivos
Copy link
Collaborator

@kersten1 yes, please. Thanks!

Signed-off-by: Kersten Richter <[email protected]>
@kersten1
Copy link
Contributor Author

@ved-rivos done

@ved-rivos
Copy link
Collaborator

@kersten1 thanks. The same sentence may want to be in qos_bandwidth.adoc as well.

same change as in capacity

Signed-off-by: Kersten Richter <[email protected]>
@kersten1 kersten1 merged commit 3a093dd into main May 23, 2024
2 checks passed
@kersten1 kersten1 deleted the kersten1-patch-1 branch May 23, 2024 13:20
@kersten1 kersten1 restored the kersten1-patch-1 branch May 23, 2024 13:53
@kersten1 kersten1 deleted the kersten1-patch-1 branch May 23, 2024 15:22
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