Merge pull request #125 from FrameworkComputer/autodetect-mec

chromioum_ec: Autodetect Microchip EC when using portio

Author: JohnAZoidberg

README.md

@@ -202,8 +202,6 @@ Options:
           Specify I2C addresses of the PD chips (Advanced)
       --pd-ports <PD_PORTS> <PD_PORTS>
           Specify I2C ports of the PD chips (Advanced)
-      --has-mec <HAS_MEC>
-          Specify the type of EC chip (MEC/MCHP or other) [possible values: true, false]
   -t, --test                        Run self-test to check if interaction with EC is possible
   -h, --help                        Print help information
 ```

completions/bash/framework_tool

@@ -50,7 +50,6 @@ _framework_tool() {
         "--driver"
         "--pd-addrs"
         "--pd-ports"
-        "--has-mec"
         "-t" "--test"
         "-h" "--help"
     )
@@ -59,7 +58,6 @@ _framework_tool() {
     local inputdeck_modes=("auto" "off" "on")
     local console_modes=("recent" "follow")
     local drivers=("portio" "cros-ec" "windows")
-    local has_mec_options=("true" "false")
     local brightness_options=("high" "medium" "low" "ultra-low")
 
     local current_word prev_word
@@ -77,8 +75,6 @@ _framework_tool() {
         COMPREPLY=( $(compgen -W "${console_modes[*]}" -- "$current_word") )
     elif [[ $prev_word == "--driver" ]]; then
         COMPREPLY=( $(compgen -W "${drivers[*]}" -- "$current_word") )
-    elif [[ $prev_word == "--has-mec" ]]; then
-        COMPREPLY=( $(compgen -W "${has_mec_options[*]}" -- "$current_word") )
     elif [[ $prev_word == "--fp-brightness" ]]; then
         COMPREPLY=( $(compgen -W "${brightness_options[*]}" -- "$current_word") )
     fi

completions/zsh/_framework_tool

@@ -48,7 +48,6 @@ options=(
   '--driver[Select which driver is used]:driver:(portio cros-ec windows)'
   '--pd-addrs[Specify I2C addresses of the PD chips (Advanced)]:pd_addrs'
   '--pd-ports[Specify I2C ports of the PD chips (Advanced)]:pd_ports'
-  '--has-mec[Specify the type of EC chip (MEC/MCHP or other)]:has_mec:(true false)'
   '-t[Run self-test to check if interaction with EC is possible]'
   '-h[Print help]'
 )

framework_lib/src/ccgx/device.rs

@@ -35,8 +35,8 @@ impl PdPort {
         let platform = &(*config).as_ref().unwrap().platform;
 
         match (platform, self) {
-            (Platform::GenericFramework((left, _), _, _), PdPort::Left01) => *left,
-            (Platform::GenericFramework((_, right), _, _), PdPort::Right23) => *right,
+            (Platform::GenericFramework((left, _), _), PdPort::Left01) => *left,
+            (Platform::GenericFramework((_, right), _), PdPort::Right23) => *right,
             // Framework AMD Platforms (CCG8)
             (
                 Platform::Framework13Amd7080
@@ -64,8 +64,8 @@ impl PdPort {
         let platform = &(*config).as_ref().unwrap().platform;
 
         Ok(match (platform, self) {
-            (Platform::GenericFramework(_, (left, _), _), PdPort::Left01) => *left,
-            (Platform::GenericFramework(_, (_, right), _), PdPort::Right23) => *right,
+            (Platform::GenericFramework(_, (left, _)), PdPort::Left01) => *left,
+            (Platform::GenericFramework(_, (_, right)), PdPort::Right23) => *right,
             (Platform::IntelGen11, _) => 6,
             (Platform::IntelGen12 | Platform::IntelGen13, PdPort::Left01) => 6,
             (Platform::IntelGen12 | Platform::IntelGen13, PdPort::Right23) => 7,

framework_lib/src/chromium_ec/mod.rs

@@ -202,24 +202,6 @@ const BOARD_VERSION_NPC_DB: [i32; BOARD_VERSION_COUNT] = [
     100, 311, 521, 721, 931, 1131, 1341, 1551, 1751, 1961, 2171, 2370, 2580, 2780, 2990, 3200,
 ];
 
-pub fn has_mec() -> bool {
-    let platform = smbios::get_platform().unwrap();
-    if let Platform::GenericFramework(_, _, has_mec) = platform {
-        return has_mec;
-    }
-
-    // TODO: Should turn this around
-    !matches!(
-        smbios::get_platform().unwrap(),
-        Platform::Framework13Amd7080
-            | Platform::Framework16Amd7080
-            | Platform::IntelCoreUltra1
-            | Platform::Framework13AmdAi300
-            | Platform::Framework12IntelGen13
-            | Platform::FrameworkDesktopAmdAiMax300
-    )
-}
-
 pub trait CrosEcDriver {
     fn read_memory(&self, offset: u16, length: u16) -> Option<Vec<u8>>;
     fn send_command(&self, command: u16, command_version: u8, data: &[u8]) -> EcResult<Vec<u8>>;
@@ -269,6 +251,7 @@ impl CrosEc {
             return None;
         }
         debug!("Chromium EC Driver: {:?}", driver);
+
         Some(CrosEc { driver })
     }
 
@@ -953,30 +936,25 @@ impl CrosEc {
         // Everything before is probably a header.
         // TODO: I don't think there are magic bytes on zephyr firmware
         //
-        if has_mec() {
-            println!("    Check MCHP magic byte at start of firmware code.");
-            // Make sure we can read at an offset and with arbitrary length
-            let data = self.read_ec_flash(FLASH_PROGRAM_OFFSET, 16).unwrap();
-            debug!("Expecting beginning with 50 48 43 4D ('PHCM' in ASCII)");
-            debug!("{:02X?}", data);
-            println!(
-                "      {:02X?} ASCII:{:?}",
-                &data[..4],
-                core::str::from_utf8(&data[..4])
-            );
-
-            if data[0..4] != [0x50, 0x48, 0x43, 0x4D] {
-                println!("      INVALID: {:02X?}", &data[0..3]);
-                res = Err(EcError::DeviceError(format!(
-                    "INVALID: {:02X?}",
-                    &data[0..3]
-                )));
-            }
+        debug!("    Check MCHP magic bytes at start of firmware code.");
+        // Make sure we can read at an offset and with arbitrary length
+        let data = self.read_ec_flash(FLASH_PROGRAM_OFFSET, 16).unwrap();
+        debug!("Expecting beginning with 50 48 43 4D ('PHCM' in ASCII)");
+        debug!("{:02X?}", data);
+        debug!(
+            "      {:02X?} ASCII:{:?}",
+            &data[..4],
+            core::str::from_utf8(&data[..4])
+        );
+
+        let has_mec = data[0..4] == [0x50, 0x48, 0x43, 0x4D];
+        if has_mec {
+            debug!("    Found MCHP magic bytes at start of firmware code.");
         }
 
         // ===== Test 4 =====
         println!("    Read flash flags");
-        let data = if has_mec() {
+        let data = if has_mec {
             self.read_ec_flash(MEC_FLASH_FLAGS, 0x80).unwrap()
         } else {
             self.read_ec_flash(NPC_FLASH_FLAGS, 0x80).unwrap()

framework_lib/src/chromium_ec/portio.rs

@@ -12,10 +12,9 @@ use log::Level;
 #[cfg(feature = "linux_pio")]
 use nix::unistd::Uid;
 use num::FromPrimitive;
-#[cfg(feature = "linux_pio")]
-use std::sync::{Arc, Mutex};
+use spin::Mutex;
 
-use crate::chromium_ec::{has_mec, portio_mec};
+use crate::chromium_ec::{portio_mec, EC_MEMMAP_ID};
 use crate::os_specific;
 use crate::util;
 
@@ -172,65 +171,68 @@ fn transfer_read(port: u16, address: u16, size: u16) -> Vec<u8> {
     buffer
 }
 
-#[cfg(feature = "linux_pio")]
+#[derive(PartialEq)]
+#[allow(dead_code)]
 enum Initialized {
     NotYet,
+    SucceededMec,
     Succeeded,
     Failed,
 }
 
-#[cfg(feature = "linux_pio")]
 lazy_static! {
-    static ref INITIALIZED: Arc<Mutex<Initialized>> = Arc::new(Mutex::new(Initialized::NotYet));
+    static ref INITIALIZED: Mutex<Initialized> = Mutex::new(Initialized::NotYet);
 }
 
-#[cfg(not(feature = "linux_pio"))]
-fn init() -> bool {
-    // Nothing to do for bare-metal (UEFI) port I/O
-    true
+fn has_mec() -> bool {
+    let init = INITIALIZED.lock();
+    *init != Initialized::Succeeded
 }
 
-// In Linux userspace has to first request access to ioports
-// TODO: Close these again after we're done
-#[cfg(feature = "linux_pio")]
 fn init() -> bool {
-    let mut init = INITIALIZED.lock().unwrap();
+    let mut init = INITIALIZED.lock();
     match *init {
         // Can directly give up, trying again won't help
         Initialized::Failed => return false,
         // Already initialized, no need to do anything.
-        Initialized::Succeeded => return true,
+        Initialized::Succeeded | Initialized::SucceededMec => return true,
         Initialized::NotYet => {}
     }
 
+    // First try on MEC
+    portio_mec::init();
+    let ec_id = portio_mec::transfer_read(MEC_MEMMAP_OFFSET + EC_MEMMAP_ID, 2);
+    if ec_id[0] == b'E' && ec_id[1] == b'C' {
+        *init = Initialized::SucceededMec;
+        return true;
+    }
+
+    // In Linux userspace has to first request access to ioports
+    // TODO: Close these again after we're done
+    #[cfg(feature = "linux_pio")]
     if !Uid::effective().is_root() {
         error!("Must be root to use port based I/O for EC communication.");
         *init = Initialized::Failed;
         return false;
     }
-
+    #[cfg(feature = "linux_pio")]
     unsafe {
-        if has_mec() {
-            portio_mec::mec_init();
-        } else {
-            // 8 for request/response header, 0xFF for response
-            let res = ioperm(EC_LPC_ADDR_HOST_ARGS as u64, 8 + 0xFF, 1);
-            if res != 0 {
-                error!(
-                    "ioperm failed. portio driver is likely block by Linux kernel lockdown mode"
-                );
-                return false;
-            }
-
-            let res = ioperm(EC_LPC_ADDR_HOST_CMD as u64, 1, 1);
-            assert_eq!(res, 0);
-            let res = ioperm(EC_LPC_ADDR_HOST_DATA as u64, 1, 1);
-            assert_eq!(res, 0);
-
-            let res = ioperm(NPC_MEMMAP_OFFSET as u64, super::EC_MEMMAP_SIZE as u64, 1);
-            assert_eq!(res, 0);
+        // 8 for request/response header, 0xFF for response
+        let res = ioperm(EC_LPC_ADDR_HOST_ARGS as u64, 8 + 0xFF, 1);
+        if res != 0 {
+            error!("ioperm failed. portio driver is likely block by Linux kernel lockdown mode");
+            return false;
         }
+
+        let res = ioperm(EC_LPC_ADDR_HOST_CMD as u64, 1, 1);
+        assert_eq!(res, 0);
+        let res = ioperm(EC_LPC_ADDR_HOST_DATA as u64, 1, 1);
+        assert_eq!(res, 0);
+
+        let res = ioperm(NPC_MEMMAP_OFFSET as u64, super::EC_MEMMAP_SIZE as u64, 1);
+        assert_eq!(res, 0);
     }
+
     *init = Initialized::Succeeded;
     true
 }

framework_lib/src/chromium_ec/portio_mec.rs

@@ -22,10 +22,12 @@ const _MEC_LPC_DATA_REGISTER1: u16 = 0x0805;
 const MEC_LPC_DATA_REGISTER2: u16 = 0x0806;
 const _MEC_LPC_DATA_REGISTER3: u16 = 0x0807;
 
-#[cfg(feature = "linux_pio")]
-pub unsafe fn mec_init() {
-    ioperm(EC_LPC_ADDR_HOST_DATA as u64, 8, 1);
-    ioperm(MEC_LPC_ADDRESS_REGISTER0 as u64, 10, 1);
+pub fn init() {
+    #[cfg(feature = "linux_pio")]
+    unsafe {
+        ioperm(EC_LPC_ADDR_HOST_DATA as u64, 8, 1);
+        ioperm(MEC_LPC_ADDRESS_REGISTER0 as u64, 10, 1);
+    }
 }
 
 // TODO: Create a wrapper

framework_lib/src/commandline/clap_std.rs

@@ -228,20 +228,15 @@ struct ClapCli {
     driver: Option<CrosEcDriverType>,
 
     /// Specify I2C addresses of the PD chips (Advanced)
-    #[clap(number_of_values = 2, requires("pd_ports"), requires("has_mec"))]
+    #[clap(number_of_values = 2, requires("pd_ports"))]
     #[arg(long)]
     pd_addrs: Vec<u16>,
 
     /// Specify I2C ports of the PD chips (Advanced)
-    #[clap(number_of_values = 2, requires("pd_addrs"), requires("has_mec"))]
+    #[clap(number_of_values = 2, requires("pd_addrs"))]
     #[arg(long)]
     pd_ports: Vec<u8>,
 
-    /// Specify the type of EC chip (MEC/MCHP or other)
-    #[clap(requires("pd_addrs"), requires("pd_ports"))]
-    #[arg(long)]
-    has_mec: Option<bool>,
-
     /// Run self-test to check if interaction with EC is possible
     #[arg(long, short)]
     test: bool,
@@ -408,7 +403,6 @@ pub fn parse(args: &[String]) -> Cli {
         driver: args.driver,
         pd_addrs,
         pd_ports,
-        has_mec: args.has_mec,
         test: args.test,
         // TODO: Set help. Not very important because Clap handles this by itself
         help: false,

framework_lib/src/commandline/mod.rs

@@ -191,7 +191,6 @@ pub struct Cli {
     pub hash: Option<String>,
     pub pd_addrs: Option<(u16, u16)>,
     pub pd_ports: Option<(u8, u8)>,
-    pub has_mec: Option<bool>,
     pub help: bool,
     pub info: bool,
     pub flash_gpu_descriptor: Option<(u8, String)>,
@@ -701,12 +700,8 @@ pub fn run_with_args(args: &Cli, _allupdate: bool) -> i32 {
     }
 
     // Must be run before any application code to set the config
-    if args.pd_addrs.is_some() && args.pd_ports.is_some() && args.has_mec.is_some() {
-        let platform = Platform::GenericFramework(
-            args.pd_addrs.unwrap(),
-            args.pd_ports.unwrap(),
-            args.has_mec.unwrap(),
-        );
+    if args.pd_addrs.is_some() && args.pd_ports.is_some() {
+        let platform = Platform::GenericFramework(args.pd_addrs.unwrap(), args.pd_ports.unwrap());
         Config::set(platform);
     }
 
@@ -1169,7 +1164,7 @@ fn selftest(ec: &CrosEc) -> Option<()> {
     } else {
         println!("  SMBIOS Platform:     Unknown");
         println!();
-        println!("Specify custom platform parameters with --pd-ports --pd-addrs --has-mec");
+        println!("Specify custom platform parameters with --pd-ports --pd-addrs");
         return None;
     };
 

framework_lib/src/commandline/uefi.rs

@@ -106,7 +106,6 @@ pub fn parse(args: &[String]) -> Cli {
         driver: Some(CrosEcDriverType::Portio),
         pd_addrs: None,
         pd_ports: None,
-        has_mec: None,
         test: false,
         help: false,
         flash_gpu_descriptor: None,
@@ -610,22 +609,6 @@ pub fn parse(args: &[String]) -> Cli {
                 None
             };
             found_an_option = true;
-        } else if arg == "--has-mec" {
-            cli.has_mec = if args.len() > i + 1 {
-                if let Ok(b) = args[i + 1].parse::<bool>() {
-                    Some(b)
-                } else {
-                    println!(
-                        "Invalid value for --has-mec: '{}'. Must be 'true' or 'false'.",
-                        args[i + 1]
-                    );
-                    None
-                }
-            } else {
-                println!("--has-mec requires extra boolean argument.");
-                None
-            };
-            found_an_option = true;
         } else if arg == "--raw-command" {
             cli.raw_command = args[1..].to_vec();
         } else if arg == "--compare-version" {
@@ -699,11 +682,10 @@ pub fn parse(args: &[String]) -> Cli {
         }
     }
 
-    let custom_platform = cli.pd_addrs.is_some() && cli.pd_ports.is_some() && cli.has_mec.is_some();
-    let no_customization =
-        cli.pd_addrs.is_none() && cli.pd_ports.is_none() && cli.has_mec.is_none();
+    let custom_platform = cli.pd_addrs.is_some() && cli.pd_ports.is_some();
+    let no_customization = cli.pd_addrs.is_none() && cli.pd_ports.is_none();
     if !(custom_platform || no_customization) {
-        println!("To customize the platform you need to provide all of --pd-addrs, --pd-ports and --has-mec");
+        println!("To customize the platform you need to provide all of --pd-addrs, and --pd-ports");
     }
 
     if args.len() == 1 && cli.paginate {