Skip to content

Commit

Permalink
transceivers: fix I2C read bug (#1787)
Browse files Browse the repository at this point in the history 
#1768 did not properly account for the FIFO behavior of the FPGA's data
buffers. The "check the status byte" portion of the loop happened
outside the part where we read the buffer, and since the buffer was just
memory-mapped registers it could be repeatedly without consequence.
Since the data was now in a FIFO, I was inadvertently draining the FIFO
before the transaction was done. This PR consolidates the "is I2C done
yet" logic into the `get_i2c_status_and_read_buffer` so calling code can
just deal with the status register and the data buffer.

Fixes #1786
  • Loading branch information
@Aaron-Hartwig
Aaron-Hartwig authored May 20, 2024
1 parent 561b577 commit 1d3f3d5
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 82 deletions.
42 changes: 30 additions & 12 deletions drv/sidecar-front-io/src/transceivers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -677,13 +677,22 @@ impl core::ops::Index<LogicalPort> for LogicalPortFailureTypes {

#[derive(Copy, Clone)]
pub struct PortI2CStatus {
pub rdata_fifo_empty: bool,
pub wdata_fifo_empty: bool,
pub done: bool,
pub error: FpgaI2CFailure,
}

impl PortI2CStatus {
pub fn new(status: u8) -> Self {
// Use QSFP::PORT0 for constants, since they're all identical
Self {
rdata_fifo_empty: (status
& Reg::QSFP::PORT0_STATUS::RDATA_FIFO_EMPTY)
!= 0,
wdata_fifo_empty: (status
& Reg::QSFP::PORT0_STATUS::WDATA_FIFO_EMPTY)
!= 0,
done: (status & Reg::QSFP::PORT0_STATUS::BUSY) == 0,
error: FpgaI2CFailure::try_from(status).unwrap_lite(),
}
Expand Down Expand Up @@ -1028,13 +1037,13 @@ impl Transceivers {
.read(Addr::QSFP_PORT0_STATUS as u16 + port_loc.port.0 as u16)
}

/// Get `buf.len()` bytes of data, where the first byte is port status and
/// trailing bytes are the I2C read buffer for a `port`. The buffer stores
/// data from the last I2C read transaction done and thus only the number of
/// bytes read will be valid in the buffer.
/// Returns a PortI2CStatus and will fill `buf.len()` bytes of data with the
/// I2C read buffer for a `port`. The buffer stores data from the last I2C
/// read transaction done and thus only the number of bytes read will be
/// valid in the buffer.
///
/// Poll the status register for a port until an I2C transaction is done.
/// Upon completion, pass the status byte back alongside the read data. The
/// Upon completion, return the status struct back and the read data. The
/// read data buffer stores I2C read data from the last transaction so the
/// FIFO will only contain that many bytes. The FPGA automatically clears
/// the read data FIFO before starting a new read, so there's no need for
Expand All @@ -1043,14 +1052,23 @@ impl Transceivers {
&self,
port: P,
buf: &mut [u8],
) -> Result<(), FpgaError> {
) -> Result<PortI2CStatus, FpgaError> {
let port_loc = port.into();
buf[0] = self.get_i2c_status(port_loc)?;
self.fpga(port_loc.controller).read_bytes(
ReadOp::ReadNoAddrIncr,
Addr::QSFP_PORT0_I2C_DATA as u16 + port_loc.port.0 as u16,
&mut buf[1..],
)
loop {
let status_byte = self.get_i2c_status(port_loc)?;
let status = PortI2CStatus::new(status_byte);

if status.done {
self.fpga(port_loc.controller).read_bytes(
ReadOp::ReadNoAddrIncr,
Addr::QSFP_PORT0_I2C_DATA as u16 + port_loc.port.0 as u16,
buf,
)?;
return Ok(status);
}

userlib::hl::sleep_for(1);
}
}

/// Write `buf.len()` bytes of data into the I2C write buffer of the `mask`
Expand Down
57 changes: 25 additions & 32 deletions drv/transceivers-server/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ use drv_fpga_api::FpgaError;
use drv_i2c_devices::pca9956b::Error;
use drv_sidecar_front_io::{
leds::{FullErrorSummary, Leds},
transceivers::{LogicalPort, LogicalPortMask, Transceivers},
transceivers::{
FpgaI2CFailure, LogicalPort, LogicalPortMask, Transceivers,
},
Reg,
};
use drv_sidecar_seq_api::{SeqError, Sequencer};
Expand Down Expand Up @@ -253,28 +255,24 @@ impl ServerImpl {

#[derive(Copy, Clone, FromBytes, AsBytes)]
#[repr(C)]
struct StatusAndTemperature {
status: u8,
struct Temperature {
temperature: zerocopy::I16<zerocopy::BigEndian>,
}

loop {
let mut out = StatusAndTemperature::new_zeroed();
self.transceivers
.get_i2c_status_and_read_buffer(port, out.as_bytes_mut())?;
if out.status & Reg::QSFP::PORT0_STATUS::BUSY == 0 {
if out.status & Reg::QSFP::PORT0_STATUS::ERROR != 0 {
return Err(FpgaError::ImplError(out.status));
} else {
// "Internally measured free side device temperatures are
// represented as a 16-bit signed twos complement value in
// increments of 1/256 degrees Celsius"
//
// - SFF-8636 rev 2.10a, Section 6.2.4
return Ok(Celsius(out.temperature.get() as f32 / 256.0));
}
}
userlib::hl::sleep_for(1);
let mut out = Temperature::new_zeroed();
let status = self
.transceivers
.get_i2c_status_and_read_buffer(port, out.as_bytes_mut())?;

if status.error == FpgaI2CFailure::NoError {
// "Internally measured free side device temperatures are
// represented as a 16-bit signed twos complement value in
// increments of 1/256 degrees Celsius"
//
// - SFF-8636 rev 2.10a, Section 6.2.4
Ok(Celsius(out.temperature.get() as f32 / 256.0))
} else {
Err(FpgaError::ImplError(status.error as u8))
}
}

Expand All @@ -287,28 +285,23 @@ impl ServerImpl {
return Err(FpgaError::CommsError);
}

let mut out = [0u8; 2]; // [status, SFF8024Identifier]
let mut out = [0u8; 1]; // [SFF8024Identifier]

// Wait for the I2C transaction to complete
loop {
self.transceivers
.get_i2c_status_and_read_buffer(port, &mut out)?;
if out[0] & Reg::QSFP::PORT0_STATUS::BUSY == 0 {
break;
}
userlib::hl::sleep_for(1);
}
let status = self
.transceivers
.get_i2c_status_and_read_buffer(port, &mut out)?;

if out[0] & Reg::QSFP::PORT0_STATUS::ERROR == 0 {
match out[1] {
if status.error == FpgaI2CFailure::NoError {
match out[0] {
0x1E => Ok(ManagementInterface::Cmis),
0x0D | 0x11 => Ok(ManagementInterface::Sff8636),
i => Ok(ManagementInterface::Unknown(i)),
}
} else {
// TODO: how should we handle this?
// Right now, we'll retry on the next pass through the loop.
Err(FpgaError::ImplError(out[0]))
Err(FpgaError::ImplError(status.error as u8))
}
}

Expand Down
62 changes: 24 additions & 38 deletions drv/transceivers-server/src/udp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use userlib::UnwrapLite;
use crate::{FrontIOStatus, ServerImpl};
use drv_sidecar_front_io::transceivers::{
FpgaI2CFailure, LogicalPort, LogicalPortFailureTypes, LogicalPortMask,
ModuleResult, ModuleResultNoFailure, ModuleResultSlim, PortI2CStatus,
ModuleResult, ModuleResultNoFailure, ModuleResultSlim,
};
use task_net_api::*;
use transceiver_messages::{
Expand Down Expand Up @@ -1156,48 +1156,34 @@ impl ServerImpl {
let mut failure_types = LogicalPortFailureTypes::default();

for port in result.success().to_indices() {
// The status register is contiguous with the output buffer, so
// we'll read them all in a single pass. This should normally
// terminate with a single read, since I2C is faster than Hubris
// IPC.
let mut buf = [0u8; 129];
let mut buf = [0u8; 128];
let port_loc = port.get_physical_location();
loop {
// If we have not encountered any errors, keep pulling full
// status + buffer payloads.
if self
.transceivers
.get_i2c_status_and_read_buffer(
port_loc,
&mut buf[0..(buf_len + 1)],
)
.is_err()
{
error.set(port);
break;
};

let status = PortI2CStatus::new(buf[0]);

// Use QSFP::PORT0 for constants, since they're all identical
if status.done {
// Check error mask
if status.error != FpgaI2CFailure::NoError {
// Record which port the error ocurred at so we can
// give the host a more meaningful error.
failure.set(port);
failure_types.0[port.0 as usize] = status.error
} else {
// Add data to payload
success.set(port);
let end_idx = idx + buf_len;
out[idx..end_idx].copy_from_slice(&buf[1..][..buf_len]);
idx = end_idx;
}
// If we have not encountered any errors, keep pulling full
// status + buffer payloads.
let status = match self
.transceivers
.get_i2c_status_and_read_buffer(port_loc, &mut buf[0..buf_len])
{
Ok(status) => status,
Err(_) => {
error.set(port);
break;
}
};

userlib::hl::sleep_for(1);
// Check error mask
if status.error != FpgaI2CFailure::NoError {
// Record which port the error ocurred at so we can
// give the host a more meaningful error.
failure.set(port);
failure_types.0[port.0 as usize] = status.error
} else {
// Add data to payload
success.set(port);
let end_idx = idx + buf_len;
out[idx..end_idx].copy_from_slice(&buf[..buf_len]);
idx = end_idx;
}
}
let mut final_result =
Expand Down

0 comments on commit 1d3f3d5

Please sign in to comment.