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

[rtl872x] Fixes USB serial port not being connectable on AMD based Windows #2625

Merged
merged 2 commits into from
Mar 3, 2023

Conversation

YutingYou
Copy link
Contributor

@YutingYou YutingYou commented Feb 17, 2023

TODO

const size_t RTW_USBD_TASK_MAIN_LOOP_SEMAPHORE_OFFSET = 364;
const size_t RTL_USB_DEV_PCD_OFFSET = 0xd8;
const size_t RTL_USB_PCD_SPINLOCK_OFFSET = 0x15c;

Most likely at the very least the above definitions need to be adjusted due to internal USB stack struct changes.

Problem

We found the USB request Get Line Coding couldn't get a response on the AMD Windows PC, the symptom could be that we can't open the com port on a serial tool software on the AMD Windows PC.

By enabling the low level USB log in hw_config.c

ConfigDebugClose = 0;
DBG_ERR_MSG_ON(MODULE_USB_OTG);
DBG_WARN_MSG_ON(MODULE_USB_OTG);
DBG_INFO_MSG_ON(MODULE_USB_OTG);
ConfigDebug[LEVEL_TRACE] |= BIT(MODULE_USB_OTG);

we can see the last log when P2 got stuck, that is a race condition to access TxFIFO between EP3 and EP0.

[MODULE_USB_OTG-LEVEL_TRACE]:NPTxFEmp: token received for EP3 when TxFIFO is not ready

Solution

Realtek helped to release a new patch and USB stack for us to fix this issue.

TODO

This patch not only fixes the USB issue, but also provides some new APIs and callbacks. If necessary, we can further improve the USB driver.

u8(*get_class_descriptor)(usb_dev_t *dev, usb_setup_req_t  *req);
u8(*clear_feature)(usb_dev_t *dev, usb_setup_req_t  *req);

u8 usbd_ep_set_stall(usb_dev_t *dev, u8 ep_addr);
u8 usbd_ep_clear_stall(usb_dev_t *dev, u8 ep_addr);
u8 usbd_ep_is_stall(usb_dev_t *dev, u8 ep_addr);
u8 usbd_ep0_set_stall(usb_dev_t *dev);

/* API for debug */
void usbd_config_debug(u8 enable);
void usbd_dump_registers(void);

Steps to Test

  1. Checkout this branch, compile firmware for P2
  2. Install a serial tool software and try to open/close the com port

Screen Shot 2023-01-05 at 17 58 57

References

[CH109872]


Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

@avtolstoy
Copy link
Member

@YutingYou Do we have to implement get_class_descriptor ? Previously class descriptor queries were coming in get_descriptor.

@YutingYou
Copy link
Contributor Author

@avtolstoy I reviewed their new CDC example, it doesn't make use of this callback, so I leave it nullptr.

@avtolstoy
Copy link
Member

I'll take a look at a few things later today:

  1. Do these values still make sense
const size_t RTL_USB_DEV_PCD_OFFSET = 0xd8;
const size_t RTL_USB_PCD_SPINLOCK_OFFSET = 0x15c;
  1. Can we replace usbd_pcd_ep_set_stall etc with new public variants
  2. Is __wrap_usb_hal_read_packet still required

@avtolstoy
Copy link
Member

DFU_Check_Reset() in bootloader doesn't seem to be working correctly anymore.

@YutingYou
Copy link
Contributor Author

OK, I'll try it on my P2.

@avtolstoy
Copy link
Member

So, looks like some internals of the various USB stack state structs did change and for example our hack in bootloader with rtw_down_sema and HAL_DFU_Process() restoring stack pointer did break.

@YutingYou
Copy link
Contributor Author

Yeah, some data structure does change in the new stack, for example, the callback register structure.

@YutingYou
Copy link
Contributor Author

@avtolstoy changing RTW_USBD_TASK_MAIN_LOOP_SEMAPHORE_OFFSET from 364 to 400 can fix this issue, could you please help double check it, then I'll push the fix. And do you know any other hacks that are usb-stack-related?

@avtolstoy
Copy link
Member

@YutingYou I'll make the changes to this branch, almost done verifying the rest of the 'internal' accesses to USB stack.

@avtolstoy
Copy link
Member

I think with the latest commit this should be working correctly now.

Copy link
Member

@scott-brust scott-brust left a comment

Choose a reason for hiding this comment

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

The SDK patch changes themselves are a bit too over my head, so I will not comment on those.
I will say that I can use my photon 2 with my AMD windows machine and open USB CDC terminal with it. It enumerates under windows in bootloader DFU mode (although I didnt flash anything on windows)

I have some comments on the other changes just to help me understand how we interact with the RTK USB stack:

Bootloader changes:

  • The bootloader uses USB for DFU, so usb stack is initialized in HAL_DFU_USB_Init()
  • The call chain is Device::attach() --> RtlUsbDriver::attach() --> and finally realtek SDK usbd_init()
  • The SDK usbd_init() call presumably creates a task(s?) for USB.
  • We store that task handle and context pointer in rtw_create_task() in the bootloader osdependencies file.
  • When the bootloader wants to run USB to enable DFU, it manually executes the RTK USB task via HAL_DFU_Process()
  • Part of the USB task in the SDK relies on rtw_down_sema()
  • When the SDK calls rtw_down_sema() we check the semaphore pointer and compare it to the USB task semaphore pointer.
  • We know the pointers address by looking into the USB task context (presumably some class or structure) at a magic offset. That offset has changed in the newest SDK as presumably that class/struct has as well.
  • If the USB task cannot acquire the semaphore, then instead of blocking or waiting, exit out of the rtw_usb_task() function and re-run the loop again (presumably until the semaphore can be acquired)

Device OS changes

  • The system implements all the functions in usbd_class_driver_t structure and passes that to the RTK SDK USB when attach() is called:
    usbd_register_class(&classDrv_);
  • When the SDK calls one of these functions, we store a pointer to the internal usb_dev_t structure of the USB stack
  • That structure has a member void *pcd that we are interested in accessing, hence the change in offset (and the note that we know the structure type so we could access it that way).
  • Not exactly sure what this pointer is, but I can see we do use it in things like flushEndpoint and stallEndpoint

@avtolstoy avtolstoy merged commit da211bb into develop Mar 3, 2023
@avtolstoy avtolstoy deleted the fix/usb-stack-and-patch branch March 3, 2023 15:43
@scott-brust scott-brust added this to the 5.3.1 milestone Mar 28, 2023
@technobly technobly changed the title Apply new usb stack and patch to fix the USB issue on Windows [rtl872x] Fixes USB serial port not being connectable on AMD based Windows Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants