diff mbox

[U-Boot] arm64 patch: gicv3 support

Message ID 1389773456-37854-1-git-send-email-fenghua@phytium.com.cn
State Changes Requested
Delegated to: Albert ARIBAUD
Headers show

Commit Message

fenghua@phytium.com.cn Jan. 15, 2014, 8:10 a.m. UTC
From: David Feng <fenghua@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.
Note: please use the latest gcc 4.8 compiler from linaro 
      which support gicv3 system register assembling.

Signed-off-by: David Feng <fenghua@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(-)

Comments

Bhupesh Sharma Jan. 15, 2014, 2:19 p.m. UTC | #1
Hi David,

Thanks for the patch.
Please see some comments inline:

> -----Original Message-----
> From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de]
> On Behalf Of fenghua@phytium.com.cn
> Sent: Wednesday, January 15, 2014 1:41 PM
> To: u-boot@lists.denx.de
> Cc: trini@ti.com
> Subject: [U-Boot] [PATCH] arm64 patch: gicv3 support
> 
> From: David Feng <fenghua@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.

 	^^^
Typo - gic.h

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

Hmmm. This seems a bit suspicious - if we reroute even IRQs and Aborts
to the cores which work in EL3, they will not be visible to Linux or
Hypervisor which work in EL2 or EL1.

Have you tried to launch linux on the ARMv8 foundation model v2 with these changes?

> Note: please use the latest gcc 4.8 compiler from linaro
>       which support gicv3 system register assembling.
>

Two generic comments :

- I see in the Foundation model README that the optional GICv3 is supported
with memory-mapped CPU interface and distributor, but I see your patch accessing them
via the sytem register interface (via msr and mrs). Is this a BUG in the README?

Did you check this patch on the latest ARMv8 foundation model - v2?

- Also how about sharing the GICv2 coding between ARMv7 and ARMv8 - some of the code
may seems like a duplication from the ARMv7 GICv2 content.
 
> Signed-off-by: David Feng <fenghua@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)
> +	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 */
> +
> +	/* 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 */
> +	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 */
> +	isb
> +
> +	mov	x0, #0x1 << 7		/* Non-Secure access to ICC_PMR_EL1
> */
> +	msr	ICC_PMR_EL1, x0
> +	isb
> +#elif defined(CONFIG_GICV2)
>  	branch_if_slave	x0, 2f
> 
>  	/* Initialize Distributor and SPIs */
> @@ -54,7 +122,7 @@ WEAK(gic_init)
> 
>  	mov	w0, #0x1 << 7		/* Non-Secure access to GICC_PMR */
>  	str	w0, [x1, GICC_PMR]
> -
> +#endif
>  	ret
>  ENDPROC(gic_init)
> 
> @@ -65,11 +133,18 @@ ENDPROC(gic_init)
>   *
> 
> *************************************************************************
> /
>  WEAK(gic_send_sgi)
> +#if defined(CONFIG_GICV3)
> +	mov	x1, #(1 << 40)
> +	orr	x1, x1, x0, lsl #24
> +	msr	ICC_ASGI1R_EL1, x1
> +	isb
> +#elif defined(CONFIG_GICV2)
>  	ldr	x1, =GICD_BASE
>  	mov	w2, #0x8000
>  	movk	w2, #0x100, lsl #16
>  	orr	w2, w2, w0
>  	str	w2, [x1, GICD_SGIR]
> +#endif
>  	ret
>  ENDPROC(gic_send_sgi)
> 
> @@ -82,11 +157,18 @@ ENDPROC(gic_send_sgi)
>   *
> 
> *************************************************************************
> /
>  WEAK(wait_for_wakeup)
> +#if defined(CONFIG_GICV3)
> +0:	wfi
> +	mrs	x0, ICC_IAR1_EL1
> +	msr	ICC_EOIR1_EL1, x0
> +	cbnz	x0, 0b
> +#elif defined(CONFIG_GICV2)
>  	ldr	x1, =GICC_BASE
>  0:	wfi
>  	ldr	w0, [x1, GICC_AIAR]
>  	str	w0, [x1, GICC_AEOIR]
>  	cbnz	w0, 0b
> +#endif
>  	ret
>  ENDPROC(wait_for_wakeup)
> 
> diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
> index bcc2603..9911817 100644
> --- a/arch/arm/cpu/armv8/start.S
> +++ b/arch/arm/cpu/armv8/start.S
> @@ -50,7 +50,10 @@ reset:
>  	 */
>  	adr	x0, vectors
>  	switch_el x1, 3f, 2f, 1f
> -3:	msr	vbar_el3, x0
> +3:	mrs	x0, scr_el3
> +	orr	x0, x0, #0xf			/* SCR_EL3.NS|IRQ|FIQ|EA */
> +	msr	scr_el3, x0
> +	msr	vbar_el3, x0
>  	msr	cptr_el3, xzr			/* Enable FP/SIMD */
>  	ldr	x0, =COUNTER_FREQUENCY
>  	msr	cntfrq_el0, x0			/* Initialize CNTFRQ */
> diff --git a/arch/arm/include/asm/gic.h b/arch/arm/include/asm/gic.h
> index ac2b2bf..bd3a80c 100644
> --- a/arch/arm/include/asm/gic.h
> +++ b/arch/arm/include/asm/gic.h
> @@ -51,4 +51,60 @@
>  #define GICC_IIDR		0x00fc
>  #define GICC_DIR		0x1000
> 
> +/* ReDistributor Registers for Control and Physical LPIs */
> +#define GICR_CTLR		0x0000
> +#define GICR_IIDR		0x0004
> +#define GICR_TYPER		0x0008
> +#define GICR_STATUSR		0x0010
> +#define GICR_WAKER		0x0014
> +#define GICR_SETLPIR		0x0040
> +#define GICR_CLRLPIR		0x0048
> +#define GICR_SEIR		0x0068
> +#define GICR_PROPBASER		0x0070
> +#define GICR_PENDBASER		0x0078
> +#define GICR_INVLPIR		0x00a0
> +#define GICR_INVALLR		0x00b0
> +#define GICR_SYNCR		0x00c0
> +#define GICR_MOVLPIR		0x0100
> +#define GICR_MOVALLR		0x0110
> +
> +/* ReDistributor Registers for SGIs and PPIs */
> +#define GICR_IGROUPRn		0x0080
> +#define GICR_ISENABLERn		0x0100
> +#define GICR_ICENABLERn		0x0180
> +#define GICR_ISPENDRn		0x0200
> +#define GICR_ICPENDRn		0x0280
> +#define GICR_ISACTIVERn		0x0300
> +#define GICR_ICACTIVERn		0x0380
> +#define GICR_IPRIORITYRn	0x0400
> +#define GICR_ICFGR0		0x0c00
> +#define GICR_ICFGR1		0x0c04
> +#define GICR_IGROUPMODRn	0x0d00
> +#define GICR_NSACRn		0x0e00
> +
> +/* Cpu Interface System Registers */
> +#define ICC_IAR0_EL1		S3_0_C12_C8_0
> +#define ICC_IAR1_EL1		S3_0_C12_C12_0
> +#define ICC_EOIR0_EL1		S3_0_C12_C8_1
> +#define ICC_EOIR1_EL1		S3_0_C12_C12_1
> +#define ICC_HPPIR0_EL1		S3_0_C12_C8_2
> +#define ICC_HPPIR1_EL1		S3_0_C12_C12_2
> +#define ICC_BPR0_EL1		S3_0_C12_C8_3
> +#define ICC_BPR1_EL1		S3_0_C12_C12_3
> +#define ICC_DIR_EL1		S3_0_C12_C11_1
> +#define ICC_PMR_EL1		S3_0_C4_C6_0
> +#define ICC_RPR_EL1		S3_0_C12_C11_3
> +#define ICC_CTLR_EL1		S3_0_C12_C12_4
> +#define ICC_CTLR_EL3		S3_6_C12_C12_4
> +#define ICC_SRE_EL1		S3_0_C12_C12_5
> +#define ICC_SRE_EL2		S3_4_C12_C9_5
> +#define ICC_SRE_EL3		S3_6_C12_C12_5
> +#define ICC_IGRPEN0_EL1		S3_0_C12_C12_6
> +#define ICC_IGRPEN1_EL1		S3_0_C12_C12_7
> +#define ICC_IGRPEN1_EL3		S3_6_C12_C12_7
> +#define ICC_SEIEN_EL1		S3_0_C12_C13_0
> +#define ICC_SGI0R_EL1		S3_0_C12_C11_7
> +#define ICC_SGI1R_EL1		S3_0_C12_C11_5
> +#define ICC_ASGI1R_EL1		S3_0_C12_C11_6
> +
>  #endif /* __GIC_H__ */
> diff --git a/include/configs/vexpress_aemv8a.h
> b/include/configs/vexpress_aemv8a.h
> index ce5f384..ac67887 100644
> --- a/include/configs/vexpress_aemv8a.h
> +++ b/include/configs/vexpress_aemv8a.h
> @@ -12,6 +12,8 @@
> 
>  #define CONFIG_REMAKE_ELF
> 
> +#define CONFIG_GICV2
> +
>  /*#define CONFIG_ARMV8_SWITCH_TO_EL1*/
> 
>  /*#define CONFIG_SYS_GENERIC_BOARD*/
> @@ -93,8 +95,13 @@
>  #define COUNTER_FREQUENCY		(0x1800000)	/* 24MHz */
> 
>  /* Generic Interrupt Controller Definitions */
> +#ifdef CONFIG_GICV3
> +#define GICD_BASE			(0x2f000000)
> +#define GICR_BASE			(0x2f100000)
> +#else
>  #define GICD_BASE			(0x2C001000)
>  #define GICC_BASE			(0x2C002000)
> +#endif
> 
>  #define CONFIG_SYS_MEMTEST_START	V2M_BASE
>  #define CONFIG_SYS_MEMTEST_END		(V2M_BASE + 0x80000000)
> --
> 1.7.9.5
> 

Regards,
Bhupesh
fenghua@phytium.com.cn Jan. 16, 2014, 1:17 a.m. UTC | #2
hi bhupesh,


> -----Original Messages-----
> From: "bhupesh.sharma@freescale.com" <bhupesh.sharma@freescale.com>
> Sent Time: 2014-01-15 22:19:02 (Wednesday)
> To: "'fenghua@phytium.com.cn'" <fenghua@phytium.com.cn>, "u-boot@lists.denx.de" <u-boot@lists.denx.de>
> Cc: "trini@ti.com" <trini@ti.com>, "arnab.basu@freescale.com" <arnab.basu@freescale.com>
> Subject: RE: [U-Boot] [PATCH] arm64 patch: gicv3 support
> 
> Hi David,
> 
> Thanks for the patch.
> Please see some comments inline:
> 
> > -----Original Message-----
> > From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de]
> > On Behalf Of fenghua@phytium.com.cn
> > Sent: Wednesday, January 15, 2014 1:41 PM
> > To: u-boot@lists.denx.de
> > Cc: trini@ti.com
> > Subject: [U-Boot] [PATCH] arm64 patch: gicv3 support
> > 
> > From: David Feng <fenghua@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.
> 
>  	^^^
> Typo - gic.h
> 
> >   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.
> 
> Hmmm. This seems a bit suspicious - if we reroute even IRQs and Aborts
> to the cores which work in EL3, they will not be visible to Linux or
> Hypervisor which work in EL2 or EL1.
> 
These bits will be cleared when jumping to el2.

> Have you tried to launch linux on the ARMv8 foundation model v2 with these changes?
> 
Yes.

> > Note: please use the latest gcc 4.8 compiler from linaro
> >       which support gicv3 system register assembling.
> >
> 
> Two generic comments :
> 
> - I see in the Foundation model README that the optional GICv3 is supported
> with memory-mapped CPU interface and distributor, but I see your patch accessing them
> via the sytem register interface (via msr and mrs). Is this a BUG in the README?
> 
The document did not describe it clearly, but actually it support.

> Did you check this patch on the latest ARMv8 foundation model - v2?
> 
Yes.

> - Also how about sharing the GICv2 coding between ARMv7 and ARMv8 - some of the code
> may seems like a duplication from the ARMv7 GICv2 content.
>  
Many codes could be shared between armv7 and armv8 such as cache maintenance and gic.
These need be rearranged seriously. We'd better wait a period of time before the armv8 codes
are more widely acquainted and get more feedback about this.

