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

[Rohini Kulkarni]

On Fri, Aug 14, 2015 at 8:33 AM, Chris Johns <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.

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


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

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

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

>
> Chris
>
> > \ No newline at end of file
> >
>



-- 
Rohini Kulkarni
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20150814/5af96324/attachment-0002.html>