On 10/15/14 8:02 PM, "Tang Yuantian-B29983" <yuantian.t...@freescale.com> wrote:
>> -----Original Message----- >> From: Sun York-R58495 >> Sent: Wednesday, October 15, 2014 11:44 PM >> To: Tang Yuantian-B29983; albert.u.b...@aribaud.net >> Cc: u-boot@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.t...@freescale.com wrote: >> > From: Tang Yuantian <yuantian.t...@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.t...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot