cpu: Add bit manipulation macros for Cortex-M #6916

jnohlgard · 2017-04-17T15:19:15Z

This PR introduces a new header bitband.h which provides macros for using the bit banding memory region found in Cortex-M3 and above, as well as a fall back for Cortex-M0 and Cortex-M0+ without bit banding.
Some Cortex-M0+ devices have their own bit manipulation equivalents, such as the Bit Manipulation Engine in Kinetis M0+ devices. On such CPUs, define BITBAND_MACROS_PROVIDED to 1 and provide your own macros.

The new macros are named bit_set32, bit_set16, and bit_set8, for setting single bits (i.e. writing a 1), and bit_clear32, bit_clear16, and bit_clear8 for clearing single bits (i.e. writing a 0).
The suffix numbers are the memory access width, some in-cpu modules have 8 bit or 16 bit registers which may require memory accesses to be the correct width to avoid undefined behaviour (Kinetis UART module is one example where using 32 bit access can mess up internal states).

This PR is a prerequisite for an upcoming Kinetis FRDM-KW41z port.

kaspar030 · 2017-04-18T08:14:31Z

Nice!

Some thoughts:

would static inlines instead of macros work for this?
would it make sense to factor out the type from the macro (e.g., BIT_SET(uint32_t, ptr, bit), and then use that for the other implementations?
does adding toggling functions make sense?

Looking forward to using this for gpio, so with LTO, a gpio_set() compiles down to a single store instruction...

jnohlgard · 2017-04-18T13:13:31Z

Static inline might work, didn't test yet.
Regarding type as parameter: it makes sense to do that, it simply didn't occur to me while refactoring.
Regarding toggle: it makes sense for the Kinetis BME implementation, which provides hardware support, don't know what it would be useful for though, other than toggling GPIOs. I would prefer to add it in a follow up together with a change that makes use of it. There's also the possible bit_load macro/function which would extract a single bit from a word.
Also, regarding GPIOs, many MCUs have specific set, clear, and toggle registers for GPIOs that our gpio drivers always should use, if possible.

jnohlgard · 2017-04-20T05:08:52Z

The static inline method seems like it would be best, I found a bunch of mistakes where I missed the address-of operator which was hidden by the cast in the macro version. I will update this PR soon-ish

jnohlgard · 2017-04-24T08:02:09Z

OK to squash?

haukepetersen · 2017-04-24T15:16:05Z

Another thought: why the fallback for Cortex-M0(+)? IMHO the bitbanging is only used in CPU specific code anyway, and in that code path it should be clear which target is worked on, right?

haukepetersen · 2017-04-24T15:16:06Z

Another thought: why the fallback for Cortex-M0(+)? IMHO the bitbanging is only used in CPU specific code anyway, and in that code path it should be clear which target is worked on, right?

haukepetersen · 2017-04-24T15:16:33Z

sorry, wrong button + browser lag...

kaspar030 · 2017-04-24T15:17:37Z

"Some Cortex-M0+ devices have their own bit manipulation equivalents, such as the Bit Manipulation Engine in Kinetis M0+ devices. On such CPUs, define BITBAND_MACROS_PROVIDED to 1 and provide your own macros."

haukepetersen · 2017-04-24T15:21:32Z

I see, never mind then.

jnohlgard · 2017-04-24T16:18:07Z

@haukepetersen this is an example of a specific implementation for a kinetis m0+
https://github.com/gebart/RIOT/blob/pr/frdm-kw41z/cpu/kinetis_common/include/bme.h

jnohlgard · 2017-04-26T11:46:43Z

@haukepetersen Also, using these functions in the STM common drivers might make sense in some situations, it will use bit-banding on the M3 and up, but still compile cleanly on the low end devices.

In Kinetis we use these macros to avoid having to disable IRQs or using mutexes while messing with clock gate registers (since the registers are shared between many different peripheral drivers)

jnohlgard · 2017-05-02T11:20:22Z

I accidentally squashed while rebasing, sorry for that. Anyway, this PR is ready for review and testing. Murdock is fine except for squashing the fixups.

haukepetersen · 2017-05-10T10:28:56Z

Changes look good to me, but I don't have a board around today, so can test only tomorrow. But please feel free to squash and make Murdock happy with that!

kaspar030 · 2017-05-10T12:54:45Z

Would it make sense to have the interface globally (not only for cortexm)? @gebart what do you think?

Otherwise, functions within "bitband.h" should be prefixed with "bitband_*".

jnohlgard · 2017-05-10T13:12:00Z

@kaspar030 It does make sense to rename the header. I guess it would be possible to move the fallback to a global header and keep the bitband.h part inside cortexm.

haukepetersen · 2017-05-11T10:11:43Z

I don't quite see the reason to make this global (yet), this is something cpu-arch internal and some other CPU arch that supports this might bring its own style of handling this, so why force this header on every arch?!

kaspar030 · 2017-05-30T08:53:14Z

so why force this header on every arch?!

The underlying bitbanding is architecture dependend, but "set bit N of the int at addr A" is not. Currently, we probably use e.g., val |= (1<< bitnum). A static inline bit_set(&val, bitnum) probably compiles to the same code if bitbanding is unavailable, but would make use of bitbanding where available...

cgundogan · 2017-05-30T14:08:51Z

cpu/cortexm_common/include/bitband.h

+
+#if DOXYGEN
+/** @brief Flag for telling if the CPU has hardware bit band support */
+#define CPU_HAS_BITBAND 1 || 0 (1 for Cortex-M3 and up, 0 for Cortex-M0)


I am seeing this style of documenting the first time. Shouldn't this go into some sort of @brief statement?

I think it looks quite nice in the generated docs:

Otherwise it would say that the definition is simply 1, even though it is CPU dependent.

I'm okay with changing it if you think it is weird to write the documentation as a code syntax error.

jnohlgard · 2017-06-01T05:14:54Z

@kaspar030 The bitband accesses to RAM on CM3 is restricted to a part of the address space (0x20000000-0x2xxxxxxx), so it may hardfault on CPUs where some RAM is addressed below 0x20000000, e.g. Kinetis.
The generic approach will also become problematic on Kinetis L when used on normal variables because the bit manipulation engine on that CPU family only supports bit manipulation on memory locations within the peripheral registers memory area (0x40000000-0x41FFFFFF).

Also there's the issue of non-atomicity in the generic implementation, if the access is already protected by a lock or disabled IRQs then another call to irq_disable/irq_restore will bloat the code a lot. The CM3 bitband accesses are atomic for free, and the number of CPU cycles for a hardware read-modify-write cycle via bitbanding is the same as for a software read-modify-write.

kaspar030 · 2017-06-01T06:41:47Z

@gebart Ok so we can't make this (easily) generic. I guess then it is fine to merge as is. I'll take last look over the code now.

kaspar030 · 2017-06-01T06:44:50Z

cpu/cortexm_common/include/bitband.h

+ * @param[in]  ptr pointer to target word
+ * @param[in]  bit bit number within the word
+ */
+static inline void bit_set32(volatile uint32_t *ptr, uint8_t bit)


Should we rename the functions to bitband_*()?

I'd rather rename the header bit.h if it is necessary, since bit banding is only one of the implementations

fine with me!

kaspar030 · 2017-06-01T06:46:27Z

cpu/cortexm_common/include/cpu.h

@@ -36,6 +36,7 @@
 #include "sched.h"
 #include "thread.h"
 #include "cpu_conf.h"
+#include "bitband.h"


Is this include necessary? It does include both ways...

Convenience or lazy, this makes the big macros immediately available in all files using cpu.h

but doesn't this contradict the 'import only what is neccessary' thing that has impacts to the build process/speed?!

addressed, moved include to the consumers

kaspar030

The bit.h header guard needs fixing. Other than that I'm fine with that header. Can't really test, as I don't have the hardeare...

kaspar030 · 2017-06-05T20:58:23Z

cpu/cortexm_common/include/bit.h

+ * @author      Joakim Nohlgård <[email protected]>
+ */
+
+#ifndef BITBAND_H


Hm, somehow this file triggers an encoding bug in the headercheck script. I suspect the beautiful a with "a" circle on top in your name. ;)
Unfortunately I cannot reproduce the bug locally.

