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

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

[Wenbin song]

The LS1043A rev1.1 silicon supports two types of GIC offset: 4K alignment
and 64K alignment. The bit SCFG_GIC400_ALIGN[GIC_ADDR_BIT] is used to choose
which offset will be used.

The LS1043A rev1.0 silicon only supports the CIG offset with 4K alignment.

If GIC_ADDR_BIT bit is set, 4K alignment is used, or else 64K alignment is used.
64K alignment is the default setting.

Overriding the weekly smp_kick_all_cpus, the new impletment is able to detect
GIC offset.

The default GIC offset in kernel device tree is using 64K alignment, it
need to be fixed if 4K alignment is detected.

Signed-off-by: Wenbin Song <wenbin.song@nxp.com>
Signed-off-by: Mingkai Hu <mingkai.hu@nxp.com>
---
Changes in v4:
	Squash [patch 2/3 v3] with this patch.
	Add comments on fix_gic_offest.
	Add the descriptions of rev1.0 GIC offset.
	Use macros to define the offset and size of GIC components.  
Changes in v3:
	Add descriptions about smp_kick_all_cpus on commit message.
	Rename the macros on commit message to match with them used in the change.
	Replace CONFIG_LS1043A with HAS_FEATURE_GIC4K_ALIGN.
Changes in v2:
	None
---
 arch/arm/cpu/armv8/fsl-layerscape/Kconfig          |  4 ++
 arch/arm/cpu/armv8/fsl-layerscape/fdt.c            | 49 +++++++++++++++++++
 arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S       | 55 ++++++++++++++++++++++
 arch/arm/include/asm/arch-fsl-layerscape/config.h  | 20 +++++++-
 .../include/asm/arch-fsl-layerscape/immap_lsch2.h  |  3 +-
 5 files changed, 128 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/armv8/fsl-layerscape/Kconfig b/arch/arm/cpu/armv8/fsl-layerscape/Kconfig
index 94ec8d5..c66c497 100644
--- a/arch/arm/cpu/armv8/fsl-layerscape/Kconfig
+++ b/arch/arm/cpu/armv8/fsl-layerscape/Kconfig
@@ -12,6 +12,7 @@ config ARCH_LS1043A
 	select SYS_FSL_DDR_VER_50
 	select SYS_FSL_ERRATUM_A010315
 	select SYS_FSL_ERRATUM_A010539
+	select HAS_FEATURE_GIC4K_ALIGN
 
 config ARCH_LS1046A
 	bool
@@ -135,4 +136,7 @@ config SYS_FSL_DDR4
 	help
 	  Enable Freescale DDR4 controller.
 
+config HAS_FEATURE_GIC4K_ALIGN
+       bool
+
 endmenu
diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
index 1a8321b..ebc7863 100644
--- a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
+++ b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
@@ -126,6 +126,52 @@ void fsl_fdt_disable_usb(void *blob)
 	}
 }
 
+#ifdef CONFIG_HAS_FEATURE_GIC4K_ALIGN
+/* Fixup gic node align with 4K */
+static void fdt_fixup_gic(void *blob)
+{
+	int offset, err;
+	u64 reg[8];
+	struct ccsr_gur __iomem *gur = (void *)(CONFIG_SYS_FSL_GUTS_ADDR);
+	unsigned int val;
+	struct ccsr_scfg __iomem *scfg = (void *)CONFIG_SYS_FSL_SCFG_ADDR;
+
+	val = gur_in32(&gur->svr) & 0xff;
+
+	if (val == REV1_1) {
+		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@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)
+
+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)
+
+#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
 	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
+	bl	fix_gic_offset
+#else
 	ldr	x0, =GICD_BASE
 	ldr	x1, =GICC_BASE
+#endif
 	bl	gic_init_secure_percpu
 #endif
 #endif
@@ -335,7 +385,12 @@ ENTRY(secondary_boot_func)
 #if defined(CONFIG_GICV3)
 	gic_wait_for_interrupt_m x0
 #elif defined(CONFIG_GICV2)
+#ifdef CONFIG_HAS_FEATURE_GIC4K_ALIGN
+	bl	fix_gic_offset
+	mov	x0, x1
+#else
         ldr     x0, =GICC_BASE
+#endif
         gic_wait_for_interrupt_m x0, w1
 #endif
 
diff --git a/arch/arm/include/asm/arch-fsl-layerscape/config.h b/arch/arm/include/asm/arch-fsl-layerscape/config.h
index 4201e0f..47e21c5 100644
--- a/arch/arm/include/asm/arch-fsl-layerscape/config.h
+++ b/arch/arm/include/asm/arch-fsl-layerscape/config.h
@@ -172,8 +172,24 @@
 #define SMMU_BASE		0x09000000
 
 /* Generic Interrupt Controller Definitions */
-#define GICD_BASE		0x01401000
-#define GICC_BASE		0x01402000
+#define GICD_BASE		0x01410000
+#define GICC_BASE		0x01420000
+#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
+#define GICD_SIZE_4K		0x1000
+#define GICC_SIZE_4K		0x2000
+#define GICH_SIZE_4K		0x2000
+#define GICV_SIZE_4K		0x2000
+#endif
+
+#define DCFG_CCSR_SVR		0x1ee00a4
+#define REV1_0			0x10
+#define REV1_1			0x11
+#define GIC_ADDR_BIT		31
+#define SCFG_GIC400_ALIGN	0x1570188
 
 #define CONFIG_SYS_FSL_ERRATUM_A008850
 #define CONFIG_SYS_FSL_ERRATUM_A009663
diff --git a/arch/arm/include/asm/arch-fsl-layerscape/immap_lsch2.h b/arch/arm/include/asm/arch-fsl-layerscape/immap_lsch2.h
index d88543d..1dfef53 100644
--- a/arch/arm/include/asm/arch-fsl-layerscape/immap_lsch2.h
+++ b/arch/arm/include/asm/arch-fsl-layerscape/immap_lsch2.h
@@ -359,7 +359,8 @@ struct ccsr_scfg {
 	u32 qspi_cfg;
 	u8 res_160[0x180-0x160];
 	u32 dmamcr;
-	u8 res_184[0x18c-0x184];
+	u8 res_184[0x188-0x184];
+	u32 gic_align;
 	u32 debug_icid;
 	u8 res_190[0x1a4-0x190];
 	u32 snpcnfgcr;
-- 
2.1.0.27.g96db324

[U-Boot] [PATCH v4 2/2] armv8/fsl-layerscape: fdt: fixup LS1043A rev1 MSI node

[Wenbin song]

The default MSI node in kernel tree is for LS1043A rev1.1 silicon,
if rev1.0 silicon used, need to fixup the MSI node to match it.

Signed-off-by: Wenbin Song <wenbin.song@nxp.com>
Signed-off-by: Mingkai Hu <mingkai.hu@nxp.com>
---
Changes in v4:
	None
Changes in v3:
	Replace CONFIG_LS1043A with HAS_FEATURE_ENHACED_MSI.
Changes in v2:
	None
---
 arch/arm/cpu/armv8/fsl-layerscape/Kconfig |  3 +
 arch/arm/cpu/armv8/fsl-layerscape/fdt.c   | 94 +++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+)

diff --git a/arch/arm/cpu/armv8/fsl-layerscape/Kconfig b/arch/arm/cpu/armv8/fsl-layerscape/Kconfig
index c66c497..d2537bb 100644
--- a/arch/arm/cpu/armv8/fsl-layerscape/Kconfig
+++ b/arch/arm/cpu/armv8/fsl-layerscape/Kconfig
@@ -13,6 +13,7 @@ config ARCH_LS1043A
 	select SYS_FSL_ERRATUM_A010315
 	select SYS_FSL_ERRATUM_A010539
 	select HAS_FEATURE_GIC4K_ALIGN
+	select HAS_FEATURE_ENHANCED_MSI
 
 config ARCH_LS1046A
 	bool
@@ -138,5 +139,7 @@ config SYS_FSL_DDR4
 
 config HAS_FEATURE_GIC4K_ALIGN
        bool
+config HAS_FEATURE_ENHANCED_MSI
+       bool
 
 endmenu
diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
index ebc7863..90114b2 100644
--- a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
+++ b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
@@ -171,6 +171,97 @@ static void fdt_fixup_gic(void *blob)
 	return;
 }
 #endif
