[PATCH v2] ARM: l2c: parse cache properties from ePAPR definitions

[Linus Walleij]

On Tue, Sep 9, 2014 at 9:42 AM, Arnd Bergmann <arnd at arndb.de> wrote:
> On Tuesday 09 September 2014 09:10:33 Linus Walleij wrote:

> I wonder if we should add another property here that tells
> the OS to override the aux register setting for way-size
> and associativity. In theory the properties above are meant
> to be there for any cache, but I don't think we want to actually
> re-compute the auxctrl register values based on this all the
> time.

OK added that.

>> +     /* All these l2 caches have the same line = block size actually */
>> +     if (!linesize) {
>> +             if (blocksize) {
>> +                     /* If linesize if not given, it is equal to blocksize */
>> +                     linesize = blocksize;
>> +             } else {
>> +                     /* Fall back to known size */
>> +                     linesize = CACHE_LINE_SIZE;
>> +             }
>> +     }
>
> Maybe add a warning for the last fallback?

OK.

>> +     /* This is the PL3x0 case */
>> +     if (max_associativity == 16 && (assoc != 8 && assoc != 16)) {
>> +             pr_err("L2C OF: cache setting yield illegal associativity\n");
>> +             pr_err("L2C OF: %d calculated, only 8 and 16 legal\n", assoc);
>> +             return;
>> +     }
>
> I'd rather see another function argument for the minimum associativity
> here. We have a few other cache controllers that are partially compatible
> with l2x0 (tauros3, aurora, bcm11351) and one of these or one we might
> add in the future could support a maximum of 16 but also some other sizes
> below 8.

Well it's more like a list in that case. PL310 supports 8 or 16 way
associativity, not 7, 9, 10...

>> +     /*
>> +      * Special checks for the PL310 that only has two settings and
>> +      * cannot be set to fully associative.
>> +      */
>> +     if (max_associativity == 16) {
>> +             if (assoc == 16)
>> +                     val |= L310_AUX_CTRL_ASSOCIATIVITY_16;
>> +             /* Else bit is left zero == 8 way associativity */
>> +     } else {
>> +             val |= (assoc << L2X0_AUX_CTRL_ASSOC_SHIFT);
>> +     }
>
> What happens if we set the bit for assoc=8 on pl310? Is that
> defined to be ignored or does it have to be zero?

Only one single bit selects associativity on PL310.

Bit 16 = 0 -> 8-way associativity
Bit 16 = 1 -> 16-way associativity

On L220 there are 1,2,3,4,5,6,7,8 ways of associativity.

>> +     /*
>> +      * The l2x0 TRMs call this size "way size" but that is incorrect:
>> +      * the thing being configured in these register bits is actually
>> +      * the cache set size, so the variable here has the right name
>> +      * but the register bit definitions following the TRM are not
>> +      * in archaic naming.
>> +      */
>
> No, I think actually the comment and the variable name are wrong here,
> and the TRM is right. I'm surprised you get the right results out of
> this. The set_size should be a relatively small number, e.g. 256 bytes
> in case of an 8-way associative cache with 32 byte lines. What is the
> pr_debug output and the properties you pass in for your example system?

It looks like that:

L2C OF: override cache size: 131072 bytes (128KB)
L2C OF: override set size: 16384 bytes (16 KB)
L2C OF: override line size: 32 bytes
L2C OF: override ways: 512 ways
L2C OF: override way size: 256 bytes
L2C OF: override associativity: 8
L2C: DT/platform modifies aux control register: 0x02020fff -> 0x02030fff
L2C-220 cache controller enabled, 8 ways, 128 kB
L2C-220: CACHE_ID 0x41000486, AUX_CTRL 0x06030fff

Based on that device tree:

        L2: l2-cache {
            compatible = "arm,l220-cache";
            reg = <0x10110000 0x1000>;
            interrupt-parent = <&intc_dc1176>;
            interrupts = <0 13 IRQ_TYPE_LEVEL_HIGH>;
            cache-unified;
            cache-level = <2>;
            /*
             * Override default cache size, sets and
             * associativity as these may be erroneously set
             * up by boot loader(s).
             */
            arm,override-auxreg;
            cache-size = <131072>; // 128kB
            cache-sets = <8>;
            cache-line-size = <32>;
        };

But I'll take a closer look to make sure we get this right.

Yours,
Linus Walleij