> > Signed-off-by: David Feng <fenghua@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)
> > +	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 */
> > +
> > +	/* 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 */
> > +	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 */
> > +	isb
> > +
> > +	mov	x0, #0x1 << 7		/* Non-Secure access to ICC_PMR_EL1
> > */
> > +	msr	ICC_PMR_EL1, x0
> > +	isb
> > +#elif defined(CONFIG_GICV2)
> >  	branch_if_slave	x0, 2f
> > 
> >  	/* Initialize Distributor and SPIs */
> > @@ -54,7 +122,7 @@ WEAK(gic_init)
> > 
> >  	mov	w0, #0x1 << 7		/* Non-Secure access to GICC_PMR */
> >  	str	w0, [x1, GICC_PMR]
> > -
> > +#endif
> >  	ret
> >  ENDPROC(gic_init)
> > 
> > @@ -65,11 +133,18 @@ ENDPROC(gic_init)
> >   *
> > 
> > *************************************************************************
> > /
> >  WEAK(gic_send_sgi)
> > +#if defined(CONFIG_GICV3)
> > +	mov	x1, #(1 << 40)
> > +	orr	x1, x1, x0, lsl #24
> > +	msr	ICC_ASGI1R_EL1, x1
> > +	isb
> > +#elif defined(CONFIG_GICV2)
> >  	ldr	x1, =GICD_BASE
> >  	mov	w2, #0x8000
> >  	movk	w2, #0x100, lsl #16
> >  	orr	w2, w2, w0
> >  	str	w2, [x1, GICD_SGIR]
> > +#endif
> >  	ret
> >  ENDPROC(gic_send_sgi)
> > 
> > @@ -82,11 +157,18 @@ ENDPROC(gic_send_sgi)
> >   *
> > 
> > *************************************************************************
> > /
> >  WEAK(wait_for_wakeup)
> > +#if defined(CONFIG_GICV3)
> > +0:	wfi
> > +	mrs	x0, ICC_IAR1_EL1
> > +	msr	ICC_EOIR1_EL1, x0
> > +	cbnz	x0, 0b
> > +#elif defined(CONFIG_GICV2)
> >  	ldr	x1, =GICC_BASE
> >  0:	wfi
> >  	ldr	w0, [x1, GICC_AIAR]
> >  	str	w0, [x1, GICC_AEOIR]
> >  	cbnz	w0, 0b
> > +#endif
> >  	ret
> >  ENDPROC(wait_for_wakeup)
> > 
> > diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
> > index bcc2603..9911817 100644
> > --- a/arch/arm/cpu/armv8/start.S
> > +++ b/arch/arm/cpu/armv8/start.S
> > @@ -50,7 +50,10 @@ reset:
> >  	 */
> >  	adr	x0, vectors
> >  	switch_el x1, 3f, 2f, 1f
> > -3:	msr	vbar_el3, x0
> > +3:	mrs	x0, scr_el3
> > +	orr	x0, x0, #0xf			/* SCR_EL3.NS|IRQ|FIQ|EA */
> > +	msr	scr_el3, x0
> > +	msr	vbar_el3, x0
> >  	msr	cptr_el3, xzr			/* Enable FP/SIMD */
> >  	ldr	x0, =COUNTER_FREQUENCY
> >  	msr	cntfrq_el0, x0			/* Initialize CNTFRQ */
> > diff --git a/arch/arm/include/asm/gic.h b/arch/arm/include/asm/gic.h
> > index ac2b2bf..bd3a80c 100644
> > --- a/arch/arm/include/asm/gic.h
> > +++ b/arch/arm/include/asm/gic.h
> > @@ -51,4 +51,60 @@
> >  #define GICC_IIDR		0x00fc
> >  #define GICC_DIR		0x1000
> > 
> > +/* ReDistributor Registers for Control and Physical LPIs */
> > +#define GICR_CTLR		0x0000
> > +#define GICR_IIDR		0x0004
> > +#define GICR_TYPER		0x0008
> > +#define GICR_STATUSR		0x0010
> > +#define GICR_WAKER		0x0014
> > +#define GICR_SETLPIR		0x0040
> > +#define GICR_CLRLPIR		0x0048
> > +#define GICR_SEIR		0x0068
> > +#define GICR_PROPBASER		0x0070
> > +#define GICR_PENDBASER		0x0078
> > +#define GICR_INVLPIR		0x00a0
> > +#define GICR_INVALLR		0x00b0
> > +#define GICR_SYNCR		0x00c0
> > +#define GICR_MOVLPIR		0x0100
> > +#define GICR_MOVALLR		0x0110
> > +
> > +/* ReDistributor Registers for SGIs and PPIs */
> > +#define GICR_IGROUPRn		0x0080
> > +#define GICR_ISENABLERn		0x0100
> > +#define GICR_ICENABLERn		0x0180
> > +#define GICR_ISPENDRn		0x0200
> > +#define GICR_ICPENDRn		0x0280
> > +#define GICR_ISACTIVERn		0x0300
> > +#define GICR_ICACTIVERn		0x0380
> > +#define GICR_IPRIORITYRn	0x0400
> > +#define GICR_ICFGR0		0x0c00
> > +#define GICR_ICFGR1		0x0c04
> > +#define GICR_IGROUPMODRn	0x0d00
> > +#define GICR_NSACRn		0x0e00
> > +
> > +/* Cpu Interface System Registers */
> > +#define ICC_IAR0_EL1		S3_0_C12_C8_0
> > +#define ICC_IAR1_EL1		S3_0_C12_C12_0
> > +#define ICC_EOIR0_EL1		S3_0_C12_C8_1
> > +#define ICC_EOIR1_EL1		S3_0_C12_C12_1
> > +#define ICC_HPPIR0_EL1		S3_0_C12_C8_2
> > +#define ICC_HPPIR1_EL1		S3_0_C12_C12_2
> > +#define ICC_BPR0_EL1		S3_0_C12_C8_3
> > +#define ICC_BPR1_EL1		S3_0_C12_C12_3
> > +#define ICC_DIR_EL1		S3_0_C12_C11_1
> > +#define ICC_PMR_EL1		S3_0_C4_C6_0
> > +#define ICC_RPR_EL1		S3_0_C12_C11_3
> > +#define ICC_CTLR_EL1		S3_0_C12_C12_4
> > +#define ICC_CTLR_EL3		S3_6_C12_C12_4
> > +#define ICC_SRE_EL1		S3_0_C12_C12_5
> > +#define ICC_SRE_EL2		S3_4_C12_C9_5
> > +#define ICC_SRE_EL3		S3_6_C12_C12_5
> > +#define ICC_IGRPEN0_EL1		S3_0_C12_C12_6
> > +#define ICC_IGRPEN1_EL1		S3_0_C12_C12_7
> > +#define ICC_IGRPEN1_EL3		S3_6_C12_C12_7
> > +#define ICC_SEIEN_EL1		S3_0_C12_C13_0
> > +#define ICC_SGI0R_EL1		S3_0_C12_C11_7
> > +#define ICC_SGI1R_EL1		S3_0_C12_C11_5
> > +#define ICC_ASGI1R_EL1		S3_0_C12_C11_6
> > +
> >  #endif /* __GIC_H__ */
> > diff --git a/include/configs/vexpress_aemv8a.h
> > b/include/configs/vexpress_aemv8a.h
> > index ce5f384..ac67887 100644
> > --- a/include/configs/vexpress_aemv8a.h
> > +++ b/include/configs/vexpress_aemv8a.h
> > @@ -12,6 +12,8 @@
> > 
> >  #define CONFIG_REMAKE_ELF
> > 
> > +#define CONFIG_GICV2
> > +
> >  /*#define CONFIG_ARMV8_SWITCH_TO_EL1*/
> > 
> >  /*#define CONFIG_SYS_GENERIC_BOARD*/
> > @@ -93,8 +95,13 @@
> >  #define COUNTER_FREQUENCY		(0x1800000)	/* 24MHz */
> > 
> >  /* Generic Interrupt Controller Definitions */
> > +#ifdef CONFIG_GICV3
> > +#define GICD_BASE			(0x2f000000)
> > +#define GICR_BASE			(0x2f100000)
> > +#else
> >  #define GICD_BASE			(0x2C001000)
> >  #define GICC_BASE			(0x2C002000)
> > +#endif
> > 
> >  #define CONFIG_SYS_MEMTEST_START	V2M_BASE
> >  #define CONFIG_SYS_MEMTEST_END		(V2M_BASE + 0x80000000)
> > --
> > 1.7.9.5
> > 
> 
> Regards,
> Bhupesh
Bhupesh Sharma Jan. 16, 2014, 3:45 a.m. UTC | #3
> -----Original Message-----
> From: FengHua [mailto:fenghua@phytium.com.cn]
> Sent: Thursday, January 16, 2014 6:47 AM
> To: Sharma Bhupesh-B45370
> Cc: u-boot@lists.denx.de; trini@ti.com; Basu Arnab-B45036
> Subject: Re: RE: [U-Boot] [PATCH] arm64 patch: gicv3 support
> 
> 
> hi bhupesh,
> 
> 
> > -----Original Messages-----
> > From: "bhupesh.sharma@freescale.com" <bhupesh.sharma@freescale.com>
> > Sent Time: 2014-01-15 22:19:02 (Wednesday)
> > To: "'fenghua@phytium.com.cn'" <fenghua@phytium.com.cn>,
> > "u-boot@lists.denx.de" <u-boot@lists.denx.de>
> > Cc: "trini@ti.com" <trini@ti.com>, "arnab.basu@freescale.com"
> > <arnab.basu@freescale.com>
> > Subject: RE: [U-Boot] [PATCH] arm64 patch: gicv3 support
> >
> > Hi David,
> >
> > Thanks for the patch.
> > Please see some comments inline:
> >
> > > -----Original Message-----
> > > From: u-boot-bounces@lists.denx.de
> > > [mailto:u-boot-bounces@lists.denx.de]
> > > On Behalf Of fenghua@phytium.com.cn
> > > Sent: Wednesday, January 15, 2014 1:41 PM
> > > To: u-boot@lists.denx.de
> > > Cc: trini@ti.com
> > > Subject: [U-Boot] [PATCH] arm64 patch: gicv3 support
> > >
> > > From: David Feng <fenghua@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.
> >
> >  	^^^
> > Typo - gic.h
> >
> > >   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.
> >
> > Hmmm. This seems a bit suspicious - if we reroute even IRQs and Aborts
> > to the cores which work in EL3, they will not be visible to Linux or
> > Hypervisor which work in EL2 or EL1.
> >
> These bits will be cleared when jumping to el2.

I seem to be missing this clear operation in your patch. Can you please point me to the same.

> > Have you tried to launch linux on the ARMv8 foundation model v2 with
> these changes?
> >
> Yes.

But I thought we still don't have GICv3 driver in Linux. So did you boot linux with GICv2 or GICv3?

> 
> > > Note: please use the latest gcc 4.8 compiler from linaro
> > >       which support gicv3 system register assembling.
> > >
> >
> > Two generic comments :
> >
> > - I see in the Foundation model README that the optional GICv3 is
> > supported with memory-mapped CPU interface and distributor, but I see
> > your patch accessing them via the sytem register interface (via msr and
> mrs). Is this a BUG in the README?
> >
> The document did not describe it clearly, but actually it support.

So this seems to be a documentation BUG, did you provide a feedback to the ARM support folks regarding the same
- they should probably fix the documentation.
 
> > Did you check this patch on the latest ARMv8 foundation model - v2?
> >
> Yes.
> 
> > - Also how about sharing the GICv2 coding between ARMv7 and ARMv8 -
> > some of the code may seems like a duplication from the ARMv7 GICv2
> content.
> >
> Many codes could be shared between armv7 and armv8 such as cache
> maintenance and gic.
> These need be rearranged seriously. We'd better wait a period of time
> before the armv8 codes are more widely acquainted and get more feedback
> about this.

I agree about the ARMv7/v8 code sharing, but with GICv3 there is an additional problem - it can be configured as 
GICv2 as well and for the same it would make sense to move common code b/w the CONFIG_GICV2/CONFIG_GICV3 code legs
to a common leg.

> 
> > > Signed-off-by: David Feng <fenghua@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)
> > > +	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 */
> > > +
> > > +	/* 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 */
> > > +	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 */
> > > +	isb
> > > +
> > > +	mov	x0, #0x1 << 7		/* Non-Secure access to ICC_PMR_EL1
> > > */
> > > +	msr	ICC_PMR_EL1, x0
> > > +	isb
> > > +#elif defined(CONFIG_GICV2)
> > >  	branch_if_slave	x0, 2f
> > >
> > >  	/* Initialize Distributor and SPIs */ @@ -54,7 +122,7 @@
> > > WEAK(gic_init)
> > >
> > >  	mov	w0, #0x1 << 7		/* Non-Secure access to GICC_PMR */
> > >  	str	w0, [x1, GICC_PMR]
> > > -
> > > +#endif
> > >  	ret
> > >  ENDPROC(gic_init)
> > >
> > > @@ -65,11 +133,18 @@ ENDPROC(gic_init)
> > >   *
> > >
> > > ********************************************************************
> > > *****
> > > /
> > >  WEAK(gic_send_sgi)
> > > +#if defined(CONFIG_GICV3)
> > > +	mov	x1, #(1 << 40)
> > > +	orr	x1, x1, x0, lsl #24
> > > +	msr	ICC_ASGI1R_EL1, x1
> > > +	isb
> > > +#elif defined(CONFIG_GICV2)
> > >  	ldr	x1, =GICD_BASE
> > >  	mov	w2, #0x8000
> > >  	movk	w2, #0x100, lsl #16
> > >  	orr	w2, w2, w0
> > >  	str	w2, [x1, GICD_SGIR]
> > > +#endif
> > >  	ret
> > >  ENDPROC(gic_send_sgi)
> > >
> > > @@ -82,11 +157,18 @@ ENDPROC(gic_send_sgi)
> > >   *
> > >
> > > ********************************************************************
> > > *****
> > > /
> > >  WEAK(wait_for_wakeup)
> > > +#if defined(CONFIG_GICV3)
> > > +0:	wfi
> > > +	mrs	x0, ICC_IAR1_EL1
> > > +	msr	ICC_EOIR1_EL1, x0
> > > +	cbnz	x0, 0b
> > > +#elif defined(CONFIG_GICV2)
> > >  	ldr	x1, =GICC_BASE
> > >  0:	wfi
> > >  	ldr	w0, [x1, GICC_AIAR]
> > >  	str	w0, [x1, GICC_AEOIR]
> > >  	cbnz	w0, 0b
> > > +#endif
> > >  	ret
> > >  ENDPROC(wait_for_wakeup)
> > >
> > > diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
> > > index bcc2603..9911817 100644
> > > --- a/arch/arm/cpu/armv8/start.S
> > > +++ b/arch/arm/cpu/armv8/start.S
> > > @@ -50,7 +50,10 @@ reset:
> > >  	 */
> > >  	adr	x0, vectors
> > >  	switch_el x1, 3f, 2f, 1f
> > > -3:	msr	vbar_el3, x0
> > > +3:	mrs	x0, scr_el3
> > > +	orr	x0, x0, #0xf			/* SCR_EL3.NS|IRQ|FIQ|EA */
> > > +	msr	scr_el3, x0
> > > +	msr	vbar_el3, x0
> > >  	msr	cptr_el3, xzr			/* Enable FP/SIMD */
> > >  	ldr	x0, =COUNTER_FREQUENCY
> > >  	msr	cntfrq_el0, x0			/* Initialize CNTFRQ */
> > > diff --git a/arch/arm/include/asm/gic.h b/arch/arm/include/asm/gic.h
> > > index ac2b2bf..bd3a80c 100644
> > > --- a/arch/arm/include/asm/gic.h
> > > +++ b/arch/arm/include/asm/gic.h
> > > @@ -51,4 +51,60 @@
> > >  #define GICC_IIDR		0x00fc
> > >  #define GICC_DIR		0x1000
> > >
> > > +/* ReDistributor Registers for Control and Physical LPIs */
> > > +#define GICR_CTLR		0x0000
> > > +#define GICR_IIDR		0x0004
> > > +#define GICR_TYPER		0x0008
> > > +#define GICR_STATUSR		0x0010
> > > +#define GICR_WAKER		0x0014
> > > +#define GICR_SETLPIR		0x0040
> > > +#define GICR_CLRLPIR		0x0048
> > > +#define GICR_SEIR		0x0068
> > > +#define GICR_PROPBASER		0x0070
> > > +#define GICR_PENDBASER		0x0078
> > > +#define GICR_INVLPIR		0x00a0
> > > +#define GICR_INVALLR		0x00b0
> > > +#define GICR_SYNCR		0x00c0
> > > +#define GICR_MOVLPIR		0x0100
> > > +#define GICR_MOVALLR		0x0110
> > > +
> > > +/* ReDistributor Registers for SGIs and PPIs */
> > > +#define GICR_IGROUPRn		0x0080
> > > +#define GICR_ISENABLERn		0x0100
> > > +#define GICR_ICENABLERn		0x0180
> > > +#define GICR_ISPENDRn		0x0200
> > > +#define GICR_ICPENDRn		0x0280
> > > +#define GICR_ISACTIVERn		0x0300
> > > +#define GICR_ICACTIVERn		0x0380
> > > +#define GICR_IPRIORITYRn	0x0400
> > > +#define GICR_ICFGR0		0x0c00
> > > +#define GICR_ICFGR1		0x0c04
> > > +#define GICR_IGROUPMODRn	0x0d00
> > > +#define GICR_NSACRn		0x0e00
> > > +
> > > +/* Cpu Interface System Registers */
> > > +#define ICC_IAR0_EL1		S3_0_C12_C8_0
> > > +#define ICC_IAR1_EL1		S3_0_C12_C12_0
> > > +#define ICC_EOIR0_EL1		S3_0_C12_C8_1
> > > +#define ICC_EOIR1_EL1		S3_0_C12_C12_1
> > > +#define ICC_HPPIR0_EL1		S3_0_C12_C8_2
> > > +#define ICC_HPPIR1_EL1		S3_0_C12_C12_2
> > > +#define ICC_BPR0_EL1		S3_0_C12_C8_3
> > > +#define ICC_BPR1_EL1		S3_0_C12_C12_3
> > > +#define ICC_DIR_EL1		S3_0_C12_C11_1
> > > +#define ICC_PMR_EL1		S3_0_C4_C6_0
> > > +#define ICC_RPR_EL1		S3_0_C12_C11_3
> > > +#define ICC_CTLR_EL1		S3_0_C12_C12_4
> > > +#define ICC_CTLR_EL3		S3_6_C12_C12_4
> > > +#define ICC_SRE_EL1		S3_0_C12_C12_5
> > > +#define ICC_SRE_EL2		S3_4_C12_C9_5
> > > +#define ICC_SRE_EL3		S3_6_C12_C12_5
> > > +#define ICC_IGRPEN0_EL1		S3_0_C12_C12_6
> > > +#define ICC_IGRPEN1_EL1		S3_0_C12_C12_7
> > > +#define ICC_IGRPEN1_EL3		S3_6_C12_C12_7
> > > +#define ICC_SEIEN_EL1		S3_0_C12_C13_0
> > > +#define ICC_SGI0R_EL1		S3_0_C12_C11_7
> > > +#define ICC_SGI1R_EL1		S3_0_C12_C11_5
> > > +#define ICC_ASGI1R_EL1		S3_0_C12_C11_6
> > > +
> > >  #endif /* __GIC_H__ */
> > > diff --git a/include/configs/vexpress_aemv8a.h
> > > b/include/configs/vexpress_aemv8a.h
> > > index ce5f384..ac67887 100644
> > > --- a/include/configs/vexpress_aemv8a.h
> > > +++ b/include/configs/vexpress_aemv8a.h
> > > @@ -12,6 +12,8 @@
> > >
> > >  #define CONFIG_REMAKE_ELF
> > >
> > > +#define CONFIG_GICV2
> > > +
> > >  /*#define CONFIG_ARMV8_SWITCH_TO_EL1*/
> > >
> > >  /*#define CONFIG_SYS_GENERIC_BOARD*/ @@ -93,8 +95,13 @@
> > >  #define COUNTER_FREQUENCY		(0x1800000)	/* 24MHz */
> > >
> > >  /* Generic Interrupt Controller Definitions */
> > > +#ifdef CONFIG_GICV3
> > > +#define GICD_BASE			(0x2f000000)
> > > +#define GICR_BASE			(0x2f100000)
> > > +#else
> > >  #define GICD_BASE			(0x2C001000)
> > >  #define GICC_BASE			(0x2C002000)
> > > +#endif
> > >
> > >  #define CONFIG_SYS_MEMTEST_START	V2M_BASE
> > >  #define CONFIG_SYS_MEMTEST_END		(V2M_BASE + 0x80000000)
> > > --
> > > 1.7.9.5
> > >
> >
> > Regards,
> > Bhupesh
> 
> 
> 
> 
> 
> 
>
fenghua@phytium.com.cn Jan. 16, 2014, 7:21 a.m. UTC | #4
> -----Original Messages-----
> From: "bhupesh.sharma@freescale.com" <bhupesh.sharma@freescale.com>
> Sent Time: 2014-01-16 11:45:18 (Thursday)
> To: 'FengHua' <fenghua@phytium.com.cn>
> Cc: "u-boot@lists.denx.de" <u-boot@lists.denx.de>, "trini@ti.com" <trini@ti.com>, "arnab.basu@freescale.com" <arnab.basu@freescale.com>
> Subject: RE: RE: [U-Boot] [PATCH] arm64 patch: gicv3 support
> 
> > -----Original Message-----
> > From: FengHua [mailto:fenghua@phytium.com.cn]
> > Sent: Thursday, January 16, 2014 6:47 AM
> > To: Sharma Bhupesh-B45370
> > Cc: u-boot@lists.denx.de; trini@ti.com; Basu Arnab-B45036
> > Subject: Re: RE: [U-Boot] [PATCH] arm64 patch: gicv3 support
> > 
> > 
> > hi bhupesh,
> > 
> > 
> > > -----Original Messages-----
> > > From: "bhupesh.sharma@freescale.com" <bhupesh.sharma@freescale.com>
> > > Sent Time: 2014-01-15 22:19:02 (Wednesday)
> > > To: "'fenghua@phytium.com.cn'" <fenghua@phytium.com.cn>,
> > > "u-boot@lists.denx.de" <u-boot@lists.denx.de>
> > > Cc: "trini@ti.com" <trini@ti.com>, "arnab.basu@freescale.com"
> > > <arnab.basu@freescale.com>
> > > Subject: RE: [U-Boot] [PATCH] arm64 patch: gicv3 support
> > >
> > > Hi David,
> > >
> > > Thanks for the patch.
> > > Please see some comments inline:
> > >
> > > > -----Original Message-----
> > > > From: u-boot-bounces@lists.denx.de
> > > > [mailto:u-boot-bounces@lists.denx.de]
> > > > On Behalf Of fenghua@phytium.com.cn
> > > > Sent: Wednesday, January 15, 2014 1:41 PM
> > > > To: u-boot@lists.denx.de
> > > > Cc: trini@ti.com
> > > > Subject: [U-Boot] [PATCH] arm64 patch: gicv3 support
> > > >
> > > > From: David Feng <fenghua@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.
> > >
> > >  	^^^
> > > Typo - gic.h
> > >
> > > >   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.
> > >
> > > Hmmm. This seems a bit suspicious - if we reroute even IRQs and Aborts
> > > to the cores which work in EL3, they will not be visible to Linux or
> > > Hypervisor which work in EL2 or EL1.
> > >
> > These bits will be cleared when jumping to el2.
> 
> I seem to be missing this clear operation in your patch. Can you please point me to the same.
> 
armv8_switch_to_el2 routine in transition.S.

