diff mbox

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

Message ID 1410176286-32533-6-git-send-email-linus.walleij@linaro.org
State New
Headers show

Commit Message

Linus Walleij Sept. 8, 2014, 11:38 a.m. UTC 
From: Florian Fainelli <f.fainelli@gmail.com>

When both 'cache-size' and 'cache-sets' are specified for a L2 cache
controller node, parse those properties and set up the
way_size based on which type of L2 cache controller we are using.

Update the L2 cache controller Device Tree binding with the optional
'cache-size' and 'cache-sets' properties. These both come from the
ePAPR specification.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/devicetree/bindings/arm/l2cc.txt |  2 +
 arch/arm/mm/cache-l2x0.c                       | 61 ++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

Comments

Arnd Bergmann Sept. 8, 2014, 12:20 p.m. UTC | #1 
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;

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Sept. 8, 2014, 12:36 p.m. UTC | #2 
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

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

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.

Florian: what was your interpretation?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Sept. 8, 2014, 1:16 p.m. UTC | #3 
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.

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


	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Fainelli Sept. 8, 2014, 7:57 p.m. UTC | #4 
On 09/08/2014 05:36 AM, 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
> 
> 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
> 
> 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.
> 
> Florian: what was your interpretation?

Yes that was my interpretation as well, that we could have 'way-size'
and 'set-size' be the same things here, now that I re-think about it, I
am not sure anymore. More to follow on Arnd's reply to this email.
--
Florian

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Sept. 9, 2014, 7:14 a.m. UTC | #5 
On Mon, Sep 8, 2014 at 3:16 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> [Me]
>> 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.

I've sent a v2 version of the patch using the terminology and calculations
from this thread, and it yields the right numbers, for PB1176, I will check
with the values from other RealView platforms to see if they also give the
same result back.

I had to squash Florians & my patch, it didn't make sense to have them
artificially separate.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt
index af527ee111c2..d33ed2344c7e 100644
--- a/Documentation/devicetree/bindings/arm/l2cc.txt
+++ b/Documentation/devicetree/bindings/arm/l2cc.txt
@@ -44,6 +44,8 @@  Optional properties:
   I/O coherent mode. Valid only when the arm,pl310-cache compatible
   string is used.
 - interrupts : 1 combined interrupt.
+- cache-size : specifies the size in bytes of the cache
+- cache-sets : specifies the number of associativity sets of the cache
 - cache-id-part: cache id part number to be used if it is not present
   on hardware
 - wt-override: If present then L2 is forced to Write through mode
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 5f2c988a06ac..61a684c743c6 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -945,6 +945,61 @@  static int l2_wt_override;
  * pass it though the device tree */
 static u32 cache_id_part_number_from_dt;
 
+static void __init l2x0_cache_size_of_parse(const struct device_node *np,
+					    u32 *aux_val, u32 *aux_mask,
+					    u32 max_way_size)
+{
+	u32 mask = 0, val = 0;
+	u32 size = 0, sets = 0;
+	u32 way_size = 0, way_size_bits = 1;
+
+	of_property_read_u32(np, "cache-size", &size);
+	of_property_read_u32(np, "cache-sets", &sets);
+
+	if (!size || !sets)
+		return;
+
+	way_size = size / sets;
+
+	if (way_size > max_way_size) {
+		pr_warn("L2C: way size %dKB is too large\n", way_size >> 10);
+		return;
+	}
+
+	way_size >>= 10;
+	switch (way_size) {
+	case 512:
+		way_size_bits = 6;
+		break;
+	case 256:
+		way_size_bits = 5;
+		break;
+	case 128:
+		way_size_bits = 4;
+		break;
+	case 64:
+		way_size_bits = 3;
+		break;
+	case 32:
+		way_size_bits = 2;
+		break;
+	case 16:
+		way_size_bits = 1;
+		break;
+	default:
+		pr_err("cache way size: %d KB is not mapped\n",
+		       way_size);
+		break;
+	}
+
+	mask |= L2C_AUX_CTRL_WAY_SIZE_MASK;
+	val |= (way_size_bits << L2C_AUX_CTRL_WAY_SIZE_SHIFT);
+
+	*aux_val &= ~mask;
+	*aux_val |= val;
+	*aux_mask &= ~mask;
+}
+
 static void __init l2x0_of_parse(const struct device_node *np,
 				 u32 *aux_val, u32 *aux_mask)
 {
@@ -974,6 +1029,8 @@  static void __init l2x0_of_parse(const struct device_node *np,
 		val |= (dirty - 1) << L2X0_AUX_CTRL_DIRTY_LATENCY_SHIFT;
 	}
 
+	l2x0_cache_size_of_parse(np, aux_val, aux_mask, SZ_256K);
+
 	*aux_val &= ~mask;
 	*aux_val |= val;
 	*aux_mask &= ~mask;
@@ -1047,6 +1104,8 @@  static void __init l2c310_of_parse(const struct device_node *np,
 		writel_relaxed((filter[0] & ~(SZ_1M - 1)) | L310_ADDR_FILTER_EN,
 			       l2x0_base + L310_ADDR_FILTER_START);
 	}
+
+	l2x0_cache_size_of_parse(np, aux_val, aux_mask, SZ_512K);
 }
 
 static const struct l2c_init_data of_l2c310_data __initconst = {
@@ -1253,6 +1312,8 @@  static void __init aurora_of_parse(const struct device_node *np,
 	*aux_val &= ~mask;
 	*aux_val |= val;
 	*aux_mask &= ~mask;
+
+	l2x0_cache_size_of_parse(np, aux_val, aux_mask, SZ_256K);
 }
 
 static const struct l2c_init_data of_aurora_with_outer_data __initconst = {