STM32 OTGv1 Suspend and Resume Fixes Topic is solved

[KarlK90]

Hi Giovanni,

I have another patch for the resume and suspend handling in the OTGv1 driver.

1. Problem:

After resuming from suspend, either by being woken by the host or by waking the host, the already setup'ed in and out endpoints remain disabled. The cause is that otg_disable_ep is called on the suspend condition - disabling the irqs for all endpoints except ep0 and they are never reactivated after coming out of suspend.

See: https://github.com/ChibiOS/ChibiOS/blob ... lld.c#L170

The only other place in the code where the endpoints are explicitly enabled is usb_lld_init_endpoint, but it doesn't make sense to call that in the wakeup callback.

See: https://github.com/ChibiOS/ChibiOS/blob ... ld.c#L1022

My solution so far is to enable the endpoints after coming out of suspend again:

Code: Select all

diff --git a/os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.c b/os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.c
index c52ba5c5dc..54f40ec302 100644
--- a/os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.c
+++ b/os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.c
@@ -170,6 +170,20 @@ static void otg_disable_ep(USBDriver *usbp) {
   otgp->DAINTMSK = DAINTMSK_OEPM(0) | DAINTMSK_IEPM(0);
 }
 
+static void otg_enable_ep(USBDriver *usbp) {
+  stm32_otg_t *otgp = usbp->otg;
+  unsigned i;
+
+  for (i = 0; i <= usbp->otgparams->num_endpoints; i++) {
+    if (usbp->epc[i]->out_state != NULL) {
+      otgp->DAINTMSK |= DAINTMSK_OEPM(i);
+    }
+    if (usbp->epc[i]->in_state != NULL) {
+      otgp->DAINTMSK |= DAINTMSK_IEPM(i);
+    }
+  }
+}
+
 static void otg_rxfifo_flush(USBDriver *usbp) {
   stm32_otg_t *otgp = usbp->otg;
 
@@ -553,6 +567,9 @@ static void usb_lld_serve_interrupt(USBDriver *usbp) {
       otgp->PCGCCTL &= ~(PCGCCTL_STPPCLK | PCGCCTL_GATEHCLK);
     }
 
+    /* Re-enable endpoint irqs if they have been disabled by suspend before. */
+    otg_enable_ep(usbp);
+
     /* Clear the Remote Wake-up Signaling.*/
     otgp->DCTL &= ~DCTL_RWUSIG;
 
@@ -595,6 +612,10 @@ static void usb_lld_serve_interrupt(USBDriver *usbp) {
         /* Set to zero to un-gate the USB core clocks.*/
         otgp->PCGCCTL &= ~(PCGCCTL_STPPCLK | PCGCCTL_GATEHCLK);
       }
+
+      /* Re-enable endpoint irqs if they have been disabled by suspend before. */
+      otg_enable_ep(usbp);
+
       _usb_wakeup(usbp);
     }


[KarlK90]

2. Problem

If the application disables the OTG clocks in low power mode in order to save some power, these clocks have to be enabled again if they want to wake the host - otherwise nothing happens:

Code: Select all

diff --git a/os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.h b/os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.h
index c9d2be1649..ddaa751ae0 100644
--- a/os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.h
+++ b/os/hal/ports/STM32/LLD/OTGv1/hal_usb_lld.h
@@ -557,6 +557,12 @@ struct USBDriver {
  */
 #define usb_lld_wakeup_host(usbp)                                           \
   do {                                                                      \
+    /* If clocks are gated off, turn them back on (may be the case if
+       coming out of suspend mode).*/                                       \
+    if ((usbp)->otg->PCGCCTL & (PCGCCTL_STPPCLK | PCGCCTL_GATEHCLK)) {      \
+      /* Set to zero to un-gate the USB core clocks.*/                      \
+      (usbp)->otg->PCGCCTL &= ~(PCGCCTL_STPPCLK | PCGCCTL_GATEHCLK);        \
+    }                                                                       \
     (usbp)->otg->DCTL |= DCTL_RWUSIG;                                       \
     /* remote wakeup doesn't trigger the wakeup interrupt, therefore
        we use the SOF interrupt to detect resume of the bus.*/              \


[Giovanni]

Hi,

Any suggestion about how to test this?

Giovanni

[KarlK90]

Hi Giovanni,

the first patch can be tested by simply suspending the device from the remote host. I did my tests with QMK Firmware but it should be sufficient to use a demo application that uses the USB peripheral e.g. RT tests over CDC and suspend the device by putting the host into sleep and waking it up again. Without the patches the application should be unresponsive on resume.

The 2nd patch would need some custom code to gate of the USB clocks in suspend after the USB device was configured and a way to trigger a remote wake-up - e.g. by reading a gpio pin.

Regards,
Stefan

[Giovanni]

Hi,

Fixed as bug 1293 with a minor change.

Please could you confirm that everything is OK now? there are also other changes to the driver fixing other problems.

Giovanni

[KarlK90]

Hi Giovanni,

thanks for applying the patch. It is almost correct - what is missing from the patch is the case when a bus resume is detected by looking at the SOF IRQs. This happens after issuing a host remote wakeup by the device itself. The peripheral won't generate a GINTSTS_WKUPINT in this case. My original patch contained the call to otg_enable_ep but it was not commited. This is the place where the call needs to be added as well: https://github.com/ChibiOS/ChibiOS/blob ... #L626-L634

[Giovanni]

Hi Karl,

Thanks, missed that... I also removed the check on the clocks, just clearing those bits without checking, it saves a little space.

Hope it is fine now.

Giovanni

[KarlK90]

Hi Giovanni,

looks good now and tested successfully. Thank you.


Stefan

[Giovanni]

Great, thanks for all support.

Giovanni