Hi: york Best Regards Wenbin Song
> -----Original Message----- > From: york sun > Sent: Wednesday, October 26, 2016 4:35 AM > To: Wenbin Song <wenbin.s...@nxp.com>; albert.u.b...@aribaud.net; > Mingkai Hu <mingkai...@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 find > node %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 from > node %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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot