[U-Boot] [PATCH] ARMv8: fix bug for flush data cache by set/way

[U-Boot] [PATCH] ARMv8: fix bug for flush data cache by set/way

[Leo Yan]

hi David,

I have checked the ARMv8's Architecture Reference Manual 
DDI0487A_a_armv8_arm.pdf chapter "C4.4.1
DC CISW, Data or unified Cache line Clean and Invalidate by Set/Way", u 
can see the description as below:

SetWay, bits [31:4]
Contains two fields:
? Way, bits[31:32-A], the number of the way to operate on.
? Set, bits[B-1:L], the number of the set to operate on.
Bits[L-1:4] are RES0.
A = Log2(ASSOCIATIVITY), L = Log2(LINELEN), B = (L + S), S = Log2(NSETS).
ASSOCIATIVITY, LINELEN (line length, in bytes), and NSETS (number of 
sets) have their usual meanings and are the values for the cache level 
being operated on. The values of A and S are rounded up to the next integer.

Also we have tested on ARMv8's soc, we found this issue will introduce 
the cache cannot flush properly into DDR so that kernel cannot boot up 
successfully.

BTW, i compared with Linux kernel's code for flushing operations and 
just commit this patch to align with it.

Thx,
Leo Yan

On 04/01/2014 11:25 AM, fenghua at phytium.com.cn wrote:
> hi Leo,
>        Please reference ARMv8-TRM, the associativity field does not have
> to be a power of 2. I made the same mistake with you previously and
>   scott identify this.
>
> *???:* Leo Yan <mailto:leoy@marvell.com>
> *????:* ?2014???3???31??, ???? ?9?:?11
> *???:* u-boot <mailto:u-boot@lists.denx.de>, David Feng
> <mailto:fenghua@phytium.com.cn>, Scott Wood <mailto:scottwood@freescale.com>
> *??:* Leo Yan <mailto:leoy@marvell.com>
>
> When flush the d$ with set/way instruction, it need calculate the way's
> offset = log2(Associativity); but in current uboot's code, it use below
> formula to calculate the offset: log2(Associativity * 2 - 1), so finally
> it cannot flush data cache properly.
>
> Signed-off-by: Leo Yan <leoy@marvell.com>
> ---
>   arch/arm/cpu/armv8/cache.S |    4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/arch/arm/cpu/armv8/cache.S b/arch/arm/cpu/armv8/cache.S
> index 546a83e..256d2e2 100644
> --- a/arch/arm/cpu/armv8/cache.S
> +++ b/arch/arm/cpu/armv8/cache.S
> @@ -30,9 +30,7 @@ ENTRY(__asm_flush_dcache_level)
>    add x2, x2, #4  /* x2 <- log2(cache line size) */
>    mov x3, #0x3ff
>    and x3, x3, x6, lsr #3 /* x3 <- max number of #ways */
> - add w4, w3, w3
> - sub w4, w4, 1  /* round up log2(#ways + 1) */
> - clz w5, w4   /* bit position of #ways */
> + clz w5, w3   /* bit position of #ways */
>    mov x4, #0x7fff
>    and x4, x4, x6, lsr #13 /* x4 <- max number of #sets */
>    /* x1 <- cache level << 1 */
> --
> 1.7.9.5
>

[U-Boot] [PATCH] ARMv8: fix bug for flush data cache by set/way

[Leo Yan]

hi David,

> On 04/01/2014 11:25 AM, fenghua at phytium.com.cn wrote:
>> hi Leo,
>>         Please reference ARMv8-TRM, the associativity field does not have
>> to be a power of 2. I made the same mistake with you previously and
>>    scott identify this.
>>
>> *???:* Leo Yan <mailto:leoy@marvell.com>
>> *????:* ?2014???3???31??, ???? ?9?:?11
>> *???:* u-boot <mailto:u-boot@lists.denx.de>, David Feng
>> <mailto:fenghua@phytium.com.cn>, Scott Wood <mailto:scottwood@freescale.com>
>> *??:* Leo Yan <mailto:leoy@marvell.com>
>>
>> When flush the d$ with set/way instruction, it need calculate the way's
>> offset = log2(Associativity); but in current uboot's code, it use below
>> formula to calculate the offset: log2(Associativity * 2 - 1), so finally
>> it cannot flush data cache properly.
>>
>> Signed-off-by: Leo Yan <leoy@marvell.com>
>> ---
>>    arch/arm/cpu/armv8/cache.S |    4 +---
>>    1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv8/cache.S b/arch/arm/cpu/armv8/cache.S
>> index 546a83e..256d2e2 100644
>> --- a/arch/arm/cpu/armv8/cache.S
>> +++ b/arch/arm/cpu/armv8/cache.S
>> @@ -30,9 +30,7 @@ ENTRY(__asm_flush_dcache_level)
>>     add x2, x2, #4  /* x2 <- log2(cache line size) */
>>     mov x3, #0x3ff
>>     and x3, x3, x6, lsr #3 /* x3 <- max number of #ways */
>> - add w4, w3, w3
>> - sub w4, w4, 1  /* round up log2(#ways + 1) */
>> - clz w5, w4   /* bit position of #ways */
>> + clz w5, w3   /* bit position of #ways */
>>     mov x4, #0x7fff
>>     and x4, x4, x6, lsr #13 /* x4 <- max number of #sets */
>>     /* x1 <- cache level << 1 */
>> --
>> 1.7.9.5
>>