Anyhow, this define (and the closing endif comment) need to be adapted to the file rename.
dist/tools/headerguards/check.sh | patch -p0 should do that for you.

updated, the headerguards script worked fine on my machine locally to fix it automatically like you wrote.

kaspar030 · 2017-06-06T06:21:31Z

@gebart please squash! Could anyone with a kinetis do a quick check?

jnohlgard · 2017-06-07T11:39:26Z

squashed

jnohlgard · 2017-06-09T03:19:54Z

reworded commit msg on kinetis commit

aabadie · 2017-06-21T10:04:49Z

This one looks in good shape: just a test remains. Is there someone with this board who can test ? Otherwise @gebart, can you provide a copy of the output of the uart/timer test applications for the boards you have ?

kaspar030 · 2017-06-21T10:29:23Z

Otherwise @gebart, can you provide a copy of the output of the uart/timer test applications for the boards you have ?

IMO a little trust goes a long way here.

aabadie · 2017-06-21T10:42:08Z

IMO a little trust goes a long way here.

+1. So an ACK would be enough then

kaspar030 · 2017-06-21T10:47:34Z

ACK. Let's see how long it takes until we want to use bit.h somewhere else. ;)

jnohlgard added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Apr 17, 2017

jnohlgard assigned haukepetersen Apr 17, 2017

