cpu/rpx0xx: add periph_usbus support #20817

fengelhardt · 2024-08-18T19:07:28Z

Contribution description

This is a WIP contribution to add USB support for the rpx0xx MCU.

Testing procedure

The code is in a quite early stage. It compiles with examples/usbus_minimal, but it hangs somewhere quite early in the initialization. I will debug it as we go, please do not look all to deeply into it yet.

With this PR I primarily want to clarify some question I have regarding the USB stack.

Issues/PRs references

#15822 is for RPx0xx feature tracking.

dylad

At first glance, you're missing NVIC_EnableIRQ(USBCTRL_IRQ_IRQn);.
I used a debugger and check the PLL configuration. It looks fine but I really think some bits are missing (currently 0) like VBUS_DETECTED and SPEED in SIE_STATUS register.

dylad · 2024-08-29T13:39:38Z

cpu/rpx0xx/periph/usbdev.c

+    io_reg_atomic_set(&USBCTRL_REGS->MAIN_CTRL,
+                      USBCTRL_REGS_MAIN_CTRL_CONTROLLER_EN_Msk);
+    io_reg_atomic_set(&USBCTRL_REGS->USB_MUXING,
+                      USBCTRL_REGS_USB_MUXING_TO_PHY_Msk);


RP2040 datasheet also add USBCTRL_REGS_USB_MUXING_SOFTCON_Msk.

you should add

io_reg_atomic_set(&USBCTRL_REGS->USB_PWR, USBCTRL_REGS_USB_PWR_VBUS_DETECT_Msk | USBCTRL_REGS_USB_PWR_VBUS_DETECT_OVERRIDE_EN_Msk);

cpu/rpx0xx/periph/usbdev.c

dylad · 2024-08-29T14:00:45Z

With these changes, I can now receive the setup request from the host. Its interrupt is fired but never served by _usbdev_esr() and it loops forever so we never response to the setup request.

dylad · 2024-08-29T14:50:07Z

cpu/rpx0xx/periph/usbdev.c

+    }
+    if (USBCTRL_REGS->INTS) {
+        /* Device specific interrupt */
+        _disable_irq();


Disabling all IRQs here will have the side effect to also clear INTS register.
So when USBUS call its ESR callback, it doesn't process any IRQs as INTS is 0.
Depending on the interruption's source, you may need to clear this register either here or in usbdev_esr

cpu/rpx0xx/periph/usbdev.c

fengelhardt · 2024-08-31T11:00:35Z

With these changes, I can now receive the setup request from the host. Its interrupt is fired but never served by _usbdev_esr() and it loops forever so we never response to the setup request.

I already wondered how to handle setup packets. usbdev_event_t has no event defined for this case, so I can not notify usbus. Do I have to handle it here in the periph driver?

fengelhardt · 2024-08-31T12:35:53Z

Added a fixup with your suggestions @dylad, and improved handling of INTS in isr_usbctrl(). Now the line reset seems to work, and the EP 0 is initialized.

I had no time to test further yet. Maybe tomorrow I can also check my packet flow.

fengelhardt · 2024-09-01T10:17:31Z

Now the setup requests are recognized by usbus, but control flow is still garbage.

dylad · 2024-09-04T11:33:01Z

From what I can tell this is normal.
Setup packets are not put in EP0 buffer but at the beginning of DPSRAM (offset 0).
Currently, you try to pass DPSRAM + 0x100 to USBUS which contains garbage, thus USBUS doesn't recognize the setup packets.
However, if you pass the right offset when you received a setup packet I can clearly see that the host is requesting the device descriptor 🎉
Unfortunately it's seem there is an issue when responding to the request. I'm not even sure the host sees the response yet. I'll try to check with wireshark next debugging session.

dylad · 2024-09-04T13:35:37Z

cpu/rpx0xx/periph/usbdev.c

+                           USBCTRL_DPRAM_EP0_IN_BUFFER_CONTROL_FULL_0_Pos, 
+                           buf_filled_bit);
+    _ep_buf_ctrl_reg_write(ep->num, ep->dir, 
+                           USBCTRL_DPRAM_EP0_IN_BUFFER_CONTROL_AVAILABLE_0_Pos, 
+                           USBCTRL_DPRAM_EP0_IN_BUFFER_CONTROL_AVAILABLE_0_Msk);


It seems to me that you're also missing to set (at least) the length field in the Buffer Control register of the endpoint which might explain why the host didn't get the response of the first setup request.
If set to 0, hardware will try to transfer 0 bytes to the host which is obviously wrong.

fengelhardt · 2024-09-05T17:01:37Z

Setup packets are not put in EP0 buffer but at the beginning of DPSRAM (offset 0).

Wow, I guess I misread the data sheet here...

Some tinkering today did not work so far, but I have an idea for solving that. Sadly I have not time over the weekend. Might take me some time to figure that out.

fengelhardt · 2024-09-12T18:36:35Z

Still garbage, unfortunately. But I have found a few bugs.

I might simply have a race condition between the setup request handling and data packet handling. Not sure how to handle that with usbus. The USB controller treats setup request differently from normal data, which makes it difficult to hand over to usbus.

Maybe my endpoint initialization is not correct, too.

dylad · 2024-09-12T19:01:51Z

I'll see if I can find some times this weekend to help you with that.

dylad

I'm having a bad time trying to understand the PID stuff on this IP.
For setup request, TinyUSB and the standalone example from raspberry PI tell that we must answer the setup request on PID 1 but there is no such reference in the datasheet.

dylad · 2024-09-17T14:34:48Z

cpu/rpx0xx/periph/usbdev.c

+    _ep_buf_ctrl_reg_write(ep->num, ep->dir, 
+                           USBCTRL_DPRAM_EP0_IN_BUFFER_CONTROL_FULL_0_Msk, 
+                           buf_filled_bit);
+    _ep_buf_ctrl_reg_write(ep->num, ep->dir, 
+                           USBCTRL_DPRAM_EP0_IN_BUFFER_CONTROL_AVAILABLE_0_Msk,
+                           USBCTRL_DPRAM_EP0_IN_BUFFER_CONTROL_AVAILABLE_0_Msk);
+    _ep_buf_ctrl_reg_write(ep->num, ep->dir, len,
+                           USBCTRL_DPRAM_EP0_IN_BUFFER_CONTROL_LENGTH_0_Msk);


Let's try to use a variable and write the whole config at once instead.
Moreover, the datasheet states that the AVAILABLE bit should be set AFTER the rest of the configuration in this register. If USB PLL is running at 48 MHz which is our case IIRC. 3 nop instructions are needed before setting the available bit.

fengelhardt · 2024-10-04T19:06:23Z

I have made some progress based on @dylad 's comments. I think I figured out the usb controller more or less. I get the setup requests and some control flow afterwards. However, I totally misunderstood how RIOT's usbus works and there is is still a problem. It seems I do not receive any packets and the control flow looks wrong.

dylad · 2024-12-18T08:23:30Z

I'm making small progress on my side. I am now stuck halfway through the enumeration process. I'll prepare a more in-depth review once the enumeration process works flawlessly.

fengelhardt · 2025-01-01T17:55:34Z

Thank you very much @dylad for your effort. I recently thought that it could only be an issue of setting the PID correctly. I tried hardcoding the PID and I could then receive at least one correct packet from the device. Maybe I find some time to study the PID handling of the tinyUSB driver to implement that here.

Wish everyone a happy new year btw.

dylad · 2025-01-02T08:31:39Z

Hi @fengelhardt Happy new year to you too.
I wasn't able to work on this lately so no progress on my side. I was planning to start the next round of review once I got enumeration fully working but that's not the case yet.
If you want I can do the next round of review now so you can also work on this on your side in the meantime. Basically, ISR management and PID need rework + a few errors here and there. What do you prefer ?

fengelhardt · 2025-01-02T17:02:05Z

Maybe let me first try to get it running. The PID handling I need to rework a lot, I think. I hope to find time in he coming days.

dylad

I'm starting the next round of review so it can help you:
The current ISR management needs a rework. Some events are called twice during a single handling which confuse USBUS.
As this is not really easy to explain with Github UI, you may look at my working branch to see what I have done on my side.
I didn't put all the changes I've made in my branch into this review as I am still debugging and thus not a 100% sure this is the right move but feel free to give it a try.

