[U-Boot] [PATCH 2/4] ARM: HYP/non-sec: Make armv7_init_nonsec() usable before relocation

[York Sun <yorksun@freescale.com>]

On 10/15/14 8:02 PM, "Tang Yuantian-B29983" <Yuantian.Tang@freescale.com>
wrote:

>> -----Original Message-----
>> From: Sun York-R58495
>> Sent: Wednesday, October 15, 2014 11:44 PM
>> To: Tang Yuantian-B29983; albert.u.boot at aribaud.net
>> Cc: u-boot at lists.denx.de; Jin Zhengxiong-R64188
>> Subject: Re: [PATCH 2/4] ARM: HYP/non-sec: Make armv7_init_nonsec()
>>usable
>> before relocation
>> 
>> On 10/09/2014 01:11 AM, Yuantian.Tang at freescale.com wrote:
>> > From: Tang Yuantian <Yuantian.Tang@freescale.com>
>> >
>> > Defining variable gic_dist_addr as a globe one prevents function
>> > armv7_init_nonsec() from being used before relocation which is the
>> > case in the deep sleep resume process on Freescale QorIQ SoC
>> > platforms.
>> > This patch removes this limitation by adding a extra same meaning
>> > local variable. In this way, no exsiting codes get corrupts.
>> >
>> > Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
>> > ---
>> >  arch/arm/cpu/armv7/virt-v7.c | 14 ++++++++------
>> >  1 file changed, 8 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/arch/arm/cpu/armv7/virt-v7.c
>> > b/arch/arm/cpu/armv7/virt-v7.c index 651ca40..e1dfce9 100644
>> > --- a/arch/arm/cpu/armv7/virt-v7.c
>> > +++ b/arch/arm/cpu/armv7/virt-v7.c
>> > @@ -75,6 +75,7 @@ int armv7_init_nonsec(void)  {
>> >  	unsigned int reg;
>> >  	unsigned itlinesnr, i;
>> > +	unsigned long gic_base_addr;
>> >
>> >  	/* check whether the CPU supports the security extensions */
>> >  	reg = read_id_pfr1();
>> > @@ -89,23 +90,24 @@ int armv7_init_nonsec(void)
>> >  	 * any access to it will trap.
>> >  	 */
>> >
>> > -	gic_dist_addr = get_gicd_base_address();
>> > -	if (gic_dist_addr == -1)
>> > +	gic_base_addr = get_gicd_base_address();
>> > +	gic_dist_addr = gic_base_addr;
>> > +	if (gic_base_addr == -1)
>> >  		return -1;
>> >
>> >  	/* enable the GIC distributor */
>> > -	writel(readl(gic_dist_addr + GICD_CTLR) | 0x03,
>> > -	       gic_dist_addr + GICD_CTLR);
>> > +	writel(readl(gic_base_addr + GICD_CTLR) | 0x03,
>> > +	       gic_base_addr + GICD_CTLR);
>> >
>> >  	/* TYPER[4:0] contains an encoded number of available interrupts */
>> > -	itlinesnr = readl(gic_dist_addr + GICD_TYPER) & 0x1f;
>> > +	itlinesnr = readl(gic_base_addr + GICD_TYPER) & 0x1f;
>> >
>> >  	/* set all bits in the GIC group registers to one to allow access
>> >  	 * from non-secure state. The first 32 interrupts are private per
>> >  	 * CPU and will be set later when enabling the GIC for each core
>> >  	 */
>> >  	for (i = 1; i <= itlinesnr; i++)
>> > -		writel((unsigned)-1, gic_dist_addr + GICD_IGROUPRn + 4 * i);
>> > +		writel((unsigned)-1, gic_base_addr + GICD_IGROUPRn + 4 * i);
>> >
>> >  #ifndef CONFIG_ARMV7_PSCI
>> >  	smp_set_core_boot_addr((unsigned long)secure_ram_addr(_smp_pen),
>> > -1);
>> >
>> 
>> Wouldn't it be better to declare gic_dist_base as a local variable? It
>>is only used
>> once outside function armv7_switch_nonsec(). It could be replaced with
>> get_gicd_base_address() call.
>> 
>I am with you. That's what I did in the first version of this patch.
>Patch links is at: http://patchwork.ozlabs.org/patch/391065/
>But Albert seems have some concerns. The attached is what we discussed.
>
>Now on the second thought, I prefer the way this patch proposed because
>if we define gic_dist_base as local variable,
>That means function get_gicd_base_address() should be usable at any time
>in any mode. Can we make sure of that in the future?

I don't strongly object introducing a new local variable. But I don't see
how the global variable is useful. Function get_gicd_base_address() should
be available all the time. It reads PERIPHBASE register, or return a
macro. It hasn't changed since the first patch added it in 2013. Not sure
if the original author Andre Przywara is available to comments.

York