Hoernchen has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-ccid-firmware/+/42782?usp=email ) Change subject: usb: properly handle data toggle bit ...................................................................... usb: properly handle data toggle bit Same config SetConfig is currently not properly handled and stale toggle bits lead to weird transfers until USB resyncs because the asf4 code itself does not properly handle the toggle bit. - USB 2.0 §9.4.5 states that ClearFeature(ENDPOINT_HALT) unconditionally resets the data toggle to DATA0 - USB 2.0 §9.1.1.5 demands toggle reset even when the same config/interface is selected The old code was only doing a proper reset upon receiving a bus reset, but the actual state reset path outlined in the spec is the set config command. The easy fix here is to split the existing reset code which already does everything we need into two parts for the actual usb reset and SetConfig. The current halt clear code only resets DTGL if the endpoint is actually halted and a fix is a bit more complicated. _usb_d_dev_ep_stall_clr() is also called from the SETUP-packet cleanup path in hal_usb_device.c _usb_d_dev_ep_stall(ep, USB_EP_STALL_CLR); _usb_d_dev_ep_stall(ep | USB_EP_DIR, USB_EP_STALL_CLR); This runs on every SETUP packet as defensive cleanup of any leftover stall from a previous failed transfer. That might be why the asf4 usb code is wrong, because a toggle reset here breaks SETUP. So the unconditional DTGL reset for halt clearing must therefore happen in _usb_d_ep_halt_clr itself, which is only reached via the ClearFeature host request (usbdc_clear_ftr_req -> usb_d_ep_halt(HALT_CLR)), using a small HPL helper function. Closes:OS#7016 Change-Id: I5dbca9bd2212407108b44815ecf566fc9c22336d --- M sysmoOCTSIM/hal/include/hpl_usb_device.h M sysmoOCTSIM/hal/src/hal_usb_device.c M sysmoOCTSIM/hpl/usb/hpl_usb.c M sysmoOCTSIM/usb/device/usbdc.c 4 files changed, 77 insertions(+), 2 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-ccid-firmware refs/changes/82/42782/1 diff --git a/sysmoOCTSIM/hal/include/hpl_usb_device.h b/sysmoOCTSIM/hal/include/hpl_usb_device.h index 43dd17f..b174abb 100644 --- a/sysmoOCTSIM/hal/include/hpl_usb_device.h +++ b/sysmoOCTSIM/hal/include/hpl_usb_device.h @@ -324,6 +324,17 @@ int32_t _usb_d_dev_ep_stall(const uint8_t ep, const enum usb_ep_stall_ctrl ctrl); /** + * \brief Set USB device endpoint data toggle (DTGL) to DATA0 or DATA1 + * \param[in] ep Endpoint address (with direction bit). + * \param[in] tgl 0 = DATA0, non-zero = DATA1. + * \return 0 on success, negative on error. + * + * USB 2.0 §9.4.5: ClearFeature(ENDPOINT_HALT) MUST unconditionally + * reinitialize the data toggle to DATA0. + */ +int32_t _usb_d_dev_ep_set_toggle(const uint8_t ep, uint8_t tgl); + +/** * \brief Read setup request data from specific endpoint * \param[in] ep Endpoint address. * \param[out] req_buf Pointer to buffer to locate the setup packet. diff --git a/sysmoOCTSIM/hal/src/hal_usb_device.c b/sysmoOCTSIM/hal/src/hal_usb_device.c index aa80ede..a9a22f0 100644 --- a/sysmoOCTSIM/hal/src/hal_usb_device.c +++ b/sysmoOCTSIM/hal/src/hal_usb_device.c @@ -536,6 +536,12 @@ if (ep_index < 0) { return -USB_ERR_PARAM; } + /* USB 2.0 §9.4.5: ClearFeature(ENDPOINT_HALT) MUST reset data toggle. + * This is the only caller that comes from host ClearFeature; + * SETUP-cleanup path in usb_d_cb_trans_setup() which also + * calls _usb_d_dev_ep_stall(STALL_CLR) must NOT touch DTGL */ + _usb_d_dev_ep_set_toggle(ep, 0); + if (_usb_d_dev_ep_stall(ep, USB_EP_STALL_GET)) { rc = _usb_d_dev_ep_stall(ep, USB_EP_STALL_CLR); if (rc < 0) { diff --git a/sysmoOCTSIM/hpl/usb/hpl_usb.c b/sysmoOCTSIM/hpl/usb/hpl_usb.c index ed9ef62..d743bf6 100644 --- a/sysmoOCTSIM/hpl/usb/hpl_usb.c +++ b/sysmoOCTSIM/hpl/usb/hpl_usb.c @@ -1734,6 +1734,16 @@ /* By default, IN endpoint will NAK all token. */ _usbd_ep_set_in_rdy(epn, 1, false); _usbd_ep_clear_bank_status(epn, 1); + /* USB 2.0 §9.4.5: Halt feature reset after SetConfiguration() or SetInterface() + * Bus reset already clears STALLRQ in HW but same-config SetConfiguration + * does not trigger a bus reset, so clear in software. */ + _usbd_ep_set_stall(epn, 1, false); + /* USB 2.0 §9.1.1.5 / §5.8.5: data toggle must start at DATA0 + * when the endpoint is configured by SetConfiguration / + * SetInterface. SAM D5x/E5x DS60001507 §38.6.2.4 and §38.6.2.2 list + * what the hardware clears on bus reset and on endpoint + * disable, EPSTATUS.DTGLIN/OUT are NOT in either list, so we must do it. */ + _usbd_ep_set_toggle(epn, 1, 0); } else { /* prevents init->enable->disable->enable again from working without reinit...*/ @@ -1749,6 +1759,10 @@ /* By default, OUT endpoint will NAK all token. */ _usbd_ep_set_out_rdy(epn, 0, false); _usbd_ep_clear_bank_status(epn, 0); + /* USB 2.0 §9.4.5: Halt feature reset by every SetConfig / SetInterface */ + _usbd_ep_set_stall(epn, 0, false); + /* USB 2.0 §9.1.1.5 / §5.8.5: data toggle must start at DATA0 when the endpoint is configured. */ + _usbd_ep_set_toggle(epn, 0, 0); } return USB_OK; @@ -1807,6 +1821,7 @@ { uint8_t epn = USB_EP_GET_N(ept->ep); bool is_stalled = _usbd_ep_is_stalled(epn, dir); + if (!is_stalled) { return ERR_NONE; } @@ -1814,6 +1829,13 @@ _usbd_ep_int_dis(epn, USB_DEVICE_EPINTFLAG_STALL0 << dir); if (_usbd_ep_is_stall_sent(epn, dir)) { _usbd_ep_ack_stall(epn, dir); + /* USB 2.0 §9.4.5: ClearFeature(ENDPOINT_HALT) "always results + * in the data toggle being reinitialized to DATA0". Only on + * actual stall clear -- this function is also called from the + * routine SETUP-packet cleanup in hal_usb_device.c, where + * clearing the toggle would break in-flight control transfer + * sequencing. Session-state DTGL reset is handled separately + * in _usb_d_dev_ep_enable. */ _usbd_ep_set_toggle(epn, dir, 0); } if (_usb_d_dev_ep_is_ctrl(ept)) { @@ -1847,6 +1869,18 @@ return rc; } +int32_t _usb_d_dev_ep_set_toggle(const uint8_t ep, uint8_t tgl) +{ + uint8_t epn = USB_EP_GET_N(ep); + bool dir = USB_EP_GET_DIR(ep); + + if (epn > CONF_USB_D_MAX_EP_N) { + return -USB_ERR_PARAM; + } + _usbd_ep_set_toggle(epn, dir, tgl); + return ERR_NONE; +} + /** * \brief Finish the transaction and invoke callback * \param[in, out] ept Pointer to endpoint information. diff --git a/sysmoOCTSIM/usb/device/usbdc.c b/sysmoOCTSIM/usb/device/usbdc.c index 38aa0e0..a02cf92 100644 --- a/sysmoOCTSIM/usb/device/usbdc.c +++ b/sysmoOCTSIM/usb/device/usbdc.c @@ -448,13 +448,32 @@ } /** + * \brief soft session-state reset. + * + * Called by usbdc_unconfig and usbdc_set_config. Stops + * card-side SERCOM USARTs and freezes SysTick driven timeouts. + * The was_unconfigured_flag tells the main loop to run the other half of the cleanup/reset. + * + * This is only the "session state" half of a bus reset. + * Bus reset additionally tears down EP existence (EPCFG.EPTYPE) via + * usbdc_unconfig's USBDF_DISABLE loop, a SetConfig does not need that (and usually happens after the reset). + */ +static void usbdc_softreset(void) +{ + reset_all_stuff_irq(); + was_unconfigured_flag = true; +} + +/** * \brief Unconfig, close all interfaces */ static void usbdc_unconfig(void) { - reset_all_stuff_irq(); - was_unconfigured_flag = true; + usbdc_softreset(); + /* In addition to the soft reset, tear down per-function state: USBDF_DISABLE calls the class driver + * (e.g. ccid_df_disable -> usb_d_ep_deinit per EP), which clears EPCFG.EPTYPE etc. + * Right thing to do for for USB bus reset and for SetConfig(0), but NOT for a same-config SetConfig. */ struct usbdf_driver *func = (struct usbdf_driver *)usbdc.func_list.head; while (NULL != func) { func->ctrl(func, USBDF_DISABLE, NULL); @@ -481,6 +500,11 @@ return true; } + /* USB 2.0 §9.1.1.5 / §9.4.5 a non-zero SetConfiguration unconditionally resets endpoint state. */ + if (usbdc.cfg_value != 0) { + usbdc_softreset(); + } + #if CONF_USBD_HS_SP if (usb_d_get_speed() == USB_SPEED_HS && usbdc.desces.hs) { cfg_desc = usb_find_cfg_desc(usbdc.desces.hs->sod, usbdc.desces.hs->eod, cfg_value); -- To view, visit https://gerrit.osmocom.org/c/osmo-ccid-firmware/+/42782?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: osmo-ccid-firmware Gerrit-Branch: master Gerrit-Change-Id: I5dbca9bd2212407108b44815ecf566fc9c22336d Gerrit-Change-Number: 42782 Gerrit-PatchSet: 1 Gerrit-Owner: Hoernchen <ewild@sysmocom.de>