Refactor USB pullup handling

matthijskooijman · matthijskooijman · commit 4ad48fab1946 · 2020-03-23T18:33:57.000+01:00
Previously, variants could define USB_DISC_PIN when they had an USB attach pullup that was disabled by default and needed a pin to be written LOW to enable it. Other hardware configurations could only overwrite the USBD_reenumerate function, like the M200 board did. This commit makes the pullup configuration more flexible. By defining the appropriate macros, enabled-by-default and disabled-by-default pullups are both supported. The output level of the pin can also be configured. The code that controls an external pullup is now merged with the code that applies the D+ output trick for fixed pullups, since the behaviour is almost identical (except for reverting to INPUT mode for the D+ pin instead of inverting the output value). Finally, for CPUs that have internal USB pullup (as indicated by the presence of a SDIS or DPPU config bit), the write-to-DP-trick is now not performed, but the internal pullup is toggled instead. This fixes stm32duino#885, also see that issue for discussion leading up to this change.
diff --git a/cores/arduino/stm32/usb/usbd_if.c b/cores/arduino/stm32/usb/usbd_if.c
@@ -10,36 +10,148 @@
 #include "usbd_if.h"
 #include "usbd_cdc_if.h"
 
+#if !defined(USBD_REENUM_DISABLED)
+
+/*
+ * Below, support for re-attaching to USB is handled. USB-attachment is
+ * detected by the presence of a pullup on one of the USB datalines.
+ *
+ * To force the USB host to re-enumerate, we must detach (disable
+ * pullup), wait a bit, and re-attach (enable pullup). This can be done
+ * while running, but must also happen at startup (including the detach
+ * and delay, since USB might have been active just before reset, so we
+ * must force a re-enumeration). On a cold boot (e.g. USB just plugged
+ * in), the forced detach might not be needed, but will not be harmful
+ * either.
+ *
+ * For pullup control, a number of cases are supported:
+ *  1. An external pullup controllable through a GPIO (e.g. using an
+ *     external transistor). The default output state (e.g. when the
+ *     GPIO is floating) can be enabled or disabled, and the output
+ *     polarity can be configured.
+ *  2. A pullup built into the chip's USB core. This is automatically
+ *     enabled by hardware or USB HAL code, and can be manually
+ *     controlled using HAL functions.
+ *  3. A fixed (always enabled) external pullup. To still allow detaching,
+ *     a bit of a hack is used: By setting the D+ to OUTPUT LOW, it
+ *     *looks* like the pullup is removed (the host has own, bigger,
+ *     pulldown, to detact detaching, so writing LOW looks the same).
+ *     This probably violates USB specifications, but does seem to work
+ *     well in practice. It probably also should only be used briefly,
+ *     keeping the pin OUTPUT LOW for longer might be problematic.
+ *
+ * These scenarios are considered in order: If the variant defines an
+ * external pullup control pin, it is used, else if an internal pullup
+ * is present, that is used, else the D+ trick is used.
+ *
+ * To configure #1, the variant should either define USBD_ATTACH_PIN and
+ * USBD_ATTACH_LEVEL when the pullup is disabled by default and the pin
+ * can be used to *attach*, or USBD_DETACH_PIN and USBD_DETACH_LEVEL
+ * when the pullup is enabled by default and the pin can be used to
+ * *detach*.
+ */
+
+/* Compatibility with the old way to specify this */
+#if defined(USB_DISC_PIN) && !defined(USBD_ATTACH_PIN)
+#define USBD_ATTACH_PIN USB_DISC_PIN
+#define USBD_ATTACH_LEVEL LOW
+#warning "USB_DISC_PIN is deprecated, use USBD_ATTACH_PIN instead"
+#endif /* defined(USB_DISC_PIN) && !defined(USBD_ATTACH_PIN) */
+
+/* Some sanity checks */
+#if defined(USBD_ATTACH_PIN) && defined(USBD_DETACH_PIN)
+#error "Cannot define both USBD_ATTACH_PIN and USBD_DETACH_PIN"
+#endif /* defined(USBD_ATTACH_PIN) && defined(USBD_DETACH_PIN) */
+#if defined(USBD_ATTACH_PIN) && !defined(USBD_ATTACH_LEVEL)
+#error "USBD_ATTACH_PIN also needs USBD_ATTACH_LEVEL defined"
+#endif /* defined(USBD_ATTACH_PIN) && !defined(USBD_ATTACH_LEVEL) */
+#if defined(USBD_DETACH_PIN) && !defined(USBD_DETACH_LEVEL)
+#error "USBD_DETACH_PIN also needs USBD_DETACH_LEVEL defined"
+#endif /* defined(USBD_DETACH_PIN) && !defined(USBD_DETACH_LEVEL) */
+
+/* Either of these bits indicate that there are internal pullups */
+#if defined(USB_BCDR_DPPU) || defined(USB_OTG_DCTL_SDIS)
+#define USBD_HAVE_INTERNAL_PULLUPS
+#endif /* defined(USB_BCDR_DPPU) || defined(USB_OTG_DCTL_SDIS) */
+
+/* Figure out which USB instance is used. This mirrors the decision made
+ * in USBD_LL_Init in usbd_conf.c. */
+#if defined(USE_USB_HS)
+#define USBD_USB_INSTANCE USB_OTG_HS
+#elif defined(USB_OTG_FS)
+#define USBD_USB_INSTANCE USB_OTG_FS
+#elif defined(USB)
+#define USBD_USB_INSTANCE USB
+#endif
+
+/*
+ * Translate pin number to a pin name (using the same define for both
+ * attach and detach pins) and also define the complementary level. This
+ * allows using the same code for the default-enabled and
+ * default-disabled case as well as for the D+ trick (which does not
+ * know the pin number, only the pin name).
+ */
+#if defined(USBD_ATTACH_PIN)
+#define USBD_PULLUP_CONTROL_PINNAME digitalPinToPinName(USBD_ATTACH_PIN)
+#define USBD_DETACH_LEVEL !(USBD_ATTACH_LEVEL)
+#elif defined(USBD_DETACH_PIN)
+#define USBD_PULLUP_CONTROL_PINNAME digitalPinToPinName(USBD_DETACH_PIN)
+#define USBD_ATTACH_LEVEL !(USBD_DETACH_LEVEL)
+#elif !defined(USBD_HAVE_INTERNAL_PULLUPS)
+/* When no USB attach and detach pins were defined, and there are also
+ * no internal pullups, assume there is a fixed external pullup and apply
+ * the D+ trick. This should happen only for the USB peripheral, since
+ * USB_OTG_HS and USB_OTG_FS always have internal pullups. */
+#if !defined(USB)
+#error "Unexpected USB configuration"
+#endif
+#define USBD_PULLUP_CONTROL_PINNAME USB_DP
+#define USBD_DETACH_LEVEL LOW
+// USBD_ATTACH_LEVEL not needed.
+#define USBD_DP_TRICK
+#endif
+
 /**
-  * @brief  Force to re-enumerate USB
+  * @brief  Force to re-enumerate USB.
+  *
+  * This is intended to be called at startup by core code. It could be
+  * used at runtime, while USB is connected, to force re-enumeration
+  * too, but that does not work in all cases (when USB is enabled on an
+  * F103C8, setting output mode on the DP pin no longer has any effect).
+  *
   * @param  None
   * @retval None
   */
