diff --git a/framework_lib/src/ccgx/device.rs b/framework_lib/src/ccgx/device.rs index 91376c6..5765501 100644 --- a/framework_lib/src/ccgx/device.rs +++ b/framework_lib/src/ccgx/device.rs @@ -10,7 +10,7 @@ use core::prelude::rust_2021::derive; use crate::ccgx::{AppVersion, BaseVersion, ControllerVersion}; use crate::chromium_ec::i2c_passthrough::*; use crate::chromium_ec::{CrosEc, EcError, EcResult}; -use crate::util::{assert_win_len, Config, Platform}; +use crate::util::{Config, Platform}; use super::*; @@ -244,6 +244,12 @@ impl PdController { err ))); } + // Don't loop forever if the EC doesn't return any data + if i2c_response.data.is_empty() { + return Err(EcError::DeviceError( + "I2C read returned no data".to_string(), + )); + } data.extend(i2c_response.data); } @@ -281,7 +287,7 @@ impl PdController { pub fn get_silicon_id(&self) -> EcResult { let data = self.ccgx_read(ControlRegisters::SiliconId, 2)?; - assert_win_len(data.len(), 2); + assert_eq!(data.len(), 2); Ok(u16::from_le_bytes([data[0], data[1]])) } @@ -342,7 +348,7 @@ impl PdController { } }; - assert_win_len(data.len(), 8); + assert_eq!(data.len(), 8); let base_ver = BaseVersion::from(&data[..4]); let app_ver = AppVersion::from(&data[4..]); println!( @@ -352,7 +358,7 @@ impl PdController { let data = self.ccgx_read(ControlRegisters::Firmware1Version, 8); let data = data.unwrap(); - assert_win_len(data.len(), 8); + assert_eq!(data.len(), 8); let base_ver = BaseVersion::from(&data[..4]); let app_ver = AppVersion::from(&data[4..]); println!( @@ -362,7 +368,7 @@ impl PdController { let data = self.ccgx_read(ControlRegisters::Firmware2Version, 8); let data = data.unwrap(); - assert_win_len(data.len(), 8); + assert_eq!(data.len(), 8); let base_ver = BaseVersion::from(&data[..4]); let app_ver = AppVersion::from(&data[4..]); println!( @@ -387,7 +393,7 @@ impl PdController { pub fn get_port_status(&self) -> EcResult { let data = self.ccgx_read(ControlRegisters::PdPortsEnable, 1)?; - assert_win_len(data.len(), 1); + assert_eq!(data.len(), 1); Ok(data[0]) } } diff --git a/framework_lib/src/chromium_ec/command.rs b/framework_lib/src/chromium_ec/command.rs index ea608b7..1d2688f 100644 --- a/framework_lib/src/chromium_ec/command.rs +++ b/framework_lib/src/chromium_ec/command.rs @@ -201,14 +201,9 @@ pub trait EcRequestRaw { Self: Sized, { let response = self.send_command_vec_extra(ec, extra_data)?; - // TODO: The Windows driver seems to return 20 more bytes than expected - #[cfg(windows)] - let expected = response.len() != std::mem::size_of::() + 20; - #[cfg(not(windows))] - let expected = response.len() != std::mem::size_of::(); - if expected { + if response.len() != std::mem::size_of::() { return Err(EcError::DeviceError(format!( - "Returned data size ({}) is not the expted size: {}", + "Returned data size ({}) is not the expected size: {}", response.len(), std::mem::size_of::() ))); diff --git a/framework_lib/src/chromium_ec/i2c_passthrough.rs b/framework_lib/src/chromium_ec/i2c_passthrough.rs index 3649ac2..405627f 100644 --- a/framework_lib/src/chromium_ec/i2c_passthrough.rs +++ b/framework_lib/src/chromium_ec/i2c_passthrough.rs @@ -63,6 +63,7 @@ impl EcI2cPassthruResponse { /// Indicate that it's a read, not a write const I2C_READ_FLAG: u16 = 1 << 15; +/// Read from a device with a one byte register address (two bytes if addr > 0xFF) pub fn i2c_read( ec: &CrosEc, i2c_port: u8, @@ -70,85 +71,45 @@ pub fn i2c_read( addr: u16, len: u16, ) -> EcResult { - trace!( - "i2c_read(i2c_port: 0x{:X}, i2c_addr: 0x{:X}, addr: 0x{:X}, len: 0x{:X})", - i2c_port, - i2c_addr, - addr, - len - ); - if usize::from(len) > MAX_I2C_CHUNK { - return EcResult::Err(EcError::DeviceError(format!( - "i2c_read too long. Must be <128, is: {}", - len - ))); - } - let addr_bytes = if addr < 0xFF { + let addr_bytes = if addr <= 0xFF { vec![addr as u8] } else { u16::to_le_bytes(addr).to_vec() }; - let messages = vec![ - EcParamsI2cPassthruMsg { - addr_and_flags: i2c_addr, - transfer_len: addr_bytes.len() as u16, - }, - EcParamsI2cPassthruMsg { - addr_and_flags: i2c_addr + I2C_READ_FLAG, - transfer_len: len, // How much to read - }, - ]; - let msgs_len = size_of::() * messages.len(); - let msgs_buffer: &[u8] = unsafe { util::any_vec_as_u8_slice(&messages) }; - - let params = EcParamsI2cPassthru { - port: i2c_port, - messages: messages.len() as u8, - msg: [], // Messages are copied right after this struct - }; - let params_len = size_of::(); - let params_buffer: &[u8] = unsafe { util::any_as_u8_slice(¶ms) }; - - let mut buffer: Vec = vec![0; params_len + msgs_len + addr_bytes.len()]; - buffer[0..params_len].copy_from_slice(params_buffer); - buffer[params_len..params_len + msgs_len].copy_from_slice(msgs_buffer); - buffer[params_len + msgs_len..].copy_from_slice(&addr_bytes); - - let data = ec.send_command(EcCommands::I2cPassthrough as u16, 0, &buffer)?; - let res: _EcI2cPassthruResponse = unsafe { std::ptr::read(data.as_ptr() as *const _) }; - let res_data = &data[size_of::<_EcI2cPassthruResponse>()..]; - // TODO: Seems to be either one, non-deterministically - debug_assert!(res.messages as usize == messages.len() || res.messages == 0); - Ok(EcI2cPassthruResponse { - i2c_status: res.i2c_status, - data: res_data.to_vec(), - }) + i2c_read_impl(ec, i2c_port, i2c_addr, &addr_bytes, len) } -/// I2C read with 16-bit addressing (for larger EEPROMs like 24C32+) -/// Always sends a 2-byte address, even for addr=0 -pub fn i2c_read_16bit_addr( +/// Read from a device that always uses two byte register addresses (LSB first) +pub fn i2c_read_addr16( ec: &CrosEc, i2c_port: u8, i2c_addr: u16, addr: u16, len: u16, +) -> EcResult { + i2c_read_impl(ec, i2c_port, i2c_addr, &u16::to_le_bytes(addr), len) +} + +fn i2c_read_impl( + ec: &CrosEc, + i2c_port: u8, + i2c_addr: u16, + addr_bytes: &[u8], + len: u16, ) -> EcResult { trace!( - "i2c_read_16bit_addr(i2c_port: 0x{:X}, i2c_addr: 0x{:X}, addr: 0x{:X}, len: 0x{:X})", + "i2c_read(i2c_port: 0x{:X}, i2c_addr: 0x{:X}, addr: {:X?}, len: 0x{:X})", i2c_port, i2c_addr, - addr, + addr_bytes, len ); if usize::from(len) > MAX_I2C_CHUNK { return EcResult::Err(EcError::DeviceError(format!( - "i2c_read too long. Must be <128, is: {}", - len + "i2c_read too long. Must be <={}, is: {}", + MAX_I2C_CHUNK, len ))); } - // Always use 16-bit addressing (big-endian for EEPROM) - let addr_bytes = u16::to_be_bytes(addr).to_vec(); let messages = vec![ EcParamsI2cPassthruMsg { addr_and_flags: i2c_addr, @@ -173,25 +134,41 @@ pub fn i2c_read_16bit_addr( let mut buffer: Vec = vec![0; params_len + msgs_len + addr_bytes.len()]; buffer[0..params_len].copy_from_slice(params_buffer); buffer[params_len..params_len + msgs_len].copy_from_slice(msgs_buffer); - buffer[params_len + msgs_len..].copy_from_slice(&addr_bytes); + buffer[params_len + msgs_len..].copy_from_slice(addr_bytes); let data = ec.send_command(EcCommands::I2cPassthrough as u16, 0, &buffer)?; let res: _EcI2cPassthruResponse = unsafe { std::ptr::read(data.as_ptr() as *const _) }; - let header_len: usize = size_of::<_EcI2cPassthruResponse>(); - /* Note on windows, you can get extra bytes back, so truncate this to the requested size */ - let res_data = &data[header_len..(len as usize + header_len)]; - debug_assert!(res.messages as usize == messages.len() || res.messages == 0); + // The EC only returns as many bytes as it managed to read. + // On NAK or timeout that's less than requested, possibly nothing at all. + let res_data = &data[size_of::<_EcI2cPassthruResponse>()..]; + // The EC stops at the first message that fails, so it can have processed + // any number of messages up to the requested count + debug_assert!(res.messages as usize <= messages.len()); + let i2c_status = res.i2c_status; trace!( - " i2c_read_16bit_addr response (len: {}, data: {:#04X?})", + " i2c_read response (status: 0x{:02X}, len: {}, data: {:#04X?})", + i2c_status, res_data.len(), - res_data.to_vec() + res_data ); Ok(EcI2cPassthruResponse { - i2c_status: res.i2c_status, + i2c_status, data: res_data.to_vec(), }) } +/// I2C read with 16-bit addressing (for larger EEPROMs like 24C32+) +/// Always sends a 2-byte address (MSB first), even for addr=0 +pub fn i2c_read_16bit_addr( + ec: &CrosEc, + i2c_port: u8, + i2c_addr: u16, + addr: u16, + len: u16, +) -> EcResult { + i2c_read_impl(ec, i2c_port, i2c_addr, &u16::to_be_bytes(addr), len) +} + /* Write address and bytes in a single I2C transfer */ pub fn i2c_write_block( ec: &CrosEc, @@ -231,7 +208,7 @@ pub fn i2c_write_block( let data = ec.send_command(EcCommands::I2cPassthrough as u16, 0, &buffer)?; let res: _EcI2cPassthruResponse = unsafe { std::ptr::read(data.as_ptr() as *const _) }; - util::assert_win_len(data.len(), size_of::<_EcI2cPassthruResponse>()); // No extra data other than the header + assert_eq!(data.len(), size_of::<_EcI2cPassthruResponse>()); // No extra data other than the header // Only assert message count if the transfer was successful // On error (NAK/timeout), the EC may return 0 messages processed @@ -283,7 +260,7 @@ pub fn i2c_write( let data = ec.send_command(EcCommands::I2cPassthrough as u16, 0, &buffer)?; let res: _EcI2cPassthruResponse = unsafe { std::ptr::read(data.as_ptr() as *const _) }; - util::assert_win_len(data.len(), size_of::<_EcI2cPassthruResponse>()); // No extra data other than the header + assert_eq!(data.len(), size_of::<_EcI2cPassthruResponse>()); // No extra data other than the header // Only assert message count if the transfer was successful // On error (NAK/timeout), the EC may return 0 messages processed diff --git a/framework_lib/src/chromium_ec/mod.rs b/framework_lib/src/chromium_ec/mod.rs index b90ad58..a4e4cd8 100644 --- a/framework_lib/src/chromium_ec/mod.rs +++ b/framework_lib/src/chromium_ec/mod.rs @@ -451,7 +451,7 @@ impl CrosEc { let limits = &[ChargeLimitControlModes::Set as u8, max, min]; let data = self.send_command(EcCommands::ChargeLimitControl as u16, 0, limits)?; - util::assert_win_len(data.len(), 0); + assert_eq!(data.len(), 0); Ok(()) } @@ -515,7 +515,7 @@ impl CrosEc { let limits = &[percentage, 0x00]; let data = self.send_command(EcCommands::FpLedLevelControl as u16, 1, limits)?; - util::assert_win_len(data.len(), 0); + assert_eq!(data.len(), 0); Ok(()) } @@ -526,7 +526,7 @@ impl CrosEc { let limits = &[level as u8, 0x00]; let data = self.send_command(EcCommands::FpLedLevelControl as u16, 0, limits)?; - util::assert_win_len(data.len(), 0); + assert_eq!(data.len(), 0); Ok(()) } @@ -1460,6 +1460,12 @@ impl CrosEc { err ))); } + // Don't loop forever if the EC doesn't return any data + if i2c_response.data.is_empty() { + return Err(EcError::DeviceError( + "I2C read returned no data".to_string(), + )); + } data.extend(i2c_response.data); } diff --git a/framework_lib/src/chromium_ec/windows.rs b/framework_lib/src/chromium_ec/windows.rs index 936243c..050d734 100644 --- a/framework_lib/src/chromium_ec/windows.rs +++ b/framework_lib/src/chromium_ec/windows.rs @@ -169,10 +169,15 @@ pub fn send_command(command: u16, command_version: u8, data: &[u8]) -> EcResult< Some(status) => return Err(EcError::Response(status)), } - // TODO: Figure out why that's sometimes bigger - let end = std::cmp::min(returned, CROSEC_CMD_MAX_REQUEST as u32); - - let out_buffer = &cmd.buffer[..(end as usize)]; + // The driver reports how many bytes of CROSEC_COMMAND it filled in, which includes its + // 20 byte header. But cmd.buffer already starts after that header, so subtract it to get + // the length of the EC response itself. + // Otherwise we'd return 20 bytes of leftover request data after the response. + let crosec_header_len = std::mem::offset_of!(CrosEcCommand, buffer); + let data_len = (returned as usize).saturating_sub(crosec_header_len); + let end = std::cmp::min(data_len, CROSEC_CMD_MAX_REQUEST); + + let out_buffer = &cmd.buffer[..end]; Ok(out_buffer.to_vec()) } diff --git a/framework_lib/src/parade_retimer.rs b/framework_lib/src/parade_retimer.rs index 6f0cf8c..7af14c7 100644 --- a/framework_lib/src/parade_retimer.rs +++ b/framework_lib/src/parade_retimer.rs @@ -1,10 +1,11 @@ +use alloc::string::ToString; use alloc::vec::Vec; use num_traits::FromPrimitive; use crate::chromium_ec::command::EcRequestRaw; use crate::chromium_ec::commands::{EcRequestGetGpuPcie, GpuVendor}; use crate::chromium_ec::i2c_passthrough::*; -use crate::chromium_ec::{CrosEc, EcResult}; +use crate::chromium_ec::{CrosEc, EcError, EcResult}; use crate::os_specific; pub fn get_version(ec: &CrosEc) -> EcResult>> { @@ -35,13 +36,16 @@ pub fn get_version(ec: &CrosEc) -> EcResult>> { i2c_response.i2c_status, i2c_response.data.len() ); - if i2c_response.i2c_status == 0x01 { - // NAK + // We know the retimer is there, because the EC reported a compatible dGPU. + // So if it doesn't answer, report that instead of pretending it doesn't exist. + if let Err(err) = i2c_response.is_successful() { warn!("Unable to communicate with dGPU Retimer. Try to force it on by plugging a cable into the dGPU"); + return Err(err); } let Some(&safe_mode) = i2c_response.data.first() else { - info!("Failed to read retimer safe mode status (empty response)"); - return Ok(None); + return Err(EcError::DeviceError( + "Retimer did not return the safe mode status".to_string(), + )); }; info!("Retimer safe mode status: {}", safe_mode); if safe_mode == 0 { @@ -61,6 +65,7 @@ pub fn get_version(ec: &CrosEc) -> EcResult>> { // Read version let i2c_response = i2c_read(ec, i2c_port, (i2c_addr + 18) >> 1, 0x01, 0x04)?; + i2c_response.is_successful()?; Ok(Some(i2c_response.data)) } diff --git a/framework_lib/src/util.rs b/framework_lib/src/util.rs index 03cc7cc..307ab31 100644 --- a/framework_lib/src/util.rs +++ b/framework_lib/src/util.rs @@ -1,6 +1,5 @@ //! Miscellaneous utility functions to use across modules -use num::{Num, NumCast}; use std::prelude::v1::*; #[cfg(feature = "uefi")] @@ -267,15 +266,6 @@ pub fn find_sequence(haystack: &[u8], needle: &[u8]) -> Option { .position(|window| window == needle) } -/// Assert length of an EC response from the windows driver -/// It's always 20 more than expected. TODO: Figure out why -pub fn assert_win_len(left: N, right: N) { - #[cfg(windows)] - assert_eq!(left, right + NumCast::from(20).unwrap()); - #[cfg(not(windows))] - assert_eq!(left, right); -} - pub fn format_duration(duration: &Duration) -> String { let hours = duration.as_secs() / 3600; let minutes = (duration.as_secs() % 3600) / 60;