From bb09df64b61ef8b0ce64db650d61558ebdf87018 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 19 May 2026 09:57:08 -0700 Subject: [PATCH 1/5] smbalert mask macro --- drv/i2c-devices/src/lib.rs | 33 +++++++++++++++++++++++++++++++ drv/i2c-devices/src/raa229620a.rs | 21 +------------------- 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/drv/i2c-devices/src/lib.rs b/drv/i2c-devices/src/lib.rs index 79b8efcf4..2b2efb182 100644 --- a/drv/i2c-devices/src/lib.rs +++ b/drv/i2c-devices/src/lib.rs @@ -178,6 +178,39 @@ macro_rules! pmbus_rail_write { }}; } +/// Write the mask `$mask` to the `SMBALERT_MASK` register for `$reg`, where +/// `$reg` is a status register. +/// +/// Importantly, `$reg` must be a PMBus `STATUS_` register. This macro +/// cannot stop you from providing any `CommandCode` as the value of `$reg`, or +/// any `CommandData` as the value of `$mask`, but, uh, don't do that. +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, diff --git a/drv/i2c-devices/src/raa229620a.rs b/drv/i2c-devices/src/raa229620a.rs index 55ba601c4..20b5b0fa3 100644 --- a/drv/i2c-devices/src/raa229620a.rs +++ b/drv/i2c-devices/src/raa229620a.rs @@ -151,26 +151,7 @@ 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) } pub fn read_vin(&self) -> Result { From 2d12eb68740c9dd1e26f78241447b235f9bda851 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 19 May 2026 10:07:04 -0700 Subject: [PATCH 2/5] wip --- drv/i2c-devices/src/lib.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drv/i2c-devices/src/lib.rs b/drv/i2c-devices/src/lib.rs index 2b2efb182..d94c9e219 100644 --- a/drv/i2c-devices/src/lib.rs +++ b/drv/i2c-devices/src/lib.rs @@ -179,11 +179,14 @@ macro_rules! pmbus_rail_write { } /// Write the mask `$mask` to the `SMBALERT_MASK` register for `$reg`, where -/// `$reg` is a status register. +/// `$reg` is a status register, and `$mask` is a `CommandData` value for that +/// register. /// /// Importantly, `$reg` must be a PMBus `STATUS_` register. This macro -/// cannot stop you from providing any `CommandCode` as the value of `$reg`, or -/// any `CommandData` as the value of `$mask`, but, uh, don't do that. +/// 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 From 4b7ef9317bc47df0d74f76c8f6d0adb3786828b4 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 19 May 2026 11:57:42 -0700 Subject: [PATCH 3/5] try to set STATUS_CML masks --- drv/cosmo-seq-server/src/vcore.rs | 75 ++++++++++++++++++++++--------- drv/i2c-devices/src/raa229620a.rs | 16 +++++++ 2 files changed, 71 insertions(+), 20 deletions(-) diff --git a/drv/cosmo-seq-server/src/vcore.rs b/drv/cosmo-seq-server/src/vcore.rs index ecb80ebf4..3ff9cbc3e 100644 --- a/drv/cosmo-seq-server/src/vcore.rs +++ b/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,31 @@ 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 2. 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_fault(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 +287,29 @@ impl VCore { ringbuf_entry!(Trace::Initialized); } + fn set_alert_config_on_both_vrms( + &self, + which: PmbusCmd, + txn: impl Fn(&Raa229620A) -> Result, + ) -> 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 { diff --git a/drv/i2c-devices/src/raa229620a.rs b/drv/i2c-devices/src/raa229620a.rs index 20b5b0fa3..1ad6e6aa2 100644 --- a/drv/i2c-devices/src/raa229620a.rs +++ b/drv/i2c-devices/src/raa229620a.rs @@ -154,6 +154,22 @@ impl Raa229620A { 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 { let vin = pmbus_rail_read!(self.device, self.rail, READ_VIN)?; Ok(Volts(vin.get()?.0)) From b15cd9f3322a8a9d1394b94498a2f5868c09cc5c Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 19 May 2026 11:59:31 -0700 Subject: [PATCH 4/5] and by "2" i mean "1" --- drv/cosmo-seq-server/src/vcore.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drv/cosmo-seq-server/src/vcore.rs b/drv/cosmo-seq-server/src/vcore.rs index 3ff9cbc3e..1a3011031 100644 --- a/drv/cosmo-seq-server/src/vcore.rs +++ b/drv/cosmo-seq-server/src/vcore.rs @@ -258,7 +258,7 @@ impl VCore { let cml_mask = { let mut mask = STATUS_CML::CommandData(0); - // Mask out SMBus alerts for STATUS_CML bit 2. This bit, "other + // 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 From 45fa7621fb3bc336048d93b5f5c149780a46eb9a Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Tue, 19 May 2026 13:21:55 -0700 Subject: [PATCH 5/5] unfuck --- drv/cosmo-seq-server/src/vcore.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drv/cosmo-seq-server/src/vcore.rs b/drv/cosmo-seq-server/src/vcore.rs index 1a3011031..761f9b01d 100644 --- a/drv/cosmo-seq-server/src/vcore.rs +++ b/drv/cosmo-seq-server/src/vcore.rs @@ -266,7 +266,9 @@ impl VCore { // 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_fault(STATUS_CML::OtherCommunicationError::Error); + mask.set_other_communication_error( + STATUS_CML::OtherCommunicationError::Error, + ); mask }; let all_ok = self @@ -290,7 +292,7 @@ impl VCore { fn set_alert_config_on_both_vrms( &self, which: PmbusCmd, - txn: impl Fn(&Raa229620A) -> Result, + txn: impl Fn(&Raa229620A) -> Result<(), raa229620a::Error>, ) -> bool { let mut all_ok = true; all_ok &=