ot_usbdev: Implementation data transfers
#270
Conversation
|
(nit pick: syntax of the PR title) |
| OtUsbdevServerCmd cmd, uint32_t id, | ||
| const void *data, size_t size) | ||
| static void ot_usbdev_server_write_packet( | ||
| OtUsbdevState *s, OtUsbdevServerCmd cmd, uint32_t id, const uint8_t *data0, |
There was a problem hiding this comment.
Nit: data0/data1 is not very informative, maybe these could have more descriptive names based on the packet header / data split?
There was a problem hiding this comment.
I will add some documentation, it does not have any particular meaning, it's just an optimization to avoid a useless allocation really.
There was a problem hiding this comment.
Yes, I had gathered it was to avoid the extra allocation. I think that because most transfers that use data0 and data1 have some meaningful difference between what is sent as data0 and what is sent as data1, it would be nice to incorporate that into the code semantics or the docs to make the code easier to follow. But it is okay to leave as is if you can't generalize this.
| uint8_t *recv_data; | ||
|
|
||
| /* endpoint transfers in progress */ | ||
| OtUsbdevServerEpXfer ep[USBDEV_PARAM_N_ENDPOINTS]; |
There was a problem hiding this comment.
I might be missing something: are these reset in any way when the USB device is reset, or if not, are they intended to persist?
There was a problem hiding this comment.
Not yet, there is a TODO in ot_usbdev_simulate_link_reset
pamaury
changed the title
ot_usbdev: Implementation data transfers
pamaury
force-pushed
the
usbdev_xfer
branch
3 times, most recently
from
13b0f76 to
b907e81
Compare
pamaury
changed the title
ot_usbdev: Implementation data transfersot_usbdev: Implementation data transfers
hw/opentitan/ot_usbdev.c
Outdated
| &s->buffer[buf_id * OT_USBDEV_MAX_PACKET_SIZE / sizeof(uint32_t)]; | ||
| while (size > 0) { | ||
| uint32_t val32 = *ptr++; | ||
| for (size_t i = 0u; i < sizeof(uint32_t); i++) { |
There was a problem hiding this comment.
Could maybe also stl_le_p here? But its also fine to leave it as is if you prefer.
There was a problem hiding this comment.
done, hopefully I didn't mess up the code
| */ | ||
| if (!ot_usbdev_is_ep_out_rxenabled(s, epnum) || | ||
| fifo8_is_empty(&s->av_out_fifo) || fifo32_num_free(&s->rx_fifo) <= 1) { | ||
| return; |
There was a problem hiding this comment.
I think you still did not add this link error? (maybe I am missing it somehow)
pamaury
force-pushed
the
usbdev_xfer
branch
2 times, most recently
from
7cc3f9c to
d961eff
Compare
There was a problem hiding this comment.
LGTM - I've left a couple of comments, feel free to address or leave as you see fit.
Also: the //sw/device/tests/crypto:sha{256,384,512}_functest in the sim_qemu_rom_with_fake_keys Earlgrey regressions are known broken right now upstream (even on FPGA, due to cryptolib backports from master where these test envs are also apparently broken). You can ignore CI failures on these tests.
| */ | ||
| if (!ot_usbdev_is_ep_out_rxenabled(s, epnum) || | ||
| fifo8_is_empty(&s->av_out_fifo) || fifo32_num_free(&s->rx_fifo) <= 1) { | ||
| return; |
There was a problem hiding this comment.
I am still somewhat confused by this, to be honest. Comparing to the BFM in the out_packet function PidTypeOutToken case if the available out buffer is empty, there is not space in the RX fifo or the endpoint is not RX enabled, then it calls cancel_out() which sets the interrupt for a link out error.
If I follow the interrupt signal through the RTL I end up in hw/ip/usbdev/rtl/usb_fs_nb_out_pe.sv where we set rollback_data (which triggers the interrupt) to 1 on any of:
(a) a bad data toggle (while also not stalled).
(b) Malformed (invalid / non-data) packets received.
(c) Stalled EP (for a non-isochronous OUT transaction).
(d) Transactions that are NAK'd due to the EP being full (for either SETUP, OUT or isochronous transactions, just at different stages). But it looks like the EP is only reported as full if either (i) all endpoints are blocked because the HW cannot take transactions at all (AV is empty, RX is full) or (ii) the endpoint is blocked (RX out disabled, or not yet setup).
So based on that, I think the BFM is correct here? But also maybe there should be a link out error on the stall case?
docs/opentitan/usbdev.md
Outdated
| | Reserved | 2 | 2 | Set to zero | | ||
| | Setup | 4 | 8 | SETUP packet | | ||
|
|
||
| Sending a `Setup` command to an endpoint automatically cancels any |
This commit implements the logic to support data transfers and wire them in the server. The protocol is updated with commands to support this. Isochronous transfers are not properly supported at the moment. Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
3 participants
This PR implements the logic to support data transfers and wire them in the server. The protocol is updated with commands to support this. Isochronous transfers are not properly supported at the moment.
There are still many TODOs, most of them are related to making everything more robust. It's very likely that the driver does not correctly handle all cancellations correctly. The handling of OUT transfers is also not quite complete for transfers requiring more than one packet, this will require some sort of scheduler. However this is enough for enumeration.
Tested using https://github.com/pamaury/opentitan/tree/qemu_ott_usb_backend (not ready for upstream yet). I have made some transfers work but I am still debugging issues.