-
 WEAK void USBD_reenumerate(void)
 {
-#ifndef USBD_REENUM_DISABLED
-  /* Re-enumerate the USB */
-#ifdef USB_DISC_PIN
-  pinMode(USB_DISC_PIN, OUTPUT);
-  digitalWrite(USB_DISC_PIN, LOW);
+#if defined(USBD_PULLUP_CONTROL_PINNAME)
+  /* Detach */
+  pin_function(USBD_PULLUP_CONTROL_PINNAME, STM_PIN_DATA(STM_MODE_OUTPUT_PP, GPIO_NOPULL, 0));
+  digitalWriteFast(USBD_PULLUP_CONTROL_PINNAME, USBD_DETACH_LEVEL);
+
+  /* Wait */
+  delay(USBD_ENUM_DELAY);
+
+  /* Attach */
+#if defined(USBD_DP_TRICK)
+  /* Revert back to input (floating), needed for the D+ trick */
+  pin_function(USBD_PULLUP_CONTROL_PINNAME, STM_PIN_DATA(STM_MODE_INPUT, GPIO_NOPULL, 0));
 #else
-#ifdef USE_USB_HS_IN_FS
-  PinName pinDP = USB_OTG_HS_DP;
-#elif defined(USB_OTG_FS)
-  PinName pinDP = USB_OTG_FS_DP;
-#else /* USB */
-  PinName pinDP =  USB_DP;
-#endif
-  pin_function(pinDP, STM_PIN_DATA(STM_MODE_OUTPUT_PP, GPIO_NOPULL, 0));
-  digitalWriteFast(pinDP, LOW);
+  digitalWriteFast(USBD_PULLUP_CONTROL_PINNAME, USBD_ATTACH_LEVEL);
+#endif /* defined(USBD_PULLUP_CONTROL_FLOATING) */
+#elif defined(USBD_HAVE_INTERNAL_PULLUPS)
+  USB_DevDisconnect(USBD_USB_INSTANCE);
   delay(USBD_ENUM_DELAY);
-  pin_function(pinDP, STM_PIN_DATA(STM_MODE_INPUT, GPIO_NOPULL, 0));
-  /*delay(USBD_ENUM_DELAY);*/
-#endif /* USB_DISC_PIN */
-#endif /* USBD_REENUM_DISABLED */
+  USB_DevConnect(USBD_USB_INSTANCE);
+#else
+#warning "No USB attach/detach method, USB might not be reliable through system resets"
+#endif
 }
 