On my side, the first request is answered, then the device is assigned a new USB address by the host. Reset is issue to apply the new address and the enumeration process starts over. The first setup request with the new address is sent and answered but after that nothing... so the host issue a reset after 5 seconds or so and we start over ...
If you have question on some changes I've made, don't hesitate to ask here or on Matrix :)

dylad · 2025-01-02T21:18:03Z

cpu/rpx0xx/periph/usbdev.c

+        (len & USBCTRL_DPRAM_EP0_IN_BUFFER_CONTROL_LENGTH_0_Msk)
+        | buf_filled_bit
+        | pid_bit
+        | USBCTRL_DPRAM_EP0_IN_BUFFER_CONTROL_AVAILABLE_0_Msk;


Don't set the AVAILABLE bit here. RP2040 datasheet states that this bit needs to be set AFTER (using your busy_wait implementation) others bits to avoid sync issue inside the controller.

dylad · 2025-01-02T21:19:10Z

cpu/rpx0xx/periph/usbdev.c

+
+static rpx0xx_usb_ep_t *_get_ep(uint8_t ep_num, usb_ep_dir_t dir)
+{
+    return &_hw_usb_dev.hw_ep[2 * ep_num + (dir == USB_EP_DIR_OUT ? 0 : 1)];


Suggested change

return &_hw_usb_dev.hw_ep[2 * ep_num + (dir == USB_EP_DIR_OUT ? 0 : 1)];

return &_hw_usb_dev.hw_ep[2 * ep_num + (dir == USB_EP_DIR_IN ? 0 : 1)];

This looks innocent but this changes is really important.

dylad · 2025-01-02T21:23:15Z

cpu/rpx0xx/periph/usbdev.c

+static void _ep_set_stall(usbdev_ep_t *ep, usbopt_enable_t enable)
+{
+    rpx0xx_usb_ep_t *hw_ep = _get_ep(ep->num, ep->dir);
+    hw_ep->next_pid = 0u;


Suggested change

hw_ep->next_pid = 0u;

I think this line should be remove for a correct PID management otherwise we end up always using the same PID.

dylad · 2025-01-02T21:25:23Z

cpu/rpx0xx/periph/usbdev.c

+    if (hw_ep->data_buf == NULL) {
+        DEBUG(" tr completed\n");
+        _hw_usb_dev.dev.epcb(&hw_ep->ep, USBDEV_EVENT_TR_COMPLETE);
+        return;
+    }


There is no need for this. USBDEV_EVENT_TR_COMPLETE should be handle by ESR or EP_ESR directly.

dylad · 2025-01-02T21:26:58Z

cpu/rpx0xx/periph/usbdev.c

+    DEBUG("[rpx0xx usb] Call _usbdev_ep_xmit(ep.num=%d, ep.dir=%s, buf=%p, len=%u)\n",
+          ep->num, PRINT_DIR(ep->dir), buf, len);
+
+    assert(buf != NULL);


You may also drop this one.
USBUS can call this function with a NULL buffer / 0 length in order to send an ACK to Host.

dylad · 2025-01-08T20:29:50Z

@fengelhardt Finally I've got some good news !

You may find all the changes I've made on my work branch.

fengelhardt · 2025-01-08T20:47:19Z

Nice, I will have a look! 08.01.2025 21:30:19 Dylan Laduranty ***@***.***>:

…

@fengelhardt[https://github.com/fengelhardt] Finally I've got some good news ! Copie.d.ecran_20250108_212408.png (view on web)[https://github.com/user-attachments/assets/3d5e2da5-49b8-43ff-bb09-83736f470f6e] You may find all the changes I've made on my work branch. — Reply to this email directly, view it on GitHub[#20817 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAY3EV6U3ZYJD3OIZVDGV4D2JWDFLAVCNFSM6AAAAABMWTG35GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNZYGU4TONRYG4]. You are receiving this because you were mentioned. [Verfolgungsbild][https://github.com/notifications/beacon/AAY3EV67TUCPEZCSOIXQMTT2JWDFLA5CNFSM6AAAAABMWTG35GWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTUZWJDTO.gif]

github-actions bot added the Area: cpu Area: CPU/MCU ports label Aug 18, 2024

dylad self-requested a review August 19, 2024 08:43

benpicco requested a review from bergzand August 19, 2024 08:43

fengelhardt changed the title ~~cpu/rpxx0xx: add periph_usbus support~~ cpu/rpx0xx: add periph_usbus support Aug 20, 2024

dylad reviewed Aug 29, 2024

View reviewed changes

cpu/rpx0xx/periph/usbdev.c Outdated Show resolved Hide resolved

fengelhardt requested a review from aabadie as a code owner September 1, 2024 10:14

github-actions bot added Area: USB Area: Universal Serial Bus Area: sys Area: System labels Sep 1, 2024

dylad reviewed Sep 4, 2024

View reviewed changes

dylad reviewed Sep 17, 2024

View reviewed changes

benpicco requested a review from OlegHahm October 7, 2024 17:20

fengelhardt added 6 commits November 6, 2024 12:28

cpu/rpxx0xx: add periph_usbus support

ee12f43

fixup! cpu/rpxx0xx: add periph_usbus support

4a2c437

fixup! fixup! cpu/rpxx0xx: add periph_usbus support

94ec546

fixup! cpu/rpxx0xx: add periph_usbus support

b8d5f90

fixup! cpu/rpxx0xx: add periph_usbus support

6d4f305

fixup! fixup! cpu/rpxx0xx: add periph_usbus support

8d3b3e3

fengelhardt force-pushed the rpix0xx_usbus branch from 2639284 to 8d3b3e3 Compare November 6, 2024 11:29

dylad reviewed Jan 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu/rpx0xx: add periph_usbus support #20817

cpu/rpx0xx: add periph_usbus support #20817

fengelhardt commented Aug 18, 2024

dylad left a comment •

edited

Loading

dylad Aug 29, 2024

dylad commented Aug 29, 2024

dylad Aug 29, 2024

fengelhardt commented Aug 31, 2024

fengelhardt commented Aug 31, 2024 •

edited

Loading

fengelhardt commented Sep 1, 2024

dylad commented Sep 4, 2024

dylad Sep 4, 2024

fengelhardt commented Sep 5, 2024

fengelhardt commented Sep 12, 2024

dylad commented Sep 12, 2024

dylad left a comment

dylad Sep 17, 2024

fengelhardt commented Oct 4, 2024

dylad commented Dec 18, 2024

fengelhardt commented Jan 1, 2025

dylad commented Jan 2, 2025

fengelhardt commented Jan 2, 2025

dylad left a comment

dylad Jan 2, 2025

dylad Jan 2, 2025

dylad Jan 2, 2025

dylad Jan 2, 2025

dylad Jan 2, 2025

dylad commented Jan 8, 2025

fengelhardt commented Jan 8, 2025 via email

	return &_hw_usb_dev.hw_ep[2 * ep_num + (dir == USB_EP_DIR_OUT ? 0 : 1)];
	return &_hw_usb_dev.hw_ep[2 * ep_num + (dir == USB_EP_DIR_IN ? 0 : 1)];

cpu/rpx0xx: add periph_usbus support #20817

Are you sure you want to change the base?

cpu/rpx0xx: add periph_usbus support #20817

Conversation

fengelhardt commented Aug 18, 2024

Contribution description

Testing procedure

Issues/PRs references

dylad left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylad commented Aug 29, 2024

Choose a reason for hiding this comment

fengelhardt commented Aug 31, 2024

fengelhardt commented Aug 31, 2024 • edited Loading

fengelhardt commented Sep 1, 2024

dylad commented Sep 4, 2024

Choose a reason for hiding this comment

fengelhardt commented Sep 5, 2024

fengelhardt commented Sep 12, 2024

dylad commented Sep 12, 2024

dylad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fengelhardt commented Oct 4, 2024

dylad commented Dec 18, 2024

fengelhardt commented Jan 1, 2025

dylad commented Jan 2, 2025

fengelhardt commented Jan 2, 2025

dylad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylad commented Jan 8, 2025

fengelhardt commented Jan 8, 2025 via email

dylad left a comment •

edited

Loading

fengelhardt commented Aug 31, 2024 •

edited

Loading