[PATCH 4/4] Code refactor xilinx-zynq BSP MMU initialization

[Chris Johns]

On 15/08/2015 8:14 pm, Rohini Kulkarni wrote:
> 
> 
> On Fri, Aug 14, 2015 at 12:41 PM, Rohini Kulkarni <krohini1593 at gmail.com
> <mailto:krohini1593 at gmail.com>> wrote:
> 
> 
> 
>     On Fri, Aug 14, 2015 at 8:33 AM, Chris Johns <chrisj at rtems.org
>     <mailto:chrisj at rtems.org>> wrote:
> 
>         On 14/08/2015 7:44 am, Rohini Kulkarni wrote:
>         > ---
>         >
>         > -BSP_START_DATA_SECTION static const arm_cp15_start_section_config
>         > -zynq_mmu_config_table[] = {
>         > +BSP_START_DATA_SECTION const arm_cp15_start_section_config
>         > +arm_cp15_start_mmu_config_table[] = {
>         >    ARMV7_CP15_START_DEFAULT_SECTIONS,
>         >    {
>         >      .begin = 0xe0000000U,
> 
>         Why not pass the table to the bsp_memory_management_initialize
>         call and
>         avoid making this table global ? Maybe all these tables should
>         be static
>         and local to their file's in all ARM bsps that do this type MMU
>         set up.
> 
>         This global table and referencing it in the call limits its use
>         and I
>         think makes things a little more fragile. See below.
> 
>     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.
> 

This is confusing and we should settling on one specific way that
supports all use cases. Personally I think the table should not be
global and it should be private to the BSP. It is not clear to me what
advantage we get with this table being global.

>         Do all ARMs devices with MMUs have cp15 or is this limited to a
>         specific
>         family ? The only reason I ask is
>         bsp_memory_management_initialize is a
>         generic name. 
> 
>     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.

>From what I could see the bsp_memory_management_initialize is at the
shared BSP level which implies this is generic. If it is specific to an
architecture and then specific to sub-set of that architecture I wonder
if should stay.

>      
> 
>         If something else exists does this call need to be updated ?
> 
>     Yes, a different definition will be required.
> 

I see this becoming complex quickly. Making all BSPs with an MMU
dependent means a change for one could break the other dependent BSPs
and this increases the testing on all effected BSPs.

> 
>         I am still a little confused what the changes gives us. Should
>         the RPi
>         not use the call and it be removed ? I do not know.
> 
>         > @@ -39,17 +40,15 @@ zynq_mmu_config_table[] = {
>         >  BSP_START_TEXT_SECTION void zynq_setup_mmu_and_cache(void) __attribute__ ((weak));
> 
>         If all this code is going to be touched I feel adding weak
>         versions for
>         all boards is a good thing.
> 
>         The Zynq and I suspect the Cyclone need this because the memory
>         mapped
>         can depend on what is loaded into the programmable logic (PL)
>         side of
>         these devices. I currently override the weak Zynq call in production
>         code based on the PL logic loaded at run time. I wonder if this
>         change
>         will break code. I am concerned it needs to get a clean link yet
>         we now
>         have globals and other code referencing those globals. If the
>         file is
>         not referenced and nothing else depends on it things are more
>         robust. 
> 
>     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.
> 

Someone may add another call to bsp_memory_management_initialize for
some reason as it is a BSP level call and that code references the
global tables which also pulls in a weak version of the BSP call. What
happens to a user's application that has code to override the weak BSP
call to provide a custom MMU table ? We would have the weak and the
non-weak version being linked and I am not sure how the linker would
resolve this. I feel this makes the code fragile.

Having weak version of the MMU initialization is a good thing.

> 
>         >
>         >  BSP_START_TEXT_SECTION void zynq_setup_mmu_and_cache(void)
>         > -{
>         > -  uint32_t ctrl = arm_cp15_start_setup_mmu_and_cache(
>         > -    ARM_CP15_CTRL_A,
>         > -    ARM_CP15_CTRL_AFE | ARM_CP15_CTRL_Z
>         > -  );
>         > -
>         > - 
>         arm_cp15_start_setup_translation_table_and_enable_mmu_and_cache(
>         > -    ctrl,
>         > -    (uint32_t *) bsp_translation_table_base,
>         > -    ARM_MMU_DEFAULT_CLIENT_DOMAIN,
>         > -    &zynq_mmu_config_table[0],
>         > -    RTEMS_ARRAY_SIZE(zynq_mmu_config_table)
>         > +{
>         > +  uint32_t bsp_initial_mmu_ctrl_set = ARM_CP15_CTRL_AFE |
>         ARM_CP15_CTRL_Z;
>         > +  uint32_t bsp_initial_mmu_ctrl_clear = ARM_CP15_CTRL_A;
>         > +
>         > +  bsp_memory_management_initialize(
>         > +    bsp_initial_mmu_ctrl_set,
>         > +    bsp_initial_mmu_ctrl_clear
>         >    );
>         >  }
>         > +
>         > +const size_t arm_cp15_start_mmu_config_table_size =
>         > +  RTEMS_ARRAY_SIZE(arm_cp15_start_mmu_config_table);
> 
>         Same here. I think we should avoid globals.
> 
>     Then I think raspberry Pi and altera should also follow this if this
>     is much more appropriate.
> 
> 
> So what is the way out here? 
> 

A few options...

1. Move all BSPs with an MMU to use bsp_memory_management_initialize and
make it support all set ups (this is difficult).
2. Make the RPi be like the Zynq and private and with a weak MMU set up
call and remove the call.
3. Make the bsp_memory_management_initialize take args to the table.
4. Something else ...

I do not think 1 is an option because it creates a change hot spot in
the code and 3 is not adding much given only the ARM devices with cp15
support would use this call and Sebastian has made a nice API here already.

I would also like to hear what Sebastian has to say.

Chris