+#ifdef CONFIG_HAS_FEATURE_ENHANCED_MSI
+static int _fdt_fixup_msi_subnode(void *blob, int parentoffset,
+				const char *name, int irq_no)
+{
+	int err, offset;
+	u32 tmp[3];
+
+	offset = fdt_subnode_offset(blob, parentoffset, name);
+	if (offset < 0) {
+		printf("WARNING: fdt_subnode_offset can't find %s: %s\n",
+		       name, fdt_strerror(offset));
+		return 0;
+	}
+
+	tmp[0] = cpu_to_fdt32(0x0);
+	tmp[1] = cpu_to_fdt32(irq_no);
+	tmp[2] = cpu_to_fdt32(0x4);
+
+	err = fdt_setprop(blob, offset, "interrupts", tmp, sizeof(tmp));
+	if (err < 0) {
+		printf("WARNING: fdt_setprop can't set %s from node %s: %s\n",
+		       "interrupts", name, fdt_strerror(err));
+		return 0;
+	}
+
+	return 1;
+}
+
+static int _fdt_fixup_pci_msi(void *blob, const char *name)
+{
+	int offset, len, err;
+	void *p;
+	int val;
+	u32 tmp[4][8];
+
+	offset = fdt_path_offset(blob, name);
+	if (offset < 0) {
+		printf("WARNING: fdt_path_offset can't find path %s: %s\n",
+		       name, fdt_strerror(offset));
+		return 0;
+	}
+
+	p = (char *)fdt_getprop(blob, offset, "interrupt-map", &len);
+	if (!p || len != sizeof(tmp)) {
+		printf("WARNING: fdt_getprop can't get %s from node %s\n",
+		       "interrupt-map", name);
+		return 0;
+	}
+
+	memcpy((char *)tmp, p, len);
+	val = fdt32_to_cpu(tmp[0][6]);
+	tmp[1][6] = cpu_to_fdt32(val + 1);
+	tmp[2][6] = cpu_to_fdt32(val + 2);
+	tmp[3][6] = cpu_to_fdt32(val + 3);
+
+	err = fdt_setprop(blob, offset, "interrupt-map", tmp, sizeof(tmp));
+	if (err < 0) {
+		printf("WARNING: fdt_setprop can't set %s from node %s: %s.\n",
+		       "interrupt-map", name, fdt_strerror(err));
+		return 0;
+	}
+	return 1;
+}
+
+/* Fixup msi to v1_0*/
+
+static void fdt_fixup_msi(void *blob)
+{
+	int nodeoffset;
+	struct ccsr_gur __iomem *gur = (void *)(CONFIG_SYS_FSL_GUTS_ADDR);
+	unsigned int val;
+
+	val = gur_in32(&gur->svr) & 0xff;
+	if (val == REV1_1)
+		return;
+
+	nodeoffset = fdt_path_offset(blob, "/soc/msi-controller");
+	if (nodeoffset < 0) {
+		printf("WARNING: fdt_path_offset can't find path %s: %s\n",
+		       "/soc/msi-controller", fdt_strerror(nodeoffset));
+		return;
+	}
+	_fdt_fixup_msi_subnode(blob, nodeoffset, "msi0 at 1571000", 116);
+	_fdt_fixup_msi_subnode(blob, nodeoffset, "msi1 at 1572000", 126);
+	_fdt_fixup_msi_subnode(blob, nodeoffset, "msi2 at 1573000", 160);
+
+	_fdt_fixup_pci_msi(blob, "/soc/pcie at 3400000");
+	_fdt_fixup_pci_msi(blob, "/soc/pcie at 3500000");
+	_fdt_fixup_pci_msi(blob, "/soc/pcie@3600000");
+}
+#endif
 
 void ft_cpu_setup(void *blob, bd_t *bd)
 {
@@ -219,4 +310,7 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 #ifdef CONFIG_HAS_FEATURE_GIC4K_ALIGN
 	fdt_fixup_gic(blob);
 #endif
+#ifdef CONFIG_HAS_FEATURE_ENHANCED_MSI
+	fdt_fixup_msi(blob);
+#endif
 }
-- 
2.1.0.27.g96db324

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

[york sun]

On 10/24/2016 12:51 AM, Wenbin song wrote:
> The LS1043A rev1.1 silicon supports two types of GIC offset: 4K alignment
> and 64K alignment. The bit SCFG_GIC400_ALIGN[GIC_ADDR_BIT] is used to choose
> which offset will be used.
>
> The LS1043A rev1.0 silicon only supports the CIG offset with 4K alignment.
>
> If GIC_ADDR_BIT bit is set, 4K alignment is used, or else 64K alignment is used.
> 64K alignment is the default setting.
>
> Overriding the weekly smp_kick_all_cpus, the new impletment is able to detect
> GIC offset.

I think you meant "weak" here. :)

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

