[PATCH] ARM l2x0: check whether l2x0 already enabled

[PATCH] ARM l2x0: check whether l2x0 already enabled

[srinidhi kasagar]

From: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
Date: Tue, 24 Nov 2009 13:59:20 +0530

If running in non-secure mode, accessing
control and auxiliary registers of l2x0
will fault.

Signed-off-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
---
 arch/arm/mm/cache-l2x0.c |   24 +++++++++++++++---------
 1 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index b480f1d..7c0d056 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -99,18 +99,24 @@ void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
 
 	l2x0_base = base;
 
-	/* disable L2X0 */
-	writel(0, l2x0_base + L2X0_CTRL);
+	/* check if l2x0 controller is already enabled.
+	 * if you are booting from non-secure mode
+	 * accessing the below registers will fault.
+	 */
+	if (!(readl(l2x0_base + L2X0_CTRL) & 1)) {
 
-	aux = readl(l2x0_base + L2X0_AUX_CTRL);
-	aux &= aux_mask;
-	aux |= aux_val;
-	writel(aux, l2x0_base + L2X0_AUX_CTRL);
+		/* L2x0 controller is disabled */
 
-	l2x0_inv_all();
+		aux = readl(l2x0_base + L2X0_AUX_CTRL);
+		aux &= aux_mask;
+		aux |= aux_val;
+		writel(aux, l2x0_base + L2X0_AUX_CTRL);
 
-	/* enable L2X0 */
-	writel(1, l2x0_base + L2X0_CTRL);
+		l2x0_inv_all();
+
+		/* enable L2X0 */
+		writel(1, l2x0_base + L2X0_CTRL);
+	}
 
 	outer_cache.inv_range = l2x0_inv_range;
 	outer_cache.clean_range = l2x0_clean_range;
-- 
1.6.3.GIT

[PATCH] ARM l2x0: check whether l2x0 already enabled

[srinidhi kasagar]

On Tue, 2009-11-24 at 13:11 +0100, Surinder P Singh wrote:
> On Tue, Nov 24, 2009 at 3:47 AM, srinidhi kasagar
> <srinidhi.kasagar@stericsson.com> wrote:
> > From: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
> > Date: Tue, 24 Nov 2009 13:59:20 +0530
> >
> > If running in non-secure mode, accessing
> > control and auxiliary registers of l2x0
> > will fault.
> >
> > Signed-off-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
> > ---
> >  arch/arm/mm/cache-l2x0.c |   24 +++++++++++++++---------
> >  1 files changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> > index b480f1d..7c0d056 100644
> > --- a/arch/arm/mm/cache-l2x0.c
> > +++ b/arch/arm/mm/cache-l2x0.c
> > @@ -99,18 +99,24 @@ void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
> >
> >        l2x0_base = base;
> >
> > -       /* disable L2X0 */
> > -       writel(0, l2x0_base + L2X0_CTRL);
> > +       /* check if l2x0 controller is already enabled.
> > +        * if you are booting from non-secure mode
> > +        * accessing the below registers will fault.
> > +        */
> > +       if (!(readl(l2x0_base + L2X0_CTRL) & 1)) {
> 
> 2 points:
> 
> 1. Since this code is also valid for devices based on pre-ARMv6, maybe
> making this code conditional for >=ARMv6 would be cleaner ?
rather it depends on l2 controller being used. L210 controllers
do not have such restrictions whereas l220/pl310 have such kind
of secure/non-secure restrictions. So would it be better to keep
condition based on l2 controller being used?

Srinidhi

[PATCH] ARM l2x0: check whether l2x0 already enabled

[Surinder P Singh]

On Tue, Nov 24, 2009 at 3:47 AM, srinidhi kasagar
<srinidhi.kasagar@stericsson.com> wrote:
> From: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
> Date: Tue, 24 Nov 2009 13:59:20 +0530
>
> If running in non-secure mode, accessing
> control and auxiliary registers of l2x0
> will fault.
>
> Signed-off-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
> ---
> ?arch/arm/mm/cache-l2x0.c | ? 24 +++++++++++++++---------
> ?1 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
> index b480f1d..7c0d056 100644
> --- a/arch/arm/mm/cache-l2x0.c
> +++ b/arch/arm/mm/cache-l2x0.c
> @@ -99,18 +99,24 @@ void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
>
> ? ? ? ?l2x0_base = base;
>
> - ? ? ? /* disable L2X0 */
> - ? ? ? writel(0, l2x0_base + L2X0_CTRL);
> + ? ? ? /* check if l2x0 controller is already enabled.
> + ? ? ? ?* if you are booting from non-secure mode
> + ? ? ? ?* accessing the below registers will fault.
> + ? ? ? ?*/
> + ? ? ? if (!(readl(l2x0_base + L2X0_CTRL) & 1)) {

2 points:

1. Since this code is also valid for devices based on pre-ARMv6, maybe
making this code conditional for >=ARMv6 would be cleaner ?

2. Even for trustzone capable devices, would'nt it be better to read
the Secure Configuration Register (SCR) to determine the CPU's current
state (secure/non-secure) and only write to the L2 cache cntrl
registers if you're in non-secure state ?

Cheers,
sp

[PATCH] ARM l2x0: check whether l2x0 already enabled

[Surinder P Singh]

On Tue, Nov 24, 2009 at 6:05 AM, srinidhi kasagar
<srinidhi.kasagar@stericsson.com> wrote:

>> 2 points:
>>
>> 1. Since this code is also valid for devices based on pre-ARMv6, maybe
>> making this code conditional for >=ARMv6 would be cleaner ?
> rather it depends on l2 controller being used. L210 controllers
> do not have such restrictions whereas l220/pl310 have such kind
> of secure/non-secure restrictions. So would it be better to keep
> condition based on l2 controller being used?
>

Thats probably better. You can read the L2 cache ID register to figure
out if its a L210/220 or PL310 and so on. You can couple this check
with the cpu secure/non-secure state before deciding to write to the
registers.

Cheers,
sp

[PATCH] ARM l2x0: check whether l2x0 already enabled

[Catalin Marinas]

On Tue, 2009-11-24 at 13:35 +0000, Surinder P Singh wrote:
> On Tue, Nov 24, 2009 at 6:05 AM, srinidhi kasagar
> <srinidhi.kasagar@stericsson.com> wrote:
> 
> >> 2 points:
> >>
> >> 1. Since this code is also valid for devices based on pre-ARMv6, maybe
> >> making this code conditional for >=ARMv6 would be cleaner ?
> > rather it depends on l2 controller being used. L210 controllers
> > do not have such restrictions whereas l220/pl310 have such kind
> > of secure/non-secure restrictions. So would it be better to keep
> > condition based on l2 controller being used?
> >
> 
> Thats probably better. You can read the L2 cache ID register to figure
> out if its a L210/220 or PL310 and so on. You can couple this check
> with the cpu secure/non-secure state before deciding to write to the
> registers.

I don't think it's worth the hassle. Just always check whether it is
already enabled without additional ifdefs. IIRC, L210 is used on
RealView PB1176 (not entirely sure).

I had a similar patch in my tree but haven't pushed it because it
required an additional fix for SMP platforms. Basically the secondary
CPUs use uncached accesses and the primary one would need to flush the
outer cache as well during the booting protocol. The platform init
function (e.g. realview_eb_init) was called too late in the process, so
I had to move the L2 initialisation to an early_init call. See below for
the RealView patch, something similar may be needed for other SMP
platforms:


RealView: Initialise the L2 cache via early_initcall

From: Catalin Marinas <catalin.marinas@arm.com>

This is required on SMP systems where the L2 cache is already enabled
when the kernel is started (e.g. in non-secure mode) and
boot_secondary() needs to call outer_clean_range().

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm/mach-realview/platsmp.c         |    3 +++
 arch/arm/mach-realview/realview_eb.c     |   24 ++++++++++++++++--------
 arch/arm/mach-realview/realview_pb11mp.c |   22 ++++++++++++++++------
 arch/arm/mach-realview/realview_pbx.c    |   15 ++++++++++-----
 4 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-realview/platsmp.c b/arch/arm/mach-realview/platsmp.c
index bf42167..ff59e75 100644
--- a/arch/arm/mach-realview/platsmp.c
+++ b/arch/arm/mach-realview/platsmp.c
@@ -127,8 +127,11 @@ int __cpuinit boot_secondary(unsigned int cpu, struct task_struct *idle)
 	 * Note that "pen_release" is the hardware CPU ID, whereas
 	 * "cpu" is Linux's internal ID.
 	 */
+	flush_cache_all();
+	outer_clean_range(__pa(&secondary_data), __pa(&secondary_data + 1));
 	pen_release = cpu;
 	flush_cache_all();
+	outer_clean_range(__pa(&pen_release), __pa(&pen_release + 1));
 
 	/*
 	 * XXX
diff --git a/arch/arm/mach-realview/realview_eb.c b/arch/arm/mach-realview/realview_eb.c
index c20fbef..fee0adc 100644
--- a/arch/arm/mach-realview/realview_eb.c
+++ b/arch/arm/mach-realview/realview_eb.c
@@ -364,20 +364,28 @@ static struct sys_timer realview_eb_timer = {
 	.init		= realview_eb_timer_init,
 };
 
+#ifdef CONFIG_CACHE_L2X0
+static int __init realview_eb_l2x0_init(void)
+{
+	if (machine_is_realview_eb_mp())
+		/*
+		 * 1MB (128KB/way), 8-way associativity, evmon/parity/share
+		 * Bits:  .... ...0 0111 1001 0000 .... .... ....
+		 */
+		l2x0_init(__io_address(REALVIEW_EB11MP_L220_BASE),
+			  0x00790000, 0xfe000fff);
+	return 0;
+}
+early_initcall(realview_eb_l2x0_init);
+#endif
+
 static void __init realview_eb_init(void)
 {
 	int i;
 
-	if (core_tile_eb11mp() || core_tile_a9mp()) {
+	if (core_tile_eb11mp() || core_tile_a9mp())
 		realview_eb11mp_fixup();
 
-#ifdef CONFIG_CACHE_L2X0
-		/* 1MB (128KB/way), 8-way associativity, evmon/parity/share enabled
-		 * Bits:  .... ...0 0111 1001 0000 .... .... .... */
-		l2x0_init(__io_address(REALVIEW_EB11MP_L220_BASE), 0x00790000, 0xfe000fff);
-#endif
-	}
-
 	realview_flash_register(&realview_eb_flash_resource, 1);
 	platform_device_register(&realview_i2c_device);
 	eth_device_register();
diff --git a/arch/arm/mach-realview/realview_pb11mp.c b/arch/arm/mach-realview/realview_pb11mp.c
index ea1e60e..75b3ecf 100644
--- a/arch/arm/mach-realview/realview_pb11mp.c
+++ b/arch/arm/mach-realview/realview_pb11mp.c
@@ -282,16 +282,26 @@ static struct sys_timer realview_pb11mp_timer = {
 	.init		= realview_pb11mp_timer_init,
 };
 
+#ifdef CONFIG_CACHE_L2X0
+static int __init realview_pb11mp_l2x0_init(void)
+{
+	if (machine_is_realview_pb11mp()) {
+		/*
+		 * 1MB (128KB/way), 8-way associativity, evmon/parity/share
+		 * Bits:  .... ...0 0111 1001 0000 .... .... ....
+		 */
+		l2x0_init(__io_address(REALVIEW_TC11MP_L220_BASE),
+			  0x00790000, 0xfe000fff);
+	}
+	return 0;
+}
+early_initcall(realview_pb11mp_l2x0_init);
+#endif
+
 static void __init realview_pb11mp_init(void)
 {
 	int i;
 
-#ifdef CONFIG_CACHE_L2X0
-	/* 1MB (128KB/way), 8-way associativity, evmon/parity/share enabled
-	 * Bits:  .... ...0 0111 1001 0000 .... .... .... */
-	l2x0_init(__io_address(REALVIEW_TC11MP_L220_BASE), 0x00790000, 0xfe000fff);
-#endif
-
 	realview_flash_register(realview_pb11mp_flash_resource,
 				ARRAY_SIZE(realview_pb11mp_flash_resource));
 	realview_eth_register(NULL, realview_pb11mp_smsc911x_resources);
diff --git a/arch/arm/mach-realview/realview_pbx.c b/arch/arm/mach-realview/realview_pbx.c
index e9d461f..0241ee1 100644
--- a/arch/arm/mach-realview/realview_pbx.c
+++ b/arch/arm/mach-realview/realview_pbx.c
@@ -289,12 +289,10 @@ static struct sys_timer realview_pbx_timer = {
 	.init		= realview_pbx_timer_init,
 };
 
-static void __init realview_pbx_init(void)
-{
-	int i;
-
 #ifdef CONFIG_CACHE_L2X0
-	if (core_tile_pbxa9mp()) {
+static int __init realview_pbx_l2x0_init(void)
+{
+	if (machine_is_realview_pbx() && core_tile_pbxa9mp()) {
 		void __iomem *l2x0_base =
 			__io_address(REALVIEW_PBX_TILE_L220_BASE);
 
@@ -306,8 +304,15 @@ static void __init realview_pbx_init(void)
 		 * Bits:  .. 0 0 0 0 1 00 1 0 1 001 0 000 0 .... .... .... */
 		l2x0_init(l2x0_base, 0x02520000, 0xc0000fff);
 	}
+	return 0;
+}
+early_initcall(realview_pbx_l2x0_init);
 #endif
 
+static void __init realview_pbx_init(void)
+{
+	int i;
+
 	realview_flash_register(realview_pbx_flash_resources,
 				ARRAY_SIZE(realview_pbx_flash_resources));
 	realview_eth_register(NULL, realview_pbx_smsc911x_resources);


-- 
Catalin

[PATCH] ARM l2x0: check whether l2x0 already enabled

[srinidhi kasagar]

On Tue, 2009-11-24 at 14:55 +0100, Catalin Marinas wrote:
> On Tue, 2009-11-24 at 13:35 +0000, Surinder P Singh wrote:
> > On Tue, Nov 24, 2009 at 6:05 AM, srinidhi kasagar
> > <srinidhi.kasagar@stericsson.com> wrote:
> > 
> > >> 2 points:
> > >>
> > >> 1. Since this code is also valid for devices based on pre-ARMv6, maybe
> > >> making this code conditional for >=ARMv6 would be cleaner ?
> > > rather it depends on l2 controller being used. L210 controllers
> > > do not have such restrictions whereas l220/pl310 have such kind
> > > of secure/non-secure restrictions. So would it be better to keep
> > > condition based on l2 controller being used?
> > >
> > 
> > Thats probably better. You can read the L2 cache ID register to figure
> > out if its a L210/220 or PL310 and so on. You can couple this check
> > with the cpu secure/non-secure state before deciding to write to the
> > registers.
> 
> I don't think it's worth the hassle. Just always check whether it is
> already enabled without additional ifdefs. IIRC, L210 is used on
> RealView PB1176 (not entirely sure).

So, does the patch which I have sent still valid which just checks
whether l2x0 is already enabled?

Srinidhi

[PATCH] ARM l2x0: check whether l2x0 already enabled

[Surinder P Singh]

> So, does the patch which I have sent still valid which just checks
> whether l2x0 is already enabled?
>

OK by me.

Cheers,
sp

[PATCH] ARM l2x0: check whether l2x0 already enabled

[Catalin Marinas]

On Wed, 2009-11-25 at 19:10 +0000, srinidhi kasagar wrote:
> On Tue, 2009-11-24 at 14:55 +0100, Catalin Marinas wrote:
> > On Tue, 2009-11-24 at 13:35 +0000, Surinder P Singh wrote:
> > > On Tue, Nov 24, 2009 at 6:05 AM, srinidhi kasagar
> > > <srinidhi.kasagar@stericsson.com> wrote:
> > >
> > > >> 2 points:
> > > >>
> > > >> 1. Since this code is also valid for devices based on pre-ARMv6, maybe
> > > >> making this code conditional for >=ARMv6 would be cleaner ?
> > > > rather it depends on l2 controller being used. L210 controllers
> > > > do not have such restrictions whereas l220/pl310 have such kind
> > > > of secure/non-secure restrictions. So would it be better to keep
> > > > condition based on l2 controller being used?
> > > >
> > >
> > > Thats probably better. You can read the L2 cache ID register to figure
> > > out if its a L210/220 or PL310 and so on. You can couple this check
> > > with the cpu secure/non-secure state before deciding to write to the
> > > registers.
> >
> > I don't think it's worth the hassle. Just always check whether it is
> > already enabled without additional ifdefs. IIRC, L210 is used on
> > RealView PB1176 (not entirely sure).
> 
> So, does the patch which I have sent still valid which just checks
> whether l2x0 is already enabled?

OK with me as well, just minor issues with the coding style (the
multi-line comment).

-- 
Catalin