[U-Boot] [PATCH v2 4/7] ARM: switch to non-secure state during bootm execution

[Andre Przywara <andre.przywara@linaro.org>]

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
>