> > > Have you tried to launch linux on the ARMv8 foundation model v2 with
> > these changes?
> > >
> > Yes.
> 
> But I thought we still don't have GICv3 driver in Linux. So did you boot linux with GICv2 or GICv3?
> 
GICv3. MAZ's gicv3 kernel git tree contains GICv3 driver.

> > 
> > > > Note: please use the latest gcc 4.8 compiler from linaro
> > > >       which support gicv3 system register assembling.
> > > >
> > >
> > > Two generic comments :
> > >
> > > - I see in the Foundation model README that the optional GICv3 is
> > > supported with memory-mapped CPU interface and distributor, but I see
> > > your patch accessing them via the sytem register interface (via msr and
> > mrs). Is this a BUG in the README?
> > >
> > The document did not describe it clearly, but actually it support.
> 
> So this seems to be a documentation BUG, did you provide a feedback to the ARM support folks regarding the same
> - they should probably fix the documentation.
I have not. Actually, I did not notice this before receiving your mail.

>  
> > > Did you check this patch on the latest ARMv8 foundation model - v2?
> > >
> > Yes.
> > 
> > > - Also how about sharing the GICv2 coding between ARMv7 and ARMv8 -
> > > some of the code may seems like a duplication from the ARMv7 GICv2
> > content.
> > >
> > Many codes could be shared between armv7 and armv8 such as cache
> > maintenance and gic.
> > These need be rearranged seriously. We'd better wait a period of time
> > before the armv8 codes are more widely acquainted and get more feedback
> > about this.
> 
> I agree about the ARMv7/v8 code sharing, but with GICv3 there is an additional problem - it can be configured as 
> GICv2 as well and for the same it would make sense to move common code b/w the CONFIG_GICV2/CONFIG_GICV3 code legs
> to a common leg.
Yes, this need more considerations.

> 
> > 
> > > > Signed-off-by: David Feng <fenghua@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)
> > > > +	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 */
> > > > +
> > > > +	/* 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 */
> > > > +	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 */
> > > > +	isb
> > > > +
> > > > +	mov	x0, #0x1 << 7		/* Non-Secure access to ICC_PMR_EL1
> > > > */
> > > > +	msr	ICC_PMR_EL1, x0
> > > > +	isb
> > > > +#elif defined(CONFIG_GICV2)
> > > >  	branch_if_slave	x0, 2f
> > > >
> > > >  	/* Initialize Distributor and SPIs */ @@ -54,7 +122,7 @@
> > > > WEAK(gic_init)
> > > >
> > > >  	mov	w0, #0x1 << 7		/* Non-Secure access to GICC_PMR */
> > > >  	str	w0, [x1, GICC_PMR]
> > > > -
> > > > +#endif
> > > >  	ret
> > > >  ENDPROC(gic_init)
> > > >
> > > > @@ -65,11 +133,18 @@ ENDPROC(gic_init)
> > > >   *
> > > >
> > > > ********************************************************************
> > > > *****
> > > > /
> > > >  WEAK(gic_send_sgi)
> > > > +#if defined(CONFIG_GICV3)
> > > > +	mov	x1, #(1 << 40)
> > > > +	orr	x1, x1, x0, lsl #24
> > > > +	msr	ICC_ASGI1R_EL1, x1
> > > > +	isb
> > > > +#elif defined(CONFIG_GICV2)
> > > >  	ldr	x1, =GICD_BASE
> > > >  	mov	w2, #0x8000
> > > >  	movk	w2, #0x100, lsl #16
> > > >  	orr	w2, w2, w0
> > > >  	str	w2, [x1, GICD_SGIR]
> > > > +#endif
> > > >  	ret
> > > >  ENDPROC(gic_send_sgi)
> > > >
> > > > @@ -82,11 +157,18 @@ ENDPROC(gic_send_sgi)
> > > >   *
> > > >
> > > > ********************************************************************
> > > > *****
> > > > /
> > > >  WEAK(wait_for_wakeup)
> > > > +#if defined(CONFIG_GICV3)
> > > > +0:	wfi
> > > > +	mrs	x0, ICC_IAR1_EL1
> > > > +	msr	ICC_EOIR1_EL1, x0
> > > > +	cbnz	x0, 0b
> > > > +#elif defined(CONFIG_GICV2)
> > > >  	ldr	x1, =GICC_BASE
> > > >  0:	wfi
> > > >  	ldr	w0, [x1, GICC_AIAR]
> > > >  	str	w0, [x1, GICC_AEOIR]
> > > >  	cbnz	w0, 0b
> > > > +#endif
> > > >  	ret
> > > >  ENDPROC(wait_for_wakeup)
> > > >
> > > > diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
> > > > index bcc2603..9911817 100644
> > > > --- a/arch/arm/cpu/armv8/start.S
> > > > +++ b/arch/arm/cpu/armv8/start.S
> > > > @@ -50,7 +50,10 @@ reset:
> > > >  	 */
> > > >  	adr	x0, vectors
> > > >  	switch_el x1, 3f, 2f, 1f
> > > > -3:	msr	vbar_el3, x0
> > > > +3:	mrs	x0, scr_el3
> > > > +	orr	x0, x0, #0xf			/* SCR_EL3.NS|IRQ|FIQ|EA */
> > > > +	msr	scr_el3, x0
> > > > +	msr	vbar_el3, x0
> > > >  	msr	cptr_el3, xzr			/* Enable FP/SIMD */
> > > >  	ldr	x0, =COUNTER_FREQUENCY
> > > >  	msr	cntfrq_el0, x0			/* Initialize CNTFRQ */
> > > > diff --git a/arch/arm/include/asm/gic.h b/arch/arm/include/asm/gic.h
> > > > index ac2b2bf..bd3a80c 100644
> > > > --- a/arch/arm/include/asm/gic.h
> > > > +++ b/arch/arm/include/asm/gic.h
> > > > @@ -51,4 +51,60 @@
> > > >  #define GICC_IIDR		0x00fc
> > > >  #define GICC_DIR		0x1000
> > > >
> > > > +/* ReDistributor Registers for Control and Physical LPIs */
> > > > +#define GICR_CTLR		0x0000
> > > > +#define GICR_IIDR		0x0004
> > > > +#define GICR_TYPER		0x0008
> > > > +#define GICR_STATUSR		0x0010
> > > > +#define GICR_WAKER		0x0014
> > > > +#define GICR_SETLPIR		0x0040
> > > > +#define GICR_CLRLPIR		0x0048
> > > > +#define GICR_SEIR		0x0068
> > > > +#define GICR_PROPBASER		0x0070
> > > > +#define GICR_PENDBASER		0x0078
> > > > +#define GICR_INVLPIR		0x00a0
> > > > +#define GICR_INVALLR		0x00b0
> > > > +#define GICR_SYNCR		0x00c0
> > > > +#define GICR_MOVLPIR		0x0100
> > > > +#define GICR_MOVALLR		0x0110
> > > > +
> > > > +/* ReDistributor Registers for SGIs and PPIs */
> > > > +#define GICR_IGROUPRn		0x0080
> > > > +#define GICR_ISENABLERn		0x0100
> > > > +#define GICR_ICENABLERn		0x0180
> > > > +#define GICR_ISPENDRn		0x0200
> > > > +#define GICR_ICPENDRn		0x0280
> > > > +#define GICR_ISACTIVERn		0x0300
> > > > +#define GICR_ICACTIVERn		0x0380
> > > > +#define GICR_IPRIORITYRn	0x0400
> > > > +#define GICR_ICFGR0		0x0c00
> > > > +#define GICR_ICFGR1		0x0c04
> > > > +#define GICR_IGROUPMODRn	0x0d00
> > > > +#define GICR_NSACRn		0x0e00
> > > > +
> > > > +/* Cpu Interface System Registers */
> > > > +#define ICC_IAR0_EL1		S3_0_C12_C8_0
> > > > +#define ICC_IAR1_EL1		S3_0_C12_C12_0
> > > > +#define ICC_EOIR0_EL1		S3_0_C12_C8_1
> > > > +#define ICC_EOIR1_EL1		S3_0_C12_C12_1
> > > > +#define ICC_HPPIR0_EL1		S3_0_C12_C8_2
> > > > +#define ICC_HPPIR1_EL1		S3_0_C12_C12_2
> > > > +#define ICC_BPR0_EL1		S3_0_C12_C8_3
> > > > +#define ICC_BPR1_EL1		S3_0_C12_C12_3
> > > > +#define ICC_DIR_EL1		S3_0_C12_C11_1
> > > > +#define ICC_PMR_EL1		S3_0_C4_C6_0
> > > > +#define ICC_RPR_EL1		S3_0_C12_C11_3
> > > > +#define ICC_CTLR_EL1		S3_0_C12_C12_4
> > > > +#define ICC_CTLR_EL3		S3_6_C12_C12_4
> > > > +#define ICC_SRE_EL1		S3_0_C12_C12_5
> > > > +#define ICC_SRE_EL2		S3_4_C12_C9_5
> > > > +#define ICC_SRE_EL3		S3_6_C12_C12_5
> > > > +#define ICC_IGRPEN0_EL1		S3_0_C12_C12_6
> > > > +#define ICC_IGRPEN1_EL1		S3_0_C12_C12_7
> > > > +#define ICC_IGRPEN1_EL3		S3_6_C12_C12_7
> > > > +#define ICC_SEIEN_EL1		S3_0_C12_C13_0
> > > > +#define ICC_SGI0R_EL1		S3_0_C12_C11_7
> > > > +#define ICC_SGI1R_EL1		S3_0_C12_C11_5
> > > > +#define ICC_ASGI1R_EL1		S3_0_C12_C11_6
> > > > +
> > > >  #endif /* __GIC_H__ */
> > > > diff --git a/include/configs/vexpress_aemv8a.h
> > > > b/include/configs/vexpress_aemv8a.h
> > > > index ce5f384..ac67887 100644
> > > > --- a/include/configs/vexpress_aemv8a.h
> > > > +++ b/include/configs/vexpress_aemv8a.h
> > > > @@ -12,6 +12,8 @@
> > > >
> > > >  #define CONFIG_REMAKE_ELF
> > > >
> > > > +#define CONFIG_GICV2
> > > > +
> > > >  /*#define CONFIG_ARMV8_SWITCH_TO_EL1*/
> > > >
> > > >  /*#define CONFIG_SYS_GENERIC_BOARD*/ @@ -93,8 +95,13 @@
> > > >  #define COUNTER_FREQUENCY		(0x1800000)	/* 24MHz */
> > > >
> > > >  /* Generic Interrupt Controller Definitions */
> > > > +#ifdef CONFIG_GICV3
> > > > +#define GICD_BASE			(0x2f000000)
> > > > +#define GICR_BASE			(0x2f100000)
> > > > +#else
> > > >  #define GICD_BASE			(0x2C001000)
> > > >  #define GICC_BASE			(0x2C002000)
> > > > +#endif
> > > >
> > > >  #define CONFIG_SYS_MEMTEST_START	V2M_BASE
> > > >  #define CONFIG_SYS_MEMTEST_END		(V2M_BASE + 0x80000000)
> > > > --
> > > > 1.7.9.5
> > > >
> > >
> > > Regards,
> > > Bhupesh
> > 
> > 
> > 
> > 
> > 
> > 
> > 
>
Arnab Basu Feb. 7, 2014, 10:01 a.m. UTC | #5
Hi David

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

