<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Sep 12, 2013 at 8:17 PM, Gedare Bloom <span dir="ltr"><<a href="mailto:gedare@rtems.org" target="_blank">gedare@rtems.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, Sep 12, 2013 at 2:10 PM, Hesham Moustafa<br>
<<a href="mailto:heshamelmatary@gmail.com">heshamelmatary@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Thu, Sep 12, 2013 at 3:01 PM, Gedare Bloom <<a href="mailto:gedare@rtems.org">gedare@rtems.org</a>> wrote:<br>
>><br>
>> On Thu, Sep 12, 2013 at 2:59 AM, Hesham AL-Matary<br>
>> <<a href="mailto:heshamelmatary@gmail.com">heshamelmatary@gmail.com</a>> wrote:<br>
>> > ---<br>
>> > .../lib/libbsp/arm/shared/include/arm-cp15-start.h | 11 ++++--<br>
>> > c/src/lib/libbsp/arm/shared/mm.c | 46<br>
>> > ++++++++++++++++++++++<br>
>> > c/src/lib/libbsp/arm/shared/mminit.c | 27 +++++++++++++<br>
>> > 3 files changed, 80 insertions(+), 4 deletions(-)<br>
>> > create mode 100644 c/src/lib/libbsp/arm/shared/mm.c<br>
>> > create mode 100644 c/src/lib/libbsp/arm/shared/mminit.c<br>
>> ><br>
>> > diff --git a/c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h<br>
>> > b/c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h<br>
>> > index 01f3104..b463b1f 100644<br>
>> > --- a/c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h<br>
>> > +++ b/c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h<br>
>> > @@ -1,4 +1,5 @@<br>
>> > -/*<br>
>> > +/*<br>
>> > + * Copyright (c) 2013 Hesham AL-Matary.<br>
>> > * Copyright (c) 2009-2013 embedded brains GmbH. All rights reserved.<br>
>> > *<br>
>> > * embedded brains GmbH<br>
>> > @@ -89,7 +90,7 @@<br>
>> > arm_cp15_start_setup_translation_table_and_enable_mmu_and_cache(<br>
>> ><br>
>> > /* Initialize translation table with invalid entries */<br>
>> Fix this comment (assuming the change below now populates the ttb with<br>
>> valid entries).<br>
>><br>
>> > for (i = 0; i < ARM_MMU_TRANSLATION_TABLE_ENTRY_COUNT; ++i) {<br>
>> > - ttb [i] = 0;<br>
>> > + ttb [i] = (i << ARM_MMU_SECT_BASE_SHIFT) |<br>
>> > ARMV7_MMU_DATA_READ_WRITE;<br>
>> > }<br>
>> ><br>
>> > for (i = 0; i < config_count; ++i) {<br>
>> > @@ -97,11 +98,13 @@<br>
>> > arm_cp15_start_setup_translation_table_and_enable_mmu_and_cache(<br>
>> > }<br>
>> ><br>
>> > /* Enable MMU and cache */<br>
>> > - ctrl |= ARM_CP15_CTRL_I | ARM_CP15_CTRL_C | ARM_CP15_CTRL_M;<br>
>> > + ctrl |= ARM_CP15_CTRL_AFE | ARM_CP15_CTRL_S | ARM_CP15_CTRL_I |<br>
>> > + ARM_CP15_CTRL_C | ARM_CP15_CTRL_M | ARM_CP15_CTRL_XP;<br>
>> > +<br>
>> > arm_cp15_set_control(ctrl);<br>
>> > }<br>
>> ><br>
>> > -BSP_START_TEXT_SECTION static inline uint32_t<br>
>> > +BSP_START_TEXT_SECTION inline uint32_t<br>
>> undo this change.<br>
><br>
> I already did, but that caused undefined reference to<br>
> arm_cp15_start_setup_mmu_and_cache<br>
> compilation error at arm-a9mpcore-start.h:89 when I was compiling realview<br>
> with --enable-smp<br>
> option.<br>
</div></div>You should figure out why that happened. Changing this function to<br>
non-inline does not seem like the right answer.<br>
</blockquote><div> The change is that it's not static anymore because it's called from</div><div>another file too. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5">
><br>
>><br>
>> > arm_cp15_start_setup_mmu_and_cache(uint32_t ctrl_clear, uint32_t<br>
>> > ctrl_set)<br>
>> > {<br>
>> > uint32_t ctrl = arm_cp15_get_control();<br>
>> > diff --git a/c/src/lib/libbsp/arm/shared/mm.c<br>
>> > b/c/src/lib/libbsp/arm/shared/mm.c<br>
>> > new file mode 100644<br>
>> > index 0000000..36e90be<br>
>> > --- /dev/null<br>
>> > +++ b/c/src/lib/libbsp/arm/shared/mm.c<br>
>> > @@ -0,0 +1,46 @@<br>
>> > +/*<br>
>> > + * Copyright (c) 2013 Hesham AL-Matary.<br>
>> > + *<br>
>> > + * The license and distribution terms for this file may be<br>
>> > + * found in the file LICENSE in this distribution or at<br>
>> > + * <a href="http://www.rtems.com/license/LICENSE" target="_blank">http://www.rtems.com/license/LICENSE</a>.<br>
>> > + */<br>
>> > +<br>
>> > +#include <bsp/mm.h><br>
>> > +#include <libcpu/arm-cp15.h><br>
>> > +#include <rtems/score/mm.h><br>
>> > +<br>
>> > +#define LIBMM_REGION_IS_CACHE_ENABLED(attr) \<br>
>> > + (((attr) >> RTEMS_MM_REGION_BIT_CACHE) & 1U)<br>
>> > +#define LIBMM_REGION_IS_PROTECTION_WRITE_ENABLED(attr) \<br>
>> > + (((attr) >> RTEMS_MM_REGION_BIT_WRITE) & 1U)<br>
>> > +#define LIBMM_REGION_IS_DEVICE_TYPE(attr) \<br>
>> > + (((attr) >> RTEMS_MM_REGION_BIT_DEVICE) & 1U)<br>
>> > +#define LIBMM_REGION_IS_SHARED_MEMORY(attr) \<br>
>> > + (((attr) >> RTEMS_MM_REGION_BIT_SHARED) & 1U)<br>
>> > +<br>
>> Should these macros be provided by the score/mm.h?<br>
>><br>
> mm.h or mmimpl.h whatever. It's used by applications calls as well as<br>
> mm.c which used these definitions to translate attributes to CPU specific<br>
> ones.<br>
</div></div>mm.h so that the macros are "visible".<br>
<div><div class="h5"><br></div></div></blockquote><div>Yes for both application and low-level implementation code. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5">
>><br>
>> > +#define ARM_MMU_BIND_SECTION_TO_CLIENT_DOMAIN \<br>
>> > + (ARM_MMU_DEFAULT_CLIENT_DOMAIN << ARM_MMU_SECT_DOMAIN_SHIFT)<br>
>> > +<br>
>> > +#define LIBMM_TRANSLATE_ATTRIBUTES_TO_CPU(attr) \<br>
>> > + ((ARM_MMU_BIND_SECTION_TO_CLIENT_DOMAIN) \<br>
>> > + | (ARM_MMU_SECT_AP_0) \<br>
>> > + | (LIBMM_REGION_IS_PROTECTION_WRITE_ENABLED(attr) ? \<br>
>> > + 0U : (ARM_MMU_SECT_AP_2)) \<br>
>> > + | (LIBMM_REGION_IS_CACHE_ENABLED(attr) ? \<br>
>> > + (ARM_MMU_SECT_TEX_0|ARM_MMU_SECT_C|ARM_MMU_SECT_B) : 0U) \<br>
>> > + | (LIBMM_REGION_IS_DEVICE_TYPE(attr) ? ARM_MMU_SECT_B : 0U) \<br>
>> > + | (LIBMM_REGION_IS_SHARED_MEMORY(attr) ? ARM_MMU_SECT_S : 0U) \<br>
>> > + | (ARM_MMU_SECT_DEFAULT))<br>
>> > +<br>
>> This macro could properly be named something simpler since it is local<br>
>> to this .c file.<br>
>><br>
> Sure, I just wanted to give it a self-describing name.<br>
>><br>
>> > +void bsp_memory_management_set_attributes(<br>
>> > + uintptr_t base,<br>
>> > + size_t size,<br>
>> > + uint32_t attr<br>
>> > +)<br>
>> > +{<br>
>> > + uintptr_t end = (uint32_t)base + (uint32_t)size;<br>
>> Don't cast base and size here.<br>
>><br>
>> > + uint32_t section_flags = LIBMM_TRANSLATE_ATTRIBUTES_TO_CPU(attr);<br>
>> > +<br>
>> > + arm_cp15_set_translation_table_entries(base, end, section_flags);<br>
>> > +}<br>
>> > diff --git a/c/src/lib/libbsp/arm/shared/mminit.c<br>
>> > b/c/src/lib/libbsp/arm/shared/mminit.c<br>
>> > new file mode 100644<br>
>> > index 0000000..333d65c<br>
>> > --- /dev/null<br>
>> > +++ b/c/src/lib/libbsp/arm/shared/mminit.c<br>
>> > @@ -0,0 +1,27 @@<br>
>> > +/*<br>
>> > + * Copyright (c) 2013 Hesham AL-Matary.<br>
>> > + *<br>
>> > + * The license and distribution terms for this file may be<br>
>> > + * found in the file LICENSE in this distribution or at<br>
>> > + * <a href="http://www.rtems.com/license/LICENSE" target="_blank">http://www.rtems.com/license/LICENSE</a>.<br>
>> > + */<br>
>> > +#include <bsp/arm-cp15-start.h><br>
>> > +#include <bsp/linker-symbols.h><br>
>> > +#include <bsp/mm.h><br>
>> > +#include <bsp/start.h><br>
>> > +<br>
>> > +extern const mm_init_start_config bsp_mm_config_table[];<br>
>> > +extern const size_t bsp_mm_config_table_size;<br>
>> > +<br>
>> > +BSP_START_TEXT_SECTION void bsp_memory_management_initialize(void)<br>
>> > +{<br>
>> > + uint32_t ctrl = arm_cp15_get_control();<br>
>> > +<br>
>> > + arm_cp15_start_setup_translation_table_and_enable_mmu_and_cache(<br>
>> > + ctrl,<br>
>> > + (uint32_t *) bsp_translation_table_base,<br>
>> > + ARM_MMU_DEFAULT_CLIENT_DOMAIN,<br>
>> > + &bsp_mm_config_table[0],<br>
>> > + bsp_mm_config_table_size<br>
>> > + );<br>
>> Fix indentation level of nested lines of arguments. Also, this<br>
>> mminit.c file does not make sense until you add at least one of the<br>
>> ARM bsps using it.<br>
>><br>
> realview, xilinx, and raspberry currently<br>
> calls the initialization function within this<br>
> mminit.c<br>
</div></div>Right, except they have not been added prior to this patch. Without<br>
knowledge about them, this code does not make sense, right? Anyway, I<br>
think it might be useful to re-arrange the order of these patches in<br>
order to add the raspberrypi mmu handling first, and then show how to<br>
consolidate the ARM MMU handling, and then offer some generic routines<br>
in the bsp layer (e.g. bsp_memory_management_initialize) that can<br>
unify the other BSPs like powerpc ones that will also have MMU/MPU<br>
requirements.<br>
<div class="HOEnZb"><div class="h5"><br></div></div></blockquote><div>Sure, that means that the next set of patches should only include raspberrypi </div><div>implementation only, Right ?</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">
>><br>
>> > +}<br>
>> > --<br>
>> > 1.8.3.1<br>
>> ><br>
>> > _______________________________________________<br>
>> > rtems-devel mailing list<br>
>> > <a href="mailto:rtems-devel@rtems.org">rtems-devel@rtems.org</a><br>
>> > <a href="http://www.rtems.org/mailman/listinfo/rtems-devel" target="_blank">http://www.rtems.org/mailman/listinfo/rtems-devel</a><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>