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

[Chris Johns]

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.

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. If something else exists does this call need to be updated ?

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.

>  
>  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.

Chris

> \ No newline at end of file
>