On 15-Jan-14 1:40 PM, fenghua@phytium.com.cn wrote:
> From: David Feng <fenghua@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@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
Arnab Basu Feb. 10, 2014, 10:29 a.m. UTC | #6
Hi David

On 10-Feb-14 1:41 PM, FengHua wrote:
> 
>>> +	/* 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?
>>
> Yes, the redistributor could be initialized by it's corresponding processor.
> But, how could we determine the correspondence of redistributors and processors?
> It will be implementation specific and the code will be a little more complicated.
> 

Each processor should compare the affinity encoded in the GICR_TYPER[63:32] bits for all redistributors 
with the affinity encoded in its own MPIDR_EL1 and only initialize the matching redistributor.
Why will this be implementation specific?

Yes code will possibly be more complicated but it will be much more robust, consider the case when
all CPUs are not being brought out of reset simultaneously. In this case we might not want to wake
up all the redistributors.

Thanks
Arnab
Albert ARIBAUD Feb. 21, 2014, 4:38 p.m. UTC | #7
Hi fenghua@phytium.com.cn,

On Wed, 15 Jan 2014 16:10:56 +0800, fenghua@phytium.com.cn wrote:

> From: David Feng <fenghua@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.
> Note: please use the latest gcc 4.8 compiler from linaro 
>       which support gicv3 system register assembling.
> 
> Signed-off-by: David Feng <fenghua@phytium.com.cn>
> ---

I am not well-versed enough in aarch64 and GIC, be it v2 or v3, so...
Is this patch OK as it is, or should a V2 follow with common GIVv2/GICv3
code merged?

Amicalement,
fenghua@phytium.com.cn Feb. 24, 2014, 1:22 p.m. UTC | #8
> -----Original Messages-----
> From: "Albert ARIBAUD" <albert.u.boot@aribaud.net>
> Sent Time: 2014-02-22 00:38:05 (Saturday)
> To: fenghua@phytium.com.cn
> Cc: u-boot@lists.denx.de, trini@ti.com
> Subject: Re: [PATCH] arm64 patch: gicv3 support
> 
> Hi fenghua@phytium.com.cn,
> 
> On Wed, 15 Jan 2014 16:10:56 +0800, fenghua@phytium.com.cn wrote:
> 
> > From: David Feng <fenghua@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.
> > Note: please use the latest gcc 4.8 compiler from linaro 
> >       which support gicv3 system register assembling.
> > 
> > Signed-off-by: David Feng <fenghua@phytium.com.cn>
> > ---
> 
> I am not well-versed enough in aarch64 and GIC, be it v2 or v3, so...
> Is this patch OK as it is, or should a V2 follow with common GIVv2/GICv3
> code merged?
> 
> Amicalement,

hi Albert,
      I am preparing the V2, there are some optimizations. But it's still Armv8 specific.
I would like to consider common Gicv2/Gicv3 code working across armv7 and armv8. 
It's not easy and I have no armv7 test environment.
      Thank you!

Best Regards,
David
diff mbox

Patch

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)
+	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 */
+
+	/* 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 */
+	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 */
+	isb
+
+	mov	x0, #0x1 << 7		/* Non-Secure access to ICC_PMR_EL1 */
+	msr	ICC_PMR_EL1, x0
+	isb
+#elif defined(CONFIG_GICV2)
 	branch_if_slave	x0, 2f
 
 	/* Initialize Distributor and SPIs */
