cosmo_seq: mask STATUS_CML bit 1 to not get annoying pmbus alerts about it (#2520)

It turns out that PMBus devices like to set this bit bascially any time
they saw any kind of I2C behavior that makes them feel uncomfortable. We
probably don't want to produce an ereport every time an I2C Thing makes
a VRM uncomfortable. So this commit masks out PMBus alerts for
STATUS_CML's "other communication fault" bit, since it's annoying and
not a big deal.

This branch does a bit of extra refactoring on top of the change from
#2511. Since we are now setting even more `SMBALERT_MASK`s in
`initialize_pmbus_alerts`, I thought it would be nice to have some
shared utilities for trying to do this.

Author: hawkw

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_CML;
 use pmbus::commands::raa229620a::STATUS_IOUT;
 use pmbus::commands::raa229620a::STATUS_WORD;
 use ringbuf::*;
@@ -43,6 +44,7 @@ pub(crate) enum Rail {
 enum PmbusCmd {
     LoadLimit,
     SetStatusIoutMask,
+    SetStatusCmlMask,
     ClearFaults,
     ReadVin,
     Status,
@@ -59,6 +61,9 @@ enum Trace {
     StatusIoutMaskSet {
         all_ok: bool,
     },
+    StatusCmlMaskSet {
+        all_ok: bool,
+    },
     PmbusAlert {
         timestamp: u64,
         alerted: Vrms,
@@ -219,15 +224,10 @@ impl VCore {
         // if possible.
 
         // Set our warn limit
-        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)
-        })
-        .is_ok();
-        all_ok &= retry_i2c_txn(Rail::VddcrCpu1, PmbusCmd::LoadLimit, || {
-            self.vddcr_cpu1.set_vin_uv_warn_limit(VCORE_UV_WARN_LIMIT)
-        })
-        .is_ok();
+        let all_ok = self
+            .set_alert_config_on_both_vrms(PmbusCmd::LoadLimit, |vrm| {
+                vrm.set_vin_uv_warn_limit(VCORE_UV_WARN_LIMIT)
+            });
         ringbuf_entry!(Trace::LimitsLoaded { all_ok });
 
         let iout_mask = {
@@ -250,19 +250,33 @@ impl VCore {
             );
             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();
+        let all_ok = self.set_alert_config_on_both_vrms(
+            PmbusCmd::SetStatusIoutMask,
+            |vrm| vrm.set_status_iout_smbalert_mask(iout_mask),
+        );
         ringbuf_entry!(Trace::StatusIoutMaskSet { all_ok });
 
+        let cml_mask = {
+            let mut mask = STATUS_CML::CommandData(0);
+            // Mask out SMBus alerts for STATUS_CML bit 1. This bit, "other
+            // fault", is basically set when the PMBus sees something happen on
+            // the I2C bus that makes it feel weird. Unfortunately, it turns out
+            // that "I2C things that make you feel kinda weird" can happen a lot
+            // in an otherwise healthy system. While we are thankful for the
+            // RAA229620A for setting the status bit that says it saw something
+            // weird, we would really rather not get an IRQ about it every time
+            // there's I2C weather. So let's not get alerts for this one.
+            mask.set_other_communication_error(
+                STATUS_CML::OtherCommunicationError::Error,
+            );
+            mask
+        };
+        let all_ok = self
+            .set_alert_config_on_both_vrms(PmbusCmd::SetStatusCmlMask, |vrm| {
+                vrm.set_status_cml_smbalert_mask_on_all_rails(cml_mask)
+            });
+        ringbuf_entry!(Trace::StatusCmlMaskSet { all_ok });
+
         // Clear our faults
         self.try_to_clear_faults(Vrms {
             pwr_cont1: true,
@@ -275,6 +289,29 @@ impl VCore {
         ringbuf_entry!(Trace::Initialized);
     }
 
+    fn set_alert_config_on_both_vrms(
+        &self,
+        which: PmbusCmd,
+        txn: impl Fn(&Raa229620A) -> Result<(), raa229620a::Error>,
+    ) -> bool {
+        let mut all_ok = true;
+        all_ok &=
+            retry_i2c_txn(
+                Rail::VddcrCpu0,
+                which,
+                &mut || txn(&self.vddcr_cpu0),
+            )
+            .is_ok();
+        all_ok &=
+            retry_i2c_txn(
+                Rail::VddcrCpu1,
+                which,
+                &mut || txn(&self.vddcr_cpu1),
+            )
+            .is_ok();
+        all_ok
+    }
+
     pub fn can_we_unmask_any_vrm_irqs_again(&mut self) -> Vrms {
         self.try_to_clear_faults(self.faulted);
         Vrms {

drv/i2c-devices/src/lib.rs

@@ -178,6 +178,42 @@ macro_rules! pmbus_rail_write {
     }};
 }
 
+/// Write the mask `$mask` to the `SMBALERT_MASK` register for `$reg`, where
+/// `$reg` is a status register, and `$mask` is a `CommandData` value for that
+/// register.
+///
+/// Importantly, `$reg` must be a PMBus `STATUS_<whatever>` register. This macro
+/// cannot stop you from providing any `CommandCode` as the value of `$reg` and
+/// any `CommandData` as the value of `$mask`, but, uh, don't do that. On the
+/// other hand, the macro *does* at least ensure that `$mask` is a `CommandData`.
+/// for the same register as `$reg`.
+macro_rules! pmbus_smbalert_mask_write {
+    ($device:expr, $rail:expr, $reg:ident, $mask:expr) => {{
+        // This assignment is just a type assertion that `$mask` is a
+        // `CommandData` for the same register as `$reg`.
+        let mask: $reg::CommandData = $mask;
+        let rpayload = [PAGE::CommandData::code(), $rail];
+        // N.B. that the status register *should* always be a single byte, but
+        // we'll do this "properly" just in case.
+        let mut payload = [0u8; $reg::CommandData::len() + 2];
+        // 0               7               15              23
+        // +---------------+---------------+---------------+
+        // | SMBALERT_MASK | register code | mask byte     |
+        // +---------------+---------------+---------------+
+        payload[0] = CommandCode::SMBALERT_MASK as u8;
+        payload[1] = $reg::CommandData::code();
+        mask.to_slice(&mut payload[2..]);
+
+        match $device.write_write(&rpayload, &payload) {
+            Err(code) => Err(Error::BadWrite {
+                cmd: CommandCode::SMBALERT_MASK as u8,
+                code,
+            }),
+            Ok(_) => Ok(()),
+        }
+    }};
+}
+
 struct BadValidation {
     cmd: u8,
     code: ResponseCode,

drv/i2c-devices/src/raa229620a.rs

@@ -151,26 +151,23 @@ impl Raa229620A {
         &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,
-            })
+        pmbus_smbalert_mask_write!(self.device, self.rail, STATUS_IOUT, mask)
+    }
+
+    /// Set the `SMBALERT_MASK` for the `STATUS_CML` register, sending page
+    /// 0xFF. Though I couldn't find explicit confirmation of this in the PMBus
+    /// standard, one must kind of assume that `STATUS_CML` bits, which are not
+    /// specific to a particular output rail, are probably set on all PMBus
+    /// pages when a CML event happens, and thus we must mask them out on all
+    /// pages to stop SMBus alerts from being generated?
+    ///
+    /// Any bits set in `mask` will be masked, suppressing SMBus alerts when
+    /// those bits in `STATUS_CML` become set.
+    pub fn set_status_cml_smbalert_mask_on_all_rails(
+        &self,
+        mask: STATUS_CML::CommandData,
+    ) -> Result<(), Error> {
+        pmbus_smbalert_mask_write!(self.device, 0xff, STATUS_CML, mask)
     }
 
     pub fn read_vin(&self) -> Result<Volts, Error> {