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