RFC: CMSIS structs vs sys_readXX/sys_writeXX

[avolkov-1221]

Hello,

I am starting this discussion as a continuation of our debate with @ithinuel regarding error #75693.

Currently, in Zephyr, for accessing hardware registers, both the CMSIS-based API using C structures and the more standard API based on sys_readXX/sys_writeXX and bitfield description macros are equally and simultaneously permitted.

As a supporter of the Linux way, I am more than convinced by the conclusions from the 2007 discussion* on banning volatile, and additionally, I want to highlight the impossibility of using C defined registers in assembly. However, someone might, in light of the past years, want to defends an opposing opinion.

IMO which API is preferable for use should be voted and documented in the "Coding Guidelines"; otherwise, every new commit of code will spark similar debates repeatedly, as the machine code produced during compilation is not equivalent.

[mathieuchopstm]

Taking your comments from 75693:

the use of bitfields from a hardware-mapped C-struct may leads to multiple unprotected non-atomic "read-modify-write" operations. Access to all hardware registers, especially the SYST_CSR, must be atomic. I.e. dedicated sys_readXX/sys_writeXX functions should be used instead of C-struct style accesses.
Yes , I know where the decls come from, and already have seen the generated code: indirectly disabled optimisation where it suppose to be, non-atomic "read-write-modify".

Could you elaborate on these aspects, and how direct access is broken?
For example, providing a code comparison between direct access and sys_readXX usage to demonstrate the issue.

[avolkov-1221]

Yes, sure I can, but not so fast: vacation time. La France :).

However, as a starting point, I can put here as an example, the "not so well coded " sys_clock_disable from cortex_m_systick.c :

void sys_clock_disable(void)
{
        SysTick->CTRL &= ~SysTick_CTRL_ENABLE_Msk;
}

Here, the interrupt handler can be called between any of three different operations:

1. read the register AND clear the SYST_CSR_TICKINT flag (damn, who made the decision to clean by reading instead of WOC?)
2. make the and in the register
3. write back the result.

If the handler will be called between 2 and 3 and re-enable the systick (as it should does) , then we will have strange result after the re-activation. It's not a disaster, but not so well.

More correct sequence here is:

1. get a spinlock; to synch access to the register for all parts of the system
2. execute write only sys_write32(0, SYST_CSR);, so the int handler will detects immediately that timer is disabled
3. execute sys_read32(SYST_CSR); to be sure that TICKINT is clear.
4. release spinlock.

With sys_read/sys_write, the sequence is easier to read and and potential problem is easier to discover then the simple &=,
that hide the non-atomicity of operation.

[ithinuel]

It seems to me that the issue here lies in the read-modify-write atomicity, not the use of struct vs sys_write*/sys_read*.

This is essentially the same as:

1. get a spinlock; to synch access to the register for all parts of the system
2. SysTick->CTRL = SomeConstant;
3. release the spinlock

sys_write*/sys_read* would not bring anything meaningful.