[U-Boot] [PATCH 01/11] powerpc/mpc85xx: Enable L2 at the beginning of U-boot for E6500

[York Sun <yorksun@freescale.com>]

On 08/24/2012 03:53 PM, Scott Wood wrote:
> On 08/17/2012 01:27 PM, York Sun wrote:
>> Using E6500 L1 cache as initram requires L2 cache enabled.
>> Add l2-cache cluster enabling.
>>
>> Signed-off-by: York Sun <yorksun@freescale.com>
>> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
>> ---
>>   arch/powerpc/cpu/mpc85xx/cpu_init.c   |   39 ++++++++++++++++++++++-
>>   arch/powerpc/cpu/mpc85xx/start.S      |   57 +++++++++++++++++++++++++++++++++
>>   arch/powerpc/include/asm/immap_85xx.h |   43 +++++++++++++++++++++++++
>>   3 files changed, 138 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/powerpc/cpu/mpc85xx/cpu_init.c b/arch/powerpc/cpu/mpc85xx/cpu_init.c
>> index 2c78905..1a2858a 100644
>> --- a/arch/powerpc/cpu/mpc85xx/cpu_init.c
>> +++ b/arch/powerpc/cpu/mpc85xx/cpu_init.c
>> @@ -309,6 +309,34 @@ static void __fsl_serdes__init(void)
>>   }
>>   __attribute__((weak, alias("__fsl_serdes__init"))) void fsl_serdes_init(void);
>>
>> +#ifdef CONFIG_E6500
>> +int enable_cluster_l2(void)
>
> If enabling L2 is required for the stack, how are we enabling it in C
> code?  Is this just for non-boot clusters?

Yes, this is for non-boot clusters. The boot cluster is enabled in start.S.

>
>> +{
>> +	int i = 0;
>> +	u32 cluster;
>> +	ccsr_gur_t *gur = (void *)(CONFIG_SYS_MPC85xx_GUTS_ADDR);
>> +	struct ccsr_cluster_l2 *l2cache;
>> +
>> +	cluster = in_be32(&gur->tp_cluster[i++].lower);
>> +	if (cluster & TP_CLUSTER_EOC)
>> +		return 0;
>> +
>> +	do {
>> +		l2cache = (void *)(CONFIG_SYS_FSL_CLUSTER_1_L2 + i * 0x40000);
>> +		cluster = in_be32(&gur->tp_cluster[i++].lower);
>> +
>> +		printf("enable l2 for cluster %d %p\n", i, l2cache);
>
> This should be a debug message (or removed), not a normal printf.

OK.

>
>> +
>> +		out_be32(&l2cache->l2csr0, L2CSR0_L2FI|L2CSR0_L2LFC);
>> +		while ((in_be32(&l2cache->l2csr0) & (L2CSR0_L2FI|L2CSR0_L2LFC)) != 0)
>> +			;
>
> Timeout?

I don't have a spec telling me how long.


>
>> @@ -322,6 +350,11 @@ int cpu_init_r(void)
>>   #ifdef CONFIG_SYS_LBC_LCRR
>>   	volatile fsl_lbc_t *lbc = LBC_BASE_ADDR;
>>   #endif
>> +#ifdef CONFIG_L2_CACHE
>> +	volatile ccsr_l2cache_t *l2cache = (void *)CONFIG_SYS_MPC85xx_L2_ADDR;
>> +#elif defined(CONFIG_E6500)
>> +	struct ccsr_cluster_l2 * l2cache = (void *)CONFIG_SYS_FSL_CLUSTER_1_L2;
>> +#endif
>
> If CONFIG_L2_CACHE doesn't apply to e6500, then CONFIG_L2_CACHE is misnamed.

Maybe it's time to introduce a new name?

>
>> diff --git a/arch/powerpc/cpu/mpc85xx/start.S b/arch/powerpc/cpu/mpc85xx/start.S
>> index 2e1d265..739127f 100644
>> --- a/arch/powerpc/cpu/mpc85xx/start.S
>> +++ b/arch/powerpc/cpu/mpc85xx/start.S
>> @@ -762,6 +762,63 @@ delete_temp_tlbs:
>>   	tlbwe
>>   #endif /* #if (CONFIG_SYS_CCSRBAR_DEFAULT != CONFIG_SYS_CCSRBAR_PHYS) */
>>
>> +#ifdef CONFIG_E6500
>> +create_ccsr_l2_tlb:
>> +	/*
>> +	 * Create a TLB for the MMR location of CCSR
>> +	 * to access L2CSR0 register
>> +	 */
>> +	lis     r0, FSL_BOOKE_MAS0(0, 0, 0)@h
>> +	ori     r0, r0, FSL_BOOKE_MAS0(0, 0, 0)@l
>> +
>> +	lis     r1, FSL_BOOKE_MAS1(1, 0, 0, 0, BOOKE_PAGESZ_4K)@h
>> +	ori     r1, r1, FSL_BOOKE_MAS1(1, 0, 0, 0, BOOKE_PAGESZ_4K)@l
>> +	lis     r2, FSL_BOOKE_MAS2(CONFIG_SYS_CCSRBAR + 0xC20000, (MAS2_I|MAS2_G))@h
>> +	ori     r2, r2, FSL_BOOKE_MAS2(CONFIG_SYS_CCSRBAR + 0xC20000, (MAS2_I|MAS2_G))@l
>> +	lis     r3, FSL_BOOKE_MAS3(CONFIG_SYS_CCSRBAR_PHYS_LOW + 0xC20000, 0, (MAS3_SW|MAS3_SR))@h
>> +	ori     r3, r3, FSL_BOOKE_MAS3(CONFIG_SYS_CCSRBAR_PHYS_LOW + 0xC20000, 0, (MAS3_SW|MAS3_SR))@l
>> +	lis	r7, CONFIG_SYS_CCSRBAR_PHYS_HIGH at h
>> +	ori	r7, r7, CONFIG_SYS_CCSRBAR_PHYS_HIGH at l
>> +	mtspr   MAS0, r0
>> +	mtspr   MAS1, r1
>> +	mtspr   MAS2, r2
>> +	mtspr   MAS3, r3
>> +	mtspr   MAS7, r7
>> +	isync
>> +	msync
>> +	tlbwe
>
> Let's make a macro (asm, not cpp) out of this instead of copy and
> pasting all over the place.  And stop misusing r1/r2.
>

That's a good idea. I am also tired of this long copy-n-paste. Using a 
macro increase the chance of overwriting registers, doesn't it? Any good 
idea to avoid?

York