Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions drv/gimlet-seq-server/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,15 @@ impl<S: SpiServer + Clone> ServerImpl<S> {
hl::sleep_for(1);
}

//
// Apply the configuration mitigation on the BMR491, if required.
//
{
let dev = i2c_config::devices::bmr491_u431(I2C.get_task_id());
let driver = drv_i2c_devices::bmr491::Bmr491::new(&dev, 0);
driver.apply_mitigation_for_rma2402311().ok();
}

//
// If our clock generator is configured to load from external EEPROM,
// we need to wait for up to 150 ms here (!).
Expand Down
98 changes: 98 additions & 0 deletions drv/i2c-devices/src/bmr491.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,104 @@ impl Bmr491 {
}
}

/// Applies a firmware-configuration workaround for the component tolerance
/// issue described in (unfortunately confidential) Flex document
/// RMA2402311, where over-eager input undervoltage sensing can result in
/// output glitches.
///
/// The theory of operation of the mitigation is as follows:
///
/// - An input filter in the R1C revision was built incorrectly and allows
/// brief negative transients to reach the controller chip.
///
/// - To stop the controller chip from reacting to these transients, Flex
/// suggests disabling automatic response to input undervoltage events.
///
/// - To retain some degree of brownout protection, they recommend instead
/// enabling the _output_ undervoltage monitor.
///
/// This function will check to see if the mitigation is already applied, or
/// if the device reports an unaffected revision. In either case, it will
/// leave the settings untouched.
pub fn apply_mitigation_for_rma2402311(&self) -> Result<(), Error> {
use pmbus::commands::bmr491::{
CommandCode, MAX_DUTY, VIN_OFF, VOUT_COMMAND, VOUT_UV_FAULT_LIMIT,
};
let mut rev = [0u8; 12];
self.device
.read_block(CommandCode::MFR_REVISION as u8, &mut rev)
.map_err(|code| Error::BadRead {
cmd: CommandCode::MFR_REVISION as u8,
code,
})?;

// Currently, the defect is known to exist in R1C parts, and may have
// been present in earlier parts (it's not clear that we have any). It
// is supposed to be fixed in R1D, and in the event that an R1E is
// released, it will _probably_ remain fixed.
//
// But since we don't know that for sure, currently we're treating only
// the R1D revision as fixed. Applying the mitigation to fixed future
// IBCs should not be destructive, so we can fix the firmware when that
// day comes.
if rev.starts_with(b"R1D ") {
// Assume the mitigation is unnecessary.
return Ok(());
Comment on lines +117 to +118
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice if we put something in the ringbuf indicating whether we've decided we have a revision that needs the mitigation or not, just so that we can have a record of what the driver decided to do someplace?

}

// Read out the affected registers to see if we've already applied the
// mitigation.
let current_vin_off = pmbus_read!(self.device, VIN_OFF)?;
let current_vout_command = pmbus_read!(self.device, VOUT_COMMAND)?;
let current_max_duty = pmbus_read!(self.device, MAX_DUTY)?;
let current_vout_uv_fault_limit =
pmbus_read!(self.device, VOUT_UV_FAULT_LIMIT)?;
let current_vout_uv_fault_response =
pmbus_read!(self.device, VOUT_UV_FAULT_RESPONSE)?;

if current_vin_off.0 == 0
&& current_vout_command.0 == 0x0060
&& current_max_duty.0 == 0xF8EA
&& current_vout_uv_fault_limit.0 == 0x0058
&& current_vout_uv_fault_response.0 == 0x80
Comment on lines +131 to +135
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpicky, probably unnecessary, but i wonder if we ought to make the mitigation VIN_OFF/VOUT_COMMAND/MAX_DUTY/VOUT_UV_FAULT_LIMIT/VOUT_UV_FAULT_RESPONSE values constants, and use them both in this check and when setting the registers later on.

that way, if we ever decide to change these values, we don't run the risk of accidentally changing the value we set but not the one we check for.

{
// The device configuration already reflects the mitigation.
return Ok(());
Comment on lines +137 to +138
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we ringbuf the driver's decisions as per my earlier comment, we should probably also say something when we believe the mitigation has already been applied.

}

// Override the VIN_OFF threshold to 0V, so that the IBC's internal
// controller treats VIN as "always above threshold."
pmbus_write!(self.device, VIN_OFF, VIN_OFF::CommandData(0))?;
// Command the power supply to produce 12 V (each LSB in this register
// is 1/8 V, so 12 * 8 = 96 = 0x60).
pmbus_write!(
self.device,
VOUT_COMMAND,
VOUT_COMMAND::CommandData(0x0060)
)?;
// Override the max duty cycle. The rationale for this is not totally
// clear, but Flex says to do it.
pmbus_write!(self.device, MAX_DUTY, MAX_DUTY::CommandData(0xF8EA))?;
// Adjust the VOUT fault to detect undervoltage on the 12V rail.
pmbus_write!(
self.device,
VOUT_UV_FAULT_LIMIT,
VOUT_UV_FAULT_LIMIT::CommandData(0x0058)
)?;
// And configure it to shut off the IBC without retry on fault. (The
// retry options are all wrong for our use case, they can cause it to
// power itself on and off repeatedly before stopping, or just stop; we
// choose the latter.)
pmbus_write!(
self.device,
VOUT_UV_FAULT_RESPONSE,
VOUT_UV_FAULT_RESPONSE::CommandData(0x80)
)?;

pmbus_write!(self.device, STORE_USER_ALL)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought RFD 630 determined we were not going to do this? Or has this PR not yet been updated to match?

Ok(())
}

pub fn read_mode(&self) -> Result<pmbus::VOutModeCommandData, Error> {
Ok(match self.mode.get() {
None => {
Expand Down