<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Aug 14, 2015 at 8:33 AM, Chris Johns <span dir="ltr"><<a href="mailto:chrisj@rtems.org" target="_blank">chrisj@rtems.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 14/08/2015 7:44 am, Rohini Kulkarni wrote:<br>
> ---<br>
><br>
> -BSP_START_DATA_SECTION static const arm_cp15_start_section_config<br>
> -zynq_mmu_config_table[] = {<br>
> +BSP_START_DATA_SECTION const arm_cp15_start_section_config<br>
> +arm_cp15_start_mmu_config_table[] = {<br>
>    ARMV7_CP15_START_DEFAULT_SECTIONS,<br>
>    {<br>
>      .begin = 0xe0000000U,<br>
<br>
Why not pass the table to the bsp_memory_management_initialize call and<br>
avoid making this table global ? Maybe all these tables should be static<br>
and local to their file's in all ARM bsps that do this type MMU set up.<br>
<br>
This global table and referencing it in the call limits its use and I<br>
think makes things a little more fragile. See below.<br></blockquote><div>I saw Raspberry Pi and altera define the arm_cp15_start_mmu_config_table. The table is declared in the arm-cp15-start.h and mminit.c uses this as the default table. So I didn't really understand why some had static tables while others didn't.<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Do all ARMs devices with MMUs have cp15 or is this limited to a specific<br>
family ? The only reason I ask is bsp_memory_management_initialize is a<br>
generic name. </blockquote><div>Not all ARM devices have cp15. The existing definition of bsp_memory_management_initialize supports only cp15. The definition would not be useful to other devices. And this definition was being replicated by some cp15 bsps again. So the reason for making these changes.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">If something else exists does this call need to be updated ?<br></blockquote><div>Yes, a different definition will be required. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I am still a little confused what the changes gives us. Should the RPi<br>
not use the call and it be removed ? I do not know.<br>
<br>
> @@ -39,17 +40,15 @@ zynq_mmu_config_table[] = {<br>
>  BSP_START_TEXT_SECTION void zynq_setup_mmu_and_cache(void) __attribute__ ((weak));<br>
<br>
If all this code is going to be touched I feel adding weak versions for<br>
all boards is a good thing.<br>
<br>
The Zynq and I suspect the Cyclone need this because the memory mapped<br>
can depend on what is loaded into the programmable logic (PL) side of<br>
these devices. I currently override the weak Zynq call in production<br>
code based on the PL logic loaded at run time. I wonder if this change<br>
will break code. I am concerned it needs to get a clean link yet we now<br>
have globals and other code referencing those globals. If the file is<br>
not referenced and nothing else depends on it things are more robust. <br></blockquote><div>I still don't have the deeper insight needed so I don't see dependencies. But if having static tables is much more robust then we should apply that to all. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
><br>
>  BSP_START_TEXT_SECTION void zynq_setup_mmu_and_cache(void)<br>
> -{<br>
> -  uint32_t ctrl = arm_cp15_start_setup_mmu_and_cache(<br>
> -    ARM_CP15_CTRL_A,<br>
> -    ARM_CP15_CTRL_AFE | ARM_CP15_CTRL_Z<br>
> -  );<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>
> -    &zynq_mmu_config_table[0],<br>
> -    RTEMS_ARRAY_SIZE(zynq_mmu_config_table)<br>
> +{<br>
> +  uint32_t bsp_initial_mmu_ctrl_set = ARM_CP15_CTRL_AFE | ARM_CP15_CTRL_Z;<br>
> +  uint32_t bsp_initial_mmu_ctrl_clear = ARM_CP15_CTRL_A;<br>
> +<br>
> +  bsp_memory_management_initialize(<br>
> +    bsp_initial_mmu_ctrl_set,<br>
> +    bsp_initial_mmu_ctrl_clear<br>
>    );<br>
>  }<br>
> +<br>
> +const size_t arm_cp15_start_mmu_config_table_size =<br>
> +  RTEMS_ARRAY_SIZE(arm_cp15_start_mmu_config_table);<br>
<br>
Same here. I think we should avoid globals. <br></blockquote><div>Then I think raspberry Pi and altera should also follow this if this is much more appropriate.<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Chris<br>
<br>
> \ No newline at end of file<br>
><br>
</blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature"><div dir="ltr">Rohini Kulkarni</div></div>
</div></div>