On 15/08/2015 8:14 pm, Rohini Kulkarni wrote:
>
>
> On Fri, Aug 14, 2015 at 12:41 PM, Rohini Kulkarni <krohini1...@gmail.com
> <mailto:krohini1...@gmail.com>> wrote:
>
>
>
> On Fri, Aug 14, 2015 at 8:33 AM, Chris Johns <chr...@rtems.org
> <mailto:chr...@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
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
