better tooling for PRBS mode for transceiver qual#238
better tooling for PRBS mode for transceiver qual#238Nieuwejaar wants to merge 2 commits intomainfrom
Conversation
Pass timing window to SDE's PRBS counter Add support for more detailed FSM data from the SDE
bnaecker
left a comment
bnaecker
left a comment
There was a problem hiding this comment.
This looks pretty good to me, thanks. I have a few suggestions, nothing major or structural.
| #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] | ||
| pub enum FsmType { | ||
| Port, | ||
| PortDfe, |
There was a problem hiding this comment.
Not sure it's worth it, but you might want to make the port-specific variants within a second enum. Something like:
pub enum FsmType {
Port(PortFsmType),
...
}
pub enum PortFsmType {
An,
Prbs,
....
}| } | ||
|
|
||
| impl FsmType { | ||
| pub fn is_port_fsm(&self) -> bool { |
There was a problem hiding this comment.
This would be simpler if you do the above suggestion, just something like matches!(self, FsmType::Port(_)).
| let errors = unsafe { | ||
| bf_pm_port_prbs_mode_stats_get(dev, fp.ptr(), &mut stats, ms) | ||
| .check_error("bf_pm_port_prbs_stats_get")?; | ||
| stats |
There was a problem hiding this comment.
Does this need to be inside the unsafe block?
| hdl: &Handle, | ||
| port_hdl: PortHdl, | ||
| ms: u32, | ||
| ) -> AsicResult<Vec<u32>> { |
There was a problem hiding this comment.
Could we document what this return value actually is? It looks like the error counts for each lane, but would be nice to know that without reading the implementation.
|
|
||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| match s.to_lowercase().as_str() { | ||
| "Mode31" | "mode31" | "31" => Ok(PortPrbsMode::Mode31), |
There was a problem hiding this comment.
Why are these modes removed? Are they not actually implemented in the ASIC?
| // | example for the next person. | ||
| // v | ||
| // (next_int, IDENT), | ||
| (11, PRBS_IMPROVEMENT), |
There was a problem hiding this comment.
Kind of a nit, but this is a little generic IMO. We might make many improvements :) Maybe something like ADD_PRBS_ERROR_COUNTS?
| path: Path<LinkPath>, | ||
| ) -> Result<HttpResponseOk<Ber>, HttpError>; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Style nit, but we should probably use the same comment delimiter here, ///
| &self, | ||
| port_id: PortId, | ||
| link_id: LinkId, | ||
| ms: u32, |
There was a problem hiding this comment.
Do we need to do any validation on this value of milliseconds? Someone could request both 0 and also u32::MAX here IIUC. That latter one especially seems bad.
2 participants
Add support for monitoring PRBS error counts
Remove PRBS modes not supported by Tofino2
Reset PRBS state when deleting a link