From aad94339fc3ea449e4a3d9e9036d15e4820e8572 Mon Sep 17 00:00:00 2001 From: Daniel Schaefer Date: Fri, 2 May 2025 19:21:32 +0800 Subject: [PATCH 1/2] chromioum_ec: Autodetect Microchip EC when using portio Signed-off-by: Daniel Schaefer --- framework_lib/src/chromium_ec/mod.rs | 1 + framework_lib/src/chromium_ec/portio.rs | 72 +++++++++++---------- framework_lib/src/chromium_ec/portio_mec.rs | 10 +-- 3 files changed, 44 insertions(+), 39 deletions(-) diff --git a/framework_lib/src/chromium_ec/mod.rs b/framework_lib/src/chromium_ec/mod.rs index 4ca04926..30621225 100644 --- a/framework_lib/src/chromium_ec/mod.rs +++ b/framework_lib/src/chromium_ec/mod.rs @@ -268,6 +268,7 @@ impl CrosEc { return None; } debug!("Chromium EC Driver: {:?}", driver); + Some(CrosEc { driver }) } diff --git a/framework_lib/src/chromium_ec/portio.rs b/framework_lib/src/chromium_ec/portio.rs index b453c41c..1f480187 100644 --- a/framework_lib/src/chromium_ec/portio.rs +++ b/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 { 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> = Arc::new(Mutex::new(Initialized::NotYet)); + static ref INITIALIZED: Mutex = 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 } diff --git a/framework_lib/src/chromium_ec/portio_mec.rs b/framework_lib/src/chromium_ec/portio_mec.rs index 974b8686..471a3219 100644 --- a/framework_lib/src/chromium_ec/portio_mec.rs +++ b/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 From 8799d7b4161bcdfa1cd1a00fd6140a6faa707652 Mon Sep 17 00:00:00 2001 From: Daniel Schaefer Date: Sat, 3 May 2025 20:19:37 +0800 Subject: [PATCH 2/2] commandline: Remove --has-mec Now we can autodetect, we don't need it anymore, yay! Signed-off-by: Daniel Schaefer --- README.md | 2 - completions/bash/framework_tool | 4 -- completions/zsh/_framework_tool | 1 - framework_lib/src/ccgx/device.rs | 8 ++-- framework_lib/src/chromium_ec/mod.rs | 53 +++++++---------------- framework_lib/src/commandline/clap_std.rs | 10 +---- framework_lib/src/commandline/mod.rs | 11 ++--- framework_lib/src/commandline/uefi.rs | 24 ++-------- framework_lib/src/smbios.rs | 4 +- framework_lib/src/util.rs | 4 +- rgbkbd.py | 2 +- 11 files changed, 32 insertions(+), 91 deletions(-) diff --git a/README.md b/README.md index 0e89f34b..0784ba54 100644 --- a/README.md +++ b/README.md @@ -202,8 +202,6 @@ Options: Specify I2C addresses of the PD chips (Advanced) --pd-ports Specify I2C ports of the PD chips (Advanced) - --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 ``` diff --git a/completions/bash/framework_tool b/completions/bash/framework_tool index f96fcc23..7031b8d5 100755 --- a/completions/bash/framework_tool +++ b/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 diff --git a/completions/zsh/_framework_tool b/completions/zsh/_framework_tool index ad6aa668..2d473c8f 100644 --- a/completions/zsh/_framework_tool +++ b/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]' ) diff --git a/framework_lib/src/ccgx/device.rs b/framework_lib/src/ccgx/device.rs index aa1af5d0..b928d24d 100644 --- a/framework_lib/src/ccgx/device.rs +++ b/framework_lib/src/ccgx/device.rs @@ -41,8 +41,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 @@ -70,8 +70,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, diff --git a/framework_lib/src/chromium_ec/mod.rs b/framework_lib/src/chromium_ec/mod.rs index 30621225..82c1e0a8 100644 --- a/framework_lib/src/chromium_ec/mod.rs +++ b/framework_lib/src/chromium_ec/mod.rs @@ -201,24 +201,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>; fn send_command(&self, command: u16, command_version: u8, data: &[u8]) -> EcResult>; @@ -953,30 +935,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() diff --git a/framework_lib/src/commandline/clap_std.rs b/framework_lib/src/commandline/clap_std.rs index 3c81c755..b4b6e5b6 100644 --- a/framework_lib/src/commandline/clap_std.rs +++ b/framework_lib/src/commandline/clap_std.rs @@ -228,20 +228,15 @@ struct ClapCli { driver: Option, /// 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, /// 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, - /// Specify the type of EC chip (MEC/MCHP or other) - #[clap(requires("pd_addrs"), requires("pd_ports"))] - #[arg(long)] - has_mec: Option, - /// 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, diff --git a/framework_lib/src/commandline/mod.rs b/framework_lib/src/commandline/mod.rs index 87b08e3f..88fd1e61 100644 --- a/framework_lib/src/commandline/mod.rs +++ b/framework_lib/src/commandline/mod.rs @@ -191,7 +191,6 @@ pub struct Cli { pub hash: Option, pub pd_addrs: Option<(u16, u16)>, pub pd_ports: Option<(u8, u8)>, - pub has_mec: Option, 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; }; diff --git a/framework_lib/src/commandline/uefi.rs b/framework_lib/src/commandline/uefi.rs index 626f0bd0..1fd9e5d8 100644 --- a/framework_lib/src/commandline/uefi.rs +++ b/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::() { - 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 { diff --git a/framework_lib/src/smbios.rs b/framework_lib/src/smbios.rs index b25ed153..1345b396 100644 --- a/framework_lib/src/smbios.rs +++ b/framework_lib/src/smbios.rs @@ -46,7 +46,7 @@ pub enum ConfigDigit0 { pub fn is_framework() -> bool { if matches!( get_platform(), - Some(Platform::GenericFramework((_, _), (_, _), _)) + Some(Platform::GenericFramework((_, _), (_, _))) ) { return true; } @@ -252,7 +252,7 @@ pub fn get_platform() -> Option { // Except if it's a GenericFramework platform let config = Config::get(); let platform = &(*config).as_ref().unwrap().platform; - if matches!(platform, Platform::GenericFramework((_, _), (_, _), _)) { + if matches!(platform, Platform::GenericFramework((_, _), (_, _))) { return Some(*platform); } } diff --git a/framework_lib/src/util.rs b/framework_lib/src/util.rs index cfb97368..972d82a3 100644 --- a/framework_lib/src/util.rs +++ b/framework_lib/src/util.rs @@ -36,8 +36,8 @@ pub enum Platform { /// Framework Desktop - AMD Ryzen AI Max 300 FrameworkDesktopAmdAiMax300, /// Generic Framework device - /// pd_addrs, pd_ports, has_mec - GenericFramework((u16, u16), (u8, u8), bool), + /// pd_addrs, pd_ports + GenericFramework((u16, u16), (u8, u8)), } #[derive(Debug, PartialEq, Clone, Copy)] diff --git a/rgbkbd.py b/rgbkbd.py index 9088b85e..c997d897 100755 --- a/rgbkbd.py +++ b/rgbkbd.py @@ -1,7 +1,7 @@ #!/usr/bin/env python3 # Example invocation in fish shell # cargo build && sudo ./target/debug/framework_tool \ -# --driver portio --has-mec false --pd-ports 1 1 --pd-addrs 64 64 \ +# --driver portio --pd-ports 1 1 --pd-addrs 64 64 \ # (./rgbkbd.py | string split ' ') BRIGHTNESS = 1