Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some stm32_fsdev optimizations #2178

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 50 additions & 18 deletions src/portable/st/stm32_fsdev/dcd_stm32_fsdev.c
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,8 @@ static TU_ATTR_ALIGNED(4) uint32_t _setup_packet[6];

static uint8_t remoteWakeCountdown; // When wake is requested

static bool previousSofState = false;

//--------------------------------------------------------------------+
// Prototypes
//--------------------------------------------------------------------+
Expand Down Expand Up @@ -297,7 +299,6 @@ void dcd_connect(uint8_t rhport)
void dcd_sof_enable(uint8_t rhport, bool en)
{
(void) rhport;
(void) en;

if (en)
{
Expand All @@ -307,6 +308,8 @@ void dcd_sof_enable(uint8_t rhport, bool en)
{
USB->CNTR &= ~USB_CNTR_SOFM;
}

previousSofState = en;
}

// Enable device interrupt
Expand Down Expand Up @@ -652,11 +655,18 @@ static void dcd_ep_ctr_handler(void)
}
}

void dcd_int_handler(uint8_t rhport) {
void dcd_int_handler(uint8_t rhport)
{
// Only handle interrupts which are triggered and currently active
uint32_t int_status = USB->ISTR;
int_status &= USB->CNTR;

(void) rhport;
// Only continue if there is something to do
if (!int_status)
{
return;
}

uint32_t int_status = USB->ISTR;
//const uint32_t handled_ints = USB_ISTR_CTR | USB_ISTR_RESET | USB_ISTR_WKUP
// | USB_ISTR_SUSP | USB_ISTR_SOF | USB_ISTR_ESOF;
// unused IRQs: (USB_ISTR_PMAOVR | USB_ISTR_ERR | USB_ISTR_L1REQ )
Expand All @@ -666,16 +676,29 @@ void dcd_int_handler(uint8_t rhport) {
// be triggered repeatedly.

/* Put SOF flag at the beginning of ISR in case to get least amount of jitter if it is used for timing purposes */
if(int_status & USB_ISTR_SOF) {
if (int_status & USB_ISTR_SOF)
{
USB->ISTR &=~USB_ISTR_SOF;
dcd_event_sof(0, USB->FNR & USB_FNR_FN, true);
dcd_event_sof(rhport, USB->FNR & USB_FNR_FN, true);

/* Ensure normal USB operation as a failsafe if a remote wakeup from the host was omitted */
USB->CNTR &= ~USB_CNTR_LPMODE;
USB->CNTR &= ~USB_CNTR_FSUSP;

/* Disable SOF if it wasn't enabled to begin with */
dcd_sof_enable(rhport, previousSofState);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an issue here, I think.

The call dcd_sof_enable(rhport, true); on line 736 sets previousSofState = true. But we expect to call dcd_sof_enable(rhport, false) here if SOF is used as a failsafe to wake the device?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. At 736 either the value should not overwritten or it should be restored.

}

if(int_status & USB_ISTR_RESET) {
if (int_status & USB_ISTR_RESET)
{
// USBRST is start of reset.
USB->ISTR &=~USB_ISTR_RESET;
dcd_handle_bus_reset();
dcd_event_bus_reset(0, TUSB_SPEED_FULL, true);
dcd_event_bus_reset(rhport, TUSB_SPEED_FULL, true);

/* Drop all potential pending USB interrupts because of the upcoming reset */
USB->ISTR = 0;

return; // Don't do the rest of the things here; perhaps they've been cleared?
}

Expand All @@ -692,29 +715,38 @@ void dcd_int_handler(uint8_t rhport) {
USB->CNTR &= ~USB_CNTR_FSUSP;

USB->ISTR &=~USB_ISTR_WKUP;
dcd_event_bus_signal(0, DCD_EVENT_RESUME, true);
dcd_event_bus_signal(rhport, DCD_EVENT_RESUME, true);
}

if (int_status & USB_ISTR_SUSP)
{
/* Suspend is asserted for both suspend and unplug events. without Vbus monitoring,
* these events cannot be differentiated, so we only trigger suspend. */
/* Don't suspend as long there is a pending remote wakeup */
if (!(USB->CNTR & USB_CNTR_RESUME))
{
/* Suspend is asserted for both suspend and unplug events. without Vbus monitoring,
* these events cannot be differentiated, so we only trigger suspend. */

/* Force low-power mode in the macrocell */
USB->CNTR |= USB_CNTR_FSUSP;
USB->CNTR |= USB_CNTR_LPMODE;

/* Force low-power mode in the macrocell */
USB->CNTR |= USB_CNTR_FSUSP;
USB->CNTR |= USB_CNTR_LPMODE;
dcd_event_bus_signal(rhport, DCD_EVENT_SUSPEND, true);

/* Enable SOF as a failsafe for the case a remote wakeup from the host is omitted */
dcd_sof_enable(rhport, true);
}

/* clear of the ISTR bit must be done after setting of CNTR_FSUSP */
USB->ISTR &=~USB_ISTR_SUSP;
dcd_event_bus_signal(0, DCD_EVENT_SUSPEND, true);
}

if(int_status & USB_ISTR_ESOF) {
if(remoteWakeCountdown == 1u)
if (int_status & USB_ISTR_ESOF)
{
if (remoteWakeCountdown == 1u)
{
USB->CNTR &= ~USB_CNTR_RESUME;
}
if(remoteWakeCountdown > 0u)
if (remoteWakeCountdown > 0u)
{
remoteWakeCountdown--;
}
Expand Down