SPI and writing data to registers

Some hardware peripheral register basics for embedded systems beginners:

As a rule of thumb, only read from peripheral registers at one single place and only write to them at one single place. Otherwise you risk subtle real-time issues. Also, doing multiple reads/writes in a row makes the code needlessly slow for absolutely nothing gained, as seen in various Arduino "tutorials" etc written by quacks.

In this case (I'm assuming 32 bit registers):

uint32_t cr1 = SPI1->CR1; // read ONCE // Now do any bit manipulations you fancy here, without concerns for performance: cr1 |= 1<<2; cr1 &= ~(1<<7); SPI1->CR1 = cr1; // write ONCE 

Example:

(In this case I just used dummy volatile variables that ended up on the stack to simulate registers)

volatile uint32_t SPI1_CR1; uint32_t cr1 = SPI1_CR1; cr1 |= 1<<2; cr1 &= ~(1<<7); SPI1_CR1 = cr1; 

Disassembling this on gcc/ARM-none-eabi -O3 gives something like:

 ldr r3, [sp, #4] bic r3, r3, #128 orr r3, r3, #4 str r3, [sp, #4] 

My cr1 variable ended up in a register. Everything is done in 4 instructions. Had I however written directly to the volatile-qualified register, then I'd get extra overhead:

 volatile uint32_t SPI1_CR1; SPI1_CR1 |= 1<<2; SPI1_CR1 &= ~(1<<7); ldr r3, [sp, #4] orr r3, r3, #4 str r3, [sp, #4] ldr r3, [sp, #4] bic r3, r3, #128 str r3, [sp, #4] 

Now regarding naming, readability and ruggedness:

• We shouldn't write magic numbers such as 1<<2 to a register. If you force the reader of your code to sit with their nose in the user manual watching the register descriptions at all times, then your code is bad.
• We can do all bit manipulations to the same register on a single line, which might improve readability and performance slightly.
• We should never write 1<<... because 1 has type int which is signed and we should never do bitwise arithmetic on signed types. Write 1u<<... instead.

For more details check out How to access a hardware register from firmware? Now as it happens it even used a generic SPI peripheral as example. After following all advise in that post, the proper code should look something like:

#define SPICR_SPIE (1u << 7) #define SPICR_CPOL (1u << 4) #define SPICR_CPHA (1u << 3) ... SPICR = SPICR_SPIE | SPICR_CPHA; 

Or in case you prefer the alternative style:

#define SPICR_SPIE(val) ((val) << 7) #define SPICR_CPOL(val) ((val) << 4) #define SPICR_CPHA(val) ((val) << 3) ... SPICR = SPICR_SPIE(1) | SPICR_CPOL(0) | SPICR_CPHA(1) ; 

In the latter form we aren't forced to use a single bit either, so it could be used for setting multiple bits like in a baudrate prescaler. However, it is then also custom to mask. Lets say there are 4 baudrate prescaler bits found from bit 3 to 6 in the register:

#define SPICR_BAUD(val) ((val & 0xF) << 3) 

4 bits = the mask 1111 = 0xF. Then shift afterwards, for readability. Something like (val << 3) & 0xE8u would be equivalent but needlessly hard to read.