[U-Boot] [PATCH v4 1/2] armv8/ls1043a: fixup GIC offset according to SVR and SCFG_GIC400_ALIGN[GIC_ADDR_BIT]

Wed Oct 26 12:39:45 CEST 2016

Hi: york

Best Regards
Wenbin Song

> -----Original Message-----
> From: york sun
> Sent: Wednesday, October 26, 2016 4:35 AM
> To: Wenbin Song <wenbin.song at nxp.com>; albert.u.boot at aribaud.net;
> Mingkai Hu <mingkai.hu at nxp.com>; u-boot at 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 at 1400000");
> > +	if (offset < 0) {
> > +		printf("WARNING: fdt_subnode_offset can't find
> node %s: %s\n",
> > +		       "interrupt-controller at 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 at 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