jnohlgard force-pushed the pr/cortexm3-bitband branch from 3d02631 to 3811708 Compare April 20, 2017 08:42

jnohlgard added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 20, 2017

jnohlgard force-pushed the pr/cortexm3-bitband branch from d3c03a3 to 195c125 Compare April 24, 2017 12:38

haukepetersen closed this Apr 24, 2017

haukepetersen reopened this Apr 24, 2017

jnohlgard force-pushed the pr/cortexm3-bitband branch 3 times, most recently from 7cb89b8 to a5e2d80 Compare May 2, 2017 08:50

This was referenced May 2, 2017

cpu+board frdm-k22f: Add NXP FRDM-K22F development board #6994

Merged

cpu+board: Add support for NXP Kinetis KW41Z and FRDM-KW41Z development board #6995

Merged

jnohlgard force-pushed the pr/cortexm3-bitband branch from c3e0cba to f4334db Compare May 10, 2017 12:12

jnohlgard force-pushed the pr/cortexm3-bitband branch from f4334db to 37592ab Compare May 30, 2017 06:51

jnohlgard mentioned this pull request May 30, 2017

drivers: Add support for KW41Z builtin transceiver #7107

Closed

cgundogan reviewed May 30, 2017

View reviewed changes

jnohlgard force-pushed the pr/cortexm3-bitband branch from 37592ab to 5bcbbba Compare June 1, 2017 05:16

kaspar030 requested changes Jun 1, 2017

View reviewed changes

jnohlgard force-pushed the pr/cortexm3-bitband branch from 5bcbbba to 0cd14d4 Compare June 5, 2017 20:20

kaspar030 requested changes Jun 5, 2017

View reviewed changes

jnohlgard force-pushed the pr/cortexm3-bitband branch from 8a93387 to 35cd5f7 Compare June 7, 2017 11:39

Joakim Nohlgård added 2 commits June 9, 2017 05:19

cortexm_common: Introduce bitbanding macros

dc3aa13

kinetis: Use bit.h functions

a63c0a8

jnohlgard force-pushed the pr/cortexm3-bitband branch from 35cd5f7 to a63c0a8 Compare June 9, 2017 03:19

jnohlgard mentioned this pull request Jun 20, 2017

cpu/cc2538/periph/gpio: use bit-banding macros for thread-safe GPIO #5753

Closed

kaspar030 approved these changes Jun 21, 2017

View reviewed changes

kaspar030 merged commit 42830d2 into RIOT-OS:master Jun 21, 2017

jnohlgard deleted the pr/cortexm3-bitband branch June 21, 2017 11:18

aabadie modified the milestone: Release 2017.07 Jun 26, 2017

cpu: Add bit manipulation macros for Cortex-M #6916

cpu: Add bit manipulation macros for Cortex-M #6916

Conversation

jnohlgard commented Apr 17, 2017 • edited Loading

kaspar030 commented Apr 18, 2017

jnohlgard commented Apr 18, 2017

jnohlgard commented Apr 20, 2017

jnohlgard commented Apr 24, 2017

haukepetersen commented Apr 24, 2017

haukepetersen commented Apr 24, 2017

haukepetersen commented Apr 24, 2017

kaspar030 commented Apr 24, 2017

haukepetersen commented Apr 24, 2017

jnohlgard commented Apr 24, 2017

jnohlgard commented Apr 26, 2017

jnohlgard commented May 2, 2017

haukepetersen commented May 10, 2017

kaspar030 commented May 10, 2017

jnohlgard commented May 10, 2017

haukepetersen commented May 11, 2017

kaspar030 commented May 30, 2017

Choose a reason for hiding this comment

jnohlgard Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

jnohlgard commented Jun 1, 2017

kaspar030 commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspar030 left a comment

Choose a reason for hiding this comment

kaspar030 Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspar030 commented Jun 6, 2017

jnohlgard commented Jun 7, 2017

jnohlgard commented Jun 9, 2017

aabadie commented Jun 21, 2017

kaspar030 commented Jun 21, 2017

aabadie commented Jun 21, 2017

kaspar030 commented Jun 21, 2017

jnohlgard commented Apr 17, 2017 •

edited

Loading

jnohlgard Jun 1, 2017 •

edited

Loading

kaspar030 Jun 5, 2017 •

edited

Loading