Fix remote wakeup issue

[lokher]

Description

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Issues Fixed or Closed by This PR

• Fix remote wakeup issue that cause by Add hotfix for chibios keyboards not wake #10088 to fix [Bug] keyboard stops responding after power cycles and found where goes wrong #9962
  Have been tested on STM32F072 , STM32F401 and STM32L432.

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[tzarc]

@LSChyi can you confirm this does not make your problematic boards regress?

This PR potentially messes with your previous fix, and I'm reluctant to merge a fix for some boards that break others in the process.

[LSChyi]

@LSChyi can you confirm this does not make your problematic boards regress?

This PR potentially messes with your previous fix, and I'm reluctant to merge a fix for some boards that break others in the process.

Confirmed with this patch, the board blackpill_f401 I used before can wake the host computer after entering the suspended mode.

[sigprof]

This is really interesting. The F411 reference manual does not seem to say that the GATEHCLK and STPPCLK bits in OTG_FS_PCGCCTL could be set by the hardware, and the ChibiOS code only clears those bits and never tries to set them, so if you actually observe that those bits are set, this may mean that the reference manual is not 100% correct.

The restart_usb_driver() implementation which was added to platforms/chibios/IC_TEENSY_3_1/board/board.c in #12870 should now be removed too (the new code will do nothing on Kinetis chips anyway), and changes to other files may also need to be reverted (the comments about USB driver restart will be wrong after applying this change).

This also removes USB_SUSPEND_WAKEUP_DELAY added in #11450 for Arm/ChibiOS boards; @spidey3 can you check that the code would actually work for you in this state? Maybe the added wait is actually unneeded if the USB disconnect is not performed; however, it is also possible that some weird hardware combinations actually need that delay.

Another concern is the _usb_wakeup() call in there — it is not a public ChibiOS API, and this call may be a duplicate if the GINTSTS_WKUPINT interrupt also comes (but from looking at #9962 seems that GINTSTS_WKUPINT does not actually come in some cases, therefore some workaround, like this one, may be needed).

[zvecr]

merge a fix for some boards that break others in the process.

Also there was #9962 (comment) from @drashna. As this removes the previous implementation, F303 would revert back to the previously broken behaviour.

[lokher]

I suspect GINTSTS_WKUPINT bit is not set correctly when STM32F401 initiate resuming, so we have to invoke _usb_wakeup() manually.

[tzarc]

Sounds like this should be a bug raised against ChibiOS, rather than being hacked around fixed within QMK.

[lokher]

I prefer to believe different USB unit types need different approach since STM32F401 integrates USB OTG rather than USB device.

Let's make some tests to trace it base on this PR.

1. Set CONSOLE_ENABLE to yes in rules.mk of keyboard
2. Add code snippet to hal_usb_lld.c as below: :

uint8_t suspend_count = 0;
uint8_t isr_wake_count = 0;

...
static void usb_lld_serve_interrupt(USBDriver *usbp) {
   ...
  /* Reset interrupt handling.*/
  if (sts & GINTSTS_USBRST) {
    ...
    isr_wake_count = 0;
    suspend_count = 0;
    ...
  }

  /* Wake-up handling.*/
  if (sts & GINTSTS_WKUPINT) {
    ...
    isr_wake_count++;
  }

  /* Suspend handling.*/
  if (sts & GINTSTS_USBSUSP) {
    suspend_count++;
    ...
  }
}

3. Add code snippet to hal_usb_lld.c as below:

uint8_t qmk_wake_count = 0;

__attribute__((weak)) void usb_wakeup(USBDriver *usbp) {
    qmk_wake_count++;
    ...
}

4. Add code snippet to your_keyboard.c as below:

extern uint8_t suspend_count, isr_wake_count, qmk_wake_count;

bool process_record_user(uint16_t keycode, keyrecord_t *record) {
#ifdef CONSOLE_ENABLE
    if (record->event.pressed) {
        switch(keycode) {
            case KC_D:
              uprintf("suspend_count: %d, isr_wake_count: %d, qmk_wake_count: %d\n", 
              suspend_count, isr_wake_count, qmk_wake_count);
              break;
            case KC_C:
              suspend_count = isr_wake_count = qmk_wake_count = 0;
              uprintf("Cleared\n");
              break;
        }
    }
#endif 
    return true;
}

5. Compile and download the firmware to the keyboard and make sure it works properly, and run QMK Toolbox or other app to show console output.
6. Put the PC into sleep mode, wait a few seconds for the keyboard to enter sleep mode, then use the keyboard to wake the PC up, and press 'D' key to trace suspend_count , isr_wake_count, qmk_wake_count value.
7. Put the PC into sleep mode, wait a few seconds for the keyboard to enter sleep mode, then use another keyboard such as
   the built-in keyboard of notebook to wake the PC up, and press 'D' key to trace suspend_count , isr_wake_count, qmk_wake_count value.
8. According to the value of suspend_count , isr_wake_count, qmk_wake_count , we are able to know wheather ISR wake up is invoked. Ideally, sum of isr_wake_count and qmk_wake_count should be equl to suspend_count . If the sum is greater than suspend_count , it means that _usb_suspend is invoked twice in a single suspend/wakeup cycle.

I did the test on STM32F401 and everything works fine, the sum of isr_wake_count and qmk_wake_count was always equl to suspend_count :

Device disconnected.
Waiting for new device:..............
Listening:
suspend_count: 0, isr_wake_count: 0, qmk_wake_count: 0
suspend_count: 0, isr_wake_count: 0, qmk_wake_count: 0
[s]suspend_count: 1, isr_wake_count: 0, qmk_wake_count: 1
suspend_count: 1, isr_wake_count: 0, qmk_wake_count: 1
suspend_count: 1, isr_wake_count: 0, qmk_wake_count: 1
[s]suspend_count: 2, isr_wake_count: 1, qmk_wake_count: 1
suspend_count: 2, isr_wake_count: 1, qmk_wake_count: 1
suspend_count: 2, isr_wake_count: 1, qmk_wake_count: 1
[s]suspend_count: 3, isr_wake_count: 1, qmk_wake_count: 2
[s]suspend_count: 4, isr_wake_count: 1, qmk_wake_count: 3

hope this help.

[lokher]

Maybe we should keep restart_usb_driver for future use, I need such API in my bluetooth build, because when setting current transport to bluetooth, I need to stop/disconnect USB transport, and restart it after setting current transport back to USB.

[stale]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[stale]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[github-actions]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[github-actions]

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.
// [stale-action-closed]