I moved forward with converting inline functions to macros and thought I
would share a little before moving on. Things I did
1. Per Greg's suggestion, I added #ifdef CONFIG_ARM_MPU to mpu.h and
<chip>_mpuinit.h
2. Moved static inline functions mpu_control and mpu_configure_region
to up_mpu.c. I did this for two reasons
1. mpu_configure_region was on the large side (55 lines)
2. I thought it was important to enforce type safety. mpu_control
accepts three bools that it uses for conditionals and I felt it
would be pretty easy for a user to pass any other data type and
potentially cause issues.
3. The spacing between comments would have made them ugly macros
3. Converted the remaining inline functions to macros
I validated everything was good by compiling the imxrt-106-evk:knsh and
nsh configs with
1. Protected build
2. Flat build, mpu enabled
3. Flat build, mpu disabled
I also took a look at the coding standard guide to make sure I was in
compliance, but I'm unsure of where I should put the 'do'
#define mpu_priv_stronglyordered(base, size) do \
{ \
/* The configure the region */ \
== OR ==
#define mpu_user_flash(base, size) \
do { \
/* The configure the region */ \
Thanks guys
- John
On 3/25/20 11:45 AM, Nathan Hartman wrote:
On Wed, Mar 25, 2020 at 11:38 AM Xiang Xiao <xiaoxiang781...@gmail.com>
wrote:
On Wed, Mar 25, 2020 at 11:33 PM John Rippetoe
<jrippe...@roboticresearch.com> wrote:
1. For each chip, files that include mpu support need to add a
conditional to two included files
#ifdef CONFIG_ARM_MPU
# include "mpu.h"
# include "<chip>_mpuinit.h"
#endif
Wouldn't it make more sense to put #ifdef CONFIG_ARM_MPU inside of
mpu.h and <chip>_mpuinit.h. That way, the problem is fixed in one
place for all time.
I hadn't considered that, but that does make more sense.
Another option would be to remove the inline qualifier and move the
functions into a .c file.
I'm equally fine with this, but I can see the appeal for wanting to keep
the small functions inlined. Of the 12 funcitons in mpu.h, 9 are just
wrappers around a call to mpu_configure_region that look like
static inline void mpu_priv_intsram(uintptr_t base, size_t size)
{
/* The configure the region */
mpu_configure_region(base, size,
MPU_RASR_TEX_SO | /* Ordered */
MPU_RASR_C | /* Cacheable */
/* Not Bufferable */
MPU_RASR_S | /* Shareable */
MPU_RASR_AP_RWNO /* P:RW U:None */
/* Instruction access */);
}
Like others have said, there is no guarantee these will be inlined.
Furthermore, the build system doesn't currently enforce a minimum C99
standard to ensure the inline keyword is even available. So I am in
favor of moving the functions to a .c file in the name of clarity.
Consolidating them into up_mpu.c seems like a good choice. If the
overhead of an additional function call is problematic, we could
predefine the flags and call mpu_configure_region() directly
#define MPU_PRIV_INTSRAM MPU_RASR_TEX_SO | /* Ordered */
MPU_RASR_C | /* Cacheable */
/* Not Bufferable */
MPU_RASR_S | /* Shareable */
MPU_RASR_AP_RWNO /* P:RW U:None */
/* Instruction access */);
mpu_configure_region(base, size, MPU_PRIV_INTSRAM);
Or change mpu_priv_intsram to macro?
That's a valid approach. Macros are C89 compliant, guaranteed to be inlined
no matter what, result in 0 code if unused, and don't cause problems in
header files.
Reminder: When implementing a function-like macro, always write it as do {
... } while (0) -- see
https://stackoverflow.com/a/257425
Cheers,
Nathan
CONFIDENTIALITY NOTICE: This communication may contain private, confidential
and privileged material for the sole use of the intended recipient. If you are
not the intended recipient, please delete this e-mail and any attachments
permanently.
