[1/1] ARM: Move SYS_CACHELINE_SIZE over to Kconfig

Message ID CAK7LNASNzbCyLcRUS6gkHPDXYgLt_9upqSQBNEMX1baZCAS3jA@mail.gmail.com
State New
Headers show

Commit Message

Masahiro Yamada Aug. 21, 2016, 4:32 p.m.
Hi Tom,


2016-08-22 0:43 GMT+09:00 Tom Rini <trini@konsulko.com>:
> On Mon, Aug 22, 2016 at 12:28:19AM +0900, Masahiro Yamada wrote:

>> Hi Tom,

>>

>>

>>

>> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig

>> > index aef901c..15cd66a 100644

>> > --- a/arch/arm/Kconfig

>> > +++ b/arch/arm/Kconfig

>> > @@ -79,6 +79,11 @@ config SYS_ARM_ARCH

>> >         default 4 if CPU_SA1100

>> >         default 8 if ARM64

>> >

>> > +config SYS_CACHELINE_SIZE

>> > +       int

>> > +       default 64 if CPU_V7 || ARM64

>

> Note!  I had a brain fart here last night and used 'printf %x' when I

> thought I was doing 'printf %d', so, no, ARM64 should get moved up to

> shift 7 / 128 bytes.



As far as I know, the line size of the cores from ARM Ltd. (CA-53, 57, 72)
is 64 byte.


ARM64 Linux increased the line size up to 128 byte for the Cavium's core
because it is multi-platform.


 ------------------------------>8--------------------------------------

commit 97303480753e48fb313dc0e15daaf11b0451cdb8
Author: Tirumalesh Chalamarla <tchalamarla@cavium.com>
Date:   Tue Sep 22 19:59:48 2015 +0200

    arm64: Increase the max granular size

    Increase the standard cacheline size to avoid having locks in the same
    cacheline.

    Cavium's ThunderX core implements cache lines of 128 byte size. With
    current granulare size of 64 bytes (L1_CACHE_SHIFT=6) two locks could
    share the same cache line leading a performance degradation.
    Increasing the size fixes that.

    Increasing the size has no negative impact to cache invalidation on
    systems with a smaller cache line. There is an impact on memory usage,
    but that's not too important for arm64 use cases.

    Signed-off-by: Tirumalesh Chalamarla <tchalamarla@cavium.com>

    Signed-off-by: Robert Richter <rrichter@cavium.com>

    Acked-by: Timur Tabi <timur@codeaurora.org>

    Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>



--------------------------------------8<-------------------------------------


U-Boot does not adopt multi-platform,
so we can make the ARCH_THUNDERX select CACHESHIFT_7
and allow other ARM64 SoCs to stay at CACHESHIFT_6.
Just my 2 cents.


>>

>>

>>

>> This idea was borrowed from Linux.

>> (you can grep "_L1_CACHE_SHIFT" in Linux Kconfig files.)

>

> I'm agreeable to moving over to shift to more obviously align with the

> Linux Kernel (and it will make other arches easier to migrate too).  But

> the UniPhier case currently looks to me like it's overloading what

> CONFIG_SYS_CACHELINE_SIZE is doing,


First, I'd like to know if CONFIG_SYS_CACHELINE_SIZE
means the line size of L1 cache,
or the largest line size across all the cache levels.



> ie in the Linux Kernel it's not

> setting the shift to 7, in 32bit.  Or is this a 64bit only feature?

> Thanks!


CACHE_UNIPHIER is a full-custom outer cache
only implemented in its 32bit SoC lineup.


UniPhier (32bit) is a very unfortunate case
where outer-cache has a different line size
than the L1 line size.


L1  (ARM native, inner):     32byte line (shift 5)
L2  (UniPhier outer cache):  128byte line (shift 7)



For Linux, it is just not upstreamed yet.


If you are interested, check the following:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/400289.html


If I set the shift to 7, it would affect all the other SoCs in
multi_v7_defconfig,
which other vendors would probably be unhappy about.

Again, it is no problem in U-Boot, which is not multi-platform.


-- 
Best Regards
Masahiro Yamada
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Comments

Masahiro Yamada Aug. 22, 2016, 11:36 a.m. | #1
Hi Tom,


2016-08-22 5:18 GMT+09:00 Tom Rini <trini@konsulko.com>:
> On Mon, Aug 22, 2016 at 01:32:52AM +0900, Masahiro Yamada wrote:

>> Hi Tom,

>>

>>

>> 2016-08-22 0:43 GMT+09:00 Tom Rini <trini@konsulko.com>:

>> > On Mon, Aug 22, 2016 at 12:28:19AM +0900, Masahiro Yamada wrote:

>> >> Hi Tom,

>> >>

>> >>

>> >>

>> >> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig

>> >> > index aef901c..15cd66a 100644

>> >> > --- a/arch/arm/Kconfig

>> >> > +++ b/arch/arm/Kconfig

>> >> > @@ -79,6 +79,11 @@ config SYS_ARM_ARCH

>> >> >         default 4 if CPU_SA1100

>> >> >         default 8 if ARM64

>> >> >

>> >> > +config SYS_CACHELINE_SIZE

>> >> > +       int

>> >> > +       default 64 if CPU_V7 || ARM64

>> >

>> > Note!  I had a brain fart here last night and used 'printf %x' when I

>> > thought I was doing 'printf %d', so, no, ARM64 should get moved up to

>> > shift 7 / 128 bytes.

>>

>>

>> As far as I know, the line size of the cores from ARM Ltd. (CA-53, 57, 72)

>> is 64 byte.

>>

>>

>> ARM64 Linux increased the line size up to 128 byte for the Cavium's core

>> because it is multi-platform.

>>

>>

>>  ------------------------------>8--------------------------------------

>>

>> commit 97303480753e48fb313dc0e15daaf11b0451cdb8

>> Author: Tirumalesh Chalamarla <tchalamarla@cavium.com>

>> Date:   Tue Sep 22 19:59:48 2015 +0200

>>

>>     arm64: Increase the max granular size

>>

>>     Increase the standard cacheline size to avoid having locks in the same

>>     cacheline.

>>

>>     Cavium's ThunderX core implements cache lines of 128 byte size. With

>>     current granulare size of 64 bytes (L1_CACHE_SHIFT=6) two locks could

>>     share the same cache line leading a performance degradation.

>>     Increasing the size fixes that.

>>

>>     Increasing the size has no negative impact to cache invalidation on

>>     systems with a smaller cache line. There is an impact on memory usage,

>>     but that's not too important for arm64 use cases.

>>

>>     Signed-off-by: Tirumalesh Chalamarla <tchalamarla@cavium.com>

>>     Signed-off-by: Robert Richter <rrichter@cavium.com>

>>     Acked-by: Timur Tabi <timur@codeaurora.org>

>>     Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>

>>

>> diff --git a/arch/arm64/include/asm/cache.h b/arch/arm64/include/asm/cache.h

>> index bde4499..5082b30 100644

>> --- a/arch/arm64/include/asm/cache.h

>> +++ b/arch/arm64/include/asm/cache.h

>> @@ -18,7 +18,7 @@

>>

>>  #include <asm/cachetype.h>

>>

>> -#define L1_CACHE_SHIFT         6

>> +#define L1_CACHE_SHIFT         7

>>  #define L1_CACHE_BYTES         (1 << L1_CACHE_SHIFT)

>>

>>  /*

>>

>> --------------------------------------8<-------------------------------------

>>

>>

>> U-Boot does not adopt multi-platform,

>> so we can make the ARCH_THUNDERX select CACHESHIFT_7

>> and allow other ARM64 SoCs to stay at CACHESHIFT_6.

>> Just my 2 cents.

>

> OK, good point, thanks!

>

>> >> This idea was borrowed from Linux.

>> >> (you can grep "_L1_CACHE_SHIFT" in Linux Kconfig files.)

>> >

>> > I'm agreeable to moving over to shift to more obviously align with the

>> > Linux Kernel (and it will make other arches easier to migrate too).  But

>> > the UniPhier case currently looks to me like it's overloading what

>> > CONFIG_SYS_CACHELINE_SIZE is doing,

>>

>> First, I'd like to know if CONFIG_SYS_CACHELINE_SIZE

>> means the line size of L1 cache,

>> or the largest line size across all the cache levels.

>

> Good question.  This I think ends up being a historical ambiguity.  My

> poking around in git log suggests that when CFG_SYS_CACHELINE_SIZE was

> added it was implicitly L1.  It's gone on to mean "cache alignment

> required for DMA".  It is often defined as L1 and only occasionally

> defined differently.  Presumably because it needs to be higher on those

> systems for DMA/etc.  In sum, I remove my objection to UniPhier maybe

> overloading something that it shouldn't, it's doing the right thing.



Right.

Given that
  likely(CONFIG_SYS_CACHELINE_SIZE == ARCH_DMA_ALIGN)

I have to set CONFIG_SYS_CACHELINE_SIZE to 128
when the outer cache is enabled because
DMA engines are located outside of the cache topology.




>> > ie in the Linux Kernel it's not

>> > setting the shift to 7, in 32bit.  Or is this a 64bit only feature?

>> > Thanks!

>>

>> CACHE_UNIPHIER is a full-custom outer cache

>> only implemented in its 32bit SoC lineup.

>>

>>

>> UniPhier (32bit) is a very unfortunate case

>> where outer-cache has a different line size

>> than the L1 line size.

>>

>>

>> L1  (ARM native, inner):     32byte line (shift 5)

>> L2  (UniPhier outer cache):  128byte line (shift 7)

>>

>>

>>

>> For Linux, it is just not upstreamed yet.

>>

>>

>> If you are interested, check the following:

>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/400289.html

>>

>>

>> If I set the shift to 7, it would affect all the other SoCs in

>> multi_v7_defconfig,

>> which other vendors would probably be unhappy about.

>>

>> Again, it is no problem in U-Boot, which is not multi-platform.

>

> Ah, OK.  So then the question is, are we OK with things like the

> following in the .config file:

> CONFIG_SYS_CACHE_SHIFT_6=y

> CONFIG_SYS_CACHE_SHIFT_7=y

> CONFIG_SYS_CACHELINE_SIZE=128

>

> ?


Yes, I think it is OK.



-- 
Best Regards
Masahiro Yamada
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Patch

diff --git a/arch/arm64/include/asm/cache.h b/arch/arm64/include/asm/cache.h
index bde4499..5082b30 100644
--- a/arch/arm64/include/asm/cache.h
+++ b/arch/arm64/include/asm/cache.h
@@ -18,7 +18,7 @@ 

 #include <asm/cachetype.h>

-#define L1_CACHE_SHIFT         6
+#define L1_CACHE_SHIFT         7
 #define L1_CACHE_BYTES         (1 << L1_CACHE_SHIFT)

 /*