From e893102cda6eb67f8641ea08f88da295b1aca8f6 Mon Sep 17 00:00:00 2001 From: Ian Date: Tue, 29 Mar 2022 11:47:19 +1300 Subject: [PATCH] Rework USB OUT endpoint interrupts (#459) * Rework USB OUT endpoint interrupts Previously, when the USB interrupt was unmasked and a host issued an OUT transfer to an endpoint that was not `read()` before the next OUT transfer attempt to that endpoint, the USB ISR would continually fire until `read()` was called on that endpoint. If a higher priority ISR didn't somehow clear the condition that prevented the `read()`, the CPU would lock up. This change moves the clearing of the endpoint's transfer complete and transfer failed interrupt flags in to `poll(), so that a USB Class can safely skip a `read()` without having to chose between discarding data, or locking up in the USB ISR. * Optimisation in USB poll() --- hal/src/thumbv6m/usb/bus.rs | 72 +++++++++++++++++++++--------------- hal/src/thumbv7em/usb/bus.rs | 72 ++++++++++++++++++++---------------- 2 files changed, 83 insertions(+), 61 deletions(-) diff --git a/hal/src/thumbv6m/usb/bus.rs b/hal/src/thumbv6m/usb/bus.rs index 584870193161..f4c31d774375 100644 --- a/hal/src/thumbv6m/usb/bus.rs +++ b/hal/src/thumbv6m/usb/bus.rs @@ -394,13 +394,6 @@ impl<'a> Bank<'a, OutBank> { .write(|w| w.trcpt0().set_bit().trfail0().set_bit()); } - /// Checks if data has been received. Returns true for failed transfers - /// as well as successful transfers. - #[inline] - fn is_transfer_complete(&self) -> bool { - self.epintflag(self.index()).read().trcpt0().bit() - } - /// Returns true if a Received Setup interrupt has occurred. /// This indicates that the read buffer holds a SETUP packet. #[inline] @@ -750,6 +743,7 @@ impl Inner { } fn suspend(&self) {} + fn resume(&self) {} fn alloc_ep( @@ -822,49 +816,69 @@ impl Inner { // unconnected & unsuspended, we do not handle them to avoid spurious // transitions. - let intbits = self.usb().epintsmry.read().bits(); - if intbits == 0 { - return PollResult::None; - } - let mut ep_out = 0; let mut ep_in_complete = 0; let mut ep_setup = 0; + let intbits = self.usb().epintsmry.read().bits(); + for ep in 0..8u16 { let mask = 1 << ep; - if (intbits & mask) == 0 { - continue; - } let idx = ep as usize; - if let Ok(bank1) = self.bank1(EndpointAddress::from_parts(idx, UsbDirection::In)) { - if bank1.is_transfer_complete() { - bank1.clear_transfer_complete(); - ep_in_complete |= mask; + if (intbits & mask) != 0 { + if let Ok(bank1) = self.bank1(EndpointAddress::from_parts(idx, UsbDirection::In)) { + if bank1.is_transfer_complete() { + bank1.clear_transfer_complete(); + ep_in_complete |= mask; + } } } + // Can't test intbits, because bk0rdy doesn't interrupt if let Ok(bank0) = self.bank0(EndpointAddress::from_parts(idx, UsbDirection::Out)) { if bank0.received_setup_interrupt() { ep_setup |= mask; - // usb-device crate: - // "This event should continue to be reported until the - // packet is read." So we don't clear the flag here, - // instead it is cleared in the read handler. + + // The RXSTP interrupt is not cleared here, because doing so + // would allow the USB hardware to overwrite the received + // data, potentially before it is `read()` - see SAMD21 + // datasheet "32.6.2.6 Management of SETUP Transactions". + // Setup events are only relevant for control endpoints, and + // in typical USB devices, endpoint 0 is the only control + // endpoint. The usb-device `poll()` method, which calls + // this `poll()`, will immediately `read()` endpoint 0 when + // its setup bit is set. } - if bank0.is_transfer_complete() { + // Clear the transfer complete and transfer failed interrupt flags + // so that execution leaves the USB interrupt until the host makes + // another transaction. The transfer failed flag may have been set + // if an OUT transaction wasn't read() from the endpoint by the + // Class; the hardware will have NAKed (unless the endpoint is + // isochronous) and the host may retry. + bank0.clear_transfer_complete(); + + // Use the bk0rdy flag via is_ready() to indicate that data has been + // received successfully, rather than the interrupting trcpt0 via + // is_transfer_ready(), because data may have been received on an + // earlier poll() which cleared trcpt0. bk0rdy is cleared in the + // endpoint read(). + if bank0.is_ready() { ep_out |= mask; } } } - PollResult::Data { - ep_out, - ep_in_complete, - ep_setup, + if ep_out == 0 && ep_in_complete == 0 && ep_setup == 0 { + PollResult::None + } else { + PollResult::Data { + ep_out, + ep_in_complete, + ep_setup, + } } } @@ -898,8 +912,6 @@ impl Inner { bank.clear_transfer_complete(); bank.set_ready(false); - drop(bank); - size } else { Err(UsbError::WouldBlock) diff --git a/hal/src/thumbv7em/usb/bus.rs b/hal/src/thumbv7em/usb/bus.rs index 5f256666426c..e8e544001bdd 100644 --- a/hal/src/thumbv7em/usb/bus.rs +++ b/hal/src/thumbv7em/usb/bus.rs @@ -393,13 +393,6 @@ impl<'a> Bank<'a, OutBank> { .write(|w| w.trcpt0().set_bit().trfail0().set_bit()); } - /// Checks if data has been received. Returns true for failed transfers - /// as well as successful transfers. - #[inline] - fn is_transfer_complete(&self) -> bool { - self.epintflag(self.index()).read().trcpt0().bit() - } - /// Returns true if a Received Setup interrupt has occurred. /// This indicates that the read buffer holds a SETUP packet. #[inline] @@ -555,7 +548,6 @@ impl Inner { fn set_stall>(&self, ep: EP, stall: bool) { let ep = ep.into(); - if ep.is_out() { if let Ok(mut bank) = self.bank0(ep) { bank.set_stall(stall); @@ -767,49 +759,69 @@ impl Inner { // unconnected & unsuspended, we do not handle them to avoid spurious // transitions. - let intbits = self.usb().epintsmry.read().bits(); - if intbits == 0 { - return PollResult::None; - } - let mut ep_out = 0; let mut ep_in_complete = 0; let mut ep_setup = 0; + let intbits = self.usb().epintsmry.read().bits(); + for ep in 0..8u16 { let mask = 1 << ep; - if (intbits & mask) == 0 { - continue; - } let idx = ep as usize; - if let Ok(bank1) = self.bank1(EndpointAddress::from_parts(idx, UsbDirection::In)) { - if bank1.is_transfer_complete() { - bank1.clear_transfer_complete(); - ep_in_complete |= mask; + if (intbits & mask) != 0 { + if let Ok(bank1) = self.bank1(EndpointAddress::from_parts(idx, UsbDirection::In)) { + if bank1.is_transfer_complete() { + bank1.clear_transfer_complete(); + ep_in_complete |= mask; + } } } + // Can't test intbits, because bk0rdy doesn't interrupt if let Ok(bank0) = self.bank0(EndpointAddress::from_parts(idx, UsbDirection::Out)) { if bank0.received_setup_interrupt() { ep_setup |= mask; - // usb-device crate: - // "This event should continue to be reported until the - // packet is read." So we don't clear the flag here, - // instead it is cleared in the read handler. + + // The RXSTP interrupt is not cleared here, because doing so + // would allow the USB hardware to overwrite the received + // data, potentially before it is `read()` - see SAMD5x + // datasheet "38.6.2.6 Management of SETUP Transactions". + // Setup events are only relevant for control endpoints, and + // in typical USB devices, endpoint 0 is the only control + // endpoint. The usb-device `poll()` method, which calls + // this `poll()`, will immediately `read()` endpoint 0 when + // its setup bit is set. } - if bank0.is_transfer_complete() { + // Clear the transfer complete and transfer failed interrupt flags + // so that execution leaves the USB interrupt until the host makes + // another transaction. The transfer failed flag may have been set + // if an OUT transaction wasn't read() from the endpoint by the + // Class; the hardware will have NAKed (unless the endpoint is + // isochronous) and the host may retry. + bank0.clear_transfer_complete(); + + // Use the bk0rdy flag via is_ready() to indicate that data has been + // received successfully, rather than the interrupting trcpt0 via + // is_transfer_ready(), because data may have been received on an + // earlier poll() which cleared trcpt0. bk0rdy is cleared in the + // endpoint read(). + if bank0.is_ready() { ep_out |= mask; } } } - PollResult::Data { - ep_out, - ep_in_complete, - ep_setup, + if ep_out == 0 && ep_in_complete == 0 && ep_setup == 0 { + PollResult::None + } else { + PollResult::Data { + ep_out, + ep_in_complete, + ep_setup, + } } } @@ -843,8 +855,6 @@ impl Inner { bank.clear_transfer_complete(); bank.set_ready(false); - drop(bank); - size } else { Err(UsbError::WouldBlock)