[U-Boot] arm64 patch: gicv3 support

[Arnab Basu]

Hi David

Sorry for the late review, please find some comments inline.

On 15-Jan-14 1:40 PM, fenghua at phytium.com.cn wrote:
> From: David Feng <fenghua at phytium.com.cn>
> 
> This patch add gicv3 support to uboot armv8 platform.
> Modifications cover 4 source files, as follows:
>   gic.S: gicv3 initialization and sgi interrupt raising.
>   goc.h: gicv3 register definitions.
>   vexpress_aemv8a.h: add CONFIG_GICV2/CONFIG_GICV3 switch.
>   start.S: set SCR_EL3.NS bit to 1, gicv3 register of ICC_SRE_EL2
>            could be accessed only when SCR_EL3.NS=1.
>            set SCR_EL3.IRQ|FIQ|EA bits, reroute all interrupts to
>            el3 at all cores, slaves could be waken up by interrupt
>            only when the interrupt is routed to it when running
>            at el3.

I cant understand why it is required that we route IRQs to EL3 here. Won't it be sufficient
to only route FIQs to EL3 and wake the secondary processors using an FIQ?

> Note: please use the latest gcc 4.8 compiler from linaro 
>       which support gicv3 system register assembling.
> 
> Signed-off-by: David Feng <fenghua at phytium.com.cn>
> 
> ---
> arch/arm/cpu/armv8/gic.S          |   84 ++++++++++++++++++++++++++++++++++++-
>  arch/arm/cpu/armv8/start.S        |    5 ++-
>  arch/arm/include/asm/gic.h        |   56 +++++++++++++++++++++++++
>  include/configs/vexpress_aemv8a.h |    7 ++++
>  4 files changed, 150 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv8/gic.S b/arch/arm/cpu/armv8/gic.S
> index 599aa8f..a08939a 100644
> --- a/arch/arm/cpu/armv8/gic.S
> +++ b/arch/arm/cpu/armv8/gic.S
> @@ -23,6 +23,74 @@
>   *
>   *************************************************************************/
>  WEAK(gic_init)
> +#if defined(CONFIG_GICV3)

There is quite a bit of code inside CONFIG_GICV3 which is very similar to code in CONFIG_GICV2
why not collect the common code in one place?

> +	branch_if_slave	x0, 3f
> +
> +	/* Initialize Distributor */
> +	ldr	x1, =GICD_BASE
> +	mov	w0, #0x37		/* EnableGrp0 | EnableGrp1NS */
> +					/* EnableGrp1S | ARE_S | ARE_NS */
> +	str	w0, [x1, GICD_CTLR]	/* Secure GICD_CTLR */
> +	ldr	w0, [x1, GICD_TYPER]
> +	and	w2, w0, #0x1f		/* ITLinesNumber */
> +	cbz	w2, 1f			/* No SPIs */
> +	add	x3, x1, (GICD_IGROUPRn + 4)
> +	add	x4, x1, (GICD_IGROUPMODRn + 4)
> +	mov	w5, #~0
> +0:	str	w5, [x3], #0x4
> +	str	wzr, [x4], #0x4		/* Config SPIs as Group1NS */
> +	sub	w2, w2, #0x1
> +	cbnz	w2, 0b
> +
> +	/* Initialize All ReDistributors */
> +1:	ldr	x1, =GICR_BASE
> +2:	mov	w0, #~0x2
> +	ldr	w2, [x1, GICR_WAKER]
> +	and	w2, w2, w0		/* Clear ProcessorSleep */
> +	str	w2, [x1, GICR_WAKER]
> +	dsb	st
> +	isb
> +0:	ldr	w0, [x1, GICR_WAKER]
> +	tbnz	w0, #2, 0b		/* Wait Children be Alive */
> +
> +	add	x2, x1, #(1 << 16)	/* SGI_Base */
> +	mov	w5, #~0
> +	str	w5, [x2, GICR_IGROUPRn]
> +	str	wzr, [x2, GICR_IGROUPMODRn]	/* SGIs|PPIs Group1NS */
> +	mov	w0, #0x1		/* Enable SGI 0 */
> +	str	w0, [x2, GICR_ISENABLERn]
> +
> +	ldr	w0, [x1, GICR_TYPER]
> +	add	x1, x1, #(2 << 16)
> +	tbz	w0, #4, 2b		/* Next ReDistributor if Exist */

I am not sure that this is a good idea. Why should the primary code initialize all redistributors?
Would it not be a better idea to make this code per cpu and let each core initialize its own
redistributor interface?

> +
> +	/* Initialize Cpu Interface */
> +3:	mrs	x0, ICC_SRE_EL3
> +	orr	x0, x0, #0xf		/* SRE & Disable IRQ/FIQ Bypass & */
> +					/* Allow EL2 access to ICC_SRE_EL2 */
> +	msr	ICC_SRE_EL3, x0
> +	isb
> +
> +	mrs	x0, ICC_SRE_EL2
> +	orr	x0, x0, #0xf		/* SRE & Disable IRQ/FIQ Bypass & */
> +					/* Allow EL1 access to ICC_SRE_EL1 */

I don't think that u-boot should be doing this unconditionally. If U-Boot is configured to
boot the OS in EL2 (i.e. CONFIG_ARMV8_SWITCH_TO_EL1 is not set) then this should be left to
the OS. This code should probably be enclosed in ifdef CONFIG_ARMV8_SWITCH_TO_EL1

> +	msr	ICC_SRE_EL2, x0
> +	isb
> +
> +	mov	x0, #0x3		/* EnableGrp1NS | EnableGrp1S */
> +	msr	ICC_IGRPEN1_EL3, x0
> +	isb
> +
> +	msr	ICC_CTLR_EL3, xzr
> +	isb
> +
> +	msr	ICC_CTLR_EL1, xzr	/* NonSecure ICC_CTLR_EL1 */

Again why is U-Boot configuring this register, isn't it best left to the software running
in non-secure EL1?

Thanks
Arnab