@@ -54,7 +122,7 @@  WEAK(gic_init)
 
 	mov	w0, #0x1 << 7		/* Non-Secure access to GICC_PMR */
 	str	w0, [x1, GICC_PMR]
-
+#endif
 	ret
 ENDPROC(gic_init)
 
@@ -65,11 +133,18 @@  ENDPROC(gic_init)
  *
  *************************************************************************/
 WEAK(gic_send_sgi)
+#if defined(CONFIG_GICV3)
+	mov	x1, #(1 << 40)
+	orr	x1, x1, x0, lsl #24
+	msr	ICC_ASGI1R_EL1, x1
+	isb
+#elif defined(CONFIG_GICV2)
 	ldr	x1, =GICD_BASE
 	mov	w2, #0x8000
 	movk	w2, #0x100, lsl #16
 	orr	w2, w2, w0
 	str	w2, [x1, GICD_SGIR]
+#endif
 	ret
 ENDPROC(gic_send_sgi)
 
@@ -82,11 +157,18 @@  ENDPROC(gic_send_sgi)
  *
  *************************************************************************/
 WEAK(wait_for_wakeup)
+#if defined(CONFIG_GICV3)
+0:	wfi
+	mrs	x0, ICC_IAR1_EL1
+	msr	ICC_EOIR1_EL1, x0
+	cbnz	x0, 0b
+#elif defined(CONFIG_GICV2)
 	ldr	x1, =GICC_BASE
 0:	wfi
 	ldr	w0, [x1, GICC_AIAR]
 	str	w0, [x1, GICC_AEOIR]
 	cbnz	w0, 0b
+#endif
 	ret
 ENDPROC(wait_for_wakeup)
 
diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
index bcc2603..9911817 100644
--- a/arch/arm/cpu/armv8/start.S
+++ b/arch/arm/cpu/armv8/start.S
@@ -50,7 +50,10 @@  reset:
 	 */
 	adr	x0, vectors
 	switch_el x1, 3f, 2f, 1f
-3:	msr	vbar_el3, x0
+3:	mrs	x0, scr_el3
+	orr	x0, x0, #0xf			/* SCR_EL3.NS|IRQ|FIQ|EA */
+	msr	scr_el3, x0
+	msr	vbar_el3, x0
 	msr	cptr_el3, xzr			/* Enable FP/SIMD */
 	ldr	x0, =COUNTER_FREQUENCY
 	msr	cntfrq_el0, x0			/* Initialize CNTFRQ */
diff --git a/arch/arm/include/asm/gic.h b/arch/arm/include/asm/gic.h
index ac2b2bf..bd3a80c 100644
--- a/arch/arm/include/asm/gic.h
+++ b/arch/arm/include/asm/gic.h
@@ -51,4 +51,60 @@ 
 #define GICC_IIDR		0x00fc
 #define GICC_DIR		0x1000
 
+/* ReDistributor Registers for Control and Physical LPIs */
+#define GICR_CTLR		0x0000
+#define GICR_IIDR		0x0004
+#define GICR_TYPER		0x0008
+#define GICR_STATUSR		0x0010
+#define GICR_WAKER		0x0014
+#define GICR_SETLPIR		0x0040
+#define GICR_CLRLPIR		0x0048
+#define GICR_SEIR		0x0068
+#define GICR_PROPBASER		0x0070
+#define GICR_PENDBASER		0x0078
+#define GICR_INVLPIR		0x00a0
+#define GICR_INVALLR		0x00b0
+#define GICR_SYNCR		0x00c0
+#define GICR_MOVLPIR		0x0100
+#define GICR_MOVALLR		0x0110
+
+/* ReDistributor Registers for SGIs and PPIs */
+#define GICR_IGROUPRn		0x0080
+#define GICR_ISENABLERn		0x0100
+#define GICR_ICENABLERn		0x0180
+#define GICR_ISPENDRn		0x0200
+#define GICR_ICPENDRn		0x0280
+#define GICR_ISACTIVERn		0x0300
+#define GICR_ICACTIVERn		0x0380
+#define GICR_IPRIORITYRn	0x0400
+#define GICR_ICFGR0		0x0c00
+#define GICR_ICFGR1		0x0c04
+#define GICR_IGROUPMODRn	0x0d00
+#define GICR_NSACRn		0x0e00
+
+/* Cpu Interface System Registers */
+#define ICC_IAR0_EL1		S3_0_C12_C8_0
+#define ICC_IAR1_EL1		S3_0_C12_C12_0
+#define ICC_EOIR0_EL1		S3_0_C12_C8_1
+#define ICC_EOIR1_EL1		S3_0_C12_C12_1
+#define ICC_HPPIR0_EL1		S3_0_C12_C8_2
+#define ICC_HPPIR1_EL1		S3_0_C12_C12_2
+#define ICC_BPR0_EL1		S3_0_C12_C8_3
+#define ICC_BPR1_EL1		S3_0_C12_C12_3
+#define ICC_DIR_EL1		S3_0_C12_C11_1
+#define ICC_PMR_EL1		S3_0_C4_C6_0
+#define ICC_RPR_EL1		S3_0_C12_C11_3
+#define ICC_CTLR_EL1		S3_0_C12_C12_4
+#define ICC_CTLR_EL3		S3_6_C12_C12_4
+#define ICC_SRE_EL1		S3_0_C12_C12_5
+#define ICC_SRE_EL2		S3_4_C12_C9_5
+#define ICC_SRE_EL3		S3_6_C12_C12_5
+#define ICC_IGRPEN0_EL1		S3_0_C12_C12_6
+#define ICC_IGRPEN1_EL1		S3_0_C12_C12_7
+#define ICC_IGRPEN1_EL3		S3_6_C12_C12_7
+#define ICC_SEIEN_EL1		S3_0_C12_C13_0
+#define ICC_SGI0R_EL1		S3_0_C12_C11_7
+#define ICC_SGI1R_EL1		S3_0_C12_C11_5
+#define ICC_ASGI1R_EL1		S3_0_C12_C11_6
+
 #endif /* __GIC_H__ */
diff --git a/include/configs/vexpress_aemv8a.h b/include/configs/vexpress_aemv8a.h
index ce5f384..ac67887 100644
--- a/include/configs/vexpress_aemv8a.h
+++ b/include/configs/vexpress_aemv8a.h
@@ -12,6 +12,8 @@ 
 
 #define CONFIG_REMAKE_ELF
 
+#define CONFIG_GICV2
+
 /*#define CONFIG_ARMV8_SWITCH_TO_EL1*/
 
 /*#define CONFIG_SYS_GENERIC_BOARD*/
@@ -93,8 +95,13 @@ 
 #define COUNTER_FREQUENCY		(0x1800000)	/* 24MHz */
 
 /* Generic Interrupt Controller Definitions */
+#ifdef CONFIG_GICV3
+#define GICD_BASE			(0x2f000000)
+#define GICR_BASE			(0x2f100000)
+#else
 #define GICD_BASE			(0x2C001000)
 #define GICC_BASE			(0x2C002000)
+#endif
 
 #define CONFIG_SYS_MEMTEST_START	V2M_BASE
 #define CONFIG_SYS_MEMTEST_END		(V2M_BASE + 0x80000000)