>
> Signed-off-by: Wenbin Song <wenbin.song@nxp.com>
> Signed-off-by: Mingkai Hu <mingkai.hu@nxp.com>
> ---
> Changes in v4:
> 	Squash [patch 2/3 v3] with this patch.
> 	Add comments on fix_gic_offest.
> 	Add the descriptions of rev1.0 GIC offset.
> 	Use macros to define the offset and size of GIC components.
> Changes in v3:
> 	Add descriptions about smp_kick_all_cpus on commit message.
> 	Rename the macros on commit message to match with them used in the change.
> 	Replace CONFIG_LS1043A with HAS_FEATURE_GIC4K_ALIGN.
> Changes in v2:
> 	None
> ---
>  arch/arm/cpu/armv8/fsl-layerscape/Kconfig          |  4 ++
>  arch/arm/cpu/armv8/fsl-layerscape/fdt.c            | 49 +++++++++++++++++++
>  arch/arm/cpu/armv8/fsl-layerscape/lowlevel.S       | 55 ++++++++++++++++++++++
>  arch/arm/include/asm/arch-fsl-layerscape/config.h  | 20 +++++++-
>  .../include/asm/arch-fsl-layerscape/immap_lsch2.h  |  3 +-
>  5 files changed, 128 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/Kconfig b/arch/arm/cpu/armv8/fsl-layerscape/Kconfig
> index 94ec8d5..c66c497 100644
> --- a/arch/arm/cpu/armv8/fsl-layerscape/Kconfig
> +++ b/arch/arm/cpu/armv8/fsl-layerscape/Kconfig
> @@ -12,6 +12,7 @@ config ARCH_LS1043A
>  	select SYS_FSL_DDR_VER_50
>  	select SYS_FSL_ERRATUM_A010315
>  	select SYS_FSL_ERRATUM_A010539
> +	select HAS_FEATURE_GIC4K_ALIGN
>
>  config ARCH_LS1046A
>  	bool
> @@ -135,4 +136,7 @@ config SYS_FSL_DDR4
>  	help
>  	  Enable Freescale DDR4 controller.
>
> +config HAS_FEATURE_GIC4K_ALIGN
> +       bool
> +
>  endmenu
> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> index 1a8321b..ebc7863 100644
> --- a/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> +++ b/arch/arm/cpu/armv8/fsl-layerscape/fdt.c
> @@ -126,6 +126,52 @@ void fsl_fdt_disable_usb(void *blob)
>  	}
>  }
>
> +#ifdef CONFIG_HAS_FEATURE_GIC4K_ALIGN
> +/* Fixup gic node align with 4K */
> +static void fdt_fixup_gic(void *blob)
> +{
> +	int offset, err;
> +	u64 reg[8];
> +	struct ccsr_gur __iomem *gur = (void *)(CONFIG_SYS_FSL_GUTS_ADDR);
> +	unsigned int val;
> +	struct ccsr_scfg __iomem *scfg = (void *)CONFIG_SYS_FSL_SCFG_ADDR;
> +
> +	val = gur_in32(&gur->svr) & 0xff;
> +
> +	if (val == REV1_1) {

This is problematic. How about for future SoCs, or other than LS1043A? 
Can we just check GIC_ADDR_BIT?

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

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

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

>  	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
> +	bl	fix_gic_offset
> +#else
>  	ldr	x0, =GICD_BASE
>  	ldr	x1, =GICC_BASE
> +#endif
>  	bl	gic_init_secure_percpu
>  #endif
>  #endif
> @@ -335,7 +385,12 @@ ENTRY(secondary_boot_func)
>  #if defined(CONFIG_GICV3)
>  	gic_wait_for_interrupt_m x0
>  #elif defined(CONFIG_GICV2)
> +#ifdef CONFIG_HAS_FEATURE_GIC4K_ALIGN
> +	bl	fix_gic_offset
> +	mov	x0, x1
> +#else
>          ldr     x0, =GICC_BASE
> +#endif
>          gic_wait_for_interrupt_m x0, w1
>  #endif
>
> diff --git a/arch/arm/include/asm/arch-fsl-layerscape/config.h b/arch/arm/include/asm/arch-fsl-layerscape/config.h
> index 4201e0f..47e21c5 100644
> --- a/arch/arm/include/asm/arch-fsl-layerscape/config.h
> +++ b/arch/arm/include/asm/arch-fsl-layerscape/config.h
> @@ -172,8 +172,24 @@
>  #define SMMU_BASE		0x09000000
>
>  /* Generic Interrupt Controller Definitions */
> -#define GICD_BASE		0x01401000
> -#define GICC_BASE		0x01402000
> +#define GICD_BASE		0x01410000
> +#define GICC_BASE		0x01420000
> +#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?

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

If GIC_ADDR_BIT is always set for SoCs with 4K alignment, you can get 
rid of the Kconfig option.

York

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

[Wenbin Song]

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 at aribaud.net;
> Mingkai Hu <mingkai.hu@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

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

[york sun]

On 10/26/2016 03:39 AM, Wenbin Song wrote:
>>> +
>>> +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.
>
>

The weak function smp_kick_all_cpus has #if to gate the code, so it 
won't run unless either GICv2 or GICv3 is enabled. You made change to 
this function but dropped the #if condition. My concern is future SoC or 
debugging on am emulator where we disable GIC intentionally, this code 
will cause trouble. I suggest you keep the #if condition.

<snip>

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

OK, so this bit is not set by hardware. Please work with design team to 
conform if you can revert the logic for backward compatibility, i.e. use 
the same bit value for rev 1.0 and rev 1.1 for 4K alignment, and a 
non-default value for 64K alignment.

York