Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply the UARTE workaround for nrf91 and nrf53 #319

Merged
merged 8 commits into from
May 14, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 90 additions & 19 deletions nrf-hal-common/src/uarte.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,17 @@ where
T: Instance,
{
pub fn new(uarte: T, mut pins: Pins, parity: Parity, baudrate: Baudrate) -> Self {
// Is the UART already on? It might be if you had a bootloader
if uarte.enable.read().bits() != 0 {
uarte.tasks_stoptx.write(|w| unsafe { w.bits(1) });
while uarte.events_txstopped.read().bits() == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

events_txstopped only fires if whoever was using it before did a stoptx. This might spin forever if that's not the case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly why the errata workaround checks for rxenable/txenable, but we can't rely on those being present in nrf52 since they're undocuemnted...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. With no check, I splatter the last line emitted by SPM. What's a good thing to check for here? Or do we not care that we configure the UART whilst it may still be enabled?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there might be side effects if it's not disabled yeah :S

Maybe always do a stoptx? I think if tx is stopped stoptx still causes a new txstopped, so the loop is guaranteed to hang? I'm not 100% sure though

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will do.

// Spin
}

// Disable UARTE instance
uarte.enable.write(|w| w.enable().disabled());
}

// Select pins
uarte.psel.rxd.write(|w| {
unsafe { w.bits(pins.rxd.psel_bits()) };
Expand Down Expand Up @@ -73,9 +84,6 @@ where
}
});

// Enable UARTE instance.
uarte.enable.write(|w| w.enable().enabled());

// Configure.
let hardware_flow_control = pins.rts.is_some() && pins.cts.is_some();
uarte
Expand All @@ -84,8 +92,72 @@ where

// Configure frequency.
uarte.baudrate.write(|w| w.baudrate().variant(baudrate));

let mut u = Uarte(uarte);

u.apply_workaround_for_enable_anomaly();

// Enable UARTE instance.
u.0.enable.write(|w| w.enable().enabled());

u
}

#[cfg(not(any(feature = "9160", feature = "5340")))]
fn apply_workaround_for_enable_anomaly(&mut self)
{
// Do nothing
}

#[cfg(any(feature = "9160", feature = "5340"))]
fn apply_workaround_for_enable_anomaly(&mut self)
{
// Apply workaround for anomalies:
// - nRF9160 - anomaly 23
// - nRF5340 - anomaly 44
let rxenable_reg: *const u32 = ((self.0.deref() as *const _ as usize) + 0x564) as *const u32;
let txenable_reg: *const u32 = ((self.0.deref() as *const _ as usize) + 0x568) as *const u32;

// NB Safety: This is taken from Nordic's driver -
// https://github.com/NordicSemiconductor/nrfx/blob/master/drivers/src/nrfx_uarte.c#L197
if unsafe { core::ptr::read_volatile(txenable_reg) } == 1 {
self.0.tasks_stoptx.write(|w| unsafe { w.bits(1) });
}

// NB Safety: This is taken from Nordic's driver -
// https://github.com/NordicSemiconductor/nrfx/blob/master/drivers/src/nrfx_uarte.c#L197
if unsafe { core::ptr::read_volatile(rxenable_reg) } == 1 {
self.0.enable.write(|w| w.enable().enabled());
self.0.tasks_stoprx.write(|w| unsafe { w.bits(1) });


let mut workaround_succeded = false;
// The UARTE is able to receive up to four bytes after the STOPRX task has been triggered.
// On lowest supported baud rate (1200 baud), with parity bit and two stop bits configured
// (resulting in 12 bits per data byte sent), this may take up to 40 ms.
for _ in 0..40000 {
// NB Safety: This is taken from Nordic's driver -
// https://github.com/NordicSemiconductor/nrfx/blob/master/drivers/src/nrfx_uarte.c#L197
if unsafe { core::ptr::read_volatile(rxenable_reg) } == 0 {
workaround_succeded = true;
break;
}
else
{
// Need to sleep for 1us here
}
}

if !workaround_succeded
{
panic!("Failed to apply workaround for UART");
}

Uarte(uarte)
let errors = self.0.errorsrc.read().bits();
// NB Safety: safe to write back the bits we just read to clear them
self.0.errorsrc.write(|w| unsafe { w.bits(errors) });
self.0.enable.write(|w| w.enable().disabled());
}
}

/// Write via UARTE.
Expand All @@ -107,10 +179,7 @@ where
// before any DMA action has started.
compiler_fence(SeqCst);

// Reset the events.
self.0.events_endtx.reset();
self.0.events_txstopped.reset();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Actually this was needed if we keep the blocking wait for txstopped below 😅 ... It should probably be moved next to that so it's more obvious why it's needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤣



// Set up the DMA write.
self.0.txd.ptr.write(|w|
// We're giving the register a pointer to the stack. Since we're
Expand All @@ -129,37 +198,39 @@ where
// values.
unsafe { w.maxcnt().bits(tx_buffer.len() as _) });

// Reset the event
self.0.events_endtx.reset();

// Start UARTE Transmit transaction.
self.0.tasks_starttx.write(|w|
// `1` is a valid value to write to task registers.
unsafe { w.bits(1) });


// Wait for transmission to end.
let mut endtx;
let mut txstopped;
loop {
endtx = self.0.events_endtx.read().bits() != 0;
txstopped = self.0.events_txstopped.read().bits() != 0;
if endtx || txstopped {
break;
}
while self.0.events_endtx.read().bits() == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the events_txstopped.reset(); above is no longer needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted. Will remove.

// TODO: Do something here which uses less power. Like `wfi`.
}

// Conservative compiler fence to prevent optimizations that do not
// take in to account actions by DMA. The fence has been placed here,
// after all possible DMA actions have completed.
compiler_fence(SeqCst);

if txstopped {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe remove the Transmit error? it'd be a breaking change though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the enum? Might as well leave it in for now in case we do need it again.

return Err(Error::Transmit);
}
// Reset the event
self.0.events_txstopped.reset();

// Lower power consumption by disabling the transmitter once we're
// finished.
self.0.tasks_stoptx.write(|w|
// `1` is a valid value to write to task registers.
unsafe { w.bits(1) });

// Wait for transmitter to stop.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tasks_stoptx write and this spin is unneeded. If you don't abort a tx, just receiving endtx already means the tx machinery is fully stopped.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to Nordic:

            // Transmitter has to be stopped by triggering the STOPTX task to achieve
            // the lowest possible level of the UARTE power consumption.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that we seem to care about power consumption (as we busy-wait during a DMA transmit to the UART)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh huh, strange. When I experimented with this, I saw UARTE using power as long as it's enabled, no matter what the tx/rx state is, so to have low power you have to disable it anyway... Embassy only waits for txstopped when disabling it, and only if the tx was aborted, which seems to work fine.

Seeing nordic has this code I'm now OK with leaving this in, though I don't fully get what the purpose is.

while self.0.events_txstopped.read().bits() == 0 {
// Spin
}

Ok(())
}

Expand Down