Apologize for top post in previous email, resend the email again.

I have checked the ARMv8's Architecture Reference Manual 
DDI0487A_a_armv8_arm.pdf chapter "C4.4.1 DC CISW, Data or unified Cache 
line Clean and Invalidate by Set/Way", u can see the description as below:

SetWay, bits [31:4]
Contains two fields:
   ? Way, bits[31:32-A], the number of the way to operate on.
   ? Set, bits[B-1:L], the number of the set to operate on.
Bits[L-1:4] are RES0.
A = Log2(ASSOCIATIVITY), L = Log2(LINELEN), B = (L + S), S = 
Log2(NSETS). ASSOCIATIVITY, LINELEN (line length, in bytes), and NSETS 
(number of sets) have their usual meanings and are the values for the 
cache level being operated on. The values of A and S are rounded up to 
the next integer.

Also we have tested on ARMv8's soc, we found this issue will introduce 
the cache cannot flush properly into DDR so that kernel cannot boot up 
successfully.

BTW, i compared with Linux kernel's code for flushing operations and 
just commit this patch to align with it.

Thx,
Leo Yan

[U-Boot] [PATCH] ARMv8: fix bug for flush data cache by set/way

[FengHua]

> -----Original Messages-----
> From: "Leo Yan" <leoy@marvell.com>
> Sent Time: 2014-03-31 09:11:47 (Monday)
> To: u-boot at lists.denx.de, "David Feng" <fenghua@phytium.com.cn>, "Scott Wood" <scottwood@freescale.com>
> Cc: "Leo Yan" <leoy@marvell.com>
> Subject: [PATCH] ARMv8: fix bug for flush data cache by set/way
> 
> When flush the d$ with set/way instruction, it need calculate the way's
> offset = log2(Associativity); but in current uboot's code, it use below
> formula to calculate the offset: log2(Associativity * 2 - 1), so finally
> it cannot flush data cache properly.
> 
> Signed-off-by: Leo Yan <leoy@marvell.com>
> ---
>  arch/arm/cpu/armv8/cache.S |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv8/cache.S b/arch/arm/cpu/armv8/cache.S
> index 546a83e..256d2e2 100644
> --- a/arch/arm/cpu/armv8/cache.S
> +++ b/arch/arm/cpu/armv8/cache.S
> @@ -30,9 +30,7 @@ ENTRY(__asm_flush_dcache_level)
>  	add	x2, x2, #4		/* x2 <- log2(cache line size) */
>  	mov	x3, #0x3ff
>  	and	x3, x3, x6, lsr #3	/* x3 <- max number of #ways */
> -	add	w4, w3, w3
> -	sub	w4, w4, 1		/* round up log2(#ways + 1) */
> -	clz	w5, w4			/* bit position of #ways */
> +	clz	w5, w3			/* bit position of #ways */
>  	mov	x4, #0x7fff
>  	and	x4, x4, x6, lsr #13	/* x4 <- max number of #sets */
>  	/* x1 <- cache level << 1 */
> -- 
> 1.7.9.5
> 

Acked-by: David.Feng <fenghua@phytium.com.cn>

Thank you.

Best Wishes.

[U-Boot] [PATCH] ARMv8: fix bug for flush data cache by set/way

[Leo Yan]

When flush the d$ with set/way instruction, it need calculate the way's
offset = log2(Associativity); but in current uboot's code, it use below
formula to calculate the offset: log2(Associativity * 2 - 1), so finally
it cannot flush data cache properly.

Signed-off-by: Leo Yan <leoy@marvell.com>
---
 arch/arm/cpu/armv8/cache.S |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/arm/cpu/armv8/cache.S b/arch/arm/cpu/armv8/cache.S
index 546a83e..256d2e2 100644
--- a/arch/arm/cpu/armv8/cache.S
+++ b/arch/arm/cpu/armv8/cache.S
@@ -30,9 +30,7 @@ ENTRY(__asm_flush_dcache_level)
 	add	x2, x2, #4		/* x2 <- log2(cache line size) */
 	mov	x3, #0x3ff
 	and	x3, x3, x6, lsr #3	/* x3 <- max number of #ways */
-	add	w4, w3, w3
-	sub	w4, w4, 1		/* round up log2(#ways + 1) */
-	clz	w5, w4			/* bit position of #ways */
+	clz	w5, w3			/* bit position of #ways */
 	mov	x4, #0x7fff
 	and	x4, x4, x6, lsr #13	/* x4 <- max number of #sets */
 	/* x1 <- cache level << 1 */
-- 
1.7.9.5

[U-Boot] [PATCH] ARMv8: fix bug for flush data cache by set/way

[Albert ARIBAUD]

Hi Leo,

On Mon, 31 Mar 2014 09:50:35 +0800, Leo Yan <leoy@marvell.com> wrote:

> When flush the d$ with set/way instruction, it need calculate the way's
> offset = log2(Associativity); but in current uboot's code, it use below
> formula to calculate the offset: log2(Associativity * 2 - 1), so finally
> it cannot flush data cache properly.
> 
> Signed-off-by: Leo Yan <leoy@marvell.com>
> ---
>  arch/arm/cpu/armv8/cache.S |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv8/cache.S b/arch/arm/cpu/armv8/cache.S
> index 546a83e..256d2e2 100644
> --- a/arch/arm/cpu/armv8/cache.S
> +++ b/arch/arm/cpu/armv8/cache.S
> @@ -30,9 +30,7 @@ ENTRY(__asm_flush_dcache_level)
>  	add	x2, x2, #4		/* x2 <- log2(cache line size) */
>  	mov	x3, #0x3ff
>  	and	x3, x3, x6, lsr #3	/* x3 <- max number of #ways */
> -	add	w4, w3, w3
> -	sub	w4, w4, 1		/* round up log2(#ways + 1) */
> -	clz	w5, w4			/* bit position of #ways */
> +	clz	w5, w3			/* bit position of #ways */
>  	mov	x4, #0x7fff
>  	and	x4, x4, x6, lsr #13	/* x4 <- max number of #sets */
>  	/* x1 <- cache level << 1 */

Applied to u-boot-arm/master, thanks!

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] ARMv8: fix bug for flush data cache by set/way

[Scott Wood]

On Mon, 2014-03-31 at 09:50 +0800, Leo Yan wrote:
> When flush the d$ with set/way instruction, it need calculate the way's
> offset = log2(Associativity); but in current uboot's code, it use below
> formula to calculate the offset: log2(Associativity * 2 - 1), so finally
> it cannot flush data cache properly.

More specifically, the "Associativity" above is actually "Associativity
- 1", and clz isn't quite log2().

> Signed-off-by: Leo Yan <leoy@marvell.com>
> ---
>  arch/arm/cpu/armv8/cache.S |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv8/cache.S b/arch/arm/cpu/armv8/cache.S
> index 546a83e..256d2e2 100644
> --- a/arch/arm/cpu/armv8/cache.S
> +++ b/arch/arm/cpu/armv8/cache.S
> @@ -30,9 +30,7 @@ ENTRY(__asm_flush_dcache_level)
>  	add	x2, x2, #4		/* x2 <- log2(cache line size) */
>  	mov	x3, #0x3ff
>  	and	x3, x3, x6, lsr #3	/* x3 <- max number of #ways */
> -	add	w4, w3, w3
> -	sub	w4, w4, 1		/* round up log2(#ways + 1) */
> -	clz	w5, w4			/* bit position of #ways */
> +	clz	w5, w3			/* bit position of #ways */
>  	mov	x4, #0x7fff
>  	and	x4, x4, x6, lsr #13	/* x4 <- max number of #sets */
>  	/* x1 <- cache level << 1 */

I believe the new code will fail on a cache with associativity 1.  x3
would be 0, since it's actually associativity minus one, and thus w5
would be 32.  w5 is later interpreted as x5, so it would actually do a
32-bit shift rather than whatever behavior ARM specifies for shift
counts larger than the word size.

-Scott