+#else /* !defined(USBD_REENUM_DISABLED) */
+WEAK void USBD_reenumerate(void) { }
+#endif
+
 #ifdef USBD_USE_CDC
 void USBD_CDC_init(void)
 {
diff --git a/variants/Generic_F401Rx/variant.h b/variants/Generic_F401Rx/variant.h
@@ -126,7 +126,9 @@ extern "C" {
 #define PIN_SERIAL_TX           PA9
 
 #ifdef ARDUINO_CoreBoard_F401RC
-#define USB_DISC_PIN            PD2
+// USB, pull this pin low to enable the USB attach pullup
+#define USBD_ATTACH_PIN         PD2
+#define USBD_ATTACH_LEVEL       LOW
 #endif
 
 #ifdef __cplusplus
diff --git a/variants/MALYANM200_F103CB/variant.cpp b/variants/MALYANM200_F103CB/variant.cpp
@@ -86,25 +86,6 @@ const PinName digitalPin[] = {
 extern "C" {
 #endif
 
-/**
-  * Initialize the variant. Turn on PB9 for USB.
-  */
-void initVariant()
-{
-  pinMode(PB9, OUTPUT);
-  digitalWrite(PB9, 1);
-}
-
-void USBD_reenumerate(void)
-{
-  pinMode(PB9, OUTPUT);
-  digitalWrite(PB9, HIGH);
-  delay(10);
-  digitalWrite(PB9, LOW);
-  delay(10);
-  digitalWrite(PB9, HIGH);
-}
-
 /**
   * @brief  System Clock Configuration
   *         The system Clock is configured as follow :
diff --git a/variants/MALYANM200_F103CB/variant.h b/variants/MALYANM200_F103CB/variant.h
@@ -105,6 +105,10 @@ extern "C" {
 #define PIN_SERIAL_RX           PA10
 #define PIN_SERIAL_TX           PA9
 
+// USB, pull this pin low to *disable* the USB attach pullup
+#define USBD_DETACH_PIN         PB9
+#define USBD_DETACH_LEVEL       LOW
+
 /* Dedicated definitions */
 #ifndef MAX_PRIORITY
 #define MAX_PRIORITY            15
diff --git a/variants/MAPLEMINI_F103CB/variant.h b/variants/MAPLEMINI_F103CB/variant.h
@@ -108,8 +108,9 @@ extern "C" {
 #define PIN_SERIAL_RX           PA10
 #define PIN_SERIAL_TX           PA9
 
-// USB
-#define USB_DISC_PIN            PB9
+// USB, pull this pin low to enable the USB attach pullup
+#define USBD_ATTACH_PIN         PB9
+#define USBD_ATTACH_LEVEL       LOW
 
 #ifdef __cplusplus
 } // extern "C"
diff --git a/variants/board_template/variant.h b/variants/board_template/variant.h
@@ -162,6 +162,19 @@ extern "C" {
 // See AN4879 https://www.st.com/content/st_com/en/search.html#q=AN4879-t=resources-page=1
 //#define USBD_VBUS_DETECTION_ENABLE
 
+// If the board has external USB pullup (on DP/DM depending on speed)
+// that can be controlled using a GPIO pin, define these:
+//  - If the the pullup is disabled (USB detached) by default, define
+//    USBD_ATTACH_PIN to the pin that, when written to
+//    USBD_ATTACH_LEVEL, attaches the pullup.
+//  - If the the pullup is enabled (attached) by default, define
+//    USBD_DETACH_PIN to the pin that, when written to
+//    USBD_DETACH_LEVEL, detaches the pullup.
+//#define USBD_ATTACH_PIN x
+//#define USBD_ATTACH_LEVEL LOW
+//#define USBD_DETACH_PIN x
+//#define USBD_DETACH_LEVEL LOW
+
 #ifdef __cplusplus
 } // extern "C"
 #endif
