[PATCH] ARM: remove unused code from __v7_mmu_cache_flush_invalidate

[PATCH] ARM: remove unused code from __v7_mmu_cache_flush_invalidate

[Masahiro Yamada]

This code is unnecessary (wrong) for the following reasons.

[1] As ARM ARM clearly says, the entire Level 1 cache maintenance
    operations are not supported for ARMv7, i.e. the bit19-16 of
    the ID_MMFR1 is always 0b0000.  The code always jumps to the
    "hierarchical" label.

[2] The value of "r0" is supposed to determine which cache operation
    should be done, invalidate or clean+invalidate.  The line
    "mcr     p15, 0, r12, c7, c14, 0" tries to clean+invalidate
    regardless of the value of "r0", this is weird.
    Anyway, as mentioned above, this line cannot be reached.

Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
---

 arch/arm/cpu/cache-armv7.S | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/arm/cpu/cache-armv7.S b/arch/arm/cpu/cache-armv7.S
index c19618b..f3f6bbb 100644
--- a/arch/arm/cpu/cache-armv7.S
+++ b/arch/arm/cpu/cache-armv7.S
@@ -68,15 +68,9 @@ ENTRY(v7_mmu_cache_flush)
 ENDPROC(v7_mmu_cache_flush)
 
 ENTRY(__v7_mmu_cache_flush_invalidate)
-		mrc	p15, 0, r12, c0, c1, 5	@ read ID_MMFR1
-		tst	r12, #0xf << 16		@ hierarchical cache (ARMv7)
-		mov	r12, #0
-		beq	hierarchical
-		mcr	p15, 0, r12, c7, c14, 0	@ clean+invalidate D
-		b	iflush
-hierarchical:
 		stmfd	sp!, {r4-r11}
 		mov	r8, r0
+		mov	r12, #0
 		mcr	p15, 0, r12, c7, c10, 5	@ DMB
 		mrc	p15, 1, r0, c0, c0, 1	@ read clidr
 		ands	r3, r0, #0x7000000	@ extract loc from clidr
-- 
1.9.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: remove unused code from __v7_mmu_cache_flush_invalidate

[Sascha Hauer]

On Wed, Jan 21, 2015 at 01:24:14PM +0900, Masahiro Yamada wrote:
> This code is unnecessary (wrong) for the following reasons.
> 
> [1] As ARM ARM clearly says, the entire Level 1 cache maintenance
>     operations are not supported for ARMv7, i.e. the bit19-16 of
>     the ID_MMFR1 is always 0b0000.  The code always jumps to the
>     "hierarchical" label.

The offending code is from the kernel from arch/arm/boot/compressed/head.S
The test for ID_MMFR1 nearly unchanged since:

commit 7d09e85448dfa78e3e58186c934449aaf6d49b50
Author: Catalin Marinas <catalin.marinas@arm.com>
Date:   Fri Jun 1 17:14:53 2007 +0100

That of course doesn't make it more correct. Maybe we should send the
same patch to the kernel and let Catalin explain why this code is
necessary (or why not)

> 
> [2] The value of "r0" is supposed to determine which cache operation
>     should be done, invalidate or clean+invalidate.  The line
>     "mcr     p15, 0, r12, c7, c14, 0" tries to clean+invalidate
>     regardless of the value of "r0", this is weird.
>     Anyway, as mentioned above, this line cannot be reached.

This is since

commit 465950ee64f6fbeb0daf138c2d43ad71be159375
Author: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
Date:   Tue May 14 15:14:56 2013 +0200

    ARM v7: added v7_mmu_cache_invalidate()

Appearantly Enrico only handled the hierarchical case.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] ARM: remove unused code from __v7_mmu_cache_flush_invalidate

[Masahiro YAMADA]

Hi Sascha,


2015-01-21 22:56 GMT+09:00 Sascha Hauer <s.hauer@pengutronix.de>:
> On Wed, Jan 21, 2015 at 01:24:14PM +0900, Masahiro Yamada wrote:
>> This code is unnecessary (wrong) for the following reasons.
>>
>> [1] As ARM ARM clearly says, the entire Level 1 cache maintenance
>>     operations are not supported for ARMv7, i.e. the bit19-16 of
>>     the ID_MMFR1 is always 0b0000.  The code always jumps to the
>>     "hierarchical" label.
>
> The offending code is from the kernel from arch/arm/boot/compressed/head.S
> The test for ID_MMFR1 nearly unchanged since:
>
> commit 7d09e85448dfa78e3e58186c934449aaf6d49b50
> Author: Catalin Marinas <catalin.marinas@arm.com>
> Date:   Fri Jun 1 17:14:53 2007 +0100
>
> That of course doesn't make it more correct. Maybe we should send the
> same patch to the kernel and let Catalin explain why this code is
> necessary (or why not)
>

OK. I will.

On the other hand, the code in arch/arm/mm/cache-v7.S
always does hierarchical operations.



ENTRY(v7_flush_dcache_all)
dmb @ ensure ordering with previous memory accesses
mrc p15, 1, r0, c0, c0, 1 @ read clidr
ands r3, r0, #0x7000000 @ extract loc from clidr
mov r3, r3, lsr #23 @ left align loc bit field
beq finished @ if loc is 0, then no need to clean
mov r10, #0 @ start clean at cache level 0
flush_levels:
add r2, r10, r10, lsr #1 @ work out 3x current cache level
mov r1, r0, lsr r2 @ extract cache type bits from clidr
and r1, r1, #7 @ mask of the bits for current cache only
cmp r1, #2 @ see what cache we have at this level
blt skip @ skip if no cache, or just i-cache
#ifdef CONFIG_PREEMPT
save_and_disable_irqs_notrace r9 @ make cssr&csidr read atomic
#endif
mcr p15, 2, r10, c0, c0, 0 @ select current cache level in cssr
isb @ isb to sych the new cssr&csidr
mrc p15, 1, r1, c0, c0, 0 @ read the new csidr
#ifdef CONFIG_PREEMPT
restore_irqs_notrace r9
#endif
and r2, r1, #7 @ extract the length of the cache lines
add r2, r2, #4 @ add 4 (line length offset)
ldr r4, =0x3ff
ands r4, r4, r1, lsr #3 @ find maximum number on the way size
clz r5, r4 @ find bit position of way size increment
ldr r7, =0x7fff
ands r7, r7, r1, lsr #13 @ extract max number of the index size
loop1:
mov r9, r7 @ create working copy of max index
loop2:
 ARM( orr r11, r10, r4, lsl r5 ) @ factor way and cache number into r11
 THUMB( lsl r6, r4, r5 )
 THUMB( orr r11, r10, r6 ) @ factor way and cache number into r11
 ARM( orr r11, r11, r9, lsl r2 ) @ factor index number into r11
 THUMB( lsl r6, r9, r2 )
 THUMB( orr r11, r11, r6 ) @ factor index number into r11
mcr p15, 0, r11, c7, c14, 2 @ clean & invalidate by set/way
subs r9, r9, #1 @ decrement the index
bge loop2
subs r4, r4, #1 @ decrement the way
bge loop1
skip:
add r10, r10, #2 @ increment cache number
cmp r3, r10
bgt flush_levels
finished:
mov r10, #0 @ swith back to cache level 0
mcr p15, 2, r10, c0, c0, 0 @ select current cache level in cssr
dsb st
isb
ret lr
ENDPROC(v7_flush_dcache_all)






-- 
Best Regards
Masahiro Yamada

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox