LailaTheElf/embassy
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

otg: fix corruption in CONTROL OUT transfers in stm32f4.

Browse Source 
The RM says we have to process STUP (and therefore clear CNAK to start the data stage)
in the DOEPINT STUP interrupt. Seems doing it in RXFLVL when we receive the data is
too early. This makes it work consistently on all chips, so the quirk is no longer needed.

Fixes #3493
Fixes #3459
This commit is contained in:
Dario Nieuwenhuis 2024-11-23 00:23:40 +01:00
parent 4f459bb918
commit 032af9d512
2 changed files with 50 additions and 79 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
embassy-stm32/src/usb/otg.rs
View File

@ -27,9 +27,7 @@ impl<T: Instance> interrupt::typelevel::Handler<T::Interrupt> for InterruptHandl
let r = T::regs(); let r = T::regs();
let state = T::state(); let state = T::state();
let setup_late_cnak = quirk_setup_late_cnak(r); on_interrupt_impl(r, state, T::ENDPOINT_COUNT);
on_interrupt_impl(r, state, T::ENDPOINT_COUNT, setup_late_cnak);
} }
} }
@ -86,7 +84,6 @@ impl<'d, T: Instance> Driver<'d, T> {
extra_rx_fifo_words: RX_FIFO_EXTRA_SIZE_WORDS, extra_rx_fifo_words: RX_FIFO_EXTRA_SIZE_WORDS,
endpoint_count: T::ENDPOINT_COUNT, endpoint_count: T::ENDPOINT_COUNT,
phy_type: PhyType::InternalFullSpeed, phy_type: PhyType::InternalFullSpeed,
quirk_setup_late_cnak: quirk_setup_late_cnak(regs),
calculate_trdt_fn: calculate_trdt::<T>, calculate_trdt_fn: calculate_trdt::<T>,
}; };
@ -116,16 +113,13 @@ impl<'d, T: Instance> Driver<'d, T> {
dp.set_as_af(dp.af_num(), AfType::output(OutputType::PushPull, Speed::VeryHigh)); dp.set_as_af(dp.af_num(), AfType::output(OutputType::PushPull, Speed::VeryHigh));
dm.set_as_af(dm.af_num(), AfType::output(OutputType::PushPull, Speed::VeryHigh)); dm.set_as_af(dm.af_num(), AfType::output(OutputType::PushPull, Speed::VeryHigh));
let regs = T::regs();
let instance = OtgInstance { let instance = OtgInstance {
regs, regs: T::regs(),
state: T::state(), state: T::state(),
fifo_depth_words: T::FIFO_DEPTH_WORDS, fifo_depth_words: T::FIFO_DEPTH_WORDS,
extra_rx_fifo_words: RX_FIFO_EXTRA_SIZE_WORDS, extra_rx_fifo_words: RX_FIFO_EXTRA_SIZE_WORDS,
endpoint_count: T::ENDPOINT_COUNT, endpoint_count: T::ENDPOINT_COUNT,
phy_type: PhyType::InternalHighSpeed, phy_type: PhyType::InternalHighSpeed,
quirk_setup_late_cnak: quirk_setup_late_cnak(regs),
calculate_trdt_fn: calculate_trdt::<T>, calculate_trdt_fn: calculate_trdt::<T>,
}; };
@ -165,8 +159,6 @@ impl<'d, T: Instance> Driver<'d, T> {
ulpi_d7 ulpi_d7
); );
let regs = T::regs();
let instance = OtgInstance { let instance = OtgInstance {
regs: T::regs(), regs: T::regs(),
state: T::state(), state: T::state(),
@ -174,7 +166,6 @@ impl<'d, T: Instance> Driver<'d, T> {
extra_rx_fifo_words: RX_FIFO_EXTRA_SIZE_WORDS, extra_rx_fifo_words: RX_FIFO_EXTRA_SIZE_WORDS,
endpoint_count: T::ENDPOINT_COUNT, endpoint_count: T::ENDPOINT_COUNT,
phy_type: PhyType::ExternalFullSpeed, phy_type: PhyType::ExternalFullSpeed,
quirk_setup_late_cnak: quirk_setup_late_cnak(regs),
calculate_trdt_fn: calculate_trdt::<T>, calculate_trdt_fn: calculate_trdt::<T>,
}; };
@ -216,8 +207,6 @@ impl<'d, T: Instance> Driver<'d, T> {
ulpi_d7 ulpi_d7
); );
let regs = T::regs();
let instance = OtgInstance { let instance = OtgInstance {
regs: T::regs(), regs: T::regs(),
state: T::state(), state: T::state(),
@ -225,7 +214,6 @@ impl<'d, T: Instance> Driver<'d, T> {
extra_rx_fifo_words: RX_FIFO_EXTRA_SIZE_WORDS, extra_rx_fifo_words: RX_FIFO_EXTRA_SIZE_WORDS,
endpoint_count: T::ENDPOINT_COUNT, endpoint_count: T::ENDPOINT_COUNT,
phy_type: PhyType::ExternalHighSpeed, phy_type: PhyType::ExternalHighSpeed,
quirk_setup_late_cnak: quirk_setup_late_cnak(regs),
calculate_trdt_fn: calculate_trdt::<T>, calculate_trdt_fn: calculate_trdt::<T>,
}; };
@ -578,7 +566,3 @@ fn calculate_trdt<T: Instance>(speed: Dspd) -> u8 {
_ => unimplemented!(), _ => unimplemented!(),
} }
} }
fn quirk_setup_late_cnak(r: Otg) -> bool {
r.cid().read().0 & 0xf000 == 0x1000
}

109
embassy-usb-synopsys-otg/src/lib.rs
View File

@ -9,7 +9,7 @@ mod fmt;
use core::cell::UnsafeCell; use core::cell::UnsafeCell;
use core::future::poll_fn; use core::future::poll_fn;
use core::marker::PhantomData; use core::marker::PhantomData;
use core::sync::atomic::{AtomicBool, AtomicU16, Ordering}; use core::sync::atomic::{AtomicBool, AtomicU16, AtomicU32, Ordering};
use core::task::Poll; use core::task::Poll;
use embassy_sync::waitqueue::AtomicWaker; use embassy_sync::waitqueue::AtomicWaker;
@ -25,18 +25,17 @@ pub mod otg_v1;
use otg_v1::{regs, vals, Otg}; use otg_v1::{regs, vals, Otg};
/// Handle interrupts. /// Handle interrupts.
pub unsafe fn on_interrupt<const MAX_EP_COUNT: usize>( pub unsafe fn on_interrupt<const MAX_EP_COUNT: usize>(r: Otg, state: &State<MAX_EP_COUNT>, ep_count: usize) {
r: Otg,
state: &State<MAX_EP_COUNT>,
ep_count: usize,
quirk_setup_late_cnak: bool,
) {
trace!("irq"); trace!("irq");
let ints = r.gintsts().read(); let ints = r.gintsts().read();
if ints.wkupint() || ints.usbsusp() || ints.usbrst() || ints.enumdne() || ints.otgint() || ints.srqint() { if ints.wkupint() || ints.usbsusp() || ints.usbrst() || ints.enumdne() || ints.otgint() || ints.srqint() {
// Mask interrupts and notify `Bus` to process them // Mask interrupts and notify `Bus` to process them
r.gintmsk().write(|_| {}); r.gintmsk().write(|w| {
w.set_iepint(true);
w.set_oepint(true);
w.set_rxflvlm(true);
});
state.bus_waker.wake(); state.bus_waker.wake();
} }
@ -64,19 +63,9 @@ pub unsafe fn on_interrupt<const MAX_EP_COUNT: usize>(
while r.grstctl().read().txfflsh() {} while r.grstctl().read().txfflsh() {}
} }
if state.cp_state.setup_ready.load(Ordering::Relaxed) == false { let data = &state.cp_state.setup_data;
// SAFETY: exclusive access ensured by atomic bool data[0].store(r.fifo(0).read().data(), Ordering::Relaxed);
let data = unsafe { &mut *state.cp_state.setup_data.get() }; data[1].store(r.fifo(0).read().data(), Ordering::Relaxed);
data[0..4].copy_from_slice(&r.fifo(0).read().0.to_ne_bytes());
data[4..8].copy_from_slice(&r.fifo(0).read().0.to_ne_bytes());
state.cp_state.setup_ready.store(true, Ordering::Release);
state.ep_states[0].out_waker.wake();
} else {
error!("received SETUP before previous finished processing");
// discard FIFO
r.fifo(0).read();
r.fifo(0).read();
}
} }
vals::Pktstsd::OUT_DATA_RX => { vals::Pktstsd::OUT_DATA_RX => {
trace!("OUT_DATA_RX ep={} len={}", ep_num, len); trace!("OUT_DATA_RX ep={} len={}", ep_num, len);
@ -110,11 +99,6 @@ pub unsafe fn on_interrupt<const MAX_EP_COUNT: usize>(
} }
vals::Pktstsd::SETUP_DATA_DONE => { vals::Pktstsd::SETUP_DATA_DONE => {
trace!("SETUP_DATA_DONE ep={}", ep_num); trace!("SETUP_DATA_DONE ep={}", ep_num);
if quirk_setup_late_cnak {
// Clear NAK to indicate we are ready to receive more data
r.doepctl(ep_num).modify(|w| w.set_cnak(true));
}
} }
x => trace!("unknown PKTSTS: {}", x.to_bits()), x => trace!("unknown PKTSTS: {}", x.to_bits()),
} }
@ -151,25 +135,30 @@ pub unsafe fn on_interrupt<const MAX_EP_COUNT: usize>(
} }
} }
// not needed? reception handled in rxflvl // out endpoint interrupt
// OUT endpoint interrupt if ints.oepint() {
// if ints.oepint() { trace!("oepint");
// let mut ep_mask = r.daint().read().oepint(); let mut ep_mask = r.daint().read().oepint();
// let mut ep_num = 0; let mut ep_num = 0;
// while ep_mask != 0 { // Iterate over endpoints while there are non-zero bits in the mask
// if ep_mask & 1 != 0 { while ep_mask != 0 {
// let ep_ints = r.doepint(ep_num).read(); if ep_mask & 1 != 0 {
// // clear all let ep_ints = r.doepint(ep_num).read();
// r.doepint(ep_num).write_value(ep_ints); // clear all
// state.ep_out_wakers[ep_num].wake(); r.doepint(ep_num).write_value(ep_ints);
// trace!("out ep={} irq val={:08x}", ep_num, ep_ints.0);
// }
// ep_mask >>= 1; if ep_ints.stup() {
// ep_num += 1; state.cp_state.setup_ready.store(true, Ordering::Release);
// } }
// } state.ep_states[ep_num].out_waker.wake();
trace!("out ep={} irq val={:08x}", ep_num, ep_ints.0);
}
ep_mask >>= 1;
ep_num += 1;
}
}
} }
/// USB PHY type /// USB PHY type
@ -236,7 +225,7 @@ unsafe impl Sync for EpState {}
struct ControlPipeSetupState { struct ControlPipeSetupState {
/// Holds received SETUP packets. Available if [Ep0State::setup_ready] is true. /// Holds received SETUP packets. Available if [Ep0State::setup_ready] is true.
setup_data: UnsafeCell<[u8; 8]>, setup_data: [AtomicU32; 2],
setup_ready: AtomicBool, setup_ready: AtomicBool,
} }
@ -265,7 +254,7 @@ impl<const EP_COUNT: usize> State<EP_COUNT> {
Self { Self {
cp_state: ControlPipeSetupState { cp_state: ControlPipeSetupState {
setup_data: UnsafeCell::new([0u8; 8]), setup_data: [const { AtomicU32::new(0) }; 2],
setup_ready: AtomicBool::new(false), setup_ready: AtomicBool::new(false),
}, },
ep_states: [NEW_EP_STATE; EP_COUNT], ep_states: [NEW_EP_STATE; EP_COUNT],
@ -480,7 +469,6 @@ impl<'d, const MAX_EP_COUNT: usize> embassy_usb_driver::Driver<'d> for Driver<'d
trace!("start"); trace!("start");
let regs = self.instance.regs; let regs = self.instance.regs;
let quirk_setup_late_cnak = self.instance.quirk_setup_late_cnak;
let cp_setup_state = &self.instance.state.cp_state; let cp_setup_state = &self.instance.state.cp_state;
( (
Bus { Bus {
@ -496,7 +484,6 @@ impl<'d, const MAX_EP_COUNT: usize> embassy_usb_driver::Driver<'d> for Driver<'d
ep_out, ep_out,
ep_in, ep_in,
regs, regs,
quirk_setup_late_cnak,
}, },
) )
} }
@ -632,6 +619,11 @@ impl<'d, const MAX_EP_COUNT: usize> Bus<'d, MAX_EP_COUNT> {
w.set_xfrcm(true); w.set_xfrcm(true);
}); });
// Unmask SETUP received EP interrupt
r.doepmsk().write(|w| {
w.set_stupm(true);
});
// Unmask and clear core interrupts // Unmask and clear core interrupts
self.restore_irqs(); self.restore_irqs();
r.gintsts().write_value(regs::Gintsts(0xFFFF_FFFF)); r.gintsts().write_value(regs::Gintsts(0xFFFF_FFFF));
@ -743,7 +735,7 @@ impl<'d, const MAX_EP_COUNT: usize> Bus<'d, MAX_EP_COUNT> {
regs.doeptsiz(index).modify(|w| { regs.doeptsiz(index).modify(|w| {
w.set_xfrsiz(ep.max_packet_size as _); w.set_xfrsiz(ep.max_packet_size as _);
if index == 0 { if index == 0 {
w.set_rxdpid_stupcnt(1); w.set_rxdpid_stupcnt(3);
} else { } else {
w.set_pktcnt(1); w.set_pktcnt(1);
} }
@ -755,8 +747,7 @@ impl<'d, const MAX_EP_COUNT: usize> Bus<'d, MAX_EP_COUNT> {
// Enable IRQs for allocated endpoints // Enable IRQs for allocated endpoints
regs.daintmsk().modify(|w| { regs.daintmsk().modify(|w| {
w.set_iepm(ep_irq_mask(&self.ep_in)); w.set_iepm(ep_irq_mask(&self.ep_in));
// OUT interrupts not used, handled in RXFLVL w.set_oepm(ep_irq_mask(&self.ep_out));
// w.set_oepm(ep_irq_mask(&self.ep_out));
}); });
} }
@ -1242,7 +1233,6 @@ pub struct ControlPipe<'d> {
setup_state: &'d ControlPipeSetupState, setup_state: &'d ControlPipeSetupState,
ep_in: Endpoint<'d, In>, ep_in: Endpoint<'d, In>,
ep_out: Endpoint<'d, Out>, ep_out: Endpoint<'d, Out>,
quirk_setup_late_cnak: bool,
} }
impl<'d> embassy_usb_driver::ControlPipe for ControlPipe<'d> { impl<'d> embassy_usb_driver::ControlPipe for ControlPipe<'d> {
@ -1255,21 +1245,20 @@ impl<'d> embassy_usb_driver::ControlPipe for ControlPipe<'d> {
self.ep_out.state.out_waker.register(cx.waker()); self.ep_out.state.out_waker.register(cx.waker());
if self.setup_state.setup_ready.load(Ordering::Relaxed) { if self.setup_state.setup_ready.load(Ordering::Relaxed) {
let data = unsafe { *self.setup_state.setup_data.get() }; let mut data = [0; 8];
data[0..4].copy_from_slice(&self.setup_state.setup_data[0].load(Ordering::Relaxed).to_ne_bytes());
data[4..8].copy_from_slice(&self.setup_state.setup_data[1].load(Ordering::Relaxed).to_ne_bytes());
self.setup_state.setup_ready.store(false, Ordering::Release); self.setup_state.setup_ready.store(false, Ordering::Release);
// EP0 should not be controlled by `Bus` so this RMW does not need a critical section // EP0 should not be controlled by `Bus` so this RMW does not need a critical section
// Receive 1 SETUP packet
self.regs.doeptsiz(self.ep_out.info.addr.index()).modify(|w| { self.regs.doeptsiz(self.ep_out.info.addr.index()).modify(|w| {
w.set_rxdpid_stupcnt(1); w.set_rxdpid_stupcnt(3);
}); });
// Clear NAK to indicate we are ready to receive more data // Clear NAK to indicate we are ready to receive more data
if !self.quirk_setup_late_cnak { self.regs
self.regs .doepctl(self.ep_out.info.addr.index())
.doepctl(self.ep_out.info.addr.index()) .modify(|w| w.set_cnak(true));
.modify(|w| w.set_cnak(true));
}
trace!("SETUP received: {:?}", Bytes(&data)); trace!("SETUP received: {:?}", Bytes(&data));
Poll::Ready(data) Poll::Ready(data)
@ -1389,8 +1378,6 @@ pub struct OtgInstance<'d, const MAX_EP_COUNT: usize> {
pub phy_type: PhyType, pub phy_type: PhyType,
/// Extra RX FIFO words needed by some implementations. /// Extra RX FIFO words needed by some implementations.
pub extra_rx_fifo_words: u16, pub extra_rx_fifo_words: u16,
/// Whether to set up late cnak
pub quirk_setup_late_cnak: bool,
/// Function to calculate TRDT value based on some internal clock speed. /// Function to calculate TRDT value based on some internal clock speed.
pub calculate_trdt_fn: fn(speed: vals::Dspd) -> u8, pub calculate_trdt_fn: fn(speed: vals::Dspd) -> u8,
} }