[PATCH v3] arm/xilinx_zynq: Disable the MMU and the data and instruction cache on boot.

[Pavel Pisa]

Hello Chris,

I am trying to think about consequences.

On Wednesday 31 of August 2016 04:16:34 Chris Johns wrote:
> The u-boot loader can enable the MMU plus the data and instruction caches.
> Disable them and if the data cache is enabled clear it before turn the
> caches off.
>
> Closes #2774.

I think that this is critical problem and has to be solved.
May be, this should be committed because it solves problems
for your actual boot and is probably OK for UP.

But for SMP, I think that there are more problems. When I look into

rtems/c/src/lib/libbsp/arm/shared/include/arm-a9mpcore-start.h

I see

> #ifdef RTEMS_SMP
> BSP_START_TEXT_SECTION static inline void
> arm_a9mpcore_start_on_secondary_processor(void)
> {
>   uint32_t ctrl;
>
>   arm_a9mpcore_start_set_vector_base();
>
>   arm_gic_irq_initialize_secondary_cpu();
>
>   ctrl = arm_cp15_start_setup_mmu_and_cache(
>     0,
>     ARM_CP15_CTRL_AFE | ARM_CP15_CTRL_Z
>   );

If you add arm_cp15_data_cache_clean_all_levels() into
arm_cp15_start_setup_mmu_and_cache() then according to my understanding
of case there can be problems if cache is already or still enabled
on primary CPU.

I have started to check how are the caches implemented on Zynq
and my debug code shows that arm_cp15_data_cache_clean_all_levels()
sees only the first cache level on the CPU. It seem that controler
is not part of the CPU architecture. So that means that for Zynq
use of arm_cp15_data_cache_clean_all_levels() is OK.

The result

cache levels:
  clidr 0x09200003 loc 1
  level 0 ctype 3
  level 0 ccsidr 0x701FE019
  line_power 0x05 associativity 0x04 way_shift 0x1E
  level 1 ctype 0
  level 2 ctype 0

This means no cache above level 1 are detected by CP15 code and level
of coherency is 1 anyway so safe for Zynq. But RPi2 shows multiple levels
for example. So I would vote to move code into per BSP part the same
way as Cyclone has that.

>   arm_cp15_set_domain_access_control(
>     ARM_CP15_DAC_DOMAIN(ARM_MMU_DEFAULT_CLIENT_DOMAIN, ARM_CP15_DAC_CLIENT)
>   );
>
>   /* FIXME: Sharing the translation table between processors is brittle */
>   arm_cp15_set_translation_table_base(
>     (uint32_t *) bsp_translation_table_base
>   );
>
>   ctrl |= ARM_CP15_CTRL_I | ARM_CP15_CTRL_C | ARM_CP15_CTRL_M;
>   arm_cp15_set_control(ctrl);
>
>   _SMP_Start_multitasking_on_secondary_processor();
> }

Further analysis for generic case.

If it is ensured that cache is disabled on primary CPU during start
of all secondary CPUs and then sequence
  invalidate cache
  wait on barrier with primary
  enable cache
  wait on barrier with promary
  start multitasking
should be OK.

But mixing can be bad. 

When I look into "altera-cyclone-v" which I expect that it is the 
most tested one by Sebastian, then it solves this problem
by explicitly putting invalidate into bsp_start_hook_0()

rtems/c/src/lib/libbsp/arm/altera-cyclone-v/startup/bspstarthooks.c

BSP_START_TEXT_SECTION void bsp_start_hook_0( void )
{
  arm_cp15_instruction_cache_invalidate();
  arm_cp15_data_cache_invalidate_all_levels();
  arm_a9mpcore_start_hook_0();
}

This is called even on secondary CPU and L1 caches setup can
be mixed for short while. But it is not problem because
L2 cache should be enabled only after

rtems/c/src/lib/libbsp/arm/altera-cyclone-v/startup/bspsmp.c:_CPU_SMP_Start_processor()
    started = _Per_CPU_State_wait_for_non_initial_state(cpu_index, 0);

finishes according to the comment.

But when I try to analyze order by traceback

_CPU_SMP_Start_processor
_SMP_Start_processor
_SMP_Handler_initialize
rtems_initialize_data_structures

RTEMS_SYSINIT_ITEM(
  rtems_initialize_data_structures,
  RTEMS_SYSINIT_DATA_STRUCTURES,
  RTEMS_SYSINIT_ORDER_MIDDLE


RTEMS_SYSINIT_BSP_WORK_AREAS
RTEMS_SYSINIT_BSP_START
RTEMS_SYSINIT_INITIAL_EXTENSIONS
RTEMS_SYSINIT_DATA_STRUCTURES 
  so there are enabled other CPUs and there is wait for sync
RTEMS_SYSINIT_USER_EXTENSIONS
RTEMS_SYSINIT_CPU_SET

L2 cache is enabled in bsp_start_hook_1() on Cyclone 

  /* Enable unified L2 cache */
  rtems_cache_enable_data();

But bsp_start_hook_1() is called early in the boot process even
on Cyclone from libbsp/arm/shared/start/start.S. So I see even there
problems to clarify that sequence is completely OK.

I am not expert for SMP and ARM for sure. But I have doubts that
all that is OK. I try follow with some patch which i consider correct.
But not sure.

There is another problem which I have noticed on RPi.
Cache invalidate when U-boots starts application with cache enabled
can lead to the lost of some data or code, because they are held
only in cache before cache invalidate and disabled.

So I suggest to flush data there before cache disable.

Best wishes,

                Pavel

PS: comments are little convoluted, because I have spent quite large amount
    of time by rethinking and generally looked at that during coordination
    of more other projects done by studnets at the university.

On Wednesday 31 of August 2016 04:16:34 Chris Johns wrote:
> The u-boot loader can enable the MMU plus the data and instruction caches.
> Disable them and if the data cache is enabled clear it before turn the
> caches off.
>
> Closes #2774.
> ---
>  c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h   | 4 ++++
>  c/src/lib/libbsp/arm/xilinx-zynq/startup/bspstartmmu.c | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h
> b/c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h index
> 01fdbb3..7734ddc 100644
> --- a/c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h
> +++ b/c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h
> @@ -166,6 +166,10 @@ arm_cp15_start_setup_mmu_and_cache(uint32_t
> ctrl_clear, uint32_t ctrl_set) {
>    uint32_t ctrl = arm_cp15_get_control();
>
> +  if ((ctrl & ARM_CP15_CTRL_C) != 0) {
> +    arm_cp15_data_cache_clean_all_levels();
> +  }
> +
>    ctrl &= ~ctrl_clear;
>    ctrl |= ctrl_set;
>
> diff --git a/c/src/lib/libbsp/arm/xilinx-zynq/startup/bspstartmmu.c
> b/c/src/lib/libbsp/arm/xilinx-zynq/startup/bspstartmmu.c index
> c7a1089..0918588 100644
> --- a/c/src/lib/libbsp/arm/xilinx-zynq/startup/bspstartmmu.c
> +++ b/c/src/lib/libbsp/arm/xilinx-zynq/startup/bspstartmmu.c
> @@ -41,7 +41,7 @@ BSP_START_TEXT_SECTION void
> zynq_setup_mmu_and_cache(void) __attribute__ ((weak) 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_A | ARM_CP15_CTRL_C | ARM_CP15_CTRL_I | ARM_CP15_CTRL_M,
>      ARM_CP15_CTRL_AFE | ARM_CP15_CTRL_Z
>    );