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

[Rohini Kulkarni]

On Fri, Aug 14, 2015 at 12:41 PM, Rohini Kulkarni <krohini1593 at gmail.com>
wrote:

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

So what is the way out here?

>
>> Chris
>>
>> > \ No newline at end of file
>> >
>>
>
>
>
> --
> Rohini Kulkarni
>



-- 
Rohini Kulkarni
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20150815/31ab0e0d/attachment.html>