Skip to content

Commit

Permalink
[opentitantool] Improvement to HyperDebug firmware upgrade
Browse files Browse the repository at this point in the history 
Firmware upgrading of HyperDebug requires entering and leaving "DFU
mode", each of which involves resetting connection with the USB device.

Previous code waited one second for the device to reset after upgrading
concluded, before attempting to restablish USB connection with the newly
flashed firmware.  This turns out to be insufficient on some machines.

This patch will wait up to five seconds, checking twice a second if the
device is available, and terminating early if it is.

Change-Id: Ib59d7f5ebaf234a5c2508253236f6bc3ee96c83f
Signed-off-by: Jes B. Klinke <jbk@chromium.org>
  • Loading branch information
@jesultra
jesultra committed Sep 26, 2024
1 parent 8f0f500 commit f1a2f34
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 13 deletions.
52 changes: 39 additions & 13 deletions sw/host/opentitanlib/src/transport/hyperdebug/dfu.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

// Firmware update protocol for HyperDebug

use anyhow::{anyhow, bail, Context, Result};
use anyhow::{anyhow, bail, Result};
use once_cell::sync::Lazy;
use regex::Regex;
use serde_annotate::Annotate;
Expand All @@ -23,8 +23,12 @@ const PID_DFU_BOOTLOADER: u16 = 0xdf11;
/// of the `opentitantool` invocation (and presenting itself with STMs VID:DID, rather than
/// Google's).
pub struct HyperdebugDfu {
// Handle to USB device which may or may not already be in DFU mode
usb_backend: RefCell<UsbBackend>,
current_firmware_version: Option<String>,
// expected USB VID:PID of the HyperDebug device when not in DFU mode
usb_vid: u16,
usb_pid: u16,
}

impl HyperdebugDfu {
Expand All @@ -42,8 +46,8 @@ impl HyperdebugDfu {
// interrupted update, as well as the ordinary case of outdated or current HyperDebug
// firmware already running.
if let Ok(usb_backend) = UsbBackend::new(
usb_vid.unwrap_or(VID_ST_MICROELECTRONICS),
usb_pid.unwrap_or(PID_DFU_BOOTLOADER),
VID_ST_MICROELECTRONICS,
PID_DFU_BOOTLOADER,
usb_serial,
) {
// HyperDebug device is already in DFU mode, we cannot query firmware version through
Expand All @@ -53,6 +57,8 @@ impl HyperdebugDfu {
return Ok(Self {
usb_backend: RefCell::new(usb_backend),
current_firmware_version: None,
usb_vid: usb_vid.unwrap_or(super::VID_GOOGLE),
usb_pid: usb_pid.unwrap_or(super::PID_HYPERDEBUG),
});
}

Expand All @@ -76,6 +82,8 @@ impl HyperdebugDfu {
Ok(Self {
usb_backend: RefCell::new(usb_backend),
current_firmware_version,
usb_vid: usb_vid.unwrap_or(super::VID_GOOGLE),
usb_pid: usb_pid.unwrap_or(super::PID_HYPERDEBUG),
})
}
}
Expand All @@ -94,6 +102,8 @@ impl Transport for HyperdebugDfu {
&update_firmware_action.firmware,
update_firmware_action.progress.as_ref(),
update_firmware_action.force,
self.usb_vid,
self.usb_pid,
)
} else {
bail!(TransportError::UnsupportedOperation)
Expand Down Expand Up @@ -167,6 +177,8 @@ pub fn update_firmware(
firmware: &Option<Vec<u8>>,
progress: &dyn ProgressIndicator,
force: bool,
usb_vid: u16,
usb_pid: u16,
) -> Result<Option<Box<dyn Annotate>>> {
let firmware: &[u8] = if let Some(vec) = firmware.as_ref() {
validate_firmware_image(vec)?;
Expand Down Expand Up @@ -196,6 +208,7 @@ pub fn update_firmware(
if wait_for_idle(usb_device, dfu_desc.dfu_interface)? != DFU_STATE_APP_IDLE {
// Device is already running DFU bootloader, proceed to firmware transfer.
do_update_firmware(usb_device, dfu_desc, firmware, progress)?;
restablish_connection(usb_device, usb_vid, usb_pid)?;
return Ok(None);
}

Expand Down Expand Up @@ -233,19 +246,32 @@ pub fn update_firmware(
let dfu_desc = scan_usb_descriptor(&dfu_device)?;
dfu_device.claim_interface(dfu_desc.dfu_interface)?;
do_update_firmware(&dfu_device, dfu_desc, firmware, progress)?;
restablish_connection(usb_device, usb_vid, usb_pid)?;
Ok(None)
}

fn restablish_connection(
usb_device: &UsbBackend,
usb_vid: u16,
usb_pid: u16,
) -> Result<()> {
// At this point, the new firmware has been completely transferred, and the USB device is
// resetting and booting the new firmware. Wait a second, then verify that device can now be
// found on the USB bus with the original DID:VID.
std::thread::sleep(std::time::Duration::from_millis(1000));
// resetting and booting the new firmware. Wait up to five seconds, repeatedly testing if the
// device can be found on the USB bus with the original DID:VID.
log::info!("Connecting to newly flashed firmware...");
let _new_device = UsbBackend::new(
usb_device.get_vendor_id(),
usb_device.get_product_id(),
Some(usb_device.get_serial_number()),
)
.context("Unable to establish connection after flashing. Possibly bad image.")?;
Ok(None)
for _ in 0..10 {
std::thread::sleep(std::time::Duration::from_millis(500));
if let Ok(_) = UsbBackend::new(
usb_vid,
usb_pid,
Some(usb_device.get_serial_number()),
) {
return Ok(());
}
}
bail!(TransportError::FirmwareProgramFailed(
"Unable to establish connection after flashing. Possibly bad image.".to_string()
));
}

fn do_update_firmware(
Expand Down
4 changes: 4 additions & 0 deletions sw/host/opentitanlib/src/transport/hyperdebug/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -808,12 +808,16 @@ impl<T: Flavor> Transport for Hyperdebug<T> {

fn dispatch(&self, action: &dyn Any) -> Result<Option<Box<dyn Annotate>>> {
if let Some(update_firmware_action) = action.downcast_ref::<UpdateFirmware>() {
let usb_vid = self.inner.usb_device.borrow().get_vendor_id();
let usb_pid = self.inner.usb_device.borrow().get_product_id();
dfu::update_firmware(
&mut self.inner.usb_device.borrow_mut(),
self.current_firmware_version.as_deref(),
&update_firmware_action.firmware,
update_firmware_action.progress.as_ref(),
update_firmware_action.force,
usb_vid,
usb_pid,
)
} else if let Some(jtag_set_pins) = action.downcast_ref::<SetJtagPins>() {
match (
Expand Down

0 comments on commit f1a2f34

Please sign in to comment.