Re: [PATCH 5/7 v6] ARM: l2c: parse 'cache-size' and 'cache-sets' properties

Florian Fainelli
2014-09-08T20:33:39Z

On 09/08/2014 06:16 AM, Arnd Bergmann wrote:
> On Monday 08 September 2014 14:36:48 Linus Walleij wrote:
>> On Mon, Sep 8, 2014 at 2:20 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Monday 08 September 2014 13:38:04 Linus Walleij wrote:
>>>> + of_property_read_u32(np, "cache-size", &size);
>>>> + of_property_read_u32(np, "cache-sets", &sets);
>>>> +
>>>> + if (!size || !sets)
>>>> + return;
>>>> +
>>>> + way_size = size / sets;
>>>
>>> Going back to this one: Isn't (size / sets) the set-size rather
>>> than the way-size?
>>>
>>> After we discussed this on IRC, I had expected a calculation like
>>>
>>> set_size = size / sets;
>>> ways = set_size / line_size;
>>> way_size = size / ways;
>>
>> First: in this PB1176 case:
>>
>> set_size = 128K/8 = 16K
>> ways = 16K/32 = 512 bytes
>> way_size = 128K/512 = 128 bytes
>
> I guess we should first try to find the right units so we know what
> we are talking about. I was under the impression that the set size
> is the number of bytes in a set, while 'ways' is counting the
> associativity and has no unit.

I agree with that.

>
> The last line also seems to incorrectly computed. Dividing 128KB
> by 512 bytes should be 256 (no unit).
>
>> Well maybe it's the ARM reference manual internal lingo that
>> is actually causing the confusion here. It will say something
>> like:
>>
>> [19:17] Way-size 3’b000 = Reserved, internally mapped to 16KB
>> 3’b001 = 16KB, this is the default value
>> 3’b010 = 32KB
>> 3’b011 = 64KB
>> 3’b100 = 128KB
>> 3’b101 = 256KB
>> 3’b110 to 3’b111 = Reserved, internally mapped to 256 KB
>>
>> OK way-size ... is in the 16 thru 256KB range, which fits nicely
>> with set size incidentally. And also corresponds to current
>> comments in the code such as this from
>> arch/arm/mach-realview/realview_pb1176.c:
>>
>> #ifdef CONFIG_CACHE_L2X0
>> /*
>> * The PL220 needs to be manually configured as the hardware
>> * doesn't report the correct sizes.
>> * 128kB (16kB/way), 8-way associativity, event monitor and
>> * parity enabled, ignore share bit, no force write allocate
>> * Bits: .... ...0 0111 0011 0000 .... .... ....
>> */
>> l2x0_init(__io_address(REALVIEW_PB1176_L220_BASE), 0x00730000,
>> 0xfe000fff);
>> #endif
>
> 16KB way-size seems realistic, yes.
>
>> I can add a comment explaining that ARMs terminology does
>> not match the academic terminology or something, and say that
>> the thing we poke into "way-size" is actually "set size", if we agree
>> that is what we're seeing here.
>
> Let's see again: 16KB way-size * 8 ways = 128KB, that seems correct.
> 16KB way-size / 32B line-size = 512 sets, that also seems realistic.
> 128KB cache-size / 512 sets = 256B set-size. 256B / 32B = 8 ways,
> so everything fits together.
>
> Now in the code:
>
> + of_property_read_u32(np, "cache-size", &size);
>
> 131072
>
> + of_property_read_u32(np, "cache-sets", &sets);
>
> 512
>
> +
> + if (!size || !sets)
> + return;
> +
> + way_size = size / sets;
>
> 256 -> impossible.
>
>
> set_size = size / sets;
>
> 256
>
> ways = set_size / line_size;
>
> 8
>
> way_size = size / ways;
>
> 16KB -> ok

So what we are missing right now is just a 'cache-line-size' property to
get the maths right.
--
Florian