On 06/28/2013 05:18 AM, Masahiro Yamada wrote:
Hi Andre.


--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -60,6 +60,8 @@ COBJS-y       += reset.o
  COBJS-y       += cache.o
  COBJS-y       += cache-cp15.o

+COBJS-$(CONFIG_ARMV7_VIRT) += virt-v7.o
+

Judging from the file name virt-v7.c,
you are thinkig this file is specific to ARMv7, aren't you?

If so, why don't you move this file
to arch/arm/cpu/armv7/ ?


+static void set_generic_timer_frequency(void)
+{
+#ifdef CONFIG_SYS_CLK_FREQ
+       unsigned int reg;
+
+       reg = read_id_pfr1();
+       if ((reg & CPUID_ARM_TIMER_MASK) == 1U << CPUID_ARM_TIMER_SHIFT)
+               asm("mcr p15, 0, %0, c14, c0, 0\n"
+               : : "r"(CONFIG_SYS_CLK_FREQ));
+#endif

CPUID_ARM_TIMER_MASK
CPUID_ARM_TIMER_SHIFT

I think these macro names are vague.
There are Generic Timer, Global Timer, Private Timer etc.

Good point.

Unlike other parts, the care for Cortex-A9 is missing here.
To be more generic, I'd like to suggest to allow Non-secure access
to Global/Private timers before switching to non-secure state.


How about like this for armv7_switch_nonsec function?


     /* check whether the CPU supports the security extensions */
     reg = read_id_pfr1();
     if ((reg & 0xF0) == 0)
             return HYP_ERR_NO_SEC_EXT;

     if ((reg & CPUID_ARM_TIMER_MASK) == 1U << CPUID_ARM_TIMER_SHIFT)
             set_generic_timer_frequency();
     else
             /* Allow Non-secure access to Global/Private timers */
             writel(0xfff, periph_base + SCU_SNSAC);

Interesting, however I don't have access to an A9 board currently to properly test this. So I will do the renaming and let the access to the other timers up to a follow-up patch.

Thanks,
Andre.


For more info about SCU Non-secure Access Control (SNSAC) Register,
please refer Cortex A9 mpcore TRM.  page 2-11
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0407g/DDI0407G_cortex_a9_mpcore_r3p0_trm.pdf



+       /* enable the GIC distributor */
+       writel(readl(&gicdptr[GICD_CTLR]) | 0x03, &gicdptr[GICD_CTLR]);

I am not sure this code is really necessary.

Because your are setting all available interrupts to Group1 just below,
I think we don't need to do this in secure state.


Best Regards
Masahiro Yamada


_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to