Hi: york
Best Regards Wenbin Song
-----Original Message----- From: york sun Sent: Wednesday, October 26, 2016 4:35 AM To: Wenbin Song wenbin.song@nxp.com; albert.u.boot@aribaud.net; Mingkai Hu mingkai.hu@nxp.com; u-boot@lists.denx.de Subject: Re: [PATCH v4 1/2] armv8/ls1043a: fixup GIC offset according to SVR and SCFG_GIC400_ALIGN[GIC_ADDR_BIT]
Overriding the weekly smp_kick_all_cpus, the new impletment is able to detect GIC offset.
I think you meant "weak" here. :)
[wenbin] sorry, this is my carelessness.
The default GIC offset in kernel device tree is using 64K alignment, it need to be fixed if 4K alignment is detected.
The "default" offset in device tree is also created by us, isn't it? I am not against you fixing it. Don't you want to check the alignment first? If the device tree has 4K alignment but you run on rev 1.1, do you want to use 64K alignment?
[wenbin] Yes, I will modify them.
- if (val == REV1_1) {
This is problematic. How about for future SoCs, or other than LS1043A? Can we just check GIC_ADDR_BIT?
[wenbin] it is not clear about the future revise ,including whether has the new revise, whether the new revise supports GIC 4K alignment and how to detect it. So I could only suppose the future revise is as same as rev1.1. Therefore, I will modify it to " if (val != REV1_0) {" in next version.
val = scfg_in32(&scfg->gic_align) & (0x01 << GIC_ADDR_BIT);if (!val)return;- }
- offset = fdt_subnode_offset(blob, 0, "interrupt-controller@1400000");
- if (offset < 0) {
printf("WARNING: fdt_subnode_offset can't findnode %s: %s\n",
"interrupt-controller@1400000", fdt_strerror(offset));return;- }
- reg[0] = cpu_to_fdt64(GICD_BASE_4K);
- reg[1] = cpu_to_fdt64(GICD_SIZE_4K);
- reg[2] = cpu_to_fdt64(GICC_BASE_4K);
- reg[3] = cpu_to_fdt64(GICC_SIZE_4K);
- reg[4] = cpu_to_fdt64(GICH_BASE_4K);
- reg[5] = cpu_to_fdt64(GICH_SIZE_4K);
- reg[6] = cpu_to_fdt64(GICV_BASE_4K);
- reg[7] = cpu_to_fdt64(GICV_SIZE_4K);
- err = fdt_setprop(blob, offset, "reg", reg, sizeof(reg));
- if (err < 0) {
printf("WARNING: fdt_setprop can't set %s fromnode %s: %s\n",
"reg", "interrupt-controller@1400000",fdt_strerror(err));return;- }
- return;
+} +#endif
void ft_cpu_setup(void *blob, bd_t *bd) { #ifdef CONFIG_FSL_LSCH2 @@ -170,4 +216,7 @@ void ft_cpu_setup(void *blob, bd_t *bd) #endif fsl_fdt_disable_usb(blob);
+#ifdef CONFIG_HAS_FEATURE_GIC4K_ALIGN
- fdt_fixup_gic(blob);
+#endif } diff --git a/arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S b/arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S index 5d0b7a4..e2b8698 100644 --- a/arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S +++ b/arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S @@ -14,6 +14,48 @@ #include <asm/arch/mp.h> #endif
+#ifdef CONFIG_HAS_FEATURE_GIC4K_ALIGN
+/* fixup GIC offset +* For LS1043a rev1.0, GIC base address align with 4k. +* For LS1043a rev1.1, if DCFG_GIC400_ALIGN[GIC_ADDR_BIT] +* is set, GIC base address align with 4K, or else align +* with 64k. +* output: +* x0: the base address of GICD +* x1: the base address of GICC +*/ +ENTRY(fix_gic_offset)
- ldr x0, =GICD_BASE
- ldr x1, =GICC_BASE
- ldr x3, =DCFG_CCSR_SVR
- ldr w3, [x3]
- rev w3, w3
- ands w3, w3, #0xff
- cmp w3, #REV1_0
- b.eq 1f
- ldr x3, =SCFG_GIC400_ALIGN
- ldr w3, [x3]
- rev w3, w3
- tbnz w3, #GIC_ADDR_BIT, 1f
- ret
+1:
- ldr x0, =GICD_BASE_4K
- ldr x1, =GICC_BASE_4K
- ret
+ENDPROC(fix_gic_offset)
The more I read it, the more I think the function name should be called get_gic_offset. You are not fixing it, the return value is the correct gic offset.
[wenbin] ok, it will be modified
+ENTRY(smp_kick_all_cpus)
- /* Kick secondary cpus up by SGI 0 interrupt */
- mov x29, lr /* Save LR */
- bl fix_gic_offset
- bl gic_kick_secondary_cpus
- mov lr, x29 /* Restore LR */
- ret
+ENDPROC(smp_kick_all_cpus)
Another way to do this is to implement a weak get_gic_offset function in start.S to return GICD_BASE. You can implement another function here to return correct offset. If you want to replace smp_kick_all_cpus, you may want to keep the #if condition to check CONFIG_GICV2 or CONFIG_GICV3, in case we need to debug without GIC.
[wenbin] hi, Could you explain what's the mean of "debug without GIC" ? The HAS_FEATURE_GIC4K_ALIGN is a feature(or, to be more exact, it is a bug for ls1043a rev1.0 silicon ) only belonging to ls1043a. So if ARCH_LS1043A is selected , the CONFIG_GICV2 will be selected, too.
+#endif
ENTRY(lowlevel_init) mov x29, lr /* Save LR */
@@ -105,15 +147,23 @@ ENTRY(lowlevel_init) /* Initialize GIC Secure Bank Status */ #if defined(CONFIG_GICV2) || defined(CONFIG_GICV3) branch_if_slave x0, 1f +#ifdef CONFIG_HAS_FEATURE_GIC4K_ALIGN
- bl fix_gic_offset
+#else ldr x0, =GICD_BASE +#endif
If you use get_gic_offset, you can get rid of this #ifdef.
[Wenbin] I will modify the get_gic_offset as a general function for fsl-layerscape.
bl gic_init_secure 1: #ifdef CONFIG_GICV3 ldr x0, =GICR_BASE bl gic_init_secure_percpu #elif defined(CONFIG_GICV2) +#ifdef CONFIG_HAS_FEATURE_GIC4K_ALIGN +#define GICD_BASE_4K 0x01401000 +#define GICC_BASE_4K 0x01402000 +#define GICH_BASE_4K 0x01404000 +#define GICV_BASE_4K 0x01406000
Are you using space instead of tab here?
[wenbin] ok, it will be modified.
+#define GICD_SIZE_4K 0x1000 +#define GICC_SIZE_4K 0x2000 +#define GICH_SIZE_4K 0x2000 +#define GICV_SIZE_4K 0x2000 +#endif
This is a bit odd. You have rev 1.0 first, which uses 4K alignment. Yet you have kernel device tree using 64K alignment. How did it work before?
Now you have rev 1.1, you make 64K as default, and replace it with 4K if a bit is set, or rev 1.0 SoC is detected. You are not changing anything actually, but to return the correct offset for the three situations, i.e. rev 1.0 SoC, rev 1.1 SoC with the bit cleared, rev 1.1 SoC with bit set.
The only "fix" part in this patch is to fixup device tree. I suggest you read the device tree to determine which alignment you have there, and update accordingly, for both 4K and 64K alignment. I don't think you presumption of 64K will stand.
[Wenbin]
Yes, I will modify them.
If GIC_ADDR_BIT is always set for SoCs with 4K alignment, you can get rid of the Kconfig option.
[Wenbin]
The GIC_ADDR_BIT is set or cleaned by PBI command. It is added on rev1.1 silicon.
York