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
