Skip to content

Commit

Permalink
🐛 Fix hangs in DUE native USB (MarlinFirmware#26572)
Browse files Browse the repository at this point in the history
  • Loading branch information
sjasonsmith authored Jan 4, 2024
1 parent 54b7da1 commit 991f433
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 9 deletions.
29 changes: 29 additions & 0 deletions Marlin/src/HAL/DUE/usb/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# USB Files Source Documentation

## Source

We sourced the USB files in Marlin from the Atmel ASF (Advanced Software Framework). The framework provides a variety of examples which were utilized in this project.

Atmel doesn't provide these files in a source repository but they can be extracted from ASF, which can be downloaded from Atmel.

[Advanced Software Framework](https://www.microchip.com/en-us/tools-resources/develop/libraries/advanced-software-framework)

## Modifications

The files are mostly unmodified except for minor cosmetic changes but some more significant changes were needed.

The changes that prompted the addition of this README file are listed below. Other changes may have been made prior to this.

1. Modified `uotghs_device_due.c` to resolve race conditions that could leave interrupts asserted when freezing the peripheral clock, resulting in hangs and watchdog resets due to the ensuing interrupt storm.

## Version Information

We don't know the exact version of ASF used as the source. However, the copyright information in the files indicates they are from 2015.

## Upgrade Considerations

We looked at the ASF 3.52.0 files released in 2022 but saw no immediate benefits to justify an upgrade. It's important to note that the files in Marlin don't follow the same folder structure as the files in ASF, which complicates the process of comparing and applying updated files.

When these files are updated it's important to carefully compare them to Marlin's versions so any improvements in the Marlin sources are brought forward.

It would be best to make Marlin's directory structure align with ASF or at least document the source of each file to ease future updates.
79 changes: 70 additions & 9 deletions Marlin/src/HAL/DUE/usb/uotghs_device_due.c
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,23 @@
//#define dbg_print printf
#define dbg_print(...)

// Marlin modification: Redefine the otg_freeze_clock and otg_unfreeze_clock macros
// to add memory barriers to ensure that any accesses to USB registers aren't re-ordered
// to occur while the clock is frozen.
#undef otg_freeze_clock
#undef otg_unfreeze_clock

#define otg_freeze_clock() do { \
__DSB(); \
Set_bits(UOTGHS->UOTGHS_CTRL, UOTGHS_CTRL_FRZCLK); \
} while (0)

#define otg_unfreeze_clock() \
do { \
Clr_bits(UOTGHS->UOTGHS_CTRL, UOTGHS_CTRL_FRZCLK); \
__DSB(); \
} while (0)

/**
* \ingroup udd_group
* \defgroup udd_udphs_group USB On-The-Go High-Speed Port for device mode (UOTGHS)
Expand Down Expand Up @@ -611,6 +628,18 @@ ISR(UDD_USB_INT_FUN)
// The wakeup interrupt is automatic acked when a suspend occur
udd_disable_wake_up_interrupt();
udd_enable_suspend_interrupt();

// Marlin modification: The RESET, SOF, and MSOF interrupts were previously
// enabled in udd_attach, which caused a race condition where they could
// be raised and unclearable with the clock is frozen. They are now
// enabled here, after the clock has been unfrozen in response to the wake
// interrupt.
udd_enable_reset_interrupt();
udd_enable_sof_interrupt();
#ifdef USB_DEVICE_HS_SUPPORT
udd_enable_msof_interrupt();
#endif

udd_sleep_mode(true); // Enter in IDLE mode
#ifdef UDC_RESUME_EVENT
UDC_RESUME_EVENT();
Expand Down Expand Up @@ -776,6 +805,27 @@ void udd_disable(void)
cpu_irq_restore(flags);
}

// Marlin modification: The original implementation did not use a memory
// barrier between disabling and clearing interrupts. This sometimes
// allowed interrupts to remain raised and unclearable after the clock
// was frozen. This helper was added to ensure that memory barriers
// are used consistently from all places where interrupts are disabled.
static void disable_and_ack_sync_interrupts()
{
// Disable USB line events
udd_disable_reset_interrupt();
udd_disable_sof_interrupt();
#ifdef USB_DEVICE_HS_SUPPORT
udd_disable_msof_interrupt();
#endif
__DSB();
udd_ack_reset();
udd_ack_sof();
#ifdef USB_DEVICE_HS_SUPPORT
udd_ack_msof();
#endif
__DSB();
}

void udd_attach(void)
{
Expand All @@ -796,17 +846,16 @@ void udd_attach(void)
udd_attach_device();

// Enable USB line events
udd_enable_reset_interrupt();
udd_enable_suspend_interrupt();
udd_enable_wake_up_interrupt();
udd_enable_sof_interrupt();
#ifdef USB_DEVICE_HS_SUPPORT
udd_enable_msof_interrupt();
#endif
// Reset following interrupts flag
udd_ack_reset();
udd_ack_sof();
udd_ack_msof();

// Marlin modification: The RESET, SOF, and MSOF interrupts were previously
// enabled here, which caused a race condition where they could be raised
// and unclearable with the clock is frozen. They are now enabled in the
// wake interrupt handler, after the clock has been unfrozen. They are now
// explicitly disabled here to ensure that they cannot be raised before
// the clock is frozen.
disable_and_ack_sync_interrupts();

// The first suspend interrupt must be forced
// The first suspend interrupt is not detected else raise it
Expand All @@ -824,6 +873,12 @@ void udd_detach(void)

// Detach device from the bus
udd_detach_device();

// Marlin modification: Added the explicit disabling of the RESET, SOF, and
// MSOF interrupts here, to ensure that they cannot be raised after the
// clock is frozen.
disable_and_ack_sync_interrupts();

otg_freeze_clock();
udd_sleep_mode(false);
}
Expand Down Expand Up @@ -2043,6 +2098,12 @@ static bool udd_ep_interrupt(void)
dbg_print("I ");
udd_disable_in_send_interrupt(ep);
// One bank is free then send a ZLP

// Marlin modification: Add a barrier to ensure in_send is disabled
// before it is cleared. This was not an observed problem, but
// other interrupts were seen to misbehave without this barrier.
__DSB();

udd_ack_in_send(ep);
udd_ack_fifocon(ep);
udd_ep_finish_job(ptr_job, false, ep);
Expand Down
1 change: 1 addition & 0 deletions buildroot/tests/DUE_archim
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ exec_test $1 $2 "Archim 1 base configuration" "$3"
# Test Archim 2
#
use_example_configs UltiMachine/Archim2
opt_enable USB_FLASH_DRIVE_SUPPORT
exec_test $1 $2 "Archim 2 base configuration" "$3"

restore_configs

0 comments on commit 991f433

Please sign in to comment.