cpu, sam0_common: fix uart TXC check

[smlng]

should fix #7617, and also adapts some register checks to use read-only bits where possible.

Tested with samr21-xpro. @travisgriggs please try this one and verify if that resolves your issue reported in #7617.

[travisgriggs]

Tested. This works.

[travisgriggs]

FWIW, I've also tested without the check completely, and that works too. And is slightly faster at spewing characters at 230400 (I didn't try any other speeds).

[travisgriggs]

I'd like to suggest a compromise with the TXC test, based on some interaction we had with Microchip/Atmel engineers. We asked them why the ASF version has the TXC check in it:

"If INTFLAG.DRE bit is set, doesn’t mean that data is transmitted (it just moved to tx shift register). To make sure the entire data frame including stop bit(s) has been transmitted and no new data was written to DATA, the Transmit Complete interrupt flag (INTFLAG.TXC) will be set.

If you have 10 bytes to send, you may not need to check TXC bit for every byte transfer. You can check the TXC bit for 10th byte transmission.

In ASF, usart_write_wait() function is to just transmit a character via the USART. This is the reason it checks both bits (DRE and TXC).”

The RIOT uart_write() function actually writes len bytes. I think the correct thing to do would be move the TXC spin check out of the loop, and just do it once after the whole buffer has been written. E.g.

void uart_write(uart_t uart, const uint8_t *data, size_t len)
{
    for (size_t i = 0; i < len; i++) {
        while (!dev(uart)->INTFLAG.bit.DRE) {}
        dev(uart)->DATA.reg = data[i];
    }
    while (!dev(uart)->INTFLAG.bit.TXC) {}
}

I have tested this as well with positive results. It's worth about 1.5% faster for my tests. Obviously, the larger your buffers are, the more savings you get. But it's not really about the speed. It just seems more consistent with what the TXC bit was meant for.

[smlng]

I think the correct thing to do would be move the TXC spin check out of the loop, and just do it once after the whole buffer has been written.

Nice idea, I agree - and thanks for the detailed background info!

[smlng]

rebased and squashed

[photonthunder]

Be great to see this bug fix get merged

[haukepetersen]

The actual fix looks good. I am however not so fond of the bit notation for flag checks and please re-fix the error bit clear...

[haukepetersen]

Please revert this change, not need for read/modify/write here, a simple write is sufficient: Writing a one to this bit will clear the flag.

[smlng]

I guess you are right, I mis-interpreted the usage of it here and thought might be safer to read/write here - just as we do in other places.

[haukepetersen]

Actually, I like the original check much better, but this might be subjective. At some point we decided to go the the .reg & .. notation consistently for all the sam0 periph drivers...

[smlng]

with the updated CMSIS headers from the newer ASF the bit fields are read-only, and reg is RW. So I would guess the compiler could optimise things a bit (more) when using read-only registers for read operations on the bit-field instead of using an RW register when using reg.

Do I make sense here?

[smlng]

Plus, I would therefore recommend to adapt all periph drivers accordingly to have it consistent again.

[haukepetersen]

Yepp, code size does not change with this, re-checked it. So yes, with them being read only why not change it. Maybe this would be cleaner done in a separate PR, but just leave it here if you want.

[kaspar030]

Maybe this would be cleaner done in a separate PR, but just leave it here if you want.

Yes, please keep the PR fix-only...

[smlng]

factored out into #7709

[kaspar030]

Parens missing... Please fix and squash. ACK.

comments addressed

[kaspar030]

ACK.