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

bugfix(otgfs): Fixes a couple issue with OTGFS #58

Merged
merged 4 commits into from
Nov 1, 2024

Conversation

Codetector1374
Copy link
Member

@Codetector1374 Codetector1374 commented Nov 1, 2024

This change address mainly 2 concerns regarding the OTGFS driver

  1. There seems to be potential edge cases where the various transfer polling function is not actually checking for the correct endpoint number before clearing the transfer flag, causing potential lost of transaction.
  2. Endpoint is somehow enabled by default, which cause any transaction sent to endpoint prior to actually calling the endpoint_set_enable function be lost. This is caused by the hardware behaving differently than what the manual describes.

Beside the two bugfix, it also lays the groundwork for USBHS by extracting some of the common code into the usb module.

This change extracted all the common USB code that will be shared
between otgfs and usbhs
Transfer flag could be cleared when it is not the end point expected.
This would be a problem under spurious wakeup.
Copy link
Member

@Dummyc0m Dummyc0m left a comment

Choose a reason for hiding this comment

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

LGTM

feel free to merge after addressing the comments.

let res = if status.mask_uis_endp() == 0 {
match status.mask_token() {
if status.mask_uis_endp() == 0 {
let res = match status.mask_token() {
UsbToken::OUT => {
if regs.rx_len().read().0 != 0 {
error!("Expected 0 len OUT stage, found non-zero len, aborting");
Copy link
Member

Choose a reason for hiding this comment

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

v.set_r_tog(true) a few lines down is not necessary.

regs.uep_rx_ctrl(i).write(|v| v.set_mask_r_res(EpRxResponse::NAK));
regs.uep_tx_ctrl(i).write(|v| v.set_mask_t_res(EpTxResponse::NAK));
self.endpoint_set_enabled(EndpointAddress::from_parts(i, Direction::In), false);
Copy link
Member

Choose a reason for hiding this comment

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

add comment

@Dummyc0m Dummyc0m self-assigned this Nov 1, 2024
Endpoints should be disabled on bus reset. According to datasheet it
looks like the UEPx_y_MOD.{RX_EN,TX_EN} should have reset value of 0 but
clearly they are reset to 1 which case our endpoints to be "enabled" by default
Copy link
Contributor

@andelf andelf left a comment

Choose a reason for hiding this comment

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

LGTM

@andelf andelf merged commit e2bee21 into ch32-rs:main Nov 1, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants