* [U-Boot] [PATCH] MIPS: fix mips_cache fallback without __builtin_mips_cache
@ 2016-03-05 3:15 Matthias Schiffer
2016-03-06 19:38 ` Daniel Schwierzeck
0 siblings, 1 reply; 5+ messages in thread
From: Matthias Schiffer @ 2016-03-05 3:15 UTC (permalink / raw
To: u-boot
The "R" constraint supplies the address of an variable in a register. Use
"r" instead and adjust asm to supply the content of addr in a register
instead.
Fixes: 2b8bcc5a ("MIPS: avoid .set ISA for cache operations")
Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
Cc: Paul Burton <paul.burton@imgtec.com>
Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
---
Hi,
I've noticed this when reading the code to understand how the cache
instruction is used. I'm not sure if this bug had any practical
consequences, or if nowadays all relevant compilers have
__builtin_mips_cache anyways.
Please keep me in Cc in follow-up mails, I'm not subscribed to the u-boot
ML.
Matthias
arch/mips/include/asm/cacheops.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/include/asm/cacheops.h b/arch/mips/include/asm/cacheops.h
index a3b07c6..002b839 100644
--- a/arch/mips/include/asm/cacheops.h
+++ b/arch/mips/include/asm/cacheops.h
@@ -16,7 +16,7 @@ static inline void mips_cache(int op, const volatile void *addr)
#ifdef __GCC_HAVE_BUILTIN_MIPS_CACHE
__builtin_mips_cache(op, addr);
#else
- __asm__ __volatile__("cache %0, %1" : : "i"(op), "R"(addr));
+ __asm__ __volatile__("cache %0, 0(%1)" : : "i"(op), "r"(addr));
#endif
}
--
2.7.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] MIPS: fix mips_cache fallback without __builtin_mips_cache
2016-03-05 3:15 [U-Boot] [PATCH] MIPS: fix mips_cache fallback without __builtin_mips_cache Matthias Schiffer
@ 2016-03-06 19:38 ` Daniel Schwierzeck
2016-03-06 19:53 ` Matthias Schiffer
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Schwierzeck @ 2016-03-06 19:38 UTC (permalink / raw
To: u-boot
Am 05.03.2016 um 04:15 schrieb Matthias Schiffer:
> The "R" constraint supplies the address of an variable in a register. Use
> "r" instead and adjust asm to supply the content of addr in a register
> instead.
>
> Fixes: 2b8bcc5a ("MIPS: avoid .set ISA for cache operations")
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> Cc: Paul Burton <paul.burton@imgtec.com>
> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
> ---
>
> Hi,
> I've noticed this when reading the code to understand how the cache
> instruction is used. I'm not sure if this bug had any practical
> consequences, or if nowadays all relevant compilers have
> __builtin_mips_cache anyways.
>
> Please keep me in Cc in follow-up mails, I'm not subscribed to the u-boot
> ML.
>
> Matthias
I've disabled the builtin code and compared dissaemblies with and without your patch. Without your patch, gcc adds an additional store instruction before each cache instruction.
E.g. for flush_dcache_range():
18: afa20008 sw v0,8(sp)
1c: bfb50008 cache 0x15,8(sp)
vs.
14: bc550000 cache 0x15,0(v0)
The cache operation works anyway, but with your patch better code is generated.
>
>
> arch/mips/include/asm/cacheops.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/mips/include/asm/cacheops.h b/arch/mips/include/asm/cacheops.h
> index a3b07c6..002b839 100644
> --- a/arch/mips/include/asm/cacheops.h
> +++ b/arch/mips/include/asm/cacheops.h
> @@ -16,7 +16,7 @@ static inline void mips_cache(int op, const volatile void *addr)
> #ifdef __GCC_HAVE_BUILTIN_MIPS_CACHE
> __builtin_mips_cache(op, addr);
> #else
> - __asm__ __volatile__("cache %0, %1" : : "i"(op), "R"(addr));
> + __asm__ __volatile__("cache %0, 0(%1)" : : "i"(op), "r"(addr));
> #endif
> }
>
>
applied to u-boot-mips/next, thanks!
--
- Daniel
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] MIPS: fix mips_cache fallback without __builtin_mips_cache
2016-03-06 19:38 ` Daniel Schwierzeck
@ 2016-03-06 19:53 ` Matthias Schiffer
2016-03-06 20:36 ` Daniel Schwierzeck
2016-03-07 9:30 ` Matthew Fortune
0 siblings, 2 replies; 5+ messages in thread
From: Matthias Schiffer @ 2016-03-06 19:53 UTC (permalink / raw
To: u-boot
On 03/06/2016 08:38 PM, Daniel Schwierzeck wrote:
>
>
> Am 05.03.2016 um 04:15 schrieb Matthias Schiffer:
>> The "R" constraint supplies the address of an variable in a register. Use
>> "r" instead and adjust asm to supply the content of addr in a register
>> instead.
>>
>> Fixes: 2b8bcc5a ("MIPS: avoid .set ISA for cache operations")
>> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
>> Cc: Paul Burton <paul.burton@imgtec.com>
>> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
>> ---
>>
>> Hi,
>> I've noticed this when reading the code to understand how the cache
>> instruction is used. I'm not sure if this bug had any practical
>> consequences, or if nowadays all relevant compilers have
>> __builtin_mips_cache anyways.
>>
>> Please keep me in Cc in follow-up mails, I'm not subscribed to the u-boot
>> ML.
>>
>> Matthias
>
> I've disabled the builtin code and compared dissaemblies with and without your patch. Without your patch, gcc adds an additional store instruction before each cache instruction.
>
> E.g. for flush_dcache_range():
>
> 18: afa20008 sw v0,8(sp)
> 1c: bfb50008 cache 0x15,8(sp)
>
> vs.
>
> 14: bc550000 cache 0x15,0(v0)
>
> The cache operation works anyway, but with your patch better code is generated.
If I understand this correctly, the code without my patch would rather
invalidate the cache for the address 8(sp), which is part of the stack,
and not the memory pointed at by v0.
>
>>
>>
>> arch/mips/include/asm/cacheops.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/mips/include/asm/cacheops.h b/arch/mips/include/asm/cacheops.h
>> index a3b07c6..002b839 100644
>> --- a/arch/mips/include/asm/cacheops.h
>> +++ b/arch/mips/include/asm/cacheops.h
>> @@ -16,7 +16,7 @@ static inline void mips_cache(int op, const volatile void *addr)
>> #ifdef __GCC_HAVE_BUILTIN_MIPS_CACHE
>> __builtin_mips_cache(op, addr);
>> #else
>> - __asm__ __volatile__("cache %0, %1" : : "i"(op), "R"(addr));
>> + __asm__ __volatile__("cache %0, 0(%1)" : : "i"(op), "r"(addr));
>> #endif
>> }
>>
>>
>
> applied to u-boot-mips/next, thanks!
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160306/b10e10a7/attachment.sig>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] MIPS: fix mips_cache fallback without __builtin_mips_cache
2016-03-06 19:53 ` Matthias Schiffer
@ 2016-03-06 20:36 ` Daniel Schwierzeck
2016-03-07 9:30 ` Matthew Fortune
1 sibling, 0 replies; 5+ messages in thread
From: Daniel Schwierzeck @ 2016-03-06 20:36 UTC (permalink / raw
To: u-boot
Am 06.03.2016 um 20:53 schrieb Matthias Schiffer:
> On 03/06/2016 08:38 PM, Daniel Schwierzeck wrote:
>>
>>
>> Am 05.03.2016 um 04:15 schrieb Matthias Schiffer:
>>> The "R" constraint supplies the address of an variable in a register. Use
>>> "r" instead and adjust asm to supply the content of addr in a register
>>> instead.
>>>
>>> Fixes: 2b8bcc5a ("MIPS: avoid .set ISA for cache operations")
>>> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
>>> Cc: Paul Burton <paul.burton@imgtec.com>
>>> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
>>> ---
>>>
>>> Hi,
>>> I've noticed this when reading the code to understand how the cache
>>> instruction is used. I'm not sure if this bug had any practical
>>> consequences, or if nowadays all relevant compilers have
>>> __builtin_mips_cache anyways.
>>>
>>> Please keep me in Cc in follow-up mails, I'm not subscribed to the u-boot
>>> ML.
>>>
>>> Matthias
>>
>> I've disabled the builtin code and compared dissaemblies with and without your patch. Without your patch, gcc adds an additional store instruction before each cache instruction.
>>
>> E.g. for flush_dcache_range():
>>
>> 18: afa20008 sw v0,8(sp)
>> 1c: bfb50008 cache 0x15,8(sp)
>>
>> vs.
>>
>> 14: bc550000 cache 0x15,0(v0)
>>
>> The cache operation works anyway, but with your patch better code is generated.
>
> If I understand this correctly, the code without my patch would rather
> invalidate the cache for the address 8(sp), which is part of the stack,
> and not the memory pointed at by v0.
"sw v0,8(sp)" stores the address in v0 on the stack (location is current stack pointer + 8 * 4 bytes). The cache op then loads this address from stack. Eventually the cache op always uses the address stored in v0 so there is/was no functional bug. But the extra copy to the stack causes a performance degradation.
--
- Daniel
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] MIPS: fix mips_cache fallback without __builtin_mips_cache
2016-03-06 19:53 ` Matthias Schiffer
2016-03-06 20:36 ` Daniel Schwierzeck
@ 2016-03-07 9:30 ` Matthew Fortune
1 sibling, 0 replies; 5+ messages in thread
From: Matthew Fortune @ 2016-03-07 9:30 UTC (permalink / raw
To: u-boot
Matthias Schiffer <mschiffer@universe-factory.net> writes:
> >
> > I've disabled the builtin code and compared dissaemblies with and
> without your patch. Without your patch, gcc adds an additional store
> instruction before each cache instruction.
> >
> > E.g. for flush_dcache_range():
> >
> > 18: afa20008 sw v0,8(sp)
> > 1c: bfb50008 cache 0x15,8(sp)
> >
> > vs.
> >
> > 14: bc550000 cache 0x15,0(v0)
> >
> > The cache operation works anyway, but with your patch better code is
> generated.
>
> If I understand this correctly, the code without my patch would rather
> invalidate the cache for the address 8(sp), which is part of the stack,
> and not the memory pointed at by v0.
Matthias is correct. These two code sequences will exhibit different
overall behaviour. The latter is much more likely to be what you wanted
from what I can see as the flush is to operate on the value of the
'addr' variable not the address of the addr variable.
The same behaviour could be achieved with:
__asm__ __volatile__("cache %0, %1" : : "i"(op), "R"(addr));
and
__asm__ __volatile__("cache %0, 0(%1)" : : "i"(op), "r"(&addr));
Hope that's useful.
Matthew
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-03-07 9:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-05 3:15 [U-Boot] [PATCH] MIPS: fix mips_cache fallback without __builtin_mips_cache Matthias Schiffer
2016-03-06 19:38 ` Daniel Schwierzeck
2016-03-06 19:53 ` Matthias Schiffer
2016-03-06 20:36 ` Daniel Schwierzeck
2016-03-07 9:30 ` Matthew Fortune
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.