[U-Boot] [PATCH 1/1] ARM: Move SYS_CACHELINE_SIZE over to Kconfig

[Masahiro Yamada]

Hi Tom,


2016-08-22 5:18 GMT+09:00 Tom Rini <trini at 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 at 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 at 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 at cavium.com>
>>     Signed-off-by: Robert Richter <rrichter at cavium.com>
>>     Acked-by: Timur Tabi <timur at codeaurora.org>
>>     Signed-off-by: Catalin Marinas <catalin.marinas at 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