Port change should not re-initialize endpoints

[mciantyre]

imxrt-usbd/src/driver.rs

Lines 405 to 408 in a2f2ce5

	if usbsts & USBSTS::PCI::mask != 0 {
	ral::write_reg!(ral::usb, self.usb, USBSTS, PCI: 1);
	self.initialize_endpoints();
	}

This branch in poll() tried to re-initialize (disable) endpoints under two conditions:

1. when exiting reset.
2. on deconfiguration.

However, we take this branch when exiting suspend. If we disable endpoints when exiting suspend, we break USB device functions.

This branch isn't properly handling the deconfiguration aspect of the problem; port change doesn't imply deconfiguration. Port change may imply device reset, but we already have dedicated code paths for that. So, we should be able to move this into a reset handling / driver initialization routine.

There's more troubleshooting and discussions in threads here.

[Finomnis]

Any news on this? Otherwise I can try to understand the documentation of the USB peripheral, and try to come up with something.

[Finomnis]

@mciantyre What's the point of configure? Why do we need that function, in general?

Trying to understand the code to come up with a sensible solution.

[mciantyre]

Nothing on my end. If you're looking for a starting point, I'd start with something like this. This passed the expected test cases from the usb-device test suite.

What's the point of configure? Why do we need that function, in general?

My interpretation of the RM is that we should only enable (non-zero) endpoints once we're configured. configure() achieves that. As the UsbBus implementer, I don't know when the device is configured, so I ask the device user to tell me.

We might not need this with driver tweaks, but I never tested without it.

[Finomnis]

It seems to work for me. Is there a reason why we shouldn't merge mciantyre@1d51056 as-is?

Also, it seems that the usb-device test suite does not contain a suspend test, if that's even possible.

[Finomnis]

"I don't know when the device is configured" - Ok I think I get it now - I didn't realize that the driver has no way of knowing the the UsbDeviceState.

That said - where exactly did you read that requirement? I didn't find it in the IMXRT1060RM.pdf yet, although this is a complex chapter and I am sure I missed it.

Also, the current way of dealing with that will set us up for failure later - if the transition to UsbDeviceState::Configured is what causes configuration in the user code, this will later also happen when transitioning away from UsbDeviceState::Suspend.

[Finomnis]

Should I clone your change and open a PR for it, or will you do it directly? Your fix works on my project, so I'm happy to +1 it.

[mciantyre]

it seems that the usb-device test suite does not contain a suspend test,

That's my issue: I can't easily reproduce and test this. If it works for you, we can move forward with #24.

where exactly did you read that requirement?

I interpreted the TXE and RXE field documentation for all non-zero ENDPTCTRL registers as the requirement.

We also need to tickle the TX / RX data toggle reset during configuration. Endpoint::enable() handles both enable and toggle reset in one go.

if the transition to UsbDeviceState::Configured is what causes configuration in the user code, this will later also happen when transitioning away from UsbDeviceState::Suspend.

Good point. I'll link to #4 so we can consider this when handling suspend.