chore(hpu): network trace dbg selftests & helpers - #3856
Conversation
|
✅ Backward-compat snapshot: everything looks good! No backward-compatibility issues detected. |
|
✅ Forward compatibility matrix unchanged: it still matches the committed baseline. MatrixWhich released versions can load data produced by this branch. A ❌ is not a failure by itself: it only means forward compatibility is not (or no longer) provided for that type. What matters is whether it was reviewed.
|
JJ-hw
left a comment
JJ-hw
left a comment
There was a problem hiding this comment.
Seems OK to me.
I'm not the more fitted to review the test part (not sure what is going on here, and if everything is tested).
General remarks:
- concerning register definition, avoid mixing fields of different natures. (ex. cmd vs status)
- I am not a big fan of clear-on-read register. The behavior is counter-intuitive (a read should be harmless). A reading "out of place" could erase the error registers, which may not be expected.
- I don't know if they exist, but if the constants are defined for register field size and offset exist, it is preferable to use them instead of numbers to avoid future errors or update if the registers are modified.
- Same for fields in packet.
| duplicate=["_0", "_1", "_2", "_3", "_4", "_5", "_6", "_7"] | ||
|
|
||
| [section.mhdma_system.register.trace_ctrl] | ||
| description="Packet-trace control and status: write frozen=1 to freeze capture now, or frozen=0 to flush and re-arm." |
There was a problem hiding this comment.
Not sure what "freeze" mean.
You mean when the bit is set to 1; we stop the capture ?
wptr and wrapped are status bits once the capture is stopped.
This is detail, but I won't mix command (which "freeze" is) and the status fields in the same register.
| field.wrapped = {size_b=1, offset_b=7, default={Cst=0x0}, description="Ring wrapped since last re-arm: 1 = all TRACE_DEPTH entries valid (rotate by wptr); 0 = only entries [0 .. wptr-1] valid (partial fill, no rotation). Re-arm does not clear the RAM, so slots beyond a partial fill hold stale prior-session data"} | ||
|
|
||
| [section.mhdma_system.register.trace_data] | ||
| description="Packet-trace read window: each read returns the next 32-bit word of the frozen trace and advances the read pointer." |
There was a problem hiding this comment.
From which read pointer ?
Especially when the system has wrapped ?
| read_access="Read" | ||
| write_access="Write" | ||
| field.mask = {size_b=31, offset_b=0, default={Cst=0x7FFFFFFF}, description="Per-error trace-trigger enable, aligned to mhdma_system.errors[11:0]; bits above the mhdma error width are ignored"} | ||
| field.inject_err = {size_b=1, offset_b=31, default={Cst=0x0}, description="Write 0->1 to inject a synthetic error: triggers the trace and sets mhdma_system.errors[31]. Sticky, cleared by reading mhdma_system.errors"} |
There was a problem hiding this comment.
Is the injected error a pulse, or a leveled signal that equals one as long as the register is 1 (i.e. user has not read it) ?
| owner="User" | ||
| read_access="Read" | ||
| write_access="Write" | ||
| field.mask = {size_b=31, offset_b=0, default={Cst=0x7FFFFFFF}, description="Per-error trace-trigger enable, aligned to mhdma_system.errors[11:0]; bits above the mhdma error width are ignored"} |
There was a problem hiding this comment.
I would not mix the mask, which is functional, with the inject_err, inside the same register.
| node < 16 && mode < 4, | ||
| "req_id field overflow: node={node} mode={mode}" | ||
| ); | ||
| (opcode << 20) | ((node as u32) << 16) | (mode << 14) |
There was a problem hiding this comment.
Could the offsets be given by the toml ?
It would be less error prone.
| dst_addr: bitfield(w0, 0, 16), | ||
| src_addr: bitfield(w0, 16, 16), | ||
| iop: bitfield(w1, 0, 8), | ||
| req: bitfield(w1, 8, 4), | ||
| mode: bitfield(w1, 12, 2), | ||
| flag: bitfield(w1, 14, 6), | ||
| hpu: bitfield(w1, 28, 4), | ||
| seq: bitfield(w2, 0, 8), | ||
| src_mac: bitfield(w2, 8, 24), | ||
| errbits: bitfield(w3, 0, PKT_TRACE_ERR_W), |
There was a problem hiding this comment.
Are these values available as constant ?
It would be less error prone, if they exist.
3 participants
Network trace helpers & selftests.
Theses board tests are basically the replacement of the old bash scripts
TODO before merge : update .hpu with the rebased bitstream over main