cosmo_seq: mask RAA229620A Iout overcurrent warnings (#2511)

See #2510 --- evidentially, AMD requires that the SVI3 fast sum
overcurrent threshold for VRMs supplying SVI3 power rails to Turin CPUs
be set at a threshold of 90% of 90% of the part's EDC amperage. This
results in the VRM setting the overcurrent warning bit in `STATUS_IOUT`
and tugging on its fault pin to request attention pretty incessantly
when an IRM Group G CPU is under heavy (but importantly, safe!) load.
AMD suggests that BMC's "mask or ignore" this warning, since the
threshold has to be set to a value that results in it being asserted
during normal operation due to reasons I am no doubt to provincial to
understand. Therefore, this commit makes Hubris mask out the
`STATUS_IOUT` bit for overcurrent warnings on the RAA229620A
`VDDCR_CPU0` and `VDDCR_CPU1` SVI3 regulators. This felt like the best
solution because it (a) shouldn't suppress any fault bits that actually
*do* represent something bad, and (b) unlike ignoring this specific
warning, doesn't result in Hubris taking a bunch of IRQs that it just
ultimately decides to do nothing about.

With help from @ericaasen, we have reproduced the warnings by manually
adjusting the threshold to a lower value, as the CPU load testing script
I was running wasn't actually able to make a 9755 hit the 297 amp
threshold. I've validated that masking the relevant bit in the
`SMBALERT_MASK` for the `STATUS_IOUT` register makes the IRQing go away.
So that's good.

I don't _love_ the function I wrote for masking this bit: it's specific
for setting `SMBALERT_MASK` for `STATUS_IOUT` and not for any other
register. I feel like it ought to be possible to write a single function
for setting `SMBALERT_MASK` for *any* status register, but the
abstractions provided by `pmbus` at time of writing made it difficult to
do so generically --- or perhaps I'm holding it wrong! Anyway, I'd
really like to get the immediate fix in first and then try to come up
with a nicer abstraction for `SMBALERT_MASK` later, since I'd like to
stop the ereport spew in R20 if possible.

In addition, I have made some changes to the `initialize_pmbus_alerts`
(née `initialize_pmbus_warnings`) method in `Vcore`. Since that method
now both sets the input undervoltage warning threshold _and_ masks the
output overcurrent warning, I've changed it to not return a `Result` so
that it cannot accidentally `?` out early without trying to set _all_
the PMBus configurations it tries to set. We probably want to attempt to
set the `SMBALERT_MASK` even if we encountered an amount of I2C Weather
that caused setting the UV threshold for one of the VRMs to fail. So now
we put the error in the ringbuf and keep going there.

Fixes #2510.

Author: hawkw

drv/cosmo-seq-server/src/main.rs

@@ -762,18 +762,11 @@ impl ServerImpl {
             notifications::SEQ_IRQ_MASK,
             sys_api::IrqControl::Enable,
         );
-        // Enable the undervoltage warning PMBus alert from the Vcore
-        // regulators.
-        //
-        // Yes, we just ignore the error here --- while that seems a bit
-        // sketchy, but what else can we do? It seems pretty bad to panic and
-        // say "nope, the computer won't turn on" because we weren't able to do
-        // an I2C transaction to turn on an interrupt that we only use for
-        // monitoring for faults. The initialize method will retry internally a
-        // few times, so we should power through any transient I2C messiness,
-        // and any I2C errors that occur get logged in the `vcore` module's
-        // ringbuf.
-        let _ = self.vcore.initialize_uv_warning();
+        // Configure PMBus alerts from the Vcore regulators. This will attempt
+        // to set a limit for the input undervoltage warning, and mask out the
+        // (spurious) output overcurrent warning (which must be set to a
+        // threshold that is not indicative of a real fault).
+        self.vcore.initialize_pmbus_alerts();
         self.seq.ier.modify(|m| {
             m.set_fanfault(true);
             m.set_thermtrip(true);

drv/cosmo-seq-server/src/vcore.rs

@@ -18,6 +18,7 @@ use drv_i2c_api::ResponseCode;
 use drv_i2c_devices::raa229620a::{self, Raa229620A};
 use ereports::pwr::{PmbusAlert, PmbusStatus};
 use fixedstr::FixedStr;
+use pmbus::commands::raa229620a::STATUS_IOUT;
 use pmbus::commands::raa229620a::STATUS_WORD;
 use ringbuf::*;
 use userlib::{TaskId, sys_get_timer, units};
@@ -41,6 +42,7 @@ pub(crate) enum Rail {
 #[derive(Copy, Clone, PartialEq)]
 enum PmbusCmd {
     LoadLimit,
+    SetStatusIoutMask,
     ClearFaults,
     ReadVin,
     Status,
@@ -51,7 +53,12 @@ enum Trace {
     None,
     Initializing,
     Initialized,
-    LimitsLoaded,
+    LimitsLoaded {
+        all_ok: bool,
+    },
+    StatusIoutMaskSet {
+        all_ok: bool,
+    },
     PmbusAlert {
         timestamp: u64,
         alerted: Vrms,
@@ -198,17 +205,63 @@ impl VCore {
         self.faulted.pwr_cont1 || self.faulted.pwr_cont2
     }
 
-    pub fn initialize_uv_warning(&mut self) -> Result<(), ResponseCode> {
+    pub fn initialize_pmbus_alerts(&mut self) {
         ringbuf_entry!(Trace::Initializing);
 
+        // Yes, we just ignore errors here --- that may seem a bit sketchy, but
+        // what else can we do? It seems pretty bad to panic and say "nope, the
+        // computer won't turn on" because we weren't able to do an I2C
+        // transaction to turn on an interrupt that we only use for monitoring.
+        // Each step will retry internally a few times, so we should power
+        // through any transient I2C messiness, and any I2C errors that occur
+        // get logged in the ringbuf. We also do *not* bail out early if any
+        // other step fails, because we would still like to do every other thing
+        // if possible.
+
         // Set our warn limit
-        retry_i2c_txn(Rail::VddcrCpu0, PmbusCmd::LoadLimit, || {
+        let mut all_ok = true;
+        all_ok &= retry_i2c_txn(Rail::VddcrCpu0, PmbusCmd::LoadLimit, || {
             self.vddcr_cpu0.set_vin_uv_warn_limit(VCORE_UV_WARN_LIMIT)
-        })?;
-        retry_i2c_txn(Rail::VddcrCpu1, PmbusCmd::LoadLimit, || {
+        })
+        .is_ok();
+        all_ok &= retry_i2c_txn(Rail::VddcrCpu1, PmbusCmd::LoadLimit, || {
             self.vddcr_cpu1.set_vin_uv_warn_limit(VCORE_UV_WARN_LIMIT)
-        })?;
-        ringbuf_entry!(Trace::LimitsLoaded);
+        })
+        .is_ok();
+        ringbuf_entry!(Trace::LimitsLoaded { all_ok });
+
+        let iout_mask = {
+            let mut mask = STATUS_IOUT::CommandData(0);
+            // Mask out SMBus alerts for output overcurrent warnings on the
+            // RAA229620A. This is necessary because the AMD power design
+            // guidelines for Turin require that the SVI3 fast overcurrent
+            // response warning threshold be 90% of the EDC for the CPU, which
+            // results in the VRMs asserting output overcurrent warnings on
+            // their SVI3 rail during normal operation of the CPU. AMD
+            // recommends that these warnings be masked out and/or ignored, so
+            // we disable them here.
+            //
+            // We don't expect to see overcurrent warnings on non-SVI3 rails, as
+            // the default value of the PMBus `IOUT_OC_WARN_LIMIT` register
+            // on the RAA229620A is 3000A, which is *well above* the overcurrent
+            // *fault* threshold.
+            mask.set_output_overcurrent_warning(
+                STATUS_IOUT::OutputOvercurrentWarning::Warning,
+            );
+            mask
+        };
+        let mut all_ok = true;
+        all_ok &=
+            retry_i2c_txn(Rail::VddcrCpu0, PmbusCmd::SetStatusIoutMask, || {
+                self.vddcr_cpu0.set_status_iout_smbalert_mask(iout_mask)
+            })
+            .is_ok();
+        all_ok &=
+            retry_i2c_txn(Rail::VddcrCpu1, PmbusCmd::SetStatusIoutMask, || {
+                self.vddcr_cpu1.set_status_iout_smbalert_mask(iout_mask)
+            })
+            .is_ok();
+        ringbuf_entry!(Trace::StatusIoutMaskSet { all_ok });
 
         // Clear our faults
         self.try_to_clear_faults(Vrms {
@@ -220,8 +273,6 @@ impl VCore {
         // our guys.
 
         ringbuf_entry!(Trace::Initialized);
-
-        Ok(())
     }
 
     pub fn can_we_unmask_any_vrm_irqs_again(&mut self) -> Vrms {

drv/i2c-devices/src/raa229620a.rs

@@ -143,6 +143,36 @@ impl Raa229620A {
         pmbus_rail_write!(self.device, self.rail, VIN_UV_WARN_LIMIT, vin)
     }
 
+    /// Set the `SMBALERT_MASK` for the `STATUS_IOUT` register.
+    ///
+    /// Any bits set in `mask` will be masked, suppressing SMBus alerts when
+    /// those bits in `STATUS_IOUT` become set.
+    pub fn set_status_iout_smbalert_mask(
+        &self,
+        mask: STATUS_IOUT::CommandData,
+    ) -> Result<(), Error> {
+        // Unfortunately, SMBALERT_MASK is kinda hard to use with the
+        // `pmbus_write!` macro family, due to not having its own `CommandData`
+        // type, and because it requires writing both the name of the status
+        // register being masked *and* the value of that register (as the mask).
+        // It's a bit odd. Probably it deserves its own
+        // `pmbus_smbalert_mask_write!` macro or something, but for now, we'll
+        // just do it manually.
+        self.device
+            .write_write(
+                &[PAGE::CommandData::code(), self.rail],
+                &[
+                    CommandCode::SMBALERT_MASK as u8,
+                    CommandCode::STATUS_IOUT as u8,
+                    mask.0,
+                ],
+            )
+            .map_err(|code| Error::BadWrite {
+                cmd: CommandCode::SMBALERT_MASK as u8,
+                code,
+            })
+    }
+
     pub fn read_vin(&self) -> Result<Volts, Error> {
         let vin = pmbus_rail_read!(self.device, self.rail, READ_VIN)?;
         Ok(Volts(vin.get()?.0))