Make enum encoded FPGA register fields more flexible (#1766)

I'll start off with a bit of context since this is a corner of the
system that doesn't get looked at often. We use SystemRDL to define
registers in the FPGA. When we build an FPGA, one of the outputs is a
JSON file that describes the registers. We commit that JSON file
alongside the bitfile in this repo, which then gets turned into Rust by
`build/fpga-regmap`. A neat part of this is that we can define enums as
a register field in the RTL and carry them into the Rust code via a
property we call `encode`.

For example, consider the [`STATUS_PORT0`
register](https://github.com/oxidecomputer/hubris/blob/master/drv/sidecar-front-io/sidecar_qsfp_x32_controller_regs.json#L1154-L1213).
Today, that would generate the following code:
```rust
...
    #[allow(non_snake_case)]
        pub mod STATUS_PORT0 {
            #[allow(dead_code)]
            #[allow(non_upper_case_globals)]
            pub const ERROR: u8 = 0b00001111;

            use num_derive::{ToPrimitive, FromPrimitive};
            #[derive(Copy, Clone, Eq, PartialEq, FromPrimitive, ToPrimitive)]
            #[allow(dead_code)]
            #[allow(non_camel_case_types, clippy::upper_case_acronyms)]
            pub enum Encoded {
                NoError = 0x00,
                NoModule = 0x01,
                NoPower = 0x02,
                PowerFault = 0x03,
                NotInitialized = 0x04,
                I2cAddressNack = 0x05,
                I2cByteNack = 0x06,
            }
            #[allow(dead_code)]
            #[allow(non_upper_case_globals)]
            pub const BUSY: u8 = 0b00010000;
        }
...
```

The presence of `encode` in the JSON yields an `Encoded` enum for the
register in the Rust. This has a couple of sharp corners, both of which
come from behavior that assumes an `encode` type is the only field in a
register.

1. You can't have two `encoded` fields within the same register because
we currently just name the enum `Encoded`.
2. If you `encoded` field lives alongside any other fields, such as is
the case with `BUSY` and `ERROR` above, you need to make sure you're
masking off the bits for the `encoded` field only when attempting to
resolve to an `Encoded` variant.

This PR addresses both of these points by:

1. Naming the enum after the field it is encoding, i.e.
`<field>_Encoded`.
2. Implementing `TryFrom<u8>` for the enum and having that automatically
handle the bit masking for the field so the programmer doesn't have to
remember that.


After this PR, that would look like:
```rust
...
        #[allow(non_snake_case)]
        pub mod STATUS_PORT0 {
            #[allow(dead_code)]
            #[allow(non_upper_case_globals)]
            pub const ERROR: u8 = 0b00001111;

            #[derive(Copy, Clone, Eq, PartialEq)]
            #[allow(dead_code)]
            pub enum ErrorEncoded {
                NoError = 0x00,
                NoModule = 0x01,
                NoPower = 0x02,
                PowerFault = 0x03,
                NotInitialized = 0x04,
                I2CAddressNack = 0x05,
                I2CByteNack = 0x06,
            }

            impl TryFrom<u8> for ErrorEncoded {
                type Error = ();
                fn try_from(x: u8) -> Result<Self, Self::Error> {
                    use crate::Reg::QSFP::STATUS_PORT0::ErrorEncoded::*;
                    let x_masked = x & ERROR;
                    match x_masked {
                        0x00 => Ok(NoError),
                        0x01 => Ok(NoModule),
                        0x02 => Ok(NoPower),
                        0x03 => Ok(PowerFault),
                        0x04 => Ok(NotInitialized),
                        0x05 => Ok(I2CAddressNack),
                        0x06 => Ok(I2CByteNack),
                        _ => Err(()),
                    }
                }
            }

            #[allow(dead_code)]
            #[allow(non_upper_case_globals)]
            pub const BUSY: u8 = 0b00010000;
        }
...
```

build/fpga-regmap/Cargo.toml

@@ -4,5 +4,6 @@ version = "0.1.0"
 edition = "2021"
 
 [dependencies]
-serde = { workspace = true }
-serde_json = { workspace = true }
+convert_case.workspace = true
+serde.workspace = true
+serde_json.workspace = true

build/fpga-regmap/src/lib.rs

@@ -2,6 +2,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
+use convert_case::{Case, Casing};
 use serde::Deserialize;
 use std::fmt::Write;
 
@@ -136,7 +137,15 @@ impl Addr {{
 
 ////////////////////////////////////////////////////////////////////////////////
 
-fn write_reg_fields(children: &[Node], prefix: &str, output: &mut String) {
+fn write_reg_fields(
+    parents: Vec<String>,
+    children: &[Node],
+    prefix: &str,
+    output: &mut String,
+) {
+    // We need this to implement the u8 -> enum conversion
+    let parent_chain = parents.join("::");
+
     for child in children.iter() {
         if let Node::Field {
             inst_name,
@@ -155,28 +164,67 @@ fn write_reg_fields(children: &[Node], prefix: &str, output: &mut String) {
 {prefix}        pub const {inst_name}: u8 = 0b{mask:08b};",
             )
             .unwrap();
+
             // Deal with optional encoded Enums on this field
             match encode {
                 Some(x) => {
+                    let name_camel = inst_name.to_case(Case::UpperCamel);
+                    let encode_name = format!("{name_camel}Encoded");
                     writeln!(
                         output,
                         "
-{prefix}        use num_derive::{{ToPrimitive, FromPrimitive}};
-{prefix}        #[derive(Copy, Clone, Eq, PartialEq, FromPrimitive, ToPrimitive)]
+{prefix}        #[derive(Copy, Clone, Eq, PartialEq)]
 {prefix}        #[allow(dead_code)]
-{prefix}        #[allow(non_camel_case_types, clippy::upper_case_acronyms)]
-{prefix}        pub enum Encoded {{"
+{prefix}        pub enum {encode_name} {{"
                     )
                     .unwrap();
+
+                    // unpack all of the enum variant -> u8 information
                     for item in x {
                         writeln!(
                             output,
                             "{prefix}            {0} = {1:#04x},",
-                            item.name, item.value
+                            item.name.to_case(Case::UpperCamel),
+                            item.value
                         )
                         .unwrap();
                     }
+
                     writeln!(output, "{prefix}        }}").unwrap();
+
+                    // We want to implement TryFrom<u8> rather than From<u8>
+                    // because the u8 -> enum conversion can fail if the value
+                    // is not a valid enum variant. Additionally, we mask off
+                    // the supplied u8 with the mask of the field so encoded
+                    // fields could be colocated in a register with other fields
+                    writeln!(
+                        output,
+                        "
+{prefix}        impl TryFrom<u8> for {encode_name} {{
+{prefix}            type Error = ();
+{prefix}            fn try_from(x: u8) -> Result<Self, Self::Error> {{
+{prefix}                use crate::{parent_chain}::{encode_name}::*;
+{prefix}                let x_masked = x & {inst_name};
+{prefix}                match x_masked {{"
+                    )
+                    .unwrap();
+                    for item in x {
+                        writeln!(
+                            output,
+                            "{prefix}                    {1:#04x} => Ok({0}),",
+                            item.name.to_case(Case::UpperCamel),
+                            item.value
+                        )
+                        .unwrap();
+                    }
+                    writeln!(
+                        output,
+                        "{prefix}                    _ => Err(()),
+{prefix}                }}
+{prefix}            }}
+{prefix}        }}\n"
+                    )
+                    .unwrap();
                 }
                 None => {}
             }
@@ -186,7 +234,12 @@ fn write_reg_fields(children: &[Node], prefix: &str, output: &mut String) {
     }
 }
 
-fn write_node(node: &Node, prefix: &str, output: &mut String) {
+fn write_node(
+    parents: &[String],
+    node: &Node,
+    prefix: &str,
+    output: &mut String,
+) {
     match node {
         Node::Reg {
             inst_name,
@@ -197,15 +250,18 @@ fn write_node(node: &Node, prefix: &str, output: &mut String) {
             if *regwidth != 8 {
                 panic!("only 8-bit registers supported");
             }
-
             writeln!(
                 output,
                 "\
 {prefix}    #[allow(non_snake_case)]
 {prefix}    pub mod {inst_name} {{",
             )
             .unwrap();
-            write_reg_fields(children, prefix, output);
+
+            // Extend the knowledge of parents as we descend
+            let mut new_parents = parents.to_owned();
+            new_parents.push(inst_name.clone());
+            write_reg_fields(new_parents, children, prefix, output);
 
             writeln!(output, "{prefix}    }}").unwrap();
         }
@@ -235,7 +291,15 @@ fn write_node(node: &Node, prefix: &str, output: &mut String) {
 {prefix}    pub mod {inst_name} {{",
             )
             .unwrap();
-            recurse_reg_map(children, &format!("    {prefix}"), output);
+
+            let mut new_parents = parents.to_owned();
+            new_parents.push(inst_name.clone());
+            recurse_reg_map(
+                &new_parents,
+                children,
+                &format!("    {prefix}"),
+                output,
+            );
             writeln!(output, "{prefix}    }}").unwrap();
         }
 
@@ -245,9 +309,14 @@ fn write_node(node: &Node, prefix: &str, output: &mut String) {
     }
 }
 
-fn recurse_reg_map(children: &[Node], prefix: &str, output: &mut String) {
+fn recurse_reg_map(
+    parents: &[String],
+    children: &[Node],
+    prefix: &str,
+    output: &mut String,
+) {
     for child in children.iter() {
-        write_node(child, prefix, output);
+        write_node(parents, child, prefix, output);
     }
 }
 
@@ -266,7 +335,10 @@ pub mod Reg {{"
     )
     .unwrap();
 
-    recurse_reg_map(children, "", output);
+    // The nested layers may require type information that requires knowledge
+    // of where they are in the tree.
+    let root = vec!["Reg".to_string()];
+    recurse_reg_map(&root, children, "", output);
 
     writeln!(output, "}}").unwrap();
 }

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

@@ -730,7 +730,7 @@ impl<S: SpiServer> ServerImpl<S> {
                         .unwrap_lite();
                     ringbuf_entry!(Trace::A1Status(status[0]));
 
-                    if status[0] == Reg::A1SMSTATUS::Encoded::DONE as u8 {
+                    if status[0] == Reg::A1SMSTATUS::A1SmEncoded::Done as u8 {
                         break;
                     }
 
@@ -786,7 +786,8 @@ impl<S: SpiServer> ServerImpl<S> {
                         .unwrap_lite();
                     ringbuf_entry!(Trace::A0Status(status[0]));
 
-                    if status[0] == Reg::A0SMSTATUS::Encoded::GROUPC_PG as u8 {
+                    if status[0] == Reg::A0SMSTATUS::A0SmEncoded::GroupcPg as u8
+                    {
                         break;
                     }
 
@@ -818,7 +819,7 @@ impl<S: SpiServer> ServerImpl<S> {
                         .unwrap_lite();
                     ringbuf_entry!(Trace::A0Power(status[0]));
 
-                    if status[0] == Reg::A0SMSTATUS::Encoded::DONE as u8 {
+                    if status[0] == Reg::A0SMSTATUS::A0SmEncoded::Done as u8 {
                         break;
                     }
 

drv/sidecar-front-io/src/transceivers.rs

@@ -6,7 +6,7 @@ use crate::{Addr, Reg};
 use drv_fpga_api::{FpgaError, FpgaUserDesign, WriteOp};
 use drv_transceivers_api::{ModuleStatus, TransceiversError, NUM_PORTS};
 use transceiver_messages::ModuleId;
-use userlib::{FromPrimitive, UnwrapLite};
+use userlib::UnwrapLite;
 use zerocopy::{byteorder, AsBytes, FromBytes, Unaligned, U16};
 
 // The transceiver modules are split across two FPGAs on the QSFP Front IO
@@ -651,7 +651,7 @@ impl ModuleResultNoFailure {
 }
 
 /// A type to provide more ergonomic access to the FPGA generated type
-pub type FpgaI2CFailure = Reg::QSFP::PORT0_STATUS::Encoded;
+pub type FpgaI2CFailure = Reg::QSFP::PORT0_STATUS::ErrorEncoded;
 
 /// A type to consolidate per-module failure types.
 ///
@@ -685,10 +685,7 @@ impl PortI2CStatus {
     pub fn new(status: u8) -> Self {
         Self {
             done: (status & Reg::QSFP::PORT0_STATUS::BUSY) == 0,
-            error: FpgaI2CFailure::from_u8(
-                status & Reg::QSFP::PORT0_STATUS::ERROR,
-            )
-            .unwrap_lite(),
+            error: FpgaI2CFailure::try_from(status).unwrap_lite(),
         }
     }
 }
@@ -1233,11 +1230,11 @@ impl Transceivers {
                 }
 
                 // cast status byte into a type
-                let failure = FpgaI2CFailure::from_u8(
-                    status_all.status[port.0 as usize]
-                        & Reg::QSFP::PORT0_STATUS::ERROR,
+                let failure = FpgaI2CFailure::try_from(
+                    status_all.status[port.0 as usize],
                 )
                 .unwrap_lite();
+
                 // if a failure occurred, mark it and record the failure type
                 if failure != FpgaI2CFailure::NoError {
                     physical_failure.get_mut(fpga_index).set(port);

drv/transceivers-server/src/main.rs

@@ -26,7 +26,7 @@ use task_thermal_api::{Thermal, ThermalError, ThermalProperties};
 use transceiver_messages::{
     message::LedState, mgmt::ManagementInterface, MAX_PACKET_SIZE,
 };
-use userlib::{sys_get_timer, task_slot, units::Celsius, FromPrimitive};
+use userlib::{sys_get_timer, task_slot, units::Celsius};
 use zerocopy::{AsBytes, FromBytes};
 
 mod udp; // UDP API is implemented in a separate file
@@ -60,10 +60,10 @@ enum Trace {
     UnknownInterface(u8, ManagementInterface),
     UnpluggedModule(usize),
     RemovedDisabledModuleThermalModel(usize),
-    TemperatureReadError(usize, Reg::QSFP::PORT0_STATUS::Encoded),
+    TemperatureReadError(usize, Reg::QSFP::PORT0_STATUS::ErrorEncoded),
     TemperatureReadUnexpectedError(usize, FpgaError),
     ThermalError(usize, ThermalError),
-    GetInterfaceError(usize, Reg::QSFP::PORT0_STATUS::Encoded),
+    GetInterfaceError(usize, Reg::QSFP::PORT0_STATUS::ErrorEncoded),
     GetInterfaceUnexpectedError(usize, FpgaError),
     InvalidPortStatusError(usize, u8),
     DisablingPorts(LogicalPortMask),
@@ -359,11 +359,12 @@ impl ServerImpl {
                             self.decode_interface(port, interface)
                     }
                     Err(FpgaError::ImplError(e)) => {
-                        match Reg::QSFP::PORT0_STATUS::Encoded::from_u8(e) {
-                            Some(val) => {
+                        match Reg::QSFP::PORT0_STATUS::ErrorEncoded::try_from(e)
+                        {
+                            Ok(val) => {
                                 ringbuf_entry!(Trace::GetInterfaceError(i, val))
                             }
-                            None => {
+                            Err(_) => {
                                 // Error code cannot be decoded
                                 ringbuf_entry!(Trace::InvalidPortStatusError(
                                     i, e
@@ -445,13 +446,14 @@ impl ServerImpl {
                 // be transient (and we'll remove the transceiver on the
                 // next pass through this function).
                 Err(FpgaError::ImplError(e)) => {
-                    use Reg::QSFP::PORT0_STATUS::Encoded;
-                    match Encoded::from_u8(e) {
-                        Some(val) => {
-                            got_nack |= matches!(val, Encoded::I2cAddressNack);
+                    use Reg::QSFP::PORT0_STATUS::ErrorEncoded;
+                    match ErrorEncoded::try_from(e) {
+                        Ok(val) => {
+                            got_nack |=
+                                matches!(val, ErrorEncoded::I2CAddressNack);
                             ringbuf_entry!(Trace::TemperatureReadError(i, val))
                         }
-                        None => {
+                        Err(_) => {
                             // Error code cannot be decoded
                             ringbuf_entry!(Trace::InvalidPortStatusError(i, e))
                         }

drv/transceivers-server/src/udp.rs

@@ -788,10 +788,10 @@ impl ServerImpl {
                         FpgaI2CFailure::NotInitialized => {
                             HwError::NotInitialized
                         }
-                        FpgaI2CFailure::I2cAddressNack => {
+                        FpgaI2CFailure::I2CAddressNack => {
                             HwError::I2cAddressNack
                         }
-                        FpgaI2CFailure::I2cByteNack => HwError::I2cByteNack,
+                        FpgaI2CFailure::I2CByteNack => HwError::I2cByteNack,
                         // We only mark failures when an error is set, so this
                         // branch should never match.
                         FpgaI2CFailure::NoError => unreachable!(),