cpu_seq: Fix a variety of hilarious bugs in sequencer IRQ handling (#2390)

Depends on #2392

There are several things wrong with the current handling of sequencer
FPGA interrupts (on Cosmo) and PMBus alerts in the Gimlet sequencer. To
wit:

1. On Cosmo, the `cosmo_seq_server` task does not handle the `NIC_MAPO`
   interrupt from the sequencer FPGA. When the NIC MAPOs, we should be
   transitioning from A0+HP back to A0, rather than just ignoring the
   NIC MAPO event. Presently, ignoring NIC MAPOs result in an improper
   thermal shutdown (see #2384), as the `thermal` task continues to poll
   the NIC's temperature sensor despite it no longer being powered, and
   extrapolates a worst-case temperature trend in the face of I2C NACKs
   from the temperature sensor, which eventually result in a thermal
   shutdown. This isn't great.
2. Both the Cosmo sequencer FPGA interrupt and the Gimlet
   V<sub>core</sub> VRM PMBus alert handling are implemented using the
   STM32 external interrupt (EXTI) peripheral, which only supports
   edge-triggered interrupts (not level-triggered). On Cosmo, where the
   FPGA IRQ line can be triggered by multiple bits in the FPGA's
   Interrupt Flags Register (IFR) being set, this means that we can miss
   interrupts in a situation where one IFR bit is set and then, *after*
   Hubris reads the IFR, a second IFR bit is *also* set while the
   `FPGA1_TO_SP_IRQ_L` line remains asserted. In this case, any bits
   which are set after the initial IFR read are completely missed by
   Hubris.

   Sadly, it turns out that this is often what happens in the event of a
   voltage sag on `V12_SYS_A2` similar to mfg-quality#140. If the
   voltage dips enough to both trigger the RAA229620A's V<sub>in</sub>
   undervolt warning **and** enough to MAPO the T6, the T6 MAPO tends to
   occur while we are in the interrupt handler but after the initial IFR
   read, so the NIC MAPO interrupt tends to be missed. Cool.
3. Our code that attempts to clear PMBus faults in the RAA229620As (on
   Cosmo)/RAA229618 (on Gimlet)...doesn't due that (see #2389), due to
   multiple _hilarious_ bugs:
   - We are currently sending the PMBus `CLEAR_FAULTS` command without a
     PMBus page (rail) associated with it. It turns out that this does
     _not_ actually result in clearing faults on all pages. Instead, it
     results in, uh, nothing happening.

     Quoth the PMBus standard:
     > Commands to clear a bit are gated by the PAGE command. The
     > CLEAR_FAULTS can be made to clear all faults on all pages by
     > setting the page command to FFh.
     >
     > --- _PMBus Power System Mgt Protocol Specification – Part II –
     > Revision 1.3.1_; §10.3 (p. 44)

     So, sending the command without a page doesn't actually do
     anything. That's great.
   - The second thing that stops this from working is _technically_ our
     fault but is due to a decision on the part of the vendor which is
     both profoundly bone-headed and somewhat poorly documented (as is
     traditional). As @nathanaelhuffman discovered, the RAA229620A
     documentation for the `VIN_UV_FAULT_RESPONSE` register, states the
     following:

     > Configures the input undervoltage fault response. **For a fault
     > to be considered cleared, the input voltage must rise by 1/16th
     > of the UV fault threshold value.**
     >
     > --- _R16DS0309EU0100 Rev.1.00_ §11.36 "VIN_OV_FAULT_RESPONSE" (p.
     > 57) (emphasis mine)

     When we configure the undervoltage warning, [we set the threshold
     to 11.75V][1]. If you remember fractions from...elementary
     school?[^1] you may have already realized that 1/16 * 11.75V =
     0.734375V. This means that for the VRM to clear the fault, the
     input voltage must reach 12.484375V (since 11.75V + 0.734375V =
     12.484375V).

     Yes, that is *almost half a volt above the nominal voltage for the
     12V rail*. It isn't supposed to do that! It's supposed to be 12V!!
     So, even if a voltage sag has ended and the input voltage to the
     VRM has returned to 12V, the fault will not clear because the
     voltage has not increased by the requisite 1/16th of the undervolt
     threshold. Isn't that wonderful?
5. There was also a FPGA bug in which the `NIC_MAPO` bit did not
   actually get unset as expected.

This branch unwinds the entire Litany of Sadness and Pain described
above, by doing the following things:

- Integrating with @nathanaelhuffman's changes in
  oxidecomputer/quartz#467, which fixes the bug with clearing the
  `IFR[NIC_MAPO]` bit, and also changes all the IFR flags to be
  write-1-to-clear. Now, we can clear them properly in the Cosmo
  sequencer's routine for handling FPGA IRQs. This required #2392 as the
  new FPGA register map wouldn't build without it.
- Actually making the NIC MAPO interrupt transition the system state
  from A0+HP to A0, which (in conjunction with #2391) fixes #2384.
- Changing the code for clearing RAA22960A and RAA229618 PMBus faults to
  do paged writes rather than unpaged writes, fixing #2389.
- Changing the V<sub>in</sub> undervolt warning threshold from 11.75V to
  11V, so that the fault actually clears when the input voltage returns
  to normal.
- On Cosmo, changing the code for handling the EXTI notification to
  continue reading the FPGA IFR register and clearing any set interrupt
  bits until the IRQ line is actually deasserted in a loop, to
  compensate for the possibility of additional bits being set whilst we
  are already handling an interrupt but after we have read the IFR.

  This does not apply to the PMBus alerts from the RAA229620As, as they
  are set in the IFR as long as the corresponding PMBus alert line to
  the FPGA is asserted, and cannot be cleared by writing back a 1 to the
  IFR. Instead, we mask them out by setting the corresponding bits in
  the FPGA Interrupt Enable Register (IER), so that they can remain
  asserted without the FPGA continuing to assert the IRQ.
- Masking out the RAA229620A PMBus alerts _half-solves_ the problem, but
  it doesn't reset the IER when the PMBus fault actually does clear, so
  we wouldn't be notified of a _subsequent_ fault condition. Thus, we
  must also set a flag that tells the sequencer task to continue trying
  to clear faults in the VRM(s) that asserted the PMBus alert
  periodically until the fault actually goes away. If it does, we then
  re-enable IRQs for the corresponding `PMALERT_L` pin(s).
- A slightly simpler version of that also had to happen on Gimlet. It's
  simpler because Gimlet only monitors a single RAA229618.

Whew okay yeah, I think that's everything. This was a whole thing.

Fixes #2384
Fixes #2389

[1]:
https://github.com/oxidecomputer/hubris/blob/939c48a1ff8e3ab059d8813f6a58b82fdaccc562/drv/cosmo-seq-server/src/vcore.rs#L85-L91
[^1]: Or middle school? When do children learn fractions?

Author: hawkw

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

@@ -16,6 +16,7 @@ use super::i2c_config;
 use drv_i2c_api::ResponseCode;
 use drv_i2c_devices::raa229620a::{self, Raa229620A};
 use fixedstr::FixedStr;
+use pmbus::commands::raa229620a::STATUS_WORD;
 use ringbuf::*;
 use userlib::{sys_get_timer, units, TaskId};
 
@@ -24,14 +25,15 @@ pub(super) struct VCore {
     vddcr_cpu0: Raa229620A,
     /// `PWR_CONT2`: This regulator controls `VDDCR_CPU1` and `VDDIO_SP5` rails.
     vddcr_cpu1: Raa229620A,
+    faulted: Vrms,
     packrat: task_packrat_api::Packrat,
 }
 
 #[derive(Copy, Clone, PartialEq, microcbor::Encode)]
 pub(crate) enum Rail {
-    #[cbor(rename = "VDDCR_CPU0")]
+    #[cbor(rename = "VDDCR_CPU0_A0")]
     VddcrCpu0,
-    #[cbor(rename = "VDDCR_CPU1")]
+    #[cbor(rename = "VDDCR_CPU1_A0")]
     VddcrCpu1,
 }
 
@@ -49,10 +51,9 @@ enum Trace {
     Initializing,
     Initialized,
     LimitsLoaded,
-    FaultsCleared(Rails),
     PmbusAlert {
         timestamp: u64,
-        rails: Rails,
+        alerted: Vrms,
     },
     Reading {
         timestamp: u64,
@@ -64,6 +65,10 @@ enum Trace {
         power_good: bool,
         faulted: bool,
     },
+    TriedToClearFaults {
+        vddcr_cpu0: Status,
+        vddcr_cpu1: Status,
+    },
     StatusWord(Rail, Result<u16, ResponseCode>),
     StatusInput(Rail, Result<u8, ResponseCode>),
     StatusVout(Rail, Result<u8, ResponseCode>),
@@ -75,20 +80,89 @@ enum Trace {
 }
 
 #[derive(Copy, Clone, PartialEq)]
-pub struct Rails {
-    pub vddcr_cpu0: bool,
-    pub vddcr_cpu1: bool,
+enum Status {
+    NotFaulted,
+    // This is on purpose; Humility will display the ringbuf entry as just the
+    // name of the variant, without the enum name.
+    #[allow(clippy::enum_variant_names)]
+    StatusWord(u16),
+    I2cError(ResponseCode),
+}
+
+impl Status {
+    fn from_result(
+        result: Result<STATUS_WORD::CommandData, ResponseCode>,
+    ) -> Self {
+        match result {
+            Ok(s) => Status::StatusWord(s.0),
+            Err(e) => Status::I2cError(e),
+        }
+    }
+
+    fn is_faulted(&self) -> bool {
+        match self {
+            Status::NotFaulted => false,
+            Status::StatusWord(word) => {
+                // Mask out the "off" bit, as "off" is not a fault.
+                //
+                // This is unfortunately the least annoying way to implement
+                // "test if any bits _other_ than this one are set" in the
+                // `pmbus` crate's current API...
+                let off_mask = {
+                    let mut mask = STATUS_WORD::CommandData(0);
+                    mask.set_off(STATUS_WORD::Off::PowerOff);
+                    !mask.0
+                };
+                // Any other `STATUS_WORD` bits indicate a fault.
+                word & off_mask != 0
+            }
+            Status::I2cError(_) => true,
+        }
+    }
+}
+
+#[derive(Copy, Clone, PartialEq)]
+pub struct Vrms {
+    pub pwr_cont1: bool,
+    pub pwr_cont2: bool,
 }
 
 ringbuf!(Trace, 60, Trace::None);
 
 ///
-/// We are going to set our input undervoltage warn limit to be 11.75 volts.
+/// Limit value for input undervoltage *warnings*.
 /// Note that we will not fault if VIN goes below this (that is, we will not
 /// lose POWER_GOOD), but the part will indicate an input fault and pull
 /// on its PMBus alert pin.
 ///
-const VCORE_UV_WARN_LIMIT: units::Volts = units::Volts(11.75);
+/// * * *
+///
+/// Okay, so this number is actually a bit finnicky.  Per the RAA22960A
+/// datasheet, in the documentation for the `VIN_UV_FAULT_RESPONSE` register,
+/// we find the following (emphasis mine):
+///
+/// > Configures the input undervoltage fault response. For a fault to be
+/// > considered cleared, **the input voltage must rise by 1/16th of the UV
+/// > fault threshold value.**
+/// >
+/// > --- R16DS0309EU0100 Rev.1.00 § 11.36, "VIN_UV_FAULT_RESPONSE" (p. 57)
+///
+/// While the documentation does not explicitly state that this also applies
+/// to *warnings* (and there is no corresponding `VIN_UV_WARN_RESPONSE`
+/// register, as the response is always just to assert the PMBus alert pin),
+/// it stands to reason that warnings would also only clear when voltage
+/// rises by 1/16th of the warning threshold value.  Empirical testing reveals
+/// that this is indeed the case.
+///
+/// So, the important thing here is that, when selecting a warning threshold,
+/// we must ensure that `lim + (1/16 * lim)` is less than the expected nominal
+/// input voltage, or else the warning will not clear even if the input voltage
+/// returns to nominal.
+///
+/// For a 12V input, 11V seems like a reasonable undervoltage warning limit.
+/// 1/16 * 11V = 0.6875V, so the warning will clear if the input voltage rises
+/// by at least 11.6875V.
+const VCORE_UV_WARN_LIMIT: units::Volts = units::Volts(11.0);
 
 ///
 /// We want to collect enough samples (at ~900µs per sample per regulator, or
@@ -108,14 +182,23 @@ impl VCore {
 
         let (device, rail) = i2c_config::pmbus::vddcr_cpu1_a0(i2c);
         let vddcr_cpu1 = Raa229620A::new(&device, rail);
+
         Self {
             vddcr_cpu0,
             vddcr_cpu1,
+            faulted: Vrms {
+                pwr_cont1: false,
+                pwr_cont2: false,
+            },
             packrat,
         }
     }
 
-    pub fn initialize_uv_warning(&self) -> Result<(), ResponseCode> {
+    pub fn is_still_faulted(&self) -> bool {
+        self.faulted.pwr_cont1 || self.faulted.pwr_cont2
+    }
+
+    pub fn initialize_uv_warning(&mut self) -> Result<(), ResponseCode> {
         ringbuf_entry!(Trace::Initializing);
 
         // Set our warn limit
@@ -128,10 +211,10 @@ impl VCore {
         ringbuf_entry!(Trace::LimitsLoaded);
 
         // Clear our faults
-        self.clear_faults(Rails {
-            vddcr_cpu0: true,
-            vddcr_cpu1: true,
-        })?;
+        self.try_to_clear_faults(Vrms {
+            pwr_cont1: true,
+            pwr_cont2: true,
+        });
 
         // The higher-level sequencer code will unmask the FPGA interrupts for
         // our guys.
@@ -141,52 +224,81 @@ impl VCore {
         Ok(())
     }
 
-    pub fn clear_faults(&self, which_rails: Rails) -> Result<(), ResponseCode> {
-        if which_rails.vddcr_cpu0 {
-            retry_i2c_txn(Rail::VddcrCpu0, PmbusCmd::ClearFaults, || {
-                self.vddcr_cpu0.clear_faults()
-            })?;
+    pub fn can_we_unmask_any_vrm_irqs_again(&mut self) -> Vrms {
+        self.try_to_clear_faults(self.faulted);
+        Vrms {
+            pwr_cont1: !self.faulted.pwr_cont1,
+            pwr_cont2: !self.faulted.pwr_cont2,
         }
+    }
 
-        if which_rails.vddcr_cpu1 {
-            retry_i2c_txn(Rail::VddcrCpu1, PmbusCmd::ClearFaults, || {
-                self.vddcr_cpu1.clear_faults()
+    fn try_to_clear_faults(&mut self, which_vrms: Vrms) {
+        fn clear_faults(
+            rail: Rail,
+            dev: &Raa229620A,
+        ) -> Result<STATUS_WORD::CommandData, ResponseCode> {
+            retry_i2c_txn(rail, PmbusCmd::ClearFaults, || {
+                dev.clear_faults_on_all_rails()
             })?;
+            Ok(dev.status_word()?)
         }
 
-        ringbuf_entry!(Trace::FaultsCleared(which_rails));
+        let vddcr_cpu0 = if which_vrms.pwr_cont1 {
+            Status::from_result(clear_faults(Rail::VddcrCpu0, &self.vddcr_cpu0))
+        } else {
+            Status::NotFaulted
+        };
 
-        Ok(())
+        let vddcr_cpu1 = if which_vrms.pwr_cont2 {
+            Status::from_result(clear_faults(Rail::VddcrCpu1, &self.vddcr_cpu1))
+        } else {
+            Status::NotFaulted
+        };
+
+        ringbuf_entry!(Trace::TriedToClearFaults {
+            vddcr_cpu0,
+            vddcr_cpu1
+        });
+
+        self.faulted.pwr_cont1 = vddcr_cpu0.is_faulted();
+        self.faulted.pwr_cont2 = vddcr_cpu1.is_faulted();
     }
 
     pub fn handle_pmbus_alert(
-        &self,
-        mut rails: Rails,
+        &mut self,
+        vrms: Vrms,
         now: u64,
         ereport_buf: &mut [u8],
     ) {
         ringbuf_entry!(Trace::PmbusAlert {
             timestamp: now,
-            rails,
+            alerted: vrms,
         });
 
-        let cpu0_state = self.record_pmbus_status(
-            now,
-            Rail::VddcrCpu0,
-            rails.vddcr_cpu0,
-            ereport_buf,
-        );
-        rails.vddcr_cpu0 |= cpu0_state.faulted;
+        let mut input_fault = false;
+        if !self.faulted.pwr_cont1 {
+            let state = self.record_pmbus_status(
+                now,
+                Rail::VddcrCpu0,
+                vrms.pwr_cont1,
+                ereport_buf,
+            );
+            input_fault |= state.input_fault;
+            self.faulted.pwr_cont1 |= state.faulted;
+        }
 
-        let cpu1_state = self.record_pmbus_status(
-            now,
-            Rail::VddcrCpu1,
-            rails.vddcr_cpu1,
-            ereport_buf,
-        );
-        rails.vddcr_cpu1 |= cpu1_state.faulted;
+        if !self.faulted.pwr_cont2 {
+            let state = self.record_pmbus_status(
+                now,
+                Rail::VddcrCpu1,
+                vrms.pwr_cont1,
+                ereport_buf,
+            );
+            input_fault |= state.input_fault;
+            self.faulted.pwr_cont2 |= state.faulted;
+        }
 
-        if cpu0_state.input_fault || cpu1_state.input_fault {
+        if input_fault {
             for _ in 0..VCORE_NSAMPLES {
                 let vddcr_cpu0_vin =
                     self.vddcr_cpu0.read_vin().unwrap_or_else(|e| {
@@ -214,7 +326,7 @@ impl VCore {
                 //
                 // Record our readings, along with a timestamp. On the one hand,
                 // it's a little exceesive to record a timestamp on every
-                // reading: it's in milliseconds, and because it takes ~900µs
+                // reading: it's in milliseconds, and because it takes ~900µs``ws
                 // per reading (so ~1800us to read from both regulators), we
                 // expect the timestamp to (basically) be incremented by 2 with
                 // every reading (with a duplicate timestamp occuring every ~7-9
@@ -231,24 +343,6 @@ impl VCore {
                 });
             }
         }
-
-        // The only way to make the pins deassert (and thus, the IRQ go
-        // away) is to tell the guys to clear the fault.
-        // N.B.: unlike other FPGA sequencer alerts, we need not clear the
-        // IFR bits for these; they are hot as long as the PMALERT pin from
-        // the RAA229620As is asserted.
-        //
-        // Per the RAA229620A datasheet (R16DS0309EU0200 Rev.2.00, page 36),
-        // clearing the fault in the regulator will deassert PMALERT_L,
-        // releasing the IRQ, but the fault bits to be reset if the fault
-        // condition still exists. Note that this does *not* cause the device to
-        // restart if it has shut down. The behavior of "CLEAR_FAULTS" is really
-        // much closer to "ACKNOWLEDGE_PMBUS_ALERT", since it doesn't actually
-        // seem to effect the state of the regulator.
-        //
-        // TODO(eliza): we will want to handle a shut down regulator more
-        // intelligently in future...
-        let _ = self.clear_faults(rails);
     }
 
     fn record_pmbus_status(

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

@@ -75,6 +75,8 @@ pub enum StateChangeReason {
     A0Mapo,
     /// System Management Error
     SmerrAssert,
+    /// NIC MAPO fault from the sequencer.
+    NicMapo,
     /// The system powered off for reasons we can't explain
     Unknown,
 }

drv/cpu-seq-api/src/lib.rs

@@ -65,6 +65,7 @@ enum I2cTxn {
     VCoreOff,
     VCoreUndervoltageInitialize,
     VCorePmbusStatus,
+    VCoreClearFaults,
     SocOn,
     SocOff,
 }
@@ -727,6 +728,10 @@ impl<S: SpiServer> NotificationHandler for ServerImpl<S> {
             }
         }
 
+        if self.vcore.is_faulted() {
+            self.vcore.try_to_clear_faults();
+        }
+
         if let Some(interval) = self.poll_interval() {
             self.deadline += interval;
             sys_set_timer(Some(self.deadline), notifications::TIMER_MASK);
@@ -1163,10 +1168,14 @@ impl<S: SpiServer> ServerImpl<S> {
     // for a thermtrip or for someone disabling NIC_PWREN_L.  If we are in
     // any other state, we don't need to poll.
     //
+    // If the Vcore VRM has a PMBus alert, we must try to clear its faults
+    // periodically, regardless of the current power state.
+    //
     fn poll_interval(&self) -> Option<u64> {
         match self.state {
             PowerState::A0 => Some(10),
             PowerState::A0PlusHP => Some(100),
+            _ if self.vcore.is_faulted() => Some(100),
             _ => None,
         }
     }