[rp2040] Make writes to SIE_CTRL aware of concurrent access #2024
Conversation
|
I approve this change so please merge if you are happy @hathach |
| @@ -562,6 +562,9 @@ bool hcd_edpt_xfer(uint8_t rhport, uint8_t dev_addr, uint8_t ep_addr, uint8_t * | |||
| uint32_t flags = USB_SIE_CTRL_START_TRANS_BITS | SIE_CTRL_BASE | | |||
| (ep_dir ? USB_SIE_CTRL_RECEIVE_DATA_BITS : USB_SIE_CTRL_SEND_DATA_BITS) | | |||
| (need_pre(dev_addr) ? USB_SIE_CTRL_PREAMBLE_EN_BITS : 0); | |||
| // write everything except the START_TRANS bit first, then wait | |||
| usb_hw->sie_ctrl = flags & ~USB_SIE_CTRL_START_TRANS_BITS; | |||
| __asm volatile ("b 1f\n1: b 1f\n1: b 1f\n1: b 1f\n1: b 1f\n1: b 1f\n1:\n" : : : "memory"); | |||
There was a problem hiding this comment.
is this 6 nop instruction, could we just write it as folows.
__asm volatile("nop; nop; nop; nop; nop; nop;" : : : "memory")Also It would be nice to add comment why we need this e.g `START_TRANS "seems" to have the same behavior as AVAILABLE bit described in section 4.1.2.5.1 Concurrent access rp2040 datasheet v2.1" etc...
There was a problem hiding this comment.
I don't have an opinion on this, I just followed what's already in _hw_endpoint_buffer_control_update32 in rp2040_usb.c. That was written by @kilograham I believe. I don't know if it makes any difference. If you want NOPs tell me if you want 6 or 12 of them as existing code delays for 12 cycles and a single NOP is 1 cycle (I think). This should only make a difference on massively overclocked chips as 3 cycles should be enough at 133MHz.
There was a problem hiding this comment.
nop is 1 cycle, b 1f is 2 cyles so they aren't the same (and the b 1f is smaller`)
as of SDK1.3.1 there is a busy_wait_at_least_cycles(cycles) method which could be used instead (i.e. busy_wait_at_least_cycles(12)
There was a problem hiding this comment.
thanks @kilograham for explanation, @jfedor2 what do you think, would you mind chaing it to busy_wait_at_least_cycles(12) as suggested.
This commit makes it so that when setting the START_TRANS bit in the SIE_CTRL register, along with some other bits, we first set all the other bits, then wait some cycles, and then set the START_TRANS bit. Doing so protects against a situation where the USB controller is reading the register at the same time and gets an incorrect value. This mirrors the procedure already applied to buffer control registers.
This fixes #1830
Some context: raspberrypi/pico-feedback#324
This commit makes it so that when setting the START_TRANS bit in the SIE_CTRL register, along with some other bits, we first set all the other bits, then wait some cycles, and then set the START_TRANS bit.
Doing so protects against a situation where the USB controller is reading the register at the same time and gets an incorrect value.
The assembler parts I just copied from _hw_endpoint_buffer_control_update32 where a similar procedure is already followed for the buffer control registers.