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

Yuantian Tang
2014-10-16T03:02:06Z

> -----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?

Regards,
Yuantian

> York

-------------- next part --------------
An embedded message was scrubbed...
From: Yuantian Tang <Yuantian.Tang@freescale.com>
Subject: RE: [PATCH] ARM: HYP/non-sec: Make a variable as a local one
Date: Mon, 22 Sep 2014 08:46:09 +0000
Size: 1951
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141016/2cf013f6/attachment.mht>