Implement dcd_edpt_close_all() and fix dcd_edpt_clear_stall() for frdm_kl25z

[kkitayam]

Describe the PR

• Implement dcd_edpt_close_all() function.
• Fix dcd_edpt_close_all() function. This function worked for endpoint 0 but others.

Additional context

• Chapter 9 tests pass with the exception of four tests regarding suspend/resume/remote-wakeup. frdm_kl25z does not have any switches, so these tests cannot be tried.
• All HID tests pass.
• The MSC tests pass with the exception of five tests: error-recovery/case 4,8,10/required-command. Not investigated yet.

[hathach]

Superb ! Thank you very much for the update. For frdm_kl25z, we could pick one of its header pin and use wire jumper for input. Though we could always do it later.

For the MSC test, it is the most tricky test, have you tried to explicitly reset the PID of endpoint to DATA0 when dcd_edpt_open() is called. Since one of the msc test sequence is reset by simply Set Config(0) then SetConfig(1), and expect all the endpoints to be reset.

[HiFiPhile]

For frdm_kl25z, we could pick one of its header pin and use wire jumper for input

For boards without I/O, I think we can make bsp functions 'weak' to make it easier to override for testing purposes while keeping git workflow clean.
For example my nuc126 board has a button and I could push the wrong file.

[hathach]

For frdm_kl25z, we could pick one of its header pin and use wire jumper for input

For boards without I/O, I think we can make bsp functions 'weak' to make it easier to override for testing purposes while keeping git workflow clean.
For example my nuc126 board has a button and I could push the wrong file.

It is not necessary, pin/port is fixed on individual board, If you have an custom nuc126 board, you should add your own board file that correctly reflect your hardware.

[kkitayam]

Thank you for your comments. I will try to test with a jumper wire.

[hathach]

I have tried this out with cdc_msc example but seems still be issue with stall/clear stall. kl25z doesn't seem to clear stall correctly. As captured in the bus, the device continue to response with STALL after a clear stall attempt from host (ubuntu 20.04), and cause it to issue bus reset for recovery. And it loops over again, until host driver give up and doesn't issue the MODE SENSE command.

transaction 7883 + 7941 is clear stall control request and ack. Afterward the msc bulk in still response with stall. looking more closely to this.

[kkitayam]

Thank you very much for your comments and checking the patch!
It is very helpful for me. I check the patch again.

When USBCV was run, no other USB devices would work, so I did not get testing logs.

[hathach]

Thank you very much for your comments and checking the patch!
It is very helpful for me. I check the patch again.

When USBCV was run, no other USB devices would work, so I did not get testing logs.

You only need to test with normal operation (confirmed issue hapend on windows as well). The side-effect would be a decent delay before the device show up. The log with LOG=2 LOGGER=rtt will show the issue (around the stall -> clear stall -> bus reset). There is an hidden (no-log) transaction as captured above, which cause host think the device didn't process the clear feature, it does send another clear feature then immediately follow by bus-reset.

USBD Xfer Complete on EP 83 with 4 bytes
  MSC xfer callback
  SCSI Data
  SCSI case 5 (Hi > Di): 192 > 4
    Stall EP 83

USBD Setup Received 02 01 00 00 83 00 00 00 
  Clear Feature
    Clear Stall EP 83
  MSC control request
  Queue EP 83 with 13 bytes ...
  Queue EP 80 with 0 bytes ...
USBD Xfer Complete on EP 80 with 0 bytes

/* There is STALL transaction here on endpoint 83 as captured on the above screenshot  */

USBD Setup Received 02 01 00 00 83 00 00 00 
  Clear Feature
  MSC control request
  Queue EP 80 with 0 bytes ...
USBD Xfer Complete on EP 80 with 0 bytes
USBD Bus Reset : Full Speed   <---- indicator that host desperately try to recover

The more I tried to alter the clear_stall() the more I think the edpt_clear_stall() does not do clear the stall response. Controller doesn't seem to update its knowledge when we do bdt_stall = 0. I am not sure how to force that (have been reading the KL25 manual back and forth for this).

[hathach]

@kkitayam I merged from master since the current state has build error with LOG=2. Also take the chance to push minor clean up for the dcd. Mostly for adding volatile (_IO) to the bd filed that could be changed by controller. It does not help with this issue, but would still be better to add.

[kkitayam]

@hathach
Thank you for your help. I found that a issue regarding stall interrupt handling.
When dcd set a stall response by setting bdt_stall = 1, it set ENDPOINT[epn].EPSTALL. I found we need to clear the bit in process_stall().

I wasn't expecting ENDPOINT[epn].EPSTALL to be set!

[kkitayam]

I confirmed MSC test pass with fbe1bf3. But, Chapter 9 regarding suspend/resume/remote wakeup fail still.
I'll check these issues

[hathach]

@hathach
Thank you for your help. I found that a issue regarding stall interrupt handling.
When dcd set a stall response by setting bdt_stall = 1, it set ENDPOINT[epn].EPSTALL. I found we need to clear the bit in process_stall().

I wasn't expecting ENDPOINT[epn].EPSTALL to be set!

Superb !!! I confirmed it passed the MSC test suite. Yeah, I have read the KL25z manual back and forth and didn't see any mentioned about controller will set this bit and the need to clear it. Now, I feel this bit can cause race condition since we can potentially stall the other direction as well e.g stalled 0x83 can also cause 0x03 stalled if we don't clear this bit fast enough. Anyway, that is just the theory. the USB frame is 1ms and we surely could clear this fast enough.

the TD 9.14 Suspend Resume test probably is dued to resume event detection. I will try to see if I could help with this.

[hathach]

@kkitayam I confirmed that we couldn't detect the RESUME signal currently, suspend detection is fine. Btw, i found an new usb tool HSETT https://www.usb.org/compliancetools#anchor_usbxhsett is very useful to manipulate the bus manually. After too tired of pressing power button on my windows machine to put it on suspend and resume. This can be done by using HSETT

[hathach]

@kkitayam Update: I found and fixed the issue, just in case you are troubleshooting it, please wait for my next push :)

[kkitayam]

@hathach Great! Thank you for your work. I am ready for your next push.

[hathach]

@kkitayam I have pushed the new commit, it passed all the suspend/resume/remote-wakeup test now. The issue is due to the RESUME detection, it seems the ISTATW RESUME is never triggered. possibly used for host. The tranceiver resume_int is correct way to detect bus activity. Also it will be triggered when the USB_USBTRC0_USBRESMEN_MASK is set regardless of the INTEN.

PS: I also push up some minor clean up, please review and let me know if those makes sense to you, especially removing the short return in the ISR. I think there is high chance that multiple interrupt are triggered at once.

[kkitayam]

@hathach Thank you for your commit. I have confirmed all Chapter 9 test pass. And, all modifications look good for me. Great!

BTW, dcd_mm32f327x_otg.c is almost the same code. We should apply this patch to the dcd.

[hathach]

@kkitayam Thank you for your testing. oh, I didn't know that mcu also use the same controller as khci. It is submitted by mcu vendor #869 . And I am unaware of the driver is based on yours. The license should name yours instead. I will correct that. Ideally, it should be the same common dcd file though, I am really bad of naming, I wish I know the USB IP name (like synopsys) to make it spot-on, but I will try.

I haven't run tested on the mcu just yet, due to their sdk issue with case-sensitive file naming which cause failed to compile on Linux. zhangslice/mm32sdk#1 I made a PR, to correct that but they are not very responsive then I switch to other works. I will try to pull out mm32 board to try again later on. Thank you very much again.

[hathach]

While trying to figure out the IP name of this controller, I just found out that this USB IP is also used by

• PIC32 family https://ww1.microchip.com/downloads/en/DeviceDoc/61126F.pdf
• DSTni-EX https://www.lantronix.com/products/dstni-ex/

[kkitayam]

It is very interesting! I've also looked into PIC18F2455 too. It looks that this MCU has a similar USB IP. The IP has PPB(Ping-Pong Buffer) instead of BDT. And it seems that PPB had been applied as a patent in 2004.
BDT may be an upgraded version of PPB by microchip.

I found that the USB IP seems to be called as VUSB in HCC-embedded's USB stack. But I couldn't find the name on any other web site.

[hathach]

@kkitayam I spent several hours to do a bit of research on this, here is my findout.

• PIC32 uses ChipIdea Fullspeed OTG core, because at that time, chipidea is part of MIPS. https://www.design-reuse.com/news/17724/analog-and-processor-ip.html

The Chipidea Full Speed USB 2.0 OTG Controller is a highly optimized, low gate count IP core that allows system developers to implement compact, cost-effective, low-power USB-based SoC solutions.

• ChipIdea previously bought this IP from Transdimension
• MIPS later on sell these to Synopsys

Overall, Freescale along with NXP seem to use the USB IP designed by TransDimension e.g lpc18xx/imxrt use highspeed version. However, since these are more commonly known as ChipIdea in other project such as linux https://github.com/torvalds/linux/blob/master/drivers/usb/chipidea/ci.h#L43 . So I intend to rename existing transdimension to chipidea highspeed, and this one to chipidea fullspeed.