Enable and disable Monorail ports based on Ignition status

[mkeeter]

This is untested for now, but I'd like to open it for review.

As part of the periodic wake() function, we ask Ignition for a presence summary, then disable any VSC7448 ports which aren't present on the Ignition network. This is motivated by a desire to reduce EMI, although we're not sure how much of a difference it will actually make.

Todo:

• Test on real hardware
• Add enabling / disabling of downstream PHYs (mostly the VSC8504 port expander)
• Switch to interrupt-driven operation, instead of using wake() (maybe not in this PR)

[rmustacc]

I think it may be worth thinking through the tight coupling this introduces. If we have a failure of ignition to communicate for whatever reason, should that actually be the thing that shuts off these ports? Are there cases that this would actually mask communication when it would actually work? Given that ignition isn't really physical presence, but something more than that, I both get the desire here and am a bit wary.

[jgallagher]

On first skim I was going to ask why we were repeating output_enabled() here as opposed to

if !self.output_enabled(index, v)? {
    return Err(VscError::AlreadyDisabled);
}

before realizing it's because we need value below this check. Would it make sense to extract the read of value into its own private method? Something like

fn read_the_value(index: u8, v: &impl Vsc7448Rw) -> Result<ValueType, VscError> {
    let value = v.read(XGANA(index).SD10G65_OB().SD10G65_OB_CFG0())?;
    Ok(value)
}

for both these new methods to use?

[mkeeter]

I tried it out, but it doesn't look like a significant improvement to me:

+    fn ob_cfg(
+        instance: u8,
+        v: &impl Vsc7448Rw,
+    ) -> Result<vsc7448_pac::hsio::serdes6g_ana_cfg::SERDES6G_OB_CFG, VscError>
+    {
+        serdes6g_read(v, instance)?;
+        v.read(HSIO().SERDES6G_ANA_CFG().SERDES6G_OB_CFG())
+    }
+
     /// Check whether the SERDES output is enabled
     pub fn output_enabled(
         instance: u8,
         v: &impl Vsc7448Rw,
     ) -> Result<bool, VscError> {
-        serdes6g_read(v, instance)?;
-        let value = v.read(HSIO().SERDES6G_ANA_CFG().SERDES6G_OB_CFG())?;
+        let value = Self::ob_cfg(instance, v)?;
         Ok(value.ob_idle() == 0)
     }

@@ -179,15 +187,12 @@ impl Config {
         instance: u8,
         v: &impl Vsc7448Rw,
     ) -> Result<(), VscError> {
-        serdes6g_read(v, instance)?;
-        let ob_cfg = HSIO().SERDES6G_ANA_CFG().SERDES6G_OB_CFG();
-
-        let mut value = v.read(ob_cfg)?;
+        let mut value = Self::ob_cfg(instance, v)?;
         if value.ob_idle() == 1 {
             return Err(VscError::AlreadyDisabled);
         }
         value.set_ob_idle(1);
-        v.write(ob_cfg, value)?;
+        v.write(HSIO().SERDES6G_ANA_CFG().SERDES6G_OB_CFG(), value)?;

         serdes6g_write(v, instance)?;
         Ok(())

[jgallagher]

Yeah, seems like a wash at best. Toss it.

[jgallagher]

Are these here primarily (or exclusively?) for testing via hiffy?

[mkeeter]

Yup, exactly.

[jgallagher]

To Robert's point - it seems like there are a couple ways this could go wrong:

1. If we can't get the presence summary, we won't change whether any ports are enabled/disabled. If we previously disabled all ports that didn't have a sled present, then we start failing to get the presence summary, then we plug in a new sled while we still can't get presence, it won't get its port turned on.
2. If we succeed in getting the presence summary but it incorrectly says a sled is not present on some port when it actually is, we're shutting off management network access to that sled. (Enabling a port if the presence summary incorrectly says a sled is present when it's not seems relatively harmless.)

I have no intution for how likely it is for ignition to fail (in either of these ways) while everything else is still okay, but it seems plausible?

[mkeeter]

I agree with the concerns about having Ignition do this autonomously, and want to hold off on merging until we come to a consensus! Perhaps we should expose the port_output_enable/disable APIs for now, but require the user to call them through Humility / MGS?

Regardless, I decided to do some testing on igor's Sidecar. Things look good: I successfully brought ports down and back up, and the links all came up fine.

I tested ports 18 (SERDES6G), 40 (VSC8504), and 48 (SERDES1G); I'd still like to test one of the ports that's using a SERDES10G.