Feedback on RISC-V Trace Control Interface Specification - Chapter 4

[ved-rivos]

Section 4
16. Table 1 - the term "trBaseEncoder" etc. used in this table implies it
is a register but it is not. Also the use of that implies there is
one base address in the system.
17. Table 1 - The definition of all the components has been discussed extensively.
Repeating the description is not required - that column should be removed.
18. Section 4.1 - "This also applies to multiple harts..." is redundant wrt to
first normative sentence.
19. Section 4.1.1 - "Optionally (and if accepta..." since this is a
configuration, the text in paraenthesis can be removed. ALso the bullet
before this wants to be rewriiten to state - if the coniguration does not
indicate stall on overflow - otherwise the previous bullet contradicts the
configuration to stall.
20. Section 4.1.1 - last bullet - should be reworded to address a) bandwdith
all along the path, not just at the sink, must be matched to the rate at
which data can be produced. Using trigger windows does not solve the
bandwidth problem for trace generated - it limits the amount of trace
data but not the bandwidth. The FIFO should be clarified as what provide
elasticity for temporary bursts but no amount of buffering can solve a
bandwidth mismatch.
21. Table 2 is quite redundant and not adding much value.
22. Table 2 - why is it disallowed to use ATB unless there are other ARM
components?
23. Section 4.1.1 - informational note - It is unclear why encapsulation in a
nexus message is needed. The ATB has ATID to indetify source of trace and
otherwise is agnostic to the payload. It is for decoder to decide based
on ATID how to interpret the payload. Further for a non RISC-V compliant
implementation there are many other custom aspects to handle like the
trace controls for non-RISC-V compliant components.
24. Section 4.1.2 - the manu diagrams are not adding much value.
25. Section 4.1.2 - information note - since every funnel needs to have a
per input disable, its not clear what the note is trying to say.
26. Section 4.2 - the discussion in this section is quite confusing. It is
well understood that memory mapped registers can be accessed though
SBA or by hart and that such accesses can be controlled by PMP. Since
this specification is to memory map the control registers this whole section
can be removed.
27. Section 4.3 - This table should be updated to only lists 2 registers
Control and implementation. The primary purpose of having the implementation
register at fixed offset is to support discovery of component type to
determine how to interpret the rest of the registers. By keeping just the
two registers and describing the componet type location as being standard
in this section, we can then indicate
If component type is 0x9 - see section 7 register details
The rest of the table is unnecessary as each component has its own table.
and this table starts becoming contradictory using names (e.g. at offset 8
) that dont match the later tables.
The paragraph on version numbering from chapter 5 can be moved here and
chapter 5 removed. The summaries can then be moed into their relevant
chapter.
29. Section 4.3 - note about access to register when tr??Active is 0 should be
normative. Unpredictable is not a good behavior to specify. Is the behavior
UNSPECIFIED? What are the range of behaviors allowed - hang? shutdown?
29. Section 4.3 - "alows debug tools to verify provided base address" - It is
unclear what verification of base address is required. Maybe the guidance
is that given the base address of a component, the tr??Imple register
30. Section 4.3 - "Some WARL fields.." -> this sentence can be removed - this
is already WARL definition.
31. section 4.3.1 - The example imply that those can be implemented as
custom extensions in the register space marked as "Reserved for more
register/sub-components". Is that right? If not then the Reserved should
just be called "Reserved for future standard extensions". It would then
be good to designate a range for custom extensions and mark it as
"Designated for custom use." The trailing normative text can be removed
as its not part of this specification.
32. Table 4 - trTeInstFilters description - "Filters to qualify instruction
trace"
33. Table 5 - trRAMData - since this field is also optional, why separate
the bigger read buffer row?
34. Table 7 - WHy reserve offset 7 for timestamp control? Is there a known
extension planned?

[mipsrobert]

All notes to Control PDF handled in 1.0.0_rc20.