ARM: prima2: remove L2 cache size override

PATCH New Barry Song 3 participants 14 messages Apr 15, 2014 – Apr 23, 2014

Subsystems (3)

Device tree bindings Arm (32-bit) Arm sub-architectures

lore

Share & tools

lore

Barry Song

author

2014-04-15 09:49 UTC #0 lore raw

PATCH 1/1 ARM: prima2: remove L2 cache size override

From: Barry Song <[email protected]>

L2 cache size has been read by l2x0_init(), it is useless to set WAY_SIZE and ASSOCIATIVITY in prima2.

Cc Russell King <[email protected]>

Signed-off-by Barry Song <[email protected]>

---
-Derived from Russell's "ARM: l2c: prima2: remove cache size override"

 arch/arm/boot/dts/marco.dtsi  |    2 +-
 arch/arm/boot/dts/prima2.dtsi |    2 +-
 arch/arm/mach-prima2/common.c |   14 +++++++++--
 arch/arm/mach-prima2/l2x0.c   |   49 -----------------------------------------
 4 files changed, 13 insertions(+), 54 deletions(-)
 delete mode 100644 arch/arm/mach-prima2/l2x0.c

arch/arm/boot/dts/marco.dtsi +1 -1

index 0c9647d..fb35422 100644
@@ -36,7 +36,7 @@
 		ranges = <0x40000000 0x40000000 0xa0000000>;
 
 		l2-cache-controller at c0030000 {
-			compatible = "sirf,marco-pl310-cache", "arm,pl310-cache";
+			compatible = "arm,pl310-cache";
 			reg = <0xc0030000 0x1000>;
 			interrupts = <0 59 0>;
 			arm,tag-latency = <1 1 1>;

arch/arm/boot/dts/prima2.dtsi +1 -1

index 1e82571..0d6588d 100644
@@ -48,7 +48,7 @@
 		ranges = <0x40000000 0x40000000 0x80000000>;
 
 		l2-cache-controller at 80040000 {
-			compatible = "arm,pl310-cache", "sirf,prima2-pl310-cache";
+			compatible = "arm,pl310-cache";
 			reg = <0x80040000 0x1000>;
 			interrupts = <59>;
 			arm,tag-latency = <1 1 1>;

arch/arm/mach-prima2/common.c +11 -3

index 47c7819..23fcade 100644
@@ -8,11 +8,10 @@
 
 #include <linux/init.h>
 #include <linux/kernel.h>
-#include <asm/sizes.h>
-#include <asm/mach-types.h>
-#include <asm/mach/arch.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
+#include <asm/hardware/cache-l2x0.h>
+#include <asm/mach/arch.h>
 #include "common.h"
 
 static void __init sirfsoc_init_late(void)

@@ -26,6 +25,12 @@ static __init void sirfsoc_map_io(void)
 	sirfsoc_map_scu();
 }
 
+static __init void sirfsoc_init_mach(void)
+{
+	l2x0_of_init(0, 0xFDFFFFFFUL);
+	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
+}
+
 #ifdef CONFIG_ARCH_ATLAS6
 static const char *atlas6_dt_match[] __initconst = {
 	"sirf,atlas6",

@@ -35,6 +40,7 @@ static const char *atlas6_dt_match[] __initconst = {
 DT_MACHINE_START(ATLAS6_DT, "Generic ATLAS6 (Flattened Device Tree)")
 	/* Maintainer: Barry Song <[email protected]> */
 	.map_io         = sirfsoc_map_io,
+	.init_machine   = sirfsoc_init_mach,
 	.init_late	= sirfsoc_init_late,
 	.dt_compat      = atlas6_dt_match,
 MACHINE_END

@@ -50,6 +56,7 @@ DT_MACHINE_START(PRIMA2_DT, "Generic PRIMA2 (Flattened Device Tree)")
 	/* Maintainer: Barry Song <[email protected]> */
 	.map_io         = sirfsoc_map_io,
 	.dma_zone_size	= SZ_256M,
+	.init_machine   = sirfsoc_init_mach,
 	.init_late	= sirfsoc_init_late,
 	.dt_compat      = prima2_dt_match,
 MACHINE_END

@@ -65,6 +72,7 @@ DT_MACHINE_START(MARCO_DT, "Generic MARCO (Flattened Device Tree)")
 	/* Maintainer: Barry Song <[email protected]> */
 	.smp            = smp_ops(sirfsoc_smp_ops),
 	.map_io         = sirfsoc_map_io,
+	.init_machine   = sirfsoc_init_mach,
 	.init_late	= sirfsoc_init_late,
 	.dt_compat      = marco_dt_match,
 MACHINE_END

arch/arm/mach-prima2/l2x0.c deleted +0 ~~-49~~

49 lines not on this page · raw message

Signature · 1 line

1.7.5.4

Russell King - ARM Linux

2014-04-15 10:00 UTC #1 lore raw

PATCH 1/1 ARM: prima2: remove L2 cache size override

Quoted 9 lines · Barry Song

From: Barry Song <[email protected]>
L2 cache size has been read by l2x0_init(), it is useless to set WAY_SIZE and ASSOCIATIVITY in prima2.
Cc Russell King <[email protected]>
Signed-off-by Barry Song <[email protected]>
---
-Derived from Russell's "ARM: l2c: prima2: remove cache size override"

You're a SMP architecture... it would be much better to initialise the L2 cache before the secondary CPUs are brought online.

In any case, across the board, it's preferable that the L2 cache is enabled before the delay loop calibration as it can have an effect on the calibrated value.

Signature · 2 lines

FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Barry Song

author

2014-04-15 10:13 UTC #2 lore raw

PATCH 1/1 ARM: prima2: remove L2 cache size override

2014-04-15 18:00 GMT+08:00 Russell King - ARM Linux <[email protected]>:

Quoted 6 lines · Barry Song

Quoted 5 lines
From: Barry Song <[email protected]>
L2 cache size has been read by l2x0_init(), it is useless to set WAY_SIZE and ASSOCIATIVITY in prima2.

Quoted 7 lines

Quoted 4 lines
Cc Russell King <[email protected]>
Signed-off-by Barry Song <[email protected]>
---
-Derived from Russell's "ARM: l2c: prima2: remove cache size override"
You're a SMP architecture... it would be much better to initialise the L2 cache before the secondary CPUs are brought online.

ok, it seems we still need to hook this into the callback of this specific machine instead of using a global early_initcall(). since now the dt compatible field has been generic enough, it will cause other platforms to execute l2x0_of_init() too.

Quoted 4 lines

In any case, across the board, it's preferable that the L2 cache is enabled before the delay loop calibration as it can have an effect on the calibrated value.

what if the real case is that l2x0 has been enabled in bootloader?

Quoted 4 lines

--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.

-barry

Russell King - ARM Linux

2014-04-15 21:56 UTC #3 lore raw

PATCH 1/1 ARM: prima2: remove L2 cache size override

Quoted 12 lines · Barry Song

2014-04-15 18:00 GMT+08:00 Russell King - ARM Linux <[email protected]>:
Quoted 11 lines · Barry Song
Quoted 9 lines
From: Barry Song <[email protected]>
L2 cache size has been read by l2x0_init(), it is useless to set WAY_SIZE and ASSOCIATIVITY in prima2.
Cc Russell King <[email protected]>
Signed-off-by Barry Song <[email protected]>
---
-Derived from Russell's "ARM: l2c: prima2: remove cache size override"

Quoted 7 lines

Quoted 2 lines
You're a SMP architecture... it would be much better to initialise the L2 cache before the secondary CPUs are brought online.
ok, it seems we still need to hook this into the callback of this specific machine instead of using a global early_initcall(). since now the dt compatible field has been generic enough, it will cause other platforms to execute l2x0_of_init() too.

What I did for Versatile Express was to move it to the init_irq stage. I'm not condoning using init_irq() for non-IRQ stuff, but that was just to see whether it was possible (the .init_early is too early for it.) Let me put it another way: I'm not thrilled by the idea of having it in init_irq() but that's a better location to call L2 cache initialisation than half-way through the kernel's initialisation.

Quoted 5 lines

Quoted 3 lines
In any case, across the board, it's preferable that the L2 cache is enabled before the delay loop calibration as it can have an effect on the calibrated value.
what if the real case is that l2x0 has been enabled in bootloader?

If the L2 cache has already been enabled, l2x0_of_init() handles it - in that situation, we can't re-do the enable (because writing to the aux ctrl register is not allowed) so we merely setup the outer cache function pointers and don't try to do any configuration or enabling.

Signature · 2 lines

FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Barry Song

author

2014-04-16 08:24 UTC #4 lore raw

PATCH 1/1 ARM: prima2: remove L2 cache size override

2014-04-16 5:56 GMT+08:00 Russell King - ARM Linux <[email protected]>:

Quoted 21 lines · Barry Song

Quoted 12 lines
2014-04-15 18:00 GMT+08:00 Russell King - ARM Linux <[email protected]>:
Quoted 11 lines · Barry Song
Quoted 9 lines
From: Barry Song <[email protected]>
L2 cache size has been read by l2x0_init(), it is useless to set WAY_SIZE and ASSOCIATIVITY in prima2.
Cc Russell King <[email protected]>
Signed-off-by Barry Song <[email protected]>
---
-Derived from Russell's "ARM: l2c: prima2: remove cache size override"
Quoted 7 lines
Quoted 2 lines
You're a SMP architecture... it would be much better to initialise the L2 cache before the secondary CPUs are brought online.
ok, it seems we still need to hook this into the callback of this specific machine instead of using a global early_initcall(). since now the dt compatible field has been generic enough, it will cause other platforms to execute l2x0_of_init() too.

Quoted 6 lines

What I did for Versatile Express was to move it to the init_irq stage. I'm not condoning using init_irq() for non-IRQ stuff, but that was just to see whether it was possible (the .init_early is too early for it.) Let me put it another way: I'm not thrilled by the idea of having it in init_irq() but that's a better location to call L2 cache initialisation than half-way through the kernel's initialisation.

i would like to believe you are right since you have dug into the callbacks and finally thinks init_irq() is the suitable stage for it even though it looks misnamed.

Quoted 11 lines

Quoted 6 lines
Quoted 4 lines
In any case, across the board, it's preferable that the L2 cache is enabled before the delay loop calibration as it can have an effect on the calibrated value.
what if the real case is that l2x0 has been enabled in bootloader?
If the L2 cache has already been enabled, l2x0_of_init() handles it - in that situation, we can't re-do the enable (because writing to the aux ctrl register is not allowed) so we merely setup the outer cache function pointers and don't try to do any configuration or enabling.

yes. i meant if that has been done in bootloader, it turns out we don't care about which callback is better any more. and for linux running in non-security, same.

Quoted 4 lines

--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.

- barry

Russell King - ARM Linux

2014-04-16 08:29 UTC #5 lore raw

PATCH 1/1 ARM: prima2: remove L2 cache size override

Quoted 3 lines · Barry Song

yes. i meant if that has been done in bootloader, it turns out we don't care about which callback is better any more. and for linux running in non-security, same.

However, I don't think it's quite as simple as that, because things like the Cortex A9 Full-Line-Of-Zeros feature need a specific enable sequence which (I think) can't be done in the boot loader.

Signature · 2 lines

FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Arnd Bergmann

2014-04-16 08:52 UTC #6 lore raw

PATCH 1/1 ARM: prima2: remove L2 cache size override

In reply to Russell King - ARM Linux

Quoted 7 lines · On Tuesday 15 April 2014 22:56:15 Russell King - ARM Linux wrote:

Quoted 5 lines · Barry Song
ok, it seems we still need to hook this into the callback of this specific machine instead of using a global early_initcall(). since now the dt compatible field has been generic enough, it will cause other platforms to execute l2x0_of_init() too.

Quoted 6 lines

What I did for Versatile Express was to move it to the init_irq stage. I'm not condoning using init_irq() for non-IRQ stuff, but that was just to see whether it was possible (the .init_early is too early for it.) Let me put it another way: I'm not thrilled by the idea of having it in init_irq() but that's a better location to call L2 cache initialisation than half-way through the kernel's initialisation.

Would it help to introduce a new .init_l2_cache() callback? That way, the architecture code can default to initializing any l2x0 that it finds in DT, and have give platforms the option of overriding the call if they need to, and we can do it at the same point during early boot for everyone.

A number of platforms currently have the same

static void __init this_platform_init(void)

{
      l2x0_of_init(0, ~0UL);
      of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
}

Where the second line is already the default for .init_machine, and it would be nice to get rid of the callback entirely, especially for the machines that don't do anything else in their platform code.

Arnd

Barry Song

author

2014-04-16 09:13 UTC #7 lore raw

PATCH 1/1 ARM: prima2: remove L2 cache size override

2014-04-16 16:52 GMT+08:00 Arnd Bergmann <[email protected]>:

Quoted 29 lines · On Tuesday 15 April 2014 22:56:15 Russell King - ARM Linux wrote:

Quoted 7 lines · Barry Song
Quoted 5 lines
ok, it seems we still need to hook this into the callback of this specific machine instead of using a global early_initcall(). since now the dt compatible field has been generic enough, it will cause other platforms to execute l2x0_of_init() too.
Quoted 6 lines
What I did for Versatile Express was to move it to the init_irq stage. I'm not condoning using init_irq() for non-IRQ stuff, but that was just to see whether it was possible (the .init_early is too early for it.) Let me put it another way: I'm not thrilled by the idea of having it in init_irq() but that's a better location to call L2 cache initialisation than half-way through the kernel's initialisation.
Would it help to introduce a new .init_l2_cache() callback? That way, the architecture code can default to initializing any l2x0 that it finds in DT, and have give platforms the option of overriding the call if they need to, and we can do it at the same point during early boot for everyone.
A number of platforms currently have the same
static void __init this_platform_init(void)
{
      l2x0_of_init(0, ~0UL);
      of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
}

Quoted 4 lines

Where the second line is already the default for .init_machine, and it would be nice to get rid of the callback entirely, especially for the machines that don't do anything else in their platform code.

it seems you mean put a general customize_l2x0_init() like customize_machine().

what is the reason stopping l2x0 from becoming a standalone driver which can initialize itself?

Quoted 2 lines

Arnd

-barry

Russell King - ARM Linux

2014-04-16 09:23 UTC #8 lore raw

PATCH 1/1 ARM: prima2: remove L2 cache size override

Quoted 35 lines · Barry Song

2014-04-16 16:52 GMT+08:00 Arnd Bergmann <[email protected]>:
Quoted 29 lines · On Tuesday 15 April 2014 22:56:15 Russell King - ARM Linux wrote:
Quoted 7 lines · Barry Song
Quoted 5 lines
ok, it seems we still need to hook this into the callback of this specific machine instead of using a global early_initcall(). since now the dt compatible field has been generic enough, it will cause other platforms to execute l2x0_of_init() too.
Quoted 6 lines
What I did for Versatile Express was to move it to the init_irq stage. I'm not condoning using init_irq() for non-IRQ stuff, but that was just to see whether it was possible (the .init_early is too early for it.) Let me put it another way: I'm not thrilled by the idea of having it in init_irq() but that's a better location to call L2 cache initialisation than half-way through the kernel's initialisation.
Would it help to introduce a new .init_l2_cache() callback? That way, the architecture code can default to initializing any l2x0 that it finds in DT, and have give platforms the option of overriding the call if they need to, and we can do it at the same point during early boot for everyone.
A number of platforms currently have the same
static void __init this_platform_init(void)
{
      l2x0_of_init(0, ~0UL);
      of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
}
Quoted 4 lines
Where the second line is already the default for .init_machine, and it would be nice to get rid of the callback entirely, especially for the machines that don't do anything else in their platform code.

Quoted 4 lines

it seems you mean put a general customize_l2x0_init() like customize_machine().
what is the reason stopping l2x0 from becoming a standalone driver which can initialize itself?

Well... if you didn't need to give this:

	l2x0_of_init(0, 0xFDFFFFFFUL);

but instead could cope with:

	l2x0_of_init(0, ~0);

then that would be one less reason for it /not/ to be a standalone driver. It's every "tweak" that the platform does that stands in the way.

Signature · 2 lines

FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Russell King - ARM Linux

2014-04-16 09:21 UTC #9 lore raw

PATCH 1/1 ARM: prima2: remove L2 cache size override

In reply to Arnd Bergmann

Quoted 15 lines · Arnd Bergmann

Quoted 7 lines · On Tuesday 15 April 2014 22:56:15 Russell King - ARM Linux wrote:
Quoted 5 lines · Barry Song
ok, it seems we still need to hook this into the callback of this specific machine instead of using a global early_initcall(). since now the dt compatible field has been generic enough, it will cause other platforms to execute l2x0_of_init() too.
Quoted 6 lines
What I did for Versatile Express was to move it to the init_irq stage. I'm not condoning using init_irq() for non-IRQ stuff, but that was just to see whether it was possible (the .init_early is too early for it.) Let me put it another way: I'm not thrilled by the idea of having it in init_irq() but that's a better location to call L2 cache initialisation than half-way through the kernel's initialisation.

Quoted 5 lines

Would it help to introduce a new .init_l2_cache() callback? That way, the architecture code can default to initializing any l2x0 that it finds in DT, and have give platforms the option of overriding the call if they need to, and we can do it at the same point during early boot for everyone.

As my ideal target is to remove l2x0_*init from platforms, I'd rather not do that unless we have a clear idea that we are going to keep this around.

Another potential solution would be to have the two auxctl values specified, and do nothing if they're both zero.

First though, we really need to work out what each platform requires from these, and turn everything else into either something the core code handles, or if there is no good reason for it, it is simply removed.

Signature · 2 lines

FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Russell King - ARM Linux

2014-04-16 09:24 UTC #10 lore raw

PATCH 1/1 ARM: prima2: remove L2 cache size override

In reply to Barry Song

arch/arm/mach-prima2/common.c static __init void sirfsoc_map_io(void) · line 30

Quoted 3 lines · Barry Song

arch/arm/mach-prima2/common.c +3 -0

+static __init void sirfsoc_init_mach(void)
+{
+	l2x0_of_init(0, 0xFDFFFFFFUL);

BTW, this should be just the plain: 0xfdffffff - no need for UL on the end. Also lower case is preferred for hex.

Signature · 2 lines

FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Russell King - ARM Linux

2014-04-23 18:21 UTC #11 lore raw

PATCH 1/1 ARM: prima2: remove L2 cache size override

Quoted 7 lines · Russell King - ARM Linux

arch/arm/mach-prima2/common.c static __init void sirfsoc_map_io(void) · line 30
Quoted 3 lines · Barry Song
arch/arm/mach-prima2/common.c +3 -0
+static __init void sirfsoc_init_mach(void)
+{
+	l2x0_of_init(0, 0xFDFFFFFFUL);
BTW, this should be just the plain: 0xfdffffff - no need for UL on the end. Also lower case is preferred for hex.

So... I provide feedback, and nothing happens.

FYI, tonight, I'm pushing out my L2C branch into linux-next with the original patch for prima2 incorporated, as my concerns against your patch have not been addressed.

Signature · 2 lines

FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Barry Song

author

2014-04-23 23:11 UTC #12 lore raw

PATCH 1/1 ARM: prima2: remove L2 cache size override

2014-04-24 2:21 GMT+08:00 Russell King - ARM Linux <[email protected]>:

Quoted 9 lines · Russell King - ARM Linux

Quoted 7 lines · Barry Song
arch/arm/mach-prima2/common.c static __init void sirfsoc_map_io(void) · line 30
Quoted 3 lines
arch/arm/mach-prima2/common.c +3 -0
+static __init void sirfsoc_init_mach(void)
+{
+   l2x0_of_init(0, 0xFDFFFFFFUL);
BTW, this should be just the plain: 0xfdffffff - no need for UL on the end. Also lower case is preferred for hex.

Quoted 5 lines

So... I provide feedback, and nothing happens.
FYI, tonight, I'm pushing out my L2C branch into linux-next with the original patch for prima2 incorporated, as my concerns against your patch have not been addressed.

Russell, i do agree with your feedback.
but, in your original patch, moving to early_initcall will make other non-csr platforms call l2x0_of_init() and finish initialization by (0, ~0) directly since other platforms also put same general compatible prop in dts. in your patch, you have dropped "sirf,prima2-pl310-cache" and "sirf,marco-pl310-cache", so it happens by matching general "arm,pl310-cache".
i am still on the way to send v2. if it is not too urgent to you, will you wait for me to send v2 by fixing issues in your feedback?

Quoted 1 line

-barry

Russell King - ARM Linux

2014-04-23 23:43 UTC #13 lore raw

PATCH 1/1 ARM: prima2: remove L2 cache size override

Quoted 16 lines · Barry Song

2014-04-24 2:21 GMT+08:00 Russell King - ARM Linux <[email protected]>:
Quoted 9 lines · Russell King - ARM Linux
Quoted 7 lines · Barry Song
arch/arm/mach-prima2/common.c static __init void sirfsoc_map_io(void) · line 30
Quoted 3 lines
arch/arm/mach-prima2/common.c +3 -0
+static __init void sirfsoc_init_mach(void)
+{
+   l2x0_of_init(0, 0xFDFFFFFFUL);
BTW, this should be just the plain: 0xfdffffff - no need for UL on the end. Also lower case is preferred for hex.
Quoted 5 lines
So... I provide feedback, and nothing happens.
FYI, tonight, I'm pushing out my L2C branch into linux-next with the original patch for prima2 incorporated, as my concerns against your patch have not been addressed.

Quoted 9 lines

Russell, i do agree with your feedback.
but, in your original patch, moving to early_initcall will make other non-csr platforms call l2x0_of_init() and finish initialization by (0, ~0) directly since other platforms also put same general compatible prop in dts. in your patch, you have dropped "sirf,prima2-pl310-cache" and "sirf,marco-pl310-cache", so it happens by matching general "arm,pl310-cache".
i am still on the way to send v2. if it is not too urgent to you, will you wait for me to send v2 by fixing issues in your feedback?

I have no problem adding a patch, or replacing my patch in the series. All I ask is that it isn't forgotten, and that it's sent before -rc6.

Signature